On Fri, Sep 30, 2022 at 4:59 PM Dong, Eddie <eddie.dong@xxxxxxxxx> 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@xxxxxxxxxx>
> > > > ---
> > > > 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@xxxxxxxxxxxxxx/.
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.
