Re: [Xen-devel] [PATCH 2/3] VT-d: correct dma_msi_set_affinity()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 08/12/16 16:01, Jan Beulich wrote:
> That commit ("VT-d: use msi_compose_msg()) together with 15aa6c6748

Which commit?

> ("amd iommu: use base platform MSI implementation") introducing the use
> of a per-CPU scratch CPU mask went too far: dma_msi_set_affinity() may,
> at least in theory, be called in interrupt context, and hence the use
> of that scratch variable is not correct.
>
> Since the function overwrites the destination information anyway,
> allow msi_compose_msg() to be called with a NULL CPU mask, avoiding the
> use of that scratch variable.

Which function overwrites what?  I can't see dma_msi_set_affinity()
doing anything to clobber msg.dest32, so I don't understand why this
change is correct.

I can see why the current behaviour is unsafe, and agree that it should
change.

~Andrew

>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> --- a/xen/arch/x86/msi.c
> +++ b/xen/arch/x86/msi.c
> @@ -160,39 +160,37 @@ static bool_t msix_memory_decoded(const
>   */
>  void msi_compose_msg(unsigned vector, const cpumask_t *cpu_mask, struct 
> msi_msg *msg)
>  {
> -    unsigned dest;
> -
>      memset(msg, 0, sizeof(*msg));
> -    if ( !cpumask_intersects(cpu_mask, &cpu_online_map) )
> +
> +    if ( vector < FIRST_DYNAMIC_VECTOR )
>          return;
>  
> -    if ( vector )
> +    if ( cpu_mask )
>      {
>          cpumask_t *mask = this_cpu(scratch_mask);
>  
> -        cpumask_and(mask, cpu_mask, &cpu_online_map);
> -        dest = cpu_mask_to_apicid(mask);
> +        if ( !cpumask_intersects(cpu_mask, &cpu_online_map) )
> +            return;
>  
> -        msg->address_hi = MSI_ADDR_BASE_HI;
> -        msg->address_lo =
> -            MSI_ADDR_BASE_LO |
> -            ((INT_DEST_MODE == 0) ?
> -             MSI_ADDR_DESTMODE_PHYS:
> -             MSI_ADDR_DESTMODE_LOGIC) |
> -            ((INT_DELIVERY_MODE != dest_LowestPrio) ?
> -             MSI_ADDR_REDIRECTION_CPU:
> -             MSI_ADDR_REDIRECTION_LOWPRI) |
> -            MSI_ADDR_DEST_ID(dest);
> -        msg->dest32 = dest;
> -
> -        msg->data =
> -            MSI_DATA_TRIGGER_EDGE |
> -            MSI_DATA_LEVEL_ASSERT |
> -            ((INT_DELIVERY_MODE != dest_LowestPrio) ?
> -             MSI_DATA_DELIVERY_FIXED:
> -             MSI_DATA_DELIVERY_LOWPRI) |
> -            MSI_DATA_VECTOR(vector);
> +        cpumask_and(mask, cpu_mask, &cpu_online_map);
> +        msg->dest32 = cpu_mask_to_apicid(mask);
>      }
> +
> +    msg->address_hi = MSI_ADDR_BASE_HI;
> +    msg->address_lo = MSI_ADDR_BASE_LO |
> +                      (INT_DEST_MODE ? MSI_ADDR_DESTMODE_LOGIC
> +                                     : MSI_ADDR_DESTMODE_PHYS) |
> +                      ((INT_DELIVERY_MODE != dest_LowestPrio)
> +                       ? MSI_ADDR_REDIRECTION_CPU
> +                       : MSI_ADDR_REDIRECTION_LOWPRI) |
> +                      MSI_ADDR_DEST_ID(msg->dest32);
> +
> +    msg->data = MSI_DATA_TRIGGER_EDGE |
> +                MSI_DATA_LEVEL_ASSERT |
> +                ((INT_DELIVERY_MODE != dest_LowestPrio)
> +                 ? MSI_DATA_DELIVERY_FIXED
> +                 : MSI_DATA_DELIVERY_LOWPRI) |
> +                MSI_DATA_VECTOR(vector);
>  }
>  
>  static bool_t read_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1085,11 +1085,10 @@ static void dma_msi_set_affinity(struct
>          return;
>      }
>  
> -    msi_compose_msg(desc->arch.vector, desc->arch.cpu_mask, &msg);
> -    /* Are these overrides really needed? */
> +    msi_compose_msg(desc->arch.vector, NULL, &msg);
>      if (x2apic_enabled)
>          msg.address_hi = dest & 0xFFFFFF00;
> -    msg.address_lo &= ~MSI_ADDR_DEST_ID_MASK;
> +    ASSERT(!(msg.address_lo & MSI_ADDR_DEST_ID_MASK));
>      msg.address_lo |= MSI_ADDR_DEST_ID(dest);
>      iommu->msi.msg = msg;
>  
>
>
>


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