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@xxxxxxx> 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@xxxxxxx>
>>> ---
>>> 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@xxxxxxxxxxxxxxxxxxxxx
>>> 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@xxxxxxxxxxxxxxxxxxxxx
>> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
