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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Am 20.11.19 um 18:50 schrieb Luben Tuikov:
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@xxxxxxx
<mailto:luben.tuikov@xxxxxxx>> wrote:

      On 2019-11-14 10:34 p.m., Aaron Liu wrote:
      > From: Huang Rui <ray.huang@xxxxxxx <mailto:ray.huang@xxxxxxx>>
      >
      > 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@xxxxxxx
<mailto:ray.huang@xxxxxxx>>
      > Reviewed-by: Alex Deucher <alexander.deucher@xxxxxxx
<mailto:alexander.deucher@xxxxxxx>>
      > ---
      >  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?

linux/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.)

Well I've been working with the Linux kernel since the mid 90ths and never ever heard of that.

Christian.


Thanks for chiming in Christian!

Regards,
Luben

Sorry,
Christian.

Regards,
Christian.

Regards,
Luben

Marek
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux