Re: [PATCH v1] usb: Add read support for HCIVERSION register to XHCI

[Philippe Mathieu-Daudé]

On 3/31/20 11:57 AM, Cameron Esfahani wrote:
> Philippe -
>  From what I've seen, access size has nothing to do with alignment.

Yes, I was wondering if you were using unaligned accesses.

I *think* the correct fix is in the "memory: Support unaligned accesses 
on aligned-only models" patch from Andrew Jeffery:
https://www.mail-archive.com/address@hidden/msg461247.html

> What the code in access_with_adjusted_size() will do is make sure that "size" is 
> >= min_access_size and <= max_access_size.
>
> So reading 2-bytes from address 2 turns into reading 4-bytes from address 2: 
> xhci_cap_read() is called with reg 2, size 4.
>
> But, due to the fact our change to support reg 2 only returns back 2-bytes, and 
> how the loops work in access_with_adjusted_size(), we only call xhci_cap_read() 
> once.
>
> It seems like we should also change impl.min_access_size for xhci_cap_ops to be 
> 2.
>
> But, after that, to support people doing strange things like reading 
> traditionally 4-byte values as 2 2-byte values, we probably need to change 
> xhci_cap_read() to handle every memory range in steps of 2-bytes.
>
> But I'll defer to Gerd on this...
>
> Cameron Esfahani
> address@hidden
>
> "Americans are very skilled at creating a custom meaning from something that's 
> mass-produced."
>
> Ann Powers
>
>
> > On Mar 31, 2020, at 12:52 AM, Philippe Mathieu-Daudé <address@hidden> wrote:
> >
> > On 3/30/20 11:44 PM, Cameron Esfahani via wrote:
> > > macOS will read HCIVERSION separate from CAPLENGTH.  Add a distinct
> > > handler for that register.
> >
> > Apparently a fix for https://bugs.launchpad.net/qemu/+bug/1693050.
> >
> > > Signed-off-by: Cameron Esfahani <address@hidden>
> > > ---
> > >   hw/usb/hcd-xhci.c | 3 +++
> > >   1 file changed, 3 insertions(+)
> > > diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> > > index b330e36fe6..061f8438de 100644
> > > --- a/hw/usb/hcd-xhci.c
> > > +++ b/hw/usb/hcd-xhci.c
> > > @@ -2739,6 +2739,9 @@ static uint64_t xhci_cap_read(void *ptr, hwaddr reg, 
> > > unsigned size)
> > >       case 0x00: /* HCIVERSION, CAPLENGTH */
> > >           ret = 0x01000000 | LEN_CAP;
> > >           break;
> > > +    case 0x02: /* HCIVERSION */
> > > +        ret = 0x0100;
> > > +        break;
> >
> > But we have:
> >
> > static const MemoryRegionOps xhci_cap_ops = {
> >     .read = xhci_cap_read,
> >     .write = xhci_cap_write,
> >     .valid.min_access_size = 1,
> >     .valid.max_access_size = 4,
> >     .impl.min_access_size = 4,
> >     .impl.max_access_size = 4,
> >     .endianness = DEVICE_LITTLE_ENDIAN,
> > };
> >
> > IIUC ".impl.min_access_size = 4" means the case 'reg == 2' can not happen. It 
> > seems we have a bug in memory.c elsewhere.
> >
> > How can we reproduce?
> >
> > If not easy, can you share the backtrace of:
> >
> > -- >8 --
> > diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> > index b330e36fe6..d021129f3f 100644
> > --- a/hw/usb/hcd-xhci.c
> > +++ b/hw/usb/hcd-xhci.c
> > @@ -2735,6 +2735,7 @@ static uint64_t xhci_cap_read(void *ptr, hwaddr reg, 
> > unsigned size)
> >      XHCIState *xhci = ptr;
> >      uint32_t ret;
> >
> > +    assert(reg != 2);
> >      switch (reg) {
> >      case 0x00: /* HCIVERSION, CAPLENGTH */
> >          ret = 0x01000000 | LEN_CAP;
> > ---
> >
> > >       case 0x04: /* HCSPARAMS 1 */
> > >           ret = ((xhci->numports_2+xhci->numports_3)<<24)
> > >               | (xhci->numintrs<<8) | xhci->numslots;