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?
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 believe we should keep the names as they had been before
this patch. This patch only moves them around.
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
