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;
 }
 
 static inline int icc_min_bpr(GICv3CPUState *cs)
-- 
2.37.2.352.g3c44437643

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.

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.

If we do want to change this, for consistency we'd want
to change icv_fullprio_mask() too.

thanks
-- PMM

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.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html

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