diff mbox series

[14/21] libs/guest: introduce helper to check cpu policy compatibility

Message ID 20210323095849.37858-15-roger.pau@citrix.com (mailing list archive)
State Superseded
Headers show
Series libs/guest: new CPUID/MSR interface | expand

Commit Message

Roger Pau Monne March 23, 2021, 9:58 a.m. UTC
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(-)

Comments

Jan Beulich March 30, 2021, 4:02 p.m. UTC | #1
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
Roger Pau Monne March 31, 2021, 12:40 p.m. UTC | #2
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.
Jan Beulich March 31, 2021, 2:57 p.m. UTC | #3
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
Andrew Cooper April 1, 2021, 4:14 p.m. UTC | #4
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 mbox series

Patch

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;
+}