[PATCH for-4.14 0/2] x86/passthrough: fixes for PVH dom0 edge triggered interrupts

[PATCH for-4.14 0/2] x86/passthrough: fixes for PVH dom0 edge triggered interrupts

[Roger Pau Monne]

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  | 14 +++++++++++++-
 xen/include/asm-x86/hvm/irq.h |  2 ++
 3 files changed, 23 insertions(+), 6 deletions(-)

-- 
2.26.2

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

[Roger Pau Monne]

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] )
     {
-        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

[PATCH for-4.14 2/2] x86/passthrough: introduce a flag for GSIs not requiring an EOI or unmask

[Roger Pau Monne]

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.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/drivers/passthrough/io.c  | 14 +++++++++++++-
 xen/include/asm-x86/hvm/irq.h |  2 ++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/xen/drivers/passthrough/io.c b/xen/drivers/passthrough/io.c
index b292e79382..be1d5b1434 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 )
+                        /*
+                         * Edge IO-APIC interrupt, no EOI or unmask to perform
+                         * and hence no timer needed.
+                         */
+                        pirq_dpci->flags |= HVM_IRQ_DPCI_NO_EOI;
                 }
             }
 
@@ -920,6 +927,11 @@ 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 )
+            {
+                spin_unlock(&d->event_lock);
+                return;
+            }
             pirq_dpci->pending++;
         }
 
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

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

[Andrew Cooper]

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>

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

[Paul Durrant]

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

Re: [PATCH for-4.14 2/2] x86/passthrough: introduce a flag for GSIs not requiring an EOI or unmask

[Andrew Cooper]

On 10/06/2020 12:51, Roger Pau Monne wrote:
> @@ -920,6 +927,11 @@ 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 )
> +            {
> +                spin_unlock(&d->event_lock);
> +                return;
> +            }

Urgh.  Could I possibly talk you into fixing hvm_dirq_assist() to have a
"goto out;" and a single unlock path ?  (How far are you expecting this
to be backported?)

I'm also totally unconvinced that the atomic test_and_clear() needs to
be done with the event lock held (it should either be non-atomic, or the
locking should be inside the if() condition), but that is probably not a
can of worms wanting opening right now...

~Andrew

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

[Roger Pau Monné]

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.

Re: [PATCH for-4.14 2/2] x86/passthrough: introduce a flag for GSIs not requiring an EOI or unmask

[Roger Pau Monné]

On Wed, Jun 10, 2020 at 01:37:15PM +0100, Andrew Cooper wrote:
> On 10/06/2020 12:51, Roger Pau Monne wrote:
> > @@ -920,6 +927,11 @@ 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 )
> > +            {
> > +                spin_unlock(&d->event_lock);
> > +                return;
> > +            }
> 
> Urgh.  Could I possibly talk you into fixing hvm_dirq_assist() to have a
> "goto out;" and a single unlock path ?  (How far are you expecting this
> to be backported?)

I was very tempted to go that way but didn't want to introduce more
churn. Since you agree I will do so.

> I'm also totally unconvinced that the atomic test_and_clear() needs to
> be done with the event lock held (it should either be non-atomic, or the
> locking should be inside the if() condition), but that is probably not a
> can of worms wanting opening right now...

There's some reasoning about all this in 104072fc1c7e6ed. I also think
naming it masked is confusing, since the underlying interrupt might
not be masked. Anyway, this seems like something I don't really want
to get into now, as it seems quite fragile.

Thanks, Roger.