[Qemu-devel] [PATCH v3 0/4] apic: Implement MSI RH bit handling, lowpri IRQ delivery

[Qemu-devel] [PATCH v3 0/4] apic: Implement MSI RH bit handling, lowpri IRQ delivery

[James Sullivan]

Changes in v2:
    * Merged in low priority IRQ delivery implementation to RH bit
    handling implementation, since both rely on the same helper
    functions for priority arbitration.
    * Corrected use of MSI data register => addr register when setting
    msi_redir_hint in apic_send_msi().
Changes in v3:
    * Fixed apic_get_arb_pri() to match the algorithm outlined in the
    AMD manual vs. the (incorrect?) Intel manual.
    * Removed apic_lowest_prio() in favour of performing the arbitration
    within apic_get_delivery_bitmask() while we iterate through the
    LAPICs (saves a few calls to an iteration over all LAPICs).
    * Modified delivery of APIC_DM_LOWPRI to be identical to
    APIC_DM_FIXED (since we account for arbitration when setting the
    delivery bitmask)

This set of patches adds the following features to QEMU:
    * Low priority delivery arbitration. Currently the first present CPU
    is always selected when lowpri delivery mode is used, and no
    arbitration is performed. Implemented arbitration in
    apic_bus_deliver() by adding the following functions:
        1) apic_get_arb_pri(APICCommonState *s)
        2) apic_compare_prio(APICCommonState *s1, APICCommonState *s2);
        3) apic_lowest_prio(const uint32_t *deliver_bitmask)
    * RH Bit handling for MSI messages. See below.

Currently, there is no handling of the MSI RH bit. This patch implements 
the following logic:

* DM=0, RH=*  : Physical destination mode. Interrupt is delivered to
                    the LAPIC with the matching APIC ID. (Subject to
                    the usual restrictions, i.e. no broadcast dest)
* DM=1, RH=0  : Logical destination mode without redirection. Interrupt
                    is delivered to all LAPICs in the logical group 
                    specified by the IRQ's destination map and delivery
                    mode.
* DM=1, RH=1  : Logical destination mode with redirection. Interrupt
                    is delivered only to the lowest priority LAPIC in the 
                    logical group specified by the dest map and the
                    delivery mode. Delivery semantics are otherwise
                    specified by the delivery_mode of the IRQ, which
                    is unchanged.

These changes reflect those made in the KVM in
http://www.spinics.net/lists/kvm/msg114915.html ("kvm: x86: Implement
handling of RH=1 for MSI delivery in KVM"), which is queued for 4.2.

James Sullivan (4):
  apic: Implement LAPIC low priority arbitration functions
  apic: Set and pass in RH bit for MSI interrupts
  apic: Add helper functions apic_match_dest,
    apic_match_[physical,logical]_dest
  apic: Handle RH=1 and lowpri delivery mode in
    apic_get_delivery_bitmask()

 hw/intc/apic.c         | 130 ++++++++++++++++++++++++++++++++-----------------
 hw/intc/ioapic.c       |   2 +-
 include/hw/i386/apic.h |   3 +-
 trace-events           |   2 +-
 4 files changed, 90 insertions(+), 47 deletions(-)

-- 
2.3.5

[Qemu-devel] [PATCH v3 1/4] apic: Implement LAPIC low priority arbitration functions

[James Sullivan]

Currently, apic_get_arb_pri() is unimplemented and returns 0.

Implemented apic_get_arb_pri() and added helper function
apic_compare_prio() to be used for LAPIC arbitration.

Signed-off-by: James Sullivan <sullivan.james.f@gmail.com>
---
Changes in v3:
    * Fixed apic_get_arb_pri to use AMD's algorithm vs. Intel's
    (incorrect?) algorithm
    * Removed apic_lowest_prio() in favour of arbitration while setting
    up the delivery bitmask (added in 4/4)

 hw/intc/apic.c | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/hw/intc/apic.c b/hw/intc/apic.c
