Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1532348 > unrolled thread

Re: [PATCH] iommu/vt-d: Flush old iotlb for kdump when the device gets context mapped

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] iommu/vt-d: Flush old iotlb for kdump when the device  gets context mapped Joerg Roedel <joro@8bytes.org> - 2016-11-29 15:40 +0100
    Re: [PATCH] iommu/vt-d: Flush old iotlb for kdump when the device  gets context mapped Xunlei Pang <xpang@redhat.com> - 2016-11-30 09:20 +0100
      Re: [PATCH] iommu/vt-d: Flush old iotlb for kdump when the device  gets context mapped Baoquan He <bhe@redhat.com> - 2016-11-30 10:10 +0100
        Re: [PATCH] iommu/vt-d: Flush old iotlb for kdump when the device  gets context mapped Baoquan He <bhe@redhat.com> - 2016-11-30 11:00 +0100
          Re: [PATCH] iommu/vt-d: Flush old iotlb for kdump when the device  gets context mapped Baoquan He <bhe@redhat.com> - 2016-11-30 11:30 +0100
            Re: [PATCH] iommu/vt-d: Flush old iotlb for kdump when the device  gets context mapped Joerg Roedel <joro@8bytes.org> - 2016-11-30 15:30 +0100
              Re: [PATCH] iommu/vt-d: Flush old iotlb for kdump when the device  gets context mapped Xunlei Pang <xpang@redhat.com> - 2016-12-01 03:20 +0100
                Re: [PATCH] iommu/vt-d: Flush old iotlb for kdump when the device  gets context mapped Joerg Roedel <joro@8bytes.org> - 2016-12-01 11:40 +0100
                  Re: [PATCH] iommu/vt-d: Flush old iotlb for kdump when the device  gets context mapped Xunlei Pang <xpang@redhat.com> - 2016-12-01 13:00 +0100

#1532348 — Re: [PATCH] iommu/vt-d: Flush old iotlb for kdump when the device gets context mapped

On Thu, Nov 17, 2016 at 10:47:28AM +0800, Xunlei Pang wrote:
> As per the comment, the code here only needs to flush context caches
> for the special domain 0 which is used to tag the
> non-present/erroneous caches, seems we should flush the old domain id
> of present entries for kdump according to the analysis, other than the
> new-allocated domain id. Let me ponder more on this.

Flushing the context entry only is fine. The old domain-id will not be
re-used anyway, so there is no point in reading it out of the context
table and flush it.

Also, please add a Fixes-tag when you re-post this patch.


	Joerg

[toc] | [next] | [standalone]

#1533055

On 11/29/2016 at 10:35 PM, Joerg Roedel wrote:
> On Thu, Nov 17, 2016 at 10:47:28AM +0800, Xunlei Pang wrote:
>> As per the comment, the code here only needs to flush context caches
>> for the special domain 0 which is used to tag the
>> non-present/erroneous caches, seems we should flush the old domain id
>> of present entries for kdump according to the analysis, other than the
>> new-allocated domain id. Let me ponder more on this.
> Flushing the context entry only is fine. The old domain-id will not be
> re-used anyway, so there is no point in reading it out of the context
> table and flush it.

Do you mean to flush the context entry using the new-allocated domain id?

Yes, old domain-id will not be re-used as they were reserved when copy, but
may still be cached by in-flight DMA access.

Here is what the things seem to be from my understanding, and why I want to
flush using the old domain id:
1) In kdump mode, old tables are copied, and all the iommu caches are flushed.
2) There comes some in-flight DMA before the device's new context is mapped,
    so translation caches(context, iotlb, etc) are created tagging old domain-id
    in the iommu hardware.
3) At the driver probe stage, the device is reset , and no in-flight DMA will exist.
    Here I assumed that the device reset won't flush the old caches in the iommu
    hardware related to this device. I haven't found any relevant specification, please
    correct me if I am wrong.
4) Then new context is setup, and new DMA is initiated, hit old cache that was
    created in 2) as currently there's no such flush action, so DMAR fault happens.

I already posted v2 to flush context/iotlb using the old domain-id:
https://lkml.org/lkml/2016/11/18/514

Regards,
Xunlei

>
> Also, please add a Fixes-tag when you re-post this patch.
>
>
> 	Joerg
>

[toc] | [prev] | [next] | [standalone]

#1533090

On 11/30/16 at 04:15pm, Xunlei Pang wrote:
> On 11/29/2016 at 10:35 PM, Joerg Roedel wrote:
> > On Thu, Nov 17, 2016 at 10:47:28AM +0800, Xunlei Pang wrote:
> >> As per the comment, the code here only needs to flush context caches
> >> for the special domain 0 which is used to tag the
> >> non-present/erroneous caches, seems we should flush the old domain id
> >> of present entries for kdump according to the analysis, other than the
> >> new-allocated domain id. Let me ponder more on this.
> > Flushing the context entry only is fine. The old domain-id will not be
> > re-used anyway, so there is no point in reading it out of the context
> > table and flush it.
> 
> Do you mean to flush the context entry using the new-allocated domain id?
> 
> Yes, old domain-id will not be re-used as they were reserved when copy, but
> may still be cached by in-flight DMA access.

