Re: [PATCH v1] KVM: VMX: enable guest access to LMCE related MSRs

[Haozhong Zhang]

On 06/05/16 21:43, Borislav Petkov wrote:
> On Sun, Jun 05, 2016 at 11:14:56PM +0800, Haozhong Zhang wrote:
> > Ashok was also involved in the development of v1 patch and it's based
> > on his v0 patch, so I think I should take his SOB?
> 
> You have at least three options:
> 
> 1.
> From: Author Name 
> 
> ...
> 
> Signed-off-by: Author Name 
> [ Submitter did this and that to the Author's patch ]
> Signed-off-by: Submitter Name 
>

Thanks! I'll take this option.

Haozhong

> So you either do that or you say something like
>
> 2. "Based on an original patch by Ashok..."
> 
> in the commit message
> 
> or
> 
> you add the tag
> 
> 3. Originally-by: Ashok..
> 
> Makes sense?
> 
> For more info see Documentation/SubmittingPatches.
> 
> -- 
> Regards/Gruss,
> Boris.
> 
> SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 
> 21284 (AG Nürnberg)
> -- 

Re: [PATCH v1] KVM: VMX: enable guest access to LMCE related MSRs

[Haozhong Zhang]

On 06/05/16 21:43, Borislav Petkov wrote:
> On Sun, Jun 05, 2016 at 11:14:56PM +0800, Haozhong Zhang wrote:
> > Ashok was also involved in the development of v1 patch and it's based
> > on his v0 patch, so I think I should take his SOB?
> 
> You have at least three options:
> 
> 1.
> From: Author Name 
> 
> ...
> 
> Signed-off-by: Author Name 
> [ Submitter did this and that to the Author's patch ]
> Signed-off-by: Submitter Name 
>

Thanks! I'll take this option.

Haozhong

> So you either do that or you say something like
>
> 2. "Based on an original patch by Ashok..."
> 
> in the commit message
> 
> or
> 
> you add the tag
> 
> 3. Originally-by: Ashok..
> 
> Makes sense?
> 
> For more info see Documentation/SubmittingPatches.
> 
> -- 
> Regards/Gruss,
> Boris.
> 
> SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 
> 21284 (AG Nürnberg)
> -- 

Re: [PATCH v1] KVM: VMX: enable guest access to LMCE related MSRs

[Borislav Petkov]

On Sun, Jun 05, 2016 at 11:14:56PM +0800, Haozhong Zhang wrote:
> Ashok was also involved in the development of v1 patch and it's based
> on his v0 patch, so I think I should take his SOB?

You have at least three options:

1.
From: Author Name 

...

Signed-off-by: Author Name 
[ Submitter did this and that to the Author's patch ]
Signed-off-by: Submitter Name 

So you either do that or you say something like

2. "Based on an original patch by Ashok..."

in the commit message

or

you add the tag

3. Originally-by: Ashok..

Makes sense?

For more info see Documentation/SubmittingPatches.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [PATCH v1] KVM: VMX: enable guest access to LMCE related MSRs

[Borislav Petkov]

On Sun, Jun 05, 2016 at 11:14:56PM +0800, Haozhong Zhang wrote:
> Ashok was also involved in the development of v1 patch and it's based
> on his v0 patch, so I think I should take his SOB?

You have at least three options:

1.
From: Author Name 

...

Signed-off-by: Author Name 
[ Submitter did this and that to the Author's patch ]
Signed-off-by: Submitter Name 

So you either do that or you say something like

2. "Based on an original patch by Ashok..."

in the commit message

or

you add the tag

3. Originally-by: Ashok..

Makes sense?

For more info see Documentation/SubmittingPatches.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [PATCH v1] KVM: VMX: enable guest access to LMCE related MSRs

[Haozhong Zhang]

On 06/04/16 13:01, Boris Petkov wrote:
> Haozhong Zhang  wrote:
> 
> >On Intel platforms, this patch adds LMCE to KVM MCE supported
> >capabilities and handles guest access to LMCE related MSRs.
> >
> >Signed-off-by: Ashok Raj 
> >Signed-off-by: Haozhong Zhang 
> 
> SOB chain needs correction wrt who the author is and who the submitter. 
> 

Ashok was also involved in the development of v1 patch and it's based
on his v0 patch, so I think I should take his SOB?

Thanks,
Haozhong

Re: [PATCH v1] KVM: VMX: enable guest access to LMCE related MSRs

[Haozhong Zhang]

On 06/04/16 13:01, Boris Petkov wrote:
> Haozhong Zhang  wrote:
> 
> >On Intel platforms, this patch adds LMCE to KVM MCE supported
> >capabilities and handles guest access to LMCE related MSRs.
> >
> >Signed-off-by: Ashok Raj 
> >Signed-off-by: Haozhong Zhang 
> 
> SOB chain needs correction wrt who the author is and who the submitter. 
> 

Ashok was also involved in the development of v1 patch and it's based
on his v0 patch, so I think I should take his SOB?

Thanks,
Haozhong

Re: [PATCH v1] KVM: VMX: enable guest access to LMCE related MSRs

[Haozhong Zhang]

On 06/03/16 17:34, Radim Krčmář wrote:
> 2016-06-03 14:08+0800, Haozhong Zhang:
> > On Intel platforms, this patch adds LMCE to KVM MCE supported
> > capabilities and handles guest access to LMCE related MSRs.
> > 
> > Signed-off-by: Ashok Raj 
> > Signed-off-by: Haozhong Zhang 
> > ---
> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > @@ -2863,6 +2863,11 @@ static int vmx_get_vmx_msr(struct kvm_vcpu *vcpu, 
> > u32 msr_index, u64 *pdata)
> > return 0;
> >  }
> >  
> > +static inline bool vmx_feature_control_msr_required(struct kvm_vcpu *vcpu)
> 
> I'd call it "present", rather than "required".  SDM does so for other
> MSRs and it is easier to understand in the condition that returns #GP if
> this function is false.
>

Agree, I'll change.

> > +{
> > +   return nested_vmx_allowed(vcpu) || (vcpu->arch.mcg_cap & MCG_LMCE_P);
> > +}
> > @@ -2904,8 +2909,15 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct 
> > msr_data *msr_info)
> > case MSR_IA32_FEATURE_CONTROL:
> > -   if (!nested_vmx_allowed(vcpu))
> > +   if (!vmx_feature_control_msr_required(vcpu))
> > return 1;
> > msr_info->data = to_vmx(vcpu)->nested.msr_ia32_feature_control;
> 
> (MSR_IA32_FEATURE_CONTROL does not depend only on nested anymore, so
>  moving msr_ia32_feature_control from struct nested_vmx to struct
>  vcpu_vmx would make sense.)
>

will move in the next version

> > break;
> > @@ -2997,8 +3009,17 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct 
> > msr_data *msr_info)
> > +   case MSR_IA32_MCG_EXT_CTL:
> > +   if (!(vcpu->arch.mcg_cap & MCG_LMCE_P) ||
> > +   !(to_vmx(vcpu)->nested.msr_ia32_feature_control &
> > + FEATURE_CONTROL_LMCE))
> 
> (This check is used twice and could be given a name too,
>  "vmx_mcg_ext_ctl_msr_present()"?)
>

will change

> > +   return 1;
> > +   if (data && data != 0x1)
> 
>   (data & ~MCG_EXT_CTL_LMCE_EN)
> 
> is a clearer check for reserved bits.
>

ditto

> > +   return -1;
> > +   vcpu->arch.mcg_ext_ctl = data;
> > +   break;
> > case MSR_IA32_FEATURE_CONTROL:
> > -   if (!nested_vmx_allowed(vcpu) ||
> > +   if (!vmx_feature_control_msr_required(vcpu) ||
> > (to_vmx(vcpu)->nested.msr_ia32_feature_control &
> >  FEATURE_CONTROL_LOCKED && !msr_info->host_initiated))
> > return 1;
> 
> Does hardware throw #GP when FEATURE_CONTROL_LMCE is set without
> MCG_LMCE_P?
>

Yes, SDM vol 2 says setting reserved bits of MSR causes #GP.

> (We could emulate that by having a mask of valid bits and also use that
>  mask in place of vmx_feature_control_msr_required().  I don't think
>  there is a reason to have only vmx_feature_control_msr_required() if
>  the hardware can #GP on individual bits too.)
>

Oh yes, I should also validate the individual bits. I'll add it in the
next version.

Thanks,
Haozhong

Re: [PATCH v1] KVM: VMX: enable guest access to LMCE related MSRs

[Haozhong Zhang]

On 06/03/16 17:34, Radim Krčmář wrote:
> 2016-06-03 14:08+0800, Haozhong Zhang:
> > On Intel platforms, this patch adds LMCE to KVM MCE supported
> > capabilities and handles guest access to LMCE related MSRs.
> > 
> > Signed-off-by: Ashok Raj 
> > Signed-off-by: Haozhong Zhang 
> > ---
> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > @@ -2863,6 +2863,11 @@ static int vmx_get_vmx_msr(struct kvm_vcpu *vcpu, 
> > u32 msr_index, u64 *pdata)
> > return 0;
> >  }
> >  
> > +static inline bool vmx_feature_control_msr_required(struct kvm_vcpu *vcpu)
> 
> I'd call it "present", rather than "required".  SDM does so for other
> MSRs and it is easier to understand in the condition that returns #GP if
> this function is false.
>

Agree, I'll change.

> > +{
> > +   return nested_vmx_allowed(vcpu) || (vcpu->arch.mcg_cap & MCG_LMCE_P);
> > +}
> > @@ -2904,8 +2909,15 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct 
> > msr_data *msr_info)
> > case MSR_IA32_FEATURE_CONTROL:
> > -   if (!nested_vmx_allowed(vcpu))
> > +   if (!vmx_feature_control_msr_required(vcpu))
> > return 1;
> > msr_info->data = to_vmx(vcpu)->nested.msr_ia32_feature_control;
> 
> (MSR_IA32_FEATURE_CONTROL does not depend only on nested anymore, so
>  moving msr_ia32_feature_control from struct nested_vmx to struct
>  vcpu_vmx would make sense.)
>

will move in the next version

> > break;
> > @@ -2997,8 +3009,17 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct 
> > msr_data *msr_info)
> > +   case MSR_IA32_MCG_EXT_CTL:
> > +   if (!(vcpu->arch.mcg_cap & MCG_LMCE_P) ||
> > +   !(to_vmx(vcpu)->nested.msr_ia32_feature_control &
> > + FEATURE_CONTROL_LMCE))
> 
> (This check is used twice and could be given a name too,
>  "vmx_mcg_ext_ctl_msr_present()"?)
>

will change

> > +   return 1;
> > +   if (data && data != 0x1)
> 
>   (data & ~MCG_EXT_CTL_LMCE_EN)
> 
> is a clearer check for reserved bits.
>

ditto

> > +   return -1;
> > +   vcpu->arch.mcg_ext_ctl = data;
> > +   break;
> > case MSR_IA32_FEATURE_CONTROL:
> > -   if (!nested_vmx_allowed(vcpu) ||
> > +   if (!vmx_feature_control_msr_required(vcpu) ||
> > (to_vmx(vcpu)->nested.msr_ia32_feature_control &
> >  FEATURE_CONTROL_LOCKED && !msr_info->host_initiated))
> > return 1;
> 
> Does hardware throw #GP when FEATURE_CONTROL_LMCE is set without
> MCG_LMCE_P?
>

Yes, SDM vol 2 says setting reserved bits of MSR causes #GP.

> (We could emulate that by having a mask of valid bits and also use that
>  mask in place of vmx_feature_control_msr_required().  I don't think
>  there is a reason to have only vmx_feature_control_msr_required() if
>  the hardware can #GP on individual bits too.)
>

Oh yes, I should also validate the individual bits. I'll add it in the
next version.

Thanks,
Haozhong

Re: [PATCH v1] KVM: VMX: enable guest access to LMCE related MSRs

[Boris Petkov]

Haozhong Zhang  wrote:

>On Intel platforms, this patch adds LMCE to KVM MCE supported
>capabilities and handles guest access to LMCE related MSRs.
>
>Signed-off-by: Ashok Raj 
>Signed-off-by: Haozhong Zhang 

SOB chain needs correction wrt who the author is and who the submitter. 


-- 
Sent from a small device: formatting sux and brevity is inevitable. 

Re: [PATCH v1] KVM: VMX: enable guest access to LMCE related MSRs

[Boris Petkov]

Haozhong Zhang  wrote:

>On Intel platforms, this patch adds LMCE to KVM MCE supported
>capabilities and handles guest access to LMCE related MSRs.
>
>Signed-off-by: Ashok Raj 
>Signed-off-by: Haozhong Zhang 

SOB chain needs correction wrt who the author is and who the submitter. 


-- 
Sent from a small device: formatting sux and brevity is inevitable. 

Re: [PATCH v1] KVM: VMX: enable guest access to LMCE related MSRs

[Radim Krčmář]

2016-06-03 14:08+0800, Haozhong Zhang:
> On Intel platforms, this patch adds LMCE to KVM MCE supported
> capabilities and handles guest access to LMCE related MSRs.
> 
> Signed-off-by: Ashok Raj 
> Signed-off-by: Haozhong Zhang 
> ---
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> @@ -2863,6 +2863,11 @@ static int vmx_get_vmx_msr(struct kvm_vcpu *vcpu, u32 
> msr_index, u64 *pdata)
>   return 0;
>  }
>  
> +static inline bool vmx_feature_control_msr_required(struct kvm_vcpu *vcpu)

I'd call it "present", rather than "required".  SDM does so for other
MSRs and it is easier to understand in the condition that returns #GP if
this function is false.

> +{
> + return nested_vmx_allowed(vcpu) || (vcpu->arch.mcg_cap & MCG_LMCE_P);
> +}
> @@ -2904,8 +2909,15 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct 
> msr_data *msr_info)
>   case MSR_IA32_FEATURE_CONTROL:
> - if (!nested_vmx_allowed(vcpu))
> + if (!vmx_feature_control_msr_required(vcpu))
>   return 1;
>   msr_info->data = to_vmx(vcpu)->nested.msr_ia32_feature_control;

(MSR_IA32_FEATURE_CONTROL does not depend only on nested anymore, so
 moving msr_ia32_feature_control from struct nested_vmx to struct
 vcpu_vmx would make sense.)

>   break;
> @@ -2997,8 +3009,17 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct 
> msr_data *msr_info)
> + case MSR_IA32_MCG_EXT_CTL:
> + if (!(vcpu->arch.mcg_cap & MCG_LMCE_P) ||
> + !(to_vmx(vcpu)->nested.msr_ia32_feature_control &
> +   FEATURE_CONTROL_LMCE))

(This check is used twice and could be given a name too,
 "vmx_mcg_ext_ctl_msr_present()"?)

> + return 1;
> + if (data && data != 0x1)

  (data & ~MCG_EXT_CTL_LMCE_EN)

is a clearer check for reserved bits.

> + return -1;
> + vcpu->arch.mcg_ext_ctl = data;
> + break;
>   case MSR_IA32_FEATURE_CONTROL:
> - if (!nested_vmx_allowed(vcpu) ||
> + if (!vmx_feature_control_msr_required(vcpu) ||
>   (to_vmx(vcpu)->nested.msr_ia32_feature_control &
>FEATURE_CONTROL_LOCKED && !msr_info->host_initiated))
>   return 1;

Does hardware throw #GP when FEATURE_CONTROL_LMCE is set without
MCG_LMCE_P?

(We could emulate that by having a mask of valid bits and also use that
 mask in place of vmx_feature_control_msr_required().  I don't think
 there is a reason to have only vmx_feature_control_msr_required() if
 the hardware can #GP on individual bits too.)

Thanks.

Re: [PATCH v1] KVM: VMX: enable guest access to LMCE related MSRs

[Radim Krčmář]

2016-06-03 14:08+0800, Haozhong Zhang:
> On Intel platforms, this patch adds LMCE to KVM MCE supported
> capabilities and handles guest access to LMCE related MSRs.
> 
> Signed-off-by: Ashok Raj 
> Signed-off-by: Haozhong Zhang 
> ---
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> @@ -2863,6 +2863,11 @@ static int vmx_get_vmx_msr(struct kvm_vcpu *vcpu, u32 
> msr_index, u64 *pdata)
>   return 0;
>  }
>  
> +static inline bool vmx_feature_control_msr_required(struct kvm_vcpu *vcpu)

I'd call it "present", rather than "required".  SDM does so for other
MSRs and it is easier to understand in the condition that returns #GP if
this function is false.

> +{
> + return nested_vmx_allowed(vcpu) || (vcpu->arch.mcg_cap & MCG_LMCE_P);
> +}
> @@ -2904,8 +2909,15 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct 
> msr_data *msr_info)
>   case MSR_IA32_FEATURE_CONTROL:
> - if (!nested_vmx_allowed(vcpu))
> + if (!vmx_feature_control_msr_required(vcpu))
>   return 1;
>   msr_info->data = to_vmx(vcpu)->nested.msr_ia32_feature_control;

(MSR_IA32_FEATURE_CONTROL does not depend only on nested anymore, so
 moving msr_ia32_feature_control from struct nested_vmx to struct
 vcpu_vmx would make sense.)

>   break;
> @@ -2997,8 +3009,17 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct 
> msr_data *msr_info)
> + case MSR_IA32_MCG_EXT_CTL:
> + if (!(vcpu->arch.mcg_cap & MCG_LMCE_P) ||
> + !(to_vmx(vcpu)->nested.msr_ia32_feature_control &
> +   FEATURE_CONTROL_LMCE))

(This check is used twice and could be given a name too,
 "vmx_mcg_ext_ctl_msr_present()"?)

> + return 1;
> + if (data && data != 0x1)

  (data & ~MCG_EXT_CTL_LMCE_EN)

is a clearer check for reserved bits.

> + return -1;
> + vcpu->arch.mcg_ext_ctl = data;
> + break;
>   case MSR_IA32_FEATURE_CONTROL:
> - if (!nested_vmx_allowed(vcpu) ||
> + if (!vmx_feature_control_msr_required(vcpu) ||
>   (to_vmx(vcpu)->nested.msr_ia32_feature_control &
>FEATURE_CONTROL_LOCKED && !msr_info->host_initiated))
>   return 1;

Does hardware throw #GP when FEATURE_CONTROL_LMCE is set without
MCG_LMCE_P?

(We could emulate that by having a mask of valid bits and also use that
 mask in place of vmx_feature_control_msr_required().  I don't think
 there is a reason to have only vmx_feature_control_msr_required() if
 the hardware can #GP on individual bits too.)

Thanks.