Re: [PATCH 6/6] hv: block guest eist/hwp cpuids and MSRs


Eddie Dong
 

-----Original Message-----
From: acrn-dev@... <acrn-dev@...> On
Behalf Of Zhou, Wu
Sent: Wednesday, August 10, 2022 2:25 AM
To: acrn-dev@...
Cc: Zhou, Wu <wu.zhou@...>
Subject: [acrn-dev] [PATCH 6/6] hv: block guest eist/hwp cpuids and MSRs

This patch is to disable guests from controlling CPU frequency.
Now ACRN has owned CPU frequency control, so guests should no longer have
it. This is done by blocking the eist/hwp cpuids and MSRs, and as well as the
hypercall on p-state info(which the DM would use to generate guest ACPI p-
state related tables).

Alternatively, ACPI p-state can be passed through to VMs if they do not share
pCPUs(or interfere other VM's CPU frequency). Offline tool will generate a flag
to indicate if the ACPI p-state is OK to pass through.
When the flag is set, the eist cpuid/MSRs and hypercalls are not blocked.

Signed-off-by: Wu Zhou <wu.zhou@...>
---
devicemodel/include/types.h | 1 +
hypervisor/arch/x86/guest/vcpuid.c | 10 ++++++
hypervisor/arch/x86/guest/vmsr.c | 38 ++++++++++++++++++--
hypervisor/common/hypercall.c | 2 +-
hypervisor/include/arch/x86/asm/cpuid.h | 2 ++
hypervisor/include/arch/x86/asm/guest/vcpu.h | 2 +-
6 files changed, 50 insertions(+), 5 deletions(-)

diff --git a/devicemodel/include/types.h b/devicemodel/include/types.h index
64527cc41..c32c6a262 100644
--- a/devicemodel/include/types.h
+++ b/devicemodel/include/types.h
@@ -10,6 +10,7 @@
#include <stdarg.h>
#include <sched.h>
#include <sys/types.h>
+#include <stdbool.h>

#define MAXCOMLEN 19 /* max command name remembered */
#define MAXINTERP PATH_MAX /* max interpreter file name length */
diff --git a/hypervisor/arch/x86/guest/vcpuid.c
b/hypervisor/arch/x86/guest/vcpuid.c
index 580098719..5900db99d 100644
--- a/hypervisor/arch/x86/guest/vcpuid.c
+++ b/hypervisor/arch/x86/guest/vcpuid.c
@@ -17,6 +17,7 @@
#include <logmsg.h>
#include <asm/rdt.h>
#include <asm/guest/vcat.h>
+#include <asm/per_cpu.h>

static inline const struct vcpuid_entry *local_find_vcpuid_entry(const struct
acrn_vcpu *vcpu,
uint32_t leaf, uint32_t subleaf)
@@ -115,6 +116,12 @@ static void init_vcpuid_entry(uint32_t leaf, uint32_t
subleaf,
entry->flags = flags;

switch (leaf) {
+
+ case 0x06U:
+ cpuid_subleaf(leaf, subleaf, &entry->eax, &entry->ebx,
&entry->ecx, &entry->edx);
+ entry->eax &= ~CPUID_06_EAX_HWP;
Should not remove all bits 7..17? We need a fixed value.

+ break;
+
case 0x07U:
if (subleaf == 0U) {
uint64_t cr4_reserved_mask =
get_cr4_reserved_bits(); @@ -627,6 +634,9 @@ static void
guest_cpuid_01h(struct acrn_vcpu *vcpu, uint32_t *eax, uint32_t *ebx
/* mask Safer Mode Extension */
*ecx &= ~CPUID_ECX_SMX;

+ if (!(get_vm_config(vcpu->vm->vm_id)->pt_acpi_pstate))
Under what condition, pt_acpi_pstate is set?

I think the cover letter is very confusing and unable to read. We need a good description to tell what the patch series do.

+ *ecx &= ~CPUID_ECX_EST;
+
/* mask SDBG for silicon debug */
*ecx &= ~CPUID_ECX_SDBG;

diff --git a/hypervisor/arch/x86/guest/vmsr.c
b/hypervisor/arch/x86/guest/vmsr.c
index 6509d34b0..d24767f3d 100644
--- a/hypervisor/arch/x86/guest/vmsr.c
+++ b/hypervisor/arch/x86/guest/vmsr.c
@@ -52,6 +52,9 @@ static uint32_t
emulated_guest_msrs[NUM_EMULATED_MSRS] = {
MSR_IA32_TIME_STAMP_COUNTER,
MSR_IA32_APIC_BASE,
MSR_IA32_PERF_CTL,
+ MSR_IA32_PM_ENABLE,
+ MSR_IA32_HWP_CAPABILITIES,
+ MSR_IA32_HWP_REQUEST,
Why put them under emulated_guest_msrs?
Not unsupported_msrs?

MSR_IA32_FEATURE_CONTROL,

MSR_IA32_MCG_CAP,
@@ -629,6 +632,21 @@ int32_t rdmsr_vmexit_handler(struct acrn_vcpu
*vcpu)
v = msr_read(msr);
break;
}
+ case MSR_IA32_PM_ENABLE:
+ {
+ vcpu_inject_gp(vcpu, 0U);
+ break;
+ }
+ case MSR_IA32_HWP_CAPABILITIES:
+ {
+ vcpu_inject_gp(vcpu, 0U);
+ break;
+ }
+ case MSR_IA32_HWP_REQUEST:
+ {
+ vcpu_inject_gp(vcpu, 0U);
+ break;
+ }
case MSR_IA32_PAT:
{
/*
@@ -1001,10 +1019,24 @@ int32_t wrmsr_vmexit_handler(struct acrn_vcpu
*vcpu)
}
case MSR_IA32_PERF_CTL:
{
- if (validate_pstate(vcpu->vm, v) != 0) {
- break;
+ if (get_vm_config(vcpu->vm->vm_id)->pt_acpi_pstate) {
+ msr_write(msr, v);
}
- msr_write(msr, v);
+ break;
+ }
+ case MSR_IA32_PM_ENABLE:
+ {
+ vcpu_inject_gp(vcpu, 0U);
+ break;
+ }
+ case MSR_IA32_HWP_CAPABILITIES:
+ {
+ vcpu_inject_gp(vcpu, 0U);
+ break;
+ }
+ case MSR_IA32_HWP_REQUEST:
+ {
+ vcpu_inject_gp(vcpu, 0U);
break;
}
case MSR_IA32_PAT:
diff --git a/hypervisor/common/hypercall.c b/hypervisor/common/hypercall.c
index e27eaf7e5..073e1be25 100644
--- a/hypervisor/common/hypercall.c
+++ b/hypervisor/common/hypercall.c
@@ -1058,7 +1058,7 @@ int32_t hcall_get_cpu_pm_state(struct acrn_vcpu
*vcpu, struct acrn_vm *target_vm
* If it is stored as per-cpu in the future,
* we need to check PMCMD_VCPUID_MASK in cmd.
*/
- if (target_vm->pm.px_cnt == 0U) {
+ if (target_vm->pm.px_cnt == 0U ||
+!(get_vm_config(target_vm->vm_id)->pt_acpi_pstate)) {
break;
}

diff --git a/hypervisor/include/arch/x86/asm/cpuid.h
b/hypervisor/include/arch/x86/asm/cpuid.h
index 491758bab..6989a2637 100644
--- a/hypervisor/include/arch/x86/asm/cpuid.h
+++ b/hypervisor/include/arch/x86/asm/cpuid.h
@@ -72,6 +72,8 @@
#define CPUID_EDX_TM1 (1U<<29U)
#define CPUID_EDX_IA64 (1U<<30U)
#define CPUID_EDX_PBE (1U<<31U)
+/* CPUID.06H:EAX.HWP*/
+#define CPUID_06_EAX_HWP (1U<<7U)
/* CPUID.07H:EBX.FSGSBASE*/
#define CPUID_EBX_FSGSBASE (1U<<0U)
/* CPUID.07H:EBX.TSC_ADJUST*/
diff --git a/hypervisor/include/arch/x86/asm/guest/vcpu.h
b/hypervisor/include/arch/x86/asm/guest/vcpu.h
index 87f6930a6..26f333f80 100644
--- a/hypervisor/include/arch/x86/asm/guest/vcpu.h
+++ b/hypervisor/include/arch/x86/asm/guest/vcpu.h
@@ -173,7 +173,7 @@ enum reset_mode;
#define SECURE_WORLD 1

#define NUM_WORLD_MSRS 2U
-#define NUM_COMMON_MSRS 23U
+#define NUM_COMMON_MSRS 26U

#ifdef CONFIG_VCAT_ENABLED
#define NUM_CAT_L2_MSRS MAX_CACHE_CLOS_NUM_ENTRIES
--
2.25.1




Join {acrn-dev@lists.projectacrn.org to automatically receive all group messages.