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

Message ID 1456854103-12095-2-git-send-email-leif.lindholm@linaro.org
State New
Headers show

Commit Message

Leif Lindholm March 1, 2016, 5:41 p.m.
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(+)

-- 
2.1.4


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Comments

Leif Lindholm March 1, 2016, 8:14 p.m. | #1
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
Leif Lindholm March 1, 2016, 8:28 p.m. | #2
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

Patch hide | download patch | download mbox

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