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;
From:   "Bae, Chang Seok" <chang.seok.bae@intel.com>
To:     "Hansen, Dave" <dave.hansen@intel.com>
CC:     Borislav Petkov <bp@suse.de>, "Lutomirski, Andy" <luto@kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@kernel.org>,
        "the arch/x86 maintainers" <x86@kernel.org>,
        "Brown, Len" <len.brown@intel.com>,
        "Macieira, Thiago" <thiago.macieira@intel.com>,
        "Liu, Jing2" <jing2.liu@intel.com>,
        "Shankar, Ravi V" <ravi.v.shankar@intel.com>,
        "Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
        "linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>
Subject: Re: [PATCH v9 25/26] intel_idle/amx: Add SPR support with XTILEDATA
 capability
Thread-Topic: [PATCH v9 25/26] intel_idle/amx: Add SPR support with XTILEDATA
 capability
Thread-Index: AQHXhVR149TJzWjXb0+t+rLnICh8Iatb2nWAgAZ5AIA=
Date:   Tue, 3 Aug 2021 21:32:29 +0000
Message-ID: <9660CC2A-F09F-49BF-B5CF-7288B726C88E@intel.com>
References: <20210730145957.7927-1-chang.seok.bae@intel.com>
 <20210730145957.7927-26-chang.seok.bae@intel.com>
 <f17eb0da-c77e-cc95-b88c-6947a68f5f9a@intel.com>
In-Reply-To: <f17eb0da-c77e-cc95-b88c-6947a68f5f9a@intel.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?LD07wdXwYLwq/TmkMEnf6VsAMiMzV4PffOdL7wGaI4j2BX3WUsWnufYYWszE?=
 =?us-ascii?Q?PmvkLIb48wNOKkPNKnL0f8+oJWqjnhn3805nFWW/l4Ew+6TQ4bjozG/cgC2f?=
 =?us-ascii?Q?wHsUt4H0KeSCppYWM2GR/Fsj49dS4e4Mk4h2vSfn6+MQN8PmuspKFRTuSToD?=
 =?us-ascii?Q?UCklL6UOUS8JxlEG1Cp52YO7Pk5GspYWN2U86jVfn20CGX31+YDBPe/6aE5m?=
 =?us-ascii?Q?gOWPDKuD+7ZG3h3/MhYPvxlMV2/yfNDF540IE3DHhqUevg1Uji4X/6r/bGaA?=
 =?us-ascii?Q?7F4NTT0nH5H71W+GIfkXN9onGD4FJJGfAOiF/FF3LO8yOFnqxvRZNjwMptgV?=
 =?us-ascii?Q?BEiGaPY3N3+z3H+vpPL9vPBDDNhrOHAYbuVQEoYMnbLMQx6lcKC4knIXk1jO?=
 =?us-ascii?Q?iNEiwwFb+2BWA4DZB/hJW8p0gVJt8KPABtXup4ii09pvs+3NReMjtc0VuFOM?=
 =?us-ascii?Q?0Wgyl0dRobqWYVyNIGB3YAVLqz8lFdh7qH/KUYGOHa//Z1HjuyVJb9ZCg7uT?=
 =?us-ascii?Q?KKlRWG4hGiyTojVHm627gBNKIlM7H6VRvC6IiFs8bXWIVtcFsCd1A2uXnfhI?=
 =?us-ascii?Q?G16UbfvI3Kf78S74jA73Rlx+8Zh8bL4fsepJgFXPj2dgV5zmAuJSoMz+TTad?=
 =?us-ascii?Q?Vv1Ek7js2G8apLxNnwwMsoE1KJzVbS/lfbJoVV2mVPY77abW6dvXxD9bx52j?=
 =?us-ascii?Q?QgRt7Z6sIEZWJSptBIG9X7GK7OIh/HHaICp5c5/nfE8UBOAh5f6+UMw/durf?=
 =?us-ascii?Q?SgnEb9Mvv/FIBcx9bAAIKQDOJBBBbOfMWhrkVuS8QnZbpCEd+18iGUrtJZAf?=
 =?us-ascii?Q?yvWWhqvEvSFEopFgIUoPjYyvDFbzI3HEXkKTcC5MLhqkJzDdrDoQX6jYTxai?=
 =?us-ascii?Q?6TB1tQ2YJOtX1YfGsI3NWr9W2aBHnzimj72gX2Wo24UEpyLaOqSEHv3TBuxu?=
 =?us-ascii?Q?75T+WB7yv/XbfbzqLgc4HO+A2pAhqjiTKKex4Z9Zb8N5jeEEcU7A8uWqshVZ?=
 =?us-ascii?Q?F7soiLHMW+5kgFfxFQKDCvCymygH0UyXMM1i+UkXIy+J88tss/OPETjr2AnR?=
 =?us-ascii?Q?+u1NopZVFZzAJMTO9pWhauOoLP0BSUbWnv01KeSSztcPFJZEjiEqgF1SLeXj?=
 =?us-ascii?Q?eucbECm2C3LluSBaRkhrAGzLYXSohfipBR+QP3XYtGkZ9B43hOMpNW4Gnzgy?=
 =?us-ascii?Q?zv2kypo92RMN8n97NcWcHKE0z0FDbNHSfnyNxPjmCbhzt0s1Xp+AfmoOAtv4?=
 =?us-ascii?Q?TwkzrQNFFLPtgNpGqT5p4/JwOIyUs8gxUK9uQVId60A1ljWLXoVHU4RDLHuc?=
 =?us-ascii?Q?vLj2o5oFyVDzYEP9sUbH1xr5?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <E3C21616BCAE3948B907DE61429A3D62@namprd11.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: PH0PR11MB4855.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 32e62206-30f4-47a9-962d-08d956c6325f
