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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.09.16 at 04:37, <feng.wu@xxxxxxxxx> wrote:
> 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.

I'm quite sure I did point out before that you talk about just affinity
changes here, yet ...

> --- a/xen/drivers/passthrough/vtd/intremap.c
> +++ b/xen/drivers/passthrough/vtd/intremap.c
> @@ -637,9 +637,12 @@ static int msi_msg_to_remap_entry(
>      remap_rte->address_hi = 0;
>      remap_rte->data = index - i;
>  
> -    memcpy(iremap_entry, &new_ire, sizeof(struct iremap_entry));
> -    iommu_flush_cache_entry(iremap_entry, sizeof(struct iremap_entry));
> -    iommu_flush_iec_index(iommu, 0, index);
> +    if ( !iremap_entry->remap.p || !iremap_entry->remap.im )
> +    {
> +        memcpy(iremap_entry, &new_ire, sizeof(struct iremap_entry));
> +        iommu_flush_cache_entry(iremap_entry, sizeof(struct iremap_entry));
> +        iommu_flush_iec_index(iommu, 0, index);
> +    }

... you suppress the update also in other cases. This _may_ be safe
at present, but you're digging a hole for someone else to fall into
down the road. Hence at the very least you should, in a to be added
"else" path, ASSERT() that nothing in the descriptor changed except
the bits representing affinity. Even better would be if in fact you
suppressed the update+flush only when nothing other than dst
changed.

Also, since you already touch this, please consider switching from the
type-unsafe memcpy() to type-safe structure assignment. And please
in any event change the sizeof()-s to sizeof(*iremap_entry).

Jan


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