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 X-Spam-Level: X-Spam-Status: No, score=-17.7 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2F177C433DB for ; Fri, 15 Jan 2021 01:19:39 +0000 (UTC) Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id C92A722CBE for ; Fri, 15 Jan 2021 01:19:38 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org C92A722CBE Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=xen-devel-bounces@lists.xenproject.org Received: from list by lists.xenproject.org with outflank-mailman.67588.120736 (Exim 4.92) (envelope-from ) id 1l0Dlx-0005nI-2r; Fri, 15 Jan 2021 01:19:29 +0000 X-Outflank-Mailman: Message body and most headers restored to incoming version Received: by outflank-mailman (output) from mailman id 67588.120736; Fri, 15 Jan 2021 01:19:29 +0000 Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1l0Dlw-0005nB-Vx; Fri, 15 Jan 2021 01:19:28 +0000 Received: by outflank-mailman (input) for mailman id 67588; Fri, 15 Jan 2021 01:19:27 +0000 Received: from all-amaz-eas1.inumbo.com ([34.197.232.57] helo=us1-amaz-eas2.inumbo.com) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1l0Dlv-0005mv-A5 for xen-devel@lists.xenproject.org; Fri, 15 Jan 2021 01:19:27 +0000 Received: from mail.kernel.org (unknown [198.145.29.99]) by us1-amaz-eas2.inumbo.com (Halon) with ESMTPS id 957bc0bf-d501-46ae-a830-745278f966c1; Fri, 15 Jan 2021 01:19:26 +0000 (UTC) Received: by mail.kernel.org (Postfix) with ESMTPSA id D7B8A22CBE; Fri, 15 Jan 2021 01:19:24 +0000 (UTC) X-BeenThere: xen-devel@lists.xenproject.org List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xenproject.org Precedence: list Sender: "Xen-devel" X-Inumbo-ID: 957bc0bf-d501-46ae-a830-745278f966c1 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1610673565; bh=xs/qPhUtYesAYOA7eWcaedtlrANV3lJhceUMqLi8ykY=; h=Date:From:To:cc:Subject:In-Reply-To:References:From; b=CgpT02EPXSsArVyLkmTFeaLUCgEmKUEgWq6KyVU2OETwModrY9pXzhL3jIkpe3r3u PbBjiZG+WHxDeaBbczLNdn0+4uQSisl173w+KFvnnFrEtwJmKRTThOu364c9oF2FRc TY5qr5WCxaYh/AkM/3i2Bl7w6CjmbrehhzdDWK39XJTN1bxqkeB3Fs6zUw2j4T08Nh a/U1K0eC2eccIRBjM7yw4p78N3vl+0t1REz+YI/L2bRNExfjicwohZKAB9w3B8WZEY D7OY8S0DAHOBTjKcTioIJuFU9CJ4N0isAmcWR6ye1tPKVi9VfA5tmKK5JVWWo8q/y9 WD330uspo50Hg== Date: Thu, 14 Jan 2021 17:19:24 -0800 (PST) From: Stefano Stabellini X-X-Sender: sstabellini@sstabellini-ThinkPad-T480s To: Oleksandr Tyshchenko cc: xen-devel@lists.xenproject.org, Oleksandr Tyshchenko , Stefano Stabellini , Julien Grall , Volodymyr Babchuk , Andrew Cooper , George Dunlap , Ian Jackson , Jan Beulich , Wei Liu , =?UTF-8?Q?Roger_Pau_Monn=C3=A9?= , Julien Grall Subject: Re: [PATCH V4 16/24] xen/mm: Handle properly reference in set_foreign_p2m_entry() on Arm In-Reply-To: <1610488352-18494-17-git-send-email-olekstysh@gmail.com> Message-ID: References: <1610488352-18494-1-git-send-email-olekstysh@gmail.com> <1610488352-18494-17-git-send-email-olekstysh@gmail.com> User-Agent: Alpine 2.21 (DEB 202 2017-01-01) MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="8323329-766898220-1610673565=:31265" This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --8323329-766898220-1610673565=:31265 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT On Tue, 12 Jan 2021, Oleksandr Tyshchenko wrote: > From: Oleksandr Tyshchenko > > This patch implements reference counting of foreign entries in > in set_foreign_p2m_entry() on Arm. This is a mandatory action if > we want to run emulator (IOREQ server) in other than dom0 domain, > as we can't trust it to do the right thing if it is not running > in dom0. So we need to grab a reference on the page to avoid it > disappearing. > > It is valid to always pass "p2m_map_foreign_rw" type to > guest_physmap_add_entry() since the current and foreign domains > would be always different. A case when they are equal would be > rejected by rcu_lock_remote_domain_by_id(). Besides the similar > comment in the code put a respective ASSERT() to catch incorrect > usage in future. > > It was tested with IOREQ feature to confirm that all the pages given > to this function belong to a domain, so we can use the same approach > as for XENMAPSPACE_gmfn_foreign handling in xenmem_add_to_physmap_one(). > > This involves adding an extra parameter for the foreign domain to > set_foreign_p2m_entry() and a helper to indicate whether the arch > supports the reference counting of foreign entries and the restriction > for the hardware domain in the common code can be skipped for it. > > Signed-off-by: Oleksandr Tyshchenko > CC: Julien Grall > [On Arm only] > Tested-by: Wei Chen Acked-by: Stefano Stabellini > --- > Please note, this is a split/cleanup/hardening of Julien's PoC: > "Add support for Guest IO forwarding to a device emulator" > > Changes RFC -> V1: > - new patch, was split from: > "[RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features" > - rewrite a logic to handle properly reference in set_foreign_p2m_entry() > instead of treating foreign entries as p2m_ram_rw > > Changes V1 -> V2: > - rebase according to the recent changes to acquire_resource() > - update patch description > - introduce arch_refcounts_p2m() > - add an explanation why p2m_map_foreign_rw is valid > - move set_foreign_p2m_entry() to p2m-common.h > - add const to new parameter > > Changes V2 -> V3: > - update patch description > - rename arch_refcounts_p2m() to arch_acquire_resource_check() > - move comment to x86’s arch_acquire_resource_check() > - return rc in Arm's set_foreign_p2m_entry() > - put a respective ASSERT() into Arm's set_foreign_p2m_entry() > > Changes V3 -> V4: > - update arch_acquire_resource_check() implementation on x86 > and common code which uses it, pass struct domain to the function > - put ASSERT() to x86/Arm set_foreign_p2m_entry() > - use arch_acquire_resource_check() in p2m_add_foreign() > instead of open-coding it > --- > xen/arch/arm/p2m.c | 26 ++++++++++++++++++++++++++ > xen/arch/x86/mm/p2m.c | 9 ++++++--- > xen/common/memory.c | 9 ++------- > xen/include/asm-arm/p2m.h | 19 +++++++++---------- > xen/include/asm-x86/p2m.h | 19 ++++++++++++++++--- > xen/include/xen/p2m-common.h | 4 ++++ > 6 files changed, 63 insertions(+), 23 deletions(-) > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index 4eeb867..d41c4fa 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -1380,6 +1380,32 @@ int guest_physmap_remove_page(struct domain *d, gfn_t gfn, mfn_t mfn, > return p2m_remove_mapping(d, gfn, (1 << page_order), mfn); > } > > +int set_foreign_p2m_entry(struct domain *d, const struct domain *fd, > + unsigned long gfn, mfn_t mfn) > +{ > + struct page_info *page = mfn_to_page(mfn); > + int rc; > + > + ASSERT(arch_acquire_resource_check(d)); > + > + if ( !get_page(page, fd) ) > + return -EINVAL; > + > + /* > + * It is valid to always use p2m_map_foreign_rw here as if this gets > + * called then d != fd. A case when d == fd would be rejected by > + * rcu_lock_remote_domain_by_id() earlier. Put a respective ASSERT() > + * to catch incorrect usage in future. > + */ > + ASSERT(d != fd); > + > + rc = guest_physmap_add_entry(d, _gfn(gfn), mfn, 0, p2m_map_foreign_rw); > + if ( rc ) > + put_page(page); > + > + return rc; > +} > + > static struct page_info *p2m_allocate_root(void) > { > struct page_info *page; > diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c > index 71fda06..cbeea85 100644 > --- a/xen/arch/x86/mm/p2m.c > +++ b/xen/arch/x86/mm/p2m.c > @@ -1323,8 +1323,11 @@ static int set_typed_p2m_entry(struct domain *d, unsigned long gfn_l, > } > > /* Set foreign mfn in the given guest's p2m table. */ > -int set_foreign_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn) > +int set_foreign_p2m_entry(struct domain *d, const struct domain *fd, > + unsigned long gfn, mfn_t mfn) > { > + ASSERT(arch_acquire_resource_check(d)); > + > return set_typed_p2m_entry(d, gfn, mfn, PAGE_ORDER_4K, p2m_map_foreign, > p2m_get_hostp2m(d)->default_access); > } > @@ -2579,7 +2582,7 @@ static int p2m_add_foreign(struct domain *tdom, unsigned long fgfn, > * hvm fixme: until support is added to p2m teardown code to cleanup any > * foreign entries, limit this to hardware domain only. > */ > - if ( !is_hardware_domain(tdom) ) > + if ( !arch_acquire_resource_check(tdom) ) > return -EPERM; > > if ( foreigndom == DOMID_XEN ) > @@ -2635,7 +2638,7 @@ static int p2m_add_foreign(struct domain *tdom, unsigned long fgfn, > * will update the m2p table which will result in mfn -> gpfn of dom0 > * and not fgfn of domU. > */ > - rc = set_foreign_p2m_entry(tdom, gpfn, mfn); > + rc = set_foreign_p2m_entry(tdom, fdom, gpfn, mfn); > if ( rc ) > gdprintk(XENLOG_WARNING, "set_foreign_p2m_entry failed. " > "gpfn:%lx mfn:%lx fgfn:%lx td:%d fd:%d\n", > diff --git a/xen/common/memory.c b/xen/common/memory.c > index 66828d9..d625a9b 100644 > --- a/xen/common/memory.c > +++ b/xen/common/memory.c > @@ -1138,12 +1138,7 @@ static int acquire_resource( > xen_pfn_t mfn_list[32]; > int rc; > > - /* > - * FIXME: Until foreign pages inserted into the P2M are properly > - * reference counted, it is unsafe to allow mapping of > - * resource pages unless the caller is the hardware domain. > - */ > - if ( paging_mode_translate(currd) && !is_hardware_domain(currd) ) > + if ( !arch_acquire_resource_check(currd) ) > return -EACCES; > > if ( copy_from_guest(&xmar, arg, 1) ) > @@ -1211,7 +1206,7 @@ static int acquire_resource( > > for ( i = 0; !rc && i < xmar.nr_frames; i++ ) > { > - rc = set_foreign_p2m_entry(currd, gfn_list[i], > + rc = set_foreign_p2m_entry(currd, d, gfn_list[i], > _mfn(mfn_list[i])); > /* rc should be -EIO for any iteration other than the first */ > if ( rc && i ) > diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h > index 28ca9a8..4f8b3b0 100644 > --- a/xen/include/asm-arm/p2m.h > +++ b/xen/include/asm-arm/p2m.h > @@ -161,6 +161,15 @@ typedef enum { > #endif > #include > > +static inline bool arch_acquire_resource_check(struct domain *d) > +{ > + /* > + * The reference counting of foreign entries in set_foreign_p2m_entry() > + * is supported on Arm. > + */ > + return true; > +} > + > static inline > void p2m_altp2m_check(struct vcpu *v, uint16_t idx) > { > @@ -392,16 +401,6 @@ static inline gfn_t gfn_next_boundary(gfn_t gfn, unsigned int order) > return gfn_add(gfn, 1UL << order); > } > > -static inline int set_foreign_p2m_entry(struct domain *d, unsigned long gfn, > - mfn_t mfn) > -{ > - /* > - * NOTE: If this is implemented then proper reference counting of > - * foreign entries will need to be implemented. > - */ > - return -EOPNOTSUPP; > -} > - > /* > * A vCPU has cache enabled only when the MMU is enabled and data cache > * is enabled. > diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h > index 7df2878..1d64c12 100644 > --- a/xen/include/asm-x86/p2m.h > +++ b/xen/include/asm-x86/p2m.h > @@ -382,6 +382,22 @@ struct p2m_domain { > #endif > #include > > +static inline bool arch_acquire_resource_check(struct domain *d) > +{ > + /* > + * The reference counting of foreign entries in set_foreign_p2m_entry() > + * is not supported for translated domains on x86. > + * > + * FIXME: Until foreign pages inserted into the P2M are properly > + * reference counted, it is unsafe to allow mapping of > + * resource pages unless the caller is the hardware domain. > + */ > + if ( paging_mode_translate(d) && !is_hardware_domain(d) ) > + return false; > + > + return true; > +} > + > /* > * Updates vCPU's n2pm to match its np2m_base in VMCx12 and returns that np2m. > */ > @@ -647,9 +663,6 @@ int p2m_finish_type_change(struct domain *d, > int p2m_is_logdirty_range(struct p2m_domain *, unsigned long start, > unsigned long end); > > -/* Set foreign entry in the p2m table (for priv-mapping) */ > -int set_foreign_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn); > - > /* Set mmio addresses in the p2m table (for pass-through) */ > int set_mmio_p2m_entry(struct domain *d, gfn_t gfn, mfn_t mfn, > unsigned int order); > diff --git a/xen/include/xen/p2m-common.h b/xen/include/xen/p2m-common.h > index 58031a6..b4bc709 100644 > --- a/xen/include/xen/p2m-common.h > +++ b/xen/include/xen/p2m-common.h > @@ -3,6 +3,10 @@ > > #include > > +/* Set foreign entry in the p2m table */ > +int set_foreign_p2m_entry(struct domain *d, const struct domain *fd, > + unsigned long gfn, mfn_t mfn); > + > /* Remove a page from a domain's p2m table */ > int __must_check > guest_physmap_remove_page(struct domain *d, gfn_t gfn, mfn_t mfn, > -- > 2.7.4 > --8323329-766898220-1610673565=:31265--