[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



>>> On 18.11.16 at 02:58, <feng.wu@xxxxxxxxx> 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.

> @@ -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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.