index 0f97b47..8f6cdd2 100644
--- a/hw/intc/apic.c
+++ b/hw/intc/apic.c
@@ -38,6 +38,7 @@ static void apic_set_irq(APICCommonState *s, int vector_num, int trigger_mode);
 static void apic_update_irq(APICCommonState *s);
 static void apic_get_delivery_bitmask(uint32_t *deliver_bitmask,
                                       uint8_t dest, uint8_t dest_mode);
+static int apic_get_arb_pri(APICCommonState *s);
 
 /* Find first bit starting from msb */
 static int apic_fls_bit(uint32_t value)
@@ -217,6 +218,12 @@ static void apic_external_nmi(APICCommonState *s)
     }\
 }
 
+static int apic_compare_prio(struct APICCommonState *cpu1,
+                             struct APICCommonState *cpu2)
+{
+    return apic_get_arb_pri(cpu1) - apic_get_arb_pri(cpu2);
+}
+
 static void apic_bus_deliver(const uint32_t *deliver_bitmask,
                              uint8_t delivery_mode, uint8_t vector_num,
                              uint8_t trigger_mode)
@@ -336,8 +343,27 @@ static int apic_get_ppr(APICCommonState *s)
 
 static int apic_get_arb_pri(APICCommonState *s)
 {
-    /* XXX: arbitration */
-    return 0;
+    int tpr, isrv, irrv, apr;
+
+    tpr = apic_get_tpr(s);
+    isrv = get_highest_priority_int(s->isr);
+    if (isrv < 0) {
+        isrv = 0;
+    }
+    isrv >>= 4;
+    irrv = get_highest_priority_int(s->irr);
+    if (irrv < 0) {
+        irrv = 0;
+    }
+    irrv >>= 4;
+
+    if ((tpr >= irrv) && (tpr > isrv)) {
+        apr = s->tpr & 0xff;
+    } else {
+        apr = (isrv > irrv) ? isrv : irrv;
+        apr <<= 4;
+    }
+    return apr;
 }
 
 
-- 
2.3.5

[Qemu-devel] [PATCH v3 2/4] apic: Set and pass in RH bit for MSI interrupts

[James Sullivan]

In apic_send_msi(), set msi_redir_hint to 0x1 when RH=1 in the
MSI Address Register.

Added an argument for msi_redir_hint to apic_deliver_irq(), and
changed calls to the function accordingly (using 0 as a default value
for non-MSI interrupts).

Signed-off-by: James Sullivan <sullivan.james.f@gmail.com>
---
Changes in v2:
    * Corrected use of MSI data register => addr register when setting 
    msi_redir_hint in apic_send_msi().

 hw/intc/apic.c         | 10 ++++++----
 hw/intc/ioapic.c       |  2 +-
 include/hw/i386/apic.h |  3 ++-
 trace-events           |  2 +-
 4 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/hw/intc/apic.c b/hw/intc/apic.c
index 8f6cdd2..a25efa1 100644
--- a/hw/intc/apic.c
+++ b/hw/intc/apic.c
@@ -287,12 +287,13 @@ static void apic_bus_deliver(const uint32_t *deliver_bitmask,
 }
 
 void apic_deliver_irq(uint8_t dest, uint8_t dest_mode, uint8_t delivery_mode,
-                      uint8_t vector_num, uint8_t trigger_mode)
+                      uint8_t vector_num, uint8_t trigger_mode,
+                      uint8_t msi_redir_hint)
 {
     uint32_t deliver_bitmask[MAX_APIC_WORDS];
 
     trace_apic_deliver_irq(dest, dest_mode, delivery_mode, vector_num,
-                           trigger_mode);
+                           trigger_mode, msi_redir_hint);
 
     apic_get_delivery_bitmask(deliver_bitmask, dest, dest_mode);
     apic_bus_deliver(deliver_bitmask, delivery_mode, vector_num, trigger_mode);
