Received-SPF: pass (google.com: domain of linux-kernel+bounces-185792-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Wed, 22 May 2024 14:10:13 +0800
From: Chao Gao <chao.gao@intel.com>
To: Sean Christopherson <seanjc@google.com>
CC: Paolo Bonzini <pbonzini@redhat.com>, <kvm@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, Kai Huang <kai.huang@intel.com>
Subject: Re: [PATCH v2 1/6] KVM: Register cpuhp and syscore callbacks when
 enabling hardware
Message-ID: <Zk2MRRkS6c5cGYSV@chao-email>
References: <20240522022827.1690416-1-seanjc@google.com>
 <20240522022827.1690416-2-seanjc@google.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240522022827.1690416-2-seanjc@google.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?0WW20S/BMy0jZeVHWWflLt3nhFz7P+JOdvbMp84R+e15DWXh2UwXMcFLZbNM?=
 =?us-ascii?Q?3LUVfRSiILEa9YArPY6fcUn+wca5bpabSeBTfPrpo1pnR/94EZ1bMXjnlmOh?=
 =?us-ascii?Q?jdpKsslRptxhj94MlbqrJwpygHEYAln739RDsDr4q46SqoEol6ipNJ9Im+mP?=
 =?us-ascii?Q?vPBxeD0oTkBWqGK3z1L/5Wo4aosiBTiyK3FDFO/uvrPuA93NSF9NAS5LCMhT?=
 =?us-ascii?Q?t/LiXRQOzX3Ttkp3Hoqj0JQ+A0Rv+Boinonl/obadTC9YTq1XNDOUxfqvPxp?=
 =?us-ascii?Q?ZBcWGdyDMUR31hqjrgt7fqGFzFNyj9br4vYXfRcQZgGk85qTAZyTiwRdN5Pi?=
 =?us-ascii?Q?/RYqcqn9pBmwSwyC4tP5x7W47/CVDpNQr+2c/7vqtqGsvMEtb59yRI9EB/xw?=
 =?us-ascii?Q?dqiQ7HUbjmKfMzatiEE8C026ckAuTxbuCOqZiXdRIlXUEjblsDJn+HpBYgC+?=
 =?us-ascii?Q?jGA4iB7s6iSinyCsW/W0nOuN9KrOMrmntuYlt6EPospOPmJ53CPAQfUXByBC?=
 =?us-ascii?Q?iKEgBvoIujvPaIzOJUiE1pVLkMi9M2lKiEBVHNmi0devGTowLVF81erm8LeO?=
 =?us-ascii?Q?BkI+e4OMKTBIPB1jzw3ZBcV9LnNwBVmOo6yKj6X78pp8QAxX3d2Tlzpsex0u?=
 =?us-ascii?Q?MiDpP7epJkeyeAS6h4Ro+b7EFoPU7ZGqpWjDRiV/ulzxljdzuX7IEiwSWkgx?=
 =?us-ascii?Q?IRBqQ5vt9FvAb8XaVrfkQhYxFYGxocYyCkUQm6drBijGxHCWeWLaoqMF0tti?=
 =?us-ascii?Q?MviiFUr9DSfA0rfXjwxMYLk7BBnKJwFlawJFY6Y4WL/1cafUmSjC70lRgMh4?=
 =?us-ascii?Q?hh2Czc0BECFhpld/3AB5+M2yEbUsBLBc8DbtYQBjhfwYWzRkbLKN7mCatJF2?=
 =?us-ascii?Q?eroj+OQZUlAeSDEr9oIxNkRZRMTguZ72ZxSJnwyOrVfb2Xw//MeaOSApmG5b?=
 =?us-ascii?Q?FbmnfOWjv8yNTcZEmzdwEaoNJ7nSKlBf/4VFG/0sd/noPHkOHel5QX9QMz1D?=
 =?us-ascii?Q?9qNUznX7XWLvko027Pl99j+GK9WrwvrvSXqpwXaUXtWcVhYUiWArkJ/mQLGh?=
 =?us-ascii?Q?DFdPTM24Pi/WwUYLeixpFC05f6FHlZ/Mf54THI10rV6wcWnhYSdIPAPlw9gY?=
 =?us-ascii?Q?bH0oVN5TRrL2V1FL6kcqOiFW+XXBaOiUzAUQW8hEo5jKGTFT3x+zodykBVTG?=
 =?us-ascii?Q?zOBrx0IgXBk5FJK1ABJFR4huwkhW+eNLqhh83JJAsIMwSRvDdN/SgSD6GTVE?=
 =?us-ascii?Q?fKppnd8MbIQnMAPZ4F6Ufe3l2ddNVWJqcTgL96I8dzwU1ND3y6X+IHS1qtV2?=
 =?us-ascii?Q?IjWiq6TvCKWE0AY9J43j7n1IqyC5V1oS2pAbknZzBHbszxcbyRFEG8oUzmXc?=
 =?us-ascii?Q?V+3iEToWOLmHeXAfRbFxUzydsoa8Kk9u2Mt9fyBMG+QhiUo1yv80xYlxcq0T?=
 =?us-ascii?Q?iWhZ5v+AjtPfm4/IZZ6SqErJCrgF+xx6FoTh50q4x5mB/pvocQc13sxq6MHI?=
 =?us-ascii?Q?ZJNzUAku3vBb4vEkznSXozGSfvkMvGwoEFb4/S/9/9OZF6/++UB1rGGyncQH?=
 =?us-ascii?Q?BD4AQ0BvTAgUifmJX2b7P8qqNwN42eEZVQZeTR1Y?=
X-MS-Exchange-CrossTenant-Network-Message-Id: becd2f3a-0daf-45ad-7fef-08dc7a25dcaa
X-MS-Exchange-CrossTenant-AuthSource: CH3PR11MB8660.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 May 2024 06:10:21.5704
 (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: j/13L3ZSXRqxysvM8wBHtlqep44C30v8ceK2NJIFtn+OPQ3rPEoi1d/ohNe5rWXi51Wajg50q9Eo6MKQdwXO8w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB6423
X-OriginatorOrg: intel.com

On Tue, May 21, 2024 at 07:28:22PM -0700, Sean Christopherson wrote:
>Register KVM's cpuhp and syscore callback when enabling virtualization
>in hardware instead of registering the callbacks during initialization,
>and let the CPU up/down framework invoke the inner enable/disable
>functions.  Registering the callbacks during initialization makes things
>more complex than they need to be, as KVM needs to be very careful about
>handling races between enabling CPUs being onlined/offlined and hardware
>being enabled/disabled.
>
>Intel TDX support will require KVM to enable virtualization during KVM
>initialization, i.e. will add another wrinkle to things, at which point
>sorting out the potential races with kvm_usage_count would become even
>more complex.
>

>Use a dedicated mutex to guard kvm_usage_count, as taking kvm_lock outside
>cpu_hotplug_lock is disallowed.  Ideally, KVM would *always* take kvm_lock
>outside cpu_hotplug_lock, but KVM x86 takes kvm_lock in several notifiers
>that may be called under cpus_read_lock().  kvmclock_cpufreq_notifier() in
>particular has callchains that are infeasible to guarantee will never be
>called with cpu_hotplug_lock held.  And practically speaking, using a
>dedicated mutex is a non-issue as the cost is a few bytes for all of KVM.

Shouldn't this part go to a separate patch?

I think so because you post a lockdep splat which indicates the existing
locking order is problematic. So, using a dedicated mutex actually fixes
some bug and needs a "Fixes:" tag, so that it can be backported separately.

And Documentation/virt/kvm/locking.rst needs to be updated accordingly.

Actually, you are doing a partial revert to the commit:

  0bf50497f03b ("KVM: Drop kvm_count_lock and instead protect kvm_usage_count with kvm_lock")

Perhaps you can handle this as a revert. After that, change the lock from
a raw_spinlock_t to a mutex.