[PATCH v3 0/6] kvm: x86: better handling of optional kvm_x86_ops

[PATCH v3 0/6] kvm: x86: better handling of optional kvm_x86_ops

[Paolo Bonzini]

This series is really two changes:

- patch 1 remove a kvm_x86_ops callback.

- patch 2 to 5 clean up optional kvm_x86_ops so that they are marked
  in kvm-x86-ops.h and the non-optional ones WARN if used incorrectly.
  As an additional outcome of the review, a few more uses of
  static_call_cond are introduced.

- patch 6 allows to NULL a few kvm_x86_ops that return a value, by
  using __static_call_ret0.

v1->v2:
- use KVM_X86_OP_OPTIONAL and KVM_X86_OP_OPTIONAL_RET0
- mark load_eoi_exitmap and set_virtual_apic_mode as optional
- fix module compilation of KVM

v2->v3:
- new patch 1 to remove .has_accelerated_tpr
- do not expand KVM_X86_OP_OPTIONAL in KVM_X86_OP
- cosmetic changes to comments

Paolo Bonzini (6):
  KVM: x86: return 1 unconditionally for availability of KVM_CAP_VAPIC
  KVM: x86: use static_call_cond for optional callbacks
  KVM: x86: remove KVM_X86_OP_NULL and mark optional kvm_x86_ops
  KVM: x86: warn on incorrectly NULL members of kvm_x86_ops
  KVM: x86: make several AVIC callbacks optional
  KVM: x86: allow defining return-0 static calls

 arch/x86/include/asm/kvm-x86-ops.h | 104 +++++++++++++++--------------
 arch/x86/include/asm/kvm_host.h    |  14 ++--
 arch/x86/kvm/lapic.c               |  24 +++----
 arch/x86/kvm/svm/avic.c            |  23 -------
 arch/x86/kvm/svm/svm.c             |  30 ---------
 arch/x86/kvm/svm/svm.h             |   1 -
 arch/x86/kvm/vmx/vmx.c             |   6 --
 arch/x86/kvm/x86.c                 |  20 ++----
 kernel/static_call.c               |   1 +
 9 files changed, 81 insertions(+), 142 deletions(-)

-- 
2.31.1

[PATCH v3 1/6] KVM: x86: return 1 unconditionally for availability of KVM_CAP_VAPIC

[Paolo Bonzini]

The two ioctl used to implement userspace-accelerated TPR,
KVM_TPR_ACCESS_REPORTING and KVM_SET_VAPIC_ADDR, are available
even if hardware-accelerated TPR can be used.  So there is
no reason not to report KVM_CAP_VAPIC.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/include/asm/kvm-x86-ops.h | 1 -
 arch/x86/include/asm/kvm_host.h    | 1 -
 arch/x86/kvm/svm/svm.c             | 6 ------
 arch/x86/kvm/vmx/vmx.c             | 6 ------
 arch/x86/kvm/x86.c                 | 4 +---
 5 files changed, 1 insertion(+), 17 deletions(-)

diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
index 9e37dc3d8863..695ed7feef7e 100644
--- a/arch/x86/include/asm/kvm-x86-ops.h
+++ b/arch/x86/include/asm/kvm-x86-ops.h
@@ -15,7 +15,6 @@ BUILD_BUG_ON(1)
 KVM_X86_OP_NULL(hardware_enable)
 KVM_X86_OP_NULL(hardware_disable)
 KVM_X86_OP_NULL(hardware_unsetup)
-KVM_X86_OP_NULL(cpu_has_accelerated_tpr)
 KVM_X86_OP(has_emulated_msr)
 KVM_X86_OP(vcpu_after_set_cpuid)
 KVM_X86_OP(vm_init)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 10815b672a26..e0d2cdfe54ab 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1318,7 +1318,6 @@ struct kvm_x86_ops {
 	int (*hardware_enable)(void);
 	void (*hardware_disable)(void);
 	void (*hardware_unsetup)(void);
-	bool (*cpu_has_accelerated_tpr)(void);
 	bool (*has_emulated_msr)(struct kvm *kvm, u32 index);
 	void (*vcpu_after_set_cpuid)(struct kvm_vcpu *vcpu);
 
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 4243bb355db0..abced3fe2013 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3912,11 +3912,6 @@ static int __init svm_check_processor_compat(void)
 	return 0;
 }
 
-static bool svm_cpu_has_accelerated_tpr(void)
-{
-	return false;
-}
-
 /*
  * The kvm parameter can be NULL (module initialization, or invocation before
  * VM creation). Be sure to check the kvm parameter before using it.
@@ -4529,7 +4524,6 @@ static struct kvm_x86_ops svm_x86_ops __initdata = {
 	.hardware_unsetup = svm_hardware_unsetup,
 	.hardware_enable = svm_hardware_enable,
 	.hardware_disable = svm_hardware_disable,
-	.cpu_has_accelerated_tpr = svm_cpu_has_accelerated_tpr,
 	.has_emulated_msr = svm_has_emulated_msr,
 
 	.vcpu_create = svm_vcpu_create,
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 70e7f00362bc..d8547144d3b7 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -541,11 +541,6 @@ static inline bool cpu_need_virtualize_apic_accesses(struct kvm_vcpu *vcpu)
 	return flexpriority_enabled && lapic_in_kernel(vcpu);
 }
 
-static inline bool vmx_cpu_has_accelerated_tpr(void)
-{
-	return flexpriority_enabled;
-}
-
 static int possible_passthrough_msr_slot(u32 msr)
 {
 	u32 i;
@@ -7714,7 +7709,6 @@ static struct kvm_x86_ops vmx_x86_ops __initdata = {
 
 	.hardware_enable = vmx_hardware_enable,
 	.hardware_disable = vmx_hardware_disable,
-	.cpu_has_accelerated_tpr = vmx_cpu_has_accelerated_tpr,
 	.has_emulated_msr = vmx_has_emulated_msr,
 
 	.vm_size = sizeof(struct kvm_vmx),
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index eaa3b5b89c5e..746f72ae2c95 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4234,6 +4234,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 	case KVM_CAP_EXIT_ON_EMULATION_FAILURE:
 	case KVM_CAP_VCPU_ATTRIBUTES:
 	case KVM_CAP_SYS_ATTRIBUTES:
+	case KVM_CAP_VAPIC:
 		r = 1;
 		break;
 	case KVM_CAP_EXIT_HYPERCALL:
@@ -4274,9 +4275,6 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 		 */
 		r = static_call(kvm_x86_has_emulated_msr)(kvm, MSR_IA32_SMBASE);
 		break;
-	case KVM_CAP_VAPIC:
-		r = !static_call(kvm_x86_cpu_has_accelerated_tpr)();
-		break;
 	case KVM_CAP_NR_VCPUS:
 		r = min_t(unsigned int, num_online_cpus(), KVM_MAX_VCPUS);
 		break;
-- 
2.31.1

Re: [PATCH v3 1/6] KVM: x86: return 1 unconditionally for availability of KVM_CAP_VAPIC

[Sean Christopherson]

On Thu, Feb 17, 2022, Paolo Bonzini wrote:
> The two ioctl used to implement userspace-accelerated TPR,

Hmm, are ioctl like LEGO?

> KVM_TPR_ACCESS_REPORTING and KVM_SET_VAPIC_ADDR, are available
> even if hardware-accelerated TPR can be used.  So there is
> no reason not to report KVM_CAP_VAPIC.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---

Reviewed-by: Sean Christopherson <seanjc@google.com>

Re: [PATCH v3 1/6] KVM: x86: return 1 unconditionally for availability of KVM_CAP_VAPIC

[Maxim Levitsky]

On Thu, 2022-02-17 at 13:08 -0500, Paolo Bonzini wrote:
> The two ioctl used to implement userspace-accelerated TPR,
> KVM_TPR_ACCESS_REPORTING and KVM_SET_VAPIC_ADDR, are available
> even if hardware-accelerated TPR can be used.  So there is
> no reason not to report KVM_CAP_VAPIC.

Just my 0.2 cents - some time ago I did some archeological digging on 
this feature:

with AVIC, read/writes to TPR register will not #VMEXIT, thus KVM_TPR_ACCESS_REPORTING
won't work.

On SVM, CR8 writes when intercepted don't go through kvm_lapic_reg_write thus won't
be reported as well, which might even be intentional since guest which uses CR8,
is not supposed to use the MMIO.

In fact AMD's manual suggests that even 32 bit guests should use CR8 to access TPR,
and provides a special optcode to do so.
I assume that is because SVM lacks the TPR threshold feature.


On VMX side of things, I also think that TPR writes are not trapped when APICv is enabled,
and when APICv is not enabled, TPR access can be trapped conditionally using the TPR
threshold feature.

Another thing to note is that the feature needs an optional rom in the guest
and it seems not to be loaded on uefi bios.

Nothing against this patch though - in fact looks like qemu doesn't check the
KVM_CAP_VAPIC at all.

Best regards,
	Maxim Levitsky

> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/include/asm/kvm-x86-ops.h | 1 -
>  arch/x86/include/asm/kvm_host.h    | 1 -
>  arch/x86/kvm/svm/svm.c             | 6 ------
>  arch/x86/kvm/vmx/vmx.c             | 6 ------
>  arch/x86/kvm/x86.c                 | 4 +---
>  5 files changed, 1 insertion(+), 17 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
> index 9e37dc3d8863..695ed7feef7e 100644
> --- a/arch/x86/include/asm/kvm-x86-ops.h
> +++ b/arch/x86/include/asm/kvm-x86-ops.h
> @@ -15,7 +15,6 @@ BUILD_BUG_ON(1)
>  KVM_X86_OP_NULL(hardware_enable)
>  KVM_X86_OP_NULL(hardware_disable)
>  KVM_X86_OP_NULL(hardware_unsetup)
> -KVM_X86_OP_NULL(cpu_has_accelerated_tpr)
>  KVM_X86_OP(has_emulated_msr)
>  KVM_X86_OP(vcpu_after_set_cpuid)
>  KVM_X86_OP(vm_init)
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 10815b672a26..e0d2cdfe54ab 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1318,7 +1318,6 @@ struct kvm_x86_ops {
>  	int (*hardware_enable)(void);
>  	void (*hardware_disable)(void);
>  	void (*hardware_unsetup)(void);
> -	bool (*cpu_has_accelerated_tpr)(void);
>  	bool (*has_emulated_msr)(struct kvm *kvm, u32 index);
>  	void (*vcpu_after_set_cpuid)(struct kvm_vcpu *vcpu);
>  
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 4243bb355db0..abced3fe2013 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -3912,11 +3912,6 @@ static int __init svm_check_processor_compat(void)
>  	return 0;
>  }
>  
> -static bool svm_cpu_has_accelerated_tpr(void)
> -{
> -	return false;
> -}
> -
>  /*
>   * The kvm parameter can be NULL (module initialization, or invocation before
>   * VM creation). Be sure to check the kvm parameter before using it.
> @@ -4529,7 +4524,6 @@ static struct kvm_x86_ops svm_x86_ops __initdata = {
>  	.hardware_unsetup = svm_hardware_unsetup,
>  	.hardware_enable = svm_hardware_enable,
>  	.hardware_disable = svm_hardware_disable,
> -	.cpu_has_accelerated_tpr = svm_cpu_has_accelerated_tpr,
>  	.has_emulated_msr = svm_has_emulated_msr,
>  
>  	.vcpu_create = svm_vcpu_create,
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 70e7f00362bc..d8547144d3b7 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -541,11 +541,6 @@ static inline bool cpu_need_virtualize_apic_accesses(struct kvm_vcpu *vcpu)
>  	return flexpriority_enabled && lapic_in_kernel(vcpu);
>  }
>  
> -static inline bool vmx_cpu_has_accelerated_tpr(void)
> -{
> -	return flexpriority_enabled;
> -}
> -
>  static int possible_passthrough_msr_slot(u32 msr)
>  {
>  	u32 i;
> @@ -7714,7 +7709,6 @@ static struct kvm_x86_ops vmx_x86_ops __initdata = {
>  
>  	.hardware_enable = vmx_hardware_enable,
>  	.hardware_disable = vmx_hardware_disable,
> -	.cpu_has_accelerated_tpr = vmx_cpu_has_accelerated_tpr,
>  	.has_emulated_msr = vmx_has_emulated_msr,
>  
>  	.vm_size = sizeof(struct kvm_vmx),
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index eaa3b5b89c5e..746f72ae2c95 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4234,6 +4234,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>  	case KVM_CAP_EXIT_ON_EMULATION_FAILURE:
>  	case KVM_CAP_VCPU_ATTRIBUTES:
>  	case KVM_CAP_SYS_ATTRIBUTES:
> +	case KVM_CAP_VAPIC:
>  		r = 1;
>  		break;
>  	case KVM_CAP_EXIT_HYPERCALL:
> @@ -4274,9 +4275,6 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>  		 */
>  		r = static_call(kvm_x86_has_emulated_msr)(kvm, MSR_IA32_SMBASE);
>  		break;
> -	case KVM_CAP_VAPIC:
> -		r = !static_call(kvm_x86_cpu_has_accelerated_tpr)();
> -		break;
>  	case KVM_CAP_NR_VCPUS:
>  		r = min_t(unsigned int, num_online_cpus(), KVM_MAX_VCPUS);
>  		break;

