[PATCH for-4.14 0/8] x86/vpt: fixes for vpt and enable vPIT for PVH dom0

[PATCH for-4.14 0/8] x86/vpt: fixes for vpt and enable vPIT for PVH dom0

[Roger Pau Monne]

Hello,

The first 6 patches on this series are fixes for HVM virtual timers or
for the handling of the emulated PIT. I think they are all candidates
for 4.14 since without those PIT is not usable (and likely other
emulated timers will also experience issues) unless the OS happens to
make a very specific use of it, ie: timers must be configured from vCPU
0 and the destination must also be set to vCPU 0. FreeBSD for example
doesn't follow such rules, as it will usually configure PIT timers from
vCPU 0 and the destination will be set to a random vCPU in the system,
and as a result gets a non functional PIT.

Patches 7 and 8 enable the usage of the emulated vPIT for PVH dom0,
which is said to be required for certain video BIOS. As I mostly test
PVH dom0 on headless systems I'm not able to assert how common this is,
but given that it's already enabled for a classic PV dom0 let's try to
not regress and also provide a working PIT for PVH dom0.

I think the whole batch is also a candidate for backporting.

Thanks, Roger.

Roger Pau Monne (8):
  x86/hvm: fix vIO-APIC build without IRQ0_SPECIAL_ROUTING
  x86/hvm: don't force vCPU 0 for IRQ 0 when using fixed destination
    mode
  x86/hvm: fix ISA IRQ 0 handling when set as lowest priority mode in IO
    APIC
  x86/vpt: only try to resume timers belonging to enabled devices
  x86/hvm: only translate ISA interrupts to GSIs in virtual timers
  x86/vpt: fix injection to remote vCPU
  x86/hvm: add hardware domain support to hvm_isa_irq_to_gsi
  x86/hvm: enable emulated PIT for PVH dom0

 xen/arch/x86/domain.c         |   5 +-
 xen/arch/x86/emul-i8254.c     |  12 +++-
 xen/arch/x86/hvm/irq.c        |  20 ++++++-
 xen/arch/x86/hvm/vioapic.c    |  47 +++++++---------
 xen/arch/x86/hvm/vpic.c       |   7 ++-
 xen/arch/x86/hvm/vpt.c        | 102 ++++++++++++++++++----------------
 xen/arch/x86/io_apic.c        |  16 +++---
 xen/include/asm-x86/hvm/irq.h |   2 +-
 xen/include/asm-x86/io_apic.h |   3 +
 9 files changed, 121 insertions(+), 93 deletions(-)

-- 
2.26.2

[PATCH for-4.14 1/8] x86/hvm: fix vIO-APIC build without IRQ0_SPECIAL_ROUTING

[Roger Pau Monne]

pit_channel0_enabled needs to be guarded with IRQ0_SPECIAL_ROUTING
since it's only used when the special handling of ISA IRQ 0 is enabled.

No functional change.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 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 b87facb0e0..bd41036137 100644
--- a/xen/arch/x86/hvm/vioapic.c
+++ b/xen/arch/x86/hvm/vioapic.c
@@ -391,10 +391,12 @@ static void ioapic_inj_irq(
     vlapic_set_irq(target, vector, trig_mode);
 }
 
+#ifdef IRQ0_SPECIAL_ROUTING
 static inline int pit_channel0_enabled(void)
 {
     return pt_active(&current->domain->arch.vpit.pt0);
 }
+#endif
 
 static void vioapic_deliver(struct hvm_vioapic *vioapic, unsigned int pin)
 {
-- 
2.26.2

[PATCH for-4.14 2/8] x86/hvm: don't force vCPU 0 for IRQ 0 when using fixed destination mode

[Roger Pau Monne]

When the IO APIC pin mapped to the ISA IRQ 0 has been configured to
use fixed delivery mode do not forcefully route interrupts to vCPU 0,
as the OS might have setup those interrupts to be injected to a
different vCPU, and injecting to vCPU 0 can cause the OS to miss such
interrupts or errors to happen due to unexpected vectors being
injected on vCPU 0.

In order to fix remove such handling altogether for fixed destination
mode pins and just inject them according to the data setup in the
IO-APIC entry.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/hvm/vioapic.c | 23 ++++-------------------
 1 file changed, 4 insertions(+), 19 deletions(-)

diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
index bd41036137..67472e5934 100644
--- a/xen/arch/x86/hvm/vioapic.c
+++ b/xen/arch/x86/hvm/vioapic.c
@@ -445,26 +445,11 @@ static void vioapic_deliver(struct hvm_vioapic *vioapic, unsigned int pin)
     }
 
     case dest_Fixed:
