Date:   Thu, 2 Mar 2023 13:36:56 +0800
From:   Chao Gao <chao.gao@intel.com>
To:     Kai Huang <kai.huang@intel.com>
CC:     <kvm@vger.kernel.org>, <pbonzini@redhat.com>, <seanjc@google.com>,
        <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] KVM: VMX: Make setup_vmcs_config() preemption disabled
Message-ID: <ZAA1+EMTIkBJvdZF@gao-cwp>
References: <20230301105438.599196-1-kai.huang@intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20230301105438.599196-1-kai.huang@intel.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?u2uld52aQ++5QrYnHiW9Ec7Coz3ApHQrAgKPCXX6lbHt29QwLGugBPkO0J5x?=
 =?us-ascii?Q?naEUFgo9FCWbNx7EyaN2trnxqxJQ9Z5lHTIaSlSOpq8FcYbADF5TJdD9JJC5?=
 =?us-ascii?Q?Cz6bl7qsQ+7ZDPH4fEA58DSrtsi9Il729104jTZs4PMlyWS27jHCy7dl4X/b?=
 =?us-ascii?Q?SZXMsjyd/jN4U67Kv7xuLBdSNJDi7UOvsTM6GTdvNVxQ0WKizw7/ZBIEXeJ0?=
 =?us-ascii?Q?J2Vl59tsfeblJgazZQS64jmnthLCsoM/oYYIHR9LvNh2zFkOZhf5I9dBZ3GZ?=
 =?us-ascii?Q?JPwwsVT6rZAaqGCOdqlPOnShLGmgFCunHoHP8hzgAHId2xDJqD5n0uMVmqCn?=
 =?us-ascii?Q?Nc4IIMq0oajCvit6xPemQx8kMNezW5L8fJt40sY+Wd/B1EjlxzkFFyT3XUIk?=
 =?us-ascii?Q?N2EKaV3rOpfmiwPzGWyV+8gBD9/ydzyn2niHrAWwnX4vaS+GmPw2wumO/2gw?=
 =?us-ascii?Q?pEzyRbZRzyaaHOSCbCLRzwok7K284ITbqIpb2SIs/9ISfi5dQ/7R2mfNIXDB?=
 =?us-ascii?Q?NfYXIq90ugfvE1V9jHGl9NXJZa279tfnTEbXtipcLI3Mada4NNLD+KlyeCti?=
 =?us-ascii?Q?EDtXKRs++ZMUTXtsmBY6awZzRRVM/R7DZgnRJAK3sI+spkOJ/c/GtkhVO8GR?=
 =?us-ascii?Q?3qLZjsYf3ZYJYQ7BksEq12ewD9EPMd0DPFSBxavKD8TitUePj3Xs4UprB/4Q?=
 =?us-ascii?Q?sygxkxDs5to6fswBc1NbskP50BCU9brJ76FdS8W9dbodDBVNzloGgN21rDDs?=
 =?us-ascii?Q?QUWX/oL6MRd6ZuSBBzqv2pVgfp2aok2eioHjcprtOXeIurErbFodmOUkUUc4?=
 =?us-ascii?Q?M1ret8xrWpES1RfG+6kWUvSvJ/cKfLZFukes5/S6KruY9bQl09O1n+PIyQRf?=
 =?us-ascii?Q?C8ImMfOMBpROcrVL47mJEHCNNP1gQ7YeVYfuw1JZQKtkNXqT2tLYaIjJmVV6?=
 =?us-ascii?Q?qMe+84Fj6oKV2hnVfUHyGPNNx7ZvjGUEnwKuSbrEmHnCCxSUkcxdvo5hAh6I?=
 =?us-ascii?Q?a44u0ywTf9yHwMub5uQX1U9/tdhO7Ssu+TqLj8bOyJpnhhf9VeorEHwXSobp?=
 =?us-ascii?Q?5fv3JN9VQMXmRLGJFt0zfB/8snRoyTubUGjpozAf8uMrlErUAMwFah2u2Fji?=
 =?us-ascii?Q?8lVVBk64oUViq5ZYeS3dFjhlM4gYlSieCmZH784ogZNNAcBtT9SQWGJktkOz?=
 =?us-ascii?Q?n1Y3uekZmp/Umhuk5wi9tEft7hO/F+yaSFoekUPHSm6xr05ar9K+jZjzPv6u?=
 =?us-ascii?Q?DXW/sLbOEBy4tUVCurFruYbJ6VTygYJfIX+KvbfXiwrt/rVD0rIuBjqHAHrX?=
 =?us-ascii?Q?58wZk3bJvPFeIgonBcFMDit2pNC9XZoByneWLd84vLKN8UOIv5ZtZE3ZT05+?=
 =?us-ascii?Q?NLbheWat4ezATLi6vHpyX2l9CV1eM9hh1Yshy79nHCnUudavMAUKUaZCx8N8?=
 =?us-ascii?Q?30Ru0hbaSI/OQo8gpMec8EcC6g4rjMj5uGNGHJxYQt76+UmeQaTNqyJNH9iB?=
 =?us-ascii?Q?xQRvstUNRryMa9c4NgmdIPDMlvLO4NbodcBaZLCWmcqGUKS8Zz9S8hTDnAJ9?=
 =?us-ascii?Q?ejjdSvDIbtHzKZyUqbhUYJ56VN4zaQl2PScEWBEb?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 57bcb25b-e7f0-45f8-281a-08db1ae01534
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB6780.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Mar 2023 05:36:33.5198
 (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: s00Nioi6TFz+OnkUNfxzM+OFb/tGfU9WHI453RnNJt3K/M1VsSDMI7oyXBcAcLhToU8iP9KQO4X5a1vhtX7Ydg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB5598
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Wed, Mar 01, 2023 at 11:54:38PM +1300, Kai Huang wrote:
>Make setup_vmcs_config() preemption disabled so it always performs on
>the same local cpu.
>
>During module loading time, KVM intends to call setup_vmcs_config() to
>set up the global VMCS configurations on _one_ cpu in hardware_setup(),
>and then calls setup_vmcs_config() on all other online cpus via sending

*all other* is misleading. The compatibility check is actually done on
*all* online cpus.

        for_each_online_cpu(cpu) {
                smp_call_function_single(cpu, kvm_x86_check_cpu_compat, &r, 1);
                if (r < 0)
                        goto out_unwind_ops;
        }

Given this, it probably is ok to not disable preemption because all CPUs
are guaranteed to be compatible later in the flow in terms of VMCS
capabilities. But we don't want to have such a subtle dependency.

Do you see any real problem with preemption enabled?

>IPI to perform VMX compatibility check.  Further more, KVM has CPU
>hotplug callback to call setup_vmcs_config() to do compatibility check
>on the "new-online" cpu to make sure it is compatible too.
>
>setup_vmcs_config() is supposed to be done on the same cpu.  This is
>true in the compatibility check code path as setup_vmcs_config() is
>called either via IPI or in per-cpu CPU hotplug thread.  However, the
>first call from hardware_setup() isn't as it is called when preemption
>is enabled.
>
>Change the existing setup_vmcs_config() to __setup_vmcs_config() and
>call the latter directly in the compatibility check code path.  Change
>setup_vmcs_config() to call __setup_vmcs_config() with preemption
>disabled so __setup_vmcs_config() is always done on the same cpu.

Maybe you can simply disable preemption in hardware_setup() although I
don't have a strong preference.

nested_vmx_setup_ctls_msrs() also reads some MSRs and sets up part of
vmcs_conf, should it be called on the same CPU as setup_vmcs_config()?