[PATCH] drm/amdgpu: implement TMZ accessor (v2)
Christian König
christian.koenig at amd.com
Tue Nov 26 11:57:59 UTC 2019
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 at lists.freedesktop.org> On Behalf Of
>>>> Luben Tuikov
>>>> Sent: Thursday, November 21, 2019 9:33 AM
>>>> To: amd-gfx at lists.freedesktop.org
>>>> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>; Tuikov, Luben
>>>> <Luben.Tuikov at amd.com>; Koenig, Christian <Christian.Koenig at amd.com>
>>>> 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 at amd.com>
>>>> ---
>>>> 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 at amd.com> anyway.
Regards,
Christian.
>
> Regards,
> Luben
>
>> Regards,
>> Luben
>>
>>> With this fixed, Reviewed-by: Aaron Liu <aaron.liu at amd.com>
>>>
>>>> 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 at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
More information about the amd-gfx
mailing list