-    {
-#ifdef IRQ0_SPECIAL_ROUTING
-        /* Do not deliver timer interrupts to VCPU != 0 */
-        if ( (irq == hvm_isa_irq_to_gsi(0)) && pit_channel0_enabled() )
-        {
-            if ( (v = d->vcpu ? d->vcpu[0] : NULL) != NULL )
-                ioapic_inj_irq(vioapic, vcpu_vlapic(v), vector,
-                               trig_mode, delivery_mode);
-        }
-        else
-#endif
-        {
-            for_each_vcpu ( d, v )
-                if ( vlapic_match_dest(vcpu_vlapic(v), NULL,
-                                       0, dest, dest_mode) )
-                    ioapic_inj_irq(vioapic, vcpu_vlapic(v), vector,
-                                   trig_mode, delivery_mode);
-        }
+        for_each_vcpu ( d, v )
+            if ( vlapic_match_dest(vcpu_vlapic(v), NULL, 0, dest, dest_mode) )
+                ioapic_inj_irq(vioapic, vcpu_vlapic(v), vector, trig_mode,
+                               delivery_mode);
         break;
-    }
 
     case dest_NMI:
     {
-- 
2.26.2

[PATCH for-4.14 3/8] x86/hvm: fix ISA IRQ 0 handling when set as lowest priority mode in IO APIC

[Roger Pau Monne]

Lowest priority destination mode does allow the vIO APIC code to
select a vCPU to inject the interrupt to, but the selected vCPU must
be part of the possible destinations configured for such IO APIC pin.

Fix the code in order to only force vCPU 0 if it's part of the
listed destinations.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/hvm/vioapic.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
index 67472e5934..e1417cc6a7 100644
--- a/xen/arch/x86/hvm/vioapic.c
+++ b/xen/arch/x86/hvm/vioapic.c
@@ -422,12 +422,13 @@ static void vioapic_deliver(struct hvm_vioapic *vioapic, unsigned int pin)
     case dest_LowestPrio:
     {
 #ifdef IRQ0_SPECIAL_ROUTING
-        /* Force round-robin to pick VCPU 0 */
-        if ( (irq == hvm_isa_irq_to_gsi(0)) && pit_channel0_enabled() )
-        {
-            v = d->vcpu ? d->vcpu[0] : NULL;
-            target = v ? vcpu_vlapic(v) : NULL;
-        }
+        struct vlapic *lapic0 = vcpu_vlapic(d->vcpu[0]);
+
+        /* Force to pick vCPU 0 if part of the destination list */
+        if ( (irq == hvm_isa_irq_to_gsi(0)) && pit_channel0_enabled() &&
+             vlapic_match_dest(lapic0, NULL, 0, dest, dest_mode) &&
+             vlapic_enabled(lapic0) )
+            target = lapic0;
         else
 #endif
             target = vlapic_lowest_prio(d, NULL, 0, dest, dest_mode);
-- 
2.26.2

[PATCH for-4.14 4/8] x86/vpt: only try to resume timers belonging to enabled devices

[Roger Pau Monne]

Check whether the emulated device is actually enabled before trying to
resume the associated timers.

Thankfully all those structures are zeroed at initialization, and
since the devices are not enabled they are never populated, which
triggers the pt->vcpu check at the beginning of pt_resume forcing an
exit from the function.

While there limit the scope of i and make it unsigned.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/hvm/vpt.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/hvm/vpt.c b/xen/arch/x86/hvm/vpt.c
index 47f2c2aa64..62c87867c5 100644
--- a/xen/arch/x86/hvm/vpt.c
+++ b/xen/arch/x86/hvm/vpt.c
@@ -636,14 +636,19 @@ static void pt_resume(struct periodic_time *pt)
 
 void pt_may_unmask_irq(struct domain *d, struct periodic_time *vlapic_pt)
 {
-    int i;
-
     if ( d )
     {
-        pt_resume(&d->arch.vpit.pt0);
-        pt_resume(&d->arch.hvm.pl_time->vrtc.pt);
-        for ( i = 0; i < HPET_TIMER_NUM; i++ )
-            pt_resume(&d->arch.hvm.pl_time->vhpet.pt[i]);
+        if ( has_vpit(d) )
+            pt_resume(&d->arch.vpit.pt0);
+        if ( has_vrtc(d) )
+            pt_resume(&d->arch.hvm.pl_time->vrtc.pt);
+        if ( has_vhpet(d) )
+        {
+            unsigned int i;
+
+            for ( i = 0; i < HPET_TIMER_NUM; i++ )
+                pt_resume(&d->arch.hvm.pl_time->vhpet.pt[i]);
+        }
     }
 
     if ( vlapic_pt )
-- 
2.26.2

[PATCH for-4.14 5/8] x86/hvm: only translate ISA interrupts to GSIs in virtual timers

[Roger Pau Monne]

Only call hvm_isa_irq_to_gsi for ISA interrupts, interrupts
originating from an IO APIC pin already use a GSI and don't need to be
translated.

I haven't observed any issues from this, but I think it's better to
use it correctly.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/hvm/vpt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/vpt.c b/xen/arch/x86/hvm/vpt.c
index 62c87867c5..6a975fc668 100644
--- a/xen/arch/x86/hvm/vpt.c
+++ b/xen/arch/x86/hvm/vpt.c
@@ -86,7 +86,7 @@ static int pt_irq_vector(struct periodic_time *pt, enum hvm_intsrc src)
         return pt->irq;
 
     isa_irq = pt->irq;
-    gsi = hvm_isa_irq_to_gsi(isa_irq);
+    gsi = pt->source == PTSRC_isa ? hvm_isa_irq_to_gsi(isa_irq) : pt->irq;
 
     if ( src == hvm_intsrc_pic )
         return (v->domain->arch.hvm.vpic[isa_irq >> 3].irq_base
-- 
2.26.2

[PATCH for-4.14 6/8] x86/vpt: fix injection to remote vCPU

[Roger Pau Monne]

vpt timers are usually added to the per-vCPU list of the vCPU where
they get setup, but depending on the timer source type that vCPU might
be different than the one where the interrupt vector gets injected.

For example the PIT timer use a PIC or IO-APIC pin in order to select
the destination vCPU and vector, which might not match the vCPU they
are configured from.

If such a situation happens pt_intr_post won't be called, and thus the
vpt will be left in a limbo where the next interrupt won't be
scheduled. Fix this by generalizing the special handling done to
IO-APIC level interrupts to be applied always when the destination
vCPU of the injected vector is different from the vCPU where the vpt
belongs to (ie: usually the one it's been configured from).

A further improvement as noted in a comment added to the code might be
to move the vpt so it's handled by the same vCPU where the vector gets
injected.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/hvm/vpt.c | 80 +++++++++++++++++++++---------------------
 1 file changed, 40 insertions(+), 40 deletions(-)

diff --git a/xen/arch/x86/hvm/vpt.c b/xen/arch/x86/hvm/vpt.c
index 6a975fc668..52ad5b90a7 100644
--- a/xen/arch/x86/hvm/vpt.c
+++ b/xen/arch/x86/hvm/vpt.c
@@ -358,59 +358,59 @@ int pt_update_irq(struct vcpu *v)
          * interrupt delivery case. Otherwise return -1 to do nothing.
          */
         vlapic_set_irq(vcpu_vlapic(v), irq, 0);
-        pt_vector = irq;
-        break;
+        return irq;
 
     case PTSRC_isa:
         hvm_isa_irq_deassert(v->domain, irq);
         if ( platform_legacy_irq(irq) && vlapic_accept_pic_intr(v) &&
              v->domain->arch.hvm.vpic[irq >> 3].int_output )
-            hvm_isa_irq_assert(v->domain, irq, NULL);
+            pt_vector = hvm_isa_irq_assert(v->domain, irq, NULL);
         else
-        {
             pt_vector = hvm_isa_irq_assert(v->domain, irq, vioapic_get_vector);
-            /*
-             * hvm_isa_irq_assert may not set the corresponding bit in vIRR
-             * when mask field of IOAPIC RTE is set. Check it again.
-             */
-            if ( pt_vector < 0 || !vlapic_test_irq(vcpu_vlapic(v), pt_vector) )
-                pt_vector = -1;
-        }
+
+        if ( pt_vector < 0 )
+            return pt_vector;
+
         break;
 
     case PTSRC_ioapic:
         pt_vector = hvm_ioapic_assert(v->domain, irq, level);
-        if ( pt_vector < 0 || !vlapic_test_irq(vcpu_vlapic(v), pt_vector) )
-        {
-            pt_vector = -1;
-            if ( level )
+        if ( pt_vector < 0 )
+            return pt_vector;
+
+        break;
+    }
+
+    ASSERT(pt_vector >= 0);
+    if ( !vlapic_test_irq(vcpu_vlapic(v), pt_vector) )
+    {
+        time_cb *cb = NULL;
+        void *cb_priv;
+
+        /*
+         * Vector has been injected to a different vCPU, call pt_irq_fired and
+         * execute the callback, since the destination vCPU(s) won't call
+         * pt_intr_post for it.
+         *
+         * TODO: move this vpt to one of the vCPUs where the vector gets
+         * injected.
+         */
+        spin_lock(&v->arch.hvm.tm_lock);
+        /* Make sure the timer is still on the list. */
+        list_for_each_entry ( pt, &v->arch.hvm.tm_list, list )
+            if ( pt == earliest_pt )
             {
-                /*
-                 * Level interrupts are always asserted because the pin assert
-                 * count is incremented regardless of whether the pin is masked
-                 * or the vector latched in IRR, so also execute the callback
-                 * associated with the timer.
-                 */
-                time_cb *cb = NULL;
-                void *cb_priv;
-
-                spin_lock(&v->arch.hvm.tm_lock);
-                /* Make sure the timer is still on the list. */
-                list_for_each_entry ( pt, &v->arch.hvm.tm_list, list )
-                    if ( pt == earliest_pt )
-                    {
-                        pt_irq_fired(v, pt);
-                        cb = pt->cb;
-                        cb_priv = pt->priv;
-                        break;
-                    }
-                spin_unlock(&v->arch.hvm.tm_lock);
-
-                if ( cb != NULL )
-                    cb(v, cb_priv);
+                pt_irq_fired(v, pt);
+                cb = pt->cb;
+                cb_priv = pt->priv;
+                break;
             }
-        }
-        break;
+        spin_unlock(&v->arch.hvm.tm_lock);
+
+        if ( cb != NULL )
+            cb(v, cb_priv);
+
+        pt_vector = -1;
     }
 
     return pt_vector;
-- 
2.26.2

[PATCH for-4.14 7/8] x86/hvm: add hardware domain support to hvm_isa_irq_to_gsi

[Roger Pau Monne]

The current function has the ISA IRQ 0 hardcoded to GSI 2 for HVM
domUs. Allow such function to also be used by the hardware domain by
taking into account the ACPI interrupt overwrites in order to get the
correct ISA to GSI mappings.

This requires passing a domain parameter to the helper, since it's not
guaranteed to always be called with current being the destination
vCPU.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/hvm/irq.c        | 20 +++++++++++++++++---
 xen/arch/x86/hvm/vioapic.c    |  2 +-
 xen/arch/x86/hvm/vpic.c       |  7 +++++--
 xen/arch/x86/hvm/vpt.c        |  5 +++--
 xen/arch/x86/io_apic.c        | 16 ++++++++--------
 xen/include/asm-x86/hvm/irq.h |  2 +-
 xen/include/asm-x86/io_apic.h |  3 +++
 7 files changed, 38 insertions(+), 17 deletions(-)

diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c
index fd02cf2e8d..6cbea63f4c 100644
--- a/xen/arch/x86/hvm/irq.c
+++ b/xen/arch/x86/hvm/irq.c
@@ -23,6 +23,7 @@
 #include <xen/sched.h>
 #include <xen/irq.h>
 #include <xen/keyhandler.h>
+#include <asm/io_apic.h>
 #include <asm/hvm/domain.h>
 #include <asm/hvm/support.h>
 #include <asm/msi.h>
@@ -212,12 +213,25 @@ void hvm_gsi_deassert(struct domain *d, unsigned int gsi)
     spin_unlock(&d->arch.hvm.irq_lock);
 }
 
+unsigned int hvm_isa_irq_to_gsi(const struct domain *d, unsigned int irq)
+{
+    int pin, apic;
+
+    if ( !is_hardware_domain(d) )
+        return irq ?: 2;
+
+    pin  = io_apic_find_isa_irq_pin(irq, mp_INT);
+    apic = io_apic_find_isa_irq_apic(irq, mp_INT);
+
+    return (pin < 0 || apic < 0) ? irq : (io_apic_gsi_base(apic) + pin);
+}
+
 int hvm_isa_irq_assert(struct domain *d, unsigned int isa_irq,
                        int (*get_vector)(const struct domain *d,
                                          unsigned int gsi))
 {
     struct hvm_irq *hvm_irq = hvm_domain_irq(d);
-    unsigned int gsi = hvm_isa_irq_to_gsi(isa_irq);
+    unsigned int gsi = hvm_isa_irq_to_gsi(d, isa_irq);
     int vector = -1;
 
     ASSERT(isa_irq <= 15);
@@ -240,7 +254,7 @@ void hvm_isa_irq_deassert(
     struct domain *d, unsigned int isa_irq)
 {
     struct hvm_irq *hvm_irq = hvm_domain_irq(d);
-    unsigned int gsi = hvm_isa_irq_to_gsi(isa_irq);
+    unsigned int gsi = hvm_isa_irq_to_gsi(d, isa_irq);
 
     ASSERT(isa_irq <= 15);
 
@@ -754,7 +768,7 @@ static int irq_load_isa(struct domain *d, hvm_domain_context_t *h)
      * This relies on the PCI IRQ state being loaded first. */
     for ( irq = 0; platform_legacy_irq(irq); irq++ )
         if ( test_bit(irq, &hvm_irq->isa_irq.i) )
-            hvm_irq->gsi_assert_count[hvm_isa_irq_to_gsi(irq)]++;
+            hvm_irq->gsi_assert_count[hvm_isa_irq_to_gsi(d, irq)]++;
 
     return 0;
 }
diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
index e1417cc6a7..34bec715b7 100644
--- a/xen/arch/x86/hvm/vioapic.c
+++ b/xen/arch/x86/hvm/vioapic.c
@@ -425,7 +425,7 @@ static void vioapic_deliver(struct hvm_vioapic *vioapic, unsigned int pin)
         struct vlapic *lapic0 = vcpu_vlapic(d->vcpu[0]);
 
         /* Force to pick vCPU 0 if part of the destination list */
-        if ( (irq == hvm_isa_irq_to_gsi(0)) && pit_channel0_enabled() &&
+        if ( (irq == hvm_isa_irq_to_gsi(d, 0)) && pit_channel0_enabled() &&
              vlapic_match_dest(lapic0, NULL, 0, dest, dest_mode) &&
              vlapic_enabled(lapic0) )
             target = lapic0;
diff --git a/xen/arch/x86/hvm/vpic.c b/xen/arch/x86/hvm/vpic.c
index 61f4b6784c..0ce3371a80 100644
--- a/xen/arch/x86/hvm/vpic.c
+++ b/xen/arch/x86/hvm/vpic.c
@@ -230,6 +230,8 @@ static void vpic_ioport_write(
         }
         else
         {
+            struct domain *currd = current->domain;
+
             /* OCW2 */
             cmd = val >> 5;
             switch ( cmd )
@@ -260,8 +262,9 @@ static void vpic_ioport_write(
                 /* Release lock and EOI the physical interrupt (if any). */
                 vpic_update_int_output(vpic);
                 vpic_unlock(vpic);
-                hvm_dpci_eoi(current->domain,
-                             hvm_isa_irq_to_gsi((addr >> 7) ? (irq|8) : irq),
+                hvm_dpci_eoi(currd,
+                             hvm_isa_irq_to_gsi(currd, (addr >> 7) ? (irq | 8)
+                                                                   : irq),
                              NULL);
                 return; /* bail immediately */
             case 6: /* Set Priority                */
diff --git a/xen/arch/x86/hvm/vpt.c b/xen/arch/x86/hvm/vpt.c
index 52ad5b90a7..1b3f902106 100644
--- a/xen/arch/x86/hvm/vpt.c
+++ b/xen/arch/x86/hvm/vpt.c
@@ -86,7 +86,8 @@ static int pt_irq_vector(struct periodic_time *pt, enum hvm_intsrc src)
         return pt->irq;
 
     isa_irq = pt->irq;
-    gsi = pt->source == PTSRC_isa ? hvm_isa_irq_to_gsi(isa_irq) : pt->irq;
+    gsi = pt->source == PTSRC_isa ? hvm_isa_irq_to_gsi(v->domain, isa_irq)
+                                  : pt->irq;
 
     if ( src == hvm_intsrc_pic )
         return (v->domain->arch.hvm.vpic[isa_irq >> 3].irq_base
@@ -128,7 +129,7 @@ static int pt_irq_masked(struct periodic_time *pt)
         if ( !(pic_imr & (1 << (pt->irq & 7))) && vlapic_accept_pic_intr(v) )
             return 0;
 
-        gsi = hvm_isa_irq_to_gsi(pt->irq);
+        gsi = hvm_isa_irq_to_gsi(v->domain, pt->irq);
     }
 
     /* Fallthrough to check if the interrupt is masked on the IO APIC. */
diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index 878ee5192d..1c000e8f76 100644
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -608,7 +608,7 @@ static int find_irq_entry(int apic, int pin, int type)
 /*
  * Find the pin to which IRQ[irq] (ISA) is connected
  */
-static int __init find_isa_irq_pin(int irq, int type)
+int io_apic_find_isa_irq_pin(int irq, int type)
 {
     int i;
 
@@ -628,7 +628,7 @@ static int __init find_isa_irq_pin(int irq, int type)
     return -1;
 }
 
-static int __init find_isa_irq_apic(int irq, int type)
+int io_apic_find_isa_irq_apic(int irq, int type)
 {
     int i;
 
@@ -1306,8 +1306,8 @@ static void __init enable_IO_APIC(void)
      * the i8259 probably is not connected the ioapic but give the
      * mptable a chance anyway.
      */
-    i8259_pin  = find_isa_irq_pin(0, mp_ExtINT);
-    i8259_apic = find_isa_irq_apic(0, mp_ExtINT);
+    i8259_pin  = io_apic_find_isa_irq_pin(0, mp_ExtINT);
+    i8259_apic = io_apic_find_isa_irq_apic(0, mp_ExtINT);
     /* Trust the MP table if nothing is setup in the hardware */
     if ((ioapic_i8259.pin == -1) && (i8259_pin >= 0)) {
         printk(KERN_WARNING "ExtINT not setup in hardware but reported by MP table\n");
@@ -1834,8 +1834,8 @@ static void __init unlock_ExtINT_logic(void)
     struct IO_APIC_route_entry entry0, entry1;
     unsigned char save_control, save_freq_select;
 
-    pin = find_isa_irq_pin(8, mp_INT);
-    apic = find_isa_irq_apic(8, mp_INT);
+    pin = io_apic_find_isa_irq_pin(8, mp_INT);
+    apic = io_apic_find_isa_irq_apic(8, mp_INT);
     if ( pin == -1 || apic == -1 )
         return;
 
@@ -1913,8 +1913,8 @@ static void __init check_timer(void)
     /*timer_ack = 1;*/
     /*enable_8259A_irq(irq_to_desc(0));*/
 
-    pin1  = find_isa_irq_pin(0, mp_INT);
-    apic1 = find_isa_irq_apic(0, mp_INT);
+    pin1  = io_apic_find_isa_irq_pin(0, mp_INT);
+    apic1 = io_apic_find_isa_irq_apic(0, mp_INT);
     pin2  = ioapic_i8259.pin;
     apic2 = ioapic_i8259.apic;
 
diff --git a/xen/include/asm-x86/hvm/irq.h b/xen/include/asm-x86/hvm/irq.h
index 532880d497..aa034bc73c 100644
--- a/xen/include/asm-x86/hvm/irq.h
+++ b/xen/include/asm-x86/hvm/irq.h
@@ -100,7 +100,7 @@ struct hvm_irq {
 #define hvm_domain_irq(d) ((d)->arch.hvm.irq)
 #define hvm_irq_size(cnt) offsetof(struct hvm_irq, gsi_assert_count[cnt])
 
-#define hvm_isa_irq_to_gsi(isa_irq) ((isa_irq) ? : 2)
+unsigned int hvm_isa_irq_to_gsi(const struct domain *d, unsigned int irq);
 
 /* Check/Acknowledge next pending interrupt. */
 struct hvm_intack hvm_vcpu_has_pending_irq(struct vcpu *v);
diff --git a/xen/include/asm-x86/io_apic.h b/xen/include/asm-x86/io_apic.h
index e006b2b8dd..1c63d1df56 100644
--- a/xen/include/asm-x86/io_apic.h
+++ b/xen/include/asm-x86/io_apic.h
@@ -205,4 +205,7 @@ unsigned highest_gsi(void);
 int ioapic_guest_read( unsigned long physbase, unsigned int reg, u32 *pval);
 int ioapic_guest_write(unsigned long physbase, unsigned int reg, u32 pval);
 
+int io_apic_find_isa_irq_pin(int irq, int type);
+int io_apic_find_isa_irq_apic(int irq, int type);
+
 #endif
-- 
2.26.2

[PATCH for-4.14 8/8] x86/hvm: enable emulated PIT for PVH dom0

[Roger Pau Monne]

Some video BIOS require a PIT in order to work properly, hence classic
PV dom0 gets partial access to the physical PIT as long as it's not in
use by Xen.

Since PVH dom0 is built on top of HVM support, there's already an
emulated PIT implementation available for use. Tweak the emulated PIT
code so it injects interrupts directly into the vIO-APIC if the legacy
PIC (i8259) is disabled. Make sure the GSI used matches the ISA IRQ 0
in the likely case there's an interrupt overwrite in the MADT ACPI
table.

Finally prevent the passthrough of the GSI that belongs to the PIT,
since interrupts will be generated by the emulated PIT instead of the
physical one.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/domain.c      |  5 +++--
 xen/arch/x86/emul-i8254.c  | 12 +++++++++---
 xen/arch/x86/hvm/vioapic.c |  9 ++++++++-
 3 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index fee6c3931a..dc0b4c2284 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -512,7 +512,8 @@ static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
     if ( is_hvm_domain(d) )
     {
         if ( is_hardware_domain(d) &&
-             emflags != (X86_EMU_VPCI | X86_EMU_LAPIC | X86_EMU_IOAPIC) )
+             emflags != (X86_EMU_VPCI | X86_EMU_LAPIC | X86_EMU_IOAPIC |
+                         X86_EMU_PIT) )
             return false;
         if ( !is_hardware_domain(d) &&
              emflags != (X86_EMU_ALL & ~X86_EMU_VPCI) &&
@@ -578,7 +579,7 @@ int arch_domain_create(struct domain *d,
 
     emflags = config->arch.emulation_flags;
 
-    if ( is_hardware_domain(d) && is_pv_domain(d) )
+    if ( is_hardware_domain(d) )
         emflags |= XEN_X86_EMU_PIT;
 
     if ( emflags & ~XEN_X86_EMU_ALL )
diff --git a/xen/arch/x86/emul-i8254.c b/xen/arch/x86/emul-i8254.c
index 73be4188ad..5aef0fe852 100644
--- a/xen/arch/x86/emul-i8254.c
+++ b/xen/arch/x86/emul-i8254.c
@@ -168,6 +168,7 @@ static void pit_load_count(PITState *pit, int channel, int val)
     u32 period;
     struct hvm_hw_pit_channel *s = &pit->hw.channels[channel];
     struct vcpu *v = vpit_vcpu(pit);
+    const struct domain *d = v ? v->domain : NULL;
 
     ASSERT(spin_is_locked(&pit->lock));
 
@@ -190,14 +191,18 @@ static void pit_load_count(PITState *pit, int channel, int val)
     case 3:
         /* Periodic timer. */
         TRACE_2D(TRC_HVM_EMUL_PIT_START_TIMER, period, period);
-        create_periodic_time(v, &pit->pt0, period, period, 0, pit_time_fired, 
+        create_periodic_time(v, &pit->pt0, period, period,
+                             has_vpic(d) ? 0 : hvm_isa_irq_to_gsi(d, 0),
+                             pit_time_fired,
                              &pit->count_load_time[channel], false);
         break;
     case 1:
     case 4:
         /* One-shot timer. */
         TRACE_2D(TRC_HVM_EMUL_PIT_START_TIMER, period, 0);
-        create_periodic_time(v, &pit->pt0, period, 0, 0, pit_time_fired,
+        create_periodic_time(v, &pit->pt0, period, 0,
+                             has_vpic(d) ? 0 : hvm_isa_irq_to_gsi(d, 0),
+                             pit_time_fired,
                              &pit->count_load_time[channel], false);
         break;
     default:
@@ -455,7 +460,8 @@ void pit_reset(struct domain *d)
     {
         TRACE_0D(TRC_HVM_EMUL_PIT_STOP_TIMER);
         destroy_periodic_time(&pit->pt0);
-        pit->pt0.source = PTSRC_isa;
+        ASSERT(has_vpic(d) || has_vioapic(d));
+        pit->pt0.source = has_vpic(d) ? PTSRC_isa : PTSRC_ioapic;
     }
 
     spin_lock(&pit->lock);
diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
index 34bec715b7..8b95e4412f 100644
--- a/xen/arch/x86/hvm/vioapic.c
+++ b/xen/arch/x86/hvm/vioapic.c
@@ -268,7 +268,14 @@ static void vioapic_write_redirent(
 
     spin_unlock(&d->arch.hvm.irq_lock);
 
-    if ( is_hardware_domain(d) && unmasked )
+    if ( is_hardware_domain(d) && unmasked &&
+         /*
+          * A PVH dom0 can have an emulated PIT that should respect any
+          * interrupt overwrites found in the ACPI MADT table, so we need to
+          * check to which GSI the ISA IRQ 0 is mapped in order to prevent
+          * identity mapping it.
+          */
+         (!has_vpit(d) || gsi != hvm_isa_irq_to_gsi(d, 0)) )
     {
         /*
          * NB: don't call vioapic_hwdom_map_gsi while holding hvm.irq_lock
-- 
2.26.2

RE: [PATCH for-4.14 1/8] x86/hvm: fix vIO-APIC build without IRQ0_SPECIAL_ROUTING

[Paul Durrant]

> -----Original Message-----
> From: Roger Pau Monne <roger.pau@citrix.com>
> Sent: 12 June 2020 16:57
> 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/8] x86/hvm: fix vIO-APIC build without IRQ0_SPECIAL_ROUTING
> 
> pit_channel0_enabled needs to be guarded with IRQ0_SPECIAL_ROUTING
> since it's only used when the special handling of ISA IRQ 0 is enabled.
> 
> No functional change.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  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 b87facb0e0..bd41036137 100644
> --- a/xen/arch/x86/hvm/vioapic.c
> +++ b/xen/arch/x86/hvm/vioapic.c
> @@ -391,10 +391,12 @@ static void ioapic_inj_irq(
>      vlapic_set_irq(target, vector, trig_mode);
>  }
> 
> +#ifdef IRQ0_SPECIAL_ROUTING
>  static inline int pit_channel0_enabled(void)
>  {
>      return pt_active(&current->domain->arch.vpit.pt0);
>  }
> +#endif

It's only called in two places. How about just manually inlining?

  Paul

> 
>  static void vioapic_deliver(struct hvm_vioapic *vioapic, unsigned int pin)
>  {
> --
> 2.26.2

Re: [PATCH for-4.14 1/8] x86/hvm: fix vIO-APIC build without IRQ0_SPECIAL_ROUTING

[Roger Pau Monné]

On Mon, Jun 15, 2020 at 11:00:38AM +0100, Paul Durrant wrote:
> > -----Original Message-----
> > From: Roger Pau Monne <roger.pau@citrix.com>
> > Sent: 12 June 2020 16:57
> > 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/8] x86/hvm: fix vIO-APIC build without IRQ0_SPECIAL_ROUTING
> > 
> > pit_channel0_enabled needs to be guarded with IRQ0_SPECIAL_ROUTING
> > since it's only used when the special handling of ISA IRQ 0 is enabled.
> > 
> > No functional change.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> >  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 b87facb0e0..bd41036137 100644
> > --- a/xen/arch/x86/hvm/vioapic.c
> > +++ b/xen/arch/x86/hvm/vioapic.c
> > @@ -391,10 +391,12 @@ static void ioapic_inj_irq(
> >      vlapic_set_irq(target, vector, trig_mode);
> >  }
> > 
> > +#ifdef IRQ0_SPECIAL_ROUTING
> >  static inline int pit_channel0_enabled(void)
> >  {
> >      return pt_active(&current->domain->arch.vpit.pt0);
> >  }
> > +#endif
> 
> It's only called in two places. How about just manually inlining?

That would be fine, as I'm also removing one of the callers in a
following patch.

Thanks, Roger.

Re: [PATCH for-4.14 8/8] x86/hvm: enable emulated PIT for PVH dom0

[Andrew Cooper]

On 12/06/2020 16:56, Roger Pau Monne wrote:
> Some video BIOS require a PIT in order to work properly, hence classic
> PV dom0 gets partial access to the physical PIT as long as it's not in
> use by Xen.

Is this actually true today?

I can believe that it may have been necessary on old hardware, but the
structure of systems has changed massively over the past 20 years, and
the PIT is very legacy these days.

We shouldn't be blindly propagating bodges like this forward.

~Andrew

Re: [PATCH for-4.14 8/8] x86/hvm: enable emulated PIT for PVH dom0

[Roger Pau Monné]

On Mon, Jun 15, 2020 at 04:33:33PM +0100, Andrew Cooper wrote:
> On 12/06/2020 16:56, Roger Pau Monne wrote:
> > Some video BIOS require a PIT in order to work properly, hence classic
> > PV dom0 gets partial access to the physical PIT as long as it's not in
> > use by Xen.
> 
> Is this actually true today?

TBH I have no idea and asked the same thing myself.

> I can believe that it may have been necessary on old hardware, but the
> structure of systems has changed massively over the past 20 years, and
> the PIT is very legacy these days.

I also wondered whether video BIOSes really changed in the last 20
years, I really have no idea. FWIW, Wikipedia still lists PIT as being
used by video BIOSes on x86 systems [0] but there are no references to
specific models or video BIOSes.

Alternatively we could add this as a Xen command line option, ie:
dom0=pit or some such. It's however not very nice to not get output
because the video BIOS doesn't function properly due to lack of PIT.

Thanks, Roger.

[0] https://en.wikipedia.org/wiki/Intel_8253#IBM_PC_programming_tips_and_hints

Re: [PATCH for-4.14 2/8] x86/hvm: don't force vCPU 0 for IRQ 0 when using fixed destination mode

[Jan Beulich]

On 12.06.2020 17:56, Roger Pau Monne wrote:
> When the IO APIC pin mapped to the ISA IRQ 0 has been configured to
> use fixed delivery mode do not forcefully route interrupts to vCPU 0,
> as the OS might have setup those interrupts to be injected to a
> different vCPU, and injecting to vCPU 0 can cause the OS to miss such
> interrupts or errors to happen due to unexpected vectors being
> injected on vCPU 0.
> 
> In order to fix remove such handling altogether for fixed destination
> mode pins and just inject them according to the data setup in the
> IO-APIC entry.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

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

I wonder though why this was done in the first place - it very much
feels like a workaround for certain guest behavior, and hence
getting rid of it may mean a certain risk of regressions. Not a
very good point in time to make risky changes ...

Jan

Re: [PATCH for-4.14 2/8] x86/hvm: don't force vCPU 0 for IRQ 0 when using fixed destination mode

[Roger Pau Monné]

On Thu, Jun 18, 2020 at 03:43:00PM +0200, Jan Beulich wrote:
> On 12.06.2020 17:56, Roger Pau Monne wrote:
> > When the IO APIC pin mapped to the ISA IRQ 0 has been configured to
> > use fixed delivery mode do not forcefully route interrupts to vCPU 0,
> > as the OS might have setup those interrupts to be injected to a
> > different vCPU, and injecting to vCPU 0 can cause the OS to miss such
> > interrupts or errors to happen due to unexpected vectors being
> > injected on vCPU 0.
> > 
> > In order to fix remove such handling altogether for fixed destination
> > mode pins and just inject them according to the data setup in the
> > IO-APIC entry.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Technically
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> I wonder though why this was done in the first place - it very much
> feels like a workaround for certain guest behavior, and hence
> getting rid of it may mean a certain risk of regressions. Not a
> very good point in time to make risky changes ...

We can defer to after the release I guess, but I will still ask for
the changes to be backported.

What we currently do is broken, up to the point that FreeBSD cannot
use the PIT because it will likely route the interrupt to a vCPU != 0
in fixed mode, and then it will just get stuck because the vector is
delivered to vCPU 0 where it's not even configured.

Roger.

Re: [PATCH for-4.14 2/8] x86/hvm: don't force vCPU 0 for IRQ 0 when using fixed destination mode

[Jan Beulich]

On 18.06.2020 15:48, Roger Pau Monné wrote:
> On Thu, Jun 18, 2020 at 03:43:00PM +0200, Jan Beulich wrote:
>> On 12.06.2020 17:56, Roger Pau Monne wrote:
>>> When the IO APIC pin mapped to the ISA IRQ 0 has been configured to
>>> use fixed delivery mode do not forcefully route interrupts to vCPU 0,
>>> as the OS might have setup those interrupts to be injected to a
>>> different vCPU, and injecting to vCPU 0 can cause the OS to miss such
>>> interrupts or errors to happen due to unexpected vectors being
>>> injected on vCPU 0.
>>>
>>> In order to fix remove such handling altogether for fixed destination
>>> mode pins and just inject them according to the data setup in the
>>> IO-APIC entry.
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>
>> Technically
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>
>> I wonder though why this was done in the first place - it very much
>> feels like a workaround for certain guest behavior, and hence
>> getting rid of it may mean a certain risk of regressions. Not a
>> very good point in time to make risky changes ...
> 
> We can defer to after the release I guess, but I will still ask for
> the changes to be backported.

That's fine, albeit if we decide to delay it until 4.15 was branched,
then I think we want to also wait longer than usual until it would hit
the stable trees. Unfortunately c8e79412c001's description is of no
help to understand what or why "time jumps" may result from delivering
the interrupt as requested.

> What we currently do is broken, up to the point that FreeBSD cannot
> use the PIT because it will likely route the interrupt to a vCPU != 0
> in fixed mode, and then it will just get stuck because the vector is
> delivered to vCPU 0 where it's not even configured.

All understood.

Jan

Re: [PATCH for-4.14 2/8] x86/hvm: don't force vCPU 0 for IRQ 0 when using fixed destination mode

[Roger Pau Monné]

On Thu, Jun 18, 2020 at 04:08:28PM +0200, Jan Beulich wrote:
> On 18.06.2020 15:48, Roger Pau Monné wrote:
> > On Thu, Jun 18, 2020 at 03:43:00PM +0200, Jan Beulich wrote:
> >> On 12.06.2020 17:56, Roger Pau Monne wrote:
> >>> When the IO APIC pin mapped to the ISA IRQ 0 has been configured to
> >>> use fixed delivery mode do not forcefully route interrupts to vCPU 0,
> >>> as the OS might have setup those interrupts to be injected to a
> >>> different vCPU, and injecting to vCPU 0 can cause the OS to miss such
> >>> interrupts or errors to happen due to unexpected vectors being
> >>> injected on vCPU 0.
> >>>
> >>> In order to fix remove such handling altogether for fixed destination
> >>> mode pins and just inject them according to the data setup in the
> >>> IO-APIC entry.
> >>>
> >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >>
> >> Technically
> >> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> >>
> >> I wonder though why this was done in the first place - it very much
> >> feels like a workaround for certain guest behavior, and hence
> >> getting rid of it may mean a certain risk of regressions. Not a
> >> very good point in time to make risky changes ...
> > 
> > We can defer to after the release I guess, but I will still ask for
> > the changes to be backported.
> 
> That's fine, albeit if we decide to delay it until 4.15 was branched,
> then I think we want to also wait longer than usual until it would hit
> the stable trees. Unfortunately c8e79412c001's description is of no
> help to understand what or why "time jumps" may result from delivering
> the interrupt as requested.

Yes, I've also looked at the original commit and have no idea what it
was actually trying to fix, and why delivering to vCPU 0 fixed it.
FWIW, I tried delivering to a different vCPU and it seems to work
fine.

Note that other timers (ie: RTC or HPET) are not tied to vCPU 0, so it
must have been something related to the PIT?

Thanks, Roger.

Re: [PATCH for-4.14 3/8] x86/hvm: fix ISA IRQ 0 handling when set as lowest priority mode in IO APIC

[Jan Beulich]

On 12.06.2020 17:56, Roger Pau Monne wrote:
> --- a/xen/arch/x86/hvm/vioapic.c
> +++ b/xen/arch/x86/hvm/vioapic.c
> @@ -422,12 +422,13 @@ static void vioapic_deliver(struct hvm_vioapic *vioapic, unsigned int pin)
>      case dest_LowestPrio:
>      {
>  #ifdef IRQ0_SPECIAL_ROUTING
> -        /* Force round-robin to pick VCPU 0 */
> -        if ( (irq == hvm_isa_irq_to_gsi(0)) && pit_channel0_enabled() )
> -        {
> -            v = d->vcpu ? d->vcpu[0] : NULL;
> -            target = v ? vcpu_vlapic(v) : NULL;
> -        }
> +        struct vlapic *lapic0 = vcpu_vlapic(d->vcpu[0]);
> +
> +        /* Force to pick vCPU 0 if part of the destination list */
> +        if ( (irq == hvm_isa_irq_to_gsi(0)) && pit_channel0_enabled() &&
> +             vlapic_match_dest(lapic0, NULL, 0, dest, dest_mode) &&
> +             vlapic_enabled(lapic0) )

The vlapic_enabled() part needs justification in the commit message
(if it is to stay), the more that the other path that patch 2 touched
doesn't have / gain it. I'm unconvinced this is a helpful check here
(or anywhere when it's not current's LAPIC that gets probed), as its
result may be stale right after probing.

Having thought about this (including patch 2) some more, I also wonder
whether, if no destination match was found, the IRQ0_SPECIAL_ROUTING
hack should become to nevertheless deliver to CPU0.

Jan

Re: [PATCH for-4.14 2/8] x86/hvm: don't force vCPU 0 for IRQ 0 when using fixed destination mode

[Jan Beulich]

On 18.06.2020 16:18, Roger Pau Monné wrote:
> On Thu, Jun 18, 2020 at 04:08:28PM +0200, Jan Beulich wrote:
>> On 18.06.2020 15:48, Roger Pau Monné wrote:
>>> On Thu, Jun 18, 2020 at 03:43:00PM +0200, Jan Beulich wrote:
>>>> On 12.06.2020 17:56, Roger Pau Monne wrote:
>>>>> When the IO APIC pin mapped to the ISA IRQ 0 has been configured to
>>>>> use fixed delivery mode do not forcefully route interrupts to vCPU 0,
>>>>> as the OS might have setup those interrupts to be injected to a
>>>>> different vCPU, and injecting to vCPU 0 can cause the OS to miss such
>>>>> interrupts or errors to happen due to unexpected vectors being
>>>>> injected on vCPU 0.
>>>>>
>>>>> In order to fix remove such handling altogether for fixed destination
>>>>> mode pins and just inject them according to the data setup in the
>>>>> IO-APIC entry.
>>>>>
>>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>>>
>>>> Technically
>>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>>>
>>>> I wonder though why this was done in the first place - it very much
>>>> feels like a workaround for certain guest behavior, and hence
>>>> getting rid of it may mean a certain risk of regressions. Not a
>>>> very good point in time to make risky changes ...
>>>
>>> We can defer to after the release I guess, but I will still ask for
>>> the changes to be backported.
>>
>> That's fine, albeit if we decide to delay it until 4.15 was branched,
>> then I think we want to also wait longer than usual until it would hit
>> the stable trees. Unfortunately c8e79412c001's description is of no
>> help to understand what or why "time jumps" may result from delivering
>> the interrupt as requested.
> 
> Yes, I've also looked at the original commit and have no idea what it
> was actually trying to fix, and why delivering to vCPU 0 fixed it.
> FWIW, I tried delivering to a different vCPU and it seems to work
> fine.

Right, I too was thinking that delivering to a "stable" CPU might be
all that's needed. In patch 3 this may then call for latching that
CPU, and preferring it over what vlapic_lowest_prio() produces.

> Note that other timers (ie: RTC or HPET) are not tied to vCPU 0, so it
> must have been something related to the PIT?

Likely, but it may easily have been that an issue was papered over
by this change. Then we won't even know whether that underlying issue
still exists.

Jan

Re: [PATCH for-4.14 4/8] x86/vpt: only try to resume timers belonging to enabled devices

[Jan Beulich]

On 12.06.2020 17:56, Roger Pau Monne wrote:
> Check whether the emulated device is actually enabled before trying to
> resume the associated timers.
> 
> Thankfully all those structures are zeroed at initialization, and
> since the devices are not enabled they are never populated, which
> triggers the pt->vcpu check at the beginning of pt_resume forcing an
> exit from the function.

So really this is a benign transformation then, rather than fixing
anything? If that's correct understanding of mine ...

> While there limit the scope of i and make it unsigned.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

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

Jan

Re: [PATCH for-4.14 5/8] x86/hvm: only translate ISA interrupts to GSIs in virtual timers

[Jan Beulich]

On 12.06.2020 17:56, Roger Pau Monne wrote:
> Only call hvm_isa_irq_to_gsi for ISA interrupts, interrupts
> originating from an IO APIC pin already use a GSI and don't need to be
> translated.
> 
> I haven't observed any issues from this, but I think it's better to
> use it correctly.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

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

However, ...

> --- a/xen/arch/x86/hvm/vpt.c
> +++ b/xen/arch/x86/hvm/vpt.c
> @@ -86,7 +86,7 @@ static int pt_irq_vector(struct periodic_time *pt, enum hvm_intsrc src)
>          return pt->irq;
>  
>      isa_irq = pt->irq;
> -    gsi = hvm_isa_irq_to_gsi(isa_irq);
> +    gsi = pt->source == PTSRC_isa ? hvm_isa_irq_to_gsi(isa_irq) : pt->irq;

... would you mind taking the opportunity and moving this ...

>      if ( src == hvm_intsrc_pic )
>          return (v->domain->arch.hvm.vpic[isa_irq >> 3].irq_base

... below here, perhaps even past the ASSERT() that follows?

(I have to admit that I find the two kinds of "source" indicators
- the "src" function parameter and "pt->source" confusing. Aren't
they supposed to match up?)

Jan

Re: [PATCH for-4.14 2/8] x86/hvm: don't force vCPU 0 for IRQ 0 when using fixed destination mode

[Roger Pau Monné]

On Thu, Jun 18, 2020 at 04:29:59PM +0200, Jan Beulich wrote:
> On 18.06.2020 16:18, Roger Pau Monné wrote:
> > On Thu, Jun 18, 2020 at 04:08:28PM +0200, Jan Beulich wrote:
> >> On 18.06.2020 15:48, Roger Pau Monné wrote:
> >>> On Thu, Jun 18, 2020 at 03:43:00PM +0200, Jan Beulich wrote:
> >>>> On 12.06.2020 17:56, Roger Pau Monne wrote:
> >>>>> When the IO APIC pin mapped to the ISA IRQ 0 has been configured to
> >>>>> use fixed delivery mode do not forcefully route interrupts to vCPU 0,
> >>>>> as the OS might have setup those interrupts to be injected to a
> >>>>> different vCPU, and injecting to vCPU 0 can cause the OS to miss such
> >>>>> interrupts or errors to happen due to unexpected vectors being
> >>>>> injected on vCPU 0.
> >>>>>
> >>>>> In order to fix remove such handling altogether for fixed destination
> >>>>> mode pins and just inject them according to the data setup in the
> >>>>> IO-APIC entry.
> >>>>>
> >>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >>>>
> >>>> Technically
> >>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> >>>>
> >>>> I wonder though why this was done in the first place - it very much
> >>>> feels like a workaround for certain guest behavior, and hence
> >>>> getting rid of it may mean a certain risk of regressions. Not a
> >>>> very good point in time to make risky changes ...
> >>>
> >>> We can defer to after the release I guess, but I will still ask for
> >>> the changes to be backported.
> >>
> >> That's fine, albeit if we decide to delay it until 4.15 was branched,
> >> then I think we want to also wait longer than usual until it would hit
> >> the stable trees. Unfortunately c8e79412c001's description is of no
> >> help to understand what or why "time jumps" may result from delivering
> >> the interrupt as requested.
> > 
> > Yes, I've also looked at the original commit and have no idea what it
> > was actually trying to fix, and why delivering to vCPU 0 fixed it.
> > FWIW, I tried delivering to a different vCPU and it seems to work
> > fine.
> 
> Right, I too was thinking that delivering to a "stable" CPU might be
> all that's needed. In patch 3 this may then call for latching that
> CPU, and preferring it over what vlapic_lowest_prio() produces.

Yes, I also considered that route for the lowest priority mode (which
is dealt with in the next patch), but for fixed mode we need to
delivered to the listed vCPUs, there's no trick we can play here
IMO.

Roger.

Re: [PATCH for-4.14 3/8] x86/hvm: fix ISA IRQ 0 handling when set as lowest priority mode in IO APIC

[Roger Pau Monné]

On Thu, Jun 18, 2020 at 04:26:08PM +0200, Jan Beulich wrote:
> On 12.06.2020 17:56, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/hvm/vioapic.c
> > +++ b/xen/arch/x86/hvm/vioapic.c
> > @@ -422,12 +422,13 @@ static void vioapic_deliver(struct hvm_vioapic *vioapic, unsigned int pin)
> >      case dest_LowestPrio:
> >      {
> >  #ifdef IRQ0_SPECIAL_ROUTING
> > -        /* Force round-robin to pick VCPU 0 */
> > -        if ( (irq == hvm_isa_irq_to_gsi(0)) && pit_channel0_enabled() )
> > -        {
> > -            v = d->vcpu ? d->vcpu[0] : NULL;
> > -            target = v ? vcpu_vlapic(v) : NULL;
> > -        }
> > +        struct vlapic *lapic0 = vcpu_vlapic(d->vcpu[0]);
> > +
> > +        /* Force to pick vCPU 0 if part of the destination list */
> > +        if ( (irq == hvm_isa_irq_to_gsi(0)) && pit_channel0_enabled() &&
> > +             vlapic_match_dest(lapic0, NULL, 0, dest, dest_mode) &&
> > +             vlapic_enabled(lapic0) )
> 
> The vlapic_enabled() part needs justification in the commit message
> (if it is to stay), the more that the other path that patch 2 touched
> doesn't have / gain it. I'm unconvinced this is a helpful check here
> (or anywhere when it's not current's LAPIC that gets probed), as its
> result may be stale right after probing.

This is modeled after what vlapic_lowest_prio does, which includes the
vlapic_enabled check. I assumed this was done to prevent injecting to
disabled lapics if possible.

I agree it's stale by the point it gets acted upon, but anyone playing
with enabling/disabling a lapic part of a destination list shouldn't
expect anything sensible to happen IMO.

> Having thought about this (including patch 2) some more, I also wonder
> whether, if no destination match was found, the IRQ0_SPECIAL_ROUTING
> hack should become to nevertheless deliver to CPU0.

Hm, that wouldn't match what real hardware would do, but would indeed
match what old Xen would do for IRQ 0. TBH I would be more comfortable
with attempting to remove this behaviour, and hence don't inject to
any vCPU if none match the list.

Thanks, Roger.

Re: [PATCH for-4.14 4/8] x86/vpt: only try to resume timers belonging to enabled devices

[Roger Pau Monné]

On Thu, Jun 18, 2020 at 04:37:57PM +0200, Jan Beulich wrote:
> On 12.06.2020 17:56, Roger Pau Monne wrote:
> > Check whether the emulated device is actually enabled before trying to
> > resume the associated timers.
> > 
> > Thankfully all those structures are zeroed at initialization, and
> > since the devices are not enabled they are never populated, which
> > triggers the pt->vcpu check at the beginning of pt_resume forcing an
> > exit from the function.
> 
> So really this is a benign transformation then, rather than fixing
> anything? If that's correct understanding of mine ...

Yes, that's my understanding also.

> > While there limit the scope of i and make it unsigned.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks, Roger.

Re: [PATCH for-4.14 5/8] x86/hvm: only translate ISA interrupts to GSIs in virtual timers

[Roger Pau Monné]

On Thu, Jun 18, 2020 at 04:47:57PM +0200, Jan Beulich wrote:
> On 12.06.2020 17:56, Roger Pau Monne wrote:
> > Only call hvm_isa_irq_to_gsi for ISA interrupts, interrupts
> > originating from an IO APIC pin already use a GSI and don't need to be
> > translated.
> > 
> > I haven't observed any issues from this, but I think it's better to
> > use it correctly.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> However, ...
> 
> > --- a/xen/arch/x86/hvm/vpt.c
> > +++ b/xen/arch/x86/hvm/vpt.c
> > @@ -86,7 +86,7 @@ static int pt_irq_vector(struct periodic_time *pt, enum hvm_intsrc src)
> >          return pt->irq;
> >  
> >      isa_irq = pt->irq;
> > -    gsi = hvm_isa_irq_to_gsi(isa_irq);
> > +    gsi = pt->source == PTSRC_isa ? hvm_isa_irq_to_gsi(isa_irq) : pt->irq;
> 
> ... would you mind taking the opportunity and moving this ...
> 
> >      if ( src == hvm_intsrc_pic )
> >          return (v->domain->arch.hvm.vpic[isa_irq >> 3].irq_base
> 
> ... below here, perhaps even past the ASSERT() that follows?
> 
> (I have to admit that I find the two kinds of "source" indicators
> - the "src" function parameter and "pt->source" confusing. Aren't
> they supposed to match up?)

They are supposed to match when the injected interrupt is the timer
one, if there's a highest priority interrupt that gets injected
instead of the timer one they don't match.

AFAICT the function it's trying to get the vector that would match the
timer using the 'src' interrupt source. TBH I think this is way more
complex than needed, but I don't plan to deal with it right now.

Thanks, Roger.

Re: [PATCH for-4.14 6/8] x86/vpt: fix injection to remote vCPU

[Jan Beulich]

On 12.06.2020 17:56, Roger Pau Monne wrote:
> vpt timers are usually added to the per-vCPU list of the vCPU where
> they get setup, but depending on the timer source type that vCPU might
> be different than the one where the interrupt vector gets injected.
> 
> For example the PIT timer use a PIC or IO-APIC pin in order to select
> the destination vCPU and vector, which might not match the vCPU they
> are configured from.
> 
> If such a situation happens pt_intr_post won't be called, and thus the
> vpt will be left in a limbo where the next interrupt won't be
> scheduled. Fix this by generalizing the special handling done to
> IO-APIC level interrupts to be applied always when the destination
> vCPU of the injected vector is different from the vCPU where the vpt
> belongs to (ie: usually the one it's been configured from).
> 
> A further improvement as noted in a comment added to the code might be
> to move the vpt so it's handled by the same vCPU where the vector gets
> injected.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/arch/x86/hvm/vpt.c | 80 +++++++++++++++++++++---------------------
>  1 file changed, 40 insertions(+), 40 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/vpt.c b/xen/arch/x86/hvm/vpt.c
> index 6a975fc668..52ad5b90a7 100644
> --- a/xen/arch/x86/hvm/vpt.c
> +++ b/xen/arch/x86/hvm/vpt.c
> @@ -358,59 +358,59 @@ int pt_update_irq(struct vcpu *v)
>           * interrupt delivery case. Otherwise return -1 to do nothing.
>           */
>          vlapic_set_irq(vcpu_vlapic(v), irq, 0);
> -        pt_vector = irq;
> -        break;
> +        return irq;
>  
>      case PTSRC_isa:
>          hvm_isa_irq_deassert(v->domain, irq);
>          if ( platform_legacy_irq(irq) && vlapic_accept_pic_intr(v) &&
>               v->domain->arch.hvm.vpic[irq >> 3].int_output )
> -            hvm_isa_irq_assert(v->domain, irq, NULL);
> +            pt_vector = hvm_isa_irq_assert(v->domain, irq, NULL);
>          else
> -        {
>              pt_vector = hvm_isa_irq_assert(v->domain, irq, vioapic_get_vector);
> -            /*
> -             * hvm_isa_irq_assert may not set the corresponding bit in vIRR
> -             * when mask field of IOAPIC RTE is set. Check it again.
> -             */

For one, the transformation done here looks to call for folding
both calls to hvm_isa_irq_assert() into one. I'm not, however,
convinced recording the function's return value is useful in the
case where it wasn't recorded before. The change is benign right
now because hvm_isa_irq_assert() will return -1 when its last
argument is NULL, but the question is whether the code here should
start depending on such behavior.

And then, according to this comment (which doesn't get retained in
any form or shape) ...

> -            if ( pt_vector < 0 || !vlapic_test_irq(vcpu_vlapic(v), pt_vector) )
> -                pt_vector = -1;
> -        }
> +
> +        if ( pt_vector < 0 )
> +            return pt_vector;
> +
>          break;
>  
>      case PTSRC_ioapic:
>          pt_vector = hvm_ioapic_assert(v->domain, irq, level);
> -        if ( pt_vector < 0 || !vlapic_test_irq(vcpu_vlapic(v), pt_vector) )
> -        {
> -            pt_vector = -1;
> -            if ( level )
> +        if ( pt_vector < 0 )
> +            return pt_vector;
> +
> +        break;
> +    }
> +
> +    ASSERT(pt_vector >= 0);
> +    if ( !vlapic_test_irq(vcpu_vlapic(v), pt_vector) )
> +    {
> +        time_cb *cb = NULL;
> +        void *cb_priv;
> +
> +        /*
> +         * Vector has been injected to a different vCPU, call pt_irq_fired and
> +         * execute the callback, since the destination vCPU(s) won't call
> +         * pt_intr_post for it.

... this isn't the only reason to come here. Beyond what the comment
says there is the hvm_domain_use_pirq() check in assert_gsi() which
would similarly result in the IRR bit not observed set here. At the
very least these cases want mentioning; I have to admit that I'm not
entirely clear yet whether your handling is correct for both, or
whether the information needs to be propagated into here.

Also instead of ASSERT(pt_vector >= 0) would you pull the respective
if() out of the switch(), to also cover the case of a fall through
without hitting any of the explicitly handled cases, resulting in
pt_vector left at its initial value of -1?

> +         * TODO: move this vpt to one of the vCPUs where the vector gets
> +         * injected.
> +         */
> +        spin_lock(&v->arch.hvm.tm_lock);
> +        /* Make sure the timer is still on the list. */
> +        list_for_each_entry ( pt, &v->arch.hvm.tm_list, list )
> +            if ( pt == earliest_pt )
>              {
> -                /*
> -                 * Level interrupts are always asserted because the pin assert
> -                 * count is incremented regardless of whether the pin is masked
> -                 * or the vector latched in IRR, so also execute the callback
> -                 * associated with the timer.
> -                 */
> -                time_cb *cb = NULL;
> -                void *cb_priv;
> -
> -                spin_lock(&v->arch.hvm.tm_lock);
> -                /* Make sure the timer is still on the list. */
> -                list_for_each_entry ( pt, &v->arch.hvm.tm_list, list )
> -                    if ( pt == earliest_pt )
> -                    {
> -                        pt_irq_fired(v, pt);
> -                        cb = pt->cb;
> -                        cb_priv = pt->priv;
> -                        break;
> -                    }
> -                spin_unlock(&v->arch.hvm.tm_lock);
> -
> -                if ( cb != NULL )
> -                    cb(v, cb_priv);
> +                pt_irq_fired(v, pt);
> +                cb = pt->cb;
> +                cb_priv = pt->priv;
> +                break;
>              }
> -        }
> -        break;
> +        spin_unlock(&v->arch.hvm.tm_lock);
> +
> +        if ( cb != NULL )
> +            cb(v, cb_priv);
> +
> +        pt_vector = -1;
>      }
>  
>      return pt_vector;

To further reduce indentation (and seeing the significant code
churn that happens here anyway), could you consider inverting the
surrounding if() to

    if ( vlapic_test_irq(vcpu_vlapic(v), pt_vector) )
        return pt_vector;    

?

Jan

Re: [PATCH for-4.14 2/8] x86/hvm: don't force vCPU 0 for IRQ 0 when using fixed destination mode

[Jan Beulich]

On 18.06.2020 16:49, Roger Pau Monné wrote:
> On Thu, Jun 18, 2020 at 04:29:59PM +0200, Jan Beulich wrote:
>> On 18.06.2020 16:18, Roger Pau Monné wrote:
>>> On Thu, Jun 18, 2020 at 04:08:28PM +0200, Jan Beulich wrote:
>>>> On 18.06.2020 15:48, Roger Pau Monné wrote:
>>>>> On Thu, Jun 18, 2020 at 03:43:00PM +0200, Jan Beulich wrote:
>>>>>> On 12.06.2020 17:56, Roger Pau Monne wrote:
>>>>>>> When the IO APIC pin mapped to the ISA IRQ 0 has been configured to
>>>>>>> use fixed delivery mode do not forcefully route interrupts to vCPU 0,
>>>>>>> as the OS might have setup those interrupts to be injected to a
>>>>>>> different vCPU, and injecting to vCPU 0 can cause the OS to miss such
>>>>>>> interrupts or errors to happen due to unexpected vectors being
>>>>>>> injected on vCPU 0.
>>>>>>>
>>>>>>> In order to fix remove such handling altogether for fixed destination
>>>>>>> mode pins and just inject them according to the data setup in the
>>>>>>> IO-APIC entry.
>>>>>>>
>>>>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>>>>>
>>>>>> Technically
>>>>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>>>>>
>>>>>> I wonder though why this was done in the first place - it very much
>>>>>> feels like a workaround for certain guest behavior, and hence
>>>>>> getting rid of it may mean a certain risk of regressions. Not a
>>>>>> very good point in time to make risky changes ...
>>>>>
>>>>> We can defer to after the release I guess, but I will still ask for
>>>>> the changes to be backported.
>>>>
>>>> That's fine, albeit if we decide to delay it until 4.15 was branched,
>>>> then I think we want to also wait longer than usual until it would hit
>>>> the stable trees. Unfortunately c8e79412c001's description is of no
>>>> help to understand what or why "time jumps" may result from delivering
>>>> the interrupt as requested.
>>>
>>> Yes, I've also looked at the original commit and have no idea what it
>>> was actually trying to fix, and why delivering to vCPU 0 fixed it.
>>> FWIW, I tried delivering to a different vCPU and it seems to work
>>> fine.
>>
>> Right, I too was thinking that delivering to a "stable" CPU might be
>> all that's needed. In patch 3 this may then call for latching that
>> CPU, and preferring it over what vlapic_lowest_prio() produces.
> 
> Yes, I also considered that route for the lowest priority mode (which
> is dealt with in the next patch), but for fixed mode we need to
> delivered to the listed vCPUs, there's no trick we can play here
> IMO.

The set may still be empty, in which case the lowest-prio consideration
(of falling back to CPU0) may still apply here as well. But of course
there's nothing to latch here, as fixed mode means multi-cast if more
than one destination matches.

Jan

Re: [PATCH for-4.14 3/8] x86/hvm: fix ISA IRQ 0 handling when set as lowest priority mode in IO APIC

[Jan Beulich]

On 18.06.2020 16:55, Roger Pau Monné wrote:
> On Thu, Jun 18, 2020 at 04:26:08PM +0200, Jan Beulich wrote:
>> On 12.06.2020 17:56, Roger Pau Monne wrote:
>>> --- a/xen/arch/x86/hvm/vioapic.c
>>> +++ b/xen/arch/x86/hvm/vioapic.c
>>> @@ -422,12 +422,13 @@ static void vioapic_deliver(struct hvm_vioapic *vioapic, unsigned int pin)
>>>      case dest_LowestPrio:
>>>      {
>>>  #ifdef IRQ0_SPECIAL_ROUTING
>>> -        /* Force round-robin to pick VCPU 0 */
>>> -        if ( (irq == hvm_isa_irq_to_gsi(0)) && pit_channel0_enabled() )
>>> -        {
>>> -            v = d->vcpu ? d->vcpu[0] : NULL;
>>> -            target = v ? vcpu_vlapic(v) : NULL;
>>> -        }
>>> +        struct vlapic *lapic0 = vcpu_vlapic(d->vcpu[0]);
>>> +
>>> +        /* Force to pick vCPU 0 if part of the destination list */
>>> +        if ( (irq == hvm_isa_irq_to_gsi(0)) && pit_channel0_enabled() &&
>>> +             vlapic_match_dest(lapic0, NULL, 0, dest, dest_mode) &&
>>> +             vlapic_enabled(lapic0) )
>>
>> The vlapic_enabled() part needs justification in the commit message
>> (if it is to stay), the more that the other path that patch 2 touched
>> doesn't have / gain it. I'm unconvinced this is a helpful check here
>> (or anywhere when it's not current's LAPIC that gets probed), as its
>> result may be stale right after probing.
> 
> This is modeled after what vlapic_lowest_prio does, which includes the
> vlapic_enabled check. I assumed this was done to prevent injecting to
> disabled lapics if possible.

All understood, but it wants justifying like that in the description,
and the discrepancy to the fixed dest mode wants taking care of
(either again verbally, or by a code change).

> I agree it's stale by the point it gets acted upon, but anyone playing
> with enabling/disabling a lapic part of a destination list shouldn't
> expect anything sensible to happen IMO.

Well, yes, agreed.

>> Having thought about this (including patch 2) some more, I also wonder
>> whether, if no destination match was found, the IRQ0_SPECIAL_ROUTING
>> hack should become to nevertheless deliver to CPU0.
> 
> Hm, that wouldn't match what real hardware would do, but would indeed
> match what old Xen would do for IRQ 0. TBH I would be more comfortable
> with attempting to remove this behaviour, and hence don't inject to
> any vCPU if none match the list.

I agree from an abstract perspective. But at the same time I fear
hard to understand / debug regressions. I'd be curious to know what
others think ...

Jan

Re: [PATCH for-4.14 7/8] x86/hvm: add hardware domain support to hvm_isa_irq_to_gsi

[Jan Beulich]

On 12.06.2020 17:56, Roger Pau Monne wrote:
> The current function has the ISA IRQ 0 hardcoded to GSI 2 for HVM
> domUs. Allow such function to also be used by the hardware domain by
> taking into account the ACPI interrupt overwrites in order to get the

Nit: overrides

> --- a/xen/arch/x86/io_apic.c
> +++ b/xen/arch/x86/io_apic.c
> @@ -608,7 +608,7 @@ static int find_irq_entry(int apic, int pin, int type)
>  /*
>   * Find the pin to which IRQ[irq] (ISA) is connected
>   */
> -static int __init find_isa_irq_pin(int irq, int type)
> +int io_apic_find_isa_irq_pin(int irq, int type)
>  {
>      int i;
>  
> @@ -628,7 +628,7 @@ static int __init find_isa_irq_pin(int irq, int type)
>      return -1;
>  }
>  
> -static int __init find_isa_irq_apic(int irq, int type)
> +int io_apic_find_isa_irq_apic(int irq, int type)
>  {

Since you touch these anyway, how about making their parameters
"unsigned int"? Preferably with this
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH for-4.14 8/8] x86/hvm: enable emulated PIT for PVH dom0

[Jan Beulich]

On 12.06.2020 17:56, Roger Pau Monne wrote:
> Some video BIOS require a PIT in order to work properly, hence classic
> PV dom0 gets partial access to the physical PIT as long as it's not in
> use by Xen.
> 
> Since PVH dom0 is built on top of HVM support, there's already an
> emulated PIT implementation available for use. Tweak the emulated PIT
> code so it injects interrupts directly into the vIO-APIC if the legacy
> PIC (i8259) is disabled. Make sure the GSI used matches the ISA IRQ 0
> in the likely case there's an interrupt overwrite in the MADT ACPI

Same nit again as for the earlier patch (also applicable to a code
comment below).

> @@ -578,7 +579,7 @@ int arch_domain_create(struct domain *d,
>  
>      emflags = config->arch.emulation_flags;
>  
> -    if ( is_hardware_domain(d) && is_pv_domain(d) )
> +    if ( is_hardware_domain(d) )
>          emflags |= XEN_X86_EMU_PIT;

Wouldn't this better go into create_dom0(), where all the other
flags get set? Or otherwise all of that be moved here (to cover
the late-hwdom case)?

> --- a/xen/arch/x86/emul-i8254.c
> +++ b/xen/arch/x86/emul-i8254.c
> @@ -168,6 +168,7 @@ static void pit_load_count(PITState *pit, int channel, int val)
>      u32 period;
>      struct hvm_hw_pit_channel *s = &pit->hw.channels[channel];
>      struct vcpu *v = vpit_vcpu(pit);
> +    const struct domain *d = v ? v->domain : NULL;

I think this local variable would better be omitted - its
initializer may raise questions, while at its use sites v
can't be NULL afaict. Plus it's not needed ...

> @@ -190,14 +191,18 @@ static void pit_load_count(PITState *pit, int channel, int val)
>      case 3:
>          /* Periodic timer. */
>          TRACE_2D(TRC_HVM_EMUL_PIT_START_TIMER, period, period);
> -        create_periodic_time(v, &pit->pt0, period, period, 0, pit_time_fired, 
> +        create_periodic_time(v, &pit->pt0, period, period,
> +                             has_vpic(d) ? 0 : hvm_isa_irq_to_gsi(d, 0),
> +                             pit_time_fired,
>                               &pit->count_load_time[channel], false);
>          break;
>      case 1:
>      case 4:
>          /* One-shot timer. */
>          TRACE_2D(TRC_HVM_EMUL_PIT_START_TIMER, period, 0);
> -        create_periodic_time(v, &pit->pt0, period, 0, 0, pit_time_fired,
> +        create_periodic_time(v, &pit->pt0, period, 0,
> +                             has_vpic(d) ? 0 : hvm_isa_irq_to_gsi(d, 0),
> +                             pit_time_fired,
>                               &pit->count_load_time[channel], false);
>          break;
>      default:

... on this path.

> --- a/xen/arch/x86/hvm/vioapic.c
> +++ b/xen/arch/x86/hvm/vioapic.c
> @@ -268,7 +268,14 @@ static void vioapic_write_redirent(
>  
>      spin_unlock(&d->arch.hvm.irq_lock);
>  
> -    if ( is_hardware_domain(d) && unmasked )
> +    if ( is_hardware_domain(d) && unmasked &&
> +         /*
> +          * A PVH dom0 can have an emulated PIT that should respect any
> +          * interrupt overwrites found in the ACPI MADT table, so we need to
> +          * check to which GSI the ISA IRQ 0 is mapped in order to prevent
> +          * identity mapping it.
> +          */
> +         (!has_vpit(d) || gsi != hvm_isa_irq_to_gsi(d, 0)) )

Isn't has_vpit() true now for Dom0, and hence that part of the
condition is kind of pointless? And shouldn't Dom0 never have seen
physical IRQ 0 in the first place (we don't allow PV Dom0 to use
that IRQ either, after all)?

Jan

Re: [PATCH for-4.14 6/8] x86/vpt: fix injection to remote vCPU

[Roger Pau Monné]

On Thu, Jun 18, 2020 at 05:12:17PM +0200, Jan Beulich wrote:
> On 12.06.2020 17:56, Roger Pau Monne wrote:
> > vpt timers are usually added to the per-vCPU list of the vCPU where
> > they get setup, but depending on the timer source type that vCPU might
> > be different than the one where the interrupt vector gets injected.
> > 
> > For example the PIT timer use a PIC or IO-APIC pin in order to select
> > the destination vCPU and vector, which might not match the vCPU they
> > are configured from.
> > 
> > If such a situation happens pt_intr_post won't be called, and thus the
> > vpt will be left in a limbo where the next interrupt won't be
> > scheduled. Fix this by generalizing the special handling done to
> > IO-APIC level interrupts to be applied always when the destination
> > vCPU of the injected vector is different from the vCPU where the vpt
> > belongs to (ie: usually the one it's been configured from).
> > 
> > A further improvement as noted in a comment added to the code might be
> > to move the vpt so it's handled by the same vCPU where the vector gets
> > injected.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> >  xen/arch/x86/hvm/vpt.c | 80 +++++++++++++++++++++---------------------
> >  1 file changed, 40 insertions(+), 40 deletions(-)
> > 
> > diff --git a/xen/arch/x86/hvm/vpt.c b/xen/arch/x86/hvm/vpt.c
> > index 6a975fc668..52ad5b90a7 100644
> > --- a/xen/arch/x86/hvm/vpt.c
> > +++ b/xen/arch/x86/hvm/vpt.c
> > @@ -358,59 +358,59 @@ int pt_update_irq(struct vcpu *v)
> >           * interrupt delivery case. Otherwise return -1 to do nothing.
> >           */
> >          vlapic_set_irq(vcpu_vlapic(v), irq, 0);
> > -        pt_vector = irq;
> > -        break;
> > +        return irq;
> >  
> >      case PTSRC_isa:
> >          hvm_isa_irq_deassert(v->domain, irq);
> >          if ( platform_legacy_irq(irq) && vlapic_accept_pic_intr(v) &&
> >               v->domain->arch.hvm.vpic[irq >> 3].int_output )
> > -            hvm_isa_irq_assert(v->domain, irq, NULL);
> > +            pt_vector = hvm_isa_irq_assert(v->domain, irq, NULL);
> >          else
> > -        {
> >              pt_vector = hvm_isa_irq_assert(v->domain, irq, vioapic_get_vector);
> > -            /*
> > -             * hvm_isa_irq_assert may not set the corresponding bit in vIRR
> > -             * when mask field of IOAPIC RTE is set. Check it again.
> > -             */
> 
> For one, the transformation done here looks to call for folding
> both calls to hvm_isa_irq_assert() into one. I'm not, however,
> convinced recording the function's return value is useful in the
> case where it wasn't recorded before. The change is benign right
> now because hvm_isa_irq_assert() will return -1 when its last
> argument is NULL, but the question is whether the code here should
> start depending on such behavior.

I see, I shouldn't have adjusted this first call to store pt_vector,
and just leave pt_vector as initialized (-1) to not rely on
hvm_isa_irq_assert returning -1.

Coalescing both calls would make the code harder to read IMO, as then
the condition of the if clause would need to be moved inside the call
to hvm_isa_irq_assert in order to decide whether to pass NULL or
vioapic_get_vector.

> And then, according to this comment (which doesn't get retained in
> any form or shape) ...
> 
> > -            if ( pt_vector < 0 || !vlapic_test_irq(vcpu_vlapic(v), pt_vector) )
> > -                pt_vector = -1;
> > -        }
> > +
> > +        if ( pt_vector < 0 )
> > +            return pt_vector;
> > +
> >          break;
> >  
> >      case PTSRC_ioapic:
> >          pt_vector = hvm_ioapic_assert(v->domain, irq, level);
> > -        if ( pt_vector < 0 || !vlapic_test_irq(vcpu_vlapic(v), pt_vector) )
> > -        {
> > -            pt_vector = -1;
> > -            if ( level )
> > +        if ( pt_vector < 0 )
> > +            return pt_vector;
> > +
> > +        break;
> > +    }
> > +
> > +    ASSERT(pt_vector >= 0);
> > +    if ( !vlapic_test_irq(vcpu_vlapic(v), pt_vector) )
> > +    {
> > +        time_cb *cb = NULL;
> > +        void *cb_priv;
> > +
> > +        /*
> > +         * Vector has been injected to a different vCPU, call pt_irq_fired and
> > +         * execute the callback, since the destination vCPU(s) won't call
> > +         * pt_intr_post for it.
> 
> ... this isn't the only reason to come here. Beyond what the comment
> says there is the hvm_domain_use_pirq() check in assert_gsi() which
> would similarly result in the IRR bit not observed set here. At the
> very least these cases want mentioning; I have to admit that I'm not
> entirely clear yet whether your handling is correct for both, or
> whether the information needs to be propagated into here.

I always forget about that weird pirq stuff (and I'm refraining from
using other adjectives) that we have for HVM.

AFAICT vpt is already broken when trying to inject interrupts
generated from it over an event channel. hvm_ioapic_assert will return
whatever garbage is in the IO-APIC entry, which will likely not be
initialized because the GSI is routed over an event channel.

I really have no idea what hvm_ioapic_assert should return in that
case, the event channel callback vector maybe?

Maybe just returning -1 would be fine, a guest using this routing of
pirqs over event channels shouldn't be using any of the emulated
timers, and hence vpt is not required to be functional in that case?

> Also instead of ASSERT(pt_vector >= 0) would you pull the respective
> if() out of the switch(), to also cover the case of a fall through
> without hitting any of the explicitly handled cases, resulting in
> pt_vector left at its initial value of -1?

Sure.

> 
> > +         * TODO: move this vpt to one of the vCPUs where the vector gets
> > +         * injected.
> > +         */
> > +        spin_lock(&v->arch.hvm.tm_lock);
> > +        /* Make sure the timer is still on the list. */
> > +        list_for_each_entry ( pt, &v->arch.hvm.tm_list, list )
> > +            if ( pt == earliest_pt )
> >              {
> > -                /*
> > -                 * Level interrupts are always asserted because the pin assert
> > -                 * count is incremented regardless of whether the pin is masked
> > -                 * or the vector latched in IRR, so also execute the callback
> > -                 * associated with the timer.
> > -                 */
> > -                time_cb *cb = NULL;
> > -                void *cb_priv;
> > -
> > -                spin_lock(&v->arch.hvm.tm_lock);
> > -                /* Make sure the timer is still on the list. */
> > -                list_for_each_entry ( pt, &v->arch.hvm.tm_list, list )
> > -                    if ( pt == earliest_pt )
> > -                    {
> > -                        pt_irq_fired(v, pt);
> > -                        cb = pt->cb;
> > -                        cb_priv = pt->priv;
> > -                        break;
> > -                    }
> > -                spin_unlock(&v->arch.hvm.tm_lock);
> > -
> > -                if ( cb != NULL )
> > -                    cb(v, cb_priv);
> > +                pt_irq_fired(v, pt);
> > +                cb = pt->cb;
> > +                cb_priv = pt->priv;
> > +                break;
> >              }
> > -        }
> > -        break;
> > +        spin_unlock(&v->arch.hvm.tm_lock);
> > +
> > +        if ( cb != NULL )
> > +            cb(v, cb_priv);
> > +
> > +        pt_vector = -1;
> >      }
> >  
> >      return pt_vector;
> 
> To further reduce indentation (and seeing the significant code
> churn that happens here anyway), could you consider inverting the
> surrounding if() to
> 
>     if ( vlapic_test_irq(vcpu_vlapic(v), pt_vector) )
>         return pt_vector;    
> 
> ?

Yup, that's indeed better.

Thanks, Roger.

Re: [PATCH for-4.14 6/8] x86/vpt: fix injection to remote vCPU

[Jan Beulich]

On 18.06.2020 19:14, Roger Pau Monné wrote:
> On Thu, Jun 18, 2020 at 05:12:17PM +0200, Jan Beulich wrote:
>> On 12.06.2020 17:56, Roger Pau Monne wrote:
>>>      case PTSRC_ioapic:
>>>          pt_vector = hvm_ioapic_assert(v->domain, irq, level);
>>> -        if ( pt_vector < 0 || !vlapic_test_irq(vcpu_vlapic(v), pt_vector) )
>>> -        {
>>> -            pt_vector = -1;
>>> -            if ( level )
>>> +        if ( pt_vector < 0 )
>>> +            return pt_vector;
>>> +
>>> +        break;
>>> +    }
>>> +
>>> +    ASSERT(pt_vector >= 0);
>>> +    if ( !vlapic_test_irq(vcpu_vlapic(v), pt_vector) )
>>> +    {
>>> +        time_cb *cb = NULL;
>>> +        void *cb_priv;
>>> +
>>> +        /*
>>> +         * Vector has been injected to a different vCPU, call pt_irq_fired and
>>> +         * execute the callback, since the destination vCPU(s) won't call
>>> +         * pt_intr_post for it.
>>
>> ... this isn't the only reason to come here. Beyond what the comment
>> says there is the hvm_domain_use_pirq() check in assert_gsi() which
>> would similarly result in the IRR bit not observed set here. At the
>> very least these cases want mentioning; I have to admit that I'm not
>> entirely clear yet whether your handling is correct for both, or
>> whether the information needs to be propagated into here.
> 
> I always forget about that weird pirq stuff (and I'm refraining from
> using other adjectives) that we have for HVM.
> 
> AFAICT vpt is already broken when trying to inject interrupts
> generated from it over an event channel. hvm_ioapic_assert will return
> whatever garbage is in the IO-APIC entry, which will likely not be
> initialized because the GSI is routed over an event channel.
> 
> I really have no idea what hvm_ioapic_assert should return in that
> case, the event channel callback vector maybe?
> 
> Maybe just returning -1 would be fine, a guest using this routing of
> pirqs over event channels shouldn't be using any of the emulated
> timers, and hence vpt is not required to be functional in that case?

I would guess(!) that -1 ought to be fine. But this whole thing
escapes me as well, so let's ask Stefano, who iirc was who
introduced this.

Jan

Re: [PATCH for-4.14 8/8] x86/hvm: enable emulated PIT for PVH dom0

[Roger Pau Monné]

On Thu, Jun 18, 2020 at 06:05:21PM +0200, Jan Beulich wrote:
> On 12.06.2020 17:56, Roger Pau Monne wrote:
> > Some video BIOS require a PIT in order to work properly, hence classic
> > PV dom0 gets partial access to the physical PIT as long as it's not in
> > use by Xen.
> > 
> > Since PVH dom0 is built on top of HVM support, there's already an
> > emulated PIT implementation available for use. Tweak the emulated PIT
> > code so it injects interrupts directly into the vIO-APIC if the legacy
> > PIC (i8259) is disabled. Make sure the GSI used matches the ISA IRQ 0
> > in the likely case there's an interrupt overwrite in the MADT ACPI
> 
> Same nit again as for the earlier patch (also applicable to a code
> comment below).
> 
> > @@ -578,7 +579,7 @@ int arch_domain_create(struct domain *d,
> >  
> >      emflags = config->arch.emulation_flags;
> >  
> > -    if ( is_hardware_domain(d) && is_pv_domain(d) )
> > +    if ( is_hardware_domain(d) )
> >          emflags |= XEN_X86_EMU_PIT;
> 
> Wouldn't this better go into create_dom0(), where all the other
> flags get set? Or otherwise all of that be moved here (to cover
> the late-hwdom case)?

I've just moved all setting of the emulation_flags to
arch_domain_create so it's done at the same place for PV and PVH.

> > --- a/xen/arch/x86/hvm/vioapic.c
> > +++ b/xen/arch/x86/hvm/vioapic.c
> > @@ -268,7 +268,14 @@ static void vioapic_write_redirent(
> >  
> >      spin_unlock(&d->arch.hvm.irq_lock);
> >  
> > -    if ( is_hardware_domain(d) && unmasked )
> > +    if ( is_hardware_domain(d) && unmasked &&
> > +         /*
> > +          * A PVH dom0 can have an emulated PIT that should respect any
> > +          * interrupt overwrites found in the ACPI MADT table, so we need to
> > +          * check to which GSI the ISA IRQ 0 is mapped in order to prevent
> > +          * identity mapping it.
> > +          */
> > +         (!has_vpit(d) || gsi != hvm_isa_irq_to_gsi(d, 0)) )
> 
> Isn't has_vpit() true now for Dom0, and hence that part of the
> condition is kind of pointless?

Well, yes, but I think we should strive for the code to be prepared to
deal with both vPIT enabled or disabled, and hence shouldn't make
assumptions.

> And shouldn't Dom0 never have seen
> physical IRQ 0 in the first place (we don't allow PV Dom0 to use
> that IRQ either, after all)?

Yes, that will fail in map_domain_pirq, so a PVH dom0 won't be able to
bind IRQ 0 anyway.

Roger.