Patchwork [v1,8/8] usb/ehci: Put RAM in undefined MMIO regions

login
register
mail settings
Submitter Peter Crosthwaite
Date Oct. 25, 2012, 9:47 a.m.
Message ID <b93383e7883d0a3382ba39136072b7f22ae86143.1351157627.git.peter.crosthwaite@xilinx.com>
Download mbox | patch
Permalink /patch/194094/
State New
Headers show

Comments

Peter Crosthwaite - Oct. 25, 2012, 9:47 a.m.
Just put RAM regions in the unimplemented spaces in the MMIO region. These
regions have undefined behaviour, but this at least stops QEMU from segfaulting
when the guest bangs on these registers (and sucessfully fakes reading and
writing the registers with no side effects).

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/usb/hcd-ehci.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)
Gerd Hoffmann - Oct. 25, 2012, 12:19 p.m.
On 10/25/12 11:47, Peter Crosthwaite wrote:
> Just put RAM regions in the unimplemented spaces in the MMIO region. These
> regions have undefined behaviour, but this at least stops QEMU from segfaulting
> when the guest bangs on these registers (and sucessfully fakes reading and
> writing the registers with no side effects).

Should not be needed, memory api should deal with that properly.
Something is fishy somewhere.  Maybe the dmacontext thing Peter Maydell
noted for patch 5.

cheers,
  Gerd
Peter Crosthwaite - Oct. 25, 2012, 1:03 p.m.
On Thu, Oct 25, 2012 at 10:19 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> On 10/25/12 11:47, Peter Crosthwaite wrote:
>> Just put RAM regions in the unimplemented spaces in the MMIO region. These
>> regions have undefined behaviour, but this at least stops QEMU from segfaulting
>> when the guest bangs on these registers (and sucessfully fakes reading and
>> writing the registers with no side effects).
>
> Should not be needed, memory api should deal with that properly.

CC Avi,

Whats going on here is there is a container of size 0x1000 created
with memory_region_init() and a handful of small subregions are
populated. the container is then mapped to a 0x1000 size region of the
system memory. What is supposed to happen when the guest access a
region in the container for which no subregion has been added? For me
it was a segfault, so i needed this patch for guest to proceed past
accesses these undefined regions.

> Something is fishy somewhere.  Maybe the dmacontext thing Peter Maydell
> noted for patch 5.
>

I think thats a separate issue. This is about the guest accessing the
EHCI MMIO region not DMA.

My implementation of P5 is functionality equivalent to Peters
proposal. Just Peters idea will save me two lines of code and a memory
leak :)

Regards,
Peter

> cheers,
>   Gerd
>
>
Peter Maydell - Oct. 25, 2012, 1:12 p.m.
On 25 October 2012 14:03, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Thu, Oct 25, 2012 at 10:19 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>> On 10/25/12 11:47, Peter Crosthwaite wrote:
>>> Just put RAM regions in the unimplemented spaces in the MMIO region. These
>>> regions have undefined behaviour, but this at least stops QEMU from segfaulting
>>> when the guest bangs on these registers (and sucessfully fakes reading and
>>> writing the registers with no side effects).
>>
>> Should not be needed, memory api should deal with that properly.
>
> CC Avi,
>
> Whats going on here is there is a container of size 0x1000 created
> with memory_region_init() and a handful of small subregions are
> populated. the container is then mapped to a 0x1000 size region of the
> system memory. What is supposed to happen when the guest access a
> region in the container for which no subregion has been added? For me
> it was a segfault, so i needed this patch for guest to proceed past
> accesses these undefined regions.

 (1) what does the hardware do? If the hardware device has (the
 equivalent of) an N bit address bus and treats accesses to unused
 addresses within the memory range defined by that bus width as RAZ/WI
 then that's what we should be modelling.
 If the hardware has real registers in this range we should also be
 modelling them (possibly as RAZ/WI with a LOG_UNIMP diagnostic).

 (2) what should the memory system do for accesses where there is
 no memory region? This is really system specific as it depends
 what the bus fabric does. For ARM the usual thing would be to
 generate a decode error response which will result in the guest
 CPU taking a data abort or prefetch abort. I don't think our
 memory system currently has any way of saying "for this access
 generate an exception"...

 (3) I don't think "qemu segfaults" is a good response to bad
 guest behaviour so that needs fixing somehow even if we can't
 model what the h/w does :-)

