Re: [PATCH 1/2] OvmfPkg/QemuBootOrderLib: skip already matched / appended UEFI boot opts

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 27 November 2017 at 19:03, Laszlo Ersek <lersek@redhat.com> wrote:
> The SetBootOrderFromQemu() function implements a nested loop where
>
> - the outer loop iterates over all OpenFirmware (OFW) device paths in the
>   QEMU boot order, and translates each to a UEFI device path prefix;
>
> - the inner loop matches the current (translated) prefix against all
>   active UEFI boot options in turn;
>
> - if the UEFI boot option is matched by the translated prefix, the UEFI
>   boot option is appended to the "new" UEFI boot order, and marked as
>   "has been appended".
>
> This patch adds a micro-optimization where already matched / appended UEFI
> boot options are skipped in the inner loop. This is not a functional
> change. A functional change would be if, as a consequence of the patch,
> some UEFI boot options would no longer be *doubly* matched.
>
> For a UEFI boot option to be matched by two translated prefixes, one of
> those prefixes would have to be a (proper, or equal) prefix of the other
> prefix. The PCI and MMIO OFW translation routines output such only in the
> following cases:
>
> - When the original OFW device paths are prefixes of each other. This is
>   not possible from the QEMU side. (Only leaf devices are bootable.)
>
> - When the translation rules in the routines are incomplete, and don't
>   look at the OFW device paths for sufficient length (i.e., at nodes where
>   they would already differ, and the difference would show up in the
>   translation output).
>
>   This would be a shortcoming of the translation routines and should be
>   fixed in TranslatePciOfwNodes() and TranslateMmioOfwNodes(), whenever
>   identified.
>
> Even in the second case, this patch would replace the double appending of
> a single UEFI boot option (matched by two different OFW device paths) with
> a correct, or cross-, matching of two different UEFI boot options. Again,
> this is not expected, but arguably it would be more correct than duplicate
> boot option appending, should it occur due to any (unexpected, unknown)
> lack of detail in the translation routines.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---
>  OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.c b/OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.c
> index 7c1f375beb20..a9a62e9d4007 100644
> --- a/OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.c
> +++ b/OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.c
> @@ -1849,31 +1849,32 @@ SetBootOrderFromQemu (
>    //
>    TranslatedSize = ARRAY_SIZE (Translated);
>    Status = TranslateOfwPath (&FwCfgPtr, ExtraPciRoots, Translated,
>               &TranslatedSize);
>    while (Status == RETURN_SUCCESS ||
>           Status == RETURN_UNSUPPORTED ||
>           Status == RETURN_PROTOCOL_ERROR ||
>           Status == RETURN_BUFFER_TOO_SMALL) {
>      if (Status == RETURN_SUCCESS) {
>        UINTN Idx;
>
>        //
>        // match translated OpenFirmware path against all active boot options
>        //
>        for (Idx = 0; Idx < ActiveCount; ++Idx) {
> -        if (Match (
> +        if (!ActiveOption[Idx].Appended &&
> +            Match (
>                Translated,
>                TranslatedSize, // contains length, not size, in CHAR16's here
>                ActiveOption[Idx].BootOption->FilePath
>                )
>              ) {
>            //
>            // match found, store ID and continue with next OpenFirmware path
>            //
>            Status = BootOrderAppend (&BootOrder, &ActiveOption[Idx]);
>            if (Status != RETURN_SUCCESS) {
>              goto ErrorFreeExtraPciRoots;
>            }
>            break;
>          }
>        } // scanned all active boot options
> --
> 2.14.1.3.gb7cf6e02401b
>
>