Re: Decrypting tt maps in ttm

Re: Decrypting tt maps in ttm

[Thomas Hellström]

On 9/18/23 17:52, Zack Rusin wrote:
> On Mon, 2023-09-18 at 17:13 +0200, Thomas Hellström wrote:
>> Hi,
>>
>> On 9/18/23 16:56, Thomas Hellström wrote:
>>> Hi Zack, Christian
>>>
>>> On 9/18/23 13:36, Christian König wrote:
>>>> Hi Zack,
>>>>
>>>> adding Thomas and Daniel.
>>>>
>>>> I briefly remember that I talked with Thomas and some other people
>>>> about that quite a while ago as well, but I don't fully remember the
>>>> outcome.
>>> Found one old thread, but didn't read it:
>>>
>>> https://lists.freedesktop.org/archives/dri-devel/2019-September/234100.html
>>>
>>>
>>> /Thomas
>>>
>>>
>> Ugh. Now starting to read that thread I have a vague recollection it all
>> ended with not supporting mapping any device pages whatsoever when SEV
>> was enabled, but rather resorting to llvmpipe and VM-local bos.
> Hi, Thomas.
>
> Thanks for finding this! I'd (of course) like to solve it properly and get vmwgfx
> running with 3d support with SEV-ES active instead of essentially disabling the
> driver when SEV-ES is active.
>
> I think there are two separate discussions there, the non-controversial one and the
> controversial one:
> 1) The non-controversial: is there a case where drivers would want encrypted memory
> for TT pages but not for io mem mappings? Because if not then as Christian pointed
> out we could just add pgprot_decrypted to ttm_io_prot and be essentially done. The
> current method of decrypting io mem but leaving sys mem mappings encrypted is a bit
> weird anyway.
>
> If the answer to that question is "yes, some driver does want the TT mappings to be
> encrypted" then your "[PATCH v2 3/4] drm/ttm, drm/vmwgfx: Correctly support support
> AMD memory encryption" solves that. I think getting one of those two in makes sense
> regardless of everything else, agreed?

Well, there is more to it I think.

IIRC, the AMD SME encryption mode has a way for a device to have the 
memory controller (?) encrypt / decrypt device traffic by using an 
address range alias, so in theory it supports encrypted TT pages, and 
the dma-layer may indeed hand encrypted DMA pages to TTM on such systems 
depending on the device's DMA mask. That's why I think that 
force_dma_unencrypted() export was needed, and If the amdgpu driver 
accesses TT memory in SME mode *without* pgprot_decrypted() and it still 
works, then I think that mode is actually used. How could it otherwise work?

But anyway, I agree SEV-ES mode would *always* want pgprot_decrypted for 
dma allocated memory, whether it's in PL_SYSTEM or in PL_TT, but I guess 
the above SME case need to be sorted out first.

>
> 2) The controversial part of your series seems to be exporting of
> force_dma_unencrypted and its usage within ttm. Personally I'm perfectly ok with
> ignoring that for now. Getting #1 in is still a million times (rounded up to the
> nearest million) faster than just using llvmpipe in the guest. And getting #1 also
> fixes modern presentation on vmwgfx.


>
> So #1 is "make the driver work", while #2 is an optimisation - I think we should
> treat them as separate things/series. Does that make sense?

I guess that depends on the tradeoff between security and speed for 
people running SEV-enabled VMs. But giving them a choice is probably not 
a bad idea.

/Thomas


>
> z
>
> P.S. I forgot to cc dri-devel on the initial email, that wasn't by design. Sorry.
> I'm ok with moving the discussion to dri-devel.

Re: Decrypting tt maps in ttm

[Alex Deucher]

On Mon, Sep 18, 2023 at 3:06 PM Thomas Hellström
<thomas.hellstrom@linux.intel.com> wrote:
>
>
> On 9/18/23 17:52, Zack Rusin wrote:
> > On Mon, 2023-09-18 at 17:13 +0200, Thomas Hellström wrote:
> >> Hi,
> >>
> >> On 9/18/23 16:56, Thomas Hellström wrote:
> >>> Hi Zack, Christian
> >>>
> >>> On 9/18/23 13:36, Christian König wrote:
> >>>> Hi Zack,
> >>>>
> >>>> adding Thomas and Daniel.
> >>>>
> >>>> I briefly remember that I talked with Thomas and some other people
> >>>> about that quite a while ago as well, but I don't fully remember the
> >>>> outcome.
> >>> Found one old thread, but didn't read it:
> >>>
> >>> https://lists.freedesktop.org/archives/dri-devel/2019-September/234100.html
> >>>
> >>>
> >>> /Thomas
> >>>
> >>>
> >> Ugh. Now starting to read that thread I have a vague recollection it all
> >> ended with not supporting mapping any device pages whatsoever when SEV
> >> was enabled, but rather resorting to llvmpipe and VM-local bos.
> > Hi, Thomas.
> >
> > Thanks for finding this! I'd (of course) like to solve it properly and get vmwgfx
> > running with 3d support with SEV-ES active instead of essentially disabling the
> > driver when SEV-ES is active.
> >
> > I think there are two separate discussions there, the non-controversial one and the
> > controversial one:
> > 1) The non-controversial: is there a case where drivers would want encrypted memory
> > for TT pages but not for io mem mappings? Because if not then as Christian pointed
> > out we could just add pgprot_decrypted to ttm_io_prot and be essentially done. The
> > current method of decrypting io mem but leaving sys mem mappings encrypted is a bit
> > weird anyway.
> >
> > If the answer to that question is "yes, some driver does want the TT mappings to be
> > encrypted" then your "[PATCH v2 3/4] drm/ttm, drm/vmwgfx: Correctly support support
> > AMD memory encryption" solves that. I think getting one of those two in makes sense
> > regardless of everything else, agreed?
>
> Well, there is more to it I think.
>
> IIRC, the AMD SME encryption mode has a way for a device to have the
> memory controller (?) encrypt / decrypt device traffic by using an
> address range alias, so in theory it supports encrypted TT pages, and
> the dma-layer may indeed hand encrypted DMA pages to TTM on such systems
> depending on the device's DMA mask. That's why I think that
> force_dma_unencrypted() export was needed, and If the amdgpu driver
> accesses TT memory in SME mode *without* pgprot_decrypted() and it still
> works, then I think that mode is actually used. How could it otherwise work?

For SME, as long as the encrypted bit is set in the physical address
used for DMA, the memory controller will handle the encrypt/decrypt
for the device.  For devices with a limited dma mask, you need to use
the IOMMU so that the encrypted bit is retained when the address hits
the memory controller.

Alex

>
> But anyway, I agree SEV-ES mode would *always* want pgprot_decrypted for
> dma allocated memory, whether it's in PL_SYSTEM or in PL_TT, but I guess
> the above SME case need to be sorted out first.
>
> >
> > 2) The controversial part of your series seems to be exporting of
> > force_dma_unencrypted and its usage within ttm. Personally I'm perfectly ok with
> > ignoring that for now. Getting #1 in is still a million times (rounded up to the
> > nearest million) faster than just using llvmpipe in the guest. And getting #1 also
> > fixes modern presentation on vmwgfx.
>
>
> >
> > So #1 is "make the driver work", while #2 is an optimisation - I think we should
> > treat them as separate things/series. Does that make sense?
>
> I guess that depends on the tradeoff between security and speed for
> people running SEV-enabled VMs. But giving them a choice is probably not
> a bad idea.
>
> /Thomas
>
>
> >
> > z
> >
> > P.S. I forgot to cc dri-devel on the initial email, that wasn't by design. Sorry.
> > I'm ok with moving the discussion to dri-devel.

Re: Decrypting tt maps in ttm

[Zack Rusin]

On Mon, 2023-09-18 at 16:21 -0400, Alex Deucher wrote:
> !! External Email
>
> On Mon, Sep 18, 2023 at 3:06 PM Thomas Hellström
> <thomas.hellstrom@linux.intel.com> wrote:
> >
> >
> > On 9/18/23 17:52, Zack Rusin wrote:
> > > On Mon, 2023-09-18 at 17:13 +0200, Thomas Hellström wrote:
> > > > Hi,
> > > >
> > > > On 9/18/23 16:56, Thomas Hellström wrote:
> > > > > Hi Zack, Christian
> > > > >
> > > > > On 9/18/23 13:36, Christian König wrote:
> > > > > > Hi Zack,
> > > > > >
> > > > > > adding Thomas and Daniel.
> > > > > >
> > > > > > I briefly remember that I talked with Thomas and some other people
> > > > > > about that quite a while ago as well, but I don't fully remember the
> > > > > > outcome.
> > > > > Found one old thread, but didn't read it:
> > > > >
> > > > > https://lists.freedesktop.org/archives/dri-devel/2019-September/234100.html
> > > > >
> > > > >
> > > > > /Thomas
> > > > >
> > > > >
> > > > Ugh. Now starting to read that thread I have a vague recollection it all
> > > > ended with not supporting mapping any device pages whatsoever when SEV
> > > > was enabled, but rather resorting to llvmpipe and VM-local bos.
> > > Hi, Thomas.
> > >
> > > Thanks for finding this! I'd (of course) like to solve it properly and get
> > > vmwgfx
> > > running with 3d support with SEV-ES active instead of essentially disabling
> > > the
> > > driver when SEV-ES is active.
> > >
> > > I think there are two separate discussions there, the non-controversial one
> > > and the
> > > controversial one:
> > > 1) The non-controversial: is there a case where drivers would want encrypted
> > > memory
> > > for TT pages but not for io mem mappings? Because if not then as Christian
> > > pointed
> > > out we could just add pgprot_decrypted to ttm_io_prot and be essentially done.
> > > The
> > > current method of decrypting io mem but leaving sys mem mappings encrypted is
> > > a bit
> > > weird anyway.
> > >
> > > If the answer to that question is "yes, some driver does want the TT mappings
> > > to be
> > > encrypted" then your "[PATCH v2 3/4] drm/ttm, drm/vmwgfx: Correctly support
> > > support
> > > AMD memory encryption" solves that. I think getting one of those two in makes
> > > sense
> > > regardless of everything else, agreed?
> >
> > Well, there is more to it I think.
> >
> > IIRC, the AMD SME encryption mode has a way for a device to have the
> > memory controller (?) encrypt / decrypt device traffic by using an
> > address range alias, so in theory it supports encrypted TT pages, and
> > the dma-layer may indeed hand encrypted DMA pages to TTM on such systems
> > depending on the device's DMA mask. That's why I think that
> > force_dma_unencrypted() export was needed, and If the amdgpu driver
> > accesses TT memory in SME mode *without* pgprot_decrypted() and it still
> > works, then I think that mode is actually used. How could it otherwise work?
>
> For SME, as long as the encrypted bit is set in the physical address
> used for DMA, the memory controller will handle the encrypt/decrypt
> for the device.  For devices with a limited dma mask, you need to use
> the IOMMU so that the encrypted bit is retained when the address hits
> the memory controller.

How does that work on systems with swiotlb, e.g. swiotlb=force, or i.e. what would
decrypt the ttm tt mappings when copying between system and vram when iommu is
disabled/absent?

z

Re: Decrypting tt maps in ttm

[Christian König]

Am 19.09.23 um 03:26 schrieb Zack Rusin:
> On Mon, 2023-09-18 at 16:21 -0400, Alex Deucher wrote:
>> !! External Email
>>
>> On Mon, Sep 18, 2023 at 3:06 PM Thomas Hellström
>> <thomas.hellstrom@linux.intel.com> wrote:
>>>
>>> On 9/18/23 17:52, Zack Rusin wrote:
>>>> On Mon, 2023-09-18 at 17:13 +0200, Thomas Hellström wrote:
>>>>> Hi,
>>>>>
>>>>> On 9/18/23 16:56, Thomas Hellström wrote:
>>>>>> Hi Zack, Christian
>>>>>>
>>>>>> On 9/18/23 13:36, Christian König wrote:
>>>>>>> Hi Zack,
>>>>>>>
>>>>>>> adding Thomas and Daniel.
>>>>>>>
>>>>>>> I briefly remember that I talked with Thomas and some other people
>>>>>>> about that quite a while ago as well, but I don't fully remember the
>>>>>>> outcome.
>>>>>> Found one old thread, but didn't read it:
>>>>>>
>>>>>> https://lists.freedesktop.org/archives/dri-devel/2019-September/234100.html
>>>>>>
>>>>>>
>>>>>> /Thomas
>>>>>>
>>>>>>
>>>>> Ugh. Now starting to read that thread I have a vague recollection it all
>>>>> ended with not supporting mapping any device pages whatsoever when SEV
>>>>> was enabled, but rather resorting to llvmpipe and VM-local bos.
>>>> Hi, Thomas.
>>>>
>>>> Thanks for finding this! I'd (of course) like to solve it properly and get
>>>> vmwgfx
>>>> running with 3d support with SEV-ES active instead of essentially disabling
>>>> the
>>>> driver when SEV-ES is active.
>>>>
>>>> I think there are two separate discussions there, the non-controversial one
>>>> and the
>>>> controversial one:
>>>> 1) The non-controversial: is there a case where drivers would want encrypted
>>>> memory
>>>> for TT pages but not for io mem mappings? Because if not then as Christian
>>>> pointed
>>>> out we could just add pgprot_decrypted to ttm_io_prot and be essentially done.
>>>> The
>>>> current method of decrypting io mem but leaving sys mem mappings encrypted is
>>>> a bit
>>>> weird anyway.
>>>>
>>>> If the answer to that question is "yes, some driver does want the TT mappings
>>>> to be
>>>> encrypted" then your "[PATCH v2 3/4] drm/ttm, drm/vmwgfx: Correctly support
>>>> support
>>>> AMD memory encryption" solves that. I think getting one of those two in makes
>>>> sense
>>>> regardless of everything else, agreed?
>>> Well, there is more to it I think.
>>>
>>> IIRC, the AMD SME encryption mode has a way for a device to have the
>>> memory controller (?) encrypt / decrypt device traffic by using an
>>> address range alias, so in theory it supports encrypted TT pages, and
>>> the dma-layer may indeed hand encrypted DMA pages to TTM on such systems
>>> depending on the device's DMA mask. That's why I think that
>>> force_dma_unencrypted() export was needed, and If the amdgpu driver
>>> accesses TT memory in SME mode *without* pgprot_decrypted() and it still
>>> works, then I think that mode is actually used. How could it otherwise work?
>> For SME, as long as the encrypted bit is set in the physical address
>> used for DMA, the memory controller will handle the encrypt/decrypt
>> for the device.  For devices with a limited dma mask, you need to use
>> the IOMMU so that the encrypted bit is retained when the address hits
>> the memory controller.
> How does that work on systems with swiotlb, e.g. swiotlb=force, or i.e. what would
> decrypt the ttm tt mappings when copying between system and vram when iommu is
> disabled/absent?

SME makes it mandatory that all devices can handle the physical address 
used for DMA, either native or with the help of IOMMU.

Hacks like SWIOTLB are not directly supported as far as I know. Maybe 
somehow SWIOTLB manually decrypts the data while copying it or something 
like this, but I'm not 100% sure if that is actually implemented.

Regards,
Christian.

>
> z
>

Re: Decrypting tt maps in ttm

[Thomas Hellström]

On 9/19/23 07:39, Christian König wrote:
> Am 19.09.23 um 03:26 schrieb Zack Rusin:
>> On Mon, 2023-09-18 at 16:21 -0400, Alex Deucher wrote:
>>> !! External Email
>>>
>>> On Mon, Sep 18, 2023 at 3:06 PM Thomas Hellström
>>> <thomas.hellstrom@linux.intel.com> wrote:
>>>>
>>>> On 9/18/23 17:52, Zack Rusin wrote:
>>>>> On Mon, 2023-09-18 at 17:13 +0200, Thomas Hellström wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 9/18/23 16:56, Thomas Hellström wrote:
>>>>>>> Hi Zack, Christian
>>>>>>>
>>>>>>> On 9/18/23 13:36, Christian König wrote:
>>>>>>>> Hi Zack,
>>>>>>>>
>>>>>>>> adding Thomas and Daniel.
>>>>>>>>
>>>>>>>> I briefly remember that I talked with Thomas and some other people
>>>>>>>> about that quite a while ago as well, but I don't fully 
>>>>>>>> remember the
>>>>>>>> outcome.
>>>>>>> Found one old thread, but didn't read it:
>>>>>>>
>>>>>>> https://lists.freedesktop.org/archives/dri-devel/2019-September/234100.html 
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> /Thomas
>>>>>>>
>>>>>>>
>>>>>> Ugh. Now starting to read that thread I have a vague recollection 
>>>>>> it all
>>>>>> ended with not supporting mapping any device pages whatsoever 
>>>>>> when SEV
>>>>>> was enabled, but rather resorting to llvmpipe and VM-local bos.
>>>>> Hi, Thomas.
>>>>>
>>>>> Thanks for finding this! I'd (of course) like to solve it properly 
>>>>> and get
>>>>> vmwgfx
>>>>> running with 3d support with SEV-ES active instead of essentially 
>>>>> disabling
>>>>> the
>>>>> driver when SEV-ES is active.
>>>>>
>>>>> I think there are two separate discussions there, the 
>>>>> non-controversial one
>>>>> and the
>>>>> controversial one:
>>>>> 1) The non-controversial: is there a case where drivers would want 
>>>>> encrypted
>>>>> memory
>>>>> for TT pages but not for io mem mappings? Because if not then as 
>>>>> Christian
>>>>> pointed
>>>>> out we could just add pgprot_decrypted to ttm_io_prot and be 
>>>>> essentially done.
>>>>> The
>>>>> current method of decrypting io mem but leaving sys mem mappings 
>>>>> encrypted is
>>>>> a bit
>>>>> weird anyway.
>>>>>
>>>>> If the answer to that question is "yes, some driver does want the 
>>>>> TT mappings
>>>>> to be
>>>>> encrypted" then your "[PATCH v2 3/4] drm/ttm, drm/vmwgfx: 
>>>>> Correctly support
>>>>> support
>>>>> AMD memory encryption" solves that. I think getting one of those 
>>>>> two in makes
>>>>> sense
>>>>> regardless of everything else, agreed?
>>>> Well, there is more to it I think.
>>>>
>>>> IIRC, the AMD SME encryption mode has a way for a device to have the
>>>> memory controller (?) encrypt / decrypt device traffic by using an
>>>> address range alias, so in theory it supports encrypted TT pages, and
>>>> the dma-layer may indeed hand encrypted DMA pages to TTM on such 
>>>> systems
>>>> depending on the device's DMA mask. That's why I think that
>>>> force_dma_unencrypted() export was needed, and If the amdgpu driver
>>>> accesses TT memory in SME mode *without* pgprot_decrypted() and it 
>>>> still
>>>> works, then I think that mode is actually used. How could it 
>>>> otherwise work?
>>> For SME, as long as the encrypted bit is set in the physical address
>>> used for DMA, the memory controller will handle the encrypt/decrypt
>>> for the device.  For devices with a limited dma mask, you need to use
>>> the IOMMU so that the encrypted bit is retained when the address hits
>>> the memory controller.
>> How does that work on systems with swiotlb, e.g. swiotlb=force, or 
>> i.e. what would
>> decrypt the ttm tt mappings when copying between system and vram when 
>> iommu is
>> disabled/absent?
>
> SME makes it mandatory that all devices can handle the physical 
> address used for DMA, either native or with the help of IOMMU.
>
> Hacks like SWIOTLB are not directly supported as far as I know. Maybe 
> somehow SWIOTLB manually decrypts the data while copying it or 
> something like this, but I'm not 100% sure if that is actually 
> implemented.
>
> Regards,
> Christian.

