| Message ID | 20200812124709.4165-5-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/vlapic: implement EOI callbacks | expand |
> -----Original Message----- > From: Roger Pau Monne <roger.pau@citrix.com> > Sent: 12 August 2020 13:47 > To: xen-devel@lists.xenproject.org > Cc: Roger Pau Monne <roger.pau@citrix.com>; Paul Durrant <paul@xen.org>; Wei Liu <wl@xen.org>; Jan > Beulich <jbeulich@suse.com>; Andrew Cooper <andrew.cooper3@citrix.com> > Subject: [PATCH 4/5] x86/viridian: switch synic to use the new EOI callback > > Switch synic interrupts to use an EOI callback in order to execute the > logic tied to the end of interrupt. This allows to remove the synic > call in vlapic_handle_EOI. > > Move and rename viridian_synic_ack_sint now that it can be made > static. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > I'm unsure about the logic in viridian_synic_deliver_timer_msg, as it > seems to only set the vector in msg_pending when the message is > already pending? See section 11.10.3 of the TLFS (SynIC Message Flags)... "The MessagePending flag indicates whether or not there are any messages pending in the message queue of the synthetic interrupt source. If there are, then an “end of message” must be performed by the guest after emptying the message slot. This allows for opportunistic writes to the EOM MSR (only when required). Note that this flag may be set by the hypervisor upon message delivery or at any time afterwards. The flag should be tested after the message slot has been emptied and if set, then there are one or more pending messages and the “end of message” should be performed." IOW it's a bit like APIC assist in that it tries to avoid a VMEXIT (in this case an access to the EOM MSR) unless it is necessary. Reading the code again I think it may well be possible to get rid of the 'msg_pending' flag since it only appears to be an optimization to avoid testing 'message_type'. I'll try dropping it and see what breaks. Paul > --- > xen/arch/x86/hvm/viridian/synic.c | 28 +++++++++++++++------------- > xen/arch/x86/hvm/vlapic.c | 4 ---- > xen/include/asm-x86/hvm/viridian.h | 1 - > 3 files changed, 15 insertions(+), 18 deletions(-) > > diff --git a/xen/arch/x86/hvm/viridian/synic.c b/xen/arch/x86/hvm/viridian/synic.c > index 94a2b88733..250f0353cf 100644 > --- a/xen/arch/x86/hvm/viridian/synic.c > +++ b/xen/arch/x86/hvm/viridian/synic.c > @@ -315,6 +315,19 @@ void viridian_synic_poll(struct vcpu *v) > viridian_time_poll_timers(v); > } > > +static void synic_ack_sint(struct vcpu *v, unsigned int vector, void *data) > +{ > + struct viridian_vcpu *vv = v->arch.hvm.viridian; > + unsigned int sintx = vv->vector_to_sintx[vector]; > + > + ASSERT(v == current); > + > + if ( sintx < ARRAY_SIZE(vv->sint) ) > + __clear_bit(array_index_nospec(sintx, ARRAY_SIZE(vv->sint)), > + &vv->msg_pending); > +} > + > + > bool viridian_synic_deliver_timer_msg(struct vcpu *v, unsigned int sintx, > unsigned int index, > uint64_t expiration, > @@ -361,7 +374,8 @@ bool viridian_synic_deliver_timer_msg(struct vcpu *v, unsigned int sintx, > memcpy(msg->u.payload, &payload, sizeof(payload)); > > if ( !vs->masked ) > - vlapic_set_irq(vcpu_vlapic(v), vs->vector, 0); > + vlapic_set_irq_callback(vcpu_vlapic(v), vs->vector, 0, > + synic_ack_sint, NULL); > > return true; > } > @@ -380,18 +394,6 @@ bool viridian_synic_is_auto_eoi_sint(const struct vcpu *v, > return vs->auto_eoi; > } > > -void viridian_synic_ack_sint(const struct vcpu *v, unsigned int vector) > -{ > - struct viridian_vcpu *vv = v->arch.hvm.viridian; > - unsigned int sintx = vv->vector_to_sintx[vector]; > - > - ASSERT(v == current); > - > - if ( sintx < ARRAY_SIZE(vv->sint) ) > - __clear_bit(array_index_nospec(sintx, ARRAY_SIZE(vv->sint)), > - &vv->msg_pending); > -} > - > void viridian_synic_save_vcpu_ctxt(const struct vcpu *v, > struct hvm_viridian_vcpu_context *ctxt) > { > diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c > index 3b3b3d7621..701ff942e6 100644 > --- a/xen/arch/x86/hvm/vlapic.c > +++ b/xen/arch/x86/hvm/vlapic.c > @@ -489,12 +489,8 @@ void vlapic_handle_EOI(struct vlapic *vlapic, u8 vector) > void *data; > unsigned long flags; > > - /* All synic SINTx vectors are edge triggered */ > - > if ( vlapic_test_vector(vector, &vlapic->regs->data[APIC_TMR]) ) > vioapic_update_EOI(d, vector); > - else if ( has_viridian_synic(d) ) > - viridian_synic_ack_sint(v, vector); > > spin_lock_irqsave(&vlapic->callback_lock, flags); > callback = vlapic->callbacks[vector].callback; > diff --git a/xen/include/asm-x86/hvm/viridian.h b/xen/include/asm-x86/hvm/viridian.h > index 844e56b38f..d387d11ce0 100644 > --- a/xen/include/asm-x86/hvm/viridian.h > +++ b/xen/include/asm-x86/hvm/viridian.h > @@ -89,7 +89,6 @@ void viridian_apic_assist_clear(const struct vcpu *v); > void viridian_synic_poll(struct vcpu *v); > bool viridian_synic_is_auto_eoi_sint(const struct vcpu *v, > unsigned int vector); > -void viridian_synic_ack_sint(const struct vcpu *v, unsigned int vector); > > #endif /* __ASM_X86_HVM_VIRIDIAN_H__ */ > > -- > 2.28.0
On Thu, Aug 13, 2020 at 09:33:43AM +0100, Paul Durrant wrote: > > -----Original Message----- > > From: Roger Pau Monne <roger.pau@citrix.com> > > Sent: 12 August 2020 13:47 > > To: xen-devel@lists.xenproject.org > > Cc: Roger Pau Monne <roger.pau@citrix.com>; Paul Durrant <paul@xen.org>; Wei Liu <wl@xen.org>; Jan > > Beulich <jbeulich@suse.com>; Andrew Cooper <andrew.cooper3@citrix.com> > > Subject: [PATCH 4/5] x86/viridian: switch synic to use the new EOI callback > > > > Switch synic interrupts to use an EOI callback in order to execute the > > logic tied to the end of interrupt. This allows to remove the synic > > call in vlapic_handle_EOI. > > > > Move and rename viridian_synic_ack_sint now that it can be made > > static. > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > --- > > I'm unsure about the logic in viridian_synic_deliver_timer_msg, as it > > seems to only set the vector in msg_pending when the message is > > already pending? > > See section 11.10.3 of the TLFS (SynIC Message Flags)... > > "The MessagePending flag indicates whether or not there are any > messages pending in the message queue of the synthetic interrupt > source. If there are, then an “end of message” must be performed by > the guest after emptying the message slot. This allows for > opportunistic writes to the EOM MSR (only when required). Note that > this flag may be set by the hypervisor upon message delivery or at > any time afterwards. The flag should be tested after the message > slot has been emptied and if set, then there are one or more pending > messages and the “end of message” should be performed." > > IOW it's a bit like APIC assist in that it tries to avoid a VMEXIT > (in this case an access to the EOM MSR) unless it is necessary. > > Reading the code again I think it may well be possible to get rid of > the 'msg_pending' flag since it only appears to be an optimization > to avoid testing 'message_type'. I'll try dropping it and see what > breaks. Ack, I think the current approach in this patch would keep the same behavior. Thanks, Roger.
> -----Original Message----- > From: Roger Pau Monné <roger.pau@citrix.com> > Sent: 13 August 2020 09:58 > To: paul@xen.org > Cc: xen-devel@lists.xenproject.org; 'Wei Liu' <wl@xen.org>; 'Jan Beulich' <jbeulich@suse.com>; 'Andrew > Cooper' <andrew.cooper3@citrix.com> > Subject: Re: [PATCH 4/5] x86/viridian: switch synic to use the new EOI callback > > On Thu, Aug 13, 2020 at 09:33:43AM +0100, Paul Durrant wrote: > > > -----Original Message----- > > > From: Roger Pau Monne <roger.pau@citrix.com> > > > Sent: 12 August 2020 13:47 > > > To: xen-devel@lists.xenproject.org > > > Cc: Roger Pau Monne <roger.pau@citrix.com>; Paul Durrant <paul@xen.org>; Wei Liu <wl@xen.org>; Jan > > > Beulich <jbeulich@suse.com>; Andrew Cooper <andrew.cooper3@citrix.com> > > > Subject: [PATCH 4/5] x86/viridian: switch synic to use the new EOI callback > > > > > > Switch synic interrupts to use an EOI callback in order to execute the > > > logic tied to the end of interrupt. This allows to remove the synic > > > call in vlapic_handle_EOI. > > > > > > Move and rename viridian_synic_ack_sint now that it can be made > > > static. > > > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > > --- > > > I'm unsure about the logic in viridian_synic_deliver_timer_msg, as it > > > seems to only set the vector in msg_pending when the message is > > > already pending? > > > > See section 11.10.3 of the TLFS (SynIC Message Flags)... > > > > "The MessagePending flag indicates whether or not there are any > > messages pending in the message queue of the synthetic interrupt > > source. If there are, then an “end of message” must be performed by > > the guest after emptying the message slot. This allows for > > opportunistic writes to the EOM MSR (only when required). Note that > > this flag may be set by the hypervisor upon message delivery or at > > any time afterwards. The flag should be tested after the message > > slot has been emptied and if set, then there are one or more pending > > messages and the “end of message” should be performed." > > > > IOW it's a bit like APIC assist in that it tries to avoid a VMEXIT > > (in this case an access to the EOM MSR) unless it is necessary. > > > > Reading the code again I think it may well be possible to get rid of > > the 'msg_pending' flag since it only appears to be an optimization > > to avoid testing 'message_type'. I'll try dropping it and see what > > breaks. > Well nothing apparently broke. The EOM handler basically becomes a no-op too, but I think this is fine because we only use the synic for delivering timer messages at the moment. Paul
diff --git a/xen/arch/x86/hvm/viridian/synic.c b/xen/arch/x86/hvm/viridian/synic.c index 94a2b88733..250f0353cf 100644 --- a/xen/arch/x86/hvm/viridian/synic.c +++ b/xen/arch/x86/hvm/viridian/synic.c @@ -315,6 +315,19 @@ void viridian_synic_poll(struct vcpu *v) viridian_time_poll_timers(v); } +static void synic_ack_sint(struct vcpu *v, unsigned int vector, void *data) +{ + struct viridian_vcpu *vv = v->arch.hvm.viridian; + unsigned int sintx = vv->vector_to_sintx[vector]; + + ASSERT(v == current); + + if ( sintx < ARRAY_SIZE(vv->sint) ) + __clear_bit(array_index_nospec(sintx, ARRAY_SIZE(vv->sint)), + &vv->msg_pending); +} + + bool viridian_synic_deliver_timer_msg(struct vcpu *v, unsigned int sintx, unsigned int index, uint64_t expiration, @@ -361,7 +374,8 @@ bool viridian_synic_deliver_timer_msg(struct vcpu *v, unsigned int sintx, memcpy(msg->u.payload, &payload, sizeof(payload)); if ( !vs->masked ) - vlapic_set_irq(vcpu_vlapic(v), vs->vector, 0); + vlapic_set_irq_callback(vcpu_vlapic(v), vs->vector, 0, + synic_ack_sint, NULL); return true; } @@ -380,18 +394,6 @@ bool viridian_synic_is_auto_eoi_sint(const struct vcpu *v, return vs->auto_eoi; } -void viridian_synic_ack_sint(const struct vcpu *v, unsigned int vector) -{ - struct viridian_vcpu *vv = v->arch.hvm.viridian; - unsigned int sintx = vv->vector_to_sintx[vector]; - - ASSERT(v == current); - - if ( sintx < ARRAY_SIZE(vv->sint) ) - __clear_bit(array_index_nospec(sintx, ARRAY_SIZE(vv->sint)), - &vv->msg_pending); -} - void viridian_synic_save_vcpu_ctxt(const struct vcpu *v, struct hvm_viridian_vcpu_context *ctxt) { diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c index 3b3b3d7621..701ff942e6 100644 --- a/xen/arch/x86/hvm/vlapic.c +++ b/xen/arch/x86/hvm/vlapic.c @@ -489,12 +489,8 @@ void vlapic_handle_EOI(struct vlapic *vlapic, u8 vector) void *data; unsigned long flags; - /* All synic SINTx vectors are edge triggered */ - if ( vlapic_test_vector(vector, &vlapic->regs->data[APIC_TMR]) ) vioapic_update_EOI(d, vector); - else if ( has_viridian_synic(d) ) - viridian_synic_ack_sint(v, vector); spin_lock_irqsave(&vlapic->callback_lock, flags); callback = vlapic->callbacks[vector].callback; diff --git a/xen/include/asm-x86/hvm/viridian.h b/xen/include/asm-x86/hvm/viridian.h index 844e56b38f..d387d11ce0 100644 --- a/xen/include/asm-x86/hvm/viridian.h +++ b/xen/include/asm-x86/hvm/viridian.h @@ -89,7 +89,6 @@ void viridian_apic_assist_clear(const struct vcpu *v); void viridian_synic_poll(struct vcpu *v); bool viridian_synic_is_auto_eoi_sint(const struct vcpu *v, unsigned int vector); -void viridian_synic_ack_sint(const struct vcpu *v, unsigned int vector); #endif /* __ASM_X86_HVM_VIRIDIAN_H__ */
Switch synic interrupts to use an EOI callback in order to execute the logic tied to the end of interrupt. This allows to remove the synic call in vlapic_handle_EOI. Move and rename viridian_synic_ack_sint now that it can be made static. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- I'm unsure about the logic in viridian_synic_deliver_timer_msg, as it seems to only set the vector in msg_pending when the message is already pending? --- xen/arch/x86/hvm/viridian/synic.c | 28 +++++++++++++++------------- xen/arch/x86/hvm/vlapic.c | 4 ---- xen/include/asm-x86/hvm/viridian.h | 1 - 3 files changed, 15 insertions(+), 18 deletions(-)