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.mayd...@linaro.org> > Sent: Friday, February 21, 2020 9:00 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 v2 1/3] arm_gic: Mask the un-supported priority bits > > On Fri, 21 Feb 2020 at 07:46, Sai Pavan Boddu > <sai.pavan.bo...@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.bo...@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