Re: [PATCH v2 08/10] dp8393x: don't force 32-bit register access

[Philippe Mathieu-Daudé]

On 7/3/21 8:21 AM, Mark Cave-Ayland wrote:
> On 02/07/2021 05:36, Finn Thain wrote:
> 
>>> On 6/25/21 8:53 AM, Mark Cave-Ayland wrote:
>>>> Commit 3fe9a838ec "dp8393x: Always use 32-bit accesses" assumed that
>>>> all accesses to the registers were 32-bit
>>
>> No, that assumption was not made there. Just take a look at my commits in
>> Linux that make 16-bit accesses. If commit 3fe9a838ec worked by accident,
>> it probably just reflects my inadequate knowledge of QEMU internals.
>>
>>>> but this is actually not the case. The access size is determined by
>>>> the CPU instruction used and not the number of physical address lines.
>>>>
>>
>> I think that's an over-simplification (in the context of commit
>> 3fe9a838ec).
> 
> Let me try and clarify this a bit more: there are 2 different changes
> incorporated into 3fe9a838ec. The first (as you mention below and also
> detailed in the commit messge), is related to handling writes to the
> upper 16-bits of a word from the device and ensuring that 32-bit
> accesses are handled correctly. This part seems correct to me based upon
> reading the datasheet and the commit message:
> 
> @@ -246,9 +246,19 @@ static void dp8393x_put(dp8393xState *s, int width,
> int offset,
>                          uint16_t val)
>  {
>      if (s->big_endian) {
> -        s->data[offset * width + width - 1] = cpu_to_be16(val);
> +        if (width == 2) {
> +            s->data[offset * 2] = 0;
> +            s->data[offset * 2 + 1] = cpu_to_be16(val);
> +        } else {
> +            s->data[offset] = cpu_to_be16(val);
> +        }
>      } else {
> -        s->data[offset * width] = cpu_to_le16(val);
> +        if (width == 2) {
> +            s->data[offset * 2] = cpu_to_le16(val);
> +            s->data[offset * 2 + 1] = 0;
> +        } else {
> +            s->data[offset] = cpu_to_le16(val);
> +        }
>      }
>  }
> 
> The second change incorporated into 3fe9a838ec (and the one this patch
> fixes) is a similar change made to the MMIO *register* accesses:
> 
> @@ -590,7 +600,7 @@ static uint64_t dp8393x_read(void *opaque, hwaddr
> addr, unsigned int size)
> 
>      DPRINTF("read 0x%04x from reg %s\n", val, reg_names[reg]);
> 
> -    return val;
> +    return s->big_endian ? val << 16 : val;
>  }
> 
> and:
> 
> @@ -598,13 +608,14 @@ static void dp8393x_write(void *opaque, hwaddr
> addr, uint64_t data,
>  {
>      dp8393xState *s = opaque;
>      int reg = addr >> s->it_shift;
> +    uint32_t val = s->big_endian ? data >> 16 : data;
> 
> This is not correct since the QEMU memory API handles any access size
> and endian conversion before the MMIO access reaches the device. It is
> this change which breaks the 32-bit accesses used by MacOS to read/write
> the dp8393x registers because it applies an additional endian swap on
> top of that already done by the memory API.
> 
>>>> The big_endian workaround applied to the register read/writes was
>>>> actually caused by forcing the access size to 32-bit when the guest OS
>>>> was using a 16-bit access. Since the registers are 16-bit then we can
>>>> simply set .impl.min_access to 2 and then the memory API will
>>>> automatically do the right thing for both 16-bit accesses used by
>>>> Linux and 32-bit accesses used by the MacOS toolbox ROM.
>>>
>>> Hmm I'm not sure. This sounds to me like the "QEMU doesn't model busses
>>> so we end using kludge to hide bugs" pattern. Can you provide a QTest
>>> (ideally) or a "-trace memory_region_ops_\*" log of your firmware
>>> accessing the dp8393x please?
>>>
>> The DP83932 chip is highly configurable, so I'm not sure that the
>> behaviour of any given firmware would resolve the question.
> 
> Indeed, I don't think that will help much here. Phil, if you would still
> like me to send you some traces after reading the explanation above then
> do let me know.

I read it and still would have few traces. We can hand-write them too.

I'd like to add qtests for some read/write,16/32(A)==B.

>> Anyway, as far as the DP83932 hardware is concerned, the behaviour of the
>> upper 16-bits of the data bus depends on the configuration programmed
>> into
>> the DP83932 registers, and whether the chip is accessed as a slave or
>> performing DMA as a master.
> 
> The important part of the commit and its associated message is that it
> only changes the *register* accesses which were introduced as part of
> 3fe9a838ec.
> 
> In the end all the patch does is remove the manual endian swap from the
> MMIO registers since QEMU's memory API does the right thing all by
> itself, and adds the tweak below:
> 
> @@ -694,7 +693,7 @@ static void dp8393x_write(void *opaque, hwaddr addr,
> uint64_t data,
>  static const MemoryRegionOps dp8393x_ops = {
>      .read = dp8393x_read,
>      .write = dp8393x_write,
> -    .impl.min_access_size = 4,
> +    .impl.min_access_size = 2,
>      .impl.max_access_size = 4,
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
> 
> As Finn points out the dp8393x registers are 16-bit so we declare the
> implementation size as 2 bytes and the maximum size as 4 bytes. This
> allows Linux to function correctly with 16-bit accesses, whilst 32-bit
> accesses done by MacOS are split into 2 separate 16-bit accesses and
> combined automatically by the memory API.

I hadn't check the datasheet yet but will have a look.

Thanks,

Phil.