Re: [Xen-devel] [PATCH v7 1/7] x86/domctl: generalize the restore of vMCE parameters

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.07.17 at 05:53, <haozhong.zhang@xxxxxxxxx> wrote:
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -302,6 +302,39 @@ static int update_domain_cpuid_info(struct domain *d,
>      return 0;
>  }
>  
> +static int vcpu_set_vmce(struct vcpu *v,
> +                         const struct xen_domctl_ext_vcpucontext *evc)
> +{
> +    /*
> +     * Sizes of vMCE parameters used by the current and past versions
> +     * of Xen in descending order. If vMCE parameters are extended,
> +     * remember to add the old size in this array.
> +     */
> +    static const unsigned int valid_vmce_size[] = {

valid_sizes[] ?

> +        sizeof(evc->vmce),
> +        sizeof(evc->mcg_cap),

The first sizeof() is fine, but I think all subsequent entries should
be offsetof() + sizeof(). Looking ahead into patch 2, you're indeed
coding it the wrong way there (just think about the validity of the
expression used when another field gets added), which to me hints
that you've fallen into your own trap.

It may then be worthwhile to macro-ize this so that the relevant
field name needs to only be handed once to the macro.

> +        0,

I think you can easily get away without this sentinel. Alternatively
there's no point in using ARRAY_SIZE() further down.

> +    };
> +    struct hvm_vmce_vcpu vmce = { };
> +    unsigned int vmce_size = evc->size - offsetof(typeof(*evc), mcg_cap);
> +    int i = 0;

unsigned int

> +    BUILD_BUG_ON(offsetof(struct xen_domctl_ext_vcpucontext, mcg_cap) !=
> +                 offsetof(struct xen_domctl_ext_vcpucontext, vmce.caps));

Please consistently use typeof(*evc) now that you move (and hence
touch) this code anyway.

> +    BUILD_BUG_ON(sizeof(evc->mcg_cap) != sizeof(evc->vmce.caps));
> +
> +    while ( i < ARRAY_SIZE(valid_vmce_size) - 1 &&
> +            vmce_size < valid_vmce_size[i] )
> +        ++i;
> +    vmce_size = valid_vmce_size[i];
> +
> +    if ( !vmce_size )
> +        return 0;
> +
> +    memcpy(&vmce, &evc->vmce, vmce_size);
> +    return vmce_restore_vcpu(v, &vmce);

Blank line before final return statement please.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel