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

[Marcelo Tosatti <mtosatti@redhat.com>]

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?