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
>> di...@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é <phi...@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 <di...@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;
>>
>
