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 3:51 PM, David Woodhouse wrote:
> On Mon, 2021-01-04 at 14:06 -0500, boris.ostrovsky@xxxxxxxxxx wrote:
>>
>> OK, but we still need to do something about virt_spin_lock_key.
> Right, I suppose we should just call xen_init_spinlocks() and then my
> defensive check stops being defensive and does what we need, including
> fixing virt_spin_lock_key.
>
> Normally it's xen_hvm_smp_prepare_boot_cpu() which calls
> xen_init_spinlocks(), and *also* calls xen_vcpu_setup() for CPU0...
> which brings me back to your other observation.
>
> I think we *should* be calling xen_vcpu_setup() for all CPUs, even when
> there's no vector callback. We can still have a per-vCPU vcpu_info page
> if we want it. It was relatively harmless that we didn't do it for
> CPU0, but it was wrong not to do so.
>
> So I think this fixes both. Will test:


I actually think this should go further in that only IPI-related ops should be 
conditioned on vector callback presence. The rest are generic VCPU routines 
that are not necessarily interrupt/event-related. And if they call something 
that *is* related then those specific routines should decide what to do based 
on xen_have_vector_callback.


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.


-boris


>
> --- a/arch/x86/xen/smp_hvm.c
> +++ b/arch/x86/xen/smp_hvm.c
> @@ -64,6 +64,8 @@ static void xen_hvm_cpu_die(unsigned int cpu)
>  
>  void __init xen_hvm_smp_init(void)
>  {
> +       smp_ops.smp_prepare_boot_cpu = xen_hvm_smp_prepare_boot_cpu;
> +
>         if (!xen_have_vector_callback)
>                 return;
>  
> @@ -72,6 +74,5 @@ void __init xen_hvm_smp_init(void)
>         smp_ops.cpu_die = xen_hvm_cpu_die;
>         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;
>  }