Re: [Xen-devel] [PATCH 3/8] x86/domctl: Implement XEN_DOMCTL_set_cpumsr_policy

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 12/09/2019 09:06, Jan Beulich wrote:
> On 11.09.2019 22:04, Andrew Cooper wrote:
>> --- a/tools/libxc/xc_cpuid_x86.c
>> +++ b/tools/libxc/xc_cpuid_x86.c
>> @@ -229,6 +229,55 @@ int xc_get_domain_cpu_policy(xc_interface *xch, uint32_t domid,
>>      return ret;
>>  }
>>  
>> +int xc_set_domain_cpu_policy(xc_interface *xch, uint32_t domid,
>> +                             uint32_t nr_leaves, xen_cpuid_leaf_t *leaves,
>> +                             uint32_t nr_msrs, xen_msr_entry_t *msrs,
>> +                             uint32_t *err_leaf_p, uint32_t *err_subleaf_p,
>> +                             uint32_t *err_msr_idx_p)
>> +{
>> +    DECLARE_DOMCTL;
>> +    DECLARE_HYPERCALL_BOUNCE(leaves,
>> +                             nr_leaves * sizeof(*leaves),
>> +                             XC_HYPERCALL_BUFFER_BOUNCE_IN);
>> +    DECLARE_HYPERCALL_BOUNCE(msrs,
>> +                             nr_msrs * sizeof(*msrs),
>> +                             XC_HYPERCALL_BUFFER_BOUNCE_IN);
> With both being IN, the respective function parameters should imo
> be pointers to const.

Ok.

>
>> +    int ret;
>> +
>> +    if ( xc_hypercall_bounce_pre(xch, leaves) )
>> +        return -1;
>> +
>> +    if ( xc_hypercall_bounce_pre(xch, msrs) )
>> +        return -1;
>> +
>> +    domctl.cmd = XEN_DOMCTL_set_cpu_policy;
>> +    domctl.domain = domid;
>> +    domctl.u.cpu_policy.nr_leaves = nr_leaves;
>> +    set_xen_guest_handle(domctl.u.cpu_policy.cpuid_policy, leaves);
>> +    domctl.u.cpu_policy.nr_msrs = nr_msrs;
>> +    set_xen_guest_handle(domctl.u.cpu_policy.msr_policy, msrs);
>> +    domctl.u.cpu_policy.err_leaf = ~0;
>> +    domctl.u.cpu_policy.err_subleaf = ~0;
>> +    domctl.u.cpu_policy.err_msr_idx = ~0;
> The fields are marked OUT only in the public header, which implies
> no initialization should be needed here, as the hypercall would
> overwrite the fields in any event.

See below.

>
>> --- a/xen/arch/x86/domctl.c
>> +++ b/xen/arch/x86/domctl.c
>> @@ -294,6 +294,65 @@ static int update_domain_cpuid_info(struct domain *d,
>>      return 0;
>>  }
>>  
>> +static int update_domain_cpu_policy(struct domain *d,
>> +                                    xen_domctl_cpu_policy_t *xdpc)
>> +{
>> +    struct cpu_policy new = {};
>> +    const struct cpu_policy *sys = is_pv_domain(d)
>> +        ? &system_policies[XEN_SYSCTL_cpu_policy_pv_max]
>> +        : &system_policies[XEN_SYSCTL_cpu_policy_hvm_max];
>> +    struct cpu_policy_errors err = INIT_CPU_POLICY_ERRORS;
>> +    int ret = -ENOMEM;
>> +
>> +    /* Start by copying the domain's existing policies. */
>> +    if ( !(new.cpuid = xmemdup(d->arch.cpuid)) ||
>> +         !(new.msr   = xmemdup(d->arch.msr)) )
> To avoid the redundant initialization, this could as well be the
> initializer of the variable.

I'm not sure that is the wisest course of action.  We wouldn't want to
proactively perform the memory allocation if new logic needs to appear
ahead of this.

In this example, the compiler ought to be able to do DSE to get rid of
the first assignment.

>
>> @@ -1476,6 +1535,27 @@ long arch_do_domctl(
>>          copyback = true;
>>          break;
>>  
>> +    case XEN_DOMCTL_set_cpu_policy:
>> +        if ( d == currd ) /* No domain_pause() */
>> +        {
>> +            ret = -EINVAL;
>> +            break;
>> +        }
>> +
>> +        domain_pause(d);
>> +
>> +        if ( d->creation_finished )
>> +            ret = -EEXIST; /* No changing once the domain is running. */
>> +        else
>> +        {
>> +            ret = update_domain_cpu_policy(d, &domctl->u.cpu_policy);
>> +            if ( ret ) /* Copy domctl->u.cpu_policy.err_* to guest. */
>> +                copyback = true;
> Due to the OUT in the public header I think it would be better to
> always copy this back (making sure the invalid markers are in place
> in case of success). But I guess we're not very consistent with
> honoring OUT like this.

This doesn't work, because an early ESRCH/EBUSY won't fill in the
pointers even with copyback being changed here.

This is why xc_set_domain_cpu_policy() fills the values to begin with.

>
>> --- a/xen/include/public/domctl.h
>> +++ b/xen/include/public/domctl.h
>> @@ -658,17 +658,23 @@ struct xen_domctl_cpuid {
>>  };
>>  
>>  /*
>> - * XEN_DOMCTL_get_cpu_policy (x86 specific)
>> + * XEN_DOMCTL_{get,set}_cpu_policy (x86 specific)
>>   *
>> - * Query the CPUID and MSR policies for a specific domain.
>> + * Query or set the CPUID and MSR policies for a specific domain.
>>   */
>>  struct xen_domctl_cpu_policy {
>>      uint32_t nr_leaves; /* IN/OUT: Number of leaves in/written to
>>                           * 'cpuid_policy'. */
>>      uint32_t nr_msrs;   /* IN/OUT: Number of MSRs in/written to
>>                           * 'msr_domain_policy' */
>> -    XEN_GUEST_HANDLE_64(xen_cpuid_leaf_t) cpuid_policy; /* OUT */
>> -    XEN_GUEST_HANDLE_64(xen_msr_entry_t) msr_policy;    /* OUT */
>> +    XEN_GUEST_HANDLE_64(xen_cpuid_leaf_t) cpuid_policy; /* IN/OUT */
>> +    XEN_GUEST_HANDLE_64(xen_msr_entry_t) msr_policy;    /* IN/OUT */
>> +    uint32_t err_leaf, err_subleaf; /* OUT, set_policy only.  If not ~0,
>> +                                     * indicates the leaf/subleaf which
>> +                                     * auditing objected to. */
>> +    uint32_t err_msr_idx;           /* OUT, set_policy only.  If not ~0,
>> +                                     * indicates the MSR idx which
>> +                                     * auditing objected to. */
>>  };
>>  typedef struct xen_domctl_cpu_policy xen_domctl_cpu_policy_t;
>>  DEFINE_XEN_GUEST_HANDLE(xen_domctl_cpu_policy_t);
> I know you're not liking the concept, but XEN_DOMCTL_INTERFACE_VERSION
> hasn't been bumped in this release cycle yet, and hence a binary
> incompatible change like this one needs to.

Oh.  The lack of bump had escaped me.

>  With at least this last
> aspect taken care of, hypervisor parts
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks,

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel