[PATCH 0/3] Fix number of priority bits in zynq gic

[PATCH 0/3] Fix number of priority bits in zynq gic

[Sai Pavan Boddu]

This patch series implements the mask for un-implemented priority bits
in arm-gic. Which will return the expected number of priority bits on
read.

Sai Pavan Boddu (3):
  arm_gic: Mask the un-supported priority bits
  cpu/a9mpcore: Add num priority bits property
  arm/xilinx_zynq: Set number of priority bits

 hw/arm/xilinx_zynq.c             | 1 +
 hw/cpu/a9mpcore.c                | 2 ++
 hw/intc/arm_gic.c                | 9 ++++++---
 hw/intc/arm_gic_common.c         | 1 +
 include/hw/cpu/a9mpcore.h        | 1 +
 include/hw/intc/arm_gic_common.h | 1 +
 6 files changed, 12 insertions(+), 3 deletions(-)

-- 
2.7.4

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

[Sai Pavan Boddu]

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(-)

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
-- 
2.7.4

Re: [PATCH 1/3] arm_gic: Mask the un-supported priority bits

[Peter Maydell]

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

RE: [PATCH 1/3] arm_gic: Mask the un-supported priority bits

[Sai Pavan Boddu]

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

[PATCH 2/3] cpu/a9mpcore: Add num priority bits property

[Sai Pavan Boddu]

Set number of priority bits property of gic as guided by machine
configuration.

Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
---
 hw/cpu/a9mpcore.c         | 2 ++
 include/hw/cpu/a9mpcore.h | 1 +
 2 files changed, 3 insertions(+)

diff --git a/hw/cpu/a9mpcore.c b/hw/cpu/a9mpcore.c
index 1f8bc8a..eb1e752 100644
--- a/hw/cpu/a9mpcore.c
+++ b/hw/cpu/a9mpcore.c
@@ -68,6 +68,7 @@ static void a9mp_priv_realize(DeviceState *dev, Error **errp)
     gicdev = DEVICE(&s->gic);
     qdev_prop_set_uint32(gicdev, "num-cpu", s->num_cpu);
     qdev_prop_set_uint32(gicdev, "num-irq", s->num_irq);
+    qdev_prop_set_uint32(gicdev, "num-prio-bits", s->n_prio_bits);
 
     /* Make the GIC's TZ support match the CPUs. We assume that
      * either all the CPUs have TZ, or none do.
@@ -167,6 +168,7 @@ static Property a9mp_priv_properties[] = {
      * Other boards may differ and should set this property appropriately.
      */
     DEFINE_PROP_UINT32("num-irq", A9MPPrivState, num_irq, 96),
+    DEFINE_PROP_UINT32("num-priority-bits", A9MPPrivState, n_prio_bits, 8),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/cpu/a9mpcore.h b/include/hw/cpu/a9mpcore.h
index 5d67ca2..4f146bd 100644
--- a/include/hw/cpu/a9mpcore.h
+++ b/include/hw/cpu/a9mpcore.h
@@ -28,6 +28,7 @@ typedef struct A9MPPrivState {
     uint32_t num_cpu;
     MemoryRegion container;
     uint32_t num_irq;
+    uint32_t n_prio_bits;
 
     A9SCUState scu;
     GICState gic;
-- 
2.7.4

Re: [PATCH 2/3] cpu/a9mpcore: Add num priority bits property

[Peter Maydell]

On Fri, 14 Feb 2020 at 13:21, Sai Pavan Boddu
<sai.pavan.boddu@xilinx.com> wrote:
>
> Set number of priority bits property of gic as guided by machine
> configuration.
>
> Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
> ---
>  hw/cpu/a9mpcore.c         | 2 ++
>  include/hw/cpu/a9mpcore.h | 1 +
>  2 files changed, 3 insertions(+)
>
> diff --git a/hw/cpu/a9mpcore.c b/hw/cpu/a9mpcore.c
> index 1f8bc8a..eb1e752 100644
> --- a/hw/cpu/a9mpcore.c
> +++ b/hw/cpu/a9mpcore.c
> @@ -68,6 +68,7 @@ static void a9mp_priv_realize(DeviceState *dev, Error **errp)
>      gicdev = DEVICE(&s->gic);
>      qdev_prop_set_uint32(gicdev, "num-cpu", s->num_cpu);
>      qdev_prop_set_uint32(gicdev, "num-irq", s->num_irq);
> +    qdev_prop_set_uint32(gicdev, "num-prio-bits", s->n_prio_bits);
>
>      /* Make the GIC's TZ support match the CPUs. We assume that
>       * either all the CPUs have TZ, or none do.
> @@ -167,6 +168,7 @@ static Property a9mp_priv_properties[] = {
>       * Other boards may differ and should set this property appropriately.
>       */
>      DEFINE_PROP_UINT32("num-irq", A9MPPrivState, num_irq, 96),
> +    DEFINE_PROP_UINT32("num-priority-bits", A9MPPrivState, n_prio_bits, 8),
>      DEFINE_PROP_END_OF_LIST(),

You should be able to just directly pass through the property
from the GIC object by calling
    object_property_add_alias(obj, "num-priority-bits", OBJECT(&s->gic),
                              "num-priority-bits", &error_abort);
at the end of a9mp_priv_initfn().

Then you don't need to have a DEFINE_PROP* for it, or a field in
the state struct, or manually pass the value on in realize.

(We don't do this for the existing num-irq and num-cpu properties
because in those cases this device itself needs to know the
values, as well as passing them on to other devices under it.)

thanks
-- PMM

Re: [PATCH 2/3] cpu/a9mpcore: Add num priority bits property

[Peter Maydell]

On Tue, 18 Feb 2020 at 18:16, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Fri, 14 Feb 2020 at 13:21, Sai Pavan Boddu
> <sai.pavan.boddu@xilinx.com> wrote:
> >
> > Set number of priority bits property of gic as guided by machine
> > configuration.
> >
> > Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
> > ---
> >  hw/cpu/a9mpcore.c         | 2 ++
> >  include/hw/cpu/a9mpcore.h | 1 +
> >  2 files changed, 3 insertions(+)
> >
> > diff --git a/hw/cpu/a9mpcore.c b/hw/cpu/a9mpcore.c
> > index 1f8bc8a..eb1e752 100644
> > --- a/hw/cpu/a9mpcore.c
> > +++ b/hw/cpu/a9mpcore.c
> > @@ -68,6 +68,7 @@ static void a9mp_priv_realize(DeviceState *dev, Error **errp)
> >      gicdev = DEVICE(&s->gic);
> >      qdev_prop_set_uint32(gicdev, "num-cpu", s->num_cpu);
> >      qdev_prop_set_uint32(gicdev, "num-irq", s->num_irq);
> > +    qdev_prop_set_uint32(gicdev, "num-prio-bits", s->n_prio_bits);
> >
> >      /* Make the GIC's TZ support match the CPUs. We assume that
> >       * either all the CPUs have TZ, or none do.
> > @@ -167,6 +168,7 @@ static Property a9mp_priv_properties[] = {
> >       * Other boards may differ and should set this property appropriately.
> >       */
> >      DEFINE_PROP_UINT32("num-irq", A9MPPrivState, num_irq, 96),
> > +    DEFINE_PROP_UINT32("num-priority-bits", A9MPPrivState, n_prio_bits, 8),
> >      DEFINE_PROP_END_OF_LIST(),
>
> You should be able to just directly pass through the property
> from the GIC object by calling
>     object_property_add_alias(obj, "num-priority-bits", OBJECT(&s->gic),
>                               "num-priority-bits", &error_abort);
> at the end of a9mp_priv_initfn().

On second thoughts, and having checked the manual:

All A9 CPUs have a GIC with 5 bits of priority. So we should
just set the num-priority-bits on the GIC object in a9mpcore.c,
and not expose it as a property of this container device.

That will be a behaviour change for:
 - exynos4210 boards
 - fsl-imx6 boards
 - highbank
 - realview
 - vexpress
as well as xilinx, but it is a bug fix, and I have enough test
images for those to check it doesn't completely break them.
So we should just go ahead and implement them correctly.

Similarly, the 11MPCore GIC always has 4 bits of priority,
so we could get the behaviour right for the 'realview-eb-mpcore'
board too.

thanks
-- PMM

[PATCH 3/3] arm/xilinx_zynq: Set number of priority bits

[Sai Pavan Boddu]

Arm GIC in Zynq SOC implements 5 priority bits i.e bits 7..3.

Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
---
 hw/arm/xilinx_zynq.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 3a0fa5b..7aa43ad 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -214,6 +214,7 @@ static void zynq_init(MachineState *machine)
 
     dev = qdev_create(NULL, TYPE_A9MPCORE_PRIV);
     qdev_prop_set_uint32(dev, "num-cpu", 1);
+    qdev_prop_set_uint32(dev, "num-priority-bits", 5);
     qdev_init_nofail(dev);
     busdev = SYS_BUS_DEVICE(dev);
     sysbus_mmio_map(busdev, 0, MPCORE_PERIPHBASE);
-- 
2.7.4