A bold guess after looking at various code and patches:

1) Devices under SME that don't support the encryption bit and SEV:
a) Coherent memory is unencrypted.
b) Streaming DMA under IOMMU: The IOMMU sets the encrypted bit.
c) Streaming DMA with SWIOTLB: The bounce buffer is unencrypted. Copying 
to/from bounce-buffer decrypts/encrypts.

2) Devices under SME that do support the encryption bit (which I believe 
is most graphics devices in general on SME systems, not just amdgpu; it 
"just works")
*) Coherent memory is encrypted. The DMA layer sets dma addresses and 
pgprot accordingly.
*) Streaming DMA is encrypted.

So the bug in TTM would then be it's not handling 1a) and 1b) correctly.

Remedy:
1b) Shouldn't be used with encryption.
1a) This is what we should try to fix. Exporting dma_force_unencrypted() 
didn't seem to be a way forward. Properly fixing this would, I guess, 
mean implement the missing functionality in the dma layer: For vmap / 
kmap we could simply reuse the virtual addresses we get back from 
dma_alloc_coherent(), but for faulting one would want something like 
dma_coherent_insert_pfn() (if it doesn't exist already) after a proper 
disussion with Christoph Hellwig.

/Thomas


>
>>
>> z
>>
>

Re: Decrypting tt maps in ttm

[Christian König]

Am 19.09.23 um 08:56 schrieb Thomas Hellström:
>
> On 9/19/23 07:39, Christian König wrote:
>> Am 19.09.23 um 03:26 schrieb Zack Rusin:
>>> On Mon, 2023-09-18 at 16:21 -0400, Alex Deucher wrote:
>>>> !! External Email
>>>>
>>>> On Mon, Sep 18, 2023 at 3:06 PM Thomas Hellström
>>>> <thomas.hellstrom@linux.intel.com> wrote:
>>>>>
>>>>> On 9/18/23 17:52, Zack Rusin wrote:
>>>>>> On Mon, 2023-09-18 at 17:13 +0200, Thomas Hellström wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 9/18/23 16:56, Thomas Hellström wrote:
>>>>>>>> Hi Zack, Christian
>>>>>>>>
>>>>>>>> On 9/18/23 13:36, Christian König wrote:
>>>>>>>>> Hi Zack,
>>>>>>>>>
>>>>>>>>> adding Thomas and Daniel.
>>>>>>>>>
>>>>>>>>> I briefly remember that I talked with Thomas and some other 
>>>>>>>>> people
>>>>>>>>> about that quite a while ago as well, but I don't fully 
>>>>>>>>> remember the
>>>>>>>>> outcome.
>>>>>>>> Found one old thread, but didn't read it:
>>>>>>>>
>>>>>>>> https://lists.freedesktop.org/archives/dri-devel/2019-September/234100.html 
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> /Thomas
>>>>>>>>
>>>>>>>>
>>>>>>> Ugh. Now starting to read that thread I have a vague 
>>>>>>> recollection it all
>>>>>>> ended with not supporting mapping any device pages whatsoever 
>>>>>>> when SEV
>>>>>>> was enabled, but rather resorting to llvmpipe and VM-local bos.
>>>>>> Hi, Thomas.
>>>>>>
>>>>>> Thanks for finding this! I'd (of course) like to solve it 
>>>>>> properly and get
>>>>>> vmwgfx
>>>>>> running with 3d support with SEV-ES active instead of essentially 
>>>>>> disabling
>>>>>> the
>>>>>> driver when SEV-ES is active.
>>>>>>
>>>>>> I think there are two separate discussions there, the 
>>>>>> non-controversial one
>>>>>> and the
>>>>>> controversial one:
>>>>>> 1) The non-controversial: is there a case where drivers would 
>>>>>> want encrypted
>>>>>> memory
>>>>>> for TT pages but not for io mem mappings? Because if not then as 
>>>>>> Christian
>>>>>> pointed
>>>>>> out we could just add pgprot_decrypted to ttm_io_prot and be 
>>>>>> essentially done.
>>>>>> The
>>>>>> current method of decrypting io mem but leaving sys mem mappings 
>>>>>> encrypted is
>>>>>> a bit
>>>>>> weird anyway.
>>>>>>
>>>>>> If the answer to that question is "yes, some driver does want the 
>>>>>> TT mappings
>>>>>> to be
>>>>>> encrypted" then your "[PATCH v2 3/4] drm/ttm, drm/vmwgfx: 
>>>>>> Correctly support
>>>>>> support
>>>>>> AMD memory encryption" solves that. I think getting one of those 
>>>>>> two in makes
>>>>>> sense
>>>>>> regardless of everything else, agreed?
>>>>> Well, there is more to it I think.
>>>>>
>>>>> IIRC, the AMD SME encryption mode has a way for a device to have the
>>>>> memory controller (?) encrypt / decrypt device traffic by using an
>>>>> address range alias, so in theory it supports encrypted TT pages, and
>>>>> the dma-layer may indeed hand encrypted DMA pages to TTM on such 
>>>>> systems
>>>>> depending on the device's DMA mask. That's why I think that
>>>>> force_dma_unencrypted() export was needed, and If the amdgpu driver
>>>>> accesses TT memory in SME mode *without* pgprot_decrypted() and it 
>>>>> still
>>>>> works, then I think that mode is actually used. How could it 
>>>>> otherwise work?
>>>> For SME, as long as the encrypted bit is set in the physical address
>>>> used for DMA, the memory controller will handle the encrypt/decrypt
>>>> for the device.  For devices with a limited dma mask, you need to use
>>>> the IOMMU so that the encrypted bit is retained when the address hits
>>>> the memory controller.
>>> How does that work on systems with swiotlb, e.g. swiotlb=force, or 
>>> i.e. what would
>>> decrypt the ttm tt mappings when copying between system and vram 
>>> when iommu is
>>> disabled/absent?
>>
>> SME makes it mandatory that all devices can handle the physical 
>> address used for DMA, either native or with the help of IOMMU.
>>
>> Hacks like SWIOTLB are not directly supported as far as I know. Maybe 
>> somehow SWIOTLB manually decrypts the data while copying it or 
>> something like this, but I'm not 100% sure if that is actually 
>> implemented.
>>
>> Regards,
>> Christian.
>
> A bold guess after looking at various code and patches:
>
> 1) Devices under SME that don't support the encryption bit and SEV:
> a) Coherent memory is unencrypted.
> b) Streaming DMA under IOMMU: The IOMMU sets the encrypted bit.
> c) Streaming DMA with SWIOTLB: The bounce buffer is unencrypted. 
> Copying to/from bounce-buffer decrypts/encrypts.
>
> 2) Devices under SME that do support the encryption bit (which I 
> believe is most graphics devices in general on SME systems, not just 
> amdgpu; it "just works")
> *) Coherent memory is encrypted. The DMA layer sets dma addresses and 
> pgprot accordingly.
> *) Streaming DMA is encrypted.
>
> So the bug in TTM would then be it's not handling 1a) and 1b) correctly.
>
> Remedy:
> 1b) Shouldn't be used with encryption.
> 1a) This is what we should try to fix. Exporting 
> dma_force_unencrypted() didn't seem to be a way forward. Properly 
> fixing this would, I guess, mean implement the missing functionality 
> in the dma layer: For vmap / kmap we could simply reuse the virtual 
> addresses we get back from dma_alloc_coherent(), but for faulting one 
> would want something like dma_coherent_insert_pfn() (if it doesn't 
> exist already) after a proper disussion with Christoph Hellwig.

Christoph once pointed me to dma_mmap_attrs() for this, but I never 
found the time to fully look into it.

Christian.

>
> /Thomas
>
>
>>
>>>
>>> z
>>>
>>

Re: Decrypting tt maps in ttm

[Zack Rusin]

On Tue, 2023-09-19 at 09:47 +0200, Christian König wrote:
> !! External Email
>
> Am 19.09.23 um 08:56 schrieb Thomas Hellström:
> >
> > On 9/19/23 07:39, Christian König wrote:
> > > Am 19.09.23 um 03:26 schrieb Zack Rusin:
> > > > On Mon, 2023-09-18 at 16:21 -0400, Alex Deucher wrote:
> > > > > !! External Email
> > > > >
> > > > > On Mon, Sep 18, 2023 at 3:06 PM Thomas Hellström
> > > > > <thomas.hellstrom@linux.intel.com> wrote:
> > > > > >
> > > > > > On 9/18/23 17:52, Zack Rusin wrote:
> > > > > > > On Mon, 2023-09-18 at 17:13 +0200, Thomas Hellström wrote:
> > > > > > > > Hi,
> > > > > > > >
> > > > > > > > On 9/18/23 16:56, Thomas Hellström wrote:
> > > > > > > > > Hi Zack, Christian
> > > > > > > > >
> > > > > > > > > On 9/18/23 13:36, Christian König wrote:
> > > > > > > > > > Hi Zack,
> > > > > > > > > >
> > > > > > > > > > adding Thomas and Daniel.
> > > > > > > > > >
> > > > > > > > > > I briefly remember that I talked with Thomas and some other
> > > > > > > > > > people
> > > > > > > > > > about that quite a while ago as well, but I don't fully
> > > > > > > > > > remember the
> > > > > > > > > > outcome.
> > > > > > > > > Found one old thread, but didn't read it:
> > > > > > > > >
> > > > > > > > > https://lists.freedesktop.org/archives/dri-devel/2019-September/234100.html
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > /Thomas
> > > > > > > > >
> > > > > > > > >
> > > > > > > > Ugh. Now starting to read that thread I have a vague
> > > > > > > > recollection it all
> > > > > > > > ended with not supporting mapping any device pages whatsoever
> > > > > > > > when SEV
> > > > > > > > was enabled, but rather resorting to llvmpipe and VM-local bos.
> > > > > > > Hi, Thomas.
> > > > > > >
> > > > > > > Thanks for finding this! I'd (of course) like to solve it
> > > > > > > properly and get
> > > > > > > vmwgfx
> > > > > > > running with 3d support with SEV-ES active instead of essentially
> > > > > > > disabling
> > > > > > > the
> > > > > > > driver when SEV-ES is active.
> > > > > > >
> > > > > > > I think there are two separate discussions there, the
> > > > > > > non-controversial one
> > > > > > > and the
> > > > > > > controversial one:
> > > > > > > 1) The non-controversial: is there a case where drivers would
> > > > > > > want encrypted
> > > > > > > memory
> > > > > > > for TT pages but not for io mem mappings? Because if not then as
> > > > > > > Christian
> > > > > > > pointed
> > > > > > > out we could just add pgprot_decrypted to ttm_io_prot and be
> > > > > > > essentially done.
> > > > > > > The
> > > > > > > current method of decrypting io mem but leaving sys mem mappings
> > > > > > > encrypted is
> > > > > > > a bit
> > > > > > > weird anyway.
> > > > > > >
> > > > > > > If the answer to that question is "yes, some driver does want the
> > > > > > > TT mappings
> > > > > > > to be
> > > > > > > encrypted" then your "[PATCH v2 3/4] drm/ttm, drm/vmwgfx:
> > > > > > > Correctly support
> > > > > > > support
> > > > > > > AMD memory encryption" solves that. I think getting one of those
> > > > > > > two in makes
> > > > > > > sense
> > > > > > > regardless of everything else, agreed?
> > > > > > Well, there is more to it I think.
> > > > > >
> > > > > > IIRC, the AMD SME encryption mode has a way for a device to have the
> > > > > > memory controller (?) encrypt / decrypt device traffic by using an
> > > > > > address range alias, so in theory it supports encrypted TT pages, and
> > > > > > the dma-layer may indeed hand encrypted DMA pages to TTM on such
> > > > > > systems
> > > > > > depending on the device's DMA mask. That's why I think that
> > > > > > force_dma_unencrypted() export was needed, and If the amdgpu driver
> > > > > > accesses TT memory in SME mode *without* pgprot_decrypted() and it
> > > > > > still
> > > > > > works, then I think that mode is actually used. How could it
> > > > > > otherwise work?
> > > > > For SME, as long as the encrypted bit is set in the physical address
> > > > > used for DMA, the memory controller will handle the encrypt/decrypt
> > > > > for the device.  For devices with a limited dma mask, you need to use
> > > > > the IOMMU so that the encrypted bit is retained when the address hits
> > > > > the memory controller.
> > > > How does that work on systems with swiotlb, e.g. swiotlb=force, or
> > > > i.e. what would
> > > > decrypt the ttm tt mappings when copying between system and vram
> > > > when iommu is
> > > > disabled/absent?
> > >
> > > SME makes it mandatory that all devices can handle the physical
> > > address used for DMA, either native or with the help of IOMMU.
> > >
> > > Hacks like SWIOTLB are not directly supported as far as I know. Maybe
> > > somehow SWIOTLB manually decrypts the data while copying it or
> > > something like this, but I'm not 100% sure if that is actually
> > > implemented.
> > >
> > > Regards,
> > > Christian.
> >
> > A bold guess after looking at various code and patches:
> >
> > 1) Devices under SME that don't support the encryption bit and SEV:
> > a) Coherent memory is unencrypted.
> > b) Streaming DMA under IOMMU: The IOMMU sets the encrypted bit.
> > c) Streaming DMA with SWIOTLB: The bounce buffer is unencrypted.
> > Copying to/from bounce-buffer decrypts/encrypts.
> >
> > 2) Devices under SME that do support the encryption bit (which I
> > believe is most graphics devices in general on SME systems, not just
> > amdgpu; it "just works")
> > *) Coherent memory is encrypted. The DMA layer sets dma addresses and
> > pgprot accordingly.
> > *) Streaming DMA is encrypted.
> >
> > So the bug in TTM would then be it's not handling 1a) and 1b) correctly.
> >
> > Remedy:
> > 1b) Shouldn't be used with encryption.
> > 1a) This is what we should try to fix. Exporting
> > dma_force_unencrypted() didn't seem to be a way forward. Properly
> > fixing this would, I guess, mean implement the missing functionality
> > in the dma layer: For vmap / kmap we could simply reuse the virtual
> > addresses we get back from dma_alloc_coherent(), but for faulting one
> > would want something like dma_coherent_insert_pfn() (if it doesn't
> > exist already) after a proper disussion with Christoph Hellwig.
>
> Christoph once pointed me to dma_mmap_attrs() for this, but I never
> found the time to fully look into it.

Hmm, yea, that would make sense
https://elixir.bootlin.com/linux/latest/source/kernel/dma/direct.c#L564
Replacing the vmap's with dma_mmap_attrs would probably fix this, but it would
require a bit of extra setup.

So we're saying that yes, we don't want unconditional pgprot_decrypt in ttm_io_prot.
We'd like to leave those tt mappings as encrypted when possible and instead maybe
add a vaddr to ttm_tt (or extract it from the pages->private via the  ttm_pool_dma,
but that seems rather ugly), plus add get_vm_area() to be able to use dma_mmap_attrs
instead of vmap when use_dma_alloc's is true in ttm_bo_vmap/ttm_bo_kmap?

z

Re: Decrypting tt maps in ttm

[Thomas Hellström]

Hi, Zack,

On 9/20/23 05:43, Zack Rusin wrote:
> On Tue, 2023-09-19 at 09:47 +0200, Christian König wrote:
>> !! External Email
>>
>> Am 19.09.23 um 08:56 schrieb Thomas Hellström:
>>> On 9/19/23 07:39, Christian König wrote:
>>>> Am 19.09.23 um 03:26 schrieb Zack Rusin:
>>>>> On Mon, 2023-09-18 at 16:21 -0400, Alex Deucher wrote:
>>>>>> !! External Email
>>>>>>
>>>>>> On Mon, Sep 18, 2023 at 3:06 PM Thomas Hellström
>>>>>> <thomas.hellstrom@linux.intel.com> wrote:
>>>>>>> On 9/18/23 17:52, Zack Rusin wrote:
>>>>>>>> On Mon, 2023-09-18 at 17:13 +0200, Thomas Hellström wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> On 9/18/23 16:56, Thomas Hellström wrote:
>>>>>>>>>> Hi Zack, Christian
>>>>>>>>>>
>>>>>>>>>> On 9/18/23 13:36, Christian König wrote:
>>>>>>>>>>> Hi Zack,
>>>>>>>>>>>
>>>>>>>>>>> adding Thomas and Daniel.
>>>>>>>>>>>
>>>>>>>>>>> I briefly remember that I talked with Thomas and some other
>>>>>>>>>>> people
>>>>>>>>>>> about that quite a while ago as well, but I don't fully
>>>>>>>>>>> remember the
>>>>>>>>>>> outcome.
>>>>>>>>>> Found one old thread, but didn't read it:
>>>>>>>>>>
>>>>>>>>>> https://lists.freedesktop.org/archives/dri-devel/2019-September/234100.html
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> /Thomas
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>> Ugh. Now starting to read that thread I have a vague
>>>>>>>>> recollection it all
>>>>>>>>> ended with not supporting mapping any device pages whatsoever
>>>>>>>>> when SEV
>>>>>>>>> was enabled, but rather resorting to llvmpipe and VM-local bos.
>>>>>>>> Hi, Thomas.
>>>>>>>>
>>>>>>>> Thanks for finding this! I'd (of course) like to solve it
>>>>>>>> properly and get
>>>>>>>> vmwgfx
>>>>>>>> running with 3d support with SEV-ES active instead of essentially
>>>>>>>> disabling
>>>>>>>> the
>>>>>>>> driver when SEV-ES is active.
>>>>>>>>
>>>>>>>> I think there are two separate discussions there, the
>>>>>>>> non-controversial one
>>>>>>>> and the
>>>>>>>> controversial one:
>>>>>>>> 1) The non-controversial: is there a case where drivers would
>>>>>>>> want encrypted
>>>>>>>> memory
>>>>>>>> for TT pages but not for io mem mappings? Because if not then as
>>>>>>>> Christian
>>>>>>>> pointed
>>>>>>>> out we could just add pgprot_decrypted to ttm_io_prot and be
>>>>>>>> essentially done.
>>>>>>>> The
>>>>>>>> current method of decrypting io mem but leaving sys mem mappings
>>>>>>>> encrypted is
>>>>>>>> a bit
>>>>>>>> weird anyway.
>>>>>>>>
>>>>>>>> If the answer to that question is "yes, some driver does want the
>>>>>>>> TT mappings
>>>>>>>> to be
>>>>>>>> encrypted" then your "[PATCH v2 3/4] drm/ttm, drm/vmwgfx:
>>>>>>>> Correctly support
>>>>>>>> support
>>>>>>>> AMD memory encryption" solves that. I think getting one of those
>>>>>>>> two in makes
>>>>>>>> sense
>>>>>>>> regardless of everything else, agreed?
>>>>>>> Well, there is more to it I think.
>>>>>>>
>>>>>>> IIRC, the AMD SME encryption mode has a way for a device to have the
>>>>>>> memory controller (?) encrypt / decrypt device traffic by using an
>>>>>>> address range alias, so in theory it supports encrypted TT pages, and
>>>>>>> the dma-layer may indeed hand encrypted DMA pages to TTM on such
>>>>>>> systems
>>>>>>> depending on the device's DMA mask. That's why I think that
>>>>>>> force_dma_unencrypted() export was needed, and If the amdgpu driver
>>>>>>> accesses TT memory in SME mode *without* pgprot_decrypted() and it
>>>>>>> still
>>>>>>> works, then I think that mode is actually used. How could it
>>>>>>> otherwise work?
>>>>>> For SME, as long as the encrypted bit is set in the physical address
>>>>>> used for DMA, the memory controller will handle the encrypt/decrypt
>>>>>> for the device.  For devices with a limited dma mask, you need to use
>>>>>> the IOMMU so that the encrypted bit is retained when the address hits
>>>>>> the memory controller.
>>>>> How does that work on systems with swiotlb, e.g. swiotlb=force, or
>>>>> i.e. what would
>>>>> decrypt the ttm tt mappings when copying between system and vram
>>>>> when iommu is
>>>>> disabled/absent?
>>>> SME makes it mandatory that all devices can handle the physical
>>>> address used for DMA, either native or with the help of IOMMU.
>>>>
>>>> Hacks like SWIOTLB are not directly supported as far as I know. Maybe
>>>> somehow SWIOTLB manually decrypts the data while copying it or
>>>> something like this, but I'm not 100% sure if that is actually
>>>> implemented.
>>>>
>>>> Regards,
>>>> Christian.
>>> A bold guess after looking at various code and patches:
>>>
>>> 1) Devices under SME that don't support the encryption bit and SEV:
>>> a) Coherent memory is unencrypted.
>>> b) Streaming DMA under IOMMU: The IOMMU sets the encrypted bit.
>>> c) Streaming DMA with SWIOTLB: The bounce buffer is unencrypted.
>>> Copying to/from bounce-buffer decrypts/encrypts.
>>>
>>> 2) Devices under SME that do support the encryption bit (which I
>>> believe is most graphics devices in general on SME systems, not just
>>> amdgpu; it "just works")
>>> *) Coherent memory is encrypted. The DMA layer sets dma addresses and
>>> pgprot accordingly.
>>> *) Streaming DMA is encrypted.
>>>
>>> So the bug in TTM would then be it's not handling 1a) and 1b) correctly.
>>>
>>> Remedy:
>>> 1b) Shouldn't be used with encryption.
>>> 1a) This is what we should try to fix. Exporting
>>> dma_force_unencrypted() didn't seem to be a way forward. Properly
>>> fixing this would, I guess, mean implement the missing functionality
>>> in the dma layer: For vmap / kmap we could simply reuse the virtual
>>> addresses we get back from dma_alloc_coherent(), but for faulting one
>>> would want something like dma_coherent_insert_pfn() (if it doesn't
>>> exist already) after a proper disussion with Christoph Hellwig.
>> Christoph once pointed me to dma_mmap_attrs() for this, but I never
>> found the time to fully look into it.
> Hmm, yea, that would make sense
> https://elixir.bootlin.com/linux/latest/source/kernel/dma/direct.c#L564
> Replacing the vmap's with dma_mmap_attrs would probably fix this, but it would
> require a bit of extra setup.
>
> So we're saying that yes, we don't want unconditional pgprot_decrypt in ttm_io_prot.
> We'd like to leave those tt mappings as encrypted when possible and instead maybe
> add a vaddr to ttm_tt (or extract it from the pages->private via the  ttm_pool_dma,
> but that seems rather ugly),

It could probably be extracted from pages->private from a helper in the 
ttm pool code, (Christian has a final saying here). However, that 
requires that all ttm_tts are built from a single dma_alloc chunk. Not 
sure that's the case? In that case we're back to square zero for vmaps.


>   plus add get_vm_area() to be able to use dma_mmap_attrs
> instead of vmap when use_dma_alloc's is true in ttm_bo_vmap/ttm_bo_kmap?

vmap is for kernel mappings, and dma_mmap_attrs is for user-space 
mappings, But dma_mmap_attrs()wouldn't be sufficient. If, for example. a 
bo is moved from VRAM to dma-coherent memory, or vice-versa we Zap the 
user-space page-table entries and replace them using faulting to the new 
address. That's why we'd need a dma_coherent_insert_pfn().

I think at the time Christoph was under the impression that you could 
replace xxx_insert_pfn() with xxx_mmap() in the fault handler, but that 
doesn't work since the latter requires the mmap_lock in write mode, 
whereas the fault handler only holds it in read mode. (That caused some 
bugs in the i915 driver when that change was attempted).

/Thomas


>
> z
>
>

Re: Decrypting tt maps in ttm

[Christian König]

Am 20.09.23 um 09:36 schrieb Thomas Hellström:
> Hi, Zack,
>
> On 9/20/23 05:43, Zack Rusin wrote:
>> On Tue, 2023-09-19 at 09:47 +0200, Christian König wrote:
>>> !! External Email
>>>
>>> Am 19.09.23 um 08:56 schrieb Thomas Hellström:
>>>> On 9/19/23 07:39, Christian König wrote:
>>>>> Am 19.09.23 um 03:26 schrieb Zack Rusin:
>>>>>> On Mon, 2023-09-18 at 16:21 -0400, Alex Deucher wrote:
>>>>>>> !! External Email
>>>>>>>
>>>>>>> On Mon, Sep 18, 2023 at 3:06 PM Thomas Hellström
>>>>>>> <thomas.hellstrom@linux.intel.com> wrote:
>>>>>>>> On 9/18/23 17:52, Zack Rusin wrote:
>>>>>>>>> On Mon, 2023-09-18 at 17:13 +0200, Thomas Hellström wrote:
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> On 9/18/23 16:56, Thomas Hellström wrote:
>>>>>>>>>>> Hi Zack, Christian
>>>>>>>>>>>
>>>>>>>>>>> On 9/18/23 13:36, Christian König wrote:
>>>>>>>>>>>> Hi Zack,
>>>>>>>>>>>>
>>>>>>>>>>>> adding Thomas and Daniel.
>>>>>>>>>>>>
>>>>>>>>>>>> I briefly remember that I talked with Thomas and some other
>>>>>>>>>>>> people
>>>>>>>>>>>> about that quite a while ago as well, but I don't fully
>>>>>>>>>>>> remember the
>>>>>>>>>>>> outcome.
>>>>>>>>>>> Found one old thread, but didn't read it:
>>>>>>>>>>>
>>>>>>>>>>> https://lists.freedesktop.org/archives/dri-devel/2019-September/234100.html 
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> /Thomas
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>> Ugh. Now starting to read that thread I have a vague
>>>>>>>>>> recollection it all
>>>>>>>>>> ended with not supporting mapping any device pages whatsoever
>>>>>>>>>> when SEV
>>>>>>>>>> was enabled, but rather resorting to llvmpipe and VM-local bos.
>>>>>>>>> Hi, Thomas.
>>>>>>>>>
>>>>>>>>> Thanks for finding this! I'd (of course) like to solve it
>>>>>>>>> properly and get
>>>>>>>>> vmwgfx
>>>>>>>>> running with 3d support with SEV-ES active instead of essentially
>>>>>>>>> disabling
>>>>>>>>> the
>>>>>>>>> driver when SEV-ES is active.
>>>>>>>>>
>>>>>>>>> I think there are two separate discussions there, the
>>>>>>>>> non-controversial one
>>>>>>>>> and the
>>>>>>>>> controversial one:
>>>>>>>>> 1) The non-controversial: is there a case where drivers would
>>>>>>>>> want encrypted
>>>>>>>>> memory
>>>>>>>>> for TT pages but not for io mem mappings? Because if not then as
>>>>>>>>> Christian
>>>>>>>>> pointed
>>>>>>>>> out we could just add pgprot_decrypted to ttm_io_prot and be
>>>>>>>>> essentially done.
>>>>>>>>> The
>>>>>>>>> current method of decrypting io mem but leaving sys mem mappings
>>>>>>>>> encrypted is
>>>>>>>>> a bit
>>>>>>>>> weird anyway.
>>>>>>>>>
>>>>>>>>> If the answer to that question is "yes, some driver does want the
>>>>>>>>> TT mappings
>>>>>>>>> to be
>>>>>>>>> encrypted" then your "[PATCH v2 3/4] drm/ttm, drm/vmwgfx:
>>>>>>>>> Correctly support
>>>>>>>>> support
>>>>>>>>> AMD memory encryption" solves that. I think getting one of those
>>>>>>>>> two in makes
>>>>>>>>> sense
>>>>>>>>> regardless of everything else, agreed?
>>>>>>>> Well, there is more to it I think.
>>>>>>>>
>>>>>>>> IIRC, the AMD SME encryption mode has a way for a device to 
>>>>>>>> have the
>>>>>>>> memory controller (?) encrypt / decrypt device traffic by using an
>>>>>>>> address range alias, so in theory it supports encrypted TT 
>>>>>>>> pages, and
>>>>>>>> the dma-layer may indeed hand encrypted DMA pages to TTM on such
>>>>>>>> systems
>>>>>>>> depending on the device's DMA mask. That's why I think that
>>>>>>>> force_dma_unencrypted() export was needed, and If the amdgpu 
>>>>>>>> driver
>>>>>>>> accesses TT memory in SME mode *without* pgprot_decrypted() and it
>>>>>>>> still
>>>>>>>> works, then I think that mode is actually used. How could it
>>>>>>>> otherwise work?
>>>>>>> For SME, as long as the encrypted bit is set in the physical 
>>>>>>> address
>>>>>>> used for DMA, the memory controller will handle the encrypt/decrypt
>>>>>>> for the device.  For devices with a limited dma mask, you need 
>>>>>>> to use
>>>>>>> the IOMMU so that the encrypted bit is retained when the address 
>>>>>>> hits
>>>>>>> the memory controller.
>>>>>> How does that work on systems with swiotlb, e.g. swiotlb=force, or
>>>>>> i.e. what would
>>>>>> decrypt the ttm tt mappings when copying between system and vram
>>>>>> when iommu is
>>>>>> disabled/absent?
>>>>> SME makes it mandatory that all devices can handle the physical
>>>>> address used for DMA, either native or with the help of IOMMU.
>>>>>
>>>>> Hacks like SWIOTLB are not directly supported as far as I know. Maybe
>>>>> somehow SWIOTLB manually decrypts the data while copying it or
>>>>> something like this, but I'm not 100% sure if that is actually
>>>>> implemented.
>>>>>
>>>>> Regards,
>>>>> Christian.
>>>> A bold guess after looking at various code and patches:
>>>>
>>>> 1) Devices under SME that don't support the encryption bit and SEV:
>>>> a) Coherent memory is unencrypted.
>>>> b) Streaming DMA under IOMMU: The IOMMU sets the encrypted bit.
>>>> c) Streaming DMA with SWIOTLB: The bounce buffer is unencrypted.
>>>> Copying to/from bounce-buffer decrypts/encrypts.
>>>>
>>>> 2) Devices under SME that do support the encryption bit (which I
>>>> believe is most graphics devices in general on SME systems, not just
>>>> amdgpu; it "just works")
>>>> *) Coherent memory is encrypted. The DMA layer sets dma addresses and
>>>> pgprot accordingly.
>>>> *) Streaming DMA is encrypted.
>>>>
>>>> So the bug in TTM would then be it's not handling 1a) and 1b) 
>>>> correctly.
>>>>
>>>> Remedy:
>>>> 1b) Shouldn't be used with encryption.
>>>> 1a) This is what we should try to fix. Exporting
>>>> dma_force_unencrypted() didn't seem to be a way forward. Properly
>>>> fixing this would, I guess, mean implement the missing functionality
>>>> in the dma layer: For vmap / kmap we could simply reuse the virtual
>>>> addresses we get back from dma_alloc_coherent(), but for faulting one
>>>> would want something like dma_coherent_insert_pfn() (if it doesn't
>>>> exist already) after a proper disussion with Christoph Hellwig.
>>> Christoph once pointed me to dma_mmap_attrs() for this, but I never
>>> found the time to fully look into it.
>> Hmm, yea, that would make sense
>> https://elixir.bootlin.com/linux/latest/source/kernel/dma/direct.c#L564
>> Replacing the vmap's with dma_mmap_attrs would probably fix this, but 
>> it would
>> require a bit of extra setup.
>>
>> So we're saying that yes, we don't want unconditional pgprot_decrypt 
>> in ttm_io_prot.
>> We'd like to leave those tt mappings as encrypted when possible and 
>> instead maybe
>> add a vaddr to ttm_tt (or extract it from the pages->private via the  
>> ttm_pool_dma,
>> but that seems rather ugly),
>
> It could probably be extracted from pages->private from a helper in 
> the ttm pool code, (Christian has a final saying here). However, that 
> requires that all ttm_tts are built from a single dma_alloc chunk. Not 
> sure that's the case? In that case we're back to square zero for vmaps.

Nope they aren't and yes we are back to square one with that.

i think when we want to make this really clean we would need to forward 
the page fault request to the ttm_pool so that it can call 
dma_mmap_attrs() or similar.

But I'm not sure if dma_mmap_attrs() is even guaranteed to work with 
reverse mapping.

>
>
>>   plus add get_vm_area() to be able to use dma_mmap_attrs
>> instead of vmap when use_dma_alloc's is true in ttm_bo_vmap/ttm_bo_kmap?
>
> vmap is for kernel mappings, and dma_mmap_attrs is for user-space 
> mappings, But dma_mmap_attrs()wouldn't be sufficient. If, for example. 
> a bo is moved from VRAM to dma-coherent memory, or vice-versa we Zap 
> the user-space page-table entries and replace them using faulting to 
> the new address. That's why we'd need a dma_coherent_insert_pfn().

Yes, exactly that.

>
> I think at the time Christoph was under the impression that you could 
> replace xxx_insert_pfn() with xxx_mmap() in the fault handler, but 
> that doesn't work since the latter requires the mmap_lock in write 
> mode, whereas the fault handler only holds it in read mode. (That 
> caused some bugs in the i915 driver when that change was attempted).

Good to know.

Christian.

>
> /Thomas
>
>
>>
>> z
>>
>>

Re: Decrypting tt maps in ttm

[Zack Rusin]

[-- Attachment #1: Type: text/plain, Size: 9332 bytes --]

On Wed, 2023-09-20 at 12:48 +0200, Christian König wrote:
> !! External Email
>
> Am 20.09.23 um 09:36 schrieb Thomas Hellström:
> > Hi, Zack,
> >
> > On 9/20/23 05:43, Zack Rusin wrote:
> > > On Tue, 2023-09-19 at 09:47 +0200, Christian König wrote:
> > > > !! External Email
> > > >
> > > > Am 19.09.23 um 08:56 schrieb Thomas Hellström:
> > > > > On 9/19/23 07:39, Christian König wrote:
> > > > > > Am 19.09.23 um 03:26 schrieb Zack Rusin:
> > > > > > > On Mon, 2023-09-18 at 16:21 -0400, Alex Deucher wrote:
> > > > > > > > !! External Email
> > > > > > > >
> > > > > > > > On Mon, Sep 18, 2023 at 3:06 PM Thomas Hellström
> > > > > > > > <thomas.hellstrom@linux.intel.com> wrote:
> > > > > > > > > On 9/18/23 17:52, Zack Rusin wrote:
> > > > > > > > > > On Mon, 2023-09-18 at 17:13 +0200, Thomas Hellström wrote:
> > > > > > > > > > > Hi,
> > > > > > > > > > >
> > > > > > > > > > > On 9/18/23 16:56, Thomas Hellström wrote:
> > > > > > > > > > > > Hi Zack, Christian
> > > > > > > > > > > >
> > > > > > > > > > > > On 9/18/23 13:36, Christian König wrote:
> > > > > > > > > > > > > Hi Zack,
> > > > > > > > > > > > >
> > > > > > > > > > > > > adding Thomas and Daniel.
> > > > > > > > > > > > >
> > > > > > > > > > > > > I briefly remember that I talked with Thomas and some
> > > > > > > > > > > > > other
> > > > > > > > > > > > > people
> > > > > > > > > > > > > about that quite a while ago as well, but I don't fully
> > > > > > > > > > > > > remember the
> > > > > > > > > > > > > outcome.
> > > > > > > > > > > > Found one old thread, but didn't read it:
> > > > > > > > > > > >
> > > > > > > > > > > > https://lists.freedesktop.org/archives/dri-devel/2019-September/234100.html
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > /Thomas
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > Ugh. Now starting to read that thread I have a vague
> > > > > > > > > > > recollection it all
> > > > > > > > > > > ended with not supporting mapping any device pages whatsoever
> > > > > > > > > > > when SEV
> > > > > > > > > > > was enabled, but rather resorting to llvmpipe and VM-local
> > > > > > > > > > > bos.
> > > > > > > > > > Hi, Thomas.
> > > > > > > > > >
> > > > > > > > > > Thanks for finding this! I'd (of course) like to solve it
> > > > > > > > > > properly and get
> > > > > > > > > > vmwgfx
> > > > > > > > > > running with 3d support with SEV-ES active instead of
> > > > > > > > > > essentially
> > > > > > > > > > disabling
> > > > > > > > > > the
> > > > > > > > > > driver when SEV-ES is active.
> > > > > > > > > >
> > > > > > > > > > I think there are two separate discussions there, the
> > > > > > > > > > non-controversial one
> > > > > > > > > > and the
> > > > > > > > > > controversial one:
> > > > > > > > > > 1) The non-controversial: is there a case where drivers would
> > > > > > > > > > want encrypted
> > > > > > > > > > memory
> > > > > > > > > > for TT pages but not for io mem mappings? Because if not then as
> > > > > > > > > > Christian
> > > > > > > > > > pointed
> > > > > > > > > > out we could just add pgprot_decrypted to ttm_io_prot and be
> > > > > > > > > > essentially done.
> > > > > > > > > > The
> > > > > > > > > > current method of decrypting io mem but leaving sys mem mappings
> > > > > > > > > > encrypted is
> > > > > > > > > > a bit
> > > > > > > > > > weird anyway.
> > > > > > > > > >
> > > > > > > > > > If the answer to that question is "yes, some driver does want
> > > > > > > > > > the
> > > > > > > > > > TT mappings
> > > > > > > > > > to be
> > > > > > > > > > encrypted" then your "[PATCH v2 3/4] drm/ttm, drm/vmwgfx:
> > > > > > > > > > Correctly support
> > > > > > > > > > support
> > > > > > > > > > AMD memory encryption" solves that. I think getting one of those
> > > > > > > > > > two in makes
> > > > > > > > > > sense
> > > > > > > > > > regardless of everything else, agreed?
> > > > > > > > > Well, there is more to it I think.
> > > > > > > > >
> > > > > > > > > IIRC, the AMD SME encryption mode has a way for a device to
> > > > > > > > > have the
> > > > > > > > > memory controller (?) encrypt / decrypt device traffic by using an
> > > > > > > > > address range alias, so in theory it supports encrypted TT
> > > > > > > > > pages, and
> > > > > > > > > the dma-layer may indeed hand encrypted DMA pages to TTM on such
> > > > > > > > > systems
> > > > > > > > > depending on the device's DMA mask. That's why I think that
> > > > > > > > > force_dma_unencrypted() export was needed, and If the amdgpu
> > > > > > > > > driver
> > > > > > > > > accesses TT memory in SME mode *without* pgprot_decrypted() and it
> > > > > > > > > still
> > > > > > > > > works, then I think that mode is actually used. How could it
> > > > > > > > > otherwise work?
> > > > > > > > For SME, as long as the encrypted bit is set in the physical
> > > > > > > > address
> > > > > > > > used for DMA, the memory controller will handle the encrypt/decrypt
> > > > > > > > for the device.  For devices with a limited dma mask, you need
> > > > > > > > to use
> > > > > > > > the IOMMU so that the encrypted bit is retained when the address
> > > > > > > > hits
> > > > > > > > the memory controller.
> > > > > > > How does that work on systems with swiotlb, e.g. swiotlb=force, or
> > > > > > > i.e. what would
> > > > > > > decrypt the ttm tt mappings when copying between system and vram
> > > > > > > when iommu is
> > > > > > > disabled/absent?
> > > > > > SME makes it mandatory that all devices can handle the physical
> > > > > > address used for DMA, either native or with the help of IOMMU.
> > > > > >
> > > > > > Hacks like SWIOTLB are not directly supported as far as I know. Maybe
> > > > > > somehow SWIOTLB manually decrypts the data while copying it or
> > > > > > something like this, but I'm not 100% sure if that is actually
> > > > > > implemented.
> > > > > >
> > > > > > Regards,
> > > > > > Christian.
> > > > > A bold guess after looking at various code and patches:
> > > > >
> > > > > 1) Devices under SME that don't support the encryption bit and SEV:
> > > > > a) Coherent memory is unencrypted.
> > > > > b) Streaming DMA under IOMMU: The IOMMU sets the encrypted bit.
> > > > > c) Streaming DMA with SWIOTLB: The bounce buffer is unencrypted.
> > > > > Copying to/from bounce-buffer decrypts/encrypts.
> > > > >
> > > > > 2) Devices under SME that do support the encryption bit (which I
> > > > > believe is most graphics devices in general on SME systems, not just
> > > > > amdgpu; it "just works")
> > > > > *) Coherent memory is encrypted. The DMA layer sets dma addresses and
> > > > > pgprot accordingly.
> > > > > *) Streaming DMA is encrypted.
> > > > >
> > > > > So the bug in TTM would then be it's not handling 1a) and 1b)
> > > > > correctly.
> > > > >
> > > > > Remedy:
> > > > > 1b) Shouldn't be used with encryption.
> > > > > 1a) This is what we should try to fix. Exporting
> > > > > dma_force_unencrypted() didn't seem to be a way forward. Properly
> > > > > fixing this would, I guess, mean implement the missing functionality
> > > > > in the dma layer: For vmap / kmap we could simply reuse the virtual
> > > > > addresses we get back from dma_alloc_coherent(), but for faulting one
> > > > > would want something like dma_coherent_insert_pfn() (if it doesn't
> > > > > exist already) after a proper disussion with Christoph Hellwig.
> > > > Christoph once pointed me to dma_mmap_attrs() for this, but I never
> > > > found the time to fully look into it.
> > > Hmm, yea, that would make sense
> > > https://elixir.bootlin.com/linux/latest/source/kernel/dma/direct.c#L564
> > > Replacing the vmap's with dma_mmap_attrs would probably fix this, but
> > > it would
> > > require a bit of extra setup.
> > >
> > > So we're saying that yes, we don't want unconditional pgprot_decrypt
> > > in ttm_io_prot.
> > > We'd like to leave those tt mappings as encrypted when possible and
> > > instead maybe
> > > add a vaddr to ttm_tt (or extract it from the pages->private via the
> > > ttm_pool_dma,
> > > but that seems rather ugly),
> >
> > It could probably be extracted from pages->private from a helper in
> > the ttm pool code, (Christian has a final saying here). However, that
> > requires that all ttm_tts are built from a single dma_alloc chunk. Not
> > sure that's the case? In that case we're back to square zero for vmaps.
>
> Nope they aren't and yes we are back to square one with that.

Well, that's my favorite square. Number one, just like me...

Maybe we're overthinking this particular problem a bit. As is use_dma_alloc in ttm
is only set in two cases:
- driver explicitly wants coherent mappings (vmwgfx, which require decrypted pages)
- driver needs swiotlb (which, as was pointed out, would require the pages to be
decrypted as well)

So use_dma_alloc always requires the pages to be decrypted. We can reuse that
information to make sure ttm_io_prot correctly marks the pages as decrypted. Like in
the attached diff. This fixes SEV-ES, doesn't require any changes in DMA and is
pretty trivial.

z

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: ttm_decrypt_tt.diff --]
[-- Type: text/x-patch; name="ttm_decrypt_tt.diff", Size: 2790 bytes --]

diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c
index fd9fd3d15101..0b3f4267130c 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_util.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
@@ -294,7 +294,13 @@ pgprot_t ttm_io_prot(struct ttm_buffer_object *bo, struct ttm_resource *res,
 	enum ttm_caching caching;
 
 	man = ttm_manager_type(bo->bdev, res->mem_type);
-	caching = man->use_tt ? bo->ttm->caching : res->bus.caching;
+	if (man->use_tt) {
+		caching = bo->ttm->caching;
+		if (bo->ttm->page_flags & TTM_TT_FLAG_DECRYPTED)
+			tmp = pgprot_decrypted(tmp);
+	} else  {
+		caching = res->bus.caching;
+	}
 
 	return ttm_prot_from_caching(caching, tmp);
 }
@@ -337,6 +343,8 @@ static int ttm_bo_kmap_ttm(struct ttm_buffer_object *bo,
 		.no_wait_gpu = false
 	};
 	struct ttm_tt *ttm = bo->ttm;
+	struct ttm_resource_manager *man =
+			ttm_manager_type(bo->bdev, bo->resource->mem_type);
 	pgprot_t prot;
 	int ret;
 
@@ -346,7 +354,8 @@ static int ttm_bo_kmap_ttm(struct ttm_buffer_object *bo,
 	if (ret)
 		return ret;
 
-	if (num_pages == 1 && ttm->caching == ttm_cached) {
+	if (num_pages == 1 && ttm->caching == ttm_cached &&
+	    !(man->use_tt && (ttm->page_flags & TTM_TT_FLAG_DECRYPTED))) {
 		/*
 		 * We're mapping a single page, and the desired
 		 * page protection is consistent with the bo.
diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c
index e0a77671edd6..d1870aceca8d 100644
--- a/drivers/gpu/drm/ttm/ttm_tt.c
+++ b/drivers/gpu/drm/ttm/ttm_tt.c
@@ -81,6 +81,8 @@ int ttm_tt_create(struct ttm_buffer_object *bo, bool zero_alloc)
 		pr_err("Illegal buffer object type\n");
 		return -EINVAL;
 	}
+	if (bdev->pool.use_dma_alloc)
+		page_flags |= TTM_TT_FLAG_DECRYPTED;
 
 	bo->ttm = bdev->funcs->ttm_tt_create(bo, page_flags);
 	if (unlikely(bo->ttm == NULL))
diff --git a/include/drm/ttm/ttm_tt.h b/include/drm/ttm/ttm_tt.h
index a4eff85b1f44..da2a0fb8e61e 100644
--- a/include/drm/ttm/ttm_tt.h
+++ b/include/drm/ttm/ttm_tt.h
@@ -79,6 +79,9 @@ struct ttm_tt {
 	 *   page_flags = TTM_TT_FLAG_EXTERNAL |
 	 *		  TTM_TT_FLAG_EXTERNAL_MAPPABLE;
 	 *
+	 * TTM_TT_FLAG_DECRYPTED: The mapped ttm pages should be marked as
+	 * not encrypted.
+	 *
 	 * TTM_TT_FLAG_PRIV_POPULATED: TTM internal only. DO NOT USE. This is
 	 * set by TTM after ttm_tt_populate() has successfully returned, and is
 	 * then unset when TTM calls ttm_tt_unpopulate().
@@ -87,8 +90,9 @@ struct ttm_tt {
 #define TTM_TT_FLAG_ZERO_ALLOC		BIT(1)
 #define TTM_TT_FLAG_EXTERNAL		BIT(2)
 #define TTM_TT_FLAG_EXTERNAL_MAPPABLE	BIT(3)
+#define TTM_TT_FLAG_DECRYPTED		BIT(4)
 
-#define TTM_TT_FLAG_PRIV_POPULATED	BIT(4)
+#define TTM_TT_FLAG_PRIV_POPULATED	BIT(5)
 	uint32_t page_flags;
 	/** @num_pages: Number of pages in the page array. */
 	uint32_t num_pages;

Re: Decrypting tt maps in ttm

[Thomas Hellström]

Hi, Zack

On 9/20/23 18:39, Zack Rusin wrote:
> On Wed, 2023-09-20 at 12:48 +0200, Christian König wrote:
>> !! External Email
>>
>> Am 20.09.23 um 09:36 schrieb Thomas Hellström:
>>> Hi, Zack,
>>>
>>> On 9/20/23 05:43, Zack Rusin wrote:
>>>> On Tue, 2023-09-19 at 09:47 +0200, Christian König wrote:
>>>>> !! External Email
>>>>>
>>>>> Am 19.09.23 um 08:56 schrieb Thomas Hellström:
>>>>>> On 9/19/23 07:39, Christian König wrote:
>>>>>>> Am 19.09.23 um 03:26 schrieb Zack Rusin:
>>>>>>>> On Mon, 2023-09-18 at 16:21 -0400, Alex Deucher wrote:
>>>>>>>>> !! External Email
>>>>>>>>>
>>>>>>>>> On Mon, Sep 18, 2023 at 3:06 PM Thomas Hellström
>>>>>>>>> <thomas.hellstrom@linux.intel.com> wrote:
>>>>>>>>>> On 9/18/23 17:52, Zack Rusin wrote:
>>>>>>>>>>> On Mon, 2023-09-18 at 17:13 +0200, Thomas Hellström wrote:
>>>>>>>>>>>> Hi,
>>>>>>>>>>>>
>>>>>>>>>>>> On 9/18/23 16:56, Thomas Hellström wrote:
>>>>>>>>>>>>> Hi Zack, Christian
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 9/18/23 13:36, Christian König wrote:
>>>>>>>>>>>>>> Hi Zack,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> adding Thomas and Daniel.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I briefly remember that I talked with Thomas and some
>>>>>>>>>>>>>> other
>>>>>>>>>>>>>> people
>>>>>>>>>>>>>> about that quite a while ago as well, but I don't fully
>>>>>>>>>>>>>> remember the
>>>>>>>>>>>>>> outcome.
>>>>>>>>>>>>> Found one old thread, but didn't read it:
>>>>>>>>>>>>>
>>>>>>>>>>>>> https://lists.freedesktop.org/archives/dri-devel/2019-September/234100.html
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> /Thomas
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>> Ugh. Now starting to read that thread I have a vague
>>>>>>>>>>>> recollection it all
>>>>>>>>>>>> ended with not supporting mapping any device pages whatsoever
>>>>>>>>>>>> when SEV
>>>>>>>>>>>> was enabled, but rather resorting to llvmpipe and VM-local
>>>>>>>>>>>> bos.
>>>>>>>>>>> Hi, Thomas.
>>>>>>>>>>>
>>>>>>>>>>> Thanks for finding this! I'd (of course) like to solve it
>>>>>>>>>>> properly and get
>>>>>>>>>>> vmwgfx
>>>>>>>>>>> running with 3d support with SEV-ES active instead of
>>>>>>>>>>> essentially
>>>>>>>>>>> disabling
>>>>>>>>>>> the
>>>>>>>>>>> driver when SEV-ES is active.
>>>>>>>>>>>
>>>>>>>>>>> I think there are two separate discussions there, the
>>>>>>>>>>> non-controversial one
>>>>>>>>>>> and the
>>>>>>>>>>> controversial one:
>>>>>>>>>>> 1) The non-controversial: is there a case where drivers would
>>>>>>>>>>> want encrypted
>>>>>>>>>>> memory
>>>>>>>>>>> for TT pages but not for io mem mappings? Because if not then as
>>>>>>>>>>> Christian
>>>>>>>>>>> pointed
>>>>>>>>>>> out we could just add pgprot_decrypted to ttm_io_prot and be
>>>>>>>>>>> essentially done.
>>>>>>>>>>> The
>>>>>>>>>>> current method of decrypting io mem but leaving sys mem mappings
>>>>>>>>>>> encrypted is
>>>>>>>>>>> a bit
>>>>>>>>>>> weird anyway.
>>>>>>>>>>>
>>>>>>>>>>> If the answer to that question is "yes, some driver does want
>>>>>>>>>>> the
>>>>>>>>>>> TT mappings
>>>>>>>>>>> to be
>>>>>>>>>>> encrypted" then your "[PATCH v2 3/4] drm/ttm, drm/vmwgfx:
>>>>>>>>>>> Correctly support
>>>>>>>>>>> support
>>>>>>>>>>> AMD memory encryption" solves that. I think getting one of those
>>>>>>>>>>> two in makes
>>>>>>>>>>> sense
>>>>>>>>>>> regardless of everything else, agreed?
>>>>>>>>>> Well, there is more to it I think.
>>>>>>>>>>
>>>>>>>>>> IIRC, the AMD SME encryption mode has a way for a device to
>>>>>>>>>> have the
>>>>>>>>>> memory controller (?) encrypt / decrypt device traffic by using an
>>>>>>>>>> address range alias, so in theory it supports encrypted TT
>>>>>>>>>> pages, and
>>>>>>>>>> the dma-layer may indeed hand encrypted DMA pages to TTM on such
>>>>>>>>>> systems
>>>>>>>>>> depending on the device's DMA mask. That's why I think that
>>>>>>>>>> force_dma_unencrypted() export was needed, and If the amdgpu
>>>>>>>>>> driver
>>>>>>>>>> accesses TT memory in SME mode *without* pgprot_decrypted() and it
>>>>>>>>>> still
>>>>>>>>>> works, then I think that mode is actually used. How could it
>>>>>>>>>> otherwise work?
>>>>>>>>> For SME, as long as the encrypted bit is set in the physical
>>>>>>>>> address
>>>>>>>>> used for DMA, the memory controller will handle the encrypt/decrypt
>>>>>>>>> for the device.  For devices with a limited dma mask, you need
>>>>>>>>> to use
>>>>>>>>> the IOMMU so that the encrypted bit is retained when the address
>>>>>>>>> hits
>>>>>>>>> the memory controller.
>>>>>>>> How does that work on systems with swiotlb, e.g. swiotlb=force, or
>>>>>>>> i.e. what would
>>>>>>>> decrypt the ttm tt mappings when copying between system and vram
>>>>>>>> when iommu is
>>>>>>>> disabled/absent?
>>>>>>> SME makes it mandatory that all devices can handle the physical
>>>>>>> address used for DMA, either native or with the help of IOMMU.
>>>>>>>
>>>>>>> Hacks like SWIOTLB are not directly supported as far as I know. Maybe
>>>>>>> somehow SWIOTLB manually decrypts the data while copying it or
>>>>>>> something like this, but I'm not 100% sure if that is actually
>>>>>>> implemented.
>>>>>>>
>>>>>>> Regards,
>>>>>>> Christian.
>>>>>> A bold guess after looking at various code and patches:
>>>>>>
>>>>>> 1) Devices under SME that don't support the encryption bit and SEV:
>>>>>> a) Coherent memory is unencrypted.
>>>>>> b) Streaming DMA under IOMMU: The IOMMU sets the encrypted bit.
>>>>>> c) Streaming DMA with SWIOTLB: The bounce buffer is unencrypted.
>>>>>> Copying to/from bounce-buffer decrypts/encrypts.
>>>>>>
>>>>>> 2) Devices under SME that do support the encryption bit (which I
>>>>>> believe is most graphics devices in general on SME systems, not just
>>>>>> amdgpu; it "just works")
>>>>>> *) Coherent memory is encrypted. The DMA layer sets dma addresses and
>>>>>> pgprot accordingly.
>>>>>> *) Streaming DMA is encrypted.
>>>>>>
>>>>>> So the bug in TTM would then be it's not handling 1a) and 1b)
>>>>>> correctly.
>>>>>>
>>>>>> Remedy:
>>>>>> 1b) Shouldn't be used with encryption.
>>>>>> 1a) This is what we should try to fix. Exporting
>>>>>> dma_force_unencrypted() didn't seem to be a way forward. Properly
>>>>>> fixing this would, I guess, mean implement the missing functionality
>>>>>> in the dma layer: For vmap / kmap we could simply reuse the virtual
>>>>>> addresses we get back from dma_alloc_coherent(), but for faulting one
>>>>>> would want something like dma_coherent_insert_pfn() (if it doesn't
>>>>>> exist already) after a proper disussion with Christoph Hellwig.
>>>>> Christoph once pointed me to dma_mmap_attrs() for this, but I never
>>>>> found the time to fully look into it.
>>>> Hmm, yea, that would make sense
>>>> https://elixir.bootlin.com/linux/latest/source/kernel/dma/direct.c#L564
>>>> Replacing the vmap's with dma_mmap_attrs would probably fix this, but
>>>> it would
>>>> require a bit of extra setup.
>>>>
>>>> So we're saying that yes, we don't want unconditional pgprot_decrypt
>>>> in ttm_io_prot.
>>>> We'd like to leave those tt mappings as encrypted when possible and
>>>> instead maybe
>>>> add a vaddr to ttm_tt (or extract it from the pages->private via the
>>>> ttm_pool_dma,
>>>> but that seems rather ugly),
>>> It could probably be extracted from pages->private from a helper in
>>> the ttm pool code, (Christian has a final saying here). However, that
>>> requires that all ttm_tts are built from a single dma_alloc chunk. Not
>>> sure that's the case? In that case we're back to square zero for vmaps.
>> Nope they aren't and yes we are back to square one with that.
> Well, that's my favorite square. Number one, just like me...
>
> Maybe we're overthinking this particular problem a bit. As is use_dma_alloc in ttm
> is only set in two cases:
> - driver explicitly wants coherent mappings (vmwgfx, which require decrypted pages)
> - driver needs swiotlb (which, as was pointed out, would require the pages to be
> decrypted as well)
>
> So use_dma_alloc always requires the pages to be decrypted.

IIRC moving forward it doesn't, since there is (or at least there was) 
implement missing TTM functionality in the dma layer and most TTM 
drivers should at least support dma coherent memory. That means all 
devices supporting a sufficiently large dma mask will break with SME and 
your proposal then.

Perhaps if we condition that on 
"cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT)" that will capture all the 
SEV cases, and limit the existing bug to the hopefully very few TTM 
devices with limited dma mask on SME.

/Thomas


> We can reuse that
> information to make sure ttm_io_prot correctly marks the pages as decrypted. Like in
> the attached diff. This fixes SEV-ES, doesn't require any changes in DMA and is
> pretty trivial.
>
> z

Re: Decrypting tt maps in ttm

[Zack Rusin]

On Wed, 2023-09-20 at 19:17 +0200, Thomas Hellström wrote:
> !! External Email
>
> Hi, Zack
>
> On 9/20/23 18:39, Zack Rusin wrote:
> > On Wed, 2023-09-20 at 12:48 +0200, Christian König wrote:
> > > !! External Email
> > >
> > > Am 20.09.23 um 09:36 schrieb Thomas Hellström:
> > > > Hi, Zack,
> > > >
> > > > On 9/20/23 05:43, Zack Rusin wrote:
> > > > > On Tue, 2023-09-19 at 09:47 +0200, Christian König wrote:
> > > > > > !! External Email
> > > > > >
> > > > > > Am 19.09.23 um 08:56 schrieb Thomas Hellström:
> > > > > > > On 9/19/23 07:39, Christian König wrote:
> > > > > > > > Am 19.09.23 um 03:26 schrieb Zack Rusin:
> > > > > > > > > On Mon, 2023-09-18 at 16:21 -0400, Alex Deucher wrote:
> > > > > > > > > > !! External Email
> > > > > > > > > >
> > > > > > > > > > On Mon, Sep 18, 2023 at 3:06 PM Thomas Hellström
> > > > > > > > > > <thomas.hellstrom@linux.intel.com> wrote:
> > > > > > > > > > > On 9/18/23 17:52, Zack Rusin wrote:
> > > > > > > > > > > > On Mon, 2023-09-18 at 17:13 +0200, Thomas Hellström wrote:
> > > > > > > > > > > > > Hi,
> > > > > > > > > > > > >
> > > > > > > > > > > > > On 9/18/23 16:56, Thomas Hellström wrote:
> > > > > > > > > > > > > > Hi Zack, Christian
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > On 9/18/23 13:36, Christian König wrote:
> > > > > > > > > > > > > > > Hi Zack,
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > adding Thomas and Daniel.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > I briefly remember that I talked with Thomas and some
> > > > > > > > > > > > > > > other
> > > > > > > > > > > > > > > people
> > > > > > > > > > > > > > > about that quite a while ago as well, but I don't
> > > > > > > > > > > > > > > fully
> > > > > > > > > > > > > > > remember the
> > > > > > > > > > > > > > > outcome.
> > > > > > > > > > > > > > Found one old thread, but didn't read it:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > https://lists.freedesktop.org/archives/dri-devel/2019-September/234100.html
> > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > /Thomas
> > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > > Ugh. Now starting to read that thread I have a vague
> > > > > > > > > > > > > recollection it all
> > > > > > > > > > > > > ended with not supporting mapping any device pages
> > > > > > > > > > > > > whatsoever
> > > > > > > > > > > > > when SEV
> > > > > > > > > > > > > was enabled, but rather resorting to llvmpipe and VM-local
> > > > > > > > > > > > > bos.
> > > > > > > > > > > > Hi, Thomas.
> > > > > > > > > > > >
> > > > > > > > > > > > Thanks for finding this! I'd (of course) like to solve it
> > > > > > > > > > > > properly and get
> > > > > > > > > > > > vmwgfx
> > > > > > > > > > > > running with 3d support with SEV-ES active instead of
> > > > > > > > > > > > essentially
> > > > > > > > > > > > disabling
> > > > > > > > > > > > the
> > > > > > > > > > > > driver when SEV-ES is active.
> > > > > > > > > > > >
> > > > > > > > > > > > I think there are two separate discussions there, the
> > > > > > > > > > > > non-controversial one
> > > > > > > > > > > > and the
> > > > > > > > > > > > controversial one:
> > > > > > > > > > > > 1) The non-controversial: is there a case where drivers
> > > > > > > > > > > > would
> > > > > > > > > > > > want encrypted
> > > > > > > > > > > > memory
> > > > > > > > > > > > for TT pages but not for io mem mappings? Because if not
> > > > > > > > > > > > then as
> > > > > > > > > > > > Christian
> > > > > > > > > > > > pointed
> > > > > > > > > > > > out we could just add pgprot_decrypted to ttm_io_prot and be
> > > > > > > > > > > > essentially done.
> > > > > > > > > > > > The
> > > > > > > > > > > > current method of decrypting io mem but leaving sys mem
> > > > > > > > > > > > mappings
> > > > > > > > > > > > encrypted is
> > > > > > > > > > > > a bit
> > > > > > > > > > > > weird anyway.
> > > > > > > > > > > >
> > > > > > > > > > > > If the answer to that question is "yes, some driver does
> > > > > > > > > > > > want
> > > > > > > > > > > > the
> > > > > > > > > > > > TT mappings
> > > > > > > > > > > > to be
> > > > > > > > > > > > encrypted" then your "[PATCH v2 3/4] drm/ttm, drm/vmwgfx:
> > > > > > > > > > > > Correctly support
> > > > > > > > > > > > support
> > > > > > > > > > > > AMD memory encryption" solves that. I think getting one of
> > > > > > > > > > > > those
> > > > > > > > > > > > two in makes
> > > > > > > > > > > > sense
> > > > > > > > > > > > regardless of everything else, agreed?
> > > > > > > > > > > Well, there is more to it I think.
> > > > > > > > > > >
> > > > > > > > > > > IIRC, the AMD SME encryption mode has a way for a device to
> > > > > > > > > > > have the
> > > > > > > > > > > memory controller (?) encrypt / decrypt device traffic by
> > > > > > > > > > > using an
> > > > > > > > > > > address range alias, so in theory it supports encrypted TT
> > > > > > > > > > > pages, and
> > > > > > > > > > > the dma-layer may indeed hand encrypted DMA pages to TTM on
> > > > > > > > > > > such
> > > > > > > > > > > systems
> > > > > > > > > > > depending on the device's DMA mask. That's why I think that
> > > > > > > > > > > force_dma_unencrypted() export was needed, and If the amdgpu
> > > > > > > > > > > driver
> > > > > > > > > > > accesses TT memory in SME mode *without* pgprot_decrypted()
> > > > > > > > > > > and it
> > > > > > > > > > > still
> > > > > > > > > > > works, then I think that mode is actually used. How could it
> > > > > > > > > > > otherwise work?
> > > > > > > > > > For SME, as long as the encrypted bit is set in the physical
> > > > > > > > > > address
> > > > > > > > > > used for DMA, the memory controller will handle the
> > > > > > > > > > encrypt/decrypt
> > > > > > > > > > for the device.  For devices with a limited dma mask, you need
> > > > > > > > > > to use
> > > > > > > > > > the IOMMU so that the encrypted bit is retained when the address
> > > > > > > > > > hits
> > > > > > > > > > the memory controller.
> > > > > > > > > How does that work on systems with swiotlb, e.g. swiotlb=force, or
> > > > > > > > > i.e. what would
> > > > > > > > > decrypt the ttm tt mappings when copying between system and vram
> > > > > > > > > when iommu is
> > > > > > > > > disabled/absent?
> > > > > > > > SME makes it mandatory that all devices can handle the physical
> > > > > > > > address used for DMA, either native or with the help of IOMMU.
> > > > > > > >
> > > > > > > > Hacks like SWIOTLB are not directly supported as far as I know.
> > > > > > > > Maybe
> > > > > > > > somehow SWIOTLB manually decrypts the data while copying it or
> > > > > > > > something like this, but I'm not 100% sure if that is actually
> > > > > > > > implemented.
> > > > > > > >
> > > > > > > > Regards,
> > > > > > > > Christian.
> > > > > > > A bold guess after looking at various code and patches:
> > > > > > >
> > > > > > > 1) Devices under SME that don't support the encryption bit and SEV:
> > > > > > > a) Coherent memory is unencrypted.
> > > > > > > b) Streaming DMA under IOMMU: The IOMMU sets the encrypted bit.
> > > > > > > c) Streaming DMA with SWIOTLB: The bounce buffer is unencrypted.
> > > > > > > Copying to/from bounce-buffer decrypts/encrypts.
> > > > > > >
> > > > > > > 2) Devices under SME that do support the encryption bit (which I
> > > > > > > believe is most graphics devices in general on SME systems, not just
> > > > > > > amdgpu; it "just works")
> > > > > > > *) Coherent memory is encrypted. The DMA layer sets dma addresses and
> > > > > > > pgprot accordingly.
> > > > > > > *) Streaming DMA is encrypted.
> > > > > > >
> > > > > > > So the bug in TTM would then be it's not handling 1a) and 1b)
> > > > > > > correctly.
> > > > > > >
> > > > > > > Remedy:
> > > > > > > 1b) Shouldn't be used with encryption.
> > > > > > > 1a) This is what we should try to fix. Exporting
> > > > > > > dma_force_unencrypted() didn't seem to be a way forward. Properly
> > > > > > > fixing this would, I guess, mean implement the missing functionality
> > > > > > > in the dma layer: For vmap / kmap we could simply reuse the virtual
> > > > > > > addresses we get back from dma_alloc_coherent(), but for faulting one
> > > > > > > would want something like dma_coherent_insert_pfn() (if it doesn't
> > > > > > > exist already) after a proper disussion with Christoph Hellwig.
> > > > > > Christoph once pointed me to dma_mmap_attrs() for this, but I never
> > > > > > found the time to fully look into it.
> > > > > Hmm, yea, that would make sense
> > > > > https://elixir.bootlin.com/linux/latest/source/kernel/dma/direct.c#L564
> > > > > Replacing the vmap's with dma_mmap_attrs would probably fix this, but
> > > > > it would
> > > > > require a bit of extra setup.
> > > > >
> > > > > So we're saying that yes, we don't want unconditional pgprot_decrypt
> > > > > in ttm_io_prot.
> > > > > We'd like to leave those tt mappings as encrypted when possible and
> > > > > instead maybe
> > > > > add a vaddr to ttm_tt (or extract it from the pages->private via the
> > > > > ttm_pool_dma,
> > > > > but that seems rather ugly),
> > > > It could probably be extracted from pages->private from a helper in
> > > > the ttm pool code, (Christian has a final saying here). However, that
> > > > requires that all ttm_tts are built from a single dma_alloc chunk. Not
> > > > sure that's the case? In that case we're back to square zero for vmaps.
> > > Nope they aren't and yes we are back to square one with that.
> > Well, that's my favorite square. Number one, just like me...
> >
> > Maybe we're overthinking this particular problem a bit. As is use_dma_alloc in
> > ttm
> > is only set in two cases:
> > - driver explicitly wants coherent mappings (vmwgfx, which require decrypted
> > pages)
> > - driver needs swiotlb (which, as was pointed out, would require the pages to be
> > decrypted as well)
> >
> > So use_dma_alloc always requires the pages to be decrypted.
>
> IIRC moving forward it doesn't, since there is (or at least there was)
> implement missing TTM functionality in the dma layer and most TTM
> drivers should at least support dma coherent memory. That means all
> devices supporting a sufficiently large dma mask will break with SME and
> your proposal then.
>
> Perhaps if we condition that on
> "cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT)" that will capture all the
> SEV cases, and limit the existing bug to the hopefully very few TTM
> devices with limited dma mask on SME.

Ah, I wasn't aware those exist, do you know what platforms are those? I can try to
find one around here to see.
And they don't really break, they just might unnecessarily decrypt tt pages, right?

In that case I'd think that we'd lean towards "unnecessarily decrypted" and not the
current "complete broken" (the memory is encrypted and the hardware can't handle
it). Although if we're actively thinking about rewriting dma/page fault handling in
ttm, then yes, this doesn't make sense. Otherwise it'd seem like a decent solution
to not being able to even present with SEV.

z

Re: Decrypting tt maps in ttm

[Thomas Hellström]

On 9/20/23 20:24, Zack Rusin wrote:
> On Wed, 2023-09-20 at 19:17 +0200, Thomas Hellström wrote:
>> !! External Email
>>
>> Hi, Zack
>>
>> On 9/20/23 18:39, Zack Rusin wrote:
>>> On Wed, 2023-09-20 at 12:48 +0200, Christian König wrote:
>>>> !! External Email
>>>>
>>>> Am 20.09.23 um 09:36 schrieb Thomas Hellström:
>>>>> Hi, Zack,
>>>>>
>>>>> On 9/20/23 05:43, Zack Rusin wrote:
>>>>>> On Tue, 2023-09-19 at 09:47 +0200, Christian König wrote:
>>>>>>> !! External Email
>>>>>>>
>>>>>>> Am 19.09.23 um 08:56 schrieb Thomas Hellström:
>>>>>>>> On 9/19/23 07:39, Christian König wrote:
>>>>>>>>> Am 19.09.23 um 03:26 schrieb Zack Rusin:
>>>>>>>>>> On Mon, 2023-09-18 at 16:21 -0400, Alex Deucher wrote:
>>>>>>>>>>> !! External Email
>>>>>>>>>>>
>>>>>>>>>>> On Mon, Sep 18, 2023 at 3:06 PM Thomas Hellström
>>>>>>>>>>> <thomas.hellstrom@linux.intel.com> wrote:
>>>>>>>>>>>> On 9/18/23 17:52, Zack Rusin wrote:
>>>>>>>>>>>>> On Mon, 2023-09-18 at 17:13 +0200, Thomas Hellström wrote:
>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 9/18/23 16:56, Thomas Hellström wrote:
>>>>>>>>>>>>>>> Hi Zack, Christian
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 9/18/23 13:36, Christian König wrote:
>>>>>>>>>>>>>>>> Hi Zack,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> adding Thomas and Daniel.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> I briefly remember that I talked with Thomas and some
>>>>>>>>>>>>>>>> other
>>>>>>>>>>>>>>>> people
>>>>>>>>>>>>>>>> about that quite a while ago as well, but I don't
>>>>>>>>>>>>>>>> fully
>>>>>>>>>>>>>>>> remember the
>>>>>>>>>>>>>>>> outcome.
>>>>>>>>>>>>>>> Found one old thread, but didn't read it:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> https://lists.freedesktop.org/archives/dri-devel/2019-September/234100.html
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> /Thomas
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Ugh. Now starting to read that thread I have a vague
>>>>>>>>>>>>>> recollection it all
>>>>>>>>>>>>>> ended with not supporting mapping any device pages
>>>>>>>>>>>>>> whatsoever
>>>>>>>>>>>>>> when SEV
>>>>>>>>>>>>>> was enabled, but rather resorting to llvmpipe and VM-local
>>>>>>>>>>>>>> bos.
>>>>>>>>>>>>> Hi, Thomas.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks for finding this! I'd (of course) like to solve it
>>>>>>>>>>>>> properly and get
>>>>>>>>>>>>> vmwgfx
>>>>>>>>>>>>> running with 3d support with SEV-ES active instead of
>>>>>>>>>>>>> essentially
>>>>>>>>>>>>> disabling
>>>>>>>>>>>>> the
>>>>>>>>>>>>> driver when SEV-ES is active.
>>>>>>>>>>>>>
>>>>>>>>>>>>> I think there are two separate discussions there, the
>>>>>>>>>>>>> non-controversial one
>>>>>>>>>>>>> and the
>>>>>>>>>>>>> controversial one:
>>>>>>>>>>>>> 1) The non-controversial: is there a case where drivers
>>>>>>>>>>>>> would
>>>>>>>>>>>>> want encrypted
>>>>>>>>>>>>> memory
>>>>>>>>>>>>> for TT pages but not for io mem mappings? Because if not
>>>>>>>>>>>>> then as
>>>>>>>>>>>>> Christian
>>>>>>>>>>>>> pointed
>>>>>>>>>>>>> out we could just add pgprot_decrypted to ttm_io_prot and be
>>>>>>>>>>>>> essentially done.
>>>>>>>>>>>>> The
>>>>>>>>>>>>> current method of decrypting io mem but leaving sys mem
>>>>>>>>>>>>> mappings
>>>>>>>>>>>>> encrypted is
>>>>>>>>>>>>> a bit
>>>>>>>>>>>>> weird anyway.
>>>>>>>>>>>>>
>>>>>>>>>>>>> If the answer to that question is "yes, some driver does
>>>>>>>>>>>>> want
>>>>>>>>>>>>> the
>>>>>>>>>>>>> TT mappings
>>>>>>>>>>>>> to be
>>>>>>>>>>>>> encrypted" then your "[PATCH v2 3/4] drm/ttm, drm/vmwgfx:
>>>>>>>>>>>>> Correctly support
>>>>>>>>>>>>> support
>>>>>>>>>>>>> AMD memory encryption" solves that. I think getting one of
>>>>>>>>>>>>> those
>>>>>>>>>>>>> two in makes
>>>>>>>>>>>>> sense
>>>>>>>>>>>>> regardless of everything else, agreed?
>>>>>>>>>>>> Well, there is more to it I think.
>>>>>>>>>>>>
>>>>>>>>>>>> IIRC, the AMD SME encryption mode has a way for a device to
>>>>>>>>>>>> have the
>>>>>>>>>>>> memory controller (?) encrypt / decrypt device traffic by
>>>>>>>>>>>> using an
>>>>>>>>>>>> address range alias, so in theory it supports encrypted TT
>>>>>>>>>>>> pages, and
>>>>>>>>>>>> the dma-layer may indeed hand encrypted DMA pages to TTM on
>>>>>>>>>>>> such
>>>>>>>>>>>> systems
>>>>>>>>>>>> depending on the device's DMA mask. That's why I think that
>>>>>>>>>>>> force_dma_unencrypted() export was needed, and If the amdgpu
>>>>>>>>>>>> driver
>>>>>>>>>>>> accesses TT memory in SME mode *without* pgprot_decrypted()
>>>>>>>>>>>> and it
>>>>>>>>>>>> still
>>>>>>>>>>>> works, then I think that mode is actually used. How could it
>>>>>>>>>>>> otherwise work?
>>>>>>>>>>> For SME, as long as the encrypted bit is set in the physical
>>>>>>>>>>> address
>>>>>>>>>>> used for DMA, the memory controller will handle the
>>>>>>>>>>> encrypt/decrypt
>>>>>>>>>>> for the device.  For devices with a limited dma mask, you need
>>>>>>>>>>> to use
>>>>>>>>>>> the IOMMU so that the encrypted bit is retained when the address
>>>>>>>>>>> hits
>>>>>>>>>>> the memory controller.
>>>>>>>>>> How does that work on systems with swiotlb, e.g. swiotlb=force, or
>>>>>>>>>> i.e. what would
>>>>>>>>>> decrypt the ttm tt mappings when copying between system and vram
>>>>>>>>>> when iommu is
>>>>>>>>>> disabled/absent?
>>>>>>>>> SME makes it mandatory that all devices can handle the physical
>>>>>>>>> address used for DMA, either native or with the help of IOMMU.
>>>>>>>>>
>>>>>>>>> Hacks like SWIOTLB are not directly supported as far as I know.
>>>>>>>>> Maybe
>>>>>>>>> somehow SWIOTLB manually decrypts the data while copying it or
>>>>>>>>> something like this, but I'm not 100% sure if that is actually
>>>>>>>>> implemented.
>>>>>>>>>
>>>>>>>>> Regards,
>>>>>>>>> Christian.
>>>>>>>> A bold guess after looking at various code and patches:
>>>>>>>>
>>>>>>>> 1) Devices under SME that don't support the encryption bit and SEV:
>>>>>>>> a) Coherent memory is unencrypted.
>>>>>>>> b) Streaming DMA under IOMMU: The IOMMU sets the encrypted bit.
>>>>>>>> c) Streaming DMA with SWIOTLB: The bounce buffer is unencrypted.
>>>>>>>> Copying to/from bounce-buffer decrypts/encrypts.
>>>>>>>>
>>>>>>>> 2) Devices under SME that do support the encryption bit (which I
>>>>>>>> believe is most graphics devices in general on SME systems, not just
>>>>>>>> amdgpu; it "just works")
>>>>>>>> *) Coherent memory is encrypted. The DMA layer sets dma addresses and
>>>>>>>> pgprot accordingly.
>>>>>>>> *) Streaming DMA is encrypted.
>>>>>>>>
>>>>>>>> So the bug in TTM would then be it's not handling 1a) and 1b)
>>>>>>>> correctly.
>>>>>>>>
>>>>>>>> Remedy:
>>>>>>>> 1b) Shouldn't be used with encryption.
>>>>>>>> 1a) This is what we should try to fix. Exporting
>>>>>>>> dma_force_unencrypted() didn't seem to be a way forward. Properly
>>>>>>>> fixing this would, I guess, mean implement the missing functionality
>>>>>>>> in the dma layer: For vmap / kmap we could simply reuse the virtual
>>>>>>>> addresses we get back from dma_alloc_coherent(), but for faulting one
>>>>>>>> would want something like dma_coherent_insert_pfn() (if it doesn't
>>>>>>>> exist already) after a proper disussion with Christoph Hellwig.
>>>>>>> Christoph once pointed me to dma_mmap_attrs() for this, but I never
>>>>>>> found the time to fully look into it.
>>>>>> Hmm, yea, that would make sense
>>>>>> https://elixir.bootlin.com/linux/latest/source/kernel/dma/direct.c#L564
>>>>>> Replacing the vmap's with dma_mmap_attrs would probably fix this, but
>>>>>> it would
>>>>>> require a bit of extra setup.
>>>>>>
>>>>>> So we're saying that yes, we don't want unconditional pgprot_decrypt
>>>>>> in ttm_io_prot.
>>>>>> We'd like to leave those tt mappings as encrypted when possible and
>>>>>> instead maybe
>>>>>> add a vaddr to ttm_tt (or extract it from the pages->private via the
>>>>>> ttm_pool_dma,
>>>>>> but that seems rather ugly),
>>>>> It could probably be extracted from pages->private from a helper in
>>>>> the ttm pool code, (Christian has a final saying here). However, that
>>>>> requires that all ttm_tts are built from a single dma_alloc chunk. Not
>>>>> sure that's the case? In that case we're back to square zero for vmaps.
>>>> Nope they aren't and yes we are back to square one with that.
>>> Well, that's my favorite square. Number one, just like me...
>>>
>>> Maybe we're overthinking this particular problem a bit. As is use_dma_alloc in
>>> ttm
>>> is only set in two cases:
>>> - driver explicitly wants coherent mappings (vmwgfx, which require decrypted
>>> pages)
>>> - driver needs swiotlb (which, as was pointed out, would require the pages to be
>>> decrypted as well)
>>>
>>> So use_dma_alloc always requires the pages to be decrypted.
>> IIRC moving forward it doesn't, since there is (or at least there was)
>> implement missing TTM functionality in the dma layer and most TTM
>> drivers should at least support dma coherent memory. That means all
>> devices supporting a sufficiently large dma mask will break with SME and
>> your proposal then.
>>
>> Perhaps if we condition that on
>> "cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT)" that will capture all the
>> SEV cases, and limit the existing bug to the hopefully very few TTM
>> devices with limited dma mask on SME.
> Ah, I wasn't aware those exist, do you know what platforms are those? I can try to
> find one around here to see.

My understanding is cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT) will 
return true in the guest iff SEV is active, and can be used in TTM as a 
poor man's force_dma_unencrypted(), enabling the functionality in your 
diff. It looks like a similar check is present in vmwgfx to detect SEV, 
but also see below.

> And they don't really break, they just might unnecessarily decrypt tt pages, right?

No, with SME, dma from hw will encrypt the content, because the dma 
layer will set the "encrypt" bit in the physical address given to the 
iommu or the device in case iommu is not active, but a subsequent 
reading the content using the CPU won't decrypt so CPU and device will 
have different views of the page.

Also the linear kernel mapping PTEs will conflict in encryption mode 
with the ones TTM sets up, and IIRC that's forbidden in the SEV spec. 
(The x86 arch code goes through some serious work to flush out caches 
and TLBs to convert a page kernel linear mapping from encrypted to 
non-encrypted,

https://elixir.bootlin.com/linux/latest/source/arch/x86/mm/pat/set_memory.c#L2129

and that is also seen as pretty heavy dma_alloc_coherent() latency).

So the pgprot_t TTM sets up *must* be identical to the one used by the 
dma layer, so anything we should be aware here that anything we do in 
TTM less than adding needed functionality in the dma layer is 
second-guessing what the dma layer does internally and is not really the 
right solution.

/Thomas


>
> In that case I'd think that we'd lean towards "unnecessarily decrypted" and not the
> current "complete broken" (the memory is encrypted and the hardware can't handle
> it). Although if we're actively thinking about rewriting dma/page fault handling in
> ttm, then yes, this doesn't make sense. Otherwise it'd seem like a decent solution
> to not being able to even present with SEV.
>
> z
>

Re: Decrypting tt maps in ttm

[Zack Rusin]

On Wed, 2023-09-20 at 21:22 +0200, Thomas Hellström wrote:
> !! External Email
>
> On 9/20/23 20:24, Zack Rusin wrote:
> > On Wed, 2023-09-20 at 19:17 +0200, Thomas Hellström wrote:
> > > !! External Email
> > >
> > > Hi, Zack
> > >
> > > On 9/20/23 18:39, Zack Rusin wrote:
> > > > On Wed, 2023-09-20 at 12:48 +0200, Christian König wrote:
> > > > > !! External Email
> > > > >
> > > > > Am 20.09.23 um 09:36 schrieb Thomas Hellström:
> > > > > > Hi, Zack,
> > > > > >
> > > > > > On 9/20/23 05:43, Zack Rusin wrote:
> > > > > > > On Tue, 2023-09-19 at 09:47 +0200, Christian König wrote:
> > > > > > > > !! External Email
> > > > > > > >
> > > > > > > > Am 19.09.23 um 08:56 schrieb Thomas Hellström:
> > > > > > > > > On 9/19/23 07:39, Christian König wrote:
> > > > > > > > > > Am 19.09.23 um 03:26 schrieb Zack Rusin:
> > > > > > > > > > > On Mon, 2023-09-18 at 16:21 -0400, Alex Deucher wrote:
> > > > > > > > > > > > !! External Email
> > > > > > > > > > > >
> > > > > > > > > > > > On Mon, Sep 18, 2023 at 3:06 PM Thomas Hellström
> > > > > > > > > > > > <thomas.hellstrom@linux.intel.com> wrote:
> > > > > > > > > > > > > On 9/18/23 17:52, Zack Rusin wrote:
> > > > > > > > > > > > > > On Mon, 2023-09-18 at 17:13 +0200, Thomas Hellström
> > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > Hi,
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > On 9/18/23 16:56, Thomas Hellström wrote:
> > > > > > > > > > > > > > > > Hi Zack, Christian
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > On 9/18/23 13:36, Christian König wrote:
> > > > > > > > > > > > > > > > > Hi Zack,
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > adding Thomas and Daniel.
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > I briefly remember that I talked with Thomas and
> > > > > > > > > > > > > > > > > some
> > > > > > > > > > > > > > > > > other
> > > > > > > > > > > > > > > > > people
> > > > > > > > > > > > > > > > > about that quite a while ago as well, but I don't
> > > > > > > > > > > > > > > > > fully
> > > > > > > > > > > > > > > > > remember the
> > > > > > > > > > > > > > > > > outcome.
> > > > > > > > > > > > > > > > Found one old thread, but didn't read it:
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > https://lists.freedesktop.org/archives/dri-devel/2019-September/234100.html
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > /Thomas
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Ugh. Now starting to read that thread I have a vague
> > > > > > > > > > > > > > > recollection it all
> > > > > > > > > > > > > > > ended with not supporting mapping any device pages
> > > > > > > > > > > > > > > whatsoever
> > > > > > > > > > > > > > > when SEV
> > > > > > > > > > > > > > > was enabled, but rather resorting to llvmpipe and VM-
> > > > > > > > > > > > > > > local
> > > > > > > > > > > > > > > bos.
> > > > > > > > > > > > > > Hi, Thomas.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Thanks for finding this! I'd (of course) like to solve
> > > > > > > > > > > > > > it
> > > > > > > > > > > > > > properly and get
> > > > > > > > > > > > > > vmwgfx
> > > > > > > > > > > > > > running with 3d support with SEV-ES active instead of
> > > > > > > > > > > > > > essentially
> > > > > > > > > > > > > > disabling
> > > > > > > > > > > > > > the
> > > > > > > > > > > > > > driver when SEV-ES is active.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > I think there are two separate discussions there, the
> > > > > > > > > > > > > > non-controversial one
> > > > > > > > > > > > > > and the
> > > > > > > > > > > > > > controversial one:
> > > > > > > > > > > > > > 1) The non-controversial: is there a case where drivers
> > > > > > > > > > > > > > would
> > > > > > > > > > > > > > want encrypted
> > > > > > > > > > > > > > memory
> > > > > > > > > > > > > > for TT pages but not for io mem mappings? Because if not
> > > > > > > > > > > > > > then as
> > > > > > > > > > > > > > Christian
> > > > > > > > > > > > > > pointed
> > > > > > > > > > > > > > out we could just add pgprot_decrypted to ttm_io_prot
> > > > > > > > > > > > > > and be
> > > > > > > > > > > > > > essentially done.
> > > > > > > > > > > > > > The
> > > > > > > > > > > > > > current method of decrypting io mem but leaving sys mem
> > > > > > > > > > > > > > mappings
> > > > > > > > > > > > > > encrypted is
> > > > > > > > > > > > > > a bit
> > > > > > > > > > > > > > weird anyway.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > If the answer to that question is "yes, some driver does
> > > > > > > > > > > > > > want
> > > > > > > > > > > > > > the
> > > > > > > > > > > > > > TT mappings
> > > > > > > > > > > > > > to be
> > > > > > > > > > > > > > encrypted" then your "[PATCH v2 3/4] drm/ttm,
> > > > > > > > > > > > > > drm/vmwgfx:
> > > > > > > > > > > > > > Correctly support
> > > > > > > > > > > > > > support
> > > > > > > > > > > > > > AMD memory encryption" solves that. I think getting one
> > > > > > > > > > > > > > of
> > > > > > > > > > > > > > those
> > > > > > > > > > > > > > two in makes
> > > > > > > > > > > > > > sense
> > > > > > > > > > > > > > regardless of everything else, agreed?
> > > > > > > > > > > > > Well, there is more to it I think.
> > > > > > > > > > > > >
> > > > > > > > > > > > > IIRC, the AMD SME encryption mode has a way for a device
> > > > > > > > > > > > > to
> > > > > > > > > > > > > have the
> > > > > > > > > > > > > memory controller (?) encrypt / decrypt device traffic by
> > > > > > > > > > > > > using an
> > > > > > > > > > > > > address range alias, so in theory it supports encrypted TT
> > > > > > > > > > > > > pages, and
> > > > > > > > > > > > > the dma-layer may indeed hand encrypted DMA pages to TTM
> > > > > > > > > > > > > on
> > > > > > > > > > > > > such
> > > > > > > > > > > > > systems
> > > > > > > > > > > > > depending on the device's DMA mask. That's why I think
> > > > > > > > > > > > > that
> > > > > > > > > > > > > force_dma_unencrypted() export was needed, and If the
> > > > > > > > > > > > > amdgpu
> > > > > > > > > > > > > driver
> > > > > > > > > > > > > accesses TT memory in SME mode *without*
> > > > > > > > > > > > > pgprot_decrypted()
> > > > > > > > > > > > > and it
> > > > > > > > > > > > > still
> > > > > > > > > > > > > works, then I think that mode is actually used. How could
> > > > > > > > > > > > > it
> > > > > > > > > > > > > otherwise work?
> > > > > > > > > > > > For SME, as long as the encrypted bit is set in the physical
> > > > > > > > > > > > address
> > > > > > > > > > > > used for DMA, the memory controller will handle the
> > > > > > > > > > > > encrypt/decrypt
> > > > > > > > > > > > for the device.  For devices with a limited dma mask, you
> > > > > > > > > > > > need
> > > > > > > > > > > > to use
> > > > > > > > > > > > the IOMMU so that the encrypted bit is retained when the
> > > > > > > > > > > > address
> > > > > > > > > > > > hits
> > > > > > > > > > > > the memory controller.
> > > > > > > > > > > How does that work on systems with swiotlb, e.g.
> > > > > > > > > > > swiotlb=force, or
> > > > > > > > > > > i.e. what would
> > > > > > > > > > > decrypt the ttm tt mappings when copying between system and
> > > > > > > > > > > vram
> > > > > > > > > > > when iommu is
> > > > > > > > > > > disabled/absent?
> > > > > > > > > > SME makes it mandatory that all devices can handle the physical
> > > > > > > > > > address used for DMA, either native or with the help of IOMMU.
> > > > > > > > > >
> > > > > > > > > > Hacks like SWIOTLB are not directly supported as far as I know.
> > > > > > > > > > Maybe
> > > > > > > > > > somehow SWIOTLB manually decrypts the data while copying it or
> > > > > > > > > > something like this, but I'm not 100% sure if that is actually
> > > > > > > > > > implemented.
> > > > > > > > > >
> > > > > > > > > > Regards,
> > > > > > > > > > Christian.
> > > > > > > > > A bold guess after looking at various code and patches:
> > > > > > > > >
> > > > > > > > > 1) Devices under SME that don't support the encryption bit and
> > > > > > > > > SEV:
> > > > > > > > > a) Coherent memory is unencrypted.
> > > > > > > > > b) Streaming DMA under IOMMU: The IOMMU sets the encrypted bit.
> > > > > > > > > c) Streaming DMA with SWIOTLB: The bounce buffer is unencrypted.
> > > > > > > > > Copying to/from bounce-buffer decrypts/encrypts.
> > > > > > > > >
> > > > > > > > > 2) Devices under SME that do support the encryption bit (which I
> > > > > > > > > believe is most graphics devices in general on SME systems, not
> > > > > > > > > just
> > > > > > > > > amdgpu; it "just works")
> > > > > > > > > *) Coherent memory is encrypted. The DMA layer sets dma addresses
> > > > > > > > > and
> > > > > > > > > pgprot accordingly.
> > > > > > > > > *) Streaming DMA is encrypted.
> > > > > > > > >
> > > > > > > > > So the bug in TTM would then be it's not handling 1a) and 1b)
> > > > > > > > > correctly.
> > > > > > > > >
> > > > > > > > > Remedy:
> > > > > > > > > 1b) Shouldn't be used with encryption.
> > > > > > > > > 1a) This is what we should try to fix. Exporting
> > > > > > > > > dma_force_unencrypted() didn't seem to be a way forward. Properly
> > > > > > > > > fixing this would, I guess, mean implement the missing
> > > > > > > > > functionality
> > > > > > > > > in the dma layer: For vmap / kmap we could simply reuse the
> > > > > > > > > virtual
> > > > > > > > > addresses we get back from dma_alloc_coherent(), but for faulting
> > > > > > > > > one
> > > > > > > > > would want something like dma_coherent_insert_pfn() (if it doesn't
> > > > > > > > > exist already) after a proper disussion with Christoph Hellwig.
> > > > > > > > Christoph once pointed me to dma_mmap_attrs() for this, but I never
> > > > > > > > found the time to fully look into it.
> > > > > > > Hmm, yea, that would make sense
> > > > > > > https://elixir.bootlin.com/linux/latest/source/kernel/dma/direct.c#L564
> > > > > > > Replacing the vmap's with dma_mmap_attrs would probably fix this, but
> > > > > > > it would
> > > > > > > require a bit of extra setup.
> > > > > > >
> > > > > > > So we're saying that yes, we don't want unconditional pgprot_decrypt
> > > > > > > in ttm_io_prot.
> > > > > > > We'd like to leave those tt mappings as encrypted when possible and
> > > > > > > instead maybe
> > > > > > > add a vaddr to ttm_tt (or extract it from the pages->private via the
> > > > > > > ttm_pool_dma,
> > > > > > > but that seems rather ugly),
> > > > > > It could probably be extracted from pages->private from a helper in
> > > > > > the ttm pool code, (Christian has a final saying here). However, that
> > > > > > requires that all ttm_tts are built from a single dma_alloc chunk. Not
> > > > > > sure that's the case? In that case we're back to square zero for vmaps.
> > > > > Nope they aren't and yes we are back to square one with that.
> > > > Well, that's my favorite square. Number one, just like me...
> > > >
> > > > Maybe we're overthinking this particular problem a bit. As is use_dma_alloc
> > > > in
> > > > ttm
> > > > is only set in two cases:
> > > > - driver explicitly wants coherent mappings (vmwgfx, which require decrypted
> > > > pages)
> > > > - driver needs swiotlb (which, as was pointed out, would require the pages
> > > > to be
> > > > decrypted as well)
> > > >
> > > > So use_dma_alloc always requires the pages to be decrypted.
> > > IIRC moving forward it doesn't, since there is (or at least there was)
> > > implement missing TTM functionality in the dma layer and most TTM
> > > drivers should at least support dma coherent memory. That means all
> > > devices supporting a sufficiently large dma mask will break with SME and
> > > your proposal then.
> > >
> > > Perhaps if we condition that on
> > > "cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT)" that will capture all the
> > > SEV cases, and limit the existing bug to the hopefully very few TTM
> > > devices with limited dma mask on SME.
> > Ah, I wasn't aware those exist, do you know what platforms are those? I can try
> > to
> > find one around here to see.
>
> My understanding is cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT) will
> return true in the guest iff SEV is active, and can be used in TTM as a
> poor man's force_dma_unencrypted(), enabling the functionality in your
> diff. It looks like a similar check is present in vmwgfx to detect SEV,
> but also see below.
>
> > And they don't really break, they just might unnecessarily decrypt tt pages,
> > right?
>
> No, with SME, dma from hw will encrypt the content, because the dma
> layer will set the "encrypt" bit in the physical address given to the
> iommu or the device in case iommu is not active, but a subsequent
> reading the content using the CPU won't decrypt so CPU and device will
> have different views of the page.
>
> Also the linear kernel mapping PTEs will conflict in encryption mode
> with the ones TTM sets up, and IIRC that's forbidden in the SEV spec.
> (The x86 arch code goes through some serious work to flush out caches
> and TLBs to convert a page kernel linear mapping from encrypted to
> non-encrypted,
>
> https://elixir.bootlin.com/linux/latest/source/arch/x86/mm/pat/set_memory.c#L2129
>
> and that is also seen as pretty heavy dma_alloc_coherent() latency).

