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

[Jürgen Groß <jgross@xxxxxxxx>]

On 12.11.20 11:23, Jan Beulich wrote:
> 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.
Correct.

> However, I still think the ordering would better be changed, to
> set a good precedent.
Okay, if you want that.

> > > >    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.
An NMI in an HVM guest due to oprofile would be a VMEXIT with NMI
reason, or just be handled completely inside the guest, right?

I don't see how this test should ever result in xen_mode being
false for an HVM guest.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: application/pgp-keys

Attachment: OpenPGP_signature
Description: OpenPGP digital signature