From mboxrd@z Thu Jan  1 00:00:00 1970
From: Dave Hansen <dave.hansen@intel.com>
Subject: Re: [PATCH 18/22] x86/fpu: Update xstate's PKRU value on write_pkru()
Date: Wed, 23 Jan 2019 09:28:24 -0800
Message-ID: <3d345560-af6c-1edb-2579-c1b59ab42eee@intel.com>
References: <20190109114744.10936-1-bigeasy@linutronix.de>
 <20190109114744.10936-19-bigeasy@linutronix.de>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Cc: x86@kernel.org, Andy Lutomirski <luto@kernel.org>,
        Paolo Bonzini <pbonzini@redhat.com>,
        =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= <rkrcmar@redhat.com>,
        kvm@vger.kernel.org, "Jason A. Donenfeld" <Jason@zx2c4.com>,
        Rik van Riel <riel@surriel.com>,
        Dave Hansen <dave.hansen@linux.intel.com>
To: Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
        linux-kernel@vger.kernel.org
Return-path: <linux-kernel-owner@vger.kernel.org>
In-Reply-To: <20190109114744.10936-19-bigeasy@linutronix.de>
Content-Language: en-US
Sender: linux-kernel-owner@vger.kernel.org
List-Id: kvm.vger.kernel.org

On 1/9/19 3:47 AM, Sebastian Andrzej Siewior wrote:
> +	pk = get_xsave_addr(&current->thread.fpu.state.xsave, XFEATURE_PKRU);
> +	/*
> +	 * The PKRU value in xstate needs to be in sync with the value that is
> +	 * written to the CPU. The FPU restore on return to userland would
> +	 * otherwise load the previous value again.
> +	 */
> +	__fpregs_changes_begin();
> +	if (pk)
> +		pk->pkru = pkru;
> +	__write_pkru(pkru);
> +	__fpregs_changes_end();
>  }

I'm not sure this is right.

The "if (pk)" check is basically to see if there was a location for
XFEATURE_PKRU in the XSAVE buffer.  The only way this can be false in
the current code is if the "init optimization" is in play and
XFEATURE_PKRU was in the init state (all 0's for PKRU).

If it were in the init state, we need to take it *out* of the init
state, both in the buffer and in the registers.  The __write_pkru()
obviously does this for the registers, but "pk->pkru = pkru" is not
enough for the XSAVE buffer.  xsave->header.xfeatures (aka. XSTATE_BV)
also needs to have XFEATURE_PKRU set.  Otherwise, two calls to this
function in succession would break.

	pk = get_xsave_addr(...xsave, XFEATURE_PKRU);
	pk->pkru = pkru;
	__write_pkru(pkru);

	pk = get_xsave_addr(...xsave, XFEATURE_PKRU);
	/* 'pk' is still NULL, won't see 'pkru' set */

I *think* just setting

	xsave->header.xfeatures |= XFEATURE_MASK_PKRU;

will fix this.  I thought we did that whole dance somewhere else in the
code, but I don't see it right now.  Might have been in some other patch.