[PATCH] iommu/vt-d: Fix PASID directory pointer coherency

[PATCH] iommu/vt-d: Fix PASID directory pointer coherency

[Jacob Pan]

On platforms that do not support IOMMU Extended capability bit 0
Page-walk Coherency, CPU caches are not snooped when IOMMU is accessing
any translation structures. IOMMU access goes only directly to
memory. Intel IOMMU code was missing a flush for the PASID table
directory that resulted in the unrecoverable fault as shown below.

This patch adds a clflush when activating a PASID table directory.
There's no need to do clflush of the PASID directory pointer when we
deactivate a context entry in that IOMMU hardware will not see the old
PASID directory pointer after we clear the context entry.

[    0.555386] DMAR: DRHD: handling fault status reg 3
[    0.555805] DMAR: [DMA Read NO_PASID] Request device [00:0d.2] fault addr 0x1026a4000 [fault reason 0x51] SM: Present bit in Directory Entry is clear
[    0.556348] DMAR: Dump dmar1 table entries for IOVA 0x1026a4000
[    0.556348] DMAR: scalable mode root entry: hi 0x0000000102448001, low 0x0000000101b3e001
[    0.556348] DMAR: context entry: hi 0x0000000000000000, low 0x0000000101b4d401
[    0.556348] DMAR: pasid dir entry: 0x0000000101b4e001
[    0.556348] DMAR: pasid table entry[0]: 0x0000000000000109
[    0.556348] DMAR: pasid table entry[1]: 0x0000000000000001
[    0.556348] DMAR: pasid table entry[2]: 0x0000000000000000
[    0.556348] DMAR: pasid table entry[3]: 0x0000000000000000
[    0.556348] DMAR: pasid table entry[4]: 0x0000000000000000
[    0.556348] DMAR: pasid table entry[5]: 0x0000000000000000
[    0.556348] DMAR: pasid table entry[6]: 0x0000000000000000
[    0.556348] DMAR: pasid table entry[7]: 0x0000000000000000
[    0.556348] DMAR: PTE not present at level 4

