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

["Durrant, Paul" <pdurrant@xxxxxxxxxxxx>]

> -----Original Message-----
> From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> Sent: 13 August 2020 11:16
> To: Paul Durrant <paul@xxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Durrant, Paul <pdurrant@xxxxxxxxxxxx>; 
> Wei Liu <wl@xxxxxxx>; Jan
> Beulich <jbeulich@xxxxxxxx>; Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Subject: RE: [EXTERNAL] [PATCH] x86 / viridian: remove the viridian_vcpu 
> msg_pending bit mask
> 
> CAUTION: This email originated from outside of the organization. Do not click 
> links or open
> attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> 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?

Yes, exactly. We'd potentially need to add code back in if we use the synic for 
other message types (e.g. implementing HvPostMessage, perhaps on top of the 
argo framework).

> 
> > 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>
> 

Thanks.

> 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?
> 

I don't think so. The test is supposed to be 'is the slot occupied'. In future 
we could introduce support for new message types and hence a check of != 
HVMSG_NONE seems like the right thing to do.

> > -    {
> > -        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.
> 

Oh yes, I'll send a v2.

  Paul

> Thanks, Roger.