Re: [PATCH] x86 / viridian: remove the viridian_vcpu msg_pending bit mask

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Aug 13, 2020 at 10:57:23AM +0100, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@xxxxxxxxxx>
> 
> The mask does not actually serve a useful purpose as we only use the SynIC
> for timer messages.

Oh, I see. I assume it doesn't make sense because there can only be a
single message pending (a timer one), and hence there isn't much value
in doing this SynIC pending tracking? 

> Dropping the mask means that the EOM MSR handler
> essentially becomes a no-op. This means we can avoid setting 'message_pending'
> for timer messages and hence avoid a VMEXIT for the EOM.
> 
> Signed-off-by: Paul Durrant <pdurrant@xxxxxxxxxx>

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

I've got some question below and one nit.

> ---
> Cc: Wei Liu <wl@xxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>
> 
> This should hopefully simplify Roger's "x86/vlapic: implement EOI callbacks"
> series a little.
> ---
>  xen/arch/x86/hvm/viridian/synic.c  | 24 +-----------------------
>  xen/arch/x86/hvm/vlapic.c          |  2 --
>  xen/include/asm-x86/hvm/viridian.h |  2 --
>  3 files changed, 1 insertion(+), 27 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/viridian/synic.c 
> b/xen/arch/x86/hvm/viridian/synic.c
> index 94a2b88733..22e2df27e5 100644
> --- a/xen/arch/x86/hvm/viridian/synic.c
> +++ b/xen/arch/x86/hvm/viridian/synic.c
> @@ -137,7 +137,6 @@ int viridian_synic_wrmsr(struct vcpu *v, uint32_t idx, 
> uint64_t val)
>          if ( !(viridian_feature_mask(d) & HVMPV_synic) )
>              return X86EMUL_EXCEPTION;
>  
> -        vv->msg_pending = 0;
>          break;
>  
>      case HV_X64_MSR_SINT0 ... HV_X64_MSR_SINT15:
> @@ -168,9 +167,6 @@ int viridian_synic_wrmsr(struct vcpu *v, uint32_t idx, 
> uint64_t val)
>          printk(XENLOG_G_INFO "%pv: VIRIDIAN SINT%u: vector: %x\n", v, sintx,
>                 vector);
>  
> -        if ( new.polling )
> -            __clear_bit(sintx, &vv->msg_pending);
> -
>          *vs = new;
>          break;
>      }
> @@ -334,9 +330,6 @@ bool viridian_synic_deliver_timer_msg(struct vcpu *v, 
> unsigned int sintx,
>          .DeliveryTime = delivery,
>      };
>  
> -    if ( test_bit(sintx, &vv->msg_pending) )
> -        return false;
> -
>      /*
>       * To avoid using an atomic test-and-set, and barrier before calling
>       * vlapic_set_irq(), this function must be called in context of the
> @@ -346,12 +339,9 @@ bool viridian_synic_deliver_timer_msg(struct vcpu *v, 
> unsigned int sintx,
>  
>      msg += sintx;
>  
> +    /* There is no need to set message_pending as we do not require an EOM */
>      if ( msg->header.message_type != HVMSG_NONE )

I think it's fine to use HVMSG_NONE ATM because Xen only knows about
timer messages, but long term wouldn't it be better to use
HVMSG_TIMER_EXPIRED?

> -    {
> -        msg->header.message_flags.msg_pending = 1;
> -        __set_bit(sintx, &vv->msg_pending);
>          return false;
> -    }
>  
>      msg->header.message_type = HVMSG_TIMER_EXPIRED;
>      msg->header.message_flags.msg_pending = 0;
> @@ -380,18 +370,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 7b5c633033..1aff4cf989 100644
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -466,8 +466,6 @@ void vlapic_handle_EOI(struct vlapic *vlapic, u8 vector)
>  
>      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);

Please also clean the comment above about SynIC SINTx being edge
triggered.

Thanks, Roger.