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

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

[James Sullivan]

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.

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 may still used to specify the semantics of the delivery beyond
its destination.

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 <sullivan.james.f@gmail.com>
---
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
Changes since v4:
    * Put error message string on single line

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

diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
index 72298b3..f2887ea 100644
--- a/arch/x86/kvm/irq_comm.c
+++ b/arch/x86/kvm/irq_comm.c
@@ -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: MSIs may not be correctly delivered 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,
-- 
2.3.1

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

[Radim Krčmář]

2015-03-13 09:14-0600, James Sullivan:
> ---
> 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
> Changes since v4:
>     * Put error message string on single line

Thanks,

Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>

> diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
> @@ -103,12 +103,24 @@ static inline void kvm_set_msi_irq(struct kvm_kernel_irq_routing_entry *e,
> +	if (e->msi.address_lo & MSI_ADDR_REDIRECTION_LOWPRI)
> +		pr_warn_once(
> +			"kvm: MSIs may not be correctly delivered with RH set.\n");

(The joys of 80+ columns!)

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

[James Sullivan]

N.B. This patch has been re-submitted in a larger patch, see

<1426555822-3280-1-git-send-email-sullivan.james.f@gmail.com>

(The new patch relies on changes made in this patch, and as such
it makes more sense to bundle them)

On 03/13/2015 09:14 AM, James Sullivan wrote:
> 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.
> 
> 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 may still used to specify the semantics of the delivery beyond
> its destination.
> 
> 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 <sullivan.james.f@gmail.com>
> ---
> 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
> Changes since v4:
>     * Put error message string on single line
> 
>  arch/x86/kvm/irq_comm.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
> index 72298b3..f2887ea 100644
> --- a/arch/x86/kvm/irq_comm.c
> +++ b/arch/x86/kvm/irq_comm.c
> @@ -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: MSIs may not be correctly delivered 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,
> 

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

[Marcelo Tosatti]

On Fri, Mar 13, 2015 at 09:14:35AM -0600, James Sullivan wrote:
> 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.
> 
> 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 may still used to specify the semantics of the delivery beyond
> its destination.
> 
> 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 <sullivan.james.f@gmail.com>

The documentation states the following:

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

* If RH is 0, then the DM bit is ignored and the message is sent ahead
independent of whether the physical or logical destination mode is used.

However, from the POV of a device writing to memory to generate an MSI 
interrupt, there is no (or i can't see any) other information that 
can be used to infer logical or physical mode for the interrupt message.

Before your patch:

(dm, rh) = (0, 0) => irq->dest_mode = 0
(dm, rh) = (0, 1) => irq->dest_mode = 0
(dm, rh) = (1, 0) => irq->dest_mode = 1
(dm, rh) = (1, 1) => irq->dest_mode = 1

After your patch:

(dm, rh) = (0, 0) => irq->dest_mode = 0
(dm, rh) = (0, 1) => irq->dest_mode = 0
(dm, rh) = (1, 0) => irq->dest_mode = 0
(dm, rh) = (1, 1) => irq->dest_mode = 1


Am i missing some explicit documentation that refers 
to (dm, rh) = (1, 0) => irq->dest_mode = 0 ?

See native_compose_msi_msg:

        msg->address_lo =
                MSI_ADDR_BASE_LO |
                ((apic->irq_dest_mode == 0) ?
                        MSI_ADDR_DEST_MODE_PHYSICAL :
                        MSI_ADDR_DEST_MODE_LOGICAL) |
                ((apic->irq_delivery_mode != dest_LowestPrio) ?
                        MSI_ADDR_REDIRECTION_CPU :
                        MSI_ADDR_REDIRECTION_LOWPRI) |
                MSI_ADDR_DEST_ID(dest);


So it does configure DM = MSI_ADDR_DEST_MODE_LOGICAL
and RH = MSI_ADDR_REDIRECTION_LOWPRI.

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

[James Sullivan]

> 
> The documentation states the following:
> 
> * When RH is 0, the interrupt is directed to the processor listed in the
> Destination ID field.
> 
> * If RH is 0, then the DM bit is ignored and the message is sent ahead
> independent of whether the physical or logical destination mode is used.
> 
> However, from the POV of a device writing to memory to generate an MSI 
> interrupt, there is no (or i can't see any) other information that 
> can be used to infer logical or physical mode for the interrupt message.
> 
> Before your patch:
> 
> (dm, rh) = (0, 0) => irq->dest_mode = 0
> (dm, rh) = (0, 1) => irq->dest_mode = 0
> (dm, rh) = (1, 0) => irq->dest_mode = 1
> (dm, rh) = (1, 1) => irq->dest_mode = 1
> 
> After your patch:
> 
> (dm, rh) = (0, 0) => irq->dest_mode = 0
> (dm, rh) = (0, 1) => irq->dest_mode = 0
> (dm, rh) = (1, 0) => irq->dest_mode = 0
> (dm, rh) = (1, 1) => irq->dest_mode = 1
> 
> 
> Am i missing some explicit documentation that refers 
> to (dm, rh) = (1, 0) => irq->dest_mode = 0 ?

>From the IA32 manual (Vol. 3, 10.11.2):

 * When RH is 0, the interrupt is directed to the processor listed
   in the Destination ID field.
 * When RH is 1 and the physical destination mode is used, the Destination
   ID field must not be set to FFH; it must point to a processor that is
   present and enabled to receive the interrupt.
 * When RH is 1 and the logical destination mode is active in a system using
   a flat addressing model, the Destination ID field must be set so that bits
   set to 1 identify processors that are present and enabled to receive the
   interrupt.
 * If RH is set to 1 and the logical destination mode is active in a system
   using cluster addressing model, then Destination ID field must not be
   set to FFH; the processors identified with this field must be present
   and enabled to receive the interrupt.

My interpretation of this is that RH=0 indicates that the Dest. ID field
contains an APIC ID, and as such destination mode is physical. When RH=1,
depending on the value of DM, we either use physical or logical dest mode.
The result of this is that logical dest mode is set just when RH=1/DM=1,
as far as I understand.

> 
> See native_compose_msi_msg:
> 
>         msg->address_lo =
>                 MSI_ADDR_BASE_LO |
>                 ((apic->irq_dest_mode == 0) ?
>                         MSI_ADDR_DEST_MODE_PHYSICAL :
>                         MSI_ADDR_DEST_MODE_LOGICAL) |
>                 ((apic->irq_delivery_mode != dest_LowestPrio) ?
>                         MSI_ADDR_REDIRECTION_CPU :
>                         MSI_ADDR_REDIRECTION_LOWPRI) |
>                 MSI_ADDR_DEST_ID(dest);
> 
> 
> So it does configure DM = MSI_ADDR_DEST_MODE_LOGICAL
> and RH = MSI_ADDR_REDIRECTION_LOWPRI.
> 

...and yet this is a good counterexample against my argument :)

What I think I'll do is revert this particular change so that dest_mode is
set independently of RH. While I'm not entirely convinced that this is the
intended interpretation, I do think that consistency with the existing logic
is probably desirable for the time being. If I can get closure on the matter
I'll re-submit that change, but for the time being I will undo it.

-James

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

[Marcelo Tosatti]

On Wed, Mar 18, 2015 at 06:59:22PM -0600, James Sullivan wrote:
> > 
> > The documentation states the following:
> > 
> > * When RH is 0, the interrupt is directed to the processor listed in the
> > Destination ID field.
> > 
> > * If RH is 0, then the DM bit is ignored and the message is sent ahead
> > independent of whether the physical or logical destination mode is used.
> > 
> > However, from the POV of a device writing to memory to generate an MSI 
> > interrupt, there is no (or i can't see any) other information that 
> > can be used to infer logical or physical mode for the interrupt message.
> > 
> > Before your patch:
> > 
> > (dm, rh) = (0, 0) => irq->dest_mode = 0
> > (dm, rh) = (0, 1) => irq->dest_mode = 0
> > (dm, rh) = (1, 0) => irq->dest_mode = 1
> > (dm, rh) = (1, 1) => irq->dest_mode = 1
> > 
> > After your patch:
> > 
> > (dm, rh) = (0, 0) => irq->dest_mode = 0
> > (dm, rh) = (0, 1) => irq->dest_mode = 0
> > (dm, rh) = (1, 0) => irq->dest_mode = 0
> > (dm, rh) = (1, 1) => irq->dest_mode = 1
> > 
> > 
> > Am i missing some explicit documentation that refers 
> > to (dm, rh) = (1, 0) => irq->dest_mode = 0 ?
> 
> >From the IA32 manual (Vol. 3, 10.11.2):
> 
>  * When RH is 0, the interrupt is directed to the processor listed
>    in the Destination ID field.
>  * When RH is 1 and the physical destination mode is used, the Destination
>    ID field must not be set to FFH; it must point to a processor that is
>    present and enabled to receive the interrupt.
>  * When RH is 1 and the logical destination mode is active in a system using
>    a flat addressing model, the Destination ID field must be set so that bits
>    set to 1 identify processors that are present and enabled to receive the
>    interrupt.
>  * If RH is set to 1 and the logical destination mode is active in a system
>    using cluster addressing model, then Destination ID field must not be
>    set to FFH; the processors identified with this field must be present
>    and enabled to receive the interrupt.
> 
> My interpretation of this is that RH=0 indicates that the Dest. ID field
> contains an APIC ID, and as such destination mode is physical. When RH=1,
> depending on the value of DM, we either use physical or logical dest mode.
> The result of this is that logical dest mode is set just when RH=1/DM=1,
> as far as I understand.
> 
> > 
> > See native_compose_msi_msg:
> > 
> >         msg->address_lo =
> >                 MSI_ADDR_BASE_LO |
> >                 ((apic->irq_dest_mode == 0) ?
> >                         MSI_ADDR_DEST_MODE_PHYSICAL :
> >                         MSI_ADDR_DEST_MODE_LOGICAL) |
> >                 ((apic->irq_delivery_mode != dest_LowestPrio) ?
> >                         MSI_ADDR_REDIRECTION_CPU :
> >                         MSI_ADDR_REDIRECTION_LOWPRI) |
> >                 MSI_ADDR_DEST_ID(dest);
> > 
> > 
> > So it does configure DM = MSI_ADDR_DEST_MODE_LOGICAL
> > and RH = MSI_ADDR_REDIRECTION_LOWPRI.
> > 
> 
> ...and yet this is a good counterexample against my argument :)
> 
> What I think I'll do is revert this particular change so that dest_mode is
> set independently of RH. While I'm not entirely convinced that this is the
> intended interpretation, I do think that consistency with the existing logic
> is probably desirable for the time being. If I can get closure on the matter
> I'll re-submit that change, but for the time being I will undo it.
> 
> -James

Just write MSI-X table entries on real hardware (say: modify
native_compose_msi_msg or MSI-X equivalent), with all RH/DM
combinations, and see what behaviour is
comes up?  

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

[Radim Krčmář]

2015-03-18 22:09-0300, Marcelo Tosatti:
> > > See native_compose_msi_msg:
> > >                 ((apic->irq_dest_mode == 0) ?
> > >                         MSI_ADDR_DEST_MODE_PHYSICAL :
> > >                         MSI_ADDR_DEST_MODE_LOGICAL) |
> > >                 ((apic->irq_delivery_mode != dest_LowestPrio) ?
> > >                         MSI_ADDR_REDIRECTION_CPU :
> > >                         MSI_ADDR_REDIRECTION_LOWPRI) |
> > > So it does configure DM = MSI_ADDR_DEST_MODE_LOGICAL
> > > and RH = MSI_ADDR_REDIRECTION_LOWPRI.
> > ...and yet this is a good counterexample against my argument :)

(It could be just to make the code nicer ... the developer might have
 known how real hardware will handle it.)

> > What I think I'll do is revert this particular change so that dest_mode is
> > set independently of RH. While I'm not entirely convinced that this is the
> > intended interpretation, I do think that consistency with the existing logic
> > is probably desirable for the time being. If I can get closure on the matter
> > I'll re-submit that change, but for the time being I will undo it.
> Just write MSI-X table entries on real hardware (say: modify
> native_compose_msi_msg or MSI-X equivalent), with all RH/DM
> combinations, and see what behaviour is
> comes up?  

I second this idea.
(We'd also get to know how RH interacts with delivery mode.)

https://software.intel.com/en-us/forums/topic/288883
said that DM=1+RH=0 delivers to physical:

  The exact quote from 10.11.1 is "When RH is 0, the interrupt is
  directed to the processor listed in the Destination ID field."
  This does not specify if physical or logical addressing mode is used.

  Experimentation shows that physical addressing mode is used
  with RH equal to zero.

and it also mentioned a disturbing behavior, which I chose to ignore:

  10.11.1 goes on to say that "When RH is 1 and the physical destination
  mode is used [i.e., DM = 0], the Destination ID field must not be set
  to 0xFF; it must point to a processor that is present and enabled to
  receive the interrupt."

  This would seem to be the exact same case as RH equal to zero;
  there, DM is ignored: "If RH is 0, then the DM bit is
  ignored and the message is sent ahead independent of whether
  the physical or logical destination mode is used."

  However, changing RH to 1 and DM to zero fails to send the message
  to the physical processor.

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

[James Sullivan]

On 03/19/2015 07:00 AM, Radim Krčmář wrote:
> 2015-03-18 22:09-0300, Marcelo Tosatti:
>>>> See native_compose_msi_msg:
>>>>                 ((apic->irq_dest_mode == 0) ?
>>>>                         MSI_ADDR_DEST_MODE_PHYSICAL :
>>>>                         MSI_ADDR_DEST_MODE_LOGICAL) |
>>>>                 ((apic->irq_delivery_mode != dest_LowestPrio) ?
>>>>                         MSI_ADDR_REDIRECTION_CPU :
>>>>                         MSI_ADDR_REDIRECTION_LOWPRI) |
>>>> So it does configure DM = MSI_ADDR_DEST_MODE_LOGICAL
>>>> and RH = MSI_ADDR_REDIRECTION_LOWPRI.
>>> ...and yet this is a good counterexample against my argument :)
> 
> (It could be just to make the code nicer ... the developer might have
>  known how real hardware will handle it.)
> 
>>> What I think I'll do is revert this particular change so that dest_mode is
>>> set independently of RH. While I'm not entirely convinced that this is the
>>> intended interpretation, I do think that consistency with the existing logic
>>> is probably desirable for the time being. If I can get closure on the matter
>>> I'll re-submit that change, but for the time being I will undo it.
>> Just write MSI-X table entries on real hardware (say: modify
>> native_compose_msi_msg or MSI-X equivalent), with all RH/DM
>> combinations, and see what behaviour is
>> comes up?  
> 
> I second this idea.
> (We'd also get to know how RH interacts with delivery mode.)
> 

I played around with native_compose_msi_msg and discovered the following:

* dm=0, rh=0 => Physical Destination Mode
* dm=0, rh=1 => Failed delivery
* dm=1, rh=0 => Logical Destination Mode, No Redirection
* dm=1, rh=1 => Logical Destination Mode, Redirection

So it seems to be the case that logical destination mode is used whenever
DM=1, regardless of RH. Furthermore, the case where DM=0 and RH=1 is
undefined, as was indicated in the closing response to the thread in
https://software.intel.com/en-us/forums/topic/288883 :

"...X86 supports logical mode but does not support redirection of physical mode
interrupts. I think they should have just said RH=1 is not defined in physical
mode, but instead they are trying to tell you what the restrictions are if you
set it on."

The default config for PCI devices seems to be DM=1,RH=1.

-James

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

[Radim Krčmář]

2015-03-19 16:51-0600, James Sullivan:
> I played around with native_compose_msi_msg and discovered the following:
> 
> * dm=0, rh=0 => Physical Destination Mode
> * dm=0, rh=1 => Failed delivery
> * dm=1, rh=0 => Logical Destination Mode, No Redirection
> * dm=1, rh=1 => Logical Destination Mode, Redirection

Great!  (What CPU family was that?)

> So it seems to be the case that logical destination mode is used whenever
> DM=1, regardless of RH. Furthermore, the case where DM=0 and RH=1 is
> undefined, as was indicated in the closing response to the thread in
> https://software.intel.com/en-us/forums/topic/288883 :

DM=0+RH=1 might be defined to "fail", but I think it's acceptable to
treat it as undefined.  (Deliver them in KVM if it improves something.)

I'm still wondering about last sentence from that link, the
parenthesised part to be exact,
  The reference to the APIC ID being 0xff is because 0xff is broadcast
  and lowest priority (what the RH bit really is for X86) is illegal
  with broadcast.

Can you also check if RH=1 does something to delivery mode?

Thanks.

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

[James Sullivan]

On 03/20/2015 09:15 AM, Radim Krčmář wrote:
> 2015-03-19 16:51-0600, James Sullivan:
>> I played around with native_compose_msi_msg and discovered the following:
>>
>> * dm=0, rh=0 => Physical Destination Mode
>> * dm=0, rh=1 => Failed delivery
>> * dm=1, rh=0 => Logical Destination Mode, No Redirection
>> * dm=1, rh=1 => Logical Destination Mode, Redirection
> 
> Great!  (What CPU family was that?)
> 

This was on Intel x86_64 (Core i5-3210m, 'Ivy Bridge').

>> So it seems to be the case that logical destination mode is used whenever
>> DM=1, regardless of RH. Furthermore, the case where DM=0 and RH=1 is
>> undefined, as was indicated in the closing response to the thread in
>> https://software.intel.com/en-us/forums/topic/288883 :
> 
> DM=0+RH=1 might be defined to "fail", but I think it's acceptable to
> treat it as undefined.  (Deliver them in KVM if it improves something.)
> 

My thoughts as well.

> I'm still wondering about last sentence from that link, the
> parenthesised part to be exact,
>   The reference to the APIC ID being 0xff is because 0xff is broadcast
>   and lowest priority (what the RH bit really is for X86) is illegal
>   with broadcast.
> 
> Can you also check if RH=1 does something to delivery mode?
> 
> Thanks.
> 

Sure, I'll look into that as well.

-James

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

[James Sullivan]

On 03/20/2015 09:22 AM, James Sullivan wrote:
> On 03/20/2015 09:15 AM, Radim Krčmář wrote:
>> 2015-03-19 16:51-0600, James Sullivan:
>>> I played around with native_compose_msi_msg and discovered the following:
>>>
>>> * dm=0, rh=0 => Physical Destination Mode
>>> * dm=0, rh=1 => Failed delivery
>>> * dm=1, rh=0 => Logical Destination Mode, No Redirection
>>> * dm=1, rh=1 => Logical Destination Mode, Redirection
>>
>> Great!  (What CPU family was that?)
>>
> 
> This was on Intel x86_64 (Core i5-3210m, 'Ivy Bridge').
> 
>>> So it seems to be the case that logical destination mode is used whenever
>>> DM=1, regardless of RH. Furthermore, the case where DM=0 and RH=1 is
>>> undefined, as was indicated in the closing response to the thread in
>>> https://software.intel.com/en-us/forums/topic/288883 :
>>
>> DM=0+RH=1 might be defined to "fail", but I think it's acceptable to
>> treat it as undefined.  (Deliver them in KVM if it improves something.)
>>
> 
> My thoughts as well.
> 
>> I'm still wondering about last sentence from that link, the
>> parenthesised part to be exact,
>>   The reference to the APIC ID being 0xff is because 0xff is broadcast
>>   and lowest priority (what the RH bit really is for X86) is illegal
>>   with broadcast.
>>
>> Can you also check if RH=1 does something to delivery mode?
>>
>> Thanks.
>>
> 
> Sure, I'll look into that as well.
> 
> -James
> 

I haven't seen any changes in the MSI Data Register for any values of RH,
but I don't have a great sample size (one machine with one set of PCI devices),
so if anyone else can confirm that I would appreciate it.

Worth noting that low prio delivery was used across the board for my PCI devices
regardless of RH=1 or 0, so it doesn't seem to be de facto the case that the RH
bit's only purpose is for lowprio delivery on x86. Again, need to have some more
PCI devices to test against to confirm anything.

-James

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

[Radim Krčmář]

2015-03-20 11:50-0600, James Sullivan:
> On 03/20/2015 09:22 AM, James Sullivan wrote:
> > On 03/20/2015 09:15 AM, Radim Krčmář wrote:
> >> 2015-03-19 16:51-0600, James Sullivan:
> >>> I played around with native_compose_msi_msg and discovered the following:
> >>>
> >>> * dm=0, rh=0 => Physical Destination Mode
> >>> * dm=0, rh=1 => Failed delivery
> >>> * dm=1, rh=0 => Logical Destination Mode, No Redirection
> >>> * dm=1, rh=1 => Logical Destination Mode, Redirection
> >>
> >> Great!  (What CPU family was that?)
> >>
> > 
> > This was on Intel x86_64 (Core i5-3210m, 'Ivy Bridge').

Thanks, it's possible that the behavior of chipsets changed since the
report on Intel's forum ...
(Lowest priority behaved differently before QPI, so it might coincide.)

> >> I'm still wondering about last sentence from that link, the
> >> parenthesised part to be exact,
> >>   The reference to the APIC ID being 0xff is because 0xff is broadcast
> >>   and lowest priority (what the RH bit really is for X86) is illegal
> >>   with broadcast.
> >>
> >> Can you also check if RH=1 does something to delivery mode?
> 
> I haven't seen any changes in the MSI Data Register for any values of RH,
> but I don't have a great sample size (one machine with one set of PCI devices),
> so if anyone else can confirm that I would appreciate it.

I meant if the delivery mode from data register isn't ignored with RH=1,
and the message delivered as if lowest-priority was set there.
(Decided by having something else than fixed or lowest-priority there.)

> Worth noting that low prio delivery was used across the board for my PCI devices
> regardless of RH=1 or 0, so it doesn't seem to be de facto the case that the RH
> bit's only purpose is for lowprio delivery on x86.

Yeah, afaik, it can be done with lowest priority delivery mode on ia64
too, so I have a hard time finding RH's intended purpose.

>                                                    Again, need to have some more
> PCI devices to test against to confirm anything.

It's impossible to test everything, and there is no conflict if we have
at most one data point ;)

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

[James Sullivan]

On 03/23/2015 03:13 PM, Radim Krčmář wrote:
> 2015-03-20 11:50-0600, James Sullivan:
>> On 03/20/2015 09:22 AM, James Sullivan wrote:
>>> On 03/20/2015 09:15 AM, Radim Krčmář wrote:
>>>> 2015-03-19 16:51-0600, James Sullivan:
>>>>> I played around with native_compose_msi_msg and discovered the following:
>>>>>
>>>>> * dm=0, rh=0 => Physical Destination Mode
>>>>> * dm=0, rh=1 => Failed delivery
>>>>> * dm=1, rh=0 => Logical Destination Mode, No Redirection
>>>>> * dm=1, rh=1 => Logical Destination Mode, Redirection
>>>>
>>>> Great!  (What CPU family was that?)
>>>>
>>>
>>> This was on Intel x86_64 (Core i5-3210m, 'Ivy Bridge').
> 
> Thanks, it's possible that the behavior of chipsets changed since the
> report on Intel's forum ...
> (Lowest priority behaved differently before QPI, so it might coincide.)
> 
>>>> I'm still wondering about last sentence from that link, the
>>>> parenthesised part to be exact,
>>>>   The reference to the APIC ID being 0xff is because 0xff is broadcast
>>>>   and lowest priority (what the RH bit really is for X86) is illegal
>>>>   with broadcast.
>>>>
>>>> Can you also check if RH=1 does something to delivery mode?
>>
>> I haven't seen any changes in the MSI Data Register for any values of RH,
>> but I don't have a great sample size (one machine with one set of PCI devices),
>> so if anyone else can confirm that I would appreciate it.
> 
> I meant if the delivery mode from data register isn't ignored with RH=1,
> and the message delivered as if lowest-priority was set there.
> (Decided by having something else than fixed or lowest-priority there.)
> 

Hmm, any thoughts on how I could test for that?

>> Worth noting that low prio delivery was used across the board for my PCI devices
>> regardless of RH=1 or 0, so it doesn't seem to be de facto the case that the RH
>> bit's only purpose is for lowprio delivery on x86.
> 
> Yeah, afaik, it can be done with lowest priority delivery mode on ia64
> too, so I have a hard time finding RH's intended purpose.
> 
>>                                                    Again, need to have some more
>> PCI devices to test against to confirm anything.
> 
> It's impossible to test everything, and there is no conflict if we have
> at most one data point ;)
> 

Very true :)

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

[Radim Krčmář]

2015-03-23 16:46-0600, James Sullivan:
> On 03/23/2015 03:13 PM, Radim Krčmář wrote:
> > I meant if the delivery mode from data register isn't ignored with RH=1,
> > and the message delivered as if lowest-priority was set there.
> > (Decided by having something else than fixed or lowest-priority there.)
> > 
> 
> Hmm, any thoughts on how I could test for that?

Set the MSI data register's delivery mode to NMI/SMI/...
The change below fails => hardware honors delivery mode.

I tested it and Linux got a lot of unexpected NMIs, so the emulation in
your latest patch looks correct.

diff --git a/arch/x86/include/asm/msidef.h b/arch/x86/include/asm/msidef.h
index 4cc48af23fef..2270e459186b 100644
--- a/arch/x86/include/asm/msidef.h
+++ b/arch/x86/include/asm/msidef.h
@@ -17,6 +17,7 @@
 #define MSI_DATA_DELIVERY_MODE_SHIFT	8
 #define  MSI_DATA_DELIVERY_FIXED	(0 << MSI_DATA_DELIVERY_MODE_SHIFT)
 #define  MSI_DATA_DELIVERY_LOWPRI	(1 << MSI_DATA_DELIVERY_MODE_SHIFT)
+#define  MSI_DATA_DELIVERY_NMI		(4 << MSI_DATA_DELIVERY_MODE_SHIFT)
 
 #define MSI_DATA_LEVEL_SHIFT		14
 #define	 MSI_DATA_LEVEL_DEASSERT	(0 << MSI_DATA_LEVEL_SHIFT)
diff --git a/arch/x86/kernel/apic/msi.c b/arch/x86/kernel/apic/msi.c
index d6ba2d660dc5..4f71737c34eb 100644
--- a/arch/x86/kernel/apic/msi.c
+++ b/arch/x86/kernel/apic/msi.c
@@ -46,7 +46,7 @@ void native_compose_msi_msg(struct pci_dev *pdev,
 		MSI_DATA_LEVEL_ASSERT |
 		((apic->irq_delivery_mode != dest_LowestPrio) ?
 			MSI_DATA_DELIVERY_FIXED :
-			MSI_DATA_DELIVERY_LOWPRI) |
+			MSI_DATA_DELIVERY_NMI) |
 		MSI_DATA_VECTOR(cfg->vector);
 }
 

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

[James Sullivan]

On 03/24/2015 08:03 AM, Radim Krčmář wrote:
> 2015-03-23 16:46-0600, James Sullivan:
>> On 03/23/2015 03:13 PM, Radim Krčmář wrote:
>>> I meant if the delivery mode from data register isn't ignored with RH=1,
>>> and the message delivered as if lowest-priority was set there.
>>> (Decided by having something else than fixed or lowest-priority there.)
>>>
>>
>> Hmm, any thoughts on how I could test for that?
> 
> Set the MSI data register's delivery mode to NMI/SMI/...
> The change below fails => hardware honors delivery mode.
> 
> I tested it and Linux got a lot of unexpected NMIs, so the emulation in
> your latest patch looks correct.
> 
> diff --git a/arch/x86/include/asm/msidef.h b/arch/x86/include/asm/msidef.h
> index 4cc48af23fef..2270e459186b 100644
> --- a/arch/x86/include/asm/msidef.h
> +++ b/arch/x86/include/asm/msidef.h
> @@ -17,6 +17,7 @@
>  #define MSI_DATA_DELIVERY_MODE_SHIFT	8
>  #define  MSI_DATA_DELIVERY_FIXED	(0 << MSI_DATA_DELIVERY_MODE_SHIFT)
>  #define  MSI_DATA_DELIVERY_LOWPRI	(1 << MSI_DATA_DELIVERY_MODE_SHIFT)
> +#define  MSI_DATA_DELIVERY_NMI		(4 << MSI_DATA_DELIVERY_MODE_SHIFT)
>  
>  #define MSI_DATA_LEVEL_SHIFT		14
>  #define	 MSI_DATA_LEVEL_DEASSERT	(0 << MSI_DATA_LEVEL_SHIFT)
> diff --git a/arch/x86/kernel/apic/msi.c b/arch/x86/kernel/apic/msi.c
> index d6ba2d660dc5..4f71737c34eb 100644
> --- a/arch/x86/kernel/apic/msi.c
> +++ b/arch/x86/kernel/apic/msi.c
> @@ -46,7 +46,7 @@ void native_compose_msi_msg(struct pci_dev *pdev,
>  		MSI_DATA_LEVEL_ASSERT |
>  		((apic->irq_delivery_mode != dest_LowestPrio) ?
>  			MSI_DATA_DELIVERY_FIXED :
> -			MSI_DATA_DELIVERY_LOWPRI) |
> +			MSI_DATA_DELIVERY_NMI) |
>  		MSI_DATA_VECTOR(cfg->vector);
>  }
>  
> 

Great, thanks.

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

[James Sullivan]

On 03/24/2015 08:03 AM, Radim Krčmář wrote:
> 2015-03-23 16:46-0600, James Sullivan:
>> On 03/23/2015 03:13 PM, Radim Krčmář wrote:
>>> I meant if the delivery mode from data register isn't ignored with RH=1,
>>> and the message delivered as if lowest-priority was set there.
>>> (Decided by having something else than fixed or lowest-priority there.)
>>>
>>
>> Hmm, any thoughts on how I could test for that?
> 
> Set the MSI data register's delivery mode to NMI/SMI/...
> The change below fails => hardware honors delivery mode.
> 
> I tested it and Linux got a lot of unexpected NMIs, so the emulation in
> your latest patch looks correct.
> 
> diff --git a/arch/x86/include/asm/msidef.h b/arch/x86/include/asm/msidef.h
> index 4cc48af23fef..2270e459186b 100644
> --- a/arch/x86/include/asm/msidef.h
> +++ b/arch/x86/include/asm/msidef.h
> @@ -17,6 +17,7 @@
>  #define MSI_DATA_DELIVERY_MODE_SHIFT	8
>  #define  MSI_DATA_DELIVERY_FIXED	(0 << MSI_DATA_DELIVERY_MODE_SHIFT)
>  #define  MSI_DATA_DELIVERY_LOWPRI	(1 << MSI_DATA_DELIVERY_MODE_SHIFT)
> +#define  MSI_DATA_DELIVERY_NMI		(4 << MSI_DATA_DELIVERY_MODE_SHIFT)
>  
>  #define MSI_DATA_LEVEL_SHIFT		14
>  #define	 MSI_DATA_LEVEL_DEASSERT	(0 << MSI_DATA_LEVEL_SHIFT)
> diff --git a/arch/x86/kernel/apic/msi.c b/arch/x86/kernel/apic/msi.c
> index d6ba2d660dc5..4f71737c34eb 100644
> --- a/arch/x86/kernel/apic/msi.c
> +++ b/arch/x86/kernel/apic/msi.c
> @@ -46,7 +46,7 @@ void native_compose_msi_msg(struct pci_dev *pdev,
>  		MSI_DATA_LEVEL_ASSERT |
>  		((apic->irq_delivery_mode != dest_LowestPrio) ?
>  			MSI_DATA_DELIVERY_FIXED :
> -			MSI_DATA_DELIVERY_LOWPRI) |
> +			MSI_DATA_DELIVERY_NMI) |
>  		MSI_DATA_VECTOR(cfg->vector);
>  }
>  
> 

Good to hear, thanks for testing that.

Any other tweaks you think are necessary for the patch set?

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

[Radim Krčmář]

2015-04-02 16:08-0600, James Sullivan:
> On 03/24/2015 08:03 AM, Radim Krčmář wrote:
> > 2015-03-23 16:46-0600, James Sullivan:
> >> On 03/23/2015 03:13 PM, Radim Krčmář wrote:
> > I tested it and Linux got a lot of unexpected NMIs, so the emulation in
> > your latest patch looks correct.
> 
> Good to hear, thanks for testing that.
> 
> Any other tweaks you think are necessary for the patch set?

No, I thought it fell off my list because I reviewed it; will do so now.

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

[Paolo Bonzini]

On 19/03/2015 23:51, James Sullivan wrote:
> I played around with native_compose_msi_msg and discovered the following:
> 
> * dm=0, rh=0 => Physical Destination Mode
> * dm=0, rh=1 => Failed delivery
> * dm=1, rh=0 => Logical Destination Mode, No Redirection
> * dm=1, rh=1 => Logical Destination Mode, Redirection
> 
> So it seems to be the case that logical destination mode is used whenever
> DM=1, regardless of RH. Furthermore, the case where DM=0 and RH=1 is
> undefined, as was indicated in the closing response to the thread in
> https://software.intel.com/en-us/forums/topic/288883 :

So this means this patch is not necessary, right?

Paolo

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

[Radim Krčmář]

2015-04-21 14:18+0200, Paolo Bonzini:
> On 19/03/2015 23:51, James Sullivan wrote:
> > I played around with native_compose_msi_msg and discovered the following:
> > 
> > * dm=0, rh=0 => Physical Destination Mode
> > * dm=0, rh=1 => Failed delivery
> > * dm=1, rh=0 => Logical Destination Mode, No Redirection
> > * dm=1, rh=1 => Logical Destination Mode, Redirection
> > 
> > So it seems to be the case that logical destination mode is used whenever
> > DM=1, regardless of RH. Furthermore, the case where DM=0 and RH=1 is
> > undefined, as was indicated in the closing response to the thread in
> > https://software.intel.com/en-us/forums/topic/288883 :
> 
> So this means this patch is not necessary, right?

Yes, and this patch was obsolete even before;  the latest version is
  [PATCH v4 0/2] kvm: x86: Implement handling of RH=1 for MSI delivery in KVM

which delivers DM=1+RH=1 as lowest priority.
(Several unfortunate things made this quite confusing ...)

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

[Marcelo Tosatti]

On Wed, Mar 18, 2015 at 06:59:22PM -0600, James Sullivan wrote:
> > 
> > The documentation states the following:
> > 
> > * When RH is 0, the interrupt is directed to the processor listed in the
> > Destination ID field.
> > 
> > * If RH is 0, then the DM bit is ignored and the message is sent ahead
> > independent of whether the physical or logical destination mode is used.
> > 
> > However, from the POV of a device writing to memory to generate an MSI 
> > interrupt, there is no (or i can't see any) other information that 
> > can be used to infer logical or physical mode for the interrupt message.
> > 
> > Before your patch:
> > 
> > (dm, rh) = (0, 0) => irq->dest_mode = 0
> > (dm, rh) = (0, 1) => irq->dest_mode = 0
> > (dm, rh) = (1, 0) => irq->dest_mode = 1
> > (dm, rh) = (1, 1) => irq->dest_mode = 1
> > 
> > After your patch:
> > 
> > (dm, rh) = (0, 0) => irq->dest_mode = 0
> > (dm, rh) = (0, 1) => irq->dest_mode = 0
> > (dm, rh) = (1, 0) => irq->dest_mode = 0
> > (dm, rh) = (1, 1) => irq->dest_mode = 1
> > 
> > 
> > Am i missing some explicit documentation that refers 
> > to (dm, rh) = (1, 0) => irq->dest_mode = 0 ?
> 
> >From the IA32 manual (Vol. 3, 10.11.2):
> 
>  * When RH is 0, the interrupt is directed to the processor listed
>    in the Destination ID field.
>  * When RH is 1 and the physical destination mode is used, the Destination
>    ID field must not be set to FFH; it must point to a processor that is
>    present and enabled to receive the interrupt.
>  * When RH is 1 and the logical destination mode is active in a system using
>    a flat addressing model, the Destination ID field must be set so that bits
>    set to 1 identify processors that are present and enabled to receive the
>    interrupt.
>  * If RH is set to 1 and the logical destination mode is active in a system
>    using cluster addressing model, then Destination ID field must not be
>    set to FFH; the processors identified with this field must be present
>    and enabled to receive the interrupt.
> 
> My interpretation of this is that RH=0 indicates that the Dest. ID field
> contains an APIC ID, and as such destination mode is physical. When RH=1,
> depending on the value of DM, we either use physical or logical dest mode.
> The result of this is that logical dest mode is set just when RH=1/DM=1,
> as far as I understand.
> 
> > 
> > See native_compose_msi_msg:
> > 
> >         msg->address_lo =
> >                 MSI_ADDR_BASE_LO |
> >                 ((apic->irq_dest_mode == 0) ?
> >                         MSI_ADDR_DEST_MODE_PHYSICAL :
> >                         MSI_ADDR_DEST_MODE_LOGICAL) |
> >                 ((apic->irq_delivery_mode != dest_LowestPrio) ?
> >                         MSI_ADDR_REDIRECTION_CPU :
> >                         MSI_ADDR_REDIRECTION_LOWPRI) |
> >                 MSI_ADDR_DEST_ID(dest);
> > 
> > 
> > So it does configure DM = MSI_ADDR_DEST_MODE_LOGICAL
> > and RH = MSI_ADDR_REDIRECTION_LOWPRI.
> > 
> 
> ...and yet this is a good counterexample against my argument :)
> 
> What I think I'll do is revert this particular change so that dest_mode is
> set independently of RH. While I'm not entirely convinced that this is the
> intended interpretation, 

Where would the logical/physical information come from, if not from the
DM bit ?

> I do think that consistency with the existing logic
> is probably desirable for the time being. If I can get closure on the matter
> I'll re-submit that change, but for the time being I will undo it.
> 
> -James