> On Aug 14, 2023, at 05:47, Oliver Steffen <ostef...@redhat.com> wrote: > > 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
Hi Vladimir. When I try to build from your branch, I eventually get this error: ``` done mv syminfo.lst.new syminfo.lst cat syminfo.lst | sort | mawk -f ./genmoddep.awk > moddep.lst || (rm -f moddep.lst; exit 1) mawk: ./genmoddep.awk: line 106: function asorti never defined make[3]: *** [Makefile:51272: moddep.lst] Error 1 make[3]: Leaving directory '/home/pmsjt/grub_github/grub-core' make[2]: *** [Makefile:28852: all] Error 2 make[2]: Leaving directory '/home/pmsjt/grub_github/grub-core' make[1]: *** [Makefile:12126: all-recursive] Error 1 make[1]: Leaving directory '/home/pmsjt/grub_github' make: *** [Makefile:3953: all] Error 2 ``` Is this something you know about? > > 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
