Re: [PATCH 2/3] x86: introduce xstate_zero

From: Jan Beulich <jbeulich@suse.com>
To: Fouad Hilly <fouad.hilly@cloud.com>
Cc: "Wei Liu" <wei.liu2@citrix.com>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH 2/3] x86: introduce xstate_zero
Date: Tue, 5 Mar 2024 10:06:17 +0100	[thread overview]
Message-ID: <d2e6e522-216b-4bcf-a75d-2bbc8e1563ce@suse.com> (raw)
In-Reply-To: <20240304091307.2295344-3-fouad.hilly@cloud.com>

On 04.03.2024 10:13, Fouad Hilly wrote:
> +void xstate_zero(uint64_t mask)
> +{
> +    bool ok;
> +    struct xsave_struct tmp;
> +
> +    tmp.fpu_sse.mxcsr = MXCSR_DEFAULT;

This is a clear indication that the function name is wrong. Perhaps it is
"reset" that was meant?

> +    tmp.xsave_hdr.xstate_bv = 0;
> +    tmp.xsave_hdr.xcomp_bv = 0;
> +    memset(tmp.xsave_hdr.reserved, 0, sizeof(tmp.xsave_hdr.reserved));
> +
> +    ok = xrstor__(&tmp, mask, mask);

There's a lot of "tmp" that is left uninitialized, which would introduce
a leak of (stack) data. I think "tmp" instead wants to have an initializer
(consisting of just the explicit setting of MXCSR, leaving everything else
to be default, i.e. zero-initialized).

> +    ASSERT(ok);
>  }
>  
>  bool xsave_enabled(const struct vcpu *v)
> @@ -731,6 +753,9 @@ int handle_xsetbv(u32 index, u64 new_bv)
>      if ( (new_bv & ~xcr0_max) || !valid_xcr0(new_bv) )
>          return -EINVAL;
>  
> +    /* Zero state components before writing new XCR0 */
> +    xstate_zero(get_xcr0());

This change isn't explained in the description, doesn't fit the subject
(i.e. mechanical and functional change likely want splitting), and is
imo wrong: Why would XSETBV gain this kind of side effect, when processed
(emulated) in Xen? What I can see may want clearing are state components
which were never enabled before by a vCPU. That would then need doing
after writing the new XCR0 value, though. And it looks like that's
already in place - is there something wrong with that code?

Or is this about clearing in hardware state components about to be turned
off? If so, it would again need to be only the delta of components that's
reset here, and their state would need saving first. Upon re-enabling of
a component, that state would then need to be available for restoring.
This need for saving of state would then also explain why what's presently
named xstate_zero() can't very well use the vCPU's ->arch.xsave_area, but
needs to have a (relatively big) on-stack variable instead.

Jan