Received-SPF: pass (google.com: domain of linux-kernel+bounces-152102-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
From: "Wang, Wei W" <wei.w.wang@intel.com>
To: Sean Christopherson <seanjc@google.com>
CC: "pbonzini@redhat.com" <pbonzini@redhat.com>, "kvm@vger.kernel.org"
	<kvm@vger.kernel.org>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>
Subject: RE: [RFC PATCH v2 4/5] KVM: x86: Remove KVM_X86_OP_OPTIONAL
Thread-Topic: [RFC PATCH v2 4/5] KVM: x86: Remove KVM_X86_OP_OPTIONAL
Thread-Index: AQHakkz4v8XEzjLts0GEpU850B7z97FvmgoAgAAKIQCAABv5AIAApn8w
Date: Sat, 20 Apr 2024 03:01:44 +0000
Message-ID: <DS0PR11MB6373A7876C312CA9A06AF349DC0C2@DS0PR11MB6373.namprd11.prod.outlook.com>
References: <20240419112952.15598-1-wei.w.wang@intel.com>
 <20240419112952.15598-5-wei.w.wang@intel.com> <ZiJ0mjZxlRsLwl3E@google.com>
 <DS0PR11MB6373D059F2BB9F196AA9D503DC0D2@DS0PR11MB6373.namprd11.prod.outlook.com>
 <ZiKNWM0XyMqbKrD2@google.com>
In-Reply-To: <ZiKNWM0XyMqbKrD2@google.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?sXHA1+QWKqfoburqccXboawRUk8ym3QsJdwQOzZhrp3ok4FXC7vJRCybs1Rm?=
 =?us-ascii?Q?ZYEgh7DRFoe9xoiCY9owQAkPli4ZlPusienXhw4jFL7HMBe6HnGLHgrlmvvB?=
 =?us-ascii?Q?z0UdXjhWewln2gEDOKUGga3TyBg3nZ4HPAsAl7uqVjyTr84nkKaCDx18Fzb8?=
 =?us-ascii?Q?Fbsbp/iHJR0C350dms9VgLzbOMiZnu4EA5yjLtXismTAYUTgya9C6oiifTcR?=
 =?us-ascii?Q?KG+iFCFgBo5K/x2siHzum0PJC4U23QZ4o6V3Xd7lfB7gP+fd8Bgx6FqoiifV?=
 =?us-ascii?Q?EwAitFPUjMLbZOUM2vw2CJgv4S7SHQ3Dygumo4P/WuX6JfqRVHZbKNQdjBRq?=
 =?us-ascii?Q?Z1Yigv5UXowLoCofkBs1PdI2HhpWdqJ3raz5uTnoFVl9jC2kPEq2Mckp2DVZ?=
 =?us-ascii?Q?iNq66ouN1f3COUhK2bvZmOwcNBtajzpEge5n5s8DChY/eT5Bf9lTfLhHYq6D?=
 =?us-ascii?Q?K2glbQmnZYtIB52/OGoUJ5hGOa87CS6qkHAh/b356tyRGDqAmTuszbPmTwzm?=
 =?us-ascii?Q?FZVlui1/r49DarlI6qSQWZwcJ2Ryf7Vk3Fmds9J0V2LjH71TGCk07wlyODbZ?=
 =?us-ascii?Q?g8QEst29BA3OdknjZn/t5F2Sk2Q1mOEscNYZmHPAOb73nvdqsr47AcfAI6yz?=
 =?us-ascii?Q?QVP6d49UHkNT+HNYrRYGU+8r4EV6giKoOPt0WjxM9NDvztYjd1bZTm9YXOAN?=
 =?us-ascii?Q?AkPxAT7Yf9py8xNPH0GpxTBrwnsY+SZqMJv75ZYP2CiOjGfMawZu1eeoxc1o?=
 =?us-ascii?Q?ApnXbXtMpirrv7x4EnivG5hNBHFXOJ88NfrhVMfY7NXvVmT1S04p9n6aq0dC?=
 =?us-ascii?Q?Caw8JTUa5hOwidggRqqkgW5X6WCg2UbW0y5BvDbIJ+lUUKhCyeQu8/skLAQ6?=
 =?us-ascii?Q?Ydc0jqcs8+mO6gKZSRbKvQ3bxWCR50ZFRTRCBGFXmZg/I4tJdJlWUEr+7bC8?=
 =?us-ascii?Q?ZuF35t+Y+xoOZ5jrKKe3K7clT8Wf0BTgZzeglp2bRTfpHUo+C+5rQASlo6sZ?=
 =?us-ascii?Q?9FLbAnqX5KCXW1t7cck9xeRIR6CuCX21F2PbjDR4elocA7ULXvyqPTCkfaOX?=
 =?us-ascii?Q?HCPmMcyhMzoNOrK4Uki08K9KelybcZ0ew0AycLi7J8ehGaygKVcK3FV51LxA?=
 =?us-ascii?Q?fnpE0iZgOuy3GunGVilhB0wi8ksRWGLrW+BbtmMJlKHysN/FmWrDIBMWuWNR?=
 =?us-ascii?Q?SZEKoOCFQyjbqKg1T85MN3dyBlNLfoRosCmHcxVoRoF2MBT7tjKZk5O3OkSl?=
 =?us-ascii?Q?nrvEr/MoL3JXzs2YCR9hG1KJPjCd1YAcP8TmYJKbWrec++7TyL3bH49xZEpt?=
 =?us-ascii?Q?G9nARLYDyST7UC1h1JvAgY0chZ8CFzZdPpUI0ukiEEM4vF/GWD9qxfLCR34P?=
 =?us-ascii?Q?13nF5rYpgPetTO+dJuBP4qWs7k7TSaUzJCXh0JEzNVf4j39j21DBjNA17uvA?=
 =?us-ascii?Q?7qJw2W1PEz53gx1GxfdNOLV698AMrqaZHC1W0fcoHocdDNClaS9DgU/PjbYK?=
 =?us-ascii?Q?5Ko6kPuYFfNQOxkktlTVMk8PxjvDy9LrRjcvfEehTSGfnEEvHRe2doRCMNcp?=
 =?us-ascii?Q?5Kj19n81dhwvNI7GlOqF1Ngx1UBis1nsyXtdJfe2?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DS0PR11MB6373.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 3a9c2e81-3422-4d40-d05c-08dc60e63610
X-MS-Exchange-CrossTenant-originalarrivaltime: 20 Apr 2024 03:01:44.5120
 (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: jfSv2wPjj8goyHpF3fdkvW1adRHszKCun/3L8hDXM1ju4sVkxkJo9i4zE1JAr5c8kLoEUhAUaEwF2nhHf/q3xg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB8227
X-OriginatorOrg: intel.com

On Friday, April 19, 2024 11:58 PM, Sean Christopherson wrote:
> On Fri, Apr 19, 2024, Wei W Wang wrote:
> > On Friday, April 19, 2024 9:42 PM, Sean Christopherson wrote:
> > > On Fri, Apr 19, 2024, Wei Wang wrote:
> > > > KVM_X86_OP and KVM_X86_OP_OPTIONAL were utilized to define and
> > > execute
> > > > static_call_update() calls on mandatory and optional hooks, respect=
ively.
> > > > Mandatory hooks were invoked via static_call() and necessitated
> > > > definition due to the presumption that an undefined hook (i.e.,
> > > > NULL) would cause
> > > > static_call() to fail. This assumption no longer holds true as
> > > > static_call() has been updated to treat a "NULL" hook as a NOP on x=
86.
> > > > Consequently, the so-called mandatory hooks are no longer required
> > > > to be defined, rendering them non-mandatory.
> > >
> > > This is wrong.  They absolutely are mandatory.  The fact that
> > > static_call() doesn't blow up doesn't make them optional.  If a
> > > vendor neglects to implement a mandatory hook, KVM *will* break,
> > > just not immediately on the static_call().
> > >
> > > The static_call() behavior is actually unfortunate, as KVM at least
> > > would prefer that it does explode on a NULL point.  I.e. better to
> > > crash the kernel (hopefully before getting to production) then to
> > > have a lurking bug just waiting to cause problems.
> > >
> > > > This eliminates the need to differentiate between mandatory and
> > > > optional hooks, allowing a single KVM_X86_OP to suffice.
> > > >
> > > > So KVM_X86_OP_OPTIONAL and the WARN_ON() associated with
> > > KVM_X86_OP
> > > > are removed to simplify usage,
> > >
> > > Just in case it isn't clear, I am very strongly opposed to removing
> > > KVM_X86_OP_OPTIONAL() and the WARN_ON() protection to ensure
> > > mandatory ops are implemented.
> >
> > OK, we can drop patch 4 and 5.
> >
> > Btw, may I know what is the boundary between mandatory and optional
> hooks?
> > For example, when adding a new hook, what criteria should we use to
> > determine whether it's mandatory, thereby requiring both SVM and VMX
> > to implement it (and seems need to be merged them together?) (I
> > searched a bit, but didn't find it)
>=20
> It's a fairly simple rule: is the hook required for functional correctnes=
s, at all
> times?
>=20
> E.g. post_set_cr3() is unique to SEV-ES+ guests, and so it's optional for=
 both
> VMX and SVM (because SEV-ES might not be enabled).
>=20
> All of the APICv related hooks are optional, because APICv support isn't
> guaranteed.
>=20
> set_tss_addr() and set_identity_map_addr() are unique to old Intel hardwa=
re.
>=20
> The mem_enc ops are unique to SEV+ (and at some point TDX), which again
> isn't guaranteed to be supported and enabled.
>=20
> For something like vcpu_precreate(), it's an arbitrary judgment call: is =
it
> cleaner to make the hook optional, or to have SVM implement a nop?
> Thankfully, there are very few of these.
>=20
> Heh, vm_destroy() should be non-optional, we should clean that up.

I think determining whether a hook is optional is easy, but classifying a h=
ook as
mandatory might be challenging due to the multiple options available to ach=
ieve
functional correctness.

Take the vm_destroy() example as you mentioned, it could be debatable to sa=
y
it's mandatory, e.g. the VMX code could be adjusted by incorporating vmx_vm=
_destroy()
into the vcpu_free() hook, and being invoked upon the first vcpu to be free=
d.
It could be even harder at the time when the first user (e.g. SVM) adds the=
 hook
and classifies that vm_destroy() is mandatory.
(not try to argue for anything, just want to gain a comprehensive understan=
ding of the rules)