[PATCH v3 2/6] KVM: x86: use static_call_cond for optional callbacks

[Paolo Bonzini]

SVM implements neither update_emulated_instruction nor
set_apic_access_page_addr.  Remove an "if" by calling them
with static_call_cond().

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/x86.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 746f72ae2c95..8a7f32563590 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8421,8 +8421,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
 			kvm_rip_write(vcpu, ctxt->eip);
 			if (r && (ctxt->tf || (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)))
 				r = kvm_vcpu_do_singlestep(vcpu);
-			if (kvm_x86_ops.update_emulated_instruction)
-				static_call(kvm_x86_update_emulated_instruction)(vcpu);
+			static_call_cond(kvm_x86_update_emulated_instruction)(vcpu);
 			__kvm_set_rflags(vcpu, ctxt->eflags);
 		}
 
@@ -9791,10 +9790,7 @@ static void kvm_vcpu_reload_apic_access_page(struct kvm_vcpu *vcpu)
 	if (!lapic_in_kernel(vcpu))
 		return;
 
-	if (!kvm_x86_ops.set_apic_access_page_addr)
-		return;
-
-	static_call(kvm_x86_set_apic_access_page_addr)(vcpu);
+	static_call_cond(kvm_x86_set_apic_access_page_addr)(vcpu);
 }
 
 void __kvm_request_immediate_exit(struct kvm_vcpu *vcpu)
-- 
2.31.1

Re: [PATCH v3 2/6] KVM: x86: use static_call_cond for optional callbacks

[Sean Christopherson]

On Thu, Feb 17, 2022, Paolo Bonzini wrote:
> SVM implements neither update_emulated_instruction nor
> set_apic_access_page_addr.  Remove an "if" by calling them
> with static_call_cond().
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---

Reviewed-by: Sean Christopherson <seanjc@google.com>

[PATCH v3 3/6] KVM: x86: remove KVM_X86_OP_NULL and mark optional kvm_x86_ops

[Paolo Bonzini]

The original use of KVM_X86_OP_NULL, which was to mark calls
that do not follow a specific naming convention, is not in use
anymore.  Instead, let's mark calls that are optional because
they are always invoked within conditionals or with static_call_cond.
Those that are _not_, i.e. those that are defined with KVM_X86_OP,
must be defined by both vendor modules or some kind of NULL pointer
dereference is bound to happen at runtime.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/include/asm/kvm-x86-ops.h | 86 +++++++++++++++---------------
 arch/x86/include/asm/kvm_host.h    |  4 +-
 arch/x86/kvm/x86.c                 |  2 +-
 3 files changed, 46 insertions(+), 46 deletions(-)

diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
index 695ed7feef7e..5e3296c07207 100644
--- a/arch/x86/include/asm/kvm-x86-ops.h
+++ b/arch/x86/include/asm/kvm-x86-ops.h
@@ -1,24 +1,24 @@
 /* SPDX-License-Identifier: GPL-2.0 */
-#if !defined(KVM_X86_OP) || !defined(KVM_X86_OP_NULL)
+#if !defined(KVM_X86_OP) || !defined(KVM_X86_OP_OPTIONAL)
 BUILD_BUG_ON(1)
 #endif
 
 /*
- * KVM_X86_OP() and KVM_X86_OP_NULL() are used to help generate
- * "static_call()"s. They are also intended for use when defining
- * the vmx/svm kvm_x86_ops. KVM_X86_OP() can be used for those
- * functions that follow the [svm|vmx]_func_name convention.
- * KVM_X86_OP_NULL() can leave a NULL definition for the
- * case where there is no definition or a function name that
- * doesn't match the typical naming convention is supplied.
+ * KVM_X86_OP() and KVM_X86_OP_OPTIONAL() are used to help generate
+ * both DECLARE/DEFINE_STATIC_CALL() invocations and
+ * "static_call_update()" calls.
+ *
+ * KVM_X86_OP_OPTIONAL() can be used for those functions that can have
+ * a NULL definition, for example if "static_call_cond()" will be used
+ * at the call sites.
  */
-KVM_X86_OP_NULL(hardware_enable)
-KVM_X86_OP_NULL(hardware_disable)
-KVM_X86_OP_NULL(hardware_unsetup)
+KVM_X86_OP(hardware_enable)
+KVM_X86_OP(hardware_disable)
+KVM_X86_OP(hardware_unsetup)
 KVM_X86_OP(has_emulated_msr)
 KVM_X86_OP(vcpu_after_set_cpuid)
 KVM_X86_OP(vm_init)
-KVM_X86_OP_NULL(vm_destroy)
+KVM_X86_OP_OPTIONAL(vm_destroy)
 KVM_X86_OP(vcpu_create)
 KVM_X86_OP(vcpu_free)
 KVM_X86_OP(vcpu_reset)
@@ -32,9 +32,9 @@ KVM_X86_OP(get_segment_base)
 KVM_X86_OP(get_segment)
 KVM_X86_OP(get_cpl)
 KVM_X86_OP(set_segment)
-KVM_X86_OP_NULL(get_cs_db_l_bits)
+KVM_X86_OP(get_cs_db_l_bits)
 KVM_X86_OP(set_cr0)
-KVM_X86_OP_NULL(post_set_cr3)
+KVM_X86_OP_OPTIONAL(post_set_cr3)
 KVM_X86_OP(is_valid_cr4)
 KVM_X86_OP(set_cr4)
 KVM_X86_OP(set_efer)
@@ -50,15 +50,15 @@ KVM_X86_OP(set_rflags)
 KVM_X86_OP(get_if_flag)
 KVM_X86_OP(flush_tlb_all)
 KVM_X86_OP(flush_tlb_current)
-KVM_X86_OP_NULL(tlb_remote_flush)
-KVM_X86_OP_NULL(tlb_remote_flush_with_range)
+KVM_X86_OP_OPTIONAL(tlb_remote_flush)
+KVM_X86_OP_OPTIONAL(tlb_remote_flush_with_range)
 KVM_X86_OP(flush_tlb_gva)
 KVM_X86_OP(flush_tlb_guest)
 KVM_X86_OP(vcpu_pre_run)
 KVM_X86_OP(vcpu_run)
-KVM_X86_OP_NULL(handle_exit)
-KVM_X86_OP_NULL(skip_emulated_instruction)
-KVM_X86_OP_NULL(update_emulated_instruction)
+KVM_X86_OP(handle_exit)
+KVM_X86_OP(skip_emulated_instruction)
+KVM_X86_OP_OPTIONAL(update_emulated_instruction)
 KVM_X86_OP(set_interrupt_shadow)
 KVM_X86_OP(get_interrupt_shadow)
 KVM_X86_OP(patch_hypercall)
@@ -72,22 +72,22 @@ KVM_X86_OP(get_nmi_mask)
 KVM_X86_OP(set_nmi_mask)
 KVM_X86_OP(enable_nmi_window)
 KVM_X86_OP(enable_irq_window)
-KVM_X86_OP(update_cr8_intercept)
+KVM_X86_OP_OPTIONAL(update_cr8_intercept)
 KVM_X86_OP(check_apicv_inhibit_reasons)
 KVM_X86_OP(refresh_apicv_exec_ctrl)
 KVM_X86_OP(hwapic_irr_update)
 KVM_X86_OP(hwapic_isr_update)
-KVM_X86_OP_NULL(guest_apic_has_interrupt)
+KVM_X86_OP_OPTIONAL(guest_apic_has_interrupt)
 KVM_X86_OP(load_eoi_exitmap)
 KVM_X86_OP(set_virtual_apic_mode)
-KVM_X86_OP_NULL(set_apic_access_page_addr)
+KVM_X86_OP_OPTIONAL(set_apic_access_page_addr)
 KVM_X86_OP(deliver_interrupt)
-KVM_X86_OP_NULL(sync_pir_to_irr)
+KVM_X86_OP_OPTIONAL(sync_pir_to_irr)
 KVM_X86_OP(set_tss_addr)
 KVM_X86_OP(set_identity_map_addr)
 KVM_X86_OP(get_mt_mask)
 KVM_X86_OP(load_mmu_pgd)
-KVM_X86_OP_NULL(has_wbinvd_exit)
+KVM_X86_OP(has_wbinvd_exit)
 KVM_X86_OP(get_l2_tsc_offset)
 KVM_X86_OP(get_l2_tsc_multiplier)
 KVM_X86_OP(write_tsc_offset)
@@ -95,35 +95,35 @@ KVM_X86_OP(write_tsc_multiplier)
 KVM_X86_OP(get_exit_info)
 KVM_X86_OP(check_intercept)
 KVM_X86_OP(handle_exit_irqoff)
-KVM_X86_OP_NULL(request_immediate_exit)
+KVM_X86_OP(request_immediate_exit)
 KVM_X86_OP(sched_in)
-KVM_X86_OP_NULL(update_cpu_dirty_logging)
-KVM_X86_OP_NULL(vcpu_blocking)
-KVM_X86_OP_NULL(vcpu_unblocking)
-KVM_X86_OP_NULL(pi_update_irte)
-KVM_X86_OP_NULL(pi_start_assignment)
-KVM_X86_OP_NULL(apicv_post_state_restore)
-KVM_X86_OP_NULL(dy_apicv_has_pending_interrupt)
-KVM_X86_OP_NULL(set_hv_timer)
-KVM_X86_OP_NULL(cancel_hv_timer)
+KVM_X86_OP_OPTIONAL(update_cpu_dirty_logging)
+KVM_X86_OP_OPTIONAL(vcpu_blocking)
+KVM_X86_OP_OPTIONAL(vcpu_unblocking)
+KVM_X86_OP_OPTIONAL(pi_update_irte)
+KVM_X86_OP_OPTIONAL(pi_start_assignment)
+KVM_X86_OP(apicv_post_state_restore)
+KVM_X86_OP_OPTIONAL(dy_apicv_has_pending_interrupt)
+KVM_X86_OP_OPTIONAL(set_hv_timer)
+KVM_X86_OP_OPTIONAL(cancel_hv_timer)
 KVM_X86_OP(setup_mce)
 KVM_X86_OP(smi_allowed)
 KVM_X86_OP(enter_smm)
 KVM_X86_OP(leave_smm)
 KVM_X86_OP(enable_smi_window)
-KVM_X86_OP_NULL(mem_enc_ioctl)
-KVM_X86_OP_NULL(mem_enc_register_region)
-KVM_X86_OP_NULL(mem_enc_unregister_region)
-KVM_X86_OP_NULL(vm_copy_enc_context_from)
-KVM_X86_OP_NULL(vm_move_enc_context_from)
+KVM_X86_OP_OPTIONAL(mem_enc_ioctl)
+KVM_X86_OP_OPTIONAL(mem_enc_register_region)
+KVM_X86_OP_OPTIONAL(mem_enc_unregister_region)
+KVM_X86_OP_OPTIONAL(vm_copy_enc_context_from)
+KVM_X86_OP_OPTIONAL(vm_move_enc_context_from)
 KVM_X86_OP(get_msr_feature)
 KVM_X86_OP(can_emulate_instruction)
 KVM_X86_OP(apic_init_signal_blocked)
-KVM_X86_OP_NULL(enable_direct_tlbflush)
-KVM_X86_OP_NULL(migrate_timers)
+KVM_X86_OP_OPTIONAL(enable_direct_tlbflush)
+KVM_X86_OP_OPTIONAL(migrate_timers)
 KVM_X86_OP(msr_filter_changed)
-KVM_X86_OP_NULL(complete_emulated_msr)
+KVM_X86_OP(complete_emulated_msr)
 KVM_X86_OP(vcpu_deliver_sipi_vector)
 
 #undef KVM_X86_OP
