On Tue, Mar 23, 2021 at 10:37 AM <[email protected]> wrote:
>
> From: Haiwei Li <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
On Thu, Mar 25, 2021, Haiwei Li wrote:
> On Tue, Mar 23, 2021 at 10:37 AM <[email protected]> wrote:
> >
> > From: Haiwei Li <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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.
On Thu, Mar 25, 2021 at 11:49 PM Sean Christopherson <[email protected]> wrote:
>
> On Thu, Mar 25, 2021, Haiwei Li wrote:
> > On Tue, Mar 23, 2021 at 10:37 AM <[email protected]> wrote:
> > >
> > > From: Haiwei Li <[email protected]>
> > >
> > > 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 <[email protected]>
> > > ---
> > > 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