[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v9 02/11] Unify GUID types
From: |
Pedro Miguel Justo |
Subject: |
Re: [PATCH v9 02/11] Unify GUID types |
Date: |
Mon, 14 Aug 2023 14:33:14 +0000 |
> On Aug 14, 2023, at 05:47, Oliver Steffen <osteffen@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]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, 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, 2023/08/14
- Re: [PATCH v9 02/11] Unify GUID types,
Pedro Miguel Justo <=
- 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
- Re: [PATCH v9 02/11] Unify GUID types, Oliver Steffen, 2023/08/25