On Tue, Jul 11, 2023 at 11:22:25AM +0200, Roger Pau Monne wrote:
> diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
> index 5b035223f4f5..5e5c8124dd74 100644
> --- a/tools/libs/guest/xg_cpuid_x86.c
> +++ b/tools/libs/guest/xg_cpuid_x86.c
> +static int xc_msr_policy(xc_interface *xch, domid_t domid,
> + const struct xc_msr *msr)
> +{
[...]
> +
> + rc = -ENOMEM;
This `rc' value looks unused, should it be moved to the if true block?
> + if ( (host = calloc(nr_msrs, sizeof(*host))) == NULL ||
> + (def = calloc(nr_msrs, sizeof(*def))) == NULL ||
> + (cur = calloc(nr_msrs, sizeof(*cur))) == NULL )
> + {
> + ERROR("Unable to allocate memory for %u CPUID leaves", nr_leaves);
> + goto fail;
> + }
> +
> + /* Get the domain's current policy. */
> + nr_leaves = 0;
> + nr_cur = nr_msrs;
> + rc = get_domain_cpu_policy(xch, domid, &nr_leaves, NULL, &nr_cur, cur);
[...]
> + for ( ; msr->index != XC_MSR_INPUT_UNUSED; ++msr )
> + {
> + xen_msr_entry_t *cur_msr = find_msr(cur, nr_cur, msr->index);
> + const xen_msr_entry_t *def_msr = find_msr(def, nr_def, msr->index);
> + const xen_msr_entry_t *host_msr = find_msr(host, nr_host,
> msr->index);
> + unsigned int i;
> +
> + if ( cur_msr == NULL || def_msr == NULL || host_msr == NULL )
> + {
> + ERROR("Missing MSR %#x", msr->index);
> + rc = -ENOENT;
> + goto fail;
> + }
> +
> + for ( i = 0; i < ARRAY_SIZE(msr->policy) - 1; i++ )
> + {
> + bool val;
> +
> + if ( msr->policy[i] == '1' )
> + val = true;
> + else if ( msr->policy[i] == '0' )
> + val = false;
> + else if ( msr->policy[i] == 'x' )
> + val = test_bit(63 - i, &def_msr->val);
> + else if ( msr->policy[i] == 'k' )
> + val = test_bit(63 - i, &host_msr->val);
> + else
> + {
> + ERROR("Bad character '%c' in policy string '%s'",
> + msr->policy[i], msr->policy);
Would it be useful to also display msr->index?
> + rc = -EINVAL;
> + goto fail;
> + }
> +
> + clear_bit(63 - i, &cur_msr->val);
> + if ( val )
> + set_bit(63 - i, &cur_msr->val);
Does this need to be first clear then set? A opposed to just do one or
the other.
> + }
> + }
> +
> + /* Feed the transformed policy back up to Xen. */
> + rc = xc_set_domain_cpu_policy(xch, domid, 0, NULL, nr_cur, cur,
> + &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! */
> +
> + fail:
Even if this label is only used on "fail", the code path is also use on
success. So a label named "out" might be more appropriate.
> + free(cur);
> + free(def);
> + free(host);
> +
> + return rc;
> +}
> +
In any case, patch looks fine to me:
Acked-by: Anthony PERARD <[email protected]>
Thanks,
--
Anthony PERARD