Thanks for this!

> So the pgprot_t TTM sets up *must* be identical to the one used by the
> dma layer, so anything we should be aware here that anything we do in
> TTM less than adding needed functionality in the dma layer is
> second-guessing what the dma layer does internally and is not really the
> right solution.

I think this is already the case for virtualized drivers, but I see what you're
saying that fixing this for them might break some real hardware and that's bad.
Playing those games with matching pgprot between ttm and dma is really fragile.

cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT) is also used in drm_need_swiotlb so
adding that check to the last patch would seem to make sense. Of course, it's up to
Christian whether that's robust enough or whether we need to think about the
dma/page fault rework to fix it properly. I'm not sure if I see any other reasonable
solution besides these two options.

z

Re: Decrypting tt maps in ttm

[Thomas Hellström]

On 9/21/23 05:51, Zack Rusin wrote:
> On Wed, 2023-09-20 at 21:22 +0200, Thomas Hellström wrote:
>> !! External Email
>>
>> On 9/20/23 20:24, Zack Rusin wrote:
>>> On Wed, 2023-09-20 at 19:17 +0200, Thomas Hellström wrote:
>>>> !! External Email
>>>>
>>>> Hi, Zack
>>>>
>>>> On 9/20/23 18:39, Zack Rusin wrote:
>>>>> On Wed, 2023-09-20 at 12:48 +0200, Christian König wrote:
>>>>>> !! External Email
>>>>>>
>>>>>> Am 20.09.23 um 09:36 schrieb Thomas Hellström:
>>>>>>> Hi, Zack,
>>>>>>>
>>>>>>> On 9/20/23 05:43, Zack Rusin wrote:
>>>>>>>> On Tue, 2023-09-19 at 09:47 +0200, Christian König wrote:
>>>>>>>>> !! External Email
>>>>>>>>>
>>>>>>>>> Am 19.09.23 um 08:56 schrieb Thomas Hellström:
>>>>>>>>>> On 9/19/23 07:39, Christian König wrote:
>>>>>>>>>>> Am 19.09.23 um 03:26 schrieb Zack Rusin:
>>>>>>>>>>>> On Mon, 2023-09-18 at 16:21 -0400, Alex Deucher wrote:
>>>>>>>>>>>>> !! External Email
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Mon, Sep 18, 2023 at 3:06 PM Thomas Hellström
>>>>>>>>>>>>> <thomas.hellstrom@linux.intel.com> wrote:
>>>>>>>>>>>>>> On 9/18/23 17:52, Zack Rusin wrote:
>>>>>>>>>>>>>>> On Mon, 2023-09-18 at 17:13 +0200, Thomas Hellström
>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On 9/18/23 16:56, Thomas Hellström wrote:
>>>>>>>>>>>>>>>>> Hi Zack, Christian
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On 9/18/23 13:36, Christian König wrote:
>>>>>>>>>>>>>>>>>> Hi Zack,
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> adding Thomas and Daniel.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> I briefly remember that I talked with Thomas and
>>>>>>>>>>>>>>>>>> some
>>>>>>>>>>>>>>>>>> other
>>>>>>>>>>>>>>>>>> people
>>>>>>>>>>>>>>>>>> about that quite a while ago as well, but I don't
>>>>>>>>>>>>>>>>>> fully
>>>>>>>>>>>>>>>>>> remember the
>>>>>>>>>>>>>>>>>> outcome.
>>>>>>>>>>>>>>>>> Found one old thread, but didn't read it:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> https://lists.freedesktop.org/archives/dri-devel/2019-September/234100.html
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> /Thomas
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Ugh. Now starting to read that thread I have a vague
>>>>>>>>>>>>>>>> recollection it all
>>>>>>>>>>>>>>>> ended with not supporting mapping any device pages
>>>>>>>>>>>>>>>> whatsoever
>>>>>>>>>>>>>>>> when SEV
>>>>>>>>>>>>>>>> was enabled, but rather resorting to llvmpipe and VM-
>>>>>>>>>>>>>>>> local
>>>>>>>>>>>>>>>> bos.
>>>>>>>>>>>>>>> Hi, Thomas.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Thanks for finding this! I'd (of course) like to solve
>>>>>>>>>>>>>>> it
>>>>>>>>>>>>>>> properly and get
>>>>>>>>>>>>>>> vmwgfx
>>>>>>>>>>>>>>> running with 3d support with SEV-ES active instead of
>>>>>>>>>>>>>>> essentially
>>>>>>>>>>>>>>> disabling
>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>> driver when SEV-ES is active.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I think there are two separate discussions there, the
>>>>>>>>>>>>>>> non-controversial one
>>>>>>>>>>>>>>> and the
>>>>>>>>>>>>>>> controversial one:
>>>>>>>>>>>>>>> 1) The non-controversial: is there a case where drivers
>>>>>>>>>>>>>>> would
>>>>>>>>>>>>>>> want encrypted
>>>>>>>>>>>>>>> memory
>>>>>>>>>>>>>>> for TT pages but not for io mem mappings? Because if not
>>>>>>>>>>>>>>> then as
>>>>>>>>>>>>>>> Christian
>>>>>>>>>>>>>>> pointed
>>>>>>>>>>>>>>> out we could just add pgprot_decrypted to ttm_io_prot
>>>>>>>>>>>>>>> and be
>>>>>>>>>>>>>>> essentially done.
>>>>>>>>>>>>>>> The
>>>>>>>>>>>>>>> current method of decrypting io mem but leaving sys mem
>>>>>>>>>>>>>>> mappings
>>>>>>>>>>>>>>> encrypted is
>>>>>>>>>>>>>>> a bit
>>>>>>>>>>>>>>> weird anyway.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> If the answer to that question is "yes, some driver does
>>>>>>>>>>>>>>> want
>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>> TT mappings
>>>>>>>>>>>>>>> to be
>>>>>>>>>>>>>>> encrypted" then your "[PATCH v2 3/4] drm/ttm,
>>>>>>>>>>>>>>> drm/vmwgfx:
>>>>>>>>>>>>>>> Correctly support
>>>>>>>>>>>>>>> support
>>>>>>>>>>>>>>> AMD memory encryption" solves that. I think getting one
>>>>>>>>>>>>>>> of
>>>>>>>>>>>>>>> those
>>>>>>>>>>>>>>> two in makes
>>>>>>>>>>>>>>> sense
>>>>>>>>>>>>>>> regardless of everything else, agreed?
>>>>>>>>>>>>>> Well, there is more to it I think.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> IIRC, the AMD SME encryption mode has a way for a device
>>>>>>>>>>>>>> to
>>>>>>>>>>>>>> have the
>>>>>>>>>>>>>> memory controller (?) encrypt / decrypt device traffic by
>>>>>>>>>>>>>> using an
>>>>>>>>>>>>>> address range alias, so in theory it supports encrypted TT
>>>>>>>>>>>>>> pages, and
>>>>>>>>>>>>>> the dma-layer may indeed hand encrypted DMA pages to TTM
>>>>>>>>>>>>>> on
>>>>>>>>>>>>>> such
>>>>>>>>>>>>>> systems
>>>>>>>>>>>>>> depending on the device's DMA mask. That's why I think
>>>>>>>>>>>>>> that
>>>>>>>>>>>>>> force_dma_unencrypted() export was needed, and If the
>>>>>>>>>>>>>> amdgpu
>>>>>>>>>>>>>> driver
>>>>>>>>>>>>>> accesses TT memory in SME mode *without*
>>>>>>>>>>>>>> pgprot_decrypted()
>>>>>>>>>>>>>> and it
>>>>>>>>>>>>>> still
>>>>>>>>>>>>>> works, then I think that mode is actually used. How could
>>>>>>>>>>>>>> it
>>>>>>>>>>>>>> otherwise work?
>>>>>>>>>>>>> For SME, as long as the encrypted bit is set in the physical
>>>>>>>>>>>>> address
>>>>>>>>>>>>> used for DMA, the memory controller will handle the
>>>>>>>>>>>>> encrypt/decrypt
>>>>>>>>>>>>> for the device.  For devices with a limited dma mask, you
>>>>>>>>>>>>> need
>>>>>>>>>>>>> to use
>>>>>>>>>>>>> the IOMMU so that the encrypted bit is retained when the
>>>>>>>>>>>>> address
>>>>>>>>>>>>> hits
>>>>>>>>>>>>> the memory controller.
>>>>>>>>>>>> How does that work on systems with swiotlb, e.g.
>>>>>>>>>>>> swiotlb=force, or
>>>>>>>>>>>> i.e. what would
>>>>>>>>>>>> decrypt the ttm tt mappings when copying between system and
>>>>>>>>>>>> vram
>>>>>>>>>>>> when iommu is
>>>>>>>>>>>> disabled/absent?
>>>>>>>>>>> SME makes it mandatory that all devices can handle the physical
>>>>>>>>>>> address used for DMA, either native or with the help of IOMMU.
>>>>>>>>>>>
>>>>>>>>>>> Hacks like SWIOTLB are not directly supported as far as I know.
>>>>>>>>>>> Maybe
>>>>>>>>>>> somehow SWIOTLB manually decrypts the data while copying it or
>>>>>>>>>>> something like this, but I'm not 100% sure if that is actually
>>>>>>>>>>> implemented.
>>>>>>>>>>>
>>>>>>>>>>> Regards,
>>>>>>>>>>> Christian.
>>>>>>>>>> A bold guess after looking at various code and patches:
>>>>>>>>>>
>>>>>>>>>> 1) Devices under SME that don't support the encryption bit and
>>>>>>>>>> SEV:
>>>>>>>>>> a) Coherent memory is unencrypted.
>>>>>>>>>> b) Streaming DMA under IOMMU: The IOMMU sets the encrypted bit.
>>>>>>>>>> c) Streaming DMA with SWIOTLB: The bounce buffer is unencrypted.
>>>>>>>>>> Copying to/from bounce-buffer decrypts/encrypts.
>>>>>>>>>>
>>>>>>>>>> 2) Devices under SME that do support the encryption bit (which I
>>>>>>>>>> believe is most graphics devices in general on SME systems, not
>>>>>>>>>> just
>>>>>>>>>> amdgpu; it "just works")
>>>>>>>>>> *) Coherent memory is encrypted. The DMA layer sets dma addresses
>>>>>>>>>> and
>>>>>>>>>> pgprot accordingly.
>>>>>>>>>> *) Streaming DMA is encrypted.
>>>>>>>>>>
>>>>>>>>>> So the bug in TTM would then be it's not handling 1a) and 1b)
>>>>>>>>>> correctly.
>>>>>>>>>>
>>>>>>>>>> Remedy:
>>>>>>>>>> 1b) Shouldn't be used with encryption.
>>>>>>>>>> 1a) This is what we should try to fix. Exporting
>>>>>>>>>> dma_force_unencrypted() didn't seem to be a way forward. Properly
>>>>>>>>>> fixing this would, I guess, mean implement the missing
>>>>>>>>>> functionality
>>>>>>>>>> in the dma layer: For vmap / kmap we could simply reuse the
>>>>>>>>>> virtual
>>>>>>>>>> addresses we get back from dma_alloc_coherent(), but for faulting
>>>>>>>>>> one
>>>>>>>>>> would want something like dma_coherent_insert_pfn() (if it doesn't
>>>>>>>>>> exist already) after a proper disussion with Christoph Hellwig.
>>>>>>>>> Christoph once pointed me to dma_mmap_attrs() for this, but I never
>>>>>>>>> found the time to fully look into it.
>>>>>>>> Hmm, yea, that would make sense
>>>>>>>> https://elixir.bootlin.com/linux/latest/source/kernel/dma/direct.c#L564
>>>>>>>> Replacing the vmap's with dma_mmap_attrs would probably fix this, but
>>>>>>>> it would
>>>>>>>> require a bit of extra setup.
>>>>>>>>
>>>>>>>> So we're saying that yes, we don't want unconditional pgprot_decrypt
>>>>>>>> in ttm_io_prot.
>>>>>>>> We'd like to leave those tt mappings as encrypted when possible and
>>>>>>>> instead maybe
>>>>>>>> add a vaddr to ttm_tt (or extract it from the pages->private via the
>>>>>>>> ttm_pool_dma,
>>>>>>>> but that seems rather ugly),
>>>>>>> It could probably be extracted from pages->private from a helper in
>>>>>>> the ttm pool code, (Christian has a final saying here). However, that
>>>>>>> requires that all ttm_tts are built from a single dma_alloc chunk. Not
>>>>>>> sure that's the case? In that case we're back to square zero for vmaps.
>>>>>> Nope they aren't and yes we are back to square one with that.
>>>>> Well, that's my favorite square. Number one, just like me...
>>>>>
>>>>> Maybe we're overthinking this particular problem a bit. As is use_dma_alloc
>>>>> in
>>>>> ttm
>>>>> is only set in two cases:
>>>>> - driver explicitly wants coherent mappings (vmwgfx, which require decrypted
>>>>> pages)
>>>>> - driver needs swiotlb (which, as was pointed out, would require the pages
>>>>> to be
>>>>> decrypted as well)
>>>>>
>>>>> So use_dma_alloc always requires the pages to be decrypted.
>>>> IIRC moving forward it doesn't, since there is (or at least there was)
>>>> implement missing TTM functionality in the dma layer and most TTM
>>>> drivers should at least support dma coherent memory. That means all
>>>> devices supporting a sufficiently large dma mask will break with SME and
>>>> your proposal then.
>>>>
>>>> Perhaps if we condition that on
>>>> "cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT)" that will capture all the
>>>> SEV cases, and limit the existing bug to the hopefully very few TTM
>>>> devices with limited dma mask on SME.
>>> Ah, I wasn't aware those exist, do you know what platforms are those? I can try
>>> to
>>> find one around here to see.
>> My understanding is cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT) will
>> return true in the guest iff SEV is active, and can be used in TTM as a
>> poor man's force_dma_unencrypted(), enabling the functionality in your
>> diff. It looks like a similar check is present in vmwgfx to detect SEV,
>> but also see below.
>>
>>> And they don't really break, they just might unnecessarily decrypt tt pages,
>>> right?
>> No, with SME, dma from hw will encrypt the content, because the dma
>> layer will set the "encrypt" bit in the physical address given to the
>> iommu or the device in case iommu is not active, but a subsequent
>> reading the content using the CPU won't decrypt so CPU and device will
>> have different views of the page.
>>
>> Also the linear kernel mapping PTEs will conflict in encryption mode
>> with the ones TTM sets up, and IIRC that's forbidden in the SEV spec.
>> (The x86 arch code goes through some serious work to flush out caches
>> and TLBs to convert a page kernel linear mapping from encrypted to
>> non-encrypted,
>>
>> https://elixir.bootlin.com/linux/latest/source/arch/x86/mm/pat/set_memory.c#L2129
>>
>> and that is also seen as pretty heavy dma_alloc_coherent() latency).
> Thanks for this!
>
>> So the pgprot_t TTM sets up *must* be identical to the one used by the
>> dma layer, so anything we should be aware here that anything we do in
>> TTM less than adding needed functionality in the dma layer is
>> second-guessing what the dma layer does internally and is not really the
>> right solution.
> I think this is already the case for virtualized drivers, but I see what you're
> saying that fixing this for them might break some real hardware and that's bad.
> Playing those games with matching pgprot between ttm and dma is really fragile.
>
> cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT) is also used in drm_need_swiotlb so
> adding that check to the last patch would seem to make sense. Of course, it's up to
> Christian whether that's robust enough or whether we need to think about the
> dma/page fault rework to fix it properly. I'm not sure if I see any other reasonable
> solution besides these two options.
Agreed.

