[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v3 2/2] x86/flushtlb: remove flush_area check on system state


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Wed, 25 May 2022 11:32:24 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=kch/SKVWSwIN4uB0vHx2zrSytidkUMUy1vMrs1Nj9y0=; b=CW7bthVtRDA4tXSIOcnRNAxdLSLpCOAK4eVyWf/qk6/1JldCMIAWMfb0rtKman8E2jInwd3geXtYAPKQCxjiDTGWMr0rqe3O7m0owY1G/yzIkWyfYhg4Jr10tT2u0zBHnAqvl42IQsyiWFj0OODt5jSLX4Zsnukv75Co9CHgY4gCR1kkYi+1PDM9Wiq54ITw0Efw1RdMBVe9+w4SPqOUNXCyhZF1QMY4LNKe5OQHt3TJk91N/CkozcIuhRAWdv4feW53/HTmxDKHfz4/YPIuUXU4lj/YwSGf9BcuW1ecp3zqnJbkea+F1XeIC/oPK+Sm7BJzBAjzTBnwdDiRk8HF6A==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=YmG3NzyBnOuxEx2uAFk7ulP3ozQA7PEYFe1lvkMy/G2gPJr89h7qaE5rbsQ27bUSTLjQhyP5vSYMXtH2QJnIhJNfC2d9Qf37NAMlSrdWzRz5koNi/xTbUqG5W6sutmfFJOcwsOyg4JVjB9ehMPypF8l6TeAkxQLoWVsWLy5r/i0vZDr5jT1h9Eq4YFiX+8JJjsDtq1uaQVO60f6A744SrCoHO7qIMKvpbvEXrYHMUynp7SH0q6aK5DWEBe96OkiWEx2msoQR1sD/trju9+KLYWWWvE/w8Yf6o0Y2HgIwNYeTnpwORRtPSgFiokkup/YTkEt0fHf8UW30EenkwtnYQA==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=citrix.com;
  • Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Wed, 25 May 2022 09:32:47 +0000
  • Ironport-data: A9a23:tkyuia7ZQZkCwv2cIVy00wxRtEzGchMFZxGqfqrLsTDasY5as4F+v mMfWm2Ga/ncYWSgKtt+b961/E9QsMfczIIxSVRpqC9mHi5G8cbLO4+Ufxz6V8+wwmwvb67FA +E2MISowBUcFyeEzvuVGuG96yE6j8lkf5KkYAL+EnkZqTRMFWFw0HqPp8Zj2tQy2YXjXlvU0 T/Pi5a31GGNimYc3l08s8pvmDs31BglkGpF1rCWTakjUG72zxH5PrpGTU2CByKQrr1vNvy7X 47+IISRpQs1yfuP5uSNyd4XemVSKlLb0JPnZnB+A8BOiTAazsA+PzpS2FPxpi67hh3Q9+2dx umhurSiSEQMM4yVwt1DTh5GAh1PDY9dxaH+dC3XXcy7lyUqclPK6tA2VgQNG9Rd/ex6R2ZT6 fYfNTYBKAiZgP67y666Te8qgdk/KM7sP8UUvXQIITPxVK56B8ycBfiVo4MItNszrpkm8fL2f c0WZCApdB3dSxZOJk0WGNQ1m+LAanzXLGcF8gPK//FfD277/gta4anKPYTpPfuxXsAPwE23p VPW4DGsav0dHJnFodafyVqujOLSmSLwWKoJCaa1sPVthTW72Wg7GBAQE1yhrpGRmkO4Ht5SN UEQ0i4vtrQpslymSMHnWB+1q2LCuQQTM+e8CMU/4QCJj7HSug+fD21cFDpZMoR65IkxWCAg0 UKPk5XxHztzvbaJSHWbsLCJsTe1PitTJmgHDcMZcTY4DxDYiNlbpnryohxLSsZZUvWd9enM/ g23
  • Ironport-hdrordr: A9a23:2ItyOqkQcfJDgcION0JY6C/8Et/pDfO+imdD5ihNYBxZY6Wkfp +V8cjzhCWftN9OYhodcLC7V5Voj0mskKKdxbNhRYtKOzOWw1dATbsSlLcKpgeNJ8SQzI5gPM tbAstD4ZjLfCJHZKXBkXaF+rQbsb66GcmT7I+xrkuFDzsaDZ2Ihz0JdjpzeXcGIDWua6BJdq Z1saF81kedkDksH42GL0hAe9KGi8zAlZrgbxJDLxk76DOWhTftzLLhCRCX0joXTjsKmN4ZgC P4uj28wp/mn+Cwyxfa2WOWx5NKmOH5wt8GIMCXkMAaJhjllw7tToV8XL+puiwzvYiUmR4Xue iJhy1lE9V46nvXcG3wiRzx2zP42DJr0HPmwU/wuwqWneXJABYBT+ZRj4NQdRXUr2A6ustn7a 5N12WF87JKEBLphk3Glpf1fiAvsnDxjWspkOYVgXAae5AZcqVtoYsW+14QOIscHRj99JssHI BVfY3hDc5tABKnhk3izylSKITGZAVxIv7GeDlOhiWt6UkZoJgjpHFohvD2nR87hecAotd/lq H5259T5cBzp/8tHNxA7dg6MLuK40z2MGXx2TGpUCLa/J9uAQO/l7fHpJMI2cqNRLskiLMPpb WpaiIriYd1QTOlNfGz
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Wed, May 25, 2022 at 10:41:51AM +0200, Jan Beulich wrote:
> On 25.05.2022 10:13, Roger Pau Monne wrote:
> > Booting with Shadow Stacks leads to the following assert on a debug
> > hypervisor:
> > 
> > Assertion 'local_irq_is_enabled()' failed at arch/x86/smp.c:265
> > ----[ Xen-4.17.0-10.24-d  x86_64  debug=y  Not tainted ]----
> > CPU:    0
> > RIP:    e008:[<ffff82d040345300>] flush_area_mask+0x40/0x13e
> > [...]
> > Xen call trace:
> >    [<ffff82d040345300>] R flush_area_mask+0x40/0x13e
> >    [<ffff82d040338a40>] F modify_xen_mappings+0xc5/0x958
> >    [<ffff82d0404474f9>] F 
> > arch/x86/alternative.c#_alternative_instructions+0xb7/0xb9
> >    [<ffff82d0404476cc>] F alternative_branches+0xf/0x12
> >    [<ffff82d04044e37d>] F __start_xen+0x1ef4/0x2776
> >    [<ffff82d040203344>] F __high_start+0x94/0xa0
> > 
> > This is due to SYS_STATE_smp_boot being set before calling
> > alternative_branches(), and the flush in modify_xen_mappings() then
> > using flush_area_all() with interrupts disabled.  Note that
> > alternative_branches() is called before APs are started, so the flush
> > must be a local one (and indeed the cpumask passed to
> > flush_area_mask() just contains one CPU).
> > 
> > Take the opportunity to simplify a bit the logic and make flush_area()
> > an alias of flush_area_all() in mm.c, taking into account that
> > cpu_online_map just contains the BSP before APs are started.  This
> > requires widening the assert in flush_area_mask() to allow being
> > called with interrupts disabled as long as it's strictly a local only
> > flush.
> > 
> > The overall result is that a conditional can be removed from
> > flush_area().
> > 
> > While there also introduce an ASSERT to check that a vCPU state flush
> > is not issued for the local CPU only.
> > 
> > Fixes: (78e072bc37 'x86/mm: avoid inadvertently degrading a TLB flush to 
> > local only')
> > Suggested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> with ...
> 
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -5070,13 +5070,8 @@ l1_pgentry_t *virt_to_xen_l1e(unsigned long v)
> >  #define l1f_to_lNf(f) (((f) & _PAGE_PRESENT) ? ((f) |  _PAGE_PSE) : (f))
> >  #define lNf_to_l1f(f) (((f) & _PAGE_PRESENT) ? ((f) & ~_PAGE_PSE) : (f))
> >  
> > -/*
> > - * map_pages_to_xen() can be called early in boot before any other
> > - * CPUs are online. Use flush_area_local() in this case.
> > - */
> > -#define flush_area(v,f) (system_state < SYS_STATE_smp_boot ?    \
> > -                         flush_area_local((const void *)v, f) : \
> > -                         flush_area_all((const void *)v, f))
> > +/* flush_area_all() can be used prior to any other CPU being online.  */
> > +#define flush_area(v, f) flush_area_all((const void *)v, f)
> 
> ... v properly parenthesized here as the code is being touched anyway:
> One less Misra-C violation. This surely can be done while committing.

Indeed.  I had my addition properly parenthesized, but forgot to do it
here when moving the line.

Thanks, Roger.



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.