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

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

[Cameron Esfahani via]

macOS will read HCIVERSION separate from CAPLENGTH.  Add a distinct
handler for that register.

Signed-off-by: Cameron Esfahani <dirty@apple.com>
---
 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;
     case 0x04: /* HCSPARAMS 1 */
         ret = ((xhci->numports_2+xhci->numports_3)<<24)
             | (xhci->numintrs<<8) | xhci->numslots;
-- 
2.24.0

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

[Philippe Mathieu-Daudé]

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 <dirty@apple.com>
> ---
>   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;
> 

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

[Cameron Esfahani via]

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
dirty@apple.com

"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é <philmd@redhat.com> 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 <dirty@apple.com>
>> ---
>>  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;

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

[Cameron Esfahani via]

Actually, reading the specification a little more, the only fields which are documented as being smaller than 4-bytes are CAPLENGTH (1-byte) and HCIVERSION (2-bytes).

So maybe change impl.min_access_size to 1 and rely on the fact that traditionally callers haven't read less than 4-bytes for any of the 4-byte fields...

Cameron Esfahani
dirty@apple.com

"In the elder days of Art, Builders wrought with greatest care each minute and unseen part; For the gods see everywhere."

"The Builders", H. W. Longfellow



> On Mar 31, 2020, at 2:57 AM, Cameron Esfahani via <qemu-devel@nongnu.org> wrote:
> 
> 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...
> 

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/qemu-devel@nongnu.org/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
> dirty@apple.com
> 
> "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é <philmd@redhat.com> 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 <dirty@apple.com>
>>> ---
>>>   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;
> 

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/qemu-devel@nongnu.org/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
>> dirty@apple.com
>>
>> "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é <philmd@redhat.com> 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 <dirty@apple.com>
>>>> ---
>>>>   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;
>>
> 

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

[Andrew Jeffery]

On Wed, 1 Apr 2020, at 21:53, Cédric Le Goater 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/qemu-devel@nongnu.org/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.

Second this! I never had the chance to circle back and fix it up.

Andrew

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

[Cameron Esfahani via]

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
dirty@apple.com

"The cake is a lie."

Common wisdom

> On Apr 1, 2020, at 4:23 AM, Cédric Le Goater <clg@kaod.org> 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/qemu-devel@nongnu.org/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
>>> dirty@apple.com
>>> 
>>> "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é <philmd@redhat.com> 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 <dirty@apple.com>
>>>>> ---
>>>>>   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;
>>> 
>> 
> 
> 

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

[Gerd Hoffmann]

On Tue, Mar 31, 2020 at 02:57:56AM -0700, Cameron Esfahani wrote:
> 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.

Ouch.  I didn't expect xhci being called like that.  IMO memory.c should
do properly aligned 4-byte reads, then extract the bytes needed, i.e.
2-byte read @ both offset 0 and 2 would result in xhci being called with
a 4-byte read at offset 0, then memory.c would extract the lower or upper
bytes and return them to the guest.

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

Well, the point of having MemoryRegionOps.valid and MemoryRegionOps.impl
in the first place is to allow memory.c handle this on behalf of the
device, so we don't end up reinventing the wheel in each and every
device ...

So, I think xhci is correct and memory.c has a bug somewhere (and it
seems Philippe is on track pinning it down ...)

cheers,
  Gerd