On 5/8/2024 6:32 PM, Thomas Gleixner wrote:
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:
+struct xfeat_component {
+ u32 xfeat_type;
+ u32 xfeat_sz;
+ u32 xfeat_off;
+ u32 xfeat_flags;
+} __packed;
Why repeating xfeat_ for all member names?
u32 type;
u32 size;
u32 offset;
u32 flags;
is sufficient and obvious, no?
+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?
First version of patch was similar to what you mentioned here and other
review comments to use existing kernel definitions.
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.
Please let us know your suggestions.
I will fix all other review comments in my next version.
+#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.
thanks,
vigneshbalu.
