Patchwork [v2,11/11] usb/ehci: Put RAM in undefined MMIO regions

login
register
mail settings
Submitter Peter Crosthwaite
Date Oct. 26, 2012, 5:47 a.m.
Message ID <0c11f3611c74b87c53cfff65ea24f8d6ec82f666.1351229557.git.peter.crosthwaite@xilinx.com>
Download mbox | patch
Permalink /patch/194365/
State New
Headers show

Comments

Peter Crosthwaite - Oct. 26, 2012, 5: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>
---
changed from v1:
greatly simplified implementation.

 hw/usb/hcd-ehci.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Peter Maydell - Oct. 26, 2012, 8:37 a.m.
On 26 October 2012 06:47, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> 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).

I definitely don't like this. RAZ/WI might be OK, but implementing
random areas of devices as RAM smells like a hack. Better would be
to identify what the guest is actually doing in these areas.

And we still need to actually identify what is segfaulting and
fix that as a separate bug -- can we have a backtrace please?

-- PMM
Gerd Hoffmann - Oct. 26, 2012, 12:36 p.m.
On 10/26/12 07: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).

Make that an io region, have the read() handler return 0xff, write
handler do nothing except maybe logging/tracing the access for debugging
purposes.  That is more correct for unassigned mmio space than backing
by memory.  Adding memory also breaks migration btw.

I somehow still think this should be handled one layer up (i.e. the
parent region) which could do the approximate arch-specific action.

Any chance the access you are seeing is at offset 0x68?

cheers,
  Gerd
Peter Maydell - Oct. 26, 2012, 12:39 p.m.
On 26 October 2012 13:36, Gerd Hoffmann <kraxel@redhat.com> wrote:
> On 10/26/12 07: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).
>
> Make that an io region, have the read() handler return 0xff, write
> handler do nothing except maybe logging/tracing the access for debugging
> purposes.  That is more correct for unassigned mmio space than backing
> by memory.  Adding memory also breaks migration btw.
>
> I somehow still think this should be handled one layer up (i.e. the
> parent region) which could do the approximate arch-specific action.

If it's really in the memory space of the device itself then our device
model should be handling it.

-- PMM
Peter Crosthwaite - Oct. 27, 2012, 12:42 a.m.
On Fri, Oct 26, 2012 at 10:36 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> On 10/26/12 07: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).
>
> Make that an io region, have the read() handler return 0xff, write
> handler do nothing except maybe logging/tracing the access for debugging
> purposes.  That is more correct for unassigned mmio space than backing
> by memory.  Adding memory also breaks migration btw.
>
> I somehow still think this should be handled one layer up (i.e. the
> parent region) which could do the approximate arch-specific action.
>
> Any chance the access you are seeing is at offset 0x68?
>

0x1a8. which for the opregbase + 0x068 for zynq so probably what you
are thinking about. I think the linux kernel is trying to explicitly
put the device in root mode rather than device mode, but those regs
are unimplemented in EHCI.

Regards,
Peter

> cheers,
>   Gerd
>
>
Peter Crosthwaite - Oct. 27, 2012, 4:08 p.m.
On Fri, Oct 26, 2012 at 10:39 PM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 26 October 2012 13:36, Gerd Hoffmann <kraxel@redhat.com> wrote:
>> On 10/26/12 07: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).
>>
>> Make that an io region, have the read() handler return 0xff, write
>> handler do nothing except maybe logging/tracing the access for debugging
>> purposes.  That is more correct for unassigned mmio space than backing
>> by memory.  Adding memory also breaks migration btw.
>>
>> I somehow still think this should be handled one layer up (i.e. the
>> parent region) which could do the approximate arch-specific action.
>
> If it's really in the memory space of the device itself then our device
> model should be handling it.
>

Yeh I admit patch is a hack and ultimately out of scope of this
series. Im going to drop it and put it on my workarounds branch for
the moment (wont appear in v3). Ill get you guys that segfault info,
and see if Gerd has any insights on what the device should actually
do.

Regards,
Peter

> -- PMM
>

Patch

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 26086b2..77874ae 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -2797,7 +2797,7 @@  static void usb_ehci_initfn(EHCIState *s, DeviceState *dev)
     qemu_register_reset(ehci_reset, s);
     qemu_add_vm_change_state_handler(usb_ehci_vm_state_change, s);
 
-    memory_region_init(&s->mem, "ehci", MMIO_SIZE);
+    memory_region_init_ram(&s->mem, "ehci", MMIO_SIZE);
     memory_region_init_io(&s->mem_caps, &ehci_mmio_caps_ops, s,
                           "capabilities", CAPA_SIZE);
     memory_region_init_io(&s->mem_opreg, &ehci_mmio_opreg_ops, s,