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

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

[James Sullivan]

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 1/9] Extended kvm_lapic_irq struct with 'bool redir_hint' for MSI delivery

[James Sullivan]

Signed-off-by: James Sullivan <sullivan.james.f@gmail.com>
---
 arch/x86/include/asm/kvm_host.h | 1 +
 1 file changed, 1 insertion(+)

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 {
-- 
2.3.1

[PATCH 2/9] Set irq->msi_redir_hint = 1 in kvm_set_msi_irq if RH=1

[James Sullivan]

Signed-off-by: James Sullivan <sullivan.james.f@gmail.com>
---
 arch/x86/kvm/irq_comm.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
index f2887ea..7e0f469 100644
--- a/arch/x86/kvm/irq_comm.c
+++ b/arch/x86/kvm/irq_comm.c
@@ -116,9 +116,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;
 }
-- 
2.3.1

[PATCH 3/9] Set default value for msi_redir_hint=false in ioapic_service

[James Sullivan]

Signed-off-by: James Sullivan <sullivan.james.f@gmail.com>
---
 arch/x86/kvm/ioapic.c | 1 +
 1 file changed, 1 insertion(+)

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);
-- 
2.3.1

[PATCH 4/9] Set default value for msi_redir_hint=false in apic_send_ipi

[James Sullivan]

Signed-off-by: James Sullivan <sullivan.james.f@gmail.com>
---
 arch/x86/kvm/lapic.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index bd4e34d..c26afc9 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
-- 
2.3.1

[PATCH 5/9] Set default value for msi_redir_hint=false in kvm_pv_kick_cpu_op

[James Sullivan]

Signed-off-by: James Sullivan <sullivan.james.f@gmail.com>
---
 arch/x86/kvm/x86.c | 1 +
 1 file changed, 1 insertion(+)

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 6/9] Deliver to only low-prio cpu in kvm_irq_delivery_to_apic_fast when MSI RH=1

[James Sullivan]

Signed-off-by: James Sullivan <sullivan.james.f@gmail.com>
---
 arch/x86/kvm/lapic.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index c26afc9..c946470 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -702,7 +702,8 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
 
 		bitmap = apic_logical_id(map, mda);
 
-		if (irq->delivery_mode == APIC_DM_LOWEST) {
+		if (irq->delivery_mode == APIC_DM_LOWEST ||
+				irq->msi_redir_hint) {
 			int l = -1;
 			for_each_set_bit(i, &bitmap, 16) {
 				if (!dst[i])
-- 
2.3.1

[PATCH 7/9] Prevent delivery to non-lowest priority vcpus in kvm_irq_delivery_to_apic

[James Sullivan]

Signed-off-by: James Sullivan <sullivan.james.f@gmail.com>
---
 arch/x86/kvm/irq_comm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
index 7e0f469..36d2ca3a 100644
--- a/arch/x86/kvm/irq_comm.c
+++ b/arch/x86/kvm/irq_comm.c
@@ -76,7 +76,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_is_dm_lowest_prio(irq) && !irq->msi_redir_hint) {
 			if (r < 0)
 				r = 0;
 			r += kvm_apic_set_irq(vcpu, irq, dest_map);
-- 
2.3.1

[PATCH 8/9] Removed TODO in kvm_set_msi_irq

[James Sullivan]

Signed-off-by: James Sullivan <sullivan.james.f@gmail.com>
---
 arch/x86/kvm/irq_comm.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
index 36d2ca3a..f993f2f 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;
-- 
2.3.1

[PATCH 9/9] Print value of msi_redir_hint in debug dump of irq in apic_send_ipi

[James Sullivan]

Signed-off-by: James Sullivan <sullivan.james.f@gmail.com>
---
 arch/x86/kvm/lapic.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index c946470..bced6d5 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -903,10 +903,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);
 }
-- 
2.3.1

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

[Radim Krčmář]

2015-03-14 18:00-0600, James Sullivan:
> * 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.

Your previous patch changed this case to physical.
(Mention in the cover letter what unmerged patches you depend on.)

> Let me know your thoughts.

Please document changes in their commits;  the information in cover
letter is virtually lost (git sucks at handling series).
If you feel like you would be repeating yourself, or have nothing to say
at all, it is a signal for rethinking the division.

The "logical change" of each patch doesn't need to be minimal -- we seek
the perfect balance between the number of patches, hunks in them, and
their complexity.  I don't see a need for more than two patches here:
 1) introduce and initialize msi_redir_hint [1-5,9/9]
 2) use msi_redir_hint for lowest-priority delivery [6-8/9]

I think it would be better to rename the kvm_is_dm_lowest_prio() helper,
check for msi_redir_hint there, move it to a header, and use in
kvm_irq_delivery_to_apic_fast() too.

Thanks.

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

[Marcelo Tosatti]

On Sat, Mar 14, 2015 at 06:00:15PM -0600, James Sullivan wrote:
> 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.

"When RH is 0, the interrupt is directed to the processor listed in the
Destination ID field."

> * 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
> 
> --
> 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