Re: [PATCH 2/2] LegacyBbs: add boot entries for virtio-blk devices

Re: [PATCH 2/2] LegacyBbs: add boot entries for virtio-blk devices

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 6327 bytes --]

On Fri, 2013-01-25 at 22:04 +0100, Laszlo Ersek wrote:
> Contributed-under: TianoCore Contribution Agreement 1.0
> 
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>


Hm. This no longer works as-is because...

> ---
>  .../Csm/LegacyBiosDxe/LegacyBbs.c                  |  134 +++++++++++++++++++-
>  1 files changed, 133 insertions(+), 1 deletions(-)
> 
> diff --git a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBbs.c b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBbs.c
> index 6ee43ad..964d7d2 100644
> --- a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBbs.c
> +++ b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBbs.c
> @@ -259,8 +259,140 @@ LegacyBiosBuildBbs (
>      }
>    }
>  
> -  return EFI_SUCCESS;
> +  //
> +  // add virtio-blk devices
> +  //
> +  {
> +    EFI_STATUS Status;
> +    UINTN      NumPciIoHandles;
> +    EFI_HANDLE *PciIoHandles;
> +
> +    BbsIndex = HddIndex * 2 + 1;
> +
> +    //
> +    // scan all handles supporting the PCI IO protocol
> +    //
> +    Status = gBS->LocateHandleBuffer (
> +                    ByProtocol,
> +                    &gEfiPciIoProtocolGuid,
> +                    NULL,
> +                    &NumPciIoHandles,
> +                    &PciIoHandles
> +                    );
> +
> +    if (Status == EFI_SUCCESS) {
> +      UINTN CurPciIo;
> +
> +      for (CurPciIo = 0; CurPciIo < NumPciIoHandles; ++CurPciIo) {
> +        EFI_OPEN_PROTOCOL_INFORMATION_ENTRY *References;
> +        UINTN                               NumReferences;
> +        UINTN                               CurRef;
> +
> +        //
> +        // on each handle supporting the PCI IO protocol, see which drivers
> +        // (agents) have a PCI IO protocol interface actually opened
> +        //
> +        Status = gBS->OpenProtocolInformation (
> +                        PciIoHandles[CurPciIo],
> +                        &gEfiPciIoProtocolGuid,
> +                        &References,
> +                        &NumReferences
> +                        );
> +        if (EFI_ERROR (Status)) {
> +          continue;
> +        }
> +
> +        for (CurRef = 0; CurRef < NumReferences; ++CurRef) {
> +          if (References[CurRef].Attributes & EFI_OPEN_PROTOCOL_BY_DRIVER) {
> +            EFI_COMPONENT_NAME2_PROTOCOL *ComponentName;
> +            CHAR16                       *DriverName;
> +
> +            //
> +            // OpenProtocol() enforced non-NULL AgentHandle for this case
> +            //
> +            ASSERT (References[CurRef].AgentHandle != NULL);
> +
> +            //
> +            // Check if the agent owning the open protocol interface can
> +            // provide its name, and if so, whether it's VirtioBlkDxe. For this
> +            // check we don't want a persistent reference to the agent's
> +            // EFI_COMPONENT_NAME2_PROTOCOL instance, therefore we use
> +            // HandleProtocol() instead of OpenProtocol().
> +            //
> +            Status = gBS->HandleProtocol (
> +                            References[CurRef].AgentHandle,
> +                            &gEfiComponentName2ProtocolGuid,
> +                            (VOID **) &ComponentName
> +                            );
> +            if (EFI_ERROR (Status)) {
> +              continue;
> +            }
> +
> +            Status = ComponentName->GetDriverName (
> +                                      ComponentName,
> +                                      "en",
> +                                      &DriverName
> +                                      );
>  
> +            if (Status == EFI_SUCCESS &&
> +                StrCmp (DriverName, L"Virtio Block Driver") == 0) {
> +              break;



... this would need to match on 'Virtio PCI Driver' instead. Obviously
that's a trivial fix but it raises the question of whether this is the
right thing to be matching on at all.

It also doesn't catch NVMe drives. 

Should we just be iterating over all block devices or something like
that instead?

> +            }
> +          }
> +        }
> +
> +        if (CurRef < NumReferences) {
> +          EFI_PCI_IO_PROTOCOL *PciIo;
> +          UINTN               Segment;
> +          UINTN               Bus;
> +          UINTN               Device;
> +          UINTN               Function;
> +
> +          //
> +          // reference by VirtioBlkDxe found, produce boot entry for device
> +          //
> +          Status = gBS->HandleProtocol (
> +                          PciIoHandles[CurPciIo],
> +                          &gEfiPciIoProtocolGuid,
> +                          (VOID **) &PciIo
> +                          );
> +          ASSERT (Status == EFI_SUCCESS);
> +
> +          Status = PciIo->GetLocation (
> +                            PciIo,
> +                            &Segment,
> +                            &Bus,
> +                            &Device,
> +                            &Function
> +                            );
> +          ASSERT (Status == EFI_SUCCESS);
> +
> +          if (Segment == 0) {
> +            BbsTable[BbsIndex].Bus                      = (UINT32) Bus;
> +            BbsTable[BbsIndex].Device                   = (UINT32) Device;
> +            BbsTable[BbsIndex].Function                 = (UINT32) Function;
> +            BbsTable[BbsIndex].Class                    = 1;
> +            BbsTable[BbsIndex].SubClass                 = 0x80;
> +            BbsTable[BbsIndex].StatusFlags.OldPosition  = 0;
> +            BbsTable[BbsIndex].StatusFlags.Reserved1    = 0;
> +            BbsTable[BbsIndex].StatusFlags.Enabled      = 0;
> +            BbsTable[BbsIndex].StatusFlags.Failed       = 0;
> +            BbsTable[BbsIndex].StatusFlags.MediaPresent = 0;
> +            BbsTable[BbsIndex].StatusFlags.Reserved2    = 0;
> +            BbsTable[BbsIndex].DeviceType               = BBS_HARDDISK;
> +            BbsTable[BbsIndex].BootPriority             = BBS_UNPRIORITIZED_ENTRY;
> +            ++BbsIndex;
> +          }
> +        }
> +
> +        FreePool (References);
> +      }
> +
> +      FreePool (PciIoHandles);
> +    }
> +  }
> +
> +  return EFI_SUCCESS;
>  }
>  
>  


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

