Add helper functions to centralize guest MSR read and write emulation.
This change consolidates the MSR emulation logic and makes it easier
to extend support for new MSR-related VM exit reasons introduced with
the immediate form of MSR instructions.
Signed-off-by: Xin Li (Intel) <xin@zytor.com>
---
arch/x86/include/asm/kvm_host.h | 1 +
arch/x86/kvm/x86.c | 67 +++++++++++++++++++++++----------
2 files changed, 49 insertions(+), 19 deletions(-)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index f19a76d3ca0e..a854d9a166fe 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -201,6 +201,7 @@ enum kvm_reg {
VCPU_EXREG_SEGMENTS,
VCPU_EXREG_EXIT_INFO_1,
VCPU_EXREG_EXIT_INFO_2,
+ VCPU_EXREG_EDX_EAX,
};
enum {
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index a1c49bc681c4..5086c3b30345 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2024,54 +2024,71 @@ static int kvm_msr_user_space(struct kvm_vcpu *vcpu, u32 index,
return 1;
}
-int kvm_emulate_rdmsr(struct kvm_vcpu *vcpu)
+static int kvm_emulate_get_msr(struct kvm_vcpu *vcpu, u32 msr, int reg)
{
- u32 ecx = kvm_rcx_read(vcpu);
u64 data;
int r;
- r = kvm_get_msr_with_filter(vcpu, ecx, &data);
+ r = kvm_get_msr_with_filter(vcpu, msr, &data);
if (!r) {
- trace_kvm_msr_read(ecx, data);
+ trace_kvm_msr_read(msr, data);
- kvm_rax_write(vcpu, data & -1u);
- kvm_rdx_write(vcpu, (data >> 32) & -1u);
+ if (reg == VCPU_EXREG_EDX_EAX) {
+ kvm_rax_write(vcpu, data & -1u);
+ kvm_rdx_write(vcpu, (data >> 32) & -1u);
+ } else {
+ kvm_register_write(vcpu, reg, data);
+ }
} else {
/* MSR read failed? See if we should ask user space */
- if (kvm_msr_user_space(vcpu, ecx, KVM_EXIT_X86_RDMSR, 0,
+ if (kvm_msr_user_space(vcpu, msr, KVM_EXIT_X86_RDMSR, 0,
complete_fast_rdmsr, r))
return 0;
- trace_kvm_msr_read_ex(ecx);
+ trace_kvm_msr_read_ex(msr);
}
return kvm_x86_call(complete_emulated_msr)(vcpu, r);
}
+
+int kvm_emulate_rdmsr(struct kvm_vcpu *vcpu)
+{
+ return kvm_emulate_get_msr(vcpu, kvm_rcx_read(vcpu), VCPU_EXREG_EDX_EAX);
+}
EXPORT_SYMBOL_GPL(kvm_emulate_rdmsr);
-int kvm_emulate_wrmsr(struct kvm_vcpu *vcpu)
+static int kvm_emulate_set_msr(struct kvm_vcpu *vcpu, u32 msr, int reg)
{
- u32 ecx = kvm_rcx_read(vcpu);
- u64 data = kvm_read_edx_eax(vcpu);
+ u64 data;
int r;
- r = kvm_set_msr_with_filter(vcpu, ecx, data);
+ if (reg == VCPU_EXREG_EDX_EAX)
+ data = kvm_read_edx_eax(vcpu);
+ else
+ data = kvm_register_read(vcpu, reg);
+
+ r = kvm_set_msr_with_filter(vcpu, msr, data);
if (!r) {
- trace_kvm_msr_write(ecx, data);
+ trace_kvm_msr_write(msr, data);
} else {
/* MSR write failed? See if we should ask user space */
- if (kvm_msr_user_space(vcpu, ecx, KVM_EXIT_X86_WRMSR, data,
+ if (kvm_msr_user_space(vcpu, msr, KVM_EXIT_X86_WRMSR, data,
complete_fast_msr_access, r))
return 0;
/* Signal all other negative errors to userspace */
if (r < 0)
return r;
- trace_kvm_msr_write_ex(ecx, data);
+ trace_kvm_msr_write_ex(msr, data);
}
return kvm_x86_call(complete_emulated_msr)(vcpu, r);
}
+
+int kvm_emulate_wrmsr(struct kvm_vcpu *vcpu)
+{
+ return kvm_emulate_set_msr(vcpu, kvm_rcx_read(vcpu), VCPU_EXREG_EDX_EAX);
+}
EXPORT_SYMBOL_GPL(kvm_emulate_wrmsr);
int kvm_emulate_as_nop(struct kvm_vcpu *vcpu)
@@ -2163,9 +2180,8 @@ static int handle_fastpath_set_tscdeadline(struct kvm_vcpu *vcpu, u64 data)
return 0;
}
-fastpath_t handle_fastpath_set_msr_irqoff(struct kvm_vcpu *vcpu)
+static fastpath_t handle_set_msr_irqoff(struct kvm_vcpu *vcpu, u32 msr, int reg)
{
- u32 msr = kvm_rcx_read(vcpu);
u64 data;
fastpath_t ret;
bool handled;
@@ -2174,11 +2190,19 @@ fastpath_t handle_fastpath_set_msr_irqoff(struct kvm_vcpu *vcpu)
switch (msr) {
case APIC_BASE_MSR + (APIC_ICR >> 4):
- data = kvm_read_edx_eax(vcpu);
+ if (reg == VCPU_EXREG_EDX_EAX)
+ data = kvm_read_edx_eax(vcpu);
+ else
+ data = kvm_register_read(vcpu, reg);
+
handled = !handle_fastpath_set_x2apic_icr_irqoff(vcpu, data);
break;
case MSR_IA32_TSC_DEADLINE:
- data = kvm_read_edx_eax(vcpu);
+ if (reg == VCPU_EXREG_EDX_EAX)
+ data = kvm_read_edx_eax(vcpu);
+ else
+ data = kvm_register_read(vcpu, reg);
+
handled = !handle_fastpath_set_tscdeadline(vcpu, data);
break;
default:
@@ -2200,6 +2224,11 @@ fastpath_t handle_fastpath_set_msr_irqoff(struct kvm_vcpu *vcpu)
return ret;
}
+
+fastpath_t handle_fastpath_set_msr_irqoff(struct kvm_vcpu *vcpu)
+{
+ return handle_set_msr_irqoff(vcpu, kvm_rcx_read(vcpu), VCPU_EXREG_EDX_EAX);
+}
EXPORT_SYMBOL_GPL(handle_fastpath_set_msr_irqoff);
/*
--
2.50.1On Wed, Jul 30, 2025, Xin Li (Intel) wrote:
> Add helper functions to centralize guest MSR read and write emulation.
> This change consolidates the MSR emulation logic and makes it easier
> to extend support for new MSR-related VM exit reasons introduced with
> the immediate form of MSR instructions.
>
> Signed-off-by: Xin Li (Intel) <xin@zytor.com>
> ---
> arch/x86/include/asm/kvm_host.h | 1 +
> arch/x86/kvm/x86.c | 67 +++++++++++++++++++++++----------
> 2 files changed, 49 insertions(+), 19 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index f19a76d3ca0e..a854d9a166fe 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -201,6 +201,7 @@ enum kvm_reg {
> VCPU_EXREG_SEGMENTS,
> VCPU_EXREG_EXIT_INFO_1,
> VCPU_EXREG_EXIT_INFO_2,
> + VCPU_EXREG_EDX_EAX,
I really, really don't want to add a "reg" for this. It's not an actual register,
and bleeds details of one specific flow throughout KVM.
The only path where KVM _needs_ to differentiate between the "legacy" instructions
and the immediate variants instruction is in the inner RDMSR helper.
For the WRMSR helper, KVM can and should simply pass in @data, not pass in a reg
and then have the helper do an if-else on the reg:
int kvm_emulate_wrmsr(struct kvm_vcpu *vcpu)
{
return __kvm_emulate_wrmsr(vcpu, kvm_rcx_read(vcpu),
kvm_read_edx_eax(vcpu));
}
EXPORT_SYMBOL_GPL(kvm_emulate_wrmsr);
int kvm_emulate_wrmsr_imm(struct kvm_vcpu *vcpu, u32 msr, int reg)
{
return __kvm_emulate_wrmsr(vcpu, msr, kvm_register_read(vcpu, reg));
}
EXPORT_SYMBOL_GPL(kvm_emulate_wrmsr_imm);
And for the RDMSR userspace completion, KVM is already eating an indirect function
call, so the wrappers can simply pass in the appropriate completion helper. It
does mean having to duplicate the vcpu->run->msr.error check, but we'd have to
duplicate the "r == VCPU_EXREG_EDX_EAX" by sharing a callback, *and* we'd also
need to be very careful about setting the effective register in the other existing
flows that utilize complete_fast_rdmsr.
Then to communicate that the legacy form with implicit destination operands is
being emulated, pass -1 for the register. It's not the prettiest, but I do like
using "reg invalid" to communicate that the destination is implicit.
static int __kvm_emulate_rdmsr(struct kvm_vcpu *vcpu, u32 msr, int reg,
int (*complete_rdmsr)(struct kvm_vcpu *))
{
u64 data;
int r;
r = kvm_get_msr_with_filter(vcpu, msr, &data);
if (!r) {
trace_kvm_msr_read(msr, data);
if (reg < 0) {
kvm_rax_write(vcpu, data & -1u);
kvm_rdx_write(vcpu, (data >> 32) & -1u);
} else {
kvm_register_write(vcpu, reg, data);
}
} else {
/* MSR read failed? See if we should ask user space */
if (kvm_msr_user_space(vcpu, msr, KVM_EXIT_X86_RDMSR, 0,
complete_rdmsr, r))
return 0;
trace_kvm_msr_read_ex(msr);
}
return kvm_x86_call(complete_emulated_msr)(vcpu, r);
}
int kvm_emulate_rdmsr(struct kvm_vcpu *vcpu)
{
return __kvm_emulate_rdmsr(vcpu, kvm_rcx_read(vcpu), -1,
complete_fast_rdmsr);
}
EXPORT_SYMBOL_GPL(kvm_emulate_rdmsr);
int kvm_emulate_rdmsr_imm(struct kvm_vcpu *vcpu, u32 msr, int reg)
{
vcpu->arch.cui_rdmsr_imm_reg = reg;
return __kvm_emulate_rdmsr(vcpu, msr, reg, complete_fast_rdmsr_imm);
}
EXPORT_SYMBOL_GPL(kvm_emulate_rdmsr_imm);
> };
>
> enum {
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index a1c49bc681c4..5086c3b30345 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2024,54 +2024,71 @@ static int kvm_msr_user_space(struct kvm_vcpu *vcpu, u32 index,
> return 1;
> }
>
> -int kvm_emulate_rdmsr(struct kvm_vcpu *vcpu)
> +static int kvm_emulate_get_msr(struct kvm_vcpu *vcpu, u32 msr, int reg)
Please keep "rdmsr" and "wrmsr" when dealing emulation of those instructions to
help differentiate from the many other MSR get/set paths. (ignore the actual
emulator hooks; that code is crusty, but not worth the churn to clean up).
> @@ -2163,9 +2180,8 @@ static int handle_fastpath_set_tscdeadline(struct kvm_vcpu *vcpu, u64 data)
> return 0;
> }
>
> -fastpath_t handle_fastpath_set_msr_irqoff(struct kvm_vcpu *vcpu)
> +static fastpath_t handle_set_msr_irqoff(struct kvm_vcpu *vcpu, u32 msr, int reg)
I think it makes sense to (a) add the x86.c code and the vmx.c code in the same
patch, and then (b) add fastpath support in a separate patch to make the initial
(combined x86.c + vmx.c) patch easier to review. Adding the x86.c plumbing/logic
before the VMX support makes the x86.c change difficult to review, as there are
no users of the new paths, and the VMX changes are quite tiny. Ignoring the arch
boilerplate, the VMX changes barely add anything relative to the x86.c changes.
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index ae2c8c10e5d2..757e4bb89f36 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -6003,6 +6003,23 @@ static int handle_notify(struct kvm_vcpu *vcpu)
return 1;
}
+static int vmx_get_msr_imm_reg(struct kvm_vcpu *vcpu)
+{
+ return vmx_get_instr_info_reg(vmcs_read32(VMX_INSTRUCTION_INFO))
+}
+
+static int handle_rdmsr_imm(struct kvm_vcpu *vcpu)
+{
+ return kvm_emulate_rdmsr_imm(vcpu, vmx_get_exit_qual(vcpu),
+ vmx_get_msr_imm_reg(vcpu));
+}
+
+static int handle_wrmsr_imm(struct kvm_vcpu *vcpu)
+{
+ return kvm_emulate_wrmsr_imm(vcpu, vmx_get_exit_qual(vcpu),
+ vmx_get_msr_imm_reg(vcpu));
+}
+
/*
* The exit handlers return 1 if the exit was handled fully and guest execution
* may resume. Otherwise they set the kvm_run parameter to indicate what needs
@@ -6061,6 +6078,8 @@ static int (*kvm_vmx_exit_handlers[])(struct kvm_vcpu *vcpu) = {
[EXIT_REASON_ENCLS] = handle_encls,
[EXIT_REASON_BUS_LOCK] = handle_bus_lock_vmexit,
[EXIT_REASON_NOTIFY] = handle_notify,
+ [EXIT_REASON_MSR_READ_IMM] = handle_rdmsr_imm,
+ [EXIT_REASON_MSR_WRITE_IMM] = handle_wrmsr_imm,
};
static const int kvm_vmx_max_exit_handlers =
@@ -6495,6 +6514,8 @@ static int __vmx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
#ifdef CONFIG_MITIGATION_RETPOLINE
if (exit_reason.basic == EXIT_REASON_MSR_WRITE)
return kvm_emulate_wrmsr(vcpu);
+ else if (exit_reason.basic == EXIT_REASON_MSR_WRITE_IMM)
+ return handle_wrmsr_imm(vcpu);
else if (exit_reason.basic == EXIT_REASON_PREEMPTION_TIMER)
return handle_preemption_timer(vcpu);
else if (exit_reason.basic == EXIT_REASON_INTERRUPT_WINDOW)
On 8/1/2025 7:37 AM, Sean Christopherson wrote:
> On Wed, Jul 30, 2025, Xin Li (Intel) wrote:
>> Add helper functions to centralize guest MSR read and write emulation.
>> This change consolidates the MSR emulation logic and makes it easier
>> to extend support for new MSR-related VM exit reasons introduced with
>> the immediate form of MSR instructions.
>>
>> Signed-off-by: Xin Li (Intel) <xin@zytor.com>
>> ---
>> arch/x86/include/asm/kvm_host.h | 1 +
>> arch/x86/kvm/x86.c | 67 +++++++++++++++++++++++----------
>> 2 files changed, 49 insertions(+), 19 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index f19a76d3ca0e..a854d9a166fe 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -201,6 +201,7 @@ enum kvm_reg {
>> VCPU_EXREG_SEGMENTS,
>> VCPU_EXREG_EXIT_INFO_1,
>> VCPU_EXREG_EXIT_INFO_2,
>> + VCPU_EXREG_EDX_EAX,
>
> I really, really don't want to add a "reg" for this. It's not an actual register,
> and bleeds details of one specific flow throughout KVM.
Sure.
>
> The only path where KVM _needs_ to differentiate between the "legacy" instructions
> and the immediate variants instruction is in the inner RDMSR helper.
>
> For the WRMSR helper, KVM can and should simply pass in @data, not pass in a reg
> and then have the helper do an if-else on the reg:
My initial patch passes @data in the WRMSR path, but to make it
consistent with the handling of RDMSR I changed it to @reg.
Yes, passing @data makes more sense because it hides unneccesary details.
>
> int kvm_emulate_wrmsr(struct kvm_vcpu *vcpu)
> {
> return __kvm_emulate_wrmsr(vcpu, kvm_rcx_read(vcpu),
> kvm_read_edx_eax(vcpu));
> }
> EXPORT_SYMBOL_GPL(kvm_emulate_wrmsr);
>
> int kvm_emulate_wrmsr_imm(struct kvm_vcpu *vcpu, u32 msr, int reg)
> {
> return __kvm_emulate_wrmsr(vcpu, msr, kvm_register_read(vcpu, reg));
> }
> EXPORT_SYMBOL_GPL(kvm_emulate_wrmsr_imm);
>
> And for the RDMSR userspace completion, KVM is already eating an indirect function
> call, so the wrappers can simply pass in the appropriate completion helper. It
> does mean having to duplicate the vcpu->run->msr.error check, but we'd have to
> duplicate the "r == VCPU_EXREG_EDX_EAX" by sharing a callback, *and* we'd also
> need to be very careful about setting the effective register in the other existing
> flows that utilize complete_fast_rdmsr.
>
> Then to communicate that the legacy form with implicit destination operands is
> being emulated, pass -1 for the register. It's not the prettiest, but I do like
> using "reg invalid" to communicate that the destination is implicit.
>
> static int __kvm_emulate_rdmsr(struct kvm_vcpu *vcpu, u32 msr, int reg,
> int (*complete_rdmsr)(struct kvm_vcpu *))
Yeah, it is a clean way to pass a userspace completion callback.
> {
> u64 data;
> int r;
>
> r = kvm_get_msr_with_filter(vcpu, msr, &data);
> if (!r) {
> trace_kvm_msr_read(msr, data);
>
> if (reg < 0) {
> kvm_rax_write(vcpu, data & -1u);
> kvm_rdx_write(vcpu, (data >> 32) & -1u);
> } else {
> kvm_register_write(vcpu, reg, data);
> }
> } else {
> /* MSR read failed? See if we should ask user space */
> if (kvm_msr_user_space(vcpu, msr, KVM_EXIT_X86_RDMSR, 0,
> complete_rdmsr, r))
> return 0;
> trace_kvm_msr_read_ex(msr);
> }
>
> return kvm_x86_call(complete_emulated_msr)(vcpu, r);
> }
>
> int kvm_emulate_rdmsr(struct kvm_vcpu *vcpu)
> {
> return __kvm_emulate_rdmsr(vcpu, kvm_rcx_read(vcpu), -1,
> complete_fast_rdmsr);
> }
> EXPORT_SYMBOL_GPL(kvm_emulate_rdmsr);
>
> int kvm_emulate_rdmsr_imm(struct kvm_vcpu *vcpu, u32 msr, int reg)
> {
> vcpu->arch.cui_rdmsr_imm_reg = reg;
>
> return __kvm_emulate_rdmsr(vcpu, msr, reg, complete_fast_rdmsr_imm);
> }
> EXPORT_SYMBOL_GPL(kvm_emulate_rdmsr_imm);
>
>> };
>>
>> enum {
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index a1c49bc681c4..5086c3b30345 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -2024,54 +2024,71 @@ static int kvm_msr_user_space(struct kvm_vcpu *vcpu, u32 index,
>> return 1;
>> }
>>
>> -int kvm_emulate_rdmsr(struct kvm_vcpu *vcpu)
>> +static int kvm_emulate_get_msr(struct kvm_vcpu *vcpu, u32 msr, int reg)
>
> Please keep "rdmsr" and "wrmsr" when dealing emulation of those instructions to
> help differentiate from the many other MSR get/set paths. (ignore the actual
> emulator hooks; that code is crusty, but not worth the churn to clean up).
Once the rules are laid out, it's easy to act :)
>
>> @@ -2163,9 +2180,8 @@ static int handle_fastpath_set_tscdeadline(struct kvm_vcpu *vcpu, u64 data)
>> return 0;
>> }
>>
>> -fastpath_t handle_fastpath_set_msr_irqoff(struct kvm_vcpu *vcpu)
>> +static fastpath_t handle_set_msr_irqoff(struct kvm_vcpu *vcpu, u32 msr, int reg)
>
> I think it makes sense to (a) add the x86.c code and the vmx.c code in the same
> patch, and then (b) add fastpath support in a separate patch to make the initial
> (combined x86.c + vmx.c) patch easier to review. Adding the x86.c plumbing/logic
> before the VMX support makes the x86.c change difficult to review, as there are
> no users of the new paths, and the VMX changes are quite tiny. Ignoring the arch
> boilerplate, the VMX changes barely add anything relative to the x86.c changes.
Will do.
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index ae2c8c10e5d2..757e4bb89f36 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -6003,6 +6003,23 @@ static int handle_notify(struct kvm_vcpu *vcpu)
> return 1;
> }
>
> +static int vmx_get_msr_imm_reg(struct kvm_vcpu *vcpu)
> +{
> + return vmx_get_instr_info_reg(vmcs_read32(VMX_INSTRUCTION_INFO))
> +}
> +
> +static int handle_rdmsr_imm(struct kvm_vcpu *vcpu)
> +{
> + return kvm_emulate_rdmsr_imm(vcpu, vmx_get_exit_qual(vcpu),
> + vmx_get_msr_imm_reg(vcpu));
> +}
> +
> +static int handle_wrmsr_imm(struct kvm_vcpu *vcpu)
> +{
> + return kvm_emulate_wrmsr_imm(vcpu, vmx_get_exit_qual(vcpu),
> + vmx_get_msr_imm_reg(vcpu));
> +}
> +
> /*
> * The exit handlers return 1 if the exit was handled fully and guest execution
> * may resume. Otherwise they set the kvm_run parameter to indicate what needs
> @@ -6061,6 +6078,8 @@ static int (*kvm_vmx_exit_handlers[])(struct kvm_vcpu *vcpu) = {
> [EXIT_REASON_ENCLS] = handle_encls,
> [EXIT_REASON_BUS_LOCK] = handle_bus_lock_vmexit,
> [EXIT_REASON_NOTIFY] = handle_notify,
> + [EXIT_REASON_MSR_READ_IMM] = handle_rdmsr_imm,
> + [EXIT_REASON_MSR_WRITE_IMM] = handle_wrmsr_imm,
> };
>
> static const int kvm_vmx_max_exit_handlers =
> @@ -6495,6 +6514,8 @@ static int __vmx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
> #ifdef CONFIG_MITIGATION_RETPOLINE
> if (exit_reason.basic == EXIT_REASON_MSR_WRITE)
> return kvm_emulate_wrmsr(vcpu);
> + else if (exit_reason.basic == EXIT_REASON_MSR_WRITE_IMM)
> + return handle_wrmsr_imm(vcpu);
> else if (exit_reason.basic == EXIT_REASON_PREEMPTION_TIMER)
> return handle_preemption_timer(vcpu);
> else if (exit_reason.basic == EXIT_REASON_INTERRUPT_WINDOW)
>
Thanks!
Xin
>-fastpath_t handle_fastpath_set_msr_irqoff(struct kvm_vcpu *vcpu)
>+static fastpath_t handle_set_msr_irqoff(struct kvm_vcpu *vcpu, u32 msr, int reg)
How about __handle_fastpath_set_msr_irqoff()? It's better to keep
"fastpath" in the function name to convey that this function is for
fastpath only.
> {
>- u32 msr = kvm_rcx_read(vcpu);
> u64 data;
> fastpath_t ret;
> bool handled;
>@@ -2174,11 +2190,19 @@ fastpath_t handle_fastpath_set_msr_irqoff(struct kvm_vcpu *vcpu)
>
> switch (msr) {
> case APIC_BASE_MSR + (APIC_ICR >> 4):
>- data = kvm_read_edx_eax(vcpu);
>+ if (reg == VCPU_EXREG_EDX_EAX)
>+ data = kvm_read_edx_eax(vcpu);
>+ else
>+ data = kvm_register_read(vcpu, reg);
...
>+
> handled = !handle_fastpath_set_x2apic_icr_irqoff(vcpu, data);
> break;
> case MSR_IA32_TSC_DEADLINE:
>- data = kvm_read_edx_eax(vcpu);
>+ if (reg == VCPU_EXREG_EDX_EAX)
>+ data = kvm_read_edx_eax(vcpu);
>+ else
>+ data = kvm_register_read(vcpu, reg);
>+
Hoist this chunk out of the switch clause to avoid duplication.
> handled = !handle_fastpath_set_tscdeadline(vcpu, data);
> break;
> default:
>@@ -2200,6 +2224,11 @@ fastpath_t handle_fastpath_set_msr_irqoff(struct kvm_vcpu *vcpu)
>
> return ret;
> }
>+
>+fastpath_t handle_fastpath_set_msr_irqoff(struct kvm_vcpu *vcpu)
>+{
>+ return handle_set_msr_irqoff(vcpu, kvm_rcx_read(vcpu), VCPU_EXREG_EDX_EAX);
>+}
> EXPORT_SYMBOL_GPL(handle_fastpath_set_msr_irqoff);
>
> /*
>--
>2.50.1
>
On 7/31/2025 3:34 AM, Chao Gao wrote:
>> -fastpath_t handle_fastpath_set_msr_irqoff(struct kvm_vcpu *vcpu)
>> +static fastpath_t handle_set_msr_irqoff(struct kvm_vcpu *vcpu, u32 msr, int reg)
>
> How about __handle_fastpath_set_msr_irqoff()? It's better to keep
> "fastpath" in the function name to convey that this function is for
> fastpath only.
This is now a static function with return type fastpath_t, so I guess
it's okay to remove fastpath from its name (It looks that Sean prefers
shorter function names if they contains enough information).
But if the protocol is to have "fastpath" in all fast path function
names, I can change it.
>
>> {
>> - u32 msr = kvm_rcx_read(vcpu);
>> u64 data;
>> fastpath_t ret;
>> bool handled;
>> @@ -2174,11 +2190,19 @@ fastpath_t handle_fastpath_set_msr_irqoff(struct kvm_vcpu *vcpu)
>>
>> switch (msr) {
>> case APIC_BASE_MSR + (APIC_ICR >> 4):
>> - data = kvm_read_edx_eax(vcpu);
>> + if (reg == VCPU_EXREG_EDX_EAX)
>> + data = kvm_read_edx_eax(vcpu);
>> + else
>> + data = kvm_register_read(vcpu, reg);
>
> ...
>
>> +
>> handled = !handle_fastpath_set_x2apic_icr_irqoff(vcpu, data);
>> break;
>> case MSR_IA32_TSC_DEADLINE:
>> - data = kvm_read_edx_eax(vcpu);
>> + if (reg == VCPU_EXREG_EDX_EAX)
>> + data = kvm_read_edx_eax(vcpu);
>> + else
>> + data = kvm_register_read(vcpu, reg);
>> +
>
> Hoist this chunk out of the switch clause to avoid duplication.
I thought about it, but didn't do so because the original code doesn't
read the MSR data from registers when a MSR is not being handled in the
fast path, which saves some cycles in most cases.
On Thu, Jul 31, 2025, Xin Li wrote:
> On 7/31/2025 3:34 AM, Chao Gao wrote:
> > > -fastpath_t handle_fastpath_set_msr_irqoff(struct kvm_vcpu *vcpu)
> > > +static fastpath_t handle_set_msr_irqoff(struct kvm_vcpu *vcpu, u32 msr, int reg)
> >
> > How about __handle_fastpath_set_msr_irqoff()? It's better to keep
> > "fastpath" in the function name to convey that this function is for
> > fastpath only.
>
> This is now a static function with return type fastpath_t, so I guess
> it's okay to remove fastpath from its name (It looks that Sean prefers
> shorter function names if they contains enough information).
>
> But if the protocol is to have "fastpath" in all fast path function
> names, I can change it.
I'm also greedy and want it both ways :-)
Spoiler alert, this is what I ended up with (completely untested at this point):
static fastpath_t __handle_fastpath_wrmsr(struct kvm_vcpu *vcpu, u32 msr,
u64 data)
switch (msr) {
case APIC_BASE_MSR + (APIC_ICR >> 4):
if (!lapic_in_kernel(vcpu) || !apic_x2apic_mode(vcpu->arch.apic) ||
kvm_x2apic_icr_write_fast(vcpu->arch.apic, data))
return EXIT_FASTPATH_NONE;
break;
case MSR_IA32_TSC_DEADLINE:
if (!kvm_can_use_hv_timer(vcpu))
return EXIT_FASTPATH_NONE;
kvm_set_lapic_tscdeadline_msr(vcpu, data);
break;
default:
return EXIT_FASTPATH_NONE;
}
trace_kvm_msr_write(msr, data);
if (!kvm_skip_emulated_instruction(vcpu))
return EXIT_FASTPATH_EXIT_USERSPACE;
return EXIT_FASTPATH_REENTER_GUEST;
}
fastpath_t handle_fastpath_wrmsr(struct kvm_vcpu *vcpu)
{
return __handle_fastpath_wrmsr(vcpu, kvm_rcx_read(vcpu),
kvm_read_edx_eax(vcpu));
}
EXPORT_SYMBOL_GPL(handle_fastpath_set_msr_irqoff);
fastpath_t handle_fastpath_wrmsr_imm(struct kvm_vcpu *vcpu, u32 msr, int reg)
{
return __handle_fastpath_wrmsr(vcpu, msr, kvm_register_read(vcpu, reg));
}
EXPORT_SYMBOL_GPL(handle_fastpath_set_msr_imm_irqoff);
> > > {
> > > - u32 msr = kvm_rcx_read(vcpu);
> > > u64 data;
> > > fastpath_t ret;
> > > bool handled;
> > > @@ -2174,11 +2190,19 @@ fastpath_t handle_fastpath_set_msr_irqoff(struct kvm_vcpu *vcpu)
> > >
> > > switch (msr) {
> > > case APIC_BASE_MSR + (APIC_ICR >> 4):
> > > - data = kvm_read_edx_eax(vcpu);
> > > + if (reg == VCPU_EXREG_EDX_EAX)
> > > + data = kvm_read_edx_eax(vcpu);
> > > + else
> > > + data = kvm_register_read(vcpu, reg);
> >
> > ...
> >
> > > +
> > > handled = !handle_fastpath_set_x2apic_icr_irqoff(vcpu, data);
> > > break;
> > > case MSR_IA32_TSC_DEADLINE:
> > > - data = kvm_read_edx_eax(vcpu);
> > > + if (reg == VCPU_EXREG_EDX_EAX)
> > > + data = kvm_read_edx_eax(vcpu);
> > > + else
> > > + data = kvm_register_read(vcpu, reg);
> > > +
> >
> > Hoist this chunk out of the switch clause to avoid duplication.
>
> I thought about it, but didn't do so because the original code doesn't read
> the MSR data from registers when a MSR is not being handled in the
> fast path, which saves some cycles in most cases.
Can you hold off on doing anything with this series? Mostly to save your time.
Long story short, I unexpectedly dove into the fastpath code this week while sorting
out an issue with the mediated PMU series, and I ended up with a series of patches
to clean things up for both the mediated PMU series and for this series.
With luck, I'll get the cleanups, the mediated PMU series, and a v2 of this series
posted tomorrow (I also have some feedback on VCPU_EXREG_EDX_EAX; we can avoid it
entirely without much fuss).
>>>> + >>>> handled = !handle_fastpath_set_x2apic_icr_irqoff(vcpu, data); >>>> break; >>>> case MSR_IA32_TSC_DEADLINE: >>>> - data = kvm_read_edx_eax(vcpu); >>>> + if (reg == VCPU_EXREG_EDX_EAX) >>>> + data = kvm_read_edx_eax(vcpu); >>>> + else >>>> + data = kvm_register_read(vcpu, reg); >>>> + >>> >>> Hoist this chunk out of the switch clause to avoid duplication. >> >> I thought about it, but didn't do so because the original code doesn't read >> the MSR data from registers when a MSR is not being handled in the >> fast path, which saves some cycles in most cases. > > Can you hold off on doing anything with this series? Mostly to save your time. Sure. > > Long story short, I unexpectedly dove into the fastpath code this week while sorting > out an issue with the mediated PMU series, and I ended up with a series of patches > to clean things up for both the mediated PMU series and for this series. > > With luck, I'll get the cleanups, the mediated PMU series, and a v2 of this series > posted tomorrow (I also have some feedback on VCPU_EXREG_EDX_EAX; we can avoid it > entirely without much fuss). > Will wait and take a look when you post them.
On 7/31/2025 9:40 AM, Xin Li wrote: >>> + >>> handled = !handle_fastpath_set_x2apic_icr_irqoff(vcpu, data); >>> break; >>> case MSR_IA32_TSC_DEADLINE: >>> - data = kvm_read_edx_eax(vcpu); >>> + if (reg == VCPU_EXREG_EDX_EAX) >>> + data = kvm_read_edx_eax(vcpu); >>> + else >>> + data = kvm_register_read(vcpu, reg); >>> + >> >> Hoist this chunk out of the switch clause to avoid duplication. > > I thought about it, but didn't do so because the original code doesn't > read the MSR data from registers when a MSR is not being handled in the > fast path, which saves some cycles in most cases. I think I can make it an inline function.
© 2016 - 2025 Red Hat, Inc.