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

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Wu, Feng
> Sent: Friday, November 18, 2016 9:59 AM
> 
> We don't set the affinity for posted format IRTE, since the
> destination of these interrupts is vCPU and the vCPU affinity
> is set during vCPU scheduling.
> 
> Signed-off-by: Feng Wu <feng.wu@xxxxxxxxx>
> ---
> v8:
> - Changes based on [6/7]

[5/7]?

> 
> v7:
> - Compare all the field in IRTE to justify whether we can suppress the update
> 
> v6:
> - Make pi_can_suppress_irte_update() a check-only function
> - Introduce another function pi_get_new_irte() to update the 'new_ire' if 
> needed
> 
> v5:
> - Only suppress affinity related IRTE updates for PI
> 
> v4:
> - Keep the construction of new_ire and only modify the hardware
> IRTE when it is not in posted mode.
> 
>  xen/drivers/passthrough/vtd/intremap.c | 87
> ++++++++++++++++++++--------------
>  1 file changed, 52 insertions(+), 35 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/vtd/intremap.c
> b/xen/drivers/passthrough/vtd/intremap.c
> index fd2a49a..0cb8c37 100644
> --- 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:

you said "we only need to update the remapped specific fields", while later...

> +         * 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.

you said "we don't update remapped specific fields"...

It's also unclear to me why we cannot change irte from posted mode back to
remapped mode. Is it defined as a VT-d arch limitation? What about the other
direction from remapped mode to posted mode?

> +         */
> +        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 */
> +        }

what about old_ire is in posted mode? If it cannot happen from posted
to remap as you explained earlier, then should be an ASSERT here.
Otherwise you just leave a condition unhandled...

>      }
>      else
>      {
> @@ -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);
> --
> 2.1.0


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