| Message ID | 20190911200504.5693-5-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: > This patch is broken out just to simplify the following two. > > For xc_cpuid_set(), document how the 'k' works because it is quite subtle. > Replace a memset() with weird calculation for a loop of 4 explicit NULL > assigments. This mirrors the free()'s in the fail path. > > For xc_cpuid_apply_policy(), const-ify the featureset pointer. It isn't > written to, and was never intended to be mutable. > > Drop three pieces of trailing whitespace. > > No functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com>
On 11.09.2019 22:05, Andrew Cooper wrote: > @@ -935,6 +935,13 @@ int xc_cpuid_set( > goto fail; > } > > + /* > + * Notes for following this algorithm: > + * > + * While it will accept any leaf data, it only makes sense to use on > + * feature leaves. regs[] initially contains the host values. This, > + * with the fall-through chain is how the 'k' option works. > + */ > for ( j = 0; j < 32; j++ ) > { > unsigned char val = !!((regs[i] & (1U << (31 - j)))); Looking at the next patch (and hence more closely at the code below here) - shouldn't the comment mention both 'k' and 's'? Jan
On 12/09/2019 09:17, Jan Beulich wrote: > On 11.09.2019 22:05, Andrew Cooper wrote: >> @@ -935,6 +935,13 @@ int xc_cpuid_set( >> goto fail; >> } >> >> + /* >> + * Notes for following this algorithm: >> + * >> + * While it will accept any leaf data, it only makes sense to use on >> + * feature leaves. regs[] initially contains the host values. This, >> + * with the fall-through chain is how the 'k' option works. >> + */ >> for ( j = 0; j < 32; j++ ) >> { >> unsigned char val = !!((regs[i] & (1U << (31 - j)))); > Looking at the next patch (and hence more closely at the code below > here) - shouldn't the comment mention both 'k' and 's'? Hmm. The loop didn't contain any logic looking at == 'k' which is why I started wondering, but I think you're right - the only use of 's' is for the feedback to the caller, not for calculation purposes. I'll adjust the comment suitably. ~Andrew
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h index e47778535d..2419a47f22 100644 --- a/tools/libxc/include/xenctrl.h +++ b/tools/libxc/include/xenctrl.h @@ -1800,7 +1800,7 @@ int xc_cpuid_set(xc_interface *xch, char **config_transformed); int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, - uint32_t *featureset, + const uint32_t *featureset, unsigned int nr_features); int xc_mca_op(xc_interface *xch, struct xen_mc *mc); int xc_mca_op_inject_v2(xc_interface *xch, unsigned int flags, diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c index 33b9e9fc85..a2d29a0fae 100644 --- a/tools/libxc/xc_cpuid_x86.c +++ b/tools/libxc/xc_cpuid_x86.c @@ -1,5 +1,5 @@ /****************************************************************************** - * xc_cpuid_x86.c + * xc_cpuid_x86.c * * Compute cpuid of a domain. * @@ -332,7 +332,7 @@ static void cpuid(const unsigned int *input, unsigned int *regs) static int get_cpuid_domain_info(xc_interface *xch, uint32_t domid, struct cpuid_domain_info *info, - uint32_t *featureset, + const uint32_t *featureset, unsigned int nr_features) { struct xen_domctl domctl = {}; @@ -807,8 +807,7 @@ static void sanitise_featureset(struct cpuid_domain_info *info) } int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, - uint32_t *featureset, - unsigned int nr_features) + const uint32_t *featureset, unsigned int nr_features) { struct cpuid_domain_info info = {}; unsigned int input[2] = { 0, 0 }, regs[4]; @@ -898,7 +897,7 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, * 'k' -> pass through host value * 's' -> pass through the first time and then keep the same value * across save/restore and migration. - * + * * For 's' and 'x' the configuration is overwritten with the value applied. */ int xc_cpuid_set( @@ -909,7 +908,8 @@ int xc_cpuid_set( unsigned int i, j, regs[4], polregs[4]; struct cpuid_domain_info info = {}; - memset(config_transformed, 0, 4 * sizeof(*config_transformed)); + for ( i = 0; i < 4; ++i ) + config_transformed[i] = NULL; rc = get_cpuid_domain_info(xch, domid, &info, NULL, 0); if ( rc ) @@ -927,7 +927,7 @@ int xc_cpuid_set( regs[i] = polregs[i]; continue; } - + config_transformed[i] = calloc(33, 1); /* 32 bits, NUL terminator. */ if ( config_transformed[i] == NULL ) { @@ -935,6 +935,13 @@ int xc_cpuid_set( goto fail; } + /* + * Notes for following this algorithm: + * + * While it will accept any leaf data, it only makes sense to use on + * feature leaves. regs[] initially contains the host values. This, + * with the fall-through chain is how the 'k' option works. + */ for ( j = 0; j < 32; j++ ) { unsigned char val = !!((regs[i] & (1U << (31 - j))));
This patch is broken out just to simplify the following two. For xc_cpuid_set(), document how the 'k' works because it is quite subtle. Replace a memset() with weird calculation for a loop of 4 explicit NULL assigments. This mirrors the free()'s in the fail path. For xc_cpuid_apply_policy(), const-ify the featureset pointer. It isn't written to, and was never intended to be mutable. Drop three pieces of trailing whitespace. No functional change. 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/include/xenctrl.h | 2 +- tools/libxc/xc_cpuid_x86.c | 21 ++++++++++++++------- 2 files changed, 15 insertions(+), 8 deletions(-)