@@ -777,8 +778,9 @@ static void apic_send_msi(hwaddr addr, uint32_t data)
     uint8_t dest_mode = (addr >> MSI_ADDR_DEST_MODE_SHIFT) & 0x1;
     uint8_t trigger_mode = (data >> MSI_DATA_TRIGGER_SHIFT) & 0x1;
     uint8_t delivery = (data >> MSI_DATA_DELIVERY_MODE_SHIFT) & 0x7;
-    /* XXX: Ignore redirection hint. */
-    apic_deliver_irq(dest, dest_mode, delivery, vector, trigger_mode);
+    uint8_t msi_redir_hint = (addr >> MSI_ADDR_REDIRECTION_SHIFT) & 0x1;
+    apic_deliver_irq(dest, dest_mode, delivery, vector, trigger_mode,
+                     msi_redir_hint);
 }
 
 static void apic_mem_writel(void *opaque, hwaddr addr, uint32_t val)
diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
index b527932..e69aa18 100644
--- a/hw/intc/ioapic.c
+++ b/hw/intc/ioapic.c
@@ -71,7 +71,7 @@ static void ioapic_service(IOAPICCommonState *s)
                     vector = entry & IOAPIC_VECTOR_MASK;
                 }
                 apic_deliver_irq(dest, dest_mode, delivery_mode,
-                                 vector, trig_mode);
+                                 vector, trig_mode, 0);
             }
         }
     }
diff --git a/include/hw/i386/apic.h b/include/hw/i386/apic.h
index 51eb6d3..9d7c0a4 100644
--- a/include/hw/i386/apic.h
+++ b/include/hw/i386/apic.h
@@ -5,7 +5,8 @@
 
 /* apic.c */
 void apic_deliver_irq(uint8_t dest, uint8_t dest_mode, uint8_t delivery_mode,
-                      uint8_t vector_num, uint8_t trigger_mode);
+                      uint8_t vector_num, uint8_t trigger_mode,
+                      uint8_t msi_redir_hint);
 int apic_accept_pic_intr(DeviceState *s);
 void apic_deliver_pic_intr(DeviceState *s, int level);
 void apic_deliver_nmi(DeviceState *d);
diff --git a/trace-events b/trace-events
index 30eba92..b430cf9 100644
--- a/trace-events
+++ b/trace-events
@@ -158,7 +158,7 @@ apic_get_irq_delivered(int apic_irq_delivered) "returning coalescing %d"
 
 # hw/intc/apic.c
 apic_local_deliver(int vector, uint32_t lvt) "vector %d delivery mode %d"
-apic_deliver_irq(uint8_t dest, uint8_t dest_mode, uint8_t delivery_mode, uint8_t vector_num, uint8_t trigger_mode) "dest %d dest_mode %d delivery_mode %d vector %d trigger_mode %d"
+apic_deliver_irq(uint8_t dest, uint8_t dest_mode, uint8_t delivery_mode, uint8_t vector_num, uint8_t trigger_mode, uint8_t msi_redir_hint) "dest %d dest_mode %d delivery_mode %d vector %d trigger_mode %d msi_redir_hint %d"
 apic_mem_readl(uint64_t addr, uint32_t val)  "%"PRIx64" = %08x"
 apic_mem_writel(uint64_t addr, uint32_t val) "%"PRIx64" = %08x"
 
-- 
2.3.5

[Qemu-devel] [PATCH v3 3/4] apic: Add helper functions apic_match_dest, apic_match_[physical, logical]_dest

[James Sullivan]

Added three helper functions apic_match_dest(),
apic_match_physical_dest(), and apic_match_logical_dest() which can be
used to determine if a logical or physical APIC ID match a given LAPIC
under a given dest_mode. This does not account for shorthand.

Signed-off-by: James Sullivan <sullivan.james.f@gmail.com>
---
 hw/intc/apic.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/hw/intc/apic.c b/hw/intc/apic.c
index a25efa1..9dd27b2 100644
--- a/hw/intc/apic.c
+++ b/hw/intc/apic.c
@@ -224,6 +224,32 @@ static int apic_compare_prio(struct APICCommonState *cpu1,
     return apic_get_arb_pri(cpu1) - apic_get_arb_pri(cpu2);
 }
 
