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.

Thanks
Ankur

Ankur Arora (9):
  OvmfPkg/CpuHotplugSmm: refactor hotplug logic
  OvmfPkg/CpuHotplugSmm: collect hot-unplug events
  OvmfPkg/CpuHotplugSmm: add Qemu Cpu Status helper
  OvmfPkg/CpuHotplugSmm: introduce UnplugCpus()
  OvmfPkg/CpuHotplugSmm: define CPU_HOT_EJECT_DATA
  OvmfPkg/SmmCpuFeaturesLib: init CPU ejection state
  OvmfPkg/CpuHotplugSmm: add CpuEject()
  OvmfPkg/CpuHotplugSmm: add worker to do CPU ejection
  OvmfPkg/SmmControl2Dxe: negotiate CPU hot-unplug

 OvmfPkg/CpuHotplugSmm/CpuHotplug.c                 | 435 ++++++++++++++++-----
 OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf            |   1 +
 OvmfPkg/CpuHotplugSmm/QemuCpuhp.c                  |  58 ++-
 OvmfPkg/CpuHotplugSmm/QemuCpuhp.h                  |   6 +
 OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h  |   2 +
 OvmfPkg/Include/Library/CpuHotEjectData.h          |  31 ++
 .../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c  |  62 +++
 .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf        |   3 +
 OvmfPkg/OvmfPkg.dec                                |   6 +
 OvmfPkg/SmmControl2Dxe/SmiFeatures.c               |  25 +-
 10 files changed, 527 insertions(+), 102 deletions(-)
 create mode 100644 OvmfPkg/Include/Library/CpuHotEjectData.h

-- 
2.9.3



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#70478): https://edk2.groups.io/g/devel/message/70478
Mute This Topic: https://groups.io/mt/79917574/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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.

Thanks
Laszlo

> 
> Thanks
> Ankur
> 
> Ankur Arora (9):
>   OvmfPkg/CpuHotplugSmm: refactor hotplug logic
>   OvmfPkg/CpuHotplugSmm: collect hot-unplug events
>   OvmfPkg/CpuHotplugSmm: add Qemu Cpu Status helper
>   OvmfPkg/CpuHotplugSmm: introduce UnplugCpus()
>   OvmfPkg/CpuHotplugSmm: define CPU_HOT_EJECT_DATA
>   OvmfPkg/SmmCpuFeaturesLib: init CPU ejection state
>   OvmfPkg/CpuHotplugSmm: add CpuEject()
>   OvmfPkg/CpuHotplugSmm: add worker to do CPU ejection
>   OvmfPkg/SmmControl2Dxe: negotiate CPU hot-unplug
> 
>  OvmfPkg/CpuHotplugSmm/CpuHotplug.c                 | 435 ++++++++++++++++-----
>  OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf            |   1 +
>  OvmfPkg/CpuHotplugSmm/QemuCpuhp.c                  |  58 ++-
>  OvmfPkg/CpuHotplugSmm/QemuCpuhp.h                  |   6 +
>  OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h  |   2 +
>  OvmfPkg/Include/Library/CpuHotEjectData.h          |  31 ++
>  .../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c  |  62 +++
>  .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf        |   3 +
>  OvmfPkg/OvmfPkg.dec                                |   6 +
>  OvmfPkg/SmmControl2Dxe/SmiFeatures.c               |  25 +-
>  10 files changed, 527 insertions(+), 102 deletions(-)
>  create mode 100644 OvmfPkg/Include/Library/CpuHotEjectData.h
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#70506): https://edk2.groups.io/g/devel/message/70506
Mute This Topic: https://groups.io/mt/79917574/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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.

Ankur

