Re: [PATCH v8 5/7] VT-d: No need to set irq affinity for posted format IRTE

[Chao Gao <chao.gao@intel.com>]

On Tue, Nov 22, 2016 at 06:03:51PM +0800, Jan Beulich wrote:
>>>> On 18.11.16 at 02:58, <feng.wu@intel.com> wrote:
>> --- a/xen/drivers/passthrough/vtd/intremap.c
>> +++ b/xen/drivers/passthrough/vtd/intremap.c
>> @@ -600,27 +600,41 @@ static int msi_msg_to_remap_entry(
>>
>>      if ( !pi_desc )
>>      {
>> -        /* 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;
>> +        /*
>> +         * We are here because we are trying to update the IRTE to remapped mode,
>> +         * we only need to update the remapped specific fields for the following
>> +         * two cases:
>> +         * 1. When we create a new IRTE. A new IRTE is created when we create a
>> +         *    new irq, so a new IRTE is always initialized with remapped format.
>> +         * 2. When the old IRTE is present and in remapped mode. Since if  the old
>> +         *    IRTE is in posted mode, we cannot update it to remapped mode and
>> +         *    this is what we need to suppress. So we don't update the remapped
>> +         *    specific fields here, we only update the commom field.
>> +         */
>> +        if ( !iremap_entry->remap.p || !iremap_entry->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 */
>> +            new_ire.remap.res_3 = 0;
>> +            new_ire.remap.res_4 = 0;
>> +            new_ire.remap.p = 1;    /* finally, set present bit */
>> +        }
>
>I disagree with this entire change, including namely point 2 of the
>comment. There should be no special casing of either transition
>here - the caller should provide you with input correctly identifying
>which of the two formats is to be generated. This certainly is
>connected to one of the comments I've just made on patch 4.
>

If we reach an agreement on patch 4, the logic will be more general and the problem 
will disappear.

>> @@ -657,25 +671,28 @@ static int msi_msg_to_remap_entry(
>>      remap_rte->address_hi = 0;
>>      remap_rte->data = index - i;
>>
>> -    if ( !pi_desc )
>> -        memcpy(iremap_entry, &new_ire, sizeof(struct iremap_entry));
>> -    else
>> +    if ( iremap_entry->val != new_ire.val )
>>      {
>> -        __uint128_t ret;
>> +        if ( !pi_desc )
>> +            memcpy(iremap_entry, &new_ire, sizeof(struct iremap_entry));
>> +        else
>> +        {
>> +            __uint128_t ret;
>>
>> -        old_ire = *iremap_entry;
>> -        ret = cmpxchg16b(iremap_entry, &old_ire, &new_ire);
>> +            old_ire = *iremap_entry;
>> +            ret = cmpxchg16b(iremap_entry, &old_ire, &new_ire);
>>
>> -        /*
>> -         * In the above, we use cmpxchg16 to atomically update the 128-bit IRTE,
>> -         * and the hardware cannot update the IRTE behind us, so the return value
>> -         * of cmpxchg16 should be the same as old_ire. This ASSERT validate it.
>> -         */
>> -        ASSERT(ret == old_ire.val);
>> -    }
>> +            /*
>> +             * In the above, we use cmpxchg16 to atomically update the 128-bit IRTE,
>> +             * and the hardware cannot update the IRTE behind us, so the return value
>> +             * of cmpxchg16 should be the same as old_ire. This ASSERT validate it.
>> +             */
>> +            ASSERT(ret == old_ire.val);
>> +        }
>>
>> -    iommu_flush_cache_entry(iremap_entry, sizeof(struct iremap_entry));
>> -    iommu_flush_iec_index(iommu, 0, index);
>> +        iommu_flush_cache_entry(iremap_entry, sizeof(struct iremap_entry));
>> +        iommu_flush_iec_index(iommu, 0, index);
>> +    }
>>
>>      unmap_vtd_domain_page(iremap_entries);
>>      spin_unlock_irqrestore(&ir_ctrl->iremap_lock, flags);
>
>This second hunk is imo all this patch should consist of.
>
>Jan
>
>_______________________________________________
>Xen-devel mailing list
>Xen-devel@lists.xen.org
>https://lists.xen.org/xen-devel

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