qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Cameron Esfahani
Subject: Re: [PATCH v1] usb: Add read support for HCIVERSION register to XHCI
Date: Tue, 31 Mar 2020 02:57:56 -0700

Philippe -
From what I've seen, access size has nothing to do with alignment.

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;




reply via email to

[Prev in Thread] Current Thread [Next in Thread]