Re: Regression due to d98849aff879 (dma-direct: handle DMA_ATTR_NO_KERNEL_MAPPING in common code)
On Wed, Aug 07, 2019 at 05:24:17PM +0200, Lucas Stach wrote: > I would suggest to place this line above the comment, as the comment > only really applies to the return value. Other than this nitpick, this > matches my understanding of the required changes, so: > > Reviewed-by: Lucas Stach Thanks, I've applied it with that fixed up. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: Regression due to d98849aff879 (dma-direct: handle DMA_ATTR_NO_KERNEL_MAPPING in common code)
Am Dienstag, den 06.08.2019, 17:44 +0200 schrieb Christoph Hellwig: > On Tue, Aug 06, 2019 at 04:06:58PM +0200, Lucas Stach wrote: > > > > dma_direct_free_pages() then needs the same check, as otherwise the cpu > > address is treated as a cookie instead of a real address and the > > encryption needs to be re-enabled. > > Ok, lets try this one instead: > > -- > From 3a7aa9fe38a5eae5d879831b4f8c1032e735a0b6 Mon Sep 17 00:00:00 2001 > From: Christoph Hellwig > Date: Tue, 6 Aug 2019 14:33:23 +0300 > Subject: dma-direct: fix DMA_ATTR_NO_KERNEL_MAPPING > > The new DMA_ATTR_NO_KERNEL_MAPPING needs to actually assign > a dma_addr to work. Also skip it if the architecture needs > forced decryption handling, as that needs a kernel virtual > address. > > Fixes: d98849aff879 (dma-direct: handle DMA_ATTR_NO_KERNEL_MAPPING in common > code) > Signed-off-by: Christoph Hellwig > --- > kernel/dma/direct.c | 7 +-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c > index 59bdceea3737..4c211c87a719 100644 > --- a/kernel/dma/direct.c > +++ b/kernel/dma/direct.c > @@ -130,11 +130,13 @@ void *dma_direct_alloc_pages(struct device *dev, size_t > size, > if (!page) > return NULL; > > - if (attrs & DMA_ATTR_NO_KERNEL_MAPPING) { > + if ((attrs & DMA_ATTR_NO_KERNEL_MAPPING) && > + !force_dma_unencrypted(dev)) { > /* remove any dirty cache lines on the kernel alias */ > if (!PageHighMem(page)) > arch_dma_prep_coherent(page, size); > /* return the page pointer as the opaque cookie */ > + *dma_handle = phys_to_dma(dev, page_to_phys(page)); I would suggest to place this line above the comment, as the comment only really applies to the return value. Other than this nitpick, this matches my understanding of the required changes, so: Reviewed-by: Lucas Stach > return page; > } > > @@ -178,7 +180,8 @@ void dma_direct_free_pages(struct device *dev, size_t > size, void *cpu_addr, > { > unsigned int page_order = get_order(size); > > - if (attrs & DMA_ATTR_NO_KERNEL_MAPPING) { > + if ((attrs & DMA_ATTR_NO_KERNEL_MAPPING) && > + !force_dma_unencrypted(dev)) { > /* cpu_addr is a struct page cookie, not a kernel address */ > __dma_direct_free_pages(dev, size, cpu_addr); > return; ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: Regression due to d98849aff879 (dma-direct: handle DMA_ATTR_NO_KERNEL_MAPPING in common code)
On Tue, Aug 06, 2019 at 03:59:40PM +, Lendacky, Thomas wrote: > As long as two different cookie types (page pointer for encrypted DMA > and virtual address returned from page_address() for unencrypted DMA) > is ok. I'm just not familiar with how the cookie is used in any other > functions, if at all. DMA_ATTR_NO_KERNEL_MAPPING is intended for memory never used in the kernel, either because it is just a buffer for device that are too cheap to enough dram, or because it is a buffer for userspace to device communication that the kernel just mediates. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: Regression due to d98849aff879 (dma-direct: handle DMA_ATTR_NO_KERNEL_MAPPING in common code)
On 8/6/19 10:46 AM, Christoph Hellwig wrote: > On Tue, Aug 06, 2019 at 02:18:49PM +, Lendacky, Thomas wrote: >> I think you need to keep everything inside the original if statement since >> the caller is expecting a page pointer to be returned in this case and not >> the page_address() which is returned when the DMA_ATTR_NO_KERNEL_MAPPING >> is not present. > > DMA_ATTR_NO_KERNEL_MAPPING is defined to return an opaque cookie, > which just happens to be a page pointer. So if we fix up the free side > as pointed out by Lucas we should be fine. As long as two different cookie types (page pointer for encrypted DMA and virtual address returned from page_address() for unencrypted DMA) is ok. I'm just not familiar with how the cookie is used in any other functions, if at all. Thanks, Tom > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: Regression due to d98849aff879 (dma-direct: handle DMA_ATTR_NO_KERNEL_MAPPING in common code)
On Tue, Aug 06, 2019 at 02:18:49PM +, Lendacky, Thomas wrote: > I think you need to keep everything inside the original if statement since > the caller is expecting a page pointer to be returned in this case and not > the page_address() which is returned when the DMA_ATTR_NO_KERNEL_MAPPING > is not present. DMA_ATTR_NO_KERNEL_MAPPING is defined to return an opaque cookie, which just happens to be a page pointer. So if we fix up the free side as pointed out by Lucas we should be fine. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: Regression due to d98849aff879 (dma-direct: handle DMA_ATTR_NO_KERNEL_MAPPING in common code)
On Tue, Aug 06, 2019 at 04:06:58PM +0200, Lucas Stach wrote: > > dma_direct_free_pages() then needs the same check, as otherwise the cpu > address is treated as a cookie instead of a real address and the > encryption needs to be re-enabled. Ok, lets try this one instead: -- >From 3a7aa9fe38a5eae5d879831b4f8c1032e735a0b6 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Tue, 6 Aug 2019 14:33:23 +0300 Subject: dma-direct: fix DMA_ATTR_NO_KERNEL_MAPPING The new DMA_ATTR_NO_KERNEL_MAPPING needs to actually assign a dma_addr to work. Also skip it if the architecture needs forced decryption handling, as that needs a kernel virtual address. Fixes: d98849aff879 (dma-direct: handle DMA_ATTR_NO_KERNEL_MAPPING in common code) Signed-off-by: Christoph Hellwig --- kernel/dma/direct.c | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c index 59bdceea3737..4c211c87a719 100644 --- a/kernel/dma/direct.c +++ b/kernel/dma/direct.c @@ -130,11 +130,13 @@ void *dma_direct_alloc_pages(struct device *dev, size_t size, if (!page) return NULL; - if (attrs & DMA_ATTR_NO_KERNEL_MAPPING) { + if ((attrs & DMA_ATTR_NO_KERNEL_MAPPING) && + !force_dma_unencrypted(dev)) { /* remove any dirty cache lines on the kernel alias */ if (!PageHighMem(page)) arch_dma_prep_coherent(page, size); /* return the page pointer as the opaque cookie */ + *dma_handle = phys_to_dma(dev, page_to_phys(page)); return page; } @@ -178,7 +180,8 @@ void dma_direct_free_pages(struct device *dev, size_t size, void *cpu_addr, { unsigned int page_order = get_order(size); - if (attrs & DMA_ATTR_NO_KERNEL_MAPPING) { + if ((attrs & DMA_ATTR_NO_KERNEL_MAPPING) && + !force_dma_unencrypted(dev)) { /* cpu_addr is a struct page cookie, not a kernel address */ __dma_direct_free_pages(dev, size, cpu_addr); return; -- 2.20.1 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: Regression due to d98849aff879 (dma-direct: handle DMA_ATTR_NO_KERNEL_MAPPING in common code)
On 8/6/19 9:06 AM, Lucas Stach wrote: > Am Dienstag, den 06.08.2019, 16:04 +0200 schrieb Christoph Hellwig: >> Ok, does this work? >> >> -- >> From 34d35f335a98f515f2516b515051e12eae744c8d Mon Sep 17 00:00:00 2001 >>> From: Christoph Hellwig >> Date: Tue, 6 Aug 2019 14:33:23 +0300 >> Subject: dma-direct: fix DMA_ATTR_NO_KERNEL_MAPPING >> >> The new DMA_ATTR_NO_KERNEL_MAPPING needs to actually assign >> a dma_addr to work. Also skip it if the architecture needs >> forced decryption handling, as that needs a kernel virtual >> address. >> >> Fixes: d98849aff879 (dma-direct: handle DMA_ATTR_NO_KERNEL_MAPPING in common >> code) >>> Signed-off-by: Christoph Hellwig >> --- >> kernel/dma/direct.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c >> index 59bdceea3737..b01064d884f2 100644 >> --- a/kernel/dma/direct.c >> +++ b/kernel/dma/direct.c >> @@ -130,11 +130,13 @@ void *dma_direct_alloc_pages(struct device *dev, >> size_t size, >>> if (!page) >>> return NULL; >> >>> - if (attrs & DMA_ATTR_NO_KERNEL_MAPPING) { >>> + if ((attrs & DMA_ATTR_NO_KERNEL_MAPPING) && >> +!force_dma_unencrypted(dev)) { I think you need to keep everything inside the original if statement since the caller is expecting a page pointer to be returned in this case and not the page_address() which is returned when the DMA_ATTR_NO_KERNEL_MAPPING is not present. > > dma_direct_free_pages() then needs the same check, as otherwise the cpu Agreed. And the cpu_addr passed in here will be the page pointer, so will need to keep everything inside the if check of the DMA_ATTR_NO_KERNEL_MAPPING attr here as well. Thanks, Tom > address is treated as a cookie instead of a real address and the > encryption needs to be re-enabled. > > Regards, > Lucas > >> /* remove any dirty cache lines on the kernel alias */ >>> if (!PageHighMem(page)) >>> arch_dma_prep_coherent(page, size); >>> /* return the page pointer as the opaque cookie */ >>> + *dma_handle = phys_to_dma(dev, page_to_phys(page)); >>> return page; >>> } >>
Re: Regression due to d98849aff879 (dma-direct: handle DMA_ATTR_NO_KERNEL_MAPPING in common code)
Am Dienstag, den 06.08.2019, 16:04 +0200 schrieb Christoph Hellwig: > Ok, does this work? > > -- > From 34d35f335a98f515f2516b515051e12eae744c8d Mon Sep 17 00:00:00 2001 > > From: Christoph Hellwig > Date: Tue, 6 Aug 2019 14:33:23 +0300 > Subject: dma-direct: fix DMA_ATTR_NO_KERNEL_MAPPING > > The new DMA_ATTR_NO_KERNEL_MAPPING needs to actually assign > a dma_addr to work. Also skip it if the architecture needs > forced decryption handling, as that needs a kernel virtual > address. > > Fixes: d98849aff879 (dma-direct: handle DMA_ATTR_NO_KERNEL_MAPPING in common > code) > > Signed-off-by: Christoph Hellwig > --- > kernel/dma/direct.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c > index 59bdceea3737..b01064d884f2 100644 > --- a/kernel/dma/direct.c > +++ b/kernel/dma/direct.c > @@ -130,11 +130,13 @@ void *dma_direct_alloc_pages(struct device *dev, size_t > size, > > if (!page) > > return NULL; > > > - if (attrs & DMA_ATTR_NO_KERNEL_MAPPING) { > > + if ((attrs & DMA_ATTR_NO_KERNEL_MAPPING) && > + !force_dma_unencrypted(dev)) { dma_direct_free_pages() then needs the same check, as otherwise the cpu address is treated as a cookie instead of a real address and the encryption needs to be re-enabled. Regards, Lucas > /* remove any dirty cache lines on the kernel alias */ > > if (!PageHighMem(page)) > > arch_dma_prep_coherent(page, size); > > /* return the page pointer as the opaque cookie */ > > + *dma_handle = phys_to_dma(dev, page_to_phys(page)); > > return page; > > } > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: Regression due to d98849aff879 (dma-direct: handle DMA_ATTR_NO_KERNEL_MAPPING in common code)
Ok, does this work? -- >From 34d35f335a98f515f2516b515051e12eae744c8d Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Tue, 6 Aug 2019 14:33:23 +0300 Subject: dma-direct: fix DMA_ATTR_NO_KERNEL_MAPPING The new DMA_ATTR_NO_KERNEL_MAPPING needs to actually assign a dma_addr to work. Also skip it if the architecture needs forced decryption handling, as that needs a kernel virtual address. Fixes: d98849aff879 (dma-direct: handle DMA_ATTR_NO_KERNEL_MAPPING in common code) Signed-off-by: Christoph Hellwig --- kernel/dma/direct.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c index 59bdceea3737..b01064d884f2 100644 --- a/kernel/dma/direct.c +++ b/kernel/dma/direct.c @@ -130,11 +130,13 @@ void *dma_direct_alloc_pages(struct device *dev, size_t size, if (!page) return NULL; - if (attrs & DMA_ATTR_NO_KERNEL_MAPPING) { + if ((attrs & DMA_ATTR_NO_KERNEL_MAPPING) && + !force_dma_unencrypted(dev)) { /* remove any dirty cache lines on the kernel alias */ if (!PageHighMem(page)) arch_dma_prep_coherent(page, size); /* return the page pointer as the opaque cookie */ + *dma_handle = phys_to_dma(dev, page_to_phys(page)); return page; } -- 2.20.1 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: Regression due to d98849aff879 (dma-direct: handle DMA_ATTR_NO_KERNEL_MAPPING in common code)
On 8/6/19 6:33 AM, Christoph Hellwig wrote: > On Tue, Aug 06, 2019 at 11:13:29AM +0200, Lucas Stach wrote: >> Hi Christoph, >> >> I just found a regression where my NVMe device is no longer able to set >> up its HMB. >> >> After subject commit dma_direct_alloc_pages() is no longer initializing >> dma_handle properly when DMA_ATTR_NO_KERNEL_MAPPING is set, as the >> function is now returning too early. >> >> Now this could easily be fixed by adding the phy_to_dma translation to >> the NO_KERNEL_MAPPING code path, but I'm not sure how this stuff >> interacts with the memory encryption stuff set up later in the >> function, so I guess this should be looked at by someone with more >> experience with this code than me. > > There is not much we can do about the memory encryption case here, > as that requires a kernel address to mark the memory as unencrypted. > > So the obvious trivial fix is probably the right one: This will present problems under SEV (probably not SME unless the DMA mask doesn't support 48-bit DMA) when an NVMe device is passed through. The Documentation states that DMA_ATTR_NO_KERNEL_MAPPING is to avoid creating the mapping because of time and resources that may be involved on some archs. Would it make sense to check for memory encryption using force_dma_unencrypted() and override the flag in those cases? Does x86 have issues where this flag is needed? It could be set that the mapping is only generated if you have to force an unencrypted DMA. The code isn't as clean and you would have to hit the dma_direct_free_pages() path, also. I suspect Power and s390 may have the same concerns here (adding them on Cc: just in case). Thanks, Tom > > > diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c > index 59bdceea3737..c49120193309 100644 > --- a/kernel/dma/direct.c > +++ b/kernel/dma/direct.c > @@ -135,6 +135,7 @@ void *dma_direct_alloc_pages(struct device *dev, size_t > size, > if (!PageHighMem(page)) > arch_dma_prep_coherent(page, size); > /* return the page pointer as the opaque cookie */ > + *dma_handle = phys_to_dma(dev, page_to_phys(page)); > return page; > } > >
Re: Regression due to d98849aff879 (dma-direct: handle DMA_ATTR_NO_KERNEL_MAPPING in common code)
Am Dienstag, den 06.08.2019, 13:33 +0200 schrieb Christoph Hellwig: > On Tue, Aug 06, 2019 at 11:13:29AM +0200, Lucas Stach wrote: > > Hi Christoph, > > > > I just found a regression where my NVMe device is no longer able to > > set > > up its HMB. > > > > After subject commit dma_direct_alloc_pages() is no longer > > initializing > > dma_handle properly when DMA_ATTR_NO_KERNEL_MAPPING is set, as the > > function is now returning too early. > > > > Now this could easily be fixed by adding the phy_to_dma translation > > to > > the NO_KERNEL_MAPPING code path, but I'm not sure how this stuff > > interacts with the memory encryption stuff set up later in the > > function, so I guess this should be looked at by someone with more > > experience with this code than me. > > There is not much we can do about the memory encryption case here, Which I would guess means we need to ignore DMA_ATTR_NO_KERNEL_MAPPING in that case instead of dropping out early? > as that requires a kernel address to mark the memory as unencrypted. > > So the obvious trivial fix is probably the right one: > > > diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c > index 59bdceea3737..c49120193309 100644 > --- a/kernel/dma/direct.c > +++ b/kernel/dma/direct.c > @@ -135,6 +135,7 @@ void *dma_direct_alloc_pages(struct device *dev, > size_t size, > if (!PageHighMem(page)) > arch_dma_prep_coherent(page, size); > /* return the page pointer as the opaque cookie */ > + *dma_handle = phys_to_dma(dev, page_to_phys(page)); > return page; > } > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: Regression due to d98849aff879 (dma-direct: handle DMA_ATTR_NO_KERNEL_MAPPING in common code)
On Tue, Aug 06, 2019 at 11:13:29AM +0200, Lucas Stach wrote: > Hi Christoph, > > I just found a regression where my NVMe device is no longer able to set > up its HMB. > > After subject commit dma_direct_alloc_pages() is no longer initializing > dma_handle properly when DMA_ATTR_NO_KERNEL_MAPPING is set, as the > function is now returning too early. > > Now this could easily be fixed by adding the phy_to_dma translation to > the NO_KERNEL_MAPPING code path, but I'm not sure how this stuff > interacts with the memory encryption stuff set up later in the > function, so I guess this should be looked at by someone with more > experience with this code than me. There is not much we can do about the memory encryption case here, as that requires a kernel address to mark the memory as unencrypted. So the obvious trivial fix is probably the right one: diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c index 59bdceea3737..c49120193309 100644 --- a/kernel/dma/direct.c +++ b/kernel/dma/direct.c @@ -135,6 +135,7 @@ void *dma_direct_alloc_pages(struct device *dev, size_t size, if (!PageHighMem(page)) arch_dma_prep_coherent(page, size); /* return the page pointer as the opaque cookie */ + *dma_handle = phys_to_dma(dev, page_to_phys(page)); return page; }
Regression due to d98849aff879 (dma-direct: handle DMA_ATTR_NO_KERNEL_MAPPING in common code)
Hi Christoph, I just found a regression where my NVMe device is no longer able to set up its HMB. After subject commit dma_direct_alloc_pages() is no longer initializing dma_handle properly when DMA_ATTR_NO_KERNEL_MAPPING is set, as the function is now returning too early. Now this could easily be fixed by adding the phy_to_dma translation to the NO_KERNEL_MAPPING code path, but I'm not sure how this stuff interacts with the memory encryption stuff set up later in the function, so I guess this should be looked at by someone with more experience with this code than me. Regards, Lucas