[PATCH v2 0/2] Implement handling of RH=1 for MSI delivery in KVM

[PATCH v2 0/2] 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() 

(The patch relies on the changes made in a prior patch that adds a check
for the RH bit in kvm_set_msi_irq(); see <5502FEDB.3030606@gmail.com>)

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

Let me know your thoughts.

-James

[PATCH v2 1/2] Extended struct kvm_lapic_irq with bool msi_redir_hint for MSI delivery

[James Sullivan]

Signed-off-by: James Sullivan <sullivan.james.f@gmail.com>
---

The following changes are added in this patch:
    * 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
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.

This patch relies on a past submission that adds a check to
kvm_set_msi_irq() for RH=1, setting APIC_DEST_LOGICAL only when
RH=1/DM=1 and otherwise defaulting to APIC_DEST_PHYSICAL.
(See <5502FEDB.3030606@gmail.com>)

 arch/x86/include/asm/kvm_host.h |  1 +
 arch/x86/kvm/ioapic.c           |  1 +
 arch/x86/kvm/irq_comm.c         | 11 ++---------
 arch/x86/kvm/lapic.c            |  6 ++++--
 arch/x86/kvm/x86.c              |  1 +
 5 files changed, 9 insertions(+), 11 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 f2887ea..5c33cca 100644
--- a/arch/x86/kvm/irq_comm.c
+++ b/arch/x86/kvm/irq_comm.c
@@ -103,12 +103,6 @@ static inline void kvm_set_msi_irq(struct kvm_kernel_irq_routing_entry *e,
 			MSI_ADDR_DEST_ID_MASK) >> MSI_ADDR_DEST_ID_SHIFT;
 	irq->vector = (e->msi.data &
 			MSI_DATA_VECTOR_MASK) >> MSI_DATA_VECTOR_SHIFT;
-	/*
-	 * TODO Deal with RH bit of MSI message address
-	 *  IF RH=1, then MSI delivers only to the processor with the
-	 *  lowest interrupt priority among processors that can receive
-	 *  the interrupt.
-	 */
 	if ((e->msi.address_lo & MSI_ADDR_REDIRECTION_LOWPRI) &&
 			(e->msi.address_lo & MSI_ADDR_DEST_MODE_LOGICAL))
 		irq->dest_mode = APIC_DEST_LOGICAL;
@@ -116,9 +110,8 @@ static inline void kvm_set_msi_irq(struct kvm_kernel_irq_routing_entry *e,
 		irq->dest_mode = APIC_DEST_PHYSICAL;
 	irq->trig_mode = (1 << MSI_DATA_TRIGGER_SHIFT) & e->msi.data;
 	irq->delivery_mode = e->msi.data & 0x700;
-	if (e->msi.address_lo & MSI_ADDR_REDIRECTION_LOWPRI)
-		pr_warn_once(
-			"kvm: MSIs may not be correctly delivered with RH set.\n");
+	irq->msi_redir_hint = ((e->msi.address_lo
+		& MSI_ADDR_REDIRECTION_LOWPRI) > 0);
 	irq->level = 1;
 	irq->shorthand = 0;
 }
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.1

[PATCH v2 2/2] Deliver to only lowest prio cpu if msi_redir_hint is true

[James Sullivan]

Signed-off-by: James Sullivan <sullivan.james.f@gmail.com>
---

The following changes are added in this patch:
    * 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 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().

This patch relies on a past submission that adds a check to
kvm_set_msi_irq() for RH=1, setting APIC_DEST_LOGICAL only when
RH=1/DM=1 and otherwise defaulting to APIC_DEST_PHYSICAL.
(See <5502FEDB.3030606@gmail.com>)

 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 5c33cca..69cdaab 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.1

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

[Radim Krčmář]

2015-03-16 11:28-0600, 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() 

Functionality is good, but patch style is not up to par.
(Documentation/SubmittingPatches)

Subjects should begin with "kvm: " (or in CAPS), followed by "x86:",
`git log $file` is a good reference.
(Proper tags protect your ancestors from getting cursed if you decide to
 Cc: linux-kernel@vger.kernel.org.)

Commit messages are really useful ... ideally they contain
 - purpose of the patch (probably won't fit in a subject)
 - design decisions
 - excerpts from documentation

If it is hard to come up with a commit message, it's likely not a good
patch -- right now, you could basically paste the cover letter if you
did the following:
 - do the renaming and moving in a first patch
 - do stuff from [1/2] in a second patch + check for irq->msi_redir_hint
   in kvm_lowest_prio_delivery()

> (The patch relies on the changes made in a prior patch that adds a check
> for the RH bit in kvm_set_msi_irq(); see <5502FEDB.3030606@gmail.com>)

I failed to spot that this patch has suboptimal Subject in my earlier
review, sorry.

I see two options now
 1) ask the maintainer to fix it when applying the patch
 2) resend; you can include the patch in this series
    (send a reply to the original one, to make it clear.)

I like option (2) better -- you'll drop the patch-dependency and you
might also change something else if you decide to.