diff mbox series

iommu/vt-d: Fix to flush cache of PASID directory table

Message ID 20230615071613.690639-1-yanfei.xu@intel.com
State New
Headers
Series iommu/vt-d: Fix to flush cache of PASID directory table |

Commit Message

Yanfei Xu June 15, 2023, 7:16 a.m. UTC 
  Even the PCI devices don't support pasid capability, PASID
table is mandatory for a PCI device in scalable mode. However
flushing cache of pasid directory table for these devices are
not taken after pasid table is allocated as the "size" of
table is zero. Fix to assign it with a page size.

Fixes: 194b3348bdbb ("iommu/vt-d: Fix PASID directory pointer coherency")
Signed-off-by: Yanfei Xu <yanfei.xu@intel.com>
---
 drivers/iommu/intel/pasid.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Zhang, Tina June 15, 2023, 11:43 p.m. UTC | #1 
Hi,

> -----Original Message-----
> From: Yanfei Xu <yanfei.xu@intel.com>
> Sent: Thursday, June 15, 2023 3:16 PM
> To: dwmw2@infradead.org; baolu.lu@linux.intel.com; joro@8bytes.org;
> will@kernel.org; robin.murphy@arm.com
> Cc: iommu@lists.linux.dev; linux-kernel@vger.kernel.org; Xu, Yanfei
> <yanfei.xu@intel.com>
> Subject: [PATCH] iommu/vt-d: Fix to flush cache of PASID directory table
> 
> Even the PCI devices don't support pasid capability, PASID table is mandatory
> for a PCI device in scalable mode. However flushing cache of pasid directory
> table for these devices are not taken after pasid table is allocated as the "size"
> of table is zero. Fix to assign it with a page size.
> 
> Fixes: 194b3348bdbb ("iommu/vt-d: Fix PASID directory pointer coherency")
> Signed-off-by: Yanfei Xu <yanfei.xu@intel.com>
> ---
>  drivers/iommu/intel/pasid.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c index
> c5d479770e12..bde7df055865 100644
> --- a/drivers/iommu/intel/pasid.c
> +++ b/drivers/iommu/intel/pasid.c
> @@ -115,7 +115,9 @@ int intel_pasid_alloc_table(struct device *dev)
>  				  intel_pasid_max_id);
> 
>  	size = max_pasid >> (PASID_PDE_SHIFT - 3);
> -	order = size ? get_order(size) : 0;
> +	if (!size)
> +		size = PAGE_SIZE;
How about merging the logic of the above few lines into this one:
size = info->pasid_supported ? max_pasid >> (PASID_PDE_SHIFT - 3) : PAGE_SIZE;

Though the logic is about the same, the suggested one seems more intuitive.

Regards,
-Tina

> +	order = get_order(size);
>  	pages = alloc_pages_node(info->iommu->node,
>  				 GFP_KERNEL | __GFP_ZERO, order);
>  	if (!pages) {
> --
> 2.34.1
>
Yanfei Xu June 16, 2023, 1:37 a.m. UTC | #2 
On 6/16/2023 7:43 AM, Zhang, Tina wrote:
> Hi,
>
>> -----Original Message-----
>> From: Yanfei Xu <yanfei.xu@intel.com>
>> Sent: Thursday, June 15, 2023 3:16 PM
>> To: dwmw2@infradead.org; baolu.lu@linux.intel.com; joro@8bytes.org;
>> will@kernel.org; robin.murphy@arm.com
>> Cc: iommu@lists.linux.dev; linux-kernel@vger.kernel.org; Xu, Yanfei
>> <yanfei.xu@intel.com>
>> Subject: [PATCH] iommu/vt-d: Fix to flush cache of PASID directory table
>>
>> Even the PCI devices don't support pasid capability, PASID table is mandatory
>> for a PCI device in scalable mode. However flushing cache of pasid directory
>> table for these devices are not taken after pasid table is allocated as the "size"
>> of table is zero. Fix to assign it with a page size.
>>
>> Fixes: 194b3348bdbb ("iommu/vt-d: Fix PASID directory pointer coherency")
>> Signed-off-by: Yanfei Xu <yanfei.xu@intel.com>
>> ---
>>   drivers/iommu/intel/pasid.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c index
>> c5d479770e12..bde7df055865 100644
>> --- a/drivers/iommu/intel/pasid.c
>> +++ b/drivers/iommu/intel/pasid.c
>> @@ -115,7 +115,9 @@ int intel_pasid_alloc_table(struct device *dev)
>>   				  intel_pasid_max_id);
>>
>>   	size = max_pasid >> (PASID_PDE_SHIFT - 3);
>> -	order = size ? get_order(size) : 0;
>> +	if (!size)
>> +		size = PAGE_SIZE;
> How about merging the logic of the above few lines into this one:
> size = info->pasid_supported ? max_pasid >> (PASID_PDE_SHIFT - 3) : PAGE_SIZE;

Yes, it would be more intuitive. But the prerequisite is if we can
make sure that the value of max_pasid shifted is still greater than
0. I roughly went through the PCIE spec and didn't find out where
defines the smallest PASID value for the PCIE device.

Thanks,
Yanfei

> Though the logic is about the same, the suggested one seems more intuitive.
>
> Regards,
> -Tina
>
>> +	order = get_order(size);
>>   	pages = alloc_pages_node(info->iommu->node,
>>   				 GFP_KERNEL | __GFP_ZERO, order);
>>   	if (!pages) {
>> --
>> 2.34.1
>>
Baolu Lu June 16, 2023, 2:01 a.m. UTC | #3 
On 6/15/23 3:16 PM, Yanfei Xu wrote:
> Even the PCI devices don't support pasid capability, PASID
> table is mandatory for a PCI device in scalable mode. However
> flushing cache of pasid directory table for these devices are
> not taken after pasid table is allocated as the "size" of
> table is zero. Fix to assign it with a page size.

Documentation/process/submitting-patches.rst

Please add more information about

- Describe your problem.
- Any background of the problem?
- How your change fixes the problem.
...

> 
> Fixes: 194b3348bdbb ("iommu/vt-d: Fix PASID directory pointer coherency")

Do you need a Cc stable?

> Signed-off-by: Yanfei Xu <yanfei.xu@intel.com>
> ---
>   drivers/iommu/intel/pasid.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
> index c5d479770e12..bde7df055865 100644
> --- a/drivers/iommu/intel/pasid.c
> +++ b/drivers/iommu/intel/pasid.c
> @@ -115,7 +115,9 @@ int intel_pasid_alloc_table(struct device *dev)
>   				  intel_pasid_max_id);
>   
>   	size = max_pasid >> (PASID_PDE_SHIFT - 3);
> -	order = size ? get_order(size) : 0;
> +	if (!size)
> +		size = PAGE_SIZE;
> +	order = get_order(size);
>   	pages = alloc_pages_node(info->iommu->node,
>   				 GFP_KERNEL | __GFP_ZERO, order);
>   	if (!pages) {

Is it similar to

diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index c5d479770e12..49fc5a038a14 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -129,7 +129,7 @@ int intel_pasid_alloc_table(struct device *dev)
         info->pasid_table = pasid_table;

         if (!ecap_coherent(info->iommu->ecap))
-               clflush_cache_range(pasid_table->table, size);
+               clflush_cache_range(pasid_table->table, (1 << order) * 
PAGE_SIZE);

         return 0;
  }

?

Best regards,
baolu
Yanfei Xu June 16, 2023, 3:06 a.m. UTC | #4 
On 6/16/2023 10:01 AM, Baolu Lu wrote:
> On 6/15/23 3:16 PM, Yanfei Xu wrote:
>> Even the PCI devices don't support pasid capability, PASID
>> table is mandatory for a PCI device in scalable mode. However
>> flushing cache of pasid directory table for these devices are
>> not taken after pasid table is allocated as the "size" of
>> table is zero. Fix to assign it with a page size.
>
> Documentation/process/submitting-patches.rst
>
> Please add more information about
>
> - Describe your problem.
> - Any background of the problem?
> - How your change fixes the problem.
> ...
>
Got it! Will improve these in commit message.

Just noticed this when reading the iommu code, no actual problem 
encountered.

>>
>> Fixes: 194b3348bdbb ("iommu/vt-d: Fix PASID directory pointer 
>> coherency")
>
> Do you need a Cc stable?

Yes, will add Cc: <stable@vger.kernel.org>

>
>> Signed-off-by: Yanfei Xu <yanfei.xu@intel.com>
>> ---
>>   drivers/iommu/intel/pasid.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
>> index c5d479770e12..bde7df055865 100644
>> --- a/drivers/iommu/intel/pasid.c
>> +++ b/drivers/iommu/intel/pasid.c
>> @@ -115,7 +115,9 @@ int intel_pasid_alloc_table(struct device *dev)
>>                     intel_pasid_max_id);
>>         size = max_pasid >> (PASID_PDE_SHIFT - 3);
>> -    order = size ? get_order(size) : 0;
>> +    if (!size)
>> +        size = PAGE_SIZE;
>> +    order = get_order(size);
>>       pages = alloc_pages_node(info->iommu->node,
>>                    GFP_KERNEL | __GFP_ZERO, order);
>>       if (!pages) {
>
> Is it similar to
A little difference that real size may less than memory size calculated by
order. But it is no effect. I think this is simpler.

Thanks,
Yanfei

>
> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
> index c5d479770e12..49fc5a038a14 100644
> --- a/drivers/iommu/intel/pasid.c
> +++ b/drivers/iommu/intel/pasid.c
> @@ -129,7 +129,7 @@ int intel_pasid_alloc_table(struct device *dev)
>         info->pasid_table = pasid_table;
>
>         if (!ecap_coherent(info->iommu->ecap))
> -               clflush_cache_range(pasid_table->table, size);
> +               clflush_cache_range(pasid_table->table, (1 << order) * 
> PAGE_SIZE);
>
>         return 0;
>  }
>
> ?
>
> Best regards,
> baolu
Baolu Lu June 16, 2023, 3:11 a.m. UTC | #5 
On 6/16/23 11:06 AM, Yanfei Xu wrote:
>>>
>>> Fixes: 194b3348bdbb ("iommu/vt-d: Fix PASID directory pointer 
>>> coherency")
>>
>> Do you need a Cc stable?
> 
> Yes, will add Cc: <stable@vger.kernel.org>

I was not asking you to Cc stable, just reminded you to consider whether
your change fixes any real problem and that problem could also happen
with stable kernels.

Best regards,
baolu
diff mbox series

Patch

diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index c5d479770e12..bde7df055865 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -115,7 +115,9 @@  int intel_pasid_alloc_table(struct device *dev)
 				  intel_pasid_max_id);
 
 	size = max_pasid >> (PASID_PDE_SHIFT - 3);
-	order = size ? get_order(size) : 0;
+	if (!size)
+		size = PAGE_SIZE;
+	order = get_order(size);
 	pages = alloc_pages_node(info->iommu->node,
 				 GFP_KERNEL | __GFP_ZERO, order);
 	if (!pages) {