Cc: <stable@vger.kernel.org>
Reported-by: Sukumar Ghorai <sukumar.ghorai@intel.com>
Signed-off-by: Ashok Raj <ashok.raj@intel.com>
Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
---
 drivers/iommu/intel/iommu.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 59df7e42fd53..b4878c7ac008 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -1976,6 +1976,12 @@ static int domain_context_mapping_one(struct dmar_domain *domain,
 		pds = context_get_sm_pds(table);
 		context->lo = (u64)virt_to_phys(table->table) |
 				context_pdts(pds);
+		/*
+		 * Scalable-mode PASID directory pointer is not snooped if the
+		 * coherent bit is not set.
+		 */
+		if (!ecap_coherent(iommu->ecap))
+			clflush_cache_range(table->table, sizeof(void *));
 
 		/* Setup the RID_PASID field: */
 		context_set_sm_rid2pasid(context, PASID_RID2PASID);
-- 
2.25.1

Re: [PATCH] iommu/vt-d: Fix PASID directory pointer coherency

[Baolu Lu]

On 2023/2/4 6:07, Jacob Pan wrote:
> On platforms that do not support IOMMU Extended capability bit 0
> Page-walk Coherency, CPU caches are not snooped when IOMMU is accessing
> any translation structures. IOMMU access goes only directly to
> memory. Intel IOMMU code was missing a flush for the PASID table
> directory that resulted in the unrecoverable fault as shown below.

Thanks for the fix.

> This patch adds a clflush when activating a PASID table directory.
> There's no need to do clflush of the PASID directory pointer when we
> deactivate a context entry in that IOMMU hardware will not see the old
> PASID directory pointer after we clear the context entry.
> 
> [    0.555386] DMAR: DRHD: handling fault status reg 3
> [    0.555805] DMAR: [DMA Read NO_PASID] Request device [00:0d.2] fault addr 0x1026a4000 [fault reason 0x51] SM: Present bit in Directory Entry is clear
> [    0.556348] DMAR: Dump dmar1 table entries for IOVA 0x1026a4000
> [    0.556348] DMAR: scalable mode root entry: hi 0x0000000102448001, low 0x0000000101b3e001
> [    0.556348] DMAR: context entry: hi 0x0000000000000000, low 0x0000000101b4d401
> [    0.556348] DMAR: pasid dir entry: 0x0000000101b4e001
> [    0.556348] DMAR: pasid table entry[0]: 0x0000000000000109
> [    0.556348] DMAR: pasid table entry[1]: 0x0000000000000001
> [    0.556348] DMAR: pasid table entry[2]: 0x0000000000000000
> [    0.556348] DMAR: pasid table entry[3]: 0x0000000000000000
> [    0.556348] DMAR: pasid table entry[4]: 0x0000000000000000
> [    0.556348] DMAR: pasid table entry[5]: 0x0000000000000000
> [    0.556348] DMAR: pasid table entry[6]: 0x0000000000000000
> [    0.556348] DMAR: pasid table entry[7]: 0x0000000000000000
> [    0.556348] DMAR: PTE not present at level 4
> 
> Cc: <stable@vger.kernel.org>
> Reported-by: Sukumar Ghorai <sukumar.ghorai@intel.com>
> Signed-off-by: Ashok Raj <ashok.raj@intel.com>
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>

Add a Fixes tag so that people know how far this fix should be back
ported.

> ---
>   drivers/iommu/intel/iommu.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 59df7e42fd53..b4878c7ac008 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -1976,6 +1976,12 @@ static int domain_context_mapping_one(struct dmar_domain *domain,
>   		pds = context_get_sm_pds(table);
>   		context->lo = (u64)virt_to_phys(table->table) |
>   				context_pdts(pds);
> +		/*
> +		 * Scalable-mode PASID directory pointer is not snooped if the
> +		 * coherent bit is not set.
> +		 */
> +		if (!ecap_coherent(iommu->ecap))
> +			clflush_cache_range(table->table, sizeof(void *));

This isn't comprehensive. The clflush should be called whenever the
pasid directory table is allocated or updated.

>   
>   		/* Setup the RID_PASID field: */
>   		context_set_sm_rid2pasid(context, PASID_RID2PASID);

Best regards,
baolu

Re: [PATCH] iommu/vt-d: Fix PASID directory pointer coherency

[Jacob Pan]

Hi Baolu,

On Sat, 4 Feb 2023 11:43:01 +0800, Baolu Lu <baolu.lu@linux.intel.com>
wrote:

> On 2023/2/4 6:07, Jacob Pan wrote:
> > On platforms that do not support IOMMU Extended capability bit 0
> > Page-walk Coherency, CPU caches are not snooped when IOMMU is accessing
> > any translation structures. IOMMU access goes only directly to
> > memory. Intel IOMMU code was missing a flush for the PASID table
> > directory that resulted in the unrecoverable fault as shown below.  
> 
> Thanks for the fix.
> 
> > This patch adds a clflush when activating a PASID table directory.
> > There's no need to do clflush of the PASID directory pointer when we
> > deactivate a context entry in that IOMMU hardware will not see the old
> > PASID directory pointer after we clear the context entry.
> > 
> > [    0.555386] DMAR: DRHD: handling fault status reg 3
> > [    0.555805] DMAR: [DMA Read NO_PASID] Request device [00:0d.2] fault
> > addr 0x1026a4000 [fault reason 0x51] SM: Present bit in Directory Entry
> > is clear [    0.556348] DMAR: Dump dmar1 table entries for IOVA
> > 0x1026a4000 [    0.556348] DMAR: scalable mode root entry: hi
> > 0x0000000102448001, low 0x0000000101b3e001 [    0.556348] DMAR: context
> > entry: hi 0x0000000000000000, low 0x0000000101b4d401 [    0.556348]
> > DMAR: pasid dir entry: 0x0000000101b4e001 [    0.556348] DMAR: pasid
> > table entry[0]: 0x0000000000000109 [    0.556348] DMAR: pasid table
> > entry[1]: 0x0000000000000001 [    0.556348] DMAR: pasid table entry[2]:
> > 0x0000000000000000 [    0.556348] DMAR: pasid table entry[3]:
> > 0x0000000000000000 [    0.556348] DMAR: pasid table entry[4]:
> > 0x0000000000000000 [    0.556348] DMAR: pasid table entry[5]:
> > 0x0000000000000000 [    0.556348] DMAR: pasid table entry[6]:
> > 0x0000000000000000 [    0.556348] DMAR: pasid table entry[7]:
> > 0x0000000000000000 [    0.556348] DMAR: PTE not present at level 4
> > 
> > Cc: <stable@vger.kernel.org>
> > Reported-by: Sukumar Ghorai <sukumar.ghorai@intel.com>
> > Signed-off-by: Ashok Raj <ashok.raj@intel.com>
> > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>  
> 
> Add a Fixes tag so that people know how far this fix should be back
> ported.
> 
will do.
> > ---
> >   drivers/iommu/intel/iommu.c | 6 ++++++
> >   1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> > index 59df7e42fd53..b4878c7ac008 100644
> > --- a/drivers/iommu/intel/iommu.c
> > +++ b/drivers/iommu/intel/iommu.c
> > @@ -1976,6 +1976,12 @@ static int domain_context_mapping_one(struct
> > dmar_domain *domain, pds = context_get_sm_pds(table);
> >   		context->lo = (u64)virt_to_phys(table->table) |
> >   				context_pdts(pds);
> > +		/*
> > +		 * Scalable-mode PASID directory pointer is not
> > snooped if the
> > +		 * coherent bit is not set.
> > +		 */
> > +		if (!ecap_coherent(iommu->ecap))
> > +			clflush_cache_range(table->table, sizeof(void
> > *));  
> 
> This isn't comprehensive. The clflush should be called whenever the
> pasid directory table is allocated or updated.
> 
allocate a pasid table does not mean it gets used by iommu hw, not until it
is programmed into context entry.

> >   
> >   		/* Setup the RID_PASID field: */
> >   		context_set_sm_rid2pasid(context, PASID_RID2PASID);  
> 
> Best regards,
> baolu
> 


Thanks,

Jacob

RE: [PATCH] iommu/vt-d: Fix PASID directory pointer coherency

[Tian, Kevin]

> From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Sent: Tuesday, February 7, 2023 1:25 AM
> 
> > > @@ -1976,6 +1976,12 @@ static int
> domain_context_mapping_one(struct
> > > dmar_domain *domain, pds = context_get_sm_pds(table);
> > >   		context->lo = (u64)virt_to_phys(table->table) |
> > >   				context_pdts(pds);
> > > +		/*
> > > +		 * Scalable-mode PASID directory pointer is not
> > > snooped if the
> > > +		 * coherent bit is not set.
> > > +		 */
> > > +		if (!ecap_coherent(iommu->ecap))
> > > +			clflush_cache_range(table->table, sizeof(void
> > > *));
> >
> > This isn't comprehensive. The clflush should be called whenever the
> > pasid directory table is allocated or updated.
> >
> allocate a pasid table does not mean it gets used by iommu hw, not until it
> is programmed into context entry.
> 

this is insufficient.

Even after this point the PASID directory entry could be changed when
a new PASID table is allocated, e.g. in intel_pasid_get_entry().

Re: [PATCH] iommu/vt-d: Fix PASID directory pointer coherency

[Jacob Pan]

Hi Kevin,

On Tue, 7 Feb 2023 00:41:16 +0000, "Tian, Kevin" <kevin.tian@intel.com>
wrote:

> > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > Sent: Tuesday, February 7, 2023 1:25 AM
> >   
> > > > @@ -1976,6 +1976,12 @@ static int  
> > domain_context_mapping_one(struct  
> > > > dmar_domain *domain, pds = context_get_sm_pds(table);
> > > >   		context->lo = (u64)virt_to_phys(table->table) |
> > > >   				context_pdts(pds);
> > > > +		/*
> > > > +		 * Scalable-mode PASID directory pointer is not
> > > > snooped if the
> > > > +		 * coherent bit is not set.
> > > > +		 */
> > > > +		if (!ecap_coherent(iommu->ecap))
> > > > +			clflush_cache_range(table->table,
> > > > sizeof(void *));  
> > >
> > > This isn't comprehensive. The clflush should be called whenever the
> > > pasid directory table is allocated or updated.
> > >  
> > allocate a pasid table does not mean it gets used by iommu hw, not
> > until it is programmed into context entry.
> >   
> 
> this is insufficient.
> 
> Even after this point the PASID directory entry could be changed when
> a new PASID table is allocated, e.g. in intel_pasid_get_entry().
> 
you are right, will include updates in v2.

Thanks,

Jacob

Re: [PATCH] iommu/vt-d: Fix PASID directory pointer coherency

[Baolu Lu]

On 2023/2/7 1:25, Jacob Pan wrote:
>>> ---
>>>    drivers/iommu/intel/iommu.c | 6 ++++++
>>>    1 file changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>>> index 59df7e42fd53..b4878c7ac008 100644
>>> --- a/drivers/iommu/intel/iommu.c
>>> +++ b/drivers/iommu/intel/iommu.c
>>> @@ -1976,6 +1976,12 @@ static int domain_context_mapping_one(struct
>>> dmar_domain *domain, pds = context_get_sm_pds(table);
>>>    		context->lo = (u64)virt_to_phys(table->table) |
>>>    				context_pdts(pds);
>>> +		/*
>>> +		 * Scalable-mode PASID directory pointer is not
>>> snooped if the
>>> +		 * coherent bit is not set.
>>> +		 */
>>> +		if (!ecap_coherent(iommu->ecap))
>>> +			clflush_cache_range(table->table, sizeof(void
>>> *));
>> This isn't comprehensive. The clflush should be called whenever the
>> pasid directory table is allocated or updated.
>>
> allocate a pasid table does not mean it gets used by iommu hw, not until it
> is programmed into context entry.

Hi Jacob,

This page is used by the device, and the device's access to this memory
is not coherent. So after the page is allocated, any changes made by the
CPU to this page must be written back to the real memory.

This patch only flushes the first 8 bytes of the table. That's not
enough.

Be aware that page allocation also requires a clflush, because at least
__GFP_ ZERO implies modification to page.

Best regards,
baolu

Re: [PATCH] iommu/vt-d: Fix PASID directory pointer coherency

[Jacob Pan]

Hi Baolu,

On Tue, 7 Feb 2023 15:10:48 +0800, Baolu Lu <baolu.lu@linux.intel.com>
wrote:

> On 2023/2/7 1:25, Jacob Pan wrote:
> >>> ---
> >>>    drivers/iommu/intel/iommu.c | 6 ++++++
> >>>    1 file changed, 6 insertions(+)
> >>>
> >>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> >>> index 59df7e42fd53..b4878c7ac008 100644
> >>> --- a/drivers/iommu/intel/iommu.c
> >>> +++ b/drivers/iommu/intel/iommu.c
> >>> @@ -1976,6 +1976,12 @@ static int domain_context_mapping_one(struct
> >>> dmar_domain *domain, pds = context_get_sm_pds(table);
> >>>    		context->lo = (u64)virt_to_phys(table->table) |
> >>>    				context_pdts(pds);
> >>> +		/*
> >>> +		 * Scalable-mode PASID directory pointer is not
> >>> snooped if the
> >>> +		 * coherent bit is not set.
> >>> +		 */
> >>> +		if (!ecap_coherent(iommu->ecap))
> >>> +			clflush_cache_range(table->table, sizeof(void
> >>> *));  
> >> This isn't comprehensive. The clflush should be called whenever the
> >> pasid directory table is allocated or updated.
> >>  
> > allocate a pasid table does not mean it gets used by iommu hw, not
> > until it is programmed into context entry.  
> 
> Hi Jacob,
> 
> This page is used by the device, and the device's access to this memory
> is not coherent. So after the page is allocated, any changes made by the
> CPU to this page must be written back to the real memory.
> 
> This patch only flushes the first 8 bytes of the table. That's not
> enough.
> 
make sense, thanks for the explanation.
> Be aware that page allocation also requires a clflush, because at least
> __GFP_ ZERO implies modification to page.
> 
Yes, zeroing dir page does change it but I think if we intercept every
update to the directory entry, it should be sufficient?

will send an updated version.


Thanks,

Jacob