[PATCH 01/12] amdgpu: add UAPI for creating encrypted buffers

Wed Nov 20 17:50:59 UTC 2019

On 2019-11-20 12:24, Christian König wrote:
> Am 20.11.19 um 18:16 schrieb Christian König:
>> Am 20.11.19 um 17:49 schrieb Luben Tuikov:
>>> On 2019-11-19 21:41, Marek Olšák wrote:
>>>> On Tue, Nov 19, 2019 at 8:52 PM Luben Tuikov <luben.tuikov at amd.com 
>>>> <mailto:luben.tuikov at amd.com>> wrote:
>>>>
>>>>      On 2019-11-14 10:34 p.m., Aaron Liu wrote:
>>>>      > From: Huang Rui <ray.huang at amd.com <mailto:ray.huang at amd.com>>
>>>>      >
>>>>      > To align the kernel uapi change from Alex:
>>>>      >
>>>>      > "Add a flag to the GEM_CREATE ioctl to create encrypted 
>>>> buffers. Buffers with
>>>>      > this flag set will be created with the TMZ bit set in the 
>>>> PTEs or engines
>>>>      > accessing them. This is required in order to properly access 
>>>> the data from the
>>>>      > engines."
>>>>      >
>>>>      > We will use GEM_CREATE_ENCRYPTED flag for secure buffer 
>>>> allocation.
>>>>      >
>>>>      > Signed-off-by: Huang Rui <ray.huang at amd.com 
>>>> <mailto:ray.huang at amd.com>>
>>>>      > Reviewed-by: Alex Deucher <alexander.deucher at amd.com 
>>>> <mailto:alexander.deucher at amd.com>>
>>>>      > ---
>>>>      >  include/drm/amdgpu_drm.h | 5 +++++
>>>>      >  1 file changed, 5 insertions(+)
>>>>      >
>>>>      > diff --git a/include/drm/amdgpu_drm.h b/include/drm/amdgpu_drm.h
>>>>      > index 5c28aa7..1a95e37 100644
>>>>      > --- a/include/drm/amdgpu_drm.h
>>>>      > +++ b/include/drm/amdgpu_drm.h
>>>>      > @@ -141,6 +141,11 @@ extern "C" {
>>>>      >   * releasing the memory
>>>>      >   */
>>>>      >  #define AMDGPU_GEM_CREATE_VRAM_WIPE_ON_RELEASE  (1 << 9)
>>>>      > +/* Flag that BO will be encrypted and that the TMZ bit 
>>>> should be
>>>>      > + * set in the PTEs when mapping this buffer via GPUVM or
>>>>      > + * accessing it with various hw blocks
>>>>      > + */
>>>>      > +#define AMDGPU_GEM_CREATE_ENCRYPTED          (1 << 10)
>>>>
>>>>      Style!
>>>>      TAB char?!
>>>>
>>>>      You have a TAB char between ".._ENCRYPTED" and "(1 << 10)"
>>>>      Do NOT add/insert TAB chars instead of space to align colunmns!
>>>>      If when you press Tab key a tab is inserted, as opposed to the 
>>>> line
>>>>      indented, then DO NOT use this editor.
>>>>      The Tab key should "indent according to mode" by inserting TAB 
>>>> chars.
>>>>      If the line is already indented, as this one is, then it should 
>>>> do nothing.
>>>>
>>>>
>>>> I disagree with this 100%. Tabs or spaces don't matter here from my 
>>>> perspective. I also disagree with your language. It's overly impolite.
>>> But it's the coding style of Linux: leading tabs only. Try it with 
>>> Emacs as described and given in
>>>
>>> linux/Documentation/process/coding-style.rst
>>>
>>> starting at line 589. And press the Tab key on an already indented 
>>> line--nothing will happen. Linux has traditionally
>>> shunned from loose TAB chars in already indented lines: leading tabs 
>>> only mode. In a proper code editor
>>> pressing the Tab key only indents according to buffer mode, it 
>>> shouldn't insert a Tab char willy-nilly.
>>> People may set their tab stops differently for different tab 
>>> positions and inserting a tab char may display
>>> incorrectly. The most portable way to align columns in an already 
>>> indented-according-to-mode line, is
>>> using spaces. (Of course this doesn't matter when using spaces to 
>>> indent, but Linux uses hard TAB chars
>>> to indent: linux/Documentation/process/coding-style.rst. (which also 
>>> seem to be set to 8 chars))
>>>
>>> It's a code review, there is no "language".
>>
>> Well the section you noted also suggest to either get rid of emacs or 
>> change it to use some saner default values. We just got rid of emacs.

Yes, it says this, quote (for those who didn't open the file):

--8<---------------------------------------------------------------------

That's OK, we all do.  You've probably been told by your long-time Unix
user helper that ``GNU emacs`` automatically formats the C sources for
you, and you've noticed that yes, it does do that, but the defaults it
uses are less than desirable (in fact, they are worse than random
typing - an infinite number of monkeys typing into GNU emacs would never
make a good program).

So, you can either get rid of GNU emacs, or change it to use saner
values.  To do the latter, you can stick the following in your .emacs file:

--8<--------------------------------------------------------------------

>>
>> Regarding tabs after the initial indentation, I've just done a quick 
>> grep and around 14% of all defines under include/ uses that so I would 
>> say that this is perfectly fine.
> 
> Fast typing with lazy eyes, that should read "around 71% of all defines".

Hmm, that's interesting. Is that in linux/include or amdgpu/include?

I've been meaning to do my own extended regex to catch those, although
I'm using Emacs and pressing Tab key only indents and would not insert
a Tab char if already indented. (So applying this regex into the pre-commit
hook of all of my Git repos would never trigger.)

I remember of olden days, circa 2000 when I first got involved with Linux,
LKML didn't like loose tabs. Also lead kernel developers are using Emacs,
so it's been my choice of editor since circa 1994 (switched from vi to Emacs
largely due to the influence of a graphics prof I had in my seniour year of uni,
and part due to LKML.)

Thanks for chiming in Christian!

Regards,
Luben

> 
> Sorry,
> Christian.
> 
>>
>> Regards,
>> Christian.
>>
>>>
>>> Regards,
>>> Luben
>>>
>>>> Marek
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>
> 