lkml.org 
[lkml]   [2022]   [Nov]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH v10 056/108] KVM: TDX: don't request KVM_REQ_APIC_PAGE_RELOAD
    Date
    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.
    \
     
     \ /
      Last update: 2022-11-22 00:56    [W:4.180 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site