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.5 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 894EDC4361B for ; Thu, 10 Dec 2020 02:30:50 +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 467CE23C81 for ; Thu, 10 Dec 2020 02:30:49 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 467CE23C81 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.48933.86609 (Exim 4.92) (envelope-from ) id 1knBj9-00045R-Eh; Thu, 10 Dec 2020 02:30:43 +0000 X-Outflank-Mailman: Message body and most headers restored to incoming version Received: by outflank-mailman (output) from mailman id 48933.86609; Thu, 10 Dec 2020 02:30:43 +0000 Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1knBj9-00045I-BJ; Thu, 10 Dec 2020 02:30:43 +0000 Received: by outflank-mailman (input) for mailman id 48933; Thu, 10 Dec 2020 02:30:42 +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 1knBj8-00044u-Cc for xen-devel@lists.xenproject.org; Thu, 10 Dec 2020 02:30:42 +0000 Received: from mail.kernel.org (unknown [198.145.29.99]) by us1-amaz-eas2.inumbo.com (Halon) with ESMTPS id 845fae40-5021-45bc-80e3-d35fb1114adb; Thu, 10 Dec 2020 02:30:41 +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: 845fae40-5021-45bc-80e3-d35fb1114adb Date: Wed, 9 Dec 2020 18:30:39 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1607567441; bh=X+iXEu7MKVRAXqalOPEivlTXGleWKjjVcnCirIwOifM=; h=From:To:cc:Subject:In-Reply-To:References:From; b=ZtKr6F6ZtL6EeGOYbmwmaI8SqxQ7KgNWYPKbs0oueXAVaJALtsNSqSQNESTCp149u DH/CLEsYHEK5V20BopU90Qlm1aqpsEpYJqxaJ5HvD4d6DyOSBfIYAd87B2Ee+BsBpN kzw4eJfG/zmdppkAgV4SgDDUb1qY3Eo9WIh0jO+jZUPm2j9EJBEpTwlZSc1/6UeIGP i60WLVGF6VFb1ZGaimV9+u9PDOPsbJoo1zacYXcbyyLkEpzt2xsXukzRtyjWWmSfD3 QgMNW7UhRQ7wAyptePNeKe3hSWQgd/V/TkeOgpTSuzfKWhxpHyg/krRGx/yzZmw4Q0 JKeJaiTalfY0Q== 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 , Julien Grall Subject: Re: [PATCH V3 21/23] xen/arm: Add mapcache invalidation handling In-Reply-To: <1606732298-22107-22-git-send-email-olekstysh@gmail.com> Message-ID: References: <1606732298-22107-1-git-send-email-olekstysh@gmail.com> <1606732298-22107-22-git-send-email-olekstysh@gmail.com> User-Agent: Alpine 2.21 (DEB 202 2017-01-01) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII On Mon, 30 Nov 2020, Oleksandr Tyshchenko wrote: > From: Oleksandr Tyshchenko > > We need to send mapcache invalidation request to qemu/demu everytime > the page gets removed from a guest. > > At the moment, the Arm code doesn't explicitely remove the existing > mapping before inserting the new mapping. Instead, this is done > implicitely by __p2m_set_entry(). > > So we need to recognize a case when old entry is a RAM page *and* > the new MFN is different in order to set the corresponding flag. > The most suitable place to do this is p2m_free_entry(), there > we can find the correct leaf type. The invalidation request > will be sent in do_trap_hypercall() later on. Why is it sent in do_trap_hypercall() ? > Signed-off-by: Oleksandr Tyshchenko > CC: Julien Grall > > --- > Please note, this is a split/cleanup/hardening of Julien's PoC: > "Add support for Guest IO forwarding to a device emulator" > > Changes V1 -> V2: > - new patch, some changes were derived from (+ new explanation): > xen/ioreq: Make x86's invalidate qemu mapcache handling common > - put setting of the flag into __p2m_set_entry() > - clarify the conditions when the flag should be set > - use domain_has_ioreq_server() > - update do_trap_hypercall() by adding local variable > > Changes V2 -> V3: > - update patch description > - move check to p2m_free_entry() > - add a comment > - use "curr" instead of "v" in do_trap_hypercall() > --- > --- > xen/arch/arm/p2m.c | 24 ++++++++++++++++-------- > xen/arch/arm/traps.c | 13 ++++++++++--- > 2 files changed, 26 insertions(+), 11 deletions(-) > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index 5b8d494..9674f6f 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -1,6 +1,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -749,17 +750,24 @@ static void p2m_free_entry(struct p2m_domain *p2m, > if ( !p2m_is_valid(entry) ) > return; > > - /* Nothing to do but updating the stats if the entry is a super-page. */ > - if ( p2m_is_superpage(entry, level) ) > + if ( p2m_is_superpage(entry, level) || (level == 3) ) > { > - p2m->stats.mappings[level]--; > - return; > - } > +#ifdef CONFIG_IOREQ_SERVER > + /* > + * If this gets called (non-recursively) then either the entry > + * was replaced by an entry with a different base (valid case) or > + * the shattering of a superpage was failed (error case). > + * So, at worst, the spurious mapcache invalidation might be sent. > + */ > + if ( domain_has_ioreq_server(p2m->domain) && > + (p2m->domain == current->domain) && p2m_is_ram(entry.p2m.type) ) > + p2m->domain->mapcache_invalidate = true; Why the (p2m->domain == current->domain) check? Shouldn't we set mapcache_invalidate to true anyway? What happens if p2m->domain != current->domain? We wouldn't want the domain to lose the mapcache_invalidate notification. > +#endif > > - if ( level == 3 ) > - { > p2m->stats.mappings[level]--; > - p2m_put_l3_page(entry); > + /* Nothing to do if the entry is a super-page. */ > + if ( level == 3 ) > + p2m_put_l3_page(entry); > return; > } > > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c > index b6077d2..151c626 100644 > --- a/xen/arch/arm/traps.c > +++ b/xen/arch/arm/traps.c > @@ -1443,6 +1443,7 @@ static void do_trap_hypercall(struct cpu_user_regs *regs, register_t *nr, > const union hsr hsr) > { > arm_hypercall_fn_t call = NULL; > + struct vcpu *curr = current; Is this just to save 3 characters? > BUILD_BUG_ON(NR_hypercalls < ARRAY_SIZE(arm_hypercall_table) ); > > @@ -1459,7 +1460,7 @@ static void do_trap_hypercall(struct cpu_user_regs *regs, register_t *nr, > return; > } > > - current->hcall_preempted = false; > + curr->hcall_preempted = false; > > perfc_incra(hypercalls, *nr); > call = arm_hypercall_table[*nr].fn; > @@ -1472,7 +1473,7 @@ static void do_trap_hypercall(struct cpu_user_regs *regs, register_t *nr, > HYPERCALL_RESULT_REG(regs) = call(HYPERCALL_ARGS(regs)); > > #ifndef NDEBUG > - if ( !current->hcall_preempted ) > + if ( !curr->hcall_preempted ) > { > /* Deliberately corrupt parameter regs used by this hypercall. */ > switch ( arm_hypercall_table[*nr].nr_args ) { > @@ -1489,8 +1490,14 @@ static void do_trap_hypercall(struct cpu_user_regs *regs, register_t *nr, > #endif > > /* Ensure the hypercall trap instruction is re-executed. */ > - if ( current->hcall_preempted ) > + if ( curr->hcall_preempted ) > regs->pc -= 4; /* re-execute 'hvc #XEN_HYPERCALL_TAG' */ > + > +#ifdef CONFIG_IOREQ_SERVER > + if ( unlikely(curr->domain->mapcache_invalidate) && > + test_and_clear_bool(curr->domain->mapcache_invalidate) ) > + ioreq_signal_mapcache_invalidate(); Why not just: if ( unlikely(test_and_clear_bool(curr->domain->mapcache_invalidate)) ) ioreq_signal_mapcache_invalidate();