From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id BBAAFC4332F for ; Wed, 14 Dec 2022 19:44:00 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=EmaFiMHBfLccpU6SA8odRwNvqOSZkK63uyhA4hVQKPE=; b=GcU8akGcE/Wshe EMRbJM50BcDdE7+t7yvZLou0E1HkK0bVOpQZPnC69vr09AWhUe/x58zEuHBizNzcyRubGLBEkCzeQ XRYdwfnJ8aM9a01uO7Ul0KNUXJI+biuioSB0Txd7ea29QoT9QIPKYavT++6Vist/r5JRSSngh6L5M npXKVuoiI4YmU6hElxNlML6W6TOUcahE/afqJvOnBj/w0IsuJz+HmLqrfbye8Vp3rDRMJqTH7cPvJ jA2rJty24PqVlQGGw2CMHwCYDFG9uYkVv/3otbb3emcUpaQjWNx6iyRGd3UQW0TMj35zHSk8Npg/u fiRZUrbQDzGqQlSRoQTw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1p5XeN-002F2W-C8; Wed, 14 Dec 2022 19:42:43 +0000 Received: from mail-pj1-x102f.google.com ([2607:f8b0:4864:20::102f]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1p5XeJ-002Evn-32 for linux-arm-kernel@lists.infradead.org; Wed, 14 Dec 2022 19:42:40 +0000 Received: by mail-pj1-x102f.google.com with SMTP id v13-20020a17090a6b0d00b00219c3be9830so289669pjj.4 for ; Wed, 14 Dec 2022 11:42:35 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=ku5jbuAQtkSIptaPybAlGHrWDUjqz+/sshvQ4yTXVKM=; b=gFMFZTb9mro+iD15z0WKOAxzMvbSbzWPvCwsv8UgFNQWpsUuXMnYS1PD3tfN9vh+X7 TxVm4jwzNUIAZ9XHYIdI0sTAawVhxSLk731/QHHR0cEuSWQT3MUJ5gn4LDqw2kfypG1G 0bQIwCJGwPZ6AKpAq3T0x0kfjiXwM8lxNtxri7D8qhKTIK3pwuhtPFeLIU9CPCLftyT7 N4dlul+9Y12mjdb0gPr7ygr3Diy6uRM/rtN0bgjbPYE8emVLaRsMCD/6CMXfynWZUtV8 FUhysphsdX7xRND/cGG0xWBHmRXb2BmyzX1vpYoDF3NQ54Xr6HVNV1VJIIrs+JDUy6J7 CZcQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=ku5jbuAQtkSIptaPybAlGHrWDUjqz+/sshvQ4yTXVKM=; b=gZB2VBpqdcIU0jYpEQ7vYl5Axwp57DkZ4E9dvnTVcXp3bNmybW1UmcD3TMhVxhJ1nk oZbHY4/xy5hSu65f2sZoZAjwCEu1QRyuzdJ3YqKRetHn48EnDLnj6I2g08VmmJydS4FP eV7WXsKJpBBSqU4Qi+zuxzriUevTETi9mjUIMlVxL8RkYrmDanh1TwscHky0ssAwSMxb aKLmkM59VKu3aul8S+f8GtLs4qmnQ2vDENirGfRb1y8OLrK7CI6tQ/Dd3E6O4Lt0G05m aFpRnA22w/Z3ZG7ITUwPiTAvvstM7+56B6gujneu1bkJRdxc1NDbyCwuTrU0KJTKyzjm MGGQ== X-Gm-Message-State: ANoB5pkTUT/sEyzvr1TATblsNHn6CvCEFhJ6uGZMo2wvZHgyPyAtuOoq cgbol+JMGvlH78xl6yFdpWuR5w== X-Google-Smtp-Source: AA0mqf475+T6XuYTtV2Re11WEmkxZATJINn1AOgHBS3Ie2ZV2UqJ+on0NpT5StpdHjXPTHMnUH99rA== X-Received: by 2002:a17:90a:2a4f:b0:219:828e:ba2 with SMTP id d15-20020a17090a2a4f00b00219828e0ba2mr871582pjg.0.1671046955097; Wed, 14 Dec 2022 11:42:35 -0800 (PST) Received: from google.com (7.104.168.34.bc.googleusercontent.com. [34.168.104.7]) by smtp.gmail.com with ESMTPSA id a1-20020a170902710100b0018b025d9a40sm2204215pll.256.2022.12.14.11.42.33 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 14 Dec 2022 11:42:33 -0800 (PST) Date: Wed, 14 Dec 2022 19:42:30 +0000 From: Sean Christopherson To: Lai Jiangshan Cc: David Matlack , Oliver Upton , "Yang, Weijiang" , Paolo Bonzini , Marc Zyngier , James Morse , Alexandru Elisei , Suzuki K Poulose , Huacai Chen , Aleksandar Markovic , Anup Patel , Atish Patra , Paul Walmsley , Palmer Dabbelt , Albert Ou , Andrew Morton , Anshuman Khandual , "Amit, Nadav" , "Matthew Wilcox (Oracle)" , Vlastimil Babka , "Liam R. Howlett" , Suren Baghdasaryan , Peter Xu , xu xin , Arnd Bergmann , Yu Zhao , Colin Cross , Hugh Dickins , Ben Gardon , Mingwei Zhang , Krish Sadhukhan , Ricardo Koller , Jing Zhang , "linux-arm-kernel@lists.infradead.org" , "kvmarm@lists.linux.dev" , "kvmarm@lists.cs.columbia.edu" , "linux-mips@vger.kernel.org" , "kvm@vger.kernel.org" , "kvm-riscv@lists.infradead.org" , "linux-riscv@lists.infradead.org" Subject: Re: [RFC PATCH 01/37] KVM: x86/mmu: Store the address space ID directly in kvm_mmu_page_role Message-ID: References: <20221208193857.4090582-1-dmatlack@google.com> <20221208193857.4090582-2-dmatlack@google.com> <22fe2332-497e-fe30-0155-e026b0eded97@intel.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20221214_114239_146536_E751542D X-CRM114-Status: GOOD ( 33.24 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Wed, Dec 14, 2022, Lai Jiangshan wrote: > On Tue, Dec 13, 2022 at 1:47 AM Sean Christopherson wrote: > > > > > My preference would be to leave .smm in x86's page role. IMO, defining multiple > > address spaces to support SMM emulation was a mistake that should be contained to > > SMM, i.e. should never be used for any other feature. And with CONFIG_KVM_SMM, > > even x86 can opt out. > > > > > I think the name ASID in kvm/x86 should be used for vmcb's ASID, > vmcs's VPID, and PCID. Using the name ASID for other purposes > would only result in unnecessary confusion. I agree in principle, but at this point fixing the problem would require a lot of churn since "as_id" is pervasive throughout the memslots code. It should be possible though, as I don't think anything in KVM's uAPI explicitly takes an as_id, i.e. it's KVM-internal terminology for the most part. > There is a bug for shadow paging when it uses two separate sets > of memslots which are using two sets of rmap and page-tracking. > > When SMM world is writing to a non-SMM page which happens to be > a guest pagetable in the non-SMM world, the write operation will > go smoothly without specially handled and the shadow page for the guest > pagetable is neither unshadowed nor marked unsync. The shadow paging > code is unaware that the shadow page has deviated from the guest > pagetable. Won't the unsync code work as intended? for_each_gfn_valid_sp_with_gptes() doesn't consume the slot and I don't see any explicit filtering on role.smm, i.e. should unsync all SPTEs for the gfn. Addressing the page-track case is a bit gross, but doesn't appear to be too difficult. I wouldn't be surprised if there are other SMM => non-SMM bugs lurking though. --- arch/x86/include/asm/kvm_page_track.h | 2 +- arch/x86/kvm/mmu/mmu.c | 7 +++--- arch/x86/kvm/mmu/mmu_internal.h | 3 ++- arch/x86/kvm/mmu/page_track.c | 32 +++++++++++++++++++-------- arch/x86/kvm/mmu/spte.c | 2 +- 5 files changed, 31 insertions(+), 15 deletions(-) diff --git a/arch/x86/include/asm/kvm_page_track.h b/arch/x86/include/asm/kvm_page_track.h index eb186bc57f6a..fdd9de31e160 100644 --- a/arch/x86/include/asm/kvm_page_track.h +++ b/arch/x86/include/asm/kvm_page_track.h @@ -63,7 +63,7 @@ void kvm_slot_page_track_add_page(struct kvm *kvm, void kvm_slot_page_track_remove_page(struct kvm *kvm, struct kvm_memory_slot *slot, gfn_t gfn, enum kvm_page_track_mode mode); -bool kvm_slot_page_track_is_active(struct kvm *kvm, +bool kvm_slot_page_track_is_active(struct kvm_vcpu *vcpu, const struct kvm_memory_slot *slot, gfn_t gfn, enum kvm_page_track_mode mode); diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c index 254bc46234e0..1c2200042133 100644 --- a/arch/x86/kvm/mmu/mmu.c +++ b/arch/x86/kvm/mmu/mmu.c @@ -2715,9 +2715,10 @@ static void kvm_unsync_page(struct kvm *kvm, struct kvm_mmu_page *sp) * were marked unsync (or if there is no shadow page), -EPERM if the SPTE must * be write-protected. */ -int mmu_try_to_unsync_pages(struct kvm *kvm, const struct kvm_memory_slot *slot, +int mmu_try_to_unsync_pages(struct kvm_vcpu *vcpu, const struct kvm_memory_slot *slot, gfn_t gfn, bool can_unsync, bool prefetch) { + struct kvm *kvm = vcpu->kvm; struct kvm_mmu_page *sp; bool locked = false; @@ -2726,7 +2727,7 @@ int mmu_try_to_unsync_pages(struct kvm *kvm, const struct kvm_memory_slot *slot, * track machinery is used to write-protect upper-level shadow pages, * i.e. this guards the role.level == 4K assertion below! */ - if (kvm_slot_page_track_is_active(kvm, slot, gfn, KVM_PAGE_TRACK_WRITE)) + if (kvm_slot_page_track_is_active(vcpu, slot, gfn, KVM_PAGE_TRACK_WRITE)) return -EPERM; /* @@ -4127,7 +4128,7 @@ static bool page_fault_handle_page_track(struct kvm_vcpu *vcpu, * guest is writing the page which is write tracked which can * not be fixed by page fault handler. */ - if (kvm_slot_page_track_is_active(vcpu->kvm, fault->slot, fault->gfn, KVM_PAGE_TRACK_WRITE)) + if (kvm_slot_page_track_is_active(vcpu, fault->slot, fault->gfn, KVM_PAGE_TRACK_WRITE)) return true; return false; diff --git a/arch/x86/kvm/mmu/mmu_internal.h b/arch/x86/kvm/mmu/mmu_internal.h index ac00bfbf32f6..38040ab27986 100644 --- a/arch/x86/kvm/mmu/mmu_internal.h +++ b/arch/x86/kvm/mmu/mmu_internal.h @@ -156,7 +156,8 @@ static inline bool kvm_mmu_page_ad_need_write_protect(struct kvm_mmu_page *sp) return kvm_x86_ops.cpu_dirty_log_size && sp->role.guest_mode; } -int mmu_try_to_unsync_pages(struct kvm *kvm, const struct kvm_memory_slot *slot, +int mmu_try_to_unsync_pages(struct kvm_vcpu *vcpu, + const struct kvm_memory_slot *slot, gfn_t gfn, bool can_unsync, bool prefetch); void kvm_mmu_gfn_disallow_lpage(const struct kvm_memory_slot *slot, gfn_t gfn); diff --git a/arch/x86/kvm/mmu/page_track.c b/arch/x86/kvm/mmu/page_track.c index 2e09d1b6249f..0e9bc837257e 100644 --- a/arch/x86/kvm/mmu/page_track.c +++ b/arch/x86/kvm/mmu/page_track.c @@ -18,6 +18,7 @@ #include "mmu.h" #include "mmu_internal.h" +#include "smm.h" bool kvm_page_track_write_tracking_enabled(struct kvm *kvm) { @@ -171,27 +172,40 @@ void kvm_slot_page_track_remove_page(struct kvm *kvm, } EXPORT_SYMBOL_GPL(kvm_slot_page_track_remove_page); +static bool __kvm_slot_page_track_is_active(const struct kvm_memory_slot *slot, + gfn_t gfn, enum kvm_page_track_mode mode) +{ + int index; + + if (!slot) + return false; + + index = gfn_to_index(gfn, slot->base_gfn, PG_LEVEL_4K); + return !!READ_ONCE(slot->arch.gfn_track[mode][index]); +} + /* * check if the corresponding access on the specified guest page is tracked. */ -bool kvm_slot_page_track_is_active(struct kvm *kvm, +bool kvm_slot_page_track_is_active(struct kvm_vcpu *vcpu, const struct kvm_memory_slot *slot, gfn_t gfn, enum kvm_page_track_mode mode) { - int index; - if (WARN_ON(!page_track_mode_is_valid(mode))) return false; - if (!slot) - return false; - if (mode == KVM_PAGE_TRACK_WRITE && - !kvm_page_track_write_tracking_enabled(kvm)) + !kvm_page_track_write_tracking_enabled(vcpu->kvm)) return false; - index = gfn_to_index(gfn, slot->base_gfn, PG_LEVEL_4K); - return !!READ_ONCE(slot->arch.gfn_track[mode][index]); + if (__kvm_slot_page_track_is_active(slot, gfn, mode)) + return true; + + if (!is_smm(vcpu)) + return false; + + return __kvm_slot_page_track_is_active(gfn_to_memslot(vcpu->kvm, gfn), + gfn, mode); } void kvm_page_track_cleanup(struct kvm *kvm) diff --git a/arch/x86/kvm/mmu/spte.c b/arch/x86/kvm/mmu/spte.c index c0fd7e049b4e..89ddd113c1b9 100644 --- a/arch/x86/kvm/mmu/spte.c +++ b/arch/x86/kvm/mmu/spte.c @@ -220,7 +220,7 @@ bool make_spte(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp, * e.g. it's write-tracked (upper-level SPs) or has one or more * shadow pages and unsync'ing pages is not allowed. */ - if (mmu_try_to_unsync_pages(vcpu->kvm, slot, gfn, can_unsync, prefetch)) { + if (mmu_try_to_unsync_pages(vcpu, slot, gfn, can_unsync, prefetch)) { pgprintk("%s: found shadow page for %llx, marking ro\n", __func__, gfn); wrprot = true; base-commit: e0ef1f14e97ff65adf6e2157952dbbd1e482065c -- _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel