Re: [Xen-devel] [PATCH for-4.13 v3 2/2] x86/vmx: always sync PIR to IRR before vmentry
On 27.11.2019 12:56, Roger Pau Monné wrote:
> On Wed, Nov 27, 2019 at 12:30:06PM +0100, Jan Beulich wrote:
>> On 27.11.2019 12:22, Roger Pau Monné wrote:
>>> On Tue, Nov 26, 2019 at 05:50:32PM +0100, Jan Beulich wrote:
On 26.11.2019 14:26, Roger Pau Monne wrote:
> --- a/xen/arch/x86/hvm/irq.c
> +++ b/xen/arch/x86/hvm/irq.c
> @@ -515,7 +515,11 @@ void hvm_set_callback_via(struct domain *d, uint64_t
> via)
> struct hvm_intack hvm_vcpu_has_pending_irq(struct vcpu *v)
> {
> struct hvm_domain *plat = >domain->arch.hvm;
> -int vector;
> +/*
> + * Always call vlapic_has_pending_irq so that PIR is synced into IRR
> when
> + * using posted interrupts.
> + */
> +int vector = vlapic_has_pending_irq(v);
Did you consider doing this conditionally either here ...
> @@ -530,7 +534,6 @@ struct hvm_intack hvm_vcpu_has_pending_irq(struct
> vcpu *v)
> if ( vlapic_accept_pic_intr(v) && plat->vpic[0].int_output )
> return hvm_intack_pic(0);
>
> -vector = vlapic_has_pending_irq(v);
> if ( vector != -1 )
> return hvm_intack_lapic(vector);
... or here?
>>>
>>> I'm afraid I don't follow. The whole point of this change is to ensure
>>> vlapic_has_pending_irq is unconditionally called in
>>> hvm_vcpu_has_pending_irq, so I'm not sure what you mean by "doing this
>>> conditionally...".
>>
>> Do it early when using interrupt posting, and keep it in its
>> current place otherwise.
>>
I ask not only because the function isn't exactly
cheap to call (as iirc you did also mention during the v2
discussion), but also because of its interaction with Viridian
and nested mode. In case of problems there, avoiding the use
of interrupt posting would be a workaround in such cases then.
>>>
>>> Would you like me to export something like vlapic_sync_pir_to_irr and
>>> call it unconditionally instead of calling vlapic_has_pending_irq?
>>
>> This looks to be another option, yes. Albeit instead of making
>> non-static (which I assume is what you mean by "export"), maybe
>> simply make this a static inline in vlapic.h then.
>
> Yes, that would work and IMO is better than moving the call to
> vlapic_has_pending_irq around. Are you OK with this approach?
I think so, yes.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.13 v3 2/2] x86/vmx: always sync PIR to IRR before vmentry
On Wed, Nov 27, 2019 at 12:30:06PM +0100, Jan Beulich wrote:
> On 27.11.2019 12:22, Roger Pau Monné wrote:
> > On Tue, Nov 26, 2019 at 05:50:32PM +0100, Jan Beulich wrote:
> >> On 26.11.2019 14:26, Roger Pau Monne wrote:
> >>> --- a/xen/arch/x86/hvm/irq.c
> >>> +++ b/xen/arch/x86/hvm/irq.c
> >>> @@ -515,7 +515,11 @@ void hvm_set_callback_via(struct domain *d, uint64_t
> >>> via)
> >>> struct hvm_intack hvm_vcpu_has_pending_irq(struct vcpu *v)
> >>> {
> >>> struct hvm_domain *plat = >domain->arch.hvm;
> >>> -int vector;
> >>> +/*
> >>> + * Always call vlapic_has_pending_irq so that PIR is synced into IRR
> >>> when
> >>> + * using posted interrupts.
> >>> + */
> >>> +int vector = vlapic_has_pending_irq(v);
> >>
> >> Did you consider doing this conditionally either here ...
> >>
> >>> @@ -530,7 +534,6 @@ struct hvm_intack hvm_vcpu_has_pending_irq(struct
> >>> vcpu *v)
> >>> if ( vlapic_accept_pic_intr(v) && plat->vpic[0].int_output )
> >>> return hvm_intack_pic(0);
> >>>
> >>> -vector = vlapic_has_pending_irq(v);
> >>> if ( vector != -1 )
> >>> return hvm_intack_lapic(vector);
> >>
> >> ... or here?
> >
> > I'm afraid I don't follow. The whole point of this change is to ensure
> > vlapic_has_pending_irq is unconditionally called in
> > hvm_vcpu_has_pending_irq, so I'm not sure what you mean by "doing this
> > conditionally...".
>
> Do it early when using interrupt posting, and keep it in its
> current place otherwise.
>
> >> I ask not only because the function isn't exactly
> >> cheap to call (as iirc you did also mention during the v2
> >> discussion), but also because of its interaction with Viridian
> >> and nested mode. In case of problems there, avoiding the use
> >> of interrupt posting would be a workaround in such cases then.
> >
> > Would you like me to export something like vlapic_sync_pir_to_irr and
> > call it unconditionally instead of calling vlapic_has_pending_irq?
>
> This looks to be another option, yes. Albeit instead of making
> non-static (which I assume is what you mean by "export"), maybe
> simply make this a static inline in vlapic.h then.
Yes, that would work and IMO is better than moving the call to
vlapic_has_pending_irq around. Are you OK with this approach?
Thanks, Roger.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.13 v3 2/2] x86/vmx: always sync PIR to IRR before vmentry
On 27.11.2019 12:22, Roger Pau Monné wrote:
> On Tue, Nov 26, 2019 at 05:50:32PM +0100, Jan Beulich wrote:
>> On 26.11.2019 14:26, Roger Pau Monne wrote:
>>> --- a/xen/arch/x86/hvm/irq.c
>>> +++ b/xen/arch/x86/hvm/irq.c
>>> @@ -515,7 +515,11 @@ void hvm_set_callback_via(struct domain *d, uint64_t
>>> via)
>>> struct hvm_intack hvm_vcpu_has_pending_irq(struct vcpu *v)
>>> {
>>> struct hvm_domain *plat = >domain->arch.hvm;
>>> -int vector;
>>> +/*
>>> + * Always call vlapic_has_pending_irq so that PIR is synced into IRR
>>> when
>>> + * using posted interrupts.
>>> + */
>>> +int vector = vlapic_has_pending_irq(v);
>>
>> Did you consider doing this conditionally either here ...
>>
>>> @@ -530,7 +534,6 @@ struct hvm_intack hvm_vcpu_has_pending_irq(struct vcpu
>>> *v)
>>> if ( vlapic_accept_pic_intr(v) && plat->vpic[0].int_output )
>>> return hvm_intack_pic(0);
>>>
>>> -vector = vlapic_has_pending_irq(v);
>>> if ( vector != -1 )
>>> return hvm_intack_lapic(vector);
>>
>> ... or here?
>
> I'm afraid I don't follow. The whole point of this change is to ensure
> vlapic_has_pending_irq is unconditionally called in
> hvm_vcpu_has_pending_irq, so I'm not sure what you mean by "doing this
> conditionally...".
Do it early when using interrupt posting, and keep it in its
current place otherwise.
>> I ask not only because the function isn't exactly
>> cheap to call (as iirc you did also mention during the v2
>> discussion), but also because of its interaction with Viridian
>> and nested mode. In case of problems there, avoiding the use
>> of interrupt posting would be a workaround in such cases then.
>
> Would you like me to export something like vlapic_sync_pir_to_irr and
> call it unconditionally instead of calling vlapic_has_pending_irq?
This looks to be another option, yes. Albeit instead of making
non-static (which I assume is what you mean by "export"), maybe
simply make this a static inline in vlapic.h then.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.13 v3 2/2] x86/vmx: always sync PIR to IRR before vmentry
On Tue, Nov 26, 2019 at 05:50:32PM +0100, Jan Beulich wrote:
> On 26.11.2019 14:26, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/hvm/irq.c
> > +++ b/xen/arch/x86/hvm/irq.c
> > @@ -515,7 +515,11 @@ void hvm_set_callback_via(struct domain *d, uint64_t
> > via)
> > struct hvm_intack hvm_vcpu_has_pending_irq(struct vcpu *v)
> > {
> > struct hvm_domain *plat = >domain->arch.hvm;
> > -int vector;
> > +/*
> > + * Always call vlapic_has_pending_irq so that PIR is synced into IRR
> > when
> > + * using posted interrupts.
> > + */
> > +int vector = vlapic_has_pending_irq(v);
>
> Did you consider doing this conditionally either here ...
>
> > @@ -530,7 +534,6 @@ struct hvm_intack hvm_vcpu_has_pending_irq(struct vcpu
> > *v)
> > if ( vlapic_accept_pic_intr(v) && plat->vpic[0].int_output )
> > return hvm_intack_pic(0);
> >
> > -vector = vlapic_has_pending_irq(v);
> > if ( vector != -1 )
> > return hvm_intack_lapic(vector);
>
> ... or here?
I'm afraid I don't follow. The whole point of this change is to ensure
vlapic_has_pending_irq is unconditionally called in
hvm_vcpu_has_pending_irq, so I'm not sure what you mean by "doing this
conditionally...".
> I ask not only because the function isn't exactly
> cheap to call (as iirc you did also mention during the v2
> discussion), but also because of its interaction with Viridian
> and nested mode. In case of problems there, avoiding the use
> of interrupt posting would be a workaround in such cases then.
Would you like me to export something like vlapic_sync_pir_to_irr and
call it unconditionally instead of calling vlapic_has_pending_irq?
Thanks, Roger.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.13 v3 2/2] x86/vmx: always sync PIR to IRR before vmentry
On 26.11.2019 19:02, Roger Pau Monné wrote:
> On Tue, Nov 26, 2019 at 05:50:32PM +0100, Jan Beulich wrote:
>> On 26.11.2019 14:26, Roger Pau Monne wrote:
>>> --- a/xen/arch/x86/hvm/irq.c
>>> +++ b/xen/arch/x86/hvm/irq.c
>>> @@ -515,7 +515,11 @@ void hvm_set_callback_via(struct domain *d, uint64_t
>>> via)
>>> struct hvm_intack hvm_vcpu_has_pending_irq(struct vcpu *v)
>>> {
>>> struct hvm_domain *plat = >domain->arch.hvm;
>>> -int vector;
>>> +/*
>>> + * Always call vlapic_has_pending_irq so that PIR is synced into IRR
>>> when
>>> + * using posted interrupts.
>>> + */
>>> +int vector = vlapic_has_pending_irq(v);
>>
>> Did you consider doing this conditionally either here ...
>>
>>> @@ -530,7 +534,6 @@ struct hvm_intack hvm_vcpu_has_pending_irq(struct vcpu
>>> *v)
>>> if ( vlapic_accept_pic_intr(v) && plat->vpic[0].int_output )
>>> return hvm_intack_pic(0);
>>>
>>> -vector = vlapic_has_pending_irq(v);
>>> if ( vector != -1 )
>>> return hvm_intack_lapic(vector);
>>
>> ... or here? I ask not only because the function isn't exactly
>> cheap to call (as iirc you did also mention during the v2
>> discussion), but also because of its interaction with Viridian
>> and nested mode. In case of problems there, avoiding the use
>> of interrupt posting would be a workaround in such cases then.
>
> TBH my preference was to do the PIR to IRR sync in vmx_intr_assist by
> directly calling vmx_sync_pir_to_irr because it was IMO less intrusive
> and confined to VMX code. I think this approach is more risky as
> vlapic_has_pending_irq does way more than simply syncing PIR to IRR.
Confining to VMX code may indeed make sense as long as we don't
support the SVM equivalent, but from an abstract pov such syncing
will likely need to happen in a vendor neutral way. In any event,
if the VMX maintainers prefer the other placement, so be it.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.13 v3 2/2] x86/vmx: always sync PIR to IRR before vmentry
> From: Roger Pau Monne
> Sent: Tuesday, November 26, 2019 9:27 PM
>
> When using posted interrupts on Intel hardware it's possible that the
> vCPU resumes execution with a stale local APIC IRR register because
> depending on the interrupts to be injected vlapic_has_pending_irq
> might not be called, and thus PIR won't be synced into IRR.
>
> Fix this by making sure PIR is always synced to IRR in
> hvm_vcpu_has_pending_irq regardless of what interrupts are pending.
>
> While there also simplify the code in __vmx_deliver_posted_interrupt:
> only raise a softirq if the vCPU is the one currently running and
> __vmx_deliver_posted_interrupt is called from interrupt context. The
as commented earlier, this is what exactly original code does. Then
what is the simplification?
> softirq is raised to make sure vmx_intr_assist is retried if the
> interrupt happens to arrive after vmx_intr_assist but before
> interrupts are disabled in vmx_do_vmentry. Also simplify the logic for
> IPIing other pCPUs, there's no need to check v->processor since the
> IPI should be sent as long as the vCPU is not the current one and it's
> running.
>
> Reported-by: Joe Jin
> Signed-off-by: Roger Pau Monné
> ---
> Cc: Juergen Gross
> ---
> Changes since v2:
> - Raise a softirq if in interrupt context and the vCPU is the current
>one.
> - Use is_running instead of runnable.
> - Remove the call to vmx_sync_pir_to_irr in vmx_intr_assist and
>instead always call vlapic_has_pending_irq in
>hvm_vcpu_has_pending_irq.
> ---
> xen/arch/x86/hvm/irq.c | 7 +++--
> xen/arch/x86/hvm/vmx/vmx.c | 64 +++---
> 2 files changed, 24 insertions(+), 47 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c
> index e03a87ad50..b50ac62a16 100644
> --- a/xen/arch/x86/hvm/irq.c
> +++ b/xen/arch/x86/hvm/irq.c
> @@ -515,7 +515,11 @@ void hvm_set_callback_via(struct domain *d,
> uint64_t via)
> struct hvm_intack hvm_vcpu_has_pending_irq(struct vcpu *v)
> {
> struct hvm_domain *plat = >domain->arch.hvm;
> -int vector;
> +/*
> + * Always call vlapic_has_pending_irq so that PIR is synced into IRR when
> + * using posted interrupts.
> + */
> +int vector = vlapic_has_pending_irq(v);
>
> if ( unlikely(v->nmi_pending) )
> return hvm_intack_nmi;
> @@ -530,7 +534,6 @@ struct hvm_intack hvm_vcpu_has_pending_irq(struct
> vcpu *v)
> if ( vlapic_accept_pic_intr(v) && plat->vpic[0].int_output )
> return hvm_intack_pic(0);
>
> -vector = vlapic_has_pending_irq(v);
> if ( vector != -1 )
> return hvm_intack_lapic(vector);
>
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index c817aec75d..4dea868cda 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -1945,57 +1945,31 @@ static void vmx_process_isr(int isr, struct vcpu
> *v)
>
> static void __vmx_deliver_posted_interrupt(struct vcpu *v)
> {
> -bool_t running = v->is_running;
> -
> vcpu_unblock(v);
> -/*
> - * Just like vcpu_kick(), nothing is needed for the following two cases:
> - * 1. The target vCPU is not running, meaning it is blocked or runnable.
> - * 2. The target vCPU is the current vCPU and we're in non-interrupt
> - * context.
> - */
> -if ( running && (in_irq() || (v != current)) )
> -{
> +if ( v->is_running && v != current )
> /*
> - * Note: Only two cases will reach here:
> - * 1. The target vCPU is running on other pCPU.
> - * 2. The target vCPU is the current vCPU.
> + * If the vCPU is running on another pCPU send an IPI to the pCPU.
> When
> + * the IPI arrives, the target vCPU may be running in non-root mode,
> + * running in root mode, runnable or blocked. If the target vCPU is
> + * running in non-root mode, the hardware will sync PIR to vIRR for
> + * 'posted_intr_vector' is a special vector handled directly by the
> + * hardware.
> *
> - * Note2: Don't worry the v->processor may change. The vCPU being
> - * moved to another processor is guaranteed to sync PIR to vIRR,
> - * due to the involved scheduling cycle.
> + * If the target vCPU is running in root-mode, the interrupt handler
> + * starts to run. Considering an IPI may arrive in the window between
> + * the call to vmx_intr_assist() and interrupts getting disabled, the
> + * interrupt handler should raise a softirq to ensure events will be
> + * delivered in time.
I prefer to original comment which covers all possible conditions that the
target vcpu might be. You may help improve it if some words are not
well-written, but removing useful information is not good there.
> */
> -unsigned int cpu = v->processor;
> -
> -/*
> - * For case 1, we send an IPI to the pCPU. When an IPI arrives, the
> - *
Re: [Xen-devel] [PATCH for-4.13 v3 2/2] x86/vmx: always sync PIR to IRR before vmentry
On Tue, Nov 26, 2019 at 05:50:32PM +0100, Jan Beulich wrote:
> On 26.11.2019 14:26, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/hvm/irq.c
> > +++ b/xen/arch/x86/hvm/irq.c
> > @@ -515,7 +515,11 @@ void hvm_set_callback_via(struct domain *d, uint64_t
> > via)
> > struct hvm_intack hvm_vcpu_has_pending_irq(struct vcpu *v)
> > {
> > struct hvm_domain *plat = >domain->arch.hvm;
> > -int vector;
> > +/*
> > + * Always call vlapic_has_pending_irq so that PIR is synced into IRR
> > when
> > + * using posted interrupts.
> > + */
> > +int vector = vlapic_has_pending_irq(v);
>
> Did you consider doing this conditionally either here ...
>
> > @@ -530,7 +534,6 @@ struct hvm_intack hvm_vcpu_has_pending_irq(struct vcpu
> > *v)
> > if ( vlapic_accept_pic_intr(v) && plat->vpic[0].int_output )
> > return hvm_intack_pic(0);
> >
> > -vector = vlapic_has_pending_irq(v);
> > if ( vector != -1 )
> > return hvm_intack_lapic(vector);
>
> ... or here? I ask not only because the function isn't exactly
> cheap to call (as iirc you did also mention during the v2
> discussion), but also because of its interaction with Viridian
> and nested mode. In case of problems there, avoiding the use
> of interrupt posting would be a workaround in such cases then.
TBH my preference was to do the PIR to IRR sync in vmx_intr_assist by
directly calling vmx_sync_pir_to_irr because it was IMO less intrusive
and confined to VMX code. I think this approach is more risky as
vlapic_has_pending_irq does way more than simply syncing PIR to IRR.
> > --- a/xen/arch/x86/hvm/vmx/vmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
> > @@ -1945,57 +1945,31 @@ static void vmx_process_isr(int isr, struct vcpu *v)
> >
> > static void __vmx_deliver_posted_interrupt(struct vcpu *v)
> > {
> > -bool_t running = v->is_running;
> > -
> > vcpu_unblock(v);
> > -/*
> > - * Just like vcpu_kick(), nothing is needed for the following two
> > cases:
> > - * 1. The target vCPU is not running, meaning it is blocked or
> > runnable.
> > - * 2. The target vCPU is the current vCPU and we're in non-interrupt
> > - * context.
> > - */
> > -if ( running && (in_irq() || (v != current)) )
> > -{
> > +if ( v->is_running && v != current )
>
> I continue to be concerned by this evaluation of ->is_running
> _after_ vcpu_unblock(), when previously (just like vcpu_kick()
> does) it was intentionally done before.
If the unblock sets v->is_running to true that's fine, Xen will send a
posted interrupt IPI and the destination pCPU will either be in root
mode and thus raise a softirq or in non-root mode and perform the PIR
to IRR sync and possible interrupt injection.
I see that caching the value of is_running might be helpful in order
to prevent pointless IPI'ing. If the vCPU wasn't running before the
unblock there's no reason to send an IPI to it, because the sync of
PIR to IRR will happen at vmentry anyway.
> I wonder anyway
> whether this and the change to irq.c should really be in a
> single patch, the more that you start the respective part of
> the description with "While there also simplify ...". But in
> the end it is up to Kevin's or Jun's judgement anyway.
Yes, that makes sense. Will wait for feedback from Kevin or Jun before
sending a new version anyway.
Thanks, Roger.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.13 v3 2/2] x86/vmx: always sync PIR to IRR before vmentry
On 26.11.2019 14:26, Roger Pau Monne wrote:
> --- a/xen/arch/x86/hvm/irq.c
> +++ b/xen/arch/x86/hvm/irq.c
> @@ -515,7 +515,11 @@ void hvm_set_callback_via(struct domain *d, uint64_t via)
> struct hvm_intack hvm_vcpu_has_pending_irq(struct vcpu *v)
> {
> struct hvm_domain *plat = >domain->arch.hvm;
> -int vector;
> +/*
> + * Always call vlapic_has_pending_irq so that PIR is synced into IRR when
> + * using posted interrupts.
> + */
> +int vector = vlapic_has_pending_irq(v);
Did you consider doing this conditionally either here ...
> @@ -530,7 +534,6 @@ struct hvm_intack hvm_vcpu_has_pending_irq(struct vcpu *v)
> if ( vlapic_accept_pic_intr(v) && plat->vpic[0].int_output )
> return hvm_intack_pic(0);
>
> -vector = vlapic_has_pending_irq(v);
> if ( vector != -1 )
> return hvm_intack_lapic(vector);
... or here? I ask not only because the function isn't exactly
cheap to call (as iirc you did also mention during the v2
discussion), but also because of its interaction with Viridian
and nested mode. In case of problems there, avoiding the use
of interrupt posting would be a workaround in such cases then.
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -1945,57 +1945,31 @@ static void vmx_process_isr(int isr, struct vcpu *v)
>
> static void __vmx_deliver_posted_interrupt(struct vcpu *v)
> {
> -bool_t running = v->is_running;
> -
> vcpu_unblock(v);
> -/*
> - * Just like vcpu_kick(), nothing is needed for the following two cases:
> - * 1. The target vCPU is not running, meaning it is blocked or runnable.
> - * 2. The target vCPU is the current vCPU and we're in non-interrupt
> - * context.
> - */
> -if ( running && (in_irq() || (v != current)) )
> -{
> +if ( v->is_running && v != current )
I continue to be concerned by this evaluation of ->is_running
_after_ vcpu_unblock(), when previously (just like vcpu_kick()
does) it was intentionally done before. I wonder anyway
whether this and the change to irq.c should really be in a
single patch, the more that you start the respective part of
the description with "While there also simplify ...". But in
the end it is up to Kevin's or Jun's judgement anyway.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.13 v3 2/2] x86/vmx: always sync PIR to IRR before vmentry
On 11/26/19 5:26 AM, Roger Pau Monne wrote: > When using posted interrupts on Intel hardware it's possible that the > vCPU resumes execution with a stale local APIC IRR register because > depending on the interrupts to be injected vlapic_has_pending_irq > might not be called, and thus PIR won't be synced into IRR. > > Fix this by making sure PIR is always synced to IRR in > hvm_vcpu_has_pending_irq regardless of what interrupts are pending. > > While there also simplify the code in __vmx_deliver_posted_interrupt: > only raise a softirq if the vCPU is the one currently running and > __vmx_deliver_posted_interrupt is called from interrupt context. The > softirq is raised to make sure vmx_intr_assist is retried if the > interrupt happens to arrive after vmx_intr_assist but before > interrupts are disabled in vmx_do_vmentry. Also simplify the logic for > IPIing other pCPUs, there's no need to check v->processor since the > IPI should be sent as long as the vCPU is not the current one and it's > running. > > Reported-by: Joe Jin > Signed-off-by: Roger Pau Monné > --- > Cc: Juergen Gross Patch works for me. Tested-by: Joe Jin Thanks, Joe ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH for-4.13 v3 2/2] x86/vmx: always sync PIR to IRR before vmentry
When using posted interrupts on Intel hardware it's possible that the
vCPU resumes execution with a stale local APIC IRR register because
depending on the interrupts to be injected vlapic_has_pending_irq
might not be called, and thus PIR won't be synced into IRR.
Fix this by making sure PIR is always synced to IRR in
hvm_vcpu_has_pending_irq regardless of what interrupts are pending.
While there also simplify the code in __vmx_deliver_posted_interrupt:
only raise a softirq if the vCPU is the one currently running and
__vmx_deliver_posted_interrupt is called from interrupt context. The
softirq is raised to make sure vmx_intr_assist is retried if the
interrupt happens to arrive after vmx_intr_assist but before
interrupts are disabled in vmx_do_vmentry. Also simplify the logic for
IPIing other pCPUs, there's no need to check v->processor since the
IPI should be sent as long as the vCPU is not the current one and it's
running.
Reported-by: Joe Jin
Signed-off-by: Roger Pau Monné
---
Cc: Juergen Gross
---
Changes since v2:
- Raise a softirq if in interrupt context and the vCPU is the current
one.
- Use is_running instead of runnable.
- Remove the call to vmx_sync_pir_to_irr in vmx_intr_assist and
instead always call vlapic_has_pending_irq in
hvm_vcpu_has_pending_irq.
---
xen/arch/x86/hvm/irq.c | 7 +++--
xen/arch/x86/hvm/vmx/vmx.c | 64 +++---
2 files changed, 24 insertions(+), 47 deletions(-)
diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c
index e03a87ad50..b50ac62a16 100644
--- a/xen/arch/x86/hvm/irq.c
+++ b/xen/arch/x86/hvm/irq.c
@@ -515,7 +515,11 @@ void hvm_set_callback_via(struct domain *d, uint64_t via)
struct hvm_intack hvm_vcpu_has_pending_irq(struct vcpu *v)
{
struct hvm_domain *plat = >domain->arch.hvm;
-int vector;
+/*
+ * Always call vlapic_has_pending_irq so that PIR is synced into IRR when
+ * using posted interrupts.
+ */
+int vector = vlapic_has_pending_irq(v);
if ( unlikely(v->nmi_pending) )
return hvm_intack_nmi;
@@ -530,7 +534,6 @@ struct hvm_intack hvm_vcpu_has_pending_irq(struct vcpu *v)
if ( vlapic_accept_pic_intr(v) && plat->vpic[0].int_output )
return hvm_intack_pic(0);
-vector = vlapic_has_pending_irq(v);
if ( vector != -1 )
return hvm_intack_lapic(vector);
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index c817aec75d..4dea868cda 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1945,57 +1945,31 @@ static void vmx_process_isr(int isr, struct vcpu *v)
static void __vmx_deliver_posted_interrupt(struct vcpu *v)
{
-bool_t running = v->is_running;
-
vcpu_unblock(v);
-/*
- * Just like vcpu_kick(), nothing is needed for the following two cases:
- * 1. The target vCPU is not running, meaning it is blocked or runnable.
- * 2. The target vCPU is the current vCPU and we're in non-interrupt
- * context.
- */
-if ( running && (in_irq() || (v != current)) )
-{
+if ( v->is_running && v != current )
/*
- * Note: Only two cases will reach here:
- * 1. The target vCPU is running on other pCPU.
- * 2. The target vCPU is the current vCPU.
+ * If the vCPU is running on another pCPU send an IPI to the pCPU. When
+ * the IPI arrives, the target vCPU may be running in non-root mode,
+ * running in root mode, runnable or blocked. If the target vCPU is
+ * running in non-root mode, the hardware will sync PIR to vIRR for
+ * 'posted_intr_vector' is a special vector handled directly by the
+ * hardware.
*
- * Note2: Don't worry the v->processor may change. The vCPU being
- * moved to another processor is guaranteed to sync PIR to vIRR,
- * due to the involved scheduling cycle.
+ * If the target vCPU is running in root-mode, the interrupt handler
+ * starts to run. Considering an IPI may arrive in the window between
+ * the call to vmx_intr_assist() and interrupts getting disabled, the
+ * interrupt handler should raise a softirq to ensure events will be
+ * delivered in time.
*/
-unsigned int cpu = v->processor;
-
-/*
- * For case 1, we send an IPI to the pCPU. When an IPI arrives, the
- * target vCPU maybe is running in non-root mode, running in root
- * mode, runnable or blocked. If the target vCPU is running in
- * non-root mode, the hardware will sync PIR to vIRR for
- * 'posted_intr_vector' is special to the pCPU. If the target vCPU is
- * running in root-mode, the interrupt handler starts to run.
- * Considering an IPI may arrive in the window between the call to
- * vmx_intr_assist() and interrupts getting disabled, the interrupt
- * handler should raise a softirq to ensure events will be delivered
- *
