diff mbox series

[v2,1/4] x86/mm: Export force_dma_unencrypted

Message ID 20190903131504.18935-2-thomas_os@shipmail.org (mailing list archive)
State New, archived
Headers show
Series Have TTM support SEV encryption with coherent memory | expand

Commit Message

Thomas Hellström (Intel) Sept. 3, 2019, 1:15 p.m. UTC
From: Thomas Hellstrom <thellstrom@vmware.com>

The force_dma_unencrypted symbol is needed by TTM to set up the correct
page protection when memory encryption is active. Export it.

Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Christian König <christian.koenig@amd.com>
Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
---
 arch/x86/mm/mem_encrypt.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Thomas Hellström (Intel) Sept. 3, 2019, 2:32 p.m. UTC | #1
Hi, Christoph,

On 9/3/19 3:46 PM, Christoph Hellwig wrote:
> On Tue, Sep 03, 2019 at 03:15:01PM +0200, Thomas Hellström (VMware) wrote:
>> From: Thomas Hellstrom <thellstrom@vmware.com>
>>
>> The force_dma_unencrypted symbol is needed by TTM to set up the correct
>> page protection when memory encryption is active. Export it.
> NAK.  This is a helper for the core DMA code and drivers have no
> business looking at it.

Is this a layer violation concern, that is, would you be ok with a 
similar helper for TTM, or is it that you want to force the graphics 
drivers into adhering strictly to the DMA api, even when it from an 
engineering perspective makes no sense?

If it's the latter, then I would like to reiterate that it would be 
better that we work to come up with a long term plan to add what's 
missing to the DMA api to help graphics drivers use coherent memory?

Thanks,

Thomas
Dave Hansen Sept. 3, 2019, 3:14 p.m. UTC | #2
On 9/3/19 6:15 AM, Thomas Hellström (VMware) wrote:
> The force_dma_unencrypted symbol is needed by TTM to set up the correct
> page protection when memory encryption is active. Export it.

It would be great if this had enough background that I didn't have to
look at patch 4 to figure out what TTM might be.

Why is TTM special?  How many other drivers would have to be modified in
a one-off fashion if we go this way?  What's the logic behind this being
a non-GPL export?
Thomas Hellström (Intel) Sept. 3, 2019, 6:50 p.m. UTC | #3
On 9/3/19 5:14 PM, Dave Hansen wrote:
> On 9/3/19 6:15 AM, Thomas Hellström (VMware) wrote:
>> The force_dma_unencrypted symbol is needed by TTM to set up the correct
>> page protection when memory encryption is active. Export it.
> It would be great if this had enough background that I didn't have to
> look at patch 4 to figure out what TTM might be.
>
> Why is TTM special?  How many other drivers would have to be modified in
> a one-off fashion if we go this way?  What's the logic behind this being
> a non-GPL export?

TTM tries to abstract mapping of graphics buffer objects regardless 
where they live. Be it in pci memory or system memory. As such it needs 
to figure out the proper page protection. For example if a buffer object 
is moved from pci memory to system memory transparently to a user-space 
application, all user-space mappings need to be killed and then 
reinstated pointing to the new location, sometimes with a new page 
protection.

I try to keep away as much as possible from the non-GPL vs GPL export 
discussions. I have no strong opinion on the subject. Although since 
sev_active() is a non-GPL export, I decided to mimic that.

Thanks
Thomas
Thomas Hellström (Intel) Sept. 3, 2019, 8:46 p.m. UTC | #4
On 9/3/19 6:22 PM, Christoph Hellwig wrote:
> On Tue, Sep 03, 2019 at 04:32:45PM +0200, Thomas Hellström (VMware) wrote:
>> Is this a layer violation concern, that is, would you be ok with a similar
>> helper for TTM, or is it that you want to force the graphics drivers into
>> adhering strictly to the DMA api, even when it from an engineering
>> perspective makes no sense?
> >From looking at DRM I strongly believe that making DRM use the DMA
> mapping properly makes a lot of sense from the engineering perspective,
> and this series is a good argument for that positions.

What I mean with "from an engineering perspective" is that drivers would 
end up with a non-trivial amount of code supporting purely academic 
cases: Setups where software rendering would be faster than gpu 
accelerated, and setups on platforms where the driver would never run 
anyway because the device would never be supported on that platform...

>   If DRM was using
> the DMA properl we would not need this series to start with, all the
> SEV handling is hidden behind the DMA API.  While we had occasional
> bugs in that support fixing it meant that it covered all drivers
> properly using that API.

That is not really true. The dma API can't handle faulting of coherent 
pages which is what this series is really all about supporting also with 
SEV active. To handle the case where we move graphics buffers or send 
them to swap space while user-space have them mapped.

To do that and still be fully dma-api compliant we would ideally need, 
for example, an exported dma_pgprot(). (dma_pgprot() by the way is still 
suffering from one of the bugs that you mention above).

Still, I need a way forward and my questions weren't really answered by 
this.

/Thomas
Andy Lutomirski Sept. 3, 2019, 9:41 p.m. UTC | #5
On Tue, Sep 3, 2019 at 1:46 PM Thomas Hellström (VMware)
<thomas_os@shipmail.org> wrote:
>
> On 9/3/19 6:22 PM, Christoph Hellwig wrote:
> > On Tue, Sep 03, 2019 at 04:32:45PM +0200, Thomas Hellström (VMware) wrote:
> >> Is this a layer violation concern, that is, would you be ok with a similar
> >> helper for TTM, or is it that you want to force the graphics drivers into
> >> adhering strictly to the DMA api, even when it from an engineering
> >> perspective makes no sense?
> > >From looking at DRM I strongly believe that making DRM use the DMA
> > mapping properly makes a lot of sense from the engineering perspective,
> > and this series is a good argument for that positions.
>
> What I mean with "from an engineering perspective" is that drivers would
> end up with a non-trivial amount of code supporting purely academic
> cases: Setups where software rendering would be faster than gpu
> accelerated, and setups on platforms where the driver would never run
> anyway because the device would never be supported on that platform...
>
> >   If DRM was using
> > the DMA properl we would not need this series to start with, all the
> > SEV handling is hidden behind the DMA API.  While we had occasional
> > bugs in that support fixing it meant that it covered all drivers
> > properly using that API.
>
> That is not really true. The dma API can't handle faulting of coherent
> pages which is what this series is really all about supporting also with
> SEV active. To handle the case where we move graphics buffers or send
> them to swap space while user-space have them mapped.
>
> To do that and still be fully dma-api compliant we would ideally need,
> for example, an exported dma_pgprot(). (dma_pgprot() by the way is still
> suffering from one of the bugs that you mention above).
>
> Still, I need a way forward and my questions weren't really answered by
> this.
>
>

I read this patch, I read force_dma_encrypted(), I read the changelog
again, and I haven't the faintest clue what TTM could possibly be
doing with force_dma_encrypted().

You're saying that TTM needs to transparently change mappings to
relocate objects in memory between system memory and device memory.
Great, I don't see the problem.  Is the issue that you need to
allocate system memory that is addressable by the GPU and that, if the
GPU has insufficient PA bits, you need unencrypted memory?  If so,
this sounds like an excellent use for the DMA API.   Rather than
kludging directly knowledge of force_dma_encrypted() into the driver,
can't you at least add, if needed, a new helper specifically to
allocate memory that can be addressed by the device?  Like
dma_alloc_coherent()?  Or, if for some reason, dma_alloc_coherent()
doesn't do what you need or your driver isn't ready to use it, then
explain *why* and introduce a new function to solve your problem?

Keep in mind that, depending on just how MKTME ends up being supported
in Linux, it's entirely possible that it will be *backwards* from what
you expect -- high address bits will be needed to ask for
*unencrypted* memory.

--Andy
Thomas Hellström (Intel) Sept. 4, 2019, 7:32 a.m. UTC | #6
On 9/4/19 8:58 AM, Christoph Hellwig wrote:
> On Tue, Sep 03, 2019 at 10:46:18PM +0200, Thomas Hellström (VMware) wrote:
>> What I mean with "from an engineering perspective" is that drivers would end
>> up with a non-trivial amount of code supporting purely academic cases:
>> Setups where software rendering would be faster than gpu accelerated, and
>> setups on platforms where the driver would never run anyway because the
>> device would never be supported on that platform...
> And actually work on cases you previously called academic and which now
> matter to you because your employer has a suddent interest in SEV.
> Academic really is in the eye of the beholder (and of those who pay
> the bills).

But in this particular case we *do* adhere to the dma api, at least as 
far as we can. But we're missing functionality.

>
>> That is not really true. The dma API can't handle faulting of coherent pages
>> which is what this series is really all about supporting also with SEV
>> active. To handle the case where we move graphics buffers or send them to
>> swap space while user-space have them mapped.
> And the only thing we need to support the fault handler is to add an
> offset to the dma_mmap_* APIs.  Which I had planned to do for Christian
> (one of the few grapics developers who actually tries to play well
> with the rest of the kernel instead of piling hacks over hacks like
> many others) anyway, but which hasn't happened yet.

That sounds great. Is there anything I can do to help out? I thought 
this was more or less a dead end since the current dma_mmap_ API 
requires the mmap_sem to be held in write mode (modifying the 
vma->vm_flags) whereas fault() only offers read mode. But that would 
definitely work.

>
>> Still, I need a way forward and my questions weren't really answered by
>> this.
> This is pretty demanding.  If you "need" a way forward just work with
> all the relevant people instead of piling ob local hacks.

But I think that was what I was trying to initiate. The question was

"If it's the latter, then I would like to reiterate that it would be 
better that we work to come up with a long term plan to add what's 
missing to the DMA api to help graphics drivers use coherent memory?"

And since you NAK'd the original patches, I was sort of hoping for a 
point in the right direction.

Thanks,

Thomas
Thomas Hellström (Intel) Sept. 4, 2019, 5:28 p.m. UTC | #7
On 9/4/19 2:22 PM, Christoph Hellwig wrote:
> On Wed, Sep 04, 2019 at 09:32:30AM +0200, Thomas Hellström (VMware) wrote:
>> That sounds great. Is there anything I can do to help out? I thought this
>> was more or less a dead end since the current dma_mmap_ API requires the
>> mmap_sem to be held in write mode (modifying the vma->vm_flags) whereas
>> fault() only offers read mode. But that would definitely work.
> We'll just need to split into a setup and faul phase.  I have some
> sketches from a while ago, let me dust them off so that you can
> try them.

I'd be happy to.

Thanks,

Thomas
diff mbox series

Patch

diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
index fece30ca8b0c..bbfe8802d63a 100644
--- a/arch/x86/mm/mem_encrypt.c
+++ b/arch/x86/mm/mem_encrypt.c
@@ -377,6 +377,7 @@  bool force_dma_unencrypted(struct device *dev)
 
 	return false;
 }
+EXPORT_SYMBOL(force_dma_unencrypted);
 
 /* Architecture __weak replacement functions */
 void __init mem_encrypt_free_decrypted_mem(void)