Re: [PATCH] MdeModulePkg/UefiBootManagerLib: Match the nested partitions

[Gary Lin <glin@suse.com>]

On Wed, Jan 23, 2019 at 03:49:16AM +0000, Ni, Ray wrote:
> 
> 
> > -----Original Message-----
> > From: Ni, Ray
> > Sent: Wednesday, January 23, 2019 11:48 AM
> > To: 'Gary Lin' <glin@suse.com>
> > Cc: edk2-devel@lists.01.org; Zeng, Star <star.zeng@intel.com>; Wang, Jian J
> > <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
> > Subject: RE: [PATCH] MdeModulePkg/UefiBootManagerLib: Match the
> > nested partitions
> > 
> > 
> > 
> > > -----Original Message-----
> > > From: Gary Lin <glin@suse.com>
> > > Sent: Wednesday, January 23, 2019 10:13 AM
> > > To: Ni, Ray <ray.ni@intel.com>
> > > Cc: edk2-devel@lists.01.org; Zeng, Star <star.zeng@intel.com>; Wang,
> > > Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
> > > Subject: Re: [PATCH] MdeModulePkg/UefiBootManagerLib: Match the
> > nested
> > > partitions
> > >
> > > On Tue, Jan 22, 2019 at 04:01:57PM +0000, Ni, Ray wrote:
> > > > (Send again. Previous mail might be lost.) Gary,
> > > Hi Ray,
> > >
> > > > I have two comments:
> > > > 1. Do you need to consider the case when the multiple partition
> > > > nodes
> > > might not be adjacent?
> > > In my cases, they are all partitions based on another partition, so
> > > all those nested partitions are adjacent to the "root" partition.
> > >
> > > I'm not sure if it's possible to create the node other than a nested
> > > partition node or a file node on a partition node.
> > Sure. Let's just assume the multiple partition nodes are adjacent.
> > 
> > >
> > > > 2. Maybe you could recursively call the function itself instead of
> > > > using a
> > > loop. In this new way, you might be able to save some code.
> > > Since my targets are the partition nodes after a partition, loop
> > > brings a bit better performance by skipping the non-partition node
> > > check and saves the overhead of function call.
> > > If you prefer recursive call, I'll modify the patch accordingly.
> > I just want to avoid having two code blocks to check device path node against
> > END and partition node.
> > First one is in original while(). Second one is introduced by this patch.
> 
> If we can avoid the duplicated code, using loop is also fine.
Ok. I'll see what I can do.

Thanks,

Gary Lin

> 
> > 
> > >
> > > Thanks,
> > >
> > > Gary Lin
> > >
> > > > > -----Original Message-----
> > > > > From: Gary Lin <glin@suse.com>
> > > > > Sent: Tuesday, January 15, 2019 5:46 PM
> > > > > To: edk2-devel@lists.01.org
> > > > > Cc: Ni, Ray <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>;
> > > > > Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > > > > <hao.a.wu@intel.com>
> > > > > Subject: [PATCH] MdeModulePkg/UefiBootManagerLib: Match the
> > > nested
> > > > > partitions
> > > > >
> > > > > In some cases, such as MD RAID1 in Linux, the bootloader may be in
> > > > > a nested EFI system partition partition. For example, sda1 and
> > > > > sdb1 are combined as md0 and the first partition of md0, md0p1, is
> > > > > an EFI system partition. Then, the bootloader can be located by
> > > > > the following device
> > > > > paths:
> > > > >
> > > > > PCI()/SATA(sda)/Partition(sda1)/Partition(md0p1)/File(bootloader.e
> > > > > fi
> > > > > )
> > > > > PCI()/SATA(sdb)/Partition(sdb1)/Partition(md0p1)/File(bootloader.e
> > > > > fi
> > > > > )
> > > > >
> > > > > To make the boot option more resilient, we may create a boot
> > > > > option with the short-form device path like
> > > "Partition(md0p1)/File(bootloader.efi)".
> > > > >
> > > > > However, BmMatchPartitionDevicePathNode() only matched the first
> > > > > partition node and ignored the nested partitions, so the firmware
> > > > > would refuse to load bootloader.efi since "Partition(md0p1)"
> > > > > doesn't match either "Partition(sda1)" or "Partition(sda2)".
> > > > >
> > > > > This commit modifies BmMatchPartitionDevicePathNode() to iterate
> > > > > all nested partitions so that the above boot option could work.
> > > > >
> > > > > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > > > > Cc: Star Zeng <star.zeng@intel.com>
> > > > > Cc: Jian J Wang <jian.j.wang@intel.com>
> > > > > Cc: Hao Wu <hao.a.wu@intel.com>
> > > > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > > > Signed-off-by: Gary Lin <glin@suse.com>
> > > > > ---
> > > > >  .../Library/UefiBootManagerLib/BmBoot.c       | 37 ++++++++++++------
> > -
> > > > >  1 file changed, 23 insertions(+), 14 deletions(-)
> > > > >
> > > > > diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> > > > > b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> > > > > index 6a23477eb873..8354c2af674b 100644
> > > > > --- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> > > > > +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> > > > > @@ -1995,21 +1995,30 @@ BmMatchPartitionDevicePathNode (
> > > > >      return FALSE;
> > > > >    }
> > > > >
> > > > > -  //
> > > > > -  // See if the harddrive device path in blockio matches the orig
> > > > > Hard Drive Node
> > > > > -  //
> > > > > -  Node = (HARDDRIVE_DEVICE_PATH *) BlockIoDevicePath;
> > > > > +  do {
> > > > > +    //
> > > > > +    // See if the harddrive device path in blockio matches the
> > > > > + orig Hard Drive
> > > > > Node
> > > > > +    //
> > > > > +    Node = (HARDDRIVE_DEVICE_PATH *) BlockIoDevicePath;
> > > > >
> > > > > -  //
> > > > > -  // Match Signature and PartitionNumber.
> > > > > -  // Unused bytes in Signature are initiaized with zeros.
> > > > > -  //
> > > > > -  return (BOOLEAN) (
> > > > > -    (Node->PartitionNumber == HardDriveDevicePath-
> > >PartitionNumber)
> > > &&
> > > > > -    (Node->MBRType == HardDriveDevicePath->MBRType) &&
> > > > > -    (Node->SignatureType == HardDriveDevicePath->SignatureType)
> > &&
> > > > > -    (CompareMem (Node->Signature, HardDriveDevicePath->Signature,
> > > sizeof
> > > > > (Node->Signature)) == 0)
> > > > > -    );
> > > > > +    //
> > > > > +    // Match Signature and PartitionNumber.
> > > > > +    // Unused bytes in Signature are initiaized with zeros.
> > > > > +    //
> > > > > +    if ((Node->PartitionNumber == HardDriveDevicePath-
> > > >PartitionNumber) &&
> > > > > +        (Node->MBRType == HardDriveDevicePath->MBRType) &&
> > > > > +        (Node->SignatureType ==
> > > > > + HardDriveDevicePath->SignatureType)
> > > &&
> > > > > +        (CompareMem (Node->Signature,
> > > > > + HardDriveDevicePath->Signature, sizeof
> > > > > (Node->Signature)) == 0)) {
> > > > > +      return TRUE;
> > > > > +    }
> > > > > +
> > > > > +    // See if a nested partition exists
> > > > > +    BlockIoDevicePath = NextDevicePathNode (BlockIoDevicePath);
> > > > > + } while (!IsDevicePathEnd (BlockIoDevicePath) &&
> > > > > +           (DevicePathType (BlockIoDevicePath) ==
> > > > > + MEDIA_DEVICE_PATH)
> > > &&
> > > > > +           (DevicePathSubType (BlockIoDevicePath) ==
> > > > > + MEDIA_HARDDRIVE_DP));
> > > > > +
> > > > > +  return FALSE;
> > > > >  }
> > > > >
> > > > >  /**
> > > > > --
> > > > > 2.20.1
> > > >
> > > >
>