Re: [PATCH 5/5] x86/xen: Don't register PV spinlock IPI when it isn't going to be used

[David Woodhouse <dwmw2@xxxxxxxxxxxxx>]

On Mon, 2021-01-04 at 18:04 -0500, boris.ostrovsky@xxxxxxxxxx wrote:
> On 1/4/21 5:37 PM, David Woodhouse wrote:
> > 
> > @@ -33,9 +33,11 @@ static void __init
> > xen_hvm_smp_prepare_cpus(unsigned int max_cpus)
> >     int cpu;
> >  
> >     native_smp_prepare_cpus(max_cpus);
> > -   WARN_ON(xen_smp_intr_init(0));
> >  
> > -   xen_init_lock_cpu(0);
> > +   if (xen_have_vector_callback) {
> > +           WARN_ON(xen_smp_intr_init(0));
> > +           xen_init_lock_cpu(0);
> 
> 
> By now you have nopvspin set so you might as well leave
> xen_init_lock_cpu(0) as is. (and then move the check inside
> xen_smp_intr_init())

I originally started doing it that way but PV guests use
xen_smp_intr_init() too, and want it to work even if nopvspin is set.
And don't set xen_have_vector_callback.

So the condition would need to be xen_pv_domain() ||
xen_have_vector_callback... or something like that. Or I could keep it
simple by keeping the new condition purely in the HVM code path, as I
did.

> 
> > +   }
> >  
> >     for_each_possible_cpu(cpu) {
> >             if (cpu == 0)
> > @@ -64,14 +66,17 @@ static void xen_hvm_cpu_die(unsigned int cpu)
> >  
> >  void __init xen_hvm_smp_init(void)
> >  {
> > -   if (!xen_have_vector_callback)
> > +   smp_ops.smp_prepare_boot_cpu = xen_hvm_smp_prepare_boot_cpu;
> > +   smp_ops.smp_prepare_cpus = xen_hvm_smp_prepare_cpus;
> > +   smp_ops.smp_cpus_done = xen_smp_cpus_done;
> > +
> > +   if (!xen_have_vector_callback) {
> > +           nopvspin = true;
> >             return;
> > +   }
> >  
> > -   smp_ops.smp_prepare_cpus = xen_hvm_smp_prepare_cpus;
> >     smp_ops.smp_send_reschedule = xen_smp_send_reschedule;
> >     smp_ops.cpu_die = xen_hvm_cpu_die;
> 
> 
> Why not xen_hvm_cpu_die too? common_cpu_die() sounds like something
> we should do, and the other three we call there will be nops.

native_cpu_die() calls that, and isn't that the function that gets
installed if we don't install our own?

> 
> >     smp_ops.send_call_func_ipi = xen_smp_send_call_function_ipi;
> >     smp_ops.send_call_func_single_ipi = 
> > xen_smp_send_call_function_single_ipi;
> > -   smp_ops.smp_prepare_boot_cpu = xen_hvm_smp_prepare_boot_cpu;
> > -   smp_ops.smp_cpus_done = xen_smp_cpus_done;
> >  }
> > 
> > > Also, for the spinlock changes specifically --- I wonder whether it
> > > would be better to reverse initial value of xen_pvspin and set it to
> > > 'true' only if initialization succeeds.
> > 
> > I looked at that but it would need to be tristate, since the
> > 'xen_nopvspin' command line option clears it from its default of being
> > enabled.
> 
> 
> Ah, right. How about setting nopvspin in xen_parse_nopvspin()?

That would make the xen_nopvspin command line option disable PV
spinlocks even under KVM.

Attachment: smime.p7s
Description: S/MIME cryptographic signature