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.coop...@citrix.com>
>> ---
>> CC: Jan Beulich <jbeul...@suse.com>
>> CC: Roger Pau Monné <roger....@citrix.com>
>> CC: Wei Liu <w...@xen.org>
>>
>> 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
