RE: [PATCH v2 03/11] x86/vlapic: introduce an EOI callback mechanism

[Paul Durrant <xadimgnik@gmail.com>]

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Roger Pau Monne
> Sent: 30 September 2020 11:41
> 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>
> Subject: [PATCH v2 03/11] x86/vlapic: introduce an EOI callback mechanism
> 
> 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.

s/not/no

> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Changes since v1:
>  - Make vlapic_set_irq an inline function on the header.
>  - Clear the callback hook in vlapic_handle_EOI.
>  - Introduce a helper to set the callback without injecting a vector.
>  - Remove unneeded parentheses.
>  - Reduce callback table by 16.
>  - Use %pv to print domain/vcpu ID.
> ---
> RFC: should callbacks also be executed in vlapic_do_init (which is
> called by vlapic_reset). We would need to make sure ISR and IRR
> are cleared using some kind of test and clear atomic functionality to
> make this race free.
> ---
>  xen/arch/x86/hvm/vlapic.c        | 62 ++++++++++++++++++++++++++++++--
>  xen/include/asm-x86/hvm/vlapic.h | 18 +++++++++-
>  2 files changed, 77 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> index ae737403f3..38c62a02e6 100644
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -144,7 +144,32 @@ 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_callback(struct vlapic *vlapic, unsigned int vec,
> +                         vlapic_eoi_callback_t *callback, void *data)
> +{
> +    unsigned long flags;
> +    unsigned int index = vec - 16;
> +
> +    if ( !callback || vec < 16 || vec >= X86_NR_VECTORS )
> +    {
> +        ASSERT_UNREACHABLE();
> +        return;
> +    }
> +
> +    spin_lock_irqsave(&vlapic->callback_lock, flags);
> +    if ( vlapic->callbacks[index].callback &&
> +         vlapic->callbacks[index].callback != callback )
> +        printk(XENLOG_G_WARNING
> +               "%pv overriding vector %#x callback %ps (%p) with %ps (%p)\n",
> +               vlapic_vcpu(vlapic), vec, vlapic->callbacks[index].callback,
> +               vlapic->callbacks[index].callback, callback, callback);
> +    vlapic->callbacks[index].callback = callback;
> +    vlapic->callbacks[index].data = data;
> +    spin_unlock_irqrestore(&vlapic->callback_lock, flags);
> +}
> +
> +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 +184,12 @@ 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 )
> +        vlapic_set_callback(vlapic, vec, callback, data);
> +

Can this not happen several times before an EOI? I.e. the vector could already be set in IRR, right?

  Paul

>      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);
> @@ -459,10 +488,24 @@ void vlapic_EOI_set(struct vlapic *vlapic)
> 
>  void vlapic_handle_EOI(struct vlapic *vlapic, u8 vector)
>  {
> +    vlapic_eoi_callback_t *callback;
> +    void *data;
> +    unsigned long flags;
> +    unsigned int index = vector - 16;
> +
>      if ( vlapic_test_vector(vector, &vlapic->regs->data[APIC_TMR]) )
>          vioapic_update_EOI(vector);
> 
>      hvm_dpci_msi_eoi(vector);
> +
> +    spin_lock_irqsave(&vlapic->callback_lock, flags);
> +    callback = vlapic->callbacks[index].callback;
> +    vlapic->callbacks[index].callback = NULL;
> +    data = vlapic->callbacks[index].data;
> +    spin_unlock_irqrestore(&vlapic->callback_lock, flags);
> +
> +    if ( callback )
> +        callback(vector, data);
>  }
> 
>  static bool_t is_multicast_dest(struct vlapic *vlapic, unsigned int short_hand,
> @@ -1629,9 +1672,23 @@ int vlapic_init(struct vcpu *v)
>      }
>      clear_page(vlapic->regs);
> 
> +    if ( !vlapic->callbacks )
> +    {
> +        vlapic->callbacks = xmalloc_array(typeof(*vlapic->callbacks),
> +                                          X86_NR_VECTORS - 16);
> +        if ( !vlapic->callbacks )
> +        {
> +            dprintk(XENLOG_ERR, "%pv: alloc vlapic callbacks error\n", v);
> +            return -ENOMEM;
> +        }
> +    }
> +    memset(vlapic->callbacks, 0, sizeof(*vlapic->callbacks) *
> +                                 (X86_NR_VECTORS - 16));
> +
>      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);
> 
> @@ -1653,6 +1710,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..c380127a71 100644
> --- a/xen/include/asm-x86/hvm/vlapic.h
> +++ b/xen/include/asm-x86/hvm/vlapic.h
> @@ -73,6 +73,8 @@
>  #define vlapic_clear_vector(vec, bitmap)                                \
>      clear_bit(VEC_POS(vec), (uint32_t *)((bitmap) + REG_POS(vec)))
> 
> +typedef void vlapic_eoi_callback_t(unsigned int vector, void *data);
> +
>  struct vlapic {
>      struct hvm_hw_lapic      hw;
>      struct hvm_hw_lapic_regs *regs;
> @@ -89,6 +91,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,7 +118,16 @@ 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(struct vlapic *vlapic, uint8_t vec, uint8_t trig);
> +void vlapic_set_callback(struct vlapic *vlapic, unsigned int vec,
> +                         vlapic_eoi_callback_t *callback, void *data);
> +void vlapic_set_irq_callback(struct vlapic *vlapic, uint8_t vec, uint8_t trig,
> +                             vlapic_eoi_callback_t *callback, void *data);
> +
> +static inline void vlapic_set_irq(struct vlapic *vlapic, uint8_t vec,
> +                                  uint8_t trig)
> +{
> +    vlapic_set_irq_callback(vlapic, vec, trig, NULL, NULL);
> +}
> 
>  int vlapic_has_pending_irq(struct vcpu *v);
>  int vlapic_ack_pending_irq(struct vcpu *v, int vector, bool_t force_ack);
> --
> 2.28.0
>