On Tue, 19 Nov 2024 at 02:53, Joel Stanley <j...@jms.id.au> wrote: > > On Mon, 18 Nov 2024 at 20:40, Peter Maydell <peter.mayd...@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". -- PMM
