| Message ID | 1346330948-12833-1-git-send-email-alex@securiforest.com |
|---|---|
| State | New |
| Headers | show |
On 08/30/12 14:49, Alejandro Martinez Ruiz wrote: > Some xHC drivers (most notably on Windows and BSD systems) read > the first capability registers using 1 and 2 bytes accesses, since > this is how they are defined in section 5.3 of the xHCI specs. > > Enabling these kind of read accesses allows Windows and FreeBSD > guests to properly recognize the host controller. > > As this is an exception to the general 4-byte aligned accesses rule, > we special-case the code path for capability reading and implement > checks to guard against wrong size/alignment combinations. No need to do that by hand, the memory api can handle it. Can you check whenever usb-next (http://www.kraxel.org/cgit/qemu/log/?h=rebase/usb-next) works for you? thanks, Gerd
Gerd, On Tue, Sep 4, 2012 at 3:30 PM, Gerd Hoffmann <kraxel@redhat.com> wrote: > On 08/30/12 14:49, Alejandro Martinez Ruiz wrote: >> Some xHC drivers (most notably on Windows and BSD systems) read >> the first capability registers using 1 and 2 bytes accesses, since >> this is how they are defined in section 5.3 of the xHCI specs. >> >> Enabling these kind of read accesses allows Windows and FreeBSD >> guests to properly recognize the host controller. >> >> As this is an exception to the general 4-byte aligned accesses rule, >> we special-case the code path for capability reading and implement >> checks to guard against wrong size/alignment combinations. > > No need to do that by hand, the memory api can handle it. Can you check > whenever usb-next > (http://www.kraxel.org/cgit/qemu/log/?h=rebase/usb-next) works for you? usb-next will fail to compile using -Werror with: hcd-xhci.c: In function ‘xhci_fire_ctl_transfer’: hcd-xhci.c:1508:14: error: variable ‘wLength’ set but not used [-Werror=unused-but-set-variable] It will also crash at runtime at host-linux.c:usb_handle_control_packet(), since a NULL value is passed from xhci_address_slot() for the USBPacket *p argument, and an assert is testing for p->result == 0. Other than that, this problem is resolved. I will perform further testing and report back, since the in-tree xHC driver has never really worked with any guest other than Linux. Thanks, Alex
Hi, > usb-next will fail to compile using -Werror with: > hcd-xhci.c: In function ‘xhci_fire_ctl_transfer’: > hcd-xhci.c:1508:14: error: variable ‘wLength’ set but not used > [-Werror=unused-but-set-variable] Fixed. Which gcc is this? Must be pretty cutting edge, even on Fedora 17 (gcc 4.7.0) I don't see that one ... > It will also crash at runtime at > host-linux.c:usb_handle_control_packet(), since a NULL value is passed > from xhci_address_slot() for the USBPacket *p argument, and an assert > is testing for p->result == 0. Fixed & pushed to usb-next. thanks, Gerd
Hi Gerd, On Thu, Sep 6, 2012 at 12:15 PM, Gerd Hoffmann <kraxel@redhat.com> wrote: > Hi, > >> usb-next will fail to compile using -Werror with: >> hcd-xhci.c: In function ‘xhci_fire_ctl_transfer’: >> hcd-xhci.c:1508:14: error: variable ‘wLength’ set but not used >> [-Werror=unused-but-set-variable] > > Fixed. > > Which gcc is this? Must be pretty cutting edge, even on Fedora 17 (gcc > 4.7.0) I don't see that one ... Actually it is from OpenSuSE's 12.1 rpm, at version 4.6.2. >> It will also crash at runtime at >> host-linux.c:usb_handle_control_packet(), since a NULL value is passed >> from xhci_address_slot() for the USBPacket *p argument, and an assert >> is testing for p->result == 0. > > Fixed & pushed to usb-next. Thanks, will try it out. Thanks, Alex
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c index 6c2ff02..6cca161 100644 --- a/hw/usb/hcd-xhci.c +++ b/hw/usb/hcd-xhci.c @@ -2320,13 +2320,30 @@ static void xhci_reset(DeviceState *dev) xhci->ev_buffer_get = 0; } -static uint32_t xhci_cap_read(XHCIState *xhci, uint32_t reg) +static uint32_t xhci_cap_read(XHCIState *xhci, uint32_t reg, unsigned size) { - uint32_t ret; + uint32_t ret = 0; + + /* + * Section 5.3 of the xHCI specification defines the first capability + * registers as being only 1 and 2 bytes in size. In fact, these are + * often accessed as 1 or 2 bytes reads. + * + * Some drivers read the first 4 bytes in one go, while others -most + * notably the original NEC Renesas driver for Windows and the *BSDs- + * read one register at a time. This is the only known exception to + * the 4 byte accesses rule, so we'll special-case the code. + */ switch (reg) { - case 0x00: /* HCIVERSION, CAPLENGTH */ - ret = 0x01000000 | LEN_CAP; + case 0x00: /* CAPLENGTH [, HCIVERSION] */ + ret = LEN_CAP; + if (size < 4) { + break; + } + /* fall-through if asking for all 4 bytes */ + case 0x02: /* HCIVERSION */ + ret |= 0x01000000 >> (4 - size) * CHAR_BIT; break; case 0x04: /* HCSPARAMS 1 */ ret = (MAXPORTS<<24) | (MAXINTRS<<8) | MAXSLOTS; @@ -2685,26 +2702,43 @@ static void xhci_doorbell_write(XHCIState *xhci, uint32_t reg, uint32_t val) static uint64_t xhci_mem_read(void *ptr, target_phys_addr_t addr, unsigned size) { + uint64_t ret = 0; XHCIState *xhci = ptr; - /* Only aligned reads are allowed on xHCI */ - if (addr & 3) { - fprintf(stderr, "xhci_mem_read: Mis-aligned read\n"); - return 0; - } - + /* Allow 1, 2 and 4-byte aligned reads on capabilities, and only + * 4-byte reads elsewhere. + */ if (addr < LEN_CAP) { - return xhci_cap_read(xhci, addr); - } else if (addr >= OFF_OPER && addr < (OFF_OPER + LEN_OPER)) { - return xhci_oper_read(xhci, addr - OFF_OPER); - } else if (addr >= OFF_RUNTIME && addr < (OFF_RUNTIME + LEN_RUNTIME)) { - return xhci_runtime_read(xhci, addr - OFF_RUNTIME); - } else if (addr >= OFF_DOORBELL && addr < (OFF_DOORBELL + LEN_DOORBELL)) { - return xhci_doorbell_read(xhci, addr - OFF_DOORBELL); + /* deny accesses to odd addresses, specially since we accept 1-byte reads */ + if (addr & 1) { + fprintf(stderr, "xhci_mem_read: invalid %ud-byte capability read at address %x\n", size, (unsigned int) addr); + goto out; + } + + /* We deal with read size down in xhci_cap_read, since access + * is variable for some addresses. + */ + ret = xhci_cap_read(xhci, addr, size); } else { - fprintf(stderr, "xhci_mem_read: Bad offset %x\n", (int)addr); - return 0; + /* non capability read */ + if (size < 4) { + fprintf(stderr, "xhci_mem_read: mis-aligned %ud-byte read on address %x\n", size, (unsigned int) addr); + goto out; + } + + if (addr >= OFF_OPER && addr < (OFF_OPER + LEN_OPER)) { + ret = xhci_oper_read(xhci, addr - OFF_OPER); + } else if (addr >= OFF_RUNTIME && addr < (OFF_RUNTIME + LEN_RUNTIME)) { + ret = xhci_runtime_read(xhci, addr - OFF_RUNTIME); + } else if (addr >= OFF_DOORBELL && addr < (OFF_DOORBELL + LEN_DOORBELL)) { + ret = xhci_doorbell_read(xhci, addr - OFF_DOORBELL); + } else { + fprintf(stderr, "xhci_mem_read: tried to read %ud bytes from bad offset %x\n", size, (unsigned int) addr); + } } + +out: + return ret; } static void xhci_mem_write(void *ptr, target_phys_addr_t addr, @@ -2732,8 +2766,9 @@ static void xhci_mem_write(void *ptr, target_phys_addr_t addr, static const MemoryRegionOps xhci_mem_ops = { .read = xhci_mem_read, .write = xhci_mem_write, - .valid.min_access_size = 4, + .valid.min_access_size = 1, .valid.max_access_size = 4, + .valid.unaligned = false, .endianness = DEVICE_LITTLE_ENDIAN, };
Some xHC drivers (most notably on Windows and BSD systems) read the first capability registers using 1 and 2 bytes accesses, since this is how they are defined in section 5.3 of the xHCI specs. Enabling these kind of read accesses allows Windows and FreeBSD guests to properly recognize the host controller. As this is an exception to the general 4-byte aligned accesses rule, we special-case the code path for capability reading and implement checks to guard against wrong size/alignment combinations. Signed-off-by: Alejandro Martinez Ruiz <alex@securiforest.com> --- hw/usb/hcd-xhci.c | 75 ++++++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 55 insertions(+), 20 deletions(-)