* [PATCH for-4.14 v2 0/2] x86/passthrough: fixes for PVH dom0 edge triggered interrupts @ 2020-06-10 14:29 Roger Pau Monne 2020-06-10 14:29 ` [PATCH for-4.14 v2 1/2] x86/passthrough: do not assert edge triggered GSIs for PVH dom0 Roger Pau Monne 2020-06-10 14:29 ` [PATCH for-4.14 v2 2/2] x86/passthrough: introduce a flag for GSIs not requiring an EOI or unmask Roger Pau Monne 0 siblings, 2 replies; 16+ messages in thread From: Roger Pau Monne @ 2020-06-10 14:29 UTC (permalink / raw) To: xen-devel; +Cc: Andrew Cooper, Roger Pau Monne, Wei Liu, Jan Beulich, paul Hello, Small series with two bugfixes to correctly handle edge triggered interrupts on PVH dom0. for-4.14 reasoning: fixes are isolated to PVH dom0 specific passthrough code (IDENTITY_GSI kind of bindings), and hence shouldn't affect passthrough to HVM domUs. Without these fixes the RTC timer won't work correctly on a PVH dom0 because it's edge triggered (GSI 8). Roger Pau Monne (2): x86/passthrough: do not assert edge triggered GSIs for PVH dom0 x86/passthrough: introduce a flag for GSIs not requiring an EOI or unmask xen/arch/x86/hvm/irq.c | 13 ++++++++----- xen/drivers/passthrough/io.c | 24 +++++++++++++++--------- xen/include/asm-x86/hvm/irq.h | 2 ++ 3 files changed, 25 insertions(+), 14 deletions(-) -- 2.26.2 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH for-4.14 v2 1/2] x86/passthrough: do not assert edge triggered GSIs for PVH dom0 2020-06-10 14:29 [PATCH for-4.14 v2 0/2] x86/passthrough: fixes for PVH dom0 edge triggered interrupts Roger Pau Monne @ 2020-06-10 14:29 ` Roger Pau Monne 2020-06-10 14:49 ` Paul Durrant 2020-06-16 6:11 ` Jan Beulich 2020-06-10 14:29 ` [PATCH for-4.14 v2 2/2] x86/passthrough: introduce a flag for GSIs not requiring an EOI or unmask Roger Pau Monne 1 sibling, 2 replies; 16+ messages in thread From: Roger Pau Monne @ 2020-06-10 14:29 UTC (permalink / raw) To: xen-devel; +Cc: Andrew Cooper, Roger Pau Monne, Wei Liu, Jan Beulich, paul Edge triggered interrupts do not assert the line, so the handling done in Xen should also avoid asserting it. Asserting the line prevents further edge triggered interrupts on the same vIO-APIC pin from being delivered, since the line is not de-asserted. One case of such kind of interrupt is the RTC timer, which is edge triggered and available to a PVH dom0. Note this should not affect domUs, as it only modifies the behavior of IDENTITY_GSI kind of passed through interrupts. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> --- Changes since v1: - Compare the triggering against VIOAPIC_{EDGE/LEVEL}_TRIG. --- xen/arch/x86/hvm/irq.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c index 9c8adbc495..fd02cf2e8d 100644 --- a/xen/arch/x86/hvm/irq.c +++ b/xen/arch/x86/hvm/irq.c @@ -169,9 +169,10 @@ void hvm_pci_intx_deassert( void hvm_gsi_assert(struct domain *d, unsigned int gsi) { + int trig = vioapic_get_trigger_mode(d, gsi); struct hvm_irq *hvm_irq = hvm_domain_irq(d); - if ( gsi >= hvm_irq->nr_gsis ) + if ( gsi >= hvm_irq->nr_gsis || trig < 0 ) { ASSERT_UNREACHABLE(); return; @@ -186,9 +187,10 @@ void hvm_gsi_assert(struct domain *d, unsigned int gsi) * to know if the GSI is pending or not. */ spin_lock(&d->arch.hvm.irq_lock); - if ( !hvm_irq->gsi_assert_count[gsi] ) + if ( trig == VIOAPIC_EDGE_TRIG || !hvm_irq->gsi_assert_count[gsi] ) { - hvm_irq->gsi_assert_count[gsi] = 1; + if ( trig == VIOAPIC_LEVEL_TRIG ) + hvm_irq->gsi_assert_count[gsi] = 1; assert_gsi(d, gsi); } spin_unlock(&d->arch.hvm.irq_lock); @@ -196,11 +198,12 @@ void hvm_gsi_assert(struct domain *d, unsigned int gsi) void hvm_gsi_deassert(struct domain *d, unsigned int gsi) { + int trig = vioapic_get_trigger_mode(d, gsi); struct hvm_irq *hvm_irq = hvm_domain_irq(d); - if ( gsi >= hvm_irq->nr_gsis ) + if ( trig <= VIOAPIC_EDGE_TRIG || gsi >= hvm_irq->nr_gsis ) { - ASSERT_UNREACHABLE(); + ASSERT(trig == VIOAPIC_EDGE_TRIG && gsi < hvm_irq->nr_gsis); return; } -- 2.26.2 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* RE: [PATCH for-4.14 v2 1/2] x86/passthrough: do not assert edge triggered GSIs for PVH dom0 2020-06-10 14:29 ` [PATCH for-4.14 v2 1/2] x86/passthrough: do not assert edge triggered GSIs for PVH dom0 Roger Pau Monne @ 2020-06-10 14:49 ` Paul Durrant 2020-06-16 6:11 ` Jan Beulich 1 sibling, 0 replies; 16+ messages in thread From: Paul Durrant @ 2020-06-10 14:49 UTC (permalink / raw) To: 'Roger Pau Monne', xen-devel Cc: 'Andrew Cooper', 'Wei Liu', 'Jan Beulich' > -----Original Message----- > From: Roger Pau Monne <roger.pau@citrix.com> > Sent: 10 June 2020 15:29 > To: xen-devel@lists.xenproject.org > Cc: paul@xen.org; 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 for-4.14 v2 1/2] x86/passthrough: do not assert edge triggered GSIs for PVH dom0 > > Edge triggered interrupts do not assert the line, so the handling done > in Xen should also avoid asserting it. Asserting the line prevents > further edge triggered interrupts on the same vIO-APIC pin from being > delivered, since the line is not de-asserted. > > One case of such kind of interrupt is the RTC timer, which is edge > triggered and available to a PVH dom0. Note this should not affect > domUs, as it only modifies the behavior of IDENTITY_GSI kind of passed > through interrupts. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Paul Durrant <paul@xen.org> Release-acked-by: Paul Durrant <paul@xen.org> > --- > Changes since v1: > - Compare the triggering against VIOAPIC_{EDGE/LEVEL}_TRIG. > --- > xen/arch/x86/hvm/irq.c | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c > index 9c8adbc495..fd02cf2e8d 100644 > --- a/xen/arch/x86/hvm/irq.c > +++ b/xen/arch/x86/hvm/irq.c > @@ -169,9 +169,10 @@ void hvm_pci_intx_deassert( > > void hvm_gsi_assert(struct domain *d, unsigned int gsi) > { > + int trig = vioapic_get_trigger_mode(d, gsi); > struct hvm_irq *hvm_irq = hvm_domain_irq(d); > > - if ( gsi >= hvm_irq->nr_gsis ) > + if ( gsi >= hvm_irq->nr_gsis || trig < 0 ) > { > ASSERT_UNREACHABLE(); > return; > @@ -186,9 +187,10 @@ void hvm_gsi_assert(struct domain *d, unsigned int gsi) > * to know if the GSI is pending or not. > */ > spin_lock(&d->arch.hvm.irq_lock); > - if ( !hvm_irq->gsi_assert_count[gsi] ) > + if ( trig == VIOAPIC_EDGE_TRIG || !hvm_irq->gsi_assert_count[gsi] ) > { > - hvm_irq->gsi_assert_count[gsi] = 1; > + if ( trig == VIOAPIC_LEVEL_TRIG ) > + hvm_irq->gsi_assert_count[gsi] = 1; > assert_gsi(d, gsi); > } > spin_unlock(&d->arch.hvm.irq_lock); > @@ -196,11 +198,12 @@ void hvm_gsi_assert(struct domain *d, unsigned int gsi) > > void hvm_gsi_deassert(struct domain *d, unsigned int gsi) > { > + int trig = vioapic_get_trigger_mode(d, gsi); > struct hvm_irq *hvm_irq = hvm_domain_irq(d); > > - if ( gsi >= hvm_irq->nr_gsis ) > + if ( trig <= VIOAPIC_EDGE_TRIG || gsi >= hvm_irq->nr_gsis ) > { > - ASSERT_UNREACHABLE(); > + ASSERT(trig == VIOAPIC_EDGE_TRIG && gsi < hvm_irq->nr_gsis); > return; > } > > -- > 2.26.2 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH for-4.14 v2 1/2] x86/passthrough: do not assert edge triggered GSIs for PVH dom0 2020-06-10 14:29 ` [PATCH for-4.14 v2 1/2] x86/passthrough: do not assert edge triggered GSIs for PVH dom0 Roger Pau Monne 2020-06-10 14:49 ` Paul Durrant @ 2020-06-16 6:11 ` Jan Beulich 2020-06-16 8:20 ` Roger Pau Monné 1 sibling, 1 reply; 16+ messages in thread From: Jan Beulich @ 2020-06-16 6:11 UTC (permalink / raw) To: Roger Pau Monne; +Cc: xen-devel, Andrew Cooper, Wei Liu, paul On 10.06.2020 16:29, Roger Pau Monne wrote: > @@ -186,9 +187,10 @@ void hvm_gsi_assert(struct domain *d, unsigned int gsi) > * to know if the GSI is pending or not. > */ > spin_lock(&d->arch.hvm.irq_lock); > - if ( !hvm_irq->gsi_assert_count[gsi] ) > + if ( trig == VIOAPIC_EDGE_TRIG || !hvm_irq->gsi_assert_count[gsi] ) > { > - hvm_irq->gsi_assert_count[gsi] = 1; > + if ( trig == VIOAPIC_LEVEL_TRIG ) > + hvm_irq->gsi_assert_count[gsi] = 1; Btw, along the lines of how you do things here, I think ... > @@ -196,11 +198,12 @@ void hvm_gsi_assert(struct domain *d, unsigned int gsi) > > void hvm_gsi_deassert(struct domain *d, unsigned int gsi) > { > + int trig = vioapic_get_trigger_mode(d, gsi); > struct hvm_irq *hvm_irq = hvm_domain_irq(d); > > - if ( gsi >= hvm_irq->nr_gsis ) > + if ( trig <= VIOAPIC_EDGE_TRIG || gsi >= hvm_irq->nr_gsis ) ... this would better have been "trig != VIOAPIC_LEVEL_TRIG", to avoid the code being dependent upon the actual values of both VIOAPIC_*_TRIG constants. Jan > - ASSERT_UNREACHABLE(); > + ASSERT(trig == VIOAPIC_EDGE_TRIG && gsi < hvm_irq->nr_gsis); > return; > } > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH for-4.14 v2 1/2] x86/passthrough: do not assert edge triggered GSIs for PVH dom0 2020-06-16 6:11 ` Jan Beulich @ 2020-06-16 8:20 ` Roger Pau Monné 0 siblings, 0 replies; 16+ messages in thread From: Roger Pau Monné @ 2020-06-16 8:20 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu, paul On Tue, Jun 16, 2020 at 08:11:12AM +0200, Jan Beulich wrote: > On 10.06.2020 16:29, Roger Pau Monne wrote: > > @@ -186,9 +187,10 @@ void hvm_gsi_assert(struct domain *d, unsigned int gsi) > > * to know if the GSI is pending or not. > > */ > > spin_lock(&d->arch.hvm.irq_lock); > > - if ( !hvm_irq->gsi_assert_count[gsi] ) > > + if ( trig == VIOAPIC_EDGE_TRIG || !hvm_irq->gsi_assert_count[gsi] ) > > { > > - hvm_irq->gsi_assert_count[gsi] = 1; > > + if ( trig == VIOAPIC_LEVEL_TRIG ) > > + hvm_irq->gsi_assert_count[gsi] = 1; > > Btw, along the lines of how you do things here, I think ... > > > @@ -196,11 +198,12 @@ void hvm_gsi_assert(struct domain *d, unsigned int gsi) > > > > void hvm_gsi_deassert(struct domain *d, unsigned int gsi) > > { > > + int trig = vioapic_get_trigger_mode(d, gsi); > > struct hvm_irq *hvm_irq = hvm_domain_irq(d); > > > > - if ( gsi >= hvm_irq->nr_gsis ) > > + if ( trig <= VIOAPIC_EDGE_TRIG || gsi >= hvm_irq->nr_gsis ) > > ... this would better have been "trig != VIOAPIC_LEVEL_TRIG", to > avoid the code being dependent upon the actual values of both > VIOAPIC_*_TRIG constants. Sure, let me send a follow up patch, it's trivial to fix. Thanks, Roger. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH for-4.14 v2 2/2] x86/passthrough: introduce a flag for GSIs not requiring an EOI or unmask 2020-06-10 14:29 [PATCH for-4.14 v2 0/2] x86/passthrough: fixes for PVH dom0 edge triggered interrupts Roger Pau Monne 2020-06-10 14:29 ` [PATCH for-4.14 v2 1/2] x86/passthrough: do not assert edge triggered GSIs for PVH dom0 Roger Pau Monne @ 2020-06-10 14:29 ` Roger Pau Monne 2020-06-11 16:26 ` Andrew Cooper ` (3 more replies) 1 sibling, 4 replies; 16+ messages in thread From: Roger Pau Monne @ 2020-06-10 14:29 UTC (permalink / raw) To: xen-devel; +Cc: Andrew Cooper, Roger Pau Monne, Wei Liu, Jan Beulich, paul There's no need to setup a timer for GSIs that are edge triggered, since those don't require any EIO or unmask, and hence couldn't block other interrupts. Note this is only used by PVH dom0, that can setup the passthrough of edge triggered interrupts from the vIO-APIC. One example of such kind of interrupt that can be used by a PVH dom0 would be the RTC timer. While there introduce an out label to do the unlock and reduce code duplication. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes since v1: - Introduce an out label that does the unlock. --- xen/drivers/passthrough/io.c | 24 +++++++++++++++--------- xen/include/asm-x86/hvm/irq.h | 2 ++ 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/xen/drivers/passthrough/io.c b/xen/drivers/passthrough/io.c index b292e79382..6b1305a3e5 100644 --- a/xen/drivers/passthrough/io.c +++ b/xen/drivers/passthrough/io.c @@ -138,7 +138,8 @@ static void pt_pirq_softirq_reset(struct hvm_pirq_dpci *pirq_dpci) bool pt_irq_need_timer(uint32_t flags) { - return !(flags & (HVM_IRQ_DPCI_GUEST_MSI | HVM_IRQ_DPCI_TRANSLATE)); + return !(flags & (HVM_IRQ_DPCI_GUEST_MSI | HVM_IRQ_DPCI_TRANSLATE | + HVM_IRQ_DPCI_NO_EOI)); } static int pt_irq_guest_eoi(struct domain *d, struct hvm_pirq_dpci *pirq_dpci, @@ -558,6 +559,12 @@ int pt_irq_create_bind( */ ASSERT(!mask); share = trigger_mode; + if ( trigger_mode == VIOAPIC_EDGE_TRIG ) + /* + * Edge IO-APIC interrupt, no EOI or unmask to perform + * and hence no timer needed. + */ + pirq_dpci->flags |= HVM_IRQ_DPCI_NO_EOI; } } @@ -897,17 +904,13 @@ static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci) send_guest_pirq(d, pirq); if ( pirq_dpci->flags & HVM_IRQ_DPCI_GUEST_MSI ) - { - spin_unlock(&d->event_lock); - return; - } + goto out; } if ( pirq_dpci->flags & HVM_IRQ_DPCI_GUEST_MSI ) { vmsi_deliver_pirq(d, pirq_dpci); - spin_unlock(&d->event_lock); - return; + goto out; } list_for_each_entry ( digl, &pirq_dpci->digl_list, list ) @@ -920,6 +923,8 @@ static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci) if ( pirq_dpci->flags & HVM_IRQ_DPCI_IDENTITY_GSI ) { hvm_gsi_assert(d, pirq->pirq); + if ( pirq_dpci->flags & HVM_IRQ_DPCI_NO_EOI ) + goto out; pirq_dpci->pending++; } @@ -927,8 +932,7 @@ static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci) { /* for translated MSI to INTx interrupt, eoi as early as possible */ __msi_pirq_eoi(pirq_dpci); - spin_unlock(&d->event_lock); - return; + goto out; } /* @@ -941,6 +945,8 @@ static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci) ASSERT(pt_irq_need_timer(pirq_dpci->flags)); set_timer(&pirq_dpci->timer, NOW() + PT_IRQ_TIME_OUT); } + + out: spin_unlock(&d->event_lock); } diff --git a/xen/include/asm-x86/hvm/irq.h b/xen/include/asm-x86/hvm/irq.h index d306cfeade..532880d497 100644 --- a/xen/include/asm-x86/hvm/irq.h +++ b/xen/include/asm-x86/hvm/irq.h @@ -121,6 +121,7 @@ struct dev_intx_gsi_link { #define _HVM_IRQ_DPCI_GUEST_PCI_SHIFT 4 #define _HVM_IRQ_DPCI_GUEST_MSI_SHIFT 5 #define _HVM_IRQ_DPCI_IDENTITY_GSI_SHIFT 6 +#define _HVM_IRQ_DPCI_NO_EOI_SHIFT 7 #define _HVM_IRQ_DPCI_TRANSLATE_SHIFT 15 #define HVM_IRQ_DPCI_MACH_PCI (1u << _HVM_IRQ_DPCI_MACH_PCI_SHIFT) #define HVM_IRQ_DPCI_MACH_MSI (1u << _HVM_IRQ_DPCI_MACH_MSI_SHIFT) @@ -129,6 +130,7 @@ struct dev_intx_gsi_link { #define HVM_IRQ_DPCI_GUEST_PCI (1u << _HVM_IRQ_DPCI_GUEST_PCI_SHIFT) #define HVM_IRQ_DPCI_GUEST_MSI (1u << _HVM_IRQ_DPCI_GUEST_MSI_SHIFT) #define HVM_IRQ_DPCI_IDENTITY_GSI (1u << _HVM_IRQ_DPCI_IDENTITY_GSI_SHIFT) +#define HVM_IRQ_DPCI_NO_EOI (1u << _HVM_IRQ_DPCI_NO_EOI_SHIFT) #define HVM_IRQ_DPCI_TRANSLATE (1u << _HVM_IRQ_DPCI_TRANSLATE_SHIFT) struct hvm_gmsi_info { -- 2.26.2 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH for-4.14 v2 2/2] x86/passthrough: introduce a flag for GSIs not requiring an EOI or unmask 2020-06-10 14:29 ` [PATCH for-4.14 v2 2/2] x86/passthrough: introduce a flag for GSIs not requiring an EOI or unmask Roger Pau Monne @ 2020-06-11 16:26 ` Andrew Cooper 2020-06-11 17:11 ` Paul Durrant 2020-06-15 17:06 ` Paul Durrant ` (2 subsequent siblings) 3 siblings, 1 reply; 16+ messages in thread From: Andrew Cooper @ 2020-06-11 16:26 UTC (permalink / raw) To: Roger Pau Monne, xen-devel; +Cc: Wei Liu, Jan Beulich, paul On 10/06/2020 15:29, Roger Pau Monne wrote: > There's no need to setup a timer for GSIs that are edge triggered, > since those don't require any EIO or unmask, and hence couldn't block > other interrupts. > > Note this is only used by PVH dom0, that can setup the passthrough of > edge triggered interrupts from the vIO-APIC. One example of such kind > of interrupt that can be used by a PVH dom0 would be the RTC timer. > > While there introduce an out label to do the unlock and reduce code > duplication. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH for-4.14 v2 2/2] x86/passthrough: introduce a flag for GSIs not requiring an EOI or unmask 2020-06-11 16:26 ` Andrew Cooper @ 2020-06-11 17:11 ` Paul Durrant 2020-06-15 16:17 ` Jan Beulich 0 siblings, 1 reply; 16+ messages in thread From: Paul Durrant @ 2020-06-11 17:11 UTC (permalink / raw) To: 'Andrew Cooper', 'Roger Pau Monne', xen-devel Cc: 'Wei Liu', 'Jan Beulich' > -----Original Message----- > From: Andrew Cooper <andrew.cooper3@citrix.com> > Sent: 11 June 2020 17:26 > To: Roger Pau Monne <roger.pau@citrix.com>; xen-devel@lists.xenproject.org > Cc: paul@xen.org; Jan Beulich <jbeulich@suse.com>; Wei Liu <wl@xen.org> > Subject: Re: [PATCH for-4.14 v2 2/2] x86/passthrough: introduce a flag for GSIs not requiring an EOI > or unmask > > On 10/06/2020 15:29, Roger Pau Monne wrote: > > There's no need to setup a timer for GSIs that are edge triggered, > > since those don't require any EIO or unmask, and hence couldn't block > > other interrupts. > > > > Note this is only used by PVH dom0, that can setup the passthrough of > > edge triggered interrupts from the vIO-APIC. One example of such kind > > of interrupt that can be used by a PVH dom0 would be the RTC timer. > > > > While there introduce an out label to do the unlock and reduce code > > duplication. > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> Release-acked-by: Paul Durrant <paul@xen.org> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH for-4.14 v2 2/2] x86/passthrough: introduce a flag for GSIs not requiring an EOI or unmask 2020-06-11 17:11 ` Paul Durrant @ 2020-06-15 16:17 ` Jan Beulich 2020-06-15 17:07 ` Paul Durrant 0 siblings, 1 reply; 16+ messages in thread From: Jan Beulich @ 2020-06-15 16:17 UTC (permalink / raw) To: paul, 'Andrew Cooper' Cc: xen-devel, 'Wei Liu', 'Roger Pau Monne' On 11.06.2020 19:11, Paul Durrant wrote: >> -----Original Message----- >> From: Andrew Cooper <andrew.cooper3@citrix.com> >> Sent: 11 June 2020 17:26 >> To: Roger Pau Monne <roger.pau@citrix.com>; xen-devel@lists.xenproject.org >> Cc: paul@xen.org; Jan Beulich <jbeulich@suse.com>; Wei Liu <wl@xen.org> >> Subject: Re: [PATCH for-4.14 v2 2/2] x86/passthrough: introduce a flag for GSIs not requiring an EOI >> or unmask >> >> On 10/06/2020 15:29, Roger Pau Monne wrote: >>> There's no need to setup a timer for GSIs that are edge triggered, >>> since those don't require any EIO or unmask, and hence couldn't block >>> other interrupts. >>> >>> Note this is only used by PVH dom0, that can setup the passthrough of >>> edge triggered interrupts from the vIO-APIC. One example of such kind >>> of interrupt that can be used by a PVH dom0 would be the RTC timer. >>> >>> While there introduce an out label to do the unlock and reduce code >>> duplication. >>> >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >> >> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> > > Release-acked-by: Paul Durrant <paul@xen.org> Strictly speaking these tags were too little for the patch to go in - the change to drivers/passthrough/io.c would also have required Paul's (or my) R-b. I take it that this was sort of implied by the R-a-b. Jan ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH for-4.14 v2 2/2] x86/passthrough: introduce a flag for GSIs not requiring an EOI or unmask 2020-06-15 16:17 ` Jan Beulich @ 2020-06-15 17:07 ` Paul Durrant 0 siblings, 0 replies; 16+ messages in thread From: Paul Durrant @ 2020-06-15 17:07 UTC (permalink / raw) To: 'Jan Beulich', 'Andrew Cooper' Cc: xen-devel, 'Wei Liu', 'Roger Pau Monne' > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: 15 June 2020 17:17 > To: paul@xen.org; 'Andrew Cooper' <andrew.cooper3@citrix.com> > Cc: 'Roger Pau Monne' <roger.pau@citrix.com>; xen-devel@lists.xenproject.org; 'Wei Liu' <wl@xen.org> > Subject: Re: [PATCH for-4.14 v2 2/2] x86/passthrough: introduce a flag for GSIs not requiring an EOI > or unmask > > On 11.06.2020 19:11, Paul Durrant wrote: > >> -----Original Message----- > >> From: Andrew Cooper <andrew.cooper3@citrix.com> > >> Sent: 11 June 2020 17:26 > >> To: Roger Pau Monne <roger.pau@citrix.com>; xen-devel@lists.xenproject.org > >> Cc: paul@xen.org; Jan Beulich <jbeulich@suse.com>; Wei Liu <wl@xen.org> > >> Subject: Re: [PATCH for-4.14 v2 2/2] x86/passthrough: introduce a flag for GSIs not requiring an > EOI > >> or unmask > >> > >> On 10/06/2020 15:29, Roger Pau Monne wrote: > >>> There's no need to setup a timer for GSIs that are edge triggered, > >>> since those don't require any EIO or unmask, and hence couldn't block > >>> other interrupts. > >>> > >>> Note this is only used by PVH dom0, that can setup the passthrough of > >>> edge triggered interrupts from the vIO-APIC. One example of such kind > >>> of interrupt that can be used by a PVH dom0 would be the RTC timer. > >>> > >>> While there introduce an out label to do the unlock and reduce code > >>> duplication. > >>> > >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > >> > >> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> > > > > Release-acked-by: Paul Durrant <paul@xen.org> > > Strictly speaking these tags were too little for the patch to go > in - the change to drivers/passthrough/io.c would also have > required Paul's (or my) R-b. I take it that this was sort of > implied by the R-a-b. FAOD I have added my R-b. Paul > > Jan ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH for-4.14 v2 2/2] x86/passthrough: introduce a flag for GSIs not requiring an EOI or unmask 2020-06-10 14:29 ` [PATCH for-4.14 v2 2/2] x86/passthrough: introduce a flag for GSIs not requiring an EOI or unmask Roger Pau Monne 2020-06-11 16:26 ` Andrew Cooper @ 2020-06-15 17:06 ` Paul Durrant 2020-06-16 6:27 ` Jan Beulich 2020-06-26 10:44 ` Wei Liu 3 siblings, 0 replies; 16+ messages in thread From: Paul Durrant @ 2020-06-15 17:06 UTC (permalink / raw) To: 'Roger Pau Monne', xen-devel Cc: 'Andrew Cooper', 'Wei Liu', 'Jan Beulich' > -----Original Message----- > From: Roger Pau Monne <roger.pau@citrix.com> > Sent: 10 June 2020 15:29 > To: xen-devel@lists.xenproject.org > Cc: paul@xen.org; 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 for-4.14 v2 2/2] x86/passthrough: introduce a flag for GSIs not requiring an EOI or > unmask > > There's no need to setup a timer for GSIs that are edge triggered, > since those don't require any EIO or unmask, and hence couldn't block > other interrupts. > > Note this is only used by PVH dom0, that can setup the passthrough of > edge triggered interrupts from the vIO-APIC. One example of such kind > of interrupt that can be used by a PVH dom0 would be the RTC timer. > > While there introduce an out label to do the unlock and reduce code > duplication. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Reviewed-by: Paul Durrant <paul@xen.org> > --- > Changes since v1: > - Introduce an out label that does the unlock. > --- > xen/drivers/passthrough/io.c | 24 +++++++++++++++--------- > xen/include/asm-x86/hvm/irq.h | 2 ++ > 2 files changed, 17 insertions(+), 9 deletions(-) > > diff --git a/xen/drivers/passthrough/io.c b/xen/drivers/passthrough/io.c > index b292e79382..6b1305a3e5 100644 > --- a/xen/drivers/passthrough/io.c > +++ b/xen/drivers/passthrough/io.c > @@ -138,7 +138,8 @@ static void pt_pirq_softirq_reset(struct hvm_pirq_dpci *pirq_dpci) > > bool pt_irq_need_timer(uint32_t flags) > { > - return !(flags & (HVM_IRQ_DPCI_GUEST_MSI | HVM_IRQ_DPCI_TRANSLATE)); > + return !(flags & (HVM_IRQ_DPCI_GUEST_MSI | HVM_IRQ_DPCI_TRANSLATE | > + HVM_IRQ_DPCI_NO_EOI)); > } > > static int pt_irq_guest_eoi(struct domain *d, struct hvm_pirq_dpci *pirq_dpci, > @@ -558,6 +559,12 @@ int pt_irq_create_bind( > */ > ASSERT(!mask); > share = trigger_mode; > + if ( trigger_mode == VIOAPIC_EDGE_TRIG ) > + /* > + * Edge IO-APIC interrupt, no EOI or unmask to perform > + * and hence no timer needed. > + */ > + pirq_dpci->flags |= HVM_IRQ_DPCI_NO_EOI; > } > } > > @@ -897,17 +904,13 @@ static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci) > send_guest_pirq(d, pirq); > > if ( pirq_dpci->flags & HVM_IRQ_DPCI_GUEST_MSI ) > - { > - spin_unlock(&d->event_lock); > - return; > - } > + goto out; > } > > if ( pirq_dpci->flags & HVM_IRQ_DPCI_GUEST_MSI ) > { > vmsi_deliver_pirq(d, pirq_dpci); > - spin_unlock(&d->event_lock); > - return; > + goto out; > } > > list_for_each_entry ( digl, &pirq_dpci->digl_list, list ) > @@ -920,6 +923,8 @@ static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci) > if ( pirq_dpci->flags & HVM_IRQ_DPCI_IDENTITY_GSI ) > { > hvm_gsi_assert(d, pirq->pirq); > + if ( pirq_dpci->flags & HVM_IRQ_DPCI_NO_EOI ) > + goto out; > pirq_dpci->pending++; > } > > @@ -927,8 +932,7 @@ static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci) > { > /* for translated MSI to INTx interrupt, eoi as early as possible */ > __msi_pirq_eoi(pirq_dpci); > - spin_unlock(&d->event_lock); > - return; > + goto out; > } > > /* > @@ -941,6 +945,8 @@ static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci) > ASSERT(pt_irq_need_timer(pirq_dpci->flags)); > set_timer(&pirq_dpci->timer, NOW() + PT_IRQ_TIME_OUT); > } > + > + out: > spin_unlock(&d->event_lock); > } > > diff --git a/xen/include/asm-x86/hvm/irq.h b/xen/include/asm-x86/hvm/irq.h > index d306cfeade..532880d497 100644 > --- a/xen/include/asm-x86/hvm/irq.h > +++ b/xen/include/asm-x86/hvm/irq.h > @@ -121,6 +121,7 @@ struct dev_intx_gsi_link { > #define _HVM_IRQ_DPCI_GUEST_PCI_SHIFT 4 > #define _HVM_IRQ_DPCI_GUEST_MSI_SHIFT 5 > #define _HVM_IRQ_DPCI_IDENTITY_GSI_SHIFT 6 > +#define _HVM_IRQ_DPCI_NO_EOI_SHIFT 7 > #define _HVM_IRQ_DPCI_TRANSLATE_SHIFT 15 > #define HVM_IRQ_DPCI_MACH_PCI (1u << _HVM_IRQ_DPCI_MACH_PCI_SHIFT) > #define HVM_IRQ_DPCI_MACH_MSI (1u << _HVM_IRQ_DPCI_MACH_MSI_SHIFT) > @@ -129,6 +130,7 @@ struct dev_intx_gsi_link { > #define HVM_IRQ_DPCI_GUEST_PCI (1u << _HVM_IRQ_DPCI_GUEST_PCI_SHIFT) > #define HVM_IRQ_DPCI_GUEST_MSI (1u << _HVM_IRQ_DPCI_GUEST_MSI_SHIFT) > #define HVM_IRQ_DPCI_IDENTITY_GSI (1u << _HVM_IRQ_DPCI_IDENTITY_GSI_SHIFT) > +#define HVM_IRQ_DPCI_NO_EOI (1u << _HVM_IRQ_DPCI_NO_EOI_SHIFT) > #define HVM_IRQ_DPCI_TRANSLATE (1u << _HVM_IRQ_DPCI_TRANSLATE_SHIFT) > > struct hvm_gmsi_info { > -- > 2.26.2 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH for-4.14 v2 2/2] x86/passthrough: introduce a flag for GSIs not requiring an EOI or unmask 2020-06-10 14:29 ` [PATCH for-4.14 v2 2/2] x86/passthrough: introduce a flag for GSIs not requiring an EOI or unmask Roger Pau Monne 2020-06-11 16:26 ` Andrew Cooper 2020-06-15 17:06 ` Paul Durrant @ 2020-06-16 6:27 ` Jan Beulich 2020-06-16 8:37 ` Roger Pau Monné 2020-06-26 10:44 ` Wei Liu 3 siblings, 1 reply; 16+ messages in thread From: Jan Beulich @ 2020-06-16 6:27 UTC (permalink / raw) To: Roger Pau Monne; +Cc: xen-devel, Andrew Cooper, Wei Liu, paul On 10.06.2020 16:29, Roger Pau Monne wrote: > @@ -558,6 +559,12 @@ int pt_irq_create_bind( > */ > ASSERT(!mask); > share = trigger_mode; > + if ( trigger_mode == VIOAPIC_EDGE_TRIG ) > + /* > + * Edge IO-APIC interrupt, no EOI or unmask to perform > + * and hence no timer needed. > + */ > + pirq_dpci->flags |= HVM_IRQ_DPCI_NO_EOI; Is this really limited to edge triggered IO-APIC interrupts? MSI ones are effectively edge triggered too, aren't they? Along the lines of irq_acktype() maskable MSI may then also not need any such arrangements? The pirq_guest_eoi() -> desc_guest_eoi() path looks to confirm this. > @@ -920,6 +923,8 @@ static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci) > if ( pirq_dpci->flags & HVM_IRQ_DPCI_IDENTITY_GSI ) > { > hvm_gsi_assert(d, pirq->pirq); > + if ( pirq_dpci->flags & HVM_IRQ_DPCI_NO_EOI ) > + goto out; Immediately ahead of this there's a similar piece of code dealing with PCI INTx. They're commonly level triggered, but I don't think there's a strict need for this to be the case. At least hvm_pci_intx_assert() -> assert_gsi() -> vioapic_irq_positive_edge() also cover the edge triggered one. Jan ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH for-4.14 v2 2/2] x86/passthrough: introduce a flag for GSIs not requiring an EOI or unmask 2020-06-16 6:27 ` Jan Beulich @ 2020-06-16 8:37 ` Roger Pau Monné 2020-06-16 8:45 ` Jan Beulich 0 siblings, 1 reply; 16+ messages in thread From: Roger Pau Monné @ 2020-06-16 8:37 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu, paul On Tue, Jun 16, 2020 at 08:27:54AM +0200, Jan Beulich wrote: > On 10.06.2020 16:29, Roger Pau Monne wrote: > > @@ -558,6 +559,12 @@ int pt_irq_create_bind( > > */ > > ASSERT(!mask); > > share = trigger_mode; > > + if ( trigger_mode == VIOAPIC_EDGE_TRIG ) > > + /* > > + * Edge IO-APIC interrupt, no EOI or unmask to perform > > + * and hence no timer needed. > > + */ > > + pirq_dpci->flags |= HVM_IRQ_DPCI_NO_EOI; > > Is this really limited to edge triggered IO-APIC interrupts? > MSI ones are effectively edge triggered too, aren't they? MSIs do sometimes require an EOI, depending on whether they can be masked, see irq_acktype. > Along the lines of irq_acktype() maskable MSI may then also > not need any such arrangements? The pirq_guest_eoi() -> > desc_guest_eoi() path looks to confirm this. Yes, that's correct. AFAICT MSI interrupts won't get the EOI timer, since pt_irq_need_timer will return false because the HVM_IRQ_DPCI_GUEST_MSI flag will be set. > > @@ -920,6 +923,8 @@ static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci) > > if ( pirq_dpci->flags & HVM_IRQ_DPCI_IDENTITY_GSI ) > > { > > hvm_gsi_assert(d, pirq->pirq); > > + if ( pirq_dpci->flags & HVM_IRQ_DPCI_NO_EOI ) > > + goto out; > > Immediately ahead of this there's a similar piece of code > dealing with PCI INTx. They're commonly level triggered, but > I don't think there's a strict need for this to be the case. > At least hvm_pci_intx_assert() -> assert_gsi() -> > vioapic_irq_positive_edge() also cover the edge triggered one. Hm, I'm not sure it's safe to passthrough edge triggered IO-APIC interrupts, as Xen will mark those as 'shared' always, and sharing edge interrupts cannot reliably work. In any case the EOI timer is definitely set for those, and needs to be disabled before exiting hvm_dirq_assist. Thanks, Roger. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH for-4.14 v2 2/2] x86/passthrough: introduce a flag for GSIs not requiring an EOI or unmask 2020-06-16 8:37 ` Roger Pau Monné @ 2020-06-16 8:45 ` Jan Beulich 2020-06-16 9:28 ` Roger Pau Monné 0 siblings, 1 reply; 16+ messages in thread From: Jan Beulich @ 2020-06-16 8:45 UTC (permalink / raw) To: Roger Pau Monné; +Cc: xen-devel, Andrew Cooper, Wei Liu, paul On 16.06.2020 10:37, Roger Pau Monné wrote: > On Tue, Jun 16, 2020 at 08:27:54AM +0200, Jan Beulich wrote: >> On 10.06.2020 16:29, Roger Pau Monne wrote: >>> @@ -920,6 +923,8 @@ static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci) >>> if ( pirq_dpci->flags & HVM_IRQ_DPCI_IDENTITY_GSI ) >>> { >>> hvm_gsi_assert(d, pirq->pirq); >>> + if ( pirq_dpci->flags & HVM_IRQ_DPCI_NO_EOI ) >>> + goto out; >> >> Immediately ahead of this there's a similar piece of code >> dealing with PCI INTx. They're commonly level triggered, but >> I don't think there's a strict need for this to be the case. >> At least hvm_pci_intx_assert() -> assert_gsi() -> >> vioapic_irq_positive_edge() also cover the edge triggered one. > > Hm, I'm not sure it's safe to passthrough edge triggered IO-APIC > interrupts, as Xen will mark those as 'shared' always, and sharing > edge interrupts cannot reliably work. In any case the EOI timer is > definitely set for those, and needs to be disabled before exiting > hvm_dirq_assist. That's the if ( !is_hardware_domain(d) ) share = BIND_PIRQ__WILL_SHARE; in pt_irq_create_bind() aiui? I wonder why we have that ... At a guess it's to accommodate pciback in Dom0 also registering a handler. But wasn't it XenoLinux'es pciback only that does so, and upstream's doesn't? Jan ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH for-4.14 v2 2/2] x86/passthrough: introduce a flag for GSIs not requiring an EOI or unmask 2020-06-16 8:45 ` Jan Beulich @ 2020-06-16 9:28 ` Roger Pau Monné 0 siblings, 0 replies; 16+ messages in thread From: Roger Pau Monné @ 2020-06-16 9:28 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu, paul On Tue, Jun 16, 2020 at 10:45:51AM +0200, Jan Beulich wrote: > On 16.06.2020 10:37, Roger Pau Monné wrote: > > On Tue, Jun 16, 2020 at 08:27:54AM +0200, Jan Beulich wrote: > >> On 10.06.2020 16:29, Roger Pau Monne wrote: > >>> @@ -920,6 +923,8 @@ static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci) > >>> if ( pirq_dpci->flags & HVM_IRQ_DPCI_IDENTITY_GSI ) > >>> { > >>> hvm_gsi_assert(d, pirq->pirq); > >>> + if ( pirq_dpci->flags & HVM_IRQ_DPCI_NO_EOI ) > >>> + goto out; > >> > >> Immediately ahead of this there's a similar piece of code > >> dealing with PCI INTx. They're commonly level triggered, but > >> I don't think there's a strict need for this to be the case. > >> At least hvm_pci_intx_assert() -> assert_gsi() -> > >> vioapic_irq_positive_edge() also cover the edge triggered one. > > > > Hm, I'm not sure it's safe to passthrough edge triggered IO-APIC > > interrupts, as Xen will mark those as 'shared' always, and sharing > > edge interrupts cannot reliably work. In any case the EOI timer is > > definitely set for those, and needs to be disabled before exiting > > hvm_dirq_assist. > > That's the > > if ( !is_hardware_domain(d) ) > share = BIND_PIRQ__WILL_SHARE; > > in pt_irq_create_bind() aiui? I wonder why we have that ... At a > guess it's to accommodate pciback in Dom0 also registering a handler. > But wasn't it XenoLinux'es pciback only that does so, and upstream's > doesn't? I'm not that familiar with pciback in Linux. I've taken a look and AFAICT modern Linux kernels will register a handler for the PCI interrupts before doing a device reset and when dealing with PV guests that use the pciif protocol (note that such IRQ is also unconditionally marked as shared in Linux). It might be safe for HVM domU passed through interrupts to set the share bit based on the triggering mode. Roger. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH for-4.14 v2 2/2] x86/passthrough: introduce a flag for GSIs not requiring an EOI or unmask 2020-06-10 14:29 ` [PATCH for-4.14 v2 2/2] x86/passthrough: introduce a flag for GSIs not requiring an EOI or unmask Roger Pau Monne ` (2 preceding siblings ...) 2020-06-16 6:27 ` Jan Beulich @ 2020-06-26 10:44 ` Wei Liu 3 siblings, 0 replies; 16+ messages in thread From: Wei Liu @ 2020-06-26 10:44 UTC (permalink / raw) To: Roger Pau Monne; +Cc: xen-devel, Andrew Cooper, Wei Liu, Jan Beulich, paul On Wed, Jun 10, 2020 at 04:29:23PM +0200, Roger Pau Monne wrote: > There's no need to setup a timer for GSIs that are edge triggered, > since those don't require any EIO or unmask, and hence couldn't block One small nit. I think you meant "EOI" here. ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2020-06-26 10:44 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-10 14:29 [PATCH for-4.14 v2 0/2] x86/passthrough: fixes for PVH dom0 edge triggered interrupts Roger Pau Monne 2020-06-10 14:29 ` [PATCH for-4.14 v2 1/2] x86/passthrough: do not assert edge triggered GSIs for PVH dom0 Roger Pau Monne 2020-06-10 14:49 ` Paul Durrant 2020-06-16 6:11 ` Jan Beulich 2020-06-16 8:20 ` Roger Pau Monné 2020-06-10 14:29 ` [PATCH for-4.14 v2 2/2] x86/passthrough: introduce a flag for GSIs not requiring an EOI or unmask Roger Pau Monne 2020-06-11 16:26 ` Andrew Cooper 2020-06-11 17:11 ` Paul Durrant 2020-06-15 16:17 ` Jan Beulich 2020-06-15 17:07 ` Paul Durrant 2020-06-15 17:06 ` Paul Durrant 2020-06-16 6:27 ` Jan Beulich 2020-06-16 8:37 ` Roger Pau Monné 2020-06-16 8:45 ` Jan Beulich 2020-06-16 9:28 ` Roger Pau Monné 2020-06-26 10:44 ` Wei Liu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).