I uploaded the entire branch (patches) to my GitHub copy: https://github.com/phcoder/GRUB/tree/rb1
Le lun. 14 août 2023, 13:07, Pedro Miguel Justo <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 <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 < > 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 < > phco...@gmail.com> a écrit : > >> >> >> Le dim. 13 août 2023, 00:57, Pedro Miguel Justo <pm...@texair.net> a >> écrit : >> >>> >>> >>> > On Aug 12, 2023, at 11:04, John Paul Adrian Glaubitz < >>> 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 <ostef...@redhat.com> >>> >>>>> Reviewed-by: Daniel Kiper <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> > > >
_______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