Joerg is saying you have flushed context entry which is the ingress,
new DMA can't get an entrance to hit the iotlb accordingly. Since you
have bolted the ingress gate. I guess

> 
> Here is what the things seem to be from my understanding, and why I want to
> flush using the old domain id:
> 1) In kdump mode, old tables are copied, and all the iommu caches are flushed.
> 2) There comes some in-flight DMA before the device's new context is mapped,
>     so translation caches(context, iotlb, etc) are created tagging old domain-id
>     in the iommu hardware.
> 3) At the driver probe stage, the device is reset , and no in-flight DMA will exist.
>     Here I assumed that the device reset won't flush the old caches in the iommu
>     hardware related to this device. I haven't found any relevant specification, please
>     correct me if I am wrong.
> 4) Then new context is setup, and new DMA is initiated, hit old cache that was
>     created in 2) as currently there's no such flush action, so DMAR fault happens.
> 
> I already posted v2 to flush context/iotlb using the old domain-id:
> https://lkml.org/lkml/2016/11/18/514
> 
> Regards,
> Xunlei
> 
> >
> > Also, please add a Fixes-tag when you re-post this patch.
> >
> >
> > 	Joerg
> >
> 
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec

[toc] | [prev] | [next] | [standalone]

#1533168

On 11/30/16 at 05:03pm, Baoquan He wrote:
> On 11/30/16 at 04:15pm, Xunlei Pang wrote:
> > On 11/29/2016 at 10:35 PM, Joerg Roedel wrote:
> > > On Thu, Nov 17, 2016 at 10:47:28AM +0800, Xunlei Pang wrote:
> > >> As per the comment, the code here only needs to flush context caches
> > >> for the special domain 0 which is used to tag the
> > >> non-present/erroneous caches, seems we should flush the old domain id
> > >> of present entries for kdump according to the analysis, other than the
> > >> new-allocated domain id. Let me ponder more on this.
> > > Flushing the context entry only is fine. The old domain-id will not be
> > > re-used anyway, so there is no point in reading it out of the context
> > > table and flush it.
> > 
> > Do you mean to flush the context entry using the new-allocated domain id?
> > 
> > Yes, old domain-id will not be re-used as they were reserved when copy, but
> > may still be cached by in-flight DMA access.
> 
> Joerg is saying you have flushed context entry which is the ingress,
> new DMA can't get an entrance to hit the iotlb accordingly. Since you
> have bolted the ingress gate. I guess

And please code comment at the bottom of iommu_init_domains(), you can
see domain 0 is a special domain id.


~~~~~~~~~~~~~~~~~~~~~~~~~
        /*
         * If Caching mode is set, then invalid translations are tagged
         * with domain-id 0, hence we need to pre-allocate it. We also
         * use domain-id 0 as a marker for non-allocated domain-id, so
         * make sure it is not used for a real domain.
         */
        set_bit(0, iommu->domain_ids);
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

And in vt-d spec, at the end of section 6.2.2 and the following
sections, you can see domain 0 is used to tag the cached entry.

I guess that's why it works with only domain 0 specified. The simple
thing to verify that is you specify another did, E.g 100 for your
flushing, see if it still works.


So, if it's just as above, v1 should be good enough.

Besides, you should use translation_pre_enabled(). If 1st kernel add
intel_iommu=off, no need to do this.

Thanks
Baoquan
> 
> > 
> > Here is what the things seem to be from my understanding, and why I want to
> > flush using the old domain id:
> > 1) In kdump mode, old tables are copied, and all the iommu caches are flushed.
> > 2) There comes some in-flight DMA before the device's new context is mapped,
> >     so translation caches(context, iotlb, etc) are created tagging old domain-id
> >     in the iommu hardware.
> > 3) At the driver probe stage, the device is reset , and no in-flight DMA will exist.
> >     Here I assumed that the device reset won't flush the old caches in the iommu
> >     hardware related to this device. I haven't found any relevant specification, please
> >     correct me if I am wrong.
> > 4) Then new context is setup, and new DMA is initiated, hit old cache that was
> >     created in 2) as currently there's no such flush action, so DMAR fault happens.
> > 
> > I already posted v2 to flush context/iotlb using the old domain-id:
> > https://lkml.org/lkml/2016/11/18/514
> > 
> > Regards,
> > Xunlei
> > 
> > >
> > > Also, please add a Fixes-tag when you re-post this patch.
> > >
> > >
> > > 	Joerg
> > >
> > 
> > 
> > _______________________________________________
> > kexec mailing list
> > kexec@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/kexec

