help On Sun, Aug 13, 2023 at 1:17 PM <grub-devel-requ...@gnu.org> wrote:
> Send Grub-devel mailing list submissions to > grub-devel@gnu.org > > To subscribe or unsubscribe via the World Wide Web, visit > https://lists.gnu.org/mailman/listinfo/grub-devel > or, via email, send a message with subject or body 'help' to > grub-devel-requ...@gnu.org > > You can reach the person managing the list at > grub-devel-ow...@gnu.org > > When replying, please edit your Subject line so it is more specific > than "Re: Contents of Grub-devel digest..." > > > Today's Topics: > > 1. Re: [PATCH v9 02/11] Unify GUID types > (Vladimir 'phcoder' Serbinenko) > > > ---------------------------------------------------------------------- > > Message: 1 > Date: Sun, 13 Aug 2023 09:46:45 +0200 > From: "Vladimir 'phcoder' Serbinenko" <phco...@gmail.com> > To: Pedro Miguel Justo <pm...@texair.net> > Cc: John Paul Adrian Glaubitz <glaub...@physik.fu-berlin.de>, The > development of GNU GRUB <grub-devel@gnu.org>, Oliver Steffen > <ostef...@redhat.com>, Gerd Hoffmann <kra...@redhat.com>, Frank > Scheiner <frank.schei...@web.de> > Subject: Re: [PATCH v9 02/11] Unify GUID types > Message-ID: > <CAEaD8JOWL9gyWt6qs36pDFioi3uE86YQhp= > etjir47qgjk6...@mail.gmail.com> > Content-Type: text/plain; charset="utf-8" > > 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 > >> > >> > -------------- next part -------------- > An HTML attachment was scrubbed... > URL: < > https://lists.gnu.org/archive/html/grub-devel/attachments/20230813/b10a3380/attachment.htm > > > -------------- next part -------------- > A non-text attachment was scrubbed... > Name: 0002-Mark-grub_gpt_partentry-as-aligned-to-8-bytes.patch > Type: text/x-diff > Size: 723 bytes > Desc: not available > URL: < > https://lists.gnu.org/archive/html/grub-devel/attachments/20230813/b10a3380/attachment.patch > > > -------------- next part -------------- > A non-text attachment was scrubbed... > Name: 0001-Add-missing-static-qualifier.patch > Type: text/x-diff > Size: 644 bytes > Desc: not available > URL: < > https://lists.gnu.org/archive/html/grub-devel/attachments/20230813/b10a3380/attachment-0001.patch > > > -------------- next part -------------- > A non-text attachment was scrubbed... > Name: 0004-Add-compile-time-asserts-for-guid-and-gpt_partentry-.patch > Type: text/x-diff > Size: 821 bytes > Desc: not available > URL: < > https://lists.gnu.org/archive/html/grub-devel/attachments/20230813/b10a3380/attachment-0002.patch > > > -------------- next part -------------- > A non-text attachment was scrubbed... > Name: 0003-Split-aligned-and-packed-guids.patch > Type: text/x-diff > Size: 9171 bytes > Desc: not available > URL: < > https://lists.gnu.org/archive/html/grub-devel/attachments/20230813/b10a3380/attachment-0003.patch > > > -------------- next part -------------- > A non-text attachment was scrubbed... > Name: 0005-Fix-typo.patch > Type: text/x-diff > Size: 1078 bytes > Desc: not available > URL: < > https://lists.gnu.org/archive/html/grub-devel/attachments/20230813/b10a3380/attachment-0004.patch > > > > ------------------------------ > > Subject: Digest Footer > > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel > > > ------------------------------ > > End of Grub-devel Digest, Vol 234, Issue 14 > ******************************************* >
_______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