[PATCH 1/2] LegacyBios: set NumberBbsEntries to the size of BbsTable

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 830 bytes --]

Signed-off-by: David Woodhouse <dwmw2@infradead.org>
---
 OvmfPkg/Csm/LegacyBiosDxe/LegacyBios.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/OvmfPkg/Csm/LegacyBiosDxe/LegacyBios.c b/OvmfPkg/Csm/LegacyBiosDxe/LegacyBios.c
index 05e3ffd2bb..69abd06c40 100644
--- a/OvmfPkg/Csm/LegacyBiosDxe/LegacyBios.c
+++ b/OvmfPkg/Csm/LegacyBiosDxe/LegacyBios.c
@@ -568,7 +568,7 @@ ShadowAndStartLegacy16 (
   //
   // Skip Floppy and possible onboard IDE drives
   //
-  EfiToLegacy16BootTable->NumberBbsEntries = 1 + 2 * MAX_IDE_CONTROLLER;
+  EfiToLegacy16BootTable->NumberBbsEntries = sizeof(Private->IntThunk->BbsTable) / sizeof(BBS_TABLE);
 
   for (Index = 0; Index < (sizeof (Private->IntThunk->BbsTable) / sizeof (BBS_TABLE)); Index++) {
     BbsTable[Index].BootPriority = BBS_IGNORE_ENTRY;


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

[PATCH 2/2] LegacyBbs: Add boot entries for VirtIO and NVME devices

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 6511 bytes --]

Signed-off-by: David Woodhouse <dwmw2@infradead.org>
---

They still end up all just called 'Harddisk', but I absolutely do not
want to reproduce all the special cases in BmBootDescription.c. I'm not
even sure I want to export that and use it; it's horrid. Why don't the
disk objects themselves have a protocol which will generate a user-
visible label for them instead of collecting special-cases like that?

But that's just cosmetic. I can now do a CSM boot from VirtIO and NVMe
drives. At least, I can after
https://mail.coreboot.org/hyperkitty/list/seabios@seabios.org/thread/NR6Z4VTZA6VKF46RAFB3Q5TUE6ZLMLXT/

 OvmfPkg/Csm/LegacyBiosDxe/LegacyBbs.c | 162 +++++++++++++++++++++++++-
 1 file changed, 157 insertions(+), 5 deletions(-)

diff --git a/OvmfPkg/Csm/LegacyBiosDxe/LegacyBbs.c b/OvmfPkg/Csm/LegacyBiosDxe/LegacyBbs.c
index 6b1dd344f3..cc84712d25 100644
--- a/OvmfPkg/Csm/LegacyBiosDxe/LegacyBbs.c
+++ b/OvmfPkg/Csm/LegacyBiosDxe/LegacyBbs.c
@@ -140,10 +140,14 @@ LegacyBiosBuildBbs (
   IN  BBS_TABLE                 *BbsTable
   )
 {
-  UINTN     BbsIndex;
-  HDD_INFO  *HddInfo;
-  UINTN     HddIndex;
-  UINTN     Index;
+  UINTN       BbsIndex;
+  HDD_INFO    *HddInfo;
+  UINTN       HddIndex;
+  UINTN       Index;
+  EFI_HANDLE  *BlockIoHandles;
+  UINTN       NumberBlockIoHandles;
+  UINTN       BlockIndex;
+  EFI_STATUS  Status;
 
   //
   // First entry is floppy.
@@ -252,8 +256,156 @@ LegacyBiosBuildBbs (
     }
   }
 
-  return EFI_SUCCESS;
+  //
+  // Add non-IDE block devices
+  //
+  BbsIndex = HddIndex * 2 + 1;
+
+  Status = gBS->LocateHandleBuffer (
+                  ByProtocol,
+                  &gEfiBlockIoProtocolGuid,
+                  NULL,
+                  &NumberBlockIoHandles,
+                  &BlockIoHandles
+                  );
+  if (!EFI_ERROR(Status)) {
+    UINTN                     Removable;
+    EFI_BLOCK_IO_PROTOCOL     *BlkIo;
+    EFI_PCI_IO_PROTOCOL       *PciIo;
+    EFI_DEVICE_PATH_PROTOCOL  *DevicePath;
+    EFI_DEVICE_PATH_PROTOCOL  *DevicePathNode;
+    EFI_HANDLE                PciHandle;
+    UINTN                     SegNum;
+    UINTN                     BusNum;
+    UINTN                     DevNum;
+    UINTN                     FuncNum;
+
+    for (Removable = 0; Removable < 2; Removable++) {
+      for (BlockIndex = 0; BlockIndex < NumberBlockIoHandles; BlockIndex++) {
+        Status = gBS->HandleProtocol (
+                        BlockIoHandles[BlockIndex],
+                        &gEfiBlockIoProtocolGuid,
+                        (VOID **) &BlkIo
+                        );
+        if (EFI_ERROR (Status)) {
+          continue;
+        }
 
+        //
+        // Skip the logical partitions
+        //
+        if (BlkIo->Media->LogicalPartition) {
+          DEBUG((EFI_D_INFO, "Partition\n"));
+          continue;
+        }
+
+        //
+        // Skip the fixed block io then the removable block io
+        //
+        if (BlkIo->Media->RemovableMedia == ((Removable == 0) ? FALSE : TRUE)) {
+          continue;
+        }
+
+        //
+        // Get Device Path
+        //
+        Status = gBS->HandleProtocol (
+                        BlockIoHandles[BlockIndex],
+                        &gEfiDevicePathProtocolGuid,
+                        (VOID **) &DevicePath
+                        );
+        if (EFI_ERROR (Status)) {
+          continue;
+        }
+
+        //
+        // Skip ATA devices as they have already been handled
+        //
+        DevicePathNode = DevicePath;
+        while (!IsDevicePathEnd (DevicePathNode)) {
+          if (DevicePathType (DevicePathNode) == MESSAGING_DEVICE_PATH &&
+              DevicePathSubType (DevicePathNode) == MSG_ATAPI_DP) {
+            break;
+          }
+          DevicePathNode = NextDevicePathNode (DevicePathNode);
+        }
+        if (!IsDevicePathEnd (DevicePathNode)) {
+            continue;
+        }
+
+        //
+        //  Locate which PCI device
+        //
+        Status = gBS->LocateDevicePath (
+                        &gEfiPciIoProtocolGuid,
+                        &DevicePath,
+                        &PciHandle
+                        );
+        if (EFI_ERROR (Status)) {
+          continue;
+        }
+
+        Status = gBS->HandleProtocol (
+                        PciHandle,
+                        &gEfiPciIoProtocolGuid,
+                        (VOID **) &PciIo
+                        );
+        if (EFI_ERROR (Status)) {
+          continue;
+        }
+
+        Status = PciIo->GetLocation (
+                          PciIo,
+                          &SegNum,
+                          &BusNum,
+                          &DevNum,
+                          &FuncNum
+                          );
+        if (EFI_ERROR (Status)) {
+          continue;
+        }
+
+        if (SegNum != 0) {
+          DEBUG((EFI_D_INFO, "CSM cannot use PCI devices in segment %d\n", SegNum));
+          continue;
+        }
+
+        DEBUG_CODE (
+          CHAR16 *PathText;
+
+          PathText = ConvertDevicePathToText(DevicePath, FALSE, FALSE);
+
+          DEBUG((EFI_D_INFO, "Add Legacy Bbs entry for PCI %d/%d/%d: %s\n",
+                 BusNum, DevNum, FuncNum, PathText));
+          FreePool(PathText);
+        );
+
+        BbsTable[BbsIndex].Bus                      = BusNum;
+        BbsTable[BbsIndex].Device                   = DevNum;
+        BbsTable[BbsIndex].Function                 = FuncNum;
+        BbsTable[BbsIndex].Class                    = 1;
+        BbsTable[BbsIndex].SubClass                 = 0x80;
+        BbsTable[BbsIndex].StatusFlags.OldPosition  = 0;
+        BbsTable[BbsIndex].StatusFlags.Reserved1    = 0;
+        BbsTable[BbsIndex].StatusFlags.Enabled      = 0;
+        BbsTable[BbsIndex].StatusFlags.Failed       = 0;
+        BbsTable[BbsIndex].StatusFlags.MediaPresent = 0;
+        BbsTable[BbsIndex].StatusFlags.Reserved2    = 0;
+        BbsTable[BbsIndex].DeviceType               = BBS_HARDDISK;
+        BbsTable[BbsIndex].BootPriority             = BBS_UNPRIORITIZED_ENTRY;
+        BbsIndex++;
+
+        if (BbsIndex == sizeof(Private->IntThunk->BbsTable) / sizeof(BBS_TABLE)) {
+          Removable = 2;
+          break;
+        }
+      }
+    }
+
+    FreePool (BlockIoHandles);
+  }
+
+  return EFI_SUCCESS;
 }
 
 


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

Re: [edk2-devel] [PATCH 1/2] LegacyBios: set NumberBbsEntries to the size of BbsTable

[Laszlo Ersek]

On 06/19/19 14:44, David Woodhouse wrote:
> Signed-off-by: David Woodhouse <dwmw2@infradead.org>
> ---
>  OvmfPkg/Csm/LegacyBiosDxe/LegacyBios.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/Csm/LegacyBiosDxe/LegacyBios.c b/OvmfPkg/Csm/LegacyBiosDxe/LegacyBios.c
> index 05e3ffd2bb..69abd06c40 100644
> --- a/OvmfPkg/Csm/LegacyBiosDxe/LegacyBios.c
> +++ b/OvmfPkg/Csm/LegacyBiosDxe/LegacyBios.c
> @@ -568,7 +568,7 @@ ShadowAndStartLegacy16 (
>    //
>    // Skip Floppy and possible onboard IDE drives
>    //
> -  EfiToLegacy16BootTable->NumberBbsEntries = 1 + 2 * MAX_IDE_CONTROLLER;
> +  EfiToLegacy16BootTable->NumberBbsEntries = sizeof(Private->IntThunk->BbsTable) / sizeof(BBS_TABLE);
>  
>    for (Index = 0; Index < (sizeof (Private->IntThunk->BbsTable) / sizeof (BBS_TABLE)); Index++) {
>      BbsTable[Index].BootPriority = BBS_IGNORE_ENTRY;
> 
> 
> 
> 

If it's possible, please write a non-empty commit message body, for each
patch in this series. Just one sentence on this patch would be nice.

I think your note on patch#2 is valuable too and should be captured in
the commit message body. Please consider formulating it with a bit more
neutral tone :)

With those updates, for these pair of patches:

Acked-by: Laszlo Ersek <lersek@redhat.com>


If you could submit your "csm" branch as a 3-part series (consisting of
these two patches, plus "OvmfPkg: Don't build in QemuVideoDxe when we
have CSM"; with my A-b's / R-b added), that would be great, I could
apply & push it in one go.

If you prefer an actual pullreq (not github pull req), we can "pioneer"
that (for edk2 anyway) too. It will still require a posting to the list,
and then I'll have to compare the commits in the topic branch (subject
to the pull request) against the patches on the list.

Thanks!
Laszlo

Re: [edk2-devel] [PATCH 1/2] LegacyBios: set NumberBbsEntries to the size of BbsTable

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 950 bytes --]

