linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] KVM: VMX: Check the corresponding bits according to the intel sdm
@ 2021-03-23  2:37 lihaiwei.kernel
  2021-03-23  3:16 ` Jim Mattson
  2021-03-25 13:22 ` Haiwei Li
  0 siblings, 2 replies; 6+ messages in thread
From: lihaiwei.kernel @ 2021-03-23  2:37 UTC (permalink / raw)
  To: linux-kernel, kvm
  Cc: pbonzini, seanjc, vkuznets, wanpengli, jmattson, joro, Haiwei Li

From: Haiwei Li <lihaiwei@tencent.com>

According to IA-32 SDM Vol.3D "A.1 BASIC VMX INFORMATION", two inspections
are missing.
* Bit 31 is always 0. Earlier versions of this manual specified that the
VMCS revision identifier was a 32-bit field in bits 31:0 of this MSR. For
all processors produced prior to this change, bit 31 of this MSR was read
as 0.
* The values of bits 47:45 and bits 63:57 are reserved and are read as 0.

Signed-off-by: Haiwei Li <lihaiwei@tencent.com>
---
 arch/x86/kvm/vmx/vmx.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 32cf828..0d6d13c 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -2577,6 +2577,20 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf,
 
 	rdmsr(MSR_IA32_VMX_BASIC, vmx_msr_low, vmx_msr_high);
 
+	/*
+	 * IA-32 SDM Vol 3D: Bit 31 is always 0.
+	 * For all earlier processors, bit 31 of this MSR was read as 0.
+	 */
+	if (vmx_msr_low & (1u<<31))
+		return -EIO;
+
+	/*
+	 * IA-32 SDM Vol 3D: bits 47:45 and bits 63:57 are reserved and are read
+	 * as 0.
+	 */
+	if (vmx_msr_high & 0xfe00e000)
+		return -EIO;
+
 	/* IA-32 SDM Vol 3B: VMCS size is never greater than 4kB. */
 	if ((vmx_msr_high & 0x1fff) > PAGE_SIZE)
 		return -EIO;
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] KVM: VMX: Check the corresponding bits according to the intel sdm
  2021-03-23  2:37 [PATCH] KVM: VMX: Check the corresponding bits according to the intel sdm lihaiwei.kernel
@ 2021-03-23  3:16 ` Jim Mattson
  2021-03-23  4:42   ` Haiwei Li
  2021-03-25 13:22 ` Haiwei Li
  1 sibling, 1 reply; 6+ messages in thread
From: Jim Mattson @ 2021-03-23  3:16 UTC (permalink / raw)
  To: Haiwei Li
  Cc: LKML, kvm list, Paolo Bonzini, Sean Christopherson,
	Vitaly Kuznetsov, Wanpeng Li, Joerg Roedel, Haiwei Li

On Mon, Mar 22, 2021 at 7:37 PM <lihaiwei.kernel@gmail.com> wrote:
>
> From: Haiwei Li <lihaiwei@tencent.com>
>
> According to IA-32 SDM Vol.3D "A.1 BASIC VMX INFORMATION", two inspections
> are missing.
> * Bit 31 is always 0. Earlier versions of this manual specified that the
> VMCS revision identifier was a 32-bit field in bits 31:0 of this MSR. For
> all processors produced prior to this change, bit 31 of this MSR was read
> as 0.

For all *Intel* processors produced prior to this change, bit 31 of
this MSR may have been 0. However, a conforming hypervisor may have
selected a full 32-bit VMCS revision identifier with the high bit set
for nested VMX. Furthermore, there are other vendors, such as VIA,
which have implemented the VMX extensions, and they, too, may have
selected a full 32-bit VMCS revision identifier with the high bit set.
Intel should know better than to change the documentation after the
horse is out of the barn.

What, exactly, is the value you are adding with this check?

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] KVM: VMX: Check the corresponding bits according to the intel sdm
  2021-03-23  3:16 ` Jim Mattson
@ 2021-03-23  4:42   ` Haiwei Li
  0 siblings, 0 replies; 6+ messages in thread
From: Haiwei Li @ 2021-03-23  4:42 UTC (permalink / raw)
  To: Jim Mattson
  Cc: LKML, kvm list, Paolo Bonzini, Sean Christopherson,
	Vitaly Kuznetsov, Wanpeng Li, Joerg Roedel, Haiwei Li

On Tue, Mar 23, 2021 at 11:16 AM Jim Mattson <jmattson@google.com> wrote:
>
> On Mon, Mar 22, 2021 at 7:37 PM <lihaiwei.kernel@gmail.com> wrote:
> >
> > From: Haiwei Li <lihaiwei@tencent.com>
> >
> > According to IA-32 SDM Vol.3D "A.1 BASIC VMX INFORMATION", two inspections
> > are missing.
> > * Bit 31 is always 0. Earlier versions of this manual specified that the
> > VMCS revision identifier was a 32-bit field in bits 31:0 of this MSR. For
> > all processors produced prior to this change, bit 31 of this MSR was read
> > as 0.
>
> For all *Intel* processors produced prior to this change, bit 31 of
> this MSR may have been 0. However, a conforming hypervisor may have
> selected a full 32-bit VMCS revision identifier with the high bit set
> for nested VMX. Furthermore, there are other vendors, such as VIA,
> which have implemented the VMX extensions, and they, too, may have
> selected a full 32-bit VMCS revision identifier with the high bit set.
> Intel should know better than to change the documentation after the
> horse is out of the barn.

Got it, thanks.

>
> What, exactly, is the value you are adding with this check?

I did this just to match the sdm.

--
Haiwei Li

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] KVM: VMX: Check the corresponding bits according to the intel sdm
  2021-03-23  2:37 [PATCH] KVM: VMX: Check the corresponding bits according to the intel sdm lihaiwei.kernel
  2021-03-23  3:16 ` Jim Mattson
@ 2021-03-25 13:22 ` Haiwei Li
  2021-03-25 15:49   ` Sean Christopherson
  1 sibling, 1 reply; 6+ messages in thread
From: Haiwei Li @ 2021-03-25 13:22 UTC (permalink / raw)
  To: LKML, kvm list
  Cc: Paolo Bonzini, Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li,
	Jim Mattson, Joerg Roedel, Haiwei Li

On Tue, Mar 23, 2021 at 10:37 AM <lihaiwei.kernel@gmail.com> wrote:
>
> From: Haiwei Li <lihaiwei@tencent.com>
>
> According to IA-32 SDM Vol.3D "A.1 BASIC VMX INFORMATION", two inspections
> are missing.
> * Bit 31 is always 0. Earlier versions of this manual specified that the
> VMCS revision identifier was a 32-bit field in bits 31:0 of this MSR. For
> all processors produced prior to this change, bit 31 of this MSR was read
> as 0.
> * The values of bits 47:45 and bits 63:57 are reserved and are read as 0.
>
> Signed-off-by: Haiwei Li <lihaiwei@tencent.com>
> ---
>  arch/x86/kvm/vmx/vmx.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 32cf828..0d6d13c 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -2577,6 +2577,20 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf,
>
>         rdmsr(MSR_IA32_VMX_BASIC, vmx_msr_low, vmx_msr_high);
>
> +       /*
> +        * IA-32 SDM Vol 3D: Bit 31 is always 0.
> +        * For all earlier processors, bit 31 of this MSR was read as 0.
> +        */
> +       if (vmx_msr_low & (1u<<31))
> +               return -EIO;

Drop this code as Jim said.

> +
> +       /*
> +        * IA-32 SDM Vol 3D: bits 47:45 and bits 63:57 are reserved and are read
> +        * as 0.
> +        */
> +       if (vmx_msr_high & 0xfe00e000)
> +               return -EIO;

Is this ok? Can we pick up the part? :)

--
Haiwei Li

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] KVM: VMX: Check the corresponding bits according to the intel sdm
  2021-03-25 13:22 ` Haiwei Li
@ 2021-03-25 15:49   ` Sean Christopherson
  2021-03-26  0:39     ` Haiwei Li
  0 siblings, 1 reply; 6+ messages in thread
From: Sean Christopherson @ 2021-03-25 15:49 UTC (permalink / raw)
  To: Haiwei Li
  Cc: LKML, kvm list, Paolo Bonzini, Vitaly Kuznetsov, Wanpeng Li,
	Jim Mattson, Joerg Roedel, Haiwei Li

On Thu, Mar 25, 2021, Haiwei Li wrote:
> On Tue, Mar 23, 2021 at 10:37 AM <lihaiwei.kernel@gmail.com> wrote:
> >
> > From: Haiwei Li <lihaiwei@tencent.com>
> >
> > According to IA-32 SDM Vol.3D "A.1 BASIC VMX INFORMATION", two inspections
> > are missing.
> > * Bit 31 is always 0. Earlier versions of this manual specified that the
> > VMCS revision identifier was a 32-bit field in bits 31:0 of this MSR. For
> > all processors produced prior to this change, bit 31 of this MSR was read
> > as 0.
> > * The values of bits 47:45 and bits 63:57 are reserved and are read as 0.
> >
> > Signed-off-by: Haiwei Li <lihaiwei@tencent.com>
> > ---
> >  arch/x86/kvm/vmx/vmx.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > index 32cf828..0d6d13c 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -2577,6 +2577,20 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf,
> >
> >         rdmsr(MSR_IA32_VMX_BASIC, vmx_msr_low, vmx_msr_high);
> >
> > +       /*
> > +        * IA-32 SDM Vol 3D: Bit 31 is always 0.
> > +        * For all earlier processors, bit 31 of this MSR was read as 0.
> > +        */
> > +       if (vmx_msr_low & (1u<<31))
> > +               return -EIO;
> 
> Drop this code as Jim said.
> 
> > +
> > +       /*
> > +        * IA-32 SDM Vol 3D: bits 47:45 and bits 63:57 are reserved and are read
> > +        * as 0.
> > +        */
> > +       if (vmx_msr_high & 0xfe00e000)
> > +               return -EIO;
> 
> Is this ok? Can we pick up the part? :)

No.  "Reserved and are read as 0" does not guarantee the bits will always be
reserved.  There are very few bits used for feature enumeration in x86 that are
guaranteed to be '0' for all eternity.

The whole point of reserving bits in registers is so that the CPU vendor, Intel
in this case, can introduce new features and enumerate them to software without
colliding with existing features or breaking software.  E.g. if Intel adds a new
feature and uses any of these bits to enumerate the feature, this check would
prevent KVM from loading on CPUs that support the feature.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] KVM: VMX: Check the corresponding bits according to the intel sdm
  2021-03-25 15:49   ` Sean Christopherson
