diff mbox series

[v2,1/3] arm_gic: Mask the un-supported priority bits

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

Commit Message

Sai Pavan Boddu Feb. 21, 2020, 7:42 a.m. UTC
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(-)

Comments

Peter Maydell Feb. 21, 2020, 3:30 p.m. UTC | #1
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
Sai Pavan Boddu Feb. 24, 2020, 9:36 a.m. UTC | #2
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 mbox series

Patch

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