Re: [Xen-devel] [PATCH v4 6/7] x86/msr: update domain policy on CPUID policy changes

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.10.17 at 10:27, <sergey.dyasli@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -124,6 +124,7 @@ static int update_domain_cpuid_info(struct domain *d,
>      }
>  
>      recalculate_cpuid_policy(d);
> +    recalculate_domain_msr_policy(d);

Considering the name of the other function, why not without the
"_domain"?

> +static void recalculate_domain_vmx_msr_policy(struct domain *d)
> +{
> +    struct msr_domain_policy *dp = d->arch.msr;
> +
> +    if ( !nestedhvm_enabled(d) || !d->arch.cpuid->basic.vmx )
> +    {
> +        memset(dp->vmx.raw, 0, sizeof(dp->vmx.raw));
> +        dp->vmx_procbased_ctls2.raw = 0;
> +        dp->vmx_ept_vpid_cap.raw = 0;
> +        memset(dp->vmx_true_ctls.raw, 0, sizeof(dp->vmx_true_ctls.raw));
> +        dp->vmx_vmfunc.raw = 0;
> +    }
> +    else
> +    {
> +        memcpy(dp->vmx.raw, hvm_max_msr_domain_policy.vmx.raw,
> +               sizeof(dp->vmx.raw));
> +        /* Get allowed CR4 bits from CPUID policy */
> +        dp->vmx.cr4_fixed1.raw = hvm_cr4_guest_valid_bits(d, false);
> +
> +        if ( dp->vmx.procbased_ctls.allowed_1.activate_secondary_controls )
> +        {
> +            dp->vmx_procbased_ctls2.raw =
> +                hvm_max_msr_domain_policy.vmx_procbased_ctls2.raw;
> +
> +            if ( dp->vmx_procbased_ctls2.allowed_1.enable_ept ||
> +                 dp->vmx_procbased_ctls2.allowed_1.enable_vpid )
> +                dp->vmx_ept_vpid_cap.raw =
> +                    hvm_max_msr_domain_policy.vmx_ept_vpid_cap.raw;
> +            else
> +                dp->vmx_ept_vpid_cap.raw = 0;
> +        }
> +        else
> +        {
> +            dp->vmx_procbased_ctls2.raw = 0;
> +            dp->vmx_ept_vpid_cap.raw = 0;
> +        }
> +
> +        if ( dp->vmx.basic.default1_zero )
> +            memcpy(dp->vmx_true_ctls.raw,
> +                   hvm_max_msr_domain_policy.vmx_true_ctls.raw,
> +                   sizeof(dp->vmx_true_ctls.raw));
> +        else
> +            memset(dp->vmx_true_ctls.raw, 0, sizeof(dp->vmx_true_ctls.raw));

Except for the "else" paths this looks pretty similar to code in one of
the earlier patches. Any chance the two copies could be morphed
into a single way of doing this, so the same logic (which will only get
more complicated over time) won't be present in two places?

Also it looks like there's an implied assumption that this won't be
called anymore after the guest is first started. While just like for
CPUID this is reasonable, is this actually being enforced anywhere?

Jan


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