Re: [PATCH] iommu/amd: Add sanity check for interrupt remapping table length macros

[Jerry Snitselaar]

Suravee Suthikulpanit @ 2020-12-10 09:24 MST:

> Currently, macros related to the interrupt remapping table length are
> defined separately. This has resulted in an oversight in which one of
> the macros were missed when changing the length. To prevent this,
> redefine the macros to add built-in sanity check.
>
> Also, rename macros to use the name of the DTE[IntTabLen] field as
> specified in the AMD IOMMU specification. There is no functional change.
>
> Suggested-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> Reviewed-by: Tom Lendacky <thomas.lendacky@xxxxxxx>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
> Cc: Will Deacon <will@xxxxxxxxxx>
> Cc: Jerry Snitselaar <jsnitsel@xxxxxxxxxx>
> Cc: Joerg Roedel <joro@xxxxxxxxxx>
> ---
>  drivers/iommu/amd/amd_iommu_types.h | 19 ++++++++++---------
>  drivers/iommu/amd/init.c            |  6 +++---
>  drivers/iommu/amd/iommu.c           |  2 +-
>  3 files changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
> index 494b42a31b7a..899ce62df3f0 100644
> --- a/drivers/iommu/amd/amd_iommu_types.h
> +++ b/drivers/iommu/amd/amd_iommu_types.h
> @@ -255,11 +255,19 @@
>  /* Bit value definition for dte irq remapping fields*/
>  #define DTE_IRQ_PHYS_ADDR_MASK	(((1ULL << 45)-1) << 6)
>  #define DTE_IRQ_REMAP_INTCTL_MASK	(0x3ULL << 60)
> -#define DTE_IRQ_TABLE_LEN_MASK	(0xfULL << 1)
>  #define DTE_IRQ_REMAP_INTCTL    (2ULL << 60)
> -#define DTE_IRQ_TABLE_LEN       (9ULL << 1)
>  #define DTE_IRQ_REMAP_ENABLE    1ULL
>  
> +/*
> + * AMD IOMMU hardware only support 512 IRTEs despite
> + * the architectural limitation of 2048 entries.
> + */
> +#define DTE_INTTAB_ALIGNMENT    128
> +#define DTE_INTTABLEN_VALUE     9ULL
> +#define DTE_INTTABLEN           (DTE_INTTABLEN_VALUE << 1)
> +#define DTE_INTTABLEN_MASK      (0xfULL << 1)
> +#define MAX_IRQS_PER_TABLE      (1 << DTE_INTTABLEN_VALUE)
> +
>  #define PAGE_MODE_NONE    0x00
>  #define PAGE_MODE_1_LEVEL 0x01
>  #define PAGE_MODE_2_LEVEL 0x02
> @@ -409,13 +417,6 @@ extern bool amd_iommu_np_cache;
>  /* Only true if all IOMMUs support device IOTLBs */
>  extern bool amd_iommu_iotlb_sup;
>  
> -/*
> - * AMD IOMMU hardware only support 512 IRTEs despite
> - * the architectural limitation of 2048 entries.
> - */
> -#define MAX_IRQS_PER_TABLE	512
> -#define IRQ_TABLE_ALIGNMENT	128
> -
>  struct irq_remap_table {
>  	raw_spinlock_t lock;
>  	unsigned min_index;
> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> index 23a790f8f550..6bec8913d064 100644
> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c
> @@ -989,10 +989,10 @@ static bool copy_device_table(void)
>  
>  		irq_v = old_devtb[devid].data[2] & DTE_IRQ_REMAP_ENABLE;
>  		int_ctl = old_devtb[devid].data[2] & DTE_IRQ_REMAP_INTCTL_MASK;
> -		int_tab_len = old_devtb[devid].data[2] & DTE_IRQ_TABLE_LEN_MASK;
> +		int_tab_len = old_devtb[devid].data[2] & DTE_INTTABLEN_MASK;
>  		if (irq_v && (int_ctl || int_tab_len)) {
>  			if ((int_ctl != DTE_IRQ_REMAP_INTCTL) ||
> -			    (int_tab_len != DTE_IRQ_TABLE_LEN)) {
> +			    (int_tab_len != DTE_INTTABLEN)) {
>  				pr_err("Wrong old irq remapping flag: %#x\n", devid);
>  				return false;
>  			}
> @@ -2674,7 +2674,7 @@ static int __init early_amd_iommu_init(void)
>  			remap_cache_sz = MAX_IRQS_PER_TABLE * (sizeof(u64) * 2);
>  		amd_iommu_irq_cache = kmem_cache_create("irq_remap_cache",
>  							remap_cache_sz,
> -							IRQ_TABLE_ALIGNMENT,
> +							DTE_INTTAB_ALIGNMENT,
>  							0, NULL);
>  		if (!amd_iommu_irq_cache)
>  			goto out;
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index b9cf59443843..f7abf16d1e3a 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -3191,7 +3191,7 @@ static void set_dte_irq_entry(u16 devid, struct irq_remap_table *table)
>  	dte	&= ~DTE_IRQ_PHYS_ADDR_MASK;
>  	dte	|= iommu_virt_to_phys(table->table);
>  	dte	|= DTE_IRQ_REMAP_INTCTL;
> -	dte	|= DTE_IRQ_TABLE_LEN;
> +	dte	|= DTE_INTTABLEN;
>  	dte	|= DTE_IRQ_REMAP_ENABLE;
>  
>  	amd_iommu_dev_table[devid].data[2] = dte;


Reviewed-by: Jerry Snitselaar <jsnitsel@xxxxxxxxxx>