| Message ID | 20210702104018.19881-4-peter.maydell@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/arm: Make virt board secure powerdown/reset work | expand |
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?
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
On 7/2/21 3:40 AM, Peter Maydell wrote: > + case 0x52c ... 0xfcc: /* Reserved */ > + goto bad_offset; Any reason to not just use default for these? Otherwise, Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
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: >> >> 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.) Perhaps just better to retain current behaviour with this patch by extending the case to the ends. If you want to check oddness of offset, use a separate patch. r~
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
On 7/8/21 2:39 AM, Peter Maydell wrote: > 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. Sounds good. r~
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); case 0x400: /* Direction */ return s->dir; case 0x404: /* Interrupt sense */ @@ -178,33 +170,70 @@ static uint64_t pl061_read(void *opaque, hwaddr offset, case 0x420: /* Alternate function select */ return s->afsel; case 0x500: /* 2mA drive */ + if (s->id != pl061_id_luminary) { + goto bad_offset; + } return s->dr2r; case 0x504: /* 4mA drive */ + if (s->id != pl061_id_luminary) { + goto bad_offset; + } return s->dr4r; case 0x508: /* 8mA drive */ + if (s->id != pl061_id_luminary) { + goto bad_offset; + } return s->dr8r; case 0x50c: /* Open drain */ + if (s->id != pl061_id_luminary) { + goto bad_offset; + } return s->odr; case 0x510: /* Pull-up */ + if (s->id != pl061_id_luminary) { + goto bad_offset; + } return s->pur; case 0x514: /* Pull-down */ + if (s->id != pl061_id_luminary) { + goto bad_offset; + } return s->pdr; case 0x518: /* Slew rate control */ + if (s->id != pl061_id_luminary) { + goto bad_offset; + } return s->slr; case 0x51c: /* Digital enable */ + if (s->id != pl061_id_luminary) { + goto bad_offset; + } return s->den; case 0x520: /* Lock */ + if (s->id != pl061_id_luminary) { + goto bad_offset; + } return s->locked; case 0x524: /* Commit */ + if (s->id != pl061_id_luminary) { + goto bad_offset; + } return s->cr; case 0x528: /* Analog mode select */ + if (s->id != pl061_id_luminary) { + goto bad_offset; + } return s->amsel; + case 0x52c ... 0xfcc: /* Reserved */ + goto bad_offset; + case 0xfd0 ... 0xffc: /* ID registers */ + return s->id[(offset - 0xfd0) >> 2]; default: + bad_offset: + qemu_log_mask(LOG_GUEST_ERROR, + "pl061_read: Bad offset %x\n", (int)offset); break; } -err_out: - qemu_log_mask(LOG_GUEST_ERROR, - "pl061_read: Bad offset %x\n", (int)offset); return 0; } @@ -214,16 +243,12 @@ static void pl061_write(void *opaque, hwaddr offset, PL061State *s = (PL061State *)opaque; uint8_t mask; - if (offset < 0x400) { + switch (offset) { + case 0 ... 0x3fc: mask = (offset >> 2) & s->dir; s->data = (s->data & ~mask) | (value & mask); pl061_update(s); return; - } - if (offset >= s->rsvd_start) { - goto err_out; - } - switch (offset) { case 0x400: /* Direction */ s->dir = value & 0xff; break; @@ -247,47 +272,80 @@ static void pl061_write(void *opaque, hwaddr offset, s->afsel = (s->afsel & ~mask) | (value & mask); break; case 0x500: /* 2mA drive */ + if (s->id != pl061_id_luminary) { + goto bad_offset; + } s->dr2r = value & 0xff; break; case 0x504: /* 4mA drive */ + if (s->id != pl061_id_luminary) { + goto bad_offset; + } s->dr4r = value & 0xff; break; case 0x508: /* 8mA drive */ + if (s->id != pl061_id_luminary) { + goto bad_offset; + } s->dr8r = value & 0xff; break; case 0x50c: /* Open drain */ + if (s->id != pl061_id_luminary) { + goto bad_offset; + } s->odr = value & 0xff; break; case 0x510: /* Pull-up */ + if (s->id != pl061_id_luminary) { + goto bad_offset; + } s->pur = value & 0xff; break; case 0x514: /* Pull-down */ + if (s->id != pl061_id_luminary) { + goto bad_offset; + } s->pdr = value & 0xff; break; case 0x518: /* Slew rate control */ + if (s->id != pl061_id_luminary) { + goto bad_offset; + } s->slr = value & 0xff; break; case 0x51c: /* Digital enable */ + if (s->id != pl061_id_luminary) { + goto bad_offset; + } s->den = value & 0xff; break; case 0x520: /* Lock */ + if (s->id != pl061_id_luminary) { + goto bad_offset; + } s->locked = (value != 0xacce551); break; case 0x524: /* Commit */ + if (s->id != pl061_id_luminary) { + goto bad_offset; + } if (!s->locked) s->cr = value & 0xff; break; case 0x528: + if (s->id != pl061_id_luminary) { + goto bad_offset; + } s->amsel = value & 0xff; break; default: - goto err_out; + bad_offset: + qemu_log_mask(LOG_GUEST_ERROR, + "pl061_write: Bad offset %x\n", (int)offset); + return; } pl061_update(s); return; -err_out: - qemu_log_mask(LOG_GUEST_ERROR, - "pl061_write: Bad offset %x\n", (int)offset); } static void pl061_reset(DeviceState *dev) @@ -343,7 +401,6 @@ static void pl061_luminary_init(Object *obj) PL061State *s = PL061(obj); s->id = pl061_id_luminary; - s->rsvd_start = 0x52c; } static void pl061_init(Object *obj) @@ -353,7 +410,6 @@ static void pl061_init(Object *obj) SysBusDevice *sbd = SYS_BUS_DEVICE(obj); s->id = pl061_id; - s->rsvd_start = 0x424; memory_region_init_io(&s->iomem, obj, &pl061_ops, s, "pl061", 0x1000); sysbus_init_mmio(sbd, &s->iomem);
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(-)