Introduce a helper to obtain a compatible cpu policy based on two
input cpu policies. Currently this is done by and'ing all CPUID leaves
and MSR entries, except for MSR_ARCH_CAPABILITIES which has the RSBA
bit or'ed.

The _AC macro is pulled from libxl_internal.h into xen-tools/libs.h
since it's required in order to use the msr-index.h header.

Note there's no need to place this helper in libx86, since the
calculation of a compatible policy shouldn't be done from the
hypervisor.

No callers of the interface introduced.

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

diff --git a/tools/include/xen-tools/libs.h b/tools/include/xen-tools/libs.h
index a16e0c38070..b9e89f9a711 100644
--- a/tools/include/xen-tools/libs.h
+++ b/tools/include/xen-tools/libs.h
@@ -63,4 +63,9 @@
 #define ROUNDUP(_x,_w) (((unsigned long)(_x)+(1UL<<(_w))-1) & ~((1UL<<(_w))-1))
 #endif
 
+#ifndef _AC
+#define __AC(X,Y)   (X##Y)
+#define _AC(X,Y)    __AC(X,Y)
+#endif
+
 #endif	/* __XEN_TOOLS_LIBS__ */
diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 165beff330f..5f3e5e17e9d 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2622,6 +2622,10 @@ int xc_cpu_policy_update_msrs(xc_interface *xch, xc_cpu_policy_t policy,
 /* Compatibility calculations. */
 bool xc_cpu_policy_is_compatible(xc_interface *xch, const xc_cpu_policy_t p1,
                                  const xc_cpu_policy_t p2);
+int xc_cpu_policy_calc_compatible(xc_interface *xch,
+                                  const xc_cpu_policy_t p1,
+                                  const xc_cpu_policy_t p2,
+                                  xc_cpu_policy_t out);
 
 int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps);
 int xc_get_cpu_featureset(xc_interface *xch, uint32_t index,
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index 30ea02a0f31..4afca3249ba 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -32,6 +32,7 @@ enum {
 #include <xen/arch-x86/cpufeatureset.h>
 };
 
+#include <xen/asm/msr-index.h>
 #include <xen/asm/x86-vendors.h>
 
 #include <xen/lib/x86/cpu-policy.h>
@@ -1115,3 +1116,117 @@ bool xc_cpu_policy_is_compatible(xc_interface *xch, const xc_cpu_policy_t p1,
 
     return false;
 }
+
+static uint64_t level_msr(unsigned int index, uint64_t val1, uint64_t val2)
+{
+    uint64_t val;
+
+    switch( index )
+    {
+    case MSR_ARCH_CAPABILITIES:
+        val = val1 & val2;
+        /*
+         * Set RSBA if present on any of the input values to notice the guest
+         * might run on vulnerable hardware at some point.
+         */
+        val |= (val1 | val2) & ARCH_CAPS_RSBA;
+        break;
+
+    default:
+        val = val1 & val2;
+        break;
+    }
+
+    return val;
+}
+
+int xc_cpu_policy_calc_compatible(xc_interface *xch,
+                                  const xc_cpu_policy_t p1,
+                                  const xc_cpu_policy_t p2,
+                                  xc_cpu_policy_t out)
+{
+    xen_cpuid_leaf_t *leaves = NULL, *p1_leaves = NULL, *p2_leaves = NULL;
+    xen_msr_entry_t *msrs = NULL, *p1_msrs = NULL, *p2_msrs = NULL;
+    unsigned int nr_leaves, nr_msrs, i, j, index;
+    unsigned int p1_nr_leaves, p1_nr_msrs, p2_nr_leaves, p2_nr_msrs;
+    int rc;
+
+    if ( xc_cpu_policy_get_size(xch, &nr_leaves, &nr_msrs) )
+    {
+        PERROR("Failed to obtain policy info size");
+        return -1;
+    }
+
+    leaves = calloc(nr_leaves, sizeof(*leaves));
+    p1_leaves = calloc(nr_leaves, sizeof(*p1_leaves));
+    p2_leaves = calloc(nr_leaves, sizeof(*p2_leaves));
+    msrs = calloc(nr_msrs, sizeof(*msrs));
+    p1_msrs = calloc(nr_msrs, sizeof(*p1_msrs));
+    p2_msrs = calloc(nr_msrs, sizeof(*p2_msrs));
+
+    p1_nr_leaves = p2_nr_leaves = nr_leaves;
+    p1_nr_msrs = p2_nr_msrs = nr_msrs;
+
+    if ( !leaves || !p1_leaves || !p2_leaves ||
+         !msrs || !p1_msrs || !p2_msrs )
+    {
+        ERROR("Failed to allocate resources");
+        errno = ENOMEM;
+        rc = -1;
+        goto out;
+    }
+
+    rc = xc_cpu_policy_serialise(xch, p1, p1_leaves, &p1_nr_leaves,
+                                 p1_msrs, &p1_nr_msrs);
+    if ( rc )
+        goto out;
+    rc = xc_cpu_policy_serialise(xch, p2, p2_leaves, &p2_nr_leaves,
+                                 p2_msrs, &p2_nr_msrs);
+    if ( rc )
+        goto out;
+
+    index = 0;
+    for ( i = 0; i < p1_nr_leaves; i++ )
+        for ( j = 0; j < p2_nr_leaves; j++ )
+            if ( p1_leaves[i].leaf == p2_leaves[j].leaf &&
+                 p1_leaves[i].subleaf == p2_leaves[j].subleaf )
+            {
+                leaves[index].leaf = p1_leaves[i].leaf;
+                leaves[index].subleaf = p1_leaves[i].subleaf;
+                leaves[index].a = p1_leaves[i].a & p2_leaves[j].a;
+                leaves[index].b = p1_leaves[i].b & p2_leaves[j].b;
+                leaves[index].c = p1_leaves[i].c & p2_leaves[j].c;
+                leaves[index].d = p1_leaves[i].d & p2_leaves[j].d;
+                index++;
+            }
+    nr_leaves = index;
+
+    index = 0;
+    for ( i = 0; i < p1_nr_msrs; i++ )
+        for ( j = 0; j < p2_nr_msrs; j++ )
+            if ( p1_msrs[i].idx == p2_msrs[j].idx )
+            {
+                msrs[index].idx = p1_msrs[i].idx;
+                msrs[index].val = level_msr(p1_msrs[i].idx,
+                                            p1_msrs[i].val, p2_msrs[j].val);
+                index++;
+            }
+    nr_msrs = index;
+
+    rc = deserialize_policy(xch, out, nr_leaves, leaves, nr_msrs, msrs);
+    if ( rc )
+    {
+        errno = -rc;
+        rc = -1;
+    }
+
+ out:
+    free(leaves);
+    free(p1_leaves);
+    free(p2_leaves);
+    free(msrs);
+    free(p1_msrs);
+    free(p2_msrs);
+
+    return rc;
+}
diff --git a/tools/libs/light/libxl_internal.h b/tools/libs/light/libxl_internal.h
index 22b1775b752..53b8939fb5a 100644
--- a/tools/libs/light/libxl_internal.h
+++ b/tools/libs/light/libxl_internal.h
@@ -126,8 +126,6 @@
 #define PVSHIM_CMDLINE "pv-shim console=xen,pv"
 
 /* Size macros. */
-#define __AC(X,Y)   (X##Y)
-#define _AC(X,Y)    __AC(X,Y)
 #define MB(_mb)     (_AC(_mb, ULL) << 20)
 #define GB(_gb)     (_AC(_gb, ULL) << 30)
 
-- 
2.30.1

On 23/03/2021 09:58, Roger Pau Monne wrote:
> Introduce a helper to obtain a compatible cpu policy based on two
> input cpu policies. Currently this is done by and'ing all CPUID leaves
> and MSR entries, except for MSR_ARCH_CAPABILITIES which has the RSBA
> bit or'ed.
>
> The _AC macro is pulled from libxl_internal.h into xen-tools/libs.h
> since it's required in order to use the msr-index.h header.
>
> Note there's no need to place this helper in libx86, since the
> calculation of a compatible policy shouldn't be done from the
> hypervisor.
>
> No callers of the interface introduced.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

I think this wants to be in libx86, because we'll want it to replace the
opencoded derivation logic in init_cpuid()

Also, we absolutely want to unit test it.  (I could have sworn I already
started some work there - maybe its hidden in one of my WIP branches).

~Andrew

On Thu, Apr 01, 2021 at 05:26:03PM +0100, Andrew Cooper wrote:
> On 23/03/2021 09:58, Roger Pau Monne wrote:
> > Introduce a helper to obtain a compatible cpu policy based on two
> > input cpu policies. Currently this is done by and'ing all CPUID leaves
> > and MSR entries, except for MSR_ARCH_CAPABILITIES which has the RSBA
> > bit or'ed.
> >
> > The _AC macro is pulled from libxl_internal.h into xen-tools/libs.h
> > since it's required in order to use the msr-index.h header.
> >
> > Note there's no need to place this helper in libx86, since the
> > calculation of a compatible policy shouldn't be done from the
> > hypervisor.
> >
> > No callers of the interface introduced.
> >
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> I think this wants to be in libx86, because we'll want it to replace the
> opencoded derivation logic in init_cpuid()

I think you mean init_domain_cpuid_policy or recalculate_cpuid_policy?
I cannot find any init_cpuid.

I'm not convinced we need this in libx86 for the hypervisor, as I
don't know exactly how we would use it there. I expect the hypervisor
to validate policies provided by the toolstack, but not for it to
create such policies unless for very specific domains (ie: dom0 or
similar) which shouldn't require any leveling.

I'm happy to place it libx86, but I think I need to understand better
why it needs to be there.

> Also, we absolutely want to unit test it.  (I could have sworn I already
> started some work there - maybe its hidden in one of my WIP branches).

I don't think we have unit tests for the xenguest library?

Thanks, Roger.

On 23.03.2021 10:58, Roger Pau Monne wrote:
> Introduce a helper to obtain a compatible cpu policy based on two
> input cpu policies. Currently this is done by and'ing all CPUID leaves
> and MSR entries, except for MSR_ARCH_CAPABILITIES which has the RSBA
> bit or'ed.

I'm afraid this is too simplistic. It might do as an initial
approximation if limited to the feature flag leaves, but you
can't reasonably AND together e.g. leaf 0 output.

> @@ -1115,3 +1116,117 @@ bool xc_cpu_policy_is_compatible(xc_interface *xch, const xc_cpu_policy_t p1,
>  
>      return false;
>  }
> +
> +static uint64_t level_msr(unsigned int index, uint64_t val1, uint64_t val2)
> +{
> +    uint64_t val;
> +
> +    switch( index )
> +    {
> +    case MSR_ARCH_CAPABILITIES:
> +        val = val1 & val2;

Considering you need this even here, how about making this the
initializer of the variable, allowing to drop "default:"
altogether?

Also, nit: Missing blank after "switch".

> +    index = 0;
> +    for ( i = 0; i < p1_nr_leaves; i++ )
> +        for ( j = 0; j < p2_nr_leaves; j++ )
> +            if ( p1_leaves[i].leaf == p2_leaves[j].leaf &&
> +                 p1_leaves[i].subleaf == p2_leaves[j].subleaf )
> +            {
> +                leaves[index].leaf = p1_leaves[i].leaf;
> +                leaves[index].subleaf = p1_leaves[i].subleaf;
> +                leaves[index].a = p1_leaves[i].a & p2_leaves[j].a;
> +                leaves[index].b = p1_leaves[i].b & p2_leaves[j].b;
> +                leaves[index].c = p1_leaves[i].c & p2_leaves[j].c;
> +                leaves[index].d = p1_leaves[i].d & p2_leaves[j].d;
> +                index++;
> +            }
> +    nr_leaves = index;
> +
> +    index = 0;
> +    for ( i = 0; i < p1_nr_msrs; i++ )
> +        for ( j = 0; j < p2_nr_msrs; j++ )
> +            if ( p1_msrs[i].idx == p2_msrs[j].idx )
> +            {
> +                msrs[index].idx = p1_msrs[i].idx;
> +                msrs[index].val = level_msr(p1_msrs[i].idx,
> +                                            p1_msrs[i].val, p2_msrs[j].val);
> +                index++;
> +            }
> +    nr_msrs = index;

Mid- to long-term I'd be afraid of this going to take too long for
at least the MSRs. Can't we build on some similarity in the ordering
of both arrays, to avoid the double for()s?

Jan