Regression due to d98849aff879 (dma-direct: handle DMA_ATTR_NO_KERNEL_MAPPING in common code)

Regression due to d98849aff879 (dma-direct: handle DMA_ATTR_NO_KERNEL_MAPPING in common code)

[Lucas Stach]

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
_______________________________________________
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)

[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,
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)

[Lucas Stach]

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)

[Lendacky, Thomas]

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;
>  	}
>  
> 
_______________________________________________
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)

[Christoph Hellwig]

Ok, does this work?

--
From 34d35f335a98f515f2516b515051e12eae744c8d Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
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 <hch@lst.de>
---
 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)

[Lucas Stach]

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 <hch@lst.de>
> 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 <hch@lst.de>
> ---
>  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)

[Lendacky, Thomas]

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 <hch@lst.de>
>> 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 <hch@lst.de>
>> ---
>>  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;
>>>  	}
>>  
_______________________________________________
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)

[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 <hch@lst.de>
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 <hch@lst.de>
---
 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)

[Christoph Hellwig]

On Tue, Aug 06, 2019 at 02:18:49PM +0000, 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)

[Lendacky, Thomas]

On 8/6/19 10:46 AM, Christoph Hellwig wrote:
> On Tue, Aug 06, 2019 at 02:18:49PM +0000, 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)

[Christoph Hellwig]

On Tue, Aug 06, 2019 at 03:59:40PM +0000, 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)

[Lucas Stach]

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 <hch@lst.de>
> 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 <hch@lst.de>
> ---
>  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 <l.stach@pengutronix.de>


>  		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)

[Christoph Hellwig]

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 <l.stach@pengutronix.de>

Thanks, I've applied it with that fixed up.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu