[PATCH 05/10] KVM: SVM: Drop vcpu_svm's pointless avic_backing_page field

Sean Christopherson posted 10 patches 2 years, 4 months ago
[PATCH 05/10] KVM: SVM: Drop vcpu_svm's pointless avic_backing_page field
Posted by Sean Christopherson 2 years, 4 months ago
Drop vcpu_svm's avic_backing_page pointer and instead grab the physical
address of KVM's vAPIC page directly from the source.  Getting a physical
address from a kernel virtual address is not an expensive operation, and
getting the physical address from a struct page is *more* expensive for
CONFIG_SPARSEMEM=y kernels.  Regardless, none of the paths that consume
the address are hot paths, i.e. shaving cycles is not a priority.

Eliminating the "cache" means KVM doesn't have to worry about the cache
being invalid, which will simplify a future fix when dealing with vCPU IDs
that are too big.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/avic.c | 4 +---
 arch/x86/kvm/svm/svm.h  | 1 -
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 954bdb45033b..e49b682c8469 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -243,7 +243,7 @@ int avic_vm_init(struct kvm *kvm)
 
 static phys_addr_t avic_get_backing_page_address(struct vcpu_svm *svm)
 {
-	return __sme_set(page_to_phys(svm->avic_backing_page));
+	return __sme_set(__pa(svm->vcpu.arch.apic->regs));
 }
 
 void avic_init_vmcb(struct vcpu_svm *svm, struct vmcb *vmcb)
@@ -305,8 +305,6 @@ static int avic_init_backing_page(struct kvm_vcpu *vcpu)
 			return ret;
 	}
 
-	svm->avic_backing_page = virt_to_page(vcpu->arch.apic->regs);
-
 	/* Setting AVIC backing page address in the phy APIC ID table */
 	entry = avic_get_physical_id_entry(vcpu, id);
 	if (!entry)
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 2237230aad98..a9fde1bb85ee 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -261,7 +261,6 @@ struct vcpu_svm {
 
 	u32 ldr_reg;
 	u32 dfr_reg;
-	struct page *avic_backing_page;
 	u64 *avic_physical_id_cache;
 
 	/*
-- 
2.41.0.694.ge786442a9b-goog
Re: [PATCH 05/10] KVM: SVM: Drop vcpu_svm's pointless avic_backing_page field
Posted by Maxim Levitsky 2 years, 2 months ago
У вт, 2023-08-15 у 14:35 -0700, Sean Christopherson пише:
> Drop vcpu_svm's avic_backing_page pointer and instead grab the physical
> address of KVM's vAPIC page directly from the source.  Getting a physical
> address from a kernel virtual address is not an expensive operation, and
> getting the physical address from a struct page is *more* expensive for
> CONFIG_SPARSEMEM=y kernels.  Regardless, none of the paths that consume
> the address are hot paths, i.e. shaving cycles is not a priority.
> 
> Eliminating the "cache" means KVM doesn't have to worry about the cache
> being invalid, which will simplify a future fix when dealing with vCPU IDs
> that are too big.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/svm/avic.c | 4 +---
>  arch/x86/kvm/svm/svm.h  | 1 -
>  2 files changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index 954bdb45033b..e49b682c8469 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -243,7 +243,7 @@ int avic_vm_init(struct kvm *kvm)
>  
>  static phys_addr_t avic_get_backing_page_address(struct vcpu_svm *svm)
>  {
> -	return __sme_set(page_to_phys(svm->avic_backing_page));
> +	return __sme_set(__pa(svm->vcpu.arch.apic->regs));

I overall agree with this patch however the old code was safer:

svm->avic_backing_page is set to physical address of the apic registers
only in the avic_init_backing_page() and after checking the 
vcpu->arch.apic->regs != NULL and now in theory NULL vcpu->arch.apic->regs
are not checked.

I know that you later add a patch which adds a similar warning, but I prefer that
you fold it with this patch.
 
>  }
>  
>  void avic_init_vmcb(struct vcpu_svm *svm, struct vmcb *vmcb)
> @@ -305,8 +305,6 @@ static int avic_init_backing_page(struct kvm_vcpu *vcpu)
>  			return ret;
>  	}
>  
> -	svm->avic_backing_page = virt_to_page(vcpu->arch.apic->regs);
> -
>  	/* Setting AVIC backing page address in the phy APIC ID table */
>  	entry = avic_get_physical_id_entry(vcpu, id);
>  	if (!entry)
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index 2237230aad98..a9fde1bb85ee 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -261,7 +261,6 @@ struct vcpu_svm {
>  
>  	u32 ldr_reg;
>  	u32 dfr_reg;
> -	struct page *avic_backing_page;
>  	u64 *avic_physical_id_cache;
>  
>  	/*


Best regards,
	Maxim Levitsky