Re: [PATCH v2 1/2] Intel-IOMMU, intr-remap: set the whole 128bits of irte when modify/free it

[Ingo Molnar <mingo@elte.hu>]

* Weidong Han <weidong.han@intel.com> wrote:

> Interrupt remapping table entry is 128bits. Currently, it only sets low
> 64bits of irte in modify_irte and free_irte. This ignores high 64bits
> setting of irte, that means source-id setting will be ignored. This patch
> sets the whole 128bits of irte when modify/free it. Following source-id
> checking patch depends on this.
> 
> Signed-off-by: Weidong Han <weidong.han@intel.com>
> ---
>  drivers/pci/intr_remapping.c |   10 +++++++---
>  1 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/intr_remapping.c b/drivers/pci/intr_remapping.c
> index f5e0ea7..946e170 100644
> --- a/drivers/pci/intr_remapping.c
> +++ b/drivers/pci/intr_remapping.c
> @@ -309,7 +309,8 @@ int modify_irte(int irq, struct irte *irte_modified)
>  	index = irq_iommu->irte_index + irq_iommu->sub_handle;
>  	irte = &iommu->ir_table->base[index];
>  
> -	set_64bit((unsigned long *)irte, irte_modified->low);
> +	set_64bit((unsigned long *)&irte->low, irte_modified->low);
> +	set_64bit((unsigned long *)&irte->high, irte_modified->high);
>
>  	__iommu_flush_cache(iommu, irte, sizeof(*irte));
>  
>  	rc = qi_flush_iec(iommu, index, 0);
> @@ -386,8 +387,11 @@ int free_irte(int irq)
>  	irte = &iommu->ir_table->base[index];
>  
>  	if (!irq_iommu->sub_handle) {
> -		for (i = 0; i < (1 << irq_iommu->irte_mask); i++)
> -			set_64bit((unsigned long *)(irte + i), 0);
> +		for (i = 0; i < (1 << irq_iommu->irte_mask); i++) {
> +			set_64bit((unsigned long *)&irte->low, 0);
> +			set_64bit((unsigned long *)&irte->high, 0);
> +			irte++;
> +		}

The loop is a bit unclean. It has a side-effect on 'irte' - and 
other patterns in the driver usually treat 'irte' as a generally 
available variable.

So the above code, while correct, opens up the possibility of later 
code added to this function relying on 'irte', thinking that it's 
set to "&iommu->ir_table->base[index]", and then breaking because 
'irte' has been iterated to the end of it in certain circumstances.

It's better to factor out the whole loop into a helper function, 
which does something like:

int flush_entries(struct irq_2_iommu *irq_iommu)
{
	struct irte *start, *entry, *end;
	struct intel_iommu *iommu;
	int index;

 	if (irq_iommu->sub_handle)
		return 0;

	iommu = irq_iommu->iommu;
	index = irq_iommu->irte_index + irq_iommu->sub_handle;

	start = iommu->ir_table->base + index;
	end = start + (1 << irq_iommu->irte_mask);

	for (entry = start; entry < end; entry++) {
		set_64bit((unsigned long *)&entry->low,  0);
		set_64bit((unsigned long *)&entry->high, 0);
	}

	return qi_flush_iec(iommu, index, irq_iommu->irte_mask);
}

Note how clearer this is - the new method has one purpose and 
'entry' is a clear iterator.

( And note how much clearer the flow of 'rc' has become as well as a 
  side-effect: it is clear now that it's set to 0 when 
  irq_iommu->sub_handle is still present. )

Thanks,

	Ingo