On Thu, 2019-06-20 at 18:12 +0200, Laszlo Ersek wrote:
> If it's possible, please write a non-empty commit message body, for each
> patch in this series. Just one sentence on this patch would be nice.


The comnit comment isn't empty. It has one sentence "LegacyBios: set
NumberBbsEntries to the size of BbsTable".

A second sentence would be redundant and render the description larger
than the patch itself. I can't think of anything useful I'd want to put
in it. Apparently neither could you when you posted the same patch in
2013 after splitting it out from a larger patch of mine. :)

https://www.mail-archive.com/edk2-devel@lists.sourceforge.net/msg01618.html

> I think your note on patch#2 is valuable too and should be captured in
> the commit message body. Please consider formulating it with a bit more
> neutral tone :)

I would prefer to express that particular concern in 'diff -up' form
when actually fixing it :)

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

Re: [edk2-devel] [PATCH 1/2] LegacyBios: set NumberBbsEntries to the size of BbsTable

[Laszlo Ersek]

On 06/20/19 19:32, David Woodhouse wrote:
> On Thu, 2019-06-20 at 18:12 +0200, Laszlo Ersek wrote:
>> If it's possible, please write a non-empty commit message body, for each
>> patch in this series. Just one sentence on this patch would be nice.
> 
> 
> The comnit comment isn't empty. It has one sentence "LegacyBios: set
> NumberBbsEntries to the size of BbsTable".

I was careful to write, "non-empty commit message *body*".

Yes, you have a subject line, and if you look at the commit with "git
show", or another tool that shows the subject (= commit "title") near
the body, things look fine. Things read pretty weird in Thunderbird
however, for example.

> A second sentence would be redundant and render the description larger
> than the patch itself. I can't think of anything useful I'd want to put
> in it. Apparently neither could you when you posted the same patch in
> 2013 after splitting it out from a larger patch of mine. :)
> 
> https://www.mail-archive.com/edk2-devel@lists.sourceforge.net/msg01618.html

My past mistake doesn't excuse the current commit message ;)

Anyway, I intentionally didn't ask for just repeating the commit msg
title in the commit msg body. You could mention why the pre-patch
expression (1 + 2 * MAX_IDE_CONTROLLER) is wrong, or wasteful. It's not
obvious at all from just the patch itself (without looking at the larger
context in the source file). You can save time for reviewers by giving
hints in the commit message. And if you *can* save time for reviewers,
you should. :)

>> I think your note on patch#2 is valuable too and should be captured in
>> the commit message body. Please consider formulating it with a bit more
>> neutral tone :)
> 
> I would prefer to express that particular concern in 'diff -up' form
> when actually fixing it :)
> 

I'm a big fan of detailed commit messages. It's OK (and welcome) to
describe current shortcomings even if you intend to fix them later.

The commit message could also capture in one or two sentences what the
patch does (locate handles with BlockIo instances, filter out some of
them based on their DevicePath instances and other criteria, then for
each handle, take the PCI B/D/F from the most specific PciIo instance on
the device path). Anyway, I wouldn't like to obsess about this.

series
Acked-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

Re: [edk2-devel] [PATCH 1/2] LegacyBios: set NumberBbsEntries to the size of BbsTable

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1774 bytes --]

On Thu, 2019-06-20 at 22:35 +0200, Laszlo Ersek wrote:
> > > I think your note on patch#2 is valuable too and should be captured in
> > > the commit message body. Please consider formulating it with a bit more
> > > neutral tone :)
> > 
> > I would prefer to express that particular concern in 'diff -up' form
> > when actually fixing it :)
> > 
> 
> I'm a big fan of detailed commit messages. It's OK (and welcome) to
> describe current shortcomings even if you intend to fix them later.

It isn't strictly a shortcoming of that patch itself, it's more of a
pre-existing shortcoming which is next on my list.

But keen-eyed reviewers or testers might potentially have said "It's
all very well exposing all these new drives but they're all just called
Harddisk". Mentioning it in the note was partly an attempt to avoid
that, but mostly an attempt to solicit discussion on how to *fix* it.

Since it appears to have completely failed to achieve the latter, I'll
try again :)

As noted, UefiBootManagerLib already has this BmGetBootDescription()
function, which is internal. Can I just rename it to
EfiBootManagerGetBootDescription() and export it without having to
change any specifications first?


Adding a generic way for block devices to report a human-readable
description in order to kill off all the device-type-specific functions
in BmBootDescription.c presumably *would* involve actually coordinating
with UEFI Specifications first?

But we could consider that a second step. If I make the LegacyBm code
just call the existing (but renamed) EfiBootManagerGetBootDescription()
then all the horrid special cases and the specification work that's
required to fix them are purely an implementation detail in
EfiBootManagerLib? 


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

Re: [edk2-devel] [PATCH 1/2] LegacyBios: set NumberBbsEntries to the size of BbsTable

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1797 bytes --]

On Thu, 2019-06-20 at 22:35 +0200, Laszlo Ersek wrote:
> My past mistake doesn't excuse the current commit message ;)
> 
> Anyway, I intentionally didn't ask for just repeating the commit msg
> title in the commit msg body. You could mention why the pre-patch
> expression (1 + 2 * MAX_IDE_CONTROLLER) is wrong, or wasteful. It's not
> obvious at all from just the patch itself (without looking at the larger
> context in the source file). You can save time for reviewers by giving
> hints in the commit message. And if you *can* save time for reviewers,
> you should. :)

With specific requests for things that weren't already in the subject
line, I'm perfectly happy to add more.

Better now?

> > > I think your note on patch#2 is valuable too and should be captured in
> > > the commit message body. Please consider formulating it with a bit more
> > > neutral tone :)
> > 
> > I would prefer to express that particular concern in 'diff -up' form
> > when actually fixing it :)
> > 
> 
> I'm a big fan of detailed commit messages. It's OK (and welcome) to
> describe current shortcomings even if you intend to fix them later.
> 
> The commit message could also capture in one or two sentences what the
> patch does (locate handles with BlockIo instances, filter out some of
> them based on their DevicePath instances and other criteria, then for
> each handle, take the PCI B/D/F from the most specific PciIo instance on
> the device path). Anyway, I wouldn't like to obsess about this.

Done.

> series
> Acked-by: Laszlo Ersek <lersek@redhat.com>

Out of interest, why Acked-by: for these two vs. Reviewed-by: for the
first one? It's all under OvmfPkg now (which is made clearer in the
series I posted on Friday than it is in the subject line here).

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

Re: [edk2-devel] [PATCH 1/2] LegacyBios: set NumberBbsEntries to the size of BbsTable

[Laszlo Ersek]

On 06/24/19 13:48, David Woodhouse wrote:
> On Thu, 2019-06-20 at 22:35 +0200, Laszlo Ersek wrote:
>> My past mistake doesn't excuse the current commit message ;)
>>
>> Anyway, I intentionally didn't ask for just repeating the commit msg
>> title in the commit msg body. You could mention why the pre-patch
>> expression (1 + 2 * MAX_IDE_CONTROLLER) is wrong, or wasteful. It's not
>> obvious at all from just the patch itself (without looking at the larger
>> context in the source file). You can save time for reviewers by giving
>> hints in the commit message. And if you *can* save time for reviewers,
>> you should. :)
> 
> With specific requests for things that weren't already in the subject
> line, I'm perfectly happy to add more.
> 
> Better now?
> 
>>>> I think your note on patch#2 is valuable too and should be captured in
>>>> the commit message body. Please consider formulating it with a bit more
>>>> neutral tone :)
>>>
>>> I would prefer to express that particular concern in 'diff -up' form
>>> when actually fixing it :)
>>>
>>
>> I'm a big fan of detailed commit messages. It's OK (and welcome) to
>> describe current shortcomings even if you intend to fix them later.
>>
>> The commit message could also capture in one or two sentences what the
>> patch does (locate handles with BlockIo instances, filter out some of
>> them based on their DevicePath instances and other criteria, then for
>> each handle, take the PCI B/D/F from the most specific PciIo instance on
>> the device path). Anyway, I wouldn't like to obsess about this.
> 
> Done.

