From: Julien Thierry <julien.thierry@arm.com>
To: Daniel Thompson <daniel.thompson@linaro.org>
Cc: Mark Rutland <mark.rutland@arm.com>,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, joel@joelfernandes.org,
marc.zyngier@arm.com, christoffer.dall@arm.com,
james.morse@arm.com, catalin.marinas@arm.com,
will.deacon@arm.com, Oleg Nesterov <oleg@redhat.com>
Subject: Re: [PATCH v6 06/24] arm64: ptrace: Provide definitions for PMR values
Date: Fri, 30 Nov 2018 11:03:29 +0000 [thread overview]
Message-ID: <01c117c9-2236-6cb6-d555-09298ef5dfd2@arm.com> (raw)
In-Reply-To: <20181130103811.lushdja552xevghm@holly.lan>
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.
>
--
Julien Thierry
next prev parent reply other threads:[~2018-11-30 11:03 UTC|newest]
Thread overview: 62+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-11-12 11:56 [PATCH v6 00/24] arm64: provide pseudo NMI with GICv3 Julien Thierry
2018-11-12 11:56 ` [PATCH v6 01/24] arm64: Remove unused daif related functions/macros Julien Thierry
2018-11-29 16:26 ` Mark Rutland
2018-11-30 18:03 ` Catalin Marinas
2018-11-12 11:56 ` [PATCH v6 02/24] arm64: cpufeature: Set SYSREG_GIC_CPUIF as a boot system feature Julien Thierry
2018-11-12 18:00 ` Suzuki K Poulose
2018-11-29 16:27 ` Mark Rutland
2018-11-30 18:07 ` Catalin Marinas
2018-11-12 11:56 ` [PATCH v6 03/24] arm64: cpufeature: Add cpufeature for IRQ priority masking Julien Thierry
2018-11-12 18:02 ` Suzuki K Poulose
2018-11-29 17:12 ` Mark Rutland
2018-12-03 10:33 ` Julien Thierry
2018-11-30 18:07 ` Catalin Marinas
2018-11-12 11:56 ` [PATCH v6 04/24] arm/arm64: gic-v3: Add PMR and RPR accessors Julien Thierry
2018-11-29 16:32 ` Mark Rutland
2018-11-30 18:07 ` Catalin Marinas
2018-11-12 11:56 ` [PATCH v6 05/24] irqchip/gic-v3: Switch to PMR masking before calling IRQ handler Julien Thierry
2018-11-29 18:12 ` Mark Rutland
2018-11-30 9:18 ` Julien Thierry
2018-12-04 16:21 ` Catalin Marinas
2018-11-12 11:56 ` [PATCH v6 06/24] arm64: ptrace: Provide definitions for PMR values Julien Thierry
2018-11-29 16:40 ` Mark Rutland
2018-11-30 8:53 ` Julien Thierry
2018-11-30 10:38 ` Daniel Thompson
2018-11-30 11:03 ` Julien Thierry [this message]
2018-11-12 11:56 ` [PATCH v6 07/24] arm64: Make PMR part of task context Julien Thierry
2018-11-29 16:46 ` Mark Rutland
2018-11-30 9:25 ` Julien Thierry
2018-12-04 17:09 ` Catalin Marinas
2018-12-04 17:30 ` Julien Thierry
2018-11-12 11:56 ` [PATCH v6 08/24] arm64: Unmask PMR before going idle Julien Thierry
2018-11-29 17:44 ` Mark Rutland
2018-11-30 10:55 ` Julien Thierry
2018-11-30 13:37 ` Mark Rutland
2018-12-03 10:38 ` Julien Thierry
2018-11-12 11:57 ` [PATCH v6 09/24] arm64: kvm: Unmask PMR before entering guest Julien Thierry
2018-11-12 11:57 ` [PATCH v6 10/24] arm64: irqflags: Use ICC_PMR_EL1 for interrupt masking Julien Thierry
2018-12-04 17:36 ` Catalin Marinas
2018-12-05 16:55 ` Julien Thierry
2018-12-05 18:26 ` Catalin Marinas
2018-12-06 9:50 ` Julien Thierry
2018-12-10 14:39 ` Catalin Marinas
2018-11-12 11:57 ` [PATCH v6 11/24] arm64: daifflags: Include PMR in daifflags restore operations Julien Thierry
2018-11-12 11:57 ` [PATCH v6 12/24] arm64: alternative: Allow alternative status checking per cpufeature Julien Thierry
2018-11-12 11:57 ` [PATCH v6 13/24] arm64: alternative: Apply alternatives early in boot process Julien Thierry
2018-11-12 11:57 ` [PATCH v6 14/24] irqchip/gic-v3: Factor group0 detection into functions Julien Thierry
2018-11-12 11:57 ` [PATCH v6 15/24] arm64: Switch to PMR masking when starting CPUs Julien Thierry
2018-12-04 17:51 ` Catalin Marinas
2018-12-04 18:11 ` Julien Thierry
2018-11-12 11:57 ` [PATCH v6 16/24] arm64: gic-v3: Implement arch support for priority masking Julien Thierry
2018-11-12 11:57 ` [PATCH v6 17/24] irqchip/gic-v3: Detect if GIC can support pseudo-NMIs Julien Thierry
2018-11-12 11:57 ` [PATCH v6 18/24] irqchip/gic-v3: Handle pseudo-NMIs Julien Thierry
2018-11-12 11:57 ` [PATCH v6 19/24] irqchip/gic: Add functions to access irq priorities Julien Thierry
2018-11-12 11:57 ` [PATCH v6 20/24] irqchip/gic-v3: Allow interrupts to be set as pseudo-NMI Julien Thierry
2018-11-12 11:57 ` [PATCH v6 21/24] arm64: Handle serror in NMI context Julien Thierry
2018-12-04 18:09 ` Catalin Marinas
2018-12-05 13:02 ` James Morse
2018-11-12 11:57 ` [PATCH v6 22/24] arm64: Skip preemption when exiting an NMI Julien Thierry
2018-11-12 11:57 ` [PATCH v6 23/24] arm64: Skip irqflags tracing for NMI in IRQs disabled context Julien Thierry
2018-11-12 11:57 ` [PATCH v6 24/24] arm64: Enable the support of pseudo-NMIs Julien Thierry
2018-11-12 12:00 ` [PATCH v6 00/24] arm64: provide pseudo NMI with GICv3 Julien Thierry
2018-11-13 14:43 ` Julien Thierry
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=01c117c9-2236-6cb6-d555-09298ef5dfd2@arm.com \
--to=julien.thierry@arm.com \
--cc=catalin.marinas@arm.com \
--cc=christoffer.dall@arm.com \
--cc=daniel.thompson@linaro.org \
--cc=james.morse@arm.com \
--cc=joel@joelfernandes.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=marc.zyngier@arm.com \
--cc=mark.rutland@arm.com \
--cc=oleg@redhat.com \
--cc=will.deacon@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).