KVM: arm64: Generalise RESx handling

Convert SCTLR_EL2 to the sysreg infrastructure, as per the 2025-12_rel revision of the Registers.json file.

Note that we slightly deviate from the above, as we stick to the ARM ARM M.a definition of SCTLR_EL2[9], which is RES0, in order to avoid dragging the POE2 definitions...

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/include/asm/sysreg.h       |  7 ---
 arch/arm64/tools/sysreg               | 69 +++++++++++++++++++++++++++
 tools/arch/arm64/include/asm/sysreg.h |  6 ---
 3 files changed, 69 insertions(+), 13 deletions(-)

arch/arm64/include/asm/sysreg.h +0 -7

index 939f9c5bbae67..30f0409b1c802 100644
@@ -504,7 +504,6 @@
 #define SYS_VPIDR_EL2                  sys_reg(3, 4, 0, 0, 0)
 #define SYS_VMPIDR_EL2                 sys_reg(3, 4, 0, 0, 5)
 
-#define SYS_SCTLR_EL2                  sys_reg(3, 4, 1, 0, 0)
 #define SYS_ACTLR_EL2                  sys_reg(3, 4, 1, 0, 1)
 #define SYS_SCTLR2_EL2                 sys_reg(3, 4, 1, 0, 3)
 #define SYS_HCR_EL2                    sys_reg(3, 4, 1, 1, 0)

@@ -837,12 +836,6 @@
 #define SCTLR_ELx_A     (BIT(1))
 #define SCTLR_ELx_M     (BIT(0))
 
-/* SCTLR_EL2 specific flags. */
-#define SCTLR_EL2_RES1 ((BIT(4))  | (BIT(5))  | (BIT(11)) | (BIT(16)) | \
-                        (BIT(18)) | (BIT(22)) | (BIT(23)) | (BIT(28)) | \
-                        (BIT(29)))
-
-#define SCTLR_EL2_BT   (BIT(36))
 #ifdef CONFIG_CPU_BIG_ENDIAN
 #define ENDIAN_SET_EL2         SCTLR_ELx_EE
 #else

arch/arm64/tools/sysreg +69 -0

index a0f6249bd4f98..969a75615d612 100644
@@ -3749,6 +3749,75 @@ UnsignedEnum     2:0     F8S1
 EndEnum
 EndSysreg
 
+Sysreg SCTLR_EL2       3       4       1       0       0
+Field  63      TIDCP
+Field  62      SPINTMASK
+Field  61      NMI
+Field  60      EnTP2
+Field  59      TCSO
+Field  58      TCSO0
+Field  57      EPAN
+Field  56      EnALS
+Field  55      EnAS0
+Field  54      EnASR
+Res0   53:50
+Field  49:46   TWEDEL
+Field  45      TWEDEn
+Field  44      DSSBS
+Field  43      ATA
+Field  42      ATA0
+Enum   41:40   TCF
+       0b00    NONE
+       0b01    SYNC
+       0b10    ASYNC
+       0b11    ASYMM
+EndEnum
+Enum   39:38   TCF0
+       0b00    NONE
+       0b01    SYNC
+       0b10    ASYNC
+       0b11    ASYMM
+EndEnum
+Field  37      ITFSB
+Field  36      BT
+Field  35      BT0
+Field  34      EnFPM
+Field  33      MSCEn
+Field  32      CMOW
+Field  31      EnIA
+Field  30      EnIB
+Field  29      LSMAOE
+Field  28      nTLSMD
+Field  27      EnDA
+Field  26      UCI
+Field  25      EE
+Field  24      E0E
+Field  23      SPAN
+Field  22      EIS
+Field  21      IESB
+Field  20      TSCXT
+Field  19      WXN
+Field  18      nTWE
+Res0   17
+Field  16      nTWI
+Field  15      UCT
+Field  14      DZE
+Field  13      EnDB
+Field  12      I
+Field  11      EOS
+Field  10      EnRCTX
+Res0   9
+Field  8       SED
+Field  7       ITD
+Field  6       nAA
+Field  5       CP15BEN
+Field  4       SA0
+Field  3       SA
+Field  2       C
+Field  1       A
+Field  0       M
+EndSysreg
+
 Sysreg HCR_EL2         3       4       1       1       0
 Field  63:60   TWEDEL
 Field  59      TWEDEn

tools/arch/arm64/include/asm/sysreg.h +0 -7

index 178b7322bf049..f75efe98e9df3 100644
@@ -847,12 +847,6 @@
 #define SCTLR_ELx_A     (BIT(1))
 #define SCTLR_ELx_M     (BIT(0))
 
-/* SCTLR_EL2 specific flags. */
-#define SCTLR_EL2_RES1 ((BIT(4))  | (BIT(5))  | (BIT(11)) | (BIT(16)) | \
-                        (BIT(18)) | (BIT(22)) | (BIT(23)) | (BIT(28)) | \
-                        (BIT(29)))
-
-#define SCTLR_EL2_BT   (BIT(36))
 #ifdef CONFIG_CPU_BIG_ENDIAN
 #define ENDIAN_SET_EL2         SCTLR_ELx_EE
 #else
--

2.47.3

We already have specific constraints for SCTLR_EL1.{EE,E0E}, and making them depend on FEAT_AA64EL1 is just buggy.

Fixes 6bd4a274b026e ("KVM: arm64: Convert SCTLR_EL1 to config-driven sanitisation")

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/kvm/config.c | 2 --
 1 file changed, 2 deletions(-)

arch/arm64/kvm/config.c +0 -3

