| Message ID | 1542023835-21446-7-git-send-email-julien.thierry@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: provide pseudo NMI with GICv3 | expand |
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? > Signed-off-by: Julien Thierry <julien.thierry@arm.com> > Suggested-by: Daniel Thompson <daniel.thompson@linaro.org> > Cc: Oleg Nesterov <oleg@redhat.com> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will.deacon@arm.com> > --- > 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? Are lower numbers higher priority? Are there restrictions on valid PMR values? 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. Thanks, Mark.
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@arm.com> >> Suggested-by: Daniel Thompson <daniel.thompson@linaro.org> >> Cc: Oleg Nesterov <oleg@redhat.com> >> Cc: Catalin Marinas <catalin.marinas@arm.com> >> Cc: Will Deacon <will.deacon@arm.com> >> --- >> 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,
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@arm.com> > >> Suggested-by: Daniel Thompson <daniel.thompson@linaro.org> > >> Cc: Oleg Nesterov <oleg@redhat.com> > >> Cc: Catalin Marinas <catalin.marinas@arm.com> > >> Cc: Will Deacon <will.deacon@arm.com> > >> --- > >> 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.
On 30/11/18 10:38, Daniel Thompson wrote: > 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@arm.com> >>>> Suggested-by: Daniel Thompson <daniel.thompson@linaro.org> >>>> Cc: Oleg Nesterov <oleg@redhat.com> >>>> Cc: Catalin Marinas <catalin.marinas@arm.com> >>>> Cc: Will Deacon <will.deacon@arm.com> >>>> --- >>>> 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). > If you were using boot-wrapper, that might have been the case as SCR_EL3.FIQ is not getting set. The fun bit is that under this configuration the magic bit still happens for non-secure accesses to priorities configured in the distributor/redistributor, but it disables the magic for non-secure PMR and RPR accesses. So you can easily end up masking too much stuff when writting to PMR when SCR_EL1.FIQ is cleared if you don't realize that what non-secure sees in the distributor is not aligned with what will be masked by PMR or presented in RPR. > 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. >
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) + /* Additional SPSR bits not exposed in the UABI */ #define PSR_IL_BIT (1 << 20)
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 that a single bit (GIC_PMR_UNMASKED_BIT) encodes the information whether interrupts are masked or not. Signed-off-by: Julien Thierry <julien.thierry@arm.com> Suggested-by: Daniel Thompson <daniel.thompson@linaro.org> Cc: Oleg Nesterov <oleg@redhat.com> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Will Deacon <will.deacon@arm.com> --- arch/arm64/include/asm/ptrace.h | 6 ++++++ 1 file changed, 6 insertions(+)