On 5/8/2024 6:32 PM, Thomas Gleixner
wrote:
First version of patch was similar to what you mentioned here and other review comments to use existing kernel definitions.Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding. On Tue, May 07 2024 at 15:23, Vignesh Balasubramanian wrote:+enum custom_feature { + FEATURE_XSAVE_FP = 0, + FEATURE_XSAVE_SSE = 1, + FEATURE_XSAVE_YMM = 2, + FEATURE_XSAVE_BNDREGS = 3, + FEATURE_XSAVE_BNDCSR = 4, + FEATURE_XSAVE_OPMASK = 5, + FEATURE_XSAVE_ZMM_Hi256 = 6, + FEATURE_XSAVE_Hi16_ZMM = 7, + FEATURE_XSAVE_PT = 8, + FEATURE_XSAVE_PKRU = 9, + FEATURE_XSAVE_PASID = 10, + FEATURE_XSAVE_CET_USER = 11, + FEATURE_XSAVE_CET_SHADOW_STACK = 12, + FEATURE_XSAVE_HDC = 13, + FEATURE_XSAVE_UINTR = 14, + FEATURE_XSAVE_LBR = 15, + FEATURE_XSAVE_HWP = 16, + FEATURE_XSAVE_XTILE_CFG = 17, + FEATURE_XSAVE_XTILE_DATA = 18, + FEATURE_MAX, + FEATURE_XSAVE_EXTENDED_START = FEATURE_XSAVE_YMM, + FEATURE_XSAVE_EXTENDED_END = FEATURE_XSAVE_XTILE_DATA, +};Why can't this use the existing 'enum xfeature' which is providing exactly the same information already?
https://lore.kernel.org/linux-mm/20240314112359.50713-1-vigbalas@xxxxxxx/T/
As per the review comment
https://lore.kernel.org/linux-mm/20240314162954.GAZfMmAnYQoRjRbRzc@fat_crate.local/
, modified the patch to be a independent of kernel internal
definitions.
Though this enum and below function "get_sub_leaf" are not useful
now, it will be required when we extend for a new/different
features.
I will fix all other review comments in my next version.
thanks,
vigneshbalu.
+#ifdef CONFIG_COREDUMP +static int get_sub_leaf(int custom_xfeat) +{ + switch (custom_xfeat) { + case FEATURE_XSAVE_YMM: return XFEATURE_YMM; + case FEATURE_XSAVE_BNDREGS: return XFEATURE_BNDREGS; + case FEATURE_XSAVE_BNDCSR: return XFEATURE_BNDCSR; + case FEATURE_XSAVE_OPMASK: return XFEATURE_OPMASK; + case FEATURE_XSAVE_ZMM_Hi256: return XFEATURE_ZMM_Hi256; + case FEATURE_XSAVE_Hi16_ZMM: return XFEATURE_Hi16_ZMM; + case FEATURE_XSAVE_PT: return XFEATURE_PT_UNIMPLEMENTED_SO_FAR; + case FEATURE_XSAVE_PKRU: return XFEATURE_PKRU; + case FEATURE_XSAVE_PASID: return XFEATURE_PASID; + case FEATURE_XSAVE_CET_USER: return XFEATURE_CET_USER; + case FEATURE_XSAVE_CET_SHADOW_STACK: return XFEATURE_CET_KERNEL_UNUSED; + case FEATURE_XSAVE_HDC: return XFEATURE_RSRVD_COMP_13; + case FEATURE_XSAVE_UINTR: return XFEATURE_RSRVD_COMP_14; + case FEATURE_XSAVE_LBR: return XFEATURE_LBR; + case FEATURE_XSAVE_HWP: return XFEATURE_RSRVD_COMP_16; + case FEATURE_XSAVE_XTILE_CFG: return XFEATURE_XTILE_CFG; + case FEATURE_XSAVE_XTILE_DATA: return XFEATURE_XTILE_DATA; + default: + pr_warn_ratelimited("Not a valid XSAVE Feature."); + return 0; + } +}This function then maps the identical enums one to one. The only actual "functionality" is the default case and that's completely pointless.
