On Wed, Jul 12, 2023 at 03:34:23PM +0100, Anthony PERARD wrote:
> 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?
This is mostly copy&paste from the cpuid counterpart, some parts of
the code tend to use this kind of error assignation, but it only makes
sense when the if is the code block is a single line in order to avoid
the braces. Since here the code block already requires braces, doing
the assign outside is pointless.
> > + 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?
Why not, will add it.
> > + 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.
Will adjust, I assume some people prefer 3 lines instead of 4 if you
use and if ... else ...?
> > + }
> > + }
> > +
> > + /* 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.per...@citrix.com>
Will make the adjustments requested.
Thanks, Roger.
