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@xxxxxxxxxxxxxxx> 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
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;
