[PATCH 1/6] KVM: x86: Mask off reserved bits in CPUID.80000001H

[PATCH 1/6] KVM: x86: Mask off reserved bits in CPUID.80000001H

[Jim Mattson]

KVM_GET_SUPPORTED_CPUID should only enumerate features that KVM
actually supports. CPUID.80000001:EBX[27:16] are reserved bits and
should be masked off.

Fixes: 0771671749b5 ("KVM: Enhance guest cpuid management")
Signed-off-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/kvm/cpuid.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 4c1c2c06e96b..ea4e213bcbfb 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -1119,6 +1119,7 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
 			entry->eax = max(entry->eax, 0x80000021);
 		break;
 	case 0x80000001:
+		entry->ebx &= ~GENMASK(27, 16);
 		cpuid_entry_override(entry, CPUID_8000_0001_EDX);
 		cpuid_entry_override(entry, CPUID_8000_0001_ECX);
 		break;
-- 
2.38.0.rc1.362.ged0d419d3c-goog

[PATCH 2/6] KVM: x86: Mask off reserved bits in CPUID.80000006H

[Jim Mattson]

KVM_GET_SUPPORTED_CPUID should only enumerate features that KVM
actually supports. CPUID.80000006H:EDX[17:16] are reserved bits and
should be masked off.

Fixes: 43d05de2bee7 ("KVM: pass through CPUID(0x80000006)")
Signed-off-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/kvm/cpuid.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index ea4e213bcbfb..90f9c295825d 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -1125,6 +1125,7 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
 		break;
 	case 0x80000006:
 		/* L2 cache and TLB: pass through host info. */
+		entry->edx &= ~GENMASK(17, 16);
 		break;
 	case 0x80000007: /* Advanced power management */
 		/* invariant TSC is CPUID.80000007H:EDX[8] */
-- 
2.38.0.rc1.362.ged0d419d3c-goog

RE: [PATCH 2/6] KVM: x86: Mask off reserved bits in CPUID.80000006H

[Dong, Eddie]

> -----Original Message-----
> From: Jim Mattson <jmattson@google.com>
> Sent: Thursday, September 29, 2022 3:52 PM
> To: kvm@vger.kernel.org; pbonzini@redhat.com; Christopherson,, Sean
> <seanjc@google.com>
> Cc: Jim Mattson <jmattson@google.com>
> Subject: [PATCH 2/6] KVM: x86: Mask off reserved bits in CPUID.80000006H
> 
> KVM_GET_SUPPORTED_CPUID should only enumerate features that KVM
> actually supports. CPUID.80000006H:EDX[17:16] are reserved bits and should
> be masked off.
> 
> Fixes: 43d05de2bee7 ("KVM: pass through CPUID(0x80000006)")
> Signed-off-by: Jim Mattson <jmattson@google.com>
> ---
>  arch/x86/kvm/cpuid.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index
> ea4e213bcbfb..90f9c295825d 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -1125,6 +1125,7 @@ static inline int __do_cpuid_func(struct
> kvm_cpuid_array *array, u32 function)
>  		break;
>  	case 0x80000006:
>  		/* L2 cache and TLB: pass through host info. */
> +		entry->edx &= ~GENMASK(17, 16);

SDM of Intel CPU says the edx is reserved=0.  I must miss something.

BTW, for those reserved bits, their meaning is not defined, and the VMM should not depend on them IMO.
What is the problem if hypervisor returns none-zero value?

Thanks Eddie

>  		break;
>  	case 0x80000007: /* Advanced power management */
>  		/* invariant TSC is CPUID.80000007H:EDX[8] */
> --
> 2.38.0.rc1.362.ged0d419d3c-goog

Re: [PATCH 2/6] KVM: x86: Mask off reserved bits in CPUID.80000006H

[Jim Mattson]

On Fri, Sep 30, 2022 at 2:21 PM Dong, Eddie <eddie.dong@intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Jim Mattson <jmattson@google.com>
> > Sent: Thursday, September 29, 2022 3:52 PM
> > To: kvm@vger.kernel.org; pbonzini@redhat.com; Christopherson,, Sean
> > <seanjc@google.com>
> > Cc: Jim Mattson <jmattson@google.com>
> > Subject: [PATCH 2/6] KVM: x86: Mask off reserved bits in CPUID.80000006H
> >
> > KVM_GET_SUPPORTED_CPUID should only enumerate features that KVM
> > actually supports. CPUID.80000006H:EDX[17:16] are reserved bits and should
> > be masked off.
> >
> > Fixes: 43d05de2bee7 ("KVM: pass through CPUID(0x80000006)")
> > Signed-off-by: Jim Mattson <jmattson@google.com>
> > ---
> >  arch/x86/kvm/cpuid.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index
> > ea4e213bcbfb..90f9c295825d 100644
> > --- a/arch/x86/kvm/cpuid.c
> > +++ b/arch/x86/kvm/cpuid.c
> > @@ -1125,6 +1125,7 @@ static inline int __do_cpuid_func(struct
> > kvm_cpuid_array *array, u32 function)
> >               break;
> >       case 0x80000006:
> >               /* L2 cache and TLB: pass through host info. */
> > +             entry->edx &= ~GENMASK(17, 16);
>
> SDM of Intel CPU says the edx is reserved=0.  I must miss something.

This is an AMD defined leaf. Therefore, the APM is authoritative.

> BTW, for those reserved bits, their meaning is not defined, and the VMM should not depend on them IMO.
> What is the problem if hypervisor returns none-zero value?

The problem arises if/when the bits become defined in the future, and
the functionality is not trivially virtualized.

> Thanks Eddie
>
> >               break;
> >       case 0x80000007: /* Advanced power management */
> >               /* invariant TSC is CPUID.80000007H:EDX[8] */
> > --
> > 2.38.0.rc1.362.ged0d419d3c-goog
>

RE: [PATCH 2/6] KVM: x86: Mask off reserved bits in CPUID.80000006H

[Dong, Eddie]

Hi Jim:

> > > KVM_GET_SUPPORTED_CPUID should only enumerate features that KVM
> > > actually supports. CPUID.80000006H:EDX[17:16] are reserved bits and
> > > should be masked off.
> > >
> > > Fixes: 43d05de2bee7 ("KVM: pass through CPUID(0x80000006)")
> > > Signed-off-by: Jim Mattson <jmattson@google.com>
> > > ---
> > >  arch/x86/kvm/cpuid.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index
> > > ea4e213bcbfb..90f9c295825d 100644
> > > --- a/arch/x86/kvm/cpuid.c
> > > +++ b/arch/x86/kvm/cpuid.c
> > > @@ -1125,6 +1125,7 @@ static inline int __do_cpuid_func(struct
> > > kvm_cpuid_array *array, u32 function)
> > >               break;
> > >       case 0x80000006:
> > >               /* L2 cache and TLB: pass through host info. */
> > > +             entry->edx &= ~GENMASK(17, 16);
> >
> > SDM of Intel CPU says the edx is reserved=0.  I must miss something.
> 
> This is an AMD defined leaf. Therefore, the APM is authoritative.

In this case, given this is the common place,  if we don't want to do conditionally for different X86 architecture (may be not necessary), can you put comments to clarify?
This way, readers won't be confused.


> 
> > BTW, for those reserved bits, their meaning is not defined, and the VMM
> should not depend on them IMO.
> > What is the problem if hypervisor returns none-zero value?
> 
> The problem arises if/when the bits become defined in the future, and the
> functionality is not trivially virtualized.

Assume the hardware defines the bit one day in future, if we are using old VMM, the VMM will view the hardware as if the feature doesn't exist, since VMM does not know the feature and view the bit as reserved. This case should work.

If we run with the future VMM, which recognizes and handles the bit/feature. The VMM could view the hardware feature to be disabled / not existed (if "1" is used to enable or stand for "having the capability"), or view the hardware feature as enabled/ existed (if "0" is used to enable or stand for "having the capability").  

In this case, whether we have this patch doesn’t give us definite answer. Am I right?


Thanks, Eddie

Re: [PATCH 2/6] KVM: x86: Mask off reserved bits in CPUID.80000006H

[Jim Mattson]

On Fri, Sep 30, 2022 at 4:59 PM Dong, Eddie <eddie.dong@intel.com> wrote:
>
> Hi Jim:
>
> > > > KVM_GET_SUPPORTED_CPUID should only enumerate features that KVM
> > > > actually supports. CPUID.80000006H:EDX[17:16] are reserved bits and
> > > > should be masked off.
> > > >
> > > > Fixes: 43d05de2bee7 ("KVM: pass through CPUID(0x80000006)")
> > > > Signed-off-by: Jim Mattson <jmattson@google.com>
> > > > ---
> > > >  arch/x86/kvm/cpuid.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index
> > > > ea4e213bcbfb..90f9c295825d 100644
> > > > --- a/arch/x86/kvm/cpuid.c
> > > > +++ b/arch/x86/kvm/cpuid.c
> > > > @@ -1125,6 +1125,7 @@ static inline int __do_cpuid_func(struct
> > > > kvm_cpuid_array *array, u32 function)
> > > >               break;
> > > >       case 0x80000006:
> > > >               /* L2 cache and TLB: pass through host info. */
> > > > +             entry->edx &= ~GENMASK(17, 16);
> > >
> > > SDM of Intel CPU says the edx is reserved=0.  I must miss something.
> >
> > This is an AMD defined leaf. Therefore, the APM is authoritative.
>
> In this case, given this is the common place,  if we don't want to do conditionally for different X86 architecture (may be not necessary), can you put comments to clarify?
> This way, readers won't be confused.

Will a single comment at the top of the function suffice?

>
> >
> > > BTW, for those reserved bits, their meaning is not defined, and the VMM
> > should not depend on them IMO.
> > > What is the problem if hypervisor returns none-zero value?
> >
> > The problem arises if/when the bits become defined in the future, and the
> > functionality is not trivially virtualized.
>
> Assume the hardware defines the bit one day in future, if we are using old VMM, the VMM will view the hardware as if the feature doesn't exist, since VMM does not know the feature and view the bit as reserved. This case should work.

The VMM should be able to simply pass the results of
KVM_GET_SUPPORTED_CPUID to KVM_SET_CPUID2, without masking off bits
that it doesn't know about.

Nonetheless, a future VMM that does know about the new hardware
feature can certainly expect that a KVM that enumerates the feature in
KVM_GET_SUPPORTED_CPUID has the ability to support the feature.

> If we run with the future VMM, which recognizes and handles the bit/feature. The VMM could view the hardware feature to be disabled / not existed (if "1" is used to enable or stand for "having the capability"), or view the hardware feature as enabled/ existed (if "0" is used to enable or stand for "having the capability").
>
> In this case, whether we have this patch doesn’t give us definite answer. Am I right?

Are you asking about the polarity of the CPUID bit?

It is unfortunate that both Intel and AMD have started to define
reverse polarity CPUID bits, because these do, in fact, cause a
forward compatibility issue. Take, for example, AMD's recent
introduction of CPUID.80000008H:EBX.EferLmsleUnsupported[bit 20]. When
the bit is set, EFER.LMSLE is not available. For quite some time, KVM
has been reporting that this feature *is* available on CPUs where it
isn't, because KVM cleared the CPUID bit based on its previous
'reserved' definition. I have a series out to address that issue:
https://lore.kernel.org/kvm/CALMp9eQ-qkjBm8qPhOaMzZLWeHJcrwksV+XLQ9DfOQ_i1aykqQ@mail.gmail.com/.

Intel did the same thing with
CPUID.(EAX=7H,ECX=0):EBX.FDP_EXCPTN_ONLY[bit 6] and
CPUID.(EAX=7H,ECX=0):EBX.ZERO_FCS_FDS[bit 13].

In the vast majority of cases, however, '0' is the right value to
report for a reserved bit to ensure forward compatibility.

RE: [PATCH 2/6] KVM: x86: Mask off reserved bits in CPUID.80000006H

[Dong, Eddie]

> >
> > In this case, given this is the common place,  if we don't want to do
> conditionally for different X86 architecture (may be not necessary), can you
> put comments to clarify?
> > This way, readers won't be confused.
> 
> Will a single comment at the top of the function suffice?

So, we are handling architecture-specific code in architecture-common path.
Not sure how it will impact others.

The idea solution will have to reshuffle it for different architecture to be processed differently.


> 
> >
> > >
> > > > BTW, for those reserved bits, their meaning is not defined, and
> > > > the VMM
> > > should not depend on them IMO.
> > > > What is the problem if hypervisor returns none-zero value?
> > >
> > > The problem arises if/when the bits become defined in the future,
> > > and the functionality is not trivially virtualized.
> >
> > Assume the hardware defines the bit one day in future, if we are using old
> VMM, the VMM will view the hardware as if the feature doesn't exist, since
> VMM does not know the feature and view the bit as reserved. This case should
> work.
> 
> The VMM should be able to simply pass the results of
> KVM_GET_SUPPORTED_CPUID to KVM_SET_CPUID2, without masking off bits
> that it doesn't know about.

At least in Intel architecture, a reserved bit (hardware return "1") reported by KVM_GET_SUPPORTED_CPUID can be set in KVM_SET_CPUID2. 
From KVM p.o.v., I think we don't assume the reserved bit (set by KVM_SET_CPUID2) must be 0.  Or I missed something?

> 
> Nonetheless, a future VMM that does know about the new hardware feature
> can certainly expect that a KVM that enumerates the feature in
> KVM_GET_SUPPORTED_CPUID has the ability to support the feature.

Taking native as example, a new application knows about the new hardware feature, should be able to run on old hardware without the feature, even if the old hardware report the reserved bit as "1".
This is usually guaranteed by the hardware vendor in specification design.  I will double check with Intel hardware guys to see if any "reserved bits" of CPUID may report "1".

VMM in this context should be able to follow the principle too.

> 
> > If we run with the future VMM, which recognizes and handles the
> bit/feature. The VMM could view the hardware feature to be disabled / not
> existed (if "1" is used to enable or stand for "having the capability"), or view
> the hardware feature as enabled/ existed (if "0" is used to enable or stand for
> "having the capability").
> >
> > In this case, whether we have this patch doesn’t give us definite answer. Am
> I right?
> 
> Are you asking about the polarity of the CPUID bit?
> 
> It is unfortunate that both Intel and AMD have started to define reverse
> polarity CPUID bits, because these do, in fact, cause a forward compatibility
> issue. Take, for example, AMD's recent introduction of
> CPUID.80000008H:EBX.EferLmsleUnsupported[bit 20]. When the bit is set,
> EFER.LMSLE is not available. For quite some time, KVM has been reporting
> that this feature *is* available on CPUs where it isn't, because KVM cleared
> the CPUID bit based on its previous 'reserved' definition. I have a series out to
> address that issue:
> https://lore.kernel.org/kvm/CALMp9eQ-
> qkjBm8qPhOaMzZLWeHJcrwksV+XLQ9DfOQ_i1aykqQ@mail.gmail.com/.

This case is different. It is because the new hardware converts a defined bit to be reserved.  What we are talking here is to convert a reserved bit to defined bit.

In this case, if one wants to remove the capability (if it is presented and run on old platform), to L1 VM. That is definitely fine.
But extending this to vastly cover all reserved bits of CPUID leaf may be too aggressive, and cause the future issues.

I will let you know if the reserved bits of CPUID in Intel architecture may report "1". If this is true, we may have to handle them differently for different architecture, i.e. Intel vs. AMD.

Will that make sense?

> 
> Intel did the same thing with
> CPUID.(EAX=7H,ECX=0):EBX.FDP_EXCPTN_ONLY[bit 6] and
> CPUID.(EAX=7H,ECX=0):EBX.ZERO_FCS_FDS[bit 13].
> 
> In the vast majority of cases, however, '0' is the right value to report for a
> reserved bit to ensure forward compatibility.

Thanks Eddie

