| Message ID | 1493226792-3237-3-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | gicv3: Correct mishandling of NS BPR1 values | expand |
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); > } >
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
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); }
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(+) -- 2.7.4