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]pm...@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]pm...@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] > phco...@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] > phco...@gmail.com> a écrit : > > > > Le dim. 13 août 2023, 00:57, Pedro Miguel Justo <[6] > pm...@texair.net> a écrit : > > > > > On Aug 12, 2023, at 11:04, John Paul Adrian Glaubitz > <[7] > glaub...@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]ostef...@redhat.com > > > >>>>> Reviewed-by: Daniel Kiper > <[9]daniel.ki...@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:pm...@texair.net > [3] mailto:pm...@texair.net > [4] mailto:phco...@gmail.com > [5] mailto:phco...@gmail.com > [6] mailto:pm...@texair.net > [7] mailto:glaub...@physik.fu-berlin.de > [8] mailto:ostef...@redhat.com > [9] mailto:daniel.ki...@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. _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
