[Qemu-devel] [PATCH v2 RESEND 0/5] apic: Implement MSI RH bit handling, lowpri IRQ

[Qemu-devel] [PATCH v2 RESEND 0/5] apic: Implement MSI RH bit handling, lowpri IRQ

[James Sullivan]

Resend of <1427224426-9025-1-git-send-email-sullivan.james.f@gmail.com>,
correcting the labelling of one of the patches to correct sequential order.

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().

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 have been reviewed and
discussed on the KVM mailing list. 

James Sullivan (5):
  apic: Implement LAPIC low priority arbitration functions
  apic: Implement low priority arbitration for IRQ delivery
  apic: Added helper function apic_match_dest,
    apic_match_[physical,logical]_dest
  apic: Set and pass in RH bit for MSI interrupts
  apic: Implement handling of RH=1 for MSI interrupt delivery

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

-- 
2.3.4

[Qemu-devel] [PATCH v2 RESEND 1/5] 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 two helper functions
apic_compare_prio() and apic_lowest_prio() to be used for LAPIC
arbitration.

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

diff --git a/hw/intc/apic.c b/hw/intc/apic.c
index 0f97b47..b372513 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)
@@ -199,6 +200,29 @@ static void apic_external_nmi(APICCommonState *s)
     apic_local_deliver(s, APIC_LVT_LINT1);
 }
 
+static int apic_compare_prio(struct APICCommonState *cpu1,
+                             struct APICCommonState *cpu2)
+{
+    return apic_get_arb_pri(cpu1) - apic_get_arb_pri(cpu2);
+}
+
+static struct APICCommonState *apic_lowest_prio(const uint32_t
+                                                *deliver_bitmask)
+{
+    APICCommonState *lowest = NULL;
+    int i, d;
+
+    for (i = 0; i < MAX_APIC_WORDS; i++) {
+        if (deliver_bitmask[i]) {
+            d = i * 32 + apic_ffs_bit(deliver_bitmask[i]);
+            if (!lowest || apic_compare_prio(local_apics[d], lowest) < 0) {
+                lowest = local_apics[d];
+            }
+        }
+    }
+    return lowest;
+}
+
 #define foreach_apic(apic, deliver_bitmask, code) \
 {\
     int __i, __j;\
@@ -336,8 +360,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 = ((tpr & isrv) > irrv) ? (tpr & isrv) : irrv;
+        apr <<= 4;
+    }
+    return apr;
 }
 
 
-- 
2.3.4

[Qemu-devel] [PATCH v2 RESEND 2/5] apic: Implement low priority arbitration for IRQ delivery

[James Sullivan]

Currently, there is no arbitration among processors for low priority IRQ
delivery. Added support for low priority arbitration to
apic_bus_deliver(), using the functions introduced in
[74c1222c5b579970fafdd6a8e919fbb2c88219c3] ("apic: Implement LAPIC low
priority arbitration functions").

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

diff --git a/hw/intc/apic.c b/hw/intc/apic.c
index b372513..47d2fb1 100644
--- a/hw/intc/apic.c
+++ b/hw/intc/apic.c
@@ -249,22 +249,10 @@ 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);
-                    }
-                }
+            /* XXX: search for focus processor */
+            apic_iter = apic_lowest_prio(deliver_bitmask);
+            if (apic_iter) {
+                apic_set_irq(apic_iter , vector_num, trigger_mode);
             }
             return;
 
-- 
2.3.4

[Qemu-devel] [PATCH v2 RESEND 3/5] apic: Added helper function 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 47d2fb1..c49eb26 100644
--- a/hw/intc/apic.c
+++ b/hw/intc/apic.c
@@ -241,6 +241,32 @@ static struct APICCommonState *apic_lowest_prio(const uint32_t
     }\
 }
 
+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.4

[Qemu-devel] [PATCH v2 RESEND 4/5] 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 c49eb26..f3ec2c8 100644
--- a/hw/intc/apic.c
+++ b/hw/intc/apic.c
@@ -318,12 +318,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);
@@ -808,8 +809,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.4

[Qemu-devel] [PATCH v2 RESEND 5/5] apic: Implement handling of RH=1 for MSI interrupt delivery

