Re: [Xen-devel] [PATCH v1 2/6] vmx: add raw_vmx_msr_policy

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 26.06.17 at 12:44, <sergey.dyasli@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -144,6 +144,8 @@ static void __init vmx_display_features(void)
>          printk(" - none\n");
>  }
>  
> +struct vmx_msr_policy __read_mostly raw_vmx_msr_policy;

Does this really need to be non-static? I don't see a use outside of
this file in the patch here at least.

> @@ -152,6 +154,74 @@ bool vmx_msr_available(struct vmx_msr_policy *p, 
> uint32_t msr)
>      return p->available & (1u << (msr - MSR_IA32_VMX_BASIC));
>  }
>  
> +int calculate_raw_policy(bool bsp)
> +{
> +    struct vmx_msr_policy policy;
> +    struct vmx_msr_policy *p = &policy;
> +    int msr;

unsigned int

> +    /* Raw policy is filled only on boot CPU */
> +    if ( bsp )
> +        p = &raw_vmx_msr_policy;
> +    else
> +        memset(&policy, 0, sizeof(policy));
> +
> +    p->available = 0x7ff;

(1u << (MSR_IA32_VMX_VMCS_ENUM + 1 - MSR_IA32_VMX_BASIC)) - 1

> +    for ( msr = MSR_IA32_VMX_BASIC; msr <= MSR_IA32_VMX_VMCS_ENUM; msr++ )
> +        rdmsrl(msr, p->msr[msr - MSR_IA32_VMX_BASIC]);
> +
> +    if ( p->basic.default1_zero )
> +    {
> +        p->available |= 0x1e000;

Same here and further down - please calculate the values from
available constants. Maybe you want to have a helper macro or
inline function.

> +    /* Check that secondary CPUs have exactly the same bits in VMX MSRs */
> +    if ( !bsp && memcmp(p, &raw_vmx_msr_policy, sizeof(*p)) != 0 )
> +    {
> +        for ( msr = MSR_IA32_VMX_BASIC; msr <= MSR_IA32_VMX_VMFUNC; msr++ )
> +        {
> +            if ( p->msr[msr - MSR_IA32_VMX_BASIC] !=
> +                 raw_vmx_msr_policy.msr[msr - MSR_IA32_VMX_BASIC] )
> +            {
> +                printk("VMX msr %#x: saw 0x%016"PRIx64" expected 
> 0x%016"PRIx64
> +                        "\n", msr, p->msr[msr - MSR_IA32_VMX_BASIC],

Please keep the newline on the same line as the rest of the format
string. It being slightly longer then really wanted is okay for format
strings.

> @@ -611,6 +624,9 @@ int vmx_cpu_up(void)
>  
>      BUG_ON(!(read_cr4() & X86_CR4_VMXE));
>  
> +    if ( (rc = calculate_raw_policy(false)) != 0 )
> +        return rc;
> +
>      /* 
>       * Ensure the current processor operating mode meets 
>       * the requred CRO fixed bits in VMX operation. 

Following here are reads of MSR_IA32_VMX_CR0_FIXED{0,1} which
you should be able to drop now, instead using the raw policy you've
just checked matches this CPU.

Btw., is it intentional that the function is being invoked for the BSP a
second time here (after start_vmx() did so already), with the flag
now being passed with the wrong value?

> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2432,6 +2432,8 @@ static void pi_notification_interrupt(struct 
> cpu_user_regs *regs)
>      raise_softirq(VCPU_KICK_SOFTIRQ);
>  }
>  
> +int calculate_raw_policy(bool bsp);

Declarations need to go in a header included by both producer and
consumer, so that someone changing only one of definition and
declaration will be forced to also change the other side.

Jan


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