Round two of trying to wrangle kvm_cpuid() into submission. Two more bug
fixes, both related to querying for an "AMD" vendor, in addition to the
fixes in v1 (tracing and Hypervisor/Centaur range checks).
In theory, everything up to the refactoring is non-controversial, i.e. we
can bikeshed the refactoring without delaying the bug fixes.
v2:
- Use Jan's patch to fix the trace bug. [Everyone]
- Rework Hypervisor/Centaur handling so that only the Hypervisor
sub-ranges get the restrictive 0xffffff00 mask, and so that Centaur's
range only gets recognized when the guest vendor is Centaur. [Jim]
- Add the aforementioned bug fixes.
- Add a patch to do build time assertions on the vendor string, which
are hand coded u32s in the emulator (for direct comparison against
CPUID register output).
- Drop the patch to add CPUID.maxphyaddr emulator helper. [Paolo]
- Redo refactoring patches to land them after all the bug fixes
and to do the refactoring without any semantic changes in the
emulator.
Jan Kiszka (1):
KVM: x86: Trace the original requested CPUID function in kvm_cpuid()
Sean Christopherson (6):
KVM: x86: Add helpers to perform CPUID-based guest vendor check
KVM x86: Extend AMD specific guest behavior to Hygon virtual CPUs
KVM: x86: Fix CPUID range checks for Hypervisor and Centaur classes
KVM: x86: Add build-time assertions on validity of vendor strings
KVM: x86: Refactor out-of-range logic to contain the madness
KVM: x86: Refactor kvm_cpuid() param that controls out-of-range logic
arch/x86/include/asm/kvm_emulate.h | 37 +++++++++-
arch/x86/kvm/cpuid.c | 111 +++++++++++++++++++++--------
arch/x86/kvm/cpuid.h | 8 ++-
arch/x86/kvm/emulate.c | 64 ++++++++---------
arch/x86/kvm/mmu/mmu.c | 3 +-
arch/x86/kvm/svm.c | 2 +-
arch/x86/kvm/x86.c | 7 +-
7 files changed, 162 insertions(+), 70 deletions(-)
--
2.24.1
Extend guest_cpuid_is_amd() to cover Hygon virtual CPUs and rename it
accordingly. Hygon CPUs use an AMD-based core and so have the same
basic behavior as AMD CPUs.
Fixes: b8f4abb652146 ("x86/kvm: Add Hygon Dhyana support to KVM")
Cc: Pu Wen <[email protected]>
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/kvm/cpuid.c | 2 +-
arch/x86/kvm/cpuid.h | 6 ++++--
arch/x86/kvm/mmu/mmu.c | 3 ++-
arch/x86/kvm/x86.c | 2 +-
4 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index b4beb3707d1b..5a9891cb2bc6 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -1013,7 +1013,7 @@ bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
* requested. AMD CPUID semantics returns all zeroes for any
* undefined leaf, whether or not the leaf is in range.
*/
- if (!entry && check_limit && !guest_cpuid_is_amd(vcpu) &&
+ if (!entry && check_limit && !guest_cpuid_is_amd_or_hygon(vcpu) &&
!cpuid_function_in_range(vcpu, function)) {
max = kvm_find_cpuid_entry(vcpu, 0, 0);
if (max) {
diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
index 13eb3e92c6a9..332068db0fc2 100644
--- a/arch/x86/kvm/cpuid.h
+++ b/arch/x86/kvm/cpuid.h
@@ -140,12 +140,14 @@ static __always_inline void guest_cpuid_clear(struct kvm_vcpu *vcpu, unsigned x8
*reg &= ~__feature_bit(x86_feature);
}
-static inline bool guest_cpuid_is_amd(struct kvm_vcpu *vcpu)
+static inline bool guest_cpuid_is_amd_or_hygon(struct kvm_vcpu *vcpu)
{
struct kvm_cpuid_entry2 *best;
best = kvm_find_cpuid_entry(vcpu, 0, 0);
- return best && is_guest_vendor_amd(best->ebx, best->ecx, best->edx);
+ return best &&
+ (is_guest_vendor_amd(best->ebx, best->ecx, best->edx) ||
+ is_guest_vendor_hygon(best->ebx, best->ecx, best->edx));
}
static inline int guest_cpuid_family(struct kvm_vcpu *vcpu)
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index a1f4e325420e..c7ef2745a4e0 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -4507,7 +4507,8 @@ static void reset_rsvds_bits_mask(struct kvm_vcpu *vcpu,
cpuid_maxphyaddr(vcpu), context->root_level,
context->nx,
guest_cpuid_has(vcpu, X86_FEATURE_GBPAGES),
- is_pse(vcpu), guest_cpuid_is_amd(vcpu));
+ is_pse(vcpu),
+ guest_cpuid_is_amd_or_hygon(vcpu));
}
static void
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index fa03f31ab33c..1a4836ed1230 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2524,7 +2524,7 @@ static void kvmclock_sync_fn(struct work_struct *work)
static bool can_set_mci_status(struct kvm_vcpu *vcpu)
{
/* McStatusWrEn enabled? */
- if (guest_cpuid_is_amd(vcpu))
+ if (guest_cpuid_is_amd_or_hygon(vcpu))
return !!(vcpu->arch.msr_hwcr & BIT_ULL(18));
return false;
--
2.24.1
From: Jan Kiszka <[email protected]>
Trace the requested CPUID function instead of the effective function,
e.g. if the requested function is out-of-range and KVM is emulating an
Intel CPU, as the intent of the tracepoint is to show if the output came
from the actual leaf as opposed to the max basic leaf via redirection.
Similarly, leave "found" as is, i.e. report that an entry was found if
and only if the requested entry was found.
Fixes: 43561123ab37 ("kvm: x86: Improve emulation of CPUID leaves 0BH and 1FH")
Signed-off-by: Jan Kiszka <[email protected]>
[Sean: Drop "found" semantic change, reword changelong accordingly ]
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/kvm/cpuid.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index b1c469446b07..b4beb3707d1b 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -1000,7 +1000,7 @@ static bool cpuid_function_in_range(struct kvm_vcpu *vcpu, u32 function)
bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
u32 *ecx, u32 *edx, bool check_limit)
{
- u32 function = *eax, index = *ecx;
+ u32 orig_function = *eax, function = *eax, index = *ecx;
struct kvm_cpuid_entry2 *entry;
struct kvm_cpuid_entry2 *max;
bool found;
@@ -1049,7 +1049,7 @@ bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
}
}
}
- trace_kvm_cpuid(function, *eax, *ebx, *ecx, *edx, found);
+ trace_kvm_cpuid(orig_function, *eax, *ebx, *ecx, *edx, found);
return found;
}
EXPORT_SYMBOL_GPL(kvm_cpuid);
--
2.24.1
On 05/03/20 02:34, Sean Christopherson wrote:
>
> In theory, everything up to the refactoring is non-controversial, i.e. we
> can bikeshed the refactoring without delaying the bug fixes.
Even the refactoring itself is much less controversial. I queued
everything, there's always time to unqueue.
Paolo
> v2:
> - Use Jan's patch to fix the trace bug. [Everyone]
> - Rework Hypervisor/Centaur handling so that only the Hypervisor
> sub-ranges get the restrictive 0xffffff00 mask, and so that Centaur's
> range only gets recognized when the guest vendor is Centaur. [Jim]
> - Add the aforementioned bug fixes.
> - Add a patch to do build time assertions on the vendor string, which
> are hand coded u32s in the emulator (for direct comparison against
> CPUID register output).
> - Drop the patch to add CPUID.maxphyaddr emulator helper. [Paolo]
> - Redo refactoring patches to land them after all the bug fixes
> and to do the refactoring without any semantic changes in the
> emulator.
On Thu, Mar 05, 2020 at 05:42:59PM +0100, Paolo Bonzini wrote:
> On 05/03/20 02:34, Sean Christopherson wrote:
> >
> > In theory, everything up to the refactoring is non-controversial, i.e. we
> > can bikeshed the refactoring without delaying the bug fixes.
>
> Even the refactoring itself is much less controversial. I queued
> everything, there's always time to unqueue.
Looks like the build-time assertions don't play nice with older versions of
gcc :-(
config: x86_64-randconfig-s2-20200305 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.2-10+deb8u1) 4.9.2
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <[email protected]>
All error/warnings (new ones prefixed by >>):
In file included from include/linux/export.h:43:0,
from include/linux/linkage.h:7,
from include/linux/preempt.h:10,
from include/linux/hardirq.h:5,
from include/linux/kvm_host.h:7,
from arch/x86/kvm/emulate.c:21:
arch/x86/kvm/emulate.c: In function 'em_cpuid':
>> include/linux/compiler.h:350:38: error: call to '__compiletime_assert_3957' declared with attribute error: BUILD_BUG_ON failed: X86EMUL_CPUID_VENDOR_AuthenticAMD_ebx != *(u32 *)"Auth" || X86EMUL_CPUID_VENDOR_AuthenticAMD_edx != *(u32 *)"enti" || X86EMUL_CPUID_VENDOR_AuthenticAMD_ecx != *(u32 *)"cAMD"
_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
On Wed, Mar 4, 2020 at 5:34 PM Sean Christopherson
<[email protected]> wrote:
>
> From: Jan Kiszka <[email protected]>
>
> Trace the requested CPUID function instead of the effective function,
> e.g. if the requested function is out-of-range and KVM is emulating an
> Intel CPU, as the intent of the tracepoint is to show if the output came
> from the actual leaf as opposed to the max basic leaf via redirection.
>
> Similarly, leave "found" as is, i.e. report that an entry was found if
> and only if the requested entry was found.
>
> Fixes: 43561123ab37 ("kvm: x86: Improve emulation of CPUID leaves 0BH and 1FH")
> Signed-off-by: Jan Kiszka <[email protected]>
> [Sean: Drop "found" semantic change, reword changelong accordingly ]
> Signed-off-by: Sean Christopherson <[email protected]>
Reviewed-by: Jim Mattson <[email protected]>
On 05/03/20 18:12, Sean Christopherson wrote:
>>> In theory, everything up to the refactoring is non-controversial, i.e. we
>>> can bikeshed the refactoring without delaying the bug fixes.
>> Even the refactoring itself is much less controversial. I queued
>> everything, there's always time to unqueue.
> Looks like the build-time assertions don't play nice with older versions of
> gcc :-(
Yes, I was quite surprised that they worked. I suppose you could write
a macro that checks against 'G', 'e', 'n', 'u', 'i', 'n', 'e', 'I', 'n',
't', 'e', 'l'...
Paolo