On Tue, 8 Jan 2019 at 16:53, Leif Lindholm <leif.lindholm@xxxxxxxxxx> wrote:
>
> On Tue, Jan 08, 2019 at 04:28:29PM +0100, Ard Biesheuvel wrote:
> > The UEFI spec and EDK2 reference implementation both define EFI_GUID as
> > struct { u32 a; u16; b; u16 c; u8 d[8]; }; and so the implied alignment
> > is 32 bits not 8 bits like our guid_t. In some cases (i.e., on 32-bit ARM),
> > this means that firmware services invoked by the kernel may assume that
> > efi_guid_t* arguments are 32-bit aligned, and use memory accessors that
> > do not tolerate misalignment. So let's set the minimum alignment to 32 bits.
> >
> > Note that the UEFI spec as well as some comments in the EDK2 code base
> > suggest that EFI_GUID should be 64-bit aligned, but this appears to be
> > a mistake, given that no code seems to exist that actually enforces that
> > or relies on it.
>
> Whereas code does exist that relies on it being 32-bit aligned...
>
Well, not entirely. Code exists that expects that the size of a struct
incorporating a efi_guid_t is not rounded up to 8 bytes.
In particular, there is
typedef struct {
efi_guid_t guid;
unsigned long table;
} efi_config_table_t;
and its EDK2 counterpart
typedef struct {
///
/// The 128-bit GUID value that uniquely identifies the system
configuration table.
///
EFI_GUID VendorGuid;
///
/// A pointer to the table associated with VendorGuid.
///
VOID *VendorTable;
} EFI_CONFIGURATION_TABLE;
neither of which are defined as packed structs, and which are used to
describe entries in the configuration table array referenced in the
UEFI system table, and so size matters. On 32-bit architectures, this
struct would change size if we increase the alignment of the
VendorGuid member, unless we turn it into a packed struct.
In any case, it seems entirely pointless to update the reference
implementation to enforce 64 bit alignment for EFI_GUID in general,
only to create a lots of issues like the above that will need to be
hunted down and fixed. So for all intents and purposes, this 64-bit
alignment of EFI_GUID in the UEFI spec can be ignored.
> > Reported-by: Heinrich Schuchardt <xypron.glpk@xxxxxx>,
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
>
> Reviewed-by: Leif Lindholm <leif.lindholm@xxxxxxxxxx>
>
Thanks.
> > ---
> > include/linux/efi.h | 15 ++++++++++++++-
> > 1 file changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/efi.h b/include/linux/efi.h
> > index 45ff763fba76..be08518c2553 100644
> > --- a/include/linux/efi.h
> > +++ b/include/linux/efi.h
> > @@ -48,7 +48,20 @@ typedef u16 efi_char16_t; /* UNICODE character */
> > typedef u64 efi_physical_addr_t;
> > typedef void *efi_handle_t;
> >
> > -typedef guid_t efi_guid_t;
> > +/*
> > + * The UEFI spec and EDK2 reference implementation both define EFI_GUID as
> > + * struct { u32 a; u16; b; u16 c; u8 d[8]; }; and so the implied alignment
> > + * is 32 bits not 8 bits like our guid_t. In some cases (i.e., on 32-bit ARM),
> > + * this means that firmware services invoked by the kernel may assume that
> > + * efi_guid_t* arguments are 32-bit aligned, and use memory accessors that
> > + * do not tolerate misalignment. So let's set the minimum alignment to 32 bits.
> > + *
> > + * Note that the UEFI spec as well as some comments in the EDK2 code base
> > + * suggest that EFI_GUID should be 64-bit aligned, but this appears to be
> > + * a mistake, given that no code seems to exist that actually enforces that
> > + * or relies on it.
> > + */
> > +typedef guid_t efi_guid_t __aligned(__alignof__(u32));
> >
> > #define EFI_GUID(a,b,c,d0,d1,d2,d3,d4,d5,d6,d7) \
> > GUID_INIT(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7)
> > --
> > 2.20.1
> >
