Re: [Xen-devel] [PATCH v2 5/6] x86/smp: use a dedicated scratch cpumask in send_IPI_mask

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Feb 18, 2020 at 11:35:37AM +0000, Andrew Cooper wrote:
> 
> 
> On 18/02/2020 11:22, Roger Pau Monné wrote:
> > On Tue, Feb 18, 2020 at 11:21:12AM +0000, Andrew Cooper wrote:
> >> On 18/02/2020 11:10, Roger Pau Monné wrote:
> >>> On Tue, Feb 18, 2020 at 10:53:45AM +0000, Andrew Cooper wrote:
> >>>> On 17/02/2020 18:43, Roger Pau Monne wrote:
> >>>>> @@ -67,7 +68,20 @@ static void send_IPI_shortcut(unsigned int shortcut, 
> >>>>> int vector,
> >>>>>  void send_IPI_mask(const cpumask_t *mask, int vector)
> >>>>>  {
> >>>>>      bool cpus_locked = false;
> >>>>> -    cpumask_t *scratch = this_cpu(scratch_cpumask);
> >>>>> +    cpumask_t *scratch = this_cpu(send_ipi_cpumask);
> >>>>> +    unsigned long flags;
> >>>>> +
> >>>>> +    if ( in_mc() || in_nmi() )
> >>>>> +    {
> >>>>> +        /*
> >>>>> +         * When in #MC or #MNI context Xen cannot use the per-CPU 
> >>>>> scratch mask
> >>>>> +         * because we have no way to avoid reentry, so do not use the 
> >>>>> APIC
> >>>>> +         * shorthand.
> >>>>> +         */
> >>>>> +        alternative_vcall(genapic.send_IPI_mask, mask, vector);
> >>>>> +        return;
> >>>> The set of things you can safely do in an NMI/MCE handler is small, and
> >>>> does not include sending IPIs.  (In reality, if you're using x2apic, it
> >>>> is safe to send an IPI because there is no risk of clobbering ICR2
> >>>> behind your outer context's back).
> >>>>
> >>>> However, if we escalate from NMI/MCE context into crash context, then
> >>>> anything goes.  In reality, we only ever send NMIs from the crash path,
> >>>> and that is not permitted to use a shorthand, making this code dead.
> >>> This was requested by Jan, as safety measure
> >> That may be, but it doesn't mean it is correct.  If execution ever
> >> enters this function in NMI/MCE context, there is a real,
> >> state-corrupting bug, higher up the call stack.
> > Ack, then I guess we should just BUG() here if ever called from #NMI
> > or #MC context?
> 
> Well.  There is a reason I suggested removing it, and not using BUG().
> 
> If NMI/MCE context escalates to crash context, we do need to send NMIs. 
> It won't be this function specifically, but it will be part of the
> general IPI infrastructure.
> 
> We definitely don't want to get into the game of trying to clobber each
> of the state variables, so the only thing throwing BUG()'s around in
> this area will do is make the crash path more fragile.

I see, panicking in such context will just clobber the previous crash
happened in NMI/MC context.

So you would rather keep the current version of falling back to the
usage of the non-shorthand IPI sending routine instead of panicking?

What about:

if ( in_mc() || in_nmi() )
{
    /*
     * When in #MC or #MNI context Xen cannot use the per-CPU scratch mask
     * because we have no way to avoid reentry, so do not use the APIC
     * shorthand. The only IPI that should be sent from such context
     * is a #NMI to shutdown the system in case of a crash.
     */
    if ( vector == APIC_DM_NMI )
        alternative_vcall(genapic.send_IPI_mask, mask, vector);
    else
        BUG();

    return;
}

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel