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@xxxxxxxxx> 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@xxxxxxxxxxxxxx/.
>
> 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
