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@xxxxxxxxxx>
> ---
>  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;