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 8:41 PM, David Woodhouse wrote:
> On Mon, 2021-01-04 at 18:44 -0500, boris.ostrovsky@xxxxxxxxxx wrote:
>> On 1/4/21 6:19 PM, David Woodhouse wrote:
>>> On Mon, 2021-01-04 at 18:04 -0500, boris.ostrovsky@xxxxxxxxxx
>>> wrote:
>>>> 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.
> But as you said, the other three things that xen_hvm_cpu_die() does are
> all no-ops (or at least we'd have to make them no-ops; I haven't
> checked). I don't see the point in calling it, only for it to do
> nothing.


For maintenance purposes. When something gets added in initialization path 
(prepare_boot_cpu() and such) we likely want to tear it down in cpu_die(). 
Today both native and Xen cpu_die() ops work the same but things change. And 
chances are people will not test no_vector_callback case so we will then have 
to hunt down what broke when.


no_vector_callback is a pretty esoteric case already so making it take 
different code path from common case, when there is little or no penalty for 
doing the latter, seems undesirable to me.


>
>>>>>   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?
> They do different things. If someone has an image (which might run on
> both Xen and KVM) which has xen_nopvspin, we don't suddenly want that
> to start disabling PV spinlocks on KVM too.


True, but this option is deprecated and will be removed in the future.


-boris