Re: [PATCH v4 2/3] xen/oprofile: use NMI continuation for sending virq to guest

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.11.2020 16:48, Jürgen Groß wrote:
> On 11.11.20 16:45, Jan Beulich wrote:
>> On 09.11.2020 10:50, Juergen Gross wrote:
>>> @@ -83,14 +85,28 @@ void passive_domain_destroy(struct vcpu *v)
>>>             model->free_msr(v);
>>>   }
>>>   
>>> +bool nmi_oprofile_send_virq(void)
>>> +{
>>> +   struct vcpu *v = this_cpu(nmi_cont_vcpu);
>>> +
>>> +   if ( v )
>>> +           send_guest_vcpu_virq(v, VIRQ_XENOPROF);
>>> +
>>> +   this_cpu(nmi_cont_vcpu) = NULL;
>>
>> What if, by the time we make it here, a 2nd NMI has arrived? I
>> agree the next overflow interrupt shouldn't arrive this
>> quickly, but I also think you want to zap the per-CPU variable
>> first here, and ...
> 
> How could that happen? This function is activated only from NMI
> context in case the NMI happened in guest mode. And it will be
> executed with higher priority than any guest, so there is a zero
> chance another NMI in guest mode can happen in between.

While I'll admit I didn't pay attention to the bogus (as far as
HVM is concerned) xen_mode check, my understanding is that the
self-IPI will be delivered once we're back in guest mode, as
that's the first time IRQs would be on again (even event checking
gets deferred by sending a self-IPI). If another NMI was latched
by that time, it would take precedence over the IRQ and would
also be delivered on the guest mode insn that the IRET returned
to.

I agree though that this is benign, as the vCPU wouldn't have
been context switched out yet, i.e. current is still the same
and there'll then merely be two NMI instances folded into one.

However, I still think the ordering would better be changed, to
set a good precedent.

>>>   static int nmi_callback(const struct cpu_user_regs *regs, int cpu)
>>>   {
>>>     int xen_mode, ovf;
>>>   
>>>     ovf = model->check_ctrs(cpu, &cpu_msrs[cpu], regs);
>>>     xen_mode = ring_0(regs);

Unrelated to the patch here (i.e. just as an observation), this
use of ring_0() looks bogus when the NMI occurred in HVM guest
mode.

Jan