x86: irq_comm: Add check for RH bit in kvm_set_msi_irq

PATCH Superseded James Sullivan 2 participants 7 messages Mar 12, 2015 – Mar 13, 2015

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

James Sullivan

author

2015-03-12 23:43 UTC #0 lore raw

PATCH v3 1/1 x86: irq_comm: Add check for RH bit in kvm_set_msi_irq

This patch adds a check in kvm_set_msi_irq for RH=1.

Currently the DM bit is the only thing used to decide irq->dest_mode (logical when DM set, physical when unset). Documentation indicates that the DM bit will be 'ignored' when the RH bit is unset, and physical destination mode is used in this case.

Fixed this to set irq->dest_mode to APIC_DEST_LOGICAL just in case both RH=1/DM=1.

This patch doesn't completely handle RH=1; if RH=1 then the delivery will behave as in low priority mode (deliver the interrupt to only the lowest priority processor), but the delivery mode is still used to specify the semantics of the delivery beyond its destination. We can't just set irq->delivery_mode to APIC_DM_LOWPRI if RH=1 since this squashes the other delivery semantics. I've documented this in the patch.

I will be trying and comparing a few options to handle this fully (extension of struct kvm_lapic_irq, introduction of MSI specific delivery functions or helpers, etc) and hope to have some patches to show in the near future.

Signed-off-by James Sullivan <[email protected]>

---
Changes since v2:
* Added one time warning message when RH=1
* Documented conflict between RH=1 and delivery mode
* Tidied code to check RH=1/DM=1 (remove bool phys, use if/else)

 arch/x86/kvm/irq_comm.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

arch/x86/kvm/irq_comm.c +15 -2

index 72298b3..5975a3d 100644
@@ -103,12 +103,25 @@ 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;
-	irq->dest_mode = (1 << MSI_ADDR_DEST_MODE_SHIFT) & e->msi.address_lo;
+	/*
+	 * 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) ^
+			(MS_ADDR_DEST_MODE_LOGICAL))
+		irq->dest_mode = APIC_DEST_LOGICAL;
+	else
+		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 may not correctly deliver MSIs "
+			"to multiple APICs with RH set.\n");
 	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,

Signature · 1 line

2.3.1

James Sullivan

author

2015-03-13 02:52 UTC #1 lore raw

(Disregard past message- submitted the wrong patch file, apologies.)

This patch adds a check for RH = 1 in kvm_set_msi_irq. Currently the DM bit is the only thing used to decide irq->dest_mode (logical when DM set, physical when unset). Documentation indicates that the DM bit will be 'ignored' when the RH bit is unset, and physical destination mode is used in this case.

Fixed this to set irq->dest_mode to APIC_DEST_LOGICAL just in case both RH=1/DM=1.

Signed-off-by James Sullivan <[email protected]>

---
Changes since v2:
* Added one time warning message when RH=1
* Documented conflict between RH=1 and delivery mode
* Tidied code to check RH=1/DM=1 (remove bool phys, use if/else)

 arch/x86/kvm/irq_comm.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

arch/x86/kvm/irq_comm.c +15 -2

index 72298b3..35b4beb 100644
@@ -103,12 +103,25 @@ 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;
-	irq->dest_mode = (1 << MSI_ADDR_DEST_MODE_SHIFT) & e->msi.address_lo;
+	/*
+	 * 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 ^
+			MSI_ADDR_DEST_MODE_LOGICAL))
+		irq->dest_mode = APIC_DEST_LOGICAL;
+	else
+		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 may not correctly deliver MSIs "
+			"with RH set.\n");
 	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,

Signature · 1 line

2.3.1

James Sullivan

author

2015-03-13 03:11 UTC #2 lore raw

PATCH v4 1/1 x86: irq_comm: Add check for RH bit in kvm_set_msi_irq

(Correction of a rather obvious and embarrassing logical error in v3).

This patch adds a check for RH=1 in kvm_set_msi_irq. Currently the DM bit is the only thing used to decide irq->dest_mode (logical when DM set, physical when unset). Documentation indicates that the DM bit will be 'ignored' when the RH bit is unset, and physical destination mode is used in this case.

Fixed this to set irq->dest_mode to APIC_DEST_LOGICAL just in case both RH=1/DM=1.

Signed-off-by James Sullivan <[email protected]>

---
Changes since v2:
* Added one time warning message when RH=1
* Documented conflict between RH=1 and delivery mode
* Tidied code to check RH=1/DM=1 (remove bool phys, use if/else)
Changes since v3:
* Fixed logical error in RH=1/DM=1 check
* Aligned quotation blocks in multiline pr_warn_once argument

 arch/x86/kvm/irq_comm.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

arch/x86/kvm/irq_comm.c +14 -2

index 72298b3..9514fed 100644
@@ -103,12 +103,24 @@ 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;
-	irq->dest_mode = (1 << MSI_ADDR_DEST_MODE_SHIFT) & e->msi.address_lo;
+	/*
+	 * 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;
+	else
+		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 may not correctly deliver MSIs "
+			     "with RH set.\n");
 	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,

Signature · 1 line

2.3.1

Radim Krčmář

Reviewed-by on v4 1/1

2015-03-13 14:40 UTC #3 lore raw

2015-03-12 21:08-0600, James Sullivan:

Quoted 21 lines

---
Changes since v2:
* Added one time warning message when RH=1
* Documented conflict between RH=1 and delivery mode
* Tidied code to check RH=1/DM=1 (remove bool phys, use if/else)
Changes since v3:
* Fixed logical error in RH=1/DM=1 check
* Aligned quotation blocks in multiline pr_warn_once argument
arch/x86/kvm/irq_comm.c +6 -1
@@ -103,12 +103,24 @@ 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;
-	irq->dest_mode = (1 << MSI_ADDR_DEST_MODE_SHIFT) & e->msi.address_lo;
+	/*
+	 * 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.
+	 */

arch/x86/kvm/irq_comm.c static inline void kvm_set_msi_irq(struct kvm_kernel_irq_routing_entry *e, · line 121

Quoted 10 lines

arch/x86/kvm/irq_comm.c +8 -0

+	if ((e->msi.address_lo & MSI_ADDR_REDIRECTION_LOWPRI) &&
+			(e->msi.address_lo & MSI_ADDR_DEST_MODE_LOGICAL))
+		irq->dest_mode = APIC_DEST_LOGICAL;
+	else
+		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 may not correctly deliver MSIs "
+			     "with RH set.\n");

Please begin the warning with "kvm: ".

It's better to have a line bit longer than 80 columns than to break a string that we might want to grep for; reword if possible, or you might prefer something like
pr_warn_once(
"long");

(Documentation/CodingStyle, Chapter 2. It's nitpicking, sorry, but we
recently had a patch that fixed just that too ...
http://permalink.gmane.org/gmane.comp.emulators.kvm.devel/133110)

Then,

Reviewed-by Radim Krčmář <[email protected]>

Thanks.

---
The warning message is very clever:
- it contains the magical "may" qualifier and being protected only by RH=1 creates weird-looking code structure, but it is technically right
1) lowest-priority delivery may be set in msi.data, which avoids our otherwise incorrect behavior with RH=1/DM=1
2) RH=1/DM=0 can't deliver to multiple APICs (broadcast is forbidden), but real hardware may overwrite delivery mode from msi.data
- being two lines apart adds to suspicion, yet it can be hint to those possible problems

I only fear it is too clever :)

James Sullivan

author

2015-03-13 14:49 UTC #4 lore raw

On 03/13/2015 08:39 AM, Radim Krčmář wrote:
...

Quoted 10 lines

The warning message is very clever:
- it contains the magical "may" qualifier and being protected only by RH=1 creates weird-looking code structure, but it is technically right
1) lowest-priority delivery may be set in msi.data, which avoids our otherwise incorrect behavior with RH=1/DM=1
2) RH=1/DM=0 can't deliver to multiple APICs (broadcast is forbidden), but real hardware may overwrite delivery mode from msi.data
- being two lines apart adds to suspicion, yet it can be hint to those possible problems

Quoted 2 lines

I only fear it is too clever :)

For the error message, how does:

kvm: MSI RH=1 unsupported, use low-priority delivery mode

Sit with you?

-James

Radim Krčmář

2015-03-13 15:08 UTC #5 lore raw

2015-03-13 08:47-0600, James Sullivan:

Quoted 12 lines · Radim Krčmář

...
Quoted 10 lines
The warning message is very clever:
- it contains the magical "may" qualifier and being protected only by RH=1 creates weird-looking code structure, but it is technically right
1) lowest-priority delivery may be set in msi.data, which avoids our otherwise incorrect behavior with RH=1/DM=1
2) RH=1/DM=0 can't deliver to multiple APICs (broadcast is forbidden), but real hardware may overwrite delivery mode from msi.data
- being two lines apart adds to suspicion, yet it can be hint to those possible problems

Quoted 8 lines

Quoted 2 lines
I only fear it is too clever :)
For the error message, how does:
kvm: MSI RH=1 unsupported, use low-priority delivery mode
Sit with you?

I actually liked the former.

New one doesn't say what is the impact of the error and the advice is not easy follow -- people usually have no idea what low-priority delivery mode is and nothing can be done outside of the guest.

(I put the rant mainly for future reviewers; the alternative I had in
was to warn only when DM=1.)

James Sullivan

author

2015-03-13 15:15 UTC #6 lore raw

Perfect, thanks for the feedback. I'll get v5 out shortly.

Quoted 31 lines · Radim Krčmář

2015-03-13 08:47-0600, James Sullivan:
Quoted 12 lines · Radim Krčmář
...
Quoted 10 lines
The warning message is very clever:
- it contains the magical "may" qualifier and being protected only by RH=1 creates weird-looking code structure, but it is technically right
1) lowest-priority delivery may be set in msi.data, which avoids our otherwise incorrect behavior with RH=1/DM=1
2) RH=1/DM=0 can't deliver to multiple APICs (broadcast is forbidden), but real hardware may overwrite delivery mode from msi.data
- being two lines apart adds to suspicion, yet it can be hint to those possible problems
Quoted 8 lines
Quoted 2 lines
I only fear it is too clever :)
For the error message, how does:
kvm: MSI RH=1 unsupported, use low-priority delivery mode
Sit with you?
I actually liked the former.
New one doesn't say what is the impact of the error and the advice is not easy follow -- people usually have no idea what low-priority delivery mode is and nothing can be done outside of the guest.
(I put the rant mainly for future reviewers; the alternative I had in
was to warn only when DM=1.)