Re: [PATCH] KVM: x86: add a bool variable to distinguish whether to use PVIPI

* Re: [PATCH] KVM: x86: add a bool variable to distinguish whether to use PVIPI
       [not found] <1655124522-42030-1-git-send-email-wangguangju@baidu.com>
@ 2022-06-13 17:16 ` Sean Christopherson
  2022-06-14  2:54   ` Chao Gao
  0 siblings, 1 reply; 6+ messages in thread
From: Sean Christopherson @ 2022-06-13 17:16 UTC (permalink / raw)
  To: wangguangju
  Cc: pbonzini, vkuznets, wanpengli, jmattson, joro, kvm, dave.hansen,
	tglx, mingo, x86, linux-kernel

The shortlog is not at all helpful, it doesn't say anything about what actual
functional change.

  KVM: x86: Don't advertise PV IPI to userspace if IPIs are virtualized

On Mon, Jun 13, 2022, wangguangju wrote:
> Commit d588bb9be1da ("KVM: VMX: enable IPI virtualization")
> enable IPI virtualization in Intel SPR platform.There is no point
> in using PVIPI if IPIv is supported, it doesn't work less good
> with PVIPI than without it.
> 
> So add a bool variable to distinguish whether to use PVIPI.

Similar complaint with the changelog, it doesn't actually call out why PV IPIs
are unwanted.

  Don't advertise PV IPI support to userspace if IPI virtualization is
  supported by the CPU.  Hardware virtualization of IPIs more performant
  as senders do not need to exit.

That said, I'm not sure that KVM should actually hide PV_SEND_IPI.  KVM still
supports the feature, and unlike sched_info_on(), IPI virtualization is platform
dependent and not fully controlled by software.  E.g. hiding PV_SEND_IPI could
cause problems when migrating from a platform without IPIv to a platform with IPIv,
as a paranoid VMM might complain that an exposed feature isn't supported by KVM.

There's also the question of what to do about AVIC.  AVIC has many more inhibits
than APICv, e.g. an x2APIC guest running on hardware that doesn't accelerate x2APIC
IPIs will probably be better off with PV IPIs.

Given that userspace should have read access to the module param, I'm tempted to
say KVM should let userspace make the decision of whether or not to advertise PV
IPIs to the guest.

> Signed-off-by: wangguangju <wangguangju@baidu.com>
> ---
>  arch/x86/include/asm/kvm_host.h | 1 +
>  arch/x86/kvm/cpuid.c            | 4 +++-
>  arch/x86/kvm/vmx/vmx.c          | 1 +
>  arch/x86/kvm/x86.c              | 3 +++
>  4 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index a4b9282..239c1a992 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1671,6 +1671,7 @@ void kvm_fire_mask_notifiers(struct kvm *kvm, unsigned irqchip, unsigned pin,
>  			     bool mask);
>  
>  extern bool tdp_enabled;
> +extern bool kvm_ipiv_cap_supported;

Please use "struct kvm_caps", which was added specifically to avoid these one-off
bools.

>  u64 vcpu_tsc_khz(struct kvm_vcpu *vcpu);
>  
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index d47222a..9643572 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -1049,7 +1049,6 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
>  			     (1 << KVM_FEATURE_PV_UNHALT) |
>  			     (1 << KVM_FEATURE_PV_TLB_FLUSH) |
>  			     (1 << KVM_FEATURE_ASYNC_PF_VMEXIT) |
> -			     (1 << KVM_FEATURE_PV_SEND_IPI) |
>  			     (1 << KVM_FEATURE_POLL_CONTROL) |
>  			     (1 << KVM_FEATURE_PV_SCHED_YIELD) |
>  			     (1 << KVM_FEATURE_ASYNC_PF_INT);
> @@ -1057,6 +1056,9 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
>  		if (sched_info_on())
>  			entry->eax |= (1 << KVM_FEATURE_STEAL_TIME);
>  
> +		if (!kvm_ipiv_cap_supported)
> +			entry->eax |= (1 << KVM_FEATURE_PV_SEND_IPI);
> +
>  		entry->ebx = 0;
>  		entry->ecx = 0;
>  		entry->edx = 0;
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index f741de4..21b67f4 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -4490,6 +4490,7 @@ static int vmx_alloc_ipiv_pid_table(struct kvm *kvm)
>  		return -ENOMEM;
>  
>  	kvm_vmx->pid_table = (void *)page_address(pages);
> +	kvm_ipiv_cap_supported = true;

This is far too late, as allocation of the table doesn't happen until the first
VM is created.

E.g.

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index b959fe24c13b..73973b5901a3 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -8221,6 +8221,7 @@ static __init int hardware_setup(void)
        kvm_caps.tsc_scaling_ratio_frac_bits = 48;
        kvm_caps.has_bus_lock_exit = cpu_has_vmx_bus_lock_detection();
        kvm_caps.has_notify_vmexit = cpu_has_notify_vmexit();
+       kvm_caps.has_ipi_virtualization = enable_ipiv;

        set_bit(0, vmx_vpid_bitmap); /* 0 is reserved for host */

diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index 501b884b8cc4..9b80aa67349f 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -23,6 +23,7 @@ struct kvm_caps {
        bool has_bus_lock_exit;
        /* notify VM exit supported? */
        bool has_notify_vmexit;
+       bool has_ipi_virtualization;

        u64 supported_mce_cap;
        u64 supported_xcr0;


>  	return 0;
>  }
>  
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index f9d0c56..099f76f 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -226,6 +226,9 @@ EXPORT_SYMBOL_GPL(enable_apicv);
>  u64 __read_mostly host_xss;
>  EXPORT_SYMBOL_GPL(host_xss);
>  
> +bool __read_mostly kvm_ipiv_cap_supported = false;
> +EXPORT_SYMBOL_GPL(kvm_ipiv_cap_supported);
> +
>  const struct _kvm_stats_desc kvm_vm_stats_desc[] = {
>  	KVM_GENERIC_VM_STATS(),
>  	STATS_DESC_COUNTER(VM, mmu_shadow_zapped),
> -- 
> 2.9.4
> 

^ permalink raw reply related	[flat|nested] 6+ messages in thread