Re: [PATCH 1/7] iommu/vt-d: Support size of the register set in DRHD

[Baolu Lu <baolu.lu@linux.intel.com>]

On 2023/1/12 4:24, kan.liang@linux.intel.com wrote:
> From: Kan Liang <kan.liang@linux.intel.com>
> 
> A new field, which indicates the size of the remapping hardware register
> set for this remapping unit, is introduced in the DMA-remapping hardware
> unit definition (DRHD) structure with the VT-d Spec 4.0.
> With the information, SW doesn't need to 'guess' the size of the
> register set anymore.
> 
> Update the struct acpi_dmar_hardware_unit to reflect the field.
> 
> Store the size of the register set in struct dmar_drhd_unit for each
> dmar device.
> 
> The 'size' information is ResvZ for the old BIOS and platforms. Fall
> back to the old guessing method. There is nothing changed.
> 
> Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
> ---
>   drivers/iommu/intel/dmar.c | 25 ++++++++++++++++++-------
>   include/acpi/actbl1.h      |  2 +-
>   include/linux/dmar.h       |  1 +
>   3 files changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iommu/intel/dmar.c b/drivers/iommu/intel/dmar.c
> index b00a0ceb2d13..6a411d964474 100644
> --- a/drivers/iommu/intel/dmar.c
> +++ b/drivers/iommu/intel/dmar.c
> @@ -399,6 +399,13 @@ dmar_find_dmaru(struct acpi_dmar_hardware_unit *drhd)
>   	return NULL;
>   }
>   
> +/* The size of the register set is 2 ^ N 4 KB pages. */
> +static unsigned long
> +dmar_get_drhd_reg_size(u8 npages)

No need to divide it into two lines. Or put this line of code directly
where it is called?

> +{
> +	return 1UL << (npages + 12);
> +}
> +
>   /*
>    * dmar_parse_one_drhd - parses exactly one DMA remapping hardware definition
>    * structure which uniquely represent one DMA remapping hardware unit
> @@ -427,6 +434,7 @@ static int dmar_parse_one_drhd(struct acpi_dmar_header *header, void *arg)
>   	memcpy(dmaru->hdr, header, header->length);
>   	dmaru->reg_base_addr = drhd->address;
>   	dmaru->segment = drhd->segment;
> +	dmaru->reg_size = dmar_get_drhd_reg_size(drhd->size);
>   	dmaru->include_all = drhd->flags & 0x1; /* BIT0: INCLUDE_ALL */
>   	dmaru->devices = dmar_alloc_dev_scope((void *)(drhd + 1),
>   					      ((void *)drhd) + drhd->header.length,
> @@ -880,6 +888,7 @@ dmar_validate_one_drhd(struct acpi_dmar_header *entry, void *arg)
>   	struct acpi_dmar_hardware_unit *drhd;
>   	void __iomem *addr;
>   	u64 cap, ecap;
> +	unsigned long size;
>   
>   	drhd = (void *)entry;
>   	if (!drhd->address) {
> @@ -887,10 +896,11 @@ dmar_validate_one_drhd(struct acpi_dmar_header *entry, void *arg)
>   		return -EINVAL;
>   	}
>   
> +	size = dmar_get_drhd_reg_size(drhd->size);
>   	if (arg)
> -		addr = ioremap(drhd->address, VTD_PAGE_SIZE);
> +		addr = ioremap(drhd->address, size);
>   	else
> -		addr = early_ioremap(drhd->address, VTD_PAGE_SIZE);
> +		addr = early_ioremap(drhd->address, size);
>   	if (!addr) {
>   		pr_warn("Can't validate DRHD address: %llx\n", drhd->address);
>   		return -EINVAL;
> @@ -902,7 +912,7 @@ dmar_validate_one_drhd(struct acpi_dmar_header *entry, void *arg)
>   	if (arg)
>   		iounmap(addr);
>   	else
> -		early_iounmap(addr, VTD_PAGE_SIZE);
> +		early_iounmap(addr, size);
>   
>   	if (cap == (uint64_t)-1 && ecap == (uint64_t)-1) {
>   		warn_invalid_dmar(drhd->address, " returns all ones");

The cap and ecap registers are always in the first page. Just map one
4K page is enough. There is no need to change it?

> @@ -956,17 +966,18 @@ static void unmap_iommu(struct intel_iommu *iommu)
>   /**
>    * map_iommu: map the iommu's registers
>    * @iommu: the iommu to map
> - * @phys_addr: the physical address of the base resgister
> + * @drhd: DMA remapping hardware definition structure
>    *
>    * Memory map the iommu's registers.  Start w/ a single page, and
>    * possibly expand if that turns out to be insufficent.
>    */
> -static int map_iommu(struct intel_iommu *iommu, u64 phys_addr)
> +static int map_iommu(struct intel_iommu *iommu, struct dmar_drhd_unit *drhd)
>   {
> +	u64 phys_addr = drhd->reg_base_addr;
>   	int map_size, err=0;
>   
>   	iommu->reg_phys = phys_addr;
> -	iommu->reg_size = VTD_PAGE_SIZE;
> +	iommu->reg_size = drhd->reg_size;
>   
>   	if (!request_mem_region(iommu->reg_phys, iommu->reg_size, iommu->name)) {
>   		pr_err("Can't reserve memory\n");
> @@ -1050,7 +1061,7 @@ static int alloc_iommu(struct dmar_drhd_unit *drhd)
>   	}
>   	sprintf(iommu->name, "dmar%d", iommu->seq_id);
>   
> -	err = map_iommu(iommu, drhd->reg_base_addr);
> +	err = map_iommu(iommu, drhd);
>   	if (err) {
>   		pr_err("Failed to map %s\n", iommu->name);
>   		goto error_free_seq_id;
> diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h
> index 4175dce3967c..bdded0ac46eb 100644
> --- a/include/acpi/actbl1.h
> +++ b/include/acpi/actbl1.h
> @@ -802,7 +802,7 @@ struct acpi_dmar_pci_path {
>   struct acpi_dmar_hardware_unit {
>   	struct acpi_dmar_header header;
>   	u8 flags;
> -	u8 reserved;
> +	u8 size;		/* Size of the register set */
>   	u16 segment;
>   	u64 address;		/* Register Base Address */
>   };
> diff --git a/include/linux/dmar.h b/include/linux/dmar.h
> index d81a51978d01..51d498f0a02b 100644
> --- a/include/linux/dmar.h
> +++ b/include/linux/dmar.h
> @@ -46,6 +46,7 @@ struct dmar_drhd_unit {
>   	u8	include_all:1;
>   	u8	gfx_dedicated:1;	/* graphic dedicated	*/
>   	struct intel_iommu *iommu;
> +	unsigned long reg_size;		/* size of register set */

Move it up and pair it with reg_base_addr.

>   };
>   
>   struct dmar_pci_path {

--
Best regards,
baolu