-- PMM
Avi Kivity - Oct. 25, 2012, 1:20 p.m.
On 10/25/2012 03:03 PM, Peter Crosthwaite wrote:
> On Thu, Oct 25, 2012 at 10:19 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>> On 10/25/12 11:47, Peter Crosthwaite wrote:
>>> Just put RAM regions in the unimplemented spaces in the MMIO region. These
>>> regions have undefined behaviour, but this at least stops QEMU from segfaulting
>>> when the guest bangs on these registers (and sucessfully fakes reading and
>>> writing the registers with no side effects).
>>
>> Should not be needed, memory api should deal with that properly.
> 
> CC Avi,
> 
> Whats going on here is there is a container of size 0x1000 created
> with memory_region_init() and a handful of small subregions are
> populated. the container is then mapped to a 0x1000 size region of the
> system memory. What is supposed to happen when the guest access a
> region in the container for which no subregion has been added? 

It falls back to the parent container.  If there isn't one, something
system-specific happens.  You can override that by initializing your
container with memory_region_init_io(); the callbacks will then receive
any accesses which are not caught by any subregion.
Avi Kivity - Oct. 25, 2012, 1:21 p.m.
On 10/25/2012 03:12 PM, Peter Maydell wrote:
>  (2) what should the memory system do for accesses where there is
>  no memory region? This is really system specific as it depends
>  what the bus fabric does. For ARM the usual thing would be to
>  generate a decode error response which will result in the guest
>  CPU taking a data abort or prefetch abort. I don't think our
>  memory system currently has any way of saying "for this access
>  generate an exception"...
> 

You could easily have the top-level container have ->ops that generate
an exception.


>  (3) I don't think "qemu segfaults" is a good response to bad
>  guest behaviour so that needs fixing somehow even if we can't
>  model what the h/w does :-)

Right, a stack trace (or a patch) would be appreciated.
Peter Maydell - Oct. 25, 2012, 1:28 p.m.
On 25 October 2012 14:21, Avi Kivity <avi@redhat.com> wrote:
> On 10/25/2012 03:12 PM, Peter Maydell wrote:
>>  (2) what should the memory system do for accesses where there is
>>  no memory region? This is really system specific as it depends
>>  what the bus fabric does. For ARM the usual thing would be to
>>  generate a decode error response which will result in the guest
>>  CPU taking a data abort or prefetch abort. I don't think our
>>  memory system currently has any way of saying "for this access
>>  generate an exception"...
>>
>
> You could easily have the top-level container have ->ops that generate
> an exception.

Ah, yes, there's an 'accepts' callback. (That's kind of awkward
as an API because it means your decode logic gets spread between
read, write and accept: there are some devices where it would be
nice to have the 'default:' case of the address switch say "unknown
offset, raise decode error". If the read callback took a uint64_t*
rather than returning the read data, we could make both read and
write return a success/decode-error type of status result.)

-- PMM
Avi Kivity - Oct. 25, 2012, 1:41 p.m.
On 10/25/2012 03:28 PM, Peter Maydell wrote:
> On 25 October 2012 14:21, Avi Kivity <avi@redhat.com> wrote:
>> On 10/25/2012 03:12 PM, Peter Maydell wrote:
>>>  (2) what should the memory system do for accesses where there is
>>>  no memory region? This is really system specific as it depends
>>>  what the bus fabric does. For ARM the usual thing would be to
>>>  generate a decode error response which will result in the guest
>>>  CPU taking a data abort or prefetch abort. I don't think our
>>>  memory system currently has any way of saying "for this access
>>>  generate an exception"...
>>>
>>
>> You could easily have the top-level container have ->ops that generate
>> an exception.
> 
> Ah, yes, there's an 'accepts' callback. (That's kind of awkward
> as an API because it means your decode logic gets spread between
> read, write and accept: there are some devices where it would be
> nice to have the 'default:' case of the address switch say "unknown
> offset, raise decode error". If the read callback took a uint64_t*
> rather than returning the read data, we could make both read and
> write return a success/decode-error type of status result.)

I actually forgot about ->accepts().  But it isn't needed for this use
case, just have the lowest priority region (the container) implement
->read/write that generate the exception.  If some other region decodes,
the container region will be ignored, if nothing decodes, you get your
exception.

wrt decode duplication, I've been thinking of a single ->service()
callback that accepts a Transaction argument, including all the details
(offset, data, and direction).  We may also do something like
MemoryRegionPortio, but not hacky, that lets you have a MemoryRegion for
each register.  That means that instead of writing two large functions
that duplicates the decode, you write one function per register, and
switch on transfer direction.
Peter Maydell - Oct. 25, 2012, 1:50 p.m.
On 25 October 2012 14:41, Avi Kivity <avi@redhat.com> wrote:
> On 10/25/2012 03:28 PM, Peter Maydell wrote:
>> On 25 October 2012 14:21, Avi Kivity <avi@redhat.com> wrote:
>>> You could easily have the top-level container have ->ops that generate
>>> an exception.
>>
>> Ah, yes, there's an 'accepts' callback. (That's kind of awkward
>> as an API because it means your decode logic gets spread between
>> read, write and accept: there are some devices where it would be
>> nice to have the 'default:' case of the address switch say "unknown
>> offset, raise decode error". If the read callback took a uint64_t*
>> rather than returning the read data, we could make both read and
>> write return a success/decode-error type of status result.)
>
> I actually forgot about ->accepts().  But it isn't needed for this use
> case, just have the lowest priority region (the container) implement
> ->read/write that generate the exception

I don't understand this -- read/write don't have any way of saying
"please generate an exception". The only thing I can see in the
API that does that is returning false from accepts().

> wrt decode duplication, I've been thinking of a single ->service()
> callback that accepts a Transaction argument, including all the details
> (offset, data, and direction).

If we do this we should make sure that the Transaction allows us to
include CPU-architecture dependent info -- for ARM we would want to
model transaction attributes like 'secure/nonsecure', 'privileged/nonpriv',
'instruction/data', etc. You also want to include in the transaction
attributes who the master end of this transaction is (so a slave
can distinguish accesses from a particular CPU core in a cluster,
for instance). This would allow us to remove some of the current
nasty hacks where devices reach into the CPUArchState to  retrieve
info that should ideally be modelled as part of the bus transaction.

-- PMM
Avi Kivity - Oct. 25, 2012, 1:57 p.m.
On 10/25/2012 03:50 PM, Peter Maydell wrote:
> On 25 October 2012 14:41, Avi Kivity <avi@redhat.com> wrote:
>> On 10/25/2012 03:28 PM, Peter Maydell wrote:
>>> On 25 October 2012 14:21, Avi Kivity <avi@redhat.com> wrote:
>>>> You could easily have the top-level container have ->ops that generate
>>>> an exception.
>>>
>>> Ah, yes, there's an 'accepts' callback. (That's kind of awkward
>>> as an API because it means your decode logic gets spread between
>>> read, write and accept: there are some devices where it would be
>>> nice to have the 'default:' case of the address switch say "unknown
>>> offset, raise decode error". If the read callback took a uint64_t*
>>> rather than returning the read data, we could make both read and
>>> write return a success/decode-error type of status result.)
>>
>> I actually forgot about ->accepts().  But it isn't needed for this use
>> case, just have the lowest priority region (the container) implement
>> ->read/write that generate the exception
> 
> I don't understand this -- read/write don't have any way of saying
> "please generate an exception". The only thing I can see in the
> API that does that is returning false from accepts().

read/write can call anything.  So if the SoC code installs the lowest
region, it has access to whatever mechanisms generate the exceptions.

(it may not have access to CPUState though).

> 
>> wrt decode duplication, I've been thinking of a single ->service()
>> callback that accepts a Transaction argument, including all the details
>> (offset, data, and direction).
> 
> If we do this we should make sure that the Transaction allows us to
> include CPU-architecture dependent info -- for ARM we would want to
> model transaction attributes like 'secure/nonsecure', 'privileged/nonpriv',
> 'instruction/data', etc. You also want to include in the transaction
> attributes who the master end of this transaction is (so a slave
> can distinguish accesses from a particular CPU core in a cluster,
> for instance). This would allow us to remove some of the current
> nasty hacks where devices reach into the CPUArchState to  retrieve
> info that should ideally be modelled as part of the bus transaction.

Sounds like good arguments for another sweep.
Peter Crosthwaite - Oct. 25, 2012, 1:59 p.m.
On Thu, Oct 25, 2012 at 11:50 PM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 25 October 2012 14:41, Avi Kivity <avi@redhat.com> wrote:
>> On 10/25/2012 03:28 PM, Peter Maydell wrote:
>>> On 25 October 2012 14:21, Avi Kivity <avi@redhat.com> wrote:
>>>> You could easily have the top-level container have ->ops that generate
>>>> an exception.
>>>
>>> Ah, yes, there's an 'accepts' callback. (That's kind of awkward
>>> as an API because it means your decode logic gets spread between
>>> read, write and accept: there are some devices where it would be
>>> nice to have the 'default:' case of the address switch say "unknown
>>> offset, raise decode error". If the read callback took a uint64_t*
>>> rather than returning the read data, we could make both read and
>>> write return a success/decode-error type of status result.)
>>
>> I actually forgot about ->accepts().  But it isn't needed for this use
>> case, just have the lowest priority region (the container) implement
>> ->read/write that generate the exception
>
> I don't understand this -- read/write don't have any way of saying
> "please generate an exception". The only thing I can see in the
> API that does that is returning false from accepts().
>
>> wrt decode duplication, I've been thinking of a single ->service()
>> callback that accepts a Transaction argument, including all the details
>> (offset, data, and direction).
>
> If we do this we should make sure that the Transaction allows us to
> include CPU-architecture dependent info -- for ARM we would want to
> model transaction attributes like 'secure/nonsecure', 'privileged/nonpriv',
> 'instruction/data', etc.

If we want these features shouldnt we just bite the bullet and
impelement AXI with its own set of QOM definitions? Axi slaves then
inherit from TYPE_AXI_SLAVE. If we want ARM specific features then we
should replace SYSBUS with an ARM bus that does these things rather
than push them up to the memory API.

Add to that ARM bus feature list exclusive/non-exclusive as well.

You also want to include in the transaction
> attributes who the master end of this transaction is (so a slave
> can distinguish accesses from a particular CPU core in a cluster,
> for instance). This would allow us to remove some of the current
> nasty hacks where devices reach into the CPUArchState to  retrieve
> info that should ideally be modelled as part of the bus transaction.
>
> -- PMM
>
Peter Maydell - Oct. 25, 2012, 2:08 p.m.
On 25 October 2012 14:59, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Thu, Oct 25, 2012 at 11:50 PM, Peter Maydell
> <peter.maydell@linaro.org> wrote:
>> On 25 October 2012 14:41, Avi Kivity <avi@redhat.com> wrote:
>>> wrt decode duplication, I've been thinking of a single ->service()
>>> callback that accepts a Transaction argument, including all the details
>>> (offset, data, and direction).
>>
>> If we do this we should make sure that the Transaction allows us to
>> include CPU-architecture dependent info -- for ARM we would want to
>> model transaction attributes like 'secure/nonsecure', 'privileged/nonpriv',
>> 'instruction/data', etc.
>
> If we want these features shouldnt we just bite the bullet and
> impelement AXI with its own set of QOM definitions? Axi slaves then
> inherit from TYPE_AXI_SLAVE. If we want ARM specific features then we
> should replace SYSBUS with an ARM bus that does these things rather
> than push them up to the memory API.

The memory API should provide the basic feature set that you can
use to implement system specific buses and transaction signals with.
We don't want to have reimplement basic support for reading and
writing all over again.

Also, we don't necesasrily want to implement AXI specifically;
we need to implement the programmer-visible parts of it, but many
of those carry over to AXI, APB, AHB...

> Add to that ARM bus feature list exclusive/non-exclusive as well.

Ah yes. We don't really model ARM exclusive transactions at all well
anywhere in QEMU ; I think it all works more by luck than anything
else at the moment...

-- PMM

Patch

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 78f9dfd..b6418bc 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -396,6 +396,8 @@  struct EHCIState {
     MemoryRegion mem_caps;
     MemoryRegion mem_opreg;
     MemoryRegion mem_ports;
+    MemoryRegion mem_other_low;
+    MemoryRegion mem_other_high;
     int companion_count;
 
     /* properties */
@@ -2773,17 +2775,27 @@  static void usb_ehci_initfn(EHCIState *s, DeviceState *dev)
     qemu_add_vm_change_state_handler(usb_ehci_vm_state_change, s);
 
     memory_region_init(&s->mem, "ehci", MMIO_SIZE);
+    if (s->capabase) {
+        memory_region_init_ram(&s->mem_other_low, "other-low", s->capabase);
+    }
     memory_region_init_io(&s->mem_caps, &ehci_mmio_caps_ops, s,
                           "capabilities", s->opregbase);
     memory_region_init_io(&s->mem_opreg, &ehci_mmio_opreg_ops, s,
                           "operational", PORTSC_BEGIN);
     memory_region_init_io(&s->mem_ports, &ehci_mmio_port_ops, s,
                           "ports", PORTSC_END - PORTSC_BEGIN);
+    memory_region_init_ram(&s->mem_other_high, "other-high", MMIO_SIZE -
+                           s->opregbase - (PORTSC_END - PORTSC_BEGIN));
 
+    if (s->capabase) {
+        memory_region_add_subregion(&s->mem, 0, &s->mem_other_low);
+    }
     memory_region_add_subregion(&s->mem, s->capabase, &s->mem_caps);
     memory_region_add_subregion(&s->mem, s->opregbase, &s->mem_opreg);
     memory_region_add_subregion(&s->mem, s->opregbase + PORTSC_BEGIN,
                                 &s->mem_ports);
+    memory_region_add_subregion(&s->mem, s->opregbase + PORTSC_END,
+                                &s->mem_other_high);
 }
 
 static int usb_ehci_sysbus_initfn(SysBusDevice *dev)