+static bool apic_match_physical_dest(struct APICCommonState *apic, uint8_t dest)
+{
+    return (dest == 0xff || apic->id == dest);
+}
+
+static bool apic_match_logical_dest(struct APICCommonState *apic, uint8_t dest)
+{
+    if (apic->dest_mode == 0xf) {
+        return (dest & apic->log_dest) > 0;
+    } else if (apic->dest_mode == 0) {
+        return ((dest & 0xf0) == (apic->log_dest & 0xf0)) &&
+                (dest & apic->log_dest & 0x0f) > 0;
+    }
+    return false;
+}
+
+static bool apic_match_dest(struct APICCommonState *apic, uint8_t dest_mode,
+                     uint8_t dest)
+{
+    if (dest_mode == 0) {
+        return apic_match_physical_dest(apic, dest);
+    } else {
+        return apic_match_logical_dest(apic, dest);
+    }
+}
+
 static void apic_bus_deliver(const uint32_t *deliver_bitmask,
                              uint8_t delivery_mode, uint8_t vector_num,
                              uint8_t trigger_mode)
-- 
2.3.5

[Qemu-devel] [PATCH v3 4/4] apic: Handle RH=1 and lowpri delivery mode in apic_get_delivery_bitmask()

[James Sullivan]

Added arguments to apic_get_delivery_bitmask() for msi_redir_hint and
for delivery_mode, changing calls to the function accordingly (using 0
as a default value for msi_redir_hint for non-MSI interrupts).

Modified the implementation of apic_get_delivery_bitmask() such that if
msi_redir_hint == 1 or delivery_mode == APIC_DM_LOWPRI, only the lowest
priority APIC will have its bitmask entry set (tie-breaking is
arbitrary). Delivery of APIC_DM_LOWPRI in apic_bus_deliver() is then
identical to APIC_DM_FIXED.

Removed function apic_ffs_bit() which was only called to support interim
LOWPRI delivery.

Signed-off-by: James Sullivan <sullivan.james.f@gmail.com>
---
Changes in v3:
    * Added extra argument to apic_get_delivery_bitmask() for
    delivery_mode
    * Perform lowpri arbitration in apic_get_delivery_bitmask() for RH=1
    and for APIC_DM_LOWPRI, saving the cost of the LAPIC iteration in v2
    * Deliver APIC_DM_LOWPRI identically to APIC_DM_FIXED (since the
    bitmask now accounts for arbitration implicitly)
    * Removed apic_ffs_bit() (no longer called)

 hw/intc/apic.c | 64 ++++++++++++++++++++++++----------------------------------
 1 file changed, 26 insertions(+), 38 deletions(-)

diff --git a/hw/intc/apic.c b/hw/intc/apic.c
index 9dd27b2..1fe3877 100644
--- a/hw/intc/apic.c
+++ b/hw/intc/apic.c
@@ -37,7 +37,9 @@ static APICCommonState *local_apics[MAX_APICS + 1];
 static void apic_set_irq(APICCommonState *s, int vector_num, int trigger_mode);
 static void apic_update_irq(APICCommonState *s);
 static void apic_get_delivery_bitmask(uint32_t *deliver_bitmask,
-                                      uint8_t dest, uint8_t dest_mode);
+                                      uint8_t dest, uint8_t dest_mode,
+                                      uint8_t delivery_mode,
+                                      uint8_t msi_redir_hint);
 static int apic_get_arb_pri(APICCommonState *s);
 
 /* Find first bit starting from msb */
@@ -46,12 +48,6 @@ static int apic_fls_bit(uint32_t value)
     return 31 - clz32(value);
 }
 
