| Message ID | 20200610115103.7592-2-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/passthrough: fixes for PVH dom0 edge triggered interrupts | expand |
On 10/06/2020 12:51, Roger Pau Monne wrote: > 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>
> -----Original Message----- > From: Roger Pau Monne <roger.pau@citrix.com> > Sent: 10 June 2020 12:51 > 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 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> > --- > 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..9a56543c1b 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 level = 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 || level < 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 ( !level || !hvm_irq->gsi_assert_count[gsi] ) We have definitions of VIOAPIC_EDGE_TRIG and VIOAPIC_LEVEL_TRIG. It seems odd not to use them here. Paul > { > - hvm_irq->gsi_assert_count[gsi] = 1; > + if ( !level ) > + 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 level = vioapic_get_trigger_mode(d, gsi); > struct hvm_irq *hvm_irq = hvm_domain_irq(d); > > - if ( gsi >= hvm_irq->nr_gsis ) > + if ( level <= 0 || gsi >= hvm_irq->nr_gsis ) > { > - ASSERT_UNREACHABLE(); > + ASSERT(level == 0 && gsi < hvm_irq->nr_gsis); > return; > } > > -- > 2.26.2
On Wed, Jun 10, 2020 at 01:33:46PM +0100, Paul Durrant wrote: > > -----Original Message----- > > From: Roger Pau Monne <roger.pau@citrix.com> > > Sent: 10 June 2020 12:51 > > 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 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> > > --- > > 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..9a56543c1b 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 level = 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 || level < 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 ( !level || !hvm_irq->gsi_assert_count[gsi] ) > > We have definitions of VIOAPIC_EDGE_TRIG and VIOAPIC_LEVEL_TRIG. It seems odd not to use them here. Urg, completely forgot about those. I think there are many places where the triggering is treated as a boolean. I will rename the local variable to 'trig' and compare against VIOAPIC_EDGE_TRIG. Thanks, Roger.
diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c index 9c8adbc495..9a56543c1b 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 level = 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 || level < 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 ( !level || !hvm_irq->gsi_assert_count[gsi] ) { - hvm_irq->gsi_assert_count[gsi] = 1; + if ( !level ) + 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 level = vioapic_get_trigger_mode(d, gsi); struct hvm_irq *hvm_irq = hvm_domain_irq(d); - if ( gsi >= hvm_irq->nr_gsis ) + if ( level <= 0 || gsi >= hvm_irq->nr_gsis ) { - ASSERT_UNREACHABLE(); + ASSERT(level == 0 && gsi < hvm_irq->nr_gsis); return; }
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> --- xen/arch/x86/hvm/irq.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)