[PATCH] x86: irq_comm: Add check for RH bit in kvm_set_msi_irq

[PATCH] x86: irq_comm: Add check for RH bit in kvm_set_msi_irq

[James Sullivan]

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 <sullivan.james.f@gmail.com>
---
 arch/x86/kvm/irq_comm.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
index 72298b3..26bbab8 100644
--- a/arch/x86/kvm/irq_comm.c
+++ b/arch/x86/kvm/irq_comm.c
@@ -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,
-- 
2.3.1

Re: [PATCH] x86: irq_comm: Add check for RH bit in kvm_set_msi_irq

[Radim Krčmář]

2015-03-09 23:05-0600, James Sullivan:
> 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.)

> Fixed this to set irq->dest_mode to logical just when both bits are set,
> and physical otherwise.
> 
> Signed-off-by: James Sullivan <sullivan.james.f@gmail.com>
> ---
> diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
> @@ -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);

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

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

Re: [PATCH] x86: irq_comm: Add check for RH bit in kvm_set_msi_irq

[James Sullivan]

On 03/10/2015 08:47 AM, Radim Krčmář wrote:
...
>> +	/*
>> +	 * 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.)
> 

Thank you, that should be as follows:

    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 */
> 
> 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;
		}
	}
.....
}

> 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

Re: [PATCH] x86: irq_comm: Add check for RH bit in kvm_set_msi_irq

[Radim Krčmář]

2015-03-10 16:39-0600, James Sullivan:
> On 03/10/2015 08:47 AM, Radim Krčmář wrote:
> >> +	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.)

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

> bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
| [...]
>     	if (irq->dest_mode == APIC_DEST_PHYSICAL) {
| [...]
> 	} else {
| [...]
> 		if (irq->delivery_mode == APIC_DM_LOWEST) {

Without lowest priority, it delivers to all matching APICs.

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

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

Re: [PATCH] x86: irq_comm: Add check for RH bit in kvm_set_msi_irq

[James Sullivan]

On 03/11/2015 07:43 AM, Radim Krčmář wrote:
> 2015-03-10 16:39-0600, James Sullivan:
>> On 03/10/2015 08:47 AM, Radim Krčmář wrote:
>>>> +	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.)
>

I'll tidy that up too.

> Your patch improves the situation, but removing the TODO is not
> warranted -- RH still doesn't do what it should:
>
>>> 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).
>

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.

[PATCH v2] x86: irq_comm: Add check for RH bit in kvm_set_msi_irq

[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 <sullivan.james.f@gmail.com>
---
 arch/x86/kvm/irq_comm.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
index 72298b3..327a4b8 100644
--- a/arch/x86/kvm/irq_comm.c
+++ b/arch/x86/kvm/irq_comm.c
@@ -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,
-- 
2.3.1

Re: [PATCH v2] x86: irq_comm: Add check for RH bit in kvm_set_msi_irq

[Radim Krčmář]

2015-03-11 09:05-0600, James Sullivan:
> 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.)

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

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

> diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
> @@ -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.)

> +	/*
> +	 * 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 ...

> +	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?

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

> +·    /*
> +·     * 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.)