Re: [PATCH v2 18/63] drm/amd/pm: Use struct_group() for memcpy() region
On Thu, Aug 19, 2021 at 10:33:43AM +0530, Lazar, Lijo wrote: > On 8/19/2021 5:29 AM, Kees Cook wrote: > > On Wed, Aug 18, 2021 at 05:12:28PM +0530, Lazar, Lijo wrote: > > > > > > On 8/18/2021 11:34 AM, Kees Cook wrote: > > > > In preparation for FORTIFY_SOURCE performing compile-time and run-time > > > > field bounds checking for memcpy(), memmove(), and memset(), avoid > > > > intentionally writing across neighboring fields. > > > > > > > > Use struct_group() in structs: > > > > struct atom_smc_dpm_info_v4_5 > > > > struct atom_smc_dpm_info_v4_6 > > > > struct atom_smc_dpm_info_v4_7 > > > > struct atom_smc_dpm_info_v4_10 > > > > PPTable_t > > > > so the grouped members can be referenced together. This will allow > > > > memcpy() and sizeof() to more easily reason about sizes, improve > > > > readability, and avoid future warnings about writing beyond the end of > > > > the first member. > > > > > > > > "pahole" shows no size nor member offset changes to any structs. > > > > "objdump -d" shows no object code changes. > > > > > > > > Cc: "Christian König" > > > > Cc: "Pan, Xinhui" > > > > Cc: David Airlie > > > > Cc: Daniel Vetter > > > > Cc: Hawking Zhang > > > > Cc: Feifei Xu > > > > Cc: Lijo Lazar > > > > Cc: Likun Gao > > > > Cc: Jiawei Gu > > > > Cc: Evan Quan > > > > Cc: amd-...@lists.freedesktop.org > > > > Cc: dri-devel@lists.freedesktop.org > > > > Signed-off-by: Kees Cook > > > > Acked-by: Alex Deucher > > > > Link: > > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2FCADnq5_Npb8uYvd%2BR4UHgf-w8-cQj3JoODjviJR_Y9w9wqJ71mQ%40mail.gmail.comdata=04%7C01%7Clijo.lazar%40amd.com%7C3861f20094074bf7328808d962a433f2%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637649279701053991%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=386LcfJJGfQfHsXBuK17LMqxJ2nFtGoj%2FUjoN2ZtJd0%3Dreserved=0 > > > > --- > > > >drivers/gpu/drm/amd/include/atomfirmware.h | 9 - > > > >.../gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h| 3 ++- > > > >drivers/gpu/drm/amd/pm/inc/smu11_driver_if_navi10.h | 3 ++- > > > >.../gpu/drm/amd/pm/inc/smu13_driver_if_aldebaran.h | 3 ++- > > > > > > Hi Kees, > > > > Hi! Thanks for looking into this. > > > > > The headers which define these structs are firmware/VBIOS interfaces and > > > are > > > picked directly from those components. There are difficulties in grouping > > > them to structs at the original source as that involves other component > > > changes. > > > > So, can you help me understand this a bit more? It sounds like these are > > generated headers, yes? I'd like to understand your constraints and > > weight them against various benefits that could be achieved here. > > > > The groupings I made do appear to be roughly documented already, > > for example: > > > > struct atom_common_table_header table_header; > > // SECTION: BOARD PARAMETERS > > + struct_group(dpm_info, > > > > Something emitted the "BOARD PARAMETERS" section heading as a comment, > > so it likely also would know where it ends, yes? The good news here is > > that for the dpm_info groups, they all end at the end of the existing > > structs, see: > > struct atom_smc_dpm_info_v4_5 > > struct atom_smc_dpm_info_v4_6 > > struct atom_smc_dpm_info_v4_7 > > struct atom_smc_dpm_info_v4_10 > > > > The matching regions in the PPTable_t structs are similarly marked with a > > "BOARD PARAMETERS" section heading comment: > > > > --- a/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h > > +++ b/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h > > @@ -643,6 +643,7 @@ typedef struct { > > // SECTION: BOARD PARAMETERS > > // SVI2 Board Parameters > > + struct_group(v4_6, > > uint16_t MaxVoltageStepGfx; // In mV(Q2) Max voltage step that SMU > > will request. Multiple steps are taken if voltage change exceeds this value. > > uint16_t MaxVoltageStepSoc; // In mV(Q2) Max voltage step that SMU > > will request. Multiple steps are taken if voltage change exceeds this value. > > @@ -728,10 +729,10 @@ typedef struct { > > uint32_t BoardVoltageCoeffB;// decode by /1000 > > uint32_t BoardReserved[7]; > > + ); > > // Padding for MMHUB - do not modify this > > uint32_t MmHubPadding[8]; // SMU internal use > > - > > } PPTable_t; > > > > Where they end seems known as well (the padding switches from a "Board" > > to "MmHub" prefix at exactly the matching size). > > > > So, given that these regions are already known by the export tool, how > > about just updating the export tool to emit a struct there? I imagine > > the problem with this would be the identifier churn needed, but that's > > entirely mechanical. > > > > However, I'm curious about another aspect of these regions: they are, > > by definition, the same. Why isn't there a single
Re: [PATCH v2 18/63] drm/amd/pm: Use struct_group() for memcpy() region
On 8/19/2021 5:29 AM, Kees Cook wrote: On Wed, Aug 18, 2021 at 05:12:28PM +0530, Lazar, Lijo wrote: On 8/18/2021 11:34 AM, Kees Cook wrote: In preparation for FORTIFY_SOURCE performing compile-time and run-time field bounds checking for memcpy(), memmove(), and memset(), avoid intentionally writing across neighboring fields. Use struct_group() in structs: struct atom_smc_dpm_info_v4_5 struct atom_smc_dpm_info_v4_6 struct atom_smc_dpm_info_v4_7 struct atom_smc_dpm_info_v4_10 PPTable_t so the grouped members can be referenced together. This will allow memcpy() and sizeof() to more easily reason about sizes, improve readability, and avoid future warnings about writing beyond the end of the first member. "pahole" shows no size nor member offset changes to any structs. "objdump -d" shows no object code changes. Cc: "Christian König" Cc: "Pan, Xinhui" Cc: David Airlie Cc: Daniel Vetter Cc: Hawking Zhang Cc: Feifei Xu Cc: Lijo Lazar Cc: Likun Gao Cc: Jiawei Gu Cc: Evan Quan Cc: amd-...@lists.freedesktop.org Cc: dri-devel@lists.freedesktop.org Signed-off-by: Kees Cook Acked-by: Alex Deucher Link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2FCADnq5_Npb8uYvd%2BR4UHgf-w8-cQj3JoODjviJR_Y9w9wqJ71mQ%40mail.gmail.comdata=04%7C01%7Clijo.lazar%40amd.com%7C3861f20094074bf7328808d962a433f2%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637649279701053991%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=386LcfJJGfQfHsXBuK17LMqxJ2nFtGoj%2FUjoN2ZtJd0%3Dreserved=0 --- drivers/gpu/drm/amd/include/atomfirmware.h | 9 - .../gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h| 3 ++- drivers/gpu/drm/amd/pm/inc/smu11_driver_if_navi10.h | 3 ++- .../gpu/drm/amd/pm/inc/smu13_driver_if_aldebaran.h | 3 ++- Hi Kees, Hi! Thanks for looking into this. The headers which define these structs are firmware/VBIOS interfaces and are picked directly from those components. There are difficulties in grouping them to structs at the original source as that involves other component changes. So, can you help me understand this a bit more? It sounds like these are generated headers, yes? I'd like to understand your constraints and weight them against various benefits that could be achieved here. The groupings I made do appear to be roughly documented already, for example: struct atom_common_table_header table_header; // SECTION: BOARD PARAMETERS + struct_group(dpm_info, Something emitted the "BOARD PARAMETERS" section heading as a comment, so it likely also would know where it ends, yes? The good news here is that for the dpm_info groups, they all end at the end of the existing structs, see: struct atom_smc_dpm_info_v4_5 struct atom_smc_dpm_info_v4_6 struct atom_smc_dpm_info_v4_7 struct atom_smc_dpm_info_v4_10 The matching regions in the PPTable_t structs are similarly marked with a "BOARD PARAMETERS" section heading comment: --- a/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h +++ b/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h @@ -643,6 +643,7 @@ typedef struct { // SECTION: BOARD PARAMETERS // SVI2 Board Parameters + struct_group(v4_6, uint16_t MaxVoltageStepGfx; // In mV(Q2) Max voltage step that SMU will request. Multiple steps are taken if voltage change exceeds this value. uint16_t MaxVoltageStepSoc; // In mV(Q2) Max voltage step that SMU will request. Multiple steps are taken if voltage change exceeds this value. @@ -728,10 +729,10 @@ typedef struct { uint32_t BoardVoltageCoeffB;// decode by /1000 uint32_t BoardReserved[7]; + ); // Padding for MMHUB - do not modify this uint32_t MmHubPadding[8]; // SMU internal use - } PPTable_t; Where they end seems known as well (the padding switches from a "Board" to "MmHub" prefix at exactly the matching size). So, given that these regions are already known by the export tool, how about just updating the export tool to emit a struct there? I imagine the problem with this would be the identifier churn needed, but that's entirely mechanical. However, I'm curious about another aspect of these regions: they are, by definition, the same. Why isn't there a single struct describing them already, given the existing redundancy? For example, look at the member names: maxvoltagestepgfx vs MaxVoltageStepGfx. Why aren't these the same? And then why aren't they described separately? Fixing that would cut down on the redundancy here, and in the renaming, you can fix the identifiers as well. It should be straight forward to write a Coccinelle script to do this renaming for you after extracting the structure. The driver_if_* files updates are frequent and it is error prone to manually group them each time we pick them for any update. Why are these structs updated? It looks like
Re: [PATCH v2 18/63] drm/amd/pm: Use struct_group() for memcpy() region
On Wed, Aug 18, 2021 at 05:12:28PM +0530, Lazar, Lijo wrote: > > On 8/18/2021 11:34 AM, Kees Cook wrote: > > In preparation for FORTIFY_SOURCE performing compile-time and run-time > > field bounds checking for memcpy(), memmove(), and memset(), avoid > > intentionally writing across neighboring fields. > > > > Use struct_group() in structs: > > struct atom_smc_dpm_info_v4_5 > > struct atom_smc_dpm_info_v4_6 > > struct atom_smc_dpm_info_v4_7 > > struct atom_smc_dpm_info_v4_10 > > PPTable_t > > so the grouped members can be referenced together. This will allow > > memcpy() and sizeof() to more easily reason about sizes, improve > > readability, and avoid future warnings about writing beyond the end of > > the first member. > > > > "pahole" shows no size nor member offset changes to any structs. > > "objdump -d" shows no object code changes. > > > > Cc: "Christian König" > > Cc: "Pan, Xinhui" > > Cc: David Airlie > > Cc: Daniel Vetter > > Cc: Hawking Zhang > > Cc: Feifei Xu > > Cc: Lijo Lazar > > Cc: Likun Gao > > Cc: Jiawei Gu > > Cc: Evan Quan > > Cc: amd-...@lists.freedesktop.org > > Cc: dri-devel@lists.freedesktop.org > > Signed-off-by: Kees Cook > > Acked-by: Alex Deucher > > Link: > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2FCADnq5_Npb8uYvd%2BR4UHgf-w8-cQj3JoODjviJR_Y9w9wqJ71mQ%40mail.gmail.comdata=04%7C01%7Clijo.lazar%40amd.com%7C92b8d2f072f0444b9f8508d9620f6971%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637648640625729624%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=rKh5LUXCRUsorYM3kSpG2tkB%2Fczwl9I9EBnWBCtbg6Q%3Dreserved=0 > > --- > > drivers/gpu/drm/amd/include/atomfirmware.h | 9 - > > .../gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h| 3 ++- > > drivers/gpu/drm/amd/pm/inc/smu11_driver_if_navi10.h | 3 ++- > > .../gpu/drm/amd/pm/inc/smu13_driver_if_aldebaran.h | 3 ++- > > Hi Kees, Hi! Thanks for looking into this. > The headers which define these structs are firmware/VBIOS interfaces and are > picked directly from those components. There are difficulties in grouping > them to structs at the original source as that involves other component > changes. So, can you help me understand this a bit more? It sounds like these are generated headers, yes? I'd like to understand your constraints and weight them against various benefits that could be achieved here. The groupings I made do appear to be roughly documented already, for example: struct atom_common_table_header table_header; // SECTION: BOARD PARAMETERS + struct_group(dpm_info, Something emitted the "BOARD PARAMETERS" section heading as a comment, so it likely also would know where it ends, yes? The good news here is that for the dpm_info groups, they all end at the end of the existing structs, see: struct atom_smc_dpm_info_v4_5 struct atom_smc_dpm_info_v4_6 struct atom_smc_dpm_info_v4_7 struct atom_smc_dpm_info_v4_10 The matching regions in the PPTable_t structs are similarly marked with a "BOARD PARAMETERS" section heading comment: --- a/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h +++ b/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h @@ -643,6 +643,7 @@ typedef struct { // SECTION: BOARD PARAMETERS // SVI2 Board Parameters + struct_group(v4_6, uint16_t MaxVoltageStepGfx; // In mV(Q2) Max voltage step that SMU will request. Multiple steps are taken if voltage change exceeds this value. uint16_t MaxVoltageStepSoc; // In mV(Q2) Max voltage step that SMU will request. Multiple steps are taken if voltage change exceeds this value. @@ -728,10 +729,10 @@ typedef struct { uint32_t BoardVoltageCoeffB;// decode by /1000 uint32_t BoardReserved[7]; + ); // Padding for MMHUB - do not modify this uint32_t MmHubPadding[8]; // SMU internal use - } PPTable_t; Where they end seems known as well (the padding switches from a "Board" to "MmHub" prefix at exactly the matching size). So, given that these regions are already known by the export tool, how about just updating the export tool to emit a struct there? I imagine the problem with this would be the identifier churn needed, but that's entirely mechanical. However, I'm curious about another aspect of these regions: they are, by definition, the same. Why isn't there a single struct describing them already, given the existing redundancy? For example, look at the member names: maxvoltagestepgfx vs MaxVoltageStepGfx. Why aren't these the same? And then why aren't they described separately? Fixing that would cut down on the redundancy here, and in the renaming, you can fix the identifiers as well. It should be straight forward to write a Coccinelle script to do this renaming for you after extracting the structure. > The driver_if_* files updates are frequent and it is error prone to manually > group them
Re: [PATCH v2 18/63] drm/amd/pm: Use struct_group() for memcpy() region
On 8/18/2021 11:34 AM, Kees Cook wrote: In preparation for FORTIFY_SOURCE performing compile-time and run-time field bounds checking for memcpy(), memmove(), and memset(), avoid intentionally writing across neighboring fields. Use struct_group() in structs: struct atom_smc_dpm_info_v4_5 struct atom_smc_dpm_info_v4_6 struct atom_smc_dpm_info_v4_7 struct atom_smc_dpm_info_v4_10 PPTable_t so the grouped members can be referenced together. This will allow memcpy() and sizeof() to more easily reason about sizes, improve readability, and avoid future warnings about writing beyond the end of the first member. "pahole" shows no size nor member offset changes to any structs. "objdump -d" shows no object code changes. Cc: "Christian König" Cc: "Pan, Xinhui" Cc: David Airlie Cc: Daniel Vetter Cc: Hawking Zhang Cc: Feifei Xu Cc: Lijo Lazar Cc: Likun Gao Cc: Jiawei Gu Cc: Evan Quan Cc: amd-...@lists.freedesktop.org Cc: dri-devel@lists.freedesktop.org Signed-off-by: Kees Cook Acked-by: Alex Deucher Link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2FCADnq5_Npb8uYvd%2BR4UHgf-w8-cQj3JoODjviJR_Y9w9wqJ71mQ%40mail.gmail.comdata=04%7C01%7Clijo.lazar%40amd.com%7C92b8d2f072f0444b9f8508d9620f6971%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637648640625729624%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=rKh5LUXCRUsorYM3kSpG2tkB%2Fczwl9I9EBnWBCtbg6Q%3Dreserved=0 --- drivers/gpu/drm/amd/include/atomfirmware.h | 9 - .../gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h| 3 ++- drivers/gpu/drm/amd/pm/inc/smu11_driver_if_navi10.h | 3 ++- .../gpu/drm/amd/pm/inc/smu13_driver_if_aldebaran.h | 3 ++- Hi Kees, The headers which define these structs are firmware/VBIOS interfaces and are picked directly from those components. There are difficulties in grouping them to structs at the original source as that involves other component changes. The driver_if_* files updates are frequent and it is error prone to manually group them each time we pick them for any update. Our usage of memcpy in this way is restricted only to a very few places. As another option - is it possible to have a helper function/macro like memcpy_fortify() which takes the extra arguments and does the extra compile time checks? We will use the helper whenever we have such kind of usage. Thanks, Lijo drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c| 6 +++--- drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 12 drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c | 6 +++--- 7 files changed, 28 insertions(+), 14 deletions(-) diff --git a/drivers/gpu/drm/amd/include/atomfirmware.h b/drivers/gpu/drm/amd/include/atomfirmware.h index 44955458fe38..7bf3edf15410 100644 --- a/drivers/gpu/drm/amd/include/atomfirmware.h +++ b/drivers/gpu/drm/amd/include/atomfirmware.h @@ -2081,6 +2081,7 @@ struct atom_smc_dpm_info_v4_5 { struct atom_common_table_header table_header; // SECTION: BOARD PARAMETERS + struct_group(dpm_info, // I2C Control struct smudpm_i2c_controller_config_v2 I2cControllers[8]; @@ -2159,7 +2160,7 @@ struct atom_smc_dpm_info_v4_5 uint32_t MvddRatio; // This is used for MVDD Vid workaround. It has 16 fractional bits (Q16.16) uint32_t BoardReserved[9]; - + ); }; struct atom_smc_dpm_info_v4_6 @@ -2168,6 +2169,7 @@ struct atom_smc_dpm_info_v4_6 // section: board parameters uint32_t i2c_padding[3]; // old i2c control are moved to new area + struct_group(dpm_info, uint16_t maxvoltagestepgfx; // in mv(q2) max voltage step that smu will request. multiple steps are taken if voltage change exceeds this value. uint16_t maxvoltagestepsoc; // in mv(q2) max voltage step that smu will request. multiple steps are taken if voltage change exceeds this value. @@ -2246,12 +2248,14 @@ struct atom_smc_dpm_info_v4_6 // reserved uint32_t boardreserved[10]; + ); }; struct atom_smc_dpm_info_v4_7 { struct atom_common_table_header table_header; // SECTION: BOARD PARAMETERS + struct_group(dpm_info, // I2C Control struct smudpm_i2c_controller_config_v2 I2cControllers[8]; @@ -2348,6 +2352,7 @@ struct atom_smc_dpm_info_v4_7 uint8_t Padding8_Psi2; uint32_t BoardReserved[5]; + ); }; struct smudpm_i2c_controller_config_v3 @@ -2478,6 +2483,7 @@ struct atom_smc_dpm_info_v4_10 struct atom_common_table_header table_header; // SECTION: BOARD PARAMETERS + struct_group(dpm_info, // Telemetry Settings uint16_t GfxMaxCurrent; // in Amps uint8_t GfxOffset; // in Amps @@ -2524,6 +2530,7 @@ struct atom_smc_dpm_info_v4_10 uint16_t spare5; uint32_t reserved[16]; + ); }; /* diff --git a/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h