Re: [PATCH v2 3/7] iommu/vt-d: Expose ISA direct mapping region via iommu_get_resv_regions

[James Sewart]

Hey Lu,

> On 15 Mar 2019, at 02:19, Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx> wrote:
> 
> Hi James,
> 
> On 3/14/19 7:58 PM, James Sewart wrote:
>> To support mapping ISA region via iommu_group_create_direct_mappings,
>> make sure its exposed by iommu_get_resv_regions. This allows
>> deduplication of reserved region mappings
>> Signed-off-by: James Sewart <jamessewart@xxxxxxxxxx>
>> ---
>>  drivers/iommu/intel-iommu.c | 42 +++++++++++++++++++++++++++++--------
>>  1 file changed, 33 insertions(+), 9 deletions(-)
>> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
>> index 8e0a4e2ff77f..2e00e8708f06 100644
>> --- a/drivers/iommu/intel-iommu.c
>> +++ b/drivers/iommu/intel-iommu.c
>> @@ -337,6 +337,8 @@ static LIST_HEAD(dmar_rmrr_units);
>>  #define for_each_rmrr_units(rmrr) \
>>  	list_for_each_entry(rmrr, &dmar_rmrr_units, list)
>>  +static struct iommu_resv_region *isa_resv_region;
>> +
>>  /* bitmap for indexing intel_iommus */
>>  static int g_num_of_iommus;
>>  @@ -2780,26 +2782,34 @@ static inline int iommu_prepare_rmrr_dev(struct dmar_rmrr_unit *rmrr,
>>  					  rmrr->end_address);
>>  }
>>  +static inline struct iommu_resv_region *iommu_get_isa_resv_region(void)
>> +{
>> +	if (!isa_resv_region)
>> +		isa_resv_region = iommu_alloc_resv_region(0,
>> +				16*1024*1024,
>> +				0, IOMMU_RESV_DIRECT);
>> +
>> +	return isa_resv_region;
>> +}
>> +
>>  #ifdef CONFIG_INTEL_IOMMU_FLOPPY_WA
>> -static inline void iommu_prepare_isa(void)
>> +static inline void iommu_prepare_isa(struct pci_dev *pdev)
>>  {
>> -	struct pci_dev *pdev;
>>  	int ret;
>> +	struct iommu_resv_region *reg = iommu_get_isa_resv_region();
>>  -	pdev = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, NULL);
>> -	if (!pdev)
>> +	if (!reg)
>>  		return;
>>    	pr_info("Prepare 0-16MiB unity mapping for LPC\n");
>> -	ret = iommu_prepare_identity_map(&pdev->dev, 0, 16*1024*1024 - 1);
>> +	ret = iommu_prepare_identity_map(&pdev->dev, reg->start,
>> +			reg->start + reg->length - 1);
>>    	if (ret)
>>  		pr_err("Failed to create 0-16MiB identity map - floppy might not work\n");
>> -
>> -	pci_dev_put(pdev);
>>  }
>>  #else
>> -static inline void iommu_prepare_isa(void)
>> +static inline void iommu_prepare_isa(struct pci_dev *pdev)
>>  {
>>  	return;
>>  }
>> @@ -3289,6 +3299,7 @@ static int __init init_dmars(void)
>>  	struct dmar_rmrr_unit *rmrr;
>>  	bool copied_tables = false;
>>  	struct device *dev;
>> +	struct pci_dev *pdev;
>>  	struct intel_iommu *iommu;
>>  	int i, ret;
>>  @@ -3469,7 +3480,11 @@ static int __init init_dmars(void)
>>  		}
>>  	}
>>  -	iommu_prepare_isa();
>> +	pdev = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, NULL);
>> +	if (pdev) {
>> +		iommu_prepare_isa(pdev);
>> +		pci_dev_put(pdev);
>> +	}
>>    domains_done:
>>  @@ -5266,6 +5281,7 @@ static void intel_iommu_get_resv_regions(struct device *device,
>>  	struct iommu_resv_region *reg;
>>  	struct dmar_rmrr_unit *rmrr;
>>  	struct device *i_dev;
>> +	struct pci_dev *pdev;
>>  	int i;
>>    	rcu_read_lock();
>> @@ -5280,6 +5296,14 @@ static void intel_iommu_get_resv_regions(struct device *device,
>>  	}
>>  	rcu_read_unlock();
>>  +	if (dev_is_pci(device)) {
>> +		pdev = to_pci_dev(device);
>> +		if ((pdev->class >> 8) == PCI_CLASS_BRIDGE_ISA) {
>> +			reg = iommu_get_isa_resv_region();
>> +			list_add_tail(&reg->list, head);
>> +		}
>> +	}
>> +
> 
> Just wondering why not just
> 
> +#ifdef CONFIG_INTEL_IOMMU_FLOPPY_WA
> +	if (dev_is_pci(device)) {
> +		pdev = to_pci_dev(device);
> +		if ((pdev->class >> 8) == PCI_CLASS_BRIDGE_ISA) {
> +			reg = iommu_alloc_resv_region(0,
> +				16*1024*1024,
> +				0, IOMMU_RESV_DIRECT);
> +			if (reg)
> +				list_add_tail(&reg->list, head);
> +		}
> +	}
> +#endif
> 
> and, remove all other related code?

At this point in the patchset if we remove iommu_prepare_isa then the ISA 
region wonât be mapped to the device. Only once the dma domain is allocable 
will the reserved regions be mapped by iommu_group_create_direct_mappings.

Theres an issue that if we choose to alloc a new resv_region with type 
IOMMU_RESV_DIRECT, we will need to refactor intel_iommu_put_resv_regions 
to free this entry type which means refactoring the rmrr regions in 
get_resv_regions. Should this work be in this patchset?

> 
> Best regards,
> Lu Baolu

Cheers,
James.