[5/8] tools/libxc: Rework xc_cpuid_set() to use {get, set}_cpu_policy()
diff mbox series

Message ID 20190911200504.5693-6-andrew.cooper3@citrix.com
State Superseded
Headers show
Series
  • x86/cpuid: Switch to using XEN_DOMCTL_set_cpumsr_policy
Related show

Commit Message

Andrew Cooper Sept. 11, 2019, 8:05 p.m. UTC
The purpose of this change is to stop using xc_cpuid_do_domctl(), and to stop
basing decisions on a local CPUID instruction.  This is not an appropriate way
to construct policy information for other domains.

Obtain the host and domain-max policies from Xen, and mix the results as
before.  Provide rather more error logging than before.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Ian Jackson <Ian.Jackson@citrix.com>
---
 tools/libxc/xc_cpuid_x86.c | 95 ++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 84 insertions(+), 11 deletions(-)

Comments

Jan Beulich Sept. 12, 2019, 8:19 a.m. UTC | #1
On 11.09.2019 22:05, Andrew Cooper wrote:
> The purpose of this change is to stop using xc_cpuid_do_domctl(), and to stop
> basing decisions on a local CPUID instruction.  This is not an appropriate way
> to construct policy information for other domains.
> 
> Obtain the host and domain-max policies from Xen, and mix the results as
> before.  Provide rather more error logging than before.

And this passing through of host values is meant to stay long
term? Shouldn't this rather be passing through of max-policy
values, with max-policy long term wider than default-policy? The
change itself looks good to me, but before giving my R-b I'd
like to understand this aspect.

Jan
Andrew Cooper Sept. 12, 2019, 8:36 a.m. UTC | #2
On 12/09/2019 09:19, Jan Beulich wrote:
> On 11.09.2019 22:05, Andrew Cooper wrote:
>> The purpose of this change is to stop using xc_cpuid_do_domctl(), and to stop
>> basing decisions on a local CPUID instruction.  This is not an appropriate way
>> to construct policy information for other domains.
>>
>> Obtain the host and domain-max policies from Xen, and mix the results as
>> before.  Provide rather more error logging than before.
> And this passing through of host values is meant to stay long
> term? Shouldn't this rather be passing through of max-policy
> values, with max-policy long term wider than default-policy? The
> change itself looks good to me, but before giving my R-b I'd
> like to understand this aspect.

There is a very large amount wrong with xc_cpuid_set().

For one, its behaviour is only useful for feature leaves, but it will
operate on any leaves the user requests, and while it is capable of
returning errors, libxl doesn't check the return value and continues
blindly forwards.

Also from the L1TF embargo days is a series of work (or at least, the
start of) which is a total overhaul of how libxl and libxc interact when
it comes CPUID and MSR settings.  Neither xc_cpuid_set() nor
xc_cpuid_apply_policy() will survive to the end.

For now, I've opted with not changing the semantics of the calls while
altering the data-transfer mechanism, to avoid conflating the two areas
of work.

~Andrew
Jan Beulich Sept. 12, 2019, 9:11 a.m. UTC | #3
On 12.09.2019 10:36, Andrew Cooper wrote:
> On 12/09/2019 09:19, Jan Beulich wrote:
>> On 11.09.2019 22:05, Andrew Cooper wrote:
>>> The purpose of this change is to stop using xc_cpuid_do_domctl(), and to stop
>>> basing decisions on a local CPUID instruction.  This is not an appropriate way
>>> to construct policy information for other domains.
>>>
>>> Obtain the host and domain-max policies from Xen, and mix the results as
>>> before.  Provide rather more error logging than before.
>> And this passing through of host values is meant to stay long
>> term? Shouldn't this rather be passing through of max-policy
>> values, with max-policy long term wider than default-policy? The
>> change itself looks good to me, but before giving my R-b I'd
>> like to understand this aspect.
> 
> There is a very large amount wrong with xc_cpuid_set().
> 
> For one, its behaviour is only useful for feature leaves, but it will
> operate on any leaves the user requests, and while it is capable of
> returning errors, libxl doesn't check the return value and continues
> blindly forwards.
> 
> Also from the L1TF embargo days is a series of work (or at least, the
> start of) which is a total overhaul of how libxl and libxc interact when
> it comes CPUID and MSR settings.  Neither xc_cpuid_set() nor
> xc_cpuid_apply_policy() will survive to the end.
> 
> For now, I've opted with not changing the semantics of the calls while
> altering the data-transfer mechanism, to avoid conflating the two areas
> of work.

