From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web09.6334.1611232208147093536 for ; Thu, 21 Jan 2021 04:30:08 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=T6ZU1mVI; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1611232207; 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=rFG4fapSDd524p8Jmfqwnm3JxAtYqHFgE1V64fZyZrw=; b=T6ZU1mVI30B7o9VL5XI+n/TkA5NoJpgMUT//RP+fBqGWgGcNIMLtk4esZ11ncDK4cmOIie vrA0ZT4mw6N4JYt251Pzv6yI+qD5/+rimYVoceY3xOJ4vqRMHWNUIGA8xrxd2QNQfvOJQ4 22QQ7ExtwQHe35dljFdmO1JJqEk8PpE= 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-43-YkyTRuOtNcOCm4MOUR5doQ-1; Thu, 21 Jan 2021 07:30:03 -0500 X-MC-Unique: YkyTRuOtNcOCm4MOUR5doQ-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id DCC6DAFA81; Thu, 21 Jan 2021 12:30:01 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-115-169.ams2.redhat.com [10.36.115.169]) by smtp.corp.redhat.com (Postfix) with ESMTP id 71B02100AE42; Thu, 21 Jan 2021 12:30:00 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v4 0/9] support CPU hot-unplug To: devel@edk2.groups.io, ankur.a.arora@oracle.com Cc: imammedo@redhat.com, boris.ostrovsky@oracle.com References: <20210118063457.358581-1-ankur.a.arora@oracle.com> <5d932166-9413-d195-230f-8d7c9a70aa2e@oracle.com> From: "Laszlo Ersek" Message-ID: <668b5faa-5c8b-97b5-f679-a431768d4f94@redhat.com> Date: Thu, 21 Jan 2021 13:29:59 +0100 MIME-Version: 1.0 In-Reply-To: <5d932166-9413-d195-230f-8d7c9a70aa2e@oracle.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 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: 8bit Hi Ankur, On 01/18/21 19:35, Ankur Arora wrote: > On 2021-01-18 9:09 a.m., Laszlo Ersek wrote: >> On 01/18/21 07:34, Ankur Arora wrote: >>> Hi, >>> >>> This series adds support for CPU hot-unplug with OVMF. >>> >>> Please see this in conjunction with the QEMU secureboot hot-unplug v2 >>> series posted here (now upstreamed): >>>    >>> https://lore.kernel.org/qemu-devel/20201207140739.3829993-1-imammedo@redhat.com/ >>> >>> >>> Patches 1 and 3, >>>    ("OvmfPkg/CpuHotplugSmm: refactor hotplug logic") >>>    ("OvmfPkg/CpuHotplugSmm: add Qemu Cpu Status helper") >>> are either refactors or add support functions. >>> >>>    OvmfPkg/CpuHotplugSmm: introduce UnplugCpus() >>>    OvmfPkg/CpuHotplugSmm: add CpuEject() >>>    OvmfPkg/CpuHotplugSmm: add worker to do CPU ejection >>> >>> Patch 2 and 9, >>>    ("OvmfPkg/CpuHotplugSmm: collect hot-unplug events") >>>    ("OvmfPkg/SmmControl2Dxe: negotiate CPU hot-unplug") >>> handle the QEMU protocol logic for collection of CPU hot-unplug events >>> or the protocol negotiation. >>> >>> Patch 4, >>>    ("OvmfPkg/CpuHotplugSmm: introduce UnplugCpus()") >>> adds the MMI logic for CPU hot-unplug handling and informing >>> the PiSmmCpuDxeSmm of CPU removal. >>> >>> Patches 5 and 6, >>>    ("OvmfPkg/CpuHotplugSmm: define CPU_HOT_EJECT_DATA") >>>    ("OvmfPkg/SmmCpuFeaturesLib: init CPU ejection state") >>> sets up state for doing the CPU ejection as part of hot-unplug. >>> >>> Patches 7, and 8, >>>    ("OvmfPkg/CpuHotplugSmm: introduce UnplugCpus()") >>>    ("OvmfPkg/CpuHotplugSmm: add worker to do CPU ejection") >>> add the CPU ejection logic. >>> >>> Testing (with QEMU 5.2.50): >>>   - Stable with randomized CPU plug/unplug (guest maxcpus=1,8,128) >>>   - Synthetic tests with simultaneous multi CPU hot-unplug >>>   - Negotiation with/without CPU hotplug enabled >>> >>> Also at: >>>    github.com/terminus/edk2/ hot-unplug-v4 >>> >>> Changelog: >>> v4: >>>    - Gets rid of unnecessary UefiCpuPkg changes >>> >>> v3: >>>    - Use a saner PCD based interface to share state between >>> PiSmmCpuDxeSmm >>>      and OvmfPkg/CpuHotplugSmm >>>    - Cleaner split of the hot-unplug code >>>    URL: >>> https://patchew.org/EDK2/20210115074533.277448-1-ankur.a.arora@oracle.com/ >>> >>> >>> v2: >>>    - Do the ejection via SmmCpuFeaturesRendezvousExit() >>>    URL: >>> https://patchew.org/EDK2/20210107195515.106158-1-ankur.a.arora@oracle.com/ >>> >>> >>> RFC: >>>    URL: >>> https://patchew.org/EDK2/20201208053432.2690694-1-ankur.a.arora@oracle.com/ >>> >>> >>> >>> Please review. >> >> I've got this series in my review queue (confirming). >> >> I'd like to finish review on the >> series first, >> since that's what I've got mostly in my mind at this point. >> >> I hope to start reviewing the unplug series in a few days. > > Sounds good. Thanks. I apologize for coming back with more formalities. The patches are somewhat incorrectly composed. Looking at the very first patch email for example, the quoted-printable encoding of the email shows: - a hard \r in each context line of the patch (encoded as "=0D"), - no \r character in any newly added line of the patch. This *inconsistency* is a problem -- once I apply the patch with git-am, it creates files with mixed LF / CRLF line terminators. Every source file (new files in particular) should be purely CRLF. Please update your editor's settings to stick with the line terminator style of the file that's being edited. (When creating new files, you may have to switch to CRLF explicitly.) Furthermore, please convert all of the patches to purely CRLF as follows: - run a git-rebase with "edit" actions - at each stage, determine the set of files updated/created by the commit - run "unix2dos" on all those files - squash the result into the commit - continue (You could script this too, using the "exec" git-rebase action, but doing it manually could be tolerable as well.) Now, of course I can do this myself with the patches, on my end, but (assuming at least one more version of the patch set is going to be necessary), fixing your settings and the patches both, for future manipulation, would now be timely. ... For reviewing non-trivial patch series, I tend to apply them first on a local branch (nothing beats the power of the full git toolkit during a review); that's how I found this. For catching such issues early on, please run "PatchCheck.py" before posting, e.g. as in: $ python3 BaseTools/Scripts/PatchCheck.py master..topic_branch (In the present case, PatchCheck reports lots of "not CRLF" errors.) An even better idea is to push your topic branch (which you're about to post to the list) to your edk2 fork on github.com, and then submit a pull request against the "tianocore/edk2" project. The pull request will be auto-closed in the end (it will never be merged), but the goal is to trigger a CI run on the patch set, and to give you the error messages if any. PatchCheck runs as a part of CI, too. (github.com PRs are used for actual merging by edk2 maintainers, but for that, they set the "push" label on the subject PRs, and the "push" label is restricted to maintainers.) I apologize about the extra delay. Would you be OK posting v5? Thanks! Laszlo