diff mbox series

[for-4.14,v2,1/2] x86/passthrough: do not assert edge triggered GSIs for PVH dom0

Message ID 20200610142923.9074-2-roger.pau@citrix.com (mailing list archive)
State New, archived
Headers show
Series x86/passthrough: fixes for PVH dom0 edge triggered interrupts | expand

Commit Message

Roger Pau Monné June 10, 2020, 2:29 p.m. UTC 
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(-)

Comments

Paul Durrant June 10, 2020, 2:49 p.m. UTC | #1 
> -----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
Jan Beulich June 16, 2020, 6:11 a.m. UTC | #2 
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;
>      }
>  
>
Roger Pau Monné June 16, 2020, 8:20 a.m. UTC | #3 
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.
diff mbox series

Patch

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;
     }