Re: [Xen-devel] [PATCH v7 4/6] VT-d: No need to set irq affinity for posted format IRTE

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.11.16 at 07:28, <feng.wu@xxxxxxxxx> wrote:
>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> Sent: Tuesday, November 8, 2016 1:00 AM
>> >>> On 07.11.16 at 09:10, <feng.wu@xxxxxxxxx> wrote:
>> > --- a/xen/drivers/passthrough/vtd/intremap.c
>> > +++ b/xen/drivers/passthrough/vtd/intremap.c
>> > @@ -597,31 +597,34 @@ static int msi_msg_to_remap_entry(
>> >
>> >      memcpy(&new_ire, iremap_entry, sizeof(struct iremap_entry));
>> >
>> > -    /* Set interrupt remapping table entry */
>> > -    new_ire.remap.fpd = 0;
>> > -    new_ire.remap.dm = (msg->address_lo >> MSI_ADDR_DESTMODE_SHIFT)
>> & 0x1;
>> > -    new_ire.remap.tm = (msg->data >> MSI_DATA_TRIGGER_SHIFT) & 0x1;
>> > -    new_ire.remap.dlm = (msg->data >> MSI_DATA_DELIVERY_MODE_SHIFT) &
>> 0x1;
>> > -    /* Hardware require RH = 1 for LPR delivery mode */
>> > -    new_ire.remap.rh = (new_ire.remap.dlm == dest_LowestPrio);
>> > -    new_ire.remap.avail = 0;
>> > -    new_ire.remap.res_1 = 0;
>> > -    new_ire.remap.vector = (msg->data >> MSI_DATA_VECTOR_SHIFT) &
>> > -                            MSI_DATA_VECTOR_MASK;
>> > -    new_ire.remap.res_2 = 0;
>> > -    if ( x2apic_enabled )
>> > -        new_ire.remap.dst = msg->dest32;
>> > -    else
>> > -        new_ire.remap.dst = ((msg->address_lo >> MSI_ADDR_DEST_ID_SHIFT)
>> > -                             & 0xff) << 8;
>> > -
>> >      if ( pdev )
>> >          set_msi_source_id(pdev, &new_ire);
>> >      else
>> >          set_hpet_source_id(msi_desc->hpet_id, &new_ire);
>> > -    new_ire.remap.res_3 = 0;
>> > -    new_ire.remap.res_4 = 0;
>> > -    new_ire.remap.p = 1;    /* finally, set present bit */
>> > +
>> > +    if ( !new_ire.remap.p || !new_ire.remap.im )
>> > +    {
>> > +        /* Set interrupt remapping table entry */
>> > +        new_ire.remap.fpd = 0;
>> > +        new_ire.remap.dm = (msg->address_lo >> MSI_ADDR_DESTMODE_SHIFT)
>> & 0x1;
>> > +        new_ire.remap.tm = (msg->data >> MSI_DATA_TRIGGER_SHIFT) & 0x1;
>> > +        new_ire.remap.dlm = (msg->data >> MSI_DATA_DELIVERY_MODE_SHIFT)
>> & 0x1;
>> > +        /* Hardware require RH = 1 for LPR delivery mode */
>> > +        new_ire.remap.rh = (new_ire.remap.dlm == dest_LowestPrio);
>> > +        new_ire.remap.avail = 0;
>> > +        new_ire.remap.res_1 = 0;
>> > +        new_ire.remap.vector = (msg->data >> MSI_DATA_VECTOR_SHIFT) &
>> > +                                MSI_DATA_VECTOR_MASK;
>> > +        new_ire.remap.res_2 = 0;
>> > +        if ( x2apic_enabled )
>> > +            new_ire.remap.dst = msg->dest32;
>> > +        else
>> > +            new_ire.remap.dst = ((msg->address_lo >> 
>> > MSI_ADDR_DEST_ID_SHIFT)
>> > +                                 & 0xff) << 8;
>> > +        new_ire.remap.res_3 = 0;
>> > +        new_ire.remap.res_4 = 0;
>> > +        new_ire.remap.p = 1;    /* finally, set present bit */
>> > +    }
>> 
>> Why does setting the fields depend on the previous state of p and
>> im? 
> 
> Okay, here are the questions. How do you think we should set the
> 'new_ire'? how to decide to set it to remapped mode or posted mode?
> 
> Here, I set 'new_ire' based on the following policy:
> 1. if previous p is 0, which means we initial a new IRTE, then we can
> only set it to remapped mode. We never set a new IRTE to posted mode.

That's because ...? (And the answer would really belong in a code
comment, if that conditional is to stay.)

> 2. if previous p is 1 and it is in remapped mode, we can only set it to
> remapped mode in _this_ function, setting it to posted mode is in
> another function: pi_update_irte().

Which may be part of the problem: Why are there two functions?

> 3. The common field are set for both format, which is covered by
> set_msi_source_id()/set_hpet_source_id()

Yes, and that's fine.

>> And where's the else to deal with the posted format case? 
> 
> We don't need the else part, 'new_ire' is gotten from old 'iremap_entry',
> and if 'iremap_entry' is in posted mode, we don't need to modify the
> posted related bit in 'new_ire' and we only need to update the common
> field which is done by set_msi_source_id()/set_hpet_source_id() above.

As said, updating the common fields is fine, but I did ask before
why updating the posted mode specific fields is unnecessary here.
Once again - the function ought to be doing what its name says,
without making (undocumented / un-ASSERT()ed-for) assumptions
on caller behavior.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel