| Message ID | 20250210184150.2145093-8-maz@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | KVM: arm64: Revamp Fine Grained Trap handling | expand |
Hi Marc, On Mon, 10 Feb 2025 at 18:42, Marc Zyngier <maz@kernel.org> wrote: > > In the process of decoupling KVM's view of the FGT bits from the > wider architectural state, use KVM's own FGT tables to build > a synthitic view of what is actually known. synthitic -> synthetic > This allows for some checking along the way. > > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > arch/arm64/include/asm/kvm_arm.h | 4 ++ > arch/arm64/include/asm/kvm_host.h | 14 ++++ > arch/arm64/kvm/emulate-nested.c | 102 ++++++++++++++++++++++++++++++ > 3 files changed, 120 insertions(+) > > diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h > index 8d94a6c0ed5c4..e424085f2aaca 100644 > --- a/arch/arm64/include/asm/kvm_arm.h > +++ b/arch/arm64/include/asm/kvm_arm.h > @@ -359,6 +359,10 @@ > #define __HAFGRTR_EL2_MASK (GENMASK(49, 17) | GENMASK(4, 0)) > #define __HAFGRTR_EL2_nMASK ~(__HAFGRTR_EL2_RES0 | __HAFGRTR_EL2_MASK) > > +/* Because the sysreg file mixes R and W... */ > +#define HFGRTR_EL2_RES0 HFGxTR_EL2_RES0 (0) > +#define HFGWTR_EL2_RES0 (HFGRTR_EL2_RES0 | __HFGRTR_ONLY_MASK) __HFGRTR_ONLY_MASK is a hand-crafted bitmask. The only bit remaining in HFGxTR_EL2 that is RES0 is bit 51. If that were to be used as an HFGRTR-only bit without __HFGRTR_ONLY_MASK getting updated, then aggregate_fgt() below would set its bit in hfgwtr_masks. Could this be a problem if this happens and the polarity of this bit ends up being negative, thereby setting the corresponding nmask bit? Cheers, /fuad > + > /* Similar definitions for HCRX_EL2 */ > #define __HCRX_EL2_RES0 HCRX_EL2_RES0 > #define __HCRX_EL2_MASK (BIT(6)) > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index 7cfa024de4e34..4e67d4064f409 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -569,6 +569,20 @@ struct kvm_sysreg_masks { > } mask[NR_SYS_REGS - __SANITISED_REG_START__]; > }; > > +struct fgt_masks { > + const char *str; > + u64 mask; > + u64 nmask; > + u64 res0; > +}; > + > +extern struct fgt_masks hfgrtr_masks; > +extern struct fgt_masks hfgwtr_masks; > +extern struct fgt_masks hfgitr_masks; > +extern struct fgt_masks hdfgrtr_masks; > +extern struct fgt_masks hdfgwtr_masks; > +extern struct fgt_masks hafgrtr_masks; > + > struct kvm_cpu_context { > struct user_pt_regs regs; /* sp = sp_el0 */ > > diff --git a/arch/arm64/kvm/emulate-nested.c b/arch/arm64/kvm/emulate-nested.c > index 607d37bab70b4..bbfe89c37a86e 100644 > --- a/arch/arm64/kvm/emulate-nested.c > +++ b/arch/arm64/kvm/emulate-nested.c > @@ -2033,6 +2033,101 @@ static u32 encoding_next(u32 encoding) > return sys_reg(op0 + 1, 0, 0, 0, 0); > } > > +#define FGT_MASKS(__n, __m) \ > + struct fgt_masks __n = { .str = #__m, .res0 = __m, } > + > +FGT_MASKS(hfgrtr_masks, HFGRTR_EL2_RES0); > +FGT_MASKS(hfgwtr_masks, HFGWTR_EL2_RES0); > +FGT_MASKS(hfgitr_masks, HFGITR_EL2_RES0); > +FGT_MASKS(hdfgrtr_masks, HDFGRTR_EL2_RES0); > +FGT_MASKS(hdfgwtr_masks, HDFGWTR_EL2_RES0); > +FGT_MASKS(hafgrtr_masks, HAFGRTR_EL2_RES0); > + > +static __init bool aggregate_fgt(union trap_config tc) > +{ > + struct fgt_masks *rmasks, *wmasks; > + > + switch (tc.fgt) { > + case HFGxTR_GROUP: > + rmasks = &hfgrtr_masks; > + wmasks = &hfgwtr_masks; > + break; > + case HDFGRTR_GROUP: > + rmasks = &hdfgrtr_masks; > + wmasks = &hdfgwtr_masks; > + break; > + case HAFGRTR_GROUP: > + rmasks = &hafgrtr_masks; > + wmasks = NULL; > + break; > + case HFGITR_GROUP: > + rmasks = &hfgitr_masks; > + wmasks = NULL; > + break; > + } > + > + /* > + * A bit can be reserved in either the R or W register, but > + * not both. > + */ > + if ((BIT(tc.bit) & rmasks->res0) && > + (!wmasks || (BIT(tc.bit) & wmasks->res0))) > + return false; > + > + if (tc.pol) > + rmasks->mask |= BIT(tc.bit) & ~rmasks->res0; > + else > + rmasks->nmask |= BIT(tc.bit) & ~rmasks->res0; > + > + if (wmasks) { > + if (tc.pol) > + wmasks->mask |= BIT(tc.bit) & ~wmasks->res0; > + else > + wmasks->nmask |= BIT(tc.bit) & ~wmasks->res0; > + } > + > + return true; > +} > + > +static __init int check_fgt_masks(struct fgt_masks *masks) > +{ > + unsigned long duplicate = masks->mask & masks->nmask; > + u64 res0 = masks->res0; > + int ret = 0; > + > + if (duplicate) { > + int i; > + > + for_each_set_bit(i, &duplicate, 64) { > + kvm_err("%s[%d] bit has both polarities\n", > + masks->str, i); > + } > + > + ret = -EINVAL; > + } > + > + masks->res0 = ~(masks->mask | masks->nmask); > + if (masks->res0 != res0) > + kvm_info("Implicit %s = %016llx, expecting %016llx\n", > + masks->str, masks->res0, res0); > + > + return ret; > +} > + > +static __init int check_all_fgt_masks(int ret) > +{ > + int err = 0; > + > + err |= check_fgt_masks(&hfgrtr_masks); > + err |= check_fgt_masks(&hfgwtr_masks); > + err |= check_fgt_masks(&hfgitr_masks); > + err |= check_fgt_masks(&hdfgrtr_masks); > + err |= check_fgt_masks(&hdfgwtr_masks); > + err |= check_fgt_masks(&hafgrtr_masks); > + > + return ret ?: err; > +} > + > int __init populate_nv_trap_config(void) > { > int ret = 0; > @@ -2097,8 +2192,15 @@ int __init populate_nv_trap_config(void) > ret = xa_err(prev); > print_nv_trap_error(fgt, "Failed FGT insertion", ret); > } > + > + if (!aggregate_fgt(tc)) { > + ret = -EINVAL; > + print_nv_trap_error(fgt, "FGT bit is reserved", ret); > + } > } > > + ret = check_all_fgt_masks(ret); > + > kvm_info("nv: %ld fine grained trap handlers\n", > ARRAY_SIZE(encoding_to_fgt)); > > -- > 2.39.2 >
On Tue, 04 Mar 2025 16:55:50 +0000, Fuad Tabba <tabba@google.com> wrote: > > Hi Marc, > > On Mon, 10 Feb 2025 at 18:42, Marc Zyngier <maz@kernel.org> wrote: > > > > In the process of decoupling KVM's view of the FGT bits from the > > wider architectural state, use KVM's own FGT tables to build > > a synthitic view of what is actually known. > > synthitic -> synthetic > > > > This allows for some checking along the way. > > > > Signed-off-by: Marc Zyngier <maz@kernel.org> > > --- > > arch/arm64/include/asm/kvm_arm.h | 4 ++ > > arch/arm64/include/asm/kvm_host.h | 14 ++++ > > arch/arm64/kvm/emulate-nested.c | 102 ++++++++++++++++++++++++++++++ > > 3 files changed, 120 insertions(+) > > > > diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h > > index 8d94a6c0ed5c4..e424085f2aaca 100644 > > --- a/arch/arm64/include/asm/kvm_arm.h > > +++ b/arch/arm64/include/asm/kvm_arm.h > > @@ -359,6 +359,10 @@ > > #define __HAFGRTR_EL2_MASK (GENMASK(49, 17) | GENMASK(4, 0)) > > #define __HAFGRTR_EL2_nMASK ~(__HAFGRTR_EL2_RES0 | __HAFGRTR_EL2_MASK) > > > > +/* Because the sysreg file mixes R and W... */ > > +#define HFGRTR_EL2_RES0 HFGxTR_EL2_RES0 (0) > > +#define HFGWTR_EL2_RES0 (HFGRTR_EL2_RES0 | __HFGRTR_ONLY_MASK) > > __HFGRTR_ONLY_MASK is a hand-crafted bitmask. The only bit remaining > in HFGxTR_EL2 that is RES0 is bit 51. If that were to be used as an > HFGRTR-only bit without __HFGRTR_ONLY_MASK getting updated, then > aggregate_fgt() below would set its bit in hfgwtr_masks. Could this be > a problem if this happens and the polarity of this bit ends up being > negative, thereby setting the corresponding nmask bit? This could become a problem indeed. But the only fix for that is to kill the HFGxTR stupidity and describe all the bits as needed so that we stop assuming things. I'm half tempted to do that next. Thanks, M.
On Tue, 04 Mar 2025 16:55:50 +0000, Fuad Tabba <tabba@google.com> wrote: > > Hi Marc, > > On Mon, 10 Feb 2025 at 18:42, Marc Zyngier <maz@kernel.org> wrote: > > > > In the process of decoupling KVM's view of the FGT bits from the > > wider architectural state, use KVM's own FGT tables to build > > a synthitic view of what is actually known. > > synthitic -> synthetic Ah, I missed that one earlier. Will fix. > > > > This allows for some checking along the way. > > > > Signed-off-by: Marc Zyngier <maz@kernel.org> > > --- > > arch/arm64/include/asm/kvm_arm.h | 4 ++ > > arch/arm64/include/asm/kvm_host.h | 14 ++++ > > arch/arm64/kvm/emulate-nested.c | 102 ++++++++++++++++++++++++++++++ > > 3 files changed, 120 insertions(+) > > > > diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h > > index 8d94a6c0ed5c4..e424085f2aaca 100644 > > --- a/arch/arm64/include/asm/kvm_arm.h > > +++ b/arch/arm64/include/asm/kvm_arm.h > > @@ -359,6 +359,10 @@ > > #define __HAFGRTR_EL2_MASK (GENMASK(49, 17) | GENMASK(4, 0)) > > #define __HAFGRTR_EL2_nMASK ~(__HAFGRTR_EL2_RES0 | __HAFGRTR_EL2_MASK) > > > > +/* Because the sysreg file mixes R and W... */ > > +#define HFGRTR_EL2_RES0 HFGxTR_EL2_RES0 (0) > > +#define HFGWTR_EL2_RES0 (HFGRTR_EL2_RES0 | __HFGRTR_ONLY_MASK) > > __HFGRTR_ONLY_MASK is a hand-crafted bitmask. The only bit remaining > in HFGxTR_EL2 that is RES0 is bit 51. If that were to be used as an > HFGRTR-only bit without __HFGRTR_ONLY_MASK getting updated, then > aggregate_fgt() below would set its bit in hfgwtr_masks. Could this be > a problem if this happens and the polarity of this bit ends up being > negative, thereby setting the corresponding nmask bit? So I ended up doing exactly what I threatened to do, which is to completely get rid of the HFGxTR nonsense, and bring HFG{R,W}TR to their full glory. The diffstat is a bit annoying: arch/arm64/include/asm/el2_setup.h | 14 +-- arch/arm64/include/asm/kvm_arm.h | 4 +- arch/arm64/include/asm/kvm_host.h | 3 +- arch/arm64/kvm/emulate-nested.c | 154 ++++++++++++------------- arch/arm64/kvm/hyp/include/hyp/switch.h | 4 +- arch/arm64/kvm/hyp/vgic-v3-sr.c | 8 +- arch/arm64/kvm/nested.c | 42 +++---- arch/arm64/kvm/sys_regs.c | 20 ++-- arch/arm64/tools/sysreg | 194 ++++++++++++++++++++------------ 9 files changed, 250 insertions(+), 193 deletions(-) but at least it puts all registers in the same bucket, and we don't assume anything anymore. I'll repost the series on Monday, once I'm on holiday. Thanks, M.
diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h index 8d94a6c0ed5c4..e424085f2aaca 100644 --- a/arch/arm64/include/asm/kvm_arm.h +++ b/arch/arm64/include/asm/kvm_arm.h @@ -359,6 +359,10 @@ #define __HAFGRTR_EL2_MASK (GENMASK(49, 17) | GENMASK(4, 0)) #define __HAFGRTR_EL2_nMASK ~(__HAFGRTR_EL2_RES0 | __HAFGRTR_EL2_MASK) +/* Because the sysreg file mixes R and W... */ +#define HFGRTR_EL2_RES0 HFGxTR_EL2_RES0 +#define HFGWTR_EL2_RES0 (HFGRTR_EL2_RES0 | __HFGRTR_ONLY_MASK) + /* Similar definitions for HCRX_EL2 */ #define __HCRX_EL2_RES0 HCRX_EL2_RES0 #define __HCRX_EL2_MASK (BIT(6)) diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index 7cfa024de4e34..4e67d4064f409 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -569,6 +569,20 @@ struct kvm_sysreg_masks { } mask[NR_SYS_REGS - __SANITISED_REG_START__]; }; +struct fgt_masks { + const char *str; + u64 mask; + u64 nmask; + u64 res0; +}; + +extern struct fgt_masks hfgrtr_masks; +extern struct fgt_masks hfgwtr_masks; +extern struct fgt_masks hfgitr_masks; +extern struct fgt_masks hdfgrtr_masks; +extern struct fgt_masks hdfgwtr_masks; +extern struct fgt_masks hafgrtr_masks; + struct kvm_cpu_context { struct user_pt_regs regs; /* sp = sp_el0 */ diff --git a/arch/arm64/kvm/emulate-nested.c b/arch/arm64/kvm/emulate-nested.c index 607d37bab70b4..bbfe89c37a86e 100644 --- a/arch/arm64/kvm/emulate-nested.c +++ b/arch/arm64/kvm/emulate-nested.c @@ -2033,6 +2033,101 @@ static u32 encoding_next(u32 encoding) return sys_reg(op0 + 1, 0, 0, 0, 0); } +#define FGT_MASKS(__n, __m) \ + struct fgt_masks __n = { .str = #__m, .res0 = __m, } + +FGT_MASKS(hfgrtr_masks, HFGRTR_EL2_RES0); +FGT_MASKS(hfgwtr_masks, HFGWTR_EL2_RES0); +FGT_MASKS(hfgitr_masks, HFGITR_EL2_RES0); +FGT_MASKS(hdfgrtr_masks, HDFGRTR_EL2_RES0); +FGT_MASKS(hdfgwtr_masks, HDFGWTR_EL2_RES0); +FGT_MASKS(hafgrtr_masks, HAFGRTR_EL2_RES0); + +static __init bool aggregate_fgt(union trap_config tc) +{ + struct fgt_masks *rmasks, *wmasks; + + switch (tc.fgt) { + case HFGxTR_GROUP: + rmasks = &hfgrtr_masks; + wmasks = &hfgwtr_masks; + break; + case HDFGRTR_GROUP: + rmasks = &hdfgrtr_masks; + wmasks = &hdfgwtr_masks; + break; + case HAFGRTR_GROUP: + rmasks = &hafgrtr_masks; + wmasks = NULL; + break; + case HFGITR_GROUP: + rmasks = &hfgitr_masks; + wmasks = NULL; + break; + } + + /* + * A bit can be reserved in either the R or W register, but + * not both. + */ + if ((BIT(tc.bit) & rmasks->res0) && + (!wmasks || (BIT(tc.bit) & wmasks->res0))) + return false; + + if (tc.pol) + rmasks->mask |= BIT(tc.bit) & ~rmasks->res0; + else + rmasks->nmask |= BIT(tc.bit) & ~rmasks->res0; + + if (wmasks) { + if (tc.pol) + wmasks->mask |= BIT(tc.bit) & ~wmasks->res0; + else + wmasks->nmask |= BIT(tc.bit) & ~wmasks->res0; + } + + return true; +} + +static __init int check_fgt_masks(struct fgt_masks *masks) +{ + unsigned long duplicate = masks->mask & masks->nmask; + u64 res0 = masks->res0; + int ret = 0; + + if (duplicate) { + int i; + + for_each_set_bit(i, &duplicate, 64) { + kvm_err("%s[%d] bit has both polarities\n", + masks->str, i); + } + + ret = -EINVAL; + } + + masks->res0 = ~(masks->mask | masks->nmask); + if (masks->res0 != res0) + kvm_info("Implicit %s = %016llx, expecting %016llx\n", + masks->str, masks->res0, res0); + + return ret; +} + +static __init int check_all_fgt_masks(int ret) +{ + int err = 0; + + err |= check_fgt_masks(&hfgrtr_masks); + err |= check_fgt_masks(&hfgwtr_masks); + err |= check_fgt_masks(&hfgitr_masks); + err |= check_fgt_masks(&hdfgrtr_masks); + err |= check_fgt_masks(&hdfgwtr_masks); + err |= check_fgt_masks(&hafgrtr_masks); + + return ret ?: err; +} + int __init populate_nv_trap_config(void) { int ret = 0; @@ -2097,8 +2192,15 @@ int __init populate_nv_trap_config(void) ret = xa_err(prev); print_nv_trap_error(fgt, "Failed FGT insertion", ret); } + + if (!aggregate_fgt(tc)) { + ret = -EINVAL; + print_nv_trap_error(fgt, "FGT bit is reserved", ret); + } } + ret = check_all_fgt_masks(ret); + kvm_info("nv: %ld fine grained trap handlers\n", ARRAY_SIZE(encoding_to_fgt));
In the process of decoupling KVM's view of the FGT bits from the wider architectural state, use KVM's own FGT tables to build a synthitic view of what is actually known. This allows for some checking along the way. Signed-off-by: Marc Zyngier <maz@kernel.org> --- arch/arm64/include/asm/kvm_arm.h | 4 ++ arch/arm64/include/asm/kvm_host.h | 14 ++++ arch/arm64/kvm/emulate-nested.c | 102 ++++++++++++++++++++++++++++++ 3 files changed, 120 insertions(+)