| Message ID | 20231114165404.681826-1-ben.dooks@codethink.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/intc/arm_gicv3: ICC_PMR_EL1 high bits should be RAZ | expand |
On 14/11/2023 17:14, Peter Maydell wrote: > On Tue, 14 Nov 2023 at 16:54, Ben Dooks <ben.dooks@codethink.co.uk> wrote: >> >> The ICC_PMR_ELx bit msak returned from icc_fullprio_mask >> should technically also remove any bit above 7 as these >> are marked reserved (read 0) and should therefore should >> not be written as anything other than 0. >> >> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> >> --- >> 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 d07b13eb27..986044df79 100644 >> --- a/hw/intc/arm_gicv3_cpuif.c >> +++ b/hw/intc/arm_gicv3_cpuif.c >> @@ -803,7 +803,7 @@ static uint32_t icc_fullprio_mask(GICv3CPUState *cs) >> * with the group priority, whose mask depends on the value of BPR >> * for the interrupt group.) >> */ >> - return ~0U << (8 - cs->pribits); >> + return (~0U << (8 - cs->pribits)) & 0xff; >> } > > The upper bits of ICC_PMR_ELx are defined as RES0, which has a > complicated technical definition which you can find in the GIC > architecture specification glossary. It's valid for RES0 bits to > be implemented as reads-as-written, which is the way our current > implementation works. Valid guest code should never be writing > any non-zero value into those bits. Yeah, got some proprietary test code that is trying write-1 and then assuming read-0. > What problem are you running into that you're trying to fix > with this patch? If our implementation misbehaves as a result > of letting these high bits through into cs->icc_pmr_el1 that > would be a good reason for making the change. See above, local test code issue. > If we do want to change this, for consistency we'd want > to change icv_fullprio_mask() too. If this isn't useful then I'll keep it as a local patch for now.
On Tue, 14 Nov 2023 at 17:23, Ben Dooks <ben.dooks@codethink.co.uk> wrote: > > On 14/11/2023 17:14, Peter Maydell wrote: > > On Tue, 14 Nov 2023 at 16:54, Ben Dooks <ben.dooks@codethink.co.uk> wrote: > >> > >> The ICC_PMR_ELx bit msak returned from icc_fullprio_mask > >> should technically also remove any bit above 7 as these > >> are marked reserved (read 0) and should therefore should > >> not be written as anything other than 0. > >> > >> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> > >> --- > >> 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 d07b13eb27..986044df79 100644 > >> --- a/hw/intc/arm_gicv3_cpuif.c > >> +++ b/hw/intc/arm_gicv3_cpuif.c > >> @@ -803,7 +803,7 @@ static uint32_t icc_fullprio_mask(GICv3CPUState *cs) > >> * with the group priority, whose mask depends on the value of BPR > >> * for the interrupt group.) > >> */ > >> - return ~0U << (8 - cs->pribits); > >> + return (~0U << (8 - cs->pribits)) & 0xff; > >> } > > > > The upper bits of ICC_PMR_ELx are defined as RES0, which has a > > complicated technical definition which you can find in the GIC > > architecture specification glossary. It's valid for RES0 bits to > > be implemented as reads-as-written, which is the way our current > > implementation works. Valid guest code should never be writing > > any non-zero value into those bits. > > Yeah, got some proprietary test code that is trying write-1 and > then assuming read-0. > > > What problem are you running into that you're trying to fix > > with this patch? If our implementation misbehaves as a result > > of letting these high bits through into cs->icc_pmr_el1 that > > would be a good reason for making the change. > > See above, local test code issue. Ah, right. That is technically bogus, but it wouldn't be the first time test case code explored the UNPREDICTABLE and IMPDEF reaches of the architecture :-) Looking at our GIC code, we *will* do something wrong if the guest writes to these high bits, because later on we do checks like if (cs->hppi.prio >= cs->icc_pmr_el1) { that will do the wrong thing. So I think we do want to mask out these high bits (being easier than trying to ignore them later). But I think we should be consistent between the icc and icv code paths. So if you want to write a patch that either: * masks out the high bits in both icc_fullprio_mask() and icv_fullprio_mask() * clears out those high bits directly in icc_pmr_write() (whichever you think looks nicer) I'd be happy to take that upstream. (Currently icv_pmr_write() happens to be written in a way that it discards those high bits as part of the deposit64() into ich_vmcr_el2.) thanks -- PMM
diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c index d07b13eb27..986044df79 100644 --- a/hw/intc/arm_gicv3_cpuif.c +++ b/hw/intc/arm_gicv3_cpuif.c @@ -803,7 +803,7 @@ static uint32_t icc_fullprio_mask(GICv3CPUState *cs) * with the group priority, whose mask depends on the value of BPR * for the interrupt group.) */ - return ~0U << (8 - cs->pribits); + return (~0U << (8 - cs->pribits)) & 0xff; } static inline int icc_min_bpr(GICv3CPUState *cs)
The ICC_PMR_ELx bit msak returned from icc_fullprio_mask should technically also remove any bit above 7 as these are marked reserved (read 0) and should therefore should not be written as anything other than 0. Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> --- hw/intc/arm_gicv3_cpuif.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)