Re: [PATCH] drm/amdgpu: implement TMZ accessor (v2)

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

 

Am 21.11.19 um 04:39 schrieb Luben Tuikov:

On 2019-11-20 10:21 p.m., Luben Tuikov wrote:

On 2019-11-20 10:02 p.m., Liu, Aaron wrote:

-----Original Message-----
From: amd-gfx <amd-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of
Luben Tuikov
Sent: Thursday, November 21, 2019 9:33 AM
To: amd-gfx@xxxxxxxxxxxxxxxxxxxxx
Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Tuikov, Luben
<Luben.Tuikov@xxxxxxx>; Koenig, Christian <Christian.Koenig@xxxxxxx>
Subject: [PATCH] drm/amdgpu: implement TMZ accessor (v2)

Implement an accessor of adev->tmz.enabled. Let not code around access it
as "if (adev->tmz.enabled)"
as the organization may change. Instead...

Recruit "bool amdgpu_is_tmz(adev)" to return exactly this Boolean value.
That is, this function is now an accessor of an already initialized and set adev
and adev->tmz.

Add "void amdgpu_tmz_set(adev)" to check and set
adev->tmz.* at initialization time. After which
one uses "bool amdgpu_is_tmz(adev)" to query whether adev supports TMZ.

Also, remove circular header file include.

v2: Remove amdgpu_tmz.[ch] as requested.

Signed-off-by: Luben Tuikov <luben.tuikov@xxxxxxx>
---
  drivers/gpu/drm/amd/amdgpu/Makefile        |  2 +-
  drivers/gpu/drm/amd/amdgpu/amdgpu.h        |  6 ++-
  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  3 +-
  drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c    | 23 ++++++++++
  drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h    |  9 ++++
  drivers/gpu/drm/amd/amdgpu/amdgpu_tmz.c    | 52 ----------------------
  drivers/gpu/drm/amd/amdgpu/amdgpu_tmz.h    | 39 ----------------
  7 files changed, 39 insertions(+), 95 deletions(-)  delete mode 100644
drivers/gpu/drm/amd/amdgpu/amdgpu_tmz.c
  delete mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_tmz.h

diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile
b/drivers/gpu/drm/amd/amdgpu/Makefile
index 83ee1c676e3a..7ae3b22c5628 100644
--- a/drivers/gpu/drm/amd/amdgpu/Makefile
+++ b/drivers/gpu/drm/amd/amdgpu/Makefile
@@ -55,7 +55,7 @@ amdgpu-y += amdgpu_device.o amdgpu_kms.o \
  	amdgpu_vf_error.o amdgpu_sched.o amdgpu_debugfs.o
amdgpu_ids.o \
  	amdgpu_gmc.o amdgpu_mmhub.o amdgpu_xgmi.o amdgpu_csa.o
amdgpu_ras.o amdgpu_vm_cpu.o \
  	amdgpu_vm_sdma.o amdgpu_discovery.o amdgpu_ras_eeprom.o
amdgpu_nbio.o \
-	amdgpu_umc.o smu_v11_0_i2c.o amdgpu_tmz.o
+	amdgpu_umc.o smu_v11_0_i2c.o

  amdgpu-$(CONFIG_PERF_EVENTS) += amdgpu_pmu.o

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index d120fe58ebea..805e12ef13ea 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -90,7 +90,6 @@
  #include "amdgpu_mes.h"
  #include "amdgpu_umc.h"
  #include "amdgpu_mmhub.h"
-#include "amdgpu_tmz.h"

  #define MAX_GPU_INSTANCE		16

@@ -1266,5 +1265,10 @@ _name##_show(struct device *dev,
			\
  									\
  static struct device_attribute pmu_attr_##_name = __ATTR_RO(_name)

