diff mbox series

[v3,7/8] KVM:X86: Add XSS bit 11 and 12 support for CET xsaves/xrstors.

Message ID 20190225132716.6982-8-weijiang.yang@intel.com (mailing list archive)
State New, archived
Headers show
Series This patch-set is to enable Guest CET support | expand

Commit Message

Yang Weijiang Feb. 25, 2019, 1:27 p.m. UTC 
For Guest XSS, right now, only bit 11(user states) and bit 12
(supervisor states) are supported, if other bits are being set,
need to modify KVM_SUPPORTED_XSS macro to have support.

Signed-off-by: Zhang Yi Z <yi.z.zhang@linux.intel.com>
Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
---
 arch/x86/kvm/vmx.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Comments

Yang Weijiang Feb. 28, 2019, 8:44 a.m. UTC | #1 
On Thu, Feb 28, 2019 at 08:25:12AM -0800, Sean Christopherson wrote:
> On Mon, Feb 25, 2019 at 09:27:15PM +0800, Yang Weijiang wrote:
> > For Guest XSS, right now, only bit 11(user states) and bit 12
> > (supervisor states) are supported, if other bits are being set,
> > need to modify KVM_SUPPORTED_XSS macro to have support.
> 
> The changelog should describe what the change does directly.  Referencing
> specific bits implies that the code is explicitly checking for said bits,
> which it does not.  And there's no need to advise readers on how to add
> more bits in the future, e.g. the KVM_SUPPORTED_XSS macro may not exist
> the next time new bits are added.  Just cover what the patch does and why.
> 
> Something like:
> 
> KVM: x86: Allow the guest to set supported bits in XSS
> 
> ...now that KVM supports setting CET related bits.  Previously, KVM did
> not support setting any bits in XSS and so hardcoded its check to inject
> a #GP if the guest attempted to write a non-zero value to IA32_XSS.
>
good point! Will change these description.
> > 
> > Signed-off-by: Zhang Yi Z <yi.z.zhang@linux.intel.com>
> > Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
> > ---
> >  arch/x86/kvm/vmx.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > index d32cee9ee079..68908ed7b151 100644
> > --- a/arch/x86/kvm/vmx.c
> > +++ b/arch/x86/kvm/vmx.c
> > @@ -47,6 +47,7 @@
> >  #include <asm/virtext.h>
> >  #include <asm/mce.h>
> >  #include <asm/fpu/internal.h>
> > +#include <asm/fpu/types.h>
> >  #include <asm/perf_event.h>
> >  #include <asm/debugreg.h>
> >  #include <asm/kexec.h>
> > @@ -4336,12 +4337,13 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> >  	case MSR_IA32_XSS:
> >  		if (!vmx_xsaves_supported())
> >  			return 1;
> > +
> >  		/*
> > -		 * The only supported bit as of Skylake is bit 8, but
> > -		 * it is not supported on KVM.
> > +		 * Check bits being set are supported in KVM.
> 
> I'd drop the comment altogether, it's pretty obvious from the code that
> were checking which bits are supported.
you won't see these redundancies in next version ;)
> >  		 */
> > -		if (data != 0)
> > +		if (data & ~kvm_supported_xss())
> >  			return 1;
> > +
> >  		vcpu->arch.ia32_xss = data;
> >  		if (vcpu->arch.ia32_xss != host_xss)
> >  			add_atomic_switch_msr(vmx, MSR_IA32_XSS,
> > -- 
> > 2.17.1
> >
Sean Christopherson Feb. 28, 2019, 4:25 p.m. UTC | #2 
On Mon, Feb 25, 2019 at 09:27:15PM +0800, Yang Weijiang wrote:
> For Guest XSS, right now, only bit 11(user states) and bit 12
> (supervisor states) are supported, if other bits are being set,
> need to modify KVM_SUPPORTED_XSS macro to have support.

The changelog should describe what the change does directly.  Referencing
specific bits implies that the code is explicitly checking for said bits,
which it does not.  And there's no need to advise readers on how to add
more bits in the future, e.g. the KVM_SUPPORTED_XSS macro may not exist
the next time new bits are added.  Just cover what the patch does and why.

Something like:

KVM: x86: Allow the guest to set supported bits in XSS

...now that KVM supports setting CET related bits.  Previously, KVM did
not support setting any bits in XSS and so hardcoded its check to inject
a #GP if the guest attempted to write a non-zero value to IA32_XSS.

> 
> Signed-off-by: Zhang Yi Z <yi.z.zhang@linux.intel.com>
> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
> ---
>  arch/x86/kvm/vmx.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index d32cee9ee079..68908ed7b151 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -47,6 +47,7 @@
>  #include <asm/virtext.h>
>  #include <asm/mce.h>
>  #include <asm/fpu/internal.h>
> +#include <asm/fpu/types.h>
>  #include <asm/perf_event.h>
>  #include <asm/debugreg.h>
>  #include <asm/kexec.h>
> @@ -4336,12 +4337,13 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  	case MSR_IA32_XSS:
>  		if (!vmx_xsaves_supported())
>  			return 1;
> +
>  		/*
> -		 * The only supported bit as of Skylake is bit 8, but
> -		 * it is not supported on KVM.
> +		 * Check bits being set are supported in KVM.

I'd drop the comment altogether, it's pretty obvious from the code that
were checking which bits are supported.

>  		 */
> -		if (data != 0)
> +		if (data & ~kvm_supported_xss())
>  			return 1;
> +
>  		vcpu->arch.ia32_xss = data;
>  		if (vcpu->arch.ia32_xss != host_xss)
>  			add_atomic_switch_msr(vmx, MSR_IA32_XSS,
> -- 
> 2.17.1
>
Paolo Bonzini March 8, 2019, 10:49 a.m. UTC | #3 
On 25/02/19 14:27, Yang Weijiang wrote:
> For Guest XSS, right now, only bit 11(user states) and bit 12
> (supervisor states) are supported, if other bits are being set,
> need to modify KVM_SUPPORTED_XSS macro to have support.
> 
> Signed-off-by: Zhang Yi Z <yi.z.zhang@linux.intel.com>
> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
> ---
>  arch/x86/kvm/vmx.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index d32cee9ee079..68908ed7b151 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -47,6 +47,7 @@
>  #include <asm/virtext.h>
>  #include <asm/mce.h>
>  #include <asm/fpu/internal.h>
> +#include <asm/fpu/types.h>
>  #include <asm/perf_event.h>
>  #include <asm/debugreg.h>
>  #include <asm/kexec.h>
> @@ -4336,12 +4337,13 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  	case MSR_IA32_XSS:
>  		if (!vmx_xsaves_supported())
>  			return 1;
> +
>  		/*
> -		 * The only supported bit as of Skylake is bit 8, but
> -		 * it is not supported on KVM.
> +		 * Check bits being set are supported in KVM.
>  		 */
> -		if (data != 0)
> +		if (data & ~kvm_supported_xss())
>  			return 1;
> +
>  		vcpu->arch.ia32_xss = data;
>  		if (vcpu->arch.ia32_xss != host_xss)
>  			add_atomic_switch_msr(vmx, MSR_IA32_XSS,
> 

Luwei, can you work with Weijiang to add XSAVES support for Processor
Tracing?

Paolo
Paolo Bonzini March 8, 2019, 11:32 a.m. UTC | #4 
On 28/02/19 09:44, Yang Weijiang wrote:
>>>  		if (!vmx_xsaves_supported())
>>>  			return 1;
>>> +
>>>  		/*
>>> -		 * The only supported bit as of Skylake is bit 8, but
>>> -		 * it is not supported on KVM.
>>> +		 * Check bits being set are supported in KVM.
>> I'd drop the comment altogether, it's pretty obvious from the code that
>> were checking which bits are supported.
> you won't see these redundancies in next version ;)
>>>  		 */
>>> -		if (data != 0)
>>> +		if (data & ~kvm_supported_xss())
>>>  			return 1;

You should instead check this against CPUID[0xD, 1].EDX:ECX.  If CET is
disabled in CPUID, the guest should not be able to set it in MSR_IA32_CSS.

Paolo
Yang Weijiang March 10, 2019, 12:20 p.m. UTC | #5 
On Fri, Mar 08, 2019 at 12:32:04PM +0100, Paolo Bonzini wrote:
> On 28/02/19 09:44, Yang Weijiang wrote:
> >>>  		if (!vmx_xsaves_supported())
> >>>  			return 1;
> >>> +
> >>>  		/*
> >>> -		 * The only supported bit as of Skylake is bit 8, but
> >>> -		 * it is not supported on KVM.
> >>> +		 * Check bits being set are supported in KVM.
> >> I'd drop the comment altogether, it's pretty obvious from the code that
> >> were checking which bits are supported.
> > you won't see these redundancies in next version ;)
> >>>  		 */
> >>> -		if (data != 0)
> >>> +		if (data & ~kvm_supported_xss())
> >>>  			return 1;
> 
> You should instead check this against CPUID[0xD, 1].EDX:ECX.  If CET is
> disabled in CPUID, the guest should not be able to set it in MSR_IA32_CSS.
> 
> Paolo
Thanks, OK, will change it.
Yang Weijiang March 10, 2019, 1:35 p.m. UTC | #6 
On Sun, Mar 10, 2019 at 08:20:30PM +0800, Yang Weijiang wrote:
> On Fri, Mar 08, 2019 at 12:32:04PM +0100, Paolo Bonzini wrote:
> > On 28/02/19 09:44, Yang Weijiang wrote:
> > >>>  		if (!vmx_xsaves_supported())
> > >>>  			return 1;
> > >>> +
> > >>>  		/*
> > >>> -		 * The only supported bit as of Skylake is bit 8, but
> > >>> -		 * it is not supported on KVM.
> > >>> +		 * Check bits being set are supported in KVM.
> > >> I'd drop the comment altogether, it's pretty obvious from the code that
> > >> were checking which bits are supported.
> > > you won't see these redundancies in next version ;)
> > >>>  		 */
> > >>> -		if (data != 0)
> > >>> +		if (data & ~kvm_supported_xss())
> > >>>  			return 1;
> > 
> > You should instead check this against CPUID[0xD, 1].EDX:ECX.  If CET is
> > disabled in CPUID, the guest should not be able to set it in MSR_IA32_CSS.
> > 
> > Paolo
> Thanks, OK, will change it.
Hi, Paolo,
How about change kvm_supported_xss() as below so that CPUID[0xd,1] check
is implied in host_xss contents, vmx_supported_xss() now just returns host_xss
in vmx.c. The purpose of this change is to make XSS data check
common for other XSS based features.

+u64 kvm_supported_xss(void)
+{
+       return KVM_SUPPORTED_XSS & kvm_x86_ops->vmx_supported_xss();
+}
+
Luwei Kang March 11, 2019, 1:29 a.m. UTC | #7 
> > For Guest XSS, right now, only bit 11(user states) and bit 12
> > (supervisor states) are supported, if other bits are being set, need
> > to modify KVM_SUPPORTED_XSS macro to have support.
> >
> > Signed-off-by: Zhang Yi Z <yi.z.zhang@linux.intel.com>
> > Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
> > ---
> >  arch/x86/kvm/vmx.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index
> > d32cee9ee079..68908ed7b151 100644
> > --- a/arch/x86/kvm/vmx.c
> > +++ b/arch/x86/kvm/vmx.c
> > @@ -47,6 +47,7 @@
> >  #include <asm/virtext.h>
> >  #include <asm/mce.h>
> >  #include <asm/fpu/internal.h>
> > +#include <asm/fpu/types.h>
> >  #include <asm/perf_event.h>
> >  #include <asm/debugreg.h>
> >  #include <asm/kexec.h>
> > @@ -4336,12 +4337,13 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> >  	case MSR_IA32_XSS:
> >  		if (!vmx_xsaves_supported())
> >  			return 1;
> > +
> >  		/*
> > -		 * The only supported bit as of Skylake is bit 8, but
> > -		 * it is not supported on KVM.
> > +		 * Check bits being set are supported in KVM.
> >  		 */
> > -		if (data != 0)
> > +		if (data & ~kvm_supported_xss())
> >  			return 1;
> > +
> >  		vcpu->arch.ia32_xss = data;
> >  		if (vcpu->arch.ia32_xss != host_xss)
> >  			add_atomic_switch_msr(vmx, MSR_IA32_XSS,
> >
> 
> Luwei, can you work with Weijiang to add XSAVES support for Processor Tracing?

Hi Paolo,
    As we talk long time before. I will do that.

Thanks,
Luwei Kang

> 
> Paolo
Paolo Bonzini March 11, 2019, 3:32 p.m. UTC | #8 
On 10/03/19 14:35, Yang Weijiang wrote:
>>>> -		if (data != 0)
>>>> +		if (data & ~kvm_supported_xss())
>>>>  			return 1;
>>> You should instead check this against CPUID[0xD, 1].EDX:ECX.  If CET is
>>> disabled in CPUID, the guest should not be able to set it in MSR_IA32_CSS.
>>>
>>> Paolo
>> Thanks, OK, will change it.
> Hi, Paolo,
> How about change kvm_supported_xss() as below so that CPUID[0xd,1] check
> is implied in host_xss contents, vmx_supported_xss() now just returns host_xss
> in vmx.c. The purpose of this change is to make XSS data check
> common for other XSS based features.
> 
> +u64 kvm_supported_xss(void)
> +{
> +       return KVM_SUPPORTED_XSS & kvm_x86_ops->vmx_supported_xss();
> +}
> +
> 

This is correct; however, you should also check against the *guest*'s
CPUID[0xD, 1].EDX:ECX.

One possibility is to add a field kvm->guest_supported_xss and update it
in kvm_update_cpuid, then here you do

	if (data & ~kvm->guest_supported_xss)
		return 1;

Thanks,

Paolo
Yang Weijiang March 12, 2019, 2:30 p.m. UTC | #9 
On Mon, Mar 11, 2019 at 04:32:32PM +0100, Paolo Bonzini wrote:
> On 10/03/19 14:35, Yang Weijiang wrote:
> >>>> -		if (data != 0)
> >>>> +		if (data & ~kvm_supported_xss())
> >>>>  			return 1;
> >>> You should instead check this against CPUID[0xD, 1].EDX:ECX.  If CET is
> >>> disabled in CPUID, the guest should not be able to set it in MSR_IA32_CSS.
> >>>
> >>> Paolo
> >> Thanks, OK, will change it.
> > Hi, Paolo,
> > How about change kvm_supported_xss() as below so that CPUID[0xd,1] check
> > is implied in host_xss contents, vmx_supported_xss() now just returns host_xss
> > in vmx.c. The purpose of this change is to make XSS data check
> > common for other XSS based features.
> > 
> > +u64 kvm_supported_xss(void)
> > +{
> > +       return KVM_SUPPORTED_XSS & kvm_x86_ops->vmx_supported_xss();
> > +}
> > +
> > 
> 
> This is correct; however, you should also check against the *guest*'s
> CPUID[0xD, 1].EDX:ECX.
> 
> One possibility is to add a field kvm->guest_supported_xss and update it
> in kvm_update_cpuid, then here you do
> 
> 	if (data & ~kvm->guest_supported_xss)
> 		return 1;
> 
> Thanks,
> 
> Paolo
Thanks Paolo, I'll add the change in next version.
diff mbox series

Patch

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index d32cee9ee079..68908ed7b151 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -47,6 +47,7 @@ 
 #include <asm/virtext.h>
 #include <asm/mce.h>
 #include <asm/fpu/internal.h>
+#include <asm/fpu/types.h>
 #include <asm/perf_event.h>
 #include <asm/debugreg.h>
 #include <asm/kexec.h>
@@ -4336,12 +4337,13 @@  static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case MSR_IA32_XSS:
 		if (!vmx_xsaves_supported())
 			return 1;
+
 		/*
-		 * The only supported bit as of Skylake is bit 8, but
-		 * it is not supported on KVM.
+		 * Check bits being set are supported in KVM.
 		 */
-		if (data != 0)
+		if (data & ~kvm_supported_xss())
 			return 1;
+
 		vcpu->arch.ia32_xss = data;
 		if (vcpu->arch.ia32_xss != host_xss)
 			add_atomic_switch_msr(vmx, MSR_IA32_XSS,