| Message ID | 1582270927-2568-2-git-send-email-sai.pavan.boddu@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/3] arm_gic: Mask the un-supported priority bits | expand |
On Fri, 21 Feb 2020 at 07:46, Sai Pavan Boddu <sai.pavan.boddu@xilinx.com> wrote: > > Priority bits implemented in arm-gic can be 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 | 26 ++++++++++++++++++++++++-- > hw/intc/arm_gic_common.c | 1 + > include/hw/intc/arm_gic_common.h | 1 + > 3 files changed, 26 insertions(+), 2 deletions(-) > > diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c > index 1d7da7b..dec8767 100644 > --- a/hw/intc/arm_gic.c > +++ b/hw/intc/arm_gic.c > @@ -641,6 +641,23 @@ uint32_t gic_acknowledge_irq(GICState *s, int cpu, MemTxAttrs attrs) > return ret; > } > > +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 unimpBits; > + > + if (gic_is_vcpu(cpu)) { > + unimpBits = GIC_VIRT_MAX_GROUP_PRIO_BITS; > + } else { > + unimpBits = 8 - s->n_prio_bits; This isn't right; GIC_VIRT_MAX_GROUP_PRIO_BITS should be handled the same way as s->n_prio_bits. The expression I suggested in my comment on your v1 should work: 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); > + } > + return ~0U << unimpBits; > +} > + > void gic_dist_set_priority(GICState *s, int cpu, int irq, uint8_t val, > MemTxAttrs attrs) > { You seem to have lost the part of the patch which applies the mask in gic_dist_set_priority(). If the GIC only has 5 bits of priority we should not allow the guest to set more than that. > @@ -669,7 +686,7 @@ static uint32_t gic_dist_get_priority(GICState *s, int cpu, int irq, > } > prio = (prio << 1) & 0xff; /* Non-secure view */ > } > - return prio; > + return prio & gic_fullprio_mask(s, cpu); > } > > static void gic_set_priority_mask(GICState *s, int cpu, uint8_t pmask, > @@ -684,7 +701,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 & gic_fullprio_mask(s, cpu); > } > > static uint32_t gic_get_priority_mask(GICState *s, int cpu, MemTxAttrs attrs) > @@ -2055,6 +2072,11 @@ static void arm_gic_realize(DeviceState *dev, Error **errp) > return; > } > > + if (s->n_prio_bits > 8) { > + error_setg(errp, "num-priority-bits cannot be greater than 8"); > + return; > + } You need to also check that the value is at least as large as the lowest permitted value, as I suggested in my v1 comment. thanks -- PMM
Hi Peter, Will send V3 for below comments. In v2 I may have confused with functionality of group priority interrupt bits. Now things look clear. Thanks. Regards, Sai Pavan > -----Original Message----- > From: Peter Maydell <peter.maydell@linaro.org> > Sent: Friday, February 21, 2020 9:00 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 v2 1/3] arm_gic: Mask the un-supported priority bits > > On Fri, 21 Feb 2020 at 07:46, Sai Pavan Boddu > <sai.pavan.boddu@xilinx.com> wrote: > > > > Priority bits implemented in arm-gic can be 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 | 26 ++++++++++++++++++++++++-- > > hw/intc/arm_gic_common.c | 1 + > > include/hw/intc/arm_gic_common.h | 1 + > > 3 files changed, 26 insertions(+), 2 deletions(-) > > > > diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c index > > 1d7da7b..dec8767 100644 > > --- a/hw/intc/arm_gic.c > > +++ b/hw/intc/arm_gic.c > > @@ -641,6 +641,23 @@ uint32_t gic_acknowledge_irq(GICState *s, int > cpu, MemTxAttrs attrs) > > return ret; > > } > > > > +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 unimpBits; > > + > > + if (gic_is_vcpu(cpu)) { > > + unimpBits = GIC_VIRT_MAX_GROUP_PRIO_BITS; > > + } else { > > + unimpBits = 8 - s->n_prio_bits; > > This isn't right; GIC_VIRT_MAX_GROUP_PRIO_BITS should be handled the > same way as s->n_prio_bits. The expression I suggested in my comment on > your v1 should work: > > 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); > > > + } > > + return ~0U << unimpBits; > > +} > > + > > void gic_dist_set_priority(GICState *s, int cpu, int irq, uint8_t val, > > MemTxAttrs attrs) { > > > You seem to have lost the part of the patch which applies the mask in > gic_dist_set_priority(). If the GIC only has 5 bits of priority we should not > allow the guest to set more than that. > > > @@ -669,7 +686,7 @@ static uint32_t gic_dist_get_priority(GICState *s, > int cpu, int irq, > > } > > prio = (prio << 1) & 0xff; /* Non-secure view */ > > } > > - return prio; > > + return prio & gic_fullprio_mask(s, cpu); > > } > > > > static void gic_set_priority_mask(GICState *s, int cpu, uint8_t > > pmask, @@ -684,7 +701,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 & gic_fullprio_mask(s, cpu); > > } > > > > static uint32_t gic_get_priority_mask(GICState *s, int cpu, > > MemTxAttrs attrs) @@ -2055,6 +2072,11 @@ static void > arm_gic_realize(DeviceState *dev, Error **errp) > > return; > > } > > > > + if (s->n_prio_bits > 8) { > > + error_setg(errp, "num-priority-bits cannot be greater than 8"); > > + return; > > + } > > You need to also check that the value is at least as large as the lowest > permitted value, as I suggested in my v1 comment. > > thanks > -- PMM
diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c index 1d7da7b..dec8767 100644 --- a/hw/intc/arm_gic.c +++ b/hw/intc/arm_gic.c @@ -641,6 +641,23 @@ uint32_t gic_acknowledge_irq(GICState *s, int cpu, MemTxAttrs attrs) return ret; } +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 unimpBits; + + if (gic_is_vcpu(cpu)) { + unimpBits = GIC_VIRT_MAX_GROUP_PRIO_BITS; + } else { + unimpBits = 8 - s->n_prio_bits; + } + return ~0U << unimpBits; +} + void gic_dist_set_priority(GICState *s, int cpu, int irq, uint8_t val, MemTxAttrs attrs) { @@ -669,7 +686,7 @@ static uint32_t gic_dist_get_priority(GICState *s, int cpu, int irq, } prio = (prio << 1) & 0xff; /* Non-secure view */ } - return prio; + return prio & gic_fullprio_mask(s, cpu); } static void gic_set_priority_mask(GICState *s, int cpu, uint8_t pmask, @@ -684,7 +701,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 & gic_fullprio_mask(s, cpu); } static uint32_t gic_get_priority_mask(GICState *s, int cpu, MemTxAttrs attrs) @@ -2055,6 +2072,11 @@ static void arm_gic_realize(DeviceState *dev, Error **errp) return; } + if (s->n_prio_bits > 8) { + error_setg(errp, "num-priority-bits cannot be greater than 8"); + return; + } + /* This creates distributor, main CPU interface (s->cpuiomem[0]) and if * enabled, virtualization extensions related interfaces (main virtual * interface (s->vifaceiomem[0]) and virtual CPU interface). diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c index e6c4fe7..7b44d56 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-priority-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 be 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 | 26 ++++++++++++++++++++++++-- hw/intc/arm_gic_common.c | 1 + include/hw/intc/arm_gic_common.h | 1 + 3 files changed, 26 insertions(+), 2 deletions(-)