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

[boris.ostrovsky@xxxxxxxxxx]

On 1/4/21 6:19 PM, David Woodhouse wrote:
> 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.


OK.


>
>>> +   }
>>>  
>>>     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?


True.


Still, a Xen guest should call Xen-specific cpu_die() routine if possible. 
Especially since (now) other cpu (i.e. non-IPI) ops will call Xen versions.


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


xen_nopvspin is deprecated and nopvspin is recommended anyway so I think it 
should be OK, no?



-boris