@ 2021-03-26  0:39     ` Haiwei Li
  0 siblings, 0 replies; 6+ messages in thread
From: Haiwei Li @ 2021-03-26  0:39 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: LKML, kvm list, Paolo Bonzini, Vitaly Kuznetsov, Wanpeng Li,
	Jim Mattson, Joerg Roedel, Haiwei Li

On Thu, Mar 25, 2021 at 11:49 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Thu, Mar 25, 2021, Haiwei Li wrote:
> > On Tue, Mar 23, 2021 at 10:37 AM <lihaiwei.kernel@gmail.com> wrote:
> > >
> > > From: Haiwei Li <lihaiwei@tencent.com>
> > >
> > > According to IA-32 SDM Vol.3D "A.1 BASIC VMX INFORMATION", two inspections
> > > are missing.
> > > * Bit 31 is always 0. Earlier versions of this manual specified that the
> > > VMCS revision identifier was a 32-bit field in bits 31:0 of this MSR. For
> > > all processors produced prior to this change, bit 31 of this MSR was read
> > > as 0.
> > > * The values of bits 47:45 and bits 63:57 are reserved and are read as 0.
> > >
> > > Signed-off-by: Haiwei Li <lihaiwei@tencent.com>
> > > ---
> > >  arch/x86/kvm/vmx/vmx.c | 14 ++++++++++++++
> > >  1 file changed, 14 insertions(+)
> > >
> > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > > index 32cf828..0d6d13c 100644
> > > --- a/arch/x86/kvm/vmx/vmx.c
> > > +++ b/arch/x86/kvm/vmx/vmx.c
> > > @@ -2577,6 +2577,20 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf,
> > >
> > >         rdmsr(MSR_IA32_VMX_BASIC, vmx_msr_low, vmx_msr_high);
> > >
> > > +       /*
> > > +        * IA-32 SDM Vol 3D: Bit 31 is always 0.
> > > +        * For all earlier processors, bit 31 of this MSR was read as 0.
> > > +        */
> > > +       if (vmx_msr_low & (1u<<31))
> > > +               return -EIO;
> >
> > Drop this code as Jim said.
> >
> > > +
> > > +       /*
> > > +        * IA-32 SDM Vol 3D: bits 47:45 and bits 63:57 are reserved and are read
> > > +        * as 0.
> > > +        */
> > > +       if (vmx_msr_high & 0xfe00e000)
> > > +               return -EIO;
> >
> > Is this ok? Can we pick up the part? :)
>
> No.  "Reserved and are read as 0" does not guarantee the bits will always be
> reserved.  There are very few bits used for feature enumeration in x86 that are
> guaranteed to be '0' for all eternity.
>
> The whole point of reserving bits in registers is so that the CPU vendor, Intel
> in this case, can introduce new features and enumerate them to software without
> colliding with existing features or breaking software.  E.g. if Intel adds a new
> feature and uses any of these bits to enumerate the feature, this check would
> prevent KVM from loading on CPUs that support the feature.

Got it, only explicit restrictions should be checked. Thanks.

--
Haiwei Li

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-03-26  0:41 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-23  2:37 [PATCH] KVM: VMX: Check the corresponding bits according to the intel sdm lihaiwei.kernel
2021-03-23  3:16 ` Jim Mattson
2021-03-23  4:42   ` Haiwei Li
2021-03-25 13:22 ` Haiwei Li
2021-03-25 15:49   ` Sean Christopherson
2021-03-26  0:39     ` Haiwei Li

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).