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(-)
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@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@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
--
2.11.0
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@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@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?
>
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@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Wei Liu <wl@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
On 18.11.2021 07:51, Wei Chen wrote:
> On 2021/11/18 0:48, Andrew Cooper wrote:
>> --- 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?
No. call_data.wait is irrelevant when no function is to be called.
If you look at the "else" branch, all that would be needed there
is again just the cpumask_clear_cpu() invocation. Hence easiest to
wire it like Andrew did (leaving aside my personal dislike of goto).
Jan
On 17.11.2021 17: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@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Wei Liu <wl@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. I can accept this, albeit personally I would have preferred the extra if() over the goto. Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
On 18/11/2021 09:58, Jan Beulich wrote:
> On 17.11.2021 17: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@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Wei Liu <wl@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.
> I can accept this, albeit personally I would have preferred the extra if()
> over the goto.
Just so it's been posted. This is what the if/else looks like:
diff --git a/xen/common/smp.c b/xen/common/smp.c
index 79f4ebd14502..ff569bbe9d53 100644
--- a/xen/common/smp.c
+++ b/xen/common/smp.c
@@ -87,7 +87,11 @@ void smp_call_function_interrupt(void)
irq_enter();
- if ( call_data.wait )
+ if ( unlikely(!func) )
+ {
+ cpumask_clear_cpu(cpu, &call_data.selected);
+ }
+ else if ( call_data.wait )
{
(*func)(info);
smp_mb();
GCC does manage to fold this into the goto version presented originally.
If everyone else thinks this version is clearer to read then I'll go
with it.
~Andrew
Hi Andrew,
> -----Original Message-----
> From: Andrew Cooper <amc96@srcf.net>
> Sent: 2021年11月18日 18:35
> To: Jan Beulich <jbeulich@suse.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; Wei Chen <Wei.Chen@arm.com>
> Cc: Roger Pau Monné <roger.pau@citrix.com>; Wei Liu <wl@xen.org>; Xen-
> devel <xen-devel@lists.xenproject.org>
> Subject: Re: [PATCH] xen/smp: Support NULL IPI function pointers
>
> On 18/11/2021 09:58, Jan Beulich wrote:
> > On 17.11.2021 17: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@citrix.com>
> >> ---
> >> CC: Jan Beulich <JBeulich@suse.com>
> >> CC: Roger Pau Monné <roger.pau@citrix.com>
> >> CC: Wei Liu <wl@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.
> > I can accept this, albeit personally I would have preferred the extra
> if()
> > over the goto.
>
> Just so it's been posted. This is what the if/else looks like:
>
> diff --git a/xen/common/smp.c b/xen/common/smp.c
> index 79f4ebd14502..ff569bbe9d53 100644
> --- a/xen/common/smp.c
> +++ b/xen/common/smp.c
> @@ -87,7 +87,11 @@ void smp_call_function_interrupt(void)
>
> irq_enter();
>
> - if ( call_data.wait )
> + if ( unlikely(!func) )
> + {
> + cpumask_clear_cpu(cpu, &call_data.selected);
> + }
> + else if ( call_data.wait )
> {
> (*func)(info);
> smp_mb();
>
>
> GCC does manage to fold this into the goto version presented originally.
>
> If everyone else thinks this version is clearer to read then I'll go
> with it.
This version is much clearer to read. But if there will be some code
comments in goto version to make it easy to read. I am ok for either.
>
> ~Andrew
> On 18 Nov 2021, at 10:35, Andrew Cooper <amc96@srcf.net> wrote:
>
> On 18/11/2021 09:58, Jan Beulich wrote:
>> On 17.11.2021 17: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@citrix.com>
>>> ---
>>> CC: Jan Beulich <JBeulich@suse.com>
>>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>> CC: Wei Liu <wl@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.
>> I can accept this, albeit personally I would have preferred the extra if()
>> over the goto.
>
> Just so it's been posted. This is what the if/else looks like:
>
> diff --git a/xen/common/smp.c b/xen/common/smp.c
> index 79f4ebd14502..ff569bbe9d53 100644
> --- a/xen/common/smp.c
> +++ b/xen/common/smp.c
> @@ -87,7 +87,11 @@ void smp_call_function_interrupt(void)
>
> irq_enter();
>
> - if ( call_data.wait )
> + if ( unlikely(!func) )
> + {
> + cpumask_clear_cpu(cpu, &call_data.selected);
> + }
> + else if ( call_data.wait )
> {
> (*func)(info);
> smp_mb();
>
>
> GCC does manage to fold this into the goto version presented originally.
>
> If everyone else thinks this version is clearer to read then I'll go
> with it.
Hi Andrew,
I would vote for the long version if it’s not a problem.
Cheers,
Luca
>
> ~Andrew
© 2016 - 2024 Red Hat, Inc.