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 <[email protected]>
---
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) {
--
2.34.1
Hi,
> -----Original Message-----
> From: Yanfei Xu <[email protected]>
> Sent: Thursday, June 15, 2023 3:16 PM
> To: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]
> Cc: [email protected]; [email protected]; Xu, Yanfei
> <[email protected]>
> 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 <[email protected]>
> ---
> 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
>
On 6/16/2023 7:43 AM, Zhang, Tina wrote:
> Hi,
>
>> -----Original Message-----
>> From: Yanfei Xu <[email protected]>
>> Sent: Thursday, June 15, 2023 3:16 PM
>> To: [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected]
>> Cc: [email protected]; [email protected]; Xu, Yanfei
>> <[email protected]>
>> 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 <[email protected]>
>> ---
>> 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
>>
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 <[email protected]>
> ---
> 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
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: <[email protected]>
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
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: <[email protected]>
>
>> Signed-off-by: Yanfei Xu <[email protected]>
>> ---
>> 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