Re: [PATCH v2] KVM: x86: Add dedicated helper to get CPUID entry with significant index

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 7/14/22 12:58, Maxim Levitsky wrote:
On Tue, 2022-07-12 at 17:09 +0000, Sean Christopherson wrote:
On Tue, Jul 12, 2022, Sean Christopherson wrote:
On Tue, Jul 12, 2022, Maxim Levitsky wrote:
On Tue, 2022-07-12 at 00:06 +0000, Sean Christopherson wrote:
+               /*
+                * Function matches and index is significant; not specifying an
+                * exact index in this case is a KVM bug.
+                */
Nitpick: Why KVM bug? Bad userspace can also provide a index-significant entry for cpuid
leaf for which index is not significant in the x86 spec.

Ugh, you're right.

We could arrange a table of all known leaves and for each leaf if it has an index
in the x86 spec, and warn/reject the userspace CPUID info if it doesn't match.

We have such a table, cpuid_function_is_indexed().  The alternative would be to
do:

                 WARN_ON_ONCE(index == KVM_CPUID_INDEX_NOT_SIGNIFICANT &&
                              cpuid_function_is_indexed(function));

The problem with rejecting userspace CPUID on mismatch is that it could break
userspace :-/  Of course, this entire patch would also break userspace to some
extent, e.g. if userspace is relying on an exact match on index==0.  The only
difference being the guest lookups with an exact index would still work.

I think the restriction we could put in place would be that userspace can make
a leaf more relaxed, e.g. to play nice if userspace forgets to set the SIGNFICANT
flag, but rejects attempts to make guest CPUID more restrictive, i.e. disallow
setting the SIGNFICANT flag on leafs that KVM doesn't enumerate as significant.

+               WARN_ON_ONCE(index == KVM_CPUID_INDEX_NOT_SIGNIFICANT);

Actually, better idea.  Let userspace do whatever, and have direct KVM lookups
for functions that architecturally don't have a significant index use the first
entry even if userspace set the SIGNIFICANT flag.  That will mostly maintain
backwards compatibility, the only thing that would break is if userspace set the
SIGNIFICANT flag _and_ provided a non-zero index _and_ relied on KVM to not match
the entry.

Makes sense as well.

Squashed and queued, thanks.

Regarding function and index, I never remember the "function" name, but it's pretty obvious that the index is ecx so there's no ambiguity. And using different names for inputs and outputs makes it clear that there are no games with in/out parameters.

Paolo
Paolo




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux