On Mon, 30 Sept 2024 at 22:28, Strahinja Jankovic
<strahinjapjanko...@gmail.com> wrote:
>
> Hi Peter,
>
> Thank you very much for the review and detailed comments.
>
> I will try to address all comments in the v2 of the patches, but I have some
> questions I added below.
>
> On Mon, Sep 30, 2024 at 4:45 PM Peter Maydell <peter.mayd...@linaro.org>
> wrote:
>> > +static uint64_t allwinner_a10_spi_read(void *opaque, hwaddr offset,
>> > + unsigned size)
>> > +{
>> > + uint32_t value = 0;
>> > + AWA10SPIState *s = opaque;
>> > + uint32_t index = offset >> 2;
>>
>> The MemoryRegionOps defines that size == 1 is permitted,
>> but this calculation of index without any validation
>> of offset means that if the guest writes a byte to
>> offset 1 we will treat that identically to writing a byte
>> to offset 0. This probably isn't what the hardware does.
>
> I checked once more the User manual, and it does not mention
> unaligned access (example for RXDATA)
>
> In 8-bits SPI bus width, this register can be accessed in byte,
> half-word or word unit by AHB. In byte accessing method,
> if there are words in RXFIFO, the top word is returned and
> the RXFIFO depth is decreased by 1. In half-word accessing
> method, the two SPI bursts are returned and the RXFIFO
> depth is decrease by 2. In word accessing method, the four
> SPI bursts are returned and the RXFIFO depth is decreased
> by 4.
>
> I chose not to cover the half-word and word access methods,
> since neither Linux kernel nor U-Boot use it
> (both use only byte access).
>
> I guess that I could add `.valid.unaligned = false` when
> initializing `MemoryRegionOps`?
This wouldn't help, because a 2-byte load at offset 2
is not "unaligned" from the memory system's point of view.
(Also, unaligned = false is the default.)
Looking again at your code I see I misread it a bit:
I thought it was doing a switch() on index, but it is on
offset.
Using a switch on offset makes it easy to handle the case
of "bogus write to an offset that isn't the start of a
register":
switch (offset) {
case SOME_REG_OFFSET:
/*
* handle it; if size 1/2/4 behave differently,
* then look at 'size' to do the right thing.
*/
break;
/* etc */
default:
qemu_log_mask(LOG_GUEST_ERROR,
"%s: bad offset 0x%x\n", __func__,
(uint32_t)offset);
break;
}
Then both the "offset that's not a defined register"
and "offset that's not aligned to the start of a register"
go into the default case, we log it as a guest error,
and do nothing.
thanks
-- PMM
