Re: [PATCH v2 1/6] libs/guest: introduce support for setting guest MSRs

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

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 <anthony.perard@xxxxxxxxxx>

Thanks,

-- 
Anthony PERARD