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

Roger Pau Monne posted 11 patches 1 month, 1 week ago

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

Posted by Roger Pau Monne 1 month, 1 week ago
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.

The setter of the callback will be in charge for setting the callback
again on resume. That is the reason why vlapic_set_callback is not a
static function.

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

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v2:
 - Fix commit message typo.
 - Expand commit message.
 - Also print a warning if the callback data is overridden.
 - Properly free memory in case of error in vlapic_init.

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.
---
 xen/arch/x86/hvm/vlapic.c        | 64 +++++++++++++++++++++++++++++++-
 xen/include/asm-x86/hvm/vlapic.h | 18 ++++++++-
 2 files changed, 79 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 98e4ba67d79..851a1f5bd6c 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -144,7 +144,35 @@ 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 ||
+          vlapic->callbacks[index].data != data) )
+        printk(XENLOG_G_WARNING
+               "%pv overriding vector %#x callback %ps (%p) data %p "
+               "with %ps (%p) data %p\n",
+               vlapic_vcpu(vlapic), vec, vlapic->callbacks[index].callback,
+               vlapic->callbacks[index].callback, vlapic->callbacks[index].data,
+               callback, callback, data);
+    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 +187,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);
+
     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 +491,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,
@@ -1620,9 +1666,22 @@ int vlapic_init(struct vcpu *v)
 
     clear_page(vlapic->regs);
 
+    vlapic->callbacks = xmalloc_array(typeof(*vlapic->callbacks),
+                                      X86_NR_VECTORS - 16);
+    if ( !vlapic->callbacks )
+    {
+        dprintk(XENLOG_ERR, "%pv: alloc vlapic callbacks error\n", v);
+        unmap_domain_page_global(vlapic->regs);
+        free_domheap_page(vlapic->regs_page);
+        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);
 
@@ -1644,6 +1703,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 8f908928c35..c380127a719 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.30.1


Re: [PATCH v3 03/11] x86/vlapic: introduce an EOI callback mechanism

Posted by Jan Beulich 1 month ago
On 31.03.2021 12:32, 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.
> 
> 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.
> 
> The setter of the callback will be in charge for setting the callback
> again on resume. That is the reason why vlapic_set_callback is not a
> static function.

I'm struggling with your use of "resume" here: Resuming from S3
doesn't require re-doing anything that's kept in memory, does it?
So what meaning does the word have here?

Apart from this, and with the xzalloc_array() change requested
by Andrew, this looks good to me.

Jan

Re: [PATCH v3 03/11] x86/vlapic: introduce an EOI callback mechanism

Posted by Roger Pau Monné 1 month ago
On Wed, Apr 07, 2021 at 04:55:43PM +0200, Jan Beulich wrote:
> On 31.03.2021 12:32, 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.
> > 
> > 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.
> > 
> > The setter of the callback will be in charge for setting the callback
> > again on resume. That is the reason why vlapic_set_callback is not a
> > static function.
> 
> I'm struggling with your use of "resume" here: Resuming from S3
> doesn't require re-doing anything that's kept in memory, does it?
> So what meaning does the word have here?

Right, I can see the confusion. Resume here means a guest being
migrated or restored, not Xen itself being resumed. Callbacks are not
part of the exported guest state, and hence any emulated device that
requires a callback will have to register it as part of loading the
saved state.

> Apart from this, and with the xzalloc_array() change requested
> by Andrew, this looks good to me.

Thanks, Roger.

Re: [PATCH v3 03/11] x86/vlapic: introduce an EOI callback mechanism

Posted by Jan Beulich 1 month ago
On 07.04.2021 18:27, Roger Pau Monné wrote:
> On Wed, Apr 07, 2021 at 04:55:43PM +0200, Jan Beulich wrote:
>> On 31.03.2021 12:32, 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.
>>>
>>> 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.
>>>
>>> The setter of the callback will be in charge for setting the callback
>>> again on resume. That is the reason why vlapic_set_callback is not a
>>> static function.
>>
>> I'm struggling with your use of "resume" here: Resuming from S3
>> doesn't require re-doing anything that's kept in memory, does it?
>> So what meaning does the word have here?
> 
> Right, I can see the confusion. Resume here means a guest being
> migrated or restored, not Xen itself being resumed. Callbacks are not
> part of the exported guest state, and hence any emulated device that
> requires a callback will have to register it as part of loading the
> saved state.
> 
>> Apart from this, and with the xzalloc_array() change requested
>> by Andrew, this looks good to me.

In which case with this change and "resume" replaced suitably in the
description
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH v3 03/11] x86/vlapic: introduce an EOI callback mechanism

Posted by Roger Pau Monné 1 month ago
On Thu, Apr 08, 2021 at 08:20:15AM +0200, Jan Beulich wrote:
> On 07.04.2021 18:27, Roger Pau Monné wrote:
> > On Wed, Apr 07, 2021 at 04:55:43PM +0200, Jan Beulich wrote:
> >> On 31.03.2021 12:32, 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.
> >>>
> >>> 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.
> >>>
> >>> The setter of the callback will be in charge for setting the callback
> >>> again on resume. That is the reason why vlapic_set_callback is not a
> >>> static function.
> >>
> >> I'm struggling with your use of "resume" here: Resuming from S3
> >> doesn't require re-doing anything that's kept in memory, does it?
> >> So what meaning does the word have here?
> > 
> > Right, I can see the confusion. Resume here means a guest being
> > migrated or restored, not Xen itself being resumed. Callbacks are not
> > part of the exported guest state, and hence any emulated device that
> > requires a callback will have to register it as part of loading the
> > saved state.
> > 
> >> Apart from this, and with the xzalloc_array() change requested
> >> by Andrew, this looks good to me.
> 
> In which case with this change and "resume" replaced suitably in the
> description

I've worded it as:

"The setter of the callback will be in charge for setting the callback
again on guest restore or resume, as callbacks are not saved as part
of the vlapic state. That is the reason why vlapic_set_callback is not
a static function."

> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks. I also want to make the warning message in vlapic_set_callback
conditional to the vector being pending in ISR or IRR, so I will not
add your RB this time if that's fine.

Roger.

Re: [PATCH v3 03/11] x86/vlapic: introduce an EOI callback mechanism

Posted by Jan Beulich 1 month ago
On 08.04.2021 11:12, Roger Pau Monné wrote:
> On Thu, Apr 08, 2021 at 08:20:15AM +0200, Jan Beulich wrote:
>> On 07.04.2021 18:27, Roger Pau Monné wrote:
>>> On Wed, Apr 07, 2021 at 04:55:43PM +0200, Jan Beulich wrote:
>>>> On 31.03.2021 12:32, 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.
>>>>>
>>>>> 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.
>>>>>
>>>>> The setter of the callback will be in charge for setting the callback
>>>>> again on resume. That is the reason why vlapic_set_callback is not a
>>>>> static function.
>>>>
>>>> I'm struggling with your use of "resume" here: Resuming from S3
>>>> doesn't require re-doing anything that's kept in memory, does it?
>>>> So what meaning does the word have here?
>>>
>>> Right, I can see the confusion. Resume here means a guest being
>>> migrated or restored, not Xen itself being resumed. Callbacks are not
>>> part of the exported guest state, and hence any emulated device that
>>> requires a callback will have to register it as part of loading the
>>> saved state.
>>>
>>>> Apart from this, and with the xzalloc_array() change requested
>>>> by Andrew, this looks good to me.
>>
>> In which case with this change and "resume" replaced suitably in the
>> description
> 
> I've worded it as:
> 
> "The setter of the callback will be in charge for setting the callback
> again on guest restore or resume, as callbacks are not saved as part
> of the vlapic state. That is the reason why vlapic_set_callback is not
> a static function."

Hmm, you still mention "resume", which makes me continue to wonder
what you're thinking of beyond guest restore.

Jan

Re: [PATCH v3 03/11] x86/vlapic: introduce an EOI callback mechanism

Posted by Roger Pau Monné 1 month ago
On Thu, Apr 08, 2021 at 12:49:44PM +0200, Jan Beulich wrote:
> On 08.04.2021 11:12, Roger Pau Monné wrote:
> > On Thu, Apr 08, 2021 at 08:20:15AM +0200, Jan Beulich wrote:
> >> On 07.04.2021 18:27, Roger Pau Monné wrote:
> >>> On Wed, Apr 07, 2021 at 04:55:43PM +0200, Jan Beulich wrote:
> >>>> On 31.03.2021 12:32, 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.
> >>>>>
> >>>>> 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.
> >>>>>
> >>>>> The setter of the callback will be in charge for setting the callback
> >>>>> again on resume. That is the reason why vlapic_set_callback is not a
> >>>>> static function.
> >>>>
> >>>> I'm struggling with your use of "resume" here: Resuming from S3
> >>>> doesn't require re-doing anything that's kept in memory, does it?
> >>>> So what meaning does the word have here?
> >>>
> >>> Right, I can see the confusion. Resume here means a guest being
> >>> migrated or restored, not Xen itself being resumed. Callbacks are not
> >>> part of the exported guest state, and hence any emulated device that
> >>> requires a callback will have to register it as part of loading the
> >>> saved state.
> >>>
> >>>> Apart from this, and with the xzalloc_array() change requested
> >>>> by Andrew, this looks good to me.
> >>
> >> In which case with this change and "resume" replaced suitably in the
> >> description
> > 
> > I've worded it as:
> > 
> > "The setter of the callback will be in charge for setting the callback
> > again on guest restore or resume, as callbacks are not saved as part
> > of the vlapic state. That is the reason why vlapic_set_callback is not
> > a static function."
> 
> Hmm, you still mention "resume", which makes me continue to wonder
> what you're thinking of beyond guest restore.

Urg, yes, let me remove that resume.

Thanks, Roger.

Re: [PATCH v3 03/11] x86/vlapic: introduce an EOI callback mechanism

Posted by Andrew Cooper 1 month, 1 week ago
On 31/03/2021 11:32, Roger Pau Monne wrote:
> @@ -1620,9 +1666,22 @@ int vlapic_init(struct vcpu *v)
>  
>      clear_page(vlapic->regs);
>  
> +    vlapic->callbacks = xmalloc_array(typeof(*vlapic->callbacks),
> +                                      X86_NR_VECTORS - 16);
> +    if ( !vlapic->callbacks )
> +    {
> +        dprintk(XENLOG_ERR, "%pv: alloc vlapic callbacks error\n", v);
> +        unmap_domain_page_global(vlapic->regs);
> +        free_domheap_page(vlapic->regs_page);
> +        return -ENOMEM;
> +    }
> +    memset(vlapic->callbacks, 0,
> +           sizeof(*vlapic->callbacks) * (X86_NR_VECTORS - 16));

xzalloc_array() instead of memset().  Also, we shouldn't be printing for
-ENOMEM cases.

As for the construction/teardown logic, vlapic_init()'s caller already
vlapic_destory().  Therefore, the existing error path you've copied is
buggy because it will cause a double-free if __map_domain_page_global()
fails.

I'll do a cleanup patch to fix the idempotency, which needs backporting too.

~Andrew


Re: [PATCH v3 03/11] x86/vlapic: introduce an EOI callback mechanism

Posted by Roger Pau Monné 1 month, 1 week ago
On Wed, Mar 31, 2021 at 12:47:54PM +0100, Andrew Cooper wrote:
> On 31/03/2021 11:32, Roger Pau Monne wrote:
> > @@ -1620,9 +1666,22 @@ int vlapic_init(struct vcpu *v)
> >  
> >      clear_page(vlapic->regs);
> >  
> > +    vlapic->callbacks = xmalloc_array(typeof(*vlapic->callbacks),
> > +                                      X86_NR_VECTORS - 16);
> > +    if ( !vlapic->callbacks )
> > +    {
> > +        dprintk(XENLOG_ERR, "%pv: alloc vlapic callbacks error\n", v);
> > +        unmap_domain_page_global(vlapic->regs);
> > +        free_domheap_page(vlapic->regs_page);
> > +        return -ENOMEM;
> > +    }
> > +    memset(vlapic->callbacks, 0,
> > +           sizeof(*vlapic->callbacks) * (X86_NR_VECTORS - 16));
> 
> xzalloc_array() instead of memset().  Also, we shouldn't be printing for
> -ENOMEM cases.
> 
> As for the construction/teardown logic, vlapic_init()'s caller already
> vlapic_destory().  Therefore, the existing error path you've copied is
> buggy because it will cause a double-free if __map_domain_page_global()
> fails.

Right, let me adjust that path.

> I'll do a cleanup patch to fix the idempotency, which needs backporting too.

Ack, I don't mind doiing one myself either.

Thanks, Roger.