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

login
register
mail settings
Submitter Benjamin Serebrin via iommu
Date March 14, 2019, 11:58 a.m.
Message ID <F531CABB-FFDA-4E0A-A3D5-095FF338DD5A@arista.com>
Download mbox | patch
Permalink /patch/748707/
State New
Headers show

Comments

Benjamin Serebrin via iommu - March 14, 2019, 11:58 a.m.
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@arista.com>
---
 drivers/iommu/intel-iommu.c | 42 +++++++++++++++++++++++++++++--------
 1 file changed, 33 insertions(+), 9 deletions(-)
Lu Baolu - March 15, 2019, 2:19 a.m.
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@arista.com>
> ---
>   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?

Best regards,
Lu Baolu
Benjamin Serebrin via iommu - March 22, 2019, 9:57 a.m.
Hey Lu,

> On 15 Mar 2019, at 02:19, Lu Baolu <baolu.lu@linux.intel.com> 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@arista.com>
>> ---
>>  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.
Lu Baolu - March 25, 2019, 2:03 a.m.
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@linux.intel.com> 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@arista.com>
>>> ---
>>>   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.

Best regards,
Lu Baolu
Benjamin Serebrin via iommu - March 25, 2019, 12:57 p.m.
Hey Lu,

> On 25 Mar 2019, at 02:03, Lu Baolu <baolu.lu@linux.intel.com> 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@linux.intel.com> 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@arista.com>
>>>> ---
>>>>  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.
Lu Baolu - March 26, 2019, 1:10 a.m.
Hi,

On 3/25/19 8:57 PM, James Sewart wrote:
> Hey Lu,
> 
>> On 25 Mar 2019, at 02:03, Lu Baolu <baolu.lu@linux.intel.com> 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@linux.intel.com> 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@arista.com>
>>>>> ---
>>>>>   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.

Ah yes! Thanks for the explanation.

> 
> 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.

Okay, I second your implementation now.

> 
>>
>> Best regards,
>> Lu Baolu
> 
> Cheers,
> James.
> 

Best regards,
Lu Baolu
Lu Baolu - March 26, 2019, 1:24 a.m.
Hi James,

On 3/25/19 8:57 PM, James Sewart wrote:
>>> 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.
> 

By the way, there's another way in my mind. Let's add a new region type
for LPC devices, e.x. IOMMU_RESV_LPC, and then handle it in the same way
as those MSI regions. Just FYI.

Best regards,
Lu Baolu
Benjamin Serebrin via iommu - March 28, 2019, 6:37 p.m.
Hey Lu,

> On 26 Mar 2019, at 01:24, Lu Baolu <baolu.lu@linux.intel.com> wrote:
> 
> Hi James,
> 
> On 3/25/19 8:57 PM, James Sewart wrote:
>>>> 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.
> 
> By the way, there's another way in my mind. Let's add a new region type
> for LPC devices, e.x. IOMMU_RESV_LPC, and then handle it in the same way
> as those MSI regions. Just FYI.

This solution would require adding some extra code to 
iommu_group_create_direct_mappings as currently only type 
IOMMU_RESV_DIRECT is identity mapped, other types are only reserved.


> 
> Best regards,
> Lu Baolu

Cheers,
James.
Benjamin Serebrin via iommu - March 29, 2019, 3:26 p.m.
Hey Lu,

I’ve attached a preliminary v3, if you could take a look and run some tests 
that would be great.

Since v2 i’ve added your default domain type patches, the new device_group 
handler, and incorporated Jacob’s feedback.

> On 28 Mar 2019, at 18:37, James Sewart <jamessewart@arista.com> wrote:
> 
> Hey Lu,
> 
>> On 26 Mar 2019, at 01:24, Lu Baolu <baolu.lu@linux.intel.com> wrote:
>> 
>> Hi James,
>> 
>> On 3/25/19 8:57 PM, James Sewart wrote:
>>>>> 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.
>> 
>> By the way, there's another way in my mind. Let's add a new region type
>> for LPC devices, e.x. IOMMU_RESV_LPC, and then handle it in the same way
>> as those MSI regions. Just FYI.
> 
> This solution would require adding some extra code to 
> iommu_group_create_direct_mappings as currently only type 
> IOMMU_RESV_DIRECT is identity mapped, other types are only reserved.
> 
> 
>> 
>> Best regards,
>> Lu Baolu
> 
> Cheers,
> James.

Cheers,
James.
Lu Baolu - April 4, 2019, 6:49 a.m.
Hi James,

I did a sanity test from my end. The test machine fails to boot. I
haven't seen any valuable kernel log. It results in a purple screen.

Best regards,
Lu Baolu

On 3/29/19 11:26 PM, James Sewart wrote:
> Hey Lu,
> 
> I’ve attached a preliminary v3, if you could take a look and run some tests
> that would be great.
> 
> Since v2 i’ve added your default domain type patches, the new device_group
> handler, and incorporated Jacob’s feedback.
> 
>> On 28 Mar 2019, at 18:37, James Sewart <jamessewart@arista.com> wrote:
>>
>> Hey Lu,
>>
>>> On 26 Mar 2019, at 01:24, Lu Baolu <baolu.lu@linux.intel.com> wrote:
>>>
>>> Hi James,
>>>
>>> On 3/25/19 8:57 PM, James Sewart wrote:
>>>>>> 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.
>>>
>>> By the way, there's another way in my mind. Let's add a new region type
>>> for LPC devices, e.x. IOMMU_RESV_LPC, and then handle it in the same way
>>> as those MSI regions. Just FYI.
>>
>> This solution would require adding some extra code to
>> iommu_group_create_direct_mappings as currently only type
>> IOMMU_RESV_DIRECT is identity mapped, other types are only reserved.
>>
>>
>>>
>>> Best regards,
>>> Lu Baolu
>>
>> Cheers,
>> James.
> 
> Cheers,
> James.
>
Benjamin Serebrin via iommu - April 5, 2019, 6:02 p.m.
Hey Lu,

My bad, did some debugging on my end. The issue was swapping out 
find_domain for iommu_get_domain_for_dev. It seems in some situations the 
domain is not attached to the group but the device is expected to have the 
domain still stored in its archdata.

I’ve attached the final patch with find_domain unremoved which seems to 
work in my testing.

Cheers,
James.
> On 4 Apr 2019, at 07:49, Lu Baolu <baolu.lu@linux.intel.com> wrote:
> 
> Hi James,
> 
> I did a sanity test from my end. The test machine fails to boot. I
> haven't seen any valuable kernel log. It results in a purple screen.
> 
> Best regards,
> Lu Baolu
> 
> On 3/29/19 11:26 PM, James Sewart wrote:
>> Hey Lu,
>> I’ve attached a preliminary v3, if you could take a look and run some tests
>> that would be great.
>> Since v2 i’ve added your default domain type patches, the new device_group
>> handler, and incorporated Jacob’s feedback.
>>> On 28 Mar 2019, at 18:37, James Sewart <jamessewart@arista.com> wrote:
>>> 
>>> Hey Lu,
>>> 
>>>> On 26 Mar 2019, at 01:24, Lu Baolu <baolu.lu@linux.intel.com> wrote:
>>>> 
>>>> Hi James,
>>>> 
>>>> On 3/25/19 8:57 PM, James Sewart wrote:
>>>>>>> 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.
>>>> 
>>>> By the way, there's another way in my mind. Let's add a new region type
>>>> for LPC devices, e.x. IOMMU_RESV_LPC, and then handle it in the same way
>>>> as those MSI regions. Just FYI.
>>> 
>>> This solution would require adding some extra code to
>>> iommu_group_create_direct_mappings as currently only type
>>> IOMMU_RESV_DIRECT is identity mapped, other types are only reserved.
>>> 
>>> 
>>>> 
>>>> Best regards,
>>>> Lu Baolu
>>> 
>>> Cheers,
>>> James.
>> Cheers,
>> James.
Lu Baolu - April 8, 2019, 2:43 a.m.
Hi,

On 4/6/19 2:02 AM, James Sewart wrote:
> Hey Lu,
> 
> My bad, did some debugging on my end. The issue was swapping out
> find_domain for iommu_get_domain_for_dev. It seems in some situations the
> domain is not attached to the group but the device is expected to have the
> domain still stored in its archdata.
> 
> I’ve attached the final patch with find_domain unremoved which seems to
> work in my testing.

This version works for me now.

