Re: [PATCH 03/11] hw/gpio/pl061: Clean up read/write offset handling logic

[Peter Maydell]

On Fri, 2 Jul 2021 at 12:02, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Hi Peter,
>
> On 7/2/21 12:40 PM, Peter Maydell wrote:
> > Currently the pl061_read() and pl061_write() functions handle offsets
> > using a combination of three if() statements and a switch().  Clean
> > this up to use just a switch, using case ranges.
> >
> > This requires that instead of catching accesses to the luminary-only
> > registers on a stock PL061 via a check on s->rsvd_start we use
> > an "is this luminary?" check in the cases for each luminary-only
> > register.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >  hw/gpio/pl061.c | 106 ++++++++++++++++++++++++++++++++++++------------
> >  1 file changed, 81 insertions(+), 25 deletions(-)
> >
> > diff --git a/hw/gpio/pl061.c b/hw/gpio/pl061.c
> > index a6ace88895d..0f5d12e6d5a 100644
> > --- a/hw/gpio/pl061.c
> > +++ b/hw/gpio/pl061.c
> > @@ -55,7 +55,6 @@ struct PL061State {
> >      qemu_irq irq;
> >      qemu_irq out[N_GPIOS];
> >      const unsigned char *id;
> > -    uint32_t rsvd_start; /* reserved area: [rsvd_start, 0xfcc] */
> >  };
> >
> >  static const VMStateDescription vmstate_pl061 = {
> > @@ -151,16 +150,9 @@ static uint64_t pl061_read(void *opaque, hwaddr offset,
> >  {
> >      PL061State *s = (PL061State *)opaque;
> >
> > -    if (offset < 0x400) {
> > -        return s->data & (offset >> 2);
> > -    }
> > -    if (offset >= s->rsvd_start && offset <= 0xfcc) {
> > -        goto err_out;
> > -    }
> > -    if (offset >= 0xfd0 && offset < 0x1000) {
> > -        return s->id[(offset - 0xfd0) >> 2];
> > -    }
> >      switch (offset) {
> > +    case 0x0 ... 0x3fc: /* Data */
> > +        return s->data & (offset >> 2);
>
> Don't we need to set pl061_ops.impl.min/max_access_size = 4
> to keep the same logic?

I think the hardware intends to permit accesses of any width, but only
at 4-byte boundaries. There is a slight behaviour change here:
accesses to 0x3fd, 0x3fe, 0x3ff now fall into the default case (ie error)
rather than being treated like 0x3fc, and similarly accesses to 0xfdd,
0xfde, 0xfdf are errors rather than treated like 0xfdc. But I think
that it's probably more correct to consider those to be errors.

(We could explicitly check and goto err_out if (offset & 3)
right at the top, I suppose.)

-- PMM