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

[Peter Maydell]

On Wed, 7 Jul 2021 at 02:25, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 7/2/21 4:45 AM, Peter Maydell wrote:
> > On Fri, 2 Jul 2021 at 12:02, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >> On 7/2/21 12:40 PM, Peter Maydell wrote:
> >>>   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.)
>
> Perhaps just better to retain current behaviour with this patch by extending 
> the case to
> the ends.

Makes sense. I propose to squash this diff into this patch, which
should make it a no-behaviour-change refactor, and then queue the
series to target-arm.next, since it's now all reviewed except for
patch 11 which is a comment-only change. (If you think this is a bit
fast I'm happy to post a v2 instead -- as a bugfix patchset this is
still OK post-softfreeze, so it's just a matter of my personal convenience
to be able to put it into the arm pull I'm going to do either this
afternoon or tomorrow.)

diff --git a/hw/gpio/pl061.c b/hw/gpio/pl061.c
index 0f5d12e6d5a..b21b230402f 100644
--- a/hw/gpio/pl061.c
+++ b/hw/gpio/pl061.c
@@ -151,7 +151,7 @@ static uint64_t pl061_read(void *opaque, hwaddr offset,
     PL061State *s = (PL061State *)opaque;

     switch (offset) {
-    case 0x0 ... 0x3fc: /* Data */
+    case 0x0 ... 0x3ff: /* Data */
         return s->data & (offset >> 2);
     case 0x400: /* Direction */
         return s->dir;
@@ -224,9 +224,7 @@ static uint64_t pl061_read(void *opaque, hwaddr offset,
             goto bad_offset;
         }
         return s->amsel;
-    case 0x52c ... 0xfcc: /* Reserved */
-        goto bad_offset;
-    case 0xfd0 ... 0xffc: /* ID registers */
+    case 0xfd0 ... 0xfff: /* ID registers */
         return s->id[(offset - 0xfd0) >> 2];
     default:
     bad_offset:
@@ -244,7 +242,7 @@ static void pl061_write(void *opaque, hwaddr offset,
     uint8_t mask;

     switch (offset) {
-    case 0 ... 0x3fc:
+    case 0 ... 0x3ff:
         mask = (offset >> 2) & s->dir;
         s->data = (s->data & ~mask) | (value & mask);
         pl061_update(s);

thanks
-- PMM