[PATCH v3 00/13] libs/guest: new CPUID/MSR interface

[PATCH v3 00/13] libs/guest: new CPUID/MSR interface

[Roger Pau Monne]

Hello,

The following series introduces a new CPUID/MSR interface for the
xenguest library. Such interface handles both CPUID and MSRs using the
same opaque object, and provides some helpers for the user to peek or
modify such data without exposing the backing type. This is useful for
future development as CPUID and MSRs are closely related, so it makes
handling those much easier if they are inside the same object (ie: a
change to a CPUID bit might expose or hide an MSR).

In this patch series libxl and other in tree users have been switched to
use the new interface, so it shouldn't result in any functional change
from a user point of view.

Note there are still some missing pieces likely. The way to modify CPUID
data is not ideal, as it requires fetching a leaf and modifying it
directly. We might want some kind of interface in order to set specific
CPUID features more easily, but that's to be discussed, and would be
done as a follow up series.

Thanks, Roger.

Roger Pau Monne (13):
  libxl: don't ignore the return value from xc_cpuid_apply_policy
  libs/guest: allow fetching a specific CPUID leaf from a cpu policy
  libs/guest: allow fetching a specific MSR entry from a cpu policy
  libs/guest: allow updating a cpu policy CPUID data
  libs/guest: allow updating a cpu policy MSR data
  libs/guest: introduce helper to check cpu policy compatibility
  libs/guest: obtain a compatible cpu policy from two input ones
  libs/guest: make a cpu policy compatible with older Xen versions
  libs/guest: introduce helper set cpu topology in cpu policy
  libs/guest: rework xc_cpuid_xend_policy
  libs/guest: apply a featureset into a cpu policy
  libs/{light,guest}: implement xc_cpuid_apply_policy in libxl
  libs/guest: (re)move xc_cpu_policy_apply_cpuid

 tools/include/libxl.h             |   6 +-
 tools/include/xen-tools/libs.h    |   5 +
 tools/include/xenctrl.h           |  75 ++-
 tools/libs/guest/Makefile         |   2 +-
 tools/libs/guest/xg_cpuid_x86.c   | 832 ++++++++++++++++--------------
 tools/libs/light/libxl_cpuid.c    | 231 ++++++++-
 tools/libs/light/libxl_create.c   |   5 +-
 tools/libs/light/libxl_dom.c      |   2 +-
 tools/libs/light/libxl_internal.h |  32 +-
 tools/libs/light/libxl_nocpuid.c  |   5 +-
 10 files changed, 744 insertions(+), 451 deletions(-)

-- 
2.31.1

[PATCH v3 01/13] libxl: don't ignore the return value from xc_cpuid_apply_policy

[Roger Pau Monne]

Also change libxl__cpuid_legacy to propagate the error from
xc_cpuid_apply_policy into callers.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Changes since v2:
 - Use 'r' for xc_cpuid_apply_policy return value.
 - Use LOGEVD to print error message.

Changes since v1:
 - Return ERROR_FAIL on error.
---
 tools/libs/light/libxl_cpuid.c    | 15 +++++++++++----
 tools/libs/light/libxl_create.c   |  5 +++--
 tools/libs/light/libxl_dom.c      |  2 +-
 tools/libs/light/libxl_internal.h |  4 ++--
 tools/libs/light/libxl_nocpuid.c  |  5 +++--
 5 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
index f32c5d3a6f6..eb6feaa96d1 100644
--- a/tools/libs/light/libxl_cpuid.c
+++ b/tools/libs/light/libxl_cpuid.c
@@ -426,11 +426,13 @@ int libxl_cpuid_parse_config_xend(libxl_cpuid_policy_list *cpuid,
     return 0;
 }
 
-void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
-                         libxl_domain_build_info *info)
+int libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
+                        libxl_domain_build_info *info)
 {
+    GC_INIT(ctx);
     bool pae = true;
     bool itsc;
+    int r;
 
     /*
      * Gross hack.  Using libxl_defbool_val() here causes libvirt to crash in
@@ -469,8 +471,13 @@ void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
     itsc = (libxl_defbool_val(info->disable_migrate) ||
             info->tsc_mode == LIBXL_TSC_MODE_ALWAYS_EMULATE);
 
-    xc_cpuid_apply_policy(ctx->xch, domid, restore, NULL, 0,
-                          pae, itsc, nested_virt, info->cpuid);
+    r = xc_cpuid_apply_policy(ctx->xch, domid, restore, NULL, 0,
+                              pae, itsc, nested_virt, info->cpuid);
+    if (r)
+        LOGEVD(ERROR, -r, domid, "Failed to apply CPUID policy");
+
+    GC_FREE;
+    return r ? ERROR_FAIL : 0;
 }
 
 static const char *input_names[2] = { "leaf", "subleaf" };
diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
index 43e9ba9c634..e356b2106d4 100644
--- a/tools/libs/light/libxl_create.c
+++ b/tools/libs/light/libxl_create.c
@@ -1434,6 +1434,7 @@ int libxl__srm_callout_callback_static_data_done(unsigned int missing,
 
     libxl_domain_config *d_config = dcs->guest_config;
     libxl_domain_build_info *info = &d_config->b_info;
+    int rc = 0;
 
     /*
      * CPUID/MSR information is not present in pre Xen-4.14 streams.
@@ -1443,9 +1444,9 @@ int libxl__srm_callout_callback_static_data_done(unsigned int missing,
      * stream doesn't contain any CPUID data.
      */
     if (missing & XGR_SDD_MISSING_CPUID)
-        libxl__cpuid_legacy(ctx, dcs->guest_domid, true, info);
+        rc = libxl__cpuid_legacy(ctx, dcs->guest_domid, true, info);
 
-    return 0;
+    return rc;
 }
 
 void libxl__srm_callout_callback_restore_results(xen_pfn_t store_mfn,
diff --git a/tools/libs/light/libxl_dom.c b/tools/libs/light/libxl_dom.c
index 842a51c86cb..e9f58ee4b2b 100644
--- a/tools/libs/light/libxl_dom.c
+++ b/tools/libs/light/libxl_dom.c
@@ -384,7 +384,7 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
      * being migrated-in/restored have CPUID handled during the
      * static_data_done() callback. */
     if (!state->restore)
-        libxl__cpuid_legacy(ctx, domid, false, info);
+        rc = libxl__cpuid_legacy(ctx, domid, false, info);
 
     return rc;
 }
diff --git a/tools/libs/light/libxl_internal.h b/tools/libs/light/libxl_internal.h
index c6a4a187f5b..44a2f3c8fe3 100644
--- a/tools/libs/light/libxl_internal.h
+++ b/tools/libs/light/libxl_internal.h
@@ -2052,8 +2052,8 @@ typedef yajl_gen_status (*libxl__gen_json_callback)(yajl_gen hand, void *);
 _hidden char *libxl__object_to_json(libxl_ctx *ctx, const char *type,
                                     libxl__gen_json_callback gen, void *p);
 
-_hidden void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool retore,
-                                 libxl_domain_build_info *info);
+_hidden int libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool retore,
+                                libxl_domain_build_info *info);
 
 /* Calls poll() again - useful to check whether a signaled condition
  * is still true.  Cannot fail.  Returns currently-true revents. */
diff --git a/tools/libs/light/libxl_nocpuid.c b/tools/libs/light/libxl_nocpuid.c
index f47336565b9..0630959e760 100644
--- a/tools/libs/light/libxl_nocpuid.c
+++ b/tools/libs/light/libxl_nocpuid.c
@@ -34,9 +34,10 @@ int libxl_cpuid_parse_config_xend(libxl_cpuid_policy_list *cpuid,
     return 0;
 }
 
