Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 28 Jul 2021 15:18:35 +0800
From:   Pengfei Xu <pengfei.xu@intel.com>
To:     Dave Hansen <dave.hansen@intel.com>
Cc:     Shuah Khan <skhan@linuxfoundation.org>,
        linux-kselftest <linux-kselftest@vger.kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        Heng Su <heng.su@intel.com>,
        Yu Yu-cheng <yu-cheng.yu@intel.com>,
        Yu Fenghua <fenghua.yu@intel.com>,
        Luck Tony <tony.luck@intel.com>,
        Mehta Sohil <sohil.mehta@intel.com>,
        Chen Yu C <yu.c.chen@intel.com>
Subject: Re: [RFC PATCH v1 0/2] Introduce XSAVE/XRSTOR self-test
Message-ID: <20210728071835.GA12735@fedora29.sh.intel.com>
References: <cover.1627355565.git.pengfei.xu@intel.com>
 <af8e6de6-c428-d3f9-9abf-f8bd5cc95838@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <af8e6de6-c428-d3f9-9abf-f8bd5cc95838@intel.com>
User-Agent: Mutt/1.12.1 (2019-06-15)
Precedence: bulk

Hi Dave,
  Thanks for comments!

On 2021-07-27 at 10:46:45 -0700, Dave Hansen wrote:
> On 7/26/21 8:34 PM, Pengfei Xu wrote:
> > The XSAVE feature set supports the saving and restoring of state components
> > such as FPU, which is used for process context switching.
> 
> This sentence is really awkward.  It reads at first as saying that the
> FPU is used for context switching.  Can you rephrase.
> 
  Thanks for advice, I will update it as below if it's appropriate:
"
The XSAVE feature set supports the saving and restoring of state components,
which is used for process context switching. The state components include
x87 state for FPU execution environment, SSE state, AVX state and so on.
"

> > In order to ensure that XSAVE works correctly, add XSAVE basic test for
> > XSAVE architecture functionality.
> 
> This sentence needs to be start on the same line as the previous one,
> *or* be in a new paragraph.  Please rewrap it.
> 
  Will rewrap it.

> > This patch set tests XSAVE/XRSTOR instructions on x86 platforms and verify if
> > the XSAVE/XRSTOR works correctly during signal handling.
> 
> This reads to me like you are going to test the XSAVE/XRSTOR
> instructions *in* a signal handler, instead of testing the XSAVE/XRSTOR
> instructions that the kernel uses at signal entry/exit.
> 
> Also, the kernel does *NOT* *USE* XSAVE/XRSTOR in many cases to
> save/restore signal state.  The changelog could be read as implying that
> it does.
> 
  Yes, how about updating it as below:
"
This patch set tests and verifies the basic functions of XSAVE/XRSTOR in
user space; during and after signal processing on the x86 platform, the
XSAVE contents of the process should not be changed.
"

> > Cases such as signal handling, process creation, other xstate(except FPU)
> > tests for XSAVE check, etc. will be added to the Linux kernel self-test.
> > If appropriate, it is even planned to add the [1] mentioned XSAVE issues
> > reproduce and some XSAVE anomaly tests to the kernel self-test.
> 
> This is not clear whether it is talking about *this* series int he
> future tense (will be added) or whether it is talking about future *work*.
> 
> Maybe something like this:
> 
> 	This series introduces only the most basic XSAVE tests.  In the
> 	future, the intention is to continue expanding the scope of
> 	these selftests to include more kernel XSAVE-related
> 	functionality and XSAVE-managed features like AMX and shadow
> 	stacks.
Thanks for advice, will update it.

-Pengfei
BR.