Re: [PATCH v3] docs: Add debugging chapter to development documentation
Thu, 15 Jun 2023 13:06:09 +0200 Daniel Kiper : > Right now patch is in the git repo. If you want to improve that part of > doc please send a fix to grub-devel and CC interested folks. Yeah, that is very unfortunate, because it breaks the build: https://lists.gnu.org/archive/html/bug-grub/2023-06/msg6.html 5a3d2b4742dfe4bfe2b51f7b712bc107f75e84ed is the first bad commit For some reason there is no --disable-docs for configure? Olaf pgp0esDXnERiS.pgp Description: Digitale Signatur von OpenPGP ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v9 02/11] Unify GUID types
On Tue, 15 Aug 2023 at 05:42, Vladimir 'phcoder' Serbinenko
wrote:
>>
>> .
>>
>> Ard thinks that a 64 bit alignment for EFI GUIDs is a mistake - see [1]
>> and [2]. Hence Linux uses `__aligned(__alignof__(u32))` for "efi_guid_t"
>> since 2019.
>
>
> My patch presents a reliability paradigm: accept all inputs, produce outputs
> that are always aligned to 8-byte. It can make sense even though I agree that
> requiring 8-byte alignment for UUID is weird. Yet it's difficult to know if
> some obscure firmware does rely on it in some one in a thousand edge case
+1
The Linux side change ensures that the OS does not present misaligned
GUIDs to the firmware.
The problem with 8-byte alignment is that it deviates from EDK2, and
could result in struct layout changes due to padding. Given that EDK2
uses 32-bit alignment only, some of the struct types it defines might
suddenly need the __packed attribute on 32-bit builds if the alignment
of the GUID type is increased to 64 bits.
E.g.,
struct {
void *
guid
}
will have no padding in 32-bit EDK2 based firmware builds, even if the
spec claims that GUIDs are 64-bit aligned.
So the lowest risk change for Linux was to increase to 32-bit
alignment, as it fixes any potential issues with misaligned
load-multiple instructions on ARM, and other architectures don't care
that much about alignment anyway.
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v9 02/11] Unify GUID types
On 8/15/23 10:17, Ard Biesheuvel wrote:
> On Tue, 15 Aug 2023 at 05:42, Vladimir 'phcoder' Serbinenko
> wrote:
>>>
>>> .
>>>
>>> Ard thinks that a 64 bit alignment for EFI GUIDs is a mistake - see [1]
>>> and [2]. Hence Linux uses `__aligned(__alignof__(u32))` for "efi_guid_t"
>>> since 2019.
>>
>>
>> My patch presents a reliability paradigm: accept all inputs, produce outputs
>> that are always aligned to 8-byte. It can make sense even though I agree
>> that requiring 8-byte alignment for UUID is weird. Yet it's difficult to
>> know if some obscure firmware does rely on it in some one in a thousand edge
>> case
>
> +1
>
> The Linux side change ensures that the OS does not present misaligned
> GUIDs to the firmware.
>
> The problem with 8-byte alignment is that it deviates from EDK2, and
> could result in struct layout changes due to padding. Given that EDK2
> uses 32-bit alignment only, some of the struct types it defines might
> suddenly need the __packed attribute on 32-bit builds if the alignment
> of the GUID type is increased to 64 bits.
>
> E.g.,
>
> struct {
> void *
> guid
> }
>
> will have no padding in 32-bit EDK2 based firmware builds, even if the
> spec claims that GUIDs are 64-bit aligned.
>
> So the lowest risk change for Linux was to increase to 32-bit
> alignment, as it fixes any potential issues with misaligned
> load-multiple instructions on ARM, and other architectures don't care
> that much about alignment anyway.
>
This is terrible.
First, the Itanium firmware in question clearly enforces the 8-byte
alignment. (BTW, have we checked Itanium with a 4-byte but not 8-byte
alignment?) Edk2 has killed off Itanium support, but the UEFI spec still
specifies a binding for Itanium. And, we're having this discussion in
the first place because of Itanium. So we need to make up our minds
about adhering to the 8-byte alignment or not.
Second, the fact that edk2 does not align EFI_GUID to 8 bytes, contrary
to the spec, is absolutely terrible. I don't understand then how it
worked on Itanium before Itanium support was killed.
What I can imagine is the following situation:
- the UEFI spec does not match reality (the reference implementation),
and the actually required alignment is 4 bytes only
- Itanium is happy with the 4 bytes alignment too (IIUC grub's malloc
aligns at 1 byte by default, so the 4-byte alignment could still be
violated in practice)
I'd really like if edk2 changed EFI_GUID to match the spec, but a
git-grep for EFI_GUID in edk2 header files returns 800+ lines. Noone
ever will audit all those lines, to check for the potantial padding
increase that you raise. Absolutely terrible.
Either way, I'd say grub still needs two distinct types, one packed
*and* aligned (at 4 bytes), and another only packed (but not aligned).
Laszlo
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH] templates/linux_xen: Fix XSM entries generation
It turns out that setting $xen_version in linux_entry_xsm() override
$xen_version in the loop over $xen_list. This means that only one
entry per Xen version is going to enable XSM, but all further entries
are going to have "(XSM enabled)" in their titles without enabling
XSM.
When a "xenpolicy-$xen_version" file was found for the current
$xen_version, it would be overwrite $xen_version to add "(XSM
enabled)" to the menu entry title. Once change, the next call to
linux_entry_xsm() would also have this modified $xen_version and would
look for the file "xenpolicy-*(XSM enabled)" and fail.
Signed-off-by: Anthony PERARD
---
util/grub.d/20_linux_xen.in | 16
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/util/grub.d/20_linux_xen.in b/util/grub.d/20_linux_xen.in
index e46b757da..94dd8be13 100644
--- a/util/grub.d/20_linux_xen.in
+++ b/util/grub.d/20_linux_xen.in
@@ -98,7 +98,7 @@ linux_entry_xsm ()
{
os="$1"
version="$2"
- xen_version="$3"
+ entry_xen_version="$3"
type="$4"
args="$5"
xen_args="$6"
@@ -107,25 +107,25 @@ linux_entry_xsm ()
# corresponding policy file.
xenpolicy=
if ${xsm} ; then
- xenpolicy="xenpolicy-$xen_version"
+ xenpolicy="xenpolicy-$entry_xen_version"
if test ! -e "${xen_dirname}/${xenpolicy}" ; then
return
fi
xen_args="$xen_args flask=enforcing"
- xen_version="$(gettext_printf "%s (XSM enabled)" "$xen_version")"
- # xen_version is used for messages only; actual file is xen_basename
+ entry_xen_version="$(gettext_printf "%s (XSM enabled)"
"$entry_xen_version")"
+ # entry_xen_version is used for messages only; actual file is
xen_basename
fi
if [ -z "$boot_device_id" ]; then
boot_device_id="$(grub_get_device_id "${GRUB_DEVICE}")"
fi
if [ x$type != xsimple ] ; then
if [ x$type = xrecovery ] ; then
- title="$(gettext_printf "%s, with Xen %s and Linux %s (recovery
mode)" "${os}" "${xen_version}" "${version}")"
+ title="$(gettext_printf "%s, with Xen %s and Linux %s (recovery
mode)" "${os}" "${entry_xen_version}" "${version}")"
else
- title="$(gettext_printf "%s, with Xen %s and Linux %s" "${os}"
"${xen_version}" "${version}")"
+ title="$(gettext_printf "%s, with Xen %s and Linux %s" "${os}"
"${entry_xen_version}" "${version}")"
fi
replacement_title="$(echo "Advanced options for ${OS}" | sed
's,>,>>,g')>$(echo "$title" | sed 's,>,>>,g')"
- if [ x"Xen ${xen_version}>$title" = x"$GRUB_ACTUAL_DEFAULT" ]; then
+ if [ x"Xen ${entry_xen_version}>$title" = x"$GRUB_ACTUAL_DEFAULT" ]; then
quoted="$(echo "$GRUB_ACTUAL_DEFAULT" | grub_quote)"
title_correction_code="${title_correction_code}if [ \"x\$default\" =
'$quoted' ]; then default='$(echo "$replacement_title" | grub_quote)'; fi;"
grub_warn "$(gettext_printf "Please don't use old title \`%s' for
GRUB_DEFAULT, use \`%s' (for versions before 2.00) or \`%s' (for 2.00 or
later)" "$GRUB_ACTUAL_DEFAULT" "$replacement_title"
"gnulinux-advanced-$boot_device_id>gnulinux-$version-$type-$boot_device_id")"
@@ -143,7 +143,7 @@ linux_entry_xsm ()
prepare_boot_cache="$(prepare_grub_to_access_device ${GRUB_DEVICE_BOOT} |
grub_add_tab)"
fi
printf '%s\n' "${prepare_boot_cache}" | sed "s/^/$submenu_indentation/"
- xmessage="$(gettext_printf "Loading Xen %s ..." ${xen_version})"
+ xmessage="$(gettext_printf "Loading Xen %s ..." ${entry_xen_version})"
lmessage="$(gettext_printf "Loading Linux %s ..." ${version})"
sed "s/^/$submenu_indentation/" << EOF
echo'$(echo "$xmessage" | grub_quote)'
--
Anthony PERARD
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v9 02/11] Unify GUID types
Le mar. 15 août 2023, 14:00, Laszlo Ersek a écrit :
> On 8/15/23 10:17, Ard Biesheuvel wrote:
> > On Tue, 15 Aug 2023 at 05:42, Vladimir 'phcoder' Serbinenko
> > wrote:
> >>>
> >>> .
> >>>
> >>> Ard thinks that a 64 bit alignment for EFI GUIDs is a mistake - see [1]
> >>> and [2]. Hence Linux uses `__aligned(__alignof__(u32))` for
> "efi_guid_t"
> >>> since 2019.
> >>
> >>
> >> My patch presents a reliability paradigm: accept all inputs, produce
> outputs that are always aligned to 8-byte. It can make sense even though I
> agree that requiring 8-byte alignment for UUID is weird. Yet it's difficult
> to know if some obscure firmware does rely on it in some one in a thousand
> edge case
> >
> > +1
> >
> > The Linux side change ensures that the OS does not present misaligned
> > GUIDs to the firmware.
> >
> > The problem with 8-byte alignment is that it deviates from EDK2, and
> > could result in struct layout changes due to padding.
See my audit in previous message
> > Given that EDK2
> > uses 32-bit alignment only, some of the struct types it defines might
> > suddenly need the __packed attribute on 32-bit builds if the alignment
> > of the GUID type is increased to 64 bits.
> >
> > E.g.,
> >
> > struct {
> > void *
> > guid
> > }
> >
> > will have no padding in 32-bit EDK2 based firmware builds, even if the
> > spec claims that GUIDs are 64-bit aligned.
> >
Yes, this is weird but in case of reference implementation contradicting
spec, then reference implementation takes priority in practice.
>
> > So the lowest risk change for Linux was to increase to 32-bit
> > alignment, as it fixes any potential issues with misaligned
> > load-multiple instructions on ARM, and other architectures don't care
> > that much about alignment anyway.
> >
>
> This is terrible.
>
> First, the Itanium firmware in question clearly enforces the 8-byte
> alignment. (BTW, have we checked Itanium with a 4-byte but not 8-byte
> alignment?)
No, we didn't AFAIK
> specifies a binding for Itanium. And, we're having this discussion in
> the first place because of Itanium. So we need to make up our minds
> about adhering to the 8-byte alignment or not.
>
I'm biased towards allocating at 8-byte alignment but accepting any
alignment
>
> Second, the fact that edk2 does not align EFI_GUID to 8 bytes, contrary
> to the spec, is absolutely terrible. I don't understand then how it
> worked on Itanium before Itanium support was killed.
>
No members in guid have uint64_t type so most likely only 32-bit alignment
was used in most cases. 64-bit alignment allows accessing guid as 2
uint64_t's but I doubt it was ever widely used. A rare use might still
happen though.
>
> What I can imagine is the following situation:
> - the UEFI spec does not match reality (the reference implementation),
> and the actually required alignment is 4 bytes only
> - Itanium is happy with the 4 bytes alignment too (IIUC grub's malloc
> aligns at 1 byte by default, so the 4-byte alignment could still be
> violated in practice)
>
grub-malloc is 16-byte aligned.
The problem is that stack and static allocations are aligned only to type
alignment.
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v3] docs: Add debugging chapter to development documentation
On Tue, 15 Aug 2023 10:14:07 +0200 Olaf Hering wrote: > Thu, 15 Jun 2023 13:06:09 +0200 Daniel Kiper : > > > Right now patch is in the git repo. If you want to improve that part of > > doc please send a fix to grub-devel and CC interested folks. > > Yeah, that is very unfortunate, because it breaks the build: > https://lists.gnu.org/archive/html/bug-grub/2023-06/msg6.html > > 5a3d2b4742dfe4bfe2b51f7b712bc107f75e84ed is the first bad commit > > For some reason there is no --disable-docs for configure? I'm not seeing this issue when building on Debian 11. I'm successfully building the info, html, and pdf documentation from the texi files using debians texinfo package at version 6.7.0.dfsg.2-6. Are you sure you are building from clean sources? What version of texinfo are you at? Glenn ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v3] docs: Add debugging chapter to development documentation
Tue, 15 Aug 2023 12:31:29 -0500 Glenn Washburn : > I'm not seeing this issue when building on Debian 11. I'm successfully > building the info, html, and pdf documentation from the texi files > using debians texinfo package at version 6.7.0.dfsg.2-6. Are you sure > you are building from clean sources? What version of texinfo are you at? The sources are clean, makeinfo 4.13a is used. Olaf pgp7oSGrVuBL3.pgp Description: Digitale Signatur von OpenPGP ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v3] docs: Add debugging chapter to development documentation
On Tue, 15 Aug 2023 22:37:08 +0200 Olaf Hering wrote: > Tue, 15 Aug 2023 12:31:29 -0500 Glenn Washburn : > > > I'm not seeing this issue when building on Debian 11. I'm successfully > > building the info, html, and pdf documentation from the texi files > > using debians texinfo package at version 6.7.0.dfsg.2-6. Are you sure > > you are building from clean sources? What version of texinfo are you at? > > The sources are clean, makeinfo 4.13a is used. Is that a 6+ year old version[1]? Here's my output: $ makeinfo --version texi2any (GNU texinfo) 6.7 Anyway, since you're the first and *so far* only person seeing this, could you submit a patch that fixes this for you? It would be great to support older makeinfos (if that is indeed the issue) in the upcoming release. Glenn [1] https://lists.gnu.org/archive/html/bug-gnulib/2016-06/msg00017.html ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
