Re: [PATCH 3/4] types: Split aligned and packed guids

[Daniel Kiper]

On Tue, Oct 31, 2023 at 08:35:26PM +0100, Vladimir 'phcoder' Serbinenko wrote:
> From 48c239ad9efc40563015052383d80830d410c2c8 Mon Sep 17 00:00:00 2001
> From: Vladimir Serbinenko <phcoder@gmail.com>
> Date: Sun, 13 Aug 2023 09:18:23 +0200
> Subject: [PATCH 3/4] types: Split aligned and packed guids
>
> On ia64 alignment requirements are strict. When we pass a pointer to
> UUID it needs to be at least 4-byte aligned or EFI will crash.
> On the other hand in device path there is no padding for UUID, so we
> need 2 types in one formor another. Make 4-byte aligned and unaligned types
>
> Signed-off-by: Vladimir Serbinenko <phcoder@gmail.com>
> ---
>  grub-core/commands/efi/lsefi.c    |  2 +-
>  grub-core/efiemu/runtime/efiemu.c | 33 +++++++++++++++++++++----------
>  grub-core/kern/efi/efi.c          |  2 +-
>  grub-core/kern/misc.c             |  2 +-
>  grub-core/loader/i386/xnu.c       |  2 +-
>  include/grub/efi/api.h            | 12 +++++------
>  include/grub/efiemu/efiemu.h      |  4 ++--
>  include/grub/efiemu/runtime.h     |  2 +-
>  include/grub/types.h              | 11 ++++++++++-
>  9 files changed, 46 insertions(+), 24 deletions(-)
>
> diff --git a/grub-core/commands/efi/lsefi.c b/grub-core/commands/efi/lsefi.c
> index 95cba5baf..7b8316d41 100644
> --- a/grub-core/commands/efi/lsefi.c
> +++ b/grub-core/commands/efi/lsefi.c
> @@ -96,7 +96,7 @@ grub_cmd_lsefi (grub_command_t cmd __attribute__ ((unused)),
>        grub_efi_handle_t handle = handles[i];
>        grub_efi_status_t status;
>        grub_efi_uintn_t num_protocols;
> -      grub_guid_t **protocols;
> +      grub_packed_guid_t **protocols;
>        grub_efi_device_path_t *dp;
>
>        grub_printf ("Handle %p\n", handle);
> diff --git a/grub-core/efiemu/runtime/efiemu.c 
> b/grub-core/efiemu/runtime/efiemu.c
> index c84b30652..f6b6d19f7 100644
> --- a/grub-core/efiemu/runtime/efiemu.c
> +++ b/grub-core/efiemu/runtime/efiemu.c
> @@ -66,7 +66,7 @@ efiemu_convert_pointer (grub_efi_uintn_t debug_disposition,
>
>  grub_efi_status_t __grub_efi_api
>  efiemu_get_variable (grub_efi_char16_t *variable_name,
> -                  const grub_guid_t *vendor_guid,
> +                  const grub_packed_guid_t *vendor_guid,
>                    grub_efi_uint32_t *attributes,
>                    grub_efi_uintn_t *data_size,
>                    void *data);
> @@ -74,11 +74,11 @@ efiemu_get_variable (grub_efi_char16_t *variable_name,
>  grub_efi_status_t __grub_efi_api
>  efiemu_get_next_variable_name (grub_efi_uintn_t *variable_name_size,
>                              grub_efi_char16_t *variable_name,
> -                            grub_guid_t *vendor_guid);
> +                            grub_packed_guid_t *vendor_guid);
>
>  grub_efi_status_t __grub_efi_api
>  efiemu_set_variable (grub_efi_char16_t *variable_name,
> -                  const grub_guid_t *vendor_guid,
> +                  const grub_packed_guid_t *vendor_guid,
>                    grub_efi_uint32_t attributes,
>                    grub_efi_uintn_t data_size,
>                    void *data);
> @@ -416,7 +416,7 @@ EFI_FUNC (efiemu_convert_pointer) (grub_efi_uintn_t 
> debug_disposition,
>
>  /* Find variable by name and GUID. */
>  static struct efi_variable *
> -find_variable (const grub_guid_t *vendor_guid,
> +find_variable (const grub_packed_guid_t *vendor_guid,
>              grub_efi_char16_t *variable_name)
>  {
>    grub_uint8_t *ptr;
> @@ -438,7 +438,7 @@ find_variable (const grub_guid_t *vendor_guid,
>
>  grub_efi_status_t __grub_efi_api
>  EFI_FUNC (efiemu_get_variable) (grub_efi_char16_t *variable_name,
> -                             const grub_guid_t *vendor_guid,
> +                             const grub_packed_guid_t *vendor_guid,
>                               grub_efi_uint32_t *attributes,
>                               grub_efi_uintn_t *data_size,
>                               void *data)
> @@ -464,7 +464,7 @@ EFI_FUNC (efiemu_get_variable) (grub_efi_char16_t 
> *variable_name,
>  grub_efi_status_t __grub_efi_api EFI_FUNC
>  (efiemu_get_next_variable_name) (grub_efi_uintn_t *variable_name_size,
>                                grub_efi_char16_t *variable_name,
> -                              grub_guid_t *vendor_guid)
> +                              grub_packed_guid_t *vendor_guid)
>  {
>    struct efi_variable *efivar;
>    LOG ('l');
> @@ -503,7 +503,7 @@ grub_efi_status_t __grub_efi_api EFI_FUNC
>
>  grub_efi_status_t __grub_efi_api
>  EFI_FUNC (efiemu_set_variable) (grub_efi_char16_t *variable_name,
> -                             const grub_guid_t *vendor_guid,
> +                             const grub_packed_guid_t *vendor_guid,
>                               grub_efi_uint32_t attributes,
>                               grub_efi_uintn_t data_size,
>                               void *data)
> @@ -597,9 +597,22 @@ struct grub_efi_runtime_services efiemu_runtime_services 
> =
>    .set_virtual_address_map = efiemu_set_virtual_address_map,
>    .convert_pointer = efiemu_convert_pointer,
>
> -  .get_variable = efiemu_get_variable,
> -  .get_next_variable_name = efiemu_get_next_variable_name,
> -  .set_variable = efiemu_set_variable,
> +  .get_variable = (grub_efi_status_t
> +                (__grub_efi_api *) (grub_efi_char16_t *variable_name,
> +                                    const grub_guid_t *vendor_guid,
> +                                    grub_efi_uint32_t *attributes,
> +                                    grub_efi_uintn_t *data_size,
> +                                    void *data)) efiemu_get_variable,
> +  .get_next_variable_name = (grub_efi_status_t
> +                          (__grub_efi_api *) (grub_efi_uintn_t 
> *variable_name_size,
> +                                              grub_efi_char16_t 
> *variable_name,
> +                                              grub_guid_t *vendor_guid)) 
> efiemu_get_next_variable_name,
> +  .set_variable = (grub_efi_status_t
> +                (__grub_efi_api *) (grub_efi_char16_t *variable_name,
> +                                    const grub_guid_t *vendor_guid,
> +                                    grub_efi_uint32_t attributes,
> +                                    grub_efi_uintn_t data_size,
> +                                    void *data)) efiemu_set_variable,

These casts looks strange for me. Could not we change the functions
declarations in the grub_efi_runtime_services? If not I think this
change should be explained in the commit message. Or leave efiemu_*
functions as is...

The other patches in these series LGTM. When we are done with the thing
mentioned above I will ask folks for more testing.

Daniel