On 02.01.2025 09:45, Tu Dinh wrote: > --- a/xen/arch/x86/include/asm/xstate.h > +++ b/xen/arch/x86/include/asm/xstate.h > @@ -33,13 +33,13 @@ extern uint32_t mxcsr_mask; > #define XSTATE_FP_SSE (X86_XCR0_X87 | X86_XCR0_SSE) > #define XCNTXT_MASK (X86_XCR0_X87 | X86_XCR0_SSE | X86_XCR0_YMM | \ > X86_XCR0_OPMASK | X86_XCR0_ZMM | X86_XCR0_HI_ZMM | \ > - XSTATE_NONLAZY) > + XSTATE_NONLAZY | XSTATE_XSAVES_ONLY)
This is odd - in the sole pre-existing place where the #define is used you then remove X86_XSS_STATES again. Plus please see also https://lists.xen.org/archives/html/xen-devel/2021-04/msg01336.html. > #define XSTATE_ALL (~(1ULL << 63)) > #define XSTATE_NONLAZY (X86_XCR0_BNDREGS | X86_XCR0_BNDCSR | X86_XCR0_PKRU | > \ > X86_XCR0_TILE_CFG | X86_XCR0_TILE_DATA) > #define XSTATE_LAZY (XSTATE_ALL & ~XSTATE_NONLAZY) > -#define XSTATE_XSAVES_ONLY 0 > +#define XSTATE_XSAVES_ONLY (X86_XSS_LBR) > #define XSTATE_COMPACTION_ENABLED (1ULL << 63) > > #define XSTATE_XSS (1U << 0) > @@ -91,6 +91,21 @@ struct xstate_bndcsr { > uint64_t bndstatus; > }; > > +struct xstate_lbr_entry { > + uint64_t lbr_from_ip; > + uint64_t lbr_to_ip; > + uint64_t lbr_info; > +}; > + > +struct xstate_lbr { > + uint64_t lbr_ctl; > + uint64_t lbr_depth; > + uint64_t ler_from_ip; > + uint64_t ler_to_ip; > + uint64_t ler_info; > + struct xstate_lbr_entry entries[32]; > +}; Doesn't this 32 appear in an earlier patch as well? They need tying together via a #define then. Also nit: Hard tabs slipped in. > @@ -114,6 +129,9 @@ int xstate_alloc_save_area(struct vcpu *v); > void xstate_init(struct cpuinfo_x86 *c); > unsigned int xstate_uncompressed_size(uint64_t xcr0); > unsigned int xstate_compressed_size(uint64_t xstates); > +void *get_xstate_component_comp(struct xsave_struct *xstate, > + unsigned int size, > + uint64_t component); > > static inline uint64_t xgetbv(unsigned int index) > { > diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c > index 289cf10b78..68a419ac8e 100644 > --- a/xen/arch/x86/msr.c > +++ b/xen/arch/x86/msr.c > @@ -522,8 +522,7 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t > val) > if ( !cp->xstate.xsaves ) > goto gp_fault; > > - /* No XSS features currently supported for guests */ > - if ( val != 0 ) > + if ( val & ~(uint64_t)XSTATE_XSAVES_ONLY ) > goto gp_fault; Imo we'd be better off arranging for casts like this to not be required. It's too easy to leave one out unintentionally. > @@ -210,7 +214,7 @@ void expand_xsave_states(const struct vcpu *v, void > *dest, unsigned int size) > * non-compacted offset. > */ > src = xstate; > - valid = xstate_bv & ~XSTATE_FP_SSE; > + valid = xstate_bv & ~XSTATE_FP_SSE & ~X86_XSS_STATES; > while ( valid ) > { > u64 feature = valid & -valid; > @@ -276,7 +280,7 @@ void compress_xsave_states(struct vcpu *v, const void > *src, unsigned int size) > * possibly compacted offset. > */ > dest = xstate; > - valid = xstate_bv & ~XSTATE_FP_SSE; > + valid = xstate_bv & ~XSTATE_FP_SSE & ~X86_XSS_STATES; > while ( valid ) > { > u64 feature = valid & -valid; I can't figure why these two changes are needed, and the description isn't of any help here either. > @@ -1072,6 +1085,30 @@ void xstate_set_init(uint64_t mask) > BUG(); > } > > +void *get_xstate_component_comp(struct xsave_struct *xstate, > + unsigned int size, > + uint64_t component) > +{ > + uint16_t comp_offsets[sizeof(xfeature_mask) * 8]; > + uint16_t offset; > + unsigned int i = ilog2(component); > + > + ASSERT(generic_hweightl(component) == 1); > + > + if ( !(xstate->xsave_hdr.xcomp_bv & XSTATE_COMPACTION_ENABLED) || > + i >= xstate_features || > + xstate_sizes[i] == 0 || > + !(xstate->xsave_hdr.xcomp_bv & component) ) > + return NULL; > + > + setup_xstate_comp(comp_offsets, xstate->xsave_hdr.xcomp_bv); > + offset = comp_offsets[i]; > + if ( xstate_sizes[i] + offset > size ) > + return NULL; > + > + return (void *)xstate + offset; > +} The function is unused at this point. Besides this being a Misra violation (unreachable code), it also means it's left unclear what the purpose is. That would, for example, influence whether there should be some "const" applied. I find it particularly worrying that the function returns a pointer to non-const, thus permitting the caller to fiddle with the contents. Similarly it's left unclear what the "size" parameter is for. Jan