index 9c04f895d3769..0bcdb39885734 100644
@@ -1140,8 +1140,6 @@ static const struct reg_bits_to_feat_map sctlr_el1_feat_map[] = {
                   SCTLR_EL1_TWEDEn,
                   FEAT_TWED),
        NEEDS_FEAT(SCTLR_EL1_UCI        |
-                  SCTLR_EL1_EE         |
-                  SCTLR_EL1_E0E        |
                   SCTLR_EL1_WXN        |
                   SCTLR_EL1_nTWE       |
                   SCTLR_EL1_nTWI       |
--

2.47.3

Computing the FGU bits is made oddly complicated, as we use the RES0 helper instead of using a specific abstraction.

Introduce such an abstraction, which is going to make things significantly simpler in the future.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/kvm/config.c | 57 ++++++++++++++++++-----------------------
 1 file changed, 25 insertions(+), 32 deletions(-)

arch/arm64/kvm/config.c +25 -33

index 0bcdb39885734..2122599f7cbbd 100644
@@ -1335,26 +1335,30 @@ static u64 compute_res0_bits(struct kvm *kvm,
 static u64 compute_reg_res0_bits(struct kvm *kvm,
                                 const struct reg_feat_map_desc *r,
                                 unsigned long require, unsigned long exclude)
-
 {
        u64 res0;
 
        res0 = compute_res0_bits(kvm, r->bit_feat_map, r->bit_feat_map_sz,
                                 require, exclude);
 
-       /*
-        * If computing FGUs, don't take RES0 or register existence
-        * into account -- we're not computing bits for the register
-        * itself.
-        */
-       if (!(exclude & NEVER_FGU)) {
-               res0 |= compute_res0_bits(kvm, &r->feat_map, 1, require, exclude);
-               res0 |= ~reg_feat_map_bits(&r->feat_map);
-       }
+       res0 |= compute_res0_bits(kvm, &r->feat_map, 1, require, exclude);
+       res0 |= ~reg_feat_map_bits(&r->feat_map);
 
        return res0;
 }
 
+static u64 compute_fgu_bits(struct kvm *kvm, const struct reg_feat_map_desc *r)
+{
+       /*
+        * If computing FGUs, we collect the unsupported feature bits as
+        * RES0 bits, but don't take the actual RES0 bits or register
+        * existence into account -- we're not computing bits for the
+        * register itself.
+        */
+       return compute_res0_bits(kvm, r->bit_feat_map, r->bit_feat_map_sz,
+                                0, NEVER_FGU);
+}
+
 static u64 compute_reg_fixed_bits(struct kvm *kvm,
                                  const struct reg_feat_map_desc *r,
                                  u64 *fixed_bits, unsigned long require,

@@ -1370,40 +1374,29 @@ void compute_fgu(struct kvm *kvm, enum fgt_group_id fgt)
 
        switch (fgt) {
        case HFGRTR_GROUP:
-               val |= compute_reg_res0_bits(kvm, &hfgrtr_desc,
-                                            0, NEVER_FGU);
-               val |= compute_reg_res0_bits(kvm, &hfgwtr_desc,
-                                            0, NEVER_FGU);
+               val |= compute_fgu_bits(kvm, &hfgrtr_desc);
+               val |= compute_fgu_bits(kvm, &hfgwtr_desc);
                break;
        case HFGITR_GROUP:
-               val |= compute_reg_res0_bits(kvm, &hfgitr_desc,
-                                            0, NEVER_FGU);
+               val |= compute_fgu_bits(kvm, &hfgitr_desc);
                break;
        case HDFGRTR_GROUP:
-               val |= compute_reg_res0_bits(kvm, &hdfgrtr_desc,
-                                            0, NEVER_FGU);
-               val |= compute_reg_res0_bits(kvm, &hdfgwtr_desc,
-                                            0, NEVER_FGU);
+               val |= compute_fgu_bits(kvm, &hdfgrtr_desc);
+               val |= compute_fgu_bits(kvm, &hdfgwtr_desc);
                break;
        case HAFGRTR_GROUP:
-               val |= compute_reg_res0_bits(kvm, &hafgrtr_desc,
-                                            0, NEVER_FGU);
+               val |= compute_fgu_bits(kvm, &hafgrtr_desc);
                break;
        case HFGRTR2_GROUP:
-               val |= compute_reg_res0_bits(kvm, &hfgrtr2_desc,
-                                            0, NEVER_FGU);
-               val |= compute_reg_res0_bits(kvm, &hfgwtr2_desc,
-                                            0, NEVER_FGU);
+               val |= compute_fgu_bits(kvm, &hfgrtr2_desc);
+               val |= compute_fgu_bits(kvm, &hfgwtr2_desc);
                break;
        case HFGITR2_GROUP:
-               val |= compute_reg_res0_bits(kvm, &hfgitr2_desc,
-                                            0, NEVER_FGU);
+               val |= compute_fgu_bits(kvm, &hfgitr2_desc);
                break;
        case HDFGRTR2_GROUP:
-               val |= compute_reg_res0_bits(kvm, &hdfgrtr2_desc,
-                                            0, NEVER_FGU);
-               val |= compute_reg_res0_bits(kvm, &hdfgwtr2_desc,
-                                            0, NEVER_FGU);
+               val |= compute_fgu_bits(kvm, &hdfgrtr2_desc);
+               val |= compute_fgu_bits(kvm, &hdfgwtr2_desc);
                break;
        default:
                BUG();
--

2.47.3

We have so far mostly tracked RES0 bits, but only made a few attempts at being just as strict for RES1 bits (probably because they are both rarer and harder to handle).

Start scratching the surface by introducing a data structure tracking RES0 and RES1 bits at the same time.

Note that contrary to the usual idiom, this structure is mostly passed around by value -- the ABI handles it nicely, and the resulting code is much nicer.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/include/asm/kvm_host.h |  17 ++--
 arch/arm64/kvm/config.c           | 122 +++++++++++++++-------------
 arch/arm64/kvm/nested.c           | 129 +++++++++++++++---------------
 3 files changed, 144 insertions(+), 124 deletions(-)

arch/arm64/include/asm/kvm_host.h +5 -0

index b552a1e03848c..a7e4cd8ebf56f 100644
@@ -626,13 +626,20 @@ enum vcpu_sysreg {
        NR_SYS_REGS     /* Nothing after this line! */
 };
 
+struct resx {
+       u64     res0;
+       u64     res1;
+};
+
 struct kvm_sysreg_masks {

arch/arm64/include/asm/kvm_host.h +2 -1

+
 struct fgt_masks {
        const char      *str;
        u64             mask;

@@ -1607,7 +1614,7 @@ static inline bool kvm_arch_has_irq_bypass(void)
 }
 
 void compute_fgu(struct kvm *kvm, enum fgt_group_id fgt);
-void get_reg_fixed_bits(struct kvm *kvm, enum vcpu_sysreg reg, u64 *res0, u64 *res1);
+struct resx get_reg_fixed_bits(struct kvm *kvm, enum vcpu_sysreg reg);
 void check_feature_map(void);
 void kvm_vcpu_load_fgt(struct kvm_vcpu *vcpu);
 

arch/arm64/kvm/config.c +0 -0

index 2122599f7cbbd..a907195bd44b6 100644

arch/arm64/kvm/config.c +67 -53

+struct resx __compute_fixed_bits(struct kvm *kvm,
                                const struct reg_bits_to_feat_map *map,
                                int map_size,
                                u64 *fixed_bits,
                                unsigned long require,
                                unsigned long exclude)
 {
-       u64 val = 0;
+       struct resx resx = {};
 
        for (int i = 0; i < map_size; i++) {
                bool match;

@@ -1316,13 +1317,14 @@ static u64 __compute_fixed_bits(struct kvm *kvm,
                        match = idreg_feat_match(kvm, &map[i]);
 
                if (!match || (map[i].flags & FIXED_VALUE))
-                       val |= reg_feat_map_bits(&map[i]);
+                       resx.res0 |= reg_feat_map_bits(&map[i]);
        }
 
-       return val;
+       return resx;
 }
 
-static u64 compute_res0_bits(struct kvm *kvm,
+static
+struct resx compute_resx_bits(struct kvm *kvm,
                             const struct reg_bits_to_feat_map *map,
                             int map_size,
                             unsigned long require,

@@ -1332,34 +1334,43 @@ static u64 compute_res0_bits(struct kvm *kvm,
                                    require, exclude | FIXED_VALUE);
 }
 
-static u64 compute_reg_res0_bits(struct kvm *kvm,
+static
+struct resx compute_reg_resx_bits(struct kvm *kvm,
                                 const struct reg_feat_map_desc *r,
                                 unsigned long require, unsigned long exclude)
 {
-       u64 res0;
+       struct resx resx, tmp;
 
-       res0 = compute_res0_bits(kvm, r->bit_feat_map, r->bit_feat_map_sz,
+       resx = compute_resx_bits(kvm, r->bit_feat_map, r->bit_feat_map_sz,
                                 require, exclude);
 
-       res0 |= compute_res0_bits(kvm, &r->feat_map, 1, require, exclude);
-       res0 |= ~reg_feat_map_bits(&r->feat_map);
+       tmp = compute_resx_bits(kvm, &r->feat_map, 1, require, exclude);
+
+       resx.res0 |= tmp.res0;
+       resx.res0 |= ~reg_feat_map_bits(&r->feat_map);
+       resx.res1 |= tmp.res1;
 
-       return res0;
+       return resx;
 }
 
 static u64 compute_fgu_bits(struct kvm *kvm, const struct reg_feat_map_desc *r)
 {
+       struct resx resx;
+
        /*
         * If computing FGUs, we collect the unsupported feature bits as
-        * RES0 bits, but don't take the actual RES0 bits or register
+        * RESx bits, but don't take the actual RESx bits or register
         * existence into account -- we're not computing bits for the
         * register itself.
         */
-       return compute_res0_bits(kvm, r->bit_feat_map, r->bit_feat_map_sz,
+       resx = compute_resx_bits(kvm, r->bit_feat_map, r->bit_feat_map_sz,
                                 0, NEVER_FGU);
+
+       return resx.res0 | resx.res1;
 }
 
-static u64 compute_reg_fixed_bits(struct kvm *kvm,
+static
+struct resx compute_reg_fixed_bits(struct kvm *kvm,
                                  const struct reg_feat_map_desc *r,
                                  u64 *fixed_bits, unsigned long require,
                                  unsigned long exclude)

@@ -1405,91 +1416,94 @@ void compute_fgu(struct kvm *kvm, enum fgt_group_id fgt)
        kvm->arch.fgu[fgt] = val;
 }
 
-void get_reg_fixed_bits(struct kvm *kvm, enum vcpu_sysreg reg, u64 *res0, u64 *res1)
+struct resx get_reg_fixed_bits(struct kvm *kvm, enum vcpu_sysreg reg)
 {
        u64 fixed = 0, mask;
+       struct resx resx;
 
        switch (reg) {
        case HFGRTR_EL2:
-               *res0 = compute_reg_res0_bits(kvm, &hfgrtr_desc, 0, 0);
-               *res1 = HFGRTR_EL2_RES1;
+               resx = compute_reg_resx_bits(kvm, &hfgrtr_desc, 0, 0);
+               resx.res1 |= HFGRTR_EL2_RES1;
                break;
        case HFGWTR_EL2:
-               *res0 = compute_reg_res0_bits(kvm, &hfgwtr_desc, 0, 0);
-               *res1 = HFGWTR_EL2_RES1;
+               resx = compute_reg_resx_bits(kvm, &hfgwtr_desc, 0, 0);
+               resx.res1 |= HFGWTR_EL2_RES1;
                break;
        case HFGITR_EL2:
-               *res0 = compute_reg_res0_bits(kvm, &hfgitr_desc, 0, 0);
-               *res1 = HFGITR_EL2_RES1;
+               resx = compute_reg_resx_bits(kvm, &hfgitr_desc, 0, 0);
+               resx.res1 |= HFGITR_EL2_RES1;
                break;
        case HDFGRTR_EL2:
-               *res0 = compute_reg_res0_bits(kvm, &hdfgrtr_desc, 0, 0);
-               *res1 = HDFGRTR_EL2_RES1;
+               resx = compute_reg_resx_bits(kvm, &hdfgrtr_desc, 0, 0);
+               resx.res1 |= HDFGRTR_EL2_RES1;
                break;
        case HDFGWTR_EL2:
-               *res0 = compute_reg_res0_bits(kvm, &hdfgwtr_desc, 0, 0);
-               *res1 = HDFGWTR_EL2_RES1;
+               resx = compute_reg_resx_bits(kvm, &hdfgwtr_desc, 0, 0);
+               resx.res1 |= HDFGWTR_EL2_RES1;
                break;
        case HAFGRTR_EL2:
-               *res0 = compute_reg_res0_bits(kvm, &hafgrtr_desc, 0, 0);
-               *res1 = HAFGRTR_EL2_RES1;
+               resx = compute_reg_resx_bits(kvm, &hafgrtr_desc, 0, 0);
+               resx.res1 |= HAFGRTR_EL2_RES1;
                break;
        case HFGRTR2_EL2:
-               *res0 = compute_reg_res0_bits(kvm, &hfgrtr2_desc, 0, 0);
-               *res1 = HFGRTR2_EL2_RES1;
+               resx = compute_reg_resx_bits(kvm, &hfgrtr2_desc, 0, 0);
+               resx.res1 |= HFGRTR2_EL2_RES1;
                break;
        case HFGWTR2_EL2:
-               *res0 = compute_reg_res0_bits(kvm, &hfgwtr2_desc, 0, 0);
-               *res1 = HFGWTR2_EL2_RES1;
+               resx = compute_reg_resx_bits(kvm, &hfgwtr2_desc, 0, 0);
+               resx.res1 |= HFGWTR2_EL2_RES1;
                break;
        case HFGITR2_EL2:
-               *res0 = compute_reg_res0_bits(kvm, &hfgitr2_desc, 0, 0);
-               *res1 = HFGITR2_EL2_RES1;
+               resx = compute_reg_resx_bits(kvm, &hfgitr2_desc, 0, 0);
+               resx.res1 |= HFGITR2_EL2_RES1;
                break;
        case HDFGRTR2_EL2:
-               *res0 = compute_reg_res0_bits(kvm, &hdfgrtr2_desc, 0, 0);
-               *res1 = HDFGRTR2_EL2_RES1;
+               resx = compute_reg_resx_bits(kvm, &hdfgrtr2_desc, 0, 0);
+               resx.res1 |= HDFGRTR2_EL2_RES1;
                break;
        case HDFGWTR2_EL2:
-               *res0 = compute_reg_res0_bits(kvm, &hdfgwtr2_desc, 0, 0);
-               *res1 = HDFGWTR2_EL2_RES1;
+               resx = compute_reg_resx_bits(kvm, &hdfgwtr2_desc, 0, 0);
+               resx.res1 |= HDFGWTR2_EL2_RES1;
                break;
        case HCRX_EL2:
-               *res0 = compute_reg_res0_bits(kvm, &hcrx_desc, 0, 0);
-               *res1 = __HCRX_EL2_RES1;
+               resx = compute_reg_resx_bits(kvm, &hcrx_desc, 0, 0);
+               resx.res1 |= __HCRX_EL2_RES1;
                break;
        case HCR_EL2:
-               mask = compute_reg_fixed_bits(kvm, &hcr_desc, &fixed, 0, 0);
-               *res0 = compute_reg_res0_bits(kvm, &hcr_desc, 0, 0);
-               *res0 |= (mask & ~fixed);
-               *res1 = HCR_EL2_RES1 | (mask & fixed);
+               mask = compute_reg_fixed_bits(kvm, &hcr_desc, &fixed, 0, 0).res0;
+               resx = compute_reg_resx_bits(kvm, &hcr_desc, 0, 0);
+               resx.res0 |= (mask & ~fixed);
+               resx.res1 |= HCR_EL2_RES1 | (mask & fixed);
                break;
        case SCTLR2_EL1:
        case SCTLR2_EL2:
-               *res0 = compute_reg_res0_bits(kvm, &sctlr2_desc, 0, 0);
-               *res1 = SCTLR2_EL1_RES1;
+               resx = compute_reg_resx_bits(kvm, &sctlr2_desc, 0, 0);
+               resx.res1 |= SCTLR2_EL1_RES1;
                break;
        case TCR2_EL2:
-               *res0 = compute_reg_res0_bits(kvm, &tcr2_el2_desc, 0, 0);
-               *res1 = TCR2_EL2_RES1;
+               resx = compute_reg_resx_bits(kvm, &tcr2_el2_desc, 0, 0);
+               resx.res1 |= TCR2_EL2_RES1;
                break;
        case SCTLR_EL1:
-               *res0 = compute_reg_res0_bits(kvm, &sctlr_el1_desc, 0, 0);
-               *res1 = SCTLR_EL1_RES1;
+               resx = compute_reg_resx_bits(kvm, &sctlr_el1_desc, 0, 0);
+               resx.res1 |= SCTLR_EL1_RES1;
                break;
        case MDCR_EL2:
-               *res0 = compute_reg_res0_bits(kvm, &mdcr_el2_desc, 0, 0);
-               *res1 = MDCR_EL2_RES1;
+               resx = compute_reg_resx_bits(kvm, &mdcr_el2_desc, 0, 0);
+               resx.res1 |= MDCR_EL2_RES1;
                break;
        case VTCR_EL2:
-               *res0 = compute_reg_res0_bits(kvm, &vtcr_el2_desc, 0, 0);
-               *res1 = VTCR_EL2_RES1;
+               resx = compute_reg_resx_bits(kvm, &vtcr_el2_desc, 0, 0);
+               resx.res1 |= VTCR_EL2_RES1;
                break;
        default:
                WARN_ON_ONCE(1);
-               *res0 = *res1 = 0;
+               resx = (typeof(resx)){};
                break;
        }
+
+       return resx;
 }
 
 static __always_inline struct fgt_masks *__fgt_reg_to_masks(enum vcpu_sysreg reg)

arch/arm64/kvm/nested.c +64 -66

index 486eba72bb027..c5a45bc62153e 100644
@@ -1683,22 +1683,19 @@ u64 kvm_vcpu_apply_reg_masks(const struct kvm_vcpu *vcpu,
        return v;
 }
 
-static __always_inline void set_sysreg_masks(struct kvm *kvm, int sr, u64 res0, u64 res1)
+static __always_inline void set_sysreg_masks(struct kvm *kvm, int sr, struct resx resx)
 {
-       int i = sr - __SANITISED_REG_START__;
-
        BUILD_BUG_ON(!__builtin_constant_p(sr));
        BUILD_BUG_ON(sr < __SANITISED_REG_START__);
        BUILD_BUG_ON(sr >= NR_SYS_REGS);
 
-       kvm->arch.sysreg_masks->mask[i].res0 = res0;
-       kvm->arch.sysreg_masks->mask[i].res1 = res1;
+       kvm_set_sysreg_resx(kvm, sr, resx);
 }
 
 int kvm_init_nv_sysregs(struct kvm_vcpu *vcpu)
 {
        struct kvm *kvm = vcpu->kvm;
-       u64 res0, res1;
+       struct resx resx;
 
        lockdep_assert_held(&kvm->arch.config_lock);
 

@@ -1711,110 +1708,112 @@ int kvm_init_nv_sysregs(struct kvm_vcpu *vcpu)
                return -ENOMEM;
 
        /* VTTBR_EL2 */
-       res0 = res1 = 0;
+       resx = (typeof(resx)){};
        if (!kvm_has_feat_enum(kvm, ID_AA64MMFR1_EL1, VMIDBits, 16))
-               res0 |= GENMASK(63, 56);
+               resx.res0 |= GENMASK(63, 56);
        if (!kvm_has_feat(kvm, ID_AA64MMFR2_EL1, CnP, IMP))
-               res0 |= VTTBR_CNP_BIT;
-       set_sysreg_masks(kvm, VTTBR_EL2, res0, res1);
+               resx.res0 |= VTTBR_CNP_BIT;
+       set_sysreg_masks(kvm, VTTBR_EL2, resx);
 
        /* VTCR_EL2 */
-       get_reg_fixed_bits(kvm, VTCR_EL2, &res0, &res1);
-       set_sysreg_masks(kvm, VTCR_EL2, res0, res1);
+       resx = get_reg_fixed_bits(kvm, VTCR_EL2);
+       set_sysreg_masks(kvm, VTCR_EL2, resx);
 
        /* VMPIDR_EL2 */
-       res0 = GENMASK(63, 40) | GENMASK(30, 24);
-       res1 = BIT(31);
-       set_sysreg_masks(kvm, VMPIDR_EL2, res0, res1);
+       resx.res0 = GENMASK(63, 40) | GENMASK(30, 24);
+       resx.res1 = BIT(31);
+       set_sysreg_masks(kvm, VMPIDR_EL2, resx);
 
        /* HCR_EL2 */
-       get_reg_fixed_bits(kvm, HCR_EL2, &res0, &res1);
-       set_sysreg_masks(kvm, HCR_EL2, res0, res1);
+       resx = get_reg_fixed_bits(kvm, HCR_EL2);
+       set_sysreg_masks(kvm, HCR_EL2, resx);
 
        /* HCRX_EL2 */
-       get_reg_fixed_bits(kvm, HCRX_EL2, &res0, &res1);
-       set_sysreg_masks(kvm, HCRX_EL2, res0, res1);
+       resx = get_reg_fixed_bits(kvm, HCRX_EL2);
+       set_sysreg_masks(kvm, HCRX_EL2, resx);
 
        /* HFG[RW]TR_EL2 */
-       get_reg_fixed_bits(kvm, HFGRTR_EL2, &res0, &res1);
-       set_sysreg_masks(kvm, HFGRTR_EL2, res0, res1);
-       get_reg_fixed_bits(kvm, HFGWTR_EL2, &res0, &res1);
-       set_sysreg_masks(kvm, HFGWTR_EL2, res0, res1);
+       resx = get_reg_fixed_bits(kvm, HFGRTR_EL2);
+       set_sysreg_masks(kvm, HFGRTR_EL2, resx);
+       resx = get_reg_fixed_bits(kvm, HFGWTR_EL2);
+       set_sysreg_masks(kvm, HFGWTR_EL2, resx);
 
        /* HDFG[RW]TR_EL2 */
-       get_reg_fixed_bits(kvm, HDFGRTR_EL2, &res0, &res1);
-       set_sysreg_masks(kvm, HDFGRTR_EL2, res0, res1);
-       get_reg_fixed_bits(kvm, HDFGWTR_EL2, &res0, &res1);
-       set_sysreg_masks(kvm, HDFGWTR_EL2, res0, res1);
+       resx = get_reg_fixed_bits(kvm, HDFGRTR_EL2);
+       set_sysreg_masks(kvm, HDFGRTR_EL2, resx);
+       resx = get_reg_fixed_bits(kvm, HDFGWTR_EL2);
+       set_sysreg_masks(kvm, HDFGWTR_EL2, resx);
 
        /* HFGITR_EL2 */
-       get_reg_fixed_bits(kvm, HFGITR_EL2, &res0, &res1);
-       set_sysreg_masks(kvm, HFGITR_EL2, res0, res1);
+       resx = get_reg_fixed_bits(kvm, HFGITR_EL2);
+       set_sysreg_masks(kvm, HFGITR_EL2, resx);
 
        /* HAFGRTR_EL2 - not a lot to see here */
-       get_reg_fixed_bits(kvm, HAFGRTR_EL2, &res0, &res1);
-       set_sysreg_masks(kvm, HAFGRTR_EL2, res0, res1);
+       resx = get_reg_fixed_bits(kvm, HAFGRTR_EL2);
+       set_sysreg_masks(kvm, HAFGRTR_EL2, resx);
 
        /* HFG[RW]TR2_EL2 */
-       get_reg_fixed_bits(kvm, HFGRTR2_EL2, &res0, &res1);
-       set_sysreg_masks(kvm, HFGRTR2_EL2, res0, res1);
-       get_reg_fixed_bits(kvm, HFGWTR2_EL2, &res0, &res1);
-       set_sysreg_masks(kvm, HFGWTR2_EL2, res0, res1);
+       resx = get_reg_fixed_bits(kvm, HFGRTR2_EL2);
+       set_sysreg_masks(kvm, HFGRTR2_EL2, resx);
+       resx = get_reg_fixed_bits(kvm, HFGWTR2_EL2);
+       set_sysreg_masks(kvm, HFGWTR2_EL2, resx);
 
        /* HDFG[RW]TR2_EL2 */
-       get_reg_fixed_bits(kvm, HDFGRTR2_EL2, &res0, &res1);
-       set_sysreg_masks(kvm, HDFGRTR2_EL2, res0, res1);
-       get_reg_fixed_bits(kvm, HDFGWTR2_EL2, &res0, &res1);
-       set_sysreg_masks(kvm, HDFGWTR2_EL2, res0, res1);
+       resx = get_reg_fixed_bits(kvm, HDFGRTR2_EL2);
+       set_sysreg_masks(kvm, HDFGRTR2_EL2, resx);
+       resx = get_reg_fixed_bits(kvm, HDFGWTR2_EL2);
+       set_sysreg_masks(kvm, HDFGWTR2_EL2, resx);
 
        /* HFGITR2_EL2 */
-       get_reg_fixed_bits(kvm, HFGITR2_EL2, &res0, &res1);
-       set_sysreg_masks(kvm, HFGITR2_EL2, res0, res1);
+       resx = get_reg_fixed_bits(kvm, HFGITR2_EL2);
+       set_sysreg_masks(kvm, HFGITR2_EL2, resx);
 
        /* TCR2_EL2 */
-       get_reg_fixed_bits(kvm, TCR2_EL2, &res0, &res1);
-       set_sysreg_masks(kvm, TCR2_EL2, res0, res1);
+       resx = get_reg_fixed_bits(kvm, TCR2_EL2);
+       set_sysreg_masks(kvm, TCR2_EL2, resx);
 
        /* SCTLR_EL1 */
-       get_reg_fixed_bits(kvm, SCTLR_EL1, &res0, &res1);
-       set_sysreg_masks(kvm, SCTLR_EL1, res0, res1);
+       resx = get_reg_fixed_bits(kvm, SCTLR_EL1);
+       set_sysreg_masks(kvm, SCTLR_EL1, resx);
 
        /* SCTLR2_ELx */
-       get_reg_fixed_bits(kvm, SCTLR2_EL1, &res0, &res1);
-       set_sysreg_masks(kvm, SCTLR2_EL1, res0, res1);
-       get_reg_fixed_bits(kvm, SCTLR2_EL2, &res0, &res1);
-       set_sysreg_masks(kvm, SCTLR2_EL2, res0, res1);
+       resx = get_reg_fixed_bits(kvm, SCTLR2_EL1);
+       set_sysreg_masks(kvm, SCTLR2_EL1, resx);
+       resx = get_reg_fixed_bits(kvm, SCTLR2_EL2);
+       set_sysreg_masks(kvm, SCTLR2_EL2, resx);
 
        /* MDCR_EL2 */
-       get_reg_fixed_bits(kvm, MDCR_EL2, &res0, &res1);
-       set_sysreg_masks(kvm, MDCR_EL2, res0, res1);
+       resx = get_reg_fixed_bits(kvm, MDCR_EL2);
+       set_sysreg_masks(kvm, MDCR_EL2, resx);
 
        /* CNTHCTL_EL2 */
-       res0 = GENMASK(63, 20);
-       res1 = 0;
+       resx.res0 = GENMASK(63, 20);
+       resx.res1 = 0;
        if (!kvm_has_feat(kvm, ID_AA64PFR0_EL1, RME, IMP))
-               res0 |= CNTHCTL_CNTPMASK | CNTHCTL_CNTVMASK;
+               resx.res0 |= CNTHCTL_CNTPMASK | CNTHCTL_CNTVMASK;
        if (!kvm_has_feat(kvm, ID_AA64MMFR0_EL1, ECV, CNTPOFF)) {
-               res0 |= CNTHCTL_ECV;
+               resx.res0 |= CNTHCTL_ECV;
                if (!kvm_has_feat(kvm, ID_AA64MMFR0_EL1, ECV, IMP))
-                       res0 |= (CNTHCTL_EL1TVT | CNTHCTL_EL1TVCT |
-                                CNTHCTL_EL1NVPCT | CNTHCTL_EL1NVVCT);
+                       resx.res0 |= (CNTHCTL_EL1TVT | CNTHCTL_EL1TVCT |
+                                     CNTHCTL_EL1NVPCT | CNTHCTL_EL1NVVCT);
        }
        if (!kvm_has_feat(kvm, ID_AA64MMFR1_EL1, VH, IMP))
-               res0 |= GENMASK(11, 8);
-       set_sysreg_masks(kvm, CNTHCTL_EL2, res0, res1);
+               resx.res0 |= GENMASK(11, 8);
+       set_sysreg_masks(kvm, CNTHCTL_EL2, resx);
 
        /* ICH_HCR_EL2 */
-       res0 = ICH_HCR_EL2_RES0;
-       res1 = ICH_HCR_EL2_RES1;
+       resx.res0 = ICH_HCR_EL2_RES0;
+       resx.res1 = ICH_HCR_EL2_RES1;
        if (!(kvm_vgic_global_state.ich_vtr_el2 & ICH_VTR_EL2_TDS))
-               res0 |= ICH_HCR_EL2_TDIR;
+               resx.res0 |= ICH_HCR_EL2_TDIR;
        /* No GICv4 is presented to the guest */
-       res0 |= ICH_HCR_EL2_DVIM | ICH_HCR_EL2_vSGIEOICount;
-       set_sysreg_masks(kvm, ICH_HCR_EL2, res0, res1);
+       resx.res0 |= ICH_HCR_EL2_DVIM | ICH_HCR_EL2_vSGIEOICount;
+       set_sysreg_masks(kvm, ICH_HCR_EL2, resx);
 
        /* VNCR_EL2 */
-       set_sysreg_masks(kvm, VNCR_EL2, VNCR_EL2_RES0, VNCR_EL2_RES1);
+       resx.res0 = VNCR_EL2_RES0;
+       resx.res1 = VNCR_EL2_RES1;
+       set_sysreg_masks(kvm, VNCR_EL2, resx);
 
 out:
        for (enum vcpu_sysreg sr = __SANITISED_REG_START__; sr < NR_SYS_REGS; sr++)
--

2.47.3

Add a new helper to retrieve the RESx values for a given system register, and use it for the runtime sanitisation.

This results in slightly better code generation for a fairly hot path in the hypervisor.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/include/asm/kvm_host.h | 13 +++++++++++++
 arch/arm64/kvm/emulate-nested.c   | 10 +---------
 arch/arm64/kvm/nested.c           | 13 ++++---------
 3 files changed, 18 insertions(+), 18 deletions(-)

arch/arm64/include/asm/kvm_host.h +3 -0

index a7e4cd8ebf56f..9dca94e4361f0 100644
@@ -635,6 +635,19 @@ struct kvm_sysreg_masks {
        struct resx mask[NR_SYS_REGS - __SANITISED_REG_START__];
 };
 
+#define kvm_get_sysreg_resx(k, sr)                                     \
+       ({                                                              \
+               struct kvm_sysreg_masks *__masks;                       \

arch/arm64/include/asm/kvm_host.h +0 -0

 #define kvm_set_sysreg_resx(k, sr, resx)               \
        do {                                            \
                (k)->arch.sysreg_masks->mask[sr - __SANITISED_REG_START__] = resx; \

arch/arm64/kvm/emulate-nested.c +1 -9

index 774cfbf5b43ba..43334cd2db9e5 100644
@@ -2427,15 +2427,7 @@ static enum trap_behaviour compute_trap_behaviour(struct kvm_vcpu *vcpu,
 
 static u64 kvm_get_sysreg_res0(struct kvm *kvm, enum vcpu_sysreg sr)
 {
-       struct kvm_sysreg_masks *masks;
-
-       /* Only handle the VNCR-backed regs for now */
-       if (sr < __VNCR_START__)
-               return 0;
-
-       masks = kvm->arch.sysreg_masks;
-
-       return masks->mask[sr - __SANITISED_REG_START__].res0;
+       return kvm_get_sysreg_resx(kvm, sr).res0;
 }
 
 static bool check_fgt_bit(struct kvm_vcpu *vcpu, enum vcpu_sysreg sr,

arch/arm64/kvm/nested.c +4 -10

index c5a45bc62153e..75a23f1c56d13 100644
@@ -1669,16 +1669,11 @@ u64 limit_nv_id_reg(struct kvm *kvm, u32 reg, u64 val)
 u64 kvm_vcpu_apply_reg_masks(const struct kvm_vcpu *vcpu,
                             enum vcpu_sysreg sr, u64 v)
 {
-       struct kvm_sysreg_masks *masks;
-
-       masks = vcpu->kvm->arch.sysreg_masks;
-
-       if (masks) {
-               sr -= __SANITISED_REG_START__;
+       struct resx resx;
 
-               v &= ~masks->mask[sr].res0;
-               v |= masks->mask[sr].res1;
-       }
+       resx = kvm_get_sysreg_resx(vcpu->kvm, sr);
+       v &= ~resx.res0;
+       v |= resx.res1;
 
        return v;
 }
--

2.47.3

Hi Marc,

Quoted 22 lines · Marc Zyngier

Add a new helper to retrieve the RESx values for a given system register, and use it for the runtime sanitisation.

This results in slightly better code generation for a fairly hot path in the hypervisor.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/include/asm/kvm_host.h | 13 +++++++++++++
 arch/arm64/kvm/emulate-nested.c   | 10 +---------
 arch/arm64/kvm/nested.c           | 13 ++++---------
 3 files changed, 18 insertions(+), 18 deletions(-)

arch/arm64/include/asm/kvm_host.h +0 -0

index a7e4cd8ebf56f..9dca94e4361f0 100644
@@ -635,6 +635,19 @@ struct kvm_sysreg_masks {
        struct resx mask[NR_SYS_REGS - __SANITISED_REG_START__];
 };
 

arch/arm64/include/asm/kvm_host.h struct kvm_sysreg_masks { · line 650

Quoted 13 lines

arch/arm64/include/asm/kvm_host.h +13 -0

+#define kvm_get_sysreg_resx(k, sr)                                     \
+       ({                                                              \
+               struct kvm_sysreg_masks *__masks;                       \
+               struct resx __resx = {};                                \
+                                                                       \
+               __masks = (k)->arch.sysreg_masks;                       \
+               if (likely(__masks &&                                   \
+                          sr >= __SANITISED_REG_START__ &&             \
+                          sr < NR_SYS_REGS))                           \
+                       __resx = __masks->mask[sr - __SANITISED_REG_START__]; \
+               __resx;                                                 \
+       })
+

This now covers all registers that need to be sanitized, not just VNCR-backed ones now.

nit: wouldn't it be better to capture sr in a local variable rather than reuse it? It is an enum, but it would make checkpatch feel slightly better :)

Reviewed-by Fuad Tabba <[email protected]>

The FGT registers have their computed RESx bits stashed in specific descriptors, which we can easily use when computing the masks used for the guest.

This removes a bit of boilerplate code.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/kvm/config.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

arch/arm64/kvm/config.c +5 -11

index a907195bd44b6..8d152605999ba 100644
@@ -1344,6 +1344,11 @@ struct resx compute_reg_resx_bits(struct kvm *kvm,
 	resx = compute_resx_bits(kvm, r->bit_feat_map, r->bit_feat_map_sz,
 				 require, exclude);
 
+	if (r->feat_map.flags & MASKS_POINTER) {
+		resx.res0 |= r->feat_map.masks->res0;
+		resx.res1 |= r->feat_map.masks->res1;
+	}
+
 	tmp = compute_resx_bits(kvm, &r->feat_map, 1, require, exclude);
 
 	resx.res0 |= tmp.res0;

@@ -1424,47 +1429,36 @@ struct resx get_reg_fixed_bits(struct kvm *kvm, enum vcpu_sysreg reg)
 	switch (reg) {
 	case HFGRTR_EL2:
 		resx = compute_reg_resx_bits(kvm, &hfgrtr_desc, 0, 0);
-		resx.res1 |= HFGRTR_EL2_RES1;
 		break;
 	case HFGWTR_EL2:
 		resx = compute_reg_resx_bits(kvm, &hfgwtr_desc, 0, 0);
-		resx.res1 |= HFGWTR_EL2_RES1;
 		break;
 	case HFGITR_EL2:
 		resx = compute_reg_resx_bits(kvm, &hfgitr_desc, 0, 0);
-		resx.res1 |= HFGITR_EL2_RES1;
 		break;
 	case HDFGRTR_EL2:
 		resx = compute_reg_resx_bits(kvm, &hdfgrtr_desc, 0, 0);
-		resx.res1 |= HDFGRTR_EL2_RES1;
 		break;
 	case HDFGWTR_EL2:
 		resx = compute_reg_resx_bits(kvm, &hdfgwtr_desc, 0, 0);
-		resx.res1 |= HDFGWTR_EL2_RES1;
 		break;
 	case HAFGRTR_EL2:
 		resx = compute_reg_resx_bits(kvm, &hafgrtr_desc, 0, 0);
-		resx.res1 |= HAFGRTR_EL2_RES1;
 		break;
 	case HFGRTR2_EL2:
 		resx = compute_reg_resx_bits(kvm, &hfgrtr2_desc, 0, 0);
-		resx.res1 |= HFGRTR2_EL2_RES1;
 		break;
 	case HFGWTR2_EL2:
 		resx = compute_reg_resx_bits(kvm, &hfgwtr2_desc, 0, 0);
-		resx.res1 |= HFGWTR2_EL2_RES1;
 		break;
 	case HFGITR2_EL2:
 		resx = compute_reg_resx_bits(kvm, &hfgitr2_desc, 0, 0);
-		resx.res1 |= HFGITR2_EL2_RES1;
 		break;
 	case HDFGRTR2_EL2:
 		resx = compute_reg_resx_bits(kvm, &hdfgrtr2_desc, 0, 0);
-		resx.res1 |= HDFGRTR2_EL2_RES1;
 		break;
 	case HDFGWTR2_EL2:
 		resx = compute_reg_resx_bits(kvm, &hdfgwtr2_desc, 0, 0);
-		resx.res1 |= HDFGWTR2_EL2_RES1;
 		break;
 	case HCRX_EL2:
 		resx = compute_reg_resx_bits(kvm, &hcrx_desc, 0, 0);

Signature · 1 line

2.47.3

The FGT registers have their computed RESx bits stashed in specific descriptors, which we can easily use when computing the masks used for the guest.

This removes a bit of boilerplate code.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/kvm/config.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

arch/arm64/kvm/config.c +5 -12

index a907195bd44b6..8d152605999ba 100644
@@ -1344,6 +1344,11 @@ struct resx compute_reg_resx_bits(struct kvm *kvm,
        resx = compute_resx_bits(kvm, r->bit_feat_map, r->bit_feat_map_sz,
                                 require, exclude);
 
+       if (r->feat_map.flags & MASKS_POINTER) {
+               resx.res0 |= r->feat_map.masks->res0;
+               resx.res1 |= r->feat_map.masks->res1;
+       }
+
        tmp = compute_resx_bits(kvm, &r->feat_map, 1, require, exclude);
 
        resx.res0 |= tmp.res0;

@@ -1424,47 +1429,36 @@ struct resx get_reg_fixed_bits(struct kvm *kvm, enum vcpu_sysreg reg)
        switch (reg) {
        case HFGRTR_EL2:
                resx = compute_reg_resx_bits(kvm, &hfgrtr_desc, 0, 0);
-               resx.res1 |= HFGRTR_EL2_RES1;
                break;
        case HFGWTR_EL2:
                resx = compute_reg_resx_bits(kvm, &hfgwtr_desc, 0, 0);
-               resx.res1 |= HFGWTR_EL2_RES1;
                break;
        case HFGITR_EL2:
                resx = compute_reg_resx_bits(kvm, &hfgitr_desc, 0, 0);
-               resx.res1 |= HFGITR_EL2_RES1;
                break;
        case HDFGRTR_EL2:
                resx = compute_reg_resx_bits(kvm, &hdfgrtr_desc, 0, 0);
-               resx.res1 |= HDFGRTR_EL2_RES1;
                break;
        case HDFGWTR_EL2:
                resx = compute_reg_resx_bits(kvm, &hdfgwtr_desc, 0, 0);
-               resx.res1 |= HDFGWTR_EL2_RES1;
                break;
        case HAFGRTR_EL2:
                resx = compute_reg_resx_bits(kvm, &hafgrtr_desc, 0, 0);
-               resx.res1 |= HAFGRTR_EL2_RES1;
                break;
        case HFGRTR2_EL2:
                resx = compute_reg_resx_bits(kvm, &hfgrtr2_desc, 0, 0);
-               resx.res1 |= HFGRTR2_EL2_RES1;
                break;
        case HFGWTR2_EL2:
                resx = compute_reg_resx_bits(kvm, &hfgwtr2_desc, 0, 0);
-               resx.res1 |= HFGWTR2_EL2_RES1;
                break;
        case HFGITR2_EL2:
                resx = compute_reg_resx_bits(kvm, &hfgitr2_desc, 0, 0);
-               resx.res1 |= HFGITR2_EL2_RES1;
                break;
        case HDFGRTR2_EL2:
                resx = compute_reg_resx_bits(kvm, &hdfgrtr2_desc, 0, 0);
-               resx.res1 |= HDFGRTR2_EL2_RES1;
                break;
        case HDFGWTR2_EL2:
                resx = compute_reg_resx_bits(kvm, &hdfgwtr2_desc, 0, 0);
-               resx.res1 |= HDFGWTR2_EL2_RES1;
                break;
        case HCRX_EL2:
                resx = compute_reg_resx_bits(kvm, &hcrx_desc, 0, 0);
--

2.47.3

So far, when a bit field is tied to an unsupported feature, we set it as RES0. This is almost forrect, but there are a few exceptions

where the bits become RES1.

Add a AS_RES1 qualifier that instruct the RESx computing code to simply do that.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/kvm/config.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

arch/arm64/kvm/config.c +7 -2

index 8d152605999ba..6a4674fabf865 100644
@@ -24,6 +24,7 @@ struct reg_bits_to_feat_map {
 #define	CALL_FUNC	BIT(1)	/* Needs to evaluate tons of crap */
 #define	FIXED_VALUE	BIT(2)	/* RAZ/WI or RAO/WI in KVM */
 #define	MASKS_POINTER	BIT(3)	/* Pointer to fgt_masks struct instead of bits */
+#define	AS_RES1		BIT(4)	/* RES1 when not supported */
 
 	unsigned long	flags;
 

@@ -1316,8 +1317,12 @@ struct resx __compute_fixed_bits(struct kvm *kvm,
 		else
 			match = idreg_feat_match(kvm, &map[i]);
 
-		if (!match || (map[i].flags & FIXED_VALUE))
-			resx.res0 |= reg_feat_map_bits(&map[i]);
+		if (!match || (map[i].flags & FIXED_VALUE)) {
+			if (map[i].flags & AS_RES1)
+ 				resx.res1 |= reg_feat_map_bits(&map[i]);
+			else
+				resx.res0 |= reg_feat_map_bits(&map[i]);
+		}
 	}
 
 	return resx;

Signature · 1 line

2.47.3

So far, when a bit field is tied to an unsupported feature, we set it as RES0. This is almost forrect, but there are a few exceptions where the bits become RES1.

Add a AS_RES1 qualifier that instruct the RESx computing code to simply do that.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/kvm/config.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

arch/arm64/kvm/config.c +1 -0

index 8d152605999ba..6a4674fabf865 100644
@@ -24,6 +24,7 @@ struct reg_bits_to_feat_map {
 #define        CALL_FUNC       BIT(1)  /* Needs to evaluate tons of crap */
 #define        FIXED_VALUE     BIT(2)  /* RAZ/WI or RAO/WI in KVM */
 #define        MASKS_POINTER   BIT(3)  /* Pointer to fgt_masks struct instead of bits */
+#define        AS_RES1         BIT(4)  /* RES1 when not supported */
 
        unsigned long   flags;
 

@@ -1316,8 +1317,12 @@ struct resx __compute_fixed_bits(struct kvm *kvm,
                else

arch/arm64/kvm/config.c +0 -1

        }
 
        return resx;
 
 
 
--

2.47.3

A bunch of SCTLR_EL1 bits must be set to RES1 when the controlling

feature is not present. Add the AS_RES1 qualifier where needed.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/kvm/config.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

arch/arm64/kvm/config.c +14 -13

index 6a4674fabf865..68ed5af2b4d53 100644
@@ -1085,27 +1085,28 @@ static const DECLARE_FEAT_MAP(tcr2_el2_desc, TCR2_EL2,
                              tcr2_el2_feat_map, FEAT_TCR2);
 
 static const struct reg_bits_to_feat_map sctlr_el1_feat_map[] = {
-       NEEDS_FEAT(SCTLR_EL1_CP15BEN    |
-                  SCTLR_EL1_ITD        |
-                  SCTLR_EL1_SED,
-                  FEAT_AA32EL0),
+       NEEDS_FEAT(SCTLR_EL1_CP15BEN, FEAT_AA32EL0),
+       NEEDS_FEAT_FLAG(SCTLR_EL1_ITD   |
+                       SCTLR_EL1_SED,
+                       AS_RES1, FEAT_AA32EL0),
        NEEDS_FEAT(SCTLR_EL1_BT0        |
                   SCTLR_EL1_BT1,
                   FEAT_BTI),
        NEEDS_FEAT(SCTLR_EL1_CMOW, FEAT_CMOW),
-       NEEDS_FEAT(SCTLR_EL1_TSCXT, feat_csv2_2_csv2_1p2),
-       NEEDS_FEAT(SCTLR_EL1_EIS        |
-                  SCTLR_EL1_EOS,
-                  FEAT_ExS),
+       NEEDS_FEAT_FLAG(SCTLR_EL1_TSCXT,
+                       AS_RES1, feat_csv2_2_csv2_1p2),
+       NEEDS_FEAT_FLAG(SCTLR_EL1_EIS   |
+                       SCTLR_EL1_EOS,
+                       AS_RES1, FEAT_ExS),
        NEEDS_FEAT(SCTLR_EL1_EnFPM, FEAT_FPMR),
        NEEDS_FEAT(SCTLR_EL1_IESB, FEAT_IESB),
        NEEDS_FEAT(SCTLR_EL1_EnALS, FEAT_LS64),
        NEEDS_FEAT(SCTLR_EL1_EnAS0, FEAT_LS64_ACCDATA),
        NEEDS_FEAT(SCTLR_EL1_EnASR, FEAT_LS64_V),
        NEEDS_FEAT(SCTLR_EL1_nAA, FEAT_LSE2),
-       NEEDS_FEAT(SCTLR_EL1_LSMAOE     |
-                  SCTLR_EL1_nTLSMD,
-                  FEAT_LSMAOC),
+       NEEDS_FEAT_FLAG(SCTLR_EL1_LSMAOE        |
+                       SCTLR_EL1_nTLSMD,
+                       AS_RES1, FEAT_LSMAOC),
        NEEDS_FEAT(SCTLR_EL1_EE, FEAT_MixedEnd),
        NEEDS_FEAT(SCTLR_EL1_E0E, feat_mixedendel0),
        NEEDS_FEAT(SCTLR_EL1_MSCEn, FEAT_MOPS),

@@ -1121,7 +1122,8 @@ static const struct reg_bits_to_feat_map sctlr_el1_feat_map[] = {
        NEEDS_FEAT(SCTLR_EL1_NMI        |
                   SCTLR_EL1_SPINTMASK,
                   FEAT_NMI),
-       NEEDS_FEAT(SCTLR_EL1_SPAN, FEAT_PAN),
+       NEEDS_FEAT_FLAG(SCTLR_EL1_SPAN,
+                       AS_RES1, FEAT_PAN),
        NEEDS_FEAT(SCTLR_EL1_EPAN, FEAT_PAN3),
        NEEDS_FEAT(SCTLR_EL1_EnDA       |
                   SCTLR_EL1_EnDB       |
--

2.47.3

Now that we have the AS_RES1 constraint, it becomes trivial to express the HCR_EL2.RW behaviour.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/kvm/config.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

arch/arm64/kvm/config.c +1 -15

index 68ed5af2b4d53..39487182057a3 100644
@@ -389,19 +389,6 @@ static bool feat_vmid16(struct kvm *kvm)
        return kvm_has_feat_enum(kvm, ID_AA64MMFR1_EL1, VMIDBits, 16);
 }
 
-static bool compute_hcr_rw(struct kvm *kvm, u64 *bits)
-{
-       /* This is purely academic: AArch32 and NV are mutually exclusive */
-       if (bits) {
-               if (kvm_has_feat(kvm, FEAT_AA32EL1))
-                       *bits &= ~HCR_EL2_RW;
-               else
-                       *bits |= HCR_EL2_RW;
-       }
-
-       return true;
-}
-
 static bool compute_hcr_e2h(struct kvm *kvm, u64 *bits)
 {
        if (bits) {

@@ -967,7 +954,7 @@ static const DECLARE_FEAT_MAP(hcrx_desc, __HCRX_EL2,
 
 static const struct reg_bits_to_feat_map hcr_feat_map[] = {
        NEEDS_FEAT(HCR_EL2_TID0, FEAT_AA32EL0),
-       NEEDS_FEAT_FIXED(HCR_EL2_RW, compute_hcr_rw),
+       NEEDS_FEAT_FLAG(HCR_EL2_RW, AS_RES1, FEAT_AA32EL1),
        NEEDS_FEAT(HCR_EL2_HCD, not_feat_aa64el3),
        NEEDS_FEAT(HCR_EL2_AMO          |
                   HCR_EL2_BSU          |
--

2.47.3

The FIXED_VALUE qualifier (mostly used for HCR_EL2) is pointlessly complicated, as it tries to piggy-back on the previous RES0 handling while being done in a different phase, on different data.

Instead, make it an integral part of the RESx computation, and allow it to directly set RESx bits. This is much easier to understand.

It also paves the way for some additional changes to that will allow the full removal of the FIXED_VALUE handling.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/kvm/config.c | 67 ++++++++++++++---------------------------
 1 file changed, 22 insertions(+), 45 deletions(-)

arch/arm64/kvm/config.c +22 -46

index 39487182057a3..4fac04d3132c0 100644
@@ -37,7 +37,7 @@ struct reg_bits_to_feat_map {
                        s8      lo_lim;
                };
                bool    (*match)(struct kvm *);
-               bool    (*fval)(struct kvm *, u64 *);
+               bool    (*fval)(struct kvm *, struct resx *);
        };
 };
 

@@ -389,14 +389,12 @@ static bool feat_vmid16(struct kvm *kvm)
        return kvm_has_feat_enum(kvm, ID_AA64MMFR1_EL1, VMIDBits, 16);
 }
 
-static bool compute_hcr_e2h(struct kvm *kvm, u64 *bits)
+static bool compute_hcr_e2h(struct kvm *kvm, struct resx *bits)
 {
-       if (bits) {
-               if (kvm_has_feat(kvm, FEAT_E2H0))
-                       *bits &= ~HCR_EL2_E2H;
-               else
-                       *bits |= HCR_EL2_E2H;
-       }
+       if (kvm_has_feat(kvm, FEAT_E2H0))
+               bits->res0 |= HCR_EL2_E2H;
+       else
+               bits->res1 |= HCR_EL2_E2H;
 
        return true;
 }

@@ -1281,12 +1279,11 @@ static bool idreg_feat_match(struct kvm *kvm, const struct reg_bits_to_feat_map
 }
 
 static
-struct resx __compute_fixed_bits(struct kvm *kvm,
-                               const struct reg_bits_to_feat_map *map,
-                               int map_size,
-                               u64 *fixed_bits,
-                               unsigned long require,
-                               unsigned long exclude)
+struct resx compute_resx_bits(struct kvm *kvm,
+                             const struct reg_bits_to_feat_map *map,
+                             int map_size,
+                             unsigned long require,
+                             unsigned long exclude)
 {
        struct resx resx = {};
 

@@ -1299,14 +1296,18 @@ struct resx __compute_fixed_bits(struct kvm *kvm,
                if (map[i].flags & exclude)
                        continue;
 
-               if (map[i].flags & CALL_FUNC)
-                       match = (map[i].flags & FIXED_VALUE) ?
-                               map[i].fval(kvm, fixed_bits) :
-                               map[i].match(kvm);
-               else
+               switch (map[i].flags & (CALL_FUNC | FIXED_VALUE)) {
+               case CALL_FUNC | FIXED_VALUE:
+                       map[i].fval(kvm, &resx);
+                       continue;
+               case CALL_FUNC:
+                       match = map[i].match(kvm);
+                       break;
+               default:
                        match = idreg_feat_match(kvm, &map[i]);
+               }
 
-               if (!match || (map[i].flags & FIXED_VALUE)) {
+               if (!match) {
                        if (map[i].flags & AS_RES1)
                                resx.res1 |= reg_feat_map_bits(&map[i]);
                        else

@@ -1317,17 +1318,6 @@ struct resx __compute_fixed_bits(struct kvm *kvm,
        return resx;
 }
 
-static
-struct resx compute_resx_bits(struct kvm *kvm,
-                            const struct reg_bits_to_feat_map *map,
-                            int map_size,
-                            unsigned long require,
-                            unsigned long exclude)
-{
-       return __compute_fixed_bits(kvm, map, map_size, NULL,
-                                   require, exclude | FIXED_VALUE);
-}
-
 static
 struct resx compute_reg_resx_bits(struct kvm *kvm,
                                 const struct reg_feat_map_desc *r,

@@ -1368,16 +1358,6 @@ static u64 compute_fgu_bits(struct kvm *kvm, const struct reg_feat_map_desc *r)
        return resx.res0 | resx.res1;
 }
 
-static
-struct resx compute_reg_fixed_bits(struct kvm *kvm,
-                                 const struct reg_feat_map_desc *r,
-                                 u64 *fixed_bits, unsigned long require,
-                                 unsigned long exclude)
-{
-       return __compute_fixed_bits(kvm, r->bit_feat_map, r->bit_feat_map_sz,
-                                   fixed_bits, require | FIXED_VALUE, exclude);
-}
-
 void compute_fgu(struct kvm *kvm, enum fgt_group_id fgt)
 {
        u64 val = 0;

@@ -1417,7 +1397,6 @@ void compute_fgu(struct kvm *kvm, enum fgt_group_id fgt)
 
 struct resx get_reg_fixed_bits(struct kvm *kvm, enum vcpu_sysreg reg)
 {
-       u64 fixed = 0, mask;
        struct resx resx;
 
        switch (reg) {

@@ -1459,10 +1438,8 @@ struct resx get_reg_fixed_bits(struct kvm *kvm, enum vcpu_sysreg reg)
                resx.res1 |= __HCRX_EL2_RES1;
                break;
        case HCR_EL2:
-               mask = compute_reg_fixed_bits(kvm, &hcr_desc, &fixed, 0, 0).res0;
                resx = compute_reg_resx_bits(kvm, &hcr_desc, 0, 0);
-               resx.res0 |= (mask & ~fixed);
-               resx.res1 |= HCR_EL2_RES1 | (mask & fixed);
+               resx.res1 |= HCR_EL2_RES1;
                break;
        case SCTLR2_EL1:
        case SCTLR2_EL2:
--

2.47.3

A bunch of EL2 configuration are very similar to their EL1 counterpart, with the added constraint that HCR_EL2.E2H being 1.

For us, this means HCR_EL2.E2H being RES1, which is something we can statically evaluate.

Add a REQUIRES_E2H1 constraint, which allows us to express conditions in a much simpler way (without extra code). Existing occurrences are converted, before we add a lot more.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/kvm/config.c | 38 ++++++++++++++------------------------
 1 file changed, 14 insertions(+), 24 deletions(-)

arch/arm64/kvm/config.c +11 -24

index 4fac04d3132c0..1990cebc77c66 100644
@@ -25,6 +25,7 @@ struct reg_bits_to_feat_map {
 #define        FIXED_VALUE     BIT(2)  /* RAZ/WI or RAO/WI in KVM */
 #define        MASKS_POINTER   BIT(3)  /* Pointer to fgt_masks struct instead of bits */
 #define        AS_RES1         BIT(4)  /* RES1 when not supported */
+#define        REQUIRES_E2H1   BIT(5)  /* Add HCR_EL2.E2H RES1 as a pre-condition */
 
        unsigned long   flags;
 

@@ -311,21 +312,6 @@ static bool feat_trbe_mpam(struct kvm *kvm)
                (read_sysreg_s(SYS_TRBIDR_EL1) & TRBIDR_EL1_MPAM));
 }
 
-static bool feat_asid2_e2h1(struct kvm *kvm)
-{
-       return kvm_has_feat(kvm, FEAT_ASID2) && !kvm_has_feat(kvm, FEAT_E2H0);
-}
-
-static bool feat_d128_e2h1(struct kvm *kvm)
-{
-       return kvm_has_feat(kvm, FEAT_D128) && !kvm_has_feat(kvm, FEAT_E2H0);
-}
-
-static bool feat_mec_e2h1(struct kvm *kvm)
-{
-       return kvm_has_feat(kvm, FEAT_MEC) && !kvm_has_feat(kvm, FEAT_E2H0);
-}
-
 static bool feat_ebep_pmuv3_ss(struct kvm *kvm)
 {
        return kvm_has_feat(kvm, FEAT_EBEP) || kvm_has_feat(kvm, FEAT_PMUv3_SS);

@@ -1045,15 +1031,15 @@ static const DECLARE_FEAT_MAP(sctlr2_desc, SCTLR2_EL1,
                              sctlr2_feat_map, FEAT_SCTLR2);
 
 static const struct reg_bits_to_feat_map tcr2_el2_feat_map[] = {
-       NEEDS_FEAT(TCR2_EL2_FNG1        |
-                  TCR2_EL2_FNG0        |
-                  TCR2_EL2_A2,
-                  feat_asid2_e2h1),
-       NEEDS_FEAT(TCR2_EL2_DisCH1      |
-                  TCR2_EL2_DisCH0      |
-                  TCR2_EL2_D128,
-                  feat_d128_e2h1),
-       NEEDS_FEAT(TCR2_EL2_AMEC1, feat_mec_e2h1),
+       NEEDS_FEAT_FLAG(TCR2_EL2_FNG1   |
+                       TCR2_EL2_FNG0   |
+                       TCR2_EL2_A2,
+                       REQUIRES_E2H1, FEAT_ASID2),
+       NEEDS_FEAT_FLAG(TCR2_EL2_DisCH1 |
+                       TCR2_EL2_DisCH0 |
+                       TCR2_EL2_D128,
+                       REQUIRES_E2H1, FEAT_D128),
+       NEEDS_FEAT_FLAG(TCR2_EL2_AMEC1, REQUIRES_E2H1, FEAT_MEC),
        NEEDS_FEAT(TCR2_EL2_AMEC0, FEAT_MEC),
        NEEDS_FEAT(TCR2_EL2_HAFT, FEAT_HAFT),
        NEEDS_FEAT(TCR2_EL2_PTTWI       |

@@ -1285,6 +1271,7 @@ struct resx compute_resx_bits(struct kvm *kvm,
                              unsigned long require,
                              unsigned long exclude)
 {
+       bool e2h0 = kvm_has_feat(kvm, FEAT_E2H0);
        struct resx resx = {};
 
        for (int i = 0; i < map_size; i++) {

arch/arm64/kvm/config.c +0 -1

                if (!match) {
                        if (map[i].flags & AS_RES1)
                                resx.res1 |= reg_feat_map_bits(&map[i]);
--

2.47.3

"Thanks" to VHE, SCTLR_EL2 radically changes shape depending on the value of HCR_EL2.E2H, as a lot of the bits that didn't have much meaning with E2H=0 start impacting EL0 with E2H=1.

This has a direct impact on the RESx behaviour of these bits, and we need a way to express them.

For this purpose, introduce a set of 4 new constaints that, when the controlling feature is not present, force the RESx value to be either 0 or 1 depending on the value of E2H.

This allows diverging RESx values depending on the value of E2H, something that is required by a bunch of SCTLR_EL2 bits.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/kvm/config.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

arch/arm64/kvm/config.c +15 -0

index 1990cebc77c66..7063fffc22799 100644
@@ -26,6 +26,10 @@ struct reg_bits_to_feat_map {
 #define        MASKS_POINTER   BIT(3)  /* Pointer to fgt_masks struct instead of bits */
 #define        AS_RES1         BIT(4)  /* RES1 when not supported */
 #define        REQUIRES_E2H1   BIT(5)  /* Add HCR_EL2.E2H RES1 as a pre-condition */
+#define        RES0_WHEN_E2H0  BIT(6)  /* RES0 when E2H=0 and not supported */
+#define        RES0_WHEN_E2H1  BIT(7)  /* RES0 when E2H=1 and not supported */
+#define        RES1_WHEN_E2H0  BIT(8)  /* RES1 when E2H=0 and not supported */
+#define        RES1_WHEN_E2H1  BIT(9)  /* RES1 when E2H=1 and not supported */
 
        unsigned long   flags;
 

@@ -1298,10 +1302,24 @@ struct resx compute_resx_bits(struct kvm *kvm,
                        match &= !e2h0;
 
                if (!match) {
+                       u64 bits = reg_feat_map_bits(&map[i]);
+
+                       if (e2h0) {
+                               if      (map[i].flags & RES1_WHEN_E2H0)
+                                       resx.res1 |= bits;
+                               else if (map[i].flags & RES0_WHEN_E2H0)
+                                       resx.res0 |= bits;
+                       } else {
+                               if      (map[i].flags & RES1_WHEN_E2H1)
+                                       resx.res1 |= bits;
+                               else if (map[i].flags & RES0_WHEN_E2H1)

arch/arm64/kvm/config.c +0 -1

                }
        }
 
--

2.47.3

Hi Marc,

Quoted 50 lines · Marc Zyngier

"Thanks" to VHE, SCTLR_EL2 radically changes shape depending on the value of HCR_EL2.E2H, as a lot of the bits that didn't have much meaning with E2H=0 start impacting EL0 with E2H=1.

This has a direct impact on the RESx behaviour of these bits, and we need a way to express them.

For this purpose, introduce a set of 4 new constaints that, when the controlling feature is not present, force the RESx value to be either 0 or 1 depending on the value of E2H.

This allows diverging RESx values depending on the value of E2H, something that is required by a bunch of SCTLR_EL2 bits.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/kvm/config.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

arch/arm64/kvm/config.c +15 -0

index 1990cebc77c66..7063fffc22799 100644
@@ -26,6 +26,10 @@ struct reg_bits_to_feat_map {
 #define        MASKS_POINTER   BIT(3)  /* Pointer to fgt_masks struct instead of bits */
 #define        AS_RES1         BIT(4)  /* RES1 when not supported */
 #define        REQUIRES_E2H1   BIT(5)  /* Add HCR_EL2.E2H RES1 as a pre-condition */
+#define        RES0_WHEN_E2H0  BIT(6)  /* RES0 when E2H=0 and not supported */
+#define        RES0_WHEN_E2H1  BIT(7)  /* RES0 when E2H=1 and not supported */
+#define        RES1_WHEN_E2H0  BIT(8)  /* RES1 when E2H=0 and not supported */
+#define        RES1_WHEN_E2H1  BIT(9)  /* RES1 when E2H=1 and not supported */
 
        unsigned long   flags;
 

@@ -1298,10 +1302,24 @@ struct resx compute_resx_bits(struct kvm *kvm,
                        match &= !e2h0;
 
                if (!match) {
+                       u64 bits = reg_feat_map_bits(&map[i]);
+
+                       if (e2h0) {
+                               if      (map[i].flags & RES1_WHEN_E2H0)
+                                       resx.res1 |= bits;
+                               else if (map[i].flags & RES0_WHEN_E2H0)
+                                       resx.res0 |= bits;
+                       } else {
+                               if      (map[i].flags & RES1_WHEN_E2H1)
+                                       resx.res1 |= bits;
+                               else if (map[i].flags & RES0_WHEN_E2H1)

arch/arm64/kvm/config.c struct resx compute_resx_bits(struct kvm *kvm, · line 1322

Quoted 10 lines

arch/arm64/kvm/config.c +6 -3

+                                       resx.res0 |= bits;
+                       }
+
                        if (map[i].flags & AS_RES1)
-                               resx.res1 |= reg_feat_map_bits(&map[i]);
-                       else
-                               resx.res0 |= reg_feat_map_bits(&map[i]);
+                               resx.res1 |= bits;
+                       else if (!(resx.res1 & bits))
+                               resx.res0 |= bits;

The logic here feels a bit more complex than necessary, specifically regarding the interaction between the E2H checks and the fallthrough to AS_RES1.

Although AS_RES1 and RES0_WHEN_E2H0 are mutually exclusive in practice, the current structure technically permits a scenario where both res0 and res1 get set if the flags are mixed (the e2h0 block sets res0, and the AS_RES1 block falls through and sets res1). This cannot be ruled out by looking at this function alone.

It might be cleaner (and safer) to determine the res1 first, and
then apply the masks. Something like:

This also brings up a side point: given the visual similarity of these flags, it is quite easy to make a typo and accidentally combine incompatible flags (e.g., AS_RES1 | RESx_WHEN_E2Hx, or RES0_WHEN_E2H0 | RES1_WHEN_E2H0), would it be worth adding a check to warn on obviously invalid combinations?

Or maybe even redefining AS_RES1 to be (RES1_WHEN_E2H1|RES1_WHEN_E2H0), which is what it is conceptually. That could simplify this code even further:

Hey Fuad,

Quoted 78 lines · Fuad Tabba

Hi Marc,

Quoted 50 lines · Marc Zyngier

"Thanks" to VHE, SCTLR_EL2 radically changes shape depending on the value of HCR_EL2.E2H, as a lot of the bits that didn't have much meaning with E2H=0 start impacting EL0 with E2H=1.

This has a direct impact on the RESx behaviour of these bits, and we need a way to express them.

For this purpose, introduce a set of 4 new constaints that, when the controlling feature is not present, force the RESx value to be either 0 or 1 depending on the value of E2H.

This allows diverging RESx values depending on the value of E2H, something that is required by a bunch of SCTLR_EL2 bits.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/kvm/config.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

arch/arm64/kvm/config.c +15 -0

index 1990cebc77c66..7063fffc22799 100644
@@ -26,6 +26,10 @@ struct reg_bits_to_feat_map {
 #define        MASKS_POINTER   BIT(3)  /* Pointer to fgt_masks struct instead of bits */
 #define        AS_RES1         BIT(4)  /* RES1 when not supported */
 #define        REQUIRES_E2H1   BIT(5)  /* Add HCR_EL2.E2H RES1 as a pre-condition */
+#define        RES0_WHEN_E2H0  BIT(6)  /* RES0 when E2H=0 and not supported */
+#define        RES0_WHEN_E2H1  BIT(7)  /* RES0 when E2H=1 and not supported */
+#define        RES1_WHEN_E2H0  BIT(8)  /* RES1 when E2H=0 and not supported */
+#define        RES1_WHEN_E2H1  BIT(9)  /* RES1 when E2H=1 and not supported */
 
        unsigned long   flags;
 

@@ -1298,10 +1302,24 @@ struct resx compute_resx_bits(struct kvm *kvm,
                        match &= !e2h0;
 
                if (!match) {
+                       u64 bits = reg_feat_map_bits(&map[i]);
+
+                       if (e2h0) {
+                               if      (map[i].flags & RES1_WHEN_E2H0)
+                                       resx.res1 |= bits;
+                               else if (map[i].flags & RES0_WHEN_E2H0)
+                                       resx.res0 |= bits;
+                       } else {
+                               if      (map[i].flags & RES1_WHEN_E2H1)
+                                       resx.res1 |= bits;
+                               else if (map[i].flags & RES0_WHEN_E2H1)

arch/arm64/kvm/config.c struct resx compute_resx_bits(struct kvm *kvm, · line 1322

Quoted 10 lines

arch/arm64/kvm/config.c +6 -3

+                                       resx.res0 |= bits;
+                       }
+
                        if (map[i].flags & AS_RES1)
-                               resx.res1 |= reg_feat_map_bits(&map[i]);
-                       else
-                               resx.res0 |= reg_feat_map_bits(&map[i]);
+                               resx.res1 |= bits;
+                       else if (!(resx.res1 & bits))
+                               resx.res0 |= bits;

The logic here feels a bit more complex than necessary, specifically regarding the interaction between the E2H checks and the fallthrough to AS_RES1.

Although AS_RES1 and RES0_WHEN_E2H0 are mutually exclusive in practice, the current structure technically permits a scenario where both res0 and res1 get set if the flags are mixed (the e2h0 block sets res0, and the AS_RES1 block falls through and sets res1). This cannot be ruled out by looking at this function alone.

It might be cleaner (and safer) to determine the res1 first, and
then apply the masks. Something like:

Review

Quoted 9 lines

(quoted diff) +8 -0

+                       bool is_res1 = false;
+
+                       if (map[i].flags & AS_RES1)
+                               is_res1 = true;
+                       else if (e2h0)
+                               is_res1 = (map[i].flags & RES1_WHEN_E2H0);
+                       else
+                               is_res1 = (map[i].flags & RES1_WHEN_E2H1);
...

I think you have just put your finger on something that escaped me so far. You are totally right that the code as written today is ugly, and the trick to work out that we need to account the bits as RES0 is awful.

But it additionally outlines something else: since RES0 is an implicit property (we don't specify a flag for it), RES0_WHEN_E2Hx could also be implicit properties. I couldn't find an example where anything would break. This would also avoid the combination with AS_RES1 by construction.

Quoted 11 lines

This also brings up a side point: given the visual similarity of these flags, it is quite easy to make a typo and accidentally combine incompatible flags (e.g., AS_RES1 | RESx_WHEN_E2Hx, or RES0_WHEN_E2H0 | RES1_WHEN_E2H0), would it be worth adding a check to warn on obviously invalid combinations?

Or maybe even redefining AS_RES1 to be (RES1_WHEN_E2H1|RES1_WHEN_E2H0), which is what it is conceptually. That could simplify this code even further:

Review

Quoted 4 lines

(quoted diff) +4 -0

+                       if (e2h0)
+                               is_res1 = (map[i].flags & RES1_WHEN_E2H0);
+                       else
+                               is_res1 = (map[i].flags & RES1_WHEN_E2H1);

While that would work, I think this is a step too far. Eventually, we should be able to sanitise things outside of NV, and RES1 should not depend on E2H at all in this case.

I ended up with the following hack, completely untested (needs renumbering, and the rest of SCTLR_EL2 repainted). Let me know what you think.

Thanks,

        M.

arch/arm64/kvm/config.c +2 -15

index 562513a4683e2..204e5aeda4d24 100644
@@ -26,8 +26,6 @@ struct reg_bits_to_feat_map {
 #define        MASKS_POINTER   BIT(3)  /* Pointer to fgt_masks struct instead of bits */
 #define        AS_RES1         BIT(4)  /* RES1 when not supported */
 #define        REQUIRES_E2H1   BIT(5)  /* Add HCR_EL2.E2H RES1 as a pre-condition */
-#define        RES0_WHEN_E2H0  BIT(6)  /* RES0 when E2H=0 and not supported */
-#define        RES0_WHEN_E2H1  BIT(7)  /* RES0 when E2H=1 and not supported */
 #define        RES1_WHEN_E2H0  BIT(8)  /* RES1 when E2H=0 and not supported */
 #define        RES1_WHEN_E2H1  BIT(9)  /* RES1 when E2H=1 and not supported */
 

@@ -1375,22 +1373,15 @@ struct resx compute_resx_bits(struct kvm *kvm,
 
                if (!match) {
                        u64 bits = reg_feat_map_bits(&map[i]);
+                       bool res1;
 
-                       if (e2h0) {
-                               if      (map[i].flags & RES1_WHEN_E2H0)
-                                       resx.res1 |= bits;
-                               else if (map[i].flags & RES0_WHEN_E2H0)
-                                       resx.res0 |= bits;
-                       } else {
-                               if      (map[i].flags & RES1_WHEN_E2H1)
-                                       resx.res1 |= bits;
-                               else if (map[i].flags & RES0_WHEN_E2H1)
-                                       resx.res0 |= bits;
-                       }
-
-                       if (map[i].flags & AS_RES1)
+                       res1  = (map[i].flags & AS_RES1);

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

Instead of hacking the RES1 bits at runtime, move them into the register descriptors. This makes it significantly nicer.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/kvm/config.c | 36 +++++++++++++++++++++++++++++-------
 1 file changed, 29 insertions(+), 7 deletions(-)

arch/arm64/kvm/config.c +1 -0

index 7063fffc22799..d5871758f1fcc 100644
@@ -30,6 +30,7 @@ struct reg_bits_to_feat_map {
 #define        RES0_WHEN_E2H1  BIT(7)  /* RES0 when E2H=1 and not supported */
 #define        RES1_WHEN_E2H0  BIT(8)  /* RES1 when E2H=0 and not supported */
 #define        RES1_WHEN_E2H1  BIT(9)  /* RES1 when E2H=1 and not supported */
+#define        FORCE_RESx      BIT(10) /* Unconditional RESx */
 
        unsigned long   flags;
 

@@ -107,6 +108,11 @@ struct reg_feat_map_desc {
  */

arch/arm64/kvm/config.c +15 -8

+
 static bool not_feat_aa64el3(struct kvm *kvm)
 {
        return !kvm_has_feat(kvm, FEAT_AA64EL3);

@@ -1009,6 +1024,8 @@ static const struct reg_bits_to_feat_map hcr_feat_map[] = {
                   HCR_EL2_TWEDEn,
                   FEAT_TWED),
        NEEDS_FEAT_FIXED(HCR_EL2_E2H, compute_hcr_e2h),
+       FORCE_RES0(HCR_EL2_RES0),
+       FORCE_RES1(HCR_EL2_RES1),
 };
 
 static const DECLARE_FEAT_MAP(hcr_desc, HCR_EL2,

@@ -1029,6 +1046,8 @@ static const struct reg_bits_to_feat_map sctlr2_feat_map[] = {
                   SCTLR2_EL1_CPTM      |
                   SCTLR2_EL1_CPTM0,
                   FEAT_CPA2),
+       FORCE_RES0(SCTLR2_EL1_RES0),
+       FORCE_RES1(SCTLR2_EL1_RES1),
 };
 
 static const DECLARE_FEAT_MAP(sctlr2_desc, SCTLR2_EL1,

@@ -1054,6 +1073,8 @@ static const struct reg_bits_to_feat_map tcr2_el2_feat_map[] = {
                   TCR2_EL2_E0POE,
                   FEAT_S1POE),
        NEEDS_FEAT(TCR2_EL2_PIE, FEAT_S1PIE),
+       FORCE_RES0(TCR2_EL2_RES0),
+       FORCE_RES1(TCR2_EL2_RES1),
 };
 
 static const DECLARE_FEAT_MAP(tcr2_el2_desc, TCR2_EL2,

@@ -1131,6 +1152,8 @@ static const struct reg_bits_to_feat_map sctlr_el1_feat_map[] = {
                   SCTLR_EL1_A          |
                   SCTLR_EL1_M,
                   FEAT_AA64EL1),
+       FORCE_RES0(SCTLR_EL1_RES0),
+       FORCE_RES1(SCTLR_EL1_RES1),
 };
 
 static const DECLARE_FEAT_MAP(sctlr_el1_desc, SCTLR_EL1,

@@ -1165,6 +1188,8 @@ static const struct reg_bits_to_feat_map mdcr_el2_feat_map[] = {
                   MDCR_EL2_TDE         |
                   MDCR_EL2_TDRA,
                   FEAT_AA64EL1),
+       FORCE_RES0(MDCR_EL2_RES0),
+       FORCE_RES1(MDCR_EL2_RES1),
 };
 
 static const DECLARE_FEAT_MAP(mdcr_el2_desc, MDCR_EL2,

@@ -1203,6 +1228,8 @@ static const struct reg_bits_to_feat_map vtcr_el2_feat_map[] = {
                   VTCR_EL2_SL0         |
                   VTCR_EL2_T0SZ,
                   FEAT_AA64EL1),
+       FORCE_RES0(VTCR_EL2_RES0),
+       FORCE_RES1(VTCR_EL2_RES1),
 };
 
 static const DECLARE_FEAT_MAP(vtcr_el2_desc, VTCR_EL2,

@@ -1214,7 +1241,8 @@ static void __init check_feat_map(const struct reg_bits_to_feat_map *map,
        u64 mask = 0;
 
        for (int i = 0; i < map_size; i++)
-               mask |= map[i].bits;
+               if (!(map[i].flags & FORCE_RESx))
+                       mask |= map[i].bits;
 
        if (mask != ~resx)
                kvm_err("Undefined %s behaviour, bits %016llx\n",

@@ -1447,28 +1475,22 @@ struct resx get_reg_fixed_bits(struct kvm *kvm, enum vcpu_sysreg reg)
                break;
        case HCR_EL2:
                resx = compute_reg_resx_bits(kvm, &hcr_desc, 0, 0);
-               resx.res1 |= HCR_EL2_RES1;
                break;
        case SCTLR2_EL1:
        case SCTLR2_EL2:
                resx = compute_reg_resx_bits(kvm, &sctlr2_desc, 0, 0);
-               resx.res1 |= SCTLR2_EL1_RES1;
                break;
        case TCR2_EL2:
                resx = compute_reg_resx_bits(kvm, &tcr2_el2_desc, 0, 0);
-               resx.res1 |= TCR2_EL2_RES1;
                break;
        case SCTLR_EL1:
                resx = compute_reg_resx_bits(kvm, &sctlr_el1_desc, 0, 0);
-               resx.res1 |= SCTLR_EL1_RES1;
                break;
        case MDCR_EL2:
                resx = compute_reg_resx_bits(kvm, &mdcr_el2_desc, 0, 0);
-               resx.res1 |= MDCR_EL2_RES1;
                break;
        case VTCR_EL2:
                resx = compute_reg_resx_bits(kvm, &vtcr_el2_desc, 0, 0);
-               resx.res1 |= VTCR_EL2_RES1;
                break;
        default:
                WARN_ON_ONCE(1);
--

2.47.3

Hi Marc,

Quoted 23 lines · Marc Zyngier

Instead of hacking the RES1 bits at runtime, move them into the register descriptors. This makes it significantly nicer.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/kvm/config.c | 36 +++++++++++++++++++++++++++++-------
 1 file changed, 29 insertions(+), 7 deletions(-)

arch/arm64/kvm/config.c +1 -0

index 7063fffc22799..d5871758f1fcc 100644
@@ -30,6 +30,7 @@ struct reg_bits_to_feat_map {
 #define        RES0_WHEN_E2H1  BIT(7)  /* RES0 when E2H=1 and not supported */
 #define        RES1_WHEN_E2H0  BIT(8)  /* RES1 when E2H=0 and not supported */
 #define        RES1_WHEN_E2H1  BIT(9)  /* RES1 when E2H=1 and not supported */
+#define        FORCE_RESx      BIT(10) /* Unconditional RESx */
 
        unsigned long   flags;
 

@@ -107,6 +108,11 @@ struct reg_feat_map_desc {
  */

arch/arm64/kvm/config.c struct reg_feat_map_desc { · line 118

Quoted 10 lines

arch/arm64/kvm/config.c +5 -0

 #define NEEDS_FEAT(m, ...)     NEEDS_FEAT_FLAG(m, 0, __VA_ARGS__)
 
+/* Declare fixed RESx bits */
+#define FORCE_RES0(m)          NEEDS_FEAT_FLAG(m, FORCE_RESx, enforce_resx)
+#define FORCE_RES1(m)          NEEDS_FEAT_FLAG(m, FORCE_RESx | AS_RES1, \
+                                               enforce_resx)
+
 /*
  * Declare the dependency between a non-FGT register, a set of
  * feature, and the set of individual bits it contains. This generates

nit: features

arch/arm64/kvm/config.c struct reg_feat_map_desc { · line 243

Quoted 9 lines

arch/arm64/kvm/config.c +5 -0

@@ -230,6 +236,15 @@ struct reg_feat_map_desc {
 #define FEAT_HCX               ID_AA64MMFR1_EL1, HCX, IMP
 #define FEAT_S2PIE             ID_AA64MMFR3_EL1, S2PIE, IMP
 
+static bool enforce_resx(struct kvm *kvm)
+{
+       /*
+        * Returning false here means that the RESx bits will be always
+        * addded to the fixed set bit. Yes, this is counter-intuitive.

nit: added

arch/arm64/kvm/config.c struct reg_feat_map_desc { · line 246

Quoted 3 lines

arch/arm64/kvm/config.c +3 -0

+        */
+       return false;
+}

I see what you're doing here, but it took me a while to get it and convince myself that there aren't any bugs (my self couldn't find any bugs, but I wouldn't trust him that much). You already introduce a new flag, FORCE_RESx. Why not just check that directly in the compute_resx_bits() loop, before the check for CALL_FUNC?

+ if (map[i].flags & FORCE_RESx)
+ match = false;
+ else if (map[i].flags & CALL_FUNC)
...

The way it is now, to understand FORCE_RES0, you must trace a flag, a macro expansion, and a function pointer, just to set a boolean to false.

Quoted 6 lines

I see what you're doing here, but it took me a while to get it and convince myself that there aren't any bugs (my self couldn't find any bugs, but I wouldn't trust him that much). You already introduce a new flag, FORCE_RESx. Why not just check that directly in the compute_resx_bits() loop, before the check for CALL_FUNC?

Quoted 8 lines

+ if (map[i].flags & FORCE_RESx)
+ match = false;
+ else if (map[i].flags & CALL_FUNC)
...

The way it is now, to understand FORCE_RES0, you must trace a flag, a macro expansion, and a function pointer, just to set a boolean to false.

With that scheme, you'd write something like:

+#define FORCE_RES0(m) NEEDS_FEAT_FLAG(m, FORCE_RESx)

This construct would need a new __NEEDS_FEAT_0() macro that doesn't take any argument other than flags. Something like below (untested).

        M.

arch/arm64/kvm/config.c +12 -15

index 9485e1f2dc0b7..364bdd1e5be51 100644
@@ -79,6 +79,12 @@ struct reg_feat_map_desc {
                .match = (fun),                         \
        }
 
+#define __NEEDS_FEAT_0(m, f, w, ...)                   \
+       {                                               \
+               .w      = (m),                          \
+               .flags = (f),                           \
+       }
+
 #define __NEEDS_FEAT_FLAG(m, f, w, ...)                        \
        CONCATENATE(__NEEDS_FEAT_, COUNT_ARGS(__VA_ARGS__))(m, f, w, __VA_ARGS__)
 

@@ -95,9 +101,8 @@ struct reg_feat_map_desc {
 #define NEEDS_FEAT(m, ...)     NEEDS_FEAT_FLAG(m, 0, __VA_ARGS__)
 
 /* Declare fixed RESx bits */
-#define FORCE_RES0(m)          NEEDS_FEAT_FLAG(m, FORCE_RESx, enforce_resx)
-#define FORCE_RES1(m)          NEEDS_FEAT_FLAG(m, FORCE_RESx | AS_RES1, \
-                                               enforce_resx)
+#define FORCE_RES0(m)          NEEDS_FEAT_FLAG(m, FORCE_RESx)
+#define FORCE_RES1(m)          NEEDS_FEAT_FLAG(m, FORCE_RESx | AS_RES1)
 
 /*
  * Declare the dependency between a non-FGT register, a set of

@@ -221,15 +226,6 @@ struct reg_feat_map_desc {
 #define FEAT_HCX               ID_AA64MMFR1_EL1, HCX, IMP
 #define FEAT_S2PIE             ID_AA64MMFR3_EL1, S2PIE, IMP
 
-static bool enforce_resx(struct kvm *kvm)
-{
-       /*
-        * Returning false here means that the RESx bits will be always
-        * addded to the fixed set bit. Yes, this is counter-intuitive.
-        */
-       return false;
-}
-
 static bool not_feat_aa64el3(struct kvm *kvm)
 {
        return !kvm_has_feat(kvm, FEAT_AA64EL3);

@@ -996,7 +992,7 @@ static const struct reg_bits_to_feat_map hcr_feat_map[] = {
        NEEDS_FEAT(HCR_EL2_TWEDEL       |
                   HCR_EL2_TWEDEn,
                   FEAT_TWED),
-       NEEDS_FEAT_FLAG(HCR_EL2_E2H, RES1_WHEN_E2H1, enforce_resx),
+       NEEDS_FEAT_FLAG(HCR_EL2_E2H, RES1_WHEN_E2H1 | FORCE_RESx),
        FORCE_RES0(HCR_EL2_RES0),
        FORCE_RES1(HCR_EL2_RES1),
 };

@@ -1362,7 +1358,9 @@ struct resx compute_resx_bits(struct kvm *kvm,
                if (map[i].flags & exclude)
                        continue;
 
-               if (map[i].flags & CALL_FUNC)
+               if (map[i].flags & FORCE_RESx)
+                       match = false;
+               else if (map[i].flags & CALL_FUNC)
                        match = map[i].match(kvm);
                else
                        match = idreg_feat_match(kvm, &map[i]);
 
--

Without deviation from the norm, progress is not possible.

Quoted 62 lines · Fuad Tabba

Hi Marc,

Quoted 23 lines · Marc Zyngier

Instead of hacking the RES1 bits at runtime, move them into the register descriptors. This makes it significantly nicer.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/kvm/config.c | 36 +++++++++++++++++++++++++++++-------
 1 file changed, 29 insertions(+), 7 deletions(-)

arch/arm64/kvm/config.c +1 -0

index 7063fffc22799..d5871758f1fcc 100644
@@ -30,6 +30,7 @@ struct reg_bits_to_feat_map {
 #define        RES0_WHEN_E2H1  BIT(7)  /* RES0 when E2H=1 and not supported */
 #define        RES1_WHEN_E2H0  BIT(8)  /* RES1 when E2H=0 and not supported */
 #define        RES1_WHEN_E2H1  BIT(9)  /* RES1 when E2H=1 and not supported */
+#define        FORCE_RESx      BIT(10) /* Unconditional RESx */
 
        unsigned long   flags;
 

@@ -107,6 +108,11 @@ struct reg_feat_map_desc {
  */

arch/arm64/kvm/config.c struct reg_feat_map_desc { · line 118

Quoted 10 lines

arch/arm64/kvm/config.c +5 -0

 #define NEEDS_FEAT(m, ...)     NEEDS_FEAT_FLAG(m, 0, __VA_ARGS__)
 
+/* Declare fixed RESx bits */
+#define FORCE_RES0(m)          NEEDS_FEAT_FLAG(m, FORCE_RESx, enforce_resx)
+#define FORCE_RES1(m)          NEEDS_FEAT_FLAG(m, FORCE_RESx | AS_RES1, \
+                                               enforce_resx)
+
 /*
  * Declare the dependency between a non-FGT register, a set of
  * feature, and the set of individual bits it contains. This generates

nit: features

arch/arm64/kvm/config.c struct reg_feat_map_desc { · line 243

Quoted 9 lines

arch/arm64/kvm/config.c +5 -0

@@ -230,6 +236,15 @@ struct reg_feat_map_desc {
 #define FEAT_HCX               ID_AA64MMFR1_EL1, HCX, IMP
 #define FEAT_S2PIE             ID_AA64MMFR3_EL1, S2PIE, IMP
 
+static bool enforce_resx(struct kvm *kvm)
+{
+       /*
+        * Returning false here means that the RESx bits will be always
+        * addded to the fixed set bit. Yes, this is counter-intuitive.

nit: added

arch/arm64/kvm/config.c struct reg_feat_map_desc { · line 246

Quoted 3 lines

arch/arm64/kvm/config.c +3 -0

+        */
+       return false;
+}

I see what you're doing here, but it took me a while to get it and convince myself that there aren't any bugs (my self couldn't find any bugs, but I wouldn't trust him that much). You already introduce a new flag, FORCE_RESx. Why not just check that directly in the compute_resx_bits() loop, before the check for CALL_FUNC?

Quoted 8 lines

+ if (map[i].flags & FORCE_RESx)
+ match = false;
+ else if (map[i].flags & CALL_FUNC)
...

The way it is now, to understand FORCE_RES0, you must trace a flag, a macro expansion, and a function pointer, just to set a boolean to false.

With that scheme, you'd write something like:

+#define FORCE_RES0(m) NEEDS_FEAT_FLAG(m, FORCE_RESx)

This construct would need a new __NEEDS_FEAT_0() macro that doesn't take any argument other than flags. Something like below (untested).

	M.

arch/arm64/kvm/config.c +8 -12

index 9485e1f2dc0b7..364bdd1e5be51 100644
@@ -79,6 +79,12 @@ struct reg_feat_map_desc {
 		.match = (fun),				\
 	}
 
+#define __NEEDS_FEAT_0(m, f, w, ...)			\
+	{						\
+		.w	= (m),				\
+		.flags = (f),				\
+	}
+
 #define __NEEDS_FEAT_FLAG(m, f, w, ...)			\
 	CONCATENATE(__NEEDS_FEAT_, COUNT_ARGS(__VA_ARGS__))(m, f, w, __VA_ARGS__)
 

@@ -95,9 +101,8 @@ struct reg_feat_map_desc {
 #define NEEDS_FEAT(m, ...)	NEEDS_FEAT_FLAG(m, 0, __VA_ARGS__)
 
 /* Declare fixed RESx bits */
-#define FORCE_RES0(m)		NEEDS_FEAT_FLAG(m, FORCE_RESx, enforce_resx)
-#define FORCE_RES1(m)		NEEDS_FEAT_FLAG(m, FORCE_RESx | AS_RES1, \
-						enforce_resx)
+#define FORCE_RES0(m)		NEEDS_FEAT_FLAG(m, FORCE_RESx)
+#define FORCE_RES1(m)		NEEDS_FEAT_FLAG(m, FORCE_RESx | AS_RES1)
 
 /*
  * Declare the dependency between a non-FGT register, a set of

@@ -221,15 +226,6 @@ struct reg_feat_map_desc {
 #define FEAT_HCX		ID_AA64MMFR1_EL1, HCX, IMP
 #define FEAT_S2PIE		ID_AA64MMFR3_EL1, S2PIE, IMP
 
-static bool enforce_resx(struct kvm *kvm)
-{
-	/*
-	 * Returning false here means that the RESx bits will be always
-	 * addded to the fixed set bit. Yes, this is counter-intuitive.
-	 */
-	return false;
-}
-
 static bool not_feat_aa64el3(struct kvm *kvm)
 {
 	return !kvm_has_feat(kvm, FEAT_AA64EL3);