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

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


  • To: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Wed, 25 May 2022 08:02:17 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.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=Noj3KK3nSzUgBcc6nC9rQgxRTm0VWrtL+J8ZkvoFO9c=; b=efnXhtMwMC/C8cR90KC+meb9cynYGWGG7LiAC73wBWlu+JNWqdM2INhgzXIi8ouA5g/O/tlg9e9NWMdY1MrwhCls/wqFiii6svobEypwEx3Wpf3ptxr1pcqLpmZ3kSx7tP61xBq4T8H4eygZ7deUkoEuEfT8ZOOplExG2ZD0KsZv/CsTXoLDBagwmDLw5bc55fpw2oox6ai9QFIMnt/EF5fYAqZ0TwTqbNJGFrdYS3xBzjB4vy3F9jBBW/MLQDzFjyYCC0oSbECubjCJnsPuJcDD8NjOESEfrFEN+LjAVT2ih298vT8F7E5SmoKGRS3qHVTHzKwTiVgMzCaG2p95bA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=nwdeRwGzelQTLbPJwRdSPhXibcC9afsCUKUtbn16DsflQxSyvD/hgzxe8b788gWR7m/s68iDuC7vgm/TmIsZD1hZxK1qT1aJ9nsemZrCGH5S4CPAPcfhSzqqW0IJAkKiDF6IdHqIgqQklobOM2EyhRbQnRe/8TzzA6qukjtYRZ0c1alNEC5TA7EscV4esIFX+n09ARkE02v7lc0+zo24ULy0mmhAWgI5Wzdf6u94hClv3hPL+YprN6CN4sjd2XSZkXluJOn9xJ4OdNn8NzlHgv7NKrNTeArtj7qNJStQS2C09faDC7xZUciM8SJFWOSqOnAArYhUruBre2ylrq+oJA==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Wed, 25 May 2022 06:02:33 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 24.05.2022 18:46, Roger Pau Monné wrote:
> On Tue, May 24, 2022 at 05:27:35PM +0200, Jan Beulich wrote:
>> On 24.05.2022 12:50, 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 intorduce
>>> flush_area_all() as an alias for flush_area_mask(&cpu_online_map...),
>>
>> This is now stale - you don't introduce flush_area_all() here.
>> Sadly nothing is said to justify the addition of a cast there,
>> which - as said before - I think is a little risky (as many
>> casts are), and hence would imo better be avoided.
> 
> So prior to this change there are no direct callers to
> flush_area_all(), and hence all callers use flush_area() which has the
> cast.  Now that I remove flush_area() and modify callers to use
> flush_area_all() directly it seems natural to also move the cast
> there.  While I agree that having casts is not desirable, I wouldn't
> consider this change as adding them.  Merely moving them but the
> result is that the callers get the cast like they used to do.

I'd agree with all of this if the change was local to mm.c. As I'd
like to see the macro in flushtlb.h left unchanged, did you consider
retaining flush_area() as a wrapper in mm.c, reduced to merely
invoking flush_area_all() with the cast added? That would also
reduce the code churn of the patch.

>>> --- a/xen/arch/x86/smp.c
>>> +++ b/xen/arch/x86/smp.c
>>> @@ -262,7 +262,10 @@ void flush_area_mask(const cpumask_t *mask, const void 
>>> *va, unsigned int flags)
>>>  {
>>>      unsigned int cpu = smp_processor_id();
>>>  
>>> -    ASSERT(local_irq_is_enabled());
>>> +    /* Local flushes can be performed with interrupts disabled. */
>>> +    ASSERT(local_irq_is_enabled() || cpumask_subset(mask, 
>>> cpumask_of(cpu)));
>>> +    /* Exclude use of FLUSH_VCPU_STATE for the local CPU. */
>>> +    ASSERT(!cpumask_test_cpu(cpu, mask) || !(flags & FLUSH_VCPU_STATE));
>>
>> What about FLUSH_FORCE_IPI? This won't work either with IRQs off,
>> I'm afraid. Or wait - that flag's name doesn't really look to
>> force the use of an IPI, it's still constrained to remote
>> requests. I think this wants mentioning in one of the comments,
>> not the least to also have grep match there then (right now grep
>> output gives the impression as if the flag wasn't consumed
>> anywhere).
> 
> Would you be fine with adding:
> 
> Note that FLUSH_FORCE_IPI doesn't need to be handled explicitly, as
> it's main purpose is to prevent the usage of the hypervisor assisted
> flush if available, not to force the sending of an IPI even for cases
> where it won't be sent.

Hmm, yes, that's even more verbose than I would have expected it to
be. Just one point: I'm not sure about "main" there. Is there really
another purpose?

Of course an alternative would be to rename the flag to properly
express what it's for (e.g. FLUSH_NO_HV_ASSIST). This would then
eliminate the need for a comment, afaic at least.

Jan




 


Rackspace

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