diff mbox series

x86 / viridian: remove the viridian_vcpu msg_pending bit mask

Message ID 20200813095723.1429-1-paul@xen.org (mailing list archive)
State Superseded
Headers show
Series x86 / viridian: remove the viridian_vcpu msg_pending bit mask | expand

Commit Message

Paul Durrant Aug. 13, 2020, 9:57 a.m. UTC 
From: Paul Durrant <pdurrant@amazon.com>

The mask does not actually serve a useful purpose as we only use the SynIC
for timer messages. 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@amazon.com>
---
Cc: Wei Liu <wl@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>

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

Comments

Roger Pau Monné Aug. 13, 2020, 10:15 a.m. UTC | #1 
On Thu, Aug 13, 2020 at 10:57:23AM +0100, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
> 
> 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@amazon.com>

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

I've got some question below and one nit.

> ---
> Cc: Wei Liu <wl@xen.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: "Roger Pau Monné" <roger.pau@citrix.com>
> 
> 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.
Durrant, Paul Aug. 13, 2020, 10:23 a.m. UTC | #2 
> -----Original Message-----
> From: Roger Pau Monné <roger.pau@citrix.com>
> Sent: 13 August 2020 11:16
> To: Paul Durrant <paul@xen.org>
> Cc: xen-devel@lists.xenproject.org; Durrant, Paul <pdurrant@amazon.co.uk>; Wei Liu <wl@xen.org>; Jan
> Beulich <jbeulich@suse.com>; Andrew Cooper <andrew.cooper3@citrix.com>
> 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@amazon.com>
> >
> > 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@amazon.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> 

Thanks.

> I've got some question below and one nit.
> 
> > ---
> > Cc: Wei Liu <wl@xen.org>
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > Cc: "Roger Pau Monné" <roger.pau@citrix.com>
> >
> > 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.
diff mbox series

Patch

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 )
-    {
-        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);
 
     hvm_dpci_msi_eoi(d, vector);
 }
diff --git a/xen/include/asm-x86/hvm/viridian.h b/xen/include/asm-x86/hvm/viridian.h
index 844e56b38f..cbf77d9c76 100644
--- a/xen/include/asm-x86/hvm/viridian.h
+++ b/xen/include/asm-x86/hvm/viridian.h
@@ -31,7 +31,6 @@  struct viridian_vcpu
     struct viridian_page vp_assist;
     bool apic_assist_pending;
     bool polled;
-    unsigned int msg_pending;
     uint64_t scontrol;
     uint64_t siefp;
     struct viridian_page simp;
@@ -89,7 +88,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__ */