Re: [PATCH 2/6] KVM: x86: Mask off reserved bits in CPUID.80000006H

[Jim Mattson]

Before we get even deeper into this discussion, let me point out that
the basic principle of reporting zero in KVM_GET_SUPPORTED_CPUID for
reserved bits is not something new I am introducing in this series. It
has been standard practice for a long time. See, for example, commit
404e0a19e155 ("KVM: cpuid: mask more bits in leaf 0xd and subleaves").
I am just fixing a few cases that have been overlooked.

On Mon, Oct 3, 2022 at 12:35 PM Dong, Eddie <eddie.dong@intel.com> wrote:
>
> > >
> > > In this case, given this is the common place,  if we don't want to do
> > conditionally for different X86 architecture (may be not necessary), can you
> > put comments to clarify?
> > > This way, readers won't be confused.
> >
> > Will a single comment at the top of the function suffice?
>
> So, we are handling architecture-specific code in architecture-common path.
> Not sure how it will impact others.
>
> The idea solution will have to reshuffle it for different architecture to be processed differently.

I hope we can expect that Intel and AMD will continue to respect each
other's definitions in the future, as they have done in the past.
>
> >
> > >
> > > >
> > > > > BTW, for those reserved bits, their meaning is not defined, and
> > > > > the VMM
> > > > should not depend on them IMO.
> > > > > What is the problem if hypervisor returns none-zero value?
> > > >
> > > > The problem arises if/when the bits become defined in the future,
> > > > and the functionality is not trivially virtualized.
> > >
> > > Assume the hardware defines the bit one day in future, if we are using old
> > VMM, the VMM will view the hardware as if the feature doesn't exist, since
> > VMM does not know the feature and view the bit as reserved. This case should
> > work.
> >
> > The VMM should be able to simply pass the results of
> > KVM_GET_SUPPORTED_CPUID to KVM_SET_CPUID2, without masking off bits
> > that it doesn't know about.
>
> At least in Intel architecture, a reserved bit (hardware return "1") reported by KVM_GET_SUPPORTED_CPUID can be set in KVM_SET_CPUID2.
> From KVM p.o.v., I think we don't assume the reserved bit (set by KVM_SET_CPUID2) must be 0.  Or I missed something?

Hardware reserved CPUID bits are always zero today, though that may
not be architecturally specified.

> >
> > Nonetheless, a future VMM that does know about the new hardware feature
> > can certainly expect that a KVM that enumerates the feature in
> > KVM_GET_SUPPORTED_CPUID has the ability to support the feature.
>
> Taking native as example, a new application knows about the new hardware feature, should be able to run on old hardware without the feature, even if the old hardware report the reserved bit as "1".
> This is usually guaranteed by the hardware vendor in specification design.  I will double check with Intel hardware guys to see if any "reserved bits" of CPUID may report "1".

If reserved bits can be reported as one by the hardware, then the
KVM_GET_SUPPORTED_CPUID API is completely useless as currently
defined. It would have to report both a mask of supported bits and the
values of those bits.

> VMM in this context should be able to follow the principle too.
>
> >
> > > If we run with the future VMM, which recognizes and handles the
> > bit/feature. The VMM could view the hardware feature to be disabled / not
> > existed (if "1" is used to enable or stand for "having the capability"), or view
> > the hardware feature as enabled/ existed (if "0" is used to enable or stand for
> > "having the capability").
> > >
> > > In this case, whether we have this patch doesn’t give us definite answer. Am
> > I right?
> >
> > Are you asking about the polarity of the CPUID bit?
> >
> > It is unfortunate that both Intel and AMD have started to define reverse
> > polarity CPUID bits, because these do, in fact, cause a forward compatibility
> > issue. Take, for example, AMD's recent introduction of
> > CPUID.80000008H:EBX.EferLmsleUnsupported[bit 20]. When the bit is set,
> > EFER.LMSLE is not available. For quite some time, KVM has been reporting
> > that this feature *is* available on CPUs where it isn't, because KVM cleared
> > the CPUID bit based on its previous 'reserved' definition. I have a series out to
> > address that issue:
> > https://lore.kernel.org/kvm/CALMp9eQ-
> > qkjBm8qPhOaMzZLWeHJcrwksV+XLQ9DfOQ_i1aykqQ@mail.gmail.com/.
>
> This case is different. It is because the new hardware converts a defined bit to be reserved.  What we are talking here is to convert a reserved bit to defined bit.

No. The new hardware defined a previously reserved *CPUID* bit. This
is exactly what we are talking about. The new CPUID bit just happens
to indicate that an EFER bit is now reserved, but that is neither here
nor there.

> In this case, if one wants to remove the capability (if it is presented and run on old platform), to L1 VM. That is definitely fine.
> But extending this to vastly cover all reserved bits of CPUID leaf may be too aggressive, and cause the future issues.
>
> I will let you know if the reserved bits of CPUID in Intel architecture may report "1". If this is true, we may have to handle them differently for different architecture, i.e. Intel vs. AMD.
>
> Will that make sense?

No, that doesn't make sense.

> >
> > Intel did the same thing with
> > CPUID.(EAX=7H,ECX=0):EBX.FDP_EXCPTN_ONLY[bit 6] and
> > CPUID.(EAX=7H,ECX=0):EBX.ZERO_FCS_FDS[bit 13].
> >
> > In the vast majority of cases, however, '0' is the right value to report for a
> > reserved bit to ensure forward compatibility.
>
> Thanks Eddie

