| Message ID | 20190520162809.2677-5-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hw/intc/arm_gicv3: Four simple bugfixes | expand |
Hi Peter, On 5/20/19 6:28 PM, Peter Maydell wrote: > The ICC_CTLR_EL3 register includes some bits which are aliases > of bits in the ICC_CTLR_EL1(S) and (NS) registers. QEMU chooses > to keep those bits in the cs->icc_ctlr_el1[] struct fields. > Unfortunately a missing '~' in the code to update the bits > in those fields meant that writing to ICC_CTLR_EL3 would corrupt > the ICC_CLTR_EL1 register values. How did you notice? Simply reviewing? > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > hw/intc/arm_gicv3_cpuif.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c > index 000bdbd6247..3b212d91c8f 100644 > --- a/hw/intc/arm_gicv3_cpuif.c > +++ b/hw/intc/arm_gicv3_cpuif.c > @@ -1856,7 +1856,7 @@ static void icc_ctlr_el3_write(CPUARMState *env, const ARMCPRegInfo *ri, > trace_gicv3_icc_ctlr_el3_write(gicv3_redist_affid(cs), value); > > /* *_EL1NS and *_EL1S bits are aliases into the ICC_CTLR_EL1 bits. */ > - cs->icc_ctlr_el1[GICV3_NS] &= (ICC_CTLR_EL1_CBPR | ICC_CTLR_EL1_EOIMODE); > + cs->icc_ctlr_el1[GICV3_NS] &= ~(ICC_CTLR_EL1_CBPR | ICC_CTLR_EL1_EOIMODE); > if (value & ICC_CTLR_EL3_EOIMODE_EL1NS) { > cs->icc_ctlr_el1[GICV3_NS] |= ICC_CTLR_EL1_EOIMODE; > } > @@ -1864,7 +1864,7 @@ static void icc_ctlr_el3_write(CPUARMState *env, const ARMCPRegInfo *ri, > cs->icc_ctlr_el1[GICV3_NS] |= ICC_CTLR_EL1_CBPR; > } > > - cs->icc_ctlr_el1[GICV3_S] &= (ICC_CTLR_EL1_CBPR | ICC_CTLR_EL1_EOIMODE); > + cs->icc_ctlr_el1[GICV3_S] &= ~(ICC_CTLR_EL1_CBPR | ICC_CTLR_EL1_EOIMODE); > if (value & ICC_CTLR_EL3_EOIMODE_EL1S) { > cs->icc_ctlr_el1[GICV3_S] |= ICC_CTLR_EL1_EOIMODE; > } >
diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c index 000bdbd6247..3b212d91c8f 100644 --- a/hw/intc/arm_gicv3_cpuif.c +++ b/hw/intc/arm_gicv3_cpuif.c @@ -1856,7 +1856,7 @@ static void icc_ctlr_el3_write(CPUARMState *env, const ARMCPRegInfo *ri, trace_gicv3_icc_ctlr_el3_write(gicv3_redist_affid(cs), value); /* *_EL1NS and *_EL1S bits are aliases into the ICC_CTLR_EL1 bits. */ - cs->icc_ctlr_el1[GICV3_NS] &= (ICC_CTLR_EL1_CBPR | ICC_CTLR_EL1_EOIMODE); + cs->icc_ctlr_el1[GICV3_NS] &= ~(ICC_CTLR_EL1_CBPR | ICC_CTLR_EL1_EOIMODE); if (value & ICC_CTLR_EL3_EOIMODE_EL1NS) { cs->icc_ctlr_el1[GICV3_NS] |= ICC_CTLR_EL1_EOIMODE; } @@ -1864,7 +1864,7 @@ static void icc_ctlr_el3_write(CPUARMState *env, const ARMCPRegInfo *ri, cs->icc_ctlr_el1[GICV3_NS] |= ICC_CTLR_EL1_CBPR; } - cs->icc_ctlr_el1[GICV3_S] &= (ICC_CTLR_EL1_CBPR | ICC_CTLR_EL1_EOIMODE); + cs->icc_ctlr_el1[GICV3_S] &= ~(ICC_CTLR_EL1_CBPR | ICC_CTLR_EL1_EOIMODE); if (value & ICC_CTLR_EL3_EOIMODE_EL1S) { cs->icc_ctlr_el1[GICV3_S] |= ICC_CTLR_EL1_EOIMODE; }
The ICC_CTLR_EL3 register includes some bits which are aliases of bits in the ICC_CTLR_EL1(S) and (NS) registers. QEMU chooses to keep those bits in the cs->icc_ctlr_el1[] struct fields. Unfortunately a missing '~' in the code to update the bits in those fields meant that writing to ICC_CTLR_EL3 would corrupt the ICC_CLTR_EL1 register values. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/intc/arm_gicv3_cpuif.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) -- 2.20.1