| Message ID | 20210323095849.37858-15-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: > --- a/tools/libs/guest/xg_cpuid_x86.c > +++ b/tools/libs/guest/xg_cpuid_x86.c > @@ -1098,3 +1098,20 @@ 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 p1, > + const xc_cpu_policy_t p2) > +{ > + struct cpu_policy_errors err; Don't you need an initializer here for ... > + int rc = x86_cpu_policies_are_compatible(p1, p2, &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); ... these checks to have a chance of actually triggering? (I'm also not certain -1 is a good indicator, but I guess we have been using it elsewhere as well.) Jan
On Tue, Mar 30, 2021 at 06:02:45PM +0200, Jan Beulich wrote: > On 23.03.2021 10:58, Roger Pau Monne wrote: > > --- a/tools/libs/guest/xg_cpuid_x86.c > > +++ b/tools/libs/guest/xg_cpuid_x86.c > > @@ -1098,3 +1098,20 @@ 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 p1, > > + const xc_cpu_policy_t p2) > > +{ > > + struct cpu_policy_errors err; > > Don't you need an initializer here for ... > > > + int rc = x86_cpu_policies_are_compatible(p1, p2, &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); > > ... these checks to have a chance of actually triggering? (I'm also > not certain -1 is a good indicator, but I guess we have been using it > elsewhere as well.) Well, this is strictly the error path, at which point I would expect err to be properly set by x86_cpu_policies_are_compatible. I can however initialize err for safety here. Thanks, Roger.
On 31.03.2021 14:40, Roger Pau Monné wrote: > On Tue, Mar 30, 2021 at 06:02:45PM +0200, Jan Beulich wrote: >> On 23.03.2021 10:58, Roger Pau Monne wrote: >>> --- a/tools/libs/guest/xg_cpuid_x86.c >>> +++ b/tools/libs/guest/xg_cpuid_x86.c >>> @@ -1098,3 +1098,20 @@ 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 p1, >>> + const xc_cpu_policy_t p2) >>> +{ >>> + struct cpu_policy_errors err; >> >> Don't you need an initializer here for ... >> >>> + int rc = x86_cpu_policies_are_compatible(p1, p2, &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); >> >> ... these checks to have a chance of actually triggering? (I'm also >> not certain -1 is a good indicator, but I guess we have been using it >> elsewhere as well.) > > Well, this is strictly the error path, at which point I would expect > err to be properly set by x86_cpu_policies_are_compatible. I can > however initialize err for safety here. Aiui in the error case one of the two, but not both fields would be set. Without initializer you'd likely find both of them != -1, though. Jan
On 23/03/2021 09:58, Roger Pau Monne wrote: > 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> > --- > tools/include/xenctrl.h | 4 ++++ > tools/libs/guest/Makefile | 2 +- > tools/libs/guest/xg_cpuid_x86.c | 17 +++++++++++++++++ > 3 files changed, 22 insertions(+), 1 deletion(-) > > diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h > index 164a287b367..165beff330f 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 p1, > + const xc_cpu_policy_t p2); Exposing this in the interface is useful and necessary. However, the operation is not commutative. p1 and p2 are not interchangeable, is why the libx86 function has specifically named parameters. This distinction needs making clear at this level as well. ~Andrew
diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h index 164a287b367..165beff330f 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 p1, + const xc_cpu_policy_t p2); + 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 f7b662f31aa..30ea02a0f31 100644 --- a/tools/libs/guest/xg_cpuid_x86.c +++ b/tools/libs/guest/xg_cpuid_x86.c @@ -1098,3 +1098,20 @@ 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 p1, + const xc_cpu_policy_t p2) +{ + struct cpu_policy_errors err; + int rc = x86_cpu_policies_are_compatible(p1, p2, &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> --- tools/include/xenctrl.h | 4 ++++ tools/libs/guest/Makefile | 2 +- tools/libs/guest/xg_cpuid_x86.c | 17 +++++++++++++++++ 3 files changed, 22 insertions(+), 1 deletion(-)