Hi Peter,
All your suggestions look good, I will send at V2. But I think I have done a
mistake in V1, More comments inline below.
> -----Original Message-----
> From: Peter Maydell <peter.mayd...@linaro.org>
> Sent: Tuesday, February 18, 2020 11:40 PM
> To: Sai Pavan Boddu <saip...@xilinx.com>
> Cc: Edgar E . Iglesias <edgar.igles...@gmail.com>; Alistair Francis
> <alist...@alistair23.me>; Anthony Liguori <anth...@codemonkey.ws>;
> Andreas Färber <afaer...@suse.de>; qemu-arm <qemu-...@nongnu.org>;
> QEMU Developers <qemu-devel@nongnu.org>
> Subject: Re: [PATCH 1/3] arm_gic: Mask the un-supported priority bits
>
> On Fri, 14 Feb 2020 at 13:21, Sai Pavan Boddu
> <sai.pavan.bo...@xilinx.com> wrote:
> >
> > Priority bits implemented in arm-gic can 8 to 4, un-implemented bits
> > are read as zeros(RAZ).
>
> This is nice to see -- I've known our GIC was a bit out-of-spec in this area
> but
> it's good to see it's less painful to retrofit than I thought it might be.
>
> > Signed-off-by: Sai Pavan Boddu <sai.pavan.bo...@xilinx.com>
> > ---
> > hw/intc/arm_gic.c | 9 ++++++---
> > hw/intc/arm_gic_common.c | 1 +
> > include/hw/intc/arm_gic_common.h | 1 +
> > 3 files changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c index
> > 1d7da7b..8875330 100644
> > --- a/hw/intc/arm_gic.c
> > +++ b/hw/intc/arm_gic.c
> > @@ -43,6 +43,9 @@
> > } \
> > } while (0)
> >
> > +#define UMASK(n) \
> > + ((((1 << n) - 1) << (8 - n)) & 0xFF)
>
> This is a bit confusingly named (usually 'umask' is the file-permission mask
> on
> unix). I think it's worth following the pattern used in
> hw/intc/arm_gicv3_cpuif.c for icv_fullprio_mask(), and using a function with
> a comment describing it. Also, you've not considered the virtualization parts
> of the GIC, which also use these codepaths. In those cases, the value of the
> mask is always based on GIC_VIRT_MAX_GROUP_PRIO_BITS of priority (a
> GICv2 has 5 bits of priority in the VGIC, always). So:
>
> static uint32_t gic_fullprio_mask(GICState *s, int cpu) {
> /*
> * Return a mask word which clears the unimplemented priority
> * bits from a priority value for an interrupt. (Not to be
> * confused with the group priority, whose mask depends on BPR.)
> */
> int pribits;
>
> if (gic_is_vcpu(cpu)) {
> pribits = GIC_VIRT_MAX_GROUP_PRIO_BITS;
> } else {
> pribits = s->n_prio_bits;
> }
> return ~0U << (8 - s->n_prio_bits);
> }
>
> > +
> > static const uint8_t gic_id_11mpcore[] = {
> > 0x00, 0x00, 0x00, 0x00, 0x90, 0x13, 0x04, 0x00, 0x0d, 0xf0, 0x05,
> > 0xb1 }; @@ -652,9 +655,9 @@ void gic_dist_set_priority(GICState *s,
> > int cpu, int irq, uint8_t val,
> > }
> >
> > if (irq < GIC_INTERNAL) {
> > - s->priority1[irq][cpu] = val;
> > + s->priority1[irq][cpu] = val & UMASK(s->n_prio_bits) ;
> > } else {
> > - s->priority2[(irq) - GIC_INTERNAL] = val;
> > + s->priority2[(irq) - GIC_INTERNAL] = val &
> > + UMASK(s->n_prio_bits);
> > }
> > }
>
> Slightly cleaner to just put
> val &= gic_fullprio_mask(s);
> before the if() rather than doing the same thing in both branches.
>
> >
> > @@ -684,7 +687,7 @@ static void gic_set_priority_mask(GICState *s, int
> cpu, uint8_t pmask,
> > return;
> > }
> > }
> > - s->priority_mask[cpu] = pmask;
> > + s->priority_mask[cpu] = pmask & UMASK(s->n_prio_bits);
[Sai Pavan Boddu] mask should be applied in " gic_dist_get_priority ", as we
miss group priority bits here.
Let me know, if my understanding is correct.
Thanks for the review.
Regards,
Sai Pavan
> > }
> >
> > static uint32_t gic_get_priority_mask(GICState *s, int cpu,
> > MemTxAttrs attrs) diff --git a/hw/intc/arm_gic_common.c
> > b/hw/intc/arm_gic_common.c index e6c4fe7..e4668c7 100644
> > --- a/hw/intc/arm_gic_common.c
> > +++ b/hw/intc/arm_gic_common.c
> > @@ -357,6 +357,7 @@ static Property arm_gic_common_properties[] = {
> > DEFINE_PROP_BOOL("has-security-extensions", GICState, security_extn,
> 0),
> > /* True if the GIC should implement the virtualization extensions */
> > DEFINE_PROP_BOOL("has-virtualization-extensions", GICState,
> > virt_extn, 0),
> > + DEFINE_PROP_UINT32("num-prio-bits", GICState, n_prio_bits, 8),
>
> In patch 2 you use "num-priority-bits" for the proprety name on the
> a9mpcore object. I like that better, and I think we should name the property
> the same thing on both devices.
>
> You should have some code in the realize method which sanity checks the
> n_prio_bits value we are passed. It can't be more than 8, and I'm not sure
> what the lowest valid value is. Your commit message says 4. I'm pretty sure
> that if the GIC has the virt extensions then it can't be less than
> GIC_VIRT_MAX_GROUP_PRIO_BITS (ie 5).
>
> thanks
> -- PMM