> 
> Cheers,
> James.

Best regards,
Lu Baolu

> 
> 
> 
> 
> 
>> On 4 Apr 2019, at 07:49, Lu Baolu <baolu.lu@linux.intel.com> wrote:
>>
>> Hi James,
>>
>> I did a sanity test from my end. The test machine fails to boot. I
>> haven't seen any valuable kernel log. It results in a purple screen.
>>
>> Best regards,
>> Lu Baolu
>>
>> On 3/29/19 11:26 PM, James Sewart wrote:
>>> Hey Lu,
>>> I’ve attached a preliminary v3, if you could take a look and run some tests
>>> that would be great.
>>> Since v2 i’ve added your default domain type patches, the new device_group
>>> handler, and incorporated Jacob’s feedback.
>>>> On 28 Mar 2019, at 18:37, James Sewart <jamessewart@arista.com> wrote:
>>>>
>>>> Hey Lu,
>>>>
>>>>> On 26 Mar 2019, at 01:24, Lu Baolu <baolu.lu@linux.intel.com> wrote:
>>>>>
>>>>> Hi James,
>>>>>
>>>>> On 3/25/19 8:57 PM, James Sewart wrote:
>>>>>>>> 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.
>>>>>
>>>>> By the way, there's another way in my mind. Let's add a new region type
>>>>> for LPC devices, e.x. IOMMU_RESV_LPC, and then handle it in the same way
>>>>> as those MSI regions. Just FYI.
>>>>
>>>> This solution would require adding some extra code to
>>>> iommu_group_create_direct_mappings as currently only type
>>>> IOMMU_RESV_DIRECT is identity mapped, other types are only reserved.
>>>>
>>>>
>>>>>
>>>>> Best regards,
>>>>> Lu Baolu
>>>>
>>>> Cheers,
>>>> James.
>>> Cheers,
>>> James.
>
Lu Baolu - April 10, 2019, 5:22 a.m.
Hi James,

On 4/6/19 2:02 AM, James Sewart wrote:
> Hey Lu,
> 
> My bad, did some debugging on my end. The issue was swapping out
> find_domain for iommu_get_domain_for_dev. It seems in some situations the
> domain is not attached to the group but the device is expected to have the
> domain still stored in its archdata.
> 
> I’ve attached the final patch with find_domain unremoved which seems to
> work in my testing.
> 

Just looked into your v3 patch set and some thoughts from my end posted
here just for your information.

Let me post the problems we want to address.

1. When allocating a new group for a device, how should we determine the
type of the default domain?

2. If we need to put a device into an existing group which uses a
different type of domain from what the device desires to use, we might
break the functionality of the device.

My new thought is letting the iommu generic code to determine the
default domain type (hence my proposed vendor specific default domain
type patches could be dropped).

If the default domain type is dynamical mapping, and later in 
iommu_no_mapping(), we determines that we must use an identity domain,
we then call iommu_request_dm_for_dev(dev).

If the default domain type is identity mapping, and later in
iommu_no_mapping(), we determined that we must use a dynamical domain,
we then call iommu_request_dma_domain_for_dev(dev).

We already have iommu_request_dm_for_dev() in iommu.c. We only need to
implement iommu_request_dma_domain_for_dev().

With this done, your patch titled "Create an IOMMU group for devices
that require an identity map" could also be dropped.

Any thoughts?

> Cheers,
> James.

Best regards,
Lu Baolu
Benjamin Serebrin via iommu - April 15, 2019, 2:16 p.m.
Hey Lu,

