[PATCH] amdgpu/pm: Modify sysfs pp_dpm_sclk to have only read premission in ONEVF mode
Nikolic, Marina
Marina.Nikolic at amd.com
Tue Dec 14 13:14:08 UTC 2021
[AMD Official Use Only]
Hello,
Thank you all for fast response and useful advice.
I made new changes based on suggestion from @Lazar, Lijo<mailto:Lijo.Lazar at amd.com>.
Though, I have made it general (any setting in sriov case) as that is in my request (details: SWDEV-313004).
Please find the new patch below.
Thank you and best regards,
Marina
>From 9afe76f6e4036143bed0047d71c05069fdeb44ee Mon Sep 17 00:00:00 2001
From: Marina Nikolic <Marina.Nikolic at amd.com>
Date: Tue, 14 Dec 2021 20:57:53 +0800
Subject: [PATCH] amdgpu/pm: Modify sysfs pp_dpm_sclk to have only read
permission in ONEVF mode
== Description ==
Due to security reasons setting through sysfs
should only be allowed in passthrough mode.
Options that are not mapped as SMU messages
do not have any mechanism to distinguish between
passthorugh, onevf and mutivf usecase.
A unified approach is needed.
== Changes ==
This patch introduces a new mechanizm to distinguish
ONEVF and PASSTHROUGH use case on sysfs level
and prohibit setting (writting to sysfs).
It also applies the new mechanizm on pp_dpm_sclk sysfs file.
== Test ==
Writing to pp_dpm_sclk sysfs file in passthrough mode will succeed.
Writing to pp_dpm_sclk sysfs file in ONEVF mode will yield error:
"calling process does not have sufficient permission to execute a command".
Sysfs pp_dpm_sclk will not be created in MULTIVF mode.
Signed-off-by: Marina Nikolic <Marina.Nikolic at amd.com>
---
drivers/gpu/drm/amd/pm/amdgpu_pm.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/amd/pm/amdgpu_pm.c b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
index 082539c70fd4..d2b168babc7d 100644
--- a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
+++ b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
@@ -2133,6 +2133,12 @@ static int default_attr_update(struct amdgpu_device *adev, struct amdgpu_device_
}
}
+ /* security: setting should not be allowed from VF */
+ if (amdgpu_sriov_vf(adev)) {
+ dev_attr->attr.mode &= ~S_IWUGO;
+ dev_attr->store = NULL;
+ }
+
#undef DEVICE_ATTR_IS
return 0;
--
2.20.1
________________________________
From: Lazar, Lijo <Lijo.Lazar at amd.com>
Sent: Tuesday, December 14, 2021 5:36 AM
To: Quan, Evan <Evan.Quan at amd.com>; Kasiviswanathan, Harish <Harish.Kasiviswanathan at amd.com>; Nikolic, Marina <Marina.Nikolic at amd.com>; Deng, Emily <Emily.Deng at amd.com>
Cc: Kitchen, Greg <Greg.Kitchen at amd.com>
Subject: RE: [PATCH] amdgpu/pm: Modify sysfs pp_dpm_sclk to have only read premission in ONEVF mode
[AMD Official Use Only]
Ah, just noticed that below code won't compile (misses one '{'). Regardless, hope you get the idea.
Thanks,
Lijo
-----Original Message-----
From: Lazar, Lijo
Sent: Tuesday, December 14, 2021 10:04 AM
To: Quan, Evan <Evan.Quan at amd.com>; Kasiviswanathan, Harish <Harish.Kasiviswanathan at amd.com>; Nikolic, Marina <Marina.Nikolic at amd.com>; Deng, Emily <Emily.Deng at amd.com>
Cc: Kitchen, Greg <Greg.Kitchen at amd.com>
Subject: RE: [PATCH] amdgpu/pm: Modify sysfs pp_dpm_sclk to have only read premission in ONEVF mode
[AMD Official Use Only]
Hi Marina,
attr_update is the recommended method to dynamically update the attribute properties.
If it's only for sclk, you may do as below (I guess, passthrough and VF are mutually exclusive).
--- a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
+++ b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
@@ -2132,6 +2132,13 @@ static int default_attr_update(struct amdgpu_device *adev, struct amdgpu_device_
dev_attr->store = NULL;
}
}
+ if (DEVICE_ATTR_IS(pp_dpm_sclk)) {
+ /* Don't allow to set GFX clock in VF mode*/
+ if (amdgpu_sriov_vf(adev))
+ dev_attr->attr.mode &= ~S_IWUGO;
+ dev_attr->store = NULL;
+ }
+ }
Thanks,
Lijo
-----Original Message-----
From: Quan, Evan <Evan.Quan at amd.com>
Sent: Tuesday, December 14, 2021 8:32 AM
To: Kasiviswanathan, Harish <Harish.Kasiviswanathan at amd.com>; Nikolic, Marina <Marina.Nikolic at amd.com>; Lazar, Lijo <Lijo.Lazar at amd.com>; Deng, Emily <Emily.Deng at amd.com>
Cc: Kitchen, Greg <Greg.Kitchen at amd.com>
Subject: RE: [PATCH] amdgpu/pm: Modify sysfs pp_dpm_sclk to have only read premission in ONEVF mode
[AMD Official Use Only]
+Emily from SRIOV team
I think driver uses the Macros below to tell about the ONEVF and PASSTHROUGH support:
#define amdgpu_sriov_is_pp_one_vf(adev) \
((adev)->virt.gim_feature & AMDGIM_FEATURE_PP_ONE_VF)
#define amdgpu_passthrough(adev) \
((adev)->virt.caps & AMDGPU_PASSTHROUGH_MODE)
@Deng, Emily please correct me if anything wrong.
BR
Evan
> -----Original Message-----
> From: Kasiviswanathan, Harish <Harish.Kasiviswanathan at amd.com>
> Sent: Tuesday, December 14, 2021 1:50 AM
> To: Kasiviswanathan, Harish <Harish.Kasiviswanathan at amd.com>; Nikolic,
> Marina <Marina.Nikolic at amd.com>; Quan, Evan <Evan.Quan at amd.com>;
> Lazar, Lijo <Lijo.Lazar at amd.com>
> Cc: Kitchen, Greg <Greg.Kitchen at amd.com>
> Subject: RE: [PATCH] amdgpu/pm: Modify sysfs pp_dpm_sclk to have only
> read premission in ONEVF mode
>
> [AMD Official Use Only]
>
> Hi Lijo and Evan,
>
> Could you please advice Marina on how to go about this? Does driver
> have a mechanism to distinguish between ONEVF and PASSTHROUGH case?
>
> Best Regards,
> Harish
>
>
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> On Behalf Of
> Kasiviswanathan, Harish
> Sent: Monday, December 13, 2021 12:40 PM
> To: Nikolic, Marina <Marina.Nikolic at amd.com>; amd-
> gfx at lists.freedesktop.org
> Cc: Nikolic, Marina <Marina.Nikolic at amd.com>
> Subject: RE: [PATCH] amdgpu/pm: Modify sysfs pp_dpm_sclk to have only
> read premission in ONEVF mode
>
> [AMD Official Use Only]
>
> Some comment inline.
>
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> On Behalf Of
> Marina Nikolic
> Sent: Friday, December 10, 2021 10:06 AM
> To: amd-gfx at lists.freedesktop.org
> Cc: Nikolic, Marina <Marina.Nikolic at amd.com>
> Subject: [PATCH] amdgpu/pm: Modify sysfs pp_dpm_sclk to have only read
> premission in ONEVF mode
>
> == Description ==
> Due to security reasons setting through sysfs should only be allowed
> in passthrough mode.
> Options that are not mapped as SMU messages do not have any mechanizm
> to distinguish between passthorugh, onevf and mutivf usecase.
> A unified approach is needed.
>
> == Changes ==
> This patch introduces a new mechanizm to distinguish ONEVF and
> PASSTHROUGH
>
> [HK]: It is not clear how you distinguish between ONEVF and PASSTHROUGH.
> Could you please elaborate?
>
> use case on sysfs level
> and prohibit setting (writting to sysfs).
> It also applies the new mechanizm on pp_dpm_sclk sysfs file.
>
> == Test ==
> Writing to pp_dpm_sclk sysfs file in passthrough mode will succeed.
> Writing to pp_dpm_sclk sysfs file in ONEVF mode will yield error:
> "calling process does not have sufficient permission to execute a command".
> Sysfs pp_dpm_sclk will not be created in MULTIVF mode.
>
> Signed-off-by: Marina Nikolic <marina.nikolic at amd.com>
> ---
> drivers/gpu/drm/amd/pm/amdgpu_pm.c | 4 ++--
> drivers/gpu/drm/amd/pm/inc/amdgpu_pm.h | 4 ++++
> 2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> index 082539c70fd4..0ccc23ee76a8 100644
> --- a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> +++ b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> @@ -2021,7 +2021,7 @@ static struct amdgpu_device_attr
> amdgpu_device_attrs[] = {
> AMDGPU_DEVICE_ATTR_RO(pp_cur_state,
> ATTR_FLAG_BASIC|ATTR_FLAG_ONEVF),
> AMDGPU_DEVICE_ATTR_RW(pp_force_state,
> ATTR_FLAG_BASIC|ATTR_FLAG_ONEVF),
> AMDGPU_DEVICE_ATTR_RW(pp_table,
> ATTR_FLAG_BASIC|ATTR_FLAG_ONEVF),
> - AMDGPU_DEVICE_ATTR_RW(pp_dpm_sclk,
> ATTR_FLAG_BASIC|ATTR_FLAG_ONEVF),
> + AMDGPU_DEVICE_ATTR_RRW(pp_dpm_sclk,
> ATTR_FLAG_BASIC, ATTR_FLAG_ONEVF),
>
> [HK]: Wouldn't this macro try to create two sysfs entries with same name.
> The second time the function would fail.
>
> AMDGPU_DEVICE_ATTR_RW(pp_dpm_mclk,
> ATTR_FLAG_BASIC|ATTR_FLAG_ONEVF),
> AMDGPU_DEVICE_ATTR_RW(pp_dpm_socclk,
> ATTR_FLAG_BASIC|ATTR_FLAG_ONEVF),
> AMDGPU_DEVICE_ATTR_RW(pp_dpm_fclk,
> ATTR_FLAG_BASIC|ATTR_FLAG_ONEVF),
> @@ -3504,7 +3504,7 @@ int amdgpu_pm_sysfs_init(struct amdgpu_device
> *adev)
> break;
> case SRIOV_VF_MODE_BARE_METAL:
> default:
> - mask = ATTR_FLAG_MASK_ALL;
> + mask = ATTR_FLAG_BASIC;
> break;
> }
>
> diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_pm.h
> b/drivers/gpu/drm/amd/pm/inc/amdgpu_pm.h
> index a920515e2274..1a30d9c48d13 100644
> --- a/drivers/gpu/drm/amd/pm/inc/amdgpu_pm.h
> +++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_pm.h
> @@ -79,6 +79,10 @@ struct amdgpu_device_attr_entry {
> amdgpu_get_##_name, NULL,
> \
> _flags, ##__VA_ARGS__)
>
> +#define AMDGPU_DEVICE_ATTR_RRW(_name, _flags_full,
> _flags_restricted, ...) \
> + AMDGPU_DEVICE_ATTR_RW(_name, _flags_full, ##__VA_ARGS__),
> \
> + AMDGPU_DEVICE_ATTR_RO(_name, _flags_restricted,
> ##__VA_ARGS__)
> +
> int amdgpu_pm_sysfs_init(struct amdgpu_device *adev); int
> amdgpu_pm_virt_sysfs_init(struct amdgpu_device *adev); void
> amdgpu_pm_sysfs_fini(struct amdgpu_device *adev);
> --
> 2.20.1
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20211214/ba82a176/attachment-0001.htm>
More information about the amd-gfx
mailing list