| Message ID | 20190911200504.5693-6-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/cpuid: Switch to using XEN_DOMCTL_set_cpumsr_policy | expand |
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
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
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
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
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; }
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(-)