RE: [PATCH 2/6] KVM: x86: Mask off reserved bits in CPUID.80000006H

[Dong, Eddie]

> Hardware reserved CPUID bits are always zero today, though that may not be
> architecturally specified.

entry->edx is initialized to native value in do_host_cpuid(), which executes physical CPUID. 
I guess I am disconnected here.

Re: [PATCH 2/6] KVM: x86: Mask off reserved bits in CPUID.80000006H

[Jim Mattson]

On Tue, Oct 4, 2022 at 5:08 PM Dong, Eddie <eddie.dong@intel.com> wrote:
>
> > Hardware reserved CPUID bits are always zero today, though that may not be
> > architecturally specified.
>
> entry->edx is initialized to native value in do_host_cpuid(), which executes physical CPUID.
> I guess I am disconnected here.

Hardware values should only be passed through for features that KVM
can support. Reserved bits should be set to 0, because KVM has no idea
whether or not it will be able to support them once they are defined.

Perhaps an example will help.

At one time, leaf 7 was completely reserved. Following the principle
that KVM should not pass through reserved CPUID bits, KVM zeroed out
this leaf prior to commit 611c120f7486 ("KVM: Mask function7 ebx
against host capability word9").
Suppose that the legacy KVM had, as you suggest, passed through the
hardware values for leaf 7. As CPUs appeared with SMEP, SMAP, Intel
Processor Trace, SGX, and a whole slew of other features, that version
of KVM would claim that it supported those features. Not true.

How would userspace be able to tell a version of KVM that could really
support SMEP from one that just blindly passed the bit through without
knowing what it meant? The KVM_GET_SUPPORTED_CPUID results would be
identical.

In some cases, if KVM claims to support a feature that it doesn't
(like SMEP), a guest that tries to use the feature will fail to boot
(e.g. setting CR4.SMEP will raise an unexpected #GP).

However, as you alluded to earlier, zeroing out reserved bits does not
always work out. Again, looking at leaf 7, the old KVM that clears all
of leaf 7 claims legacy x87 behavior with respect to the FPU data
pointer, FPU CS and FPU DS values, even on newer chips where that is
not true. This is because of the two "reverse polarity" feature bits
in leaf 7, where '0' indicates the presence of the feature and '1'
indicates that the feature has been removed. At least, in this case,
userspace can tell if KVM is wrong, just by querying CPUID leaf 7
itself. Long after leaf 7 support was added to KVM, it continued to
make the mistake of clearing those two bits. That bug wasn't addressed
until commit e3bcfda012ed ("KVM: x86: Report deprecated x87 features
in supported CPUID"). Fortunately, no software actually looks at those
two bits.

The KVM_GET_SUPPORTED_CPUID API is abysmal, but it is what we have for
now. The best thing we can do is to zero out reserved bits. Passing
through the hardware values is likely to get us into trouble in the
future, when those bits are defined to mean something that we don't
support.

RE: [PATCH 2/6] KVM: x86: Mask off reserved bits in CPUID.80000006H

[Dong, Eddie]

I see. Thanks Jim!
It makes sense.
Eddie

> -----Original Message-----
> From: Jim Mattson <jmattson@google.com>
> Sent: Tuesday, October 4, 2022 7:59 PM
> To: Dong, Eddie <eddie.dong@intel.com>
> Cc: kvm@vger.kernel.org; pbonzini@redhat.com; Christopherson,, Sean
> <seanjc@google.com>
> Subject: Re: [PATCH 2/6] KVM: x86: Mask off reserved bits in
> CPUID.80000006H
> 
> On Tue, Oct 4, 2022 at 5:08 PM Dong, Eddie <eddie.dong@intel.com> wrote:
> >
> > > Hardware reserved CPUID bits are always zero today, though that may
> > > not be architecturally specified.
> >
> > entry->edx is initialized to native value in do_host_cpuid(), which executes
> physical CPUID.
> > I guess I am disconnected here.
> 
> Hardware values should only be passed through for features that KVM can
> support. Reserved bits should be set to 0, because KVM has no idea whether
> or not it will be able to support them once they are defined.
> 
> Perhaps an example will help.
> 
> At one time, leaf 7 was completely reserved. Following the principle that KVM
> should not pass through reserved CPUID bits, KVM zeroed out this leaf prior to
> commit 611c120f7486 ("KVM: Mask function7 ebx against host capability
> word9").
> Suppose that the legacy KVM had, as you suggest, passed through the
> hardware values for leaf 7. As CPUs appeared with SMEP, SMAP, Intel
> Processor Trace, SGX, and a whole slew of other features, that version of KVM
> would claim that it supported those features. Not true.
> 
> How would userspace be able to tell a version of KVM that could really support
> SMEP from one that just blindly passed the bit through without knowing what
> it meant? The KVM_GET_SUPPORTED_CPUID results would be identical.
> 
> In some cases, if KVM claims to support a feature that it doesn't (like SMEP), a
> guest that tries to use the feature will fail to boot (e.g. setting CR4.SMEP will
> raise an unexpected #GP).
> 
> However, as you alluded to earlier, zeroing out reserved bits does not always
> work out. Again, looking at leaf 7, the old KVM that clears all of leaf 7 claims
> legacy x87 behavior with respect to the FPU data pointer, FPU CS and FPU DS
> values, even on newer chips where that is not true. This is because of the two
> "reverse polarity" feature bits in leaf 7, where '0' indicates the presence of the
> feature and '1'
> indicates that the feature has been removed. At least, in this case, userspace
> can tell if KVM is wrong, just by querying CPUID leaf 7 itself. Long after leaf 7
> support was added to KVM, it continued to make the mistake of clearing those
> two bits. That bug wasn't addressed until commit e3bcfda012ed ("KVM: x86:
> Report deprecated x87 features in supported CPUID"). Fortunately, no
> software actually looks at those two bits.
> 
> The KVM_GET_SUPPORTED_CPUID API is abysmal, but it is what we have for
> now. The best thing we can do is to zero out reserved bits. Passing through the
> hardware values is likely to get us into trouble in the future, when those bits
> are defined to mean something that we don't support.

Re: [PATCH 2/6] KVM: x86: Mask off reserved bits in CPUID.80000006H

[Sean Christopherson]

On Thu, Sep 29, 2022, Jim Mattson wrote:
> KVM_GET_SUPPORTED_CPUID should only enumerate features that KVM
> actually supports. CPUID.80000006H:EDX[17:16] are reserved bits and
> should be masked off.
> 
> Fixes: 43d05de2bee7 ("KVM: pass through CPUID(0x80000006)")
> Signed-off-by: Jim Mattson <jmattson@google.com>
> ---
>  arch/x86/kvm/cpuid.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index ea4e213bcbfb..90f9c295825d 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -1125,6 +1125,7 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
>  		break;
>  	case 0x80000006:
>  		/* L2 cache and TLB: pass through host info. */

Might be worth tweaking the comment, e.g. to

		/* L2 cache and TLB: advertise host info, sans reserved bits. */

> +		entry->edx &= ~GENMASK(17, 16);
>  		break;
>  	case 0x80000007: /* Advanced power management */
>  		/* invariant TSC is CPUID.80000007H:EDX[8] */
> -- 
> 2.38.0.rc1.362.ged0d419d3c-goog
> 

[PATCH 3/6] KVM: x86: Mask off reserved bits in CPUID.80000008H

[Jim Mattson]

KVM_GET_SUPPORTED_CPUID should only enumerate features that KVM
actually supports. The following ranges of CPUID.80000008H are reserved
and should be masked off:
    EDX[31:18]
    EDX[11:8]

Fixes: 24c82e576b78 ("KVM: Sanitize cpuid")
Signed-off-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/kvm/cpuid.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 90f9c295825d..15318f3f415e 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -1156,6 +1156,7 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
 
 		entry->eax = g_phys_as | (virt_as << 8);
 		entry->edx = 0;
+		entry->ecx &= ~(GENMASK(31, 18) | GENMASK(11, 8));
 		cpuid_entry_override(entry, CPUID_8000_0008_EBX);
 		break;
 	}
-- 
2.38.0.rc1.362.ged0d419d3c-goog

Re: [PATCH 3/6] KVM: x86: Mask off reserved bits in CPUID.80000008H

[Sean Christopherson]

On Thu, Sep 29, 2022, Jim Mattson wrote:
> KVM_GET_SUPPORTED_CPUID should only enumerate features that KVM
> actually supports. The following ranges of CPUID.80000008H are reserved
> and should be masked off:
>     EDX[31:18]
>     EDX[11:8]

Changelog says EDX, code and APM says ECX.

> Fixes: 24c82e576b78 ("KVM: Sanitize cpuid")
> Signed-off-by: Jim Mattson <jmattson@google.com>
> ---
>  arch/x86/kvm/cpuid.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 90f9c295825d..15318f3f415e 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -1156,6 +1156,7 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
>  
>  		entry->eax = g_phys_as | (virt_as << 8);
>  		entry->edx = 0;
> +		entry->ecx &= ~(GENMASK(31, 18) | GENMASK(11, 8));

Would it makes sense to also zero out the PerfTscSize bits?  KVM doesn't emulate
MSR_F15H_PTSC.

Uber nit, ECX comes before EDX in both alphabetical and register index order :-D

>  		cpuid_entry_override(entry, CPUID_8000_0008_EBX);
>  		break;
>  	}
> -- 
> 2.38.0.rc1.362.ged0d419d3c-goog
> 

[PATCH 4/6] KVM: x86: Mask off reserved bits in CPUID.8000001AH

[Jim Mattson]

KVM_GET_SUPPORTED_CPUID should only enumerate features that KVM
actually supports. In the case of CPUID.8000001AH, only three bits are
currently defined. The 125 reserved bits should be masked off.

Fixes: 24c82e576b78 ("KVM: Sanitize cpuid")
Signed-off-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/kvm/cpuid.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 15318f3f415e..5d1ec390aa45 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -1175,6 +1175,9 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
 		entry->ecx = entry->edx = 0;
 		break;
 	case 0x8000001a:
+		entry->eax &= GENMASK(2, 0);
+		entry->ebx = entry->ecx = entry->edx = 0;
+		break;
 	case 0x8000001e:
 		break;
 	case 0x8000001F:
-- 
2.38.0.rc1.362.ged0d419d3c-goog

[PATCH 5/6] KVM: x86: Mask off reserved bits in CPUID.8000001EH

[Jim Mattson]

KVM_GET_SUPPORTED_CPUID should only enumerate features that KVM
actually supports. The following ranges of CPUID.8000001EH are reserved
and should be masked off:
    EBX[31:16]
    ECX[31:11]
    EDX[31:0]

Fixes: 382409b4c43e ("kvm: x86: Include CPUID leaf 0x8000001e in kvm's supported CPUID")
Signed-off-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/kvm/cpuid.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 5d1ec390aa45..576cbcf489ce 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -1179,6 +1179,9 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
 		entry->ebx = entry->ecx = entry->edx = 0;
 		break;
 	case 0x8000001e:
+		entry->ebx &= ~GENMASK(31, 16);
+		entry->ecx &= ~GENMASK(31, 11);
+		entry->edx = 0;
 		break;
 	case 0x8000001F:
 		if (!kvm_cpu_cap_has(X86_FEATURE_SEV)) {
-- 
2.38.0.rc1.362.ged0d419d3c-goog

Re: [PATCH 5/6] KVM: x86: Mask off reserved bits in CPUID.8000001EH

[Sean Christopherson]

On Thu, Sep 29, 2022, Jim Mattson wrote:
> KVM_GET_SUPPORTED_CPUID should only enumerate features that KVM
> actually supports. The following ranges of CPUID.8000001EH are reserved
> and should be masked off:
>     EBX[31:16]
>     ECX[31:11]

LOL, APM is buggy, it says all bits in ECX are reserved.

  31:0  -                Reserved.
  10:8 NodesPerProcessor
  7:0  NodeId

Advertising NodeId seems all kinds of wrong :-(

>     EDX[31:0]
> 
> Fixes: 382409b4c43e ("kvm: x86: Include CPUID leaf 0x8000001e in kvm's supported CPUID")
> Signed-off-by: Jim Mattson <jmattson@google.com>
> ---
>  arch/x86/kvm/cpuid.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 5d1ec390aa45..576cbcf489ce 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -1179,6 +1179,9 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
>  		entry->ebx = entry->ecx = entry->edx = 0;
>  		break;
>  	case 0x8000001e:
> +		entry->ebx &= ~GENMASK(31, 16);
> +		entry->ecx &= ~GENMASK(31, 11);
> +		entry->edx = 0;
>  		break;
>  	case 0x8000001F:
>  		if (!kvm_cpu_cap_has(X86_FEATURE_SEV)) {
> -- 
> 2.38.0.rc1.362.ged0d419d3c-goog
> 

Re: [PATCH 5/6] KVM: x86: Mask off reserved bits in CPUID.8000001EH

[Paolo Bonzini]

On 10/20/22 00:53, Sean Christopherson wrote:
> On Thu, Sep 29, 2022, Jim Mattson wrote:
>> KVM_GET_SUPPORTED_CPUID should only enumerate features that KVM
>> actually supports. The following ranges of CPUID.8000001EH are reserved
>> and should be masked off:
>>      EBX[31:16]
>>      ECX[31:11]
> LOL, APM is buggy, it says all bits in ECX are reserved.
> 
>    31:0  -                Reserved.
>    10:8 NodesPerProcessor
>    7:0  NodeId
> 
> Advertising NodeId seems all kinds of wrong 🙁

Yeah I don't think there is any sensible way to pass this down via 
KVM_GET_SUPPORTED_CPUID.  Making it all zeros is the only way, userspace 
can always compute it on its own based on the topology that it wants the 
guest to see.  I'll send a separate patch.

Paolo

[PATCH 6/6] KVM: x86: Mask off reserved bits in CPUID.8000001FH

[Jim Mattson]

KVM_GET_SUPPORTED_CPUID should only enumerate features that KVM
actually supports. CPUID.8000001FH:EBX[31:16] are reserved bits and
should be masked off.

Fixes: 8765d75329a3 ("KVM: X86: Extend CPUID range to include new leaf")
Signed-off-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/kvm/cpuid.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 576cbcf489ce..58dabc9e54db 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -1188,7 +1188,7 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
 			entry->eax = entry->ebx = entry->ecx = entry->edx = 0;
 		} else {
 			cpuid_entry_override(entry, CPUID_8000_001F_EAX);
-
+			entry->ebx &= ~GENMASK(31, 16);
 			/*
 			 * Enumerate '0' for "PA bits reduction", the adjusted
 			 * MAXPHYADDR is enumerated directly (see 0x80000008).
-- 
2.38.0.rc1.362.ged0d419d3c-goog

Re: [PATCH 6/6] KVM: x86: Mask off reserved bits in CPUID.8000001FH

[Paolo Bonzini]

On 9/30/22 00:52, Jim Mattson wrote:
> KVM_GET_SUPPORTED_CPUID should only enumerate features that KVM
> actually supports. CPUID.8000001FH:EBX[31:16] are reserved bits and
> should be masked off.
> 
> Fixes: 8765d75329a3 ("KVM: X86: Extend CPUID range to include new leaf")
> Signed-off-by: Jim Mattson <jmattson@google.com>
> ---
>   arch/x86/kvm/cpuid.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 576cbcf489ce..58dabc9e54db 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -1188,7 +1188,7 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
>   			entry->eax = entry->ebx = entry->ecx = entry->edx = 0;
>   		} else {
>   			cpuid_entry_override(entry, CPUID_8000_001F_EAX);
> -
> +			entry->ebx &= ~GENMASK(31, 16);
>   			/*
>   			 * Enumerate '0' for "PA bits reduction", the adjusted
>   			 * MAXPHYADDR is enumerated directly (see 0x80000008).

I think 15:12 (number of VMPLs supported) should also be masked off 
since KVM does not support SEV-SNP.

Paolo

RE: [PATCH 1/6] KVM: x86: Mask off reserved bits in CPUID.80000001H

[Dong, Eddie]

> -----Original Message-----
> From: Jim Mattson <jmattson@google.com>
> Sent: Thursday, September 29, 2022 3:52 PM
> To: kvm@vger.kernel.org; pbonzini@redhat.com; Christopherson,, Sean
> <seanjc@google.com>
> Cc: Jim Mattson <jmattson@google.com>
> Subject: [PATCH 1/6] KVM: x86: Mask off reserved bits in CPUID.80000001H
> 
> KVM_GET_SUPPORTED_CPUID should only enumerate features that KVM
> actually supports. CPUID.80000001:EBX[27:16] are reserved bits and should
> be masked off.
> 
> Fixes: 0771671749b5 ("KVM: Enhance guest cpuid management")
> Signed-off-by: Jim Mattson <jmattson@google.com>
> ---
>  arch/x86/kvm/cpuid.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index
> 4c1c2c06e96b..ea4e213bcbfb 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -1119,6 +1119,7 @@ static inline int __do_cpuid_func(struct
> kvm_cpuid_array *array, u32 function)
>  			entry->eax = max(entry->eax, 0x80000021);
>  		break;
>  	case 0x80000001:
> +		entry->ebx &= ~GENMASK(27, 16);

ebx of leaf 0x80000001 is reserved, at least from SDM of Intel processor. Do I miss something?

>  		cpuid_entry_override(entry, CPUID_8000_0001_EDX);
>  		cpuid_entry_override(entry, CPUID_8000_0001_ECX);
>  		break;
> --
> 2.38.0.rc1.362.ged0d419d3c-goog

Re: [PATCH 1/6] KVM: x86: Mask off reserved bits in CPUID.80000001H

[Jim Mattson]

On Fri, Sep 30, 2022 at 2:04 PM Dong, Eddie <eddie.dong@intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Jim Mattson <jmattson@google.com>
> > Sent: Thursday, September 29, 2022 3:52 PM
> > To: kvm@vger.kernel.org; pbonzini@redhat.com; Christopherson,, Sean
> > <seanjc@google.com>
> > Cc: Jim Mattson <jmattson@google.com>
> > Subject: [PATCH 1/6] KVM: x86: Mask off reserved bits in CPUID.80000001H
> >
> > KVM_GET_SUPPORTED_CPUID should only enumerate features that KVM
> > actually supports. CPUID.80000001:EBX[27:16] are reserved bits and should
> > be masked off.
> >
> > Fixes: 0771671749b5 ("KVM: Enhance guest cpuid management")
> > Signed-off-by: Jim Mattson <jmattson@google.com>
> > ---
> >  arch/x86/kvm/cpuid.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index
> > 4c1c2c06e96b..ea4e213bcbfb 100644
> > --- a/arch/x86/kvm/cpuid.c
> > +++ b/arch/x86/kvm/cpuid.c
> > @@ -1119,6 +1119,7 @@ static inline int __do_cpuid_func(struct
> > kvm_cpuid_array *array, u32 function)
> >                       entry->eax = max(entry->eax, 0x80000021);
> >               break;
> >       case 0x80000001:
> > +             entry->ebx &= ~GENMASK(27, 16);
>
> ebx of leaf 0x80000001 is reserved, at least from SDM of Intel processor. Do I miss something?

This is an AMD defined leaf, so the APM is authoritative.

> >               cpuid_entry_override(entry, CPUID_8000_0001_EDX);
> >               cpuid_entry_override(entry, CPUID_8000_0001_ECX);
> >               break;
> > --
> > 2.38.0.rc1.362.ged0d419d3c-goog
>

Re: [PATCH 1/6] KVM: x86: Mask off reserved bits in CPUID.80000001H

[Sean Christopherson]

On Thu, Sep 29, 2022, Jim Mattson wrote:
> KVM_GET_SUPPORTED_CPUID should only enumerate features that KVM
> actually supports. CPUID.80000001:EBX[27:16] are reserved bits and
> should be masked off.
> 
> Fixes: 0771671749b5 ("KVM: Enhance guest cpuid management")
> Signed-off-by: Jim Mattson <jmattson@google.com>
> ---

Please provide cover letters for multi-patch series, at the very least it provides
a convenient location to provide feedback to the series as a whole.

As for the patches, some nits, but nothing that can't be handled when applying,
i.e. no need to send v2 at this time.

Re: [PATCH 1/6] KVM: x86: Mask off reserved bits in CPUID.80000001H

[Paolo Bonzini]

On 10/20/22 00:58, Sean Christopherson wrote:
> On Thu, Sep 29, 2022, Jim Mattson wrote:
>> KVM_GET_SUPPORTED_CPUID should only enumerate features that KVM
>> actually supports. CPUID.80000001:EBX[27:16] are reserved bits and
>> should be masked off.
>>
>> Fixes: 0771671749b5 ("KVM: Enhance guest cpuid management")
>> Signed-off-by: Jim Mattson <jmattson@google.com>
>> ---
> 
> Please provide cover letters for multi-patch series, at the very least it provides
> a convenient location to provide feedback to the series as a whole.
> 
> As for the patches, some nits, but nothing that can't be handled when applying,
> i.e. no need to send v2 at this time.

Indeed - queued all except patch 5, thanks.

Paolo