On Tue, 2019-06-25 at 09:56 +0000, Ni, Ray wrote:
> David,
> Thanks for pointing that patch.
> 
> Now I understand it.
> Normally it's the CSM16 code that builds the boot descriptions for legacy boot options
> and LegacyBootManagerLib consumes that boot descriptions.
> 
> But in your case, LegacyBios driver builds the boot descriptions for VirtIo devices and
> LegacyBootManagerLib consumes that boot descriptions.
> 
> The flow is like below: (I understand there is no VirtIoBootOptionDescriptionHandler() in
> your current patch. But I think we could write such handler and register it through
> EfiBootManagerRegisterBootDescriptionHandler() API)
> 
>   *module*          *UefiBootManagerLib*
> LegacyBios -> GetBootOptionDescription() -> VirtIoBootOptionDescriptionHandler()
> BdsDxe -> GetBootOptionDescription() -> VirtIoBootOptionDescriptionHandler()
> 
> So instead of routing to *UefiBootManagerLib* GetBootOptionDescription(),
> why not you directly call VirtIoBootOptionDescriptionHandler() from LegacyBios?
> 
> I understand from functionality perspective, it makes no difference.
> But I care about that because it avoids LegacyBios unnecessarily depends on
> *UefiBootManagerLib*.
> 
> What do you think?

Forget VirtIO, look only at the previous two patches.

    MdeModulePkg/UefiBootManagerLib: export EfiBootManagerGetBootDescription()
    OvmfPkg/LegacyBiosDxe: Use EfiBootManagerGetBootDescription()

Their commit messages (attempt to) set this out fairly clearly.

In BmGetBootDescription() we already have a whole pile of special cases
for things including NVMe, USB, etc. to get descriptive strings that
match a given BlockIo handle.

It's bad enough that that functionality exists in that form already,
rather than being provided in a clean and generic fashion by the block
device driver itself somehow.

I absolutely did not want to reproduce it all, just to get meaningful
strings for the Legacy boot targets from the same block devices. Hence
a tweaking, renaming and exporting the existing BmGetBootDescription()
function so I can use it from LegacyBbs code.

Adding another special case for VirtIO alongside all the other special
cases that were already in BmGetBootDescription (now called
EfiBootManagerGetBootDescription()) is not really the important part
here.


Is there a particular problem with having LegacyBios depend on
UefiBootManagerLib? The code in BmDescription.c is actually fairly
standalone — should we move it out into its *own* library so that it
can be used from both places? That seems like overkill to me, when our
long term plan really ought to be to kill it with fire and let block
device drivers provide their own descriptive strings through a standard
protocol.