> On 10 Apr 2019, at 06:22, Lu Baolu <baolu.lu@linux.intel.com> wrote:
> 
> Hi James,
> 
> On 4/6/19 2:02 AM, James Sewart wrote:
>> Hey Lu,
>> My bad, did some debugging on my end. The issue was swapping out
>> find_domain for iommu_get_domain_for_dev. It seems in some situations the
>> domain is not attached to the group but the device is expected to have the
>> domain still stored in its archdata.
>> I’ve attached the final patch with find_domain unremoved which seems to
>> work in my testing.
> 
> Just looked into your v3 patch set and some thoughts from my end posted
> here just for your information.
> 
> Let me post the problems we want to address.
> 
> 1. When allocating a new group for a device, how should we determine the
> type of the default domain?
> 
> 2. If we need to put a device into an existing group which uses a
> different type of domain from what the device desires to use, we might
> break the functionality of the device.
> 
> My new thought is letting the iommu generic code to determine the
> default domain type (hence my proposed vendor specific default domain
> type patches could be dropped).
> 
> If the default domain type is dynamical mapping, and later in iommu_no_mapping(), we determines that we must use an identity domain,
> we then call iommu_request_dm_for_dev(dev).
> 
> If the default domain type is identity mapping, and later in
> iommu_no_mapping(), we determined that we must use a dynamical domain,
> we then call iommu_request_dma_domain_for_dev(dev).
> 
> We already have iommu_request_dm_for_dev() in iommu.c. We only need to
> implement iommu_request_dma_domain_for_dev().
> 
> With this done, your patch titled "Create an IOMMU group for devices
> that require an identity map" could also be dropped.
> 
> Any thoughts?

Theres a couple issues I can think of. iommu_request_dm_for_dev changes 
the domain for all devices within the devices group, if we may have 
devices with different domain requirements in the same group then only the 
last initialised device will get the domain it wants. If we want to add 
iommu_request_dma_domain_for_dev then we would still need another IOMMU 
group for identity domain devices.

Both with v3 and the proposed method here, iommu_should_identity_map is 
determining whether the device requires an identity map. In v3 this is 
called once up front by the generic code to determine for the IOMMU group 
which domain type to use. In the proposed method I think this would happen 
after initial domain allocation and would trigger the domain to be 
replaced with a different domain.

I prefer the solution in v3. What do you think?

> 
>> Cheers,
>> James.
> 
> Best regards,
> Lu Baolu

Cheers,
James.
Lu Baolu - April 16, 2019, 2:18 a.m.
Hi James,

On 4/15/19 10:16 PM, James Sewart wrote:
> Hey Lu,
> 
>> On 10 Apr 2019, at 06:22, Lu Baolu <baolu.lu@linux.intel.com> wrote:
>>
>> Hi James,
>>
>> On 4/6/19 2:02 AM, James Sewart wrote:
>>> Hey Lu,
>>> My bad, did some debugging on my end. The issue was swapping out
>>> find_domain for iommu_get_domain_for_dev. It seems in some situations the
>>> domain is not attached to the group but the device is expected to have the
>>> domain still stored in its archdata.
>>> I’ve attached the final patch with find_domain unremoved which seems to
>>> work in my testing.
>>
>> Just looked into your v3 patch set and some thoughts from my end posted
>> here just for your information.
>>
>> Let me post the problems we want to address.
>>
>> 1. When allocating a new group for a device, how should we determine the
>> type of the default domain?
>>
>> 2. If we need to put a device into an existing group which uses a
>> different type of domain from what the device desires to use, we might
>> break the functionality of the device.
>>
>> My new thought is letting the iommu generic code to determine the
>> default domain type (hence my proposed vendor specific default domain
>> type patches could be dropped).
>>
>> If the default domain type is dynamical mapping, and later in iommu_no_mapping(), we determines that we must use an identity domain,
>> we then call iommu_request_dm_for_dev(dev).
>>
>> If the default domain type is identity mapping, and later in
>> iommu_no_mapping(), we determined that we must use a dynamical domain,
>> we then call iommu_request_dma_domain_for_dev(dev).
>>
>> We already have iommu_request_dm_for_dev() in iommu.c. We only need to
>> implement iommu_request_dma_domain_for_dev().
>>
>> With this done, your patch titled "Create an IOMMU group for devices
>> that require an identity map" could also be dropped.
>>
>> Any thoughts?
> 
> Theres a couple issues I can think of. iommu_request_dm_for_dev changes
> the domain for all devices within the devices group, if we may have
> devices with different domain requirements in the same group then only the
> last initialised device will get the domain it wants. If we want to add
> iommu_request_dma_domain_for_dev then we would still need another IOMMU
> group for identity domain devices.

Current solution (a.k.a. v3) for this is to assign a new group to the
device which requires an identity mapped domain. This will break the
functionality of device direct pass-through (to user level). The iommu
group represents the minimum granularity of iommu isolation and
protection. The requirement of identity mapped domain should not be a
factor for a new group.

Both iomm_request_dma_domain_for_dev() or iommu_request_dm_for_dev()
only support groups with a single device in it.

The users could choose between domains of DMA type or IDENTITY type for
a group. If multiple devices share a single group and both don't work
for them, they have to disable the IOMMU. This isn't a valid
configuration from IOMMU's point of view.

> 
> Both with v3 and the proposed method here, iommu_should_identity_map is
> determining whether the device requires an identity map. In v3 this is
> called once up front by the generic code to determine for the IOMMU group
> which domain type to use. In the proposed method I think this would happen
> after initial domain allocation and would trigger the domain to be
> replaced with a different domain.
> 
> I prefer the solution in v3. What do you think?

The only concern of solution in v3 from my point of view is some kind of
duplication. Even we can ask the Intel IOMMU driver to tell the default
domain type, we might change it after boot up. For example, for 32-bit
pci devices, we don't know whether it's 64-bit or 32-bit capable during
IOMMU initialization, so we might tell iommu.c to use identity mapped
domain. After boot up, we check it again, and find out that it's only
32-bit capable (hence only can access physical memory below 4G) and the
default identity domain doesn't work. And we have to change it to DMA
domain via iommu_request_dma_domain_for_dev().

So to make it simple and straight-forward, we can just let iommu.c to
determine the default domain type and after that the Intel IOMMU driver
could tweak it according to the quirk bits in the driver.

Best regards,
Lu Baolu
Benjamin Serebrin via iommu - April 24, 2019, 11:47 p.m.
I can see two potential problems with these patches that should be addressed:

The default domain of a group can be changed to type
IOMMU_DOMAIN_IDENTITY via the command line. With these patches we are
returning the si_domain for type IOMMU_DOMAIN_IDENTITY. There's a
chance the shared si_domain could be freed while it is still being
used when a group is freed. For example here in the
iommu_group_release:
https://github.com/torvalds/linux/blob/cd8dead0c39457e58ec1d36db93aedca811d48f1/drivers/iommu/iommu.c#L376
"if (group->default_domain)
    iommu_domain_free(group->default_domain);"

Also now that a domain is attached to a device earlier we should
implement the is_attach_deferred call-back and use it to defer the
domain attach from iommu driver init to device driver init when iommu
is pre-enabled in kdump kernel. like in this commit:
https://github.com/torvalds/linux/commit/df3f7a6e8e855e4ff533508807cd7c3723faa51f


On Tue, Apr 16, 2019 at 3:24 AM Lu Baolu <baolu.lu@linux.intel.com> wrote:
>
> Hi James,
>
> On 4/15/19 10:16 PM, James Sewart wrote:
> > Hey Lu,
> >
> >> On 10 Apr 2019, at 06:22, Lu Baolu <baolu.lu@linux.intel.com> wrote:
> >>
> >> Hi James,
> >>
> >> On 4/6/19 2:02 AM, James Sewart wrote:
> >>> Hey Lu,
> >>> My bad, did some debugging on my end. The issue was swapping out
> >>> find_domain for iommu_get_domain_for_dev. It seems in some situations the
> >>> domain is not attached to the group but the device is expected to have the
> >>> domain still stored in its archdata.
> >>> I’ve attached the final patch with find_domain unremoved which seems to
> >>> work in my testing.
> >>
> >> Just looked into your v3 patch set and some thoughts from my end posted
> >> here just for your information.
> >>
> >> Let me post the problems we want to address.
> >>
> >> 1. When allocating a new group for a device, how should we determine the
> >> type of the default domain?
> >>
> >> 2. If we need to put a device into an existing group which uses a
> >> different type of domain from what the device desires to use, we might
> >> break the functionality of the device.
> >>
> >> My new thought is letting the iommu generic code to determine the
> >> default domain type (hence my proposed vendor specific default domain
> >> type patches could be dropped).
> >>
> >> If the default domain type is dynamical mapping, and later in iommu_no_mapping(), we determines that we must use an identity domain,
> >> we then call iommu_request_dm_for_dev(dev).
> >>
> >> If the default domain type is identity mapping, and later in
> >> iommu_no_mapping(), we determined that we must use a dynamical domain,
> >> we then call iommu_request_dma_domain_for_dev(dev).
> >>
> >> We already have iommu_request_dm_for_dev() in iommu.c. We only need to
> >> implement iommu_request_dma_domain_for_dev().
> >>
> >> With this done, your patch titled "Create an IOMMU group for devices
> >> that require an identity map" could also be dropped.
> >>
> >> Any thoughts?
> >
> > Theres a couple issues I can think of. iommu_request_dm_for_dev changes
> > the domain for all devices within the devices group, if we may have
> > devices with different domain requirements in the same group then only the
> > last initialised device will get the domain it wants. If we want to add
> > iommu_request_dma_domain_for_dev then we would still need another IOMMU
> > group for identity domain devices.
>
> Current solution (a.k.a. v3) for this is to assign a new group to the
> device which requires an identity mapped domain. This will break the
> functionality of device direct pass-through (to user level). The iommu
> group represents the minimum granularity of iommu isolation and
> protection. The requirement of identity mapped domain should not be a
> factor for a new group.
>
> Both iomm_request_dma_domain_for_dev() or iommu_request_dm_for_dev()
> only support groups with a single device in it.
>
> The users could choose between domains of DMA type or IDENTITY type for
> a group. If multiple devices share a single group and both don't work
> for them, they have to disable the IOMMU. This isn't a valid
> configuration from IOMMU's point of view.
>
> >
> > Both with v3 and the proposed method here, iommu_should_identity_map is
> > determining whether the device requires an identity map. In v3 this is
> > called once up front by the generic code to determine for the IOMMU group
> > which domain type to use. In the proposed method I think this would happen
> > after initial domain allocation and would trigger the domain to be
> > replaced with a different domain.
> >
> > I prefer the solution in v3. What do you think?
>
> The only concern of solution in v3 from my point of view is some kind of
> duplication. Even we can ask the Intel IOMMU driver to tell the default
> domain type, we might change it after boot up. For example, for 32-bit
> pci devices, we don't know whether it's 64-bit or 32-bit capable during
> IOMMU initialization, so we might tell iommu.c to use identity mapped
> domain. After boot up, we check it again, and find out that it's only
> 32-bit capable (hence only can access physical memory below 4G) and the
> default identity domain doesn't work. And we have to change it to DMA
> domain via iommu_request_dma_domain_for_dev().
>
> So to make it simple and straight-forward, we can just let iommu.c to
> determine the default domain type and after that the Intel IOMMU driver
> could tweak it according to the quirk bits in the driver.
>
> Best regards,
> Lu Baolu
Lu Baolu - April 25, 2019, 1:15 a.m.
Hi Tom,

On 4/25/19 7:47 AM, Tom Murphy wrote:
> I can see two potential problems with these patches that should be addressed:
> 
> The default domain of a group can be changed to type
> IOMMU_DOMAIN_IDENTITY via the command line. With these patches we are
> returning the si_domain for type IOMMU_DOMAIN_IDENTITY. There's a
> chance the shared si_domain could be freed while it is still being
> used when a group is freed. For example here in the
> iommu_group_release:
> https://github.com/torvalds/linux/blob/cd8dead0c39457e58ec1d36db93aedca811d48f1/drivers/iommu/iommu.c#L376
> "if (group->default_domain)
>      iommu_domain_free(group->default_domain);"
> 
> Also now that a domain is attached to a device earlier we should
> implement the is_attach_deferred call-back and use it to defer the
> domain attach from iommu driver init to device driver init when iommu
> is pre-enabled in kdump kernel. like in this commit:
> https://github.com/torvalds/linux/commit/df3f7a6e8e855e4ff533508807cd7c3723faa51f
> 

Both agreed and should be considered during development.

Best regards,
Lu Baolu

> 
> On Tue, Apr 16, 2019 at 3:24 AM Lu Baolu <baolu.lu@linux.intel.com> wrote:
>>
>> Hi James,
>>
>> On 4/15/19 10:16 PM, James Sewart wrote:
>>> Hey Lu,
>>>
>>>> On 10 Apr 2019, at 06:22, Lu Baolu <baolu.lu@linux.intel.com> wrote:
>>>>
>>>> Hi James,
>>>>
>>>> On 4/6/19 2:02 AM, James Sewart wrote:
>>>>> Hey Lu,
>>>>> My bad, did some debugging on my end. The issue was swapping out
>>>>> find_domain for iommu_get_domain_for_dev. It seems in some situations the
>>>>> domain is not attached to the group but the device is expected to have the
>>>>> domain still stored in its archdata.
>>>>> I’ve attached the final patch with find_domain unremoved which seems to
>>>>> work in my testing.
>>>>
>>>> Just looked into your v3 patch set and some thoughts from my end posted
>>>> here just for your information.
>>>>
>>>> Let me post the problems we want to address.
>>>>
>>>> 1. When allocating a new group for a device, how should we determine the
>>>> type of the default domain?
>>>>
>>>> 2. If we need to put a device into an existing group which uses a
>>>> different type of domain from what the device desires to use, we might
>>>> break the functionality of the device.
>>>>
>>>> My new thought is letting the iommu generic code to determine the
>>>> default domain type (hence my proposed vendor specific default domain
>>>> type patches could be dropped).
>>>>
>>>> If the default domain type is dynamical mapping, and later in iommu_no_mapping(), we determines that we must use an identity domain,
>>>> we then call iommu_request_dm_for_dev(dev).
>>>>
>>>> If the default domain type is identity mapping, and later in
>>>> iommu_no_mapping(), we determined that we must use a dynamical domain,
>>>> we then call iommu_request_dma_domain_for_dev(dev).
>>>>
>>>> We already have iommu_request_dm_for_dev() in iommu.c. We only need to
>>>> implement iommu_request_dma_domain_for_dev().
>>>>
>>>> With this done, your patch titled "Create an IOMMU group for devices
>>>> that require an identity map" could also be dropped.
>>>>
>>>> Any thoughts?
>>>
>>> Theres a couple issues I can think of. iommu_request_dm_for_dev changes
>>> the domain for all devices within the devices group, if we may have
>>> devices with different domain requirements in the same group then only the
>>> last initialised device will get the domain it wants. If we want to add
>>> iommu_request_dma_domain_for_dev then we would still need another IOMMU
>>> group for identity domain devices.
>>
>> Current solution (a.k.a. v3) for this is to assign a new group to the
>> device which requires an identity mapped domain. This will break the
>> functionality of device direct pass-through (to user level). The iommu
>> group represents the minimum granularity of iommu isolation and
>> protection. The requirement of identity mapped domain should not be a
>> factor for a new group.
>>
>> Both iomm_request_dma_domain_for_dev() or iommu_request_dm_for_dev()
>> only support groups with a single device in it.
>>
>> The users could choose between domains of DMA type or IDENTITY type for
>> a group. If multiple devices share a single group and both don't work
>> for them, they have to disable the IOMMU. This isn't a valid
>> configuration from IOMMU's point of view.
>>
>>>
>>> Both with v3 and the proposed method here, iommu_should_identity_map is
>>> determining whether the device requires an identity map. In v3 this is
>>> called once up front by the generic code to determine for the IOMMU group
>>> which domain type to use. In the proposed method I think this would happen
>>> after initial domain allocation and would trigger the domain to be
>>> replaced with a different domain.
>>>
>>> I prefer the solution in v3. What do you think?
>>
>> The only concern of solution in v3 from my point of view is some kind of
>> duplication. Even we can ask the Intel IOMMU driver to tell the default
>> domain type, we might change it after boot up. For example, for 32-bit
>> pci devices, we don't know whether it's 64-bit or 32-bit capable during
>> IOMMU initialization, so we might tell iommu.c to use identity mapped
>> domain. After boot up, we check it again, and find out that it's only
>> 32-bit capable (hence only can access physical memory below 4G) and the
>> default identity domain doesn't work. And we have to change it to DMA
>> domain via iommu_request_dma_domain_for_dev().
>>
>> So to make it simple and straight-forward, we can just let iommu.c to
>> determine the default domain type and after that the Intel IOMMU driver
>> could tweak it according to the quirk bits in the driver.
>>
>> Best regards,
>> Lu Baolu
>

Patch

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);
+		}
+	}
+
 	reg = iommu_alloc_resv_region(IOAPIC_RANGE_START,
 				      IOAPIC_RANGE_END - IOAPIC_RANGE_START + 1,
 				      0, IOMMU_RESV_MSI);