-/* Find first bit starting from lsb */
-static int apic_ffs_bit(uint32_t value)
-{
-    return ctz32(value);
-}
-
 static inline void apic_set_bit(uint32_t *tab, int index)
 {
     int i, mask;
@@ -258,24 +254,8 @@ static void apic_bus_deliver(const uint32_t *deliver_bitmask,
 
     switch (delivery_mode) {
         case APIC_DM_LOWPRI:
-            /* XXX: search for focus processor, arbitration */
-            {
-                int i, d;
-                d = -1;
-                for(i = 0; i < MAX_APIC_WORDS; i++) {
-                    if (deliver_bitmask[i]) {
-                        d = i * 32 + apic_ffs_bit(deliver_bitmask[i]);
-                        break;
-                    }
-                }
-                if (d >= 0) {
-                    apic_iter = local_apics[d];
-                    if (apic_iter) {
-                        apic_set_irq(apic_iter, vector_num, trigger_mode);
-                    }
-                }
-            }
-            return;
+            /* XXX: search for focus processor */
+            break;
 
         case APIC_DM_FIXED:
             break;
@@ -321,7 +301,8 @@ void apic_deliver_irq(uint8_t dest, uint8_t dest_mode, uint8_t delivery_mode,
     trace_apic_deliver_irq(dest, dest_mode, delivery_mode, vector_num,
                            trigger_mode, msi_redir_hint);
 
-    apic_get_delivery_bitmask(deliver_bitmask, dest, dest_mode);
+    apic_get_delivery_bitmask(deliver_bitmask, dest, dest_mode, delivery_mode,
+            msi_redir_hint);
     apic_bus_deliver(deliver_bitmask, delivery_mode, vector_num, trigger_mode);
 }
 
@@ -498,7 +479,9 @@ static int apic_find_dest(uint8_t dest)
 }
 
 static void apic_get_delivery_bitmask(uint32_t *deliver_bitmask,
-                                      uint8_t dest, uint8_t dest_mode)
+                                      uint8_t dest, uint8_t dest_mode,
+                                      uint8_t delivery_mode,
+                                      uint8_t msi_redir_hint)
 {
     APICCommonState *apic_iter;
     int i;
@@ -514,23 +497,27 @@ static void apic_get_delivery_bitmask(uint32_t *deliver_bitmask,
         }
     } else {
         /* XXX: cluster mode */
+        int l = -1;
         memset(deliver_bitmask, 0x00, MAX_APIC_WORDS * sizeof(uint32_t));
         for(i = 0; i < MAX_APICS; i++) {
             apic_iter = local_apics[i];
-            if (apic_iter) {
-                if (apic_iter->dest_mode == 0xf) {
-                    if (dest & apic_iter->log_dest)
-                        apic_set_bit(deliver_bitmask, i);
-                } else if (apic_iter->dest_mode == 0x0) {
-                    if ((dest & 0xf0) == (apic_iter->log_dest & 0xf0) &&
-                        (dest & apic_iter->log_dest & 0x0f)) {
-                        apic_set_bit(deliver_bitmask, i);
+            if (!apic_iter) {
+                break;
+            }
+            if (apic_match_dest(apic_iter, dest_mode, dest)) {
+                if (msi_redir_hint || delivery_mode == APIC_DM_LOWPRI) {
+                    if (l < 0 ||
+                        apic_compare_prio(apic_iter, local_apics[l]) < 0) {
+                        l = i;
                     }
+                } else {
+                    apic_set_bit(deliver_bitmask, i);
                 }
-            } else {
-                break;
             }
         }
+        if (l >= 0) {
+            apic_set_bit(deliver_bitmask, l);
+        }
     }
 }
 
@@ -563,7 +550,8 @@ static void apic_deliver(DeviceState *dev, uint8_t dest, uint8_t dest_mode,
 
     switch (dest_shorthand) {
     case 0:
-        apic_get_delivery_bitmask(deliver_bitmask, dest, dest_mode);
+        apic_get_delivery_bitmask(deliver_bitmask, dest, dest_mode,
+                delivery_mode, 0);
         break;
     case 1:
         memset(deliver_bitmask, 0x00, sizeof(deliver_bitmask));
-- 
2.3.5