X-MS-Exchange-CrossTenant-originalarrivaltime: 03 Aug 2021 21:32:29.6721
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: Ae95JverNDepV4SVuAARjf2Y2CWAOJ/z/Ol16tI7WS2nmhsGsJWJ2XpwIprgiEumjYmy0tgZl9zmunBc3y0ciCgxPdgwphIYws8IRKrJAms=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB5063
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Jul 30, 2021, at 11:41, Hansen, Dave <dave.hansen@intel.com> wrote:
>> +/**
>> + * idle_tile - Initialize TILE registers in INIT-state
>> + *
>> + * Leaving state in the dirty TILE registers may prevent the processor =
from
>> + * entering lower-power idle states. Use TILERELEASE to initialize the
>> + * state. Destroying fpregs state is safe after the fpstate update.
>> + */
>> +static inline void idle_tile(void)
>> +{
>> +	if (boot_cpu_has(X86_FEATURE_XGETBV1) && (xgetbv(1) & XFEATURE_MASK_XT=
ILE)) {
>> +		tile_release();
>> +		fpregs_deactivate(&current->thread.fpu);
>> +	}
>> +}
>=20
> This isn't obviously safe code.  There's a window in there when we have
> bogus, destroyed FPU register state but where we might be rescheduled.
>=20
> I would assume that preempt is off *somewhere* in this, but it would be
> nice to make sure of that, or at least mention the requirement for it to
> be off before this code is safe.

I can see preempt_disable() in this path:

$kernel/sched/idle.c::play_idle_precise()
--> preempt_disable()
...
--> do_idle()
    --> cpuidle_idle_call()
        --> call_cpuidle()
            --> $drivers/cpuidle/cpuidle.c::cpuidle_enter()
                --> cpuidle_enter_state()
                    --> target_state->enter()
                        --> $drivers/idle/intel_idle.c::intel_idle_tile()
                            --> idle_tile()
...
--> preempt_enable()

> I'm also not sure TILERELEASE is *technically* what you want here.
> xgetbv(1) tells you whether a feature is being tracked by the processor
> as being in its init state.  tile_release() gets a feature into its init
> state, but does not guarantee that the processor will *track* it as
> being in the init state.
>=20
> TILERELEASE is not documented to have an effect on XINUSE (init tracking)=
.
>=20
> XRSTOR, on the other hand, is at least documented to affect XINUSE.
>=20
> It sounds like we either need a documentation update, or a clear
> explanation why TILERELEASE is being used over XRSTOR.

TILERELEASE impacts INIT tracking at least on the first AMX implementation.=
 I
agree that the documentation needs some update.

Thanks,
Chang=