| Message ID | 20210323095849.37858-16-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | libs/guest: new CPUID/MSR interface | expand |
On 23.03.2021 10:58, Roger Pau Monne wrote: > Introduce a helper to obtain a compatible cpu policy based on two > input cpu policies. Currently this is done by and'ing all CPUID leaves > and MSR entries, except for MSR_ARCH_CAPABILITIES which has the RSBA > bit or'ed. I'm afraid this is too simplistic. It might do as an initial approximation if limited to the feature flag leaves, but you can't reasonably AND together e.g. leaf 0 output. > @@ -1115,3 +1116,117 @@ bool xc_cpu_policy_is_compatible(xc_interface *xch, const xc_cpu_policy_t p1, > > return false; > } > + > +static uint64_t level_msr(unsigned int index, uint64_t val1, uint64_t val2) > +{ > + uint64_t val; > + > + switch( index ) > + { > + case MSR_ARCH_CAPABILITIES: > + val = val1 & val2; Considering you need this even here, how about making this the initializer of the variable, allowing to drop "default:" altogether? Also, nit: Missing blank after "switch". > + index = 0; > + for ( i = 0; i < p1_nr_leaves; i++ ) > + for ( j = 0; j < p2_nr_leaves; j++ ) > + if ( p1_leaves[i].leaf == p2_leaves[j].leaf && > + p1_leaves[i].subleaf == p2_leaves[j].subleaf ) > + { > + leaves[index].leaf = p1_leaves[i].leaf; > + leaves[index].subleaf = p1_leaves[i].subleaf; > + leaves[index].a = p1_leaves[i].a & p2_leaves[j].a; > + leaves[index].b = p1_leaves[i].b & p2_leaves[j].b; > + leaves[index].c = p1_leaves[i].c & p2_leaves[j].c; > + leaves[index].d = p1_leaves[i].d & p2_leaves[j].d; > + index++; > + } > + nr_leaves = index; > + > + index = 0; > + for ( i = 0; i < p1_nr_msrs; i++ ) > + for ( j = 0; j < p2_nr_msrs; j++ ) > + if ( p1_msrs[i].idx == p2_msrs[j].idx ) > + { > + msrs[index].idx = p1_msrs[i].idx; > + msrs[index].val = level_msr(p1_msrs[i].idx, > + p1_msrs[i].val, p2_msrs[j].val); > + index++; > + } > + nr_msrs = index; Mid- to long-term I'd be afraid of this going to take too long for at least the MSRs. Can't we build on some similarity in the ordering of both arrays, to avoid the double for()s? Jan
On 23/03/2021 09:58, Roger Pau Monne wrote: > Introduce a helper to obtain a compatible cpu policy based on two > input cpu policies. Currently this is done by and'ing all CPUID leaves > and MSR entries, except for MSR_ARCH_CAPABILITIES which has the RSBA > bit or'ed. > > The _AC macro is pulled from libxl_internal.h into xen-tools/libs.h > since it's required in order to use the msr-index.h header. > > Note there's no need to place this helper in libx86, since the > calculation of a compatible policy shouldn't be done from the > hypervisor. > > No callers of the interface introduced. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> I think this wants to be in libx86, because we'll want it to replace the opencoded derivation logic in init_cpuid() Also, we absolutely want to unit test it. (I could have sworn I already started some work there - maybe its hidden in one of my WIP branches). ~Andrew
On Thu, Apr 01, 2021 at 05:26:03PM +0100, Andrew Cooper wrote: > On 23/03/2021 09:58, Roger Pau Monne wrote: > > Introduce a helper to obtain a compatible cpu policy based on two > > input cpu policies. Currently this is done by and'ing all CPUID leaves > > and MSR entries, except for MSR_ARCH_CAPABILITIES which has the RSBA > > bit or'ed. > > > > The _AC macro is pulled from libxl_internal.h into xen-tools/libs.h > > since it's required in order to use the msr-index.h header. > > > > Note there's no need to place this helper in libx86, since the > > calculation of a compatible policy shouldn't be done from the > > hypervisor. > > > > No callers of the interface introduced. > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > I think this wants to be in libx86, because we'll want it to replace the > opencoded derivation logic in init_cpuid() I think you mean init_domain_cpuid_policy or recalculate_cpuid_policy? I cannot find any init_cpuid. I'm not convinced we need this in libx86 for the hypervisor, as I don't know exactly how we would use it there. I expect the hypervisor to validate policies provided by the toolstack, but not for it to create such policies unless for very specific domains (ie: dom0 or similar) which shouldn't require any leveling. I'm happy to place it libx86, but I think I need to understand better why it needs to be there. > Also, we absolutely want to unit test it. (I could have sworn I already > started some work there - maybe its hidden in one of my WIP branches). I don't think we have unit tests for the xenguest library? Thanks, Roger.
diff --git a/tools/include/xen-tools/libs.h b/tools/include/xen-tools/libs.h index a16e0c38070..b9e89f9a711 100644 --- a/tools/include/xen-tools/libs.h +++ b/tools/include/xen-tools/libs.h @@ -63,4 +63,9 @@ #define ROUNDUP(_x,_w) (((unsigned long)(_x)+(1UL<<(_w))-1) & ~((1UL<<(_w))-1)) #endif +#ifndef _AC +#define __AC(X,Y) (X##Y) +#define _AC(X,Y) __AC(X,Y) +#endif + #endif /* __XEN_TOOLS_LIBS__ */ diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h index 165beff330f..5f3e5e17e9d 100644 --- a/tools/include/xenctrl.h +++ b/tools/include/xenctrl.h @@ -2622,6 +2622,10 @@ int xc_cpu_policy_update_msrs(xc_interface *xch, xc_cpu_policy_t policy, /* Compatibility calculations. */ bool xc_cpu_policy_is_compatible(xc_interface *xch, const xc_cpu_policy_t p1, const xc_cpu_policy_t p2); +int xc_cpu_policy_calc_compatible(xc_interface *xch, + const xc_cpu_policy_t p1, + const xc_cpu_policy_t p2, + xc_cpu_policy_t out); int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps); int xc_get_cpu_featureset(xc_interface *xch, uint32_t index, diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c index 30ea02a0f31..4afca3249ba 100644 --- a/tools/libs/guest/xg_cpuid_x86.c +++ b/tools/libs/guest/xg_cpuid_x86.c @@ -32,6 +32,7 @@ enum { #include <xen/arch-x86/cpufeatureset.h> }; +#include <xen/asm/msr-index.h> #include <xen/asm/x86-vendors.h> #include <xen/lib/x86/cpu-policy.h> @@ -1115,3 +1116,117 @@ bool xc_cpu_policy_is_compatible(xc_interface *xch, const xc_cpu_policy_t p1, return false; } + +static uint64_t level_msr(unsigned int index, uint64_t val1, uint64_t val2) +{ + uint64_t val; + + switch( index ) + { + case MSR_ARCH_CAPABILITIES: + val = val1 & val2; + /* + * Set RSBA if present on any of the input values to notice the guest + * might run on vulnerable hardware at some point. + */ + val |= (val1 | val2) & ARCH_CAPS_RSBA; + break; + + default: + val = val1 & val2; + break; + } + + return val; +} + +int xc_cpu_policy_calc_compatible(xc_interface *xch, + const xc_cpu_policy_t p1, + const xc_cpu_policy_t p2, + xc_cpu_policy_t out) +{ + xen_cpuid_leaf_t *leaves = NULL, *p1_leaves = NULL, *p2_leaves = NULL; + xen_msr_entry_t *msrs = NULL, *p1_msrs = NULL, *p2_msrs = NULL; + unsigned int nr_leaves, nr_msrs, i, j, index; + unsigned int p1_nr_leaves, p1_nr_msrs, p2_nr_leaves, p2_nr_msrs; + int rc; + + if ( xc_cpu_policy_get_size(xch, &nr_leaves, &nr_msrs) ) + { + PERROR("Failed to obtain policy info size"); + return -1; + } + + leaves = calloc(nr_leaves, sizeof(*leaves)); + p1_leaves = calloc(nr_leaves, sizeof(*p1_leaves)); + p2_leaves = calloc(nr_leaves, sizeof(*p2_leaves)); + msrs = calloc(nr_msrs, sizeof(*msrs)); + p1_msrs = calloc(nr_msrs, sizeof(*p1_msrs)); + p2_msrs = calloc(nr_msrs, sizeof(*p2_msrs)); + + p1_nr_leaves = p2_nr_leaves = nr_leaves; + p1_nr_msrs = p2_nr_msrs = nr_msrs; + + if ( !leaves || !p1_leaves || !p2_leaves || + !msrs || !p1_msrs || !p2_msrs ) + { + ERROR("Failed to allocate resources"); + errno = ENOMEM; + rc = -1; + goto out; + } + + rc = xc_cpu_policy_serialise(xch, p1, p1_leaves, &p1_nr_leaves, + p1_msrs, &p1_nr_msrs); + if ( rc ) + goto out; + rc = xc_cpu_policy_serialise(xch, p2, p2_leaves, &p2_nr_leaves, + p2_msrs, &p2_nr_msrs); + if ( rc ) + goto out; + + index = 0; + for ( i = 0; i < p1_nr_leaves; i++ ) + for ( j = 0; j < p2_nr_leaves; j++ ) + if ( p1_leaves[i].leaf == p2_leaves[j].leaf && + p1_leaves[i].subleaf == p2_leaves[j].subleaf ) + { + leaves[index].leaf = p1_leaves[i].leaf; + leaves[index].subleaf = p1_leaves[i].subleaf; + leaves[index].a = p1_leaves[i].a & p2_leaves[j].a; + leaves[index].b = p1_leaves[i].b & p2_leaves[j].b; + leaves[index].c = p1_leaves[i].c & p2_leaves[j].c; + leaves[index].d = p1_leaves[i].d & p2_leaves[j].d; + index++; + } + nr_leaves = index; + + index = 0; + for ( i = 0; i < p1_nr_msrs; i++ ) + for ( j = 0; j < p2_nr_msrs; j++ ) + if ( p1_msrs[i].idx == p2_msrs[j].idx ) + { + msrs[index].idx = p1_msrs[i].idx; + msrs[index].val = level_msr(p1_msrs[i].idx, + p1_msrs[i].val, p2_msrs[j].val); + index++; + } + nr_msrs = index; + + rc = deserialize_policy(xch, out, nr_leaves, leaves, nr_msrs, msrs); + if ( rc ) + { + errno = -rc; + rc = -1; + } + + out: + free(leaves); + free(p1_leaves); + free(p2_leaves); + free(msrs); + free(p1_msrs); + free(p2_msrs); + + return rc; +} diff --git a/tools/libs/light/libxl_internal.h b/tools/libs/light/libxl_internal.h index 22b1775b752..53b8939fb5a 100644 --- a/tools/libs/light/libxl_internal.h +++ b/tools/libs/light/libxl_internal.h @@ -126,8 +126,6 @@ #define PVSHIM_CMDLINE "pv-shim console=xen,pv" /* Size macros. */ -#define __AC(X,Y) (X##Y) -#define _AC(X,Y) __AC(X,Y) #define MB(_mb) (_AC(_mb, ULL) << 20) #define GB(_gb) (_AC(_gb, ULL) << 30)
Introduce a helper to obtain a compatible cpu policy based on two input cpu policies. Currently this is done by and'ing all CPUID leaves and MSR entries, except for MSR_ARCH_CAPABILITIES which has the RSBA bit or'ed. The _AC macro is pulled from libxl_internal.h into xen-tools/libs.h since it's required in order to use the msr-index.h header. Note there's no need to place this helper in libx86, since the calculation of a compatible policy shouldn't be done from the hypervisor. No callers of the interface introduced. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- tools/include/xen-tools/libs.h | 5 ++ tools/include/xenctrl.h | 4 ++ tools/libs/guest/xg_cpuid_x86.c | 115 ++++++++++++++++++++++++++++++ tools/libs/light/libxl_internal.h | 2 - 4 files changed, 124 insertions(+), 2 deletions(-)