[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 2/3] VT-d: correct dma_msi_set_affinity()
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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |