From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c0b::241; helo=mail-it0-x241.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x241.google.com (mail-it0-x241.google.com [IPv6:2607:f8b0:4001:c0b::241]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 1DF772034A794 for ; Mon, 27 Nov 2017 23:51:10 -0800 (PST) Received: by mail-it0-x241.google.com with SMTP id x13so24756243iti.4 for ; Mon, 27 Nov 2017 23:55:33 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=zwS5FFMl39fFXFq87YPLGYdsQxcuTTHTNjbPwutK/Ek=; b=hF4GXroyfDHO8hs8jVvrqQDtXfp/hlCUQ6eEEk/Az1gXxb+9L8vKi/dXGlV9aOPzWA jf7QzjgRfg9KE2EJyJFiJt1QQOulToLcqDWuPDSMs2PKAqkELMeDvr8ozR0AgI+TeI7R vvbxWLWQIuyZTGQAnCVq+DZ1f/zF/X1I7oRX8= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=zwS5FFMl39fFXFq87YPLGYdsQxcuTTHTNjbPwutK/Ek=; b=tQNx4VUZNxwGDDbzmh8LqR9eCG/bDInnOwFB06Fe/3SHXOqyC4jTH1mcPwDGuMOoEx FuE5oG9lGoeWEDX2QtKSSjMk0Z8YLAeKKaSQoubJ71GDg3lDenCojVIAnwsb8mfhCa8y YxCGANhxr9j/j14ZlFXPZ4CSHxdX4ClurcjkT+GrhR/LJB/CB8+6sSVuqfWzwlVXKWMi 0mMJDruDPX9wuZ4xJLhIDHKLlYZoE+wVpo4OdvvgC62TaA8BErKZe+ZGkiDKNQmDElnY ZRfI2yqWZOVTGF78X541IWFcXDxV4y+jy5gKBpOnYh3oDkCzajxesMhq64vBdmTYzTfL tGAg== X-Gm-Message-State: AJaThX6JaGV3NOEWg2dvoKgwNoZMxGlAHpOy5kYnG8eNnjNossTDiLKV 2Xn0KhsVY8jlaVZUHS5YGwZb7RACEPqK2uQy31LBmw== X-Google-Smtp-Source: AGs4zMYgFqiLDMQMhlHGssPgElsidoxoQkY6Cx4wR4767jW2SUsoOfv5xxC9NqQbULF4PXgLPhlMFjqaLj1s9EqEWU8= X-Received: by 10.36.31.80 with SMTP id d77mr1261120itd.65.1511855732856; Mon, 27 Nov 2017 23:55:32 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.104.16 with HTTP; Mon, 27 Nov 2017 23:55:32 -0800 (PST) In-Reply-To: <20171127190354.13699-2-lersek@redhat.com> References: <20171127190354.13699-1-lersek@redhat.com> <20171127190354.13699-2-lersek@redhat.com> From: Ard Biesheuvel Date: Tue, 28 Nov 2017 07:55:32 +0000 Message-ID: To: Laszlo Ersek Cc: edk2-devel-01 , Jordan Justen Subject: Re: [PATCH 1/2] OvmfPkg/QemuBootOrderLib: skip already matched / appended UEFI boot opts X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 28 Nov 2017 07:51:11 -0000 Content-Type: text/plain; charset="UTF-8" On 27 November 2017 at 19:03, Laszlo Ersek 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 > Cc: Jordan Justen > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Laszlo Ersek Acked-by: Ard Biesheuvel > --- > 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 > >