| Message ID | 20221114133257.1752176-1-jens.wiklander@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/intc/arm_gicv3: fix prio masking on pmr write | expand |
On Mon, 14 Nov 2022 at 13:33, Jens Wiklander <jens.wiklander@linaro.org> wrote: > > With commit 39f29e599355 ("hw/intc/arm_gicv3: Use correct number of > priority bits for the CPU") the number of priority bits was changed from > the maximum value 8 to typically 5. As a consequence a few of the lowest > bits in ICC_PMR_EL1 becomes RAZ/WI. However prior to this patch one of > these bits was still used since the supplied priority value is masked > before it's eventually right shifted with one bit. So the bit is not > lost as one might expect when the register is read again. > > The Linux kernel depends on lowest valid bit to be reset to zero, see > commit 33625282adaa ("irqchip/gic-v3: Probe for SCR_EL3 being clear > before resetting AP0Rn") for details. > > So fix this by masking the priority value after it may have been right > shifted by one bit. > > Fixes: 39f29e599355 ("hw/intc/arm_gicv3: Use correct number of priority bits for the CPU") > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> Thanks for the fix; applied to target-arm.next for 7.2. > I've only tested this patch on top of v7.1.0 since I couldn't get current > to run in my test setup. > > In case anyone wonders what I'm testing, it's a setup with Hafnium at > S-EL2, OP-TEE at S-EL1 and the Linux kernel at NS-EL1 (no NS-EL2 for > simplicity). Now is a good time to figure out what's not working with current QEMU, so that if it's a bug in QEMU we can fix it before the 7.2 release. Could you try a bisect of QEMU to see where it broke? Alternatively, if you have repro instructions and prebuilt image files I can have a look. thanks -- PMM
On Mon, Nov 14, 2022 at 3:43 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Mon, 14 Nov 2022 at 13:33, Jens Wiklander <jens.wiklander@linaro.org> wrote: > > > > With commit 39f29e599355 ("hw/intc/arm_gicv3: Use correct number of > > priority bits for the CPU") the number of priority bits was changed from > > the maximum value 8 to typically 5. As a consequence a few of the lowest > > bits in ICC_PMR_EL1 becomes RAZ/WI. However prior to this patch one of > > these bits was still used since the supplied priority value is masked > > before it's eventually right shifted with one bit. So the bit is not > > lost as one might expect when the register is read again. > > > > The Linux kernel depends on lowest valid bit to be reset to zero, see > > commit 33625282adaa ("irqchip/gic-v3: Probe for SCR_EL3 being clear > > before resetting AP0Rn") for details. > > > > So fix this by masking the priority value after it may have been right > > shifted by one bit. > > > > Fixes: 39f29e599355 ("hw/intc/arm_gicv3: Use correct number of priority bits for the CPU") > > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> > > Thanks for the fix; applied to target-arm.next for 7.2. Great, thanks. > > > I've only tested this patch on top of v7.1.0 since I couldn't get current > > to run in my test setup. > > > > In case anyone wonders what I'm testing, it's a setup with Hafnium at > > S-EL2, OP-TEE at S-EL1 and the Linux kernel at NS-EL1 (no NS-EL2 for > > simplicity). > > Now is a good time to figure out what's not working with current > QEMU, so that if it's a bug in QEMU we can fix it before the > 7.2 release. Could you try a bisect of QEMU to see where it broke? > Alternatively, if you have repro instructions and prebuilt image > files I can have a look. I've bisected and learned this: # No output at all, hangs in TF-A bl1: bad commit 48da29e485af ("target/arm: Add ptw_idx to S1Translate") # bl1 works again, but TF-A bl2 (at S-EL1 if I've understood it right) fails to load some binary: commit cead7fa4c060 ("target/arm: Two fixes for secure ptw") I'm using semihosting to load binaries and in this case it might be that QEMU refuses to load the binary into the memory pointed at by BL2. I can share the binaries (~50 meg) privately, to test with if needed. Or if you'd like instructions to build it I can provide that too. I don't mind doing further testing if that helps. Cheers, Jens
On Tue, 15 Nov 2022 at 13:41, Jens Wiklander <jens.wiklander@linaro.org> wrote: > > On Mon, Nov 14, 2022 at 3:43 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > > > On Mon, 14 Nov 2022 at 13:33, Jens Wiklander <jens.wiklander@linaro.org> wrote: > > > I've only tested this patch on top of v7.1.0 since I couldn't get current > > > to run in my test setup. > > > > > > In case anyone wonders what I'm testing, it's a setup with Hafnium at > > > S-EL2, OP-TEE at S-EL1 and the Linux kernel at NS-EL1 (no NS-EL2 for > > > simplicity). > > > > Now is a good time to figure out what's not working with current > > QEMU, so that if it's a bug in QEMU we can fix it before the > > 7.2 release. Could you try a bisect of QEMU to see where it broke? > > Alternatively, if you have repro instructions and prebuilt image > > files I can have a look. > > I've bisected and learned this: > # No output at all, hangs in TF-A bl1: > bad commit 48da29e485af ("target/arm: Add ptw_idx to S1Translate") > # bl1 works again, but TF-A bl2 (at S-EL1 if I've understood it right) > fails to load some binary: > commit cead7fa4c060 ("target/arm: Two fixes for secure ptw") OK, so it looks like maybe 48da29e485af had several bugs and we've fixed one but there's still one lurking. > I'm using semihosting to load binaries and in this case it might be > that QEMU refuses to load the binary into the memory pointed at by > BL2. Hmm, I wouldn't have expected that to change. > I can share the binaries (~50 meg) privately, to test with if needed. > Or if you'd like instructions to build it I can provide that too. Yes, if you can put the binaries somewhere I/Richard can get at them I'll have a look at this. thanks -- PMM
diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c index 8ca630e5ad1e..b17b29288c73 100644 --- a/hw/intc/arm_gicv3_cpuif.c +++ b/hw/intc/arm_gicv3_cpuif.c @@ -1016,8 +1016,6 @@ static void icc_pmr_write(CPUARMState *env, const ARMCPRegInfo *ri, trace_gicv3_icc_pmr_write(gicv3_redist_affid(cs), value); - value &= icc_fullprio_mask(cs); - if (arm_feature(env, ARM_FEATURE_EL3) && !arm_is_secure(env) && (env->cp15.scr_el3 & SCR_FIQ)) { /* NS access and Group 0 is inaccessible to NS: return the @@ -1029,6 +1027,7 @@ static void icc_pmr_write(CPUARMState *env, const ARMCPRegInfo *ri, } value = (value >> 1) | 0x80; } + value &= icc_fullprio_mask(cs); cs->icc_pmr_el1 = value; gicv3_cpuif_update(cs); }
With commit 39f29e599355 ("hw/intc/arm_gicv3: Use correct number of priority bits for the CPU") the number of priority bits was changed from the maximum value 8 to typically 5. As a consequence a few of the lowest bits in ICC_PMR_EL1 becomes RAZ/WI. However prior to this patch one of these bits was still used since the supplied priority value is masked before it's eventually right shifted with one bit. So the bit is not lost as one might expect when the register is read again. The Linux kernel depends on lowest valid bit to be reset to zero, see commit 33625282adaa ("irqchip/gic-v3: Probe for SCR_EL3 being clear before resetting AP0Rn") for details. So fix this by masking the priority value after it may have been right shifted by one bit. Fixes: 39f29e599355 ("hw/intc/arm_gicv3: Use correct number of priority bits for the CPU") Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> --- Hi, I've only tested this patch on top of v7.1.0 since I couldn't get current to run in my test setup. In case anyone wonders what I'm testing, it's a setup with Hafnium at S-EL2, OP-TEE at S-EL1 and the Linux kernel at NS-EL1 (no NS-EL2 for simplicity). Regards, Jens hw/intc/arm_gicv3_cpuif.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)