Re: [PATCH] hw/aspeed: Correct minimum access size for all models

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 1/4/25 00:49, Philippe Mathieu-Daudé wrote:
> Hi Joel,
> 
> On 19/11/24 11:29, Peter Maydell wrote:
>> On Tue, 19 Nov 2024 at 02:53, Joel Stanley <joel@jms.id.au> wrote:
>>>
>>> On Mon, 18 Nov 2024 at 20:40, Peter Maydell 
>>> <peter.maydell@linaro.org> wrote:
>>>> Have you reviewed all the device read/write function
>>>> implementations for these devices to check whether
>>>> (a) changing the .valid value does the right thing, or
>>>
>>> I read the implementation of the read/write memory ops and I believe
>>> it does the right thing. We want devices to accept reads that are of
>>> any size, instead of returning an error.
>>>
>>>> (b) whether there are cases where we should instead
>>>> be updating the implementation and setting the .impl
>>>> min access size ?
>>>
>>> Reading the documentation for impl vs valid, we definitely want to set
>>> valid to 1. There should be no machine check when performing byte
>>> reads.
>>>
>>> I don't think we want to change .impl.min from the default of 1.
>>>
>>> I'm not sure if I've missed something that you're trying to point out.
>>> Are there gotchas about setting valid.min=1 that I should know about?
>>
>> The "gotcha" is that the memory system's implementation of the
>> size 1 and 2 reads when .impl.min is 4 and .valid.min is 1
>> (as for instance with aspeed_apb2opb_ops after this patch)
>> is "read 4 bytes, return the relevant portion"
>> and the implementation of size 1 and 2 writes is "pad the
>> small value with zeroes at either end appropriately so it is
>> 4 bytes and write that". That is often the right thing for
>> the required behaviour of the device registers, but it is
>> also quite common that it is the wrong behaviour. For instance
>> for some devices the write of a byte is supposed to only modify
>> that byte, and leave the other bytes of a 4-byte register alone.
>> Or if the device has bit-clears-on-read behaviour for a register
>> then the default handling will incorrectly do that for bits
>> that the guest didn't actually read.
>>
>> Conversely if the device leaves the .impl.min at its default 1
>> and moves .valid.min from 4 to 1 (as with eg ftgmac100_ops)
>> the device will now be called for byte reads and writes at any
>> address in its range. If a write to, say, byte 3 of a 32-bit
>> register is supposed to update bits [31:24], that won't happen
>> unless the write function is changed (usually if there's a switch
>> on offset the write to something that's not at a multiple-of-4
>> will end up in the default "log an error" code path).
>>
>> What this adds up to is that it's a bit misleading to have
>> a single patch which changes the minimum access size for lots
>> of devices at once, because for each device you need to look
>> at QEMU's implementation of the read and write functions
>> together with the spec of the device, and confirm that the
>> right way to implement "byte writes are supported" for this
>> particular device is to change .valid.min, and that you don't
>> also need to make changes to the read or write function code
>> at the same time or adjust .impl.min. Putting them all in one
>> patch with no discussion in the commit message of the device
>> behaviour and implementation was just a bit of a yellow flag
>> to me that maybe this complexity wasn't considered.
>>
>> If we get this wrong for one of these devices, it's also likely
>> to be rather easier to bisect the problem if bisection
>> can track it down to "we made this change to aspeed_timer"
>> rather than "we made this change to a dozen devices all at once".
> 
> Could you respin splitting 1 device per patch?

I addressed Peter's concerns for the SCU only so we can get the
issue resolved for the next release:
https://lore.kernel.org/qemu-devel/20250331230444.88295-1-philmd@linaro.org/