-void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
-                         libxl_domain_build_info *info)
+int libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
+                        libxl_domain_build_info *info)
 {
+    return 0;
 }
 
 yajl_gen_status libxl_cpuid_policy_list_gen_json(yajl_gen hand,
-- 
2.31.1

Re: [PATCH v3 01/13] libxl: don't ignore the return value from xc_cpuid_apply_policy

[Anthony PERARD]

On Fri, Apr 30, 2021 at 05:51:59PM +0200, Roger Pau Monne wrote:
> Also change libxl__cpuid_legacy to propagate the error from
> xc_cpuid_apply_policy into callers.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> ---
> Changes since v2:
>  - Use 'r' for xc_cpuid_apply_policy return value.
>  - Use LOGEVD to print error message.
> 
> Changes since v1:
>  - Return ERROR_FAIL on error.
> ---

Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

[PATCH v3 02/13] libs/guest: allow fetching a specific CPUID leaf from a cpu policy

[Roger Pau Monne]

Introduce an interface that returns a specific leaf/subleaf from a cpu
policy in xen_cpuid_leaf_t format.

This is useful to callers can peek data from the opaque
xc_cpu_policy_t type.

No caller of the interface introduced on this patch.

Note that callers of find_leaf need to be slightly adjusted to use the
new helper parameters.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - Use find leaf.
---
 tools/include/xenctrl.h         |  3 +++
 tools/libs/guest/xg_cpuid_x86.c | 38 ++++++++++++++++++++++++++++-----
 2 files changed, 36 insertions(+), 5 deletions(-)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 27cec1b93ff..cbca7209e34 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2608,6 +2608,9 @@ int xc_cpu_policy_set_domain(xc_interface *xch, uint32_t domid,
 int xc_cpu_policy_serialise(xc_interface *xch, const xc_cpu_policy_t policy,
                             xen_cpuid_leaf_t *leaves, uint32_t *nr_leaves,
                             xen_msr_entry_t *msrs, uint32_t *nr_msrs);
+int xc_cpu_policy_get_cpuid(xc_interface *xch, const xc_cpu_policy_t policy,
+                            uint32_t leaf, uint32_t subleaf,
+                            xen_cpuid_leaf_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 0c9b3a960f0..de27826f415 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -280,9 +280,9 @@ static int compare_leaves(const void *l, const void *r)
 
 static xen_cpuid_leaf_t *find_leaf(
     xen_cpuid_leaf_t *leaves, unsigned int nr_leaves,
-    const struct xc_xend_cpuid *xend)
+    unsigned int leaf, unsigned int subleaf)
 {
-    const xen_cpuid_leaf_t key = { xend->leaf, xend->subleaf };
+    const xen_cpuid_leaf_t key = { leaf, subleaf };
 
     return bsearch(&key, leaves, nr_leaves, sizeof(*leaves), compare_leaves);
 }
@@ -365,9 +365,12 @@ static int xc_cpuid_xend_policy(
     rc = -EINVAL;
     for ( ; xend->leaf != XEN_CPUID_INPUT_UNUSED; ++xend )
     {
-        xen_cpuid_leaf_t *cur_leaf = find_leaf(cur, nr_cur, xend);
-        const xen_cpuid_leaf_t *def_leaf = find_leaf(def, nr_def, xend);
-        const xen_cpuid_leaf_t *host_leaf = find_leaf(host, nr_host, xend);
+        xen_cpuid_leaf_t *cur_leaf = find_leaf(cur, nr_cur,
+                                               xend->leaf, xend->subleaf);
+        const xen_cpuid_leaf_t *def_leaf = find_leaf(def, nr_def,
+                                                     xend->leaf, xend->subleaf);
+        const xen_cpuid_leaf_t *host_leaf = find_leaf(host, nr_host, xend->leaf,
+                                                      xend->subleaf);
 
         if ( cur_leaf == NULL || def_leaf == NULL || host_leaf == NULL )
         {
@@ -822,3 +825,28 @@ int xc_cpu_policy_serialise(xc_interface *xch, const xc_cpu_policy_t p,
     errno = 0;
     return 0;
 }
+
+int xc_cpu_policy_get_cpuid(xc_interface *xch, const xc_cpu_policy_t policy,
+                            uint32_t leaf, uint32_t subleaf,
+                            xen_cpuid_leaf_t *out)
+{
+    unsigned int nr_leaves = ARRAY_SIZE(policy->leaves);
+    xen_cpuid_leaf_t *tmp;
+    int rc;
+
+    rc = xc_cpu_policy_serialise(xch, policy, policy->leaves, &nr_leaves,
+                                 NULL, 0);
+    if ( rc )
+        return rc;
+
+    tmp = find_leaf(policy->leaves, nr_leaves, leaf, subleaf);
+    if ( !tmp )
+    {
+        /* Unable to find a matching leaf. */
+        errno = ENOENT;
+        return -1;
+    }
+
+    *out = *tmp;
+    return 0;
+}
-- 
2.31.1

Re: [PATCH v3 02/13] libs/guest: allow fetching a specific CPUID leaf from a cpu policy

[Andrew Cooper]

On 30/04/2021 16:52, Roger Pau Monne wrote:
> @@ -822,3 +825,28 @@ int xc_cpu_policy_serialise(xc_interface *xch, const xc_cpu_policy_t p,
>      errno = 0;
>      return 0;
>  }
> +
> +int xc_cpu_policy_get_cpuid(xc_interface *xch, const xc_cpu_policy_t policy,
> +                            uint32_t leaf, uint32_t subleaf,
> +                            xen_cpuid_leaf_t *out)
> +{
> +    unsigned int nr_leaves = ARRAY_SIZE(policy->leaves);
> +    xen_cpuid_leaf_t *tmp;
> +    int rc;
> +
> +    rc = xc_cpu_policy_serialise(xch, policy, policy->leaves, &nr_leaves,
> +                                 NULL, 0);
> +    if ( rc )
> +        return rc;

Sorry for not spotting this last time.

You don't need to serialise.  You can look up leaf/subleaf in O(1) time
from cpuid_policy, which was a design goal of the structure originally.

It is probably best to adapt most of the first switch statement in
guest_cpuid() to be a libx86 function.  The asserts aren't massively
interesting to keep, and instead of messing around with nospec, just
have the function return a pointer into the cpuid_policy (or NULL), and
have a single block_speculation() in Xen.  We'll also want a unit test
to go with this new function to check that out-of-range leaves don't
result in out-of-bounds reads.

~Andrew

Re: [PATCH v3 02/13] libs/guest: allow fetching a specific CPUID leaf from a cpu policy

[Roger Pau Monné]

On Tue, May 04, 2021 at 12:59:43PM +0100, Andrew Cooper wrote:
> On 30/04/2021 16:52, Roger Pau Monne wrote:
> > @@ -822,3 +825,28 @@ int xc_cpu_policy_serialise(xc_interface *xch, const xc_cpu_policy_t p,
> >      errno = 0;
> >      return 0;
> >  }
> > +
> > +int xc_cpu_policy_get_cpuid(xc_interface *xch, const xc_cpu_policy_t policy,
> > +                            uint32_t leaf, uint32_t subleaf,
> > +                            xen_cpuid_leaf_t *out)
> > +{
> > +    unsigned int nr_leaves = ARRAY_SIZE(policy->leaves);
> > +    xen_cpuid_leaf_t *tmp;
> > +    int rc;
> > +
> > +    rc = xc_cpu_policy_serialise(xch, policy, policy->leaves, &nr_leaves,
> > +                                 NULL, 0);
> > +    if ( rc )
> > +        return rc;
> 
> Sorry for not spotting this last time.
> 
> You don't need to serialise.  You can look up leaf/subleaf in O(1) time
> from cpuid_policy, which was a design goal of the structure originally.
> 
> It is probably best to adapt most of the first switch statement in
> guest_cpuid() to be a libx86 function.  The asserts aren't massively
> interesting to keep, and instead of messing around with nospec, just
> have the function return a pointer into the cpuid_policy (or NULL), and
> have a single block_speculation() in Xen.

libx86 already has array_access_nospec, so I think it's fine to just
leave the code as-is instead of adding a block_speculation in Xen and
dropping the array_access_nospec accessors?

> We'll also want a unit test
> to go with this new function to check that out-of-range leaves don't
> result in out-of-bounds reads.

Sure.

Also, whats your opinion regarding xc_cpu_policy_get_msr, should I
also split part of guest_rdmsr and place it in libx86 in order to
fetch the MSRs present in msr_policy?

Thanks, Roger.

Re: [PATCH v3 02/13] libs/guest: allow fetching a specific CPUID leaf from a cpu policy

[Andrew Cooper]

On 04/05/2021 14:47, Roger Pau Monné wrote:
> On Tue, May 04, 2021 at 12:59:43PM +0100, Andrew Cooper wrote:
>> On 30/04/2021 16:52, Roger Pau Monne wrote:
>>> @@ -822,3 +825,28 @@ int xc_cpu_policy_serialise(xc_interface *xch, const xc_cpu_policy_t p,
>>>      errno = 0;
>>>      return 0;
>>>  }
>>> +
>>> +int xc_cpu_policy_get_cpuid(xc_interface *xch, const xc_cpu_policy_t policy,
>>> +                            uint32_t leaf, uint32_t subleaf,
>>> +                            xen_cpuid_leaf_t *out)
>>> +{
>>> +    unsigned int nr_leaves = ARRAY_SIZE(policy->leaves);
>>> +    xen_cpuid_leaf_t *tmp;
>>> +    int rc;
>>> +
>>> +    rc = xc_cpu_policy_serialise(xch, policy, policy->leaves, &nr_leaves,
>>> +                                 NULL, 0);
>>> +    if ( rc )
>>> +        return rc;
>> Sorry for not spotting this last time.
>>
>> You don't need to serialise.  You can look up leaf/subleaf in O(1) time
>> from cpuid_policy, which was a design goal of the structure originally.
>>
>> It is probably best to adapt most of the first switch statement in
>> guest_cpuid() to be a libx86 function.  The asserts aren't massively
>> interesting to keep, and instead of messing around with nospec, just
>> have the function return a pointer into the cpuid_policy (or NULL), and
>> have a single block_speculation() in Xen.
> libx86 already has array_access_nospec, so I think it's fine to just
> leave the code as-is instead of adding a block_speculation in Xen and
> dropping the array_access_nospec accessors?

The same libx86 function should be used to simplify
x86_cpuid_copy_from_buffer(), which has a similar opencoded construct
for looking up the leaf/subleaf.

You might need some macro trickery to make a const and non-const
versions, or have the main version non-const, and a const-qualified
inline helper which casts away constness on the input but replaces it on
the output.

The new code can't use array_access_nospec() because it is no longer
accessing the array - merely returning a pointer.  array_index_nospec()
might be an acceptable alternative.

>> We'll also want a unit test
>> to go with this new function to check that out-of-range leaves don't
>> result in out-of-bounds reads.
> Sure.
>
> Also, whats your opinion regarding xc_cpu_policy_get_msr, should I
> also split part of guest_rdmsr and place it in libx86 in order to
> fetch the MSRs present in msr_policy?

That's harder to say.  I'd like to avoid the serialise call, but the
current msr_policy structure currently uses uint32_t for space reasons,
so you can't just create a uint64_t pointer to it.

Perhaps we should bite the bullet and use uint64_t unilaterally, so we
can create a lookup_msr_by_index() or equiv.  The next big block of MSRs
going into the policy are the VT-x ones, and they'll need to be 64 bits
wide.

~Andrew

[PATCH v3 03/13] libs/guest: allow fetching a specific MSR entry from a cpu policy

[Roger Pau Monne]

Introduce an interface that returns a specific MSR entry from a cpu
policy in xen_msr_entry_t format. Provide a helper to perform a binary
search against an array of MSR entries.

This is useful to callers can peek data from the opaque
xc_cpu_policy_t type.

No caller of the interface introduced on this patch.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - Introduce a helper to perform a binary search of the MSR entries
   array.
---
 tools/include/xenctrl.h         |  2 ++
 tools/libs/guest/xg_cpuid_x86.c | 42 +++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index cbca7209e34..605c632cf30 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2611,6 +2611,8 @@ int xc_cpu_policy_serialise(xc_interface *xch, const xc_cpu_policy_t policy,
 int xc_cpu_policy_get_cpuid(xc_interface *xch, const xc_cpu_policy_t policy,
                             uint32_t leaf, uint32_t subleaf,
                             xen_cpuid_leaf_t *out);
+int xc_cpu_policy_get_msr(xc_interface *xch, const xc_cpu_policy_t policy,
+                          uint32_t msr, xen_msr_entry_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 de27826f415..9e83daca0e6 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -850,3 +850,45 @@ int xc_cpu_policy_get_cpuid(xc_interface *xch, const xc_cpu_policy_t policy,
     *out = *tmp;
     return 0;
 }
+
+static int compare_entries(const void *l, const void *r)
+{
+    const xen_msr_entry_t *lhs = l;
+    const xen_msr_entry_t *rhs = r;
+
+    if ( lhs->idx == rhs->idx )
+        return 0;
+    return lhs->idx < rhs->idx ? -1 : 1;
+}
+
+static xen_msr_entry_t *find_entry(xen_msr_entry_t *entries,
+                                   unsigned int nr_entries, unsigned int index)
+{
+    const xen_msr_entry_t key = { index };
+
+    return bsearch(&key, entries, nr_entries, sizeof(*entries), compare_entries);
+}
+
+int xc_cpu_policy_get_msr(xc_interface *xch, const xc_cpu_policy_t policy,
+                          uint32_t msr, xen_msr_entry_t *out)
+{
+    unsigned int nr_entries = ARRAY_SIZE(policy->entries);
+    xen_msr_entry_t *tmp;
+    int rc;
+
+    rc = xc_cpu_policy_serialise(xch, policy, NULL, 0,
+                                 policy->entries, &nr_entries);
+    if ( rc )
+        return rc;
+
+    tmp = find_entry(policy->entries, nr_entries, msr);
+    if ( !tmp )
+    {
+        /* Unable to find a matching MSR. */
+        errno = ENOENT;
+        return -1;
+    }
+
+    *out = *tmp;
+    return 0;
+}
-- 
2.31.1

Re: [PATCH v3 03/13] libs/guest: allow fetching a specific MSR entry from a cpu policy

[Jan Beulich]

On 30.04.2021 17:52, Roger Pau Monne wrote:
> Introduce an interface that returns a specific MSR entry from a cpu
> policy in xen_msr_entry_t format. Provide a helper to perform a binary
> search against an array of MSR entries.
> 
> This is useful to callers can peek data from the opaque
> xc_cpu_policy_t type.
> 
> No caller of the interface introduced on this patch.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Changes since v1:
>  - Introduce a helper to perform a binary search of the MSR entries
>    array.
> ---
>  tools/include/xenctrl.h         |  2 ++
>  tools/libs/guest/xg_cpuid_x86.c | 42 +++++++++++++++++++++++++++++++++
>  2 files changed, 44 insertions(+)
> 
> diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
> index cbca7209e34..605c632cf30 100644
> --- a/tools/include/xenctrl.h
> +++ b/tools/include/xenctrl.h
> @@ -2611,6 +2611,8 @@ int xc_cpu_policy_serialise(xc_interface *xch, const xc_cpu_policy_t policy,
>  int xc_cpu_policy_get_cpuid(xc_interface *xch, const xc_cpu_policy_t policy,
>                              uint32_t leaf, uint32_t subleaf,
>                              xen_cpuid_leaf_t *out);
> +int xc_cpu_policy_get_msr(xc_interface *xch, const xc_cpu_policy_t policy,
> +                          uint32_t msr, xen_msr_entry_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 de27826f415..9e83daca0e6 100644
> --- a/tools/libs/guest/xg_cpuid_x86.c
> +++ b/tools/libs/guest/xg_cpuid_x86.c
> @@ -850,3 +850,45 @@ int xc_cpu_policy_get_cpuid(xc_interface *xch, const xc_cpu_policy_t policy,
>      *out = *tmp;
>      return 0;
>  }
> +
> +static int compare_entries(const void *l, const void *r)
> +{
> +    const xen_msr_entry_t *lhs = l;
> +    const xen_msr_entry_t *rhs = r;
> +
> +    if ( lhs->idx == rhs->idx )
> +        return 0;
> +    return lhs->idx < rhs->idx ? -1 : 1;
> +}
> +
> +static xen_msr_entry_t *find_entry(xen_msr_entry_t *entries,
> +                                   unsigned int nr_entries, unsigned int index)
> +{
> +    const xen_msr_entry_t key = { index };
> +
> +    return bsearch(&key, entries, nr_entries, sizeof(*entries), compare_entries);
> +}

Isn't "entries" / "entry" a little too generic a name here, considering
the CPUID equivalents use "leaves" / "leaf"? (Noticed really while looking
at patch 7.)

Jan

Re: [PATCH v3 03/13] libs/guest: allow fetching a specific MSR entry from a cpu policy

[Roger Pau Monné]

On Mon, May 03, 2021 at 12:41:29PM +0200, Jan Beulich wrote:
> On 30.04.2021 17:52, Roger Pau Monne wrote:
> > Introduce an interface that returns a specific MSR entry from a cpu
> > policy in xen_msr_entry_t format. Provide a helper to perform a binary
> > search against an array of MSR entries.
> > 
> > This is useful to callers can peek data from the opaque
> > xc_cpu_policy_t type.
> > 
> > No caller of the interface introduced on this patch.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > Changes since v1:
> >  - Introduce a helper to perform a binary search of the MSR entries
> >    array.
> > ---
> >  tools/include/xenctrl.h         |  2 ++
> >  tools/libs/guest/xg_cpuid_x86.c | 42 +++++++++++++++++++++++++++++++++
> >  2 files changed, 44 insertions(+)
> > 
> > diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
> > index cbca7209e34..605c632cf30 100644
> > --- a/tools/include/xenctrl.h
> > +++ b/tools/include/xenctrl.h
> > @@ -2611,6 +2611,8 @@ int xc_cpu_policy_serialise(xc_interface *xch, const xc_cpu_policy_t policy,
> >  int xc_cpu_policy_get_cpuid(xc_interface *xch, const xc_cpu_policy_t policy,
> >                              uint32_t leaf, uint32_t subleaf,
> >                              xen_cpuid_leaf_t *out);
> > +int xc_cpu_policy_get_msr(xc_interface *xch, const xc_cpu_policy_t policy,
> > +                          uint32_t msr, xen_msr_entry_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 de27826f415..9e83daca0e6 100644
> > --- a/tools/libs/guest/xg_cpuid_x86.c
> > +++ b/tools/libs/guest/xg_cpuid_x86.c
> > @@ -850,3 +850,45 @@ int xc_cpu_policy_get_cpuid(xc_interface *xch, const xc_cpu_policy_t policy,
> >      *out = *tmp;
> >      return 0;
> >  }
> > +
> > +static int compare_entries(const void *l, const void *r)
> > +{
> > +    const xen_msr_entry_t *lhs = l;
> > +    const xen_msr_entry_t *rhs = r;
> > +
> > +    if ( lhs->idx == rhs->idx )
> > +        return 0;
> > +    return lhs->idx < rhs->idx ? -1 : 1;
> > +}
> > +
> > +static xen_msr_entry_t *find_entry(xen_msr_entry_t *entries,
> > +                                   unsigned int nr_entries, unsigned int index)
> > +{
> > +    const xen_msr_entry_t key = { index };
> > +
> > +    return bsearch(&key, entries, nr_entries, sizeof(*entries), compare_entries);
> > +}
> 
> Isn't "entries" / "entry" a little too generic a name here, considering
> the CPUID equivalents use "leaves" / "leaf"? (Noticed really while looking
> at patch 7.)

Would you be fine with naming the function find_msr and leaving the
rest of the parameters names as-is?

Thanks, Roger.

Re: [PATCH v3 03/13] libs/guest: allow fetching a specific MSR entry from a cpu policy

[Jan Beulich]

On 04.05.2021 12:56, Roger Pau Monné wrote:
> On Mon, May 03, 2021 at 12:41:29PM +0200, Jan Beulich wrote:
>> On 30.04.2021 17:52, Roger Pau Monne wrote:
>>> --- a/tools/libs/guest/xg_cpuid_x86.c
>>> +++ b/tools/libs/guest/xg_cpuid_x86.c
>>> @@ -850,3 +850,45 @@ int xc_cpu_policy_get_cpuid(xc_interface *xch, const xc_cpu_policy_t policy,
>>>      *out = *tmp;
>>>      return 0;
>>>  }
>>> +
>>> +static int compare_entries(const void *l, const void *r)
>>> +{
>>> +    const xen_msr_entry_t *lhs = l;
>>> +    const xen_msr_entry_t *rhs = r;
>>> +
>>> +    if ( lhs->idx == rhs->idx )
>>> +        return 0;
>>> +    return lhs->idx < rhs->idx ? -1 : 1;
>>> +}
>>> +
>>> +static xen_msr_entry_t *find_entry(xen_msr_entry_t *entries,
>>> +                                   unsigned int nr_entries, unsigned int index)
>>> +{
>>> +    const xen_msr_entry_t key = { index };
>>> +
>>> +    return bsearch(&key, entries, nr_entries, sizeof(*entries), compare_entries);
>>> +}
>>
>> Isn't "entries" / "entry" a little too generic a name here, considering
>> the CPUID equivalents use "leaves" / "leaf"? (Noticed really while looking
>> at patch 7.)
> 
> Would you be fine with naming the function find_msr and leaving the
> rest of the parameters names as-is?

Yes. But recall I'm not the maintainer of this code anyway.

Jan

Re: [PATCH v3 03/13] libs/guest: allow fetching a specific MSR entry from a cpu policy

[Roger Pau Monné]

On Tue, May 04, 2021 at 01:40:11PM +0200, Jan Beulich wrote:
> On 04.05.2021 12:56, Roger Pau Monné wrote:
> > On Mon, May 03, 2021 at 12:41:29PM +0200, Jan Beulich wrote:
> >> On 30.04.2021 17:52, Roger Pau Monne wrote:
> >>> --- a/tools/libs/guest/xg_cpuid_x86.c
> >>> +++ b/tools/libs/guest/xg_cpuid_x86.c
> >>> @@ -850,3 +850,45 @@ int xc_cpu_policy_get_cpuid(xc_interface *xch, const xc_cpu_policy_t policy,
> >>>      *out = *tmp;
> >>>      return 0;
> >>>  }
> >>> +
> >>> +static int compare_entries(const void *l, const void *r)
> >>> +{
> >>> +    const xen_msr_entry_t *lhs = l;
> >>> +    const xen_msr_entry_t *rhs = r;
> >>> +
> >>> +    if ( lhs->idx == rhs->idx )
> >>> +        return 0;
> >>> +    return lhs->idx < rhs->idx ? -1 : 1;
> >>> +}
> >>> +
> >>> +static xen_msr_entry_t *find_entry(xen_msr_entry_t *entries,
> >>> +                                   unsigned int nr_entries, unsigned int index)
> >>> +{
> >>> +    const xen_msr_entry_t key = { index };
> >>> +
> >>> +    return bsearch(&key, entries, nr_entries, sizeof(*entries), compare_entries);
> >>> +}
> >>
> >> Isn't "entries" / "entry" a little too generic a name here, considering
> >> the CPUID equivalents use "leaves" / "leaf"? (Noticed really while looking
> >> at patch 7.)
> > 
> > Would you be fine with naming the function find_msr and leaving the
> > rest of the parameters names as-is?
> 
> Yes. But recall I'm not the maintainer of this code anyway.

You cared to provide feedback, and I'm happy to make the change.

Thanks, Roger.

Re: [PATCH v3 03/13] libs/guest: allow fetching a specific MSR entry from a cpu policy

[Andrew Cooper]

On 04/05/2021 12:58, Roger Pau Monné wrote:
> On Tue, May 04, 2021 at 01:40:11PM +0200, Jan Beulich wrote:
>> On 04.05.2021 12:56, Roger Pau Monné wrote:
>>> On Mon, May 03, 2021 at 12:41:29PM +0200, Jan Beulich wrote:
>>>> On 30.04.2021 17:52, Roger Pau Monne wrote:
>>>>> --- a/tools/libs/guest/xg_cpuid_x86.c
>>>>> +++ b/tools/libs/guest/xg_cpuid_x86.c
>>>>> @@ -850,3 +850,45 @@ int xc_cpu_policy_get_cpuid(xc_interface *xch, const xc_cpu_policy_t policy,
>>>>>      *out = *tmp;
>>>>>      return 0;
>>>>>  }
>>>>> +
>>>>> +static int compare_entries(const void *l, const void *r)
>>>>> +{
>>>>> +    const xen_msr_entry_t *lhs = l;
>>>>> +    const xen_msr_entry_t *rhs = r;
>>>>> +
>>>>> +    if ( lhs->idx == rhs->idx )
>>>>> +        return 0;
>>>>> +    return lhs->idx < rhs->idx ? -1 : 1;
>>>>> +}
>>>>> +
>>>>> +static xen_msr_entry_t *find_entry(xen_msr_entry_t *entries,
>>>>> +                                   unsigned int nr_entries, unsigned int index)
>>>>> +{
>>>>> +    const xen_msr_entry_t key = { index };
>>>>> +
>>>>> +    return bsearch(&key, entries, nr_entries, sizeof(*entries), compare_entries);
>>>>> +}
>>>> Isn't "entries" / "entry" a little too generic a name here, considering
>>>> the CPUID equivalents use "leaves" / "leaf"? (Noticed really while looking
>>>> at patch 7.)
>>> Would you be fine with naming the function find_msr and leaving the
>>> rest of the parameters names as-is?
>> Yes. But recall I'm not the maintainer of this code anyway.

This file in particular has been entirely within the x86 remit for
multiple years now, as have the other cpuid bits in misc/ and libxl.

> You cared to provide feedback, and I'm happy to make the change.

find_msr() would be a better name.  As for entries and nr_entries,
suggestions welcome.  I couldn't think of anything better for the low
level helpers.

~Andrew

Re: [PATCH v3 03/13] libs/guest: allow fetching a specific MSR entry from a cpu policy

[Jan Beulich]

On 04.05.2021 19:11, Andrew Cooper wrote:
> On 04/05/2021 12:58, Roger Pau Monné wrote:
>> On Tue, May 04, 2021 at 01:40:11PM +0200, Jan Beulich wrote:
>>> On 04.05.2021 12:56, Roger Pau Monné wrote:
>>>> On Mon, May 03, 2021 at 12:41:29PM +0200, Jan Beulich wrote:
>>>>> On 30.04.2021 17:52, Roger Pau Monne wrote:
>>>>>> --- a/tools/libs/guest/xg_cpuid_x86.c
>>>>>> +++ b/tools/libs/guest/xg_cpuid_x86.c
>>>>>> @@ -850,3 +850,45 @@ int xc_cpu_policy_get_cpuid(xc_interface *xch, const xc_cpu_policy_t policy,
>>>>>>      *out = *tmp;
>>>>>>      return 0;
>>>>>>  }
>>>>>> +
>>>>>> +static int compare_entries(const void *l, const void *r)
>>>>>> +{
>>>>>> +    const xen_msr_entry_t *lhs = l;
>>>>>> +    const xen_msr_entry_t *rhs = r;
>>>>>> +
>>>>>> +    if ( lhs->idx == rhs->idx )
>>>>>> +        return 0;
>>>>>> +    return lhs->idx < rhs->idx ? -1 : 1;
>>>>>> +}
>>>>>> +
>>>>>> +static xen_msr_entry_t *find_entry(xen_msr_entry_t *entries,
>>>>>> +                                   unsigned int nr_entries, unsigned int index)
>>>>>> +{
>>>>>> +    const xen_msr_entry_t key = { index };
>>>>>> +
>>>>>> +    return bsearch(&key, entries, nr_entries, sizeof(*entries), compare_entries);
>>>>>> +}
>>>>> Isn't "entries" / "entry" a little too generic a name here, considering
>>>>> the CPUID equivalents use "leaves" / "leaf"? (Noticed really while looking
>>>>> at patch 7.)
>>>> Would you be fine with naming the function find_msr and leaving the
>>>> rest of the parameters names as-is?
>>> Yes. But recall I'm not the maintainer of this code anyway.
> 
> This file in particular has been entirely within the x86 remit for
> multiple years now, as have the other cpuid bits in misc/ and libxl.

Well, it's somewhere in the middle imo: Its maintainers have kind of
permanently delegated to the x86 maintainers. Which still doesn't
make it part of "X86 ARCHITECTURE" until such time as the file gets
actually named there. Note how e.g. tools/misc/xen-cpuid.c already is.

Jan

[PATCH v3 04/13] libs/guest: allow updating a cpu policy CPUID data

[Roger Pau Monne]

Introduce a helper to update the CPUID policy using an array of
xen_cpuid_leaf_t entries. Note the leaves present in the input
xen_cpuid_leaf_t array will replace any existing leaves on the policy.

No user of the interface introduced on this patch.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - Don't use memcpy.
 - Drop logic to update the leaf manually - x86_cpuid_copy_from_buffer
   already does it.
 - Only print a failure message if err_leaf != -1.
---
 tools/include/xenctrl.h         |  3 +++
 tools/libs/guest/xg_cpuid_x86.c | 20 ++++++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 605c632cf30..49f919f16a7 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2613,6 +2613,9 @@ int xc_cpu_policy_get_cpuid(xc_interface *xch, const xc_cpu_policy_t policy,
                             xen_cpuid_leaf_t *out);
 int xc_cpu_policy_get_msr(xc_interface *xch, const xc_cpu_policy_t policy,
                           uint32_t msr, xen_msr_entry_t *out);
+int xc_cpu_policy_update_cpuid(xc_interface *xch, xc_cpu_policy_t policy,
+                               const xen_cpuid_leaf_t *leaves,
+                               uint32_t nr);
 
 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 9e83daca0e6..a38e75f8fb1 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -892,3 +892,23 @@ int xc_cpu_policy_get_msr(xc_interface *xch, const xc_cpu_policy_t policy,
     *out = *tmp;
     return 0;
 }
+
+int xc_cpu_policy_update_cpuid(xc_interface *xch, xc_cpu_policy_t policy,
+                               const xen_cpuid_leaf_t *leaves,
+                               uint32_t nr)
+{
+    unsigned int err_leaf = -1, err_subleaf = -1;
+    int rc = x86_cpuid_copy_from_buffer(&policy->cpuid, leaves, nr,
+                                        &err_leaf, &err_subleaf);
+
+    if ( rc )
+    {
+        if ( err_leaf != -1 )
+            ERROR("Failed to update CPUID (err leaf %#x, subleaf %#x) (%d = %s)",
+                  err_leaf, err_subleaf, -rc, strerror(-rc));
+        errno = -rc;
+        rc = -1;
+    }
+
+    return rc;
+}
-- 
2.31.1

Re: [PATCH v3 04/13] libs/guest: allow updating a cpu policy CPUID data

[Andrew Cooper]

On 30/04/2021 16:52, Roger Pau Monne wrote:
> Introduce a helper to update the CPUID policy using an array of
> xen_cpuid_leaf_t entries. Note the leaves present in the input
> xen_cpuid_leaf_t array will replace any existing leaves on the policy.
>
> No user of the interface introduced on this patch.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

[PATCH v3 05/13] libs/guest: allow updating a cpu policy MSR data

[Roger Pau Monne]

Introduce a helper to update the MSR policy using an array of
xen_msr_entry_t entries. Note the MSRs present in the input
xen_msr_entry_t array will replace any existing entries on the
policy.

No user of the interface introduced on this patch.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - Drop logic to update entries manually.
 - Only print failure message if err_msr != -1.
---
 tools/include/xenctrl.h         |  2 ++
 tools/libs/guest/xg_cpuid_x86.c | 18 ++++++++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 49f919f16a7..9a6d1b126d8 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2616,6 +2616,8 @@ int xc_cpu_policy_get_msr(xc_interface *xch, const xc_cpu_policy_t policy,
 int xc_cpu_policy_update_cpuid(xc_interface *xch, xc_cpu_policy_t policy,
                                const xen_cpuid_leaf_t *leaves,
                                uint32_t nr);
+int xc_cpu_policy_update_msrs(xc_interface *xch, xc_cpu_policy_t policy,
+                              const xen_msr_entry_t *msrs, uint32_t nr);
 
 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 a38e75f8fb1..37e55279ffe 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -912,3 +912,21 @@ int xc_cpu_policy_update_cpuid(xc_interface *xch, xc_cpu_policy_t policy,
 
     return rc;
 }
+
+int xc_cpu_policy_update_msrs(xc_interface *xch, xc_cpu_policy_t policy,
+                              const xen_msr_entry_t *msrs, uint32_t nr)
+{
+    unsigned int err_msr = -1;
+    int rc = x86_msr_copy_from_buffer(&policy->msr, msrs, nr, &err_msr);
+
+    if ( rc )
+    {
+        if ( err_msr != -1 )
+            ERROR("Failed to deserialise MSRS (err index %#x) (%d = %s)",
+                  err_msr, -rc, strerror(-rc));
+        errno = -rc;
+        rc = -1;
+    }
+
+    return rc;
+}
-- 
2.31.1

Re: [PATCH v3 05/13] libs/guest: allow updating a cpu policy MSR data

[Andrew Cooper]

On 30/04/2021 16:52, Roger Pau Monne wrote:
> Introduce a helper to update the MSR policy using an array of
> xen_msr_entry_t entries. Note the MSRs present in the input
> xen_msr_entry_t array will replace any existing entries on the
> policy.
>
> No user of the interface introduced on this patch.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

[PATCH v3 06/13] libs/guest: introduce helper to check cpu policy compatibility

[Roger Pau Monne]

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(-)

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 37e55279ffe..6b8bae00334 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -930,3 +930,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;
+}
-- 
2.31.1

Re: [PATCH v3 06/13] libs/guest: introduce helper to check cpu policy compatibility

[Andrew Cooper]

On 30/04/2021 16:52, 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>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

[PATCH v3 07/13] libs/guest: obtain a compatible cpu policy from two input ones

[Roger Pau Monne]

Introduce a helper to obtain a compatible cpu policy based on two
input cpu policies. Currently this is done by and'ing all CPUID
feature 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>
---
Changes since v2:
 - Add some comments.
 - Remove stray double semicolon.
 - AND all 0x7 subleaves (except 0.EAX).
 - Explicitly handle MSR indexes in a switch statement.
 - Error out when an unhandled MSR is found.
 - Add handling of leaf 0x80000021.

Changes since v1:
 - Only AND the feature parts of cpuid.
 - Use a binary search to find the matching leaves and msr entries.
 - Remove default case from MSR level function.
---
 tools/include/xen-tools/libs.h    |   5 ++
 tools/include/xenctrl.h           |   4 +
 tools/libs/guest/xg_cpuid_x86.c   | 137 ++++++++++++++++++++++++++++++
 tools/libs/light/libxl_internal.h |   2 -
 4 files changed, 146 insertions(+), 2 deletions(-)

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 5f699c09509..c41d794683c 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 host,
                                  const xc_cpu_policy_t guest);
+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 6b8bae00334..be2056469aa 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>
@@ -949,3 +950,139 @@ bool xc_cpu_policy_is_compatible(xc_interface *xch, const xc_cpu_policy_t host,
 
     return false;
 }
+
+static bool level_msr(const xen_msr_entry_t *e1, const xen_msr_entry_t *e2,
+                      xen_msr_entry_t *out)
+{
+    *out = (xen_msr_entry_t){ .idx = e1->idx };
+
+    switch ( e1->idx )
+    {
+    case MSR_INTEL_PLATFORM_INFO:
+        out->val = e1->val & e2->val;
+        return true;
+
+    case MSR_ARCH_CAPABILITIES:
+        out->val = e1->val & e2->val;
+        /*
+         * Set RSBA if present on any of the input values to notice the guest
+         * might run on vulnerable hardware at some point.
+         */
+        out->val |= (e1->val | e2->val) & ARCH_CAPS_RSBA;
+        return true;
+    }
+
+    return false;
+}
+
+/* Only level featuresets so far. */
+static bool level_leaf(const xen_cpuid_leaf_t *l1, const xen_cpuid_leaf_t *l2,
+                       xen_cpuid_leaf_t *out)
+{
+    *out = (xen_cpuid_leaf_t){
+        .leaf = l1->leaf,
+        .subleaf = l2->subleaf,
+    };
+
+    switch ( l1->leaf )
+    {
+    case 0x1:
+    case 0x80000001:
+        out->c = l1->c & l2->c;
+        out->d = l1->d & l2->d;
+        return true;
+
+    case 0xd:
+        if ( l1->subleaf != 1 )
+            break;
+        /*
+         * Only take Da1 into account, the rest of subleaves will be dropped
+         * and recalculated by recalculate_xstate.
+         */
+        out->a = l1->a & l2->a;
+        return true;
+
+    case 0x7:
+        if ( l1->subleaf )
+            /* subleaf 0 EAX contains the max subleaf count. */
+            out->a = l1->a & l2->a;
+        out->b = l1->b & l2->b;
+        out->c = l1->c & l2->c;
+        out->d = l1->d & l2->d;
+        return true;
+
+    case 0x80000007:
+        out->d = l1->d & l2->d;
+        return true;
+
+    case 0x80000008:
+        out->b = l1->b & l2->b;
+        return true;
+
+    case 0x80000021:
+        out->a = l1->a & l2->a;
+        return true;
+    }
+
+    return false;
+}
+
+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)
+{
+    unsigned int nr_leaves, nr_msrs, i, index;
+    unsigned int p1_nr_leaves, p2_nr_leaves;
+    unsigned int p1_nr_entries, p2_nr_entries;
+    int rc;
+
+    p1_nr_leaves = p2_nr_leaves = ARRAY_SIZE(p1->leaves);
+    p1_nr_entries = p2_nr_entries = ARRAY_SIZE(p1->entries);
+
+    rc = xc_cpu_policy_serialise(xch, p1, p1->leaves, &p1_nr_leaves,
+                                 p1->entries, &p1_nr_entries);
+    if ( rc )
+        return rc;
+    rc = xc_cpu_policy_serialise(xch, p2, p2->leaves, &p2_nr_leaves,
+                                 p2->entries, &p2_nr_entries);
+    if ( rc )
+        return rc;
+
+    index = 0;
+    for ( i = 0; i < p1_nr_leaves; i++ )
+    {
+        xen_cpuid_leaf_t *l1 = &p1->leaves[i];
+        xen_cpuid_leaf_t *l2 = find_leaf(p2->leaves, p2_nr_leaves,
+                                         l1->leaf, l1->subleaf);
+
+        if ( l2 && level_leaf(l1, l2, &out->leaves[index]) )
+            index++;
+    }
+    nr_leaves = index;
+
+    index = 0;
+    for ( i = 0; i < p1_nr_entries; i++ )
+    {
+        xen_msr_entry_t *e1 = &p1->entries[i];
+        xen_msr_entry_t *e2 = find_entry(p2->entries, p2_nr_entries, e1->idx);
+
+        if ( !e2 )
+            continue;
+        if ( !level_msr(e1, e2, &out->entries[index++]) )
+        {
+            ERROR("Unable to level MSR index %#x", e1->idx);
+            return -EINVAL;
+        }
+    }
+    nr_msrs = index;
+
+    rc = deserialize_policy(xch, out, nr_leaves, nr_msrs);
+    if ( rc )
+    {
+        errno = -rc;
+        rc = -1;
+    }
+
+    return rc;
+}
diff --git a/tools/libs/light/libxl_internal.h b/tools/libs/light/libxl_internal.h
index 44a2f3c8fe3..5709bcb93fa 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)
 
-- 
2.31.1

Re: [PATCH v3 07/13] libs/guest: obtain a compatible cpu policy from two input ones

[Jan Beulich]

On 30.04.2021 17:52, 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
> feature 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>
> ---
> Changes since v2:
>  - Add some comments.
>  - Remove stray double semicolon.
>  - AND all 0x7 subleaves (except 0.EAX).
>  - Explicitly handle MSR indexes in a switch statement.
>  - Error out when an unhandled MSR is found.
>  - Add handling of leaf 0x80000021.
> 
> Changes since v1:
>  - Only AND the feature parts of cpuid.
>  - Use a binary search to find the matching leaves and msr entries.
>  - Remove default case from MSR level function.
> ---
>  tools/include/xen-tools/libs.h    |   5 ++
>  tools/include/xenctrl.h           |   4 +
>  tools/libs/guest/xg_cpuid_x86.c   | 137 ++++++++++++++++++++++++++++++
>  tools/libs/light/libxl_internal.h |   2 -
>  4 files changed, 146 insertions(+), 2 deletions(-)
> 
> 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 5f699c09509..c41d794683c 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 host,
>                                   const xc_cpu_policy_t guest);
> +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 6b8bae00334..be2056469aa 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>
> @@ -949,3 +950,139 @@ bool xc_cpu_policy_is_compatible(xc_interface *xch, const xc_cpu_policy_t host,
>  
>      return false;
>  }
> +
> +static bool level_msr(const xen_msr_entry_t *e1, const xen_msr_entry_t *e2,
> +                      xen_msr_entry_t *out)
> +{
> +    *out = (xen_msr_entry_t){ .idx = e1->idx };
> +
> +    switch ( e1->idx )
> +    {
> +    case MSR_INTEL_PLATFORM_INFO:
> +        out->val = e1->val & e2->val;
> +        return true;
> +
> +    case MSR_ARCH_CAPABILITIES:
> +        out->val = e1->val & e2->val;
> +        /*
> +         * Set RSBA if present on any of the input values to notice the guest
> +         * might run on vulnerable hardware at some point.
> +         */
> +        out->val |= (e1->val | e2->val) & ARCH_CAPS_RSBA;
> +        return true;
> +    }
> +
> +    return false;
> +}
> +
> +/* Only level featuresets so far. */

I have to admit that I don't think I see all the implications from
this implementation restriction. All other leaves get dropped by
the caller, but it's not clear to me what this means wrt what the
guest is ultimately going to get to see.

> +static bool level_leaf(const xen_cpuid_leaf_t *l1, const xen_cpuid_leaf_t *l2,
> +                       xen_cpuid_leaf_t *out)
> +{
> +    *out = (xen_cpuid_leaf_t){
> +        .leaf = l1->leaf,
> +        .subleaf = l2->subleaf,

Since ->leaf and ->subleaf ought to match anyway, I think it would
look less odd if both initializers were taken from consistent source.

> +    };
> +
> +    switch ( l1->leaf )
> +    {
> +    case 0x1:
> +    case 0x80000001:
> +        out->c = l1->c & l2->c;
> +        out->d = l1->d & l2->d;
> +        return true;
> +
> +    case 0xd:
> +        if ( l1->subleaf != 1 )
> +            break;
> +        /*
> +         * Only take Da1 into account, the rest of subleaves will be dropped
> +         * and recalculated by recalculate_xstate.
> +         */
> +        out->a = l1->a & l2->a;
> +        return true;
> +
> +    case 0x7:
> +        if ( l1->subleaf )
> +            /* subleaf 0 EAX contains the max subleaf count. */
> +            out->a = l1->a & l2->a;

        else
            out->a = min(l1->a, l2->a);

? Or is the result from here then further passed to
x86_cpuid_policy_shrink_max_leaves() (not visible from this patch)?
(If not, the same would apply to all other multi-subleaf leaves.)

> +        out->b = l1->b & l2->b;
> +        out->c = l1->c & l2->c;
> +        out->d = l1->d & l2->d;
> +        return true;
> +
> +    case 0x80000007:
> +        out->d = l1->d & l2->d;
> +        return true;
> +
> +    case 0x80000008:
> +        out->b = l1->b & l2->b;
> +        return true;
> +
> +    case 0x80000021:
> +        out->a = l1->a & l2->a;
> +        return true;
> +    }
> +
> +    return false;
> +}
> +
> +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)

I have to admit that I find these two "const" misleading here. You
don't equally constify the other two parameters (which would e.g. be
xc_interface *const xch), and I don't think doing so is common
practice elsewhere. And what p1 and p2 point to is specifically non-
const (and cannot be const), due to ...

> +{
> +    unsigned int nr_leaves, nr_msrs, i, index;
> +    unsigned int p1_nr_leaves, p2_nr_leaves;
> +    unsigned int p1_nr_entries, p2_nr_entries;
> +    int rc;
> +
> +    p1_nr_leaves = p2_nr_leaves = ARRAY_SIZE(p1->leaves);
> +    p1_nr_entries = p2_nr_entries = ARRAY_SIZE(p1->entries);
> +
> +    rc = xc_cpu_policy_serialise(xch, p1, p1->leaves, &p1_nr_leaves,
> +                                 p1->entries, &p1_nr_entries);
> +    if ( rc )
> +        return rc;
> +    rc = xc_cpu_policy_serialise(xch, p2, p2->leaves, &p2_nr_leaves,
> +                                 p2->entries, &p2_nr_entries);

... these two calls.

Jan

Re: [PATCH v3 07/13] libs/guest: obtain a compatible cpu policy from two input ones

[Roger Pau Monné]

On Mon, May 03, 2021 at 12:43:08PM +0200, Jan Beulich wrote:
> On 30.04.2021 17:52, 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
> > feature 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>
> > ---
> > Changes since v2:
> >  - Add some comments.
> >  - Remove stray double semicolon.
> >  - AND all 0x7 subleaves (except 0.EAX).
> >  - Explicitly handle MSR indexes in a switch statement.
> >  - Error out when an unhandled MSR is found.
> >  - Add handling of leaf 0x80000021.
> > 
> > Changes since v1:
> >  - Only AND the feature parts of cpuid.
> >  - Use a binary search to find the matching leaves and msr entries.
> >  - Remove default case from MSR level function.
> > ---
> >  tools/include/xen-tools/libs.h    |   5 ++
> >  tools/include/xenctrl.h           |   4 +
> >  tools/libs/guest/xg_cpuid_x86.c   | 137 ++++++++++++++++++++++++++++++
> >  tools/libs/light/libxl_internal.h |   2 -
> >  4 files changed, 146 insertions(+), 2 deletions(-)
> > 
> > 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 5f699c09509..c41d794683c 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 host,
> >                                   const xc_cpu_policy_t guest);
> > +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 6b8bae00334..be2056469aa 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>
> > @@ -949,3 +950,139 @@ bool xc_cpu_policy_is_compatible(xc_interface *xch, const xc_cpu_policy_t host,
> >  
> >      return false;
> >  }
> > +
> > +static bool level_msr(const xen_msr_entry_t *e1, const xen_msr_entry_t *e2,
> > +                      xen_msr_entry_t *out)
> > +{
> > +    *out = (xen_msr_entry_t){ .idx = e1->idx };
> > +
> > +    switch ( e1->idx )
> > +    {
> > +    case MSR_INTEL_PLATFORM_INFO:
> > +        out->val = e1->val & e2->val;
> > +        return true;
> > +
> > +    case MSR_ARCH_CAPABILITIES:
> > +        out->val = e1->val & e2->val;
> > +        /*
> > +         * Set RSBA if present on any of the input values to notice the guest
> > +         * might run on vulnerable hardware at some point.
> > +         */
> > +        out->val |= (e1->val | e2->val) & ARCH_CAPS_RSBA;
> > +        return true;
> > +    }
> > +
> > +    return false;
> > +}
> > +
> > +/* Only level featuresets so far. */
> 
> I have to admit that I don't think I see all the implications from
> this implementation restriction. All other leaves get dropped by
> the caller, but it's not clear to me what this means wrt what the
> guest is ultimately going to get to see.

This aims to be based on what XenServer does, which I was told is to
level the featuresets. I think the caller of the function will have to
fill the part of the policy that cannot be leveled. It's likely new
helpers will be added to do that as required.

One option would be to get the default policy for the guest and then
use xc_cpu_policy_update_cpuid to apply the leveled one.

> > +static bool level_leaf(const xen_cpuid_leaf_t *l1, const xen_cpuid_leaf_t *l2,
> > +                       xen_cpuid_leaf_t *out)
> > +{
> > +    *out = (xen_cpuid_leaf_t){
> > +        .leaf = l1->leaf,
> > +        .subleaf = l2->subleaf,
> 
> Since ->leaf and ->subleaf ought to match anyway, I think it would
> look less odd if both initializers were taken from consistent source.

Sure, my bad.

> > +    };
> > +
> > +    switch ( l1->leaf )
> > +    {
> > +    case 0x1:
> > +    case 0x80000001:
> > +        out->c = l1->c & l2->c;
> > +        out->d = l1->d & l2->d;
> > +        return true;
> > +
> > +    case 0xd:
> > +        if ( l1->subleaf != 1 )
> > +            break;
> > +        /*
> > +         * Only take Da1 into account, the rest of subleaves will be dropped
> > +         * and recalculated by recalculate_xstate.
> > +         */
> > +        out->a = l1->a & l2->a;
> > +        return true;
> > +
> > +    case 0x7:
> > +        if ( l1->subleaf )
> > +            /* subleaf 0 EAX contains the max subleaf count. */
> > +            out->a = l1->a & l2->a;
> 
>         else
>             out->a = min(l1->a, l2->a);
> 
> ? Or is the result from here then further passed to
> x86_cpuid_policy_shrink_max_leaves() (not visible from this patch)?
> (If not, the same would apply to all other multi-subleaf leaves.)

Hm, might be worth to set all the max fields directly in
xc_cpu_policy_calc_compatible and also add a call to
x86_cpuid_policy_shrink_max_leaves after the leveling is done.

> > +        out->b = l1->b & l2->b;
> > +        out->c = l1->c & l2->c;
> > +        out->d = l1->d & l2->d;
> > +        return true;
> > +
> > +    case 0x80000007:
> > +        out->d = l1->d & l2->d;
> > +        return true;
> > +
> > +    case 0x80000008:
> > +        out->b = l1->b & l2->b;
> > +        return true;
> > +
> > +    case 0x80000021:
> > +        out->a = l1->a & l2->a;
> > +        return true;
> > +    }
> > +
> > +    return false;
> > +}
> > +
> > +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)
> 
> I have to admit that I find these two "const" misleading here. You
> don't equally constify the other two parameters (which would e.g. be
> xc_interface *const xch), and I don't think doing so is common
> practice elsewhere. And what p1 and p2 point to is specifically non-
> const (and cannot be const), due to ...
> 
> > +{
> > +    unsigned int nr_leaves, nr_msrs, i, index;
> > +    unsigned int p1_nr_leaves, p2_nr_leaves;
> > +    unsigned int p1_nr_entries, p2_nr_entries;
> > +    int rc;
> > +
> > +    p1_nr_leaves = p2_nr_leaves = ARRAY_SIZE(p1->leaves);
> > +    p1_nr_entries = p2_nr_entries = ARRAY_SIZE(p1->entries);
> > +
> > +    rc = xc_cpu_policy_serialise(xch, p1, p1->leaves, &p1_nr_leaves,
> > +                                 p1->entries, &p1_nr_entries);
> > +    if ( rc )
> > +        return rc;
> > +    rc = xc_cpu_policy_serialise(xch, p2, p2->leaves, &p2_nr_leaves,
> > +                                 p2->entries, &p2_nr_entries);
> 
> ... these two calls.

Right, that's a leftover from previously, where xc_cpu_policy_t didn't
also contain a buffer to store the serialized version.

Thanks, Roger.

Re: [PATCH v3 07/13] libs/guest: obtain a compatible cpu policy from two input ones

[Jan Beulich]

On 04.05.2021 13:56, Roger Pau Monné wrote:
> On Mon, May 03, 2021 at 12:43:08PM +0200, Jan Beulich wrote:
>> On 30.04.2021 17:52, Roger Pau Monne wrote:
>>> +/* Only level featuresets so far. */
>>
>> I have to admit that I don't think I see all the implications from
>> this implementation restriction. All other leaves get dropped by
>> the caller, but it's not clear to me what this means wrt what the
>> guest is ultimately going to get to see.
> 
> This aims to be based on what XenServer does, which I was told is to
> level the featuresets. I think the caller of the function will have to
> fill the part of the policy that cannot be leveled. It's likely new
> helpers will be added to do that as required.
> 
> One option would be to get the default policy for the guest and then
> use xc_cpu_policy_update_cpuid to apply the leveled one.

Could such further plans perhaps be outlined (to a degree at least)
in the description?

Jan

Re: [PATCH v3 07/13] libs/guest: obtain a compatible cpu policy from two input ones

[Andrew Cooper]

On 30/04/2021 16:52, 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
> feature 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>
> ---
> Changes since v2:
>  - Add some comments.
>  - Remove stray double semicolon.
>  - AND all 0x7 subleaves (except 0.EAX).
>  - Explicitly handle MSR indexes in a switch statement.
>  - Error out when an unhandled MSR is found.
>  - Add handling of leaf 0x80000021.
>
> Changes since v1:
>  - Only AND the feature parts of cpuid.
>  - Use a binary search to find the matching leaves and msr entries.
>  - Remove default case from MSR level function.
> ---
>  tools/include/xen-tools/libs.h    |   5 ++
>  tools/include/xenctrl.h           |   4 +
>  tools/libs/guest/xg_cpuid_x86.c   | 137 ++++++++++++++++++++++++++++++
>  tools/libs/light/libxl_internal.h |   2 -

This *needs* to be in libx86.  I don't particularly mind if you start
with it behind #ifdef __XEN__ (I'm still sure we'll need it in the
hypervisor), but this, more than just about anything else, needs to be
covered by the unit tests.

Next, you need to follow the same structure as Xen's cpuid.c for
deriving policies.  You can't just loop through the two serialised forms
like this.

To start with, you want to calculate the min of a/b->max_leaf, then loop
over that pulling information sideways from a/b.

For MSRs, all but MSR_INTEL_PLATFORM_INFO are CPUID qualified, so need
to look like:

if ( out.cpuid.feat.arch_caps )
    out.msr.arch_caps.raw =
        ((a.msr.arch_caps.raw ^ INV_MASK) &
         (b.msr.arch_caps.raw ^ INV_MASK)) ^ INV_MASK;

Where INV_MASK is the mask of arch caps bits which want inverted
polarity.  (Name subject to change - perhap ARCH_CAPS_POLARITY_MASK ?)


I'm sure I had some work starting this somewhere.  I'll see if I can
locate it.

~Andrew

[PATCH v3 08/13] libs/guest: make a cpu policy compatible with older Xen versions

[Roger Pau Monne]

Older Xen versions used to expose some CPUID bits which are no longer
exposed by default. In order to keep a compatible behavior with
guests migrated from versions of Xen that don't encode the CPUID data
on the migration stream introduce a function that sets the same bits
as older Xen versions.

This is pulled out from xc_cpuid_apply_policy which already has this
logic present.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - Move comments and explicitly mention pre-4.14 Xen.
---
 tools/include/xenctrl.h         |  4 +++
 tools/libs/guest/xg_cpuid_x86.c | 58 ++++++++++++++++++++++++---------
 2 files changed, 47 insertions(+), 15 deletions(-)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index c41d794683c..89a73fd6823 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2627,6 +2627,10 @@ int xc_cpu_policy_calc_compatible(xc_interface *xch,
                                   const xc_cpu_policy_t p2,
                                   xc_cpu_policy_t out);
 
+/* Make a policy compatible with pre-4.14 Xen versions. */
+int xc_cpu_policy_make_compatible(xc_interface *xch, xc_cpu_policy_t policy,
+                                  bool hvm);
+
 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/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index be2056469aa..855d252e067 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -446,6 +446,7 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
     unsigned int i, nr_leaves, nr_msrs;
     xen_cpuid_leaf_t *leaves = NULL;
     struct cpuid_policy *p = NULL;
+    struct xc_cpu_policy policy = { };
     uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
     uint32_t host_featureset[FEATURESET_NR_ENTRIES] = {};
     uint32_t len = ARRAY_SIZE(host_featureset);
@@ -510,21 +511,9 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
 
     if ( restore )
     {
-        /*
-         * Account for feature which have been disabled by default since Xen 4.13,
-         * so migrated-in VM's don't risk seeing features disappearing.
-         */
-        p->basic.rdrand = test_bit(X86_FEATURE_RDRAND, host_featureset);
-
-        if ( di.hvm )
-        {
-            p->feat.mpx = test_bit(X86_FEATURE_MPX, host_featureset);
-        }
-
-        /* Clamp maximum leaves to the ones supported on 4.12. */
-        p->basic.max_leaf = min(p->basic.max_leaf, 0xdu);
-        p->feat.max_subleaf = 0;
-        p->extd.max_leaf = min(p->extd.max_leaf, 0x1cu);
+        policy.cpuid = *p;
+        xc_cpu_policy_make_compatible(xch, &policy, di.hvm);
+        *p = policy.cpuid;
     }
 
     if ( featureset )
@@ -1086,3 +1075,42 @@ int xc_cpu_policy_calc_compatible(xc_interface *xch,
 
     return rc;
 }
+
+int xc_cpu_policy_make_compatible(xc_interface *xch, xc_cpu_policy_t policy,
+                                  bool hvm)
+{
+    xc_cpu_policy_t host;
+    int rc;
+
+    host = xc_cpu_policy_init();
+    if ( !host )
+    {
+        errno = ENOMEM;
+        return -1;
+    }
+
+    rc = xc_cpu_policy_get_system(xch, XEN_SYSCTL_cpu_policy_host, host);
+    if ( rc )
+    {
+        ERROR("Failed to get host policy");
+        goto out;
+    }
+
+    /*
+     * Account for features which have been disabled by default since Xen 4.13,
+     * so migrated-in VM's don't risk seeing features disappearing.
+     */
+    policy->cpuid.basic.rdrand = host->cpuid.basic.rdrand;
+
+    if ( hvm )
+        policy->cpuid.feat.mpx = host->cpuid.feat.mpx;
+
+    /* Clamp maximum leaves to the ones supported on 4.12. */
+    policy->cpuid.basic.max_leaf = min(policy->cpuid.basic.max_leaf, 0xdu);
+    policy->cpuid.feat.max_subleaf = 0;
+    policy->cpuid.extd.max_leaf = min(policy->cpuid.extd.max_leaf, 0x1cu);
+
+ out:
+    xc_cpu_policy_destroy(host);
+    return rc;
+}
-- 
2.31.1

Re: [PATCH v3 08/13] libs/guest: make a cpu policy compatible with older Xen versions

[Jan Beulich]

On 30.04.2021 17:52, Roger Pau Monne wrote:
> @@ -1086,3 +1075,42 @@ int xc_cpu_policy_calc_compatible(xc_interface *xch,
>  
>      return rc;
>  }
> +
> +int xc_cpu_policy_make_compatible(xc_interface *xch, xc_cpu_policy_t policy,
> +                                  bool hvm)

I'm concerned of the naming, and in particular the two very different
meanings of "compatible" for xc_cpu_policy_calc_compatible() and this
new one. I'm afraid I don't have a good suggestion though, short of
making the name even longer and inserting "backwards".

Jan

> +{
> +    xc_cpu_policy_t host;
> +    int rc;
> +
> +    host = xc_cpu_policy_init();
> +    if ( !host )
> +    {
> +        errno = ENOMEM;
> +        return -1;
> +    }
> +
> +    rc = xc_cpu_policy_get_system(xch, XEN_SYSCTL_cpu_policy_host, host);
> +    if ( rc )
> +    {
> +        ERROR("Failed to get host policy");
> +        goto out;
> +    }
> +
> +    /*
> +     * Account for features which have been disabled by default since Xen 4.13,
> +     * so migrated-in VM's don't risk seeing features disappearing.
> +     */
> +    policy->cpuid.basic.rdrand = host->cpuid.basic.rdrand;
> +
> +    if ( hvm )
> +        policy->cpuid.feat.mpx = host->cpuid.feat.mpx;
> +
> +    /* Clamp maximum leaves to the ones supported on 4.12. */
> +    policy->cpuid.basic.max_leaf = min(policy->cpuid.basic.max_leaf, 0xdu);
> +    policy->cpuid.feat.max_subleaf = 0;
> +    policy->cpuid.extd.max_leaf = min(policy->cpuid.extd.max_leaf, 0x1cu);
> +
> + out:
> +    xc_cpu_policy_destroy(host);
> +    return rc;
> +}
> 

Re: [PATCH v3 08/13] libs/guest: make a cpu policy compatible with older Xen versions

[Roger Pau Monné]

On Mon, May 03, 2021 at 01:09:41PM +0200, Jan Beulich wrote:
> On 30.04.2021 17:52, Roger Pau Monne wrote:
> > @@ -1086,3 +1075,42 @@ int xc_cpu_policy_calc_compatible(xc_interface *xch,
> >  
> >      return rc;
> >  }
> > +
> > +int xc_cpu_policy_make_compatible(xc_interface *xch, xc_cpu_policy_t policy,
> > +                                  bool hvm)
> 
> I'm concerned of the naming, and in particular the two very different
> meanings of "compatible" for xc_cpu_policy_calc_compatible() and this
> new one. I'm afraid I don't have a good suggestion though, short of
> making the name even longer and inserting "backwards".

Would xc_cpu_policy_make_compat_412 be acceptable?

That's the more concise one I can think of.

Thanks, Roger.

Re: [PATCH v3 08/13] libs/guest: make a cpu policy compatible with older Xen versions

[Jan Beulich]

On 04.05.2021 17:34, Roger Pau Monné wrote:
> On Mon, May 03, 2021 at 01:09:41PM +0200, Jan Beulich wrote:
>> On 30.04.2021 17:52, Roger Pau Monne wrote:
>>> @@ -1086,3 +1075,42 @@ int xc_cpu_policy_calc_compatible(xc_interface *xch,
>>>  
>>>      return rc;
>>>  }
>>> +
>>> +int xc_cpu_policy_make_compatible(xc_interface *xch, xc_cpu_policy_t policy,
>>> +                                  bool hvm)
>>
>> I'm concerned of the naming, and in particular the two very different
>> meanings of "compatible" for xc_cpu_policy_calc_compatible() and this
>> new one. I'm afraid I don't have a good suggestion though, short of
>> making the name even longer and inserting "backwards".
> 
> Would xc_cpu_policy_make_compat_412 be acceptable?
> 
> That's the more concise one I can think of.

Hmm, maybe (perhaps with an underscore inserted between 4 and 12). Yet
(sorry) a comment in the function says "since Xen 4.13", which includes
changes that have happened later. Therefore it's not really clear to me
whether the function really _only_ deals with the 4.12 / 4.13 boundary.

Jan

Re: [PATCH v3 08/13] libs/guest: make a cpu policy compatible with older Xen versions

[Roger Pau Monné]

On Wed, May 05, 2021 at 09:42:09AM +0200, Jan Beulich wrote:
> On 04.05.2021 17:34, Roger Pau Monné wrote:
> > On Mon, May 03, 2021 at 01:09:41PM +0200, Jan Beulich wrote:
> >> On 30.04.2021 17:52, Roger Pau Monne wrote:
> >>> @@ -1086,3 +1075,42 @@ int xc_cpu_policy_calc_compatible(xc_interface *xch,
> >>>  
> >>>      return rc;
> >>>  }
> >>> +
> >>> +int xc_cpu_policy_make_compatible(xc_interface *xch, xc_cpu_policy_t policy,
> >>> +                                  bool hvm)
> >>
> >> I'm concerned of the naming, and in particular the two very different
> >> meanings of "compatible" for xc_cpu_policy_calc_compatible() and this
> >> new one. I'm afraid I don't have a good suggestion though, short of
> >> making the name even longer and inserting "backwards".
> > 
> > Would xc_cpu_policy_make_compat_412 be acceptable?
> > 
> > That's the more concise one I can think of.
> 
> Hmm, maybe (perhaps with an underscore inserted between 4 and 12). Yet
> (sorry) a comment in the function says "since Xen 4.13", which includes
> changes that have happened later. Therefore it's not really clear to me
> whether the function really _only_ deals with the 4.12 / 4.13 boundary.

It should deal with any changes in the default cpuid policy that
happened after (and not including) Xen 4.12. So resulting policy is
compatible with the behaviour that Xen 4.12 had. Any changes made in
Xen 4.13 and further versions should be accounted for here.

Thanks, Roger.

Re: [PATCH v3 08/13] libs/guest: make a cpu policy compatible with older Xen versions

[Jan Beulich]

On 06.05.2021 12:23, Roger Pau Monné wrote:
> On Wed, May 05, 2021 at 09:42:09AM +0200, Jan Beulich wrote:
>> On 04.05.2021 17:34, Roger Pau Monné wrote:
>>> On Mon, May 03, 2021 at 01:09:41PM +0200, Jan Beulich wrote:
>>>> On 30.04.2021 17:52, Roger Pau Monne wrote:
>>>>> @@ -1086,3 +1075,42 @@ int xc_cpu_policy_calc_compatible(xc_interface *xch,
>>>>>  
>>>>>      return rc;
>>>>>  }
>>>>> +
>>>>> +int xc_cpu_policy_make_compatible(xc_interface *xch, xc_cpu_policy_t policy,
>>>>> +                                  bool hvm)
>>>>
>>>> I'm concerned of the naming, and in particular the two very different
>>>> meanings of "compatible" for xc_cpu_policy_calc_compatible() and this
>>>> new one. I'm afraid I don't have a good suggestion though, short of
>>>> making the name even longer and inserting "backwards".
>>>
>>> Would xc_cpu_policy_make_compat_412 be acceptable?
>>>
>>> That's the more concise one I can think of.
>>
>> Hmm, maybe (perhaps with an underscore inserted between 4 and 12). Yet
>> (sorry) a comment in the function says "since Xen 4.13", which includes
>> changes that have happened later. Therefore it's not really clear to me
>> whether the function really _only_ deals with the 4.12 / 4.13 boundary.
> 
> It should deal with any changes in the default cpuid policy that
> happened after (and not including) Xen 4.12. So resulting policy is
> compatible with the behaviour that Xen 4.12 had. Any changes made in
> Xen 4.13 and further versions should be accounted for here.

I see.

Jan

[PATCH v3 09/13] libs/guest: introduce helper set cpu topology in cpu policy

[Roger Pau Monne]

This logic is pulled out from xc_cpuid_apply_policy and placed into a
separate helper. Note the legacy part of the introduced function, as
long term Xen will require a proper topology setter function capable
of expressing a more diverse set of topologies.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 - s/xc_cpu_policy_topology/xc_cpu_policy_legacy_topology/
---
 tools/include/xenctrl.h         |   4 +
 tools/libs/guest/xg_cpuid_x86.c | 182 +++++++++++++++++---------------
 2 files changed, 103 insertions(+), 83 deletions(-)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 89a73fd6823..ec184bccd3f 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2631,6 +2631,10 @@ int xc_cpu_policy_calc_compatible(xc_interface *xch,
 int xc_cpu_policy_make_compatible(xc_interface *xch, xc_cpu_policy_t policy,
                                   bool hvm);
 
+/* Setup the legacy policy topology. */
+int xc_cpu_policy_legacy_topology(xc_interface *xch, xc_cpu_policy_t policy,
+                                  bool hvm);
+
 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/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index 855d252e067..dbf2ef67ee0 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -443,13 +443,11 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
 {
     int rc;
     xc_dominfo_t di;
-    unsigned int i, nr_leaves, nr_msrs;
+    unsigned int nr_leaves, nr_msrs;
     xen_cpuid_leaf_t *leaves = NULL;
     struct cpuid_policy *p = NULL;
     struct xc_cpu_policy policy = { };
     uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
-    uint32_t host_featureset[FEATURESET_NR_ENTRIES] = {};
-    uint32_t len = ARRAY_SIZE(host_featureset);
 
     if ( xc_domain_getinfo(xch, domid, 1, &di) != 1 ||
          di.domid != domid )
@@ -472,22 +470,6 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
          (p = calloc(1, sizeof(*p))) == NULL )
         goto out;
 
-    /* Get the host policy. */
-    rc = xc_get_cpu_featureset(xch, XEN_SYSCTL_cpu_featureset_host,
-                               &len, host_featureset);
-    if ( rc )
-    {
-        /* Tolerate "buffer too small", as we've got the bits we need. */
-        if ( errno == ENOBUFS )
-            rc = 0;
-        else
-        {
-            PERROR("Failed to obtain host featureset");
-            rc = -errno;
-            goto out;
-        }
-    }
-
     /* Get the domain's default policy. */
     nr_msrs = 0;
     rc = get_system_cpu_policy(xch, di.hvm ? XEN_SYSCTL_cpu_policy_hvm_default
@@ -571,70 +553,11 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
         }
     }
 
-    if ( !di.hvm )
-    {
-        /*
-         * On hardware without CPUID Faulting, PV guests see real topology.
-         * As a consequence, they also need to see the host htt/cmp fields.
-         */
-        p->basic.htt       = test_bit(X86_FEATURE_HTT, host_featureset);
-        p->extd.cmp_legacy = test_bit(X86_FEATURE_CMP_LEGACY, host_featureset);
-    }
-    else
-    {
-        /*
-         * Topology for HVM guests is entirely controlled by Xen.  For now, we
-         * hardcode APIC_ID = vcpu_id * 2 to give the illusion of no SMT.
-         */
-        p->basic.htt = true;
-        p->extd.cmp_legacy = false;
-
-        /*
-         * Leaf 1 EBX[23:16] is Maximum Logical Processors Per Package.
-         * Update to reflect vLAPIC_ID = vCPU_ID * 2, but make sure to avoid
-         * overflow.
-         */
-        if ( !(p->basic.lppp & 0x80) )
-            p->basic.lppp *= 2;
-
-        switch ( p->x86_vendor )
-        {
-        case X86_VENDOR_INTEL:
-            for ( i = 0; (p->cache.subleaf[i].type &&
-                          i < ARRAY_SIZE(p->cache.raw)); ++i )
-            {
-                p->cache.subleaf[i].cores_per_package =
-                    (p->cache.subleaf[i].cores_per_package << 1) | 1;
-                p->cache.subleaf[i].threads_per_cache = 0;
-            }
-            break;
-
-        case X86_VENDOR_AMD:
-        case X86_VENDOR_HYGON:
-            /*
-             * Leaf 0x80000008 ECX[15:12] is ApicIdCoreSize.
-             * Leaf 0x80000008 ECX[7:0] is NumberOfCores (minus one).
-             * Update to reflect vLAPIC_ID = vCPU_ID * 2.  But avoid
-             * - overflow,
-             * - going out of sync with leaf 1 EBX[23:16],
-             * - incrementing ApicIdCoreSize when it's zero (which changes the
-             *   meaning of bits 7:0).
-             *
-             * UPDATE: I addition to avoiding overflow, some
-             * proprietary operating systems have trouble with
-             * apic_id_size values greater than 7.  Limit the value to
-             * 7 for now.
-             */
-            if ( p->extd.nc < 0x7f )
-            {
-                if ( p->extd.apic_id_size != 0 && p->extd.apic_id_size < 0x7 )
-                    p->extd.apic_id_size++;
-
-                p->extd.nc = (p->extd.nc << 1) | 1;
-            }
-            break;
-        }
-    }
+    policy.cpuid = *p;
+    rc = xc_cpu_policy_legacy_topology(xch, &policy, di.hvm);
+    if ( rc )
+        goto out;
+    *p = policy.cpuid;
 
     rc = x86_cpuid_copy_to_buffer(p, leaves, &nr_leaves);
     if ( rc )
@@ -1114,3 +1037,96 @@ int xc_cpu_policy_make_compatible(xc_interface *xch, xc_cpu_policy_t policy,
     xc_cpu_policy_destroy(host);
     return rc;
 }
+
+int xc_cpu_policy_legacy_topology(xc_interface *xch, xc_cpu_policy_t policy,
+                                  bool hvm)
+{
+    if ( !hvm )
+    {
+        xc_cpu_policy_t host;
+        int rc;
+
+        host = xc_cpu_policy_init();
+        if ( !host )
+        {
+            errno = ENOMEM;
+            return -1;
+        }
+
+        rc = xc_cpu_policy_get_system(xch, XEN_SYSCTL_cpu_policy_host, host);
+        if ( rc )
+        {
+            ERROR("Failed to get host policy");
+            xc_cpu_policy_destroy(host);
+            return rc;
+        }
+
+
+        /*
+         * On hardware without CPUID Faulting, PV guests see real topology.
+         * As a consequence, they also need to see the host htt/cmp fields.
+         */
+        policy->cpuid.basic.htt = host->cpuid.basic.htt;
+        policy->cpuid.extd.cmp_legacy = host->cpuid.extd.cmp_legacy;
+    }
+    else
+    {
+        unsigned int i;
+
+        /*
+         * Topology for HVM guests is entirely controlled by Xen.  For now, we
+         * hardcode APIC_ID = vcpu_id * 2 to give the illusion of no SMT.
+         */
+        policy->cpuid.basic.htt = true;
+        policy->cpuid.extd.cmp_legacy = false;
+
+        /*
+         * Leaf 1 EBX[23:16] is Maximum Logical Processors Per Package.
+         * Update to reflect vLAPIC_ID = vCPU_ID * 2, but make sure to avoid
+         * overflow.
+         */
+        if ( !(policy->cpuid.basic.lppp & 0x80) )
+            policy->cpuid.basic.lppp *= 2;
+
+        switch ( policy->cpuid.x86_vendor )
+        {
+        case X86_VENDOR_INTEL:
+            for ( i = 0; (policy->cpuid.cache.subleaf[i].type &&
+                          i < ARRAY_SIZE(policy->cpuid.cache.raw)); ++i )
+            {
+                policy->cpuid.cache.subleaf[i].cores_per_package =
+                  (policy->cpuid.cache.subleaf[i].cores_per_package << 1) | 1;
+                policy->cpuid.cache.subleaf[i].threads_per_cache = 0;
+            }
+            break;
+
+        case X86_VENDOR_AMD:
+        case X86_VENDOR_HYGON:
+            /*
+             * Leaf 0x80000008 ECX[15:12] is ApicIdCoreSize.
+             * Leaf 0x80000008 ECX[7:0] is NumberOfCores (minus one).
+             * Update to reflect vLAPIC_ID = vCPU_ID * 2.  But avoid
+             * - overflow,
+             * - going out of sync with leaf 1 EBX[23:16],
+             * - incrementing ApicIdCoreSize when it's zero (which changes the
+             *   meaning of bits 7:0).
+             *
+             * UPDATE: I addition to avoiding overflow, some
+             * proprietary operating systems have trouble with
+             * apic_id_size values greater than 7.  Limit the value to
+             * 7 for now.
+             */
+            if ( policy->cpuid.extd.nc < 0x7f )
+            {
+                if ( policy->cpuid.extd.apic_id_size != 0 &&
+                     policy->cpuid.extd.apic_id_size < 0x7 )
+                    policy->cpuid.extd.apic_id_size++;
+
+                policy->cpuid.extd.nc = (policy->cpuid.extd.nc << 1) | 1;
+            }
+            break;
+        }
+    }
+
+    return 0;
+}
-- 
2.31.1

[PATCH v3 10/13] libs/guest: rework xc_cpuid_xend_policy

[Roger Pau Monne]

Rename xc_cpuid_xend_policy to xc_cpu_policy_apply_cpuid and make it
public. Modify the function internally to use the new xc_cpu_policy_*
set of functions. Also don't apply the passed policy to a domain
directly, and instead modify the provided xc_cpu_policy_t. The caller
will be responsible of applying the modified cpu policy to the domain.

Note that further patches will end up removing this function, as the
callers should have the necessary helpers to modify an xc_cpu_policy_t
themselves.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/include/xenctrl.h         |   4 +
 tools/libs/guest/xg_cpuid_x86.c | 182 ++++++++++++++------------------
 2 files changed, 84 insertions(+), 102 deletions(-)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index ec184bccd3f..ce0785f7654 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2635,6 +2635,10 @@ int xc_cpu_policy_make_compatible(xc_interface *xch, xc_cpu_policy_t policy,
 int xc_cpu_policy_legacy_topology(xc_interface *xch, xc_cpu_policy_t policy,
                                   bool hvm);
 
+/* Apply an xc_xend_cpuid object to the policy. */
+int xc_cpu_policy_apply_cpuid(xc_interface *xch, xc_cpu_policy_t policy,
+                              const struct xc_xend_cpuid *cpuid, bool hvm);
+
 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/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index dbf2ef67ee0..d313a093af6 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -288,124 +288,107 @@ static xen_cpuid_leaf_t *find_leaf(
     return bsearch(&key, leaves, nr_leaves, sizeof(*leaves), compare_leaves);
 }
 
-static int xc_cpuid_xend_policy(
-    xc_interface *xch, uint32_t domid, const struct xc_xend_cpuid *xend)
+int xc_cpu_policy_apply_cpuid(xc_interface *xch, xc_cpu_policy_t policy,
+                              const struct xc_xend_cpuid *cpuid, bool hvm)
 {
     int rc;
-    xc_dominfo_t di;
-    unsigned int nr_leaves, nr_msrs;
-    uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
-    /*
-     * Three full policies.  The host, default for the domain type,
-     * and domain current.
-     */
-    xen_cpuid_leaf_t *host = NULL, *def = NULL, *cur = NULL;
-    unsigned int nr_host, nr_def, nr_cur;
-
-    if ( xc_domain_getinfo(xch, domid, 1, &di) != 1 ||
-         di.domid != domid )
-    {
-        ERROR("Failed to obtain d%d info", domid);
-        rc = -ESRCH;
-        goto fail;
-    }
-
-    rc = xc_cpu_policy_get_size(xch, &nr_leaves, &nr_msrs);
-    if ( rc )
-    {
-        PERROR("Failed to obtain policy info size");
-        rc = -errno;
-        goto fail;
-    }
+    xc_cpu_policy_t host = NULL, def = NULL;
 
-    rc = -ENOMEM;
-    if ( (host = calloc(nr_leaves, sizeof(*host))) == NULL ||
-         (def  = calloc(nr_leaves, sizeof(*def)))  == NULL ||
-         (cur  = calloc(nr_leaves, sizeof(*cur)))  == NULL )
-    {
-        ERROR("Unable to allocate memory for %u CPUID leaves", nr_leaves);
-        goto fail;
-    }
-
-    /* Get the domain's current policy. */
-    nr_msrs = 0;
-    nr_cur = nr_leaves;
-    rc = get_domain_cpu_policy(xch, domid, &nr_cur, cur, &nr_msrs, NULL);
-    if ( rc )
+    host = xc_cpu_policy_init();
+    def = xc_cpu_policy_init();
+    if ( !host || !def )
     {
-        PERROR("Failed to obtain d%d current policy", domid);
-        rc = -errno;
-        goto fail;
+        PERROR("Failed to init policies");
+        rc = -ENOMEM;
+        goto out;
     }
 
     /* Get the domain type's default policy. */
-    nr_msrs = 0;
-    nr_def = nr_leaves;
-    rc = get_system_cpu_policy(xch, di.hvm ? XEN_SYSCTL_cpu_policy_hvm_default
+    rc = xc_cpu_policy_get_system(xch, hvm ? XEN_SYSCTL_cpu_policy_hvm_default
                                            : XEN_SYSCTL_cpu_policy_pv_default,
-                               &nr_def, def, &nr_msrs, NULL);
+                                  def);
     if ( rc )
     {
-        PERROR("Failed to obtain %s def policy", di.hvm ? "hvm" : "pv");
-        rc = -errno;
-        goto fail;
+        PERROR("Failed to obtain %s def policy", hvm ? "hvm" : "pv");
+        goto out;
     }
 
     /* Get the host policy. */
-    nr_msrs = 0;
-    nr_host = nr_leaves;
-    rc = get_system_cpu_policy(xch, XEN_SYSCTL_cpu_policy_host,
-                               &nr_host, host, &nr_msrs, NULL);
+    rc = xc_cpu_policy_get_system(xch, XEN_SYSCTL_cpu_policy_host, host);
     if ( rc )
     {
         PERROR("Failed to obtain host policy");
-        rc = -errno;
-        goto fail;
+        goto out;
     }
 
     rc = -EINVAL;
-    for ( ; xend->leaf != XEN_CPUID_INPUT_UNUSED; ++xend )
+    for ( ; cpuid->leaf != XEN_CPUID_INPUT_UNUSED; ++cpuid )
     {
-        xen_cpuid_leaf_t *cur_leaf = find_leaf(cur, nr_cur,
-                                               xend->leaf, xend->subleaf);
-        const xen_cpuid_leaf_t *def_leaf = find_leaf(def, nr_def,
-                                                     xend->leaf, xend->subleaf);
-        const xen_cpuid_leaf_t *host_leaf = find_leaf(host, nr_host, xend->leaf,
-                                                      xend->subleaf);
-
-        if ( cur_leaf == NULL || def_leaf == NULL || host_leaf == NULL )
+        xen_cpuid_leaf_t cur_leaf;
+        xen_cpuid_leaf_t def_leaf;
+        xen_cpuid_leaf_t host_leaf;
+
+        rc = xc_cpu_policy_get_cpuid(xch, policy, cpuid->leaf, cpuid->subleaf,
+                                     &cur_leaf);
+        if ( rc )
         {
-            ERROR("Missing leaf %#x, subleaf %#x", xend->leaf, xend->subleaf);
-            goto fail;
+            ERROR("Failed to get current policy leaf %#x subleaf %#x",
+                  cpuid->leaf, cpuid->subleaf);
+            goto out;
+        }
+        rc = xc_cpu_policy_get_cpuid(xch, def, cpuid->leaf, cpuid->subleaf,
+                                     &def_leaf);
+        if ( rc )
+        {
+            ERROR("Failed to get def policy leaf %#x subleaf %#x",
+                  cpuid->leaf, cpuid->subleaf);
+            goto out;
+        }
+        rc = xc_cpu_policy_get_cpuid(xch, host, cpuid->leaf, cpuid->subleaf,
+                                     &host_leaf);
+        if ( rc )
+        {
+            ERROR("Failed to get host policy leaf %#x subleaf %#x",
+                  cpuid->leaf, cpuid->subleaf);
+            goto out;
         }
 
-        for ( unsigned int i = 0; i < ARRAY_SIZE(xend->policy); i++ )
+        for ( unsigned int i = 0; i < ARRAY_SIZE(cpuid->policy); i++ )
         {
-            uint32_t *cur_reg = &cur_leaf->a + i;
-            const uint32_t *def_reg = &def_leaf->a + i;
-            const uint32_t *host_reg = &host_leaf->a + i;
+            uint32_t *cur_reg = &cur_leaf.a + i;
+            const uint32_t *def_reg = &def_leaf.a + i;
+            const uint32_t *host_reg = &host_leaf.a + i;
 
-            if ( xend->policy[i] == NULL )
+            if ( cpuid->policy[i] == NULL )
                 continue;
 
             for ( unsigned int j = 0; j < 32; j++ )
             {
                 bool val;
 
-                if ( xend->policy[i][j] == '1' )
+                switch ( cpuid->policy[i][j] )
+                {
+                case '1':
                     val = true;
-                else if ( xend->policy[i][j] == '0' )
+                    break;
+
+                case '0':
                     val = false;
-                else if ( xend->policy[i][j] == 'x' )
+                    break;
+
+                case 'x':
                     val = test_bit(31 - j, def_reg);
-                else if ( xend->policy[i][j] == 'k' ||
-                          xend->policy[i][j] == 's' )
+                    break;
+
+                case 'k':
+                case 's':
                     val = test_bit(31 - j, host_reg);
-                else
-                {
+                    break;
+
+                default:
                     ERROR("Bad character '%c' in policy[%d] string '%s'",
-                          xend->policy[i][j], i, xend->policy[i]);
-                    goto fail;
+                          cpuid->policy[i][j], i, cpuid->policy[i]);
+                    goto out;
                 }
 
                 clear_bit(31 - j, cur_reg);
@@ -413,25 +396,19 @@ static int xc_cpuid_xend_policy(
                     set_bit(31 - j, cur_reg);
             }
         }
-    }
 
-    /* Feed the transformed currrent policy back up to Xen. */
-    rc = xc_set_domain_cpu_policy(xch, domid, nr_cur, cur, 0, NULL,
-                                  &err_leaf, &err_subleaf, &err_msr);
-    if ( rc )
-    {
-        PERROR("Failed to set d%d's policy (err leaf %#x, subleaf %#x, msr %#x)",
-               domid, err_leaf, err_subleaf, err_msr);
-        rc = -errno;
-        goto fail;
+        rc = xc_cpu_policy_update_cpuid(xch, policy, &cur_leaf, 1);
+        if ( rc )
+        {
+            PERROR("Failed to set policy leaf %#x subleaf %#x",
+                   cpuid->leaf, cpuid->subleaf);
+            goto out;
+        }
     }
 
-    /* Success! */
-
- fail:
-    free(cur);
-    free(def);
-    free(host);
+ out:
+    xc_cpu_policy_destroy(def);
+    xc_cpu_policy_destroy(host);
 
     return rc;
 }
@@ -439,7 +416,7 @@ static int xc_cpuid_xend_policy(
 int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
                           const uint32_t *featureset, unsigned int nr_features,
                           bool pae, bool itsc, bool nested_virt,
-                          const struct xc_xend_cpuid *xend)
+                          const struct xc_xend_cpuid *cpuid)
 {
     int rc;
     xc_dominfo_t di;
@@ -559,6 +536,10 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
         goto out;
     *p = policy.cpuid;
 
+    rc = xc_cpu_policy_apply_cpuid(xch, &policy, cpuid, di.hvm);
+    if ( rc )
+        goto out;
+
     rc = x86_cpuid_copy_to_buffer(p, leaves, &nr_leaves);
     if ( rc )
     {
@@ -576,9 +557,6 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
         goto out;
     }
 
-    if ( xend && (rc = xc_cpuid_xend_policy(xch, domid, xend)) )
-        goto out;
-
     rc = 0;
 
 out:
-- 
2.31.1

[PATCH v3 11/13] libs/guest: apply a featureset into a cpu policy

[Roger Pau Monne]

Pull out the code from xc_cpuid_apply_policy that applies a featureset
to a cpu policy and place it on it's own standalone function that's
part of the public interface.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/include/xenctrl.h         |  5 ++
 tools/libs/guest/xg_cpuid_x86.c | 95 ++++++++++++++++++++-------------
 2 files changed, 62 insertions(+), 38 deletions(-)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index ce0785f7654..3fef954d1d1 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2639,6 +2639,11 @@ int xc_cpu_policy_legacy_topology(xc_interface *xch, xc_cpu_policy_t policy,
 int xc_cpu_policy_apply_cpuid(xc_interface *xch, xc_cpu_policy_t policy,
                               const struct xc_xend_cpuid *cpuid, bool hvm);
 
+/* Apply a featureset to the policy. */
+int xc_cpu_policy_apply_featureset(xc_interface *xch, xc_cpu_policy_t policy,
+                                   const uint32_t *featureset,
+                                   unsigned int nr_features);
+
 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/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index d313a093af6..e2237e33709 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -477,46 +477,15 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
 
     if ( featureset )
     {
-        uint32_t disabled_features[FEATURESET_NR_ENTRIES],
-            feat[FEATURESET_NR_ENTRIES] = {};
-        static const uint32_t deep_features[] = INIT_DEEP_FEATURES;
-        unsigned int i, b;
-
-        /*
-         * The user supplied featureset may be shorter or longer than
-         * FEATURESET_NR_ENTRIES.  Shorter is fine, and we will zero-extend.
-         * Longer is fine, so long as it only padded with zeros.
-         */
-        unsigned int user_len = min(FEATURESET_NR_ENTRIES + 0u, nr_features);
-
-        /* Check for truncated set bits. */
-        rc = -EOPNOTSUPP;
-        for ( i = user_len; i < nr_features; ++i )
-            if ( featureset[i] != 0 )
-                goto out;
-
-        memcpy(feat, featureset, sizeof(*featureset) * user_len);
-
-        /* Disable deep dependencies of disabled features. */
-        for ( i = 0; i < ARRAY_SIZE(disabled_features); ++i )
-            disabled_features[i] = ~feat[i] & deep_features[i];
-
-        for ( b = 0; b < sizeof(disabled_features) * CHAR_BIT; ++b )
+        policy.cpuid = *p;
+        rc = xc_cpu_policy_apply_featureset(xch, &policy, featureset,
+                                            nr_features);
+        if ( rc )
         {
-            const uint32_t *dfs;
-
-            if ( !test_bit(b, disabled_features) ||
-                 !(dfs = x86_cpuid_lookup_deep_deps(b)) )
-                continue;
-
-            for ( i = 0; i < ARRAY_SIZE(disabled_features); ++i )
-            {
-                feat[i] &= ~dfs[i];
-                disabled_features[i] &= ~dfs[i];
-            }
+            ERROR("Failed to apply featureset to policy");
+            goto out;
         }
-
-        cpuid_featureset_to_policy(feat, p);
+        *p = policy.cpuid;
     }
     else
     {
@@ -1108,3 +1077,53 @@ int xc_cpu_policy_legacy_topology(xc_interface *xch, xc_cpu_policy_t policy,
 
     return 0;
 }
+
+int xc_cpu_policy_apply_featureset(xc_interface *xch, xc_cpu_policy_t policy,
+                                   const uint32_t *featureset,
+                                   unsigned int nr_features)
+{
+    uint32_t disabled_features[FEATURESET_NR_ENTRIES],
+        feat[FEATURESET_NR_ENTRIES] = {};
+    static const uint32_t deep_features[] = INIT_DEEP_FEATURES;
+    unsigned int i, b;
+
+    /*
+     * The user supplied featureset may be shorter or longer than
+     * FEATURESET_NR_ENTRIES.  Shorter is fine, and we will zero-extend.
+     * Longer is fine, so long as it only padded with zeros.
+     */
+    unsigned int user_len = min(FEATURESET_NR_ENTRIES + 0u, nr_features);
+
+    /* Check for truncated set bits. */
+    for ( i = user_len; i < nr_features; ++i )
+        if ( featureset[i] != 0 )
+        {
+            errno = EOPNOTSUPP;
+            return -1;
+        }
+
+    memcpy(feat, featureset, sizeof(*featureset) * user_len);
+
+    /* Disable deep dependencies of disabled features. */
+    for ( i = 0; i < ARRAY_SIZE(disabled_features); ++i )
+        disabled_features[i] = ~feat[i] & deep_features[i];
+
+    for ( b = 0; b < sizeof(disabled_features) * CHAR_BIT; ++b )
+    {
+        const uint32_t *dfs;
+
+        if ( !test_bit(b, disabled_features) ||
+             !(dfs = x86_cpuid_lookup_deep_deps(b)) )
+            continue;
+
+        for ( i = 0; i < ARRAY_SIZE(disabled_features); ++i )
+        {
+            feat[i] &= ~dfs[i];
+            disabled_features[i] &= ~dfs[i];
+        }
+    }
+
+    cpuid_featureset_to_policy(feat, &policy->cpuid);
+
+    return 0;
+}
-- 
2.31.1

[PATCH v3 12/13] libs/{light,guest}: implement xc_cpuid_apply_policy in libxl

[Roger Pau Monne]

With the addition of the xc_cpu_policy_* now libxl can have better
control over the cpu policy, this allows removing the
xc_cpuid_apply_policy function and instead coding the required bits by
libxl in libxl__cpuid_legacy directly.

Remove xc_cpuid_apply_policy.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v2:
 - Use 'r' for libxc return values.
 - Fix comment about making a cpu policy compatible.
 - Use LOG*D macros.
---
 tools/include/xenctrl.h         |  18 -----
 tools/libs/guest/xg_cpuid_x86.c | 122 --------------------------------
 tools/libs/light/libxl_cpuid.c  |  94 ++++++++++++++++++++++--
 3 files changed, 87 insertions(+), 147 deletions(-)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 3fef954d1d1..c6ee1142e8e 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -1890,24 +1890,6 @@ struct xc_xend_cpuid {
     char *policy[4];
 };
 
-/*
- * Make adjustments to the CPUID settings for a domain.
- *
- * This path is used in two cases.  First, for fresh boots of the domain, and
- * secondly for migrate-in/restore of pre-4.14 guests (where CPUID data was
- * missing from the stream).  The @restore parameter distinguishes these
- * cases, and the generated policy must be compatible with a 4.13.
- *
- * Either pass a full new @featureset (and @nr_features), or adjust individual
- * features (@pae, @itsc, @nested_virt).
- *
- * Then (optionally) apply legacy XEND overrides (@xend) to the result.
- */
-int xc_cpuid_apply_policy(xc_interface *xch,
-                          uint32_t domid, bool restore,
-                          const uint32_t *featureset,
-                          unsigned int nr_features, bool pae, bool itsc,
-                          bool nested_virt, const struct xc_xend_cpuid *xend);
 int xc_mca_op(xc_interface *xch, struct xen_mc *mc);
 int xc_mca_op_inject_v2(xc_interface *xch, unsigned int flags,
                         xc_cpumap_t cpumap, unsigned int nr_cpus);
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index e2237e33709..d00be5f757d 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -413,128 +413,6 @@ int xc_cpu_policy_apply_cpuid(xc_interface *xch, xc_cpu_policy_t policy,
     return rc;
 }
 
-int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
-                          const uint32_t *featureset, unsigned int nr_features,
-                          bool pae, bool itsc, bool nested_virt,
-                          const struct xc_xend_cpuid *cpuid)
-{
-    int rc;
-    xc_dominfo_t di;
-    unsigned int nr_leaves, nr_msrs;
-    xen_cpuid_leaf_t *leaves = NULL;
-    struct cpuid_policy *p = NULL;
-    struct xc_cpu_policy policy = { };
-    uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
-
-    if ( xc_domain_getinfo(xch, domid, 1, &di) != 1 ||
-         di.domid != domid )
-    {
-        ERROR("Failed to obtain d%d info", domid);
-        rc = -ESRCH;
-        goto out;
-    }
-
-    rc = xc_cpu_policy_get_size(xch, &nr_leaves, &nr_msrs);
-    if ( rc )
-    {
-        PERROR("Failed to obtain policy info size");
-        rc = -errno;
-        goto out;
-    }
-
-    rc = -ENOMEM;
-    if ( (leaves = calloc(nr_leaves, sizeof(*leaves))) == NULL ||
-         (p = calloc(1, sizeof(*p))) == NULL )
-        goto out;
-
-    /* Get the domain's default policy. */
-    nr_msrs = 0;
-    rc = get_system_cpu_policy(xch, di.hvm ? XEN_SYSCTL_cpu_policy_hvm_default
-                                           : XEN_SYSCTL_cpu_policy_pv_default,
-                               &nr_leaves, leaves, &nr_msrs, NULL);
-    if ( rc )
-    {
-        PERROR("Failed to obtain %s default policy", di.hvm ? "hvm" : "pv");
-        rc = -errno;
-        goto out;
-    }
-
-    rc = x86_cpuid_copy_from_buffer(p, leaves, nr_leaves,
-                                    &err_leaf, &err_subleaf);
-    if ( rc )
-    {
-        ERROR("Failed to deserialise CPUID (err leaf %#x, subleaf %#x) (%d = %s)",
-              err_leaf, err_subleaf, -rc, strerror(-rc));
-        goto out;
-    }
-
-    if ( restore )
-    {
-        policy.cpuid = *p;
-        xc_cpu_policy_make_compatible(xch, &policy, di.hvm);
-        *p = policy.cpuid;
-    }
-
-    if ( featureset )
-    {
-        policy.cpuid = *p;
-        rc = xc_cpu_policy_apply_featureset(xch, &policy, featureset,
-                                            nr_features);
-        if ( rc )
-        {
-            ERROR("Failed to apply featureset to policy");
-            goto out;
-        }
-        *p = policy.cpuid;
-    }
-    else
-    {
-        p->extd.itsc = itsc;
-
-        if ( di.hvm )
-        {
-            p->basic.pae = pae;
-            p->basic.vmx = nested_virt;
-            p->extd.svm = nested_virt;
-        }
-    }
-
-    policy.cpuid = *p;
-    rc = xc_cpu_policy_legacy_topology(xch, &policy, di.hvm);
-    if ( rc )
-        goto out;
-    *p = policy.cpuid;
-
-    rc = xc_cpu_policy_apply_cpuid(xch, &policy, cpuid, di.hvm);
-    if ( rc )
-        goto out;
-
-    rc = x86_cpuid_copy_to_buffer(p, leaves, &nr_leaves);
-    if ( rc )
-    {
-        ERROR("Failed to serialise CPUID (%d = %s)", -rc, strerror(-rc));
-        goto out;
-    }
-
-    rc = xc_set_domain_cpu_policy(xch, domid, nr_leaves, leaves, 0, NULL,
-                                  &err_leaf, &err_subleaf, &err_msr);
-    if ( rc )
-    {
-        PERROR("Failed to set d%d's policy (err leaf %#x, subleaf %#x, msr %#x)",
-               domid, err_leaf, err_subleaf, err_msr);
-        rc = -errno;
-        goto out;
-    }
-
-    rc = 0;
-
-out:
-    free(p);
-    free(leaves);
-
-    return rc;
-}
-
 xc_cpu_policy_t xc_cpu_policy_init(void)
 {
     return calloc(1, sizeof(struct xc_cpu_policy));
diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
index eb6feaa96d1..2489ceb49b8 100644
--- a/tools/libs/light/libxl_cpuid.c
+++ b/tools/libs/light/libxl_cpuid.c
@@ -430,9 +430,11 @@ int libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
                         libxl_domain_build_info *info)
 {
     GC_INIT(ctx);
+    xc_cpu_policy_t policy = NULL;
+    bool hvm = info->type == LIBXL_DOMAIN_TYPE_HVM;
     bool pae = true;
     bool itsc;
-    int r;
+    int r, rc = 0;
 
     /*
      * Gross hack.  Using libxl_defbool_val() here causes libvirt to crash in
@@ -443,6 +445,41 @@ int libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
      */
     bool nested_virt = info->nested_hvm.val > 0;
 
+    policy = xc_cpu_policy_init();
+    if (!policy) {
+        LOGED(ERROR, domid, "Failed to init CPU policy");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    r = xc_cpu_policy_get_domain(ctx->xch, domid, policy);
+    if (r) {
+        LOGED(ERROR, domid, "Failed to fetch domain CPU policy");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    if (restore) {
+        /*
+         * Make sure the policy is compatible with pre Xen 4.13. Note that
+         * newer Xen versions will pass policy data on the restore stream, so
+         * any adjustments done here will be superseded.
+         */
+        r = xc_cpu_policy_make_compatible(ctx->xch, policy, hvm);
+        if (r) {
+            LOGED(ERROR, domid, "Failed to setup compatible CPU policy");
+            rc = ERROR_FAIL;
+            goto out;
+        }
+    }
+
+    r = xc_cpu_policy_legacy_topology(ctx->xch, policy, hvm);
+    if (r) {
+        LOGED(ERROR, domid, "Failed to setup CPU policy topology");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
     /*
      * For PV guests, PAE is Xen-controlled (it is the 'p' that differentiates
      * the xen-3.0-x86_32 and xen-3.0-x86_32p ABIs).  It is mandatory as Xen
@@ -453,8 +490,15 @@ int libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
      *
      * HVM guests get a top-level choice of whether PAE is available.
      */
-    if (info->type == LIBXL_DOMAIN_TYPE_HVM)
+    if (hvm)
         pae = libxl_defbool_val(info->u.hvm.pae);
+    rc = libxl_cpuid_parse_config(&info->cpuid, GCSPRINTF("pae=%d", pae));
+    if (rc) {
+        LOGD(ERROR, domid, "Failed to set PAE CPUID flag");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
 
     /*
      * Advertising Invariant TSC to a guest means that the TSC frequency won't
@@ -470,14 +514,50 @@ int libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
      */
     itsc = (libxl_defbool_val(info->disable_migrate) ||
             info->tsc_mode == LIBXL_TSC_MODE_ALWAYS_EMULATE);
+    rc = libxl_cpuid_parse_config(&info->cpuid, GCSPRINTF("invtsc=%d", itsc));
+    if (rc) {
+        LOGD(ERROR, domid, "Failed to set Invariant TSC CPUID flag");
+        rc = ERROR_FAIL;
+        goto out;
+    }
 
-    r = xc_cpuid_apply_policy(ctx->xch, domid, restore, NULL, 0,
-                              pae, itsc, nested_virt, info->cpuid);
-    if (r)
-        LOGEVD(ERROR, -r, domid, "Failed to apply CPUID policy");
+    /* Set Nested virt CPUID bits for HVM. */
+    if (hvm) {
+        rc = libxl_cpuid_parse_config(&info->cpuid, GCSPRINTF("vmx=%d",
+                                                              nested_virt));
+        if (rc) {
+            LOGD(ERROR, domid, "Failed to set VMX CPUID flag");
+            rc = ERROR_FAIL;
+            goto out;
+        }
+
+        rc = libxl_cpuid_parse_config(&info->cpuid, GCSPRINTF("svm=%d",
+                                                              nested_virt));
+        if (rc) {
+            LOGD(ERROR, domid, "Failed to set SVM CPUID flag");
+            rc = ERROR_FAIL;
+            goto out;
+        }
+    }
+
+    /* Apply the bits from info->cpuid if any. */
+    r = xc_cpu_policy_apply_cpuid(ctx->xch, policy, info->cpuid, hvm);
+    if (r) {
+        LOGEVD(ERROR, domid, -r, "Failed to apply CPUID changes");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    r = xc_cpu_policy_set_domain(ctx->xch, domid, policy);
+    if (r) {
+        LOGED(ERROR, domid, "Failed to set domain CPUID policy");
+        rc = ERROR_FAIL;
+    }
 
+ out:
+    xc_cpu_policy_destroy(policy);
     GC_FREE;
-    return r ? ERROR_FAIL : 0;
+    return rc;
 }
 
 static const char *input_names[2] = { "leaf", "subleaf" };
-- 
2.31.1

Re: [PATCH v3 12/13] libs/{light,guest}: implement xc_cpuid_apply_policy in libxl

[Anthony PERARD]

On Fri, Apr 30, 2021 at 05:52:10PM +0200, Roger Pau Monne wrote:
> With the addition of the xc_cpu_policy_* now libxl can have better
> control over the cpu policy, this allows removing the
> xc_cpuid_apply_policy function and instead coding the required bits by
> libxl in libxl__cpuid_legacy directly.
> 
> Remove xc_cpuid_apply_policy.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Changes since v2:
>  - Use 'r' for libxc return values.
>  - Fix comment about making a cpu policy compatible.
>  - Use LOG*D macros.
> ---

Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

[PATCH v3 13/13] libs/guest: (re)move xc_cpu_policy_apply_cpuid

[Roger Pau Monne]

Move the logic from xc_cpu_policy_apply_cpuid into libxl, now that the
xc_cpu_policy_* helpers allow modifying a cpu policy. By moving such
parsing into libxl directly we can get rid of xc_xend_cpuid, as libxl
will now implement it's own private type for storing CPUID
information, which currently matches xc_xend_cpuid.

Note the function logic is moved as-is, but requires adapting to the
libxl coding style.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v2:
 - Use LOG*D.
 - Pass a gc to apply_policy.
 - Use 'r' for libxc return values.
---
 tools/include/libxl.h             |   6 +-
 tools/include/xenctrl.h           |  30 -------
 tools/libs/guest/xg_cpuid_x86.c   | 125 --------------------------
 tools/libs/light/libxl_cpuid.c    | 142 ++++++++++++++++++++++++++++--
 tools/libs/light/libxl_internal.h |  26 ++++++
 5 files changed, 165 insertions(+), 164 deletions(-)

diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index ae7fe27c1f2..150b7ba85ac 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -1375,10 +1375,10 @@ void libxl_bitmap_init(libxl_bitmap *map);
 void libxl_bitmap_dispose(libxl_bitmap *map);
 
 /*
- * libxl_cpuid_policy is opaque in the libxl ABI.  Users of both libxl and
- * libxc may not make assumptions about xc_xend_cpuid.
+ * libxl_cpuid_policy is opaque in the libxl ABI. Users of libxl may not make
+ * assumptions about libxl__cpuid_policy.
  */
-typedef struct xc_xend_cpuid libxl_cpuid_policy;
+typedef struct libxl__cpuid_policy libxl_cpuid_policy;
 typedef libxl_cpuid_policy * libxl_cpuid_policy_list;
 void libxl_cpuid_dispose(libxl_cpuid_policy_list *cpuid_list);
 int libxl_cpuid_policy_list_length(const libxl_cpuid_policy_list *l);
diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index c6ee1142e8e..044d05321a3 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -1864,32 +1864,6 @@ int xc_domain_debug_control(xc_interface *xch,
 
 #if defined(__i386__) || defined(__x86_64__)
 
-/*
- * CPUID policy data, expressed in the legacy XEND format.
- *
- * Policy is an array of strings, 32 chars long:
- *   policy[0] = eax
- *   policy[1] = ebx
- *   policy[2] = ecx
- *   policy[3] = edx
- *
- * The format of the string is the following:
- *   '1' -> force to 1
- *   '0' -> force to 0
- *   'x' -> we don't care (use default)
- *   'k' -> pass through host value
- *   's' -> legacy alias for 'k'
- */
-struct xc_xend_cpuid {
-    union {
-        struct {
-            uint32_t leaf, subleaf;
-        };
-        uint32_t input[2];
-    };
-    char *policy[4];
-};
-
 int xc_mca_op(xc_interface *xch, struct xen_mc *mc);
 int xc_mca_op_inject_v2(xc_interface *xch, unsigned int flags,
                         xc_cpumap_t cpumap, unsigned int nr_cpus);
@@ -2617,10 +2591,6 @@ int xc_cpu_policy_make_compatible(xc_interface *xch, xc_cpu_policy_t policy,
 int xc_cpu_policy_legacy_topology(xc_interface *xch, xc_cpu_policy_t policy,
                                   bool hvm);
 
-/* Apply an xc_xend_cpuid object to the policy. */
-int xc_cpu_policy_apply_cpuid(xc_interface *xch, xc_cpu_policy_t policy,
-                              const struct xc_xend_cpuid *cpuid, bool hvm);
-
 /* Apply a featureset to the policy. */
 int xc_cpu_policy_apply_featureset(xc_interface *xch, xc_cpu_policy_t policy,
                                    const uint32_t *featureset,
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index d00be5f757d..139aa91dd11 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -288,131 +288,6 @@ static xen_cpuid_leaf_t *find_leaf(
     return bsearch(&key, leaves, nr_leaves, sizeof(*leaves), compare_leaves);
 }
 
-int xc_cpu_policy_apply_cpuid(xc_interface *xch, xc_cpu_policy_t policy,
-                              const struct xc_xend_cpuid *cpuid, bool hvm)
-{
-    int rc;
-    xc_cpu_policy_t host = NULL, def = NULL;
-
-    host = xc_cpu_policy_init();
-    def = xc_cpu_policy_init();
-    if ( !host || !def )
-    {
-        PERROR("Failed to init policies");
-        rc = -ENOMEM;
-        goto out;
-    }
-
-    /* Get the domain type's default policy. */
-    rc = xc_cpu_policy_get_system(xch, hvm ? XEN_SYSCTL_cpu_policy_hvm_default
-                                           : XEN_SYSCTL_cpu_policy_pv_default,
-                                  def);
-    if ( rc )
-    {
-        PERROR("Failed to obtain %s def policy", hvm ? "hvm" : "pv");
-        goto out;
-    }
-
-    /* Get the host policy. */
-    rc = xc_cpu_policy_get_system(xch, XEN_SYSCTL_cpu_policy_host, host);
-    if ( rc )
-    {
-        PERROR("Failed to obtain host policy");
-        goto out;
-    }
-
-    rc = -EINVAL;
-    for ( ; cpuid->leaf != XEN_CPUID_INPUT_UNUSED; ++cpuid )
-    {
-        xen_cpuid_leaf_t cur_leaf;
-        xen_cpuid_leaf_t def_leaf;
-        xen_cpuid_leaf_t host_leaf;
-
-        rc = xc_cpu_policy_get_cpuid(xch, policy, cpuid->leaf, cpuid->subleaf,
-                                     &cur_leaf);
-        if ( rc )
-        {
-            ERROR("Failed to get current policy leaf %#x subleaf %#x",
-                  cpuid->leaf, cpuid->subleaf);
-            goto out;
-        }
-        rc = xc_cpu_policy_get_cpuid(xch, def, cpuid->leaf, cpuid->subleaf,
-                                     &def_leaf);
-        if ( rc )
-        {
-            ERROR("Failed to get def policy leaf %#x subleaf %#x",
-                  cpuid->leaf, cpuid->subleaf);
-            goto out;
-        }
-        rc = xc_cpu_policy_get_cpuid(xch, host, cpuid->leaf, cpuid->subleaf,
-                                     &host_leaf);
-        if ( rc )
-        {
-            ERROR("Failed to get host policy leaf %#x subleaf %#x",
-                  cpuid->leaf, cpuid->subleaf);
-            goto out;
-        }
-
-        for ( unsigned int i = 0; i < ARRAY_SIZE(cpuid->policy); i++ )
-        {
-            uint32_t *cur_reg = &cur_leaf.a + i;
-            const uint32_t *def_reg = &def_leaf.a + i;
-            const uint32_t *host_reg = &host_leaf.a + i;
-
-            if ( cpuid->policy[i] == NULL )
-                continue;
-
-            for ( unsigned int j = 0; j < 32; j++ )
-            {
-                bool val;
-
-                switch ( cpuid->policy[i][j] )
-                {
-                case '1':
-                    val = true;
-                    break;
-
-                case '0':
-                    val = false;
-                    break;
-
-                case 'x':
-                    val = test_bit(31 - j, def_reg);
-                    break;
-
-                case 'k':
-                case 's':
-                    val = test_bit(31 - j, host_reg);
-                    break;
-
-                default:
-                    ERROR("Bad character '%c' in policy[%d] string '%s'",
-                          cpuid->policy[i][j], i, cpuid->policy[i]);
-                    goto out;
-                }
-
-                clear_bit(31 - j, cur_reg);
-                if ( val )
-                    set_bit(31 - j, cur_reg);
-            }
-        }
-
-        rc = xc_cpu_policy_update_cpuid(xch, policy, &cur_leaf, 1);
-        if ( rc )
-        {
-            PERROR("Failed to set policy leaf %#x subleaf %#x",
-                   cpuid->leaf, cpuid->subleaf);
-            goto out;
-        }
-    }
-
- out:
-    xc_cpu_policy_destroy(def);
-    xc_cpu_policy_destroy(host);
-
-    return rc;
-}
-
 xc_cpu_policy_t xc_cpu_policy_init(void)
 {
     return calloc(1, sizeof(struct xc_cpu_policy));
diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
index 2489ceb49b8..318f3bd8599 100644
--- a/tools/libs/light/libxl_cpuid.c
+++ b/tools/libs/light/libxl_cpuid.c
@@ -298,7 +298,7 @@ int libxl_cpuid_parse_config(libxl_cpuid_policy_list *cpuid, const char* str)
     char *sep, *val, *endptr;
     int i;
     const struct cpuid_flags *flag;
-    struct xc_xend_cpuid *entry;
+    struct libxl__cpuid_policy *entry;
     unsigned long num;
     char flags[33], *resstr;
 
@@ -376,7 +376,7 @@ int libxl_cpuid_parse_config_xend(libxl_cpuid_policy_list *cpuid,
     char *endptr;
     unsigned long value;
     uint32_t leaf, subleaf = XEN_CPUID_INPUT_UNUSED;
-    struct xc_xend_cpuid *entry;
+    struct libxl__cpuid_policy *entry;
 
     /* parse the leaf number */
     value = strtoul(str, &endptr, 0);
@@ -426,6 +426,137 @@ int libxl_cpuid_parse_config_xend(libxl_cpuid_policy_list *cpuid,
     return 0;
 }
 
+static int apply_cpuid(libxl__gc *gc, xc_cpu_policy_t policy,
+                       libxl_cpuid_policy_list cpuid, bool hvm, domid_t domid)
+{
+    int r, rc = 0;
+    xc_cpu_policy_t host = NULL, def = NULL;
+
+    host = xc_cpu_policy_init();
+    def = xc_cpu_policy_init();
+    if (!host || !def) {
+        LOGD(ERROR, domid, "Failed to init policies");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    /* Get the domain type's default policy. */
+    r = xc_cpu_policy_get_system(CTX->xch,
+                                 hvm ? XEN_SYSCTL_cpu_policy_hvm_default
+                                     : XEN_SYSCTL_cpu_policy_pv_default,
+                                 def);
+    if (r) {
+        LOGED(ERROR, domid, "Failed to obtain %s def policy",
+              hvm ? "hvm" : "pv");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    /* Get the host policy. */
+    r = xc_cpu_policy_get_system(CTX->xch, XEN_SYSCTL_cpu_policy_host, host);
+    if (r) {
+        LOGED(ERROR, domid, "Failed to obtain host policy");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    for (; cpuid->leaf != XEN_CPUID_INPUT_UNUSED; ++cpuid) {
+        xen_cpuid_leaf_t cur_leaf;
+        xen_cpuid_leaf_t def_leaf;
+        xen_cpuid_leaf_t host_leaf;
+
+        r = xc_cpu_policy_get_cpuid(CTX->xch, policy, cpuid->leaf,
+                                    cpuid->subleaf, &cur_leaf);
+        if (r) {
+            LOGED(ERROR, domid,
+                  "Failed to get current policy leaf %#x subleaf %#x",
+                  cpuid->leaf, cpuid->subleaf);
+            r = ERROR_FAIL;
+            goto out;
+        }
+        r = xc_cpu_policy_get_cpuid(CTX->xch, def, cpuid->leaf, cpuid->subleaf,
+                                    &def_leaf);
+        if (r) {
+            LOGED(ERROR, domid,
+                  "Failed to get def policy leaf %#x subleaf %#x",
+                  cpuid->leaf, cpuid->subleaf);
+            rc = ERROR_FAIL;
+            goto out;
+        }
+        r = xc_cpu_policy_get_cpuid(CTX->xch, host, cpuid->leaf,
+                                    cpuid->subleaf, &host_leaf);
+        if (r) {
+            LOGED(ERROR, domid,
+                  "Failed to get host policy leaf %#x subleaf %#x",
+                  cpuid->leaf, cpuid->subleaf);
+            rc = ERROR_FAIL;
+            goto out;
+        }
+
+        for (unsigned int i = 0; i < ARRAY_SIZE(cpuid->policy); i++) {
+            uint32_t *cur_reg = &cur_leaf.a + i;
+            const uint32_t *def_reg = &def_leaf.a + i;
+            const uint32_t *host_reg = &host_leaf.a + i;
+
+            if (cpuid->policy[i] == NULL)
+                continue;
+
+#define test_bit(i, r) !!(*(r) & (1u << (i)))
+#define set_bit(i, r) (*(r) |= (1u << (i)))
+#define clear_bit(i, r)  (*(r) &= ~(1u << (i)))
+            for (unsigned int j = 0; j < 32; j++) {
+                bool val;
+
+                switch (cpuid->policy[i][j]) {
+                case '1':
+                    val = true;
+                    break;
+
+                case '0':
+                    val = false;
+                    break;
+
+                case 'x':
+                    val = test_bit(31 - j, def_reg);
+                    break;
+
+                case 'k':
+                case 's':
+                    val = test_bit(31 - j, host_reg);
+                    break;
+
+                default:
+                    LOGD(ERROR, domid,
+                         "Bad character '%c' in policy[%d] string '%s'",
+                         cpuid->policy[i][j], i, cpuid->policy[i]);
+                    rc = ERROR_FAIL;
+                    goto out;
+                }
+
+                clear_bit(31 - j, cur_reg);
+                if (val)
+                    set_bit(31 - j, cur_reg);
+            }
+#undef clear_bit
+#undef set_bit
+#undef test_bit
+        }
+
+        r = xc_cpu_policy_update_cpuid(CTX->xch, policy, &cur_leaf, 1);
+        if (r) {
+            LOGED(ERROR, domid, "Failed to set policy leaf %#x subleaf %#x",
+                  cpuid->leaf, cpuid->subleaf);
+            rc = ERROR_FAIL;
+            goto out;
+        }
+    }
+
+ out:
+    xc_cpu_policy_destroy(def);
+    xc_cpu_policy_destroy(host);
+    return rc;
+}
+
 int libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
                         libxl_domain_build_info *info)
 {
@@ -541,10 +672,9 @@ int libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
     }
 
     /* Apply the bits from info->cpuid if any. */
-    r = xc_cpu_policy_apply_cpuid(ctx->xch, policy, info->cpuid, hvm);
-    if (r) {
-        LOGEVD(ERROR, domid, -r, "Failed to apply CPUID changes");
-        rc = ERROR_FAIL;
+    rc = apply_cpuid(gc, policy, info->cpuid, hvm, domid);
+    if (rc) {
+        LOGD(ERROR, domid, "Failed to apply CPUID changes");
         goto out;
     }
 
diff --git a/tools/libs/light/libxl_internal.h b/tools/libs/light/libxl_internal.h
index 5709bcb93fa..d130c073ac4 100644
--- a/tools/libs/light/libxl_internal.h
+++ b/tools/libs/light/libxl_internal.h
@@ -2050,6 +2050,32 @@ typedef yajl_gen_status (*libxl__gen_json_callback)(yajl_gen hand, void *);
 _hidden char *libxl__object_to_json(libxl_ctx *ctx, const char *type,
                                     libxl__gen_json_callback gen, void *p);
 
+/*
+ * CPUID policy data, expressed in the internal libxl format.
+ *
+ * Policy is an array of strings, 32 chars long:
+ *   policy[0] = eax
+ *   policy[1] = ebx
+ *   policy[2] = ecx
+ *   policy[3] = edx
+ *
+ * The format of the string is the following:
+ *   '1' -> force to 1
+ *   '0' -> force to 0
+ *   'x' -> we don't care (use default)
+ *   'k' -> pass through host value
+ *   's' -> legacy alias for 'k'
+ */
+struct libxl__cpuid_policy {
+    union {
+        struct {
+            uint32_t leaf, subleaf;
+        };
+        uint32_t input[2];
+    };
+    char *policy[4];
+};
+
 _hidden int libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool retore,
                                 libxl_domain_build_info *info);
 
-- 
2.31.1

Re: [PATCH v3 13/13] libs/guest: (re)move xc_cpu_policy_apply_cpuid

[Anthony PERARD]

On Fri, Apr 30, 2021 at 05:52:11PM +0200, Roger Pau Monne wrote:
> Move the logic from xc_cpu_policy_apply_cpuid into libxl, now that the
> xc_cpu_policy_* helpers allow modifying a cpu policy. By moving such
> parsing into libxl directly we can get rid of xc_xend_cpuid, as libxl
> will now implement it's own private type for storing CPUID
> information, which currently matches xc_xend_cpuid.
> 
> Note the function logic is moved as-is, but requires adapting to the
> libxl coding style.
> 
> No functional change intended.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Changes since v2:
>  - Use LOG*D.
>  - Pass a gc to apply_policy.
>  - Use 'r' for libxc return values.
> ---

Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD