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

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

>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