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=-4.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no 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 5F31EC433DF for ; Tue, 30 Jun 2020 13:04:20 +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 38A532068F for ; Tue, 30 Jun 2020 13:04:20 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 38A532068F Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=citrix.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=xen-devel-bounces@lists.xenproject.org Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1jqFve-00038N-B7; Tue, 30 Jun 2020 13:04:02 +0000 Received: from us1-rack-iad1.inumbo.com ([172.99.69.81]) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1jqFvc-00038I-Kn for xen-devel@lists.xenproject.org; Tue, 30 Jun 2020 13:04:00 +0000 X-Inumbo-ID: 29365518-bad2-11ea-bca7-bc764e2007e4 Received: from esa2.hc3370-68.iphmx.com (unknown [216.71.145.153]) by us1-rack-iad1.inumbo.com (Halon) with ESMTPS id 29365518-bad2-11ea-bca7-bc764e2007e4; Tue, 30 Jun 2020 13:03:59 +0000 (UTC) Authentication-Results: esa2.hc3370-68.iphmx.com; dkim=none (message not signed) header.i=none IronPort-SDR: SmFXgo6Z9wuA3UXh/7Qd6NGLn4cCut5oS8NbKqkID7dSCrtmTDxZoUTXIVr/PXztvaJdWICsl+ nMM0Syc2UBjv0HS4MEZPg2RlYA8Irmx8Zah/WpGOs4dn68baAxDhsrrF/BzMz2D2vnXOZYN7LF UAZUnfYi5FZCsUxg37ekwvkRSQoutHvIeT4FUvtIPTBasLqpE4wySI4cRDez4RlqhCRuIWRomX Q6oZod9wrMPCbpfm0gGwIY8Pix6F+H6pLcTVKI/70uGouCr85HoO10tuWA3hPK4s5C4sGqQuG3 +sY= X-SBRS: 2.7 X-MesageID: 21277757 X-Ironport-Server: esa2.hc3370-68.iphmx.com X-Remote-IP: 162.221.158.21 X-Policy: $RELAYED X-IronPort-AV: E=Sophos;i="5.75,297,1589256000"; d="scan'208";a="21277757" Date: Tue, 30 Jun 2020 15:03:51 +0200 From: Roger Pau =?utf-8?B?TW9ubsOp?= To: Jan Beulich Subject: Re: [PATCH for-4.14 v4] x86/tlb: fix assisted flush usage Message-ID: <20200630130351.GL735@Air-de-Roger> References: <20200626155723.91558-1-roger.pau@citrix.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-ClientProxiedBy: AMSPEX02CAS02.citrite.net (10.69.22.113) To AMSPEX02CL02.citrite.net (10.69.22.126) X-BeenThere: xen-devel@lists.xenproject.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Cc: Stefano Stabellini , Julien Grall , Wei Liu , paul@xen.org, Andrew Cooper , Ian Jackson , George Dunlap , xen-devel@lists.xenproject.org, Volodymyr Babchuk Errors-To: xen-devel-bounces@lists.xenproject.org Sender: "Xen-devel" On Tue, Jun 30, 2020 at 02:13:36PM +0200, Jan Beulich wrote: > On 26.06.2020 17:57, Roger Pau Monne wrote: > > Commit e9aca9470ed86 introduced a regression when avoiding sending > > IPIs for certain flush operations. Xen page fault handler > > (spurious_page_fault) relies on blocking interrupts in order to > > prevent handling TLB flush IPIs and thus preventing other CPUs from > > removing page tables pages. Switching to assisted flushing avoided such > > IPIs, and thus can result in pages belonging to the page tables being > > removed (and possibly re-used) while __page_fault_type is being > > executed. > > > > Force some of the TLB flushes to use IPIs, thus avoiding the assisted > > TLB flush. Those selected flushes are the page type change (when > > switching from a page table type to a different one, ie: a page that > > has been removed as a page table) and page allocation. This sadly has > > a negative performance impact on the pvshim, as less assisted flushes > > can be used. Note the flush in grant-table code is also switched to > > use an IPI even when not strictly needed. This is done so that a > > common arch_flush_tlb_mask can be introduced and always used in common > > code. > > > > Introduce a new flag (FLUSH_FORCE_IPI) and helper to force a TLB flush > > using an IPI (flush_tlb_mask_sync, x86 only). Note that the flag is > > only meaningfully defined when the hypervisor supports PV or shadow > > paging mode, as otherwise hardware assisted paging domains are in > > charge of their page tables and won't share page tables with Xen, thus > > not influencing the result of page walks performed by the spurious > > fault handler. > > > > Just passing this new flag when calling flush_area_mask prevents the > > usage of the assisted flush without any other side effects. > > > > Note the flag is not defined on Arm. > > > > Fixes: e9aca9470ed86 ('x86/tlb: use Xen L0 assisted TLB flush when available') > > Reported-by: Andrew Cooper > > Signed-off-by: Roger Pau Monné > > In principle > Reviewed-by: Jan Beulich > A few cosmetic remarks though: > > > --- a/xen/arch/x86/mm.c > > +++ b/xen/arch/x86/mm.c > > @@ -2894,7 +2894,17 @@ static int _get_page_type(struct page_info *page, unsigned long type, > > ((nx & PGT_type_mask) == PGT_writable_page)) ) > > { > > perfc_incr(need_flush_tlb_flush); > > - flush_tlb_mask(mask); > > + if ( (x & PGT_type_mask) && > > + (x & PGT_type_mask) <= PGT_root_page_table ) > > + /* > > + * If page was a page table make sure the flush is > > + * performed using an IPI in order to avoid changing > > + * the type of a page table page under the feet of > > + * spurious_page_fault. > > + */ > > + flush_tlb_mask_sync(mask); > > + else > > + flush_tlb_mask(mask); > > Effectively this now is the only user of the new macro. I'd prefer > avoiding its introduction (and hence avoiding the questionable > "_sync" suffix), doing > > flush_mask(mask, FLUSH_TLB | (... ? FLUSH_FORCE_IPI : 0)); Right, maybe placing the '(x & PGT_type_mask) && (x & PGT_type_mask) <= PGT_root_page_table' condition inside the parameter list of flush_mask will make the code hard to read, so it might be worth to keep the if? > here and ... > > > @@ -148,9 +158,24 @@ void flush_area_mask(const cpumask_t *, const void *va, unsigned int flags); > > /* Flush specified CPUs' TLBs */ > > #define flush_tlb_mask(mask) \ > > flush_mask(mask, FLUSH_TLB) > > +/* > > + * Flush specified CPUs' TLBs and force the usage of an IPI to do so. This is > > + * required for certain operations that rely on page tables themselves not > > + * being freed and reused when interrupts are blocked, as the flush IPI won't > > + * be fulfilled until exiting from that critical region. > > + */ > > +#define flush_tlb_mask_sync(mask) \ > > + flush_mask(mask, FLUSH_TLB | FLUSH_FORCE_IPI) > > #define flush_tlb_one_mask(mask,v) \ > > flush_area_mask(mask, (const void *)(v), FLUSH_TLB|FLUSH_ORDER(0)) > > > > +/* > > + * Alias the common code TLB flush helper to the sync one in order to be on the > > + * safe side. Note that not all calls from common code strictly require the > > + * _sync variant. > > + */ > > +#define arch_flush_tlb_mask flush_tlb_mask_sync > > ... > > #define arch_flush_tlb_mask(mask) \ > flush_mask(mask, FLUSH_TLB | FLUSH_FORCE_IPI) Sure. Feel free to slightly adjust the comment, I think doing s/Alias/Force/ would be enough. > here. I'd be okay making these adjustments while committing, if > you and others don't object. That's fine, I leave to your judgment whether to use the ternary operator in the _get_page_type case. Roger.