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

[Marcelo Tosatti <mtosatti@redhat.com>]

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.