[Solved] Re: How to test the git clone without "make install" ?
Hi,
i wrote:
> > So i will start a new thread with the question:
> > How do i convince the git clone to produce programs and ISO for 64 bit
> > EFI.
Vladimir 'phcoder' Serbinenko wrote:
> ./configure --with-platform=efi --target=x86_64
Fast and concise as ever. :))
Thanks, Vladimir.
It works if i do beforehand
make clean
Without "make clean" i get from the subsequent "make":
symlist.h:30:10: fatal error: ../include/grub/machine/pxe.h: No such file or
directory
30 | #include <../include/grub/machine/pxe.h>
Trying to revoke the change by
./configure && make
yields:
symlist.c: In function ‘grub_register_exported_symbols’:
symlist.c:66:31: error: ‘grub_acpi_find_fadt’ undeclared (first use in this
function)
66 | {"grub_acpi_find_fadt", grub_acpi_find_fadt, 1},
---
Well, now that i know the magic words, i find them in the manual under
"8 Porting".
Possibly it would be worthwhile to put some of this into a new chapter
"1.1 Building from the git clone"
together with a warning about re-configuring without clean-making.
Have a nice day :)
Thomas
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [Solved] Re: How to test the git clone without "make install" ?
Le ven. 14 juin 2024, 19:27, Thomas Schmitt via Grub-devel < grub-devel@gnu.org> a écrit : > Hi, > > Maximilian Stendler wrote: > > to keep the host installation clean, I would probably use a container. > > Yes, a virtual machine came to my mind. Easy to clone and to dispose. > But there must be some better way to test a utility built from git > independenly of systemwide directories. > > > Vladimir 'phcoder' Serbinenko wrote: > > Set pkgdatadir environment variable > > Ahum ... > rm /usr/local/share/grub > pkgdatadir=. ./grub-mkrescue -o /dvdbuffer/test.iso > yields indeed an ISO with EFI boot equipment. > > But what to do about /usr/local/lib/grub ? > I found option -d meanwhile. After some forth and back i came to > > > pkgdatadir=. ./grub-mkrescue -o /dvdbuffer/test.iso -d ./grub-core > > > which to my surprise creates an ISO with boot equipment for legacy BIOS: > > $ xorriso -indev /dvdbuffer/test.iso -report_el_torito plain > -report_system_area plain > ... > El Torito images : N Pltf B Emul Ld_seg Hdpt Ldsiz LBA > El Torito boot img : 1 BIOS y none 0x 0x00 41397 > El Torito img path : 1 /boot/grub/i386-pc/eltorito.img > El Torito img opts : 1 boot-info-table grub2-boot-info > ... > System area summary: MBR protective-msdos-label grub2-mbr cyl-align-off > ... > MBR partition table: N Status TypeStart Blocks > MBR partition : 1 0x80 0xcd113783 > > While i used the Debian system directories it was EFI: > > El Torito images : N Pltf B Emul Ld_seg Hdpt Ldsiz LBA > El Torito boot img : 1 UEFI y none 0x 0x00 5760 52 > El Torito img path : 1 /efi.img > ... > System area summary: MBR protective-msdos-label cyl-align-off GPT APM > ... > MBR partition table: N Status TypeStart Blocks > MBR partition : 1 0x00 0xee118015 > ... and a GPT and an Apple Partition Map for HFS+ ... > > > So i will start a new thread with the question: > How do i convince the git clone to produce programs and ISO for 64 bit > EFI. > ./configure --with-platform=efi --target=x86_64 > > > > Have a nice day :) > > Thomas > > > ___ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel > ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[Solved] Re: How to test the git clone without "make install" ?
Hi, Maximilian Stendler wrote: > to keep the host installation clean, I would probably use a container. Yes, a virtual machine came to my mind. Easy to clone and to dispose. But there must be some better way to test a utility built from git independenly of systemwide directories. Vladimir 'phcoder' Serbinenko wrote: > Set pkgdatadir environment variable Ahum ... rm /usr/local/share/grub pkgdatadir=. ./grub-mkrescue -o /dvdbuffer/test.iso yields indeed an ISO with EFI boot equipment. But what to do about /usr/local/lib/grub ? I found option -d meanwhile. After some forth and back i came to pkgdatadir=. ./grub-mkrescue -o /dvdbuffer/test.iso -d ./grub-core which to my surprise creates an ISO with boot equipment for legacy BIOS: $ xorriso -indev /dvdbuffer/test.iso -report_el_torito plain -report_system_area plain ... El Torito images : N Pltf B Emul Ld_seg Hdpt Ldsiz LBA El Torito boot img : 1 BIOS y none 0x 0x00 41397 El Torito img path : 1 /boot/grub/i386-pc/eltorito.img El Torito img opts : 1 boot-info-table grub2-boot-info ... System area summary: MBR protective-msdos-label grub2-mbr cyl-align-off ... MBR partition table: N Status TypeStart Blocks MBR partition : 1 0x80 0xcd113783 While i used the Debian system directories it was EFI: El Torito images : N Pltf B Emul Ld_seg Hdpt Ldsiz LBA El Torito boot img : 1 UEFI y none 0x 0x00 5760 52 El Torito img path : 1 /efi.img ... System area summary: MBR protective-msdos-label cyl-align-off GPT APM ... MBR partition table: N Status TypeStart Blocks MBR partition : 1 0x00 0xee118015 ... and a GPT and an Apple Partition Map for HFS+ ... So i will start a new thread with the question: How do i convince the git clone to produce programs and ISO for 64 bit EFI. Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 1/2] loader/efi/fdt: Add fdtdump command to access device tree
On Fri, Jun 14, 2024 at 06:26:00PM +0200, Tobias Heider wrote:
> On Fri, Jun 14, 2024 at 06:03:23PM +0200, Daniel Kiper wrote:
> > On Wed, Jun 12, 2024 at 01:12:28PM +0200, Tobias Heider wrote:
> > > The fdtdump command allows dumping arbitrary device tree properties
> > > and saving them to a variable similar to the smbios command.
> > >
> > > This is useful in scripts where further actions such as selecting a
> > > kernel or loading another device tree depend on the compatible or
> > > model values of the device tree provided by the firmware.
> > >
> > > For now only the root level properties of the dtb are exposed.
> > >
> > > Signed-off-by: Tobias Heider
> > > ---
> > > grub-core/loader/efi/fdt.c | 51 +-
> > > 1 file changed, 50 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/grub-core/loader/efi/fdt.c b/grub-core/loader/efi/fdt.c
> > > index 439964b9c..8fa0b3b09 100644
> > > --- a/grub-core/loader/efi/fdt.c
> > > +++ b/grub-core/loader/efi/fdt.c
> > > @@ -1,6 +1,7 @@
> > > /*
> > > * GRUB -- GRand Unified Bootloader
> > > * Copyright (C) 2013-2015 Free Software Foundation, Inc.
> > > + * Copyright (C) 2024 Canonical, Ltd.
> > > *
> > > * GRUB is free software: you can redistribute it and/or modify
> > > * it under the terms of the GNU General Public License as published by
> > > @@ -18,15 +19,18 @@
> > >
> > > #include
> > > #include
> > > +#include
> > > #include
> > > #include
> > > #include
> > > +#include
> > > #include
> > > #include
> > > #include
> > > #include
> > > #include
> > >
> > > +static void *fw_fdt;
> > > static void *loaded_fdt;
> > > static void *fdt;
> > >
> > > @@ -36,6 +40,13 @@ static void *fdt;
> > > sizeof (FDT_ADDR_CELLS_STRING) + \
> > > sizeof (FDT_SIZE_CELLS_STRING))
> > >
> > > +static const struct grub_arg_option options_fdtdump[] = {
> > > + {"prop", 'p', 0, N_("Get property."), N_("prop"),
> > > ARG_TYPE_STRING},
> > > + {"set", '\0', 0, N_("Store the value in the given variable
> > > name."),
> > > + N_("variable"), ARG_TYPE_STRING},
> > > + {0, 0, 0, 0, 0, 0}
> > > +};
> > > +
> > > void *
> > > grub_fdt_load (grub_size_t additional_size)
> > > {
> > > @@ -51,7 +62,7 @@ grub_fdt_load (grub_size_t additional_size)
> > >if (loaded_fdt)
> > > raw_fdt = loaded_fdt;
> > >else
> > > -raw_fdt = grub_efi_get_firmware_fdt();
> > > +raw_fdt = fw_fdt;
> >
> > This change seems suspicious for me. Could you explain why it is needed?
>
> Since I added grub_efi_get_firmware_fdt() to module init function and the
> device tree passed by the firmware is a fairly static property it made
> sense to me to use the address we have instead of rereading it in
> grub_fdt_load().
>
> If you are more comfortable if I don't touch that code path I can drop that
> change and simply read it twice.
I am OK with this grub_efi_get_firmware_fdt() shuffling but it has to be
explained in the commit message.
Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] ofnet: Remove 200 ms timeout in get_card_packet to reduce input latency
On Mon, May 06, 2024 at 10:34:22AM +0800, Michael Chang via Grub-devel wrote: > When grub image is netbooted on ppc64le, the keyboard input exhibits > significant latency, reports even say that characters are processed > about once per second. This issue makes interactively trying to debug a > ppc64le config very difficult. > > It seems that the latency is largely caused by a 200 ms timeout in the > idle event loop, during which the network card interface is consistently > polled for incoming packets. Often, no packets arrive during this > period, so the timeout nearly always expires, which blocks the response > to key inputs. > > Furthermore, this 200 ms timeout might not need to be enforced at this > basic layer, considering that grub performs synchronous reads and its > timeout management is actually handled by higher layers, not directly in > the card instance. Additionally, the idle polling, which reacts to > unsolicited packets like ICMP and SLAAC, would be fine at a less > frequent polling interval, rather than needing a timeout for receiving a > response. > > For these reasons, we believe the timeout in get_card_packet should be > effectively removed. According to test results, the delay has > disappeared, and it is now much easier to use interactively. > > Signed-Off-by: Michael Chang > Tested-by: Tony Jones Reviewed-by: Daniel Kiper Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Add --noefistub option for linux
On Thu, May 16, 2024 at 09:43:46PM +0300, Vladimir Serbinenko wrote:
> In some cases like loading kernel from native disk (e.g. nvme) not
> supported by EFI in question efi stub is not an option. Allow
> user to disable efi stub and fallback to older protocol
I think this patch should be considered together with NVMe patch.
Missing SOB.
> ---
> grub-core/loader/efi/linux.c | 10 +-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/grub-core/loader/efi/linux.c b/grub-core/loader/efi/linux.c
> index bfbd95aee..0bf9d9cbb 100644
> --- a/grub-core/loader/efi/linux.c
> +++ b/grub-core/loader/efi/linux.c
> @@ -459,10 +459,18 @@ grub_cmd_linux (grub_command_t cmd __attribute__
> ((unused)),
>grub_file_t file = 0;
>struct linux_arch_kernel_header lh;
>grub_err_t err;
> + int force_legacy = 0;
I would use bool here.
>grub_dl_ref (my_mod);
>
> - if (grub_is_shim_lock_enabled () == true)
> + if (argc > 0 && grub_strcmp(argv[0], "--noefistub") == 0)
> +{
> + force_legacy = 1;
> + argv++;
> + argc--;
> +}
> +
> + if (grub_is_shim_lock_enabled () == true || force_legacy)
Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 1/2] zfs: Support zstd compression
On Thu, May 16, 2024 at 10:42:25PM +0300, Vladimir Serbinenko wrote:
> Signed-off-by: Vladimir Serbinenko
> ---
> grub-core/Makefile.core.def | 1 +
> grub-core/fs/zfs/zfs.c | 32
> include/grub/zfs/zio.h | 1 +
> 3 files changed, 34 insertions(+)
>
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 8e1b1d9f3..2ba4962d5 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -1596,6 +1596,7 @@ module = {
>common = fs/zfs/zfs_lz4.c;
>common = fs/zfs/zfs_sha256.c;
>common = fs/zfs/zfs_fletcher.c;
> + cppflags = '-I$(srcdir)/lib/posix_wrap -I$(srcdir)/lib/zstd';
> };
>
> module = {
> diff --git a/grub-core/fs/zfs/zfs.c b/grub-core/fs/zfs/zfs.c
> index b5453e006..b8441faef 100644
> --- a/grub-core/fs/zfs/zfs.c
> +++ b/grub-core/fs/zfs/zfs.c
> @@ -57,6 +57,8 @@
> #include
> #include
>
> +#include
> +
> GRUB_MOD_LICENSE ("GPLv3+");
>
> #define ZPOOL_PROP_BOOTFS "bootfs"
> @@ -291,6 +293,7 @@ static const char *spa_feature_names[] = {
>"com.delphix:embedded_data",
>"com.delphix:extensible_dataset",
>"org.open-zfs:large_blocks",
> + "org.freebsd:zstd_compress",
>NULL
> };
>
> @@ -312,6 +315,34 @@ zlib_decompress (void *s, void *d,
>return grub_errno;
> }
>
> +static grub_err_t
> +zstd_decompress (void *ibuf, void *obuf, grub_size_t isize,
> + grub_size_t osize)
> +{
> + grub_size_t zstd_ret;
> + grub_uint8_t *byte_buf = (grub_uint8_t *) ibuf;
> +
> + if (isize < 8)
> + return grub_error (GRUB_ERR_BAD_COMPRESSED_DATA, "zstd data too
> short");
> +
> + grub_uint32_t c_len = grub_be_to_cpu32(grub_get_unaligned32(byte_buf));
May I ask you to define c_len at the beginning of the function?
And please fix coding style for function calls.
> + if (c_len > isize - 8)
> + return grub_error (GRUB_ERR_BAD_COMPRESSED_DATA, "zstd data announced
> size overflow");
> +
> + /* Fix magic number. */
I think the fix should be explained why it is needed.
> + byte_buf[4] = 0x28;
> + byte_buf[5] = 0xb5;
> + byte_buf[6] = 0x2f;
> + byte_buf[7] = 0xfd;
> + zstd_ret = ZSTD_decompress (obuf, osize, byte_buf + 4, c_len + 4);
> +
> + if (ZSTD_isError (zstd_ret))
> +return grub_error (GRUB_ERR_BAD_COMPRESSED_DATA, "zstd data corrupted
> (error %d)", (int) zstd_ret);
> +
> + return GRUB_ERR_NONE;
> +}
Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [RESEND V5] ieee1275/ofdisk: retry on open and read failure
On Mon, Jun 10, 2024 at 11:35:11AM +0530, Mukesh Kumar Chaurasiya wrote:
> Sometimes, when booting from a very busy SAN, the access to the
> disk can fail and then GRUB will eventually drop to GRUB prompt.
> This scenario is more frequent when deploying many machines at
> the same time using the same SAN.
> This patch aims to force the ofdisk module to retry the open or
> read function for network disks excluding after it fails. We use
excluding? Something is off here...
> DEFAULT_RETRY_TIMEOUT, which is 15 seconds to specify the time it'll
> retry to access the disk before it definitely fails. The timeout can be
> changed by setting the environment variable ofdisk_retry_timeout.
> If the environment variable fails to read, GRUB will consider the
> default value of 15 seconds.
>
> Signed-off-by: Diego Domingos
> Signed-off-by: Mukesh Kumar Chaurasiya
> ---
> docs/grub.texi | 8 +++
> grub-core/disk/ieee1275/ofdisk.c | 91 ++--
> 2 files changed, 96 insertions(+), 3 deletions(-)
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index f3bdc2564..9514271fc 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -3308,6 +3308,7 @@ These variables have special meaning to GRUB.
> * net_default_ip::
> * net_default_mac::
> * net_default_server::
> +* ofdisk_retry_timeout::
> * pager::
> * prefix::
> * pxe_blksize::
> @@ -3738,6 +3739,13 @@ The default is the value of @samp{color_normal}
> (@pxref{color_normal}).
> @xref{Network}.
>
>
> +@node ofdisk_retry_timeout
> +@subsection ofdisk_retry_timeout
> +
> +The time in seconds till which the GRUB will retry to open or read a disk in
> +case of failure to do so. This value defaults to 15 seconds.
> +
> +
> @node pager
> @subsection pager
>
> diff --git a/grub-core/disk/ieee1275/ofdisk.c
> b/grub-core/disk/ieee1275/ofdisk.c
> index c6cba0c8a..d90b9b70b 100644
> --- a/grub-core/disk/ieee1275/ofdisk.c
> +++ b/grub-core/disk/ieee1275/ofdisk.c
> @@ -24,6 +24,9 @@
> #include
> #include
> #include
> +#include
> +
> +#define RETRY_DEFAULT_TIMEOUT 15
>
> static char *last_devpath;
> static grub_ieee1275_ihandle_t last_ihandle;
> @@ -452,7 +455,7 @@ compute_dev_path (const char *name)
> }
>
> static grub_err_t
> -grub_ofdisk_open (const char *name, grub_disk_t disk)
> +grub_ofdisk_open_real (const char *name, grub_disk_t disk)
> {
>grub_ieee1275_phandle_t dev;
>char *devpath;
> @@ -525,6 +528,61 @@ grub_ofdisk_open (const char *name, grub_disk_t disk)
>return 0;
> }
>
> +static grub_uint64_t
> +grub_ofdisk_disk_timeout (grub_disk_t disk)
> +{
> + grub_uint64_t retry = RETRY_DEFAULT_TIMEOUT;
> + const char *timeout = grub_env_get ("ofdisk_retry_timeout");
> + const char *timeout_end;
> +
> + if (grub_strstr (disk->name, "fibre-channel") != NULL ||
> + grub_strstr (disk->name, "vfc-client") != NULL)
> +{
> + if (timeout == NULL)
> +{
> + return retry;
> +}
Please drop redundant curly braces.
> + retry = grub_strtoul (timeout, &timeout_end, 10);
> + /* Ignore all errors and return default timeout */
> + if (grub_errno != GRUB_ERR_NONE ||
You can drop grub_errno check from here.
> + *timeout == '\0' ||
> + *timeout_end != '\0')
> +{
> + return RETRY_DEFAULT_TIMEOUT;
> +}
Please drop redundant curly braces.
> +}
> + else
> +return 0;
> +
> + return retry;
> +}
> +
> +static grub_err_t
> +grub_ofdisk_open (const char *name, grub_disk_t disk)
> +{
> + grub_err_t err;
> + grub_uint64_t timeout = grub_get_time_ms () + (grub_ofdisk_disk_timeout
> (disk) * 1000);
> + grub_uint16_t inc = 0;
> +
> + do
> +{
> + err = grub_ofdisk_open_real (name, disk);
> + if (err == GRUB_ERR_UNKNOWN_DEVICE)
> +{
> + grub_dprintf ("ofdisk", "Failed to open disk %s.\n", name);
> +}
Please drop redundant curly braces here and below in similar cases...
> + if (grub_get_time_ms () >= timeout)
> +break;
> + grub_dprintf ("ofdisk", "Retry to open disk %s.\n", name);
> + /*
> + * Increase in wait time for subsequent requests
s/in //
> + * Cur time is used as a random number here
s/Cur time is used as a random number here/Current time is used as a source of
randomness./
> + */
> + grub_millisleep ((32 << ++inc) * (grub_get_time_ms () % 32));
> +} while (1);
> + return err;
> +}
> +
> static void
> grub_ofdisk_close (grub_disk_t disk)
> {
> @@ -568,8 +626,8 @@ grub_ofdisk_prepare (grub_disk_t disk, grub_disk_addr_t
> sector)
> }
>
> static grub_err_t
> -grub_ofdisk_read (grub_disk_t disk, grub_disk_addr_t sector,
> - grub_size_t size, char *buf)
> +grub_ofdisk_read_real (grub_disk_t disk, grub_disk_addr_t sector,
> + grub_size_t size, char *buf)
> {
>grub_err_t err;
>grub_ssize_t actual;
> @@ -587,6 +645,33 @@ grub_ofdisk_read (grub_disk_t disk, grub_disk_addr
Re: [RESEND V2] ieee1275/ofdisk: vscsi lun handling on lun len
On Mon, Jun 10, 2024 at 11:29:56AM +0530, Mukesh Kumar Chaurasiya wrote:
> The information about "vscsi-report-luns" data is a list of disk details
> with pairs of memory addresses and lengths.
>
> 8 bytes 8 bytes
> lun-addr ---> 8 bytes
> ^| buf-addr | lun-count| > -
> | | lun |
> || buf-addr | lun-count| | -
> "len" | | ... |
> ||... | | -
> | | | lun |
> || buf-addr | lun-count| | -
> V |
> |---> -
> | lun |
> -
> | ... |
> -
> | lun |
> -
> The way the expression (args.table + 4 + 8 * i) is used is incorrect and
> can be confusing. The list of LUNs doesn't end with NULL, indicated by
> while (*ptr). Usually, this loop doesn't process any LUNs because it ends
> before checking any as first reported LUN is likely to be 0. The list of
> LUNs ends based on its length, not by a NULL value.
>
> Signed-off-by: Mukesh Kumar Chaurasiya
> ---
> grub-core/disk/ieee1275/ofdisk.c | 27 ---
> 1 file changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/grub-core/disk/ieee1275/ofdisk.c
> b/grub-core/disk/ieee1275/ofdisk.c
> index c6cba0c8a..1618544a8 100644
> --- a/grub-core/disk/ieee1275/ofdisk.c
> +++ b/grub-core/disk/ieee1275/ofdisk.c
> @@ -222,8 +222,12 @@ dev_iterate (const struct grub_ieee1275_devalias *alias)
> grub_ieee1275_cell_t table;
>}
>args;
> + struct lun_buf {
> +grub_uint64_t *buf_addr;
Again, this is wrong taking into account diagram above. I think it
should be "grub_uint64_t buf_addr". Then you should convert it to the
pointer. Though please be cautious on the 32-bit platforms...
> +grub_uint64_t lun_count;
> + } *tbl;
>char *buf, *bufptr;
> - unsigned i;
> + unsigned int i, j;
>
>if (grub_ieee1275_open (alias->path, &ihandle))
> return;
> @@ -248,17 +252,18 @@ dev_iterate (const struct grub_ieee1275_devalias *alias)
> return;
>bufptr = grub_stpcpy (buf, alias->path);
>
> + tbl = (struct lun_len *) args.table;
>for (i = 0; i < args.nentries; i++)
> - {
> - grub_uint64_t *ptr;
> -
> - ptr = *(grub_uint64_t **) (args.table + 4 + 8 * i);
> - while (*ptr)
> - {
> - grub_snprintf (bufptr, 32, "/disk@%" PRIxGRUB_UINT64_T, *ptr++);
> - dev_iterate_real (buf, buf);
> - }
> - }
> +{
> + grub_uint64_t *ptr;
> +
> + ptr = (grub_uint64_t *)(grub_addr_t) tbl[i].buf_addr;
> + for (j = 0; j < tbl[i].lun_count; j++)
> + {
> + grub_snprintf (bufptr, 32, "/disk@%" PRIxGRUB_UINT64_T, *ptr++);
> + dev_iterate_real (buf, buf);
> + }
> +}
>grub_ieee1275_close (ihandle);
>grub_free (buf);
>return;
Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 1/2] loader/efi/fdt: Add fdtdump command to access device tree
On Fri, Jun 14, 2024 at 06:03:23PM +0200, Daniel Kiper wrote:
> On Wed, Jun 12, 2024 at 01:12:28PM +0200, Tobias Heider wrote:
> > The fdtdump command allows dumping arbitrary device tree properties
> > and saving them to a variable similar to the smbios command.
> >
> > This is useful in scripts where further actions such as selecting a
> > kernel or loading another device tree depend on the compatible or
> > model values of the device tree provided by the firmware.
> >
> > For now only the root level properties of the dtb are exposed.
> >
> > Signed-off-by: Tobias Heider
> > ---
> > grub-core/loader/efi/fdt.c | 51 +-
> > 1 file changed, 50 insertions(+), 1 deletion(-)
> >
> > diff --git a/grub-core/loader/efi/fdt.c b/grub-core/loader/efi/fdt.c
> > index 439964b9c..8fa0b3b09 100644
> > --- a/grub-core/loader/efi/fdt.c
> > +++ b/grub-core/loader/efi/fdt.c
> > @@ -1,6 +1,7 @@
> > /*
> > * GRUB -- GRand Unified Bootloader
> > * Copyright (C) 2013-2015 Free Software Foundation, Inc.
> > + * Copyright (C) 2024 Canonical, Ltd.
> > *
> > * GRUB is free software: you can redistribute it and/or modify
> > * it under the terms of the GNU General Public License as published by
> > @@ -18,15 +19,18 @@
> >
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > #include
> > #include
> >
> > +static void *fw_fdt;
> > static void *loaded_fdt;
> > static void *fdt;
> >
> > @@ -36,6 +40,13 @@ static void *fdt;
> > sizeof (FDT_ADDR_CELLS_STRING) + \
> > sizeof (FDT_SIZE_CELLS_STRING))
> >
> > +static const struct grub_arg_option options_fdtdump[] = {
> > + {"prop", 'p', 0, N_("Get property."), N_("prop"), ARG_TYPE_STRING},
> > + {"set", '\0', 0, N_("Store the value in the given variable name."),
> > + N_("variable"), ARG_TYPE_STRING},
> > + {0, 0, 0, 0, 0, 0}
> > +};
> > +
> > void *
> > grub_fdt_load (grub_size_t additional_size)
> > {
> > @@ -51,7 +62,7 @@ grub_fdt_load (grub_size_t additional_size)
> >if (loaded_fdt)
> > raw_fdt = loaded_fdt;
> >else
> > -raw_fdt = grub_efi_get_firmware_fdt();
> > +raw_fdt = fw_fdt;
>
> This change seems suspicious for me. Could you explain why it is needed?
Since I added grub_efi_get_firmware_fdt() to module init function and the
device tree passed by the firmware is a fairly static property it made
sense to me to use the address we have instead of rereading it in
grub_fdt_load().
If you are more comfortable if I don't touch that code path I can drop that
change and simply read it twice.
>
> >if (raw_fdt)
> >size = grub_fdt_get_totalsize (raw_fdt);
> > @@ -167,10 +178,47 @@ out:
> >return grub_errno;
> > }
> >
> > +static grub_err_t
> > +grub_cmd_fdtdump (grub_extcmd_context_t ctxt,
> > + int argc __attribute__ ((unused)),
> > + char **argv __attribute__ ((unused)))
> > +{
> > + struct grub_arg_list *state = ctxt->state;
> > + const char *value = NULL;
> > +
> > + if (fw_fdt == NULL)
> > + return grub_error (GRUB_ERR_IO,
> > + N_("No device tree found"));
> > +
> > + if (state[0].set)
> > +{
> > + value = grub_fdt_get_prop(fw_fdt, 0, state[0].arg, NULL);
>
> Missing space before "(".
>
> > +}
>
> Please drop redundant curly braces.
Thanks for the review, I'll send an update with those fixed and the two commits
merged.
>
> > + if (value == NULL)
> > +return grub_error (GRUB_ERR_OUT_OF_RANGE,
> > + N_("failed to retrieve the prop field"));
> > +
> > + if (state[1].set)
> > +grub_env_set (state[1].arg, value);
> > + else
> > +grub_printf ("%s\n", value);
> > +
> > + return GRUB_ERR_NONE;
> > +}
>
> Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 2/2] docs: document fdtdump command
On Wed, Jun 12, 2024 at 01:12:29PM +0200, Tobias Heider wrote:
> Signed-off-by: Tobias Heider
> ---
> docs/grub.texi | 26 ++
> 1 file changed, 26 insertions(+)
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index f3bdc2564..a050dc0fc 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -4373,6 +4373,7 @@ you forget a command, you can run the command
> @command{help}
> * eval::Evaluate agruments as GRUB commands
> * export:: Export an environment variable
> * false:: Do nothing, unsuccessfully
> +* fdtdump:: Retrieve device tree information
> * fwsetup:: Reboot into the firmware setup menu
> * gdbinfo:: Provide info for debugging with GDB
> * gettext:: Translate a string
> @@ -4904,6 +4905,31 @@ such as @code{if} and @code{while} (@pxref{Shell-like
> scripting}).
> @end deffn
>
>
> +@node fdtdump
> +@subsection fdtdump
> +
> +@deffn Command fdtdump @
> + [@option{--prop} @var{prop}] @
> + [@option{--set} @var{variable}]
> +Retrieve device tree information.
> +
> +The @command{fdtdump} command returns the value of a property in the device
> +tree provided by the firmware. The @option{--prop} option determines which
> +property to select.
> +
> +The default action is to print the value of the requested field to the
> console,
> +but a variable name can be specified with @option{--set} to store the value
> +instead of printing it.
> +
> +For example, this will store and then display the model string.
> +
> +@example
> +fdtdump --prop model --set machine_model
> +echo $machine_model
> +@end example
> +@end deffn
Please merge the doc update with patch #1.
Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 1/2] loader/efi/fdt: Add fdtdump command to access device tree
On Wed, Jun 12, 2024 at 01:12:28PM +0200, Tobias Heider wrote:
> The fdtdump command allows dumping arbitrary device tree properties
> and saving them to a variable similar to the smbios command.
>
> This is useful in scripts where further actions such as selecting a
> kernel or loading another device tree depend on the compatible or
> model values of the device tree provided by the firmware.
>
> For now only the root level properties of the dtb are exposed.
>
> Signed-off-by: Tobias Heider
> ---
> grub-core/loader/efi/fdt.c | 51 +-
> 1 file changed, 50 insertions(+), 1 deletion(-)
>
> diff --git a/grub-core/loader/efi/fdt.c b/grub-core/loader/efi/fdt.c
> index 439964b9c..8fa0b3b09 100644
> --- a/grub-core/loader/efi/fdt.c
> +++ b/grub-core/loader/efi/fdt.c
> @@ -1,6 +1,7 @@
> /*
> * GRUB -- GRand Unified Bootloader
> * Copyright (C) 2013-2015 Free Software Foundation, Inc.
> + * Copyright (C) 2024 Canonical, Ltd.
> *
> * GRUB is free software: you can redistribute it and/or modify
> * it under the terms of the GNU General Public License as published by
> @@ -18,15 +19,18 @@
>
> #include
> #include
> +#include
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> #include
> #include
>
> +static void *fw_fdt;
> static void *loaded_fdt;
> static void *fdt;
>
> @@ -36,6 +40,13 @@ static void *fdt;
> sizeof (FDT_ADDR_CELLS_STRING) + \
> sizeof (FDT_SIZE_CELLS_STRING))
>
> +static const struct grub_arg_option options_fdtdump[] = {
> + {"prop", 'p', 0, N_("Get property."), N_("prop"), ARG_TYPE_STRING},
> + {"set", '\0', 0, N_("Store the value in the given variable name."),
> + N_("variable"), ARG_TYPE_STRING},
> + {0, 0, 0, 0, 0, 0}
> +};
> +
> void *
> grub_fdt_load (grub_size_t additional_size)
> {
> @@ -51,7 +62,7 @@ grub_fdt_load (grub_size_t additional_size)
>if (loaded_fdt)
> raw_fdt = loaded_fdt;
>else
> -raw_fdt = grub_efi_get_firmware_fdt();
> +raw_fdt = fw_fdt;
This change seems suspicious for me. Could you explain why it is needed?
>if (raw_fdt)
>size = grub_fdt_get_totalsize (raw_fdt);
> @@ -167,10 +178,47 @@ out:
>return grub_errno;
> }
>
> +static grub_err_t
> +grub_cmd_fdtdump (grub_extcmd_context_t ctxt,
> + int argc __attribute__ ((unused)),
> + char **argv __attribute__ ((unused)))
> +{
> + struct grub_arg_list *state = ctxt->state;
> + const char *value = NULL;
> +
> + if (fw_fdt == NULL)
> + return grub_error (GRUB_ERR_IO,
> + N_("No device tree found"));
> +
> + if (state[0].set)
> +{
> + value = grub_fdt_get_prop(fw_fdt, 0, state[0].arg, NULL);
Missing space before "(".
> +}
Please drop redundant curly braces.
> + if (value == NULL)
> +return grub_error (GRUB_ERR_OUT_OF_RANGE,
> + N_("failed to retrieve the prop field"));
> +
> + if (state[1].set)
> +grub_env_set (state[1].arg, value);
> + else
> +grub_printf ("%s\n", value);
> +
> + return GRUB_ERR_NONE;
> +}
Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: How to test the git clone without "make install" ?
Hi Thomas, to keep the host installation clean, I would probably use a container. So if you are familiar with Podman or Docker, take a look at this: https://github.com/stendler/grub-fde/blob/main/Containerfile You could then use a volume mount for any input files from your host and to retrieve the output files. Hope this helps in your case, otherwise someone more knowledgeable than me could chime in. :) Cheers Max On 2024-06-14 16:16, Thomas Schmitt via Grub-devel wrote: > Hi, > > on occasion of > https://savannah.gnu.org/bugs/index.php?65880 > "heap-buffer-overflow in grub-mkrescue.c" > i try to get grub-mkrescue running from git. > > My problem is that grub_util_get_pkglibdir() returns > /usr/local/lib/grub > and grub_util_get_pkgdatadir() returns > /usr/local/share/grub > which of course do not come with a Debian installation. > So grub-mkrescue produces only a very small ISO with no boot lures or > boot programs. Quite unrealistic for testing. > > I was able to overcome this obstacle by > ln -s /usr/lib/grub /usr/local/lib/grub > ln -s /usr/share/grub /usr/local/share/grub > but i understand that now my grub-mkrescue actually copies the ISO content > from the Debian installation and not from the git clone. > > The manual > https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html > does not give me a clue how i would get the /usr/local/*/grub > directories populated with the files made from the git clone. > I guess "make install" would do it for me, but i fear that this does > too many other things to the GRUB installation of my vanilla Debian. > In general i would prefer to keep the git files away from any system > directory. > > So what can i do to make the files built from git available to > ./grub-mkrescue built from git, without frankensteining my Debian 12 ? > > > Have a nice day :) > > Thomas > > > ___ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: How to test the git clone without "make install" ?
Set pkgdatadir environment variable Le ven. 14 juin 2024, 16:17, Thomas Schmitt via Grub-devel < grub-devel@gnu.org> a écrit : > Hi, > > on occasion of > https://savannah.gnu.org/bugs/index.php?65880 > "heap-buffer-overflow in grub-mkrescue.c" > i try to get grub-mkrescue running from git. > > My problem is that grub_util_get_pkglibdir() returns > /usr/local/lib/grub > and grub_util_get_pkgdatadir() returns > /usr/local/share/grub > which of course do not come with a Debian installation. > So grub-mkrescue produces only a very small ISO with no boot lures or > boot programs. Quite unrealistic for testing. > > I was able to overcome this obstacle by > ln -s /usr/lib/grub /usr/local/lib/grub > ln -s /usr/share/grub /usr/local/share/grub > but i understand that now my grub-mkrescue actually copies the ISO content > from the Debian installation and not from the git clone. > > The manual > https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html > does not give me a clue how i would get the /usr/local/*/grub > directories populated with the files made from the git clone. > I guess "make install" would do it for me, but i fear that this does > too many other things to the GRUB installation of my vanilla Debian. > In general i would prefer to keep the git files away from any system > directory. > > So what can i do to make the files built from git available to > ./grub-mkrescue built from git, without frankensteining my Debian 12 ? > > > Have a nice day :) > > Thomas > > > ___ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel > ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
How to test the git clone without "make install" ?
Hi, on occasion of https://savannah.gnu.org/bugs/index.php?65880 "heap-buffer-overflow in grub-mkrescue.c" i try to get grub-mkrescue running from git. My problem is that grub_util_get_pkglibdir() returns /usr/local/lib/grub and grub_util_get_pkgdatadir() returns /usr/local/share/grub which of course do not come with a Debian installation. So grub-mkrescue produces only a very small ISO with no boot lures or boot programs. Quite unrealistic for testing. I was able to overcome this obstacle by ln -s /usr/lib/grub /usr/local/lib/grub ln -s /usr/share/grub /usr/local/share/grub but i understand that now my grub-mkrescue actually copies the ISO content from the Debian installation and not from the git clone. The manual https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html does not give me a clue how i would get the /usr/local/*/grub directories populated with the files made from the git clone. I guess "make install" would do it for me, but i fear that this does too many other things to the GRUB installation of my vanilla Debian. In general i would prefer to keep the git files away from any system directory. So what can i do to make the files built from git available to ./grub-mkrescue built from git, without frankensteining my Debian 12 ? Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
