Re: [PATCH] xen/smp: Support NULL IPI function pointers

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Andrew,

On 2021/11/18 0:48, Andrew Cooper wrote:
> There are several cases where the act of interrupting a remote processor has
> the required side effect.  Explicitly allow NULL function pointers so the
> calling code doesn't have to provide a stub implementation.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
>
> The wait parameter is a little weird.  It serves double duty and will confirm
> that the IPI has been taken.  All it does is let you control whether you also
> wait for the handler to complete first.  As such, it is effectively useless
> with a stub function.
>
> I don't particularly like folding into the .wait() path like that, but I
> dislike it less than an if()/else if() and adding a 3rd cpumask_clear_cpu()
> into the confusion which is this logic.
> ---
>   xen/arch/x86/mm/hap/hap.c | 11 +----------
>   xen/arch/x86/mm/p2m-ept.c | 11 ++---------
>   xen/common/smp.c          |  4 ++++
>   3 files changed, 7 insertions(+), 19 deletions(-)
>
> diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
> index 73575deb0d8a..5b269ef8b3bb 100644
> --- a/xen/arch/x86/mm/hap/hap.c
> +++ b/xen/arch/x86/mm/hap/hap.c
> @@ -696,15 +696,6 @@ static void hap_update_cr3(struct vcpu *v, int do_locking, 
> bool noflush)
>       hvm_update_guest_cr3(v, noflush);
>   }
>   
> -/*
> - * Dummy function to use with on_selected_cpus in order to trigger a vmexit on
> - * selected pCPUs. When the VM resumes execution it will get a new ASID/VPID
> - * and thus a clean TLB.
> - */
> -static void dummy_flush(void *data)
> -{
> -}
> -
>   static bool flush_tlb(bool (*flush_vcpu)(void *ctxt, struct vcpu *v),
>                         void *ctxt)
>   {
> @@ -737,7 +728,7 @@ static bool flush_tlb(bool (*flush_vcpu)(void *ctxt, struct 
> vcpu *v),
>        * not currently running will already be flushed when scheduled because of
>        * the ASID tickle done in the loop above.
>        */
> -    on_selected_cpus(mask, dummy_flush, NULL, 0);
> +    on_selected_cpus(mask, NULL, NULL, 0);
>   
>       return true;
>   }
> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
> index b2d57a3ee89a..1459f66c006b 100644
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -1236,14 +1236,6 @@ static void ept_memory_type_changed(struct p2m_domain 
> *p2m)
>           ept_sync_domain(p2m);
>   }
>   
> -static void __ept_sync_domain(void *info)
> -{
> -    /*
> -     * The invalidation will be done before VMENTER (see
> -     * vmx_vmenter_helper()).
> -     */
> -}
> -
>   static void ept_sync_domain_prepare(struct p2m_domain *p2m)
>   {
>       struct domain *d = p2m->domain;
> @@ -1269,7 +1261,8 @@ static void ept_sync_domain_prepare(struct p2m_domain 
> *p2m)
>   
>   static void ept_sync_domain_mask(struct p2m_domain *p2m, const cpumask_t *mask)
>   {
> -    on_selected_cpus(mask, __ept_sync_domain, p2m, 1);
> +    /* Invalidation will be done in vmx_vmenter_helper(). */
> +    on_selected_cpus(mask, NULL, NULL, 1);
>   }
>   
>   void ept_sync_domain(struct p2m_domain *p2m)
> diff --git a/xen/common/smp.c b/xen/common/smp.c
> index 79f4ebd14502..854ebb91a803 100644
> --- a/xen/common/smp.c
> +++ b/xen/common/smp.c
> @@ -87,10 +87,14 @@ void smp_call_function_interrupt(void)
>   
>       irq_enter();
>   
> +    if ( unlikely(!func) )
> +        goto no_func;
> +
>       if ( call_data.wait )
>       {
>           (*func)(info);
>           smp_mb();
> +    no_func:
>           cpumask_clear_cpu(cpu, &call_data.selected);
>       }
>       else
Why only apply to call_data.wait non-zero case?
Is it because func will not be NULL when call_data.wait is zero?

>