/Thomas


>
> z

Re: Decrypting tt maps in ttm

[Christian König]

Am 21.09.23 um 09:12 schrieb Thomas Hellström:
>
> On 9/21/23 05:51, Zack Rusin wrote:
>> On Wed, 2023-09-20 at 21:22 +0200, Thomas Hellström wrote:
>>> !! External Email
>>>
>>> On 9/20/23 20:24, Zack Rusin wrote:
>>>> On Wed, 2023-09-20 at 19:17 +0200, Thomas Hellström wrote:
>>>>> !! External Email
>>>>>
>>>>> Hi, Zack
>>>>>
>>>>> On 9/20/23 18:39, Zack Rusin wrote:
>>>>>> On Wed, 2023-09-20 at 12:48 +0200, Christian König wrote:
>>>>>>> !! External Email
>>>>>>>
>>>>>>> Am 20.09.23 um 09:36 schrieb Thomas Hellström:
>>>>>>>> Hi, Zack,
>>>>>>>>
>>>>>>>> On 9/20/23 05:43, Zack Rusin wrote:
>>>>>>>>> On Tue, 2023-09-19 at 09:47 +0200, Christian König wrote:
>>>>>>>>>> !! External Email
>>>>>>>>>>
>>>>>>>>>> Am 19.09.23 um 08:56 schrieb Thomas Hellström:
>>>>>>>>>>> On 9/19/23 07:39, Christian König wrote:
>>>>>>>>>>>> Am 19.09.23 um 03:26 schrieb Zack Rusin:
>>>>>>>>>>>>> On Mon, 2023-09-18 at 16:21 -0400, Alex Deucher wrote:
>>>>>>>>>>>>>> !! External Email
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Mon, Sep 18, 2023 at 3:06 PM Thomas Hellström
>>>>>>>>>>>>>> <thomas.hellstrom@linux.intel.com> wrote:
>>>>>>>>>>>>>>> On 9/18/23 17:52, Zack Rusin wrote:
>>>>>>>>>>>>>>>> On Mon, 2023-09-18 at 17:13 +0200, Thomas Hellström
>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On 9/18/23 16:56, Thomas Hellström wrote:
>>>>>>>>>>>>>>>>>> Hi Zack, Christian
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> On 9/18/23 13:36, Christian König wrote:
>>>>>>>>>>>>>>>>>>> Hi Zack,
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> adding Thomas and Daniel.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> I briefly remember that I talked with Thomas and
>>>>>>>>>>>>>>>>>>> some
>>>>>>>>>>>>>>>>>>> other
>>>>>>>>>>>>>>>>>>> people
>>>>>>>>>>>>>>>>>>> about that quite a while ago as well, but I don't
>>>>>>>>>>>>>>>>>>> fully
>>>>>>>>>>>>>>>>>>> remember the
>>>>>>>>>>>>>>>>>>> outcome.
>>>>>>>>>>>>>>>>>> Found one old thread, but didn't read it:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> https://lists.freedesktop.org/archives/dri-devel/2019-September/234100.html 
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> /Thomas
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Ugh. Now starting to read that thread I have a vague
>>>>>>>>>>>>>>>>> recollection it all
>>>>>>>>>>>>>>>>> ended with not supporting mapping any device pages
>>>>>>>>>>>>>>>>> whatsoever
>>>>>>>>>>>>>>>>> when SEV
>>>>>>>>>>>>>>>>> was enabled, but rather resorting to llvmpipe and VM-
>>>>>>>>>>>>>>>>> local
>>>>>>>>>>>>>>>>> bos.
>>>>>>>>>>>>>>>> Hi, Thomas.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Thanks for finding this! I'd (of course) like to solve
>>>>>>>>>>>>>>>> it
>>>>>>>>>>>>>>>> properly and get
>>>>>>>>>>>>>>>> vmwgfx
>>>>>>>>>>>>>>>> running with 3d support with SEV-ES active instead of
>>>>>>>>>>>>>>>> essentially
>>>>>>>>>>>>>>>> disabling
>>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>> driver when SEV-ES is active.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> I think there are two separate discussions there, the
>>>>>>>>>>>>>>>> non-controversial one
>>>>>>>>>>>>>>>> and the
>>>>>>>>>>>>>>>> controversial one:
>>>>>>>>>>>>>>>> 1) The non-controversial: is there a case where drivers
>>>>>>>>>>>>>>>> would
>>>>>>>>>>>>>>>> want encrypted
>>>>>>>>>>>>>>>> memory
>>>>>>>>>>>>>>>> for TT pages but not for io mem mappings? Because if not
>>>>>>>>>>>>>>>> then as
>>>>>>>>>>>>>>>> Christian
>>>>>>>>>>>>>>>> pointed
>>>>>>>>>>>>>>>> out we could just add pgprot_decrypted to ttm_io_prot
>>>>>>>>>>>>>>>> and be
>>>>>>>>>>>>>>>> essentially done.
>>>>>>>>>>>>>>>> The
>>>>>>>>>>>>>>>> current method of decrypting io mem but leaving sys mem
>>>>>>>>>>>>>>>> mappings
>>>>>>>>>>>>>>>> encrypted is
>>>>>>>>>>>>>>>> a bit
>>>>>>>>>>>>>>>> weird anyway.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> If the answer to that question is "yes, some driver does
>>>>>>>>>>>>>>>> want
>>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>> TT mappings
>>>>>>>>>>>>>>>> to be
>>>>>>>>>>>>>>>> encrypted" then your "[PATCH v2 3/4] drm/ttm,
>>>>>>>>>>>>>>>> drm/vmwgfx:
>>>>>>>>>>>>>>>> Correctly support
>>>>>>>>>>>>>>>> support
>>>>>>>>>>>>>>>> AMD memory encryption" solves that. I think getting one
>>>>>>>>>>>>>>>> of
>>>>>>>>>>>>>>>> those
>>>>>>>>>>>>>>>> two in makes
>>>>>>>>>>>>>>>> sense
>>>>>>>>>>>>>>>> regardless of everything else, agreed?
>>>>>>>>>>>>>>> Well, there is more to it I think.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> IIRC, the AMD SME encryption mode has a way for a device
>>>>>>>>>>>>>>> to
>>>>>>>>>>>>>>> have the
>>>>>>>>>>>>>>> memory controller (?) encrypt / decrypt device traffic by
>>>>>>>>>>>>>>> using an
>>>>>>>>>>>>>>> address range alias, so in theory it supports encrypted TT
>>>>>>>>>>>>>>> pages, and
>>>>>>>>>>>>>>> the dma-layer may indeed hand encrypted DMA pages to TTM
>>>>>>>>>>>>>>> on
>>>>>>>>>>>>>>> such
>>>>>>>>>>>>>>> systems
>>>>>>>>>>>>>>> depending on the device's DMA mask. That's why I think
>>>>>>>>>>>>>>> that
>>>>>>>>>>>>>>> force_dma_unencrypted() export was needed, and If the
>>>>>>>>>>>>>>> amdgpu
>>>>>>>>>>>>>>> driver
>>>>>>>>>>>>>>> accesses TT memory in SME mode *without*
>>>>>>>>>>>>>>> pgprot_decrypted()
>>>>>>>>>>>>>>> and it
>>>>>>>>>>>>>>> still
>>>>>>>>>>>>>>> works, then I think that mode is actually used. How could
>>>>>>>>>>>>>>> it
>>>>>>>>>>>>>>> otherwise work?
>>>>>>>>>>>>>> For SME, as long as the encrypted bit is set in the physical
>>>>>>>>>>>>>> address
>>>>>>>>>>>>>> used for DMA, the memory controller will handle the
>>>>>>>>>>>>>> encrypt/decrypt
>>>>>>>>>>>>>> for the device.  For devices with a limited dma mask, you
>>>>>>>>>>>>>> need
>>>>>>>>>>>>>> to use
>>>>>>>>>>>>>> the IOMMU so that the encrypted bit is retained when the
>>>>>>>>>>>>>> address
>>>>>>>>>>>>>> hits
>>>>>>>>>>>>>> the memory controller.
>>>>>>>>>>>>> How does that work on systems with swiotlb, e.g.
>>>>>>>>>>>>> swiotlb=force, or
>>>>>>>>>>>>> i.e. what would
>>>>>>>>>>>>> decrypt the ttm tt mappings when copying between system and
>>>>>>>>>>>>> vram
>>>>>>>>>>>>> when iommu is
>>>>>>>>>>>>> disabled/absent?
>>>>>>>>>>>> SME makes it mandatory that all devices can handle the 
>>>>>>>>>>>> physical
>>>>>>>>>>>> address used for DMA, either native or with the help of IOMMU.
>>>>>>>>>>>>
>>>>>>>>>>>> Hacks like SWIOTLB are not directly supported as far as I 
>>>>>>>>>>>> know.
>>>>>>>>>>>> Maybe
>>>>>>>>>>>> somehow SWIOTLB manually decrypts the data while copying it or
>>>>>>>>>>>> something like this, but I'm not 100% sure if that is actually
>>>>>>>>>>>> implemented.
>>>>>>>>>>>>
>>>>>>>>>>>> Regards,
>>>>>>>>>>>> Christian.
>>>>>>>>>>> A bold guess after looking at various code and patches:
>>>>>>>>>>>
>>>>>>>>>>> 1) Devices under SME that don't support the encryption bit and
>>>>>>>>>>> SEV:
>>>>>>>>>>> a) Coherent memory is unencrypted.
>>>>>>>>>>> b) Streaming DMA under IOMMU: The IOMMU sets the encrypted bit.
>>>>>>>>>>> c) Streaming DMA with SWIOTLB: The bounce buffer is 
>>>>>>>>>>> unencrypted.
>>>>>>>>>>> Copying to/from bounce-buffer decrypts/encrypts.
>>>>>>>>>>>
>>>>>>>>>>> 2) Devices under SME that do support the encryption bit 
>>>>>>>>>>> (which I
>>>>>>>>>>> believe is most graphics devices in general on SME systems, not
>>>>>>>>>>> just
>>>>>>>>>>> amdgpu; it "just works")
>>>>>>>>>>> *) Coherent memory is encrypted. The DMA layer sets dma 
>>>>>>>>>>> addresses
>>>>>>>>>>> and
>>>>>>>>>>> pgprot accordingly.
>>>>>>>>>>> *) Streaming DMA is encrypted.
>>>>>>>>>>>
>>>>>>>>>>> So the bug in TTM would then be it's not handling 1a) and 1b)
>>>>>>>>>>> correctly.
>>>>>>>>>>>
>>>>>>>>>>> Remedy:
>>>>>>>>>>> 1b) Shouldn't be used with encryption.
>>>>>>>>>>> 1a) This is what we should try to fix. Exporting
>>>>>>>>>>> dma_force_unencrypted() didn't seem to be a way forward. 
>>>>>>>>>>> Properly
>>>>>>>>>>> fixing this would, I guess, mean implement the missing
>>>>>>>>>>> functionality
>>>>>>>>>>> in the dma layer: For vmap / kmap we could simply reuse the
>>>>>>>>>>> virtual
>>>>>>>>>>> addresses we get back from dma_alloc_coherent(), but for 
>>>>>>>>>>> faulting
>>>>>>>>>>> one
>>>>>>>>>>> would want something like dma_coherent_insert_pfn() (if it 
>>>>>>>>>>> doesn't
>>>>>>>>>>> exist already) after a proper disussion with Christoph Hellwig.
>>>>>>>>>> Christoph once pointed me to dma_mmap_attrs() for this, but I 
>>>>>>>>>> never
>>>>>>>>>> found the time to fully look into it.
>>>>>>>>> Hmm, yea, that would make sense
>>>>>>>>> https://elixir.bootlin.com/linux/latest/source/kernel/dma/direct.c#L564 
>>>>>>>>>
>>>>>>>>> Replacing the vmap's with dma_mmap_attrs would probably fix 
>>>>>>>>> this, but
>>>>>>>>> it would
>>>>>>>>> require a bit of extra setup.
>>>>>>>>>
>>>>>>>>> So we're saying that yes, we don't want unconditional 
>>>>>>>>> pgprot_decrypt
>>>>>>>>> in ttm_io_prot.
>>>>>>>>> We'd like to leave those tt mappings as encrypted when 
>>>>>>>>> possible and
>>>>>>>>> instead maybe
>>>>>>>>> add a vaddr to ttm_tt (or extract it from the pages->private 
>>>>>>>>> via the
>>>>>>>>> ttm_pool_dma,
>>>>>>>>> but that seems rather ugly),
>>>>>>>> It could probably be extracted from pages->private from a 
>>>>>>>> helper in
>>>>>>>> the ttm pool code, (Christian has a final saying here). 
>>>>>>>> However, that
>>>>>>>> requires that all ttm_tts are built from a single dma_alloc 
>>>>>>>> chunk. Not
>>>>>>>> sure that's the case? In that case we're back to square zero 
>>>>>>>> for vmaps.
>>>>>>> Nope they aren't and yes we are back to square one with that.
>>>>>> Well, that's my favorite square. Number one, just like me...
>>>>>>
>>>>>> Maybe we're overthinking this particular problem a bit. As is 
>>>>>> use_dma_alloc
>>>>>> in
>>>>>> ttm
>>>>>> is only set in two cases:
>>>>>> - driver explicitly wants coherent mappings (vmwgfx, which 
>>>>>> require decrypted
>>>>>> pages)
>>>>>> - driver needs swiotlb (which, as was pointed out, would require 
>>>>>> the pages
>>>>>> to be
>>>>>> decrypted as well)
>>>>>>
>>>>>> So use_dma_alloc always requires the pages to be decrypted.
>>>>> IIRC moving forward it doesn't, since there is (or at least there 
>>>>> was)
>>>>> implement missing TTM functionality in the dma layer and most TTM
>>>>> drivers should at least support dma coherent memory. That means all
>>>>> devices supporting a sufficiently large dma mask will break with 
>>>>> SME and
>>>>> your proposal then.
>>>>>
>>>>> Perhaps if we condition that on
>>>>> "cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT)" that will capture all 
>>>>> the
>>>>> SEV cases, and limit the existing bug to the hopefully very few TTM
>>>>> devices with limited dma mask on SME.
>>>> Ah, I wasn't aware those exist, do you know what platforms are 
>>>> those? I can try
>>>> to
>>>> find one around here to see.
>>> My understanding is cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT) will
>>> return true in the guest iff SEV is active, and can be used in TTM as a
>>> poor man's force_dma_unencrypted(), enabling the functionality in your
>>> diff. It looks like a similar check is present in vmwgfx to detect SEV,
>>> but also see below.
>>>
>>>> And they don't really break, they just might unnecessarily decrypt 
>>>> tt pages,
>>>> right?
>>> No, with SME, dma from hw will encrypt the content, because the dma
>>> layer will set the "encrypt" bit in the physical address given to the
>>> iommu or the device in case iommu is not active, but a subsequent
>>> reading the content using the CPU won't decrypt so CPU and device will
>>> have different views of the page.
>>>
>>> Also the linear kernel mapping PTEs will conflict in encryption mode
>>> with the ones TTM sets up, and IIRC that's forbidden in the SEV spec.
>>> (The x86 arch code goes through some serious work to flush out caches
>>> and TLBs to convert a page kernel linear mapping from encrypted to
>>> non-encrypted,
>>>
>>> https://elixir.bootlin.com/linux/latest/source/arch/x86/mm/pat/set_memory.c#L2129 
>>>
>>>
>>> and that is also seen as pretty heavy dma_alloc_coherent() latency).
>> Thanks for this!
>>
>>> So the pgprot_t TTM sets up *must* be identical to the one used by the
>>> dma layer, so anything we should be aware here that anything we do in
>>> TTM less than adding needed functionality in the dma layer is
>>> second-guessing what the dma layer does internally and is not really 
>>> the
>>> right solution.
>> I think this is already the case for virtualized drivers, but I see 
>> what you're
>> saying that fixing this for them might break some real hardware and 
>> that's bad.
>> Playing those games with matching pgprot between ttm and dma is 
>> really fragile.
>>
>> cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT) is also used in 
>> drm_need_swiotlb so
>> adding that check to the last patch would seem to make sense. Of 
>> course, it's up to
>> Christian whether that's robust enough or whether we need to think 
>> about the
>> dma/page fault rework to fix it properly. I'm not sure if I see any 
>> other reasonable
>> solution besides these two options.
> Agreed.

Uff, of hand that looks like the right thing to do. But I'm really not 
an expert for that stuff.

I think the best thing you can do is to write a patch and send it to 
LKML and dri-devel and see if anybody objects.

Christian.

>
> /Thomas
>
>
>>
>> z