4MB flash size as build default.

4MB flash size as build default.

[Prerna]

Hi Laszlo,
I noticed that the following commit was used to make the 4MB flash layout
the default for edk2 builds briefly:

bba8dfbec3bb OvmfPkg: make the 4MB flash size the default

However, it was later reverted :

6e49d01cfb43 Revert "OvmfPkg: make the 4MB flash size the default"

presumably to fix this issue:

0c79471d6a98 OvmfPkg/PlatformPei: handle non-power-of-two spare
                  size for emu variables

Given that the above commit was also merged, why does edk2 still default to
the 2MB flash layout for builds? Do you see any other issues too with the
4MB flash layout ?

Regards,
Prerna

Re: 4MB flash size as build default.

[Laszlo Ersek]

On 06/06/18 05:05, Prerna wrote:
> Hi Laszlo,
> I noticed that the following commit was used to make the 4MB flash layout
> the default for edk2 builds briefly:
> 
> bba8dfbec3bb OvmfPkg: make the 4MB flash size the default
> 
> However, it was later reverted :
> 
> 6e49d01cfb43 Revert "OvmfPkg: make the 4MB flash size the default"
> 
> presumably to fix this issue:
> 
> 0c79471d6a98 OvmfPkg/PlatformPei: handle non-power-of-two spare
>                   size for emu variables

No, that's not exactly the reason. Please see the following sub-thread:

http://mid.mail-archive.com/bdf196e8-df87-84b9-1d6e-bbece5fa65b7@redhat.com

Briefly, the 4MB unified pflash size broke two things:
- the EmuVariableFvbRuntimeDxe driver, which would only affect QEMU
  command lines with -bios,
- the PlatformPei PEIM, which would affect QEMU command lines with
  -pflash too.

(In the linked message, I explained why I hadn't noticed even regression
#2.)

The temporary solution was to (a) restore the default to 2MB -- this
would allow people to continue using both -bios and -pflash, and (b)
kludge around the PlatformPei regression, so that the (manually
selected) 4MB build would be usable for -pflash, at the cost of wasting
a little bit of reserved memory.

In other words, the last two commits that you list have no
inter-dependency between them, instead they mitigated two aspects of the
regression.

In the longer term, I extended EmuVariableFvbRuntimeDxe (and the
interfacing code in PlatformPei) to handle the new 4MB unified size as
well. Please refer to
<https://bugzilla.tianocore.org/show_bug.cgi?id=527>. This completed the
4MB support for "-bios" from the OVMF side; however it turned out that
even Xen needed fixes.

Once Xen too received the necessary patches, we re-enabled the 4MB
default size (commit 1c47fcd465a4 -- basically a re-application of
commit bba8dfbe). Please refer to
<http://mid.mail-archive.com/d327e563-c89a-de28-ae75-db629722c70a@redhat.com>,
and the other message it references.

> Given that the above commit was also merged, why does edk2 still default to
> the 2MB flash layout for builds?

It doesn't; if you build upstream edk2, it defaults to 4MB.

We preserved the 2MB unified size in Gerd's and Fedora's OVMF builds
because the varstore structures are incompatible between the 2MB and the
4MB builds. (Please refer to section "OVMF Flash Layout" in the
"OvmfPkg/README" file.) If you have a preexistent varstore file,
originally created from the OVMF_VARS.fd template of the 2MB build, and
then you boot the same VM with the OVMF_CODE.fd binary of the 4MB build,
your VM will not boot. (It will just hang with a black screen. If you
capture the OVMF debug log, you will get some error messages, but
practically no user ever captures the debug log.)

In other words, we preserved the 2MB unified size in Fedora and in
Gerd's repo for staying compatible with the existent VMs of the users of
those distros / repos.

As one counter-example, we switched from 2MB to 4MB in RHEL-7.4. This
broke compatibility with VMs created against OVMF in RHEL-7.3, but that
was fine: in RHEL-7.3, OVMF was Tech Preview, and breaking compat with
Tech Preview packages is explicitly permitted.

Now obviously users of Fedora and Gerd's repo aren't *officially*
entitled to any kind of stability or support (which is why I don't run
Fedora for my day to day work, by the way), so we could have broken
compat there as well, right? Well, yes, if only we had wanted to field
tens or maybe hundreds of complaints on the vfio-users mailing list and
elsewhere. :) Those users most likely didn't *need* the 4MB build -- see
the motivation for the 4MB build in commit b24fca05751f --, hence
keeping compat for them was more important. Regarding direct consumers
of the upstream edk2 *source* tree, we figured they were technical
enough to deal with the change in the default size.

> Do you see any other issues too with the 4MB flash layout ?

Nope.

If you really want to use the 4MB build in Fedora (although I don't see
why you would), it's technically possible to provide the split 4MB build
in addition to the current files (under different filenames). In that
case, please file an RHBZ for the "edk2" component in Fedora (and CC
Gerd on it so he can adapt his package as well, if he intends to).

Thanks
Laszlo

Re: 4MB flash size as build default.

[Prerna]

On Wed, Jun 6, 2018 at 4:02 PM, Laszlo Ersek <lersek@redhat.com> wrote:

> On 06/06/18 05:05, Prerna wrote:
> > Hi Laszlo,
> > I noticed that the following commit was used to make the 4MB flash layout
> > the default for edk2 builds briefly:
> >
> > bba8dfbec3bb OvmfPkg: make the 4MB flash size the default
> >
> > However, it was later reverted :
> >
> > 6e49d01cfb43 Revert "OvmfPkg: make the 4MB flash size the default"
> >
> > presumably to fix this issue:
> >
> > 0c79471d6a98 OvmfPkg/PlatformPei: handle non-power-of-two spare
> >                   size for emu variables
>
> No, that's not exactly the reason. Please see the following sub-thread:
>
> http://mid.mail-archive.com/bdf196e8-df87-84b9-1d6e-
> bbece5fa65b7@redhat.com
>
> Briefly, the 4MB unified pflash size broke two things:
> - the EmuVariableFvbRuntimeDxe driver, which would only affect QEMU
>   command lines with -bios,
> - the PlatformPei PEIM, which would affect QEMU command lines with
>   -pflash too.
>
> (In the linked message, I explained why I hadn't noticed even regression
> #2.)
>
> The temporary solution was to (a) restore the default to 2MB -- this
> would allow people to continue using both -bios and -pflash, and (b)
> kludge around the PlatformPei regression, so that the (manually
> selected) 4MB build would be usable for -pflash, at the cost of wasting
> a little bit of reserved memory.
>
> In other words, the last two commits that you list have no
> inter-dependency between them, instead they mitigated two aspects of the
> regression.
>
> In the longer term, I extended EmuVariableFvbRuntimeDxe (and the
> interfacing code in PlatformPei) to handle the new 4MB unified size as
> well. Please refer to
> <https://bugzilla.tianocore.org/show_bug.cgi?id=527>. This completed the
> 4MB support for "-bios" from the OVMF side; however it turned out that
> even Xen needed fixes.
>
> Once Xen too received the necessary patches, we re-enabled the 4MB
> default size (commit 1c47fcd465a4 -- basically a re-application of
> commit bba8dfbe). Please refer to
> <http://mid.mail-archive.com/d327e563-c89a-de28-ae75-
> db629722c70a@redhat.com>,
> and the other message it references.
>
> > Given that the above commit was also merged, why does edk2 still default
> to
> > the 2MB flash layout for builds?
>
> It doesn't; if you build upstream edk2, it defaults to 4MB.
>

Thank you. It appears I had missed the new commits that re-enabled the 4MB
size as default.



>
> We preserved the 2MB unified size in Gerd's and Fedora's OVMF builds
> because the varstore structures are incompatible between the 2MB and the
> 4MB builds. (Please refer to section "OVMF Flash Layout" in the
> "OvmfPkg/README" file.) If you have a preexistent varstore file,
> originally created from the OVMF_VARS.fd template of the 2MB build, and
> then you boot the same VM with the OVMF_CODE.fd binary of the 4MB build,
> your VM will not boot. (It will just hang with a black screen. If you
> capture the OVMF debug log, you will get some error messages, but
> practically no user ever captures the debug log.)
>
> In other words, we preserved the 2MB unified size in Fedora and in
> Gerd's repo for staying compatible with the existent VMs of the users of
> those distros / repos.
>

yes, I had played around with these two different layouts.
I found my VMs to hang and not boot in case I mismatched the OVMF_VARS.fd.



>
> As one counter-example, we switched from 2MB to 4MB in RHEL-7.4. This
> broke compatibility with VMs created against OVMF in RHEL-7.3, but that
> was fine: in RHEL-7.3, OVMF was Tech Preview, and breaking compat with
> Tech Preview packages is explicitly permitted.
>
> Now obviously users of Fedora and Gerd's repo aren't *officially*
> entitled to any kind of stability or support (which is why I don't run
> Fedora for my day to day work, by the way), so we could have broken
> compat there as well, right? Well, yes, if only we had wanted to field
> tens or maybe hundreds of complaints on the vfio-users mailing list and
> elsewhere. :) Those users most likely didn't *need* the 4MB build -- see
> the motivation for the 4MB build in commit b24fca05751f --, hence
> keeping compat for them was more important. Regarding direct consumers
> of the upstream edk2 *source* tree, we figured they were technical
> enough to deal with the change in the default size.
>
> > Do you see any other issues too with the 4MB flash layout ?
>
> Nope.
>
> If you really want to use the 4MB build in Fedora (although I don't see
> why you would), it's technically possible to provide the split 4MB build
> in addition to the current files (under different filenames). In that
> case, please file an RHBZ for the "edk2" component in Fedora (and CC
> Gerd on it so he can adapt his package as well, if he intends to).
>
>
I do not necessarily need Fedora. Will be happy to build from upstream.
Thank you for patiently explaining the implications of non-power-of-2 for

EmuVariableFvbRuntimeDxe.

My limited reading had just spotted the breakage for PlatformPei PEIM.
This clearly was a commit I had missed from looking at upstream git log.

Regards,
Prerna