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

[Paul Durrant <xadimgnik@xxxxxxxxx>]

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

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