[Qemu-devel] [PATCH 0/3] gicv3: Correct mishandling of NS BPR1 values

[Qemu-devel] [PATCH 0/3] gicv3: Correct mishandling of NS BPR1 values

[Peter Maydell]

I happened to be looking at the GICv3 spec and our implementation,
and I realized that we don't correctly calculate the mask values
to get group priorities based on BPR values if the NS BPR1 is
being used. This BPR, unlike the S BPR1 or the BPR0, has
off-by-one semantics, so instead of "use bits [7:1]" being
bpr == 0, it is bpr == 1, and so on, with an NS BPR1 value of
0 being impossible.

Patch 3 in this set fixes this error, and fixes the comments
so they're actually accurate. Patches 1 and 2 are preliminary
bug fixes which ensure that the NS BPR1 values are actually
constrained to be greater than 0, so that the assert()s added
in patch 3 can't fire.

I'm sending this as the last thing before I head out of the
door, so will respond to review comments in June...

thanks
-- PMM


Peter Maydell (3):
  hw/intc/arm_gicv3_cpuif: Fix reset value for VMCR_EL2.VBPR1
  hw/intc/arm_gicv3_cpuif: Don't let BPR be set below its minimum
  hw/intc/arm_gicv3_cpuif: Fix priority masking for NS BPR1

 hw/intc/arm_gicv3_cpuif.c | 49 ++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 44 insertions(+), 5 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/3] hw/intc/arm_gicv3_cpuif: Fix reset value for VMCR_EL2.VBPR1

[Peter Maydell]

We were setting the VBPR1 field of VMCR_EL2 to icv_min_vbpr()
on reset, but this is not correct. The field should reset to
the minimum value of ICV_BPR0_EL1 plus one.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/arm_gicv3_cpuif.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
index 0b20856..d31eba0 100644
--- a/hw/intc/arm_gicv3_cpuif.c
+++ b/hw/intc/arm_gicv3_cpuif.c
@@ -2014,7 +2014,7 @@ static void icc_reset(CPUARMState *env, const ARMCPRegInfo *ri)
     cs->ich_hcr_el2 = 0;
     memset(cs->ich_lr_el2, 0, sizeof(cs->ich_lr_el2));
     cs->ich_vmcr_el2 = ICH_VMCR_EL2_VFIQEN |
-        (icv_min_vbpr(cs) << ICH_VMCR_EL2_VBPR1_SHIFT) |
+        ((icv_min_vbpr(cs) + 1) << ICH_VMCR_EL2_VBPR1_SHIFT) |
         (icv_min_vbpr(cs) << ICH_VMCR_EL2_VBPR0_SHIFT);
 }
 
-- 
2.7.4

Re: [Qemu-devel] [Qemu-arm] [PATCH 1/3] hw/intc/arm_gicv3_cpuif: Fix reset value for VMCR_EL2.VBPR1

[Philippe Mathieu-Daudé]

On 04/26/2017 02:13 PM, Peter Maydell wrote:
> We were setting the VBPR1 field of VMCR_EL2 to icv_min_vbpr()
> on reset, but this is not correct. The field should reset to
> the minimum value of ICV_BPR0_EL1 plus one.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/intc/arm_gicv3_cpuif.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
> index 0b20856..d31eba0 100644
> --- a/hw/intc/arm_gicv3_cpuif.c
> +++ b/hw/intc/arm_gicv3_cpuif.c
> @@ -2014,7 +2014,7 @@ static void icc_reset(CPUARMState *env, const ARMCPRegInfo *ri)
>      cs->ich_hcr_el2 = 0;
>      memset(cs->ich_lr_el2, 0, sizeof(cs->ich_lr_el2));
>      cs->ich_vmcr_el2 = ICH_VMCR_EL2_VFIQEN |
> -        (icv_min_vbpr(cs) << ICH_VMCR_EL2_VBPR1_SHIFT) |
> +        ((icv_min_vbpr(cs) + 1) << ICH_VMCR_EL2_VBPR1_SHIFT) |

Indeed "a BPR of 0 is impossible (the minimum value is 1)"

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>          (icv_min_vbpr(cs) << ICH_VMCR_EL2_VBPR0_SHIFT);
>  }
>
>

[Qemu-devel] [PATCH 2/3] hw/intc/arm_gicv3_cpuif: Don't let BPR be set below its minimum

[Peter Maydell]

