RE: [PATCH 4/5] x86/viridian: switch synic to use the new EOI callback

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Roger Pau Monne <roger.pau@xxxxxxxxxx>
> Sent: 12 August 2020 13:47
> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Roger Pau Monne <roger.pau@xxxxxxxxxx>; Paul Durrant <paul@xxxxxxx>; Wei 
> Liu <wl@xxxxxxx>; Jan
> Beulich <jbeulich@xxxxxxxx>; Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> 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@xxxxxxxxxx>
> ---
> 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