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.
>
> > 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