icc_bpr_write() was not enforcing that writing a value below the
minimum for the BPR should behave as if the BPR was set to the
minimum value. This doesn't make a difference for the secure
BPRs (since we define the minimum for the QEMU implementation
as zero) but did mean we were allowing the NS BPR1 to be set to
0 when 1 should be the lowest value.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/arm_gicv3_cpuif.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
index d31eba0..e660b3f 100644
--- a/hw/intc/arm_gicv3_cpuif.c
+++ b/hw/intc/arm_gicv3_cpuif.c
@@ -1388,6 +1388,7 @@ static void icc_bpr_write(CPUARMState *env, const ARMCPRegInfo *ri,
 {
     GICv3CPUState *cs = icc_cs_from_env(env);
     int grp = (ri->crm == 8) ? GICV3_G0 : GICV3_G1;
+    uint64_t minval;
 
     if (icv_access(env, grp == GICV3_G0 ? HCR_FMO : HCR_IMO)) {
         icv_bpr_write(env, ri, value);
@@ -1415,6 +1416,11 @@ static void icc_bpr_write(CPUARMState *env, const ARMCPRegInfo *ri,
         return;
     }
 
+    minval = (grp == GICV3_G1NS) ? GIC_MIN_BPR_NS : GIC_MIN_BPR;
+    if (value < minval) {
+        value = minval;
+    }
+
     cs->icc_bpr[grp] = value & 7;
     gicv3_cpuif_update(cs);
 }
-- 
2.7.4

Re: [Qemu-devel] [PATCH 2/3] hw/intc/arm_gicv3_cpuif: Don't let BPR be set below its minimum

[Philippe Mathieu-Daudé]

On 04/26/2017 02:13 PM, Peter Maydell wrote:
> icc_bpr_write() was not enforcing that writing a value below the
> minimum for the BPR should behave as if the BPR was set to the
> minimum value. This doesn't make a difference for the secure
> BPRs (since we define the minimum for the QEMU implementation
> as zero) but did mean we were allowing the NS BPR1 to be set to
> 0 when 1 should be the lowest value.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/intc/arm_gicv3_cpuif.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
> index d31eba0..e660b3f 100644
> --- a/hw/intc/arm_gicv3_cpuif.c
> +++ b/hw/intc/arm_gicv3_cpuif.c
> @@ -1388,6 +1388,7 @@ static void icc_bpr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>  {
>      GICv3CPUState *cs = icc_cs_from_env(env);
>      int grp = (ri->crm == 8) ? GICV3_G0 : GICV3_G1;
> +    uint64_t minval;
>
>      if (icv_access(env, grp == GICV3_G0 ? HCR_FMO : HCR_IMO)) {
>          icv_bpr_write(env, ri, value);
> @@ -1415,6 +1416,11 @@ static void icc_bpr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>          return;
>      }
>
> +    minval = (grp == GICV3_G1NS) ? GIC_MIN_BPR_NS : GIC_MIN_BPR;
> +    if (value < minval) {
> +        value = minval;
> +    }
> +

which is:

     if (grp == GICV3_G1NS) {
         value = MAX(value, GIC_MIN_BPR_NS);
     }

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>      cs->icc_bpr[grp] = value & 7;
>      gicv3_cpuif_update(cs);
>  }
>

Re: [Qemu-devel] [PATCH 2/3] hw/intc/arm_gicv3_cpuif: Don't let BPR be set below its minimum

[Peter Maydell]

On 14 May 2017 at 06:31, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On 04/26/2017 02:13 PM, Peter Maydell wrote:
>> +    minval = (grp == GICV3_G1NS) ? GIC_MIN_BPR_NS : GIC_MIN_BPR;
>> +    if (value < minval) {
>> +        value = minval;
>> +    }
>> +
>
>
> which is:
>
>     if (grp == GICV3_G1NS) {
>         value = MAX(value, GIC_MIN_BPR_NS);
>     }

Only if you assume GIC_MIN_BPR must always be 0, which isn't
necessarily the case. (One day we might need to update it to
be a per-device configurable parameter, like the VBPR min.)

thanks
-- PMM

[Qemu-devel] [PATCH 3/3] hw/intc/arm_gicv3_cpuif: Fix priority masking for NS BPR1

[Peter Maydell]

When we calculate the mask to use to get the group priority from
an interrupt priority, the way that NS BPR1 is handled differs
from how BPR0 and S BPR1 work -- a BPR1 value of 1 means
the group priority is in bits [7:1], whereas for BPR0 and S BPR1
this is indicated by a 0 BPR value.

Subtract 1 from the BPR value before creating the mask if
we're using the NS BPR value, for both hardware and virtual
interrupts, as the GICv3 pseudocode does, and fix the comments
accordingly.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/arm_gicv3_cpuif.c | 41 +++++++++++++++++++++++++++++++++++++----
 1 file changed, 37 insertions(+), 4 deletions(-)

diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
index e660b3f..eec53c8 100644
--- a/hw/intc/arm_gicv3_cpuif.c
+++ b/hw/intc/arm_gicv3_cpuif.c
@@ -216,18 +216,34 @@ static uint32_t icv_gprio_mask(GICv3CPUState *cs, int group)
 {
     /* Return a mask word which clears the subpriority bits from
      * a priority value for a virtual interrupt in the specified group.
-     * This depends on the VBPR value:
+     * This depends on the VBPR value. If using VBPR0 then:
      *  a BPR of 0 means the group priority bits are [7:1];
      *  a BPR of 1 means they are [7:2], and so on down to
      *  a BPR of 7 meaning no group priority bits at all.
+     * If using VBPR1 then:
+     *  a BPR of 0 is impossible (the minimum value is 1)
+     *  a BPR of 1 means the group priority bits are [7:1];
+     *  a BPR of 2 means they are [7:2], and so on down to
+     *  a BPR of 6 meaning no group priority bits at all.
+     *
      * Which BPR to use depends on the group of the interrupt and
      * the current ICH_VMCR_EL2.VCBPR settings.
+     *
+     * This corresponds to the VGroupBits() pseudocode.
      */
+    int bpr;
+
     if (group == GICV3_G1NS && cs->ich_vmcr_el2 & ICH_VMCR_EL2_VCBPR) {
         group = GICV3_G0;
     }
 
-    return ~0U << (read_vbpr(cs, group) + 1);
+    bpr = read_vbpr(cs, group);
+    if (group == GICV3_G1NS) {
+        assert(bpr > 0);
+        bpr--;
+    }
+
+    return ~0U << (bpr + 1);
 }
 
 static bool icv_hppi_can_preempt(GICv3CPUState *cs, uint64_t lr)
@@ -674,20 +690,37 @@ static uint32_t icc_gprio_mask(GICv3CPUState *cs, int group)
 {
     /* Return a mask word which clears the subpriority bits from
      * a priority value for an interrupt in the specified group.
-     * This depends on the BPR value:
+     * This depends on the BPR value. For CBPR0 (S or NS):
      *  a BPR of 0 means the group priority bits are [7:1];
      *  a BPR of 1 means they are [7:2], and so on down to
      *  a BPR of 7 meaning no group priority bits at all.
+     * For CBPR1 NS:
+     *  a BPR of 0 is impossible (the minimum value is 1)
+     *  a BPR of 1 means the group priority bits are [7:1];
+     *  a BPR of 2 means they are [7:2], and so on down to
+     *  a BPR of 6 meaning no group priority bits at all.
+     *
      * Which BPR to use depends on the group of the interrupt and
      * the current ICC_CTLR.CBPR settings.
+     *
+     * This corresponds to the GroupBits() pseudocode.
      */
+    int bpr;
+
     if ((group == GICV3_G1 && cs->icc_ctlr_el1[GICV3_S] & ICC_CTLR_EL1_CBPR) ||
         (group == GICV3_G1NS &&
          cs->icc_ctlr_el1[GICV3_NS] & ICC_CTLR_EL1_CBPR)) {
         group = GICV3_G0;
     }
 
-    return ~0U << ((cs->icc_bpr[group] & 7) + 1);
+    bpr = cs->icc_bpr[group] & 7;
+
+    if (group == GICV3_G1NS) {
+        assert(bpr > 0);
+        bpr--;
+    }
+
+    return ~0U << (bpr + 1);
 }
 
 static bool icc_no_enabled_hppi(GICv3CPUState *cs)
-- 
2.7.4

Re: [Qemu-devel] [Qemu-arm] [PATCH 3/3] hw/intc/arm_gicv3_cpuif: Fix priority masking for NS BPR1

[Philippe Mathieu-Daudé]

Hi Peter,

On 04/26/2017 02:13 PM, Peter Maydell wrote:
> When we calculate the mask to use to get the group priority from
> an interrupt priority, the way that NS BPR1 is handled differs
> from how BPR0 and S BPR1 work -- a BPR1 value of 1 means
> the group priority is in bits [7:1], whereas for BPR0 and S BPR1
> this is indicated by a 0 BPR value.
>
> Subtract 1 from the BPR value before creating the mask if
> we're using the NS BPR value, for both hardware and virtual
> interrupts, as the GICv3 pseudocode does, and fix the comments
> accordingly.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/intc/arm_gicv3_cpuif.c | 41 +++++++++++++++++++++++++++++++++++++----
>  1 file changed, 37 insertions(+), 4 deletions(-)
>
> diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
> index e660b3f..eec53c8 100644
> --- a/hw/intc/arm_gicv3_cpuif.c
> +++ b/hw/intc/arm_gicv3_cpuif.c
> @@ -216,18 +216,34 @@ static uint32_t icv_gprio_mask(GICv3CPUState *cs, int group)
>  {
>      /* Return a mask word which clears the subpriority bits from
>       * a priority value for a virtual interrupt in the specified group.
> -     * This depends on the VBPR value:
> +     * This depends on the VBPR value. If using VBPR0 then:

just aesthetic but I'd move "If using VBPR0 then:" to a new line.

>       *  a BPR of 0 means the group priority bits are [7:1];
>       *  a BPR of 1 means they are [7:2], and so on down to
>       *  a BPR of 7 meaning no group priority bits at all.
> +     * If using VBPR1 then:
> +     *  a BPR of 0 is impossible (the minimum value is 1)
> +     *  a BPR of 1 means the group priority bits are [7:1];
> +     *  a BPR of 2 means they are [7:2], and so on down to
> +     *  a BPR of 6 meaning no group priority bits at all.

This last line seems wrong to me, probably due to copying VBPR0.
I think this should be:

         *  a BPR of 7 meaning the group priority is [7].

> +     *
>       * Which BPR to use depends on the group of the interrupt and
>       * the current ICH_VMCR_EL2.VCBPR settings.
> +     *
> +     * This corresponds to the VGroupBits() pseudocode.

Very welcome comment!

>       */
> +    int bpr;
> +
>      if (group == GICV3_G1NS && cs->ich_vmcr_el2 & ICH_VMCR_EL2_VCBPR) {
>          group = GICV3_G0;
>      }
>
> -    return ~0U << (read_vbpr(cs, group) + 1);
> +    bpr = read_vbpr(cs, group);
> +    if (group == GICV3_G1NS) {
> +        assert(bpr > 0);

Shouldn't happen but we never know!

> +        bpr--;

Yes.

> +    }
> +
> +    return ~0U << (bpr + 1);
>  }
>
>  static bool icv_hppi_can_preempt(GICv3CPUState *cs, uint64_t lr)
> @@ -674,20 +690,37 @@ static uint32_t icc_gprio_mask(GICv3CPUState *cs, int group)
>  {
>      /* Return a mask word which clears the subpriority bits from
>       * a priority value for an interrupt in the specified group.
> -     * This depends on the BPR value:
> +     * This depends on the BPR value. For CBPR0 (S or NS):
>       *  a BPR of 0 means the group priority bits are [7:1];
>       *  a BPR of 1 means they are [7:2], and so on down to
>       *  a BPR of 7 meaning no group priority bits at all.
> +     * For CBPR1 NS:
> +     *  a BPR of 0 is impossible (the minimum value is 1)
> +     *  a BPR of 1 means the group priority bits are [7:1];
> +     *  a BPR of 2 means they are [7:2], and so on down to
> +     *  a BPR of 6 meaning no group priority bits at all.

Same here:

         "*  a BPR of 7 meaning the group priority is [7]."

If you agree with upgrading those comments:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> +     *
>       * Which BPR to use depends on the group of the interrupt and
>       * the current ICC_CTLR.CBPR settings.
> +     *
> +     * This corresponds to the GroupBits() pseudocode.
>       */
> +    int bpr;
> +
>      if ((group == GICV3_G1 && cs->icc_ctlr_el1[GICV3_S] & ICC_CTLR_EL1_CBPR) ||
>          (group == GICV3_G1NS &&
>           cs->icc_ctlr_el1[GICV3_NS] & ICC_CTLR_EL1_CBPR)) {
>          group = GICV3_G0;
>      }
>
> -    return ~0U << ((cs->icc_bpr[group] & 7) + 1);
> +    bpr = cs->icc_bpr[group] & 7;
> +
> +    if (group == GICV3_G1NS) {
> +        assert(bpr > 0);
> +        bpr--;
> +    }
> +
> +    return ~0U << (bpr + 1);
>  }
>
>  static bool icc_no_enabled_hppi(GICv3CPUState *cs)
>

Regards,

Phil.

Re: [Qemu-devel] [Qemu-arm] [PATCH 3/3] hw/intc/arm_gicv3_cpuif: Fix priority masking for NS BPR1

[Peter Maydell]

On 14 May 2017 at 06:21, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> If you agree with upgrading those comments:
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Yes, I agree with all those comment changes. Since they
were the only changes I've put the updated patchset
straight into target-arm.next.

Thanks for the careful review!

-- PMM