Re: [Xen-devel] [PATCH v2 1/2] xen/x86: cleanup unused NMI/MCE code

[Juergen Gross <jgross@xxxxxxxx>]

On 23.07.19 20:48, Andrew Cooper wrote:
> On 23/07/2019 19:25, Juergen Gross wrote:
> > pv_raise_interrupt() is only called for NMIs these days, so the MCE
> > specific part can be removed. Rename pv_raise_interrupt() to
> > pv_raise_nmi() and NMI_MCE_SOFTIRQ to NMI_SOFTIRQ.
> For posterity, it would be helpful to explicitly identify 355b0469a8
> which introduced NMI and MCE pinning (where previously there was no NMI
> pinning beforehand AFAICT), and then 3a91769d6e which removed the MCE
> pinning.
Okay.

> Stated like that, I doubt the NMI pinning was ever relevant in practice.
Indeed.

> > Additionally there is no need to pin the vcpu the NMI is delivered
> > to, that is a leftover of (already removed) MCE handling. So remove
> > the pinning, too.
> >
> > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> Everything LGTM.  A few trivial notes.
>
> > diff --git a/xen/arch/x86/pv/traps.c b/xen/arch/x86/pv/traps.c
> > index 1740784ff2..9436c80047 100644
> > --- a/xen/arch/x86/pv/traps.c
> > +++ b/xen/arch/x86/pv/traps.c
> > @@ -136,47 +136,21 @@ bool set_guest_nmi_trapbounce(void)
> >       return !null_trap_bounce(curr, tb);
> >   }
> >   
> > -struct softirq_trap {
> > -    struct domain *domain;   /* domain to inject trap */
> > -    struct vcpu *vcpu;       /* vcpu to inject trap */
> > -    unsigned int processor;  /* physical cpu to inject trap */
> > -};
> > +static DEFINE_PER_CPU(struct vcpu *, softirq_nmi_vcpu);
> >   
> > -static DEFINE_PER_CPU(struct softirq_trap, softirq_trap);
> > -
> > -static void nmi_mce_softirq(void)
> > +static void nmi_softirq(void)
> >   {
> >       unsigned int cpu = smp_processor_id();
> > -    struct softirq_trap *st = &per_cpu(softirq_trap, cpu);
> > -
> > -    BUG_ON(st->vcpu == NULL);
> > -
> > -    /*
> > -     * Set the tmp value unconditionally, so that the check in the iret
> > -     * hypercall works.
> > -     */
> > -    cpumask_copy(st->vcpu->cpu_hard_affinity_tmp,
> > -                 st->vcpu->cpu_hard_affinity);
> > +    struct vcpu **v_ptr = &per_cpu(softirq_nmi_vcpu, cpu);
> There is only a single use of 'cpu' here, so you can drop that and use
> this_cpu(softirq_nmi_vcpu) instead.
Okay.

> > diff --git a/xen/include/asm-x86/pv/traps.h b/xen/include/asm-x86/pv/traps.h
> > index fcc75f5e9a..47d6cf5fc4 100644
> > --- a/xen/include/asm-x86/pv/traps.h
> > +++ b/xen/include/asm-x86/pv/traps.h
> > @@ -27,8 +27,8 @@
> >   
> >   void pv_trap_init(void);
> >   
> > -/* Deliver interrupt to PV guest. Return 0 on success. */
> > -int pv_raise_interrupt(struct vcpu *v, uint8_t vector);
> > +/* Deliver NMI to PV guest. Return 0 on success. */
> > +int pv_raise_nmi(struct vcpu *v);
> >   
> >   int pv_emulate_privileged_op(struct cpu_user_regs *regs);
> >   void pv_emulate_gate_op(struct cpu_user_regs *regs);
> > @@ -46,8 +46,8 @@ static inline bool pv_trap_callback_registered(const struct 
> > vcpu *v,
> >   
> >   static inline void pv_trap_init(void) {}
> >   
> > -/* Deliver interrupt to PV guest. Return 0 on success. */
> > -static inline int pv_raise_interrupt(struct vcpu *v, uint8_t vector) { return 
> > -EOPNOTSUPP; }
> > +/* Deliver NMI to PV guest. Return 0 on success. */
> > +static inline int pv_raise_nmi(struct vcpu *v) { return -EOPNOTSUPP; }
> I don't think duplicating the function description here is useful.
> Instead, I'd recommend dropping these lines, and commenting it once in
> pv/traps.c.  That should include the fact that it is expected to be used
> NMI context, which means its not safe to use printk() etc in there.
Will do.


Juergen

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