Re: [edk2-devel] [PATCH v4 0/9] support CPU hot-unplug

["Laszlo Ersek" <lersek@redhat.com>]

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
>> <https://bugzilla.tianocore.org/show_bug.cgi?id=3059> 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