From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mx.groups.io with SMTP id smtpd.web10.167997.1673880162795320773 for ; Mon, 16 Jan 2023 06:42:42 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=RdoR9klJ; spf=pass (domain: kernel.org, ip: 139.178.84.217, mailfrom: ardb@kernel.org) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 2BCAD60FE5 for ; Mon, 16 Jan 2023 14:42:42 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 14767C4339B for ; Mon, 16 Jan 2023 14:42:41 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1673880161; bh=PV45wJhcDI9rkfCBi0Lb/KXr81pJlEVjYVBZTCUtNho=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=RdoR9klJpmriGedjPFpEXdukPVAzkdbf8CgJyjOU1/hXk6UTGcMvNr5hEkmNWUSoX cN+EjtjgtR3EPEUGVkWlvQ6EtSNs/sma4H5LMhySa4luYnbcOya6BjnWUVMtDOqovP lI+yUTVSo4y7QMYWCskBy/S0mTuNcImGWc6eeZhy6lyRAPEYLWU+xF3cMA+SgOU9RO CTdSa1V8t4IJydGIT0IPQK+p8ruDjQrSSvPxi67skeagION7ac9LBuVznmKP3lDiiV M9dd5z6Ueb3ilUep2DUGnGfsr/3Ut4UYlgiClpdNJKHnB0o44Whgm9at6f0/HEqKKU yuW80TxXTZK4g== Received: by mail-lf1-f49.google.com with SMTP id g13so43030231lfv.7 for ; Mon, 16 Jan 2023 06:42:40 -0800 (PST) X-Gm-Message-State: AFqh2kpnL/Ua5K8wzmd4YgnmY6H4rwC+ihi2uEkjPim5Tw1uaEMuwAFw a+4/2M7Hj+nlfTCe5VBeYilxyUa2onSy8yVsCpA= X-Google-Smtp-Source: AMrXdXtLWMhPvKPNmMxOCYNE5QogNvtnWfzEbUs816SXl1qzvUJsFssE5F638YlZokGUiqJ//dI7Jf4ZzYPcNjE11WE= X-Received: by 2002:ac2:5d4e:0:b0:4b5:964d:49a4 with SMTP id w14-20020ac25d4e000000b004b5964d49a4mr7802255lfd.637.1673880159012; Mon, 16 Jan 2023 06:42:39 -0800 (PST) MIME-Version: 1.0 References: <20230112082845.128463-1-lersek@redhat.com> <01020185a568604c-e16d8581-963a-4ff3-8566-bf0640ad327d-000000@eu-west-1.amazonses.com> <407c5cee-7a6c-cbc8-35cc-8f2c2724914c@redhat.com> <01020185a6bda78a-05d82180-4d1a-4af4-9a9b-ac78088d11ed-000000@eu-west-1.amazonses.com> <49e4e8bb-3bbd-0ca8-ee59-e75560deffa7@redhat.com> <20230113060354.siony3rjwpgzd5tk@sirius.home.kraxel.org> <20230113093205.oh7euprqlmp26wpu@sirius.home.kraxel.org> <20230113122246.uabdhut4ziwerivm@sirius.home.kraxel.org> In-Reply-To: <20230113122246.uabdhut4ziwerivm@sirius.home.kraxel.org> From: "Ard Biesheuvel" Date: Mon, 16 Jan 2023 15:42:27 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [PATCH v2] OvmfPkg/PlatformInitLib: catch QEMU's CPU hotplug reg block regression To: Gerd Hoffmann Cc: Laszlo Ersek , devel@edk2.groups.io, Michael Brown , Ard Biesheuvel , Brijesh Singh , Erdem Aktas , James Bottomley , Jiewen Yao , Jordan Justen , Min Xu , Oliver Steffen , Sebastien Boeuf , Tom Lendacky Content-Type: text/plain; charset="UTF-8" On Fri, 13 Jan 2023 at 13:22, Gerd Hoffmann wrote: > > On Fri, Jan 13, 2023 at 11:10:54AM +0100, Laszlo Ersek wrote: > > On 1/13/23 10:32, Gerd Hoffmann wrote: > > > On Fri, Jan 13, 2023 at 07:03:54AM +0100, Gerd Hoffmann wrote: > > >> Hi, > > >> > > >>> - QEMU can be configured with other compat properties on the command > > >>> line so that "CPU hotplug with SMI" and "CPU hot-unplug with SMI" *not* > > >>> be offered to the firmware. Then QEMU will reject hotplug attempts, and > > >>> the SMM hotplug code in edk2 will not be triggered by the (virtual) > > >>> hardware. > > >> > > >> Can we have edk2 print instructions for that in the error message? > > > > > > This seems to be: > > > > > > qemu -M q35 \ > > > -global ICH9-LPC.x-smi-cpu-hotplug=off \ > > > -global ICH9-LPC.x-smi-cpu-hotunplug=off > > > > Yes, those are the flags. > > > > > But it appears to not work. > > > > They should work, but they take effect in QEMU, and not in the firmware. > > These knobs control what CPU hot(un)plug+SMI features QEMU exposes to > > the guest fw, via fw_cfg, > > Ok, I see, only the SMM code actually checks that. > > > In particular the firmware makes no further decisions based on whether > > QEMU advertized some of these features. > > I was thinking the other way around: When cpu hotplug is disabled in > qemu it should be safe to skip the whole cpu hotplug checking dance. > See test patch below. > > That would give us a config switch (turn off cpu hotplug support) which > would allow edk2 run on qemu versions with broken cpu hotplug. > > Does the idea look sane or do I miss something? > I cannot review the actual patch due to lack of x86/smm/qemu/hotplug knowledge, but if this allows us to merge Laszlo's fix without breaking all current QEMU/x86 TCG users, I'm all for it. > > commit bd2e36eba35268ab46c0125d2b9125391ea6f9fc > Author: Gerd Hoffmann > Date: Fri Jan 13 13:07:36 2023 +0100 > > skip cpu present checking when hotplug is off > > diff --git a/OvmfPkg/Library/PlatformInitLib/Platform.c b/OvmfPkg/Library/PlatformInitLib/Platform.c > index 13348afb4890..2b0f0c836f85 100644 > --- a/OvmfPkg/Library/PlatformInitLib/Platform.c > +++ b/OvmfPkg/Library/PlatformInitLib/Platform.c > @@ -415,8 +415,9 @@ PlatformMaxCpuCountInitialization ( > IN OUT EFI_HOB_PLATFORM_INFO *PlatformInfoHob > ) > { > - UINT16 BootCpuCount = 0; > - UINT32 MaxCpuCount; > + UINT16 BootCpuCount = 0; > + UINT32 MaxCpuCount; > + BOOLEAN CpuHotplugSupported = FALSE; > > // > // Try to fetch the boot CPU count. > @@ -424,6 +425,31 @@ PlatformMaxCpuCountInitialization ( > if (QemuFwCfgIsAvailable ()) { > QemuFwCfgSelectItem (QemuFwCfgItemSmpCpuCount); > BootCpuCount = QemuFwCfgRead16 (); > + DEBUG ((DEBUG_INFO, "%a: BootCpuCount: %d\n", __FUNCTION__, BootCpuCount)); > + } > + > + { > + FIRMWARE_CONFIG_ITEM SupportedFeaturesItem; > + UINTN SupportedFeaturesSize; > + UINT64 mSmiFeatures; > + EFI_STATUS Status; > + > + Status = QemuFwCfgFindFile ( > + "etc/smi/supported-features", > + &SupportedFeaturesItem, > + &SupportedFeaturesSize > + ); > + > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_INFO, "%a: etc/smi/supported-features: %r\n", __FUNCTION__, Status)); > + } else { > + QemuFwCfgSelectItem (SupportedFeaturesItem); > + QemuFwCfgReadBytes (sizeof mSmiFeatures, &mSmiFeatures); > + DEBUG ((DEBUG_INFO, "%a: etc/smi/supported-features: 0x%x\n", __FUNCTION__, mSmiFeatures)); > + if (mSmiFeatures & (BIT1 /* hotplug */ | BIT2 /* hotunplug */)) { > + CpuHotplugSupported = TRUE; > + } > + } > } > > if (BootCpuCount == 0) { > @@ -435,6 +461,9 @@ PlatformMaxCpuCountInitialization ( > // > DEBUG ((DEBUG_WARN, "%a: boot CPU count unavailable\n", __FUNCTION__)); > MaxCpuCount = PlatformInfoHob->DefaultMaxCpuNumber; > + } else if (!CpuHotplugSupported) { > + DEBUG ((DEBUG_INFO, "%a: CPU hotplug support not available\n", __FUNCTION__)); > + MaxCpuCount = BootCpuCount; > } else { > // > // We will expose BootCpuCount to MpInitLib. MpInitLib will count APs up to >