[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v9 02/11] Unify GUID types
From: |
Oliver Steffen |
Subject: |
Re: [PATCH v9 02/11] Unify GUID types |
Date: |
Mon, 14 Aug 2023 05:47:15 -0700 |
User-agent: |
alot/0.8.1 |
Quoting Vladimir 'phcoder' Serbinenko (2023-08-14 13:56:31)
> I uploaded the entire branch (patches) to my GitHub copy:
> [1]https://github.com
> /phcoder/GRUB/tree/rb1
87532c5b Deduplicate configuration table search function
is the reason for the rejection.
Thanks, Vladimir!
-Oliver
>
> Le lun. 14 août 2023, 13:07, Pedro Miguel Justo <[2]pmsjt@texair.net> a écrit
> :
>
>
> Hi Vladimir.
>
> I got a conplict while applying 0003. Out of expedience, instead of me
> trying to resolve the conflict, can you just share your baseline commit#
> over which these will apply cleanly?
>
> ```
> pmsjt@itanium:~/grub$ patch -p1 < ../
> 0001-Add-missing-static-qualifier.patch
> patching file grub-core/commands/efi/lsefi.c
> pmsjt@itanium:~/grub$ patch -p1 < ../
> 0002-Mark-grub_gpt_partentry-as-aligned-to-8-bytes.patch
> patching file include/grub/gpt_partition.h
> Hunk #1 succeeded at 67 (offset -7 lines).
> pmsjt@itanium:~/grub$ patch -p1 < ../
> 0003-Split-aligned-and-packed-guids.patch
> patching file grub-core/commands/efi/lsefi.c
> patching file grub-core/efiemu/runtime/efiemu.c
> patching file grub-core/kern/efi/efi.c
> Hunk #1 FAILED at 1039.
> 1 out of 1 hunk FAILED -- saving rejects to file grub-core/kern/efi/
> efi.c.rej
> patching file grub-core/kern/misc.c
> patching file grub-core/loader/i386/xnu.c
> patching file include/grub/efi/api.h
> patching file include/grub/efiemu/efiemu.h
> patching file include/grub/efiemu/runtime.h
> patching file include/grub/types.h
> ```
>
>
>
>
> On Aug 14, 2023, at 03:26, Pedro Miguel Justo <[3]pmsjt@texair.net>
> wrote:
>
> Hi Vladimir.
>
> Thanks for the analysis and for providing the candidate patches. I'll
> be away from my computer for a couple of days but will give it a try
> as
> soon as I can find a sliver of time.
>
> Pedro
>
>
> On Aug 13, 2023, at 08:47, Vladimir 'phcoder' Serbinenko <[4]
> phcoder@gmail.com> wrote:
>
>
> Full analysis: gpt_partentry can be marked as aligned8. But
> following are problem:
> * protocols_per_handle may return unaligned guids
> * configuration_tables array may be unaligned. On efi32 every
> entry
> is 20 bytes, so guids can't be 8-byte aligned
> * The worst offender is device path as it's packed, unpredictable
> and contains uuids.
> * Efiemu gets guid as argument and probably should be able to
> handle unaligned case. So far it's x86 only and we have no mmx and
> co so it's not a problem right now unless we enable ubsan.
> All in all we do need packed guid type unless those are resolved.
> I
> attach patches for testing. If they work, I'll rearrange them a
> bit
>
> Le dim. 13 août 2023, 05:27, Vladimir 'phcoder' Serbinenko <[5]
> phcoder@gmail.com> a écrit :
>
>
>
> Le dim. 13 août 2023, 00:57, Pedro Miguel Justo <[6]
> pmsjt@texair.net> a écrit :
>
>
>
> > On Aug 12, 2023, at 11:04, John Paul Adrian Glaubitz
> <[7]
> glaubitz@physik.fu-berlin.de> wrote:
> >
> > Hi Daniel!
> >
> > On Fri, 2023-08-11 at 17:31 +0200, Daniel Kiper wrote:
> >> On Fri, Aug 11, 2023 at 04:10:14AM -0700, Oliver
> Steffen
> wrote:
> >>> Quoting John Paul Adrian Glaubitz (2023-08-11
> 10:32:17)
> >>>> Hi Oliver!
> >>>>
> >>>> On Fri, 2023-05-26 at 13:35 +0200, Oliver Steffen
> wrote:
> >>>>> There are 3 implementations of a GUID in Grub.
> Replace them with a
> >>>>> common one, placed in types.h.
> >>>>>
> >>>>> It uses the "packed" flavor of the GUID structs, the
> alignment attribute
> >>>>> is dropped, since it is not required.
> >>>>>
> >>>>> Signed-off-by: Oliver Steffen
> <[8]osteffen@redhat.com
> >
> >>>>> Reviewed-by: Daniel Kiper
> <[9]daniel.kiper@oracle.com
> >
> >>
> >> [...]
> >>
> >>>> According to [1], this change broke GRUB on ia64:
> >>>>
> >>>> Welcome to GRUB!
> >>>>
> >>>> 7 0 0x00006B 0x000000000000001E unexpected trap
> >>>> 7 0 0x000066 0x000000000000001E trap taken, number in
> ext PE
> >>>> 7 0 0x00003C 0x0000000000005A00 trap taken, offset in
> ext PE
> >>>>
> >>>> I assume this is because of the strict alignment
> requirements on ia64.
> >>>>
> >>>> Could you have a look?
> >>>
> >>> I am very sorry for this mistake.
> >>> My goal was to unify the two GUID types we had in grub
> but I missed the
> >>> fact that in my "solution" the alignments are not
> correct in all cases.
> >>>
> >>> The quickest way out could be to revert the GUID
> unification and printf
> >>> format specifier commits:
> >>>
> >>> 6ad116e5f guid: Make use of GUID printf format
> specifier
> >>> f82dbf2bd kern/misc: Add a format specifier GUIDs
> >>> 06edd40db guid: Unify GUID types
> >>>
> >>> And use the explicit, long-winded format string for
> printing the GUID
> >>> in the bli module instead (added in the commits
> following those).
> >>>
> >>> I am open to suggestions / comments.
> >>
> >> Adrian, could you check what will happen when you add
> alignment to the
> >> grub_guid_t as it was suggested by Frank here [2]?
> >>
> >> Personally I would avoid adding another GUID type with
> just alignment
> >> requirement as the difference. Making one GUID type
> with
> always enforced
> >> alignment should not cost us a lot. Or we can enforce
> alignment on EFI
> >> platforms only.
> >
> > My Itanium hardware is not available for bootloader
> tests
> at the moment, so I would
> > like to ask Pedro Miguel Justo or Frank Scheiner to test
> the proposed fix.
>
>
> The reason that before this unification there were a
> packed
> and aligned types suggest that the packed type was
> necessary in some cases. Frank had shared the concern
> against his own suggestion that changing the unified type
> to be aligned (as opposite to packed) would likely regress
> the cases where the packed might be needed/expected.
>
> Just for kicks, I gave it a try:
>
> ```
> diff --git a/include/grub/types.h b/include/grub/types.h
> index 0d96006fe..ff415c970 100644
> --- a/include/grub/types.h
> +++ b/include/grub/types.h
> @@ -376,7 +376,7 @@ struct grub_guid
> grub_uint16_t data2;
> grub_uint16_t data3;
> grub_uint8_t data4[8];
> -} GRUB_PACKED;
> +} __attribute__ ((aligned(8)));
> typedef struct grub_guid grub_guid_t;
>
> #endif /* ! GRUB_TYPES_HEADER */
> ```
>
> As hypothesized, there are places where these structs (now
> unified) are expected to be packed, meaning to have an
> alignment of 1 (even smaller than the natural alignment of
> its larger member).
>
> One of the cases where the dependency is present is in the
> grub_gpt_partentry structure. This structure is packed and
> includes a GUID field. One struct of an enforced alignment
> ’n’ cannot host a member with enforced alignment ‘m’ where
> m > n. Thus, an 8 byte aligned grub_guid cannot be hosted
> inside a 1 byte aligned grub_gpt_partentry:
>
>
> grub_gpt_partentry doesn't need
>
> GRUB_PACKED. To be on a safe side we can
>
> add compile time assert on its size
>
>
>
> ```
> In file included from grub-core/disk/ldm.c:26:
> ./include/grub/gpt_partition.h:70:1: error: alignment 1 of
> ‘struct grub_gpt_partentry’ is less than 8 [-Werror=
> packed-not-aligned]
> 70 | } GRUB_PACKED;
> | ^
> ./include/grub/gpt_partition.h:70:1: error: alignment 1 of
> ‘struct grub_gpt_partentry’ is less than 8 [-Werror=
> packed-not-aligned]
> ```
>
> Now, we could go this rabbit hole on and try to hack
> grub_gpt_partentry, or special-case the GUID inside
> grub_gpt_partentry to be packed… but at which point will I
> just end up with the thing we were trying to avoid: more
> than one definition of GUID?
>
> >
> > Thanks,
> > Adrian
> >
> > --
> > .''`. John Paul Adrian Glaubitz
> > : :' : Debian Developer
> > `. `' Physicist
> > `- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37
> F5B5
> F913
>
>
> <0002-Mark-grub_gpt_partentry-as-aligned-to-8-bytes.patch>
> <0001-Add-missing-static-qualifier.patch>
> <0004-Add-compile-time-asserts-for-guid-and-gpt_partentry-.patch>
> <0003-Split-aligned-and-packed-guids.patch>
> <0005-Fix-typo.patch>
>
>
>
>
> References:
>
> [1] https://github.com/phcoder/GRUB/tree/rb1
> [2] mailto:pmsjt@texair.net
> [3] mailto:pmsjt@texair.net
> [4] mailto:phcoder@gmail.com
> [5] mailto:phcoder@gmail.com
> [6] mailto:pmsjt@texair.net
> [7] mailto:glaubitz@physik.fu-berlin.de
> [8] mailto:osteffen@redhat.com
> [9] mailto:daniel.kiper@oracle.com
--
🎩Oliver Steffen (he/him) - Software Engineer, Virtualization
Red Hat GmbH <https://www.redhat.com/de/global/dach>,
Registered seat: Werner-von-Siemens-Ring 12, D-85630 Grasbrunn, Germany
Commercial register: Amtsgericht München/Munich, HRB 153243,
Managing Directors: Ryan Barnhart, Charles Cachera, Michael O'Neill,
Amy Ross
Everyone has different working hours… Please do not feel obligated to
reply outside of your normal work schedule.
- Re: [PATCH v9 02/11] Unify GUID types, (continued)
- Re: [PATCH v9 02/11] Unify GUID types, Oliver Steffen, 2023/08/11
- Re: [PATCH v9 02/11] Unify GUID types, Daniel Kiper, 2023/08/11
- Re: [PATCH v9 02/11] Unify GUID types, John Paul Adrian Glaubitz, 2023/08/12
- Re: [PATCH v9 02/11] Unify GUID types, Pedro Miguel Justo, 2023/08/12
- Re: [PATCH v9 02/11] Unify GUID types, Vladimir 'phcoder' Serbinenko, 2023/08/12
- Re: [PATCH v9 02/11] Unify GUID types, Vladimir 'phcoder' Serbinenko, 2023/08/13
- Re: [PATCH v9 02/11] Unify GUID types, Pedro Miguel Justo, 2023/08/14
- Re: [PATCH v9 02/11] Unify GUID types, Pedro Miguel Justo, 2023/08/14
- Re: [PATCH v9 02/11] Unify GUID types, Vladimir 'phcoder' Serbinenko, 2023/08/14
- Re: [PATCH v9 02/11] Unify GUID types, Pedro Miguel Justo, 2023/08/14
- Re: [PATCH v9 02/11] Unify GUID types,
Oliver Steffen <=
- Re: [PATCH v9 02/11] Unify GUID types, Pedro Miguel Justo, 2023/08/14
- Re: [PATCH v9 02/11] Unify GUID types, John Paul Adrian Glaubitz, 2023/08/14
- Re: [PATCH v9 02/11] Unify GUID types, Oliver Steffen, 2023/08/14
- Re: [PATCH v9 02/11] Unify GUID types, Frank Scheiner, 2023/08/14
- Re: [PATCH v9 02/11] Unify GUID types, Pedro Miguel Justo, 2023/08/14
- Re: [PATCH v9 02/11] Unify GUID types, Pedro Miguel Justo, 2023/08/14
- Re: [PATCH v9 02/11] Unify GUID types, Vladimir 'phcoder' Serbinenko, 2023/08/14
- Re: [PATCH v9 02/11] Unify GUID types, Ard Biesheuvel, 2023/08/15
- Re: [PATCH v9 02/11] Unify GUID types, Laszlo Ersek, 2023/08/15
- Re: [PATCH v9 02/11] Unify GUID types, Vladimir 'phcoder' Serbinenko, 2023/08/15