Message ID | 1569419090-5304-12-git-send-email-ray.huang@amd.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | drm/amdgpu: introduce secure buffer object support (trusted memory zone) | expand |
Am 25.09.19 um 15:45 schrieb Huang, Ray: > From: Alex Deucher <alexander.deucher@amd.com> > > If a buffer object is secure, i.e. created with > AMDGPU_GEM_CREATE_ENCRYPTED, then the TMZ bit of > the PTEs that belong the buffer object should be > set. > > v1: design and draft the skeletion of TMZ bits setting on PTEs (Alex) > v2: return failure once create secure BO on non-TMZ platform (Ray) > v3: amdgpu_bo_encrypted() only checks the BO (Luben) > v4: move TMZ flag setting into amdgpu_vm_bo_update (Christian) > > Signed-off-by: Alex Deucher <alexander.deucher@amd.com> > Reviewed-by: Huang Rui <ray.huang@amd.com> > Signed-off-by: Huang Rui <ray.huang@amd.com> > Signed-off-by: Luben Tuikov <luben.tuikov@amd.com> > Reviewed-by: Alex Deucher <alexander.deucher@amd.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 12 +++++++++++- > drivers/gpu/drm/amd/amdgpu/amdgpu_object.h | 11 +++++++++++ > drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 5 +++++ > 3 files changed, 27 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c > index 22eab74..5332104 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c > @@ -222,7 +222,8 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data, > AMDGPU_GEM_CREATE_CPU_GTT_USWC | > AMDGPU_GEM_CREATE_VRAM_CLEARED | > AMDGPU_GEM_CREATE_VM_ALWAYS_VALID | > - AMDGPU_GEM_CREATE_EXPLICIT_SYNC)) > + AMDGPU_GEM_CREATE_EXPLICIT_SYNC | > + AMDGPU_GEM_CREATE_ENCRYPTED)) > > return -EINVAL; > > @@ -230,6 +231,11 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data, > if (args->in.domains & ~AMDGPU_GEM_DOMAIN_MASK) > return -EINVAL; > > + if (!adev->tmz.enabled && (flags & AMDGPU_GEM_CREATE_ENCRYPTED)) { > + DRM_ERROR("Cannot allocate secure buffer while tmz is disabled\n"); > + return -EINVAL; > + } > + > /* create a gem object to contain this object in */ > if (args->in.domains & (AMDGPU_GEM_DOMAIN_GDS | > AMDGPU_GEM_DOMAIN_GWS | AMDGPU_GEM_DOMAIN_OA)) { > @@ -251,6 +257,10 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data, > resv = vm->root.base.bo->tbo.resv; > } > > + if (flags & AMDGPU_GEM_CREATE_ENCRYPTED) { > + /* XXX: pad out alignment to meet TMZ requirements */ > + } > + > r = amdgpu_gem_object_create(adev, size, args->in.alignment, > (u32)(0xffffffff & args->in.domains), > flags, ttm_bo_type_device, resv, &gobj); > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h > index 5a3c177..75c7392 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h > @@ -224,6 +224,17 @@ static inline bool amdgpu_bo_explicit_sync(struct amdgpu_bo *bo) > return bo->flags & AMDGPU_GEM_CREATE_EXPLICIT_SYNC; > } > > +/** > + * amdgpu_bo_encrypted - test if the BO is encrypted > + * @bo: pointer to a buffer object > + * > + * Return true if the buffer object is encrypted, false otherwise. > + */ > +static inline bool amdgpu_bo_encrypted(struct amdgpu_bo *bo) > +{ > + return bo->flags & AMDGPU_GEM_CREATE_ENCRYPTED; > +} > + > bool amdgpu_bo_is_amdgpu_bo(struct ttm_buffer_object *bo); > void amdgpu_bo_placement_from_domain(struct amdgpu_bo *abo, u32 domain); > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > index b285ab2..8e13b1fd3 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > @@ -1688,6 +1688,11 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev, > > if (bo) { > flags = amdgpu_ttm_tt_pte_flags(adev, bo->tbo.ttm, mem); > + > + if (amdgpu_bo_encrypted(bo)) { > + flags |= AMDGPU_PTE_TMZ; > + } > + You can drop the {} here, apart from that the patch is Reviewed-by: Christian König <christian.koenig@amd.com>. From the design it would be indeed nicer to have that in amdgpu_ttm_tt_pte_flags(), but we would need to make sure that this is not called any more from binding a tt. Regards, Christian. > bo_adev = amdgpu_ttm_adev(bo->tbo.bdev); > } else { > flags = 0x0;
On Wed, Sep 25, 2019 at 9:59 AM Koenig, Christian <Christian.Koenig@amd.com> wrote: > > Am 25.09.19 um 15:45 schrieb Huang, Ray: > > From: Alex Deucher <alexander.deucher@amd.com> > > > > If a buffer object is secure, i.e. created with > > AMDGPU_GEM_CREATE_ENCRYPTED, then the TMZ bit of > > the PTEs that belong the buffer object should be > > set. > > > > v1: design and draft the skeletion of TMZ bits setting on PTEs (Alex) > > v2: return failure once create secure BO on non-TMZ platform (Ray) > > v3: amdgpu_bo_encrypted() only checks the BO (Luben) > > v4: move TMZ flag setting into amdgpu_vm_bo_update (Christian) > > > > Signed-off-by: Alex Deucher <alexander.deucher@amd.com> > > Reviewed-by: Huang Rui <ray.huang@amd.com> > > Signed-off-by: Huang Rui <ray.huang@amd.com> > > Signed-off-by: Luben Tuikov <luben.tuikov@amd.com> > > Reviewed-by: Alex Deucher <alexander.deucher@amd.com> > > --- > > drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 12 +++++++++++- > > drivers/gpu/drm/amd/amdgpu/amdgpu_object.h | 11 +++++++++++ > > drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 5 +++++ > > 3 files changed, 27 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c > > index 22eab74..5332104 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c > > @@ -222,7 +222,8 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data, > > AMDGPU_GEM_CREATE_CPU_GTT_USWC | > > AMDGPU_GEM_CREATE_VRAM_CLEARED | > > AMDGPU_GEM_CREATE_VM_ALWAYS_VALID | > > - AMDGPU_GEM_CREATE_EXPLICIT_SYNC)) > > + AMDGPU_GEM_CREATE_EXPLICIT_SYNC | > > + AMDGPU_GEM_CREATE_ENCRYPTED)) > > > > return -EINVAL; > > > > @@ -230,6 +231,11 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data, > > if (args->in.domains & ~AMDGPU_GEM_DOMAIN_MASK) > > return -EINVAL; > > > > + if (!adev->tmz.enabled && (flags & AMDGPU_GEM_CREATE_ENCRYPTED)) { > > + DRM_ERROR("Cannot allocate secure buffer while tmz is disabled\n"); > > + return -EINVAL; > > + } > > + > > /* create a gem object to contain this object in */ > > if (args->in.domains & (AMDGPU_GEM_DOMAIN_GDS | > > AMDGPU_GEM_DOMAIN_GWS | AMDGPU_GEM_DOMAIN_OA)) { > > @@ -251,6 +257,10 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data, > > resv = vm->root.base.bo->tbo.resv; > > } > > > > + if (flags & AMDGPU_GEM_CREATE_ENCRYPTED) { > > + /* XXX: pad out alignment to meet TMZ requirements */ > > + } > > + > > r = amdgpu_gem_object_create(adev, size, args->in.alignment, > > (u32)(0xffffffff & args->in.domains), > > flags, ttm_bo_type_device, resv, &gobj); > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h > > index 5a3c177..75c7392 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h > > @@ -224,6 +224,17 @@ static inline bool amdgpu_bo_explicit_sync(struct amdgpu_bo *bo) > > return bo->flags & AMDGPU_GEM_CREATE_EXPLICIT_SYNC; > > } > > > > +/** > > + * amdgpu_bo_encrypted - test if the BO is encrypted > > + * @bo: pointer to a buffer object > > + * > > + * Return true if the buffer object is encrypted, false otherwise. > > + */ > > +static inline bool amdgpu_bo_encrypted(struct amdgpu_bo *bo) > > +{ > > + return bo->flags & AMDGPU_GEM_CREATE_ENCRYPTED; > > +} > > + > > bool amdgpu_bo_is_amdgpu_bo(struct ttm_buffer_object *bo); > > void amdgpu_bo_placement_from_domain(struct amdgpu_bo *abo, u32 domain); > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > > index b285ab2..8e13b1fd3 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > > @@ -1688,6 +1688,11 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev, > > > > if (bo) { > > flags = amdgpu_ttm_tt_pte_flags(adev, bo->tbo.ttm, mem); > > + > > + if (amdgpu_bo_encrypted(bo)) { > > + flags |= AMDGPU_PTE_TMZ; > > + } > > + > > You can drop the {} here, apart from that the patch is Reviewed-by: > Christian König <christian.koenig@amd.com>. > > From the design it would be indeed nicer to have that in > amdgpu_ttm_tt_pte_flags(), but we would need to make sure that this is > not called any more from binding a tt. Don't we need that for things like evictions of tmz buffers? That needs special handling in general, which I guess we can address at that time. Alex > > Regards, > Christian. > > > bo_adev = amdgpu_ttm_adev(bo->tbo.bdev); > > } else { > > flags = 0x0; > > _______________________________________________ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Am 25.09.19 um 18:01 schrieb Alex Deucher: > On Wed, Sep 25, 2019 at 9:59 AM Koenig, Christian > <Christian.Koenig@amd.com> wrote: >> Am 25.09.19 um 15:45 schrieb Huang, Ray: >>> From: Alex Deucher <alexander.deucher@amd.com> >>> >>> If a buffer object is secure, i.e. created with >>> AMDGPU_GEM_CREATE_ENCRYPTED, then the TMZ bit of >>> the PTEs that belong the buffer object should be >>> set. >>> >>> v1: design and draft the skeletion of TMZ bits setting on PTEs (Alex) >>> v2: return failure once create secure BO on non-TMZ platform (Ray) >>> v3: amdgpu_bo_encrypted() only checks the BO (Luben) >>> v4: move TMZ flag setting into amdgpu_vm_bo_update (Christian) >>> >>> Signed-off-by: Alex Deucher <alexander.deucher@amd.com> >>> Reviewed-by: Huang Rui <ray.huang@amd.com> >>> Signed-off-by: Huang Rui <ray.huang@amd.com> >>> Signed-off-by: Luben Tuikov <luben.tuikov@amd.com> >>> Reviewed-by: Alex Deucher <alexander.deucher@amd.com> >>> --- >>> drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 12 +++++++++++- >>> drivers/gpu/drm/amd/amdgpu/amdgpu_object.h | 11 +++++++++++ >>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 5 +++++ >>> 3 files changed, 27 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c >>> index 22eab74..5332104 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c >>> @@ -222,7 +222,8 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data, >>> AMDGPU_GEM_CREATE_CPU_GTT_USWC | >>> AMDGPU_GEM_CREATE_VRAM_CLEARED | >>> AMDGPU_GEM_CREATE_VM_ALWAYS_VALID | >>> - AMDGPU_GEM_CREATE_EXPLICIT_SYNC)) >>> + AMDGPU_GEM_CREATE_EXPLICIT_SYNC | >>> + AMDGPU_GEM_CREATE_ENCRYPTED)) >>> >>> return -EINVAL; >>> >>> @@ -230,6 +231,11 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data, >>> if (args->in.domains & ~AMDGPU_GEM_DOMAIN_MASK) >>> return -EINVAL; >>> >>> + if (!adev->tmz.enabled && (flags & AMDGPU_GEM_CREATE_ENCRYPTED)) { >>> + DRM_ERROR("Cannot allocate secure buffer while tmz is disabled\n"); >>> + return -EINVAL; >>> + } >>> + >>> /* create a gem object to contain this object in */ >>> if (args->in.domains & (AMDGPU_GEM_DOMAIN_GDS | >>> AMDGPU_GEM_DOMAIN_GWS | AMDGPU_GEM_DOMAIN_OA)) { >>> @@ -251,6 +257,10 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data, >>> resv = vm->root.base.bo->tbo.resv; >>> } >>> >>> + if (flags & AMDGPU_GEM_CREATE_ENCRYPTED) { >>> + /* XXX: pad out alignment to meet TMZ requirements */ >>> + } >>> + >>> r = amdgpu_gem_object_create(adev, size, args->in.alignment, >>> (u32)(0xffffffff & args->in.domains), >>> flags, ttm_bo_type_device, resv, &gobj); >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h >>> index 5a3c177..75c7392 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h >>> @@ -224,6 +224,17 @@ static inline bool amdgpu_bo_explicit_sync(struct amdgpu_bo *bo) >>> return bo->flags & AMDGPU_GEM_CREATE_EXPLICIT_SYNC; >>> } >>> >>> +/** >>> + * amdgpu_bo_encrypted - test if the BO is encrypted >>> + * @bo: pointer to a buffer object >>> + * >>> + * Return true if the buffer object is encrypted, false otherwise. >>> + */ >>> +static inline bool amdgpu_bo_encrypted(struct amdgpu_bo *bo) >>> +{ >>> + return bo->flags & AMDGPU_GEM_CREATE_ENCRYPTED; >>> +} >>> + >>> bool amdgpu_bo_is_amdgpu_bo(struct ttm_buffer_object *bo); >>> void amdgpu_bo_placement_from_domain(struct amdgpu_bo *abo, u32 domain); >>> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >>> index b285ab2..8e13b1fd3 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >>> @@ -1688,6 +1688,11 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev, >>> >>> if (bo) { >>> flags = amdgpu_ttm_tt_pte_flags(adev, bo->tbo.ttm, mem); >>> + >>> + if (amdgpu_bo_encrypted(bo)) { >>> + flags |= AMDGPU_PTE_TMZ; >>> + } >>> + >> You can drop the {} here, apart from that the patch is Reviewed-by: >> Christian König <christian.koenig@amd.com>. >> >> From the design it would be indeed nicer to have that in >> amdgpu_ttm_tt_pte_flags(), but we would need to make sure that this is >> not called any more from binding a tt. > Don't we need that for things like evictions of tmz buffers? That > needs special handling in general, which I guess we can address at > that time. Yeah, exactly. The problem is that we sometimes doesn't use bo->mem, but rather a separate memory descriptor to get the PTE infos. I think we could remove the call in amdgpu_ttm_backend_bind() where this is relevant, because we now manage GART separately. But that's a larger change and probably not something for this patch set. Going to put it on my TODO list. Christian. > > Alex > >> Regards, >> Christian. >> >>> bo_adev = amdgpu_ttm_adev(bo->tbo.bdev); >>> } else { >>> flags = 0x0; >> _______________________________________________ >> amd-gfx mailing list >> amd-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx > _______________________________________________ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c index 22eab74..5332104 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c @@ -222,7 +222,8 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data, AMDGPU_GEM_CREATE_CPU_GTT_USWC | AMDGPU_GEM_CREATE_VRAM_CLEARED | AMDGPU_GEM_CREATE_VM_ALWAYS_VALID | - AMDGPU_GEM_CREATE_EXPLICIT_SYNC)) + AMDGPU_GEM_CREATE_EXPLICIT_SYNC | + AMDGPU_GEM_CREATE_ENCRYPTED)) return -EINVAL; @@ -230,6 +231,11 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data, if (args->in.domains & ~AMDGPU_GEM_DOMAIN_MASK) return -EINVAL; + if (!adev->tmz.enabled && (flags & AMDGPU_GEM_CREATE_ENCRYPTED)) { + DRM_ERROR("Cannot allocate secure buffer while tmz is disabled\n"); + return -EINVAL; + } + /* create a gem object to contain this object in */ if (args->in.domains & (AMDGPU_GEM_DOMAIN_GDS | AMDGPU_GEM_DOMAIN_GWS | AMDGPU_GEM_DOMAIN_OA)) { @@ -251,6 +257,10 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data, resv = vm->root.base.bo->tbo.resv; } + if (flags & AMDGPU_GEM_CREATE_ENCRYPTED) { + /* XXX: pad out alignment to meet TMZ requirements */ + } + r = amdgpu_gem_object_create(adev, size, args->in.alignment, (u32)(0xffffffff & args->in.domains), flags, ttm_bo_type_device, resv, &gobj); diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h index 5a3c177..75c7392 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h @@ -224,6 +224,17 @@ static inline bool amdgpu_bo_explicit_sync(struct amdgpu_bo *bo) return bo->flags & AMDGPU_GEM_CREATE_EXPLICIT_SYNC; } +/** + * amdgpu_bo_encrypted - test if the BO is encrypted + * @bo: pointer to a buffer object + * + * Return true if the buffer object is encrypted, false otherwise. + */ +static inline bool amdgpu_bo_encrypted(struct amdgpu_bo *bo) +{ + return bo->flags & AMDGPU_GEM_CREATE_ENCRYPTED; +} + bool amdgpu_bo_is_amdgpu_bo(struct ttm_buffer_object *bo); void amdgpu_bo_placement_from_domain(struct amdgpu_bo *abo, u32 domain); diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index b285ab2..8e13b1fd3 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c @@ -1688,6 +1688,11 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev, if (bo) { flags = amdgpu_ttm_tt_pte_flags(adev, bo->tbo.ttm, mem); + + if (amdgpu_bo_encrypted(bo)) { + flags |= AMDGPU_PTE_TMZ; + } + bo_adev = amdgpu_ttm_adev(bo->tbo.bdev); } else { flags = 0x0;