[toc] | [prev] | [next] | [standalone]

#1533194

On 11/30/16 at 05:53pm, Baoquan He wrote:
> On 11/30/16 at 05:03pm, Baoquan He wrote:
> > On 11/30/16 at 04:15pm, Xunlei Pang wrote:
> > > On 11/29/2016 at 10:35 PM, Joerg Roedel wrote:
> > > > On Thu, Nov 17, 2016 at 10:47:28AM +0800, Xunlei Pang wrote:
> > > >> As per the comment, the code here only needs to flush context caches
> > > >> for the special domain 0 which is used to tag the
> > > >> non-present/erroneous caches, seems we should flush the old domain id
> > > >> of present entries for kdump according to the analysis, other than the
> > > >> new-allocated domain id. Let me ponder more on this.
> > > > Flushing the context entry only is fine. The old domain-id will not be
> > > > re-used anyway, so there is no point in reading it out of the context
> > > > table and flush it.
> > > 
> > > Do you mean to flush the context entry using the new-allocated domain id?
> > > 
> > > Yes, old domain-id will not be re-used as they were reserved when copy, but
> > > may still be cached by in-flight DMA access.
> > 
> > Joerg is saying you have flushed context entry which is the ingress,
> > new DMA can't get an entrance to hit the iotlb accordingly. Since you
> > have bolted the ingress gate. I guess
> 

OK, talked with Xunlei. The old cache could be entry with present bit
set.

> And please code comment at the bottom of iommu_init_domains(), you can
> see domain 0 is a special domain id.
> 
> 
> ~~~~~~~~~~~~~~~~~~~~~~~~~
>         /*
>          * If Caching mode is set, then invalid translations are tagged
>          * with domain-id 0, hence we need to pre-allocate it. We also
>          * use domain-id 0 as a marker for non-allocated domain-id, so
>          * make sure it is not used for a real domain.
>          */
>         set_bit(0, iommu->domain_ids);
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> And in vt-d spec, at the end of section 6.2.2 and the following
> sections, you can see domain 0 is used to tag the cached entry.
> 
> I guess that's why it works with only domain 0 specified. The simple
> thing to verify that is you specify another did, E.g 100 for your
> flushing, see if it still works.
> 
> 
> So, if it's just as above, v1 should be good enough.
> 
> Besides, you should use translation_pre_enabled(). If 1st kernel add
> intel_iommu=off, no need to do this.
> 
> Thanks
> Baoquan
> > 
> > > 
> > > Here is what the things seem to be from my understanding, and why I want to
> > > flush using the old domain id:
> > > 1) In kdump mode, old tables are copied, and all the iommu caches are flushed.
> > > 2) There comes some in-flight DMA before the device's new context is mapped,
> > >     so translation caches(context, iotlb, etc) are created tagging old domain-id
> > >     in the iommu hardware.
> > > 3) At the driver probe stage, the device is reset , and no in-flight DMA will exist.
> > >     Here I assumed that the device reset won't flush the old caches in the iommu
> > >     hardware related to this device. I haven't found any relevant specification, please
> > >     correct me if I am wrong.
> > > 4) Then new context is setup, and new DMA is initiated, hit old cache that was
> > >     created in 2) as currently there's no such flush action, so DMAR fault happens.
> > > 
> > > I already posted v2 to flush context/iotlb using the old domain-id:
> > > https://lkml.org/lkml/2016/11/18/514
> > > 
> > > Regards,
> > > Xunlei
> > > 
> > > >
> > > > Also, please add a Fixes-tag when you re-post this patch.
> > > >
> > > >
> > > > 	Joerg
> > > >
> > > 
> > > 
> > > _______________________________________________
> > > kexec mailing list
> > > kexec@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/kexec

[toc] | [prev] | [next] | [standalone]

#1533340

On Wed, Nov 30, 2016 at 06:23:34PM +0800, Baoquan He wrote:
> OK, talked with Xunlei. The old cache could be entry with present bit
> set.

-EPARSE

Anyway, what I was trying to say is, that the IOMMU TLB is tagged with
domain-ids, and that there is also a context-cache which maps device-ids
to domain-ids.

If we update the context entry then we need to flush only the context
entry, as it will point to a new domain-id then and future IOTLB lookups
in the IOMMU will be using the new domain-id and do not match the old
entries.



	Joerg

[toc] | [prev] | [next] | [standalone]

#1533748

On 11/30/2016 at 10:26 PM, Joerg Roedel wrote:
> On Wed, Nov 30, 2016 at 06:23:34PM +0800, Baoquan He wrote:
>> OK, talked with Xunlei. The old cache could be entry with present bit
>> set.
> -EPARSE
>
> Anyway, what I was trying to say is, that the IOMMU TLB is tagged with
> domain-ids, and that there is also a context-cache which maps device-ids
> to domain-ids.
>
> If we update the context entry then we need to flush only the context
> entry, as it will point to a new domain-id then and future IOTLB lookups
> in the IOMMU will be using the new domain-id and do not match the old
> entries.

Hi Joerg,

Thanks for the explanation, and we still need to flush context cache using old domain-id, right?
How about the following update?

index 3965e73..624eac9 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -2024,6 +2024,25 @@ static int domain_context_mapping_one(struct dmar_domain *domain,
        if (context_present(context))
                goto out_unlock;
 
+       /*
+        * For kdump cases, old valid entries may be cached due to the
+        * in-flight DMA and copied pgtable, but there is no unmapping
+        * behaviour for them, thus we need an explicit cache flush for
+        * the newly-mapped device. For kdump, at this point, the device
+        * is supposed to finish reset at its driver probe stage, so no
+        * in-flight DMA will exist, and we don't need to worry anymore
+        * hereafter.
+        */
+       if (context_copied(context)) {
+               u16 did_old = context_domain_id(context);
+
+               if (did_old >= 0 && did_old < cap_ndoms(iommu->cap))
+                       iommu->flush.flush_context(iommu, did_old,
+                                                  (((u16)bus) << 8) | devfn,
+                                                  DMA_CCMD_MASK_NOBIT,
+                                                  DMA_CCMD_DEVICE_INVL);
+       }
+
        pgd = domain->pgd;

[toc] | [prev] | [next] | [standalone]

#1533958

On Thu, Dec 01, 2016 at 10:15:45AM +0800, Xunlei Pang wrote:
> index 3965e73..624eac9 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -2024,6 +2024,25 @@ static int domain_context_mapping_one(struct dmar_domain *domain,
>         if (context_present(context))
>                 goto out_unlock;
>  
> +       /*
> +        * For kdump cases, old valid entries may be cached due to the
> +        * in-flight DMA and copied pgtable, but there is no unmapping
> +        * behaviour for them, thus we need an explicit cache flush for
> +        * the newly-mapped device. For kdump, at this point, the device
> +        * is supposed to finish reset at its driver probe stage, so no
> +        * in-flight DMA will exist, and we don't need to worry anymore
> +        * hereafter.
> +        */
> +       if (context_copied(context)) {
> +               u16 did_old = context_domain_id(context);
> +
> +               if (did_old >= 0 && did_old < cap_ndoms(iommu->cap))
> +                       iommu->flush.flush_context(iommu, did_old,
> +                                                  (((u16)bus) << 8) | devfn,
> +                                                  DMA_CCMD_MASK_NOBIT,
> +                                                  DMA_CCMD_DEVICE_INVL);
> +       }
> +
>         pgd = domain->pgd;

Yes, this looks better. Have you tested it the same way as the old
patch?


	Joerg

[toc] | [prev] | [next] | [standalone]

#1533997

On 12/01/2016 at 06:33 PM, Joerg Roedel wrote:
> On Thu, Dec 01, 2016 at 10:15:45AM +0800, Xunlei Pang wrote:
>> index 3965e73..624eac9 100644
>> --- a/drivers/iommu/intel-iommu.c
>> +++ b/drivers/iommu/intel-iommu.c
>> @@ -2024,6 +2024,25 @@ static int domain_context_mapping_one(struct dmar_domain *domain,
>>         if (context_present(context))
>>                 goto out_unlock;
>>  
>> +       /*
>> +        * For kdump cases, old valid entries may be cached due to the
>> +        * in-flight DMA and copied pgtable, but there is no unmapping
>> +        * behaviour for them, thus we need an explicit cache flush for
>> +        * the newly-mapped device. For kdump, at this point, the device
>> +        * is supposed to finish reset at its driver probe stage, so no
>> +        * in-flight DMA will exist, and we don't need to worry anymore
>> +        * hereafter.
>> +        */
>> +       if (context_copied(context)) {
>> +               u16 did_old = context_domain_id(context);
>> +
>> +               if (did_old >= 0 && did_old < cap_ndoms(iommu->cap))
>> +                       iommu->flush.flush_context(iommu, did_old,
>> +                                                  (((u16)bus) << 8) | devfn,
>> +                                                  DMA_CCMD_MASK_NOBIT,
>> +                                                  DMA_CCMD_DEVICE_INVL);
>> +       }
>> +
>>         pgd = domain->pgd;
> Yes, this looks better. Have you tested it the same way as the old
> patch?

Yes, I have tested and it works, will send v3 later.

Regards,
Xunlei

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web