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

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

 



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.

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