linux-kernel - Re: [PATCH v10 056/108] KVM: TDX: don't request KVM_REQ_APIC_PAGE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 21 Nov 2022 23:55:58 +0000
From:   "Huang, Kai" <kai.huang@...el.com>
To:     "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Yamahata, Isaku" <isaku.yamahata@...el.com>
CC:     "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "Shahar, Sagi" <sagis@...gle.com>,
        "Aktas, Erdem" <erdemaktas@...gle.com>,
        "isaku.yamahata@...il.com" <isaku.yamahata@...il.com>,
        "dmatlack@...gle.com" <dmatlack@...gle.com>,
        "Christopherson,, Sean" <seanjc@...gle.com>
Subject: Re: [PATCH v10 056/108] KVM: TDX: don't request
 KVM_REQ_APIC_PAGE_RELOAD

On Sat, 2022-10-29 at 23:22 -0700, isaku.yamahata@...el.com wrote:
> From: Isaku Yamahata <isaku.yamahata@...el.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@...el.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.