[RFC 0/3] Add/use helper functions for finding UEFI config tables

[RFC 0/3] Add/use helper functions for finding UEFI config tables

[Leif Lindholm]

(Triggered by Alex's recent patches)

There are a few places in the code manually iterating across the
config tables provided through the UEFI system table. This set
implements a common search function and converts some existing
functions to use it.

I have not yet converted commands/efi/loadbios.c given that it
searches for multiple things in one go. Would be a trivial change.

Also, for cleanliness of call-sites a better approach may be to macroize
the function so that it was possible to simply go:
  ptr = grub_efi_find_config_table(MY_VERY_SPECIAL_GUID);
This was not my preference, since that hides the fact that the
underlying datatype is a 128-bit struct.

Leif Lindholm (3):
  efi: add configuration table search function
  arm64 linux loader: use grub_efi_find_config_table to find DT
  acpi: use grub_efi_find_config_table to find tables

 grub-core/kern/efi/acpi.c    | 28 ++++------------------------
 grub-core/kern/efi/efi.c     | 18 ++++++++++++++++++
 grub-core/loader/arm64/fdt.c | 16 +---------------
 include/grub/efi/efi.h       |  3 +++
 4 files changed, 26 insertions(+), 39 deletions(-)

-- 
2.1.4

[RFC 1/3] efi: add configuration table search function

[Leif Lindholm]

Several places in the code iterates through the configuration tables
presented by the system table.
Add new function grub_efi_find_config_table to use for this instead.
---
 grub-core/kern/efi/efi.c | 18 ++++++++++++++++++
 include/grub/efi/efi.h   |  3 +++
 2 files changed, 21 insertions(+)

diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
index caf9bcc..41686ee 100644
--- a/grub-core/kern/efi/efi.c
+++ b/grub-core/kern/efi/efi.c
@@ -911,3 +911,21 @@ grub_efi_compare_device_paths (const grub_efi_device_path_t *dp1,
 
   return 0;
 }
+
+/* Return pointer to vendor table identified by guid. */
+void *
+grub_efi_find_config_table (const grub_efi_guid_t *guid)
+{
+  grub_efi_uintn_t i;
+
+  for (i = 0; i < grub_efi_system_table->num_table_entries; i++)
+    {
+      grub_efi_configuration_table_t *table =
+	&grub_efi_system_table->configuration_table[i];
+
+      if (! grub_memcmp (&table->vendor_guid, guid,
+			 sizeof (grub_efi_packed_guid_t)))
+	return (void *) table->vendor_table;
+    }
+  return NULL;
+}
diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
index 0e6fd86..65c7f95 100644
--- a/include/grub/efi/efi.h
+++ b/include/grub/efi/efi.h
@@ -77,6 +77,9 @@ int
 EXPORT_FUNC (grub_efi_compare_device_paths) (const grub_efi_device_path_t *dp1,
 					     const grub_efi_device_path_t *dp2);
 
+void *
+EXPORT_FUNC (grub_efi_find_config_table) (const grub_efi_guid_t *guid);
+
 extern void (*EXPORT_VAR(grub_efi_net_config)) (grub_efi_handle_t hnd, 
 						char **device,
 						char **path);
-- 
2.1.4

Re: [RFC 1/3] efi: add configuration table search function

[Andrei Borzenkov]

01.03.2016 20:41, Leif Lindholm пишет:
> Several places in the code iterates through the configuration tables
> presented by the system table.
> Add new function grub_efi_find_config_table to use for this instead.
> ---
>  grub-core/kern/efi/efi.c | 18 ++++++++++++++++++
>  include/grub/efi/efi.h   |  3 +++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
> index caf9bcc..41686ee 100644
> --- a/grub-core/kern/efi/efi.c
> +++ b/grub-core/kern/efi/efi.c
> @@ -911,3 +911,21 @@ grub_efi_compare_device_paths (const grub_efi_device_path_t *dp1,
>  
>    return 0;
>  }
> +
> +/* Return pointer to vendor table identified by guid. */
> +void *
> +grub_efi_find_config_table (const grub_efi_guid_t *guid)
> +{
> +  grub_efi_uintn_t i;
> +
> +  for (i = 0; i < grub_efi_system_table->num_table_entries; i++)
> +    {
> +      grub_efi_configuration_table_t *table =
> +	&grub_efi_system_table->configuration_table[i];
> +
> +      if (! grub_memcmp (&table->vendor_guid, guid,
> +			 sizeof (grub_efi_packed_guid_t)))

Either both types are exactly the same and one should be dropped or this
cannot be right. In view of recent discussion about structure member
alignments I am not sure why grub_efi_packed_guid_t is needed and the
code already was inconsistent in usage.

> +	return (void *) table->vendor_table;
> +    }
> +  return NULL;
> +}
> diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
> index 0e6fd86..65c7f95 100644
> --- a/include/grub/efi/efi.h
> +++ b/include/grub/efi/efi.h
> @@ -77,6 +77,9 @@ int
>  EXPORT_FUNC (grub_efi_compare_device_paths) (const grub_efi_device_path_t *dp1,
>  					     const grub_efi_device_path_t *dp2);
>  
> +void *
> +EXPORT_FUNC (grub_efi_find_config_table) (const grub_efi_guid_t *guid);
> +
>  extern void (*EXPORT_VAR(grub_efi_net_config)) (grub_efi_handle_t hnd, 
>  						char **device,
>  						char **path);
> 

Re: [RFC 1/3] efi: add configuration table search function

[Vladimir 'phcoder' Serbinenko]

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

Le mar. 1 mars 2016 21:09, Andrei Borzenkov <arvidjaar@gmail.com> a écrit :

> 01.03.2016 20:41, Leif Lindholm пишет:
> > Several places in the code iterates through the configuration tables
> > presented by the system table.
> > Add new function grub_efi_find_config_table to use for this instead.
> > ---
> >  grub-core/kern/efi/efi.c | 18 ++++++++++++++++++
> >  include/grub/efi/efi.h   |  3 +++
> >  2 files changed, 21 insertions(+)
> >
> > diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
> > index caf9bcc..41686ee 100644
> > --- a/grub-core/kern/efi/efi.c
> > +++ b/grub-core/kern/efi/efi.c
> > @@ -911,3 +911,21 @@ grub_efi_compare_device_paths (const
> grub_efi_device_path_t *dp1,
> >
> >    return 0;
> >  }
> > +
> > +/* Return pointer to vendor table identified by guid. */
> > +void *
> > +grub_efi_find_config_table (const grub_efi_guid_t *guid)
> > +{
> > +  grub_efi_uintn_t i;
> > +
> > +  for (i = 0; i < grub_efi_system_table->num_table_entries; i++)
> > +    {
> > +      grub_efi_configuration_table_t *table =
> > +     &grub_efi_system_table->configuration_table[i];
> > +
> > +      if (! grub_memcmp (&table->vendor_guid, guid,
> > +                      sizeof (grub_efi_packed_guid_t)))
>
> Either both types are exactly the same and one should be dropped or this
> cannot be right. In view of recent discussion about structure member
> alignments I am not sure why grub_efi_packed_guid_t is needed and the
> code already was inconsistent in usage.
>
Types differ only in alignment, not in internal structure. We can guarantee
alignment of structure that we allocated but not of some EFI pointer

>
>
>
> > +     return (void *) table->vendor_table;
> > +    }
> > +  return NULL;
> > +}
> > diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
> > index 0e6fd86..65c7f95 100644
> > --- a/include/grub/efi/efi.h
> > +++ b/include/grub/efi/efi.h
> > @@ -77,6 +77,9 @@ int
> >  EXPORT_FUNC (grub_efi_compare_device_paths) (const
> grub_efi_device_path_t *dp1,
> >                                            const grub_efi_device_path_t
> *dp2);
> >
> > +void *
> > +EXPORT_FUNC (grub_efi_find_config_table) (const grub_efi_guid_t *guid);
> > +
> >  extern void (*EXPORT_VAR(grub_efi_net_config)) (grub_efi_handle_t hnd,
> >                                               char **device,
> >                                               char **path);
> >
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #2: Type: text/html, Size: 3564 bytes --]

Re: [RFC 1/3] efi: add configuration table search function

[Leif Lindholm]

On Tue, Mar 01, 2016 at 08:13:50PM +0000, Vladimir 'phcoder' Serbinenko wrote:
> Le mar. 1 mars 2016 21:09, Andrei Borzenkov <arvidjaar@gmail.com> a écrit :
> 
> > 01.03.2016 20:41, Leif Lindholm пишет:
> > > Several places in the code iterates through the configuration tables
> > > presented by the system table.
> > > Add new function grub_efi_find_config_table to use for this instead.
> > > ---
> > >  grub-core/kern/efi/efi.c | 18 ++++++++++++++++++
> > >  include/grub/efi/efi.h   |  3 +++
> > >  2 files changed, 21 insertions(+)
> > >
> > > diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
> > > index caf9bcc..41686ee 100644
> > > --- a/grub-core/kern/efi/efi.c
> > > +++ b/grub-core/kern/efi/efi.c
> > > @@ -911,3 +911,21 @@ grub_efi_compare_device_paths (const
> > grub_efi_device_path_t *dp1,
> > >
> > >    return 0;
> > >  }
> > > +
> > > +/* Return pointer to vendor table identified by guid. */
> > > +void *
> > > +grub_efi_find_config_table (const grub_efi_guid_t *guid)
> > > +{
> > > +  grub_efi_uintn_t i;
> > > +
> > > +  for (i = 0; i < grub_efi_system_table->num_table_entries; i++)
> > > +    {
> > > +      grub_efi_configuration_table_t *table =
> > > +     &grub_efi_system_table->configuration_table[i];
> > > +
> > > +      if (! grub_memcmp (&table->vendor_guid, guid,
> > > +                      sizeof (grub_efi_packed_guid_t)))
> >
> > Either both types are exactly the same and one should be dropped or this
> > cannot be right. In view of recent discussion about structure member
> > alignments I am not sure why grub_efi_packed_guid_t is needed and the
> > code already was inconsistent in usage.
> >
> Types differ only in alignment, not in internal structure. We can guarantee
> alignment of structure that we allocated but not of some EFI pointer

Of course you can.
The UEFI specification explicitly states
"Unless otherwise specified, aligned on a 64-bit boundary."
which is double the natural alignment of the largest member
defined in this struct.

/
    Leif

Re: [RFC 1/3] efi: add configuration table search function

[Vladimir 'phcoder' Serbinenko]

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

On Tuesday, March 1, 2016, Leif Lindholm <leif.lindholm@linaro.org> wrote:

> On Tue, Mar 01, 2016 at 08:13:50PM +0000, Vladimir 'phcoder' Serbinenko
> wrote:
> > Le mar. 1 mars 2016 21:09, Andrei Borzenkov <arvidjaar@gmail.com
> <javascript:;>> a écrit :
> >
> > > 01.03.2016 20:41, Leif Lindholm пишет:
> > > > Several places in the code iterates through the configuration tables
> > > > presented by the system table.
> > > > Add new function grub_efi_find_config_table to use for this instead.
> > > > ---
> > > >  grub-core/kern/efi/efi.c | 18 ++++++++++++++++++
> > > >  include/grub/efi/efi.h   |  3 +++
> > > >  2 files changed, 21 insertions(+)
> > > >
> > > > diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
> > > > index caf9bcc..41686ee 100644
> > > > --- a/grub-core/kern/efi/efi.c
> > > > +++ b/grub-core/kern/efi/efi.c
> > > > @@ -911,3 +911,21 @@ grub_efi_compare_device_paths (const
> > > grub_efi_device_path_t *dp1,
> > > >
> > > >    return 0;
> > > >  }
> > > > +
> > > > +/* Return pointer to vendor table identified by guid. */
> > > > +void *
> > > > +grub_efi_find_config_table (const grub_efi_guid_t *guid)
> > > > +{
> > > > +  grub_efi_uintn_t i;
> > > > +
> > > > +  for (i = 0; i < grub_efi_system_table->num_table_entries; i++)
> > > > +    {
> > > > +      grub_efi_configuration_table_t *table =
> > > > +     &grub_efi_system_table->configuration_table[i];
> > > > +
> > > > +      if (! grub_memcmp (&table->vendor_guid, guid,
> > > > +                      sizeof (grub_efi_packed_guid_t)))
> > >
> > > Either both types are exactly the same and one should be dropped or
> this
> > > cannot be right. In view of recent discussion about structure member
> > > alignments I am not sure why grub_efi_packed_guid_t is needed and the
> > > code already was inconsistent in usage.
> > >
> > Types differ only in alignment, not in internal structure. We can
> guarantee
> > alignment of structure that we allocated but not of some EFI pointer
>
> Of course you can.
> The UEFI specification explicitly states
> "Unless otherwise specified, aligned on a 64-bit boundary."
> which is double the natural alignment of the largest member
> defined in this struct.
>
> Lies, damn lies and firmware specifications. I'd avoid relying on firmware
not having subtle bugs with this.

> /
>     Leif
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org <javascript:;>
> https://lists.gnu.org/mailman/listinfo/grub-devel
>


-- 
Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #2: Type: text/html, Size: 3697 bytes --]

Re: [RFC 1/3] efi: add configuration table search function

[Leif Lindholm]

On Tue, Mar 01, 2016 at 11:08:45PM +0300, Andrei Borzenkov wrote:
> 01.03.2016 20:41, Leif Lindholm пишет:
> > Several places in the code iterates through the configuration tables
> > presented by the system table.
> > Add new function grub_efi_find_config_table to use for this instead.
> > ---
> >  grub-core/kern/efi/efi.c | 18 ++++++++++++++++++
> >  include/grub/efi/efi.h   |  3 +++
> >  2 files changed, 21 insertions(+)
> > 
> > diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
> > index caf9bcc..41686ee 100644
> > --- a/grub-core/kern/efi/efi.c
> > +++ b/grub-core/kern/efi/efi.c
> > @@ -911,3 +911,21 @@ grub_efi_compare_device_paths (const grub_efi_device_path_t *dp1,
> >  
> >    return 0;
> >  }
> > +
> > +/* Return pointer to vendor table identified by guid. */
> > +void *
> > +grub_efi_find_config_table (const grub_efi_guid_t *guid)
> > +{
> > +  grub_efi_uintn_t i;
> > +
> > +  for (i = 0; i < grub_efi_system_table->num_table_entries; i++)
> > +    {
> > +      grub_efi_configuration_table_t *table =
> > +	&grub_efi_system_table->configuration_table[i];
> > +
> > +      if (! grub_memcmp (&table->vendor_guid, guid,
> > +			 sizeof (grub_efi_packed_guid_t)))
> 
> Either both types are exactly the same and one should be dropped or this
> cannot be right. In view of recent discussion about structure member
> alignments I am not sure why grub_efi_packed_guid_t is needed and the
> code already was inconsistent in usage.

Ah, yes, I missed that one on copying from acpi.c.

My view (as can be seen in 3/3) is that the packed version is not
needed/correct.

/
    Leif

> > +	return (void *) table->vendor_table;
> > +    }
> > +  return NULL;
> > +}
> > diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
> > index 0e6fd86..65c7f95 100644
> > --- a/include/grub/efi/efi.h
> > +++ b/include/grub/efi/efi.h
> > @@ -77,6 +77,9 @@ int
> >  EXPORT_FUNC (grub_efi_compare_device_paths) (const grub_efi_device_path_t *dp1,
> >  					     const grub_efi_device_path_t *dp2);
> >  
> > +void *
> > +EXPORT_FUNC (grub_efi_find_config_table) (const grub_efi_guid_t *guid);
> > +
> >  extern void (*EXPORT_VAR(grub_efi_net_config)) (grub_efi_handle_t hnd, 
> >  						char **device,
> >  						char **path);
> > 
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

[RFC 2/3] arm64 linux loader: use grub_efi_find_config_table to find DT

[Leif Lindholm]

Use new helper function rather than manually iterating through system
table.
---
 grub-core/loader/arm64/fdt.c | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/grub-core/loader/arm64/fdt.c b/grub-core/loader/arm64/fdt.c
index 5202c14..16222a4 100644
--- a/grub-core/loader/arm64/fdt.c
+++ b/grub-core/loader/arm64/fdt.c
@@ -31,23 +31,9 @@ static void *fdt;
 static void *
 get_firmware_fdt (void)
 {
-  grub_efi_configuration_table_t *tables;
   grub_efi_guid_t fdt_guid = GRUB_EFI_DEVICE_TREE_GUID;
-  void *firmware_fdt = NULL;
-  unsigned int i;
 
-  /* Look for FDT in UEFI config tables. */
-  tables = grub_efi_system_table->configuration_table;
-
-  for (i = 0; i < grub_efi_system_table->num_table_entries; i++)
-    if (grub_memcmp (&tables[i].vendor_guid, &fdt_guid, sizeof (fdt_guid)) == 0)
-      {
-	firmware_fdt = tables[i].vendor_table;
-	grub_dprintf ("linux", "found registered FDT @ %p\n", firmware_fdt);
-	break;
-      }
-
-  return firmware_fdt;
+  return grub_efi_find_config_table (&fdt_guid);
 }
 
 void *
-- 
2.1.4

[RFC 3/3] acpi: use grub_efi_find_config_table to find tables

[Leif Lindholm]

Use new helper function rather than manually iterating through UEFI
system table.
---
 grub-core/kern/efi/acpi.c | 28 ++++------------------------
 1 file changed, 4 insertions(+), 24 deletions(-)

diff --git a/grub-core/kern/efi/acpi.c b/grub-core/kern/efi/acpi.c
index 74f8cd1..523d682 100644
--- a/grub-core/kern/efi/acpi.c
+++ b/grub-core/kern/efi/acpi.c
@@ -25,35 +25,15 @@
 struct grub_acpi_rsdp_v10 *
 grub_machine_acpi_get_rsdpv1 (void)
 {
-  unsigned i;
-  static grub_efi_packed_guid_t acpi_guid = GRUB_EFI_ACPI_TABLE_GUID;
+  static grub_efi_guid_t acpi_guid = GRUB_EFI_ACPI_TABLE_GUID;
 
-  for (i = 0; i < grub_efi_system_table->num_table_entries; i++)
-    {
-      grub_efi_packed_guid_t *guid =
-	&grub_efi_system_table->configuration_table[i].vendor_guid;
-
-      if (! grub_memcmp (guid, &acpi_guid, sizeof (grub_efi_packed_guid_t)))
-	return (struct grub_acpi_rsdp_v10 *)
-	  grub_efi_system_table->configuration_table[i].vendor_table;
-    }
-  return 0;
+  return grub_efi_find_config_table (&acpi_guid);
 }
 
 struct grub_acpi_rsdp_v20 *
 grub_machine_acpi_get_rsdpv2 (void)
 {
-  unsigned i;
-  static grub_efi_packed_guid_t acpi20_guid = GRUB_EFI_ACPI_20_TABLE_GUID;
-
-  for (i = 0; i < grub_efi_system_table->num_table_entries; i++)
-    {
-      grub_efi_packed_guid_t *guid =
-	&grub_efi_system_table->configuration_table[i].vendor_guid;
+  static grub_efi_guid_t acpi20_guid = GRUB_EFI_ACPI_20_TABLE_GUID;
 
-      if (! grub_memcmp (guid, &acpi20_guid, sizeof (grub_efi_packed_guid_t)))
-	return (struct grub_acpi_rsdp_v20 *)
-	  grub_efi_system_table->configuration_table[i].vendor_table;
-    }
-  return 0;
+  return grub_efi_find_config_table (&acpi20_guid);
 }
-- 
2.1.4

Re: [RFC 0/3] Add/use helper functions for finding UEFI config tables

[Alexander Graf]

On 03/01/2016 06:41 PM, Leif Lindholm wrote:
> (Triggered by Alex's recent patches)
>
> There are a few places in the code manually iterating across the
> config tables provided through the UEFI system table. This set
> implements a common search function and converts some existing
> functions to use it.
>
> I have not yet converted commands/efi/loadbios.c given that it
> searches for multiple things in one go. Would be a trivial change.
>
> Also, for cleanliness of call-sites a better approach may be to macroize
> the function so that it was possible to simply go:
>    ptr = grub_efi_find_config_table(MY_VERY_SPECIAL_GUID);
> This was not my preference, since that hides the fact that the
> underlying datatype is a 128-bit struct.

Looks reasonable to me.

Reviewed-by: Alexander Graf <agraf@suse.de>


Alex