[PATCH v4 0/2] kvm: x86: Implement handling of RH=1 for MSI delivery in KVM

[PATCH v4 0/2] kvm: x86: Implement handling of RH=1 for MSI delivery in KVM

[James Sullivan]

Changes Since v1:
    * Reworked patches into two commits:
        1) [Patch v2 1/2] Extended struct kvm_lapic_irq with bool
            msi_redir_hint
            * Initialize msi_redir_hint = true in kvm_set_msi_irq when RH=1
            * Initialize msi_redir_hint = false otherwise
            * Added value of msi_redir_hint to debug message dump of IRQ in
                apic_send_ipi
        2) [Patch v2 2/2] Deliver to only lowest prio CPU if msi_redir_hint 
           is true 
            * Move kvm_is_dm_lowest_prio() -> lapic.h, rename to
                kvm_lowest_prio_delivery, set condition to
                (APIC_DM_LOWPRI || msi_redir_hint)
            * Change check in kvm_irq_delivery_to_apic_fast() for
                APIC_DM_LOWPRI or msi_redir_hint to a check for
                kvm_is_dm_lowest_prio() 
Changes since v2:
    * Extend Patch 1/2 ("kvm: x86: Extended struct kvm_lapic_irq with 
    msi_redir_hint for MSI delivery") with older patch to set the value
    of dest_mode in kvm_set_msi_irq() to be APIC_DEST_LOGICAL only when 
    RH=1/DM=1, and APIC_DEST_PHYSICAL otherwise. 
    (<5502FEDB.3030606@gmail.com>)
    This was done to decouple the patch dependency and to collect all
    efforts to implement RH bit handling into one submission.
    * Patch formatting
Changes since v3:
    * Revert logic for setting dest_mode; irq->dest_mode is now set
    independently of RH=1. (See <20150318225225.GA8702@amt.cnet>).
    The reason for this is to maintain consistenty with the interpretation
    of MSI destination mode selection in native_compose_msi_msg().

This series of patches extends the KVM interrupt delivery mechanism
to correctly account for the MSI Redirection Hint bit. The RH bit is 
used in logical destination mode to indicate that the delivery of the
interrupt shall only be to the lowest priority candidate LAPIC.

Currently, there is no handling of the MSI RH bit in the KVM interrupt
delivery mechanism. 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.

In other words, the RH bit is ignored in physical destination mode, and
when it is set in logical destination mode causes delivery to only apply
to the lowest priority processor in the logical group. The IA32 manual
is in slight contradiction with itself on this matter, but this patch
agrees with this interpretation of the RH bit:

    https://software.intel.com/en-us/forums/topic/288883

This patch has passed some rudimentary tests using an SMP QEMU guest and
virtio sourced MSIs, but I haven't done experiments with passing through 
PCI hardware (intend to start working on this).

-James

[PATCH v4 1/2] kvm: x86: Extended struct kvm_lapic_irq with msi_redir_hint for MSI delivery

[James Sullivan]

Extended struct kvm_lapic_irq with bool msi_redir_hint, which will
be used to determine if the delivery of the MSI should target only
the lowest priority CPU in the logical group specified for delivery.
(In physical dest mode, the RH bit is not relevant). Initialized the value
of msi_redir_hint to true when RH=1 in kvm_set_msi_irq(), and initialized
to false in all other cases.

Added value of msi_redir_hint to a debug message dump of an IRQ in
apic_send_ipi().

Signed-off-by: James Sullivan <sullivan.james.f@gmail.com>
---
Changes since v1:
    * Squashed a number of smaller commits into this one commit,
        which adds and initializes the msi_redir_hint variable
        and extends existing debug messages to display its value.
Changes since v2:
    * Added old patch (<5502FEDB.3030606@gmail.com>) to set the value of
    dest_mode in kvm_set_msi_irq() to be APIC_DEST_LOGICAL only when RH=1/DM=1,
    and APIC_DEST_PHYSICAL otherwise. This decouples the dependency of 
    this patch set on the previous submission and collects all efforts
    to implement RH bit handling into one submission.
    * Patch formatting
Changes since v3:
    * Reverted logic for setting dest_mode to RH=1/DM=1 (see
    <20150318225225.GA8702@amt.cnet>; this is for consistency with
    the interpretation of DM and RH in native_compose_msi_msg())

 arch/x86/include/asm/kvm_host.h | 1 +
 arch/x86/kvm/ioapic.c           | 1 +
 arch/x86/kvm/irq_comm.c         | 3 ++-
 arch/x86/kvm/lapic.c            | 6 ++++--
 arch/x86/kvm/x86.c              | 1 +
 5 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index a236e39..77feaf4 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -685,6 +685,7 @@ struct kvm_lapic_irq {
 	u32 trig_mode;
 	u32 shorthand;
 	u32 dest_id;
+	bool msi_redir_hint;
 };
 
 struct kvm_x86_ops {
diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c
index b1947e0..61f0874 100644
--- a/arch/x86/kvm/ioapic.c
+++ b/arch/x86/kvm/ioapic.c
@@ -347,6 +347,7 @@ static int ioapic_service(struct kvm_ioapic *ioapic, int irq, bool line_status)
 	irqe.delivery_mode = entry->fields.delivery_mode << 8;
 	irqe.level = 1;
 	irqe.shorthand = 0;
+	irqe.msi_redir_hint = false;
 
 	if (irqe.trig_mode == IOAPIC_EDGE_TRIG)
 		ioapic->irr &= ~(1 << irq);
diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
index 72298b3..80c10af 100644
--- a/arch/x86/kvm/irq_comm.c
+++ b/arch/x86/kvm/irq_comm.c
@@ -106,9 +106,10 @@ static inline void kvm_set_msi_irq(struct kvm_kernel_irq_routing_entry *e,
 	irq->dest_mode = (1 << MSI_ADDR_DEST_MODE_SHIFT) & e->msi.address_lo;
 	irq->trig_mode = (1 << MSI_DATA_TRIGGER_SHIFT) & e->msi.data;
 	irq->delivery_mode = e->msi.data & 0x700;
+	irq->msi_redir_hint = ((e->msi.address_lo
+		& MSI_ADDR_REDIRECTION_LOWPRI) > 0);
 	irq->level = 1;
 	irq->shorthand = 0;
-	/* TODO Deal with RH bit of MSI message address */
 }
 
 int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index bd4e34d..a15c444 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -892,6 +892,7 @@ static void apic_send_ipi(struct kvm_lapic *apic)
 	irq.level = icr_low & APIC_INT_ASSERT;
 	irq.trig_mode = icr_low & APIC_INT_LEVELTRIG;
 	irq.shorthand = icr_low & APIC_SHORT_MASK;
+	irq.msi_redir_hint = false;
 	if (apic_x2apic_mode(apic))
 		irq.dest_id = icr_high;
 	else
@@ -901,10 +902,11 @@ static void apic_send_ipi(struct kvm_lapic *apic)
 
 	apic_debug("icr_high 0x%x, icr_low 0x%x, "
 		   "short_hand 0x%x, dest 0x%x, trig_mode 0x%x, level 0x%x, "
-		   "dest_mode 0x%x, delivery_mode 0x%x, vector 0x%x\n",
+		   "dest_mode 0x%x, delivery_mode 0x%x, vector 0x%x, "
+		   "msi_redir_hint 0x%x\n",
 		   icr_high, icr_low, irq.shorthand, irq.dest_id,
 		   irq.trig_mode, irq.level, irq.dest_mode, irq.delivery_mode,
-		   irq.vector);
+		   irq.vector, irq.msi_redir_hint);
 
 	kvm_irq_delivery_to_apic(apic->vcpu->kvm, apic, &irq, NULL);
 }
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index bd7a70b..03e9b09 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5902,6 +5902,7 @@ static void kvm_pv_kick_cpu_op(struct kvm *kvm, unsigned long flags, int apicid)
 	lapic_irq.shorthand = 0;
 	lapic_irq.dest_mode = 0;
 	lapic_irq.dest_id = apicid;
+	lapic_irq.msi_redir_hint = false;
 
 	lapic_irq.delivery_mode = APIC_DM_REMRD;
 	kvm_irq_delivery_to_apic(kvm, 0, &lapic_irq, NULL);
-- 
2.3.3

[PATCH v4 2/2] kvm: x86: Deliver MSI IRQ to only lowest prio cpu if msi_redir_hint is true

[James Sullivan]

An MSI interrupt should only be delivered to the lowest priority CPU
when it has RH=1, regardless of the delivery mode. Modified
kvm_is_dm_lowest_prio() to check for either irq->delivery_mode == APIC_DM_LOWPRI
or irq->msi_redir_hint.

Moved kvm_is_dm_lowest_prio() into lapic.h and renamed to
kvm_lowest_prio_delivery().

Changed a check in kvm_irq_delivery_to_apic_fast() from
irq->delivery_mode == APIC_DM_LOWPRI to kvm_is_dm_lowest_prio().

Signed-off-by: James Sullivan <sullivan.james.f@gmail.com>
---
Changes since v1:
    * Squashed a number of smaller commits into this one commit,
        which implements MSI delivery to only the lowest-priority
        CPU whenever RH=1 using the above helper
        kvm_lowest_prio_delivery().
Changes since v2:
    * Patch formatting
Changes since v3:
    N/A

 arch/x86/kvm/irq_comm.c | 11 ++++-------
 arch/x86/kvm/lapic.c    |  3 +--
 arch/x86/kvm/lapic.h    |  6 ++++++
 3 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
index 80c10af..9efff9e 100644
--- a/arch/x86/kvm/irq_comm.c
+++ b/arch/x86/kvm/irq_comm.c
@@ -31,6 +31,8 @@
 
 #include "ioapic.h"
 
+#include "lapic.h"
+
 static int kvm_set_pic_irq(struct kvm_kernel_irq_routing_entry *e,
 			   struct kvm *kvm, int irq_source_id, int level,
 			   bool line_status)
@@ -48,11 +50,6 @@ static int kvm_set_ioapic_irq(struct kvm_kernel_irq_routing_entry *e,
 				line_status);
 }
 
-inline static bool kvm_is_dm_lowest_prio(struct kvm_lapic_irq *irq)
-{
-	return irq->delivery_mode == APIC_DM_LOWEST;
-}
-
 int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
 		struct kvm_lapic_irq *irq, unsigned long *dest_map)
 {
@@ -60,7 +57,7 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
 	struct kvm_vcpu *vcpu, *lowest = NULL;
 
 	if (irq->dest_mode == 0 && irq->dest_id == 0xff &&
-			kvm_is_dm_lowest_prio(irq)) {
+			kvm_lowest_prio_delivery(irq)) {
 		printk(KERN_INFO "kvm: apic: phys broadcast and lowest prio\n");
 		irq->delivery_mode = APIC_DM_FIXED;
 	}
@@ -76,7 +73,7 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
 					irq->dest_id, irq->dest_mode))
 			continue;
 
-		if (!kvm_is_dm_lowest_prio(irq)) {
+		if (!kvm_lowest_prio_delivery(irq)) {
 			if (r < 0)
 				r = 0;
 			r += kvm_apic_set_irq(vcpu, irq, dest_map);
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index a15c444..f8b21d5 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -701,8 +701,7 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
 		dst = map->logical_map[cid];
 
 		bitmap = apic_logical_id(map, mda);
-
-		if (irq->delivery_mode == APIC_DM_LOWEST) {
+		if (kvm_lowest_prio_delivery(irq)) {
 			int l = -1;
 			for_each_set_bit(i, &bitmap, 16) {
 				if (!dst[i])
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index 0bc6c65..ed7e2fa 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -168,6 +168,12 @@ static inline bool kvm_apic_has_events(struct kvm_vcpu *vcpu)
 	return vcpu->arch.apic->pending_events;
 }
 
+static inline bool kvm_lowest_prio_delivery(struct kvm_lapic_irq *irq)
+{
+	return (irq->delivery_mode == APIC_DM_LOWEST ||
+			irq->msi_redir_hint);
+}
+
 bool kvm_apic_pending_eoi(struct kvm_vcpu *vcpu, int vector);
 
 void wait_lapic_expire(struct kvm_vcpu *vcpu);
-- 
2.3.3

Re: [PATCH v4 0/2] kvm: x86: Implement handling of RH=1 for MSI delivery in KVM

[Radim Krčmář]

2015-03-18 19:26-0600, James Sullivan:
> Changes since v3:
>     * Revert logic for setting dest_mode; irq->dest_mode is now set
>     independently of RH=1. (See <20150318225225.GA8702@amt.cnet>).
>     The reason for this is to maintain consistenty with the interpretation
>     of MSI destination mode selection in native_compose_msi_msg().

Logic matches real hardware, thanks for confirming that.

Two patch series
Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>

(See my v3 review for comments.)

Re: [PATCH v4 0/2] kvm: x86: Implement handling of RH=1 for MSI delivery in KVM

[Paolo Bonzini]

On 19/03/2015 02:26, James Sullivan wrote:
> Changes Since v1:
>     * Reworked patches into two commits:
>         1) [Patch v2 1/2] Extended struct kvm_lapic_irq with bool
>             msi_redir_hint
>             * Initialize msi_redir_hint = true in kvm_set_msi_irq when RH=1
>             * Initialize msi_redir_hint = false otherwise
>             * Added value of msi_redir_hint to debug message dump of IRQ in
>                 apic_send_ipi
>         2) [Patch v2 2/2] Deliver to only lowest prio CPU if msi_redir_hint 
>            is true 
>             * Move kvm_is_dm_lowest_prio() -> lapic.h, rename to
>                 kvm_lowest_prio_delivery, set condition to
>                 (APIC_DM_LOWPRI || msi_redir_hint)
>             * Change check in kvm_irq_delivery_to_apic_fast() for
>                 APIC_DM_LOWPRI or msi_redir_hint to a check for
>                 kvm_is_dm_lowest_prio() 
> Changes since v2:
>     * Extend Patch 1/2 ("kvm: x86: Extended struct kvm_lapic_irq with 
>     msi_redir_hint for MSI delivery") with older patch to set the value
>     of dest_mode in kvm_set_msi_irq() to be APIC_DEST_LOGICAL only when 
>     RH=1/DM=1, and APIC_DEST_PHYSICAL otherwise. 
>     (<5502FEDB.3030606@gmail.com>)
>     This was done to decouple the patch dependency and to collect all
>     efforts to implement RH bit handling into one submission.
>     * Patch formatting
> Changes since v3:
>     * Revert logic for setting dest_mode; irq->dest_mode is now set
>     independently of RH=1. (See <20150318225225.GA8702@amt.cnet>).
>     The reason for this is to maintain consistenty with the interpretation
>     of MSI destination mode selection in native_compose_msi_msg().
> 
> This series of patches extends the KVM interrupt delivery mechanism
> to correctly account for the MSI Redirection Hint bit. The RH bit is 
> used in logical destination mode to indicate that the delivery of the
> interrupt shall only be to the lowest priority candidate LAPIC.
> 
> Currently, there is no handling of the MSI RH bit in the KVM interrupt
> delivery mechanism. 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.
> 
> In other words, the RH bit is ignored in physical destination mode, and
> when it is set in logical destination mode causes delivery to only apply
> to the lowest priority processor in the logical group. The IA32 manual
> is in slight contradiction with itself on this matter, but this patch
> agrees with this interpretation of the RH bit:
> 
>     https://software.intel.com/en-us/forums/topic/288883
> 
> This patch has passed some rudimentary tests using an SMP QEMU guest and
> virtio sourced MSIs, but I haven't done experiments with passing through 
> PCI hardware (intend to start working on this).
> 
> -James
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Queued for 4.2, thanks.

Paolo