| Message ID | f0f134dcf59f901e4b8960c7b3f242dcd42b1c40.1667110240.git.isaku.yamahata@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM TDX basic feature support | expand |
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.
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.
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.
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); }