And with this then, as promised,
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan
Andrew Cooper Sept. 12, 2019, 1:21 p.m. UTC | #4
On 12/09/2019 10:11, Jan Beulich wrote:
> On 12.09.2019 10:36, Andrew Cooper wrote:
>> On 12/09/2019 09:19, Jan Beulich wrote:
>>> On 11.09.2019 22:05, Andrew Cooper wrote:
>>>> The purpose of this change is to stop using xc_cpuid_do_domctl(), and to stop
>>>> basing decisions on a local CPUID instruction.  This is not an appropriate way
>>>> to construct policy information for other domains.
>>>>
>>>> Obtain the host and domain-max policies from Xen, and mix the results as
>>>> before.  Provide rather more error logging than before.
>>> And this passing through of host values is meant to stay long
>>> term? Shouldn't this rather be passing through of max-policy
>>> values, with max-policy long term wider than default-policy? The
>>> change itself looks good to me, but before giving my R-b I'd
>>> like to understand this aspect.
>> There is a very large amount wrong with xc_cpuid_set().
>>
>> For one, its behaviour is only useful for feature leaves, but it will
>> operate on any leaves the user requests, and while it is capable of
>> returning errors, libxl doesn't check the return value and continues
>> blindly forwards.
>>
>> Also from the L1TF embargo days is a series of work (or at least, the
>> start of) which is a total overhaul of how libxl and libxc interact when
>> it comes CPUID and MSR settings.  Neither xc_cpuid_set() nor
>> xc_cpuid_apply_policy() will survive to the end.
>>
>> For now, I've opted with not changing the semantics of the calls while
>> altering the data-transfer mechanism, to avoid conflating the two areas
>> of work.
> And with this then, as promised,
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

I'll expand the commit message with a note about this.  It will likely
be helpful for future people doing code archaeology.

~Andrew

Patch
diff mbox series

diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index a2d29a0fae..d1a2b61214 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -905,20 +905,80 @@  int xc_cpuid_set(
     const char **config, char **config_transformed)
 {
     int rc;
-    unsigned int i, j, regs[4], polregs[4];
-    struct cpuid_domain_info info = {};
+    unsigned int i, j, regs[4] = {}, polregs[4] = {};
+    xc_dominfo_t di;
+    xen_cpuid_leaf_t *leaves = NULL;
+    unsigned int nr_leaves, policy_leaves, nr_msrs;
+    uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
 
     for ( i = 0; i < 4; ++i )
         config_transformed[i] = NULL;
 
-    rc = get_cpuid_domain_info(xch, domid, &info, NULL, 0);
+    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_get_cpu_policy_size(xch, &nr_leaves, &nr_msrs);
     if ( rc )
-        goto out;
+    {
+        PERROR("Failed to obtain policy info size");
+        rc = -errno;
+        goto fail;
+    }
 
-    cpuid(input, regs);
+    rc = -ENOMEM;
+    if ( (leaves = calloc(nr_leaves, sizeof(*leaves))) == NULL )
+    {
+        ERROR("Unable to allocate memory for %u CPUID leaves", nr_leaves);
+        goto fail;
+    }
 
-    memcpy(polregs, regs, sizeof(regs));
-    xc_cpuid_policy(&info, input, polregs);
+    /* Get the domain's max policy. */
+    nr_msrs = 0;
+    policy_leaves = nr_leaves;
+    rc = xc_get_system_cpu_policy(xch, di.hvm ? XEN_SYSCTL_cpu_policy_hvm_max
+                                              : XEN_SYSCTL_cpu_policy_pv_max,
+                                  &policy_leaves, leaves, &nr_msrs, NULL);
+    if ( rc )
+    {
+        PERROR("Failed to obtain %s max policy", di.hvm ? "hvm" : "pv");
+        rc = -errno;
+        goto fail;
+    }
+    for ( i = 0; i < policy_leaves; ++i )
+        if ( leaves[i].leaf == input[0] && leaves[i].subleaf == input[1] )
+        {
+            polregs[0] = leaves[i].a;
+            polregs[1] = leaves[i].b;
+            polregs[2] = leaves[i].c;
+            polregs[3] = leaves[i].d;
+            break;
+        }
+
+    /* Get the host policy. */
+    nr_msrs = 0;
+    policy_leaves = nr_leaves;
+    rc = xc_get_system_cpu_policy(xch, XEN_SYSCTL_cpu_policy_host,
+                                  &policy_leaves, leaves, &nr_msrs, NULL);
+    if ( rc )
+    {
+        PERROR("Failed to obtain host policy");
+        rc = -errno;
+        goto fail;
+    }
+    for ( i = 0; i < policy_leaves; ++i )
+        if ( leaves[i].leaf == input[0] && leaves[i].subleaf == input[1] )
+        {
+            regs[0] = leaves[i].a;
+            regs[1] = leaves[i].b;
+            regs[2] = leaves[i].c;
+            regs[3] = leaves[i].d;
+            break;
+        }
 
     for ( i = 0; i < 4; i++ )
     {
@@ -969,9 +1029,21 @@  int xc_cpuid_set(
         }
     }
 
-    rc = xc_cpuid_do_domctl(xch, domid, input, regs);
-    if ( rc == 0 )
-        goto out;
+    /* Feed the transformed leaf back up to Xen. */
+    leaves[0] = (xen_cpuid_leaf_t){ input[0], input[1],
+                                    regs[0], regs[1], regs[2], regs[3] };
+    rc = xc_set_domain_cpu_policy(xch, domid, 1, 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 fail;
+    }
+
+    /* Success! */
+    goto out;
 
  fail:
     for ( i = 0; i < 4; i++ )
@@ -981,6 +1053,7 @@  int xc_cpuid_set(
     }
 
  out:
-    free_cpuid_domain_info(&info);
+    free(leaves);
+
     return rc;
 }