On Tue, Oct 1, 2024 at 11:24 AM Peter Maydell <peter.mayd...@linaro.org>
wrote:
> 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.
>
Great, thanks for the suggestion! I will include it in v2.
Best regards,
Strahinja
>
> thanks
> -- PMM
>
