[PATCH v3 0/6] x86/intr: HVM guest interrupt handling fixes/cleanup

[PATCH v3 0/6] x86/intr: HVM guest interrupt handling fixes/cleanup

[Roger Pau Monne]

Hello,

The following series aims to fix some shortcomings of guest interrupt
handling when using passthrough devices. The first 5 patches are
bugfixes or cleanups, which I think should solve the issue(s) that
caused the dpci EOI timer to be introduced. However neither me nor
others seem to be able to reproduce the original issue, so it's hard to
tell.

It's my opinion that we should remove the timer and see what explodes
(if anything). That's the only way we will be able to figure out what
the original issue was, and how to fix it without introducing yet
another per-guest-irq related timer.

Thanks, Roger.

Roger Pau Monne (6):
  x86/vioapic: top word redir entry writes don't trigger interrupts
  x86/vioapic: issue EOI to dpci when switching pin to edge trigger mode
  x86/vpic: force int output to low when in init mode
  x86/vpic: don't trigger unmask event until end of init
  x86/vpic: issue dpci EOI for cleared pins at ICW1
  x86/dpci: remove the dpci EOI timer

 xen/arch/x86/hvm/vioapic.c            | 19 ++++++
 xen/arch/x86/hvm/vpic.c               | 36 ++++++++--
 xen/drivers/passthrough/vtd/x86/hvm.c |  3 -
 xen/drivers/passthrough/x86/hvm.c     | 95 +--------------------------
 xen/include/asm-x86/hvm/irq.h         |  3 -
 xen/include/xen/iommu.h               |  5 --
 6 files changed, 52 insertions(+), 109 deletions(-)

-- 
2.29.2

[PATCH v3 1/6] x86/vioapic: top word redir entry writes don't trigger interrupts

[Roger Pau Monne]

Top word writes just update the destination of the interrupt, but
since there's no change on the masking or the triggering mode no
guest interrupt injection can result of such write. Add an assert to
that effect.

Requested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v2:
 - New in this version.
---
 xen/arch/x86/hvm/vioapic.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
index 804bc77279..e3ee747b7d 100644
--- a/xen/arch/x86/hvm/vioapic.c
+++ b/xen/arch/x86/hvm/vioapic.c
@@ -262,6 +262,8 @@ static void vioapic_write_redirent(
               !ent.fields.remote_irr &&
               hvm_irq->gsi_assert_count[gsi] )
     {
+        /* A top word write should never trigger an interrupt injection. */
+        ASSERT(!top_word);
         pent->fields.remote_irr = 1;
         vioapic_deliver(vioapic, idx);
     }
-- 
2.29.2

[PATCH v3 2/6] x86/vioapic: issue EOI to dpci when switching pin to edge trigger mode

[Roger Pau Monne]

When an IO-APIC pin is switched from level to edge trigger mode the
IRR bit is cleared, so it can be used as a way to EOI an interrupt at
the IO-APIC level.

Such EOI however does not get forwarded to the dpci code like it's
done for the local APIC initiated EOI. This change adds the code in
order to notify dpci of such EOI, so that dpci and the interrupt
controller are in sync.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v2:
 - Fix comment message missing 'edge'.
 - Add asserts that previous triggering mode was level and it's not a
   top word write.
---
 xen/arch/x86/hvm/vioapic.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
index e3ee747b7d..87370dd417 100644
--- a/xen/arch/x86/hvm/vioapic.c
+++ b/xen/arch/x86/hvm/vioapic.c
@@ -219,6 +219,7 @@ static void vioapic_write_redirent(
     struct domain *d = vioapic_domain(vioapic);
     struct hvm_irq *hvm_irq = hvm_domain_irq(d);
     union vioapic_redir_entry *pent, ent;
+    bool prev_level;
     int unmasked = 0;
     unsigned int gsi;
 
@@ -234,6 +235,7 @@ static void vioapic_write_redirent(
 
     pent = &vioapic->redirtbl[idx];
     ent  = *pent;
+    prev_level = ent.fields.trig_mode == VIOAPIC_LEVEL_TRIG;
 
     if ( top_word )
     {
@@ -270,6 +272,21 @@ static void vioapic_write_redirent(
 
     spin_unlock(&d->arch.hvm.irq_lock);
 
+    if ( ent.fields.trig_mode == VIOAPIC_EDGE_TRIG &&
+         ent.fields.remote_irr && is_iommu_enabled(d) )
+    {
+            /*
+             * Since IRR has been cleared and further interrupts can be
+             * injected also attempt to deassert any virtual line of passed
+             * through devices using this pin. Switching a pin from level to
+             * edge trigger mode can be used as a way to EOI an interrupt at
+             * the IO-APIC level.
+             */
+            ASSERT(prev_level);
+            ASSERT(!top_word);
+            hvm_dpci_eoi(d, gsi);
+    }
+
     if ( is_hardware_domain(d) && unmasked )
     {
         /*
-- 
2.29.2

[PATCH v3 3/6] x86/vpic: force int output to low when in init mode

[Roger Pau Monne]

When the PIC is on the init sequence prevent interrupt delivery. The
state of the registers is in the process of being set during the init
phase, so it makes sense to prevent any int line changes during that
process.

Requested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v2:
 - New in this version.
---
 xen/arch/x86/hvm/vpic.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/hvm/vpic.c b/xen/arch/x86/hvm/vpic.c
index c1c1de7fd0..9195155ff0 100644
--- a/xen/arch/x86/hvm/vpic.c
+++ b/xen/arch/x86/hvm/vpic.c
@@ -101,11 +101,14 @@ static void vpic_update_int_output(struct hvm_hw_vpic *vpic)
     irq = vpic_get_highest_priority_irq(vpic);
     TRACE_3D(TRC_HVM_EMUL_PIC_INT_OUTPUT, vpic->int_output, vpic->is_master,
              irq);
-    if ( vpic->int_output == (irq >= 0) )
+    if ( vpic->int_output == (!vpic->init_state && irq >= 0) )
         return;
 
-    /* INT line transition L->H or H->L. */
-    vpic->int_output = !vpic->int_output;
+    /*
+     * INT line transition L->H or H->L.
+     * Force line status to L when in init mode.
+     */
+    vpic->int_output = !vpic->init_state && !vpic->int_output;
 
     if ( vpic->int_output )
     {
-- 
2.29.2

[PATCH v3 4/6] x86/vpic: don't trigger unmask event until end of init

[Roger Pau Monne]

Wait until the end of the init sequence to trigger the unmask event.
Note that it will be unconditionally triggered, but that's harmless if
not unmask actually happened.

While there change the variable type to bool.

Requested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v2:
 - New in this version.
---
 xen/arch/x86/hvm/vpic.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/vpic.c b/xen/arch/x86/hvm/vpic.c
index 9195155ff0..795a76768d 100644
--- a/xen/arch/x86/hvm/vpic.c
+++ b/xen/arch/x86/hvm/vpic.c
@@ -188,7 +188,8 @@ static void vpic_ioport_write(
     struct hvm_hw_vpic *vpic, uint32_t addr, uint32_t val)
 {
     int priority, cmd;
-    uint8_t mask, unmasked = 0;
+    uint8_t mask;
+    bool unmasked = false;
 
     vpic_lock(vpic);
 
@@ -200,7 +201,6 @@ static void vpic_ioport_write(
             /* Clear edge-sensing logic. */
             vpic->irr &= vpic->elcr;
 
-            unmasked = vpic->imr;
             /* No interrupts masked or in service. */
             vpic->imr = vpic->isr = 0;
 
@@ -294,13 +294,17 @@ static void vpic_ioport_write(
             /* ICW3 */
             vpic->init_state++;
             if ( !(vpic->init_state & 4) )
+            {
                 vpic->init_state = 0; /* No ICW4: init done */
+                unmasked = true;
+            }
             break;
         case 3:
             /* ICW4 */
             vpic->special_fully_nested_mode = (val >> 4) & 1;
             vpic->auto_eoi = (val >> 1) & 1;
             vpic->init_state = 0;
+            unmasked = true;
             break;
         }
     }
-- 
2.29.2

[PATCH v3 5/6] x86/vpic: issue dpci EOI for cleared pins at ICW1

[Roger Pau Monne]

When pins are cleared from either ISR or IRR as part of the
initialization sequence forward the clearing of those pins to the dpci
EOI handler, as it is equivalent to an EOI. Not doing so can bring the
interrupt controller state out of sync with the dpci handling logic,
that expects a notification when a pin has been EOI'ed.

Fixes: 7b3cb5e5416 ('IRQ injection changes for HVM PCI passthru.')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v2:
 - Remove the unmask label.
---
 xen/arch/x86/hvm/vpic.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/xen/arch/x86/hvm/vpic.c b/xen/arch/x86/hvm/vpic.c
index 795a76768d..f465b7f997 100644
--- a/xen/arch/x86/hvm/vpic.c
+++ b/xen/arch/x86/hvm/vpic.c
@@ -197,6 +197,8 @@ static void vpic_ioport_write(
     {
         if ( val & 0x10 )
         {
+            unsigned int pending = vpic->isr | (vpic->irr & ~vpic->elcr);
+
             /* ICW1 */
             /* Clear edge-sensing logic. */
             vpic->irr &= vpic->elcr;
@@ -220,6 +222,24 @@ static void vpic_ioport_write(
             }
 
             vpic->init_state = ((val & 3) << 2) | 1;
+            vpic_update_int_output(vpic);
+            vpic_unlock(vpic);
+
+            /*
+             * Forward the EOI of any pending or in service interrupt that has
+             * been cleared from IRR or ISR, or else the dpci logic will get
+             * out of sync with the state of the interrupt controller.
+             */
+            while ( pending )
+            {
+                unsigned int pin = __scanbit(pending, 8);
+
+                ASSERT(pin < 8);
+                hvm_dpci_eoi(current->domain,
+                             hvm_isa_irq_to_gsi((addr >> 7) ? (pin | 8) : pin));
+                __clear_bit(pin, &pending);
+            }
+            return;
         }
         else if ( val & 0x08 )
         {
-- 
2.29.2

[PATCH v3 6/6] x86/dpci: remove the dpci EOI timer

[Roger Pau Monne]

Current interrupt pass though code will setup a timer for each
interrupt injected to the guest that requires an EOI from the guest.
Such timer would perform two actions if the guest doesn't EOI the
interrupt before a given period of time. The first one is deasserting
the virtual line, the second is perform an EOI of the physical
interrupt source if it requires such.

The deasserting of the guest virtual line is wrong, since it messes
with the interrupt status of the guest. This seems to have been done
in order to compensate for missing deasserts when certain interrupt
controller actions are performed. The original motivation of the
introduction of the timer was to fix issues when a GSI was shared
between different guests. We believe that other changes in the
interrupt handling code (ie: proper propagation of EOI related actions
to dpci) will have fixed such errors now.

Performing an EOI of the physical interrupt source is redundant, since
there's already a timer that takes care of this for all interrupts,
not just the HVM dpci ones, see irq_guest_action_t struct eoi_timer
field.

Since both of the actions performed by the dpci timer are not
required, remove it altogether.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Changes since v1:
 - Add parentheses.
---
 xen/drivers/passthrough/vtd/x86/hvm.c |  3 -
 xen/drivers/passthrough/x86/hvm.c     | 95 +--------------------------
 xen/include/asm-x86/hvm/irq.h         |  3 -
 xen/include/xen/iommu.h               |  5 --
 4 files changed, 2 insertions(+), 104 deletions(-)

diff --git a/xen/drivers/passthrough/vtd/x86/hvm.c b/xen/drivers/passthrough/vtd/x86/hvm.c
index f77b35815c..b531fe907a 100644
--- a/xen/drivers/passthrough/vtd/x86/hvm.c
+++ b/xen/drivers/passthrough/vtd/x86/hvm.c
@@ -36,10 +36,7 @@ static int _hvm_dpci_isairq_eoi(struct domain *d,
         {
             hvm_pci_intx_deassert(d, digl->device, digl->intx);
             if ( --pirq_dpci->pending == 0 )
-            {
-                stop_timer(&pirq_dpci->timer);
                 pirq_guest_eoi(dpci_pirq(pirq_dpci));
-            }
         }
     }
 
diff --git a/xen/drivers/passthrough/x86/hvm.c b/xen/drivers/passthrough/x86/hvm.c
index a6e2863c14..351daafdc9 100644
--- a/xen/drivers/passthrough/x86/hvm.c
+++ b/xen/drivers/passthrough/x86/hvm.c
@@ -136,77 +136,6 @@ static void pt_pirq_softirq_reset(struct hvm_pirq_dpci *pirq_dpci)
     pirq_dpci->masked = 0;
 }
 
-bool pt_irq_need_timer(uint32_t flags)
-{
-    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,
-                            void *arg)
-{
-    if ( __test_and_clear_bit(_HVM_IRQ_DPCI_EOI_LATCH_SHIFT,
-                              &pirq_dpci->flags) )
-    {
-        pirq_dpci->masked = 0;
-        pirq_dpci->pending = 0;
-        pirq_guest_eoi(dpci_pirq(pirq_dpci));
-    }
-
-    return 0;
-}
-
-static void pt_irq_time_out(void *data)
-{
-    struct hvm_pirq_dpci *irq_map = data;
-    const struct hvm_irq_dpci *dpci;
-    const struct dev_intx_gsi_link *digl;
-
-    spin_lock(&irq_map->dom->event_lock);
-
-    if ( irq_map->flags & HVM_IRQ_DPCI_IDENTITY_GSI )
-    {
-        ASSERT(is_hardware_domain(irq_map->dom));
-        /*
-         * Identity mapped, no need to iterate over the guest GSI list to find
-         * other pirqs sharing the same guest GSI.
-         *
-         * In the identity mapped case the EOI can also be done now, this way
-         * the iteration over the list of domain pirqs is avoided.
-         */
-        hvm_gsi_deassert(irq_map->dom, dpci_pirq(irq_map)->pirq);
-        irq_map->flags |= HVM_IRQ_DPCI_EOI_LATCH;
-        pt_irq_guest_eoi(irq_map->dom, irq_map, NULL);
-        spin_unlock(&irq_map->dom->event_lock);
-        return;
-    }
-
-    dpci = domain_get_irq_dpci(irq_map->dom);
-    if ( unlikely(!dpci) )
-    {
-        ASSERT_UNREACHABLE();
-        spin_unlock(&irq_map->dom->event_lock);
-        return;
-    }
-    list_for_each_entry ( digl, &irq_map->digl_list, list )
-    {
-        unsigned int guest_gsi = hvm_pci_intx_gsi(digl->device, digl->intx);
-        const struct hvm_girq_dpci_mapping *girq;
-
-        list_for_each_entry ( girq, &dpci->girq[guest_gsi], list )
-        {
-            struct pirq *pirq = pirq_info(irq_map->dom, girq->machine_gsi);
-
-            pirq_dpci(pirq)->flags |= HVM_IRQ_DPCI_EOI_LATCH;
-        }
-        hvm_pci_intx_deassert(irq_map->dom, digl->device, digl->intx);
-    }
-
-    pt_pirq_iterate(irq_map->dom, pt_irq_guest_eoi, NULL);
-
-    spin_unlock(&irq_map->dom->event_lock);
-}
-
 struct hvm_irq_dpci *domain_get_irq_dpci(const struct domain *d)
 {
     if ( !d || !is_hvm_domain(d) )
@@ -568,15 +497,10 @@ int pt_irq_create_bind(
                 }
             }
 
-            /* Init timer before binding */
-            if ( pt_irq_need_timer(pirq_dpci->flags) )
-                init_timer(&pirq_dpci->timer, pt_irq_time_out, pirq_dpci, 0);
             /* Deal with gsi for legacy devices */
             rc = pirq_guest_bind(d->vcpu[0], info, share);
             if ( unlikely(rc) )
             {
-                if ( pt_irq_need_timer(pirq_dpci->flags) )
-                    kill_timer(&pirq_dpci->timer);
                 /*
                  * There is no path for __do_IRQ to schedule softirq as
                  * IRQ_GUEST is not set. As such we can reset 'dom' directly.
@@ -743,8 +667,6 @@ int pt_irq_destroy_bind(
     {
         pirq_guest_unbind(d, pirq);
         msixtbl_pt_unregister(d, pirq);
-        if ( pt_irq_need_timer(pirq_dpci->flags) )
-            kill_timer(&pirq_dpci->timer);
         pirq_dpci->flags = 0;
         /*
          * See comment in pt_irq_create_bind's PT_IRQ_TYPE_MSI before the
@@ -934,16 +856,6 @@ static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci)
             __msi_pirq_eoi(pirq_dpci);
             goto out;
         }
-
-        /*
-         * Set a timer to see if the guest can finish the interrupt or not. For
-         * example, the guest OS may unmask the PIC during boot, before the
-         * guest driver is loaded. hvm_pci_intx_assert() may succeed, but the
-         * guest will never deal with the irq, then the physical interrupt line
-         * will never be deasserted.
-         */
-        ASSERT(pt_irq_need_timer(pirq_dpci->flags));
-        set_timer(&pirq_dpci->timer, NOW() + PT_IRQ_TIME_OUT);
     }
 
  out:
@@ -967,10 +879,10 @@ static void hvm_pirq_eoi(struct pirq *pirq)
      * since interrupt is still not EOIed
      */
     if ( --pirq_dpci->pending ||
-         !pt_irq_need_timer(pirq_dpci->flags) )
+         /* When the interrupt source is MSI no Ack should be performed. */
+         (pirq_dpci->flags & HVM_IRQ_DPCI_TRANSLATE) )
         return;
 
-    stop_timer(&pirq_dpci->timer);
     pirq_guest_eoi(pirq);
 }
 
@@ -1042,9 +954,6 @@ static int pci_clean_dpci_irq(struct domain *d,
 
     pirq_guest_unbind(d, dpci_pirq(pirq_dpci));
 
-    if ( pt_irq_need_timer(pirq_dpci->flags) )
-        kill_timer(&pirq_dpci->timer);
-
     list_for_each_entry_safe ( digl, tmp, &pirq_dpci->digl_list, list )
     {
         list_del(&digl->list);
diff --git a/xen/include/asm-x86/hvm/irq.h b/xen/include/asm-x86/hvm/irq.h
index 7f76f6c437..07b1ab99cd 100644
--- a/xen/include/asm-x86/hvm/irq.h
+++ b/xen/include/asm-x86/hvm/irq.h
@@ -117,7 +117,6 @@ struct dev_intx_gsi_link {
 #define _HVM_IRQ_DPCI_MACH_PCI_SHIFT            0
 #define _HVM_IRQ_DPCI_MACH_MSI_SHIFT            1
 #define _HVM_IRQ_DPCI_MAPPED_SHIFT              2
-#define _HVM_IRQ_DPCI_EOI_LATCH_SHIFT           3
 #define _HVM_IRQ_DPCI_GUEST_PCI_SHIFT           4
 #define _HVM_IRQ_DPCI_GUEST_MSI_SHIFT           5
 #define _HVM_IRQ_DPCI_IDENTITY_GSI_SHIFT        6
@@ -126,7 +125,6 @@ struct dev_intx_gsi_link {
 #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)
 #define HVM_IRQ_DPCI_MAPPED          (1u << _HVM_IRQ_DPCI_MAPPED_SHIFT)
-#define HVM_IRQ_DPCI_EOI_LATCH       (1u << _HVM_IRQ_DPCI_EOI_LATCH_SHIFT)
 #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)
@@ -171,7 +169,6 @@ struct hvm_pirq_dpci {
     struct list_head digl_list;
     struct domain *dom;
     struct hvm_gmsi_info gmsi;
-    struct timer timer;
     struct list_head softirq_list;
 };
 
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 863a68fe16..192ee07127 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -183,11 +183,6 @@ int pt_irq_destroy_bind(struct domain *, const struct xen_domctl_bind_pt_irq *);
 void hvm_dpci_isairq_eoi(struct domain *d, unsigned int isairq);
 struct hvm_irq_dpci *domain_get_irq_dpci(const struct domain *);
 void free_hvm_irq_dpci(struct hvm_irq_dpci *dpci);
-#ifdef CONFIG_HVM
-bool pt_irq_need_timer(uint32_t flags);
-#else
-static inline bool pt_irq_need_timer(unsigned int flags) { return false; }
-#endif
 
 struct msi_desc;
 struct msi_msg;
-- 
2.29.2

Re: [PATCH v3 1/6] x86/vioapic: top word redir entry writes don't trigger interrupts

[Jan Beulich]

On 26.01.2021 14:45, Roger Pau Monne wrote:
> Top word writes just update the destination of the interrupt, but
> since there's no change on the masking or the triggering mode no
> guest interrupt injection can result of such write. Add an assert to
> that effect.
> 
> Requested-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH v3 2/6] x86/vioapic: issue EOI to dpci when switching pin to edge trigger mode

[Jan Beulich]

On 26.01.2021 14:45, Roger Pau Monne wrote:
> When an IO-APIC pin is switched from level to edge trigger mode the
> IRR bit is cleared, so it can be used as a way to EOI an interrupt at
> the IO-APIC level.
> 
> Such EOI however does not get forwarded to the dpci code like it's
> done for the local APIC initiated EOI. This change adds the code in
> order to notify dpci of such EOI, so that dpci and the interrupt
> controller are in sync.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH v3 3/6] x86/vpic: force int output to low when in init mode

[Jan Beulich]

On 26.01.2021 14:45, Roger Pau Monne wrote:
> When the PIC is on the init sequence prevent interrupt delivery. The
> state of the registers is in the process of being set during the init
> phase, so it makes sense to prevent any int line changes during that
> process.
> 
> Requested-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

I guess I'll change the other tag in this case to Suggested-by
though, should I end up committing this.

Jan

Re: [PATCH v3 4/6] x86/vpic: don't trigger unmask event until end of init

[Jan Beulich]

On 26.01.2021 14:45, Roger Pau Monne wrote:
> Wait until the end of the init sequence to trigger the unmask event.
> Note that it will be unconditionally triggered, but that's harmless if
> not unmask actually happened.
> 
> While there change the variable type to bool.
> 
> Requested-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH v3 5/6] x86/vpic: issue dpci EOI for cleared pins at ICW1

[Jan Beulich]

On 26.01.2021 14:45, Roger Pau Monne wrote:
> When pins are cleared from either ISR or IRR as part of the
> initialization sequence forward the clearing of those pins to the dpci
> EOI handler, as it is equivalent to an EOI. Not doing so can bring the
> interrupt controller state out of sync with the dpci handling logic,
> that expects a notification when a pin has been EOI'ed.
> 
> Fixes: 7b3cb5e5416 ('IRQ injection changes for HVM PCI passthru.')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

As said before, under the assumption that the clearing of IRR
and ISR that we do is correct, I agree with the change. I'd
like to give it some time though before giving my R-b here, for
the inquiry to hopefully get answered. After all there's still
the possibility of us needing to instead squash that clearing
(which then would seem to result in getting things in sync the
other way around).

Jan

> --- a/xen/arch/x86/hvm/vpic.c
> +++ b/xen/arch/x86/hvm/vpic.c
> @@ -197,6 +197,8 @@ static void vpic_ioport_write(
>      {
>          if ( val & 0x10 )
>          {
> +            unsigned int pending = vpic->isr | (vpic->irr & ~vpic->elcr);
> +
>              /* ICW1 */
>              /* Clear edge-sensing logic. */
>              vpic->irr &= vpic->elcr;
> @@ -220,6 +222,24 @@ static void vpic_ioport_write(
>              }
>  
>              vpic->init_state = ((val & 3) << 2) | 1;
> +            vpic_update_int_output(vpic);
> +            vpic_unlock(vpic);
> +
> +            /*
> +             * Forward the EOI of any pending or in service interrupt that has
> +             * been cleared from IRR or ISR, or else the dpci logic will get
> +             * out of sync with the state of the interrupt controller.
> +             */
> +            while ( pending )
> +            {
> +                unsigned int pin = __scanbit(pending, 8);
> +
> +                ASSERT(pin < 8);
> +                hvm_dpci_eoi(current->domain,
> +                             hvm_isa_irq_to_gsi((addr >> 7) ? (pin | 8) : pin));
> +                __clear_bit(pin, &pending);
> +            }
> +            return;
>          }
>          else if ( val & 0x08 )
>          {
> 

Re: [PATCH v3 0/6] x86/intr: HVM guest interrupt handling fixes/cleanup

[Jan Beulich]

Ian,

On 26.01.2021 14:45, Roger Pau Monne wrote:
> The following series aims to fix some shortcomings of guest interrupt
> handling when using passthrough devices. The first 5 patches are
> bugfixes or cleanups, which I think should solve the issue(s) that
> caused the dpci EOI timer to be introduced. However neither me nor
> others seem to be able to reproduce the original issue, so it's hard to
> tell.
> 
> It's my opinion that we should remove the timer and see what explodes
> (if anything). That's the only way we will be able to figure out what
> the original issue was, and how to fix it without introducing yet
> another per-guest-irq related timer.
> 
> Thanks, Roger.
> 
> Roger Pau Monne (6):
>   x86/vioapic: top word redir entry writes don't trigger interrupts
>   x86/vioapic: issue EOI to dpci when switching pin to edge trigger mode
>   x86/vpic: force int output to low when in init mode
>   x86/vpic: don't trigger unmask event until end of init
>   x86/vpic: issue dpci EOI for cleared pins at ICW1
>   x86/dpci: remove the dpci EOI timer

while half of this series was still submitted in time, I'd still
like to raise the question of including part or all of it in
4.15. In particular the last change is one which I would prefer
to see happen early in a release cycle. Risk assessment is
pretty difficult, I'm afraid (Roger can correct me here), as at
least some of what gets adjusted are cases we don't normally
expect to be exercised. (FAOD patch 5 is still pending a R-b
tag.)

Roger, if you could give your own judgement on which of the
changes you would view as more or less clear 4.15 candidates,
this may help Ian take a decision.

Jan

Re: [PATCH v3 5/6] x86/vpic: issue dpci EOI for cleared pins at ICW1

[Roger Pau Monné]

On Tue, Jan 26, 2021 at 05:57:49PM +0100, Jan Beulich wrote:
> On 26.01.2021 14:45, Roger Pau Monne wrote:
> > When pins are cleared from either ISR or IRR as part of the
> > initialization sequence forward the clearing of those pins to the dpci
> > EOI handler, as it is equivalent to an EOI. Not doing so can bring the
> > interrupt controller state out of sync with the dpci handling logic,
> > that expects a notification when a pin has been EOI'ed.
> > 
> > Fixes: 7b3cb5e5416 ('IRQ injection changes for HVM PCI passthru.')
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> As said before, under the assumption that the clearing of IRR
> and ISR that we do is correct, I agree with the change. I'd
> like to give it some time though before giving my R-b here, for
> the inquiry to hopefully get answered. After all there's still
> the possibility of us needing to instead squash that clearing
> (which then would seem to result in getting things in sync the
> other way around).

OK, let's wait a bit to see what Intel replies. I assume this would
qualify as a bugfix for getting it committed later?

Thanks, Roger.

Re: [PATCH v3 0/6] x86/intr: HVM guest interrupt handling fixes/cleanup

[Roger Pau Monné]

On Tue, Jan 26, 2021 at 06:07:23PM +0100, Jan Beulich wrote:
> Ian,
> 
> On 26.01.2021 14:45, Roger Pau Monne wrote:
> > The following series aims to fix some shortcomings of guest interrupt
> > handling when using passthrough devices. The first 5 patches are
> > bugfixes or cleanups, which I think should solve the issue(s) that
> > caused the dpci EOI timer to be introduced. However neither me nor
> > others seem to be able to reproduce the original issue, so it's hard to
> > tell.
> > 
> > It's my opinion that we should remove the timer and see what explodes
> > (if anything). That's the only way we will be able to figure out what
> > the original issue was, and how to fix it without introducing yet
> > another per-guest-irq related timer.
> > 
> > Thanks, Roger.
> > 
> > Roger Pau Monne (6):
> >   x86/vioapic: top word redir entry writes don't trigger interrupts
> >   x86/vioapic: issue EOI to dpci when switching pin to edge trigger mode
> >   x86/vpic: force int output to low when in init mode
> >   x86/vpic: don't trigger unmask event until end of init
> >   x86/vpic: issue dpci EOI for cleared pins at ICW1
> >   x86/dpci: remove the dpci EOI timer
> 
> while half of this series was still submitted in time, I'd still
> like to raise the question of including part or all of it in
> 4.15. In particular the last change is one which I would prefer
> to see happen early in a release cycle. Risk assessment is
> pretty difficult, I'm afraid (Roger can correct me here), as at
> least some of what gets adjusted are cases we don't normally
> expect to be exercised. (FAOD patch 5 is still pending a R-b
> tag.)
> 
> Roger, if you could give your own judgement on which of the
> changes you would view as more or less clear 4.15 candidates,
> this may help Ian take a decision.

I agree, the only risky patch is the last one, mainly because we have
no way to reproduce the issue that code was fixing. It's my assumption
that all the fixes prior to the last patch should address the same
issues the timer was trying to address.

So my recommendation would be to not commit the last patch unless all
the prior ones have been committed, and that would include 5/6 which
is still missing a R-b.

Thanks, Roger.

Re: [PATCH v3 5/6] x86/vpic: issue dpci EOI for cleared pins at ICW1

[Jan Beulich]

On 27.01.2021 10:15, Roger Pau Monné wrote:
> On Tue, Jan 26, 2021 at 05:57:49PM +0100, Jan Beulich wrote:
>> On 26.01.2021 14:45, Roger Pau Monne wrote:
>>> When pins are cleared from either ISR or IRR as part of the
>>> initialization sequence forward the clearing of those pins to the dpci
>>> EOI handler, as it is equivalent to an EOI. Not doing so can bring the
>>> interrupt controller state out of sync with the dpci handling logic,
>>> that expects a notification when a pin has been EOI'ed.
>>>
>>> Fixes: 7b3cb5e5416 ('IRQ injection changes for HVM PCI passthru.')
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>
>> As said before, under the assumption that the clearing of IRR
>> and ISR that we do is correct, I agree with the change. I'd
>> like to give it some time though before giving my R-b here, for
>> the inquiry to hopefully get answered. After all there's still
>> the possibility of us needing to instead squash that clearing
>> (which then would seem to result in getting things in sync the
>> other way around).
> 
> OK, let's wait a bit to see what Intel replies. I assume this would
> qualify as a bugfix for getting it committed later?

It would be up to Ian then to judge, but as long as it fixes an
actual bug, my understanding is it will still qualify for being
considered.

Jan

Re: [PATCH v3 5/6] x86/vpic: issue dpci EOI for cleared pins at ICW1

[Jan Beulich]

On 26.01.2021 17:57, Jan Beulich wrote:
> On 26.01.2021 14:45, Roger Pau Monne wrote:
>> When pins are cleared from either ISR or IRR as part of the
>> initialization sequence forward the clearing of those pins to the dpci
>> EOI handler, as it is equivalent to an EOI. Not doing so can bring the
>> interrupt controller state out of sync with the dpci handling logic,
>> that expects a notification when a pin has been EOI'ed.
>>
>> Fixes: 7b3cb5e5416 ('IRQ injection changes for HVM PCI passthru.')
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> As said before, under the assumption that the clearing of IRR
> and ISR that we do is correct, I agree with the change. I'd
> like to give it some time though before giving my R-b here, for
> the inquiry to hopefully get answered. After all there's still
> the possibility of us needing to instead squash that clearing
> (which then would seem to result in getting things in sync the
> other way around).

Still haven't heard anything, so

Reviewed-by: Jan Beulich <jbeulich@suse.com>

In the worst case we'd need to consider reverting later on.

Jan