Date   

[RFC] BaseTools/Source/Python as a standalone python package in independent repo

Matthew Carlson
 

Hello Tianocore Community,

I’m submitting an RFC, proposing the movement of the basetools folder in edk2 to a separate repo and treated as a separate python project.

We talked about it during the April 16th design meeting and on devel (https://edk2.groups.io/g/devel/topic/73069134#58048), feel free to look over the discussion.

Here’s a basic overview of the what and the why behind this proposal:

Why a separate repo?
The recent efforts in expanding the role of CI in the platform and core code of EDK2 will pay big dividends in the future, leading to higher quality code and easier integrations for everyone. Having basetools as it’s own repo would simplify adding a similar CI/linting process and unit-tests to the basetools python code, leading to higher quality code.

A second major benefit is it would allow others that write tools for UEFI and Edk2 to leverage this vast resource of python code using standard python package inclusion. It would allow those tools to be decoupled from edk2 source and provide a consistent and managed user experience. The python project would be published as a Pip module for those that want to leverage the basetools modules the same way they leverage the existing python ecosystem. Packing basetools as a pip module, would reach the most developers and provide the most flexibility and versatility. There are numerous way this could be used; Pip is just one method suggested here. Other ways to leverage this are described below.

Why a pip module?
The investment into basetools is sizable and it has some amazing functionality that’s difficult to reproduce. For example, the DSC, FDF, INF, and DEC parsers handle an incredible amount of edge cases. If I wanted to write a tool that could do a CI validation check or build a UEFI capsule, currently I would need to clone all of EDK2 to get basetools. If it was in a separate repo and available as a wheel, as a developer, I could include it via pip and have that dependency managed with virtual environment or just in the global cache. In addition, other tools that currently are difficult to build would become possible with access to the Basetools functionality.

However, there have been some concerns expressed about having a global basetools and the impact this has on developers with multiple workspaces (potentially at different versions). There are several tools and strategies to consider for managing this dependency. Some outlined below.

How will this change your workflow?
If this moved there would have to be a change for all platforms using edk2 and we have been evaluating options. These could become requirements a developer must do before building edk2 or with minimal effort could be added to the edksetup or build scripts. These can also be more easily isolated if python virtual environments are used.

For those just consuming released versions of basetools python code:
Option A: leverage Python package management and install a released version from Pypi into a per project virtual environment.
Option B: leverage pip to install from the git repo at known tag/release (pip_requirements)

For those wanting to do active development of basetools within an edk2 project:
Option C: Clone the python package source and install the package locally (pip install -e ./). All changes made in the local package are reflected into your python site packages.

Option D: An offline server that builds edk2. You would need to download the wheel, tar, or git repo before hand. Pip supports installing from a variety of sources: https://packaging.python.org/tutorials/installing-packages/ such as a tar.gz, a local folder, or a wheel.

PIP vs Submodules:
The issue with submodule and not using python packages (pip is really just a super convenient helper to install that consistently) is that it requires the paradigm of file system python path management. Think of this as edk1 vs edk2 public includes (if you remember back that far), basically you just figure out the path from yourself to the file you want and add that. It gives you no ability to apply consistent ways to access modules and in the end means you just have a folder with python scripts/modules in it. It causes major pain for downstream consumers when/if there is ever refactoring (and we know there needs to be refactoring). It leads to others not being able to extend or leverage these modules without introducing significant fragility.

What Pip does is gives you a way to consistently install and access the python modules. You can pip install from a local copy of the git repo or you can pip install a "release" using something like pypi. Either way it doesn't change how consuming code accesses the python modules. This is the value of pip in this case.

If there is a strong desire from the developer community to have a workflow that avoids pip I believe a little documentation could handle that. Pip is just a helper. Python packages can be built from the source and installed. I see no reason to recommend this as it requires numerous more commands but if avoiding pip is your goal, it is possible.

More value for python package management (PIP):
As we start looking at refactoring and potentially moving tools from C to python we can take advantage of package management to lower our maintenance burden. For example Brotli has a pypi release and thus we wouldn't need to carry that code. https://pypi.org/project/Brotli/

Versioning and Dependencies:
To minimize the dependency challenges and "bisectability" I would suggest we leverage the versioning capabilities within pip and repo tagging. With versioning you have lots of options as you can lock to a specific version which requires an update each time or you can use some sort of floating version within the tuple of version (xx.yy.zz). These two tools can make this pretty flexible.

In a scenario of DEC or INF syntax change, the suggested workflow would be:
1. Create the issue for basetools
2. Update basetools python
3. Write the unit test that shows it works as expected
4. Check in and make a release
5. Update edk2 pip-requirements.txt to require at least this new version. This gives you the tracking necessary to align with the tools.
6. Use this new feature in the edk2 fw code.

In the scenario of a change in BaseTools that causes a break for a downstream project (like EDK2 or a closed source platform), it is easy to simply checkout the previous pip_requirements and pip install the previous release or to install locally (using option C), checking out the commit in Basetools and inspecting the git history to debug the issue. It’s easy for a developer to modify source locally and test against local code. This is covered by workflow option C. It is very easy to manage and is one of the reasons we are proposing this change.

Demo:
We have a demo of what this would look like: https://github.com/matthewfcarlson/edk2-pytool-base/
And the EDK2 that leverages it https://github.com/matthewfcarlson/edk2/tree/feature/pip-basetools

Contribution/Dev Process:
Since this is a separate repo, it will follow a slightly different contribution and code review process.
1. Github PR process will be used for contributions and code review feedback
a. The yet to be released “Tianocore PR archiver” will be used to send to a dedicated list for basetools patch review archive
2. PRs will only be committed if they keep linear history (no merge commits)
3. The PR review must be approved by at least 2 members of the basetools team (not including the author)
4. The PR must pass all automated checks
a. Formatting/style
b. Unit tests
c. Code coverage (can’t commit change that would decrease overall %)
d. DCO enforcement - https://probot.github.io/apps/dco/
e. See other python requirements from the Python coding standard
5. Github Issues will be used for non-security sensitive bugs/issues/feature requests

Releases:
1. Version will follow Semantic Versioning (https://semver.org/) xx.yy.zz
a. X is major version and will update at incompatible change for Core basetools API
b. Y is minor version. Will be updated when new functionality is added in compatible manner
c. Z is patch version. Updated with each change
d. Target 1.0.0 for 2020 Q3 stable tag timeframe
2. Git tags will be created for each version
3. Github “release” will be created for each version
4. Git branches will not be created proactively. If servicing is determined to be needed then a branch can be created from the tag.
5. Pypi release will be made for each version
6. Github milestones will be used for tracking content in every minor version
What happens next?
Right now, we’re gathering feedback and seeing if anyone has an concerns or platforms that this would not work for. We’d love to hear what you have to say. Baring any serious concerns, we’d move forward with:
1. Create new GitHub repo on tianocore for the basetools project
2. Develop the testing, PR, and release process
3. Release the initial version to pypi
4. Delete the source folder in edk2 repo and replace with readme and method to get pip version installed (this patch will be post Q2 stable tag to give developers time to adjust).
5. Continually improve basetools and add more testing
This RFC will close May 11th-12th, please respond with comments and questions before that date.

What’s the long-term plan?
The current tentative long term plan is to merge some or all of basetools in with the existing edk2-pytool-library repo. This would likely involve the conversion of C based basetools to python based ones. This is still an active conversation, and we’d like to hear your thoughts.


Matthew Carlson
Core UEFI
Microsoft


Re: [RFCv2] code-first process for UEFI-forum specifications

Samer El-Haj-Mahmoud
 

Acked-by: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@...>

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Leif Lindholm
via Groups.Io
Sent: Monday, March 23, 2020 3:06 PM
To: devel@edk2.groups.io; rfc@edk2.groups.io
Cc: Felixp@...; Mark Doran <mark.doran@...>; Andrew Fish
<afish@...>; Laszlo Ersek <lersek@...>; Michael D Kinney
<michael.d.kinney@...>
Subject: [edk2-rfc] [RFCv2] code-first process for UEFI-forum specifications

Changes to v2 of this proposal:
- Feedback from Laszlo[a] and Mike[b] incorporated.
- I opted to view Mike's responses to Laszlo's questions as
accepted, as no follow-up was made.

Feedback from Felix[c] *not* incorporated, as while I agree with all of it, I am
not convinced that information should go here, but should instead reside with
the UEFI Forum. (This text documents the public part of the process - it would
cause me slight impedance mismatch to have it also document the non-public
part.)

[a] https://edk2.groups.io/g/devel/message/53422
[b] https://edk2.groups.io/g/devel/message/53738
[c] https://edk2.groups.io/g/devel/message/54453

/
Leif

---

This is a proposal for a process by which new features can be added to UEFI
forum specifications after first having been designed and prototyped in the
open.

This process lets changes and the development of new features happen in the
open, without violating the UEFI forum bylaws which prevent publication of
code for in-draft features/changes.

The process does not in fact change the UEFI bylaws - the change is that the
development (of both specification and code) happens in the open. The
resulting specification update is then submitted to the appropriate working
goup as an Engineering Change Request (ECR), and voted on. For the UEFI
Forum, this is a change in workflow, not a change in process.

ECRs are tracked in a UEFI Forum Mantis instance, access restricted to UEFI
Forum Members. TianoCore will enable this new process by providing areas
on https://bugzilla.tianocore.org/ to track both specification updates and
reference implementations and new repositories under
https://github.com/tianocore/ dedicated to hold "code first".


## Bugzilla

bugzilla.tianocore.org will have a product category each for
* ACPI Specification
* PI Specification
* UEFI Specification

Each product category will have a separate components for
* Specification
* Reference implementation


## Github
New repositories will be added for holding the text changes and the source
code.

Specification text changes will be held within the affected source repository,
in the Github flavour of markdown, in a file (or split across several files) with
.md suffix.
(This one may break down where we have a specification change affecting
multiple specifications, but at that point we can track it with multiple BZ
entries)

Reference implementations targeting EDK2 will be held in branches on edk2-
staging.
Additional repositories for implementing reference features in additional
open source projects can be added in the future, as required.


## Intended workflow
The entity initiating a specifiation update raises a Bugzilla in the appropriate
area in bugzilla.tianocore.org. This entry contains the outline of the change,
and the full initial draft text is attached.

If multiple specification updates are interdependent, especially if between
different specifications, then multiple bugzilla entries should be created.
These bugzilla entries *must* be linked together with dependencies.

After the BZs have been created, new branches should be created in the
relevant repositories for each bugzilla - the branch names should be BZ####,
where #### describes the bugzilla ID assigned, optionally followed by a '-' and
something more descriptive. If multiple bugzilla entries must coexist on a
single branch, one of them is designated the 'top-level', with dependencies
properly tracked.
That BZ will be the one naming the branch.


## Source code
In order to ensure draft code does not accidentally leak into production use,
and to signify when the changeover from draft to final happens, *all* new or
modified[1] identifiers need to be prefixed with the relevant BZ####.

[1] Modified in a non-backwards-compatible way. If, for example, a statically
sized array is grown - this does not need to be prefixed. But a tag in a
comment would be *highly* recommended.

### File names
New public header files need the prefix. I.e. `Bz1234MyNewProtocol.h`
Private header files do not need the prefix.

### Contents

The tagging must follow the coding style used by each affected codebase.
Examples:

| Released in spec | Draft version in tree | Comment |
| --- | --- | --- |
| `FunctionName` | `Bz1234FunctionName` | |
| `HEADER_MACRO` | `BZ1234_HEADER_MACRO` | |

For data structures or enums, any new or non-backwards-compatible structs
or fields require a prefix. As above, growing an existing array in an existing
struct requires no prefix.

| `typedef SOME_STRUCT` | `BZ1234_SOME_STRUCT` | Typedef only [2]
|
| `StructField` | `Bz1234StructField` | In existing struct[3] |
| `typedef SOME_ENUM` | `BZ1234_SOME_ENUM` | Typedef only [2]
|

[2] If the struct or enum definition is separate from the typedef in the public
header, the definition does not need the prefix.
[3] Individual fields in newly added typedefd struct do not need prefix, the
struct already carried the prefix.

Variable prefixes indicating global scope ('g' or 'm') go before the BZ prefix.

| `gSomeGuid` | `gBz1234SomeGuid` | |

Local identifiers, including module-global ones (m-prefixed) do not require a
BZ prefix.

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.


Re: [RFC] fix for virt-install failure with OVMF?

Laszlo Ersek
 

Hi Aaron.

(I'm responding through the groups.io webui because it's way too late for a full email fetch now. So the threading & quoting will most likely be broken. Sorry about that.)

The "--initrd-inject" and "--extra-args" options are very relevant in this case. (And yes they do justify using --location.) The idea is that "--initrd-inject" modifies the temporary copy of the initrd that was copied out of the ISO (which was mounted as a local directory containing a distro install tree). "--initrd-inject" then adds the "guest-31156337-ks.cfg" file to the root of the initrd.

The "--extra-args" option goes with it. It creates an "-append" option on the QEMU command line. It tells the kernel [*] to look for a kickstart file by the name of "guest-31156337-ks.cfg" in the initial ramdisk.

[*] More precisely, the guest kernel doesn't care about "ks=..."; it's the user-space installer program (Anaconda), launched from the initrd by the kernel, that consumes "ks=..." from the kernel command line.

So these virt-install options are actually crucial (and so are the contents of the kickstart file), because they tell the guest installer what to do, after OVMF launches the guest kernel.

What devices OVMF connects in the BDS phase (that is, before ExitBootServices()), should be irrelevant wrt. how Anaconda locates a kickstart file in the initial ramdisk, and how Anaconda processes the looked-up kickstart file.

So with the new information available, I agree that "--location" is needed, because you, indeed, do *not* want a "UEFI CD-ROM boot". Instead you want a direct kernel boot, with a kickstart file injected into the initrd from the host side. However, I think OVMF's current code still does the right thing in that case, because the UEFI drivers that may or may not have connected e.g. a SATA device in the BDS phase should be totally irrelevant after ExitBootServices(). And Anaconda runs (and consumes the kickstart file) way after ExitBootServices(). The set of devices that were connected under UEFI could play a role for the kernel's UEFI stub, yes, but I don't think the kernel's UEFI stub plays any role in the installation you're trying to perform.

I'd suggest looking into the kernel log, the systemd log, and the installer logs, produced in the guest. From the log you pasted before ("Entering emergency mode. Exit the shell to continue."), it seems like the initrd is broken, and Anaconda cannot be started. This kind of error message is usually printed when the root filesystem is busted, and the kernel cannot "switch root" from the initrd to the on-disk root filesystem.

Regarding why it seems to work when you reorder PlatformBdsConnectSequence() vs. TryRunningQemuKernel() -- I have no idea. In that case, is your kickstart file really correctly processed by Anaconda?

I could imagine a (virtual) hardware problem -- assuming the firmware SATA driver doesn't ever initialize the hardware, the kernel driver could fail to access the disk, or some such... But that would require either QEMU's device model or the kernel's driver to be horribly broken. Do you see identical behavior if you use virtio-scsi?

Thanks
Laszlo


Re: [RFC] fix for virt-install failure with OVMF?

Aaron Young
 

Hi Lazslo, thanks for the comments.

My apologies - the actual virt-install command our Q/A team is using is this (which has extra args for a kick start file):

virt-install --name guest-31156337 --memory 4096 --vcpus 4 --disk path=/guest-31156337/guest-31156337-kvm.img,size=10,device=disk,bus=virtio,sparse=yes --location /ISO/OracleLinux-R7-U7-Server-x86_64-dvd.iso --nographics --initrd-inject=/guest-31156337/log/guest-31156337-ks.cfg --network bridge=virbr0,model=virtio --os-type linux --os-variant ol7.7 --noreboot --boot=hd,cdrom,loader=//usr/share/OVMF/OVMF_CODE.pure-efi-guest-31156337.fd,loader_ro=yes,loader_type=pflash,loader_secure=no,nvram=/usr/share/OVMF/OVMF_VARS.pure-efi-guest-31156337.fd --extra-args="ks=file:/guest-31156337-ks.cfg ip=dhcp console=tty0 console=ttyS0,115200n8

I had changed the args (below) in an effort to simplify things (i.e. I had removed the --extra-args and --initrd-inject args for the kick start file which I had thought were not germane to the issue).

Per the virt-install docs (which you site below) and the other examples on-line to use virt-install with a kickstart (i.e. https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/7/html/virtualization_deployment_and_administration_guide/sect-guest_virtual_machine_installation_overview-creating_guests_with_virt_install), the --location arg seems to be necessary to use a kickstart file (?).

Do you have any further comments/suggestions in light of needing to use a kickstart file and thus the --location arg?

Thanks again in advance,

-Aaron

On 04/22/2020 11:33 AM, Laszlo Ersek wrote:
On 04/22/20 01:12, aaron.young@... wrote:
Hello, we have found what we think is a BUG in OVMF, but wanted to run
it by the rfc list first to confirm.

We have discovered that the following virt-install command causes the
latest OVMF code to fail to boot into the installer ISO.

virt-install --boot uefi --name Guest1 --ram 4096 --vcpus 1 --disk
path=/Disks/Guest1_disk.img --location
/ISO/OracleLinux-R7-U4-Server-x86_64-dvd.iso --network
bridge=virbr0,model=virtio --os-type linux --noreboot
--boot=hd,cdrom,loader=/usr/share/OVMF/OVMF_CODE.pure-efi.fd,loader_ro=yes,loader_type=pflash,loader_secure=no,nvram=/Images/OVMF_VARS.pure-efi.Guest1.fd
--video vga --graphics vnc,port=5999

Instead of booting to the installer ISO, we drop into the kernel shell
like so:
This is by-design. The behavior is expected, and your virt-install
command line is wrong.

The "--location" parameter invokes a direct kernel boot. When you pass a
local ISO image with "--location", that makes no difference in this
regard; it's still a direct kernel boot. The virt-install documentation
explains:

"""
-l LOCATION
--location OPTIONS
Distribution tree installation source. virt-install can
recognize certain distribution trees and fetches a bootable
kernel/initrd pair to launch the install.
[...]
--location allows things like --extra-args for kernel
arguments, and using --initrd-inject. If you want to use
those options with CDROM media, you have a few options:

* Run virt-install as root and do --location ISO

* Mount the ISO at a local directory, and do --location
DIRECTORY
[...]
DIRECTORY
Path to a local directory containing an installable
distribution image. Note that the directory will not be
accessible by the guest after initial boot, so the OS
installer will need another way to access the rest of
the install media.

ISO Mount the ISO and probe the directory. This requires
running virt-install as root, and has the same VM
access caveat as DIRECTORY.
"""

In other words, with your above command line, you are not booting the
ElTorito UEFI boot image that's embedded in the ISO -- you are not
performing a "UEFI CD-ROM boot". Instead, virt-install mounts the ISO
image, locates the kernel (vmlinuz) and initrd files in the directory
tree, and launches a guest with direct (that is, fw_cfg) kernel boot.

[snip]

For completeness, here is the qemu command that is exec'd by the
virt-install command for reference:
Right, please see the "-kernel" and "-initrd" options on the cmdline:

/usr/bin/qemu-system-x86_64 [...] -kernel
/var/lib/libvirt/boot/virtinst-vmlinuz.jdC2ks -initrd
/var/lib/libvirt/boot/virtinst-initrd.img.aHyTeQ [...]
Those are temporary kernel and initrd files; extracted from your ISO.
The way OVMF reacts to these QEMU options is expected.

Remove the "--location" option from your virt-install command line.
Instead, use

--disk path=/ISO/OracleLinux-R7-U4-Server-x86_64-dvd.iso,device=cdrom

Further comments I have:

* "--boot uefi" is not needed if you spell out "--boot
loader=/.../OVMF_CODE.fd,loader_ro=yes,loader_type=pflash,nvram_template=/.../OVMF_VARS.fd".

"--boot uefi" is a shorthand for the latter, relying on libvirtd and
its configuration to locate a firmware executable (and a compatible
variable store template) for you.

* "--boot loader_secure=no" is the default, so no need to spell it out.

* Not sure what "--noreboot" is useful for, I never use it.

* Setting the boot order with "--boot=hd,cdrom" is legacy BIOS style;
it's best not to use it with UEFI. Instead, use the "boot_order"
property with the individual "--disk" options.

* "--vcpus 1" is also the default, no need to spell it out.

* The "nvram=..." property for "--boot" does not seem very useful. It
means that you want to reuse a pre-existent variable store file with
the newly installed domain. This is useful only in exceptional cases;
normally you want libvirtd to create the new domain's variable store
from the variable store template given with "nvram_template".

So ultimately I would try:

virt-install \
--name Guest1 \
--ram 4096 \
--disk path=/Disks/Guest1_disk.img,size=10,format=qcow2,boot_order=1 \
--disk path=/ISO/OracleLinux-R7-U4-Server-x86_64-dvd.iso,device=cdrom,readonly,boot_order=2 \
--network bridge=virbr0,model=virtio \
--os-type linux \
--boot loader=/usr/share/OVMF/OVMF_CODE.pure-efi.fd,loader_ro=yes,loader_type=pflash,nvram_template=/usr/share/OVMF/OVMF_VARS.pure-efi.fd \
--video vga \
--graphics vnc,port=5999

Hope this helps,
Laszlo



Re: [RFC] fix for virt-install failure with OVMF?

Laszlo Ersek
 

On 04/22/20 01:12, aaron.young@... wrote:

Hello, we have found what we think is a BUG in OVMF, but wanted to run
it by the rfc list first to confirm.

We have discovered that the following virt-install command causes the
latest OVMF code to fail to boot into the installer ISO.

virt-install --boot uefi --name Guest1 --ram 4096 --vcpus 1 --disk
path=/Disks/Guest1_disk.img --location
/ISO/OracleLinux-R7-U4-Server-x86_64-dvd.iso --network
bridge=virbr0,model=virtio --os-type linux --noreboot
--boot=hd,cdrom,loader=/usr/share/OVMF/OVMF_CODE.pure-efi.fd,loader_ro=yes,loader_type=pflash,loader_secure=no,nvram=/Images/OVMF_VARS.pure-efi.Guest1.fd
--video vga --graphics vnc,port=5999

Instead of booting to the installer ISO, we drop into the kernel shell
like so:
This is by-design. The behavior is expected, and your virt-install
command line is wrong.

The "--location" parameter invokes a direct kernel boot. When you pass a
local ISO image with "--location", that makes no difference in this
regard; it's still a direct kernel boot. The virt-install documentation
explains:

"""
-l LOCATION
--location OPTIONS
Distribution tree installation source. virt-install can
recognize certain distribution trees and fetches a bootable
kernel/initrd pair to launch the install.
[...]
--location allows things like --extra-args for kernel
arguments, and using --initrd-inject. If you want to use
those options with CDROM media, you have a few options:

* Run virt-install as root and do --location ISO

* Mount the ISO at a local directory, and do --location
DIRECTORY
[...]
DIRECTORY
Path to a local directory containing an installable
distribution image. Note that the directory will not be
accessible by the guest after initial boot, so the OS
installer will need another way to access the rest of
the install media.

ISO Mount the ISO and probe the directory. This requires
running virt-install as root, and has the same VM
access caveat as DIRECTORY.
"""

In other words, with your above command line, you are not booting the
ElTorito UEFI boot image that's embedded in the ISO -- you are not
performing a "UEFI CD-ROM boot". Instead, virt-install mounts the ISO
image, locates the kernel (vmlinuz) and initrd files in the directory
tree, and launches a guest with direct (that is, fw_cfg) kernel boot.

[snip]

For completeness, here is the qemu command that is exec'd by the
virt-install command for reference:
Right, please see the "-kernel" and "-initrd" options on the cmdline:

/usr/bin/qemu-system-x86_64 [...] -kernel
/var/lib/libvirt/boot/virtinst-vmlinuz.jdC2ks -initrd
/var/lib/libvirt/boot/virtinst-initrd.img.aHyTeQ [...]
Those are temporary kernel and initrd files; extracted from your ISO.
The way OVMF reacts to these QEMU options is expected.

Remove the "--location" option from your virt-install command line.
Instead, use

--disk path=/ISO/OracleLinux-R7-U4-Server-x86_64-dvd.iso,device=cdrom

Further comments I have:

* "--boot uefi" is not needed if you spell out "--boot
loader=/.../OVMF_CODE.fd,loader_ro=yes,loader_type=pflash,nvram_template=/.../OVMF_VARS.fd".

"--boot uefi" is a shorthand for the latter, relying on libvirtd and
its configuration to locate a firmware executable (and a compatible
variable store template) for you.

* "--boot loader_secure=no" is the default, so no need to spell it out.

* Not sure what "--noreboot" is useful for, I never use it.

* Setting the boot order with "--boot=hd,cdrom" is legacy BIOS style;
it's best not to use it with UEFI. Instead, use the "boot_order"
property with the individual "--disk" options.

* "--vcpus 1" is also the default, no need to spell it out.

* The "nvram=..." property for "--boot" does not seem very useful. It
means that you want to reuse a pre-existent variable store file with
the newly installed domain. This is useful only in exceptional cases;
normally you want libvirtd to create the new domain's variable store
from the variable store template given with "nvram_template".

So ultimately I would try:

virt-install \
--name Guest1 \
--ram 4096 \
--disk path=/Disks/Guest1_disk.img,size=10,format=qcow2,boot_order=1 \
--disk path=/ISO/OracleLinux-R7-U4-Server-x86_64-dvd.iso,device=cdrom,readonly,boot_order=2 \
--network bridge=virbr0,model=virtio \
--os-type linux \
--boot loader=/usr/share/OVMF/OVMF_CODE.pure-efi.fd,loader_ro=yes,loader_type=pflash,nvram_template=/usr/share/OVMF/OVMF_VARS.pure-efi.fd \
--video vga \
--graphics vnc,port=5999

Hope this helps,
Laszlo


[RFC] fix for virt-install failure with OVMF?

Aaron Young
 

Hello, we have found what we think is a BUG in OVMF, but wanted to run it by the rfc list first to confirm.

We have discovered that the following virt-install command causes the latest OVMF code to fail to boot into the installer ISO.

virt-install --boot uefi --name Guest1 --ram 4096 --vcpus 1 --disk
path=/Disks/Guest1_disk.img --location /ISO/OracleLinux-R7-U4-Server-x86_64-dvd.iso --network bridge=virbr0,model=virtio --os-type linux --noreboot --boot=hd,cdrom,loader=/usr/share/OVMF/OVMF_CODE.pure-efi.fd,loader_ro=yes,loader_type=pflash,loader_secure=no,nvram=/Images/OVMF_VARS.pure-efi.Guest1.fd --video vga --graphics vnc,port=5999

Instead of booting to the installer ISO, we drop into the kernel shell like so:
...

Entering emergency mode. Exit the shell to continue.
Type "journalctl" to view system logs.
You might want to save "/run/initramfs/rdsosreport.txt" to a USB stick
or /boot
after mounting them and attach it to a bug report.


:/# ls /sys/block
:/#

---

This change to the code allows this virt-install command (above) to succeed:

diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
index 45d0ee9..997acaf 100644
--- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
+++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
@@ -1514,14 +1514,14 @@ PlatformBootManagerAfterConsole (
Tcg2PhysicalPresenceLibProcessRequest (NULL);

//
- // Process QEMU's -kernel command line option
+ // Perform some platform specific connect sequence
//
- TryRunningQemuKernel ();
+ PlatformBdsConnectSequence ();

//
- // Perform some platform specific connect sequence
+ // Process QEMU's -kernel command line option
//
- PlatformBdsConnectSequence ();
+ TryRunningQemuKernel ();

EfiBootManagerRefreshAllBootOption ();

i.e. if we move the TryRunningQemuKernel() call to after PlatformBdsConnectSequence(), it works... i.e. the virt-install commands boots to the installer ISO and all is good.

What it appears to be happening is the CD/IDE device for the ISO is not being connected with the existing code. The code change allows the CD to be connected before we call TryRunningQemuKernel(). With the code change we see this in the debug log, but without the code change we do not:
< Found Mass Storage device: PciRoot(0x0)/Pci(0x1,0x1)
< SataControllerStart START
< InstallProtocolInterface: A1E37052-80D9-4E65-A317-3E9A55C43EC9 BEF07EA0
< SataControllerStart END status = Success
< ==AtaAtapiPassThru Start== Controller = BEFA3C98
< [secondary] channel [master] [cdrom ] device
< CalculateBestPioMode: AdvancedPioMode = 3
< IdeInitCalculateMode: PioMode = 3
< CalculateBestUdmaMode: DeviceUDmaMode = 203F
< IdeInitCalculateMode: UdmaMode = 5


My question is: Is this a legitimate change to the boot flow to OVMF? Is there a reason that TryRunningQemuKernel() is currently called prior to PlatformBdsConnectSequence()? (Other than just an optimization to reduce unnecessary connections)? I fear that this change in the boot flow fixes this case but may break something else.

For completeness, here is the qemu command that is exec'd by the virt-install command for reference:
/usr/bin/qemu-system-x86_64 -name guest=Guest1,debug-threads=on -S -object secret,id=masterKey0,format
=raw,file=/var/lib/libvirt/qemu/domain-5-Guest1/master-key.aes -machine pc-i440fx-3.1,accel=kvm,usb=of
f,dump-guest-core=off -cpu Skylake-Client-IBRS -drive file=/usr/share/OVMF/OVMF_CODE.pure-efi.fd,if=pf
lash,format=raw,unit=0,readonly=on -drive file=/Images/OVMF_VARS.pure-efi.G
uest1.fd,if=pflash,format=raw,unit=1 -m 4096 -realtime mlock=off -smp 1,sockets=1,cores=1,threads=1 -u
uid 5a3a191d-8cac-4400-80bb-59136bd05580 -no-user-config -nodefaults -chardev socket,id=charmonitor,fd
=26,server,nowait -mon chardev=charmonitor,id=monitor,mode=control -rtc base=utc,driftfix=slew -global
kvm-pit.lost_tick_policy=delay -no-hpet -no-reboot -global PIIX4_PM.disable_s3=1 -global PIIX4_PM.dis
able_s4=1 -boot strict=on -kernel /var/lib/libvirt/boot/virtinst-vmlinuz.jdC2ks -initrd /var/lib/libvi
rt/boot/virtinst-initrd.img.aHyTeQ -device ich9-usb-ehci1,id=usb,bus=pci.0,addr=0x4.0x7 -device ich9-u
sb-uhci1,masterbus=usb.0,firstport=0,bus=pci.0,multifunction=on,addr=0x4 -device ich9-usb-uhci2,master
bus=usb.0,firstport=2,bus=pci.0,addr=0x4.0x1 -device ich9-usb-uhci3,masterbus=usb.0,firstport=4,bus=pc
i.0,addr=0x4.0x2 -drive file=/Disks/Guest1_disk.img,format=raw,if=none,id=d
rive-ide0-0-0 -device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 -drive file
=/ISO/OracleLinux-R7-U4-Server-x86_64-dvd.iso,format=raw,if=none,id=drive-i
de0-0-1,readonly=on -device ide-cd,bus=ide.0,unit=1,drive=drive-ide0-0-1,id=ide0-0-1 -netdev tap,fd=28
,id=hostnet0,vhost=on,vhostfd=30 -device virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:04:f4:9e,
bus=pci.0,addr=0x3 -chardev pty,id=charserial0 -device isa-serial,chardev=charserial0,id=serial0 -vnc
127.0.0.1:99 -device VGA,id=video0,vgamem_mb=16,bus=pci.0,addr=0x2 -device virtio-balloon-pci,id=ballo
on0,bus=pci.0,addr=0x5 -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=den
y -msg timestamp=on

Any comments/suggestions/etc. are welcome. I plan to send the patch to the devel list in the next day or so.

Thanks in advance,

-Aaron Young


RFC: ASIX USB drivers

Samer El-Haj-Mahmoud
 

EDK2 community,

(and CCing OptionRomPkg maintainers),

ASIX (https://www.asix.com.tw/) has graciously contributed the source code of their latest USB NIC drivers to be up-streamed to TianoCore.

The original contribution (ZIP files) is available here: https://github.com/samerhaj/uefi_drivers/tree/master/UsbNetworking/Asix

I cleaned up the code a bit, and prepared it under this branch on my edk2-platform fork: https://github.com/samerhaj/edk2-platforms/tree/ASIX_USB_Networking

You will find the following new folders under /Drivers/OptionRomPkg/Bus/Usb/UsbNetworking :

/Ax88179
- UEFI driver version 2.9.0 for Ax88179 and Ax88178a
- Source provided by ASIX as: https://github.com/samerhaj/uefi_drivers/blob/master/UsbNetworking/Asix/zip/source/AX88179_178a_UEFI_v2.9.0_Source.zip
- Source corresponds to the binary driver available at: https://www.asix.com.tw/products.php?op=pItemdetail&PItemID=131;71;112

/Ax88772c
- UEFI driver version 2.8.0 for Ax88772c / Ax88772b / Ax88772a
- Source provided by ASIX as: https://github.com/samerhaj/uefi_drivers/blob/master/UsbNetworking/Asix/zip/source/AX88772C_772B_772A_UEFI_v2.8.0_Source.zip
- Source corresponds to the binary driver available at: https://www.asix.com.tw/download.php?sub=driverdetail&PItemID=136

The new Ax88772c driver can effectively replace the existing driver in edk2-platforms/Drivers/OptionRomPkg/Bus/Usb/UsbNetworking/Ax88772b. I compared the code of the new driver with the hisoty of changes to the existing Ax88772b driver (tracing the history back to when OptionRomPkg was under edk2). As far as I can tell, all changes are accounted for in Ax88772c, except the following commits from edk2/OptionRomPkg:

Revision: 45e675f2d0eeda0511b5d6e0ed54f62f94c3826f
OptionRomPkg: Ax88772b: Fixing compilation

Revision: 9a1c4beca0463066660635c2494bb8cf66d4c4bd
OptionRomPkg: Ax88772b: support for multiple dongles and chips

Revision: 4986bbaf1151d1cccc1f2589bd13a86b539676b2
Ax88772: Add logic to separate packet, fix MTU issue. Ax88772b: Fix driver model unload function, fix SCT test failures.

Can folks please take a look at my branch and comment on anything else that might be missing, and on the readiness of this code to get upstreamed to edk2-platform ? I also appreciate any testing on this code, since I only have an AX88772A with IDs ids:

Vendor0x0B95 - ASIX Electronics Corp.
idProduct0x7720
bcdDevice0x0001 - Device# = 00.01
iManufacturer0x01 - ASIX Elec. Corp.
iProduct0x02 - AX88772A

Thanks,
--Samer



IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.


Re: [edk2-devel] [RFC][PATCH v1] UefiCpuPkg/MpInitLib DXE: Reduce AP status check interval

Brian J. Johnson
 

On 3/23/20 9:37 AM, Ni, Ray wrote:
Laszlo,
Adding a PCD means platform integrators need to consider which value to set.
Most of the time, they may just use the default PCD value.
Then, why not we add the PCD later when a real case is met?
The patch changes existent behavior; it is not for a newly introduced
feature.

Because most platforms are not in the edk2 tree, we don't know what
platforms could be regressed by increasing the polling frequency
tenfold. (And remember that the polling action has O(n) cost, where "n"
is the logical processor count.)

Under your suggestion, the expression "real case is met" amounts to
"someone reports a regression" (possibly after the next stable tag,
even). I don't think that's a good idea.
In particular, the patch is motivated by RegisterCpuFeaturesLib -- the
CpuFeaturesInitialize() function -- on some platform(s) that Hao uses.
But there are platforms that don't use RegisterCpuFeaturesLib, and still
use MpInitLib.
OK. I agree with your suggestion.
Thank you. I agree with Laszlo: MP initialization is tricky to scale, and platform dependent. My group builds real machines with thousands of APs, and we have had to do various tweaks to the MP init. code. Having a PCD to adjust this timeout will be very useful.

Thanks,
--
Brian J. Johnson
Enterprise X86 Lab

Hewlett Packard Enterprise
brian.johnson@...


Re: [edk2-devel] [RFCv2] code-first process for UEFI-forum specifications

Laszlo Ersek
 

On 03/23/20 20:05, Leif Lindholm wrote:
Changes to v2 of this proposal:
- Feedback from Laszlo[a] and Mike[b] incorporated.
- I opted to view Mike's responses to Laszlo's questions as
accepted, as no follow-up was made.

Feedback from Felix[c] *not* incorporated, as while I agree with all
of it, I am not convinced that information should go here, but should
instead reside with the UEFI Forum. (This text documents the public
part of the process - it would cause me slight impedance mismatch to
have it also document the non-public part.)

[a] https://edk2.groups.io/g/devel/message/53422
[b] https://edk2.groups.io/g/devel/message/53738
[c] https://edk2.groups.io/g/devel/message/54453

/
Leif

---

This is a proposal for a process by which new features can be added to UEFI
forum specifications after first having been designed and prototyped in the
open.

This process lets changes and the development of new features happen in the
open, without violating the UEFI forum bylaws which prevent publication of
code for in-draft features/changes.

The process does not in fact change the UEFI bylaws - the change is that the
development (of both specification and code) happens in the open. The resulting
specification update is then submitted to the appropriate working goup as an
Engineering Change Request (ECR), and voted on. For the UEFI Forum, this is a
change in workflow, not a change in process.

ECRs are tracked in a UEFI Forum Mantis instance, access restricted to UEFI
Forum Members. TianoCore will enable this new process by providing areas on
https://bugzilla.tianocore.org/ to track both specification updates and
reference implementations and new repositories under
https://github.com/tianocore/ dedicated to hold "code first".


## Bugzilla

bugzilla.tianocore.org will have a product category each for
* ACPI Specification
* PI Specification
* UEFI Specification

Each product category will have a separate components for
* Specification
* Reference implementation


## Github
New repositories will be added for holding the text changes and the source code.

Specification text changes will be held within the affected source repository,
in the Github flavour of markdown, in a file (or split across several files)
with .md suffix.
(This one may break down where we have a specification change affecting multiple
specifications, but at that point we can track it with multiple BZ entries)

Reference implementations targeting EDK2 will be held in branches on
edk2-staging.
Additional repositories for implementing reference features in
additional open source projects can be added in the future, as required.


## Intended workflow
The entity initiating a specifiation update raises a Bugzilla in the appropriate
area in bugzilla.tianocore.org. This entry contains the outline of the change,
and the full initial draft text is attached.

If multiple specification updates are interdependent, especially if between
different specifications, then multiple bugzilla entries should be created.
These bugzilla entries *must* be linked together with dependencies.

After the BZs have been created, new branches should be created in the relevant
repositories for each bugzilla - the branch names should be BZ####, where ####
describes the bugzilla ID assigned, optionally followed by a '-' and something
more descriptive. If multiple bugzilla entries must coexist on a single branch,
one of them is designated the 'top-level', with dependencies properly tracked.
That BZ will be the one naming the branch.


## Source code
In order to ensure draft code does not accidentally leak into production use,
and to signify when the changeover from draft to final happens, *all* new or
modified[1] identifiers need to be prefixed with the relevant BZ####.

[1] Modified in a non-backwards-compatible way. If, for example, a statically
sized array is grown - this does not need to be prefixed. But a tag in a
comment would be *highly* recommended.

### File names
New public header files need the prefix. I.e. `Bz1234MyNewProtocol.h`
Private header files do not need the prefix.

### Contents

The tagging must follow the coding style used by each affected codebase.
Examples:

| Released in spec | Draft version in tree | Comment |
| --- | --- | --- |
| `FunctionName` | `Bz1234FunctionName` | |
| `HEADER_MACRO` | `BZ1234_HEADER_MACRO` | |

For data structures or enums, any new or non-backwards-compatible structs or
fields require a prefix. As above, growing an existing array in an existing
struct requires no prefix.

| `typedef SOME_STRUCT` | `BZ1234_SOME_STRUCT` | Typedef only [2] |
| `StructField` | `Bz1234StructField` | In existing struct[3] |
| `typedef SOME_ENUM` | `BZ1234_SOME_ENUM` | Typedef only [2] |

[2] If the struct or enum definition is separate from the typedef in the public
header, the definition does not need the prefix.
[3] Individual fields in newly added typedefd struct do not need prefix, the
struct already carried the prefix.

Variable prefixes indicating global scope ('g' or 'm') go before the BZ prefix.

| `gSomeGuid` | `gBz1234SomeGuid` | |

Local identifiers, including module-global ones (m-prefixed) do not require a
BZ prefix.
Acked-by: Laszlo Ersek <lersek@...>


Re: [RFCv2] code-first process for UEFI-forum specifications

Ni, Ray
 


## Github
New repositories will be added for holding the text changes and the source code.

Specification text changes will be held within the affected source repository,
in the Github flavour of markdown, in a file (or split across several files)
with .md suffix.
What's the case when multiple .MD files are needed?

(This one may break down where we have a specification change affecting multiple
specifications, but at that point we can track it with multiple BZ entries)


## Source code
In order to ensure draft code does not accidentally leak into production use,
and to signify when the changeover from draft to final happens, *all* new or
modified[1] identifiers need to be prefixed with the relevant BZ####.

[1] Modified in a non-backwards-compatible way. If, for example, a statically
sized array is grown - this does not need to be prefixed. But a tag in a
comment would be *highly* recommended.
If a protocol is enhanced to provide more interfaces with increased revision number,
would you like the protocol name to be prefixed with BZ####?
Or just the new interfaces added to the protocol are prefixed the BZ####?
I think just prefixing the new interfaces can meet the purpose.

But the protocol definition is changed, it also needs to be prefixed according to this flow.
Can you clarify a bit more?


### File names
New public header files need the prefix. I.e. `Bz1234MyNewProtocol.h`
Private header files do not need the prefix.

### Contents

The tagging must follow the coding style used by each affected codebase.
Examples:

| Released in spec | Draft version in tree | Comment |
| --- | --- | --- |
| `FunctionName` | `Bz1234FunctionName` | |
| `HEADER_MACRO` | `BZ1234_HEADER_MACRO` | |
If FunctionName or HEADER_MACRO is defined in non-public header files, I don't
think they require the prefix. Do you agree?

For data structures or enums, any new or non-backwards-compatible structs or
fields require a prefix. As above, growing an existing array in an existing
struct requires no prefix.

| `typedef SOME_STRUCT` | `BZ1234_SOME_STRUCT` | Typedef only [2] |
| `StructField` | `Bz1234StructField` | In existing struct[3] |
| `typedef SOME_ENUM` | `BZ1234_SOME_ENUM` | Typedef only [2] |

[2] If the struct or enum definition is separate from the typedef in the public
header, the definition does not need the prefix.
What does "separate" mean?
Does it mean "struct or enum in the public header BzXXX.h don't need the prefix"?
If yes, then I think macros defined in BzXXX.h also don't need the prefix.

[3] Individual fields in newly added typedefd struct do not need prefix, the
struct already carried the prefix.

Variable prefixes indicating global scope ('g' or 'm') go before the BZ prefix.

| `gSomeGuid` | `gBz1234SomeGuid` | |

Local identifiers, including module-global ones (m-prefixed) do not require a
BZ prefix.
I think only the names (struct type name, enum type name, interface name, protocol/ppi name)
defined in public header files need the BZ prefix when the public header doesn't have prefix.
Right?


Re: [edk2-devel] [RFC][PATCH v1] UefiCpuPkg/MpInitLib DXE: Reduce AP status check interval

Wu, Hao A
 

-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Brian
J. Johnson
Sent: Tuesday, March 24, 2020 12:39 AM
To: devel@edk2.groups.io; Ni, Ray; Laszlo Ersek; Wu, Hao A;
rfc@edk2.groups.io
Cc: Dong, Eric; Kinney, Michael D; Zeng, Star
Subject: Re: [edk2-devel] [RFC][PATCH v1] UefiCpuPkg/MpInitLib DXE: Reduce
AP status check interval

On 3/23/20 9:37 AM, Ni, Ray wrote:
Laszlo,
Adding a PCD means platform integrators need to consider which value to
set.
Most of the time, they may just use the default PCD value.
Then, why not we add the PCD later when a real case is met?
The patch changes existent behavior; it is not for a newly introduced
feature.

Because most platforms are not in the edk2 tree, we don't know what
platforms could be regressed by increasing the polling frequency
tenfold. (And remember that the polling action has O(n) cost, where "n"
is the logical processor count.)

Under your suggestion, the expression "real case is met" amounts to
"someone reports a regression" (possibly after the next stable tag,
even). I don't think that's a good idea.
In particular, the patch is motivated by RegisterCpuFeaturesLib -- the
CpuFeaturesInitialize() function -- on some platform(s) that Hao uses.
But there are platforms that don't use RegisterCpuFeaturesLib, and still
use MpInitLib.
OK. I agree with your suggestion.
Thank you. I agree with Laszlo: MP initialization is tricky to scale,
and platform dependent. My group builds real machines with thousands of
APs, and we have had to do various tweaks to the MP init. code. Having
a PCD to adjust this timeout will be very useful.

Thanks all for the feedbacks. Please grant me some time to prepare a new
version of the patch.

Best Regards,
Hao Wu



Thanks,
--
Brian J. Johnson
Enterprise X86 Lab

Hewlett Packard Enterprise
brian.johnson@...



[RFCv2] code-first process for UEFI-forum specifications

Leif Lindholm
 

Changes to v2 of this proposal:
- Feedback from Laszlo[a] and Mike[b] incorporated.
- I opted to view Mike's responses to Laszlo's questions as
accepted, as no follow-up was made.

Feedback from Felix[c] *not* incorporated, as while I agree with all
of it, I am not convinced that information should go here, but should
instead reside with the UEFI Forum. (This text documents the public
part of the process - it would cause me slight impedance mismatch to
have it also document the non-public part.)

[a] https://edk2.groups.io/g/devel/message/53422
[b] https://edk2.groups.io/g/devel/message/53738
[c] https://edk2.groups.io/g/devel/message/54453

/
Leif

---

This is a proposal for a process by which new features can be added to UEFI
forum specifications after first having been designed and prototyped in the
open.

This process lets changes and the development of new features happen in the
open, without violating the UEFI forum bylaws which prevent publication of
code for in-draft features/changes.

The process does not in fact change the UEFI bylaws - the change is that the
development (of both specification and code) happens in the open. The resulting
specification update is then submitted to the appropriate working goup as an
Engineering Change Request (ECR), and voted on. For the UEFI Forum, this is a
change in workflow, not a change in process.

ECRs are tracked in a UEFI Forum Mantis instance, access restricted to UEFI
Forum Members. TianoCore will enable this new process by providing areas on
https://bugzilla.tianocore.org/ to track both specification updates and
reference implementations and new repositories under
https://github.com/tianocore/ dedicated to hold "code first".


## Bugzilla

bugzilla.tianocore.org will have a product category each for
* ACPI Specification
* PI Specification
* UEFI Specification

Each product category will have a separate components for
* Specification
* Reference implementation


## Github
New repositories will be added for holding the text changes and the source code.

Specification text changes will be held within the affected source repository,
in the Github flavour of markdown, in a file (or split across several files)
with .md suffix.
(This one may break down where we have a specification change affecting multiple
specifications, but at that point we can track it with multiple BZ entries)

Reference implementations targeting EDK2 will be held in branches on
edk2-staging.
Additional repositories for implementing reference features in
additional open source projects can be added in the future, as required.


## Intended workflow
The entity initiating a specifiation update raises a Bugzilla in the appropriate
area in bugzilla.tianocore.org. This entry contains the outline of the change,
and the full initial draft text is attached.

If multiple specification updates are interdependent, especially if between
different specifications, then multiple bugzilla entries should be created.
These bugzilla entries *must* be linked together with dependencies.

After the BZs have been created, new branches should be created in the relevant
repositories for each bugzilla - the branch names should be BZ####, where ####
describes the bugzilla ID assigned, optionally followed by a '-' and something
more descriptive. If multiple bugzilla entries must coexist on a single branch,
one of them is designated the 'top-level', with dependencies properly tracked.
That BZ will be the one naming the branch.


## Source code
In order to ensure draft code does not accidentally leak into production use,
and to signify when the changeover from draft to final happens, *all* new or
modified[1] identifiers need to be prefixed with the relevant BZ####.

[1] Modified in a non-backwards-compatible way. If, for example, a statically
sized array is grown - this does not need to be prefixed. But a tag in a
comment would be *highly* recommended.

### File names
New public header files need the prefix. I.e. `Bz1234MyNewProtocol.h`
Private header files do not need the prefix.

### Contents

The tagging must follow the coding style used by each affected codebase.
Examples:

| Released in spec | Draft version in tree | Comment |
| --- | --- | --- |
| `FunctionName` | `Bz1234FunctionName` | |
| `HEADER_MACRO` | `BZ1234_HEADER_MACRO` | |

For data structures or enums, any new or non-backwards-compatible structs or
fields require a prefix. As above, growing an existing array in an existing
struct requires no prefix.

| `typedef SOME_STRUCT` | `BZ1234_SOME_STRUCT` | Typedef only [2] |
| `StructField` | `Bz1234StructField` | In existing struct[3] |
| `typedef SOME_ENUM` | `BZ1234_SOME_ENUM` | Typedef only [2] |

[2] If the struct or enum definition is separate from the typedef in the public
header, the definition does not need the prefix.
[3] Individual fields in newly added typedefd struct do not need prefix, the
struct already carried the prefix.

Variable prefixes indicating global scope ('g' or 'm') go before the BZ prefix.

| `gSomeGuid` | `gBz1234SomeGuid` | |

Local identifiers, including module-global ones (m-prefixed) do not require a
BZ prefix.


Re: [edk2-devel] [RFC][PATCH v1] UefiCpuPkg/MpInitLib DXE: Reduce AP status check interval

Ni, Ray
 

Laszlo,
Adding a PCD means platform integrators need to consider which value to set.
Most of the time, they may just use the default PCD value.
Then, why not we add the PCD later when a real case is met?
The patch changes existent behavior; it is not for a newly introduced
feature.

Because most platforms are not in the edk2 tree, we don't know what
platforms could be regressed by increasing the polling frequency
tenfold. (And remember that the polling action has O(n) cost, where "n"
is the logical processor count.)

Under your suggestion, the expression "real case is met" amounts to
"someone reports a regression" (possibly after the next stable tag,
even). I don't think that's a good idea.
In particular, the patch is motivated by RegisterCpuFeaturesLib -- the
CpuFeaturesInitialize() function -- on some platform(s) that Hao uses.
But there are platforms that don't use RegisterCpuFeaturesLib, and still
use MpInitLib.
OK. I agree with your suggestion.


Re: [edk2-devel] [RFC][PATCH v1] UefiCpuPkg/MpInitLib DXE: Reduce AP status check interval

Laszlo Ersek
 

On 03/16/20 02:37, Ni, Ray wrote:
The use case is valid, IMO. And the commit message is helpful.

But I really think this constant should be PCD. Here's why I think a
platform might want to control it:

- The best (finest) possible resolution for timer events is platform
dependent, IIUC. The duration of the "idle tick" is platform-specific.
And, it likely makes no sense to set AP_CHECK_INTERVAL to a duration
that's around, or under, what the arch timer resolution allows for.

- In the other direction, CheckAndUpdateApsStatus() contains a loop that
counts up to CpuMpData->CpuCount. In a very large system (hundreds or
maybe thousands of APs) this function may have non-negligible cost.

I suggest introducing a PCD for this (measured in msecs) and using 100
msecs as the default.
Laszlo,
Adding a PCD means platform integrators need to consider which value to set.
Most of the time, they may just use the default PCD value.
Then, why not we add the PCD later when a real case is met?
The patch changes existent behavior; it is not for a newly introduced
feature.

Because most platforms are not in the edk2 tree, we don't know what
platforms could be regressed by increasing the polling frequency
tenfold. (And remember that the polling action has O(n) cost, where "n"
is the logical processor count.)

Under your suggestion, the expression "real case is met" amounts to
"someone reports a regression" (possibly after the next stable tag,
even). I don't think that's a good idea.

In particular, the patch is motivated by RegisterCpuFeaturesLib -- the
CpuFeaturesInitialize() function -- on some platform(s) that Hao uses.
But there are platforms that don't use RegisterCpuFeaturesLib, and still
use MpInitLib.

Thanks,
Laszlo


Re: [edk2-devel] [RFC][PATCH v1] UefiCpuPkg/MpInitLib DXE: Reduce AP status check interval

Ni, Ray
 

The use case is valid, IMO. And the commit message is helpful.

But I really think this constant should be PCD. Here's why I think a
platform might want to control it:

- The best (finest) possible resolution for timer events is platform
dependent, IIUC. The duration of the "idle tick" is platform-specific.
And, it likely makes no sense to set AP_CHECK_INTERVAL to a duration
that's around, or under, what the arch timer resolution allows for.

- In the other direction, CheckAndUpdateApsStatus() contains a loop that
counts up to CpuMpData->CpuCount. In a very large system (hundreds or
maybe thousands of APs) this function may have non-negligible cost.

I suggest introducing a PCD for this (measured in msecs) and using 100
msecs as the default.
Laszlo,
Adding a PCD means platform integrators need to consider which value to set.
Most of the time, they may just use the default PCD value.
Then, why not we add the PCD later when a real case is met?

Thanks,
Ray


Re: [RFC][PATCH v1] UefiCpuPkg/MpInitLib DXE: Reduce AP status check interval

Laszlo Ersek
 

Hi Hao,

On 03/13/20 14:22, Hao A Wu wrote:
This commit will reduce the interval of the AP status check event from 100
milliseconds to 10 milliseconds.

(I searched the history of the 100ms interval, it seems no comment or log
message was mentioned for the choice of this value. Looks like the value
is selected by experience.)

The purpose is to reduce the response time when the BSP calls below
EFI_MP_SERVICES_PROTOCOL services in a non-blocking manner:

* StartupAllAPs()
* StartupThisAP()

Reducing the check interval will benefit the performance for the case when
the BSP uses WaitForEvent() or uses CheckEvent() in a loop to wait for
AP(s) to complete the task, especially when the task can be finished
considerably fast on AP(s).

An example is within function CpuFeaturesInitialize() under
UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c,
where BSP will perform the same task with APs and requires all the
processors to finish the task before BSP proceeds to its next task.

Impact:
A. The impact is minimal when there is no non-blocking calls of the
StartupAllAPs/StartupThisAp MP services, because the check function
CheckAndUpdateApsStatus() will return directly when there is no
registered wait event (i.e. no non-blocking request).

B. There will be a performance tradeoff when BSP continues to proceed
other tasks after submitting a non-blocking StartupAllAPs/StartupThisAP
request. If the AP status check takes a good portion of the shortened
interval, BSP will have less time slice working on its own task before
all the APs complete their tasks.

My investigation for Impact B is that it is a rare scenario in the edk2
code base.

Unitests:
A. OS boot successfully.
B. System (with 24 threads) boot time reduced. Almost all the saved time
comes from the above-mentioned case in CpuFeaturesInitialize().

Cc: Eric Dong <eric.dong@...>
Cc: Ray Ni <ray.ni@...>
Cc: Laszlo Ersek <lersek@...>
Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Star Zeng <star.zeng@...>
Signed-off-by: Hao A Wu <hao.a.wu@...>
---
UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
index a987c32109..9ba886e8ed 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
@@ -15,7 +15,7 @@

#include <Protocol/Timer.h>

-#define AP_CHECK_INTERVAL (EFI_TIMER_PERIOD_MILLISECONDS (100))
+#define AP_CHECK_INTERVAL (EFI_TIMER_PERIOD_MILLISECONDS (10))
#define AP_SAFE_STACK_SIZE 128

CPU_MP_DATA *mCpuMpData = NULL;
The use case is valid, IMO. And the commit message is helpful.

But I really think this constant should be PCD. Here's why I think a
platform might want to control it:

- The best (finest) possible resolution for timer events is platform
dependent, IIUC. The duration of the "idle tick" is platform-specific.
And, it likely makes no sense to set AP_CHECK_INTERVAL to a duration
that's around, or under, what the arch timer resolution allows for.

- In the other direction, CheckAndUpdateApsStatus() contains a loop that
counts up to CpuMpData->CpuCount. In a very large system (hundreds or
maybe thousands of APs) this function may have non-negligible cost.

I suggest introducing a PCD for this (measured in msecs) and using 100
msecs as the default.

Thanks
Laszlo


[RFC][PATCH v1] UefiCpuPkg/MpInitLib DXE: Reduce AP status check interval

Wu, Hao A
 

This commit will reduce the interval of the AP status check event from 100
milliseconds to 10 milliseconds.

(I searched the history of the 100ms interval, it seems no comment or log
message was mentioned for the choice of this value. Looks like the value
is selected by experience.)

The purpose is to reduce the response time when the BSP calls below
EFI_MP_SERVICES_PROTOCOL services in a non-blocking manner:

* StartupAllAPs()
* StartupThisAP()

Reducing the check interval will benefit the performance for the case when
the BSP uses WaitForEvent() or uses CheckEvent() in a loop to wait for
AP(s) to complete the task, especially when the task can be finished
considerably fast on AP(s).

An example is within function CpuFeaturesInitialize() under
UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c,
where BSP will perform the same task with APs and requires all the
processors to finish the task before BSP proceeds to its next task.

Impact:
A. The impact is minimal when there is no non-blocking calls of the
StartupAllAPs/StartupThisAp MP services, because the check function
CheckAndUpdateApsStatus() will return directly when there is no
registered wait event (i.e. no non-blocking request).

B. There will be a performance tradeoff when BSP continues to proceed
other tasks after submitting a non-blocking StartupAllAPs/StartupThisAP
request. If the AP status check takes a good portion of the shortened
interval, BSP will have less time slice working on its own task before
all the APs complete their tasks.

My investigation for Impact B is that it is a rare scenario in the edk2
code base.

Unitests:
A. OS boot successfully.
B. System (with 24 threads) boot time reduced. Almost all the saved time
comes from the above-mentioned case in CpuFeaturesInitialize().

Cc: Eric Dong <eric.dong@...>
Cc: Ray Ni <ray.ni@...>
Cc: Laszlo Ersek <lersek@...>
Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Star Zeng <star.zeng@...>
Signed-off-by: Hao A Wu <hao.a.wu@...>
---
UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
index a987c32109..9ba886e8ed 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
@@ -15,7 +15,7 @@

#include <Protocol/Timer.h>

-#define AP_CHECK_INTERVAL (EFI_TIMER_PERIOD_MILLISECONDS (100))
+#define AP_CHECK_INTERVAL (EFI_TIMER_PERIOD_MILLISECONDS (10))
#define AP_SAFE_STACK_SIZE 128

CPU_MP_DATA *mCpuMpData = NULL;
--
2.12.0.windows.1


Re: [edk2-devel] [edk2-rfc] [RFC] code-first process for UEFI-forum specifications

Samer El-Haj-Mahmoud
 

Thanks Leif.

As far as I know, the main feedback I heard is "when will this start?"... So, the sooner the better .. Thanks for taking the lead and driving!

-----Original Message-----
From: Leif Lindholm <leif@...>
Sent: Wednesday, March 11, 2020 12:03 PM
To: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@...>
Cc: devel@edk2.groups.io; Felixp@...; rfc@edk2.groups.io
Subject: Re: [edk2-devel] [edk2-rfc] [RFC] code-first process for UEFI-forum specifications

Hi Samer,

I had, perhaps excessively, been waiting for more feedback.

I promised Mike yesterday that I will rework based on feedback and send out next week at the latest. If people have no further comments then, we can adopt the process and start using it.

Regards,

Leif

On Wed, Mar 11, 2020 at 15:52:07 +0000, Samer El-Haj-Mahmoud wrote:
Has there been any progress on this "code-first process" proposal? Any timeline on when we should expect it to be launched?

Thanks,
--Samer


-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Felix
Polyudov via Groups.Io
Sent: Friday, February 14, 2020 10:30 AM
To: rfc@edk2.groups.io; 'leif@...' <leif@...>;
devel@edk2.groups.io
Subject: Re: [edk2-devel] [edk2-rfc] [RFC] code-first process for
UEFI-forum specifications

Leif,

The process does not in fact change the UEFI bylaws - the change is
that the development (of both specification and code) happens in the
open. The resulting specification update is then submitted to the
appropriate working goup as an Engineering Change Request (ECR), and
voted on. For the UEFI Forum, this is a change in workflow, not a change in process.
I think it would be good to add more details regarding the interaction between edk2 and UEFI forum.
Here is what I suggest:
Each specification update Bugzilla ticket must have a sponsor. A sponsor is a person or a company that will be presenting change request to the UEFI forum.
A sponsor has to be identified early in the process. Preferably along with Buzilla ticket creation.
It is sponsor's responsibility to officially submit ECR to the UEFI forum by creating a mantis ticket with a Bugzilla link.
There are two reasons to create mantis ticket early in the process:
- Creation of the ticket exposes the effort to the UEFI forum thus enabling early feedback from the members(via Bugzilla), which may reduce number of iterations in the
implement --> get feedback --> re-implement cycle.
- edk2 effort will be taken into consideration while scheduling future
specification releases


Please consider the environment before printing this email.

The information contained in this message may be confidential and proprietary to American Megatrends (AMI). This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited. Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.



IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.


Re: [edk2-devel] [edk2-rfc] [RFC] code-first process for UEFI-forum specifications

Samer El-Haj-Mahmoud
 

Has there been any progress on this "code-first process" proposal? Any timeline on when we should expect it to be launched?

Thanks,
--Samer

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Felix Polyudov via Groups.Io
Sent: Friday, February 14, 2020 10:30 AM
To: rfc@edk2.groups.io; 'leif@...' <leif@...>; devel@edk2.groups.io
Subject: Re: [edk2-devel] [edk2-rfc] [RFC] code-first process for UEFI-forum specifications

Leif,

The process does not in fact change the UEFI bylaws - the change is
that the development (of both specification and code) happens in the
open. The resulting specification update is then submitted to the
appropriate working goup as an Engineering Change Request (ECR), and
voted on. For the UEFI Forum, this is a change in workflow, not a change in process.
I think it would be good to add more details regarding the interaction between edk2 and UEFI forum.
Here is what I suggest:
Each specification update Bugzilla ticket must have a sponsor. A sponsor is a person or a company that will be presenting change request to the UEFI forum.
A sponsor has to be identified early in the process. Preferably along with Buzilla ticket creation.
It is sponsor's responsibility to officially submit ECR to the UEFI forum by creating a mantis ticket with a Bugzilla link.
There are two reasons to create mantis ticket early in the process:
- Creation of the ticket exposes the effort to the UEFI forum thus enabling early feedback from the members(via Bugzilla), which may reduce number of iterations in the
implement --> get feedback --> re-implement cycle.
- edk2 effort will be taken into consideration while scheduling future specification releases


Please consider the environment before printing this email.

The information contained in this message may be confidential and proprietary to American Megatrends (AMI). This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited. Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.



IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.


Re: [edk2-devel] [edk2-rfc] [RFC] code-first process for UEFI-forum specifications

Doran, Mark <mark.doran@...>
 

The one thing we don't have in hand for this is a template into which we can put prospective ECR submission material that is collected as part of the code-first process.

We could just use the ECR template that is used for regular proposals with the UEFI Work Groups. However, the thought was to add some verbiage that template which effectively says: anything put in the proposed ECR template comes with permission to give ownership and rights in that content to the UEFI Forum so that it can publish the material as part of the specs under the same rights as all the other spec material there.

Since TianoCore doesn't have a legal team per se, we're having Intel legal work something like that with a view to contributing that to the community for review and hopefully adoption in short order.

That said, I don't think we need to wait on the template to begin using a code-first approach for things we would ultimately like to end up in the formal specs once code is "done". We really only need that template at the point that the ECR content is also done, meaning reflective of sufficiently completed code work.

If we do start assembling content that looks like ECR fodder in parallel with code development, I'd suggest that we keep track of who is putting words into that fodder so that when it gets moved into the template prior to formal submission as an ECR we can get assurance from those people that they are OK with the T's & C's in the template.

Fundamentally we just need to make sure that everyone contributing to the development of one of these code first things is OK with spec description stuff being incorporated into a UEFI document under UEFI T's & C's as the end product. UEFI T's & C's are designed to make sure that anyone can read the spec just for the asking and that there are no undesired IP complications (as there have been in some other standards) for people that choose to implement to the specs.

--
Cheers,

Mark.

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Samer El-Haj-Mahmoud
Sent: Wednesday, March 11, 2020 9:32 AM
To: Leif Lindholm <leif@...>
Cc: devel@edk2.groups.io; Felixp@...; rfc@edk2.groups.io
Subject: Re: [edk2-devel] [edk2-rfc] [RFC] code-first process for UEFI-forum specifications

Thanks Leif.

As far as I know, the main feedback I heard is "when will this start?"... So, the sooner the better .. Thanks for taking the lead and driving!



-----Original Message-----
From: Leif Lindholm <leif@...>
Sent: Wednesday, March 11, 2020 12:03 PM
To: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@...>
Cc: devel@edk2.groups.io; Felixp@...; rfc@edk2.groups.io
Subject: Re: [edk2-devel] [edk2-rfc] [RFC] code-first process for UEFI-forum specifications

Hi Samer,

I had, perhaps excessively, been waiting for more feedback.

I promised Mike yesterday that I will rework based on feedback and send out next week at the latest. If people have no further comments then, we can adopt the process and start using it.

Regards,

Leif

On Wed, Mar 11, 2020 at 15:52:07 +0000, Samer El-Haj-Mahmoud wrote:
Has there been any progress on this "code-first process" proposal? Any timeline on when we should expect it to be launched?

Thanks,
--Samer


-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Felix
Polyudov via Groups.Io
Sent: Friday, February 14, 2020 10:30 AM
To: rfc@edk2.groups.io; 'leif@...' <leif@...>;
devel@edk2.groups.io
Subject: Re: [edk2-devel] [edk2-rfc] [RFC] code-first process for
UEFI-forum specifications

Leif,

The process does not in fact change the UEFI bylaws - the change is
that the development (of both specification and code) happens in the
open. The resulting specification update is then submitted to the
appropriate working goup as an Engineering Change Request (ECR), and
voted on. For the UEFI Forum, this is a change in workflow, not a change in process.
I think it would be good to add more details regarding the interaction between edk2 and UEFI forum.
Here is what I suggest:
Each specification update Bugzilla ticket must have a sponsor. A sponsor is a person or a company that will be presenting change request to the UEFI forum.
A sponsor has to be identified early in the process. Preferably along with Buzilla ticket creation.
It is sponsor's responsibility to officially submit ECR to the UEFI forum by creating a mantis ticket with a Bugzilla link.
There are two reasons to create mantis ticket early in the process:
- Creation of the ticket exposes the effort to the UEFI forum thus enabling early feedback from the members(via Bugzilla), which may reduce number of iterations in the
implement --> get feedback --> re-implement cycle.
- edk2 effort will be taken into consideration while scheduling future
specification releases


Please consider the environment before printing this email.

The information contained in this message may be confidential and proprietary to American Megatrends (AMI). This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited. Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.



IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

521 - 540 of 789