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

[Cédric Le Goater]

Hello,

On 3/31/20 1:02 PM, Philippe Mathieu-Daudé wrote:
> 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

Here is an updated version I have been keeping since : 

        
https://github.com/legoater/qemu/commit/d57ac950c4be47a2bafd6c6a96dec2922c2ecd65

which breaks qtest :

        microbit-test.c:307:test_nrf51_gpio: assertion failed (actual == 0x01): 
(0 == 1)

I haven't had time to look at this closely but it would be nice to get this 
patch merged. It's a requirement for the Aspeed ADC model.

Thanks,

c. 

>>
>> 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;
>>
>