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