-#undef KVM_X86_OP_NULL
+#undef KVM_X86_OP_OPTIONAL
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index e0d2cdfe54ab..7d733f601106 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1540,14 +1540,14 @@ extern struct kvm_x86_ops kvm_x86_ops;
 
 #define KVM_X86_OP(func) \
 	DECLARE_STATIC_CALL(kvm_x86_##func, *(((struct kvm_x86_ops *)0)->func));
-#define KVM_X86_OP_NULL KVM_X86_OP
+#define KVM_X86_OP_OPTIONAL KVM_X86_OP
 #include <asm/kvm-x86-ops.h>
 
 static inline void kvm_ops_static_call_update(void)
 {
 #define KVM_X86_OP(func) \
 	static_call_update(kvm_x86_##func, kvm_x86_ops.func);
-#define KVM_X86_OP_NULL KVM_X86_OP
+#define KVM_X86_OP_OPTIONAL KVM_X86_OP
 #include <asm/kvm-x86-ops.h>
 }
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 8a7f32563590..c3d44e6a3454 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -130,7 +130,7 @@ struct kvm_x86_ops kvm_x86_ops __read_mostly;
 #define KVM_X86_OP(func)					     \
 	DEFINE_STATIC_CALL_NULL(kvm_x86_##func,			     \
 				*(((struct kvm_x86_ops *)0)->func));
-#define KVM_X86_OP_NULL KVM_X86_OP
+#define KVM_X86_OP_OPTIONAL KVM_X86_OP
 #include <asm/kvm-x86-ops.h>
 EXPORT_STATIC_CALL_GPL(kvm_x86_get_cs_db_l_bits);
 EXPORT_STATIC_CALL_GPL(kvm_x86_cache_reg);
-- 
2.31.1

Re: [PATCH v3 3/6] KVM: x86: remove KVM_X86_OP_NULL and mark optional kvm_x86_ops

[Sean Christopherson]

On Thu, Feb 17, 2022, Paolo Bonzini wrote:
> The original use of KVM_X86_OP_NULL, which was to mark calls
> that do not follow a specific naming convention, is not in use
> anymore.  Instead, let's mark calls that are optional because
> they are always invoked within conditionals or with static_call_cond.
> Those that are _not_, i.e. those that are defined with KVM_X86_OP,
> must be defined by both vendor modules or some kind of NULL pointer
> dereference is bound to happen at runtime.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---

Reviewed-by: Sean Christopherson <seanjc@google.com>

[PATCH v3 4/6] KVM: x86: warn on incorrectly NULL members of kvm_x86_ops

[Paolo Bonzini]

Use the newly corrected KVM_X86_OP annotations to warn about possible
NULL pointer dereferences as soon as the vendor module is loaded.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/include/asm/kvm_host.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 7d733f601106..a7e82fc1f1f3 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1545,10 +1545,13 @@ extern struct kvm_x86_ops kvm_x86_ops;
 
 static inline void kvm_ops_static_call_update(void)
 {
-#define KVM_X86_OP(func) \
+#define __KVM_X86_OP(func) \
 	static_call_update(kvm_x86_##func, kvm_x86_ops.func);
-#define KVM_X86_OP_OPTIONAL KVM_X86_OP
+#define KVM_X86_OP(func) \
+	WARN_ON(!kvm_x86_ops.func); __KVM_X86_OP(func)
+#define KVM_X86_OP_OPTIONAL __KVM_X86_OP
 #include <asm/kvm-x86-ops.h>
+#undef __KVM_X86_OP
 }
 
 #define __KVM_HAVE_ARCH_VM_ALLOC
-- 
2.31.1

Re: [PATCH v3 4/6] KVM: x86: warn on incorrectly NULL members of kvm_x86_ops

[Sean Christopherson]

On Thu, Feb 17, 2022, Paolo Bonzini wrote:
> Use the newly corrected KVM_X86_OP annotations to warn about possible
> NULL pointer dereferences as soon as the vendor module is loaded.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---

Reviewed-by: Sean Christopherson <seanjc@google.com>

[PATCH v3 5/6] KVM: x86: make several AVIC callbacks optional

[Paolo Bonzini]

SVM does not need them, so mark them as optional and delete the
implementation.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/include/asm/kvm-x86-ops.h | 10 +++++-----
 arch/x86/kvm/lapic.c               | 24 ++++++++++--------------
 arch/x86/kvm/svm/avic.c            | 18 ------------------
 arch/x86/kvm/svm/svm.c             |  4 ----
 arch/x86/kvm/svm/svm.h             |  1 -
 arch/x86/kvm/x86.c                 |  4 ++--
 6 files changed, 17 insertions(+), 44 deletions(-)

diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
index 5e3296c07207..c0ec066a8599 100644
--- a/arch/x86/include/asm/kvm-x86-ops.h
+++ b/arch/x86/include/asm/kvm-x86-ops.h
@@ -75,11 +75,11 @@ KVM_X86_OP(enable_irq_window)
 KVM_X86_OP_OPTIONAL(update_cr8_intercept)
 KVM_X86_OP(check_apicv_inhibit_reasons)
 KVM_X86_OP(refresh_apicv_exec_ctrl)
-KVM_X86_OP(hwapic_irr_update)
-KVM_X86_OP(hwapic_isr_update)
+KVM_X86_OP_OPTIONAL(hwapic_irr_update)
+KVM_X86_OP_OPTIONAL(hwapic_isr_update)
 KVM_X86_OP_OPTIONAL(guest_apic_has_interrupt)
-KVM_X86_OP(load_eoi_exitmap)
-KVM_X86_OP(set_virtual_apic_mode)
+KVM_X86_OP_OPTIONAL(load_eoi_exitmap)
+KVM_X86_OP_OPTIONAL(set_virtual_apic_mode)
 KVM_X86_OP_OPTIONAL(set_apic_access_page_addr)
 KVM_X86_OP(deliver_interrupt)
 KVM_X86_OP_OPTIONAL(sync_pir_to_irr)
@@ -102,7 +102,7 @@ KVM_X86_OP_OPTIONAL(vcpu_blocking)
 KVM_X86_OP_OPTIONAL(vcpu_unblocking)
 KVM_X86_OP_OPTIONAL(pi_update_irte)
 KVM_X86_OP_OPTIONAL(pi_start_assignment)
-KVM_X86_OP(apicv_post_state_restore)
+KVM_X86_OP_OPTIONAL(apicv_post_state_restore)
 KVM_X86_OP_OPTIONAL(dy_apicv_has_pending_interrupt)
 KVM_X86_OP_OPTIONAL(set_hv_timer)
 KVM_X86_OP_OPTIONAL(cancel_hv_timer)
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index dd4e2888c244..47f8606559a9 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -492,8 +492,7 @@ static inline void apic_clear_irr(int vec, struct kvm_lapic *apic)
 	if (unlikely(vcpu->arch.apicv_active)) {
 		/* need to update RVI */
 		kvm_lapic_clear_vector(vec, apic->regs + APIC_IRR);
-		static_call(kvm_x86_hwapic_irr_update)(vcpu,
-				apic_find_highest_irr(apic));
+		static_call_cond(kvm_x86_hwapic_irr_update)(vcpu, apic_find_highest_irr(apic));
 	} else {
 		apic->irr_pending = false;
 		kvm_lapic_clear_vector(vec, apic->regs + APIC_IRR);
@@ -523,7 +522,7 @@ static inline void apic_set_isr(int vec, struct kvm_lapic *apic)
 	 * just set SVI.
 	 */
 	if (unlikely(vcpu->arch.apicv_active))
-		static_call(kvm_x86_hwapic_isr_update)(vcpu, vec);
+		static_call_cond(kvm_x86_hwapic_isr_update)(vcpu, vec);
 	else {
 		++apic->isr_count;
 		BUG_ON(apic->isr_count > MAX_APIC_VECTOR);
@@ -571,8 +570,7 @@ static inline void apic_clear_isr(int vec, struct kvm_lapic *apic)
 	 * and must be left alone.
 	 */
 	if (unlikely(vcpu->arch.apicv_active))
-		static_call(kvm_x86_hwapic_isr_update)(vcpu,
-						apic_find_highest_isr(apic));
+		static_call_cond(kvm_x86_hwapic_isr_update)(vcpu, apic_find_highest_isr(apic));
 	else {
 		--apic->isr_count;
 		BUG_ON(apic->isr_count < 0);
@@ -2288,7 +2286,7 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value)
 		kvm_apic_set_x2apic_id(apic, vcpu->vcpu_id);
 
 	if ((old_value ^ value) & (MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE))
-		static_call(kvm_x86_set_virtual_apic_mode)(vcpu);
+		static_call_cond(kvm_x86_set_virtual_apic_mode)(vcpu);
 
 	apic->base_address = apic->vcpu->arch.apic_base &
 			     MSR_IA32_APICBASE_BASE;
@@ -2374,9 +2372,9 @@ void kvm_lapic_reset(struct kvm_vcpu *vcpu, bool init_event)
 	vcpu->arch.pv_eoi.msr_val = 0;
 	apic_update_ppr(apic);
 	if (vcpu->arch.apicv_active) {
-		static_call(kvm_x86_apicv_post_state_restore)(vcpu);
-		static_call(kvm_x86_hwapic_irr_update)(vcpu, -1);
-		static_call(kvm_x86_hwapic_isr_update)(vcpu, -1);
+		static_call_cond(kvm_x86_apicv_post_state_restore)(vcpu);
+		static_call_cond(kvm_x86_hwapic_irr_update)(vcpu, -1);
+		static_call_cond(kvm_x86_hwapic_isr_update)(vcpu, -1);
 	}
 
 	vcpu->arch.apic_arb_prio = 0;
@@ -2639,11 +2637,9 @@ int kvm_apic_set_state(struct kvm_vcpu *vcpu, struct kvm_lapic_state *s)
 	kvm_apic_update_apicv(vcpu);
 	apic->highest_isr_cache = -1;
 	if (vcpu->arch.apicv_active) {
-		static_call(kvm_x86_apicv_post_state_restore)(vcpu);
-		static_call(kvm_x86_hwapic_irr_update)(vcpu,
-				apic_find_highest_irr(apic));
-		static_call(kvm_x86_hwapic_isr_update)(vcpu,
-				apic_find_highest_isr(apic));
+		static_call_cond(kvm_x86_apicv_post_state_restore)(vcpu);
+		static_call_cond(kvm_x86_hwapic_irr_update)(vcpu, apic_find_highest_irr(apic));
+		static_call_cond(kvm_x86_hwapic_isr_update)(vcpu, apic_find_highest_isr(apic));
 	}
 	kvm_make_request(KVM_REQ_EVENT, vcpu);
 	if (ioapic_in_kernel(vcpu->kvm))
diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index abd0e664bf22..4245cb99b497 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -586,19 +586,6 @@ void avic_apicv_post_state_restore(struct kvm_vcpu *vcpu)
 	avic_handle_ldr_update(vcpu);
 }
 
-void avic_set_virtual_apic_mode(struct kvm_vcpu *vcpu)
-{
-	return;
-}
-
-void avic_hwapic_irr_update(struct kvm_vcpu *vcpu, int max_irr)
-{
-}
-
-void avic_hwapic_isr_update(struct kvm_vcpu *vcpu, int max_isr)
-{
-}
-
 static int avic_set_pi_irte_mode(struct kvm_vcpu *vcpu, bool activate)
 {
 	int ret = 0;
@@ -663,11 +650,6 @@ void avic_refresh_apicv_exec_ctrl(struct kvm_vcpu *vcpu)
 	avic_set_pi_irte_mode(vcpu, activated);
 }
 
-void avic_load_eoi_exitmap(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap)
-{
-	return;
-}
-
 bool avic_dy_apicv_has_pending_interrupt(struct kvm_vcpu *vcpu)
 {
 	return false;
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index abced3fe2013..3daca34020fa 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -4589,12 +4589,8 @@ static struct kvm_x86_ops svm_x86_ops __initdata = {
 	.enable_nmi_window = svm_enable_nmi_window,
 	.enable_irq_window = svm_enable_irq_window,
 	.update_cr8_intercept = svm_update_cr8_intercept,
-	.set_virtual_apic_mode = avic_set_virtual_apic_mode,
 	.refresh_apicv_exec_ctrl = avic_refresh_apicv_exec_ctrl,
 	.check_apicv_inhibit_reasons = avic_check_apicv_inhibit_reasons,
-	.load_eoi_exitmap = avic_load_eoi_exitmap,
-	.hwapic_irr_update = avic_hwapic_irr_update,
-	.hwapic_isr_update = avic_hwapic_isr_update,
 	.apicv_post_state_restore = avic_apicv_post_state_restore,
 
 	.set_tss_addr = svm_set_tss_addr,
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index dddcaa827c5f..70850cbe5bcb 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -582,7 +582,6 @@ void avic_apicv_post_state_restore(struct kvm_vcpu *vcpu);
 void avic_set_virtual_apic_mode(struct kvm_vcpu *vcpu);
 void avic_refresh_apicv_exec_ctrl(struct kvm_vcpu *vcpu);
 bool avic_check_apicv_inhibit_reasons(ulong bit);
-void avic_load_eoi_exitmap(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap);
 void avic_hwapic_irr_update(struct kvm_vcpu *vcpu, int max_irr);
 void avic_hwapic_isr_update(struct kvm_vcpu *vcpu, int max_isr);
 bool avic_dy_apicv_has_pending_interrupt(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c3d44e6a3454..ab1c4778824a 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -9763,11 +9763,11 @@ static void vcpu_load_eoi_exitmap(struct kvm_vcpu *vcpu)
 		bitmap_or((ulong *)eoi_exit_bitmap,
 			  vcpu->arch.ioapic_handled_vectors,
 			  to_hv_synic(vcpu)->vec_bitmap, 256);
-		static_call(kvm_x86_load_eoi_exitmap)(vcpu, eoi_exit_bitmap);
+		static_call_cond(kvm_x86_load_eoi_exitmap)(vcpu, eoi_exit_bitmap);
 		return;
 	}
 
-	static_call(kvm_x86_load_eoi_exitmap)(
+	static_call_cond(kvm_x86_load_eoi_exitmap)(
 		vcpu, (u64 *)vcpu->arch.ioapic_handled_vectors);
 }
 
-- 
2.31.1

Re: [PATCH v3 5/6] KVM: x86: make several AVIC callbacks optional

[Sean Christopherson]

Nit, s/AVIC/APICv in the shortlog.  The "AVIC" callbacks are being deleted, not
made optional, it's kvm_x86_ops' APICv hooks that are becoming optional.

On Thu, Feb 17, 2022, Paolo Bonzini wrote:
> SVM does not need them, so mark them as optional and delete the
> implementation.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/include/asm/kvm-x86-ops.h | 10 +++++-----
>  arch/x86/kvm/lapic.c               | 24 ++++++++++--------------
>  arch/x86/kvm/svm/avic.c            | 18 ------------------
>  arch/x86/kvm/svm/svm.c             |  4 ----
>  arch/x86/kvm/svm/svm.h             |  1 -
>  arch/x86/kvm/x86.c                 |  4 ++--
>  6 files changed, 17 insertions(+), 44 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
> index 5e3296c07207..c0ec066a8599 100644
> --- a/arch/x86/include/asm/kvm-x86-ops.h
> +++ b/arch/x86/include/asm/kvm-x86-ops.h
> @@ -75,11 +75,11 @@ KVM_X86_OP(enable_irq_window)
>  KVM_X86_OP_OPTIONAL(update_cr8_intercept)
>  KVM_X86_OP(check_apicv_inhibit_reasons)
>  KVM_X86_OP(refresh_apicv_exec_ctrl)
> -KVM_X86_OP(hwapic_irr_update)
> -KVM_X86_OP(hwapic_isr_update)
> +KVM_X86_OP_OPTIONAL(hwapic_irr_update)
> +KVM_X86_OP_OPTIONAL(hwapic_isr_update)
>  KVM_X86_OP_OPTIONAL(guest_apic_has_interrupt)
> -KVM_X86_OP(load_eoi_exitmap)
> -KVM_X86_OP(set_virtual_apic_mode)
> +KVM_X86_OP_OPTIONAL(load_eoi_exitmap)
> +KVM_X86_OP_OPTIONAL(set_virtual_apic_mode)
>  KVM_X86_OP_OPTIONAL(set_apic_access_page_addr)
>  KVM_X86_OP(deliver_interrupt)
>  KVM_X86_OP_OPTIONAL(sync_pir_to_irr)
> @@ -102,7 +102,7 @@ KVM_X86_OP_OPTIONAL(vcpu_blocking)
>  KVM_X86_OP_OPTIONAL(vcpu_unblocking)
>  KVM_X86_OP_OPTIONAL(pi_update_irte)
>  KVM_X86_OP_OPTIONAL(pi_start_assignment)
> -KVM_X86_OP(apicv_post_state_restore)
> +KVM_X86_OP_OPTIONAL(apicv_post_state_restore)

apicv_post_state_restore() isn't conditional, it's implemented and wired up
unconditionally by both VMX and SVM.

Re: [PATCH v3 5/6] KVM: x86: make several AVIC callbacks optional

[Paolo Bonzini]

On 2/18/22 17:23, Sean Christopherson wrote:
> The "AVIC" callbacks are being deleted, not
> made optional, it's kvm_x86_ops' APICv hooks that are becoming optional.

Maybe "make several APIC virtualization callbacks optional".

>> +KVM_X86_OP_OPTIONAL(apicv_post_state_restore)
>
> apicv_post_state_restore() isn't conditional, it's implemented and wired up
> unconditionally by both VMX and SVM.

True, on the other hand there's no reason why a hypothetical third 
vendor would have to support it.  The call is conditional to 
apicv_active being true.

Paolo

Re: [PATCH v3 5/6] KVM: x86: make several AVIC callbacks optional

[Sean Christopherson]

On Fri, Feb 18, 2022, Paolo Bonzini wrote:
> On 2/18/22 17:23, Sean Christopherson wrote:
> > The "AVIC" callbacks are being deleted, not
> > made optional, it's kvm_x86_ops' APICv hooks that are becoming optional.
> 
> Maybe "make several APIC virtualization callbacks optional".

Works for me.

> > > +KVM_X86_OP_OPTIONAL(apicv_post_state_restore)
> > 
> > apicv_post_state_restore() isn't conditional, it's implemented and wired up
> > unconditionally by both VMX and SVM.
> 
> True, on the other hand there's no reason why a hypothetical third vendor
> would have to support it.  The call is conditional to apicv_active being
> true.

Ah, right, even if the the static_call_cond() is unnecessary because we want to
make the hook mandatory if APICv is supported, APICv itself may not be supported.

With the new shortlog,

Reviewed-by: Sean Christopherson <seanjc@google.com>

Re: [PATCH v3 5/6] KVM: x86: make several AVIC callbacks optional

[Paolo Bonzini]

On 2/18/22 18:37, Sean Christopherson wrote:
>> True, on the other hand there's no reason why a hypothetical third vendor
>> would have to support it.  The call is conditional to apicv_active being
>> true.
> Ah, right, even if the the static_call_cond() is unnecessary because we want to
> make the hook mandatory if APICv is supported, APICv itself may not be supported.

I'm not even sure we want to make it mandatory, in fact.

Also new full log:

----
All their invocations are conditional on vcpu->arch.apicv_active,
meaning that they need not be implemented by vendor code: even
though at the moment both vendors implement APIC virtualization,
all of them should be optional.  In fact SVM does not need many of
them, and their implementation can be deleted now.
----

Paolo

[PATCH v3 6/6] KVM: x86: allow defining return-0 static calls

[Paolo Bonzini]

A few vendor callbacks are only used by VMX, but they return an integer
or bool value.  Introduce KVM_X86_OP_OPTIONAL_RET0 for them: if a func is
NULL in struct kvm_x86_ops, it will be changed to __static_call_return0
when updating static calls.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/include/asm/kvm-x86-ops.h | 15 +++++++++------
 arch/x86/include/asm/kvm_host.h    |  4 ++++
 arch/x86/kvm/svm/avic.c            |  5 -----
 arch/x86/kvm/svm/svm.c             | 20 --------------------
 arch/x86/kvm/x86.c                 |  2 +-
 kernel/static_call.c               |  1 +
 6 files changed, 15 insertions(+), 32 deletions(-)

diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
index c0ec066a8599..29affccb353c 100644
--- a/arch/x86/include/asm/kvm-x86-ops.h
+++ b/arch/x86/include/asm/kvm-x86-ops.h
@@ -10,7 +10,9 @@ BUILD_BUG_ON(1)
  *
  * KVM_X86_OP_OPTIONAL() can be used for those functions that can have
  * a NULL definition, for example if "static_call_cond()" will be used
- * at the call sites.
+ * at the call sites.  KVM_X86_OP_OPTIONAL_RET0() can be used likewise
+ * to make a definition optional, but in this case the default will
+ * be __static_call_return0.
  */
 KVM_X86_OP(hardware_enable)
 KVM_X86_OP(hardware_disable)
@@ -77,15 +79,15 @@ KVM_X86_OP(check_apicv_inhibit_reasons)
 KVM_X86_OP(refresh_apicv_exec_ctrl)
 KVM_X86_OP_OPTIONAL(hwapic_irr_update)
 KVM_X86_OP_OPTIONAL(hwapic_isr_update)
-KVM_X86_OP_OPTIONAL(guest_apic_has_interrupt)
+KVM_X86_OP_OPTIONAL_RET0(guest_apic_has_interrupt)
 KVM_X86_OP_OPTIONAL(load_eoi_exitmap)
 KVM_X86_OP_OPTIONAL(set_virtual_apic_mode)
 KVM_X86_OP_OPTIONAL(set_apic_access_page_addr)
 KVM_X86_OP(deliver_interrupt)
 KVM_X86_OP_OPTIONAL(sync_pir_to_irr)
-KVM_X86_OP(set_tss_addr)
-KVM_X86_OP(set_identity_map_addr)
-KVM_X86_OP(get_mt_mask)
+KVM_X86_OP_OPTIONAL_RET0(set_tss_addr)
+KVM_X86_OP_OPTIONAL_RET0(set_identity_map_addr)
+KVM_X86_OP_OPTIONAL_RET0(get_mt_mask)
 KVM_X86_OP(load_mmu_pgd)
 KVM_X86_OP(has_wbinvd_exit)
 KVM_X86_OP(get_l2_tsc_offset)
@@ -103,7 +105,7 @@ KVM_X86_OP_OPTIONAL(vcpu_unblocking)
 KVM_X86_OP_OPTIONAL(pi_update_irte)
 KVM_X86_OP_OPTIONAL(pi_start_assignment)
 KVM_X86_OP_OPTIONAL(apicv_post_state_restore)
-KVM_X86_OP_OPTIONAL(dy_apicv_has_pending_interrupt)
+KVM_X86_OP_OPTIONAL_RET0(dy_apicv_has_pending_interrupt)
 KVM_X86_OP_OPTIONAL(set_hv_timer)
 KVM_X86_OP_OPTIONAL(cancel_hv_timer)
 KVM_X86_OP(setup_mce)
@@ -127,3 +129,4 @@ KVM_X86_OP(vcpu_deliver_sipi_vector)
 
 #undef KVM_X86_OP
 #undef KVM_X86_OP_OPTIONAL
+#undef KVM_X86_OP_OPTIONAL_RET0
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index a7e82fc1f1f3..8e512f25a930 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1541,6 +1541,7 @@ extern struct kvm_x86_ops kvm_x86_ops;
 #define KVM_X86_OP(func) \
 	DECLARE_STATIC_CALL(kvm_x86_##func, *(((struct kvm_x86_ops *)0)->func));
 #define KVM_X86_OP_OPTIONAL KVM_X86_OP
+#define KVM_X86_OP_OPTIONAL_RET0 KVM_X86_OP
 #include <asm/kvm-x86-ops.h>
 
 static inline void kvm_ops_static_call_update(void)
@@ -1550,6 +1551,9 @@ static inline void kvm_ops_static_call_update(void)
 #define KVM_X86_OP(func) \
 	WARN_ON(!kvm_x86_ops.func); __KVM_X86_OP(func)
 #define KVM_X86_OP_OPTIONAL __KVM_X86_OP
+#define KVM_X86_OP_OPTIONAL_RET0(func) \
+	static_call_update(kvm_x86_##func, kvm_x86_ops.func ? : \
+			   (void *) __static_call_return0);
 #include <asm/kvm-x86-ops.h>
 #undef __KVM_X86_OP
 }
diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 4245cb99b497..d4fa8c4f3a9a 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -650,11 +650,6 @@ void avic_refresh_apicv_exec_ctrl(struct kvm_vcpu *vcpu)
 	avic_set_pi_irte_mode(vcpu, activated);
 }
 
-bool avic_dy_apicv_has_pending_interrupt(struct kvm_vcpu *vcpu)
-{
-	return false;
-}
-
 static void svm_ir_list_del(struct vcpu_svm *svm, struct amd_iommu_pi_data *pi)
 {
 	unsigned long flags;
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 3daca34020fa..7038c76fa841 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3528,16 +3528,6 @@ static void svm_enable_nmi_window(struct kvm_vcpu *vcpu)
 	svm->vmcb->save.rflags |= (X86_EFLAGS_TF | X86_EFLAGS_RF);
 }
 
-static int svm_set_tss_addr(struct kvm *kvm, unsigned int addr)
-{
-	return 0;
-}
-
-static int svm_set_identity_map_addr(struct kvm *kvm, u64 ident_addr)
-{
-	return 0;
-}
-
 static void svm_flush_tlb_current(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
@@ -3934,11 +3924,6 @@ static bool svm_has_emulated_msr(struct kvm *kvm, u32 index)
 	return true;
 }
 
-static u64 svm_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
-{
-	return 0;
-}
-
 static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
@@ -4593,10 +4578,6 @@ static struct kvm_x86_ops svm_x86_ops __initdata = {
 	.check_apicv_inhibit_reasons = avic_check_apicv_inhibit_reasons,
 	.apicv_post_state_restore = avic_apicv_post_state_restore,
 
-	.set_tss_addr = svm_set_tss_addr,
-	.set_identity_map_addr = svm_set_identity_map_addr,
-	.get_mt_mask = svm_get_mt_mask,
-
 	.get_exit_info = svm_get_exit_info,
 
 	.vcpu_after_set_cpuid = svm_vcpu_after_set_cpuid,
@@ -4621,7 +4602,6 @@ static struct kvm_x86_ops svm_x86_ops __initdata = {
 	.nested_ops = &svm_nested_ops,
 
 	.deliver_interrupt = svm_deliver_interrupt,
-	.dy_apicv_has_pending_interrupt = avic_dy_apicv_has_pending_interrupt,
 	.pi_update_irte = avic_pi_update_irte,
 	.setup_mce = svm_setup_mce,
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index ab1c4778824a..d3da64106685 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -131,6 +131,7 @@ struct kvm_x86_ops kvm_x86_ops __read_mostly;
 	DEFINE_STATIC_CALL_NULL(kvm_x86_##func,			     \
 				*(((struct kvm_x86_ops *)0)->func));
 #define KVM_X86_OP_OPTIONAL KVM_X86_OP
+#define KVM_X86_OP_OPTIONAL_RET0 KVM_X86_OP
 #include <asm/kvm-x86-ops.h>
 EXPORT_STATIC_CALL_GPL(kvm_x86_get_cs_db_l_bits);
 EXPORT_STATIC_CALL_GPL(kvm_x86_cache_reg);
@@ -12016,7 +12017,6 @@ void kvm_arch_flush_shadow_memslot(struct kvm *kvm,
 static inline bool kvm_guest_apic_has_interrupt(struct kvm_vcpu *vcpu)
 {
 	return (is_guest_mode(vcpu) &&
-			kvm_x86_ops.guest_apic_has_interrupt &&
 			static_call(kvm_x86_guest_apic_has_interrupt)(vcpu));
 }
 
diff --git a/kernel/static_call.c b/kernel/static_call.c
index 43ba0b1e0edb..76abd46fe6ee 100644
--- a/kernel/static_call.c
+++ b/kernel/static_call.c
@@ -503,6 +503,7 @@ long __static_call_return0(void)
 {
 	return 0;
 }
+EXPORT_SYMBOL_GPL(__static_call_return0)
 
 #ifdef CONFIG_STATIC_CALL_SELFTEST
 
-- 
2.31.1

Re: [PATCH v3 6/6] KVM: x86: allow defining return-0 static calls

[Sean Christopherson]

On Thu, Feb 17, 2022, Paolo Bonzini wrote:
> A few vendor callbacks are only used by VMX, but they return an integer
> or bool value.  Introduce KVM_X86_OP_OPTIONAL_RET0 for them: if a func is
> NULL in struct kvm_x86_ops, it will be changed to __static_call_return0
> when updating static calls.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/include/asm/kvm-x86-ops.h | 15 +++++++++------
>  arch/x86/include/asm/kvm_host.h    |  4 ++++
>  arch/x86/kvm/svm/avic.c            |  5 -----
>  arch/x86/kvm/svm/svm.c             | 20 --------------------
>  arch/x86/kvm/x86.c                 |  2 +-
>  kernel/static_call.c               |  1 +
>  6 files changed, 15 insertions(+), 32 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
> index c0ec066a8599..29affccb353c 100644
> --- a/arch/x86/include/asm/kvm-x86-ops.h
> +++ b/arch/x86/include/asm/kvm-x86-ops.h
> @@ -10,7 +10,9 @@ BUILD_BUG_ON(1)
>   *
>   * KVM_X86_OP_OPTIONAL() can be used for those functions that can have
>   * a NULL definition, for example if "static_call_cond()" will be used
> - * at the call sites.
> + * at the call sites.  KVM_X86_OP_OPTIONAL_RET0() can be used likewise
> + * to make a definition optional, but in this case the default will
> + * be __static_call_return0.

__static_call_return0()

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index ab1c4778824a..d3da64106685 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -131,6 +131,7 @@ struct kvm_x86_ops kvm_x86_ops __read_mostly;
>  	DEFINE_STATIC_CALL_NULL(kvm_x86_##func,			     \
>  				*(((struct kvm_x86_ops *)0)->func));
>  #define KVM_X86_OP_OPTIONAL KVM_X86_OP
> +#define KVM_X86_OP_OPTIONAL_RET0 KVM_X86_OP
>  #include <asm/kvm-x86-ops.h>
>  EXPORT_STATIC_CALL_GPL(kvm_x86_get_cs_db_l_bits);
>  EXPORT_STATIC_CALL_GPL(kvm_x86_cache_reg);
> @@ -12016,7 +12017,6 @@ void kvm_arch_flush_shadow_memslot(struct kvm *kvm,
>  static inline bool kvm_guest_apic_has_interrupt(struct kvm_vcpu *vcpu)
>  {
>  	return (is_guest_mode(vcpu) &&
> -			kvm_x86_ops.guest_apic_has_interrupt &&
>  			static_call(kvm_x86_guest_apic_has_interrupt)(vcpu));

Can you opportunistically align the indentation and drop the outer parantheses? I.e.

	return is_guest_mode(vcpu) &&
	       static_call(kvm_x86_guest_apic_has_interrupt)(vcpu);

>  }
>  
> diff --git a/kernel/static_call.c b/kernel/static_call.c
> index 43ba0b1e0edb..76abd46fe6ee 100644
> --- a/kernel/static_call.c
> +++ b/kernel/static_call.c
> @@ -503,6 +503,7 @@ long __static_call_return0(void)
>  {
>  	return 0;
>  }
> +EXPORT_SYMBOL_GPL(__static_call_return0)

This doesn't compile, it needs a semicolon.

Re: [PATCH v3 6/6] KVM: x86: allow defining return-0 static calls

[Paolo Bonzini]

On 2/18/22 17:29, Sean Christopherson wrote:
> On Thu, Feb 17, 2022, Paolo Bonzini wrote:
>> A few vendor callbacks are only used by VMX, but they return an integer
>> or bool value.  Introduce KVM_X86_OP_OPTIONAL_RET0 for them: if a func is
>> NULL in struct kvm_x86_ops, it will be changed to __static_call_return0
>> when updating static calls.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>   arch/x86/include/asm/kvm-x86-ops.h | 15 +++++++++------
>>   arch/x86/include/asm/kvm_host.h    |  4 ++++
>>   arch/x86/kvm/svm/avic.c            |  5 -----
>>   arch/x86/kvm/svm/svm.c             | 20 --------------------
>>   arch/x86/kvm/x86.c                 |  2 +-
>>   kernel/static_call.c               |  1 +
>>   6 files changed, 15 insertions(+), 32 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
>> index c0ec066a8599..29affccb353c 100644
>> --- a/arch/x86/include/asm/kvm-x86-ops.h
>> +++ b/arch/x86/include/asm/kvm-x86-ops.h
>> @@ -10,7 +10,9 @@ BUILD_BUG_ON(1)
>>    *
>>    * KVM_X86_OP_OPTIONAL() can be used for those functions that can have
>>    * a NULL definition, for example if "static_call_cond()" will be used
>> - * at the call sites.
>> + * at the call sites.  KVM_X86_OP_OPTIONAL_RET0() can be used likewise
>> + * to make a definition optional, but in this case the default will
>> + * be __static_call_return0.
> 
> __static_call_return0()
> 
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index ab1c4778824a..d3da64106685 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -131,6 +131,7 @@ struct kvm_x86_ops kvm_x86_ops __read_mostly;
>>   	DEFINE_STATIC_CALL_NULL(kvm_x86_##func,			     \
>>   				*(((struct kvm_x86_ops *)0)->func));
>>   #define KVM_X86_OP_OPTIONAL KVM_X86_OP
>> +#define KVM_X86_OP_OPTIONAL_RET0 KVM_X86_OP
>>   #include <asm/kvm-x86-ops.h>
>>   EXPORT_STATIC_CALL_GPL(kvm_x86_get_cs_db_l_bits);
>>   EXPORT_STATIC_CALL_GPL(kvm_x86_cache_reg);
>> @@ -12016,7 +12017,6 @@ void kvm_arch_flush_shadow_memslot(struct kvm *kvm,
>>   static inline bool kvm_guest_apic_has_interrupt(struct kvm_vcpu *vcpu)
>>   {
>>   	return (is_guest_mode(vcpu) &&
>> -			kvm_x86_ops.guest_apic_has_interrupt &&
>>   			static_call(kvm_x86_guest_apic_has_interrupt)(vcpu));
> 
> Can you opportunistically align the indentation and drop the outer parantheses? I.e.
> 
> 	return is_guest_mode(vcpu) &&
> 	       static_call(kvm_x86_guest_apic_has_interrupt)(vcpu);

Hmm, I like having the parentheses (despite "return not being a 
function") because editors are inconsistent in what indentation to use 
after return.

Some use a tab (which does the right thing just by chance with Linux 
because "return " is as long as a tab is wide), but vim for example does 
the totally awkward

int f()
{
         return a &&
                 b;
}

Of course I can fix the indentation.

Paolo

>>   }
>>   
>> diff --git a/kernel/static_call.c b/kernel/static_call.c
>> index 43ba0b1e0edb..76abd46fe6ee 100644
>> --- a/kernel/static_call.c
>> +++ b/kernel/static_call.c
>> @@ -503,6 +503,7 @@ long __static_call_return0(void)
>>   {
>>   	return 0;
>>   }
>> +EXPORT_SYMBOL_GPL(__static_call_return0)
> 
> This doesn't compile, it needs a semicolon.
> 

Re: [PATCH v3 6/6] KVM: x86: allow defining return-0 static calls

[Sean Christopherson]

On Fri, Feb 18, 2022, Paolo Bonzini wrote:
> On 2/18/22 17:29, Sean Christopherson wrote:
> > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > index ab1c4778824a..d3da64106685 100644
> > > --- a/arch/x86/kvm/x86.c
> > > +++ b/arch/x86/kvm/x86.c
> > > @@ -131,6 +131,7 @@ struct kvm_x86_ops kvm_x86_ops __read_mostly;
> > >   	DEFINE_STATIC_CALL_NULL(kvm_x86_##func,			     \
> > >   				*(((struct kvm_x86_ops *)0)->func));
> > >   #define KVM_X86_OP_OPTIONAL KVM_X86_OP
> > > +#define KVM_X86_OP_OPTIONAL_RET0 KVM_X86_OP
> > >   #include <asm/kvm-x86-ops.h>
> > >   EXPORT_STATIC_CALL_GPL(kvm_x86_get_cs_db_l_bits);
> > >   EXPORT_STATIC_CALL_GPL(kvm_x86_cache_reg);
> > > @@ -12016,7 +12017,6 @@ void kvm_arch_flush_shadow_memslot(struct kvm *kvm,
> > >   static inline bool kvm_guest_apic_has_interrupt(struct kvm_vcpu *vcpu)
> > >   {
> > >   	return (is_guest_mode(vcpu) &&
> > > -			kvm_x86_ops.guest_apic_has_interrupt &&
> > >   			static_call(kvm_x86_guest_apic_has_interrupt)(vcpu));
> > 
> > Can you opportunistically align the indentation and drop the outer parantheses? I.e.
> > 
> > 	return is_guest_mode(vcpu) &&
> > 	       static_call(kvm_x86_guest_apic_has_interrupt)(vcpu);
> 
> Hmm, I like having the parentheses (despite "return not being a function")
> because editors are inconsistent in what indentation to use after return.
> 
> Some use a tab (which does the right thing just by chance with Linux because
> "return " is as long as a tab is wide), but vim for example does the totally

Uh, no, vim inserts a tab.  "return " isn't as long as a tab is wide.  That's 7
chars, tabs are 8, which is exactly the problem.

I'm ok with
	
	return (is_guest_mode(vcpu) &&
		static_call(kvm_x86_guest_apic_has_interrupt)(vcpu));

I care more about alignment than unnecessary (), but I'd still prefer

	return is_guest_mode(vcpu) &&
	       static_call(kvm_x86_guest_apic_has_interrupt)(vcpu));

> awkward
> 
> int f()
> {
>         return a &&
>                 b;
> }
> 
> Of course I can fix the indentation.
> 
> Paolo
> 
> > >   }
> > > diff --git a/kernel/static_call.c b/kernel/static_call.c
> > > index 43ba0b1e0edb..76abd46fe6ee 100644
> > > --- a/kernel/static_call.c
> > > +++ b/kernel/static_call.c
> > > @@ -503,6 +503,7 @@ long __static_call_return0(void)
> > >   {
> > >   	return 0;
> > >   }
> > > +EXPORT_SYMBOL_GPL(__static_call_return0)
> > 
> > This doesn't compile, it needs a semicolon.
> > 
> 

Re: [PATCH v3 6/6] KVM: x86: allow defining return-0 static calls

[Maxim Levitsky]

On Thu, 2022-02-17 at 13:08 -0500, Paolo Bonzini wrote:
> A few vendor callbacks are only used by VMX, but they return an integer
> or bool value.  Introduce KVM_X86_OP_OPTIONAL_RET0 for them: if a func is
> NULL in struct kvm_x86_ops, it will be changed to __static_call_return0
> when updating static calls.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/include/asm/kvm-x86-ops.h | 15 +++++++++------
>  arch/x86/include/asm/kvm_host.h    |  4 ++++
>  arch/x86/kvm/svm/avic.c            |  5 -----
>  arch/x86/kvm/svm/svm.c             | 20 --------------------
>  arch/x86/kvm/x86.c                 |  2 +-
>  kernel/static_call.c               |  1 +
>  6 files changed, 15 insertions(+), 32 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
> index c0ec066a8599..29affccb353c 100644
> --- a/arch/x86/include/asm/kvm-x86-ops.h
> +++ b/arch/x86/include/asm/kvm-x86-ops.h
> @@ -10,7 +10,9 @@ BUILD_BUG_ON(1)
>   *
>   * KVM_X86_OP_OPTIONAL() can be used for those functions that can have
>   * a NULL definition, for example if "static_call_cond()" will be used
> - * at the call sites.
> + * at the call sites.  KVM_X86_OP_OPTIONAL_RET0() can be used likewise
> + * to make a definition optional, but in this case the default will
> + * be __static_call_return0.
>   */
>  KVM_X86_OP(hardware_enable)
>  KVM_X86_OP(hardware_disable)
> @@ -77,15 +79,15 @@ KVM_X86_OP(check_apicv_inhibit_reasons)
>  KVM_X86_OP(refresh_apicv_exec_ctrl)
>  KVM_X86_OP_OPTIONAL(hwapic_irr_update)
>  KVM_X86_OP_OPTIONAL(hwapic_isr_update)
> -KVM_X86_OP_OPTIONAL(guest_apic_has_interrupt)
> +KVM_X86_OP_OPTIONAL_RET0(guest_apic_has_interrupt)
>  KVM_X86_OP_OPTIONAL(load_eoi_exitmap)
>  KVM_X86_OP_OPTIONAL(set_virtual_apic_mode)
>  KVM_X86_OP_OPTIONAL(set_apic_access_page_addr)
>  KVM_X86_OP(deliver_interrupt)
>  KVM_X86_OP_OPTIONAL(sync_pir_to_irr)
> -KVM_X86_OP(set_tss_addr)
> -KVM_X86_OP(set_identity_map_addr)
> -KVM_X86_OP(get_mt_mask)
> +KVM_X86_OP_OPTIONAL_RET0(set_tss_addr)
> +KVM_X86_OP_OPTIONAL_RET0(set_identity_map_addr)
> +KVM_X86_OP_OPTIONAL_RET0(get_mt_mask)
>  KVM_X86_OP(load_mmu_pgd)
>  KVM_X86_OP(has_wbinvd_exit)
>  KVM_X86_OP(get_l2_tsc_offset)
> @@ -103,7 +105,7 @@ KVM_X86_OP_OPTIONAL(vcpu_unblocking)
>  KVM_X86_OP_OPTIONAL(pi_update_irte)
>  KVM_X86_OP_OPTIONAL(pi_start_assignment)
>  KVM_X86_OP_OPTIONAL(apicv_post_state_restore)
> -KVM_X86_OP_OPTIONAL(dy_apicv_has_pending_interrupt)
> +KVM_X86_OP_OPTIONAL_RET0(dy_apicv_has_pending_interrupt)
>  KVM_X86_OP_OPTIONAL(set_hv_timer)
>  KVM_X86_OP_OPTIONAL(cancel_hv_timer)
>  KVM_X86_OP(setup_mce)
> @@ -127,3 +129,4 @@ KVM_X86_OP(vcpu_deliver_sipi_vector)
>  
>  #undef KVM_X86_OP
>  #undef KVM_X86_OP_OPTIONAL
> +#undef KVM_X86_OP_OPTIONAL_RET0
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index a7e82fc1f1f3..8e512f25a930 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1541,6 +1541,7 @@ extern struct kvm_x86_ops kvm_x86_ops;
>  #define KVM_X86_OP(func) \
>  	DECLARE_STATIC_CALL(kvm_x86_##func, *(((struct kvm_x86_ops *)0)->func));
>  #define KVM_X86_OP_OPTIONAL KVM_X86_OP
> +#define KVM_X86_OP_OPTIONAL_RET0 KVM_X86_OP
>  #include <asm/kvm-x86-ops.h>
>  
>  static inline void kvm_ops_static_call_update(void)
> @@ -1550,6 +1551,9 @@ static inline void kvm_ops_static_call_update(void)
>  #define KVM_X86_OP(func) \
>  	WARN_ON(!kvm_x86_ops.func); __KVM_X86_OP(func)
>  #define KVM_X86_OP_OPTIONAL __KVM_X86_OP
> +#define KVM_X86_OP_OPTIONAL_RET0(func) \
> +	static_call_update(kvm_x86_##func, kvm_x86_ops.func ? : \
> +			   (void *) __static_call_return0);
>  #include <asm/kvm-x86-ops.h>
>  #undef __KVM_X86_OP
>  }
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index 4245cb99b497..d4fa8c4f3a9a 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -650,11 +650,6 @@ void avic_refresh_apicv_exec_ctrl(struct kvm_vcpu *vcpu)
>  	avic_set_pi_irte_mode(vcpu, activated);
>  }
>  
> -bool avic_dy_apicv_has_pending_interrupt(struct kvm_vcpu *vcpu)
> -{
> -	return false;
> -}
> -
>  static void svm_ir_list_del(struct vcpu_svm *svm, struct amd_iommu_pi_data *pi)
>  {
>  	unsigned long flags;
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 3daca34020fa..7038c76fa841 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -3528,16 +3528,6 @@ static void svm_enable_nmi_window(struct kvm_vcpu *vcpu)
>  	svm->vmcb->save.rflags |= (X86_EFLAGS_TF | X86_EFLAGS_RF);
>  }
>  
> -static int svm_set_tss_addr(struct kvm *kvm, unsigned int addr)
> -{
> -	return 0;
> -}
> -
> -static int svm_set_identity_map_addr(struct kvm *kvm, u64 ident_addr)
> -{
> -	return 0;
> -}
> -
>  static void svm_flush_tlb_current(struct kvm_vcpu *vcpu)
>  {
>  	struct vcpu_svm *svm = to_svm(vcpu);
> @@ -3934,11 +3924,6 @@ static bool svm_has_emulated_msr(struct kvm *kvm, u32 index)
>  	return true;
>  }
>  
> -static u64 svm_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
> -{
> -	return 0;
> -}
> -
>  static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>  {
>  	struct vcpu_svm *svm = to_svm(vcpu);
> @@ -4593,10 +4578,6 @@ static struct kvm_x86_ops svm_x86_ops __initdata = {
>  	.check_apicv_inhibit_reasons = avic_check_apicv_inhibit_reasons,
>  	.apicv_post_state_restore = avic_apicv_post_state_restore,
>  
> -	.set_tss_addr = svm_set_tss_addr,
> -	.set_identity_map_addr = svm_set_identity_map_addr,
> -	.get_mt_mask = svm_get_mt_mask,
> -
>  	.get_exit_info = svm_get_exit_info,
>  
>  	.vcpu_after_set_cpuid = svm_vcpu_after_set_cpuid,
> @@ -4621,7 +4602,6 @@ static struct kvm_x86_ops svm_x86_ops __initdata = {
>  	.nested_ops = &svm_nested_ops,
>  
>  	.deliver_interrupt = svm_deliver_interrupt,
> -	.dy_apicv_has_pending_interrupt = avic_dy_apicv_has_pending_interrupt,
>  	.pi_update_irte = avic_pi_update_irte,
>  	.setup_mce = svm_setup_mce,
>  
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index ab1c4778824a..d3da64106685 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -131,6 +131,7 @@ struct kvm_x86_ops kvm_x86_ops __read_mostly;
>  	DEFINE_STATIC_CALL_NULL(kvm_x86_##func,			     \
>  				*(((struct kvm_x86_ops *)0)->func));
>  #define KVM_X86_OP_OPTIONAL KVM_X86_OP
> +#define KVM_X86_OP_OPTIONAL_RET0 KVM_X86_OP
>  #include <asm/kvm-x86-ops.h>
>  EXPORT_STATIC_CALL_GPL(kvm_x86_get_cs_db_l_bits);
>  EXPORT_STATIC_CALL_GPL(kvm_x86_cache_reg);
> @@ -12016,7 +12017,6 @@ void kvm_arch_flush_shadow_memslot(struct kvm *kvm,
>  static inline bool kvm_guest_apic_has_interrupt(struct kvm_vcpu *vcpu)
>  {
>  	return (is_guest_mode(vcpu) &&
> -			kvm_x86_ops.guest_apic_has_interrupt &&
>  			static_call(kvm_x86_guest_apic_has_interrupt)(vcpu));
>  }
>  
> diff --git a/kernel/static_call.c b/kernel/static_call.c
> index 43ba0b1e0edb..76abd46fe6ee 100644
> --- a/kernel/static_call.c
> +++ b/kernel/static_call.c
> @@ -503,6 +503,7 @@ long __static_call_return0(void)
>  {
>  	return 0;
>  }
> +EXPORT_SYMBOL_GPL(__static_call_return0)
>  
>  #ifdef CONFIG_STATIC_CALL_SELFTEST
>  


Believe me or not, but this patch introduced a regression with 32 bit KVM.
(32 bit L1, 32 bit L2, 64 bit L0 since I am not crazy enough)

The following partial revert "fixes" it:


diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
index 20f64e07e359..3388072b2e3b 100644
--- a/arch/x86/include/asm/kvm-x86-ops.h
+++ b/arch/x86/include/asm/kvm-x86-ops.h
@@ -88,7 +88,7 @@ KVM_X86_OP(deliver_interrupt)
 KVM_X86_OP_OPTIONAL(sync_pir_to_irr)
 KVM_X86_OP_OPTIONAL_RET0(set_tss_addr)
 KVM_X86_OP_OPTIONAL_RET0(set_identity_map_addr)
-KVM_X86_OP_OPTIONAL_RET0(get_mt_mask)
+KVM_X86_OP(get_mt_mask)
 KVM_X86_OP(load_mmu_pgd)
 KVM_X86_OP(has_wbinvd_exit)
 KVM_X86_OP(get_l2_tsc_offset)
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index a09b4f1a18f6..0c09292b0611 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -4057,6 +4057,11 @@ static bool svm_has_emulated_msr(struct kvm *kvm, u32 index)
        return true;
 }
 
+static u64 svm_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
+{
+       return 0;
+}
+
 static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 {
        struct vcpu_svm *svm = to_svm(vcpu);
@@ -4718,6 +4723,7 @@ static struct kvm_x86_ops svm_x86_ops __initdata = {
        .check_apicv_inhibit_reasons = avic_check_apicv_inhibit_reasons,
        .apicv_post_state_restore = avic_apicv_post_state_restore,
 
+       .get_mt_mask = svm_get_mt_mask,
        .get_exit_info = svm_get_exit_info,
 
        .vcpu_after_set_cpuid = svm_vcpu_after_set_cpuid,


I guess something with returning u64 made 32 compiler unhappy.

I suspect that __static_call_return0 returns long, which means it returns
32 bit value on 32 bit system.

Maybe we can make it return u64. I don't know if that will break something else though.

This is what is printed when trying to start a VM:


[   22.821817] ------------[ cut here ]------------
[   22.822751] spte = 0xfffffc4465825, level = 1, rsvd bits = 0xffff000000000000
[   22.822771] WARNING: CPU: 14 PID: 3245 at arch/x86/kvm/mmu/spte.c:182 make_spte+0x2d1/0x319 [kvm]
[   22.824452] Modules linked in: uinput kvm_amd(O) kvm(O) irqbypass xt_MASQUERADE xt_conntrack ipt_REJECT tun bridge iptable_mangle iptable_nat nf_nat ebtable_filter ebtables ip6table_filter
ip6_tables rpcsec_gss_krb5 auth_rpcgss nfsv4 dns_resolver nfs lockd grace fscache netfs rfkill sunrpc dm_mirror dm_region_hash dm_log snd_hda_codec_generic snd_hda_intel snd_intel_dspcfg snd_hda_codec
snd_hwdep snd_hda_core snd_seq snd_seq_device joydev snd_pcm input_leds snd_timer snd crc32_pclmul pcspkr lpc_ich virtio_input mfd_core rtc_cmos button sch_fq_codel ext4 mbcache jbd2 hid_generic
virtio_gpu usbhid virtio_dma_buf hid drm_shmem_helper sd_mod t10_pi drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops cec virtio_net drm net_failover virtio_console i2c_core failover
virtio_scsi ahci libahci crc32c_intel xhci_pci libata xhci_hcd virtio_pci virtio virtio_pci_legacy_dev virtio_pci_modern_dev virtio_ring fuse ipv6 autofs4 [last unloaded: irqbypass]
[   22.832788] CPU: 14 PID: 3245 Comm: CPU 0/KVM Tainted: G           O      5.17.0-rc8.unstable #49
[   22.833681] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.14.0-24-g8f3e834 04/01/2014
[   22.834615] EIP: make_spte+0x2d1/0x319 [kvm]
[   22.835088] Code: 01 0f ac f9 07 83 e1 01 8d 0c 89 8d 54 0b ff ff b4 d0 8c 00 00 00 ff b4 d0 88 00 00 00 53 57 56 68 1c 07 9c f0 e8 49 43 c1 d0 <0f> 0b 83 c4 18 89 f0 83 e0 02 74 29 8b 45 e0 f6 80
84 00 00 00 01
[   22.836951] EAX: 00000041 EBX: 00000001 ECX: 00000027 EDX: ed41cd0c
[   22.837570] ESI: c4465825 EDI: 000fffff EBP: c5491bb8 ESP: c5491b74
[   22.838209] DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068 EFLAGS: 00010286
[   22.838914] CR0: 80050033 CR2: 00000000 CR3: 044e5400 CR4: 00350ef0
[   22.839533] Call Trace:
[   22.839788]  mmu_set_spte+0x152/0x319 [kvm]
[   22.840242]  direct_page_fault+0x487/0x510 [kvm]
[   22.840754]  ? kvm_mtrr_check_gfn_range_consistency+0x66/0xe0 [kvm]
[   22.841413]  kvm_tdp_page_fault+0x73/0x7b [kvm]
[   22.841908]  kvm_mmu_page_fault+0x3bb/0x6a6 [kvm]
[   22.842407]  ? timekeeping_get_ns+0x10/0x72
[   22.842849]  ? apic_find_highest_isr+0x24/0x2b [kvm]
[   22.843372]  ? __apic_update_ppr+0x2c/0x67 [kvm]
[   22.843871]  ? apic_update_ppr+0x23/0x57 [kvm]
[   22.844344]  ? kvm_lapic_set_tpr+0x28/0x2a [kvm]
[   22.844875]  npf_interception+0x89/0x91 [kvm_amd]
[   22.845348]  ? npf_interception+0x89/0x91 [kvm_amd]
[   22.845879]  svm_invoke_exit_handler+0x30/0xb6 [kvm_amd]
[   22.846410]  svm_handle_exit+0x17c/0x184 [kvm_amd]
[   22.846914]  kvm_arch_vcpu_ioctl_run+0x11e5/0x14a4 [kvm]
[   22.847478]  ? mod_objcg_state+0xf1/0x10a
[   22.847901]  kvm_vcpu_ioctl+0x16e/0x516 [kvm]
[   22.848365]  ? kvm_vcpu_ioctl+0x16e/0x516 [kvm]
[   22.848879]  ? try_to_wake_up+0x1a0/0x1c9
[   22.849299]  ? put_task_struct+0x15/0x23
[   22.849698]  ? wake_up_q+0x2e/0x39
[   22.850060]  ? __fget+0x1e/0x25
[   22.850377]  ? kvm_uevent_notify_change+0x19a/0x19a [kvm]
[   22.850976]  vfs_ioctl+0x1c/0x26
[   22.851308]  __ia32_sys_ioctl+0x735/0x767
[   22.851714]  ? __ia32_sys_futex_time32+0x125/0x145
[   22.852211]  ? exit_to_user_mode_prepare+0xe2/0x142
[   22.852701]  __do_fast_syscall_32+0x78/0x97
[   22.853149]  do_fast_syscall_32+0x29/0x5b
[   22.853549]  do_SYSENTER_32+0x15/0x17
[   22.853919]  entry_SYSENTER_32+0xa2/0xfb
[   22.854334] EIP: 0xa835c53d
[   22.854624] Code: c4 01 10 03 03 74 c0 01 10 05 03 74 b8 01 10 06 03 74 b4 01 10 07 03 74 b0 01 10 08 03 74 d8 01 00 51 52 55 89 e5 0f 34 cd 80 <5d> 5a 59 c3 90 90 90 90 8d 76 00 58 b8 77 00 00 00
cd 80 90 8d 76
[   22.856520] EAX: ffffffda EBX: 00000021 ECX: 0000ae80 EDX: 00000000
[   22.857195] ESI: 096509e0 EDI: 3acfeb40 EBP: 3acfcf48 ESP: 3acfcef8
[   22.857818] DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 007b EFLAGS: 00000212
[   22.858503] ---[ end trace 0000000000000000 ]---
[   22.859768] get_mmio_spte: reserved bits set on MMU-present spte, addr 0xfffff000, hierarchy:
[   22.860631] ------ spte = 0x8913827 level = 2, rsvd bits = 0xffff000000000000
[   22.860633] ------ spte = 0xfffffc4465825 level = 1, rsvd bits = 0xffff000000000000
[   22.861357] ------------[ cut here ]------------
[   22.862590] WARNING: CPU: 14 PID: 3245 at arch/x86/kvm/mmu/mmu.c:3812 kvm_mmu_page_fault+0x2ad/0x6a6 [kvm]
[   22.863620] Modules linked in: uinput kvm_amd(O) kvm(O) irqbypass xt_MASQUERADE xt_conntrack ipt_REJECT tun bridge iptable_mangle iptable_nat nf_nat ebtable_filter ebtables ip6table_filter
ip6_tables rpcsec_gss_krb5 auth_rpcgss nfsv4 dns_resolver nfs lockd grace fscache netfs rfkill sunrpc dm_mirror dm_region_hash dm_log snd_hda_codec_generic snd_hda_intel snd_intel_dspcfg snd_hda_codec
snd_hwdep snd_hda_core snd_seq snd_seq_device joydev snd_pcm input_leds snd_timer snd crc32_pclmul pcspkr lpc_ich virtio_input mfd_core rtc_cmos button sch_fq_codel ext4 mbcache jbd2 hid_generic
virtio_gpu usbhid virtio_dma_buf hid drm_shmem_helper sd_mod t10_pi drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops cec virtio_net drm net_failover virtio_console i2c_core failover
virtio_scsi ahci libahci crc32c_intel xhci_pci libata xhci_hcd virtio_pci virtio virtio_pci_legacy_dev virtio_pci_modern_dev virtio_ring fuse ipv6 autofs4 [last unloaded: irqbypass]
[   22.871921] CPU: 14 PID: 3245 Comm: CPU 0/KVM Tainted: G        W  O      5.17.0-rc8.unstable #49
[   22.872809] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.14.0-24-g8f3e834 04/01/2014
[   22.873749] EIP: kvm_mmu_page_fault+0x2ad/0x6a6 [kvm]
[   22.874281] Code: b4 c2 8c 00 00 00 ff b4 c2 88 00 00 00 ff b5 68 ff ff ff 53 51 68 71 03 9c f0 e8 d5 93 c1 d0 89 b5 68 ff ff ff 83 c4 18 eb a6 <0f> 0b bb ea ff ff ff e9 ce 03 00 00 8b 85 7c ff ff
ff 8b 55 80 e8
[   22.876139] EAX: 00000047 EBX: 000fffff ECX: 00000027 EDX: ed41cd0c
[   22.876767] ESI: 00000000 EDI: 00000001 EBP: c5491d80 ESP: c5491cc0
[   22.877400] DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068 EFLAGS: 00010202
[   22.878092] CR0: 80050033 CR2: 00000000 CR3: 044e5400 CR4: 00350ef0
[   22.878716] Call Trace:
[   22.878962]  ? kvm_arch_vcpu_load+0x1a3/0x1ab [kvm]
[   22.879493]  ? avic_update_iommu_vcpu_affinity.constprop.0.isra.0+0xe/0x4a [kvm_amd]
[   22.880257]  ? apic_find_highest_isr+0x24/0x2b [kvm]
[   22.880790]  ? apic_update_ppr+0x23/0x57 [kvm]
[   22.881296]  ? kvm_lapic_set_tpr+0x28/0x2a [kvm]
[   22.881803]  npf_interception+0x89/0x91 [kvm_amd]
[   22.882273]  ? npf_interception+0x89/0x91 [kvm_amd]
[   22.882771]  svm_invoke_exit_handler+0x30/0xb6 [kvm_amd]
[   22.883308]  svm_handle_exit+0x17c/0x184 [kvm_amd]
[   22.883801]  kvm_arch_vcpu_ioctl_run+0x11e5/0x14a4 [kvm]
[   22.884354]  ? mod_objcg_state+0xf1/0x10a
[   22.884764]  kvm_vcpu_ioctl+0x16e/0x516 [kvm]
[   22.885250]  ? kvm_vcpu_ioctl+0x16e/0x516 [kvm]
[   22.885735]  ? try_to_wake_up+0x1a0/0x1c9
[   22.886137]  ? put_task_struct+0x15/0x23
[   22.886532]  ? wake_up_q+0x2e/0x39
[   22.886885]  ? __fget+0x1e/0x25
[   22.887229]  ? kvm_uevent_notify_change+0x19a/0x19a [kvm]
[   22.887796]  vfs_ioctl+0x1c/0x26
[   22.888125]  __ia32_sys_ioctl+0x735/0x767
[   22.888532]  ? __ia32_sys_futex_time32+0x125/0x145
[   22.889006]  ? exit_to_user_mode_prepare+0xe2/0x142
[   22.889516]  __do_fast_syscall_32+0x78/0x97
[   22.889936]  do_fast_syscall_32+0x29/0x5b
[   22.890337]  do_SYSENTER_32+0x15/0x17
[   22.890716]  entry_SYSENTER_32+0xa2/0xfb
[   22.891107] EIP: 0xa835c53d
[   22.891398] Code: c4 01 10 03 03 74 c0 01 10 05 03 74 b8 01 10 06 03 74 b4 01 10 07 03 74 b0 01 10 08 03 74 d8 01 00 51 52 55 89 e5 0f 34 cd 80 <5d> 5a 59 c3 90 90 90 90 8d 76 00 58 b8 77 00 00 00
cd 80 90 8d 76
[   22.893339] EAX: ffffffda EBX: 00000021 ECX: 0000ae80 EDX: 00000000
[   22.894090] ESI: 096509e0 EDI: 3acfeb40 EBP: 3acfcf48 ESP: 3acfcef8
[   22.894849] DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 007b EFLAGS: 00000212
[   22.895697] ---[ end trace 0000000000000000 ]---

Best regards,
	Maxim Levitsky

RE: [PATCH v3 6/6] KVM: x86: allow defining return-0 static calls

[David Laight]

> I guess something with returning u64 made 32 compiler unhappy.
> 
> I suspect that __static_call_return0 returns long, which means it returns
> 32 bit value on 32 bit system.
> 
> Maybe we can make it return u64. I don't know if that will break something else though.

32 bit system will return a 64bit value in %edx:%eax.
Since %rdx is a scratch register (caller saved) it is fine.

Is __statc_call_return0() ever used to return a NULL pointer?
ISTR some ABI (maybe m68k ones) that returned pointers and
integers in different registers.
Plausibly that was only 35 years ago.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v3 6/6] KVM: x86: allow defining return-0 static calls

[Paolo Bonzini]

On 3/17/22 18:43, Maxim Levitsky wrote:
> diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
> index 20f64e07e359..3388072b2e3b 100644
> --- a/arch/x86/include/asm/kvm-x86-ops.h
> +++ b/arch/x86/include/asm/kvm-x86-ops.h
> @@ -88,7 +88,7 @@ KVM_X86_OP(deliver_interrupt)
>   KVM_X86_OP_OPTIONAL(sync_pir_to_irr)
>   KVM_X86_OP_OPTIONAL_RET0(set_tss_addr)
>   KVM_X86_OP_OPTIONAL_RET0(set_identity_map_addr)
> -KVM_X86_OP_OPTIONAL_RET0(get_mt_mask)
> +KVM_X86_OP(get_mt_mask)
>   KVM_X86_OP(load_mmu_pgd)
>   KVM_X86_OP(has_wbinvd_exit)
>   KVM_X86_OP(get_l2_tsc_offset)
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index a09b4f1a18f6..0c09292b0611 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -4057,6 +4057,11 @@ static bool svm_has_emulated_msr(struct kvm *kvm, u32 index)
>          return true;
>   }
>   
> +static u64 svm_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
> +{
> +       return 0;
> +}
> +
>   static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>   {
>          struct vcpu_svm *svm = to_svm(vcpu);
> @@ -4718,6 +4723,7 @@ static struct kvm_x86_ops svm_x86_ops __initdata = {
>          .check_apicv_inhibit_reasons = avic_check_apicv_inhibit_reasons,
>          .apicv_post_state_restore = avic_apicv_post_state_restore,
>   
> +       .get_mt_mask = svm_get_mt_mask,
>          .get_exit_info = svm_get_exit_info,
>   
>          .vcpu_after_set_cpuid = svm_vcpu_after_set_cpuid,

Thanks, I'll send it as a complete patch.  Please reply there with your 
Signed-off-by.

Related to this, I don't see anything in arch/x86/kernel/static_call.c 
that limits this code to x86-64:

                 if (func == &__static_call_return0) {
                         emulate = code;
                         code = &xor5rax;
                 }


On 32-bit, it will be patched as "dec ax; xor eax, eax" or something 
like that.  Fortunately it doesn't corrupt any callee-save register but 
it is not just a bit funky, it's also not a single instruction.

Paolo

Re: [PATCH v3 6/6] KVM: x86: allow defining return-0 static calls

[Peter Zijlstra]

On Fri, Mar 18, 2022 at 05:29:20PM +0100, Paolo Bonzini wrote:
> On 3/17/22 18:43, Maxim Levitsky wrote:
> > diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
> > index 20f64e07e359..3388072b2e3b 100644
> > --- a/arch/x86/include/asm/kvm-x86-ops.h
> > +++ b/arch/x86/include/asm/kvm-x86-ops.h
> > @@ -88,7 +88,7 @@ KVM_X86_OP(deliver_interrupt)
> >   KVM_X86_OP_OPTIONAL(sync_pir_to_irr)
> >   KVM_X86_OP_OPTIONAL_RET0(set_tss_addr)
> >   KVM_X86_OP_OPTIONAL_RET0(set_identity_map_addr)
> > -KVM_X86_OP_OPTIONAL_RET0(get_mt_mask)
> > +KVM_X86_OP(get_mt_mask)
> >   KVM_X86_OP(load_mmu_pgd)
> >   KVM_X86_OP(has_wbinvd_exit)
> >   KVM_X86_OP(get_l2_tsc_offset)
> > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > index a09b4f1a18f6..0c09292b0611 100644
> > --- a/arch/x86/kvm/svm/svm.c
> > +++ b/arch/x86/kvm/svm/svm.c
> > @@ -4057,6 +4057,11 @@ static bool svm_has_emulated_msr(struct kvm *kvm, u32 index)
> >          return true;
> >   }
> > +static u64 svm_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
> > +{
> > +       return 0;
> > +}
> > +
> >   static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
> >   {
> >          struct vcpu_svm *svm = to_svm(vcpu);
> > @@ -4718,6 +4723,7 @@ static struct kvm_x86_ops svm_x86_ops __initdata = {
> >          .check_apicv_inhibit_reasons = avic_check_apicv_inhibit_reasons,
> >          .apicv_post_state_restore = avic_apicv_post_state_restore,
> > +       .get_mt_mask = svm_get_mt_mask,
> >          .get_exit_info = svm_get_exit_info,
> >          .vcpu_after_set_cpuid = svm_vcpu_after_set_cpuid,
> 
> Thanks, I'll send it as a complete patch.  Please reply there with your
> Signed-off-by.

Yeah, ret0 should only be used with up-to 'long' return values.

So ACK on that patch.

> Related to this, I don't see anything in arch/x86/kernel/static_call.c that
> limits this code to x86-64:
> 
>                 if (func == &__static_call_return0) {
>                         emulate = code;
>                         code = &xor5rax;
>                 }
> 
> 
> On 32-bit, it will be patched as "dec ax; xor eax, eax" or something like
> that.  Fortunately it doesn't corrupt any callee-save register but it is not
> just a bit funky, it's also not a single instruction.

Urggghh.. that's fairly yuck. So there's two options I suppose:

	0x66, 0x66, 0x66, 0x31, 0xc0

Which is a tripple prefix xor %eax, %eax, which, IIRC should still clear
the whole 64bit on 64bit and *should* still not trigger the prefix
decoding penalty some frontends have (which is >3 IIRC).

Or we can emit:

	0xb8, 0x00, 0x00, 0x00, 0x00

which decodes to: mov $0x0,%eax, which is less efficient in some
front-ends since it doesn't always get picked up in register rename
stage.

Re: [PATCH v3 6/6] KVM: x86: allow defining return-0 static calls

[Peter Zijlstra]

On Fri, Mar 18, 2022 at 06:28:37PM +0100, Peter Zijlstra wrote:
> > Related to this, I don't see anything in arch/x86/kernel/static_call.c that
> > limits this code to x86-64:
> > 
> >                 if (func == &__static_call_return0) {
> >                         emulate = code;
> >                         code = &xor5rax;
> >                 }
> > 
> > 
> > On 32-bit, it will be patched as "dec ax; xor eax, eax" or something like
> > that.  Fortunately it doesn't corrupt any callee-save register but it is not
> > just a bit funky, it's also not a single instruction.
> 
> Urggghh.. that's fairly yuck. So there's two options I suppose:
> 
> 	0x66, 0x66, 0x66, 0x31, 0xc0

Argh, that turns into: xorw %ax, %ax.

Let me see if there's another option.

> Which is a tripple prefix xor %eax, %eax, which, IIRC should still clear
> the whole 64bit on 64bit and *should* still not trigger the prefix
> decoding penalty some frontends have (which is >3 IIRC).
> 
> Or we can emit:
> 
> 	0xb8, 0x00, 0x00, 0x00, 0x00
> 
> which decodes to: mov $0x0,%eax, which is less efficient in some
> front-ends since it doesn't always get picked up in register rename
> stage.
> 
> 

Re: [PATCH v3 6/6] KVM: x86: allow defining return-0 static calls

[Peter Zijlstra]

On Fri, Mar 18, 2022 at 06:47:32PM +0100, Peter Zijlstra wrote:
> On Fri, Mar 18, 2022 at 06:28:37PM +0100, Peter Zijlstra wrote:
> > > Related to this, I don't see anything in arch/x86/kernel/static_call.c that
> > > limits this code to x86-64:
> > > 
> > >                 if (func == &__static_call_return0) {
> > >                         emulate = code;
> > >                         code = &xor5rax;
> > >                 }
> > > 
> > > 
> > > On 32-bit, it will be patched as "dec ax; xor eax, eax" or something like
> > > that.  Fortunately it doesn't corrupt any callee-save register but it is not
> > > just a bit funky, it's also not a single instruction.
> > 
> > Urggghh.. that's fairly yuck. So there's two options I suppose:
> > 
> > 	0x66, 0x66, 0x66, 0x31, 0xc0
> 
> Argh, that turns into: xorw %ax, %ax.
> 
> Let me see if there's another option.

Amazingly:

  0x2e, 0x2e, 0x2e, 0x31, 0xc0

seems to actually work.. I've build and ran and decoded the below on
32bit and 64bit (arguably on the same 64bit host).


---
#include <stdio.h>

long zero(void)
{
	long z = -1L;

	asm (".byte 0x2e, 0x2e, 0x2e, 0x31, 0xc0" : "=a" (z) );

	return z;
}

void main(void)
{
	printf("%ld\n", zero());
}

RE: [PATCH v3 6/6] KVM: x86: allow defining return-0 static calls

[David Laight]

From: Peter Zijlstra
> Sent: 18 March 2022 18:02
> 
> On Fri, Mar 18, 2022 at 06:47:32PM +0100, Peter Zijlstra wrote:
> > On Fri, Mar 18, 2022 at 06:28:37PM +0100, Peter Zijlstra wrote:
> > > > Related to this, I don't see anything in arch/x86/kernel/static_call.c that
> > > > limits this code to x86-64:
> > > >
> > > >                 if (func == &__static_call_return0) {
> > > >                         emulate = code;
> > > >                         code = &xor5rax;
> > > >                 }
> > > >
> > > >
> > > > On 32-bit, it will be patched as "dec ax; xor eax, eax" or something like
> > > > that.  Fortunately it doesn't corrupt any callee-save register but it is not
> > > > just a bit funky, it's also not a single instruction.
> > >
> > > Urggghh.. that's fairly yuck. So there's two options I suppose:
> > >
> > > 	0x66, 0x66, 0x66, 0x31, 0xc0
> >
> > Argh, that turns into: xorw %ax, %ax.
> >
> > Let me see if there's another option.
> 
> Amazingly:
> 
>   0x2e, 0x2e, 0x2e, 0x31, 0xc0
> 
> seems to actually work.. I've build and ran and decoded the below on
> 32bit and 64bit (arguably on the same 64bit host).

Not really amazing...
In 64bit mode all accesses to 32bit registers zero the
high bits.
So 'xor %eax,%eax' zeros all of %rax in 64bit mode.
So three segment override prefixes will extend it to 5 bytes.

Think I'd pick the FS or GS override (0x64 or 0x65).
Just in case someone decides that CS/DS/ES/SS prefix will
mean something else in 64bit mode.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v3 6/6] KVM: x86: allow defining return-0 static calls

[Maxim Levitsky]

On Fri, 2022-03-18 at 17:29 +0100, Paolo Bonzini wrote:
> On 3/17/22 18:43, Maxim Levitsky wrote:
> > diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
> > index 20f64e07e359..3388072b2e3b 100644
> > --- a/arch/x86/include/asm/kvm-x86-ops.h
> > +++ b/arch/x86/include/asm/kvm-x86-ops.h
> > @@ -88,7 +88,7 @@ KVM_X86_OP(deliver_interrupt)
> >   KVM_X86_OP_OPTIONAL(sync_pir_to_irr)
> >   KVM_X86_OP_OPTIONAL_RET0(set_tss_addr)
> >   KVM_X86_OP_OPTIONAL_RET0(set_identity_map_addr)
> > -KVM_X86_OP_OPTIONAL_RET0(get_mt_mask)
> > +KVM_X86_OP(get_mt_mask)
> >   KVM_X86_OP(load_mmu_pgd)
> >   KVM_X86_OP(has_wbinvd_exit)
> >   KVM_X86_OP(get_l2_tsc_offset)
> > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > index a09b4f1a18f6..0c09292b0611 100644
> > --- a/arch/x86/kvm/svm/svm.c
> > +++ b/arch/x86/kvm/svm/svm.c
> > @@ -4057,6 +4057,11 @@ static bool svm_has_emulated_msr(struct kvm *kvm, u32 index)
> >          return true;
> >   }
> >   
> > +static u64 svm_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
> > +{
> > +       return 0;
> > +}
> > +
> >   static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
> >   {
> >          struct vcpu_svm *svm = to_svm(vcpu);
> > @@ -4718,6 +4723,7 @@ static struct kvm_x86_ops svm_x86_ops __initdata = {
> >          .check_apicv_inhibit_reasons = avic_check_apicv_inhibit_reasons,
> >          .apicv_post_state_restore = avic_apicv_post_state_restore,
> >   
> > +       .get_mt_mask = svm_get_mt_mask,
> >          .get_exit_info = svm_get_exit_info,
> >   
> >          .vcpu_after_set_cpuid = svm_vcpu_after_set_cpuid,
> 
> Thanks, I'll send it as a complete patch.  Please reply there with your 
> Signed-off-by.


Honestly, I haven't meant to include this as a fix, but only as a proof of the issue,
but I don't have anything against using this until the underlying issue is fixed.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky

> 
> Related to this, I don't see anything in arch/x86/kernel/static_call.c 
> that limits this code to x86-64:
> 
>                  if (func == &__static_call_return0) {
>                          emulate = code;
>                          code = &xor5rax;
>                  }
> 
> 
> On 32-bit, it will be patched as "dec ax; xor eax, eax" or something 
> like that.  Fortunately it doesn't corrupt any callee-save register but 
> it is not just a bit funky, it's also not a single instruction.
> 
> Paolo
>