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

[James Sewart]

Hey Lu,

> On 25 Mar 2019, at 02:03, Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx> wrote:
> 
> Hi James,
> 
> On 3/22/19 5:57 PM, James Sewart wrote:
>> 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.
> 
> Yes. So if we put the allocation code here, it won't impact anything and
> will take effect as soon as the dma domain is allocatable.
> 
>> 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?
> 
> Do you mean the rmrr regions are not allocated in get_resv_regions, but
> are freed in put_resv_regions? I think we should fix this in this patch
> set since this might impact the device passthrough if we don't do it.

Theyâre not allocated and not freed currently, only type IOMMU_RESV_MSI is 
freed in put_resv_regions. If we allocate a new resv_region with type 
IOMMU_RESV_DIRECT for the isa region, then it wonât be freed. If we modify 
put_resv_regions to free type IOMMU_RESV_DIRECT, then we will try to free 
the static RMRR regions.

Either the ISA region is static and not freed as with my implementation, 
or the RMRR regions are converted to be allocated on each call to 
get_resv_regions and freed in put_resv_regions.

> 
> Best regards,
> Lu Baolu

Cheers,
James.