Re: [PATCH] hw/intc/arm_gicv3: ICC_PMR_EL1 high bits should be RAZ

[Peter Maydell <peter.maydell@linaro.org>]

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