| Message ID | 20210413140140.73690-15-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | libs/guest: new CPUID/MSR interface | expand |
On 13.04.2021 16:01, Roger Pau Monne wrote: > --- a/tools/libs/guest/xg_cpuid_x86.c > +++ b/tools/libs/guest/xg_cpuid_x86.c > @@ -925,3 +925,22 @@ int xc_cpu_policy_update_msrs(xc_interface *xch, xc_cpu_policy_t policy, > > return rc; > } > + > +bool xc_cpu_policy_is_compatible(xc_interface *xch, const xc_cpu_policy_t host, > + const xc_cpu_policy_t guest) > +{ > + struct cpu_policy_errors err = INIT_CPU_POLICY_ERRORS; > + struct cpu_policy h = { &host->cpuid, &host->msr }; > + struct cpu_policy g = { &guest->cpuid, &guest->msr }; > + int rc = x86_cpu_policies_are_compatible(&h, &g, &err); > + > + if ( !rc ) > + return true; > + > + if ( err.leaf != -1 ) > + ERROR("Leaf %#x subleaf %#x is not compatible", err.leaf, err.subleaf); > + if ( err.msr != -1 ) > + ERROR("MSR index %#x is not compatible", err.msr); Personally I'm against making assumptions like these ones about what (in this case) INIT_CPU_POLICY_ERRORS actually expands to (i.e. three times -1). I can see how alternatives to this are quickly going to get ugly, so I'll leave it to others to judge. Jan
On Wed, Apr 14, 2021 at 03:36:54PM +0200, Jan Beulich wrote: > On 13.04.2021 16:01, Roger Pau Monne wrote: > > --- a/tools/libs/guest/xg_cpuid_x86.c > > +++ b/tools/libs/guest/xg_cpuid_x86.c > > @@ -925,3 +925,22 @@ int xc_cpu_policy_update_msrs(xc_interface *xch, xc_cpu_policy_t policy, > > > > return rc; > > } > > + > > +bool xc_cpu_policy_is_compatible(xc_interface *xch, const xc_cpu_policy_t host, > > + const xc_cpu_policy_t guest) > > +{ > > + struct cpu_policy_errors err = INIT_CPU_POLICY_ERRORS; > > + struct cpu_policy h = { &host->cpuid, &host->msr }; > > + struct cpu_policy g = { &guest->cpuid, &guest->msr }; > > + int rc = x86_cpu_policies_are_compatible(&h, &g, &err); > > + > > + if ( !rc ) > > + return true; > > + > > + if ( err.leaf != -1 ) > > + ERROR("Leaf %#x subleaf %#x is not compatible", err.leaf, err.subleaf); > > + if ( err.msr != -1 ) > > + ERROR("MSR index %#x is not compatible", err.msr); > > Personally I'm against making assumptions like these ones about what > (in this case) INIT_CPU_POLICY_ERRORS actually expands to (i.e. three > times -1). I can see how alternatives to this are quickly going to > get ugly, so I'll leave it to others to judge. Would you like me to define a separate POLICY_ERROR? ie: #define INIT_CPU_POLICY_ERROR -1 #define INIT_CPU_POLICY_ERRORS { INIT_CPU_POLICY_ERROR, \ INIT_CPU_POLICY_ERROR, \ INIT_CPU_POLICY_ERROR } We already have a bunch of open coded -1 checks anyway, but might prevent new ones from appearing. Thanks, Roger.
On 22.04.2021 10:22, Roger Pau Monné wrote: > On Wed, Apr 14, 2021 at 03:36:54PM +0200, Jan Beulich wrote: >> On 13.04.2021 16:01, Roger Pau Monne wrote: >>> --- a/tools/libs/guest/xg_cpuid_x86.c >>> +++ b/tools/libs/guest/xg_cpuid_x86.c >>> @@ -925,3 +925,22 @@ int xc_cpu_policy_update_msrs(xc_interface *xch, xc_cpu_policy_t policy, >>> >>> return rc; >>> } >>> + >>> +bool xc_cpu_policy_is_compatible(xc_interface *xch, const xc_cpu_policy_t host, >>> + const xc_cpu_policy_t guest) >>> +{ >>> + struct cpu_policy_errors err = INIT_CPU_POLICY_ERRORS; >>> + struct cpu_policy h = { &host->cpuid, &host->msr }; >>> + struct cpu_policy g = { &guest->cpuid, &guest->msr }; >>> + int rc = x86_cpu_policies_are_compatible(&h, &g, &err); >>> + >>> + if ( !rc ) >>> + return true; >>> + >>> + if ( err.leaf != -1 ) >>> + ERROR("Leaf %#x subleaf %#x is not compatible", err.leaf, err.subleaf); >>> + if ( err.msr != -1 ) >>> + ERROR("MSR index %#x is not compatible", err.msr); >> >> Personally I'm against making assumptions like these ones about what >> (in this case) INIT_CPU_POLICY_ERRORS actually expands to (i.e. three >> times -1). I can see how alternatives to this are quickly going to >> get ugly, so I'll leave it to others to judge. > > Would you like me to define a separate POLICY_ERROR? ie: > > #define INIT_CPU_POLICY_ERROR -1 > #define INIT_CPU_POLICY_ERRORS { INIT_CPU_POLICY_ERROR, \ > INIT_CPU_POLICY_ERROR, \ > INIT_CPU_POLICY_ERROR } I would prefer this; I'm not sure (nit: properly parenthesized) -1 is the value to use though, considering the fields are unsigned. I wonder what Andrew thinks, as he did introduce all of this. > We already have a bunch of open coded -1 checks anyway, but might > prevent new ones from appearing. Indeed. Jan
diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h index 9a6d1b126d8..5f699c09509 100644 --- a/tools/include/xenctrl.h +++ b/tools/include/xenctrl.h @@ -2619,6 +2619,10 @@ int xc_cpu_policy_update_cpuid(xc_interface *xch, xc_cpu_policy_t policy, int xc_cpu_policy_update_msrs(xc_interface *xch, xc_cpu_policy_t policy, const xen_msr_entry_t *msrs, uint32_t nr); +/* Compatibility calculations. */ +bool xc_cpu_policy_is_compatible(xc_interface *xch, const xc_cpu_policy_t host, + const xc_cpu_policy_t guest); + int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps); int xc_get_cpu_featureset(xc_interface *xch, uint32_t index, uint32_t *nr_features, uint32_t *featureset); diff --git a/tools/libs/guest/Makefile b/tools/libs/guest/Makefile index 604e1695d6a..6d2a1d5bbc0 100644 --- a/tools/libs/guest/Makefile +++ b/tools/libs/guest/Makefile @@ -40,7 +40,7 @@ $(patsubst %.c,%.opic,$(ELF_SRCS-y)): CFLAGS += -Wno-pointer-sign ifeq ($(CONFIG_X86),y) # Add libx86 to the build vpath %.c ../../../xen/lib/x86 -SRCS-y += cpuid.c msr.c +SRCS-y += cpuid.c msr.c policy.c endif # new domain builder diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c index 26b09322dfa..bd2f31dd87f 100644 --- a/tools/libs/guest/xg_cpuid_x86.c +++ b/tools/libs/guest/xg_cpuid_x86.c @@ -925,3 +925,22 @@ int xc_cpu_policy_update_msrs(xc_interface *xch, xc_cpu_policy_t policy, return rc; } + +bool xc_cpu_policy_is_compatible(xc_interface *xch, const xc_cpu_policy_t host, + const xc_cpu_policy_t guest) +{ + struct cpu_policy_errors err = INIT_CPU_POLICY_ERRORS; + struct cpu_policy h = { &host->cpuid, &host->msr }; + struct cpu_policy g = { &guest->cpuid, &guest->msr }; + int rc = x86_cpu_policies_are_compatible(&h, &g, &err); + + if ( !rc ) + return true; + + if ( err.leaf != -1 ) + ERROR("Leaf %#x subleaf %#x is not compatible", err.leaf, err.subleaf); + if ( err.msr != -1 ) + ERROR("MSR index %#x is not compatible", err.msr); + + return false; +}
Such helpers is just a wrapper to the existing x86_cpu_policies_are_compatible function. This requires building policy.c from libx86 on user land also. No user of the interface introduced. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes since v1: - Initialize err. - Explicitly name parameters as host and guest. --- tools/include/xenctrl.h | 4 ++++ tools/libs/guest/Makefile | 2 +- tools/libs/guest/xg_cpuid_x86.c | 19 +++++++++++++++++++ 3 files changed, 24 insertions(+), 1 deletion(-)