| Message ID | 1581686212-9625-2-git-send-email-sai.pavan.boddu@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix number of priority bits in zynq gic | expand |
On Fri, 14 Feb 2020 at 13:21, Sai Pavan Boddu <sai.pavan.boddu@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.boddu@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); > } > > 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
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.maydell@linaro.org> > Sent: Tuesday, February 18, 2020 11:40 PM > To: Sai Pavan Boddu <saipava@xilinx.com> > Cc: Edgar E . Iglesias <edgar.iglesias@gmail.com>; Alistair Francis > <alistair@alistair23.me>; Anthony Liguori <anthony@codemonkey.ws>; > Andreas Färber <afaerber@suse.de>; qemu-arm <qemu-arm@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.boddu@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.boddu@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
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) + 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); } } @@ -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); } 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), DEFINE_PROP_END_OF_LIST(), }; diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h index b5585fe..6e0d6b8 100644 --- a/include/hw/intc/arm_gic_common.h +++ b/include/hw/intc/arm_gic_common.h @@ -96,6 +96,7 @@ typedef struct GICState { uint16_t priority_mask[GIC_NCPU_VCPU]; uint16_t running_priority[GIC_NCPU_VCPU]; uint16_t current_pending[GIC_NCPU_VCPU]; + uint32_t n_prio_bits; /* If we present the GICv2 without security extensions to a guest, * the guest can configure the GICC_CTLR to configure group 1 binary point
Priority bits implemented in arm-gic can 8 to 4, un-implemented bits are read as zeros(RAZ). Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@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(-)