public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH] MdeModulePkg/UefiBootManagerLib: Match the nested partitions
@ 2019-01-15  9:45 Gary Lin
  2019-01-15 11:58 ` Laszlo Ersek
  2019-01-22 16:01 ` Ni, Ray
  0 siblings, 2 replies; 9+ messages in thread
From: Gary Lin @ 2019-01-15  9:45 UTC (permalink / raw)
  To: edk2-devel; +Cc: Ruiyu Ni, Star Zeng, Jian J Wang, Hao Wu

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.efi)
PCI()/SATA(sdb)/Partition(sdb1)/Partition(md0p1)/File(bootloader.efi)

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



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] MdeModulePkg/UefiBootManagerLib: Match the nested partitions
  2019-01-15  9:45 [PATCH] MdeModulePkg/UefiBootManagerLib: Match the nested partitions Gary Lin
@ 2019-01-15 11:58 ` Laszlo Ersek
  2019-01-16  2:16   ` Gary Lin
  2019-01-22 16:01 ` Ni, Ray
  1 sibling, 1 reply; 9+ messages in thread
From: Laszlo Ersek @ 2019-01-15 11:58 UTC (permalink / raw)
  To: Gary Lin, edk2-devel; +Cc: Ruiyu Ni, Hao Wu, Star Zeng

On 01/15/19 10:45, Gary Lin wrote:
> 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.efi)
> PCI()/SATA(sdb)/Partition(sdb1)/Partition(md0p1)/File(bootloader.efi)

How does edk2 recognize the nested partition md0p1 in the first place?

I would assume that the "outer" partitions (sda1, sdb1) start with some
kind of MD RAID1 header that allows Linux to combine sda1 and sdb1 into
md0p1. How does edk2 get past that header?

Hmmm... based on
<https://raid.wiki.kernel.org/index.php/RAID_superblock_formats>, does
Linux use "footers" instead of "headers"?

Thanks,
Laszlo

> 
> 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;
>  }
>  
>  /**
> 



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] MdeModulePkg/UefiBootManagerLib: Match the nested partitions
  2019-01-15 11:58 ` Laszlo Ersek
@ 2019-01-16  2:16   ` Gary Lin
  2019-01-16  9:02     ` Laszlo Ersek
  0 siblings, 1 reply; 9+ messages in thread
From: Gary Lin @ 2019-01-16  2:16 UTC (permalink / raw)
  To: Laszlo Ersek; +Cc: edk2-devel, Ruiyu Ni, Hao Wu, Star Zeng

On Tue, Jan 15, 2019 at 12:58:10PM +0100, Laszlo Ersek wrote:
> On 01/15/19 10:45, Gary Lin wrote:
> > 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.efi)
> > PCI()/SATA(sdb)/Partition(sdb1)/Partition(md0p1)/File(bootloader.efi)
> 
> How does edk2 recognize the nested partition md0p1 in the first place?
> 
> I would assume that the "outer" partitions (sda1, sdb1) start with some
> kind of MD RAID1 header that allows Linux to combine sda1 and sdb1 into
> md0p1. How does edk2 get past that header?
> 
> Hmmm... based on
> <https://raid.wiki.kernel.org/index.php/RAID_superblock_formats>, does
> Linux use "footers" instead of "headers"?
> 
See the "Sub-versions of the version-1 superblock" section:
<https://raid.wiki.kernel.org/index.php/RAID_superblock_formats#Sub-versions_of_the_version-1_superblock>

For MD 0.9 and 1.0, the metadata is stored in the end of the disk, and
those partitions, nested or not, are just like the normal partitions, so
the firmare can see them without the knowledge of MD metadata. MD 1.1
and 1.2 would be a different story because those two use "headers".

Anyway, I have tested RAID1 with MD 1.0 and OVMF actually appended md0p1
in the device path. I only need to iterate the nested partitions in the
match function to make my boot option work.

Thanks,

Gary Lin

> Thanks,
> Laszlo
> 
> > 
> > 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;
> >  }
> >  
> >  /**
> > 
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] MdeModulePkg/UefiBootManagerLib: Match the nested partitions
  2019-01-16  2:16   ` Gary Lin
@ 2019-01-16  9:02     ` Laszlo Ersek
  0 siblings, 0 replies; 9+ messages in thread
From: Laszlo Ersek @ 2019-01-16  9:02 UTC (permalink / raw)
  To: Gary Lin; +Cc: edk2-devel, Ruiyu Ni, Hao Wu, Star Zeng

On 01/16/19 03:16, Gary Lin wrote:
> On Tue, Jan 15, 2019 at 12:58:10PM +0100, Laszlo Ersek wrote:
>> On 01/15/19 10:45, Gary Lin wrote:
>>> 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.efi)
>>> PCI()/SATA(sdb)/Partition(sdb1)/Partition(md0p1)/File(bootloader.efi)
>>
>> How does edk2 recognize the nested partition md0p1 in the first place?
>>
>> I would assume that the "outer" partitions (sda1, sdb1) start with some
>> kind of MD RAID1 header that allows Linux to combine sda1 and sdb1 into
>> md0p1. How does edk2 get past that header?
>>
>> Hmmm... based on
>> <https://raid.wiki.kernel.org/index.php/RAID_superblock_formats>, does
>> Linux use "footers" instead of "headers"?
>>
> See the "Sub-versions of the version-1 superblock" section:
> <https://raid.wiki.kernel.org/index.php/RAID_superblock_formats#Sub-versions_of_the_version-1_superblock>
> 
> For MD 0.9 and 1.0, the metadata is stored in the end of the disk, and
> those partitions, nested or not, are just like the normal partitions, so
> the firmare can see them without the knowledge of MD metadata. MD 1.1
> and 1.2 would be a different story because those two use "headers".
> 
> Anyway, I have tested RAID1 with MD 1.0 and OVMF actually appended md0p1
> in the device path. I only need to iterate the nested partitions in the
> match function to make my boot option work.

Thanks for the explanation! I'll let Ray review the change.
Laszlo

>>>
>>> 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;
>>>  }
>>>  
>>>  /**
>>>
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
>>



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] MdeModulePkg/UefiBootManagerLib: Match the nested partitions
  2019-01-15  9:45 [PATCH] MdeModulePkg/UefiBootManagerLib: Match the nested partitions Gary Lin
  2019-01-15 11:58 ` Laszlo Ersek
@ 2019-01-22 16:01 ` Ni, Ray
  2019-01-23  2:12   ` Gary Lin
  1 sibling, 1 reply; 9+ messages in thread
From: Ni, Ray @ 2019-01-22 16:01 UTC (permalink / raw)
  To: 'Gary Lin', edk2-devel@lists.01.org
  Cc: Zeng, Star, Wang, Jian J, Wu, Hao A

(Send again. Previous mail might be lost.)
Gary,
I have two comments:
1. Do you need to consider the case when the multiple partition nodes might not be 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.

> -----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.efi)
> PCI()/SATA(sdb)/Partition(sdb1)/Partition(md0p1)/File(bootloader.efi)
> 
> 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



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] MdeModulePkg/UefiBootManagerLib: Match the nested partitions
  2019-01-22 16:01 ` Ni, Ray
@ 2019-01-23  2:12   ` Gary Lin
  2019-01-23  3:48     ` Ni, Ray
  0 siblings, 1 reply; 9+ messages in thread
From: Gary Lin @ 2019-01-23  2:12 UTC (permalink / raw)
  To: Ni, Ray; +Cc: edk2-devel@lists.01.org, Zeng, Star, Wang, Jian J, Wu, Hao A

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.

> 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.

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.efi)
> > PCI()/SATA(sdb)/Partition(sdb1)/Partition(md0p1)/File(bootloader.efi)
> > 
> > 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
> 
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] MdeModulePkg/UefiBootManagerLib: Match the nested partitions
  2019-01-23  2:12   ` Gary Lin
@ 2019-01-23  3:48     ` Ni, Ray
  2019-01-23  3:49       ` Ni, Ray
  0 siblings, 1 reply; 9+ messages in thread
From: Ni, Ray @ 2019-01-23  3:48 UTC (permalink / raw)
  To: 'Gary Lin'
  Cc: edk2-devel@lists.01.org, Zeng, Star, Wang, Jian J, Wu, Hao A



> -----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.

> 
> 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.efi
> > > )
> > > PCI()/SATA(sdb)/Partition(sdb1)/Partition(md0p1)/File(bootloader.efi
> > > )
> > >
> > > 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
> >
> >


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] MdeModulePkg/UefiBootManagerLib: Match the nested partitions
  2019-01-23  3:48     ` Ni, Ray
@ 2019-01-23  3:49       ` Ni, Ray
  2019-01-23  4:02         ` Gary Lin
  0 siblings, 1 reply; 9+ messages in thread
From: Ni, Ray @ 2019-01-23  3:49 UTC (permalink / raw)
  To: 'Gary Lin'
  Cc: 'edk2-devel@lists.01.org', Zeng, Star, Wang, Jian J,
	Wu, Hao A



> -----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.

> 
> >
> > 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
> > >
> > >


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] MdeModulePkg/UefiBootManagerLib: Match the nested partitions
  2019-01-23  3:49       ` Ni, Ray
@ 2019-01-23  4:02         ` Gary Lin
  0 siblings, 0 replies; 9+ messages in thread
From: Gary Lin @ 2019-01-23  4:02 UTC (permalink / raw)
  To: Ni, Ray
  Cc: 'edk2-devel@lists.01.org', Zeng, Star, Wang, Jian J,
	Wu, Hao A

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
> > > >
> > > >
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2019-01-23  4:02 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-01-15  9:45 [PATCH] MdeModulePkg/UefiBootManagerLib: Match the nested partitions Gary Lin
2019-01-15 11:58 ` Laszlo Ersek
2019-01-16  2:16   ` Gary Lin
2019-01-16  9:02     ` Laszlo Ersek
2019-01-22 16:01 ` Ni, Ray
2019-01-23  2:12   ` Gary Lin
2019-01-23  3:48     ` Ni, Ray
2019-01-23  3:49       ` Ni, Ray
2019-01-23  4:02         ` Gary Lin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox