[PATCH 0/5] x86/vlapic: implement EOI callbacks

[PATCH 0/5] x86/vlapic: implement EOI callbacks

[Roger Pau Monne]

Hello,

The following series attempts to implement EOI callbacks for vlapic
injected vectors, this allows to remove the hardcoded callbacks in
vlapic_handle_EOI. Instead a new vlapic vector injection helper is
provided, that takes two extra parameters in order to pass a callback and
an opaque data blob that will be called once the vector is EOI'ed by the
guest.

My plan with this is to be able to provide EOI callbacks for all
interrupt types that we inject to the guest, this is only the first step
of that work by changing the emulated lapic to use this model. The
IO-APIC and PIC still need to be switched to a similar model.

Long term having such callbacks would greatly simplify the virtual
periodic timers code, as being able to get a callback when the injected
interrupt is EOI'ed simplifies the handling of missed ticks, as we would
no longer need to check at each vmentry whether a virtual interrupt
timer is being injected.

Thanks, Roger.

Roger Pau Monne (5):
  x86/hvm: change EOI exit bitmap helper parameter
  x86/vlapic: introduce an EOI callback mechanism
  x86/vmsi: use the newly introduced EOI callbacks
  x86/viridian: switch synic to use the new EOI callback
  x86/vioapic: switch to use the EOI callback mechanism

 xen/arch/x86/hvm/vioapic.c         | 90 +++++++++++++++---------------
 xen/arch/x86/hvm/viridian/synic.c  | 28 +++++-----
 xen/arch/x86/hvm/vlapic.c          | 61 ++++++++++++++++----
 xen/arch/x86/hvm/vmsi.c            | 36 +++++++-----
 xen/arch/x86/hvm/vmx/vmx.c         |  4 +-
 xen/drivers/passthrough/io.c       |  4 +-
 xen/include/asm-x86/hvm/hvm.h      |  2 +-
 xen/include/asm-x86/hvm/io.h       |  2 +-
 xen/include/asm-x86/hvm/viridian.h |  1 -
 xen/include/asm-x86/hvm/vlapic.h   | 10 ++++
 10 files changed, 150 insertions(+), 88 deletions(-)

-- 
2.28.0

[PATCH 1/5] x86/hvm: change EOI exit bitmap helper parameter

[Roger Pau Monne]

Change the last parameter of the update_eoi_exit_bitmap helper to be a
set/clear boolean instead of a triggering field. This is already
inline with how the function is implemented, and will allow deciding
whether an exit is required by the higher layers that call into
update_eoi_exit_bitmap. Note that the current behavior is not changed
by this patch.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/hvm/vmx/vmx.c    | 4 ++--
 xen/include/asm-x86/hvm/hvm.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index eb54aadfba..1c04a7e3fc 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1885,9 +1885,9 @@ static void vmx_set_info_guest(struct vcpu *v)
     vmx_vmcs_exit(v);
 }
 
-static void vmx_update_eoi_exit_bitmap(struct vcpu *v, u8 vector, u8 trig)
+static void vmx_update_eoi_exit_bitmap(struct vcpu *v, uint8_t vector, bool set)
 {
-    if ( trig )
+    if ( set )
         vmx_set_eoi_exit_bitmap(v, vector);
     else
         vmx_clear_eoi_exit_bitmap(v, vector);
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index 1eb377dd82..be0d8b0a4d 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -192,7 +192,7 @@ struct hvm_function_table {
     void (*nhvm_domain_relinquish_resources)(struct domain *d);
 
     /* Virtual interrupt delivery */
-    void (*update_eoi_exit_bitmap)(struct vcpu *v, u8 vector, u8 trig);
+    void (*update_eoi_exit_bitmap)(struct vcpu *v, uint8_t vector, bool set);
     void (*process_isr)(int isr, struct vcpu *v);
     void (*deliver_posted_intr)(struct vcpu *v, u8 vector);
     void (*sync_pir_to_irr)(struct vcpu *v);
-- 
2.28.0

Re: [PATCH 1/5] x86/hvm: change EOI exit bitmap helper parameter

[Andrew Cooper]

On 12/08/2020 13:47, Roger Pau Monne wrote:
> Change the last parameter of the update_eoi_exit_bitmap helper to be a
> set/clear boolean instead of a triggering field. This is already
> inline with how the function is implemented, and will allow deciding

"in line"

> whether an exit is required by the higher layers that call into
> update_eoi_exit_bitmap. Note that the current behavior is not changed
> by this patch.
>
> No functional change intended.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

RE: [PATCH 1/5] x86/hvm: change EOI exit bitmap helper parameter

[Tian, Kevin]

> From: Roger Pau Monne <roger.pau@citrix.com>
> Sent: Wednesday, August 12, 2020 8:47 PM
arameter
> 
> Change the last parameter of the update_eoi_exit_bitmap helper to be a
> set/clear boolean instead of a triggering field. This is already
> inline with how the function is implemented, and will allow deciding
> whether an exit is required by the higher layers that call into
> update_eoi_exit_bitmap. Note that the current behavior is not changed
> by this patch.
> 
> No functional change intended.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

> ---
>  xen/arch/x86/hvm/vmx/vmx.c    | 4 ++--
>  xen/include/asm-x86/hvm/hvm.h | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index eb54aadfba..1c04a7e3fc 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -1885,9 +1885,9 @@ static void vmx_set_info_guest(struct vcpu *v)
>      vmx_vmcs_exit(v);
>  }
> 
> -static void vmx_update_eoi_exit_bitmap(struct vcpu *v, u8 vector, u8 trig)
> +static void vmx_update_eoi_exit_bitmap(struct vcpu *v, uint8_t vector, bool
> set)
>  {
> -    if ( trig )
> +    if ( set )
>          vmx_set_eoi_exit_bitmap(v, vector);
>      else
>          vmx_clear_eoi_exit_bitmap(v, vector);
> diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-
> x86/hvm/hvm.h
> index 1eb377dd82..be0d8b0a4d 100644
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -192,7 +192,7 @@ struct hvm_function_table {
>      void (*nhvm_domain_relinquish_resources)(struct domain *d);
> 
>      /* Virtual interrupt delivery */
> -    void (*update_eoi_exit_bitmap)(struct vcpu *v, u8 vector, u8 trig);
> +    void (*update_eoi_exit_bitmap)(struct vcpu *v, uint8_t vector, bool set);
>      void (*process_isr)(int isr, struct vcpu *v);
>      void (*deliver_posted_intr)(struct vcpu *v, u8 vector);
>      void (*sync_pir_to_irr)(struct vcpu *v);
> --
> 2.28.0

[PATCH 2/5] x86/vlapic: introduce an EOI callback mechanism

[Roger Pau Monne]

Add a new vlapic_set_irq_callback helper in order to inject a vector
and set a callback to be executed when the guest performs the end of
interrupt acknowledgment.

Such functionality will be used to migrate the current ad hoc handling
done in vlapic_handle_EOI for the vectors that require some logic to
be executed when the end of interrupt is performed.

No current users are migrated to use this new functionality yet, so
not functional change expected as a result.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/hvm/vlapic.c        | 54 ++++++++++++++++++++++++++++++--
 xen/include/asm-x86/hvm/vlapic.h | 10 ++++++
 2 files changed, 62 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 7b5c633033..7369be468b 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -144,7 +144,8 @@ bool vlapic_test_irq(const struct vlapic *vlapic, uint8_t vec)
     return vlapic_test_vector(vec, &vlapic->regs->data[APIC_IRR]);
 }
 
-void vlapic_set_irq(struct vlapic *vlapic, uint8_t vec, uint8_t trig)
+void vlapic_set_irq_callback(struct vlapic *vlapic, uint8_t vec, uint8_t trig,
+                             vlapic_eoi_callback_t *callback, void *data)
 {
     struct vcpu *target = vlapic_vcpu(vlapic);
 
@@ -159,8 +160,26 @@ void vlapic_set_irq(struct vlapic *vlapic, uint8_t vec, uint8_t trig)
     else
         vlapic_clear_vector(vec, &vlapic->regs->data[APIC_TMR]);
 
+    if ( callback )
+    {
+        unsigned long flags;
+
+        spin_lock_irqsave(&vlapic->callback_lock, flags);
+        vlapic->callbacks[vec].callback = callback;
+        vlapic->callbacks[vec].data = data;
+        spin_unlock_irqrestore(&vlapic->callback_lock, flags);
+    }
+    else
+        /*
+         * Removing the callback can be done with a single atomic operation
+         * without requiring the lock, as the callback data doesn't need to be
+         * cleared.
+         */
+        write_atomic(&vlapic->callbacks[vec].callback, NULL);
+
     if ( hvm_funcs.update_eoi_exit_bitmap )
-        alternative_vcall(hvm_funcs.update_eoi_exit_bitmap, target, vec, trig);
+        alternative_vcall(hvm_funcs.update_eoi_exit_bitmap, target, vec,
+                          trig || callback);
 
     if ( hvm_funcs.deliver_posted_intr )
         alternative_vcall(hvm_funcs.deliver_posted_intr, target, vec);
@@ -168,6 +187,11 @@ void vlapic_set_irq(struct vlapic *vlapic, uint8_t vec, uint8_t trig)
         vcpu_kick(target);
 }
 
+void vlapic_set_irq(struct vlapic *vlapic, uint8_t vec, uint8_t trig)
+{
+    vlapic_set_irq_callback(vlapic, vec, trig, NULL, NULL);
+}
+
 static int vlapic_find_highest_isr(const struct vlapic *vlapic)
 {
     return vlapic_find_highest_vector(&vlapic->regs->data[APIC_ISR]);
@@ -461,6 +485,9 @@ void vlapic_handle_EOI(struct vlapic *vlapic, u8 vector)
 {
     struct vcpu *v = vlapic_vcpu(vlapic);
     struct domain *d = v->domain;
+    vlapic_eoi_callback_t *callback;
+    void *data;
+    unsigned long flags;
 
     /* All synic SINTx vectors are edge triggered */
 
@@ -470,6 +497,14 @@ void vlapic_handle_EOI(struct vlapic *vlapic, u8 vector)
         viridian_synic_ack_sint(v, vector);
 
     hvm_dpci_msi_eoi(d, vector);
+
+    spin_lock_irqsave(&vlapic->callback_lock, flags);
+    callback = vlapic->callbacks[vector].callback;
+    data = vlapic->callbacks[vector].data;
+    spin_unlock_irqrestore(&vlapic->callback_lock, flags);
+
+    if ( callback )
+        callback(v, vector, data);
 }
 
 static bool_t is_multicast_dest(struct vlapic *vlapic, unsigned int short_hand,
@@ -1636,9 +1671,23 @@ int vlapic_init(struct vcpu *v)
     }
     clear_page(vlapic->regs);
 
+    if ( !vlapic->callbacks )
+    {
+        vlapic->callbacks = xmalloc_array(typeof(*(vlapic->callbacks)),
+                                          X86_NR_VECTORS);
+        if ( !vlapic->callbacks )
+        {
+            dprintk(XENLOG_ERR, "alloc vlapic callbacks error: %d/%d\n",
+                    v->domain->domain_id, v->vcpu_id);
+            return -ENOMEM;
+        }
+    }
+    memset(vlapic->callbacks, 0, sizeof(*(vlapic->callbacks)) * X86_NR_VECTORS);
+
     vlapic_reset(vlapic);
 
     spin_lock_init(&vlapic->esr_lock);
+    spin_lock_init(&vlapic->callback_lock);
 
     tasklet_init(&vlapic->init_sipi.tasklet, vlapic_init_sipi_action, v);
 
@@ -1660,6 +1709,7 @@ void vlapic_destroy(struct vcpu *v)
     destroy_periodic_time(&vlapic->pt);
     unmap_domain_page_global(vlapic->regs);
     free_domheap_page(vlapic->regs_page);
+    XFREE(vlapic->callbacks);
 }
 
 /*
diff --git a/xen/include/asm-x86/hvm/vlapic.h b/xen/include/asm-x86/hvm/vlapic.h
index 8f908928c3..6782508a68 100644
--- a/xen/include/asm-x86/hvm/vlapic.h
+++ b/xen/include/asm-x86/hvm/vlapic.h
@@ -73,6 +73,9 @@
 #define vlapic_clear_vector(vec, bitmap)                                \
     clear_bit(VEC_POS(vec), (uint32_t *)((bitmap) + REG_POS(vec)))
 
+typedef void vlapic_eoi_callback_t(struct vcpu *v, unsigned int vector,
+                                   void *data);
+
 struct vlapic {
     struct hvm_hw_lapic      hw;
     struct hvm_hw_lapic_regs *regs;
@@ -89,6 +92,11 @@ struct vlapic {
         uint32_t             icr, dest;
         struct tasklet       tasklet;
     } init_sipi;
+    struct {
+        vlapic_eoi_callback_t *callback;
+        void                 *data;
+    } *callbacks;
+    spinlock_t               callback_lock;
 };
 
 /* vlapic's frequence is 100 MHz */
@@ -111,6 +119,8 @@ void vlapic_reg_write(struct vcpu *v, unsigned int reg, uint32_t val);
 bool_t is_vlapic_lvtpc_enabled(struct vlapic *vlapic);
 
 bool vlapic_test_irq(const struct vlapic *vlapic, uint8_t vec);
+void vlapic_set_irq_callback(struct vlapic *vlapic, uint8_t vec, uint8_t trig,
+                             vlapic_eoi_callback_t *callback, void *data);
 void vlapic_set_irq(struct vlapic *vlapic, uint8_t vec, uint8_t trig);
 
 int vlapic_has_pending_irq(struct vcpu *v);
-- 
2.28.0

Re: [PATCH 2/5] x86/vlapic: introduce an EOI callback mechanism

[Andrew Cooper]

On 12/08/2020 13:47, Roger Pau Monne wrote:
> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> index 7b5c633033..7369be468b 100644
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -159,8 +160,26 @@ void vlapic_set_irq(struct vlapic *vlapic, uint8_t vec, uint8_t trig)
>      else
>          vlapic_clear_vector(vec, &vlapic->regs->data[APIC_TMR]);
>  
> +    if ( callback )
> +    {
> +        unsigned long flags;
> +
> +        spin_lock_irqsave(&vlapic->callback_lock, flags);
> +        vlapic->callbacks[vec].callback = callback;
> +        vlapic->callbacks[vec].data = data;
> +        spin_unlock_irqrestore(&vlapic->callback_lock, flags);
> +    }
> +    else
> +        /*
> +         * Removing the callback can be done with a single atomic operation
> +         * without requiring the lock, as the callback data doesn't need to be
> +         * cleared.
> +         */
> +        write_atomic(&vlapic->callbacks[vec].callback, NULL);
> +
>      if ( hvm_funcs.update_eoi_exit_bitmap )
> -        alternative_vcall(hvm_funcs.update_eoi_exit_bitmap, target, vec, trig);
> +        alternative_vcall(hvm_funcs.update_eoi_exit_bitmap, target, vec,
> +                          trig || callback);

I can't reason about this being correct.

When you say EOI, what property do you want, exactly, because there are
a couple of options.

All interrupts, edge or level, require acking at the local APIC, to mark
the interrupt as no longer in service.  For edge interrupts and hardware
APIC acceleration, this will be completed without a VMExit.

Level interrupts from the IO-APIC further require EOI'ing there. 
Whether this requires an explicit action or not depends on the LAPIC
"EOI Broadcast" setting.  I'm not sure if we virtualise and/or support
this setting.


What exactly are we going to want to do from these callbacks
(eventually, not just in this series alone)?

If it can't be made to work for level interrupts only, I'm afraid I
don't think this plan is viable.

(Some trivial comments to follow, but this is the meaty question.)

>  
>      if ( hvm_funcs.deliver_posted_intr )
>          alternative_vcall(hvm_funcs.deliver_posted_intr, target, vec);
> @@ -168,6 +187,11 @@ void vlapic_set_irq(struct vlapic *vlapic, uint8_t vec, uint8_t trig)
>          vcpu_kick(target);
>  }
>  
> +void vlapic_set_irq(struct vlapic *vlapic, uint8_t vec, uint8_t trig)
> +{
> +    vlapic_set_irq_callback(vlapic, vec, trig, NULL, NULL);

Static inline in the header file?

> @@ -1636,9 +1671,23 @@ int vlapic_init(struct vcpu *v)
>      }
>      clear_page(vlapic->regs);
>  
> +    if ( !vlapic->callbacks )
> +    {
> +        vlapic->callbacks = xmalloc_array(typeof(*(vlapic->callbacks)),
> +                                          X86_NR_VECTORS);

This is a large data structure.  At a minimum, you can subtract 16 from
it, because vectors 0 thru 0xf can't legally be targetted by interrupts.

Sadly, I don't think it can be reduced beyond that, because a guest
could arrange for every other vector to become pending in level mode,
even if the overwhelming common option for VMs these days would be to
have no level interrupts at all.

~Andrew

Re: [PATCH 2/5] x86/vlapic: introduce an EOI callback mechanism

[Roger Pau Monné]

On Thu, Aug 13, 2020 at 03:33:37PM +0100, Andrew Cooper wrote:
> On 12/08/2020 13:47, Roger Pau Monne wrote:
> > diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> > index 7b5c633033..7369be468b 100644
> > --- a/xen/arch/x86/hvm/vlapic.c
> > +++ b/xen/arch/x86/hvm/vlapic.c
> > @@ -159,8 +160,26 @@ void vlapic_set_irq(struct vlapic *vlapic, uint8_t vec, uint8_t trig)
> >      else
> >          vlapic_clear_vector(vec, &vlapic->regs->data[APIC_TMR]);
> >  
> > +    if ( callback )
> > +    {
> > +        unsigned long flags;
> > +
> > +        spin_lock_irqsave(&vlapic->callback_lock, flags);
> > +        vlapic->callbacks[vec].callback = callback;
> > +        vlapic->callbacks[vec].data = data;
> > +        spin_unlock_irqrestore(&vlapic->callback_lock, flags);
> > +    }
> > +    else
> > +        /*
> > +         * Removing the callback can be done with a single atomic operation
> > +         * without requiring the lock, as the callback data doesn't need to be
> > +         * cleared.
> > +         */
> > +        write_atomic(&vlapic->callbacks[vec].callback, NULL);
> > +
> >      if ( hvm_funcs.update_eoi_exit_bitmap )
> > -        alternative_vcall(hvm_funcs.update_eoi_exit_bitmap, target, vec, trig);
> > +        alternative_vcall(hvm_funcs.update_eoi_exit_bitmap, target, vec,
> > +                          trig || callback);
> 
> I can't reason about this being correct.

Note the 'trig' part should have been dropped in patch 5, as then
the vIO-APIC will properly register the EOI callback for level
triggered interrupts.

Note that if we ever implement EOI suppression the vIO-APIC callback
would then have to check whether the bit is currently set in order to
decide whether to perform the EOI on the vIO-APIC or not.

This just requests a vmexit whenever the caller of vlapic_set_irq
requires an EOI callback, so that it can be executed when using the
virtual interrupt delivery feature that would normally avoid such
vmexit.

> When you say EOI, what property do you want, exactly, because there are
> a couple of options.

Here (in the update_eoi_exit_bitmap context) I want a vmexit whenever
an EIO callback for the injected vector needs to be executed, that's
requested by the caller of vlapic_set_irq. We have to keep the 'trig'
part because vIO-APIC is not switched to use the new callback
infrastructure in this patch.

> All interrupts, edge or level, require acking at the local APIC, to mark
> the interrupt as no longer in service.  For edge interrupts and hardware
> APIC acceleration, this will be completed without a VMExit.

It would be completed without a vmexit as long as the corresponding
vector bit in the EOI exit bitmap is not set when using virtual
interrupt delivery.

I think this is currently wrong, as we require a vmexit to happen for
non-maskable edge MSI interrupts, and the corresponding EOI exit
bitmap bit doesn't seem to be set?

Maybe there's something I'm missing.

> Level interrupts from the IO-APIC further require EOI'ing there. 
> Whether this requires an explicit action or not depends on the LAPIC
> "EOI Broadcast" setting.  I'm not sure if we virtualise and/or support
> this setting.

No, our current vlapic implementation doesn't support EOI broadcast
suppression AFAICT, as bit 24 in VLAPIC_VERSION is 0. So the EOI of a
level interrupts is always broadcasted to the IO-APIC(s).

> 
> What exactly are we going to want to do from these callbacks
> (eventually, not just in this series alone)?

So this series just moves the current hooks in vlapic_handle_EOI to
become dynamically set by the users that inject the vectors.

We also need EOI callbacks for edge triggered interrupts, as
non-maskable edge MSIs from passed-through devices currently require
an EOI on the physical lapic when the guest also performs such EOI,
see hvm_dpci_msi_eoi.

> If it can't be made to work for level interrupts only, I'm afraid I
> don't think this plan is viable.

I think it can be made to work, the code here will keep the callback
for level triggered interrupts, so that the EOI is forwarded to the
vIO-APIC, but I don't see why this would be limited to level
interrupts only, we could need the same for edge interrupts, as it
seems like the SynIC viridian extensions could also make use of this
if we ever implement them fully.

> (Some trivial comments to follow, but this is the meaty question.)
> 
> >  
> >      if ( hvm_funcs.deliver_posted_intr )
> >          alternative_vcall(hvm_funcs.deliver_posted_intr, target, vec);
> > @@ -168,6 +187,11 @@ void vlapic_set_irq(struct vlapic *vlapic, uint8_t vec, uint8_t trig)
> >          vcpu_kick(target);
> >  }
> >  
> > +void vlapic_set_irq(struct vlapic *vlapic, uint8_t vec, uint8_t trig)
> > +{
> > +    vlapic_set_irq_callback(vlapic, vec, trig, NULL, NULL);
> 
> Static inline in the header file?

Sure.

> > @@ -1636,9 +1671,23 @@ int vlapic_init(struct vcpu *v)
> >      }
> >      clear_page(vlapic->regs);
> >  
> > +    if ( !vlapic->callbacks )
> > +    {
> > +        vlapic->callbacks = xmalloc_array(typeof(*(vlapic->callbacks)),
> > +                                          X86_NR_VECTORS);
> 
> This is a large data structure.  At a minimum, you can subtract 16 from
> it, because vectors 0 thru 0xf can't legally be targetted by interrupts.
> 
> Sadly, I don't think it can be reduced beyond that, because a guest
> could arrange for every other vector to become pending in level mode,
> even if the overwhelming common option for VMs these days would be to
> have no level interrupts at all.

I'm still not sure I understand why you mention level triggered
interrupts specifically here, the lapic EOI is performed for both
level and edge trigger interrupts in order to clear the bit in ISR,
and hence we could have an EOI callback regardless of the triggering
mode?

It's just a matter of the caller requesting an EOI callback, and then
it will be executed when the guest performs the EOI, regardless of
whether the interrupt is level or edge triggered.

Thanks, Roger.

Re: [PATCH 2/5] x86/vlapic: introduce an EOI callback mechanism

[Jan Beulich]

On 12.08.2020 14:47, Roger Pau Monne wrote:
> Add a new vlapic_set_irq_callback helper in order to inject a vector
> and set a callback to be executed when the guest performs the end of
> interrupt acknowledgment.

One thing I don't understand at all for now is how these
callbacks are going to be re-instated after migration for
not-yet-EOIed interrupts.

> @@ -1636,9 +1671,23 @@ int vlapic_init(struct vcpu *v)
>      }
>      clear_page(vlapic->regs);
>  
> +    if ( !vlapic->callbacks )
> +    {
> +        vlapic->callbacks = xmalloc_array(typeof(*(vlapic->callbacks)),

There's a pair of not really needed parentheses here (also
again a little further down).

> +                                          X86_NR_VECTORS);
> +        if ( !vlapic->callbacks )
> +        {
> +            dprintk(XENLOG_ERR, "alloc vlapic callbacks error: %d/%d\n",
> +                    v->domain->domain_id, v->vcpu_id);

Please use %pd.

Jan

[PATCH 3/5] x86/vmsi: use the newly introduced EOI callbacks

[Roger Pau Monne]

Remove the unconditional call to hvm_dpci_msi_eoi in vlapic_handle_EOI
and instead use the newly introduced EOI callback mechanism in order
to register a callback for MSI vectors injected from passed through
devices.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/hvm/vlapic.c    |  2 --
 xen/arch/x86/hvm/vmsi.c      | 36 ++++++++++++++++++++++--------------
 xen/drivers/passthrough/io.c |  4 +++-
 xen/include/asm-x86/hvm/io.h |  2 +-
 4 files changed, 26 insertions(+), 18 deletions(-)

diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 7369be468b..3b3b3d7621 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -496,8 +496,6 @@ void vlapic_handle_EOI(struct vlapic *vlapic, u8 vector)
     else if ( has_viridian_synic(d) )
         viridian_synic_ack_sint(v, vector);
 
-    hvm_dpci_msi_eoi(d, vector);
-
     spin_lock_irqsave(&vlapic->callback_lock, flags);
     callback = vlapic->callbacks[vector].callback;
     data = vlapic->callbacks[vector].data;
diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
index 7ca19353ab..e192c4c6da 100644
--- a/xen/arch/x86/hvm/vmsi.c
+++ b/xen/arch/x86/hvm/vmsi.c
@@ -44,11 +44,9 @@
 #include <asm/event.h>
 #include <asm/io_apic.h>
 
-static void vmsi_inj_irq(
-    struct vlapic *target,
-    uint8_t vector,
-    uint8_t trig_mode,
-    uint8_t delivery_mode)
+static void vmsi_inj_irq(struct vlapic *target, uint8_t vector,
+                         uint8_t trig_mode, uint8_t delivery_mode,
+                         vlapic_eoi_callback_t *callback, void *data)
 {
     HVM_DBG_LOG(DBG_LEVEL_VLAPIC, "vmsi_inj_irq: vec %02x trig %d dm %d\n",
                 vector, trig_mode, delivery_mode);
@@ -57,17 +55,17 @@ static void vmsi_inj_irq(
     {
     case dest_Fixed:
     case dest_LowestPrio:
-        vlapic_set_irq(target, vector, trig_mode);
+        vlapic_set_irq_callback(target, vector, trig_mode, callback, data);
         break;
     default:
         BUG();
     }
 }
 
-int vmsi_deliver(
-    struct domain *d, int vector,
-    uint8_t dest, uint8_t dest_mode,
-    uint8_t delivery_mode, uint8_t trig_mode)
+static int vmsi_deliver_callback(struct domain *d, int vector, uint8_t dest,
+                                 uint8_t dest_mode, uint8_t delivery_mode,
+                                 uint8_t trig_mode,
+                                 vlapic_eoi_callback_t *callback, void *data)
 {
     struct vlapic *target;
     struct vcpu *v;
@@ -78,7 +76,8 @@ int vmsi_deliver(
         target = vlapic_lowest_prio(d, NULL, 0, dest, dest_mode);
         if ( target != NULL )
         {
-            vmsi_inj_irq(target, vector, trig_mode, delivery_mode);
+            vmsi_inj_irq(target, vector, trig_mode, delivery_mode, callback,
+                         data);
             break;
         }
         HVM_DBG_LOG(DBG_LEVEL_VLAPIC, "null MSI round robin: vector=%02x\n",
@@ -89,8 +88,8 @@ int vmsi_deliver(
         for_each_vcpu ( d, v )
             if ( vlapic_match_dest(vcpu_vlapic(v), NULL,
                                    0, dest, dest_mode) )
-                vmsi_inj_irq(vcpu_vlapic(v), vector,
-                             trig_mode, delivery_mode);
+                vmsi_inj_irq(vcpu_vlapic(v), vector, trig_mode, delivery_mode,
+                             callback, data);
         break;
 
     default:
@@ -103,6 +102,14 @@ int vmsi_deliver(
     return 0;
 }
 
+
+int vmsi_deliver(struct domain *d, int vector, uint8_t dest, uint8_t dest_mode,
+                 uint8_t delivery_mode, uint8_t trig_mode)
+{
+    return vmsi_deliver_callback(d, vector, dest, dest_mode, delivery_mode,
+                                 trig_mode, NULL, NULL);
+}
+
 void vmsi_deliver_pirq(struct domain *d, const struct hvm_pirq_dpci *pirq_dpci)
 {
     uint32_t flags = pirq_dpci->gmsi.gflags;
@@ -119,7 +126,8 @@ void vmsi_deliver_pirq(struct domain *d, const struct hvm_pirq_dpci *pirq_dpci)
 
     ASSERT(pirq_dpci->flags & HVM_IRQ_DPCI_GUEST_MSI);
 
-    vmsi_deliver(d, vector, dest, dest_mode, delivery_mode, trig_mode);
+    vmsi_deliver_callback(d, vector, dest, dest_mode, delivery_mode, trig_mode,
+                          hvm_dpci_msi_eoi, NULL);
 }
 
 /* Return value, -1 : multi-dests, non-negative value: dest_vcpu_id */
diff --git a/xen/drivers/passthrough/io.c b/xen/drivers/passthrough/io.c
index 6b1305a3e5..3793029b29 100644
--- a/xen/drivers/passthrough/io.c
+++ b/xen/drivers/passthrough/io.c
@@ -874,8 +874,10 @@ static int _hvm_dpci_msi_eoi(struct domain *d,
     return 0;
 }
 
-void hvm_dpci_msi_eoi(struct domain *d, int vector)
+void hvm_dpci_msi_eoi(struct vcpu *v, unsigned int vector, void *data)
 {
+    struct domain *d = v->domain;
+
     if ( !is_iommu_enabled(d) ||
          (!hvm_domain_irq(d)->dpci && !is_hardware_domain(d)) )
        return;
diff --git a/xen/include/asm-x86/hvm/io.h b/xen/include/asm-x86/hvm/io.h
index 558426b772..450c7c8acb 100644
--- a/xen/include/asm-x86/hvm/io.h
+++ b/xen/include/asm-x86/hvm/io.h
@@ -159,7 +159,7 @@ struct hvm_hw_stdvga {
 void stdvga_init(struct domain *d);
 void stdvga_deinit(struct domain *d);
 
-extern void hvm_dpci_msi_eoi(struct domain *d, int vector);
+void hvm_dpci_msi_eoi(struct vcpu *v, unsigned int vector, void *data);
 
 /* Decode a PCI port IO access into a bus/slot/func/reg. */
 unsigned int hvm_pci_decode_addr(unsigned int cf8, unsigned int addr,
-- 
2.28.0

RE: [PATCH 3/5] x86/vmsi: use the newly introduced EOI callbacks

[Paul Durrant]

> -----Original Message-----
> From: Roger Pau Monne <roger.pau@citrix.com>
> Sent: 12 August 2020 13:47
> To: xen-devel@lists.xenproject.org
> Cc: Roger Pau Monne <roger.pau@citrix.com>; Jan Beulich <jbeulich@suse.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; Paul Durrant <paul@xen.org>
> Subject: [PATCH 3/5] x86/vmsi: use the newly introduced EOI callbacks
> 
> Remove the unconditional call to hvm_dpci_msi_eoi in vlapic_handle_EOI
> and instead use the newly introduced EOI callback mechanism in order
> to register a callback for MSI vectors injected from passed through
> devices.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/arch/x86/hvm/vlapic.c    |  2 --
>  xen/arch/x86/hvm/vmsi.c      | 36 ++++++++++++++++++++++--------------
>  xen/drivers/passthrough/io.c |  4 +++-
>  xen/include/asm-x86/hvm/io.h |  2 +-
>  4 files changed, 26 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> index 7369be468b..3b3b3d7621 100644
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -496,8 +496,6 @@ void vlapic_handle_EOI(struct vlapic *vlapic, u8 vector)
>      else if ( has_viridian_synic(d) )
>          viridian_synic_ack_sint(v, vector);
> 
> -    hvm_dpci_msi_eoi(d, vector);
> -
>      spin_lock_irqsave(&vlapic->callback_lock, flags);
>      callback = vlapic->callbacks[vector].callback;
>      data = vlapic->callbacks[vector].data;
> diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
> index 7ca19353ab..e192c4c6da 100644
> --- a/xen/arch/x86/hvm/vmsi.c
> +++ b/xen/arch/x86/hvm/vmsi.c
> @@ -44,11 +44,9 @@
>  #include <asm/event.h>
>  #include <asm/io_apic.h>
> 
> -static void vmsi_inj_irq(
> -    struct vlapic *target,
> -    uint8_t vector,
> -    uint8_t trig_mode,
> -    uint8_t delivery_mode)
> +static void vmsi_inj_irq(struct vlapic *target, uint8_t vector,
> +                         uint8_t trig_mode, uint8_t delivery_mode,
> +                         vlapic_eoi_callback_t *callback, void *data)
>  {
>      HVM_DBG_LOG(DBG_LEVEL_VLAPIC, "vmsi_inj_irq: vec %02x trig %d dm %d\n",
>                  vector, trig_mode, delivery_mode);
> @@ -57,17 +55,17 @@ static void vmsi_inj_irq(
>      {
>      case dest_Fixed:
>      case dest_LowestPrio:
> -        vlapic_set_irq(target, vector, trig_mode);
> +        vlapic_set_irq_callback(target, vector, trig_mode, callback, data);
>          break;
>      default:
>          BUG();
>      }
>  }
> 
> -int vmsi_deliver(
> -    struct domain *d, int vector,
> -    uint8_t dest, uint8_t dest_mode,
> -    uint8_t delivery_mode, uint8_t trig_mode)
> +static int vmsi_deliver_callback(struct domain *d, int vector, uint8_t dest,
> +                                 uint8_t dest_mode, uint8_t delivery_mode,
> +                                 uint8_t trig_mode,
> +                                 vlapic_eoi_callback_t *callback, void *data)
>  {
>      struct vlapic *target;
>      struct vcpu *v;
> @@ -78,7 +76,8 @@ int vmsi_deliver(
>          target = vlapic_lowest_prio(d, NULL, 0, dest, dest_mode);
>          if ( target != NULL )
>          {
> -            vmsi_inj_irq(target, vector, trig_mode, delivery_mode);
> +            vmsi_inj_irq(target, vector, trig_mode, delivery_mode, callback,
> +                         data);
>              break;
>          }
>          HVM_DBG_LOG(DBG_LEVEL_VLAPIC, "null MSI round robin: vector=%02x\n",
> @@ -89,8 +88,8 @@ int vmsi_deliver(
>          for_each_vcpu ( d, v )
>              if ( vlapic_match_dest(vcpu_vlapic(v), NULL,
>                                     0, dest, dest_mode) )
> -                vmsi_inj_irq(vcpu_vlapic(v), vector,
> -                             trig_mode, delivery_mode);
> +                vmsi_inj_irq(vcpu_vlapic(v), vector, trig_mode, delivery_mode,
> +                             callback, data);
>          break;
> 
>      default:
> @@ -103,6 +102,14 @@ int vmsi_deliver(
>      return 0;
>  }
> 
> +
> +int vmsi_deliver(struct domain *d, int vector, uint8_t dest, uint8_t dest_mode,
> +                 uint8_t delivery_mode, uint8_t trig_mode)
> +{
> +    return vmsi_deliver_callback(d, vector, dest, dest_mode, delivery_mode,
> +                                 trig_mode, NULL, NULL);
> +}
> +
>  void vmsi_deliver_pirq(struct domain *d, const struct hvm_pirq_dpci *pirq_dpci)
>  {
>      uint32_t flags = pirq_dpci->gmsi.gflags;
> @@ -119,7 +126,8 @@ void vmsi_deliver_pirq(struct domain *d, const struct hvm_pirq_dpci *pirq_dpci)
> 
>      ASSERT(pirq_dpci->flags & HVM_IRQ_DPCI_GUEST_MSI);
> 
> -    vmsi_deliver(d, vector, dest, dest_mode, delivery_mode, trig_mode);
> +    vmsi_deliver_callback(d, vector, dest, dest_mode, delivery_mode, trig_mode,
> +                          hvm_dpci_msi_eoi, NULL);
>  }
> 
>  /* Return value, -1 : multi-dests, non-negative value: dest_vcpu_id */
> diff --git a/xen/drivers/passthrough/io.c b/xen/drivers/passthrough/io.c
> index 6b1305a3e5..3793029b29 100644
> --- a/xen/drivers/passthrough/io.c
> +++ b/xen/drivers/passthrough/io.c
> @@ -874,8 +874,10 @@ static int _hvm_dpci_msi_eoi(struct domain *d,
>      return 0;
>  }
> 
> -void hvm_dpci_msi_eoi(struct domain *d, int vector)
> +void hvm_dpci_msi_eoi(struct vcpu *v, unsigned int vector, void *data)
>  {
> +    struct domain *d = v->domain;
> +

Could we actually drop the vcpu parameter here... i.e. is there any case where this code will be invoked with v != current?


>      if ( !is_iommu_enabled(d) ||
>           (!hvm_domain_irq(d)->dpci && !is_hardware_domain(d)) )
>         return;
> diff --git a/xen/include/asm-x86/hvm/io.h b/xen/include/asm-x86/hvm/io.h
> index 558426b772..450c7c8acb 100644
> --- a/xen/include/asm-x86/hvm/io.h
> +++ b/xen/include/asm-x86/hvm/io.h
> @@ -159,7 +159,7 @@ struct hvm_hw_stdvga {
>  void stdvga_init(struct domain *d);
>  void stdvga_deinit(struct domain *d);
> 
> -extern void hvm_dpci_msi_eoi(struct domain *d, int vector);
> +void hvm_dpci_msi_eoi(struct vcpu *v, unsigned int vector, void *data);
> 
>  /* Decode a PCI port IO access into a bus/slot/func/reg. */
>  unsigned int hvm_pci_decode_addr(unsigned int cf8, unsigned int addr,
> --
> 2.28.0

Re: [PATCH 3/5] x86/vmsi: use the newly introduced EOI callbacks

[Roger Pau Monné]

On Thu, Aug 13, 2020 at 09:19:30AM +0100, Paul Durrant wrote:
> > -----Original Message-----
> > From: Roger Pau Monne <roger.pau@citrix.com>
> > Sent: 12 August 2020 13:47
> > To: xen-devel@lists.xenproject.org
> > Cc: Roger Pau Monne <roger.pau@citrix.com>; Jan Beulich <jbeulich@suse.com>; Andrew Cooper
> > <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; Paul Durrant <paul@xen.org>
> > Subject: [PATCH 3/5] x86/vmsi: use the newly introduced EOI callbacks
> > 
> > Remove the unconditional call to hvm_dpci_msi_eoi in vlapic_handle_EOI
> > and instead use the newly introduced EOI callback mechanism in order
> > to register a callback for MSI vectors injected from passed through
> > devices.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> >  xen/arch/x86/hvm/vlapic.c    |  2 --
> >  xen/arch/x86/hvm/vmsi.c      | 36 ++++++++++++++++++++++--------------
> >  xen/drivers/passthrough/io.c |  4 +++-
> >  xen/include/asm-x86/hvm/io.h |  2 +-
> >  4 files changed, 26 insertions(+), 18 deletions(-)
> > 
> > diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> > index 7369be468b..3b3b3d7621 100644
> > --- a/xen/arch/x86/hvm/vlapic.c
> > +++ b/xen/arch/x86/hvm/vlapic.c
> > @@ -496,8 +496,6 @@ void vlapic_handle_EOI(struct vlapic *vlapic, u8 vector)
> >      else if ( has_viridian_synic(d) )
> >          viridian_synic_ack_sint(v, vector);
> > 
> > -    hvm_dpci_msi_eoi(d, vector);
> > -
> >      spin_lock_irqsave(&vlapic->callback_lock, flags);
> >      callback = vlapic->callbacks[vector].callback;
> >      data = vlapic->callbacks[vector].data;
> > diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
> > index 7ca19353ab..e192c4c6da 100644
> > --- a/xen/arch/x86/hvm/vmsi.c
> > +++ b/xen/arch/x86/hvm/vmsi.c
> > @@ -44,11 +44,9 @@
> >  #include <asm/event.h>
> >  #include <asm/io_apic.h>
> > 
> > -static void vmsi_inj_irq(
> > -    struct vlapic *target,
> > -    uint8_t vector,
> > -    uint8_t trig_mode,
> > -    uint8_t delivery_mode)
> > +static void vmsi_inj_irq(struct vlapic *target, uint8_t vector,
> > +                         uint8_t trig_mode, uint8_t delivery_mode,
> > +                         vlapic_eoi_callback_t *callback, void *data)
> >  {
> >      HVM_DBG_LOG(DBG_LEVEL_VLAPIC, "vmsi_inj_irq: vec %02x trig %d dm %d\n",
> >                  vector, trig_mode, delivery_mode);
> > @@ -57,17 +55,17 @@ static void vmsi_inj_irq(
> >      {
> >      case dest_Fixed:
> >      case dest_LowestPrio:
> > -        vlapic_set_irq(target, vector, trig_mode);
> > +        vlapic_set_irq_callback(target, vector, trig_mode, callback, data);
> >          break;
> >      default:
> >          BUG();
> >      }
> >  }
> > 
> > -int vmsi_deliver(
> > -    struct domain *d, int vector,
> > -    uint8_t dest, uint8_t dest_mode,
> > -    uint8_t delivery_mode, uint8_t trig_mode)
> > +static int vmsi_deliver_callback(struct domain *d, int vector, uint8_t dest,
> > +                                 uint8_t dest_mode, uint8_t delivery_mode,
> > +                                 uint8_t trig_mode,
> > +                                 vlapic_eoi_callback_t *callback, void *data)
> >  {
> >      struct vlapic *target;
> >      struct vcpu *v;
> > @@ -78,7 +76,8 @@ int vmsi_deliver(
> >          target = vlapic_lowest_prio(d, NULL, 0, dest, dest_mode);
> >          if ( target != NULL )
> >          {
> > -            vmsi_inj_irq(target, vector, trig_mode, delivery_mode);
> > +            vmsi_inj_irq(target, vector, trig_mode, delivery_mode, callback,
> > +                         data);
> >              break;
> >          }
> >          HVM_DBG_LOG(DBG_LEVEL_VLAPIC, "null MSI round robin: vector=%02x\n",
> > @@ -89,8 +88,8 @@ int vmsi_deliver(
> >          for_each_vcpu ( d, v )
> >              if ( vlapic_match_dest(vcpu_vlapic(v), NULL,
> >                                     0, dest, dest_mode) )
> > -                vmsi_inj_irq(vcpu_vlapic(v), vector,
> > -                             trig_mode, delivery_mode);
> > +                vmsi_inj_irq(vcpu_vlapic(v), vector, trig_mode, delivery_mode,
> > +                             callback, data);
> >          break;
> > 
> >      default:
> > @@ -103,6 +102,14 @@ int vmsi_deliver(
> >      return 0;
> >  }
> > 
> > +
> > +int vmsi_deliver(struct domain *d, int vector, uint8_t dest, uint8_t dest_mode,
> > +                 uint8_t delivery_mode, uint8_t trig_mode)
> > +{
> > +    return vmsi_deliver_callback(d, vector, dest, dest_mode, delivery_mode,
> > +                                 trig_mode, NULL, NULL);
> > +}
> > +
> >  void vmsi_deliver_pirq(struct domain *d, const struct hvm_pirq_dpci *pirq_dpci)
> >  {
> >      uint32_t flags = pirq_dpci->gmsi.gflags;
> > @@ -119,7 +126,8 @@ void vmsi_deliver_pirq(struct domain *d, const struct hvm_pirq_dpci *pirq_dpci)
> > 
> >      ASSERT(pirq_dpci->flags & HVM_IRQ_DPCI_GUEST_MSI);
> > 
> > -    vmsi_deliver(d, vector, dest, dest_mode, delivery_mode, trig_mode);
> > +    vmsi_deliver_callback(d, vector, dest, dest_mode, delivery_mode, trig_mode,
> > +                          hvm_dpci_msi_eoi, NULL);
> >  }
> > 
> >  /* Return value, -1 : multi-dests, non-negative value: dest_vcpu_id */
> > diff --git a/xen/drivers/passthrough/io.c b/xen/drivers/passthrough/io.c
> > index 6b1305a3e5..3793029b29 100644
> > --- a/xen/drivers/passthrough/io.c
> > +++ b/xen/drivers/passthrough/io.c
> > @@ -874,8 +874,10 @@ static int _hvm_dpci_msi_eoi(struct domain *d,
> >      return 0;
> >  }
> > 
> > -void hvm_dpci_msi_eoi(struct domain *d, int vector)
> > +void hvm_dpci_msi_eoi(struct vcpu *v, unsigned int vector, void *data)
> >  {
> > +    struct domain *d = v->domain;
> > +
> 
> Could we actually drop the vcpu parameter here... i.e. is there any case where this code will be invoked with v != current?

viridian_synic_wrmsr seems to call vlapic_EOI_set without enforcing v
== current (as it seems to be fine being called from v != current as
long as it's not running).

There's also a call to vlapic_EOI_set in vlapic_has_pending_irq that
I'm not sure won't be called with v != current.

In a normal hardware architecture I would say the EOI can only be
performed from the same CPU, and hence v == current, on Xen however
I'm not sure if any of the assists that we provide would allow for the
EOI to be performed from a different vCPU. I can prepare a pre-patch
to change the functions called from vlapic_handle_EOI to not take a
domain or vcpu parameter.

Thanks, Roger.

Re: [PATCH 3/5] x86/vmsi: use the newly introduced EOI callbacks

[Jan Beulich]

On 12.08.2020 14:47, Roger Pau Monne wrote:
> Remove the unconditional call to hvm_dpci_msi_eoi in vlapic_handle_EOI
> and instead use the newly introduced EOI callback mechanism in order
> to register a callback for MSI vectors injected from passed through
> devices.

In patch 2 you merely invoke the callback when the EOI arrived,
but you don't clear the callback (unless I've missed something).
Here you register the callback once per triggering of the IRQ,
without clearing it from the callback itself either. Why is it
correct / safe to keep the callback registered? What about
conflicting callbacks for shared vectors?

Jan

Re: [PATCH 3/5] x86/vmsi: use the newly introduced EOI callbacks

[Roger Pau Monné]

On Mon, Aug 24, 2020 at 04:06:31PM +0200, Jan Beulich wrote:
> On 12.08.2020 14:47, Roger Pau Monne wrote:
> > Remove the unconditional call to hvm_dpci_msi_eoi in vlapic_handle_EOI
> > and instead use the newly introduced EOI callback mechanism in order
> > to register a callback for MSI vectors injected from passed through
> > devices.
> 
> In patch 2 you merely invoke the callback when the EOI arrived,
> but you don't clear the callback (unless I've missed something).
> Here you register the callback once per triggering of the IRQ,
> without clearing it from the callback itself either.

It gets cleared on the next call to vlapic_set_irq_callback, or set to
a new callback value if the passed callback is not NULL.

> Why is it
> correct / safe to keep the callback registered?

The next vector injected is going to clear it, so should be safe, as
no vector can be injected without calling vlapic_set_irq_callback.

> What about
> conflicting callbacks for shared vectors?

In this callback model for vlapic only the last injected vector
callback would get executed. It's my understanding that lapic
vectors cannot be safely shared unless there's a higher level
interrupt controller (ie: an IO-APIC) that does the sharing.

I have further patches that also add EOI callbacks to vIO-APIC and
8259A PICs, so that all interrupt controllers can handle EOI
callbacks.

Thanks, Roger.

Re: [PATCH 3/5] x86/vmsi: use the newly introduced EOI callbacks

[Jan Beulich]

On 24.08.2020 16:44, Roger Pau Monné wrote:
> On Mon, Aug 24, 2020 at 04:06:31PM +0200, Jan Beulich wrote:
>> On 12.08.2020 14:47, Roger Pau Monne wrote:
>>> Remove the unconditional call to hvm_dpci_msi_eoi in vlapic_handle_EOI
>>> and instead use the newly introduced EOI callback mechanism in order
>>> to register a callback for MSI vectors injected from passed through
>>> devices.
>>
>> In patch 2 you merely invoke the callback when the EOI arrived,
>> but you don't clear the callback (unless I've missed something).
>> Here you register the callback once per triggering of the IRQ,
>> without clearing it from the callback itself either.
> 
> It gets cleared on the next call to vlapic_set_irq_callback, or set to
> a new callback value if the passed callback is not NULL.
> 
>> Why is it
>> correct / safe to keep the callback registered?
> 
> The next vector injected is going to clear it, so should be safe, as
> no vector can be injected without calling vlapic_set_irq_callback.

But what about duplicate EOIs, even if just by bug or erratum?
I notice, for example, that VMX'es VMEXIT handler directly
calls vlapic_handle_EOI(). I'd find it more safe if an
unexpected EOI didn't get any callback invoked.

>> What about
>> conflicting callbacks for shared vectors?
> 
> In this callback model for vlapic only the last injected vector
> callback would get executed. It's my understanding that lapic
> vectors cannot be safely shared unless there's a higher level
> interrupt controller (ie: an IO-APIC) that does the sharing.

As said on a different, but pretty recent thread: I do think
sharing is possible if devices and drivers meet certain criteria
(in particular have a way to determine which of the devices
actually require service). It's just not something one would
normally do. But iirc such a model was used in good old DOS to
overcome the 15 IRQs limit (of which quite a few had fixed
purposes, so for add-in devices there were only very few left).

Jan

Re: [PATCH 3/5] x86/vmsi: use the newly introduced EOI callbacks

[Roger Pau Monné]

On Mon, Aug 24, 2020 at 06:07:28PM +0200, Jan Beulich wrote:
> On 24.08.2020 16:44, Roger Pau Monné wrote:
> > On Mon, Aug 24, 2020 at 04:06:31PM +0200, Jan Beulich wrote:
> >> On 12.08.2020 14:47, Roger Pau Monne wrote:
> >>> Remove the unconditional call to hvm_dpci_msi_eoi in vlapic_handle_EOI
> >>> and instead use the newly introduced EOI callback mechanism in order
> >>> to register a callback for MSI vectors injected from passed through
> >>> devices.
> >>
> >> In patch 2 you merely invoke the callback when the EOI arrived,
> >> but you don't clear the callback (unless I've missed something).
> >> Here you register the callback once per triggering of the IRQ,
> >> without clearing it from the callback itself either.
> > 
> > It gets cleared on the next call to vlapic_set_irq_callback, or set to
> > a new callback value if the passed callback is not NULL.
> > 
> >> Why is it
> >> correct / safe to keep the callback registered?
> > 
> > The next vector injected is going to clear it, so should be safe, as
> > no vector can be injected without calling vlapic_set_irq_callback.
> 
> But what about duplicate EOIs, even if just by bug or erratum?
> I notice, for example, that VMX'es VMEXIT handler directly
> calls vlapic_handle_EOI().

Yes, but that's expected and required, because when using VMX LAPIC
virtualization you get an explicit vmexit (EXIT_REASON_EOI_INDUCED) on
EOI of requested vectors by using the EOI exit bitmap
(vmx_update_eoi_exit_bitmap).

> I'd find it more safe if an
> unexpected EOI didn't get any callback invoked.

OK, the callback can be certainly cleared in vlapic_handle_EOI.

> 
> >> What about
> >> conflicting callbacks for shared vectors?
> > 
> > In this callback model for vlapic only the last injected vector
> > callback would get executed. It's my understanding that lapic
> > vectors cannot be safely shared unless there's a higher level
> > interrupt controller (ie: an IO-APIC) that does the sharing.
> 
> As said on a different, but pretty recent thread: I do think
> sharing is possible if devices and drivers meet certain criteria
> (in particular have a way to determine which of the devices
> actually require service). It's just not something one would
> normally do. But iirc such a model was used in good old DOS to
> overcome the 15 IRQs limit (of which quite a few had fixed
> purposes, so for add-in devices there were only very few left).

So my callback model for the vIO-APIC/vPIC is different from the one
used for the vlapic. In that case callers must use a helper to
register/remove a callback for GSIs, and a single GSI can have
multiple callbacks attached.

Such interface works well with the vIO-APIC/vPIC because interrupts
from devices are statically assigned a GSI, and you only need to
register the callback when the device is instantiated.

For vlapic OTOH this would be more complex, as a guest might decide to
change MSI vectors constantly and thus require a non-trivial amount of
registrations/removals of callbacks.

I was assuming that any sane OS wouldn't share a lapic vector for
multiple devices, and that hence we could get away without having to
implement multiple per-vector callback support.

Would you be fine with clearing the callback in vlapic_handle_EOI and
then vlapic_set_irq_callback complaining if it finds there's a
previous callback different than the one provided already set for the
to be injected vector?

Thanks, Roger.

Re: [PATCH 3/5] x86/vmsi: use the newly introduced EOI callbacks

[Jan Beulich]

On 24.08.2020 18:39, Roger Pau Monné wrote:
> Would you be fine with clearing the callback in vlapic_handle_EOI and

Yes - as said, I'd prefer such a model.

> then vlapic_set_irq_callback complaining if it finds there's a
> previous callback different than the one provided already set for the
> to be injected vector?

For debugging purposes this may indeed be helpful, albeit I'll admit
that initially I didn't consider this as necessary.

Jan

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

[Roger Pau Monne]

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@citrix.com>
---
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?
---
 xen/arch/x86/hvm/viridian/synic.c  | 28 +++++++++++++++-------------
 xen/arch/x86/hvm/vlapic.c          |  4 ----
 xen/include/asm-x86/hvm/viridian.h |  1 -
 3 files changed, 15 insertions(+), 18 deletions(-)

diff --git a/xen/arch/x86/hvm/viridian/synic.c b/xen/arch/x86/hvm/viridian/synic.c
index 94a2b88733..250f0353cf 100644
--- a/xen/arch/x86/hvm/viridian/synic.c
+++ b/xen/arch/x86/hvm/viridian/synic.c
@@ -315,6 +315,19 @@ void viridian_synic_poll(struct vcpu *v)
     viridian_time_poll_timers(v);
 }
 
+static void synic_ack_sint(struct vcpu *v, unsigned int vector, void *data)
+{
+    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);
+}
+
+
 bool viridian_synic_deliver_timer_msg(struct vcpu *v, unsigned int sintx,
                                       unsigned int index,
                                       uint64_t expiration,
@@ -361,7 +374,8 @@ bool viridian_synic_deliver_timer_msg(struct vcpu *v, unsigned int sintx,
     memcpy(msg->u.payload, &payload, sizeof(payload));
 
     if ( !vs->masked )
-        vlapic_set_irq(vcpu_vlapic(v), vs->vector, 0);
+        vlapic_set_irq_callback(vcpu_vlapic(v), vs->vector, 0,
+                                synic_ack_sint, NULL);
 
     return true;
 }
@@ -380,18 +394,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 3b3b3d7621..701ff942e6 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -489,12 +489,8 @@ void vlapic_handle_EOI(struct vlapic *vlapic, u8 vector)
     void *data;
     unsigned long flags;
 
-    /* All synic SINTx vectors are edge triggered */
-
     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);
 
     spin_lock_irqsave(&vlapic->callback_lock, flags);
     callback = vlapic->callbacks[vector].callback;
diff --git a/xen/include/asm-x86/hvm/viridian.h b/xen/include/asm-x86/hvm/viridian.h
index 844e56b38f..d387d11ce0 100644
--- a/xen/include/asm-x86/hvm/viridian.h
+++ b/xen/include/asm-x86/hvm/viridian.h
@@ -89,7 +89,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__ */
 
-- 
2.28.0

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

[Paul Durrant]

> -----Original Message-----
> From: Roger Pau Monne <roger.pau@citrix.com>
> Sent: 12 August 2020 13:47
> To: xen-devel@lists.xenproject.org
> Cc: Roger Pau Monne <roger.pau@citrix.com>; Paul Durrant <paul@xen.org>; Wei Liu <wl@xen.org>; Jan
> Beulich <jbeulich@suse.com>; Andrew Cooper <andrew.cooper3@citrix.com>
> 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@citrix.com>
> ---
> 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.

  Paul

> ---
>  xen/arch/x86/hvm/viridian/synic.c  | 28 +++++++++++++++-------------
>  xen/arch/x86/hvm/vlapic.c          |  4 ----
>  xen/include/asm-x86/hvm/viridian.h |  1 -
>  3 files changed, 15 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/viridian/synic.c b/xen/arch/x86/hvm/viridian/synic.c
> index 94a2b88733..250f0353cf 100644
> --- a/xen/arch/x86/hvm/viridian/synic.c
> +++ b/xen/arch/x86/hvm/viridian/synic.c
> @@ -315,6 +315,19 @@ void viridian_synic_poll(struct vcpu *v)
>      viridian_time_poll_timers(v);
>  }
> 
> +static void synic_ack_sint(struct vcpu *v, unsigned int vector, void *data)
> +{
> +    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);
> +}
> +
> +
>  bool viridian_synic_deliver_timer_msg(struct vcpu *v, unsigned int sintx,
>                                        unsigned int index,
>                                        uint64_t expiration,
> @@ -361,7 +374,8 @@ bool viridian_synic_deliver_timer_msg(struct vcpu *v, unsigned int sintx,
>      memcpy(msg->u.payload, &payload, sizeof(payload));
> 
>      if ( !vs->masked )
> -        vlapic_set_irq(vcpu_vlapic(v), vs->vector, 0);
> +        vlapic_set_irq_callback(vcpu_vlapic(v), vs->vector, 0,
> +                                synic_ack_sint, NULL);
> 
>      return true;
>  }
> @@ -380,18 +394,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 3b3b3d7621..701ff942e6 100644
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -489,12 +489,8 @@ void vlapic_handle_EOI(struct vlapic *vlapic, u8 vector)
>      void *data;
>      unsigned long flags;
> 
> -    /* All synic SINTx vectors are edge triggered */
> -
>      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);
> 
>      spin_lock_irqsave(&vlapic->callback_lock, flags);
>      callback = vlapic->callbacks[vector].callback;
> diff --git a/xen/include/asm-x86/hvm/viridian.h b/xen/include/asm-x86/hvm/viridian.h
> index 844e56b38f..d387d11ce0 100644
> --- a/xen/include/asm-x86/hvm/viridian.h
> +++ b/xen/include/asm-x86/hvm/viridian.h
> @@ -89,7 +89,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__ */
> 
> --
> 2.28.0

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

[Roger Pau Monné]

On Thu, Aug 13, 2020 at 09:33:43AM +0100, Paul Durrant wrote:
> > -----Original Message-----
> > From: Roger Pau Monne <roger.pau@citrix.com>
> > Sent: 12 August 2020 13:47
> > To: xen-devel@lists.xenproject.org
> > Cc: Roger Pau Monne <roger.pau@citrix.com>; Paul Durrant <paul@xen.org>; Wei Liu <wl@xen.org>; Jan
> > Beulich <jbeulich@suse.com>; Andrew Cooper <andrew.cooper3@citrix.com>
> > 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@citrix.com>
> > ---
> > 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.

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

[Paul Durrant]

> -----Original Message-----
> From: Roger Pau Monné <roger.pau@citrix.com>
> Sent: 13 August 2020 09:58
> To: paul@xen.org
> Cc: xen-devel@lists.xenproject.org; 'Wei Liu' <wl@xen.org>; 'Jan Beulich' <jbeulich@suse.com>; 'Andrew
> Cooper' <andrew.cooper3@citrix.com>
> 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@citrix.com>
> > > Sent: 12 August 2020 13:47
> > > To: xen-devel@lists.xenproject.org
> > > Cc: Roger Pau Monne <roger.pau@citrix.com>; Paul Durrant <paul@xen.org>; Wei Liu <wl@xen.org>; Jan
> > > Beulich <jbeulich@suse.com>; Andrew Cooper <andrew.cooper3@citrix.com>
> > > 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@citrix.com>
> > > ---
> > > 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

[PATCH 5/5] x86/vioapic: switch to use the EOI callback mechanism

[Roger Pau Monne]

Switch the emulated IO-APIC code to use the local APIC EOI callback
mechanism. This allows to remove the last hardcoded callback from
vlapic_handle_EOI.

Move and rename the vioapic_update_EOI now that it can be made static.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/hvm/vioapic.c | 90 +++++++++++++++++++-------------------
 xen/arch/x86/hvm/vlapic.c  |  7 +--
 2 files changed, 47 insertions(+), 50 deletions(-)

diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
index 67d4a6237f..58a85cf9de 100644
--- a/xen/arch/x86/hvm/vioapic.c
+++ b/xen/arch/x86/hvm/vioapic.c
@@ -375,6 +375,50 @@ static const struct hvm_mmio_ops vioapic_mmio_ops = {
     .write = vioapic_write
 };
 
+static void eoi_callback(struct vcpu *v, unsigned int vector, void *data)
+{
+    struct domain *d = v->domain;
+    struct hvm_irq *hvm_irq = hvm_domain_irq(d);
+    union vioapic_redir_entry *ent;
+    unsigned int i;
+
+    ASSERT(has_vioapic(d));
+
+    spin_lock(&d->arch.hvm.irq_lock);
+
+    for ( i = 0; i < d->arch.hvm.nr_vioapics; i++ )
+    {
+        struct hvm_vioapic *vioapic = domain_vioapic(d, i);
+        unsigned int pin;
+
+        for ( pin = 0; pin < vioapic->nr_pins; pin++ )
+        {
+            ent = &vioapic->redirtbl[pin];
+            if ( ent->fields.vector != vector )
+                continue;
+
+            ent->fields.remote_irr = 0;
+
+            if ( is_iommu_enabled(d) )
+            {
+                spin_unlock(&d->arch.hvm.irq_lock);
+                hvm_dpci_eoi(d, vioapic->base_gsi + pin, ent);
+                spin_lock(&d->arch.hvm.irq_lock);
+            }
+
+            if ( (ent->fields.trig_mode == VIOAPIC_LEVEL_TRIG) &&
+                 !ent->fields.mask &&
+                 hvm_irq->gsi_assert_count[vioapic->base_gsi + pin] )
+            {
+                ent->fields.remote_irr = 1;
+                vioapic_deliver(vioapic, pin);
+            }
+        }
+    }
+
+    spin_unlock(&d->arch.hvm.irq_lock);
+}
+
 static void ioapic_inj_irq(
     struct hvm_vioapic *vioapic,
     struct vlapic *target,
@@ -388,7 +432,8 @@ static void ioapic_inj_irq(
     ASSERT((delivery_mode == dest_Fixed) ||
            (delivery_mode == dest_LowestPrio));
 
-    vlapic_set_irq(target, vector, trig_mode);
+    vlapic_set_irq_callback(target, vector, trig_mode,
+                            trig_mode ? eoi_callback : NULL, NULL);
 }
 
 static void vioapic_deliver(struct hvm_vioapic *vioapic, unsigned int pin)
@@ -495,49 +540,6 @@ void vioapic_irq_positive_edge(struct domain *d, unsigned int irq)
     }
 }
 
-void vioapic_update_EOI(struct domain *d, u8 vector)
-{
-    struct hvm_irq *hvm_irq = hvm_domain_irq(d);
-    union vioapic_redir_entry *ent;
-    unsigned int i;
-
-    ASSERT(has_vioapic(d));
-
-    spin_lock(&d->arch.hvm.irq_lock);
-
-    for ( i = 0; i < d->arch.hvm.nr_vioapics; i++ )
-    {
-        struct hvm_vioapic *vioapic = domain_vioapic(d, i);
-        unsigned int pin;
-
-        for ( pin = 0; pin < vioapic->nr_pins; pin++ )
-        {
-            ent = &vioapic->redirtbl[pin];
-            if ( ent->fields.vector != vector )
-                continue;
-
-            ent->fields.remote_irr = 0;
-
-            if ( is_iommu_enabled(d) )
-            {
-                spin_unlock(&d->arch.hvm.irq_lock);
-                hvm_dpci_eoi(d, vioapic->base_gsi + pin, ent);
-                spin_lock(&d->arch.hvm.irq_lock);
-            }
-
-            if ( (ent->fields.trig_mode == VIOAPIC_LEVEL_TRIG) &&
-                 !ent->fields.mask &&
-                 hvm_irq->gsi_assert_count[vioapic->base_gsi + pin] )
-            {
-                ent->fields.remote_irr = 1;
-                vioapic_deliver(vioapic, pin);
-            }
-        }
-    }
-
-    spin_unlock(&d->arch.hvm.irq_lock);
-}
-
 int vioapic_get_mask(const struct domain *d, unsigned int gsi)
 {
     unsigned int pin = 0; /* See gsi_vioapic */
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 701ff942e6..129b20ff10 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -483,22 +483,17 @@ void vlapic_EOI_set(struct vlapic *vlapic)
 
 void vlapic_handle_EOI(struct vlapic *vlapic, u8 vector)
 {
-    struct vcpu *v = vlapic_vcpu(vlapic);
-    struct domain *d = v->domain;
     vlapic_eoi_callback_t *callback;
     void *data;
     unsigned long flags;
 
-    if ( vlapic_test_vector(vector, &vlapic->regs->data[APIC_TMR]) )
-        vioapic_update_EOI(d, vector);
-
     spin_lock_irqsave(&vlapic->callback_lock, flags);
     callback = vlapic->callbacks[vector].callback;
     data = vlapic->callbacks[vector].data;
     spin_unlock_irqrestore(&vlapic->callback_lock, flags);
 
     if ( callback )
-        callback(v, vector, data);
+        callback(vlapic_vcpu(vlapic), vector, data);
 }
 
 static bool_t is_multicast_dest(struct vlapic *vlapic, unsigned int short_hand,
-- 
2.28.0

Re: [PATCH 5/5] x86/vioapic: switch to use the EOI callback mechanism

[Jan Beulich]

On 12.08.2020 14:47, Roger Pau Monne wrote:
> --- a/xen/arch/x86/hvm/vioapic.c
> +++ b/xen/arch/x86/hvm/vioapic.c
> @@ -375,6 +375,50 @@ static const struct hvm_mmio_ops vioapic_mmio_ops = {
>      .write = vioapic_write
>  };
>  
> +static void eoi_callback(struct vcpu *v, unsigned int vector, void *data)
> +{
> +    struct domain *d = v->domain;
> +    struct hvm_irq *hvm_irq = hvm_domain_irq(d);
> +    union vioapic_redir_entry *ent;

While you move the code, could you restrict this variable's scope?

> +    unsigned int i;
> +
> +    ASSERT(has_vioapic(d));
> +
> +    spin_lock(&d->arch.hvm.irq_lock);
> +
> +    for ( i = 0; i < d->arch.hvm.nr_vioapics; i++ )
> +    {
> +        struct hvm_vioapic *vioapic = domain_vioapic(d, i);
> +        unsigned int pin;
> +
> +        for ( pin = 0; pin < vioapic->nr_pins; pin++ )
> +        {
> +            ent = &vioapic->redirtbl[pin];
> +            if ( ent->fields.vector != vector )
> +                continue;
> +
> +            ent->fields.remote_irr = 0;
> +
> +            if ( is_iommu_enabled(d) )
> +            {
> +                spin_unlock(&d->arch.hvm.irq_lock);
> +                hvm_dpci_eoi(d, vioapic->base_gsi + pin, ent);
> +                spin_lock(&d->arch.hvm.irq_lock);
> +            }

Just as a remark (simply because of it catching my attention) -
this intermediate dropping of the lock can't really be good. We
may want (need?) to think about ways to avoid this.

Jan