> 
> Thanks
> Laszlo
> 
>>
>> Thanks
>> Ankur
>>
>> Ankur Arora (9):
>>    OvmfPkg/CpuHotplugSmm: refactor hotplug logic
>>    OvmfPkg/CpuHotplugSmm: collect hot-unplug events
>>    OvmfPkg/CpuHotplugSmm: add Qemu Cpu Status helper
>>    OvmfPkg/CpuHotplugSmm: introduce UnplugCpus()
>>    OvmfPkg/CpuHotplugSmm: define CPU_HOT_EJECT_DATA
>>    OvmfPkg/SmmCpuFeaturesLib: init CPU ejection state
>>    OvmfPkg/CpuHotplugSmm: add CpuEject()
>>    OvmfPkg/CpuHotplugSmm: add worker to do CPU ejection
>>    OvmfPkg/SmmControl2Dxe: negotiate CPU hot-unplug
>>
>>   OvmfPkg/CpuHotplugSmm/CpuHotplug.c                 | 435 ++++++++++++++++-----
>>   OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf            |   1 +
>>   OvmfPkg/CpuHotplugSmm/QemuCpuhp.c                  |  58 ++-
>>   OvmfPkg/CpuHotplugSmm/QemuCpuhp.h                  |   6 +
>>   OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h  |   2 +
>>   OvmfPkg/Include/Library/CpuHotEjectData.h          |  31 ++
>>   .../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c  |  62 +++
>>   .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf        |   3 +
>>   OvmfPkg/OvmfPkg.dec                                |   6 +
>>   OvmfPkg/SmmControl2Dxe/SmiFeatures.c               |  25 +-
>>   10 files changed, 527 insertions(+), 102 deletions(-)
>>   create mode 100644 OvmfPkg/Include/Library/CpuHotEjectData.h
>>
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#70508): https://edk2.groups.io/g/devel/message/70508
Mute This Topic: https://groups.io/mt/79917574/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#70645): https://edk2.groups.io/g/devel/message/70645
Mute This Topic: https://groups.io/mt/79917574/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 2021-01-21 4:29 a.m., Laszlo Ersek wrote:
> 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.)
> 

PatchCheck.py is great. Thanks.

> 
> 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?

Sure. Just a side question which I should have asked you earlier: are
the coding standards listed somewhere?
I had looked for answers to similar questions but the "TianoCore C style
guide" doesn't mention either PatchCheck.py or rules around CRLF.

(Though now that I do look at it, the CRLF rules are listed in the
"EDK II C Coding Standards_2.1 Draft.PDF".)

Thanks for the comments and my apologies for making you review all of
these non-substantive changes.


Ankur

> 
> Thanks!
> Laszlo
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#70654): https://edk2.groups.io/g/devel/message/70654
Mute This Topic: https://groups.io/mt/79917574/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 01/21/21 20:11, Ankur Arora wrote:
> On 2021-01-21 4:29 a.m., Laszlo Ersek wrote:

>> 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.)
>>
> 
> PatchCheck.py is great. Thanks.
> 
>>
>> 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?
> 
> Sure.

Thanks for your understanding, I'm relieved.

> Just a side question which I should have asked you earlier: are
> the coding standards listed somewhere?

Yes, see "C Coding Standards" at
<https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Documentation#specifications>.

Although...

> I had looked for answers to similar questions but the "TianoCore C style
> guide" doesn't mention either PatchCheck.py or rules around CRLF.

... you may have found exactly that, already. Unfortunately, like all
documentation in the world, this document is somewhat out of date.

There is also a tool, called ECC, that contains a builtin C language
parser, that enforces various style requirements. It's built into the CI
system and runs on github -- however, we have it disabled for OVMF, in
commit ef3e73c6a0c0 ("OvmfPkg: Disable EccCheck CI until EccCheck issues
are fixed", 2020-12-14). There are two reasons for that:

- Some stuff that ECC enforces is just overzealous -- in some cases, we
relax the coding style under OvmfPkg specifically, but ECC cannot easily
be configured to tolerate those relaxations (for example, you cannot
configure it to ignore a particular error code *regardless* of where it
occurs under OvmfPkg).

- In some cases, ECC rejects C language constructs that are both valid
and conformant to the coding style (i.e., ECC has its own bugs).


So we usually "teach" these quirks to contributors during review. It's
quite a lot of investment, which is part of why we hope that
contributors stick around.


An alternative is this: in your topic branch that you intend to post to
edk2-devel, include an *extra* patch (not to be posted to the list) that
*reverts* commit ef3e73c6a0c0. And submit a github.com PR with this
variant of your topic branch. As a result, ECC will be unleashed on (the
end-stage of) your patch series. If you fix all of the ECC issues, then
reviewers should make next to zero *style* complaints -- on the other
hand, you could be updating code that ECC complains about *incorrectly*
(due to an ECC bug, or because the subject style rule is something we
deem superfluous, or at least "possible to bend", under OvmfPkg).

So perhaps try to investigate this latter avenue, and see if you are
willing to deal with what ECC throws at you. :)

> Thanks for the comments and my apologies for making you review all of
> these non-substantive changes.

It's a standard part of reviewing the first few contributions of a new
community member; it's too bad we don't have better stuff in place. We
do have docs but they are slightly outdated, and we do have ECC but with
warts.

We also have recommendations (not official requirements!) on setting up
git, as a part of
<https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers>
-- which is likely out of date somewhat --; the

  BaseTools/Scripts/SetupGit.py

utility automates more or less the same settings.

We also have

  BaseTools/Scripts/GetMaintainer.py

which lets you determine the Cc:'s you should put in each commit message...

And when you dump all this *process* on a new contributor, they run away
screaming -- kind of justifiedly. :/

Thanks
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#70655): https://edk2.groups.io/g/devel/message/70655
Mute This Topic: https://groups.io/mt/79917574/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 2021-01-21 11:51 a.m., Laszlo Ersek wrote:
> On 01/21/21 20:11, Ankur Arora wrote:
>> On 2021-01-21 4:29 a.m., Laszlo Ersek wrote:
> 
>>> 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.)
>>>
>>
>> PatchCheck.py is great. Thanks.
>>
>>>
>>> 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?
>>
>> Sure.
> 
> Thanks for your understanding, I'm relieved.
> 
>> Just a side question which I should have asked you earlier: are
>> the coding standards listed somewhere?
> 
> Yes, see "C Coding Standards" at
> <https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Documentation#specifications>.
> 
> Although...
> 
>> I had looked for answers to similar questions but the "TianoCore C style
>> guide" doesn't mention either PatchCheck.py or rules around CRLF.
> 
> ... you may have found exactly that, already. Unfortunately, like all
> documentation in the world, this document is somewhat out of date.
> 
> There is also a tool, called ECC, that contains a builtin C language
> parser, that enforces various style requirements. It's built into the CI
> system and runs on github -- however, we have it disabled for OVMF, in
> commit ef3e73c6a0c0 ("OvmfPkg: Disable EccCheck CI until EccCheck issues
> are fixed", 2020-12-14). There are two reasons for that:
> 
> - Some stuff that ECC enforces is just overzealous -- in some cases, we
> relax the coding style under OvmfPkg specifically, but ECC cannot easily
> be configured to tolerate those relaxations (for example, you cannot
> configure it to ignore a particular error code *regardless* of where it
> occurs under OvmfPkg).
> 
> - In some cases, ECC rejects C language constructs that are both valid
> and conformant to the coding style (i.e., ECC has its own bugs).
> 
> 
> So we usually "teach" these quirks to contributors during review. It's
> quite a lot of investment, which is part of why we hope that
> contributors stick around.

A very understandable sentiment :).

> 
> An alternative is this: in your topic branch that you intend to post to
> edk2-devel, include an *extra* patch (not to be posted to the list) that
> *reverts* commit ef3e73c6a0c0. And submit a github.com PR with this
> variant of your topic branch. As a result, ECC will be unleashed on (the
> end-stage of) your patch series. If you fix all of the ECC issues, then
> reviewers should make next to zero *style* complaints -- on the other
> hand, you could be updating code that ECC complains about *incorrectly*
> (due to an ECC bug, or because the subject style rule is something we
> deem superfluous, or at least "possible to bend", under OvmfPkg).
> 
> So perhaps try to investigate this latter avenue, and see if you are
> willing to deal with what ECC throws at you. :)

Can't guarantee that I'll see it through but my curiosity on what ECC
will throw at me is piqued. Will try.

> 
>> Thanks for the comments and my apologies for making you review all of
>> these non-substantive changes.
> 
> It's a standard part of reviewing the first few contributions of a new
> community member; it's too bad we don't have better stuff in place. We
> do have docs but they are slightly outdated, and we do have ECC but with
> warts.
> 
> We also have recommendations (not official requirements!) on setting up
> git, as a part of
> <https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers>
> -- which is likely out of date somewhat --; the

I guess that's the nature of non-executable bits... out of date
soon as they are written. Thanks, this and the scripts below both
were pretty useful.

>    BaseTools/Scripts/SetupGit.py
> 
> utility automates more or less the same settings.
> 
> We also have
> 
>    BaseTools/Scripts/GetMaintainer.py
> 
> which lets you determine the Cc:'s you should put in each commit message...
> 
> And when you dump all this *process* on a new contributor, they run away
> screaming -- kind of justifiedly. :/

Heh, I think I see the appeal of this gradual introduction of process.

Personally speaking, it is after working on this feature and seeing
things  like SafeUintnMult(), that you see that FW generally and EDK2
specifically really does need to be written differently from, for
instance the kernel.

Thanks
Ankur
  
> 
> Thanks
> Laszlo
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#70671): https://edk2.groups.io/g/devel/message/70671
Mute This Topic: https://groups.io/mt/79917574/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 01/22/21 07:32, Ankur Arora wrote:
> On 2021-01-21 11:51 a.m., Laszlo Ersek wrote:

>> So perhaps try to investigate this latter avenue, and see if you are
>> willing to deal with what ECC throws at you. :)
> 
> Can't guarantee that I'll see it through but my curiosity on what ECC
> will throw at me is piqued. Will try.

Thanks!
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#70676): https://edk2.groups.io/g/devel/message/70676
Mute This Topic: https://groups.io/mt/79917574/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-