[James Sullivan]

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

Modified the implementation of apic_get_delivery_bitmask() to account
for the RH bit of an MSI IRQ. The RH bit indicates that the message
should target only the lowest-priority processor among those specified
by the logical destination of the IRQ.

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

diff --git a/hw/intc/apic.c b/hw/intc/apic.c
index f3ec2c8..f4cc1df 100644
--- a/hw/intc/apic.c
+++ b/hw/intc/apic.c
@@ -37,7 +37,8 @@ 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 msi_redir_hint);
 static int apic_get_arb_pri(APICCommonState *s);
 
 /* Find first bit starting from msb */
@@ -326,7 +327,7 @@ 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, msi_redir_hint);
     apic_bus_deliver(deliver_bitmask, delivery_mode, vector_num, trigger_mode);
 }
 
@@ -503,7 +504,8 @@ 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 msi_redir_hint)
 {
     APICCommonState *apic_iter;
     int i;
@@ -519,23 +521,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) {
+                    if (l < 0 ||
+                        apic_compare_prio(apic_iter, local_apics[l]) < 0) {
+                        l = i;
                     }
+                } else {
+                    apic_set_bit(deliver_bitmask, i);
                 }
-            } else {
-                break;
             }
         }
+        if (msi_redir_hint && l >= 0) {
+            apic_set_bit(deliver_bitmask, l);
+        }
     }
 }
 
@@ -568,7 +574,7 @@ 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, 0);
         break;
     case 1:
         memset(deliver_bitmask, 0x00, sizeof(deliver_bitmask));
-- 
2.3.4

Re: [Qemu-devel] [PATCH v2 RESEND 1/5] apic: Implement LAPIC low priority arbitration functions

[Radim Krčmář]

2015-04-06 17:45-0600, James Sullivan:
> Currently, apic_get_arb_pri() is unimplemented and returns 0.
> 
> Implemented apic_get_arb_pri() and added two helper functions
> apic_compare_prio() and apic_lowest_prio() to be used for LAPIC
> arbitration.
> 
> Signed-off-by: James Sullivan <sullivan.james.f@gmail.com>
> ---
> +static int apic_compare_prio(struct APICCommonState *cpu1,
> +                             struct APICCommonState *cpu2)
> +{
> +    return apic_get_arb_pri(cpu1) - apic_get_arb_pri(cpu2);
> +}
> +
> +static struct APICCommonState *apic_lowest_prio(const uint32_t
> +                                                *deliver_bitmask)
> +{
> +    APICCommonState *lowest = NULL;
> +    int i, d;
> +
> +    for (i = 0; i < MAX_APIC_WORDS; i++) {
> +        if (deliver_bitmask[i]) {
> +            d = i * 32 + apic_ffs_bit(deliver_bitmask[i]);
> +            if (!lowest || apic_compare_prio(local_apics[d], lowest) < 0) {
> +                lowest = local_apics[d];

deliver_bitmask is 8 times u32 to express all 255 possible APICs.
apic_ffs_bit() takes the last set bit, so this loop incorrectly
considers only up to 8 candidates for lowest priority delivery.
foreach_apic() could be used when fixing it, which would also avoid a
'local_apic[d] == NULL' crash.

> +            }
> +        }
> +    }
> +    return lowest;

(For consideration:
 APM 2-16.2.2 Lowest Priority Messages and Arbitration
   If there is a tie for lowest priority, the local APIC with the highest
   APIC ID is selected.

 Intel is undefined in spec and picks the lowest APIC ID in practice.)

> @@ -336,8 +360,27 @@ static int apic_get_ppr(APICCommonState *s)
>  
>  static int apic_get_arb_pri(APICCommonState *s)

(I'd call it apic_get_apr() -- we return the state of that register.)

>  {
> -    /* 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 = ((tpr & isrv) > irrv) ? (tpr & isrv) : irrv;
> +        apr <<= 4;
> +    }
> +    return apr;
>  }

(This function is called too much IMO.
 The trivial optimization is to memorize apic_get_arb_pri() of lowest
 priority LAPIC.  And you can instantly return if it is 0.
 The more complicated one is to use ARB as a real LAPIC register and
 update it on TPR/ISR/IRR change, so apic_get_arb_pri() would be just
 'return s->apr;'.)

Re: [Qemu-devel] [PATCH v2 RESEND 5/5] apic: Implement handling of RH=1 for MSI interrupt delivery

[Radim Krčmář]

2015-04-06 17:45-0600, James Sullivan:
> Added argument to apic_get_delivery_bitmask() for msi_redir_hint,
> and changed calls to the function accordingly (using 0 as a default
> value for non-MSI interrupts).
> 
> Modified the implementation of apic_get_delivery_bitmask() to account
> for the RH bit of an MSI IRQ. The RH bit indicates that the message
> should target only the lowest-priority processor among those specified
> by the logical destination of the IRQ.
> 
> Signed-off-by: James Sullivan <sullivan.james.f@gmail.com>
> ---
> diff --git a/hw/intc/apic.c b/hw/intc/apic.c
> @@ -519,23 +521,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) {
> +                break;
> +            }

(I wonder if QEMU would allow
  'for(i = 0; i < MAX_APICS && (apic_iter = local_apics[i]); i++) {')

> +            if (apic_match_dest(apic_iter, dest_mode, dest)) {
> +                if (msi_redir_hint) {

You could check for APIC_DM_LOWPRI here as well and save the
apic_lowest_prio() loop in patch [1/4].
LOWPRI would be delivered like FIXED.

> +                    if (l < 0 ||
> +                        apic_compare_prio(apic_iter, local_apics[l]) < 0) {
> +                        l = i;

(Btw. lowest priority has a lot of cases that are forbidden ...
 - in combination with physical broadcast
 - in combination with clustered logical broadcast
 - to invalid/disabled destinations

 These most likely won't work correctly in real hardware.
 Lowest priority is a bad concept for large systems, which is why Intel
 stopped implementing it.)

Re: [Qemu-devel] [PATCH v2 RESEND 1/5] apic: Implement LAPIC low priority arbitration functions

[James Sullivan]

On 04/23/2015 07:49 AM, Radim Krčmář wrote:
> 2015-04-06 17:45-0600, James Sullivan:
>> Currently, apic_get_arb_pri() is unimplemented and returns 0.
>>
>> Implemented apic_get_arb_pri() and added two helper functions
>> apic_compare_prio() and apic_lowest_prio() to be used for LAPIC
>> arbitration.
>>
>> Signed-off-by: James Sullivan <sullivan.james.f@gmail.com>
>> ---
>> +static int apic_compare_prio(struct APICCommonState *cpu1,
>> +                             struct APICCommonState *cpu2)
>> +{
>> +    return apic_get_arb_pri(cpu1) - apic_get_arb_pri(cpu2);
>> +}
>> +
>> +static struct APICCommonState *apic_lowest_prio(const uint32_t
>> +                                                *deliver_bitmask)
>> +{
>> +    APICCommonState *lowest = NULL;
>> +    int i, d;
>> +
>> +    for (i = 0; i < MAX_APIC_WORDS; i++) {
>> +        if (deliver_bitmask[i]) {
>> +            d = i * 32 + apic_ffs_bit(deliver_bitmask[i]);
>> +            if (!lowest || apic_compare_prio(local_apics[d], lowest) < 0) {
>> +                lowest = local_apics[d];
> 
> deliver_bitmask is 8 times u32 to express all 255 possible APICs.
> apic_ffs_bit() takes the last set bit, so this loop incorrectly
> considers only up to 8 candidates for lowest priority delivery.
> foreach_apic() could be used when fixing it, which would also avoid a
> 'local_apic[d] == NULL' crash.
> 

Dumb mistake, I'll fix the former point. I shied away from
foreach_apic() because I think it's a bit messy to embed a block or an
`if` statement into the macro like so:

foreach_apic(apic,bmask,
	if (cond)
	    foo();
);

But if people are okay with that sort of thing we can switch to it.

>> +            }
>> +        }
>> +    }
>> +    return lowest;
> 
> (For consideration:
>  APM 2-16.2.2 Lowest Priority Messages and Arbitration
>    If there is a tie for lowest priority, the local APIC with the highest
>    APIC ID is selected.
> 
>  Intel is undefined in spec and picks the lowest APIC ID in practice.)
> 

Pretty quick change there, set lowest = local_apics[d] when
apic_compare_prio(local_apics[d], lowest) <= 0 rather than < 0

>> @@ -336,8 +360,27 @@ static int apic_get_ppr(APICCommonState *s)
>>  
>>  static int apic_get_arb_pri(APICCommonState *s)
> 
> (I'd call it apic_get_apr() -- we return the state of that register.)
>

Good point, more consistent with other functions too.

>>  {
>> -    /* 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 = ((tpr & isrv) > irrv) ? (tpr & isrv) : irrv;
>> +        apr <<= 4;
>> +    }
>> +    return apr;
>>  }
> 
> (This function is called too much IMO.
>  The trivial optimization is to memorize apic_get_arb_pri() of lowest
>  priority LAPIC.  And you can instantly return if it is 0.
>  The more complicated one is to use ARB as a real LAPIC register and
>  update it on TPR/ISR/IRR change, so apic_get_arb_pri() would be just
>  'return s->apr;'.)
> 

The latter approach would be smart, I'll look into it.

Re: [Qemu-devel] [PATCH v2 RESEND 5/5] apic: Implement handling of RH=1 for MSI interrupt delivery

[James Sullivan]

On 04/23/2015 08:14 AM, Radim Krčmář wrote:
> 2015-04-06 17:45-0600, James Sullivan:
>> Added argument to apic_get_delivery_bitmask() for msi_redir_hint,
>> and changed calls to the function accordingly (using 0 as a default
>> value for non-MSI interrupts).
>>
>> Modified the implementation of apic_get_delivery_bitmask() to account
>> for the RH bit of an MSI IRQ. The RH bit indicates that the message
>> should target only the lowest-priority processor among those specified
>> by the logical destination of the IRQ.
>>
>> Signed-off-by: James Sullivan <sullivan.james.f@gmail.com>
>> ---
>> diff --git a/hw/intc/apic.c b/hw/intc/apic.c
>> @@ -519,23 +521,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) {
>> +                break;
>> +            }
> 
> (I wonder if QEMU would allow
>   'for(i = 0; i < MAX_APICS && (apic_iter = local_apics[i]); i++) {')
> 
>> +            if (apic_match_dest(apic_iter, dest_mode, dest)) {
>> +                if (msi_redir_hint) {
> 
> You could check for APIC_DM_LOWPRI here as well and save the
> apic_lowest_prio() loop in patch [1/4].
> LOWPRI would be delivered like FIXED.
> 

I was considering doing that, saving the loop is a big plus. My concern
was that it would change get_delivery_bitmask's semantics in a way that
could be confusing (i.e. it is currently expected that the caller is
responsible for doing arbitration after the fact, this would flip that
responsibility).

>> +                    if (l < 0 ||
>> +                        apic_compare_prio(apic_iter, local_apics[l]) < 0) {
>> +                        l = i;
> 
> (Btw. lowest priority has a lot of cases that are forbidden ...
>  - in combination with physical broadcast
>  - in combination with clustered logical broadcast
>  - to invalid/disabled destinations
> 
>  These most likely won't work correctly in real hardware.
>  Lowest priority is a bad concept for large systems, which is why Intel
>  stopped implementing it.)
> 

Checking for such illegal cases should probably happen in the delivery
routines before we set the delivery bitmask (in apic_bus_deliver()?)

Re: [Qemu-devel] [PATCH v2 RESEND 1/5] apic: Implement LAPIC low priority arbitration functions

[Radim Krčmář]

2015-04-23 12:34-0600, James Sullivan:
> On 04/23/2015 07:49 AM, Radim Krčmář wrote:
>> 2015-04-06 17:45-0600, James Sullivan:
>>> Currently, apic_get_arb_pri() is unimplemented and returns 0.
>>>
>>> Implemented apic_get_arb_pri() and added two helper functions
>>> apic_compare_prio() and apic_lowest_prio() to be used for LAPIC
>>> arbitration.
>>>
>>> Signed-off-by: James Sullivan <sullivan.james.f@gmail.com>
>>> ---
>>> +    for (i = 0; i < MAX_APIC_WORDS; i++) {
>>> +        if (deliver_bitmask[i]) {
>>> +            d = i * 32 + apic_ffs_bit(deliver_bitmask[i]);
>>> +            if (!lowest || apic_compare_prio(local_apics[d], lowest) < 0) {
>>> +                lowest = local_apics[d];
>> 
>> deliver_bitmask is 8 times u32 to express all 255 possible APICs.
>> apic_ffs_bit() takes the last set bit, so this loop incorrectly
>> considers only up to 8 candidates for lowest priority delivery.
>> foreach_apic() could be used when fixing it, which would also avoid a
>> 'local_apic[d] == NULL' crash.
>> 
> 
> Dumb mistake, I'll fix the former point. I shied away from
> foreach_apic() because I think it's a bit messy to embed a block or an
> `if` statement into the macro like so:
> 
> foreach_apic(apic,bmask,
> 	if (cond)
> 	    foo();
> );
> 
> But if people are okay with that sort of thing we can switch to it.

Yeah, I wouldn't use it either :)
It could use the canonical form (and optimizations for sparse bitmasks)
  foreach(...)
    code;

Moving this logic to the loop in [4/4] would be an elegant solution.

> > (For consideration:
> >  APM 2-16.2.2 Lowest Priority Messages and Arbitration
> >    If there is a tie for lowest priority, the local APIC with the highest
> >    APIC ID is selected.
> > 
> >  Intel is undefined in spec and picks the lowest APIC ID in practice.)
> > 
> 
> Pretty quick change there, set lowest = local_apics[d] when
> apic_compare_prio(local_apics[d], lowest) <= 0 rather than < 0

local_apics[] aren't indexed by APIC ID, which brings two complications
- QEMU allows writing to APIC ID.
  Luckily, the spec allows us to make it read only, but even then
- local_apics[] might not be ordered like their APIC IDs; (I don't know)
  and future development can change that, so we should also encode the
  expecation somewhere.  (Comments don't work very well.)

Taking a look at the real APIC ID would be safest.
(Silently ignoring it is also a viable option.)

Re: [Qemu-devel] [PATCH v2 RESEND 5/5] apic: Implement handling of RH=1 for MSI interrupt delivery

[Radim Krčmář]

2015-04-23 13:08-0600, James Sullivan:
> On 04/23/2015 08:14 AM, Radim Krčmář wrote:
>> 2015-04-06 17:45-0600, James Sullivan:
>>> diff --git a/hw/intc/apic.c b/hw/intc/apic.c
>>> @@ -519,23 +521,27 @@ static void apic_get_delivery_bitmask(uint32_t *deliver_bitmask,
>>> +            if (apic_match_dest(apic_iter, dest_mode, dest)) {
>>> +                if (msi_redir_hint) {
>> 
>> You could check for APIC_DM_LOWPRI here as well and save the
>> apic_lowest_prio() loop in patch [1/4].
>> LOWPRI would be delivered like FIXED.
> 
> I was considering doing that, saving the loop is a big plus. My concern
> was that it would change get_delivery_bitmask's semantics in a way that
> could be confusing (i.e. it is currently expected that the caller is
> responsible for doing arbitration after the fact, this would flip that
> responsibility).

Good concern!  In this case, I think it's ok to change semantics, as the
function is static.  All callers are going to be in this module and they
call apic_bus_deliver() right after apic_get_delivery_bitmask() now.

(It's not like we could break interrupt delivery with this change, only
 logging or some other meta-operation would be affected.)

>> (Btw. lowest priority has a lot of cases that are forbidden ...
>>  - in combination with physical broadcast
>>  - in combination with clustered logical broadcast
>>  - to invalid/disabled destinations
>> 
>>  These most likely won't work correctly in real hardware.
>>  Lowest priority is a bad concept for large systems, which is why Intel
>>  stopped implementing it.)
>> 
> 
> Checking for such illegal cases should probably happen in the delivery
> routines before we set the delivery bitmask (in apic_bus_deliver()?)

What we have now is fine.  I presented them to lift constraints that
might have prevented you from making different decisions;
software mustn't configure them, so we're free to do whatever.

(I think it would be better if they didn't work, but haven't read enough
 QEMU code to know what is the approach here.)