[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
RE: [PATCH v2 1/3] arm_gic: Mask the un-supported priority bits
From: |
Sai Pavan Boddu |
Subject: |
RE: [PATCH v2 1/3] arm_gic: Mask the un-supported priority bits |
Date: |
Mon, 24 Feb 2020 09:36:19 +0000 |
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 <address@hidden>
> Sent: Friday, February 21, 2020 9:00 PM
> To: Sai Pavan Boddu <address@hidden>
> Cc: Edgar E . Iglesias <address@hidden>; Alistair Francis
> <address@hidden>; Anthony Liguori <address@hidden>;
> Andreas Färber <address@hidden>; qemu-arm <address@hidden>;
> QEMU Developers <address@hidden>
> 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
> <address@hidden> 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 <address@hidden>
> > ---
> > 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