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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

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

>
> 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.

> 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.

~Andrew

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