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

[Cameron Esfahani]

I had a look at this failing test case after running applying Cedric's patch 
(https://github.com/legoater/qemu/commit/d57ac950c4be47a2bafd6c6a96dec2922c2ecd65).

It looks like this is just a bug in the test case.  The test case is attempting 
to verify that the CNF registers are programmed correctly by sampling the first 
and last CNF register.  The bug is that NRF51_GPIO_REG_CNF_END doesn't actually 
refer to the start of the last valid CNF register.  It refers to the last byte 
of the last valid CNF register.  So either NRF51_GPIO_REG_CNF_END needs to be 
adjusted to 0x77C or we need to subtract 3 to get to the start of the register. 
 Considering the NRF51 doesn't appear to support unaligned access, it seems 
like changing NRF51_GPIO_REG_CNF_END to 0x77C is reasonable.

Here's a patch which seems to fix it for me:

diff --git a/include/hw/gpio/nrf51_gpio.h b/include/hw/gpio/nrf51_gpio.h
index 337ee53..1d62bbc 100644
--- a/include/hw/gpio/nrf51_gpio.h
+++ b/include/hw/gpio/nrf51_gpio.h
@@ -42,7 +42,7 @@
 #define NRF51_GPIO_REG_DIRSET       0x518
 #define NRF51_GPIO_REG_DIRCLR       0x51C
 #define NRF51_GPIO_REG_CNF_START    0x700
-#define NRF51_GPIO_REG_CNF_END      0x77F
+#define NRF51_GPIO_REG_CNF_END      0x77C
 
 #define NRF51_GPIO_PULLDOWN 1
 #define NRF51_GPIO_PULLUP 3

Considering this change works for pre-Cedric patch and post, I'll post at 
official version shortly.

And hopefully this unblocks review of Cedric's patch...

Cameron Esfahani
address@hidden

"The cake is a lie."

Common wisdom

> On Apr 1, 2020, at 4:23 AM, Cédric Le Goater <address@hidden> wrote:
> 
> 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;
>>> 
>> 
> 
>