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

[Daniel Thompson]

On Fri, Nov 30, 2018 at 08:53:47AM +0000, Julien Thierry wrote:
> 
> 
> 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?

I think selecting bit 6 dates back to when I was working on this.

I originally used bit 7 but switched due to problems on the FVP at the
time (my memory is a little hazy here but it felt like it wasn't
doing the magic shift properly when running in non-secure mode).

Once the patchset was running on real hardware I kept on with bit 6 
figuring that, given the magic shift from non-secure mode is a little
odd, it would remain furtile soil for future silicon bugs (I was
watching a lot of patches go past on the ML working round bugs in
non-Arm GIC implementations and ended up feeling rather paranoid
about things like that).


Daniel.