On 3/14/2024 10:09 PM, Dave Hansen
wrote:
Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding. On 3/14/24 09:29, Borislav Petkov wrote:That argument breaks down a bit on the flags though: xc.xfeat_flags = xstate_flags[i]; Because it comes _directly_ from CPUID with zero filtering: cpuid_count(XSTATE_CPUID, i, &eax, &ebx, &ecx, &edx); ... xstate_flags[i] = ecx; So this layout is quite dependent on what's in x86's CPUID.Yeah, no, this should not be copying CPUID flags - those flags should be *translated* to independently defined flags which describe those buffers.Ditto for: xc.xfeat_type = i; Right now, that's bound to CPUID and XSAVE. "feat_type==10" can only ever be PKRU and that's derived from the XSAVE architecture. If you want this to be extensible to things outside of the XSAVE architecture, it needs to be something actually extensible and not entangled with XSAVE. In other words "xc.xfeat_type" can enumerate XSAVE state components being in the dump, but it should not be limited to XSAVE. Just as an example: enum feat_type { FEATURE_XSAVE_PKRU, FEATURE_XSAVE__YMM, FEATURE_XSAVE_BNDREGS, FEATURE_XSAVE_BNDCSR, ... RANDOM_STATE_NOT_XSAVE }; See how feat_type==1 is PKRU and *NOT* feat_type==10? That opens the door to RANDOM_STATE_NOT_XSAVE or anything else you want. This would be _actually_ extensible.
Thanks for the review.
I will add new enum, instead of using "enum
xfeature".
Currently we are retaining the flags field. The
value will be set to zero at this point, and the field will be
reserved for future use.
GDB / LLDB would not require this field at this point. Do let
us know if this is not OK.
-thanks,
vigneshbalu.
