* [PATCH] drm/amdgpu: Add a GEM_CREATE mask and bugfix
@ 2020-02-15 0:17 Luben Tuikov
2020-02-17 14:44 ` Alex Deucher
0 siblings, 1 reply; 9+ messages in thread
From: Luben Tuikov @ 2020-02-15 0:17 UTC (permalink / raw)
To: amd-gfx; +Cc: Luben Tuikov
Add a AMDGPU_GEM_CREATE_MASK and use it to check
for valid/invalid GEM create flags coming in from
userspace.
Fix a bug in checking whether TMZ is supported at
GEM create time.
Signed-off-by: Luben Tuikov <luben.tuikov@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 11 ++---------
include/uapi/drm/amdgpu_drm.h | 2 ++
2 files changed, 4 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index b51a060c637d..74bb79e64fa3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -221,21 +221,14 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data,
int r;
/* reject invalid gem flags */
- if (flags & ~(AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED |
- AMDGPU_GEM_CREATE_NO_CPU_ACCESS |
- 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_ENCRYPTED))
-
+ if (flags & ~AMDGPU_GEM_CREATE_MASK)
return -EINVAL;
/* reject invalid gem domains */
if (args->in.domains & ~AMDGPU_GEM_DOMAIN_MASK)
return -EINVAL;
- if (amdgpu_is_tmz(adev) && (flags & AMDGPU_GEM_CREATE_ENCRYPTED)) {
+ if (!amdgpu_is_tmz(adev) && flags & AMDGPU_GEM_CREATE_ENCRYPTED) {
DRM_ERROR("Cannot allocate secure buffer since TMZ is disabled\n");
return -EINVAL;
}
diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
index eaf94a421901..c8463cdf4448 100644
--- a/include/uapi/drm/amdgpu_drm.h
+++ b/include/uapi/drm/amdgpu_drm.h
@@ -141,6 +141,8 @@ extern "C" {
*/
#define AMDGPU_GEM_CREATE_ENCRYPTED (1 << 10)
+#define AMDGPU_GEM_CREATE_MASK ((1 << 11)-1)
+
struct drm_amdgpu_gem_create_in {
/** the requested memory size */
__u64 bo_size;
--
2.25.0.232.gd8437c57fa
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/amdgpu: Add a GEM_CREATE mask and bugfix
2020-02-15 0:17 [PATCH] drm/amdgpu: Add a GEM_CREATE mask and bugfix Luben Tuikov
@ 2020-02-17 14:44 ` Alex Deucher
2020-02-17 15:08 ` Christian König
2020-02-18 21:36 ` Luben Tuikov
0 siblings, 2 replies; 9+ messages in thread
From: Alex Deucher @ 2020-02-17 14:44 UTC (permalink / raw)
To: Luben Tuikov; +Cc: amd-gfx list
On Fri, Feb 14, 2020 at 7:17 PM Luben Tuikov <luben.tuikov@amd.com> wrote:
>
> Add a AMDGPU_GEM_CREATE_MASK and use it to check
> for valid/invalid GEM create flags coming in from
> userspace.
>
> Fix a bug in checking whether TMZ is supported at
> GEM create time.
>
> Signed-off-by: Luben Tuikov <luben.tuikov@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 11 ++---------
> include/uapi/drm/amdgpu_drm.h | 2 ++
> 2 files changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index b51a060c637d..74bb79e64fa3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -221,21 +221,14 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data,
> int r;
>
> /* reject invalid gem flags */
> - if (flags & ~(AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED |
> - AMDGPU_GEM_CREATE_NO_CPU_ACCESS |
> - 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_ENCRYPTED))
> -
I'd rather keep the list explicit so no one ends up forgetting to
update the mask the next time new flags are added.
Alex
> + if (flags & ~AMDGPU_GEM_CREATE_MASK)
> return -EINVAL;
>
> /* reject invalid gem domains */
> if (args->in.domains & ~AMDGPU_GEM_DOMAIN_MASK)
> return -EINVAL;
>
> - if (amdgpu_is_tmz(adev) && (flags & AMDGPU_GEM_CREATE_ENCRYPTED)) {
> + if (!amdgpu_is_tmz(adev) && flags & AMDGPU_GEM_CREATE_ENCRYPTED) {
> DRM_ERROR("Cannot allocate secure buffer since TMZ is disabled\n");
> return -EINVAL;
> }
> diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
> index eaf94a421901..c8463cdf4448 100644
> --- a/include/uapi/drm/amdgpu_drm.h
> +++ b/include/uapi/drm/amdgpu_drm.h
> @@ -141,6 +141,8 @@ extern "C" {
> */
> #define AMDGPU_GEM_CREATE_ENCRYPTED (1 << 10)
>
> +#define AMDGPU_GEM_CREATE_MASK ((1 << 11)-1)
> +
> struct drm_amdgpu_gem_create_in {
> /** the requested memory size */
> __u64 bo_size;
> --
> 2.25.0.232.gd8437c57fa
>
> _______________________________________________
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/amdgpu: Add a GEM_CREATE mask and bugfix
2020-02-17 14:44 ` Alex Deucher
@ 2020-02-17 15:08 ` Christian König
2020-02-18 21:46 ` Luben Tuikov
2020-02-18 21:36 ` Luben Tuikov
1 sibling, 1 reply; 9+ messages in thread
From: Christian König @ 2020-02-17 15:08 UTC (permalink / raw)
To: Alex Deucher, Luben Tuikov; +Cc: amd-gfx list
Am 17.02.20 um 15:44 schrieb Alex Deucher:
> On Fri, Feb 14, 2020 at 7:17 PM Luben Tuikov <luben.tuikov@amd.com> wrote:
>> Add a AMDGPU_GEM_CREATE_MASK and use it to check
>> for valid/invalid GEM create flags coming in from
>> userspace.
>>
>> Fix a bug in checking whether TMZ is supported at
>> GEM create time.
>>
>> Signed-off-by: Luben Tuikov <luben.tuikov@amd.com>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 11 ++---------
>> include/uapi/drm/amdgpu_drm.h | 2 ++
>> 2 files changed, 4 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> index b51a060c637d..74bb79e64fa3 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> @@ -221,21 +221,14 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data,
>> int r;
>>
>> /* reject invalid gem flags */
>> - if (flags & ~(AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED |
>> - AMDGPU_GEM_CREATE_NO_CPU_ACCESS |
>> - 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_ENCRYPTED))
>> -
> I'd rather keep the list explicit so no one ends up forgetting to
> update the mask the next time new flags are added.
Additional to that this patch is bogus.
We intentionally filter out the flags here which userspace is allowed to
specify in the GEM interface, but after this patch we would allow all
flags to be specified.
Christian.
>
> Alex
>
>> + if (flags & ~AMDGPU_GEM_CREATE_MASK)
>> return -EINVAL;
>>
>> /* reject invalid gem domains */
>> if (args->in.domains & ~AMDGPU_GEM_DOMAIN_MASK)
>> return -EINVAL;
>>
>> - if (amdgpu_is_tmz(adev) && (flags & AMDGPU_GEM_CREATE_ENCRYPTED)) {
>> + if (!amdgpu_is_tmz(adev) && flags & AMDGPU_GEM_CREATE_ENCRYPTED) {
>> DRM_ERROR("Cannot allocate secure buffer since TMZ is disabled\n");
>> return -EINVAL;
>> }
>> diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
>> index eaf94a421901..c8463cdf4448 100644
>> --- a/include/uapi/drm/amdgpu_drm.h
>> +++ b/include/uapi/drm/amdgpu_drm.h
>> @@ -141,6 +141,8 @@ extern "C" {
>> */
>> #define AMDGPU_GEM_CREATE_ENCRYPTED (1 << 10)
>>
>> +#define AMDGPU_GEM_CREATE_MASK ((1 << 11)-1)
>> +
>> struct drm_amdgpu_gem_create_in {
>> /** the requested memory size */
>> __u64 bo_size;
>> --
>> 2.25.0.232.gd8437c57fa
>>
>> _______________________________________________
>> 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
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/amdgpu: Add a GEM_CREATE mask and bugfix
2020-02-17 14:44 ` Alex Deucher
2020-02-17 15:08 ` Christian König
@ 2020-02-18 21:36 ` Luben Tuikov
2020-02-18 21:40 ` Alex Deucher
1 sibling, 1 reply; 9+ messages in thread
From: Luben Tuikov @ 2020-02-18 21:36 UTC (permalink / raw)
To: Alex Deucher; +Cc: amd-gfx list
On 2020-02-17 9:44 a.m., Alex Deucher wrote:
> On Fri, Feb 14, 2020 at 7:17 PM Luben Tuikov <luben.tuikov@amd.com> wrote:
>>
>> Add a AMDGPU_GEM_CREATE_MASK and use it to check
>> for valid/invalid GEM create flags coming in from
>> userspace.
>>
>> Fix a bug in checking whether TMZ is supported at
>> GEM create time.
>>
>> Signed-off-by: Luben Tuikov <luben.tuikov@amd.com>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 11 ++---------
>> include/uapi/drm/amdgpu_drm.h | 2 ++
>> 2 files changed, 4 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> index b51a060c637d..74bb79e64fa3 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> @@ -221,21 +221,14 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data,
>> int r;
>>
>> /* reject invalid gem flags */
>> - if (flags & ~(AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED |
>> - AMDGPU_GEM_CREATE_NO_CPU_ACCESS |
>> - 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_ENCRYPTED))
>> -
>
> I'd rather keep the list explicit so no one ends up forgetting to
> update the mask the next time new flags are added.
>
> Alex
What about the bugfix below?
Why did no one comment on it?
>
>> + if (flags & ~AMDGPU_GEM_CREATE_MASK)
>> return -EINVAL;
>>
>> /* reject invalid gem domains */
>> if (args->in.domains & ~AMDGPU_GEM_DOMAIN_MASK)
>> return -EINVAL;
>>
>> - if (amdgpu_is_tmz(adev) && (flags & AMDGPU_GEM_CREATE_ENCRYPTED)) {
>> + if (!amdgpu_is_tmz(adev) && flags & AMDGPU_GEM_CREATE_ENCRYPTED) {
This one right here?
What's going on?
Regards,
Luben
>> DRM_ERROR("Cannot allocate secure buffer since TMZ is disabled\n");
>> return -EINVAL;
>> }
>> diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
>> index eaf94a421901..c8463cdf4448 100644
>> --- a/include/uapi/drm/amdgpu_drm.h
>> +++ b/include/uapi/drm/amdgpu_drm.h
>> @@ -141,6 +141,8 @@ extern "C" {
>> */
>> #define AMDGPU_GEM_CREATE_ENCRYPTED (1 << 10)
>>
>> +#define AMDGPU_GEM_CREATE_MASK ((1 << 11)-1)
>> +
>> struct drm_amdgpu_gem_create_in {
>> /** the requested memory size */
>> __u64 bo_size;
>> --
>> 2.25.0.232.gd8437c57fa
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cluben.tuikov%40amd.com%7C622cd0e399a041e8ad4908d7b3b7e97d%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637175474785178580&sdata=CLlIUaQVOhMibxBtkHtbBZQ9d%2FjNBSRtZ8Db%2FYN2Hj8%3D&reserved=0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/amdgpu: Add a GEM_CREATE mask and bugfix
2020-02-18 21:36 ` Luben Tuikov
@ 2020-02-18 21:40 ` Alex Deucher
0 siblings, 0 replies; 9+ messages in thread
From: Alex Deucher @ 2020-02-18 21:40 UTC (permalink / raw)
To: Luben Tuikov; +Cc: amd-gfx list
On Tue, Feb 18, 2020 at 4:36 PM Luben Tuikov <luben.tuikov@amd.com> wrote:
>
> On 2020-02-17 9:44 a.m., Alex Deucher wrote:
> > On Fri, Feb 14, 2020 at 7:17 PM Luben Tuikov <luben.tuikov@amd.com> wrote:
> >>
> >> Add a AMDGPU_GEM_CREATE_MASK and use it to check
> >> for valid/invalid GEM create flags coming in from
> >> userspace.
> >>
> >> Fix a bug in checking whether TMZ is supported at
> >> GEM create time.
> >>
> >> Signed-off-by: Luben Tuikov <luben.tuikov@amd.com>
> >> ---
> >> drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 11 ++---------
> >> include/uapi/drm/amdgpu_drm.h | 2 ++
> >> 2 files changed, 4 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> >> index b51a060c637d..74bb79e64fa3 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> >> @@ -221,21 +221,14 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data,
> >> int r;
> >>
> >> /* reject invalid gem flags */
> >> - if (flags & ~(AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED |
> >> - AMDGPU_GEM_CREATE_NO_CPU_ACCESS |
> >> - 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_ENCRYPTED))
> >> -
> >
> > I'd rather keep the list explicit so no one ends up forgetting to
> > update the mask the next time new flags are added.
> >
> > Alex
>
> What about the bugfix below?
>
> Why did no one comment on it?
>
> >
> >> + if (flags & ~AMDGPU_GEM_CREATE_MASK)
> >> return -EINVAL;
> >>
> >> /* reject invalid gem domains */
> >> if (args->in.domains & ~AMDGPU_GEM_DOMAIN_MASK)
> >> return -EINVAL;
> >>
> >> - if (amdgpu_is_tmz(adev) && (flags & AMDGPU_GEM_CREATE_ENCRYPTED)) {
> >> + if (!amdgpu_is_tmz(adev) && flags & AMDGPU_GEM_CREATE_ENCRYPTED) {
>
> This one right here?
>
> What's going on?
Yes, this bug fix is correct, but the other change in this patch is
not. I should have suggested that you split out just the bug fix. I
thought you were going to resend just the fix after the feedback on
the other portion, but I guess I should have been more explicit.
Alex
>
> Regards,
> Luben
>
> >> DRM_ERROR("Cannot allocate secure buffer since TMZ is disabled\n");
> >> return -EINVAL;
> >> }
> >> diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
> >> index eaf94a421901..c8463cdf4448 100644
> >> --- a/include/uapi/drm/amdgpu_drm.h
> >> +++ b/include/uapi/drm/amdgpu_drm.h
> >> @@ -141,6 +141,8 @@ extern "C" {
> >> */
> >> #define AMDGPU_GEM_CREATE_ENCRYPTED (1 << 10)
> >>
> >> +#define AMDGPU_GEM_CREATE_MASK ((1 << 11)-1)
> >> +
> >> struct drm_amdgpu_gem_create_in {
> >> /** the requested memory size */
> >> __u64 bo_size;
> >> --
> >> 2.25.0.232.gd8437c57fa
> >>
> >> _______________________________________________
> >> amd-gfx mailing list
> >> amd-gfx@lists.freedesktop.org
> >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cluben.tuikov%40amd.com%7C622cd0e399a041e8ad4908d7b3b7e97d%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637175474785178580&sdata=CLlIUaQVOhMibxBtkHtbBZQ9d%2FjNBSRtZ8Db%2FYN2Hj8%3D&reserved=0
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/amdgpu: Add a GEM_CREATE mask and bugfix
2020-02-17 15:08 ` Christian König
@ 2020-02-18 21:46 ` Luben Tuikov
2020-02-19 8:20 ` Christian König
2020-02-19 11:27 ` Huang Rui
0 siblings, 2 replies; 9+ messages in thread
From: Luben Tuikov @ 2020-02-18 21:46 UTC (permalink / raw)
To: christian.koenig, Alex Deucher; +Cc: amd-gfx list
On 2020-02-17 10:08 a.m., Christian König wrote:
> Am 17.02.20 um 15:44 schrieb Alex Deucher:
>> On Fri, Feb 14, 2020 at 7:17 PM Luben Tuikov <luben.tuikov@amd.com> wrote:
>>> Add a AMDGPU_GEM_CREATE_MASK and use it to check
>>> for valid/invalid GEM create flags coming in from
>>> userspace.
>>>
>>> Fix a bug in checking whether TMZ is supported at
>>> GEM create time.
>>>
>>> Signed-off-by: Luben Tuikov <luben.tuikov@amd.com>
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 11 ++---------
>>> include/uapi/drm/amdgpu_drm.h | 2 ++
>>> 2 files changed, 4 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> index b51a060c637d..74bb79e64fa3 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> @@ -221,21 +221,14 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data,
>>> int r;
>>>
>>> /* reject invalid gem flags */
>>> - if (flags & ~(AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED |
>>> - AMDGPU_GEM_CREATE_NO_CPU_ACCESS |
>>> - 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_ENCRYPTED))
>>> -
>> I'd rather keep the list explicit so no one ends up forgetting to
>> update the mask the next time new flags are added.
>
> Additional to that this patch is bogus.
So the only "additional" thing you're contributing to the review of
this patch is calling it "bogus". Characterizing the patch with an adjective
as "bogus" is very disturbing. What's next?
>
> We intentionally filter out the flags here which userspace is allowed to
> specify in the GEM interface, but after this patch we would allow all
> flags to be specified.
I thought that that could be the case. But in your review why not
recommend we have a mask to check user-settable flags? So the
actual flags are collected and visible in one place and well
understood that that is what is being checked and well-defined
by a macro with an appropriate name?
Why did NO ONE comment on the bug fix below? No one.
Regards,
Luben
>
> Christian.
>
>
>
>>
>> Alex
>>
>>> + if (flags & ~AMDGPU_GEM_CREATE_MASK)
>>> return -EINVAL;
>>>
>>> /* reject invalid gem domains */
>>> if (args->in.domains & ~AMDGPU_GEM_DOMAIN_MASK)
>>> return -EINVAL;
>>>
>>> - if (amdgpu_is_tmz(adev) && (flags & AMDGPU_GEM_CREATE_ENCRYPTED)) {
>>> + if (!amdgpu_is_tmz(adev) && flags & AMDGPU_GEM_CREATE_ENCRYPTED) {
>>> DRM_ERROR("Cannot allocate secure buffer since TMZ is disabled\n");
>>> return -EINVAL;
>>> }
>>> diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
>>> index eaf94a421901..c8463cdf4448 100644
>>> --- a/include/uapi/drm/amdgpu_drm.h
>>> +++ b/include/uapi/drm/amdgpu_drm.h
>>> @@ -141,6 +141,8 @@ extern "C" {
>>> */
>>> #define AMDGPU_GEM_CREATE_ENCRYPTED (1 << 10)
>>>
>>> +#define AMDGPU_GEM_CREATE_MASK ((1 << 11)-1)
>>> +
>>> struct drm_amdgpu_gem_create_in {
>>> /** the requested memory size */
>>> __u64 bo_size;
>>> --
>>> 2.25.0.232.gd8437c57fa
>>>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cluben.tuikov%40amd.com%7Cb1fdc3970a224fc61fdc08d7b3bb4613%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637175489240077250&sdata=rE8A6jKAIX081ZhxxcMc%2BpGdXvsLUdrAW4AkLsTpNxg%3D&reserved=0
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cluben.tuikov%40amd.com%7Cb1fdc3970a224fc61fdc08d7b3bb4613%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637175489240077250&sdata=rE8A6jKAIX081ZhxxcMc%2BpGdXvsLUdrAW4AkLsTpNxg%3D&reserved=0
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/amdgpu: Add a GEM_CREATE mask and bugfix
2020-02-18 21:46 ` Luben Tuikov
@ 2020-02-19 8:20 ` Christian König
2020-02-19 22:03 ` Luben Tuikov
2020-02-19 11:27 ` Huang Rui
1 sibling, 1 reply; 9+ messages in thread
From: Christian König @ 2020-02-19 8:20 UTC (permalink / raw)
To: Luben Tuikov, Alex Deucher; +Cc: amd-gfx list
Am 18.02.20 um 22:46 schrieb Luben Tuikov:
> On 2020-02-17 10:08 a.m., Christian König wrote:
>> Am 17.02.20 um 15:44 schrieb Alex Deucher:
>>> On Fri, Feb 14, 2020 at 7:17 PM Luben Tuikov <luben.tuikov@amd.com> wrote:
>>>> Add a AMDGPU_GEM_CREATE_MASK and use it to check
>>>> for valid/invalid GEM create flags coming in from
>>>> userspace.
>>>>
>>>> Fix a bug in checking whether TMZ is supported at
>>>> GEM create time.
>>>>
>>>> Signed-off-by: Luben Tuikov <luben.tuikov@amd.com>
>>>> ---
>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 11 ++---------
>>>> include/uapi/drm/amdgpu_drm.h | 2 ++
>>>> 2 files changed, 4 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>> index b51a060c637d..74bb79e64fa3 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>> @@ -221,21 +221,14 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data,
>>>> int r;
>>>>
>>>> /* reject invalid gem flags */
>>>> - if (flags & ~(AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED |
>>>> - AMDGPU_GEM_CREATE_NO_CPU_ACCESS |
>>>> - 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_ENCRYPTED))
>>>> -
>>> I'd rather keep the list explicit so no one ends up forgetting to
>>> update the mask the next time new flags are added.
>> Additional to that this patch is bogus.
> So the only "additional" thing you're contributing to the review of
> this patch is calling it "bogus". Characterizing the patch with an adjective
> as "bogus" is very disturbing. What's next?
Well the patch is technical incorrect and breaks the code in a very
subtle and hard to detect manner. Alex didn't noticed that either.
I'm not a native speaker of English, but as far as I have learned
"bogus" is the right adjective for that.
>> We intentionally filter out the flags here which userspace is allowed to
>> specify in the GEM interface, but after this patch we would allow all
>> flags to be specified.
> I thought that that could be the case.
Then why did you send out a patch which is seriously broken like that?
I mean if I hadn't noticed it by chance we would have committed this and
added a potentially security problematic bug to the IOCTL interface.
> But in your review why not
> recommend we have a mask to check user-settable flags? So the
> actual flags are collected and visible in one place and well
> understood that that is what is being checked and well-defined
> by a macro with an appropriate name?
See the flags tested here are the flags currently accepted by the
amdgpu_gem_create_ioctl() function. It doesn't say anything about what
the kernel would accept in the future.
So when we move that into the UAPI header we give userspace a
technically incorrect value.
> Why did NO ONE comment on the bug fix below? No one.
Because you mixed up a style change into some bug fix. That people go
for the problematic parts during code review is not really surprising at
all.
Regards,
Christian.
>
> Regards,
> Luben
>
>> Christian.
>>
>>
>>
>>> Alex
>>>
>>>> + if (flags & ~AMDGPU_GEM_CREATE_MASK)
>>>> return -EINVAL;
>>>>
>>>> /* reject invalid gem domains */
>>>> if (args->in.domains & ~AMDGPU_GEM_DOMAIN_MASK)
>>>> return -EINVAL;
>>>>
>>>> - if (amdgpu_is_tmz(adev) && (flags & AMDGPU_GEM_CREATE_ENCRYPTED)) {
>>>> + if (!amdgpu_is_tmz(adev) && flags & AMDGPU_GEM_CREATE_ENCRYPTED) {
>>>> DRM_ERROR("Cannot allocate secure buffer since TMZ is disabled\n");
>>>> return -EINVAL;
>>>> }
>>>> diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
>>>> index eaf94a421901..c8463cdf4448 100644
>>>> --- a/include/uapi/drm/amdgpu_drm.h
>>>> +++ b/include/uapi/drm/amdgpu_drm.h
>>>> @@ -141,6 +141,8 @@ extern "C" {
>>>> */
>>>> #define AMDGPU_GEM_CREATE_ENCRYPTED (1 << 10)
>>>>
>>>> +#define AMDGPU_GEM_CREATE_MASK ((1 << 11)-1)
>>>> +
>>>> struct drm_amdgpu_gem_create_in {
>>>> /** the requested memory size */
>>>> __u64 bo_size;
>>>> --
>>>> 2.25.0.232.gd8437c57fa
>>>>
>>>> _______________________________________________
>>>> amd-gfx mailing list
>>>> amd-gfx@lists.freedesktop.org
>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cluben.tuikov%40amd.com%7Cb1fdc3970a224fc61fdc08d7b3bb4613%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637175489240077250&sdata=rE8A6jKAIX081ZhxxcMc%2BpGdXvsLUdrAW4AkLsTpNxg%3D&reserved=0
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cluben.tuikov%40amd.com%7Cb1fdc3970a224fc61fdc08d7b3bb4613%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637175489240077250&sdata=rE8A6jKAIX081ZhxxcMc%2BpGdXvsLUdrAW4AkLsTpNxg%3D&reserved=0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/amdgpu: Add a GEM_CREATE mask and bugfix
2020-02-18 21:46 ` Luben Tuikov
2020-02-19 8:20 ` Christian König
@ 2020-02-19 11:27 ` Huang Rui
1 sibling, 0 replies; 9+ messages in thread
From: Huang Rui @ 2020-02-19 11:27 UTC (permalink / raw)
To: Luben Tuikov; +Cc: Alex Deucher, christian.koenig, amd-gfx list
On Tue, Feb 18, 2020 at 04:46:21PM -0500, Luben Tuikov wrote:
> On 2020-02-17 10:08 a.m., Christian König wrote:
> > Am 17.02.20 um 15:44 schrieb Alex Deucher:
> >> On Fri, Feb 14, 2020 at 7:17 PM Luben Tuikov <luben.tuikov@amd.com> wrote:
> >>> Add a AMDGPU_GEM_CREATE_MASK and use it to check
> >>> for valid/invalid GEM create flags coming in from
> >>> userspace.
> >>>
> >>> Fix a bug in checking whether TMZ is supported at
> >>> GEM create time.
> >>>
> >>> Signed-off-by: Luben Tuikov <luben.tuikov@amd.com>
> >>> ---
> >>> drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 11 ++---------
> >>> include/uapi/drm/amdgpu_drm.h | 2 ++
> >>> 2 files changed, 4 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> >>> index b51a060c637d..74bb79e64fa3 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> >>> @@ -221,21 +221,14 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data,
> >>> int r;
> >>>
> >>> /* reject invalid gem flags */
> >>> - if (flags & ~(AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED |
> >>> - AMDGPU_GEM_CREATE_NO_CPU_ACCESS |
> >>> - 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_ENCRYPTED))
> >>> -
> >> I'd rather keep the list explicit so no one ends up forgetting to
> >> update the mask the next time new flags are added.
> >
> > Additional to that this patch is bogus.
>
> So the only "additional" thing you're contributing to the review of
> this patch is calling it "bogus". Characterizing the patch with an adjective
> as "bogus" is very disturbing. What's next?
>
> >
> > We intentionally filter out the flags here which userspace is allowed to
> > specify in the GEM interface, but after this patch we would allow all
> > flags to be specified.
>
> I thought that that could be the case. But in your review why not
> recommend we have a mask to check user-settable flags? So the
> actual flags are collected and visible in one place and well
> understood that that is what is being checked and well-defined
> by a macro with an appropriate name?
>
> Why did NO ONE comment on the bug fix below? No one.
>
I missed the bugfix patch before, sorry. (There are many patches from
amd-gfx one day, if you didn't cc me, the mail may be missed). So I found
the issue after sync up with drm-next and then give the fix. Hope you can
understand and don't take it to heart.
Thanks,
Ray
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/amdgpu: Add a GEM_CREATE mask and bugfix
2020-02-19 8:20 ` Christian König
@ 2020-02-19 22:03 ` Luben Tuikov
0 siblings, 0 replies; 9+ messages in thread
From: Luben Tuikov @ 2020-02-19 22:03 UTC (permalink / raw)
To: Christian König, Alex Deucher; +Cc: amd-gfx list
On 2020-02-19 3:20 a.m., Christian König wrote:
> Am 18.02.20 um 22:46 schrieb Luben Tuikov:
>> On 2020-02-17 10:08 a.m., Christian König wrote:
>>> Am 17.02.20 um 15:44 schrieb Alex Deucher:
>>>> On Fri, Feb 14, 2020 at 7:17 PM Luben Tuikov <luben.tuikov@amd.com> wrote:
>>>>> Add a AMDGPU_GEM_CREATE_MASK and use it to check
>>>>> for valid/invalid GEM create flags coming in from
>>>>> userspace.
>>>>>
>>>>> Fix a bug in checking whether TMZ is supported at
>>>>> GEM create time.
>>>>>
>>>>> Signed-off-by: Luben Tuikov <luben.tuikov@amd.com>
>>>>> ---
>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 11 ++---------
>>>>> include/uapi/drm/amdgpu_drm.h | 2 ++
>>>>> 2 files changed, 4 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>> index b51a060c637d..74bb79e64fa3 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>> @@ -221,21 +221,14 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data,
>>>>> int r;
>>>>>
>>>>> /* reject invalid gem flags */
>>>>> - if (flags & ~(AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED |
>>>>> - AMDGPU_GEM_CREATE_NO_CPU_ACCESS |
>>>>> - 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_ENCRYPTED))
>>>>> -
>>>> I'd rather keep the list explicit so no one ends up forgetting to
>>>> update the mask the next time new flags are added.
>>> Additional to that this patch is bogus.
>> So the only "additional" thing you're contributing to the review of
>> this patch is calling it "bogus". Characterizing the patch with an adjective
>> as "bogus" is very disturbing. What's next?
>
> Well the patch is technical incorrect and breaks the code in a very
> subtle and hard to detect manner. Alex didn't noticed that either.
>
> I'm not a native speaker of English, but as far as I have learned
> "bogus" is the right adjective for that.
>
>>> We intentionally filter out the flags here which userspace is allowed to
>>> specify in the GEM interface, but after this patch we would allow all
>>> flags to be specified.
>> I thought that that could be the case.
>
> Then why did you send out a patch which is seriously broken like that?
>
> I mean if I hadn't noticed it by chance we would have committed this and
> added a potentially security problematic bug to the IOCTL interface.
>
>> But in your review why not
>> recommend we have a mask to check user-settable flags? So the
>> actual flags are collected and visible in one place and well
>> understood that that is what is being checked and well-defined
>> by a macro with an appropriate name?
>
> See the flags tested here are the flags currently accepted by the
> amdgpu_gem_create_ioctl() function. It doesn't say anything about what
> the kernel would accept in the future.
>
> So when we move that into the UAPI header we give userspace a
> technically incorrect value.
>
>> Why did NO ONE comment on the bug fix below? No one.
>
> Because you mixed up a style change into some bug fix. That people go
> for the problematic parts during code review is not really surprising at
> all.
I wouldn't have expected a petulant-childlike response.
I would've expected a leadership-like position, something like:
Split your patch into two. The second part is okay with the addition
of DRM_PRINT_ONCE, and the first part admits bits which we don't
want user space to control.
Regards,
Luben
>
> Regards,
> Christian.
>
>>
>> Regards,
>> Luben
>>
>>> Christian.
>>>
>>>
>>>
>>>> Alex
>>>>
>>>>> + if (flags & ~AMDGPU_GEM_CREATE_MASK)
>>>>> return -EINVAL;
>>>>>
>>>>> /* reject invalid gem domains */
>>>>> if (args->in.domains & ~AMDGPU_GEM_DOMAIN_MASK)
>>>>> return -EINVAL;
>>>>>
>>>>> - if (amdgpu_is_tmz(adev) && (flags & AMDGPU_GEM_CREATE_ENCRYPTED)) {
>>>>> + if (!amdgpu_is_tmz(adev) && flags & AMDGPU_GEM_CREATE_ENCRYPTED) {
>>>>> DRM_ERROR("Cannot allocate secure buffer since TMZ is disabled\n");
>>>>> return -EINVAL;
>>>>> }
>>>>> diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
>>>>> index eaf94a421901..c8463cdf4448 100644
>>>>> --- a/include/uapi/drm/amdgpu_drm.h
>>>>> +++ b/include/uapi/drm/amdgpu_drm.h
>>>>> @@ -141,6 +141,8 @@ extern "C" {
>>>>> */
>>>>> #define AMDGPU_GEM_CREATE_ENCRYPTED (1 << 10)
>>>>>
>>>>> +#define AMDGPU_GEM_CREATE_MASK ((1 << 11)-1)
>>>>> +
>>>>> struct drm_amdgpu_gem_create_in {
>>>>> /** the requested memory size */
>>>>> __u64 bo_size;
>>>>> --
>>>>> 2.25.0.232.gd8437c57fa
>>>>>
>>>>> _______________________________________________
>>>>> amd-gfx mailing list
>>>>> amd-gfx@lists.freedesktop.org
>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cluben.tuikov%40amd.com%7Cb1fdc3970a224fc61fdc08d7b3bb4613%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637175489240077250&sdata=rE8A6jKAIX081ZhxxcMc%2BpGdXvsLUdrAW4AkLsTpNxg%3D&reserved=0
>>>> _______________________________________________
>>>> amd-gfx mailing list
>>>> amd-gfx@lists.freedesktop.org
>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cluben.tuikov%40amd.com%7Cb1fdc3970a224fc61fdc08d7b3bb4613%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637175489240077250&sdata=rE8A6jKAIX081ZhxxcMc%2BpGdXvsLUdrAW4AkLsTpNxg%3D&reserved=0
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-02-19 22:03 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-15 0:17 [PATCH] drm/amdgpu: Add a GEM_CREATE mask and bugfix Luben Tuikov
2020-02-17 14:44 ` Alex Deucher
2020-02-17 15:08 ` Christian König
2020-02-18 21:46 ` Luben Tuikov
2020-02-19 8:20 ` Christian König
2020-02-19 22:03 ` Luben Tuikov
2020-02-19 11:27 ` Huang Rui
2020-02-18 21:36 ` Luben Tuikov
2020-02-18 21:40 ` Alex Deucher
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.