+static inline bool amdgpu_is_tmz(struct amdgpu_device *adev) {
+       return adev->tmz.enabled;
+}
+
  #endif

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index b1408c5e4640..56836054e6a8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -64,7 +64,6 @@
  #include "amdgpu_xgmi.h"
  #include "amdgpu_ras.h"
  #include "amdgpu_pmu.h"
-#include "amdgpu_tmz.h"

  #include <linux/suspend.h>

@@ -1073,7 +1072,7 @@ static int amdgpu_device_check_arguments(struct
amdgpu_device *adev)

  	adev->firmware.load_type = amdgpu_ucode_get_load_type(adev,
amdgpu_fw_load_type);

-	adev->tmz.enabled = amdgpu_is_tmz(adev);
+	amdgpu_tmz_set(adev);

  	return ret;
  }
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
index a12f33c0f5df..a0245d8b2f37 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
@@ -333,3 +333,26 @@ void amdgpu_gmc_ras_fini(struct amdgpu_device
*adev)
  	amdgpu_mmhub_ras_fini(adev);
  	amdgpu_xgmi_ras_fini(adev);
  }
+
+/**
+ * amdgpu_tmz_set -- check and set if a device supports TMZ
+ * @adev: amdgpu_device pointer
+ *
+ * Check and set if an the device @adev supports Trusted Memory
+ * Zones (TMZ).
+ */
+void amdgpu_tmz_set(struct amdgpu_device *adev) {
+	if (!amdgpu_tmz)
+		return;
+
+	if (adev->asic_type < CHIP_RAVEN ||
+	    adev->asic_type == CHIP_ARCTURUS) {
+		dev_warn(adev->dev, "Trusted Memory Zone (TMZ) feature
not supported\n");
+		return;
+	}
+
+	adev->tmz.enabled = true;
+
+	dev_info(adev->dev, "Trusted Memory Zone (TMZ) feature
supported and
+enabled\n"); }

Hi Luben,
TMZ is just a specific feature and I think this is a nice change that moving amdgpu_tmz to amdgpu_gmc.h.
Another thing, you can rename amdgpu_tmz_set to amdgpu_gmc_tmz_set in amdgpu_gmc.h/ amdgpu_gmc.c
In amdgpu_gmc.c, all functions are prefixed with amdgpu_gmc_*.

Yes, I understand this GMC naming. Here's the reasoning:

1. I tried to keep the same name as was already in the code before this patch,
    amdgpu_tmz_/verb/().

2. I feel that TMZ, while a function _of_ the GMC and Crypto, isn't necessarily
    directly driving/programming/etc the GMC _hardware block_, just indirectly,
    via flags present in PTEs, arguments to registers, etc., i.e. as in input,
    and not necessarily being part of the GMC _driver_ code.

3. Names could become too long, very very long, by stringing along
    three-letter acronyms. While the function is indeed in amdgpu_gmc.c, it is
    TMZ-feature related, indirectly, so I belive keeping the same name as it had before,
    to be correct and succinct enough to be descriptive.

As Christian has pointed out several times "It's a feature, not an IP block",
which was the reason he'd been wanting to remove amdgpu_tmz.[ch]. It ended up
in amdgpu_gmc, as it seemed to be the closest place to put it.

Another point which I forgot to add is:

Usage. How will this be used and what does it _mean_?

amdgpu_tmz_set(adev);

VS.

amdgpu_gmc_tmz_set(adev);

Are we really setting anything in the GMC IP block?


Actually yes, but only indirectly.
We set what flags are inserted into the PTEs which are then essentially processed by the GMC. 



We're only asking that the device be known as a device which
supports and may receive TMZ BOs. It's an abstraction (TMZ) over a
device abstraction (struct amdgpu_device).

Similar argument applies to the static inline

amdgpu_is_tmz(adev);

VS

amdgpu_gmc_is_tmz(adev);

Are we asking about the state of the GMC block, or are
we asking about the feature support in the amdgpu device
abstraction?
I would name that amdgpu_gmc_is_tmz_enabled(), but that is not a hard requirement. 


I believe we should keep the names as they had been before
this patch. This patch only moves them around.
It's just the naming, feel free to add a Acked-by: Christian König <christian.koenig@xxxxxxx> anyway. 

Regards,
Christian.



Regards,
Luben


Regards,
Luben


With this fixed, Reviewed-by: Aaron Liu <aaron.liu@xxxxxxx>


diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
index 406736a1bd3d..1abd935a073e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
@@ -267,4 +267,13 @@ bool amdgpu_gmc_filter_faults(struct
amdgpu_device *adev, uint64_t addr,  int amdgpu_gmc_ras_late_init(struct
amdgpu_device *adev);  void amdgpu_gmc_ras_fini(struct amdgpu_device
*adev);

+/*
+ * Trusted Memory Zone particulars
+ */
+struct amdgpu_tmz {
+	bool	enabled;
+};
+
+extern void amdgpu_tmz_set(struct amdgpu_device *adev);
+
  #endif
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_tmz.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_tmz.c
deleted file mode 100644
index 823527a0fa47..000000000000
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_tmz.c
+++ /dev/null
@@ -1,52 +0,0 @@
-/*
- * Copyright 2019 Advanced Micro Devices, Inc.
- *
- * Permission is hereby granted, free of charge, to any person obtaining a
- * copy of this software and associated documentation files (the "Software"),
- * to deal in the Software without restriction, including without limitation
- * the rights to use, copy, modify, merge, publish, distribute, sublicense,
- * and/or sell copies of the Software, and to permit persons to whom the
- * Software is furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO
EVENT SHALL
- * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM,
DAMAGES OR
- * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
OTHERWISE,
- * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR
THE USE OR
- * OTHER DEALINGS IN THE SOFTWARE.
- */
-
-#include <linux/device.h>
-
-#include <drm/amd_asic_type.h>
-
-#include "amdgpu.h"
-#include "amdgpu_tmz.h"
-
-
-/**
- * amdgpu_is_tmz - validate trust memory zone
- *
- * @adev: amdgpu_device pointer
- *
- * Return true if @dev supports trusted memory zones (TMZ), and return
false if
- * @dev does not support TMZ.
- */
-bool amdgpu_is_tmz(struct amdgpu_device *adev) -{
-	if (!amdgpu_tmz)
-		return false;
-
-	if (adev->asic_type < CHIP_RAVEN || adev->asic_type ==
CHIP_ARCTURUS) {
-		dev_warn(adev->dev, "doesn't support trusted memory
zones (TMZ)\n");
-		return false;
-	}
-
-	dev_info(adev->dev, "TMZ feature is enabled\n");
-
-	return true;
-}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_tmz.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu_tmz.h
deleted file mode 100644
index 28e05177fb89..000000000000
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_tmz.h
+++ /dev/null
@@ -1,39 +0,0 @@
-/*
- * Copyright 2019 Advanced Micro Devices, Inc.
- *
- * Permission is hereby granted, free of charge, to any person obtaining a
- * copy of this software and associated documentation files (the "Software"),
- * to deal in the Software without restriction, including without limitation
- * the rights to use, copy, modify, merge, publish, distribute, sublicense,
- * and/or sell copies of the Software, and to permit persons to whom the
- * Software is furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO
EVENT SHALL
- * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM,
DAMAGES OR
- * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
OTHERWISE,
- * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR
THE USE OR
- * OTHER DEALINGS IN THE SOFTWARE.
- *
- */
-
-#ifndef __AMDGPU_TMZ_H__
-#define __AMDGPU_TMZ_H__
-
-#include "amdgpu.h"
-
-/*
- * Trust memory zone stuff
- */
-struct amdgpu_tmz {
-	bool	enabled;
-};
-
-
-extern bool amdgpu_is_tmz(struct amdgpu_device *adev);
-
-#endif
--
2.24.0.155.gd9f6f3b619

_______________________________________________
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