[src/trunk]: src/sys/arch/arm/cortex Fix interrupt priorities on N1 SDP.

[jmcneill <jmcneill%NetBSD.org@localhost>]

details:   https://anonhg.NetBSD.org/src/rev/003cc1d4a4b0
branches:  trunk
changeset: 946295:003cc1d4a4b0
user:      jmcneill <jmcneill%NetBSD.org@localhost>
date:      Sun Nov 22 20:17:39 2020 +0000

description:
Fix interrupt priorities on N1 SDP.

The GICv3 architecture specification is not clear on the NS view of
priority registers, and there doesn't seem to be any consistency in how
these are implemented in both real and emulated environments.

The previous fix for this issue was meant to detect what we thought at the
time was a bug on the Rockchip RK3399. At that time the theory was somehow
EL1 has a secure view of the hardware, and this is causing us to have the
wrong view of IPRIORITYRn based on IHI0069F section 4.8.6 "Software
accesses of interrupt priority". But it turns out that this is not the
full picture. While I was able to confirm that yes, we do have secure
access to the GIC on RK3399 from EL1, the view of IPRIORITYRn differs
depending on whether you are using the Rockchip TF-A as included with
https://github.com/ayufan-rock64/linux-u-boot (shifted view), or mainline
TF-A from pkgsrc (unshifted view).

So to detect this quirk, we need three things: A method to detect if we
have S access to GIC registers, a method to see how many PMR bits are
implemented, and a method to see how many IPRIORITYRn bits are implemented.

To detect S access, we can try to toggle GICD_CTRL.EnableGrp1S. This bit
is either RES0 (security extensions not implemented), RAZ/WI (non-secure
access in two security state systems) or RW (secure access in two security
state systems).

To read the number of PMR and IPRIORITYRn bits supported, we can write all
1s to the register fields and read them back.

For the RK3399 (Rockchip TF-A) quirk, we assume a shifted view of
IPRIORITYRn if we have detected S accesses, and the PMR and IPRIORITYRn
values differ. The S access test is required because some real hardware
implementations (Ampere eMAG) were observed to report different PMR and
IPRIORITYRn masks, but present an unshifted view of IPRIORITYRn.

During testing, I also discovered that QEMU 5.1 requires this shifted view
workaround as well -- as far as I can tell, this is a QEMU bug. We can't
detect it the same way as RK3399 because security is disabled in the
emulated GIC, and the PMR and IPRIORITYRn tests both return 0xff! So now
if the GICv3 driver sees this configuration, it assumes that the shifted
view is required.

Honestly, this feature is so poorly documented that maybe it is better to
give up on HW priorities and preemption and use a single flat model like
Linux and FreeBSD does.

Tested on Arm N1 SDP, ROCKpro64 (RK3399) with Rockchip and pkgsrc TF-A,
Pinebook Pro (RK3399), Lenovo HR330A (Ampere eMAG), QEMU 5.1 (gic-version=3),
AWS EC2 a1.medium (Graviton).

diffstat:

 sys/arch/arm/cortex/gicv3.c |  72 ++++++++++++++++++++++++++++++++------------
 1 files changed, 52 insertions(+), 20 deletions(-)

diffs (124 lines):

diff -r a7bb4478385d -r 003cc1d4a4b0 sys/arch/arm/cortex/gicv3.c
--- a/sys/arch/arm/cortex/gicv3.c       Sun Nov 22 20:01:46 2020 +0000
+++ b/sys/arch/arm/cortex/gicv3.c       Sun Nov 22 20:17:39 2020 +0000
@@ -1,4 +1,4 @@
-/* $NetBSD: gicv3.c,v 1.33 2020/11/21 11:44:00 jmcneill Exp $ */
+/* $NetBSD: gicv3.c,v 1.34 2020/11/22 20:17:39 jmcneill Exp $ */
 
 /*-
  * Copyright (c) 2018 Jared McNeill <jmcneill%invisible.ca@localhost>
@@ -31,7 +31,7 @@
 #define        _INTR_PRIVATE
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: gicv3.c,v 1.33 2020/11/21 11:44:00 jmcneill Exp $");
+__KERNEL_RCSID(0, "$NetBSD: gicv3.c,v 1.34 2020/11/22 20:17:39 jmcneill Exp $");
 
 #include <sys/param.h>
 #include <sys/kernel.h>
@@ -755,26 +755,37 @@
        pic_do_pending_ints(I32_bit, oldipl, frame);
 }
 
-static int
-gicv3_detect_pmr_bits(struct gicv3_softc *sc)
+static bool
+gicv3_access_is_secure(struct gicv3_softc *sc)
+{
+       const uint32_t octlr = gicd_read_4(sc, GICD_CTRL);
+       gicd_write_4(sc, GICD_CTRL, octlr ^ GICD_CTRL_EnableGrp1S);
+       const uint32_t nctlr = gicd_read_4(sc, GICD_CTRL);
+       gicd_write_4(sc, GICD_CTRL, octlr);
+
+       return nctlr != octlr;
+}
+
+static uint8_t
+gicv3_get_pmr_bits(struct gicv3_softc *sc)
 {
        const uint32_t opmr = icc_pmr_read();
-       icc_pmr_write(0xbf);
+       icc_pmr_write(0xff);
        const uint32_t npmr = icc_pmr_read();
        icc_pmr_write(opmr);
 
-       return NBBY - (ffs(npmr) - 1);
+       return npmr;
 }
 
-static int
-gicv3_detect_ipriority_bits(struct gicv3_softc *sc)
+static uint8_t
+gicv3_get_ipriority_bits(struct gicv3_softc *sc)
 {
        const uint32_t oipriorityr = gicd_read_4(sc, GICD_IPRIORITYRn(8));
        gicd_write_4(sc, GICD_IPRIORITYRn(8), oipriorityr | 0xff);
        const uint32_t nipriorityr = gicd_read_4(sc, GICD_IPRIORITYRn(8));
        gicd_write_4(sc, GICD_IPRIORITYRn(8), oipriorityr);
 
-       return NBBY - (ffs(nipriorityr & 0xff) - 1);
+       return nipriorityr & 0xff;
 }
 
 int
@@ -782,6 +793,7 @@
 {
        const uint32_t gicd_typer = gicd_read_4(sc, GICD_TYPER);
        const uint32_t gicd_ctrl = gicd_read_4(sc, GICD_CTRL);
+       const bool access_s = gicv3_access_is_secure(sc);
        int n;
 
        KASSERT(CPU_IS_PRIMARY(curcpu()));
@@ -794,21 +806,41 @@
        sc->sc_priority_shift = 4;
        sc->sc_pmr_shift = 4;
 
+       uint8_t pmr_mask = gicv3_get_pmr_bits(sc);
+       uint8_t ipriority_mask = gicv3_get_ipriority_bits(sc);
+
        if ((gicd_ctrl & GICD_CTRL_DS) == 0) {
-               const int pmr_bits = gicv3_detect_pmr_bits(sc);
-               const int ipriority_bits = gicv3_detect_ipriority_bits(sc);
-
-               if (ipriority_bits != pmr_bits)
+               if (access_s) {
+                       /*
+                        * Security is enabled and we appear to have secure access
+                        * to GIC registers. This is not normal, but has been observed
+                        * on Rockchip RK3399. To make things worse, with Rockchip
+                        * TF-A the views of PMR and IPRIORITYRn are different, so we
+                        * need to compensate for this by shifting writes to
+                        * IPRIORITYRn right one. Mainline TF-A does not seem to have
+                        * this problem, so detect it by comparing the number of
+                        * writable bits in the PMR and IPRIORITYRn registers.
+                        */
+                       if (pmr_mask < ipriority_mask) {
+                               --sc->sc_priority_shift;
+                       }
+               }
+       } else {
+               /*
+                * Security is disabled. QEMU 5.1 reports different views of
+                * PMR and IPRIORIRYRn here.
+                */
+               if (pmr_mask == 0xff && ipriority_mask == 0xff) {
                        --sc->sc_priority_shift;
-
-               aprint_verbose_dev(sc->sc_dev, "%d pmr bits, %d ipriority bits\n",
-                   pmr_bits, ipriority_bits);
-       } else {
-               aprint_verbose_dev(sc->sc_dev, "security disabled\n");
+               }
        }
 
-       aprint_verbose_dev(sc->sc_dev, "priority shift %d, pmr shift %d\n",
-           sc->sc_priority_shift, sc->sc_pmr_shift);
+       aprint_verbose_dev(sc->sc_dev,
+           "security %sabled%s, priority shift %d (0x%02x), pmr shift %d (0x%02x)\n",
+           (gicd_ctrl & GICD_CTRL_DS) != 0 ? "dis" : "en",
+           access_s ? ", secure access" : "",
+           sc->sc_priority_shift, ipriority_mask,
+           sc->sc_pmr_shift, pmr_mask);
 
        sc->sc_pic.pic_ops = &gicv3_picops;
        sc->sc_pic.pic_maxsources = GICD_TYPER_LINES(gicd_typer);