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

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

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.

Ack, I think the current approach in this patch would keep the same
behavior.

Thanks, Roger.