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

[Andrew Cooper <amc96@xxxxxxxx>]

On 18/11/2021 06:51, Wei Chen wrote:
> 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?

This was explicitly discussed:

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

~Andrew