Thanks!

>> series
>> Acked-by: Laszlo Ersek <lersek@redhat.com>
> 
> Out of interest, why Acked-by: for these two vs. Reviewed-by: for the
> first one? It's all under OvmfPkg now (which is made clearer in the
> series I posted on Friday than it is in the subject line here).
> 

For me anyway, R-b means a (reasonably) deep understanding & agreement.
Acked-by means "looks okay, or at least I'm not worried about
regressions in this area -- I'll let you do whatever you need to do". :)

Thanks
Laszlo

Re: [edk2-devel] [PATCH 1/2] LegacyBios: set NumberBbsEntries to the size of BbsTable

[Laszlo Ersek]

On 06/21/19 12:59, David Woodhouse wrote:
> On Thu, 2019-06-20 at 22:35 +0200, Laszlo Ersek wrote:
>>>> I think your note on patch#2 is valuable too and should be captured in
>>>> the commit message body. Please consider formulating it with a bit more
>>>> neutral tone :)
>>>
>>> I would prefer to express that particular concern in 'diff -up' form
>>> when actually fixing it :)
>>>
>>
>> I'm a big fan of detailed commit messages. It's OK (and welcome) to
>> describe current shortcomings even if you intend to fix them later.
> 
> It isn't strictly a shortcoming of that patch itself, it's more of a
> pre-existing shortcoming which is next on my list.
> 
> But keen-eyed reviewers or testers might potentially have said "It's
> all very well exposing all these new drives but they're all just called
> Harddisk". Mentioning it in the note was partly an attempt to avoid
> that, but mostly an attempt to solicit discussion on how to *fix* it.
> 
> Since it appears to have completely failed to achieve the latter, I'll
> try again :)
> 
> As noted, UefiBootManagerLib already has this BmGetBootDescription()
> function, which is internal. Can I just rename it to
> EfiBootManagerGetBootDescription() and export it without having to
> change any specifications first?

I would say "yes", and we certainly have precedent for that. Please see
commit 4ed2440d4415 ("MdeModulePkg/UefiBootManagerLib: Expose
*GetLoadOptionBuffer() API", 2016-05-04).

> Adding a generic way for block devices to report a human-readable
> description in order to kill off all the device-type-specific functions
> in BmBootDescription.c presumably *would* involve actually coordinating
> with UEFI Specifications first?
> 
> But we could consider that a second step. If I make the LegacyBm code
> just call the existing (but renamed) EfiBootManagerGetBootDescription()
> then all the horrid special cases and the specification work that's
> required to fix them are purely an implementation detail in
> EfiBootManagerLib? 

I think exposing EfiBootManagerGetBootDescription() as a public
function, as-is, is a no-brainer, if platforms need it.

*Changing* EfiBootManagerGetBootDescription() is hairier.
UefiBootManagerLib strives for strict spec compliance (and minimalism),
if I remember correctly. However, I'm not a big fan of that approach
myself, and recently, "extend first, standardize second" has seemed more
accepted/tolerated than before. (I'm an active proponent of this latter
approach.)

A new hook into PlatformBootManagerLib might help, either way. Please
see TianoCore#982, and commit range cef7ecf6cdb4..1010873becc5.

So, please ask Ray (CC'd) :)

Thanks
Laszlo

Re: [edk2-devel] [PATCH 1/2] LegacyBios: set NumberBbsEntries to the size of BbsTable

[Laszlo Ersek]

On 06/25/19 00:08, Laszlo Ersek wrote:
> On 06/21/19 12:59, David Woodhouse wrote:

>> Adding a generic way for block devices to report a human-readable
>> description in order to kill off all the device-type-specific functions
>> in BmBootDescription.c presumably *would* involve actually coordinating
>> with UEFI Specifications first?
>>
>> But we could consider that a second step. If I make the LegacyBm code
>> just call the existing (but renamed) EfiBootManagerGetBootDescription()
>> then all the horrid special cases and the specification work that's
>> required to fix them are purely an implementation detail in
>> EfiBootManagerLib? 
> 
> I think exposing EfiBootManagerGetBootDescription() as a public
> function, as-is, is a no-brainer, if platforms need it.
> 
> *Changing* EfiBootManagerGetBootDescription() is hairier.
> UefiBootManagerLib strives for strict spec compliance (and minimalism),
> if I remember correctly. However, I'm not a big fan of that approach
> myself, and recently, "extend first, standardize second" has seemed more
> accepted/tolerated than before. (I'm an active proponent of this latter
> approach.)
> 
> A new hook into PlatformBootManagerLib might help, either way. Please
> see TianoCore#982, and commit range cef7ecf6cdb4..1010873becc5.
> 
> So, please ask Ray (CC'd) :)

Wait, we already have EfiBootManagerRegisterBootDescriptionHandler().
Could that help?

Thanks,
Laszlo

Re: [edk2-devel] [PATCH 1/2] LegacyBios: set NumberBbsEntries to the size of BbsTable

[Ni, Ray]

Can you obtain the description from the NV storage?

> -----Original Message-----
> From: David Woodhouse <dwmw2@infradead.org>
> Sent: Tuesday, June 25, 2019 2:06 PM
> To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io;
> jljusten@gmail.com; afish@apple.com; Paolo Bonzini <pbonzini@redhat.com>
> Cc: Ni, Ray <ray.ni@intel.com>
> Subject: Re: [edk2-devel] [PATCH 1/2] LegacyBios: set NumberBbsEntries to
> the size of BbsTable
> 
> 
> 
> On 24 June 2019 23:22:03 BST, Laszlo Ersek <lersek@redhat.com> wrote:
> >Wait, we already have EfiBootManagerRegisterBootDescriptionHandler().
> >Could that help?
> 
> No, that allows you to register a function to *provide* a description. We
> need to *obtain* a description.
> 
> See the patch series I posted on Friday; I think I have this working sanely now.

Re: [edk2-devel] [PATCH 1/2] LegacyBios: set NumberBbsEntries to the size of BbsTable

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 652 bytes --]

On Tue, 2019-06-25 at 00:08 +0200, Laszlo Ersek wrote:
> > But we could consider that a second step. If I make the LegacyBm code
> > just call the existing (but renamed) EfiBootManagerGetBootDescription()
> > then all the horrid special cases and the specification work that's
> > required to fix them are purely an implementation detail in
> > EfiBootManagerLib? 
> 
> I think exposing EfiBootManagerGetBootDescription() as a public
> function, as-is, is a no-brainer, if platforms need it.
> 
> *Changing* EfiBootManagerGetBootDescription() is hairier.

It isn't exported yet, so I wouldn't be changing an existing exported
function.


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

Re: [edk2-devel] [PATCH 1/2] LegacyBios: set NumberBbsEntries to the size of BbsTable

[Laszlo Ersek]

On 06/25/19 09:06, Ni, Ray wrote:
> Can you obtain the description from the NV storage?

No, nothing carries persistent (data-like) description for these
devices. IIUC we just want to reuse the description generator logic (=
code) from UefiBootManagerLib, with some customization.

Thanks
Laszlo

> 
>> -----Original Message-----
>> From: David Woodhouse <dwmw2@infradead.org>
>> Sent: Tuesday, June 25, 2019 2:06 PM
>> To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io;
>> jljusten@gmail.com; afish@apple.com; Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Ni, Ray <ray.ni@intel.com>
>> Subject: Re: [edk2-devel] [PATCH 1/2] LegacyBios: set NumberBbsEntries to
>> the size of BbsTable
>>
>>
>>
>> On 24 June 2019 23:22:03 BST, Laszlo Ersek <lersek@redhat.com> wrote:
>>> Wait, we already have EfiBootManagerRegisterBootDescriptionHandler().
>>> Could that help?
>>
>> No, that allows you to register a function to *provide* a description. We
>> need to *obtain* a description.
>>
>> See the patch series I posted on Friday; I think I have this working sanely now.