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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.07.18 at 22:03, <andrew.cooper3@xxxxxxxxxx> 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.

> --- 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?

Jan


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