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

[Jan Beulich <jbeulich@suse.com>]

On 12.09.2019 15:15, Andrew Cooper wrote:
> On 12/09/2019 09:06, Jan Beulich wrote:
>> On 11.09.2019 22:04, Andrew Cooper wrote:
>>> --- 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.

Okay. I said "could" in the first place to make clear this
really is just an option to consider.

>>> @@ -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.

Oh, right. Perhaps the public header comments then want refining,
since ...

>>> --- 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. */

... what is being said here isn't true in the case you mention
if the caller didn't set the fields accordingly.

Jan

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