Re: [PATCH v2 12/13] x86/sysctl: Implement XEN_SYSCTL_get_cpu_policy

["Jan Beulich" <JBeulich@suse.com>]

>>> On 17.07.18 at 18:50, <andrew.cooper3@citrix.com> wrote:
> On 16/07/18 12:54, Jan Beulich wrote:
>>>>> On 13.07.18 at 22:03, <andrew.cooper3@citrix.com> wrote:
>>> @@ -322,6 +323,76 @@ long arch_do_sysctl(
>>>          break;
>>>      }
>>>  
>>> +    case XEN_SYSCTL_get_cpu_policy:
>>> +    {
>>> +        const struct cpu_policy *policy;
>>> +
>>> +        /* Bad policy index? */
>>> +        if ( sysctl->u.cpu_policy.index >= ARRAY_SIZE(system_policies) )
>>> +        {
>>> +            ret = -EINVAL;
>>> +            break;
>>> +        }
>>> +        policy = &system_policies[
>>> +            array_index_nospec(sysctl->u.cpu_policy.index,
>>> +                               ARRAY_SIZE(system_policies))];
>>> +
>>> +        /* Request for maximum number of leaves/MSRs? */
>>> +        if ( guest_handle_is_null(sysctl->u.cpu_policy.cpuid_policy) )
>>> +        {
>>> +            sysctl->u.cpu_policy.nr_leaves = CPUID_MAX_SERIALISED_LEAVES;
>>> +            if ( __copy_field_to_guest(u_sysctl, sysctl,
>>> +                                       u.cpu_policy.nr_leaves) )
>>> +            {
>>> +                ret = -EFAULT;
>>> +                break;
>>> +            }
>> Could I talk you into avoiding the redundancy here and fold this
>> copying with ...
>>
>>> +        }
>>> +        if ( guest_handle_is_null(sysctl->u.cpu_policy.msr_policy) )
>>> +        {
>>> +            sysctl->u.cpu_policy.nr_msrs = MSR_MAX_SERIALISED_ENTRIES;
>>> +            if ( __copy_field_to_guest(u_sysctl, sysctl,
>>> +                                       u.cpu_policy.nr_msrs) )
>>> +            {
>>> +                ret = -EFAULT;
>>> +                break;
>>> +            }
>>> +        }
>>> +
>>> +        /* Serialise the information the caller wants. */
>>> +        if ( !guest_handle_is_null(sysctl->u.cpu_policy.cpuid_policy) )
>>> +        {
>>> +            if ( (ret = x86_cpuid_copy_to_buffer(
>>> +                      policy->cpuid,
>>> +                      sysctl->u.cpu_policy.cpuid_policy,
>>> +                      &sysctl->u.cpu_policy.nr_leaves)) )
>>> +                break;
>>> +
>>> +            if ( __copy_field_to_guest(u_sysctl, sysctl,
>>> +                                       u.cpu_policy.nr_leaves)  )
>>> +            {
>>> +                ret = -EFAULT;
>>> +                break;
>>> +            }
>> ... this (and the MSR ones respectively), by moving both out of and
>> past their outer if()-s? This would the also call for two if/else-if pairs
>> instead of four if()-s.
> 
> I

???

>>> --- a/xen/include/public/sysctl.h
>>> +++ b/xen/include/public/sysctl.h
>>> @@ -1075,12 +1075,25 @@ struct xen_sysctl_set_parameter {
>>>   *  - Default_*: Default set of features a PV or HVM guest can use.  This is
>>>   *               the security supported set.
>>>   */
>>> +struct xen_sysctl_cpu_policy {
>>>  #define XEN_SYSCTL_cpu_policy_raw          0
>>>  #define XEN_SYSCTL_cpu_policy_host         1
>>>  #define XEN_SYSCTL_cpu_policy_pv_max       2
>>>  #define XEN_SYSCTL_cpu_policy_hvm_max      3
>>>  #define XEN_SYSCTL_cpu_policy_pv_default   4
>>>  #define XEN_SYSCTL_cpu_policy_hvm_default  5
>>> +    uint32_t index;       /* IN: Which policy to query? */
>>> +    uint32_t nr_leaves;   /* IN/OUT: Number of leaves in/written to
>>> +                           * 'cpuid_policy', or the maximum number of leaves
>>> +                           * if the guest handle is NULL. */
>>> +    uint32_t nr_msrs;     /* IN/OUT: Number of MSRs in/written to
>>> +                           * 'msr_policy', or the maximum number of MSRs if
>>> +                           * the guest handle is NULL. */
>>> +    XEN_GUEST_HANDLE_64(xen_cpuid_leaf_t) cpuid_policy; /* OUT: */
>>> +    XEN_GUEST_HANDLE_64(xen_msr_entry_t) msr_policy;    /* OUT: */
>> Stray colons in the comments?
>>
>> I'm also not overly happy to see there's still no explicit padding here.
>> I know you dislike it, but I think as long as we have no better
>> replacement to the interface versioning, we should at least try to
>> limit the number of bumps it needs, and that calls for making padding
>> explicit, zeroing it for output and checking it to be zero when input,
>> so that the field can be assigned meaning subsequently. Otherwise
>> why did we tell others to add explicit padding over the last so many
>> years, without you voicing your opinion to the contrary?
> 
> I've never (knowingly) requested padding in the unstable interface. 
> I've certainly requested rearrangements for better packing, and
> requested padding for bits of the stable ABI.

I don't recall you requesting such, indeed, but I also don't recall you
objecting to me (and iirc also a few others) doing so.

> The sysctl structure itself is full of holes, and this is not an
> appropriate time or place to be making partial changes to the ABI.

Well, I'm not going to insist, since as you say it's inconsistent at
present anyway. But we'll want to settle on one of the other
model for new additions, and then consider converting existing
ones accordingly.

Jan


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