Skip to contentSkip to conversation

x86: irq_comm: Add check for RH bit in kvm_set_msi_irq

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

Subsystems (3)
x86 KVM KVM x86

v1 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

James Sullivan

author
#0 lore raw

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

Hi folks,

This is a small patch that implements logic to handle the RH bit being set in the msi message address register. 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 logical just when both bits are set, and physical otherwise.

Signed-off-by James Sullivan <[email protected]>
---
 arch/x86/kvm/irq_comm.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)
arch/x86/kvm/irq_comm.c +10 -2
index 72298b3..26bbab8 100644
@@ -97,18 +97,26 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
 static inline void kvm_set_msi_irq(struct kvm_kernel_irq_routing_entry *e,
 				   struct kvm_lapic_irq *irq)
 {
+	bool phys;
 	trace_kvm_msi_set_irq(e->msi.address_lo, e->msi.data);
 
 	irq->dest_id = (e->msi.address_lo &
 			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;
+	/*
+	 * Set dest_mode to logical just in case both the RH and DM
+	 * bits are set, otherwise default to physical.
+	 */
+	phys = ((e->msi.address_lo & (MSI_ADDR_REDIRECTION_LOWPRI |
+			MSI_ADDR_DEST_MODE_LOGICAL)) !=
+		(MSI_ADDR_REDIRECTION_LOWPRI |
+			MSI_ADDR_DEST_MODE_LOGICAL));
+	irq->dest_mode = phys ? 0 : (MSI_ADDR_DEST_MODE_LOGICAL);
 	irq->trig_mode = (1 << MSI_DATA_TRIGGER_SHIFT) & e->msi.data;
 	irq->delivery_mode = e->msi.data & 0x700;
 	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
#1 lore raw

2015-03-09 23:05-0600, James Sullivan:

Quoted 8 lines

Hi folks,

This is a small patch that implements logic to handle the RH bit being set in the msi message address register. 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.

(I found that https://software.intel.com/en-us/forums/topic/288883 agrees
with this definition of "ignored"; and it explains the weird design at the bottom.)

Quoted 17 lines

Fixed this to set irq->dest_mode to logical just when both bits are set, and physical otherwise.

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

---

arch/x86/kvm/irq_comm.c +1 -0
@@ -97,18 +97,26 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
 static inline void kvm_set_msi_irq(struct kvm_kernel_irq_routing_entry *e,
 				   struct kvm_lapic_irq *irq)
 {
+	bool phys;
 	trace_kvm_msi_set_irq(e->msi.address_lo, e->msi.data);
 
 	irq->dest_id = (e->msi.address_lo &
 			MSI_ADDR_DEST_ID_MASK) >> MSI_ADDR_DEST_ID_SHIFT;
 	irq->vector = (e->msi.data &
 			MSI_DATA_VECTOR_MASK) >> MSI_DATA_VECTOR_SHIFT;

arch/x86/kvm/irq_comm.c int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src, · line 115

Quoted 10 lines
arch/x86/kvm/irq_comm.c +9 -1
-	irq->dest_mode = (1 << MSI_ADDR_DEST_MODE_SHIFT) & e->msi.address_lo;
+	/*
+	 * Set dest_mode to logical just in case both the RH and DM
+	 * bits are set, otherwise default to physical.
+	 */
+	phys = ((e->msi.address_lo & (MSI_ADDR_REDIRECTION_LOWPRI |
+			MSI_ADDR_DEST_MODE_LOGICAL)) !=
+		(MSI_ADDR_REDIRECTION_LOWPRI |
+			MSI_ADDR_DEST_MODE_LOGICAL));
+	irq->dest_mode = phys ? 0 : (MSI_ADDR_DEST_MODE_LOGICAL);

(Should be APIC_DEST_LOGICAL. All works because it is a boolean and we
only checked for APIC_DEST_PHYSICAL, which is 0.)

arch/x86/kvm/irq_comm.c int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src, · line 111

Quoted 5 lines
arch/x86/kvm/irq_comm.c +0 -1
 	irq->trig_mode = (1 << MSI_DATA_TRIGGER_SHIFT) & e->msi.data;
 	irq->delivery_mode = e->msi.data & 0x700;
 	irq->level = 1;
 	irq->shorthand = 0;
-	/* TODO Deal with RH bit of MSI message address */

RH bit still isn't deal with -- we do not use lowest-priority-like delivery in logical destination mode ...

How does DM=1/RH=1 work on real hardware?
(There seem to be interesting corner cases with irq->delivery_mode like
APIC_DM_NMI.)

Thanks.

author
#2 lore raw

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

Quoted 4 lines
Quoted 4 lines
arch/x86/kvm/irq_comm.c +4 -0
+	/*
+	 * Set dest_mode to logical just in case both the RH and DM
+	 * bits are set, otherwise default to physical.
+	 */
Quoted 9 lines

arch/x86/kvm/irq_comm.c int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src, · line 115

Quoted 5 lines
arch/x86/kvm/irq_comm.c +5 -0
+	phys = ((e->msi.address_lo & (MSI_ADDR_REDIRECTION_LOWPRI |
+			MSI_ADDR_DEST_MODE_LOGICAL)) !=
+		(MSI_ADDR_REDIRECTION_LOWPRI |
+			MSI_ADDR_DEST_MODE_LOGICAL));
+	irq->dest_mode = phys ? 0 : (MSI_ADDR_DEST_MODE_LOGICAL);

(Should be APIC_DEST_LOGICAL. All works because it is a boolean and we
only checked for APIC_DEST_PHYSICAL, which is 0.)

Thank you, that should be as follows:

    irq->dest_mode = phys ? (APIC_DEST_PHYSICAL) : (APIC_DEST_LOGICAL);

?

Quoted 8 lines

arch/x86/kvm/irq_comm.c int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src, · line 111

Quoted 5 lines
arch/x86/kvm/irq_comm.c +0 -1
 	irq->trig_mode = (1 << MSI_DATA_TRIGGER_SHIFT) & e->msi.data;
 	irq->delivery_mode = e->msi.data & 0x700;
 	irq->level = 1;
 	irq->shorthand = 0;
-	/* TODO Deal with RH bit of MSI message address */

RH bit still isn't deal with -- we do not use lowest-priority-like delivery in logical destination mode ...

Just want to make sure I understand this comment-
Isn't low-pri delivery mode used in kvm_irq_delivery_to_apic_fast when irq->dest_mode > APIC_DEST_PHYSICAL (ie, logical)? (See below.)

Do you mean that this patch will interfere with this? As long as irq->dest_mode is still APIC_DEST_LOGICAL this shouldn't change.

bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
struct kvm_lapic_irq *irq, int *r, unsigned long *dest_map)
{
.....

    	if (irq->dest_mode == APIC_DEST_PHYSICAL) {
		if (irq->dest_id >= ARRAY_SIZE(map->phys_map))
			goto out;

		dst = &map->phys_map[irq->dest_id];
	} else {
		u32 mda = irq->dest_id << (32 - map->ldr_bits);
		u16 cid = apic_cluster_id(map, mda);

		if (cid >= ARRAY_SIZE(map->logical_map))
			goto out;

		dst = map->logical_map[cid];

		bitmap = apic_logical_id(map, mda);

		if (irq->delivery_mode == APIC_DM_LOWEST) {
			int l = -1;
			for_each_set_bit(i, &bitmap, 16) {
				if (!dst[i])
					continue;
				if (l < 0)
					l = i;
				else if (kvm_apic_compare_prio(dst[i]->vcpu, dst[l]->vcpu) < 0)
					l = i;
			}

			bitmap = (l >= 0) ? 1 << l : 0;
		}
	}

.....
}

Quoted 4 lines

How does DM=1/RH=1 work on real hardware?
(There seem to be interesting corner cases with irq->delivery_mode like
APIC_DM_NMI.)

The IA32 manual says that if DM=1/RH=1, we operate in logical destination mode similarly to other IPIs. I don't believe this patch introduces any invalid settings listed in section 10-21, Vol. 3, so this shouldn't create any weirdness.

Thanks
-James

#3 lore raw

2015-03-10 16:39-0600, James Sullivan:

Quoted 10 lines · Radim Krčmář
Quoted 5 lines

arch/x86/kvm/irq_comm.c int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src, · line 115

Quoted 1 line
arch/x86/kvm/irq_comm.c +1 -0
+	irq->dest_mode = phys ? 0 : (MSI_ADDR_DEST_MODE_LOGICAL);

(Should be APIC_DEST_LOGICAL. All works because it is a boolean and we
only checked for APIC_DEST_PHYSICAL, which is 0.)

Thank you, that should be as follows:

    irq->dest_mode = phys ? (APIC_DEST_PHYSICAL) : (APIC_DEST_LOGICAL);

?

Yes, thanks. (No need for parentheses around macros, we "agreed" to cover that in definition and they don't make this more readable.)

Quoted 8 lines
Quoted 4 lines

arch/x86/kvm/irq_comm.c int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src, · line 111

Quoted 1 line
arch/x86/kvm/irq_comm.c +0 -1
-	/* TODO Deal with RH bit of MSI message address */

RH bit still isn't deal with -- we do not use lowest-priority-like delivery in logical destination mode ...

Just want to make sure I understand this comment-
Isn't low-pri delivery mode used in kvm_irq_delivery_to_apic_fast when irq->dest_mode > APIC_DEST_PHYSICAL (ie, logical)? (See below.)

Not necessarily -- we have 'dest_mode' and 'delivery_mode';
- dest_mode is either logical or physical
- delivery_mode is fixed/lowest-priority/NMI/...

Logical destination can be used with multiple delivery modes and lowest-priority is just one of them:

Quoted 1 line

bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,

| [...]

Quoted 1 line
    	if (irq->dest_mode == APIC_DEST_PHYSICAL) {

| [...]

Quoted 1 line
	} else {

| [...]

Quoted 1 line
		if (irq->delivery_mode == APIC_DM_LOWEST) {

Without lowest priority, it delivers to all matching APICs.

Quoted 2 lines

Do you mean that this patch will interfere with this? As long as irq->dest_mode is still APIC_DEST_LOGICAL this shouldn't change.

Your patch improves the situation, but removing the TODO is not warranted -- RH still doesn't do what it should:

Quoted 7 lines
Quoted 3 lines

How does DM=1/RH=1 work on real hardware?
(There seem to be interesting corner cases with irq->delivery_mode like
APIC_DM_NMI.)

The IA32 manual says that if DM=1/RH=1, we operate in logical destination mode similarly to other IPIs. I don't believe this patch introduces any invalid settings listed in section 10-21, Vol. 3, so this shouldn't create any weirdness.

Quoting SDM Jan 2015, 10.11.1 Message Address Register Format:
This bit [RH] indicates whether the message should be directed to the processor with the lowest interrupt priority among processors that can receive the interrupt.

=> it should behave like lowest priority delivery. Deliver to just one APIC -- we don't do that.

KVM interrupt delivery functions can only specify lowest priority though delivery_mode and we would have to rework KVM if MSI can set something else in the delivery_mode (like NMI).

author
#4 lore raw
Quoted 8 lines · Radim Krčmář

2015-03-10 16:39-0600, James Sullivan:

Quoted 7 lines · Radim Krčmář
Quoted 5 lines

arch/x86/kvm/irq_comm.c int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src, · line 115

Quoted 1 line
arch/x86/kvm/irq_comm.c +1 -0
+	irq->dest_mode = phys ? 0 : (MSI_ADDR_DEST_MODE_LOGICAL);

(Should be APIC_DEST_LOGICAL. All works because it is a boolean and we
only checked for APIC_DEST_PHYSICAL, which is 0.)

Quoted 7 lines
Quoted 3 lines

Thank you, that should be as follows:

    irq->dest_mode = phys ? (APIC_DEST_PHYSICAL) : (APIC_DEST_LOGICAL);

?

Yes, thanks. (No need for parentheses around macros, we "agreed" to cover that in definition and they don't make this more readable.)

I'll tidy that up too.

Quoted 16 lines

Your patch improves the situation, but removing the TODO is not warranted -- RH still doesn't do what it should:

Quoted 7 lines
Quoted 3 lines

How does DM=1/RH=1 work on real hardware?
(There seem to be interesting corner cases with irq->delivery_mode like
APIC_DM_NMI.)

The IA32 manual says that if DM=1/RH=1, we operate in logical destination mode similarly to other IPIs. I don't believe this patch introduces any invalid settings listed in section 10-21, Vol. 3, so this shouldn't create any weirdness.

Quoting SDM Jan 2015, 10.11.1 Message Address Register Format:
This bit [RH] indicates whether the message should be directed to the processor with the lowest interrupt priority among processors that can receive the interrupt.

Quoted 7 lines

=> it should behave like lowest priority delivery. Deliver to just one APIC -- we don't do that.

KVM interrupt delivery functions can only specify lowest priority though delivery_mode and we would have to rework KVM if MSI can set something else in the delivery_mode (like NMI).

I see-- That will pose issues when you want some particular semantics of one delivery mode (such as ignoring vector info in NMI) but want low-pri delivery. Short of writing an MSI specific delivery function and calling it in kvm_set_msi, I can't think of a way to get this. Do you think that's warranted?

I'll document this and resubmit this patch with the TODO for now. Thanks for the insight.

author
#5 lore raw

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

In reply to James Sullivan

This is a revised patch to the previous submission, adding a check for RH=1/DM=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.

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.

A future fix may be to implement a separate interrupt delivery function for MSI interrupts in kvm_set_msi, which is a bit hacky but probably the only way to do this without modifying the kvm_lapic_irq struct. I'll write this up and see how it looks, unless there are major objections.

Signed-off-by James Sullivan <[email protected]>
---
 arch/x86/kvm/irq_comm.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)
arch/x86/kvm/irq_comm.c +18 -2
index 72298b3..327a4b8 100644
@@ -97,18 +97,34 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
 static inline void kvm_set_msi_irq(struct kvm_kernel_irq_routing_entry *e,
 				   struct kvm_lapic_irq *irq)
 {
+	bool phys;
 	trace_kvm_msi_set_irq(e->msi.address_lo, e->msi.data);
 
 	irq->dest_id = (e->msi.address_lo &
 			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;
+	/*
+	 * Set dest_mode to logical just in case both RH=1/DH=1,
+	 * otherwise default to physical
+	 */
+	phys = ((e->msi.address_lo & (MSI_ADDR_REDIRECTION_LOWPRI |
+			MSI_ADDR_DEST_MODE_LOGICAL)) !=
+		(MSI_ADDR_REDIRECTION_LOWPRI |
+			MSI_ADDR_DEST_MODE_LOGICAL));
+	irq->dest_mode = phys ? APIC_DEST_PHYSICAL : APIC_DEST_LOGICAL;
 	irq->trig_mode = (1 << MSI_DATA_TRIGGER_SHIFT) & e->msi.data;
 	irq->delivery_mode = e->msi.data & 0x700;
 	irq->level = 1;
 	irq->shorthand = 0;
-	/* TODO Deal with RH bit of MSI message address */
+	/*
+	 * 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. However other delivery semantics are still
+	 *  specified by the delivery mode, so we can't default to
+	 *  APIC_DM_LOWPRI if RH=1.
+	 */
 }
 
 int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
Signature · 1 line
2.3.1
#6 lore raw

2015-03-11 09:05-0600, James Sullivan:

Quoted 2 lines

This is a revised patch to the previous submission, adding a check for RH=1/DM=1 in kvm_set_msi_irq.

(Btw. you can put it below the /^---$/ line, where it gets automatically
trimmed -- information on changes since the last version is very important in review, but of little use later.)

Quoted 5 lines

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.

It's possible that hardware designers didn't want to waste silicon either, thus overwriting delivery mode would be the correct, and easy, thing to do. (Or have I missed its definition?)

Quoted 4 lines

A future fix may be to implement a separate interrupt delivery function for MSI interrupts in kvm_set_msi, which is a bit hacky but probably the only way to do this without modifying the kvm_lapic_irq struct. I'll write this up and see how it looks, unless there are major objections.

Thank you; no objections without patches :)

I think that exploring other options makes sense here.
We would probably re-implement most of the code, so adding extra parameter to interrupt delivery functions would be easier to maintain. Extending 'struct kvm_lapic_irq' seems acceptable too: it's an internal structure and we already have kvm_is_dm_lowest_prio() helper.

arch/x86/kvm/irq_comm.c int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src, · line 101

Quoted 7 lines
arch/x86/kvm/irq_comm.c +1 -0
@@ -97,18 +97,34 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
 static inline void kvm_set_msi_irq(struct kvm_kernel_irq_routing_entry *e,
 				   struct kvm_lapic_irq *irq)
 {
+	bool phys;
 	trace_kvm_msi_set_irq(e->msi.address_lo, e->msi.data);

(We usually have have an empty line after declarations.
I've seen patches that fixed just this, because some tool complains.)

arch/x86/kvm/irq_comm.c int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src, · line 110

Quoted 4 lines
arch/x86/kvm/irq_comm.c +4 -0
+	/*
+	 * Set dest_mode to logical just in case both RH=1/DH=1,
+	 * otherwise default to physical
+	 */

This comment contains the same algorithm written in other (less defined) language, right next to a similarly complex implementation ...

arch/x86/kvm/irq_comm.c int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src, · line 115

Quoted 5 lines
arch/x86/kvm/irq_comm.c +5 -0
+	phys = ((e->msi.address_lo & (MSI_ADDR_REDIRECTION_LOWPRI |
+			MSI_ADDR_DEST_MODE_LOGICAL)) !=
+		(MSI_ADDR_REDIRECTION_LOWPRI |
+			MSI_ADDR_DEST_MODE_LOGICAL));
+	irq->dest_mode = phys ? APIC_DEST_PHYSICAL : APIC_DEST_LOGICAL;

Wouldn't changing white spacing, using 'if', defining a union mask, abstracting the all-bits-in-mask-are-set operation, or something help to make this code clear without a comment?

arch/x86/kvm/irq_comm.c int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src, · line 117

Quoted 1 line
arch/x86/kvm/irq_comm.c +0 -0
 	irq->delivery_mode = e->msi.data & 0x700;

A system that configured DM and RH might not handle delivery to multiple APICs. Please add a warning (once), to ease debugging if/when this feature gets its first user.

Thanks.

arch/x86/kvm/irq_comm.c int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src, · line 119

Quoted 8 lines
arch/x86/kvm/irq_comm.c +8 -0
+·    /*
+·     * 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. However other delivery semantics are still
+·     *  specified by the delivery mode, so we can't default to
+·     *  APIC_DM_LOWPRI if RH=1.
+·     */

(We would, if there was some benefit in erring that way.)