From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by mx.groups.io with SMTP id smtpd.web10.3570.1614117213171738626 for ; Tue, 23 Feb 2021 13:53:34 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=RkdGq/po; spf=pass (domain: redhat.com, ip: 63.128.21.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1614117212; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=h7KwO9+uIaeJo1JT5MFC0dW6BNJ0sKktkWaMmtTv+ng=; b=RkdGq/poFW1Z6Qgrfv9/Ou7tQqP3BbqbCGJMnJ4ZPLlo5DYX+IWoscIVV763dgFpl5/buN Ydm6laOF5QhCQxFrGtOdPBzpdF4iRdyArjsi2IXIil909xHEFTwNsoZmcKAxwArVEKlI3H hrXlZUoGtn8yEJSA4xwfxWtZD8g7jL4= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-518-MQ9toYNWM6ucezRy4z3VAQ-1; Tue, 23 Feb 2021 16:52:23 -0500 X-MC-Unique: MQ9toYNWM6ucezRy4z3VAQ-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 98D8419357B6; Tue, 23 Feb 2021 21:52:21 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-109.ams2.redhat.com [10.36.113.109]) by smtp.corp.redhat.com (Postfix) with ESMTP id CF22760BD8; Tue, 23 Feb 2021 21:52:19 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v8 10/10] OvmfPkg/SmmControl2Dxe: negotiate CPU hot-unplug To: devel@edk2.groups.io, ankur.a.arora@oracle.com Cc: imammedo@redhat.com, boris.ostrovsky@oracle.com, Jordan Justen , Ard Biesheuvel , Aaron Young References: <20210222071928.1401820-1-ankur.a.arora@oracle.com> <20210222071928.1401820-11-ankur.a.arora@oracle.com> From: "Laszlo Ersek" Message-ID: <6e05b2fe-2a55-c52e-7c91-eb2f2d4d3f3f@redhat.com> Date: Tue, 23 Feb 2021 22:52:18 +0100 MIME-Version: 1.0 In-Reply-To: <20210222071928.1401820-11-ankur.a.arora@oracle.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 02/22/21 08:19, Ankur Arora wrote: > Advertise OVMF support for CPU hot-unplug and negotiate it > if QEMU requests the feature. > > Cc: Laszlo Ersek > Cc: Jordan Justen > Cc: Ard Biesheuvel > Cc: Igor Mammedov > Cc: Boris Ostrovsky > Cc: Aaron Young > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132 > Signed-off-by: Ankur Arora > --- > > Notes: > Addresses the following review comments: > (1,3) s/hot unplug/hot-unplug/ > (2) Get rid of the reference to the made up ICH9_APM_CNT_CPU_HOT_UNPLUG > (4,6) Remove the artificial tie in between > ICH9_LPC_SMI_F_CPU_HOTPLUG, ICH9_LPC_SMI_F_CPU_HOT_UNPLUG. > (5) Fully spell out "SMI on CPU hot-unplug". > (7) Emit separate messages on negotiation (or not) of > ICH9_LPC_SMI_F_CPU_HOT_UNPLUG. > > OvmfPkg/SmmControl2Dxe/SmiFeatures.c | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/OvmfPkg/SmmControl2Dxe/SmiFeatures.c b/OvmfPkg/SmmControl2Dxe/SmiFeatures.c > index c9d875543205..b1d59a559dae 100644 > --- a/OvmfPkg/SmmControl2Dxe/SmiFeatures.c > +++ b/OvmfPkg/SmmControl2Dxe/SmiFeatures.c > @@ -29,6 +29,12 @@ > // > #define ICH9_LPC_SMI_F_CPU_HOTPLUG BIT1 > > +// The following bit value stands for "enable CPU hot-unplug, and inject an SMI > +// with control value ICH9_APM_CNT_CPU_HOTPLUG upon hot-unplug", in the > +// "etc/smi/supported-features" and "etc/smi/requested-features" fw_cfg files. > +// > +#define ICH9_LPC_SMI_F_CPU_HOT_UNPLUG BIT2 > + (1) The comment formatting is inconsistent with the rest. The line right after the (extant) "ICH9_LPC_SMI_F_CPU_HOTPLUG" macro definition should be "//", and not an empty line. In fact, this new hunk should not introduce any empty line. > // > // Provides a scratch buffer (allocated in EfiReservedMemoryType type memory) > // for the S3 boot script fragment to write to and read from. > @@ -112,7 +118,8 @@ NegotiateSmiFeatures ( > QemuFwCfgReadBytes (sizeof mSmiFeatures, &mSmiFeatures); > > // > - // We want broadcast SMI, SMI on CPU hotplug, and nothing else. > + // We want broadcast SMI, SMI on CPU hotplug, SMI on CPU hot-unplug > + // and nothing else. > // > RequestedFeaturesMask = ICH9_LPC_SMI_F_BROADCAST; > if (!MemEncryptSevIsEnabled ()) { > @@ -120,8 +127,10 @@ NegotiateSmiFeatures ( > // For now, we only support hotplug with SEV disabled. > // > RequestedFeaturesMask |= ICH9_LPC_SMI_F_CPU_HOTPLUG; > + RequestedFeaturesMask |= ICH9_LPC_SMI_F_CPU_HOT_UNPLUG; > } > mSmiFeatures &= RequestedFeaturesMask; > + (2) Spurious empty line addition (it's likely left over from removing the earlier attempt to "be smarter than QEMU"). The rest seems fine. Thanks! Laszlo > QemuFwCfgSelectItem (mRequestedFeaturesItem); > QemuFwCfgWriteBytes (sizeof mSmiFeatures, &mSmiFeatures); > > @@ -166,6 +175,13 @@ NegotiateSmiFeatures ( > __FUNCTION__)); > } > > + if ((mSmiFeatures & ICH9_LPC_SMI_F_CPU_HOT_UNPLUG) == 0) { > + DEBUG ((DEBUG_INFO, "%a: CPU hot-unplug not negotiated\n", __FUNCTION__)); > + } else { > + DEBUG ((DEBUG_INFO, "%a: CPU hot-unplug with SMI negotiated\n", > + __FUNCTION__)); > + } > + > // > // Negotiation successful (although we may not have gotten the optimal > // feature set). >