Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 20 Aug 2021 13:58:56 +0100
Message-ID: <87pmu8qnlb.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Alexandru Elisei <alexandru.elisei@arm.com>
Cc:     Chen-Yu Tsai <wenst@chromium.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] irqchip/gic-v3: Fix priority comparison when non-secure priorities are used
In-Reply-To: <061bafb6-b0fe-bd7c-1567-a642d3ddd4a5@arm.com>
References: <20210811171505.1502090-1-wenst@chromium.org>
        <87fsvfal4n.wl-maz@kernel.org>
        <79eabae1-e4a3-7a12-7aa0-3680569584e5@arm.com>
        <871r6yajy7.wl-maz@kernel.org>
        <061bafb6-b0fe-bd7c-1567-a642d3ddd4a5@arm.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1
 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

Hi Alex,

On Thu, 12 Aug 2021 15:24:03 +0100,
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> 
> Hi Marc,
> 
> On 8/12/21 2:09 PM, Marc Zyngier wrote:
> > On Thu, 12 Aug 2021 12:51:34 +0100,
> > Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> >> Hi,
> >>
> >> After re-familiarizing myself with the spec, it starting to look to
> >> me like indeed there's something not quite right (read as: totally
> >> broken) with my patch.
> >>
> >> Arm IHI 0069F, the pseudocode for reading ICC_RPR_EL1 (page 11-797),
> >> says that the priority returned is unchanged if SCTLR_EL3.FIQ ==
> >> 0.
> > Sure, but look at what ICC_RPR_EL1 does for FIQ==1:
> >
> > <quote>
> > if HaveEL(EL3) && !IsSecure() && SCR_EL3.FIQ == '1' then
> >     // A Non-secure GIC access and Group 0 inaccessible to Non-secure.
> >         if pPriority<7> == '0' then
> > 	    // Priority is in Secure half and not visible to Non-secure
> > 	    Priority = Zeros();
> >         elsif !IsOnes(pPriority) then
> > 	    // Non-secure access and not idle, so physical priority must be shifted
> >             pPriority<7:0> = (pPriority AND PRIMask())<6:0>:'0';
> >
> > return ZeroExtend(pPriority);
> > </quote>
> >
> > See how the the priority is shifted *left* (bits [6:0] end up in
> > [7:1])?
> 
> Yes, when SCR_EL3.FIQ=1, but gic_nonsecure_priorities is enabled
> when SCR_EL3.FIQ=0 (gic_has_group0()). In that case ICC_RPR_EL1
> returns (what I assume to be) the highest priority interrupt from
> ICC_AP0R_EL1, ICC_AP1R_EL1NS and ICC_AP1R_EL1S. Isn't that the
> secure view (or Distributor value) of the priority?

Yup. I guess I got confused with what "non-secure" priorities mean in
this context.

[...]

> I don't see how that is the case - ICC_RPR_EL1 contains the priority
> value as seen by the Distributor, and non-secure priorities get
> right-shifted when SCR_EL3.FIQ=0, meaning that GICD_INT_NMI_PRI
> becomes (GICD_INT_NMI_PRI >> 1) | 0x80 in the Distributor. Can you
> elaborate where I'm contradicting myself?

I think I know why I confused myself. When FIQ==0, G0 is NS. On the
face of it, this should mean that no shift occurs. However, G1S is
still in the picture, and we get the extra shift to preserve the
ordering with G1S.

This is a different configuration from that of a guest, where G0 is
also NS, but there is no shift at all, as there is no G1S.

The GIC strikes back. Again.

I run some more tests with this patch, and merge it of nothing breaks.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.