Re: [PATCH v6 06/24] arm64: ptrace: Provide definitions for PMR values

[Julien Thierry]

On 29/11/18 16:40, Mark Rutland wrote:
> On Mon, Nov 12, 2018 at 11:56:57AM +0000, Julien Thierry wrote:
>> Introduce fixed values for PMR that are going to be used to mask and
>> unmask interrupts by priority. These values are chosent in such a way
> 
> Nit: s/chosent/chosen/
> 
>> that a single bit (GIC_PMR_UNMASKED_BIT) encodes the information whether
>> interrupts are masked or not.
> 
> There's no GIC_PMR_UNMASKED_BIT in this patch. Should that be
> GIC_PRIO_STATUS_BIT?
> 

Yep, forgot to update the commit message when renaming, thanks.

>> Signed-off-by: Julien Thierry <julien.thierry@xxxxxxx>
>> Suggested-by: Daniel Thompson <daniel.thompson@xxxxxxxxxx>
>> Cc: Oleg Nesterov <oleg@xxxxxxxxxx>
>> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
>> Cc: Will Deacon <will.deacon@xxxxxxx>
>> ---
>>  arch/arm64/include/asm/ptrace.h | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
>> index fce22c4..ce6998c 100644
>> --- a/arch/arm64/include/asm/ptrace.h
>> +++ b/arch/arm64/include/asm/ptrace.h
>> @@ -25,6 +25,12 @@
>>  #define CurrentEL_EL1		(1 << 2)
>>  #define CurrentEL_EL2		(2 << 2)
>>  
>> +/* PMR values used to mask/unmask interrupts */
>> +#define GIC_PRIO_IRQON		0xf0
>> +#define GIC_PRIO_STATUS_SHIFT	6
>> +#define GIC_PRIO_STATUS_BIT	(1 << GIC_PRIO_STATUS_SHIFT)
>> +#define GIC_PRIO_IRQOFF		(GIC_PRIO_IRQON ^ GIC_PRIO_STATUS_BIT)
> 
> Could you elaborate on the GIC priority logic a bit?
> 

Yes, I'll give details below.

> Are lower numbers higher priority?
> 

Yes, that is the case.

> Are there restrictions on valid PMR values?
> 

Yes, there are at most 8 priority bits but implementations are free to
implement a number of priority bits:
- between 5 and 8 when GIC runs two security states (bits [7:3] always
being implemented and [2:0] being optional), but non-secure side is
always deprived or the lowest implemented bit
- between 4 and 8 when GIC runs only one security state (bits [7:4]
implemented, bits [3:0] optional)

This is detailed in section 4.8 "Interrupt prioritization" of the GICv3
architecture specification.

So Linux should always be able to see bits [7:4].

> IIUC GIC_PRIO_IRQOFF is 0xb0 (aka 0b10110000), which seems a little
> surprising. I'd have expected that we'd use the most signficant bit.
> 

So, re-reading the GICv3 spec, I believe this sparked from a confusion...

The idea was that the GICv3 specification would recommend to keep
non-secure group-1 interrupts at a lower priority that group-0 (and
secure group-1 interrupts) interrupts, and to do so the idea was to
always keep bit[7] == 1 for non-secure group-1.

So, we would need to have priority bit[7] == 1 for both normal
interrupts and pseudo-NMIs, and using the most significant bit to mask
would mean masking pseudo-NMIs as well.

However, I only find mention of this in the notes of section 4.8.6
"Software accesses of interrupt priority". The section only applies to
GIC with two security states, and the recommendation of writing
non-secure group-1 priorities with bit[7] == 1 is only directed at
writes from the secure side. From the non-secure side, the GIC already
does some magic to enforce that the value kept in the distributor has
bit[7] == 1.

So, I believe that from the non-secure point of view, we could define
pseudo-NMI priority as e.g. 0x40 (which the GIC will convert to 0xa0)
and use the most significant bit of PMR to mask normal interrupts which
would be more intuitive.

Marc, as GIC expert do you agree with this? Or is there a reason we
should keep bit[7] == 1 for non-secure group-1 priorities?

Thanks,

-- 
Julien Thierry