Re: [PATCH] OvmfPkg/QemuBootOrderLib: drop too strict "/HD(" suffix from vblk prefix

[Laszlo Ersek <lersek@redhat.com>]

On 09/08/16 00:59, Jordan Justen wrote:
> Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>

Thank you, pushed as commit d796d33f1844.

Laszlo

> On 2016-09-07 04:42:43, Laszlo Ersek wrote:
>> Translating QEMU's virtio-block OpenFirmware device path to a UEFI device
>> path prefix was one of the earliest case handled in QemuBootOrderLib. At
>> that time, I terminated the translation output (the UEFI devpath prefix)
>> with a "/HD(" suffix.
>>
>> The intent was for the translation to prefix-match only boot options with
>> HD() device path nodes in them, that is, no auto-generated "device level"
>> boot options. This was motivated by prioritizing specific boot options
>> created by OS installers over auto-generated "device level" options.
>>
>> However, practice has shown that:
>>
>> - OS installers place their installed boot options first in the boot order
>>   anyway,
>>
>> - other device types (SATA disks, virtio-scsi disks), where "/HD(" is not
>>   appended, work just fine,
>>
>> - requiring "/HD(" actually causes problems: after the OS-installed
>>   specific boot option has been lost (or purposely removed), the
>>   auto-generated "device level" boot option does the right thing (see the
>>   Default Boot Behavior under
>>   <http://blog.uncooperative.org/blog/2014/02/06/the-efi-system-partition/>).
>>   The "/HD(" requirement causes such boot options to be dropped, which
>>   prevents "fallback.efi" from running.
>>
>> Relax the matching by removing the "/HD(" suffix from the translated
>> prefix.
>>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Fixes: e06a4cd134064590aa1a855ff4b973023279e805
>> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1373812
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>
>> Notes:
>>     Public branch:
>>     <https://github.com/lersek/edk2/commits/drop_hdprefix_vblk_v1>
>>
>>  OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.c b/OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.c
>> index 86082301a8f5..8cbbdb0568fa 100644
>> --- a/OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.c
>> +++ b/OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.c
>> @@ -864,29 +864,29 @@ TranslatePciOfwNodes (
>>      // OpenFirmware device path (virtio-blk disk):
>>      //
>>      //   /pci@i0cf8/scsi@6[,3]/disk@0,0
>>      //        ^          ^  ^       ^ ^
>>      //        |          |  |       fixed
>>      //        |          |  PCI function corresponding to disk (optional)
>>      //        |          PCI slot holding disk
>>      //        PCI root at system bus port, PIO
>>      //
>>      // UEFI device path prefix:
>>      //
>> -    //   PciRoot(0x0)/Pci(0x6,0x0)/HD( -- if PCI function is 0 or absent
>> -    //   PciRoot(0x0)/Pci(0x6,0x3)/HD( -- if PCI function is present and nonzero
>> +    //   PciRoot(0x0)/Pci(0x6,0x0) -- if PCI function is 0 or absent
>> +    //   PciRoot(0x0)/Pci(0x6,0x3) -- if PCI function is present and nonzero
>>      //
>>      Written = UnicodeSPrintAsciiFormat (
>>        Translated,
>>        *TranslatedSize * sizeof (*Translated), // BufferSize in bytes
>> -      "PciRoot(0x%x)%s/Pci(0x%Lx,0x%Lx)/HD(",
>> +      "PciRoot(0x%x)%s/Pci(0x%Lx,0x%Lx)",
>>        PciRoot,
>>        Bridges,
>>        PciDevFun[0],
>>        PciDevFun[1]
>>        );
>>    } else if (NumNodes >= FirstNonBridge + 3 &&
>>               SubstringEq (OfwNode[FirstNonBridge + 0].DriverName, "scsi") &&
>>               SubstringEq (OfwNode[FirstNonBridge + 1].DriverName, "channel") &&
>>               SubstringEq (OfwNode[FirstNonBridge + 2].DriverName, "disk")
>>               ) {
>>      //
>> @@ -1109,28 +1109,28 @@ TranslateMmioOfwNodes (
>>        SubstringEq (OfwNode[1].DriverName, "disk")) {
>>      //
>>      // OpenFirmware device path (virtio-blk disk):
>>      //
>>      //   /virtio-mmio@000000000a003c00/disk@0,0
>>      //                ^                     ^ ^
>>      //                |                     fixed
>>      //                base address of virtio-mmio register block
>>      //
>>      // UEFI device path prefix:
>>      //
>> -    //   <VenHwString>/HD(
>> +    //   <VenHwString>
>>      //
>>      Written = UnicodeSPrintAsciiFormat (
>>                  Translated,
>>                  *TranslatedSize * sizeof (*Translated), // BufferSize in bytes
>> -                "%s/HD(",
>> +                "%s",
>>                  VenHwString
>>                  );
>>    } else if (NumNodes >= 3 &&
>>               SubstringEq (OfwNode[1].DriverName, "channel") &&
>>               SubstringEq (OfwNode[2].DriverName, "disk")) {
>>      //
>>      // OpenFirmware device path (virtio-scsi disk):
>>      //
>>      //   /virtio-mmio@000000000a003a00/channel@0/disk@2,3
>>      //                ^                        ^      ^ ^
>>      //                |                        |      | LUN
>> -- 
>> 2.9.2
>>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>