Re: Regression with CET: [PATCH v1] x86/mm: avoid inadvertently degrading a TLB flush to local only

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.04.2022 09:37, Jan Beulich wrote:
> On 27.04.2022 20:44, David Vrabel wrote:
>>
>>
>> On 27/04/2022 19:03, Andrew Cooper wrote:
>>> On 19/04/2022 16:03, David Vrabel wrote:
>>>> From: David Vrabel <dvrabel@xxxxxxxxxxxx>
>>>>
>>>> If the direct map is incorrectly modified with interrupts disabled,
>>>> the required TLB flushes are degraded to flushing the local CPU only.
>>>>
>>>> This could lead to very hard to diagnose problems as different CPUs will
>>>> end up with different views of memory. Although, no such issues have yet
>>>> been identified.
>>>>
>>>> Change the check in the flush_area() macro to look at system_state
>>>> instead. This defers the switch from local to all later in the boot
>>>> (see xen/arch/x86/setup.c:__start_xen()). This is fine because
>>>> additional PCPUs are not brought up until after the system state is
>>>> SYS_STATE_smp_boot.
>>>>
>>>> Signed-off-by: David Vrabel <dvrabel@xxxxxxxxxxxx>
>>>
>>> This explodes on CET systems:
>>>
>>> (XEN) Assertion 'local_irq_is_enabled()' failed at arch/x86/smp.c:265
>>> (XEN) ----[ Xen-4.17.0-10.24-d  x86_64  debug=y  Not tainted ]----
>>> (XEN) CPU:    0
>>> (XEN) RIP:    e008:[<ffff82d040345300>] flush_area_mask+0x40/0x13e
>>> <snip>
>>> (XEN) Xen call trace:
>>> (XEN)    [<ffff82d040345300>] R flush_area_mask+0x40/0x13e
>>> (XEN)    [<ffff82d040338a40>] F modify_xen_mappings+0xc5/0x958
>>> (XEN)    [<ffff82d0404474f9>] F
>>> arch/x86/alternative.c#_alternative_instructions+0xb7/0xb9
>>> (XEN)    [<ffff82d0404476cc>] F alternative_branches+0xf/0x12
>>> (XEN)    [<ffff82d04044e37d>] F __start_xen+0x1ef4/0x2776
>>> (XEN)    [<ffff82d040203344>] F __high_start+0x94/0xa0
>>> (XEN)
>>> (XEN)
>>> (XEN) ****************************************
>>> (XEN) Panic on CPU 0:
>>> (XEN) Assertion 'local_irq_is_enabled()' failed at arch/x86/smp.c:265
>>> (XEN) ****************************************
>>> (XEN)
>>>
>>> We really did want a local-only flush here, because we specifically
>>> intended to make self-modifying changes before bringing secondary CPUs up.
>>
>> I think the transition to SYS_STATE_smp_boot system state should be 
>> later. i.e., the last point were only 1 PCPU is guaranteed.
> 
> I'm not sure there isn't code which assumes pre-SMP initcalls to happen
> in this state already. So it may take addition of yet another state if
> no other solution can be found.

Or maybe this again shouldn't be using system_state but num_online_cpus()?

Jan