[v10,056/108] KVM: TDX: don't request KVM_REQ_APIC_PAGE_RELOAD

Message ID f0f134dcf59f901e4b8960c7b3f242dcd42b1c40.1667110240.git.isaku.yamahata@intel.com
State New
Headers
Series KVM TDX basic feature support |

Commit Message

Isaku Yamahata Oct. 30, 2022, 6:22 a.m. UTC
  From: Isaku Yamahata <isaku.yamahata@intel.com>

TDX doesn't need APIC page depending on vapic and its callback is
WARN_ON_ONCE(is_tdx).  To avoid unnecessary overhead and WARN_ON_ONCE(),
skip requesting KVM_REQ_APIC_PAGE_RELOAD when TD.

  WARNING: arch/x86/kvm/vmx/main.c:696 vt_set_apic_access_page_addr+0x3c/0x50 [kvm_intel]
  RIP: 0010:vt_set_apic_access_page_addr+0x3c/0x50 [kvm_intel]
  Call Trace:
   vcpu_enter_guest+0x145d/0x24d0 [kvm]
   kvm_arch_vcpu_ioctl_run+0x25d/0xcc0 [kvm]
   kvm_vcpu_ioctl+0x414/0xa30 [kvm]
   __x64_sys_ioctl+0xc0/0x100
   do_syscall_64+0x39/0xc0
   entry_SYSCALL_64_after_hwframe+0x44/0xae

Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
---
 arch/x86/kvm/x86.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
  

Comments

Kai Huang Nov. 21, 2022, 11:55 p.m. UTC | #1
On Sat, 2022-10-29 at 23:22 -0700, isaku.yamahata@intel.com wrote:
> From: Isaku Yamahata <isaku.yamahata@intel.com>
> 
> TDX doesn't need APIC page depending on vapic and its callback is
> WARN_ON_ONCE(is_tdx).  To avoid unnecessary overhead and WARN_ON_ONCE(),
> skip requesting KVM_REQ_APIC_PAGE_RELOAD when TD.
> 
>   WARNING: arch/x86/kvm/vmx/main.c:696 vt_set_apic_access_page_addr+0x3c/0x50 [kvm_intel]
>   RIP: 0010:vt_set_apic_access_page_addr+0x3c/0x50 [kvm_intel]
>   Call Trace:
>    vcpu_enter_guest+0x145d/0x24d0 [kvm]
>    kvm_arch_vcpu_ioctl_run+0x25d/0xcc0 [kvm]
>    kvm_vcpu_ioctl+0x414/0xa30 [kvm]
>    __x64_sys_ioctl+0xc0/0x100
>    do_syscall_64+0x39/0xc0
>    entry_SYSCALL_64_after_hwframe+0x44/0xae
> 
> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> ---
>  arch/x86/kvm/x86.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 3868605462ed..5dadd0f9a10e 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -10487,7 +10487,9 @@ void kvm_arch_mmu_notifier_invalidate_range(struct kvm *kvm,
>  	 * Update it when it becomes invalid.
>  	 */
>  	apic_address = gfn_to_hva(kvm, APIC_DEFAULT_PHYS_BASE >> PAGE_SHIFT);
> -	if (start <= apic_address && apic_address < end)
> +	/* TDX doesn't need APIC page. */
> +	if (kvm->arch.vm_type != KVM_X86_TDX_VM &&
> +	    start <= apic_address && apic_address < end)
>  		kvm_make_all_cpus_request(kvm, KVM_REQ_APIC_PAGE_RELOAD);
>  }
>  

In patch "[PATCH v10 105/108] KVM: TDX: Add methods to ignore accesses to CPU
state", you have:

+static void vt_set_apic_access_page_addr(struct kvm_vcpu *vcpu)
+{
+	if (WARN_ON_ONCE(is_td_vcpu(vcpu)))
+		return;
+
+	vmx_set_apic_access_page_addr(vcpu);
+}

If you drop the WARN_ON_ONCE() above, you can just drop this patch.

For this particular case, I don't find it is quite necessary to change the
common x86 code as done in this patch.  In fact, SVM doesn't have a
set_apic_access_page_addr() callback which is consistent with just return if VM
is TD in vt_set_apic_access_page_addr().

Also, I don't particularly like the idea of having a lot of "is_td(kvm)" in the
common x86 code as if similar technology happens in the future, you will need to
have another "is_td_similar_vm(kvm)" thing.

If modifying common x86 code is necessary, then it would make more sense to
introduce some common flag, and make TD guest set that flag.

Btw, this patch just comes out of blue from the  middle of a bunch of MMU
patches.  Shouldn't it be moved to "patches which handles interrupt related
staff"?

Btw2, by saying above, does it make sense to split patch "[PATCH v10 105/108]
KVM: TDX: Add methods to ignore accesses to CPU state" based on category such as
MMU/interrupt, etc?  Particularly, in that patch, some callbacks have WARN() or
KVM_BUG_ON() against TD guest, but some don't.  The logic behind those decisions
highly depend on previous patches.  To me, it makes more sense to just move
logic related things together.
  
Isaku Yamahata Dec. 16, 2022, 12:11 a.m. UTC | #2
On Mon, Nov 21, 2022 at 11:55:58PM +0000,
"Huang, Kai" <kai.huang@intel.com> wrote:

> On Sat, 2022-10-29 at 23:22 -0700, isaku.yamahata@intel.com wrote:
> > From: Isaku Yamahata <isaku.yamahata@intel.com>
> > 
> > TDX doesn't need APIC page depending on vapic and its callback is
> > WARN_ON_ONCE(is_tdx).  To avoid unnecessary overhead and WARN_ON_ONCE(),
> > skip requesting KVM_REQ_APIC_PAGE_RELOAD when TD.
> > 
> >   WARNING: arch/x86/kvm/vmx/main.c:696 vt_set_apic_access_page_addr+0x3c/0x50 [kvm_intel]
> >   RIP: 0010:vt_set_apic_access_page_addr+0x3c/0x50 [kvm_intel]
> >   Call Trace:
> >    vcpu_enter_guest+0x145d/0x24d0 [kvm]
> >    kvm_arch_vcpu_ioctl_run+0x25d/0xcc0 [kvm]
> >    kvm_vcpu_ioctl+0x414/0xa30 [kvm]
> >    __x64_sys_ioctl+0xc0/0x100
> >    do_syscall_64+0x39/0xc0
> >    entry_SYSCALL_64_after_hwframe+0x44/0xae
> > 
> > Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> > ---
> >  arch/x86/kvm/x86.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 3868605462ed..5dadd0f9a10e 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -10487,7 +10487,9 @@ void kvm_arch_mmu_notifier_invalidate_range(struct kvm *kvm,
> >  	 * Update it when it becomes invalid.
> >  	 */
> >  	apic_address = gfn_to_hva(kvm, APIC_DEFAULT_PHYS_BASE >> PAGE_SHIFT);
> > -	if (start <= apic_address && apic_address < end)
> > +	/* TDX doesn't need APIC page. */
> > +	if (kvm->arch.vm_type != KVM_X86_TDX_VM &&
> > +	    start <= apic_address && apic_address < end)
> >  		kvm_make_all_cpus_request(kvm, KVM_REQ_APIC_PAGE_RELOAD);
> >  }
> >  
> 
> In patch "[PATCH v10 105/108] KVM: TDX: Add methods to ignore accesses to CPU
> state", you have:
> 
> +static void vt_set_apic_access_page_addr(struct kvm_vcpu *vcpu)
> +{
> +	if (WARN_ON_ONCE(is_td_vcpu(vcpu)))
> +		return;
> +
> +	vmx_set_apic_access_page_addr(vcpu);
> +}
> 
> If you drop the WARN_ON_ONCE() above, you can just drop this patch.
> 
> For this particular case, I don't find it is quite necessary to change the
> common x86 code as done in this patch.  In fact, SVM doesn't have a
> set_apic_access_page_addr() callback which is consistent with just return if VM
> is TD in vt_set_apic_access_page_addr().
> 

Oh, yes. I will drop this patch with removing WARN_ON_ONCE().


> Also, I don't particularly like the idea of having a lot of "is_td(kvm)" in the
> common x86 code as if similar technology happens in the future, you will need to
> have another "is_td_similar_vm(kvm)" thing.

Currently KVM_CAP_VM_TYPES has such check in x86 kvm common code.


> If modifying common x86 code is necessary, then it would make more sense to
> introduce some common flag, and make TD guest set that flag.
> 
> Btw, this patch just comes out of blue from the  middle of a bunch of MMU
> patches.  Shouldn't it be moved to "patches which handles interrupt related
> staff"?
> 
> Btw2, by saying above, does it make sense to split patch "[PATCH v10 105/108]
> KVM: TDX: Add methods to ignore accesses to CPU state" based on category such as
> MMU/interrupt, etc?  Particularly, in that patch, some callbacks have WARN() or
> KVM_BUG_ON() against TD guest, but some don't.  The logic behind those decisions
> highly depend on previous patches.  To me, it makes more sense to just move
> logic related things together.

Ok, I'll split it up to cpu states/KVM MMU/interrupt parts.
  
Kai Huang Dec. 16, 2022, 12:31 a.m. UTC | #3
On Thu, 2022-12-15 at 16:11 -0800, Isaku Yamahata wrote:
> > Btw2, by saying above, does it make sense to split patch "[PATCH v10
> > 105/108]
> > KVM: TDX: Add methods to ignore accesses to CPU state" based on category
> > such as
> > MMU/interrupt, etc?  Particularly, in that patch, some callbacks have WARN()
> > or
> > KVM_BUG_ON() against TD guest, but some don't.  The logic behind those
> > decisions
> > highly depend on previous patches.  To me, it makes more sense to just move
> > logic related things together.
> 
> Ok, I'll split it up to cpu states/KVM MMU/interrupt parts.

If I recall correctly, originally (long time ago before starting to upstream),
what we did was we have a patch to make all kvm_x86_ops callback KVM_BUG_ON()
for TDX guest, then we fix those KVM_BUG_ON() in later patches in separate
patches.

We don't need to do the exact same way, but this also seems reasonable to me. 
For instance, at the beginning we can mark KVM_BUG_ON() for all callbacks which
reads/writes CPU states (which is reasonable anyway), and in later patches we
remove the KVM_BUG_ON() if needed when handling specific logic.

Simply my 2cents above.  Just for your reference.  My real comment is we should
put relevant parts together so it's easy to review.
  

Patch

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 3868605462ed..5dadd0f9a10e 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -10487,7 +10487,9 @@  void kvm_arch_mmu_notifier_invalidate_range(struct kvm *kvm,
 	 * Update it when it becomes invalid.
 	 */
 	apic_address = gfn_to_hva(kvm, APIC_DEFAULT_PHYS_BASE >> PAGE_SHIFT);
-	if (start <= apic_address && apic_address < end)
+	/* TDX doesn't need APIC page. */
+	if (kvm->arch.vm_type != KVM_X86_TDX_VM &&
+	    start <= apic_address && apic_address < end)
 		kvm_make_all_cpus_request(kvm, KVM_REQ_APIC_PAGE_RELOAD);
 }