| Message ID | 20200930104108.35969-4-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/intr: introduce EOI callbacks and fix vPT | expand |
> -----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 >
On 30.09.2020 13:49, Paul Durrant wrote: >> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Roger Pau Monne >> Sent: 30 September 2020 11:41 >> >> @@ -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? Yes, but I take it the assumption is that it'll always be the same callback that ought to get set here. Hence the warning printk() in that function in case it isn't. What I wonder while looking at this function is whether the TMR handling is correct. The SDM says "Upon acceptance of an interrupt into the IRR, ..." which I read as "when the IRR bit transitions from 0 to 1" (but I can see room for reading this differently). Jan
On 30.09.2020 12:41, 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. On v1 I did ask "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." Afaics I didn't get an answer on this. > --- > 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. I guess this can't be decided at this point of the series, as it may depend on what exactly the callbacks mean to do. It may even be that whether a callback wants to do something depends on whether it gets called "normally" or from vlapic_do_init(). > --- 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; Should "data" perhaps also be compared in the override check above? > @@ -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)); While it resembles code earlier in this function, it widens an existing memory leak (I'll make a patch for that one) and also makes it appear as if this function could be called more than once for a vCPU (maybe I'll also make a patch for this). Jan
On Fri, Oct 02, 2020 at 11:39:50AM +0200, Jan Beulich wrote: > On 30.09.2020 12:41, 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. > > On v1 I did ask > > "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." > > Afaics I didn't get an answer on this. Oh sorry, I remember your comment and I've changed further patches to address this. The setter of the callback will be in charge for setting the callback again on resume. That's why vlapic_set_callback is not a static function, and is added to the header. Patch 5/11 [0] contains an example of hw the vIO-APIC resets the callbacks on load. > > > --- > > 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. > > I guess this can't be decided at this point of the series, as it > may depend on what exactly the callbacks mean to do. It may even > be that whether a callback wants to do something depends on > whether it gets called "normally" or from vlapic_do_init(). Well, lets try to get some progress on the other questions and we will eventually get here I think. > > --- 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; > > Should "data" perhaps also be compared in the override check above? Could do, there might indeed be cases where the callback is the same but data has changed and should be interesting to log. Thanks, Roger. [0] https://lore.kernel.org/xen-devel/20200930104108.35969-6-roger.pau@citrix.com/
On 13.10.2020 16:30, Roger Pau Monné wrote: > On Fri, Oct 02, 2020 at 11:39:50AM +0200, Jan Beulich wrote: >> On 30.09.2020 12:41, 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. >> >> On v1 I did ask >> >> "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." >> >> Afaics I didn't get an answer on this. > > Oh sorry, I remember your comment and I've changed further patches to > address this. > > The setter of the callback will be in charge for setting the callback > again on resume. That's why vlapic_set_callback is not a static > function, and is added to the header. > > Patch 5/11 [0] contains an example of hw the vIO-APIC resets the callbacks > on load. Ah, I see - I didn't get there yet. Could you mention this behavior in the description here, or maybe in a code comment next to the declaration (or definition) of the function? Jan
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); + 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);
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> --- 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(-)