[PATCH v3 1/2] mm: Adjust new region size to take management overhead into account
When grub_memalign() encounters out-of-memory, it will try
grub_mm_add_region_fn() to request more memory from system firmware.
However, the size passed to it doesn't take region management overhead
into account. Adding a memory area of "size" bytes may result in a heap
region of less than "size" bytes truely avaliable. Thus, the new region
may not be adequate for current allocation request, confusing
out-of-memory handling code.
This patch introduces GRUB_MM_MGMT_OVERHEAD to address the region
management overhead (e.g. metadata, padding). The value of this new
constant must be large enough to make sure grub_malloc(size) always
success after a successful call to grub_mm_init_region(addr, size +
GRUB_MM_MGMT_OVERHEAD), for any given addr and size (assuming no
interger overflow).
The size passed to grub_mm_add_region_fn() is now set to "size + align +
GRUB_MM_MGMT_OVERHEAD", thus if grub_mm_add_region_fn() succeeded,
current allocation request can always success.
Signed-off-by: Zhang Boyang
---
grub-core/kern/mm.c | 54 +++--
1 file changed, 52 insertions(+), 2 deletions(-)
diff --git a/grub-core/kern/mm.c b/grub-core/kern/mm.c
index ae2279133..5c0e5bbbf 100644
--- a/grub-core/kern/mm.c
+++ b/grub-core/kern/mm.c
@@ -83,6 +83,43 @@
+/*
+ * GRUB_MM_MGMT_OVERHEAD is an upper bound of management overhead of
+ * each region, with any possible padding taken into account.
+ *
+ * The value must be large enough to make sure grub_malloc(size) always
+ * success after a successful call to
+ * grub_mm_init_region(addr, size + GRUB_MM_MGMT_OVERHEAD), for any given
+ * addr and size (assuming no interger overflow).
+ *
+ * The worst case which has maximum overhead is shown in the figure below:
+ *
+ * +-- addr
+ * v |<- size ->|
+ * +-+++--+-+
+ * | padding | grub_mm_region | grub_mm_header | usable bytes | padding |
+ * +-+++--+-+
+ * |<- a ->|<- b ->|<- c ->|<-d ->|<- e ->|
+ * ^
+ *addr % GRUB_MM_ALIGN == 1+--/ This will be the pointer
+ *a == GRUB_MM_ALIGN - 1 | returned by next
+ *| grub_malloc(size),
+ *b == sizeof (struct grub_mm_region) | if no other suitable free
+ *c == sizeof (struct grub_mm_header) \ block is available.
+ *
+ *size % GRUB_MM_ALIGN == 1
+ *d == size
+ *e == GRUB_MM_ALIGN - 1
+ *
+ * Therefore, the maximum overhead is:
+ *a + b + c + e == (GRUB_MM_ALIGN - 1) + sizeof (struct grub_mm_region)
+ * + sizeof (struct grub_mm_header) + (GRUB_MM_ALIGN - 1)
+ */
+#define GRUB_MM_MGMT_OVERHEAD ((GRUB_MM_ALIGN - 1) \
+ + sizeof (struct grub_mm_region) \
+ + sizeof (struct grub_mm_header) \
+ + (GRUB_MM_ALIGN - 1))
+
grub_mm_region_t grub_mm_base;
grub_mm_add_region_func_t grub_mm_add_region_fn;
@@ -230,6 +267,11 @@ grub_mm_init_region (void *addr, grub_size_t size)
grub_dprintf ("regions", "No: considering a new region at %p of size %"
PRIxGRUB_SIZE "\n",
addr, size);
+ /*
+ * If you want to modify the code below, please also take a look at
+ * GRUB_MM_MGMT_OVERHEAD and make sure it is synchronized with the code.
+ */
+
/* Allocate a region from the head. */
r = (grub_mm_region_t) ALIGN_UP ((grub_addr_t) addr, GRUB_MM_ALIGN);
@@ -410,6 +452,7 @@ grub_memalign (grub_size_t align, grub_size_t size)
{
grub_mm_region_t r;
grub_size_t n = ((size + GRUB_MM_ALIGN - 1) >> GRUB_MM_ALIGN_LOG2) + 1;
+ grub_size_t grow;
int count = 0;
if (!grub_mm_base)
@@ -424,6 +467,13 @@ grub_memalign (grub_size_t align, grub_size_t size)
if ((size + align) > ~(grub_size_t) 0x10)
goto fail;
+ /*
+ * Pre-calculate the size of heap growth (if applicable),
+ * with region management overhead taken into account.
+ */
+ if (grub_add (size + align, GRUB_MM_MGMT_OVERHEAD, &grow))
+goto fail;
+
align = (align >> GRUB_MM_ALIGN_LOG2);
if (align == 0)
align = 1;
@@ -447,7 +497,7 @@ grub_memalign (grub_size_t align, grub_size_t size)
count++;
if (grub_mm_add_region_fn != NULL &&
- grub_mm_add_region_fn (size, GRUB_MM_ADD_REGION_CONSECUTIVE) ==
GRUB_ERR_NONE)
+ grub_mm_add_region_fn (grow, GRUB_MM_ADD_REGION_CONSECUTIVE) ==
GRUB_ERR_NONE)
goto again;
/* fallthrough */
@@ -462,7 +512,7 @@ grub_memalign (grub_size_t align, grub_size_t size)
* Try again even if this fails, in case it was able to partially
* satisfy the request
*/
- grub_mm_add_region_fn (size, GRUB_MM_ADD_REGION_NONE);
+ grub_mm_add_region_fn (grow, GRUB_MM_
[PATCH v3 2/2] mm: Preallocate some space when adding new regions
When grub_memalign() encounters out-of-memory, it will try grub_mm_add_region_fn() to request more memory from system firmware. However, it doesn't preallocate memory space for future allocation requests. In extreme cases, it requires one call to grub_mm_add_region_fn() for each memory allocation request. This can be very slow. This patch introduces GRUB_MM_HEAP_GROW_EXTRA, the minimal heap growth granularity. The new region size is now set to the bigger one of its original value and GRUB_MM_HEAP_GROW_EXTRA. Thus, it will result in some memory space preallocated if current allocations request is small. The value of GRUB_MM_HEAP_GROW_EXTRA is set to 1MB. If this value is smaller, the cost of small memory allocations will be higher. If this value is larger, more memory will be wasted and it might cause out-of-memory on machines with small amount of RAM. Signed-off-by: Zhang Boyang --- grub-core/kern/mm.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/grub-core/kern/mm.c b/grub-core/kern/mm.c index 5c0e5bbbf..16c28cab8 100644 --- a/grub-core/kern/mm.c +++ b/grub-core/kern/mm.c @@ -120,6 +120,9 @@ + sizeof (struct grub_mm_header) \ + (GRUB_MM_ALIGN - 1)) +/* Minimal heap growth granularity when existing heap space is exhausted. */ +#define GRUB_MM_HEAP_GROW_EXTRA 0x10 + grub_mm_region_t grub_mm_base; grub_mm_add_region_func_t grub_mm_add_region_fn; @@ -468,11 +471,12 @@ grub_memalign (grub_size_t align, grub_size_t size) goto fail; /* - * Pre-calculate the size of heap growth (if applicable), + * Pre-calculate the optimal size of heap growth (if applicable), * with region management overhead taken into account. */ if (grub_add (size + align, GRUB_MM_MGMT_OVERHEAD, &grow)) goto fail; + grow = grub_max (grow, GRUB_MM_HEAP_GROW_EXTRA); align = (align >> GRUB_MM_ALIGN_LOG2); if (align == 0) -- 2.30.2 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH v3 0/2] mm: Better handling of adding new regions
Hi, This is the V3 patchset. V2 is at: https://lists.gnu.org/archive/html/grub-devel/2022-12/msg00164.html V1 is at: https://lists.gnu.org/archive/html/grub-devel/2022-11/msg00147.html For changes in V2->V3, please see my reply at: https://lists.gnu.org/archive/html/grub-devel/2022-12/msg00250.html Best Regards, Zhang Boyang Zhang Boyang (2): mm: Adjust new region size to take management overhead into account mm: Preallocate some space when adding new regions grub-core/kern/mm.c | 58 +++-- 1 file changed, 56 insertions(+), 2 deletions(-) -- 2.30.2 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH] verifiers: Don't return error for deferred image
When boot from menu and the flag GRUB_VERIFY_FLAGS_DEFER_AUTH is set,
grub returns error:
Booting a command list
error: verification requested but nobody cares: (hd0,gpt1)/Image.
Press any key to continue...
In this case, the image should be deferred for authentication, grub
should return the file handle and pass down to later firmware (e.g.
U-Boot, etc) for authentication.
For this purpose, rather than returning error, this patch prints log
and returns file handler.
Signed-off-by: Leo Yan
---
grub-core/kern/verifiers.c | 6 +-
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/grub-core/kern/verifiers.c b/grub-core/kern/verifiers.c
index 75d7994cf..ada753e69 100644
--- a/grub-core/kern/verifiers.c
+++ b/grub-core/kern/verifiers.c
@@ -115,11 +115,7 @@ grub_verifiers_open (grub_file_t io, enum grub_file_type
type)
if (!ver)
{
if (defer)
- {
- grub_error (GRUB_ERR_ACCESS_DENIED,
- N_("verification requested but nobody cares: %s"),
io->name);
- goto fail_noclose;
- }
+ grub_printf("%s verification is deferred\n", io->name);
/* No verifiers wanted to verify. Just return underlying file. */
return io;
--
2.35.1
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] verifiers: Don't return error for deferred image
Hi,
On 2022/12/22 19:14, Leo Yan wrote:
When boot from menu and the flag GRUB_VERIFY_FLAGS_DEFER_AUTH is set,
grub returns error:
Booting a command list
error: verification requested but nobody cares: (hd0,gpt1)/Image.
Press any key to continue...
In this case, the image should be deferred for authentication, grub
should return the file handle and pass down to later firmware (e.g.
U-Boot, etc) for authentication.
This is probably not what verification framework designed to be. It
seems to be designed to verify files during GRUB is executing (e.g.
check file signature if UEFI Secure Boot is enabled).
By the way, I didn't understand what does "return the file handle and
pass down to later firmware" means. If you means you want GRUB call into
firmware's function, you can write a verifier to do that and register
your verifier with grub_verifier_register().
Best Regards,
Zhang Boyang
For this purpose, rather than returning error, this patch prints log
and returns file handler.
Signed-off-by: Leo Yan
---
grub-core/kern/verifiers.c | 6 +-
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/grub-core/kern/verifiers.c b/grub-core/kern/verifiers.c
index 75d7994cf..ada753e69 100644
--- a/grub-core/kern/verifiers.c
+++ b/grub-core/kern/verifiers.c
@@ -115,11 +115,7 @@ grub_verifiers_open (grub_file_t io, enum grub_file_type
type)
if (!ver)
{
if (defer)
- {
- grub_error (GRUB_ERR_ACCESS_DENIED,
- N_("verification requested but nobody cares: %s"),
io->name);
- goto fail_noclose;
- }
+ grub_printf("%s verification is deferred\n", io->name);
/* No verifiers wanted to verify. Just return underlying file. */
return io;
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] verifiers: Don't return error for deferred image
Hi Boyang, On Thu, Dec 22, 2022 at 07:25:13PM +0800, Zhang Boyang wrote: > Hi, > > On 2022/12/22 19:14, Leo Yan wrote: > > When boot from menu and the flag GRUB_VERIFY_FLAGS_DEFER_AUTH is set, > > grub returns error: > > > > Booting a command list > > > > error: verification requested but nobody cares: (hd0,gpt1)/Image. > > > > Press any key to continue... > > > > In this case, the image should be deferred for authentication, grub > > should return the file handle and pass down to later firmware (e.g. > > U-Boot, etc) for authentication. > > This is probably not what verification framework designed to be. It seems to > be designed to verify files during GRUB is executing (e.g. check file > signature if UEFI Secure Boot is enabled). Good point. We expect the solution is grub can defer authentication for an image and invokes EFI LoadImage service, then EFI loader can load and verify the image. For more specific, now I am debugging U-boot EFI with grub, since U-boot EFI provides functionality for loading and authentication image (see efi_load_image() in [1]), this is my purpose to use U-boot EFI to authenticate kernel image (and even for initrd image). > By the way, I didn't understand what does "return the file handle and pass > down to later firmware" means. If you means you want GRUB call into > firmware's function, you can write a verifier to do that and register your > verifier with grub_verifier_register(). To be clear, I am not experienced for EFI and grub, I try my best to give info :) As explained above, we don't want to introduce any new verifier in grub, it's about we want to verify image in U-boot EFT rather than in grub. So this is why I wrote this patch to dimiss the failure in grub and pass image info to U-boot EFI service. (and sorry my commit log introduced confusion). Thanks, Leo [1] https://github.com/u-boot/u-boot/blob/master/lib/efi_loader/efi_boottime.c#L2021 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v2 8/8] serial: Add ability to specify MMIO ports via 'serial'
Thank you for quick turn around!
Now patches looks much better but...
On Thu, Dec 22, 2022 at 05:12:57PM +1100, Benjamin Herrenschmidt wrote:
> From: Benjamin Herrenschmidt
>
> This adds the ability to explicitely add an MMIO based serial port
> via the 'serial' command. The syntax is:
>
> serial --port=mmio,{.b,.w,.l,.q}
>
> Signed-off-by: Benjamin Herrenschmidt
> ---
> docs/grub.texi | 26 ++--
> grub-core/term/serial.c | 53 +
> 2 files changed, 72 insertions(+), 7 deletions(-)
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index fd22a69fa..502ca2ef7 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -4168,8 +4168,24 @@ Commands usable anywhere in the menu and in the
> command-line.
> @deffn Command serial [@option{--unit=unit}] [@option{--port=port}]
> [@option{--speed=speed}] [@option{--word=word}] [@option{--parity=parity}]
> [@option{--stop=stop}]
> Initialize a serial device. @var{unit} is a number in the range 0-3
> specifying which serial port to use; default is 0, which corresponds to
> -the port often called COM1. @var{port} is the I/O port where the UART
> -is to be found; if specified it takes precedence over @var{unit}.
> +the port often called COM1.
> +
> +@var{port} is the I/O port where the UART is to be found or, if prefixed
> +with @samp{mmio,}, the MMIO address of the UART. If specified it takes
> +precedence over @var{unit}.
> +
> +Additionally, an MMIO address can be suffixed with:
> +@itemize @bullet
> +@item
> +@samp{.b} for bytes access (default)
> +@item
> +@samp{.w} for 16-bit word access
> +@item
> +@samp{.l} for 32-bit long word access or
> +@item
> +@samp{.q} for 64-bit long long word access
> +@end itemize
> +
> @var{speed} is the transmission speed; default is 9600. @var{word} and
> @var{stop} are the number of data bits and stop bits. Data bits must
> be in the range 5-8 and stop bits must be 1 or 2. Default is 8 data
> @@ -4185,6 +4201,12 @@ The serial port is not used as a communication channel
> unless the
> @command{terminal_input} or @command{terminal_output} command is used
> (@pxref{terminal_input}, @pxref{terminal_output}).
>
> +Examples:
> +@example
> +serial --port=3f8 --speed=9600
> +serial --port=mmio,fefb.l --speed=115200
> +@end example
> +
> See also @ref{Serial terminal}.
> @end deffn
>
> diff --git a/grub-core/term/serial.c b/grub-core/term/serial.c
> index ed7b398b7..d374495cb 100644
> --- a/grub-core/term/serial.c
> +++ b/grub-core/term/serial.c
> @@ -152,8 +152,8 @@ grub_serial_find (const char *name)
>break;
>
> #if (defined(__mips__) || defined (__i386__) || defined (__x86_64__)) &&
> !defined(GRUB_MACHINE_EMU) && !defined(GRUB_MACHINE_ARC)
> - if (!port && grub_memcmp (name, "port", sizeof ("port") - 1) == 0
> - && grub_isxdigit (name [sizeof ("port") - 1]))
> + if (!port && grub_strncmp (name, "port", grub_strlen ("port")) == 0
Maybe I was a bit imprecise but I was thinking about converting
grub_memcmp() to grub_strncmp() in your patches/changes only.
Sorry about that.
If you want still to do that I would prefer if you do this in separate
patch. And please do not change "sizeof () - 1" to "grub_strlen ()".
In the worst case I am OK with a sentence in the commit message saying
you change grub_memcmp() to grub_strncmp() on the occasion. Otherwise it
is confusing to see such changes in the patch. Though separate patch
is preferred way to do this...
And please replace grub_memcmp() with grub_strncmp() and 4 with
'sizeof ("mmio") - 1' in patch #3.
> + && grub_isxdigit (name [grub_strlen ("port")]))
> {
>name = grub_serial_ns8250_add_port (grub_strtoul (&name[sizeof
> ("port") - 1],
> 0, 16), NULL);
> @@ -164,6 +164,49 @@ grub_serial_find (const char *name)
> if (grub_strcmp (port->name, name) == 0)
> break;
> }
> + if (!port && grub_strncmp (name, "mmio,", grub_strlen ("mmio,")) == 0
> + && grub_isxdigit (name [grub_strlen ("mmio,")]))
> +{
> + const char *p1, *p = &name[grub_strlen ("mmio,")];
s/grub_strlen ("mmio,")/sizeof ("mmio,") - 1/
> + grub_addr_t addr = grub_strtoul (p, &p1, 16);
> + unsigned int acc_size = 1;
> + unsigned int nlen = p1 - p;
> +
> + /* Check address validity and general syntax */
> + if (addr == 0 || (p1[0] != '\0' && p1[0] != '.'))
This condition looks wrong. I think it should be
if (*p == '\0' || (*p1 != '.' && *p1 != '\0') || addr == 0)
I think the most important thing is lack of "*p == '\0'". Am I right?
And I changed a logic a bit to make it more readable...
Additionally, maybe we should print an error here to give a hint to
a user what is wrong. The grub_error(..., N_()) is your friend...
> +return NULL;
> + if (p1[0] == '.')
> +switch(p1[1])
> + {
> + case 'w':
> +acc_size = 2;
> +break;
> +
Re: Handling large allocations (bypassing mm?)
On Fri, 16 Dec 2022 at 18:55, Robbie Harwood wrote: > > Ard Biesheuvel writes: > > > As for supporing kernels from 2012: I don't see why upstream GRUB > > should care about that. If your distro fork supports those today, you > > will simply need to carry those patches out of tree a bit longer. > > No, it's not a question of distros supporting themselves like this. For > better or worse, people expect to be able to install many OSs on their > drive and have any grub be able to boot any of them. > > One such use case I've seen is hardware testing: someone will install > all the operating systems they care about on a drive, then plug it in to > the machine with the hardware and try all of them in sequence. And of > course there're always hobbyists who just think it's fun to do things > like that - they file bugs too :) > > In any case, it's not a little bit of time we're talking about here - > even though RHEL 6 is only guaranteed into 2024 right now, RHEL 8 is > slated to be here until at least 2031. (Source: > https://access.redhat.com/support/policy/updates/errata/ ) > Fair enough. So are you saying that all downstream GRUBs are mutually compatible, and can boot other distro kernel images, and it is simply mainline GRUB that needs to be modified to incorporate those existing out-of-tree changes? My stake in this is mainly from the opposite side, i.e., avoiding pointless per-arch deviations when doing EFI boot from any bootloader. Upstreaming the EFI handover protocol and other x86 quirks into GRUB while we already have all the pieces we need to make EFI boot almost entirely arch-agnostic seems like a mistake to me. And given that the x86 EFI handover protocol is not in mainline GRUB at the moment, I wonder how this all fits with your (plural) assertion that older kernels must be supported by mainline GRUB, as it doesn't support any of those today. ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] verifiers: Don't return error for deferred image
Hi, On 2022/12/22 20:22, Leo Yan wrote: Hi Boyang, On Thu, Dec 22, 2022 at 07:25:13PM +0800, Zhang Boyang wrote: Hi, On 2022/12/22 19:14, Leo Yan wrote: When boot from menu and the flag GRUB_VERIFY_FLAGS_DEFER_AUTH is set, grub returns error: Booting a command list error: verification requested but nobody cares: (hd0,gpt1)/Image. Press any key to continue... In this case, the image should be deferred for authentication, grub should return the file handle and pass down to later firmware (e.g. U-Boot, etc) for authentication. This is probably not what verification framework designed to be. It seems to be designed to verify files during GRUB is executing (e.g. check file signature if UEFI Secure Boot is enabled). Good point. We expect the solution is grub can defer authentication for an image and invokes EFI LoadImage service, then EFI loader can load and verify the image. Since you mentioned "authentication" and "verify", I guess you are using/implementing some kind of secure boot mechanism. Is it an standard UEFI Secure Boot implementation? The standard UEFI Secure Boot for Linux works like this (at least on x86): 1) Firmware verifies and loads shim (shimx64.efi), which is signed by Microsoft. (https://github.com/rhboot/shim ) 2) shim registers an EFI protocol, to provide an API for verifying files. 3) shim verifies and loads GRUB (grubx64.efi), using certificate embedded in itself. The certificate is generated by vendor (e.g. Debian). 4) GRUB opens kernel image file and loads and executes that file. The key point is, during grub_file_open() of the kernel image, the verifier framework will call shim lock verifier, which calls the API provided by shim, to verify the signature of kernel file. (grub-core/kern/efi/sb.c) In the above example, the kernel is loaded by GRUB, not shim, not EFI firmware. GRUB calls the API provided by shim to verify the kernel. Another example is GRUB can also chainload Windows: 1) 2) 3) is same 4) GRUB invokes EFI firmware's LoadFile() and StartImage(). The verification is completely delegated to EFI firmware. shim is not envolved. In this example, the verification is done by EFI firmware, and GRUB/shim has no control of it. You can't even chainload GRUB itself because it's not signed by Microsoft. For more specific, now I am debugging U-boot EFI with grub, since U-boot EFI provides functionality for loading and authentication image (see efi_load_image() in [1]), this is my purpose to use U-boot EFI to authenticate kernel image (and even for initrd image). It seems efi_load_image() is just a wrapper of EFI firmware's LoadFile() and there is no implementation of verification in U-boot? I'd like to ask what kind of image you are trying to boot/execute? If it is an EFI application, I think you can "chainloader" it and forget U-boot (if U-boot have no verification in it self). Let me guess: You are using standard Secure Boot, and want to boot linux kernel image directly signed by keys in EFI firmware, and your GRUB is installed with --disable-shim-lock. If this is what you are doing, you might run into the following situation: 1) During GRUB init, grub_efi_init() detects Secure Boot is enabled, so it calls grub_lockdown(). [grub-core/kern/efi/init.c] 2) grub_lockdown() registers lockdown verifier. [grub-core/kern/lockdown.c] 3) grub_efi_init() also calls grub_shim_lock_verifier_setup(). However, grub_shim_lock_verifier_setup() does nothing because of "--disable-shim-lock" [grub-core/kern/efi/sb.c] 4) During GRUB load linux kernel, the lockdown verifier sets a flag (GRUB_VERIFY_FLAGS_DEFER_AUTH) for kernel file, and expect shim lock verifier to do the verification. 5) However, there is no shim lock verifier. As a security measure, it reports the error "verification requested but nobody cares". So the root cause seems like --disable-shim-lock is broken? As a workaround, you can use shim: build shim for you platform and install GRUB with shim support. You can also submit a fix to --disable-shim-lock (recommended). However it should be done very carefully. I'm afraid you can't remove the security measure (i.e. the "verification requested but nobody cares") directly. I think it is a good idea to add an EFI verifier (like shim verifier), which use LoadImage() to do verification, and enable it if (and only if) --disable-shim-lock is specified. You can talk to GRUB maintainers about your proposal before coding. The above is what I guess about what's happening. It might be wrong. Please point out if there is something wrong. By the way, I didn't understand what does "return the file handle and pass down to later firmware" means. If you means you want GRUB call into firmware's function, you can write a verifier to do that and register your verifier with grub_verifier_register(). To be clear, I am not experienced for EFI and grub, I try my best to give info :) Me too :) Best Regards, Zhang
[RFC PATCH v3 0/1] kern/dl: Add module version check
Hi,
This is the V3 of my patch.
V2 is at:
https://lists.gnu.org/archive/html/grub-devel/2022-12/msg00234.html
V1 is at:
https://lists.gnu.org/archive/html/grub-devel/2022-12/msg00213.html
[ TD;LR ]
1) The check is always enforced when GRUB is locked down, i.e. modules
will be refused to load if they have mismatched version
2) If built with "--disable-modver-check", modules can always be loaded
even if they have mismatched version, and no message will be displayed.
3) If built with "--enable-modver-check=audit", modules can always be
loaded even if they have mismatched version, but if mismatch is found, a
warning message will be displayed. This is the default.
4) If built with "--enable-modver-check=enforce", the behavior is same as 1)
[ Why this patch is useful, even for BIOS boot ]
Because it helps people diagnose broken (or improper) GRUB installations.
For example, if you google "452: out of range pointer", you will got a
lot of results in 2022. I think the most of them are related to
mismatched modules. However, these problem are often not properly
diagnosed because they disappear magically, e.g. update whole system
(which triggers grub reinstall). There are several people even suspect
there are problems with their hard disk / BIOS. However, the root cause
is 052e6068be62 ("mm: When adding a region, merge with region after as
well as before") changed the layout of `struct grub_mm_region`, which is
both used in main program and "relocator.mod", so the module reads the
wrong field and crashes GRUB. Please the commit did nothing wrong
because there is no API/ABI compatibility guarantees in GRUB.
If there are warning messages about mismatched modules, user will easily
notice there are problems with their GRUB installation.
[ Why not enforce this check to prevent crashes ]
As Glenn & Pete said, most mismatched modules isn't harmful. At most
times, GRUB with mismatched modules can boot Linux happily, even if
these modules come from another Linux distribution. This enables user to
fix his/her GRUB installation without using a boot/rescue disk, because
the user can boot the existing Linux using the existing (but improperly
installed) GRUB.
[ Why warning can be disabled ]
Some tools like Rufus relies on mismatched modules. Some advanced users
also doesn't like redundant warnings for their existing known-to-work
configurations.
However, it's highly unrecommended to disable this warning.
[ Why this patch is a prerequisite for external signed module support ]
Consider this scenario:
1) GRUB 2.XX is free of vulnerabilities
2) GRUB 2.YY is also free of vulnerabilities
3) So GRUB 2.XX shares same SBAT numbers with GRUB 2.YY, therefore SBAT
can't help in version check
4) If there is no version check, it's possible to load GRUB 2.YY modules
into GRUB 2.XX (and vice versa)
5) However, due to some changes in API or ABI, although unlikely, there
is possibility that there are vulnerabilities when using GRUB 2.YY
modules with GRUB 2.XX (and vice versa)
6) So we must enforce version check to prevent this from happening
However, because version string is only consisted of PACKAGE_VERSION, it
must be unique for one given vendor (signer). For example, version
string need to be different for Debian 10 and Debian 11 even they both
use GRUB 2.06, and no two build in Debian 10 (or Debian 11) have same
version string.
Best Regards,
Zhang Boyang
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[RFC PATCH v3 1/1] kern/dl: Add module version check
This patch add version information to GRUB modules. Specifically,
PACKAGE_VERSION is embedded as null-terminated string in .modver
section. This string is checked at module loading time.
If GRUB is locked down, modules with mismatched version will be
rejected. This is necessary for implementing external signed modules
securely (in future).
If GRUB is not locked down, the behavior is configurable at build time
by "--enable-modver-check={audit|enforce}". If the option is set to
"enforce", the behavior is same as lockdown mode. If the option is set
to "audit" (this is the default), modules can still be loaded even if
they have mismatched version, and a warning message will be emited. If
the option is disabled by "--disable-modver-check", modules can always
be loaded and no warning message will be generated. Please note this
option doesn't have any effect if GRUB is locked down.
It should be pointed out that the version string is only consisted of
PACKAGE_VERSION. Any difference not reflected in PACKAGE_VERSION is not
noticed by version check (e.g. configure options).
Link: https://lists.gnu.org/archive/html/grub-devel/2022-12/msg00280.html
Signed-off-by: Zhang Boyang
---
config.h.in | 5 +
configure.ac | 12 ++
grub-core/Makefile.am | 2 +-
grub-core/genmod.sh.in| 28 ++-
grub-core/kern/dl.c | 42 +++
util/grub-module-verifierXX.c | 10 +
6 files changed, 87 insertions(+), 12 deletions(-)
diff --git a/config.h.in b/config.h.in
index 4d1e50eba..bac67acc5 100644
--- a/config.h.in
+++ b/config.h.in
@@ -13,6 +13,11 @@
#define MM_DEBUG @MM_DEBUG@
#endif
+#define MODVER_CHECK @MODVER_CHECK@
+#define MODVER_CHECK_IGNORE0
+#define MODVER_CHECK_AUDIT 1
+#define MODVER_CHECK_ENFORCE 2
+
/* Define to 1 to enable disk cache statistics. */
#define DISK_CACHE_STATS @DISK_CACHE_STATS@
#define BOOT_TIME_STATS @BOOT_TIME_STATS@
diff --git a/configure.ac b/configure.ac
index 93626b798..e9fe6c25a 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1521,6 +1521,18 @@ else
fi
AC_SUBST([MM_DEBUG])
+AC_ARG_ENABLE([modver-check],
+ AS_HELP_STRING([--enable-modver-check={audit|enforce}],
+ [enable module version check for non-lockdown
mode (default=audit)]))
+if test "x$enable_modver_check" = xno; then
+ MODVER_CHECK=0
+elif test "x$enable_modver_check" = xenforce; then
+ MODVER_CHECK=2
+else
+ MODVER_CHECK=1
+fi
+AC_SUBST([MODVER_CHECK])
+
AC_ARG_ENABLE([cache-stats],
AS_HELP_STRING([--enable-cache-stats],
[enable disk cache statistics collection]))
diff --git a/grub-core/Makefile.am b/grub-core/Makefile.am
index 80e7a83ed..d76aa80f4 100644
--- a/grub-core/Makefile.am
+++ b/grub-core/Makefile.am
@@ -40,7 +40,7 @@ gentrigtables$(BUILD_EXEEXT): gentrigtables.c
CLEANFILES += gentrigtables$(BUILD_EXEEXT)
build-grub-module-verifier$(BUILD_EXEEXT):
$(top_srcdir)/util/grub-module-verifier.c
$(top_srcdir)/util/grub-module-verifier32.c
$(top_srcdir)/util/grub-module-verifier64.c
$(top_srcdir)/grub-core/kern/emu/misc.c $(top_srcdir)/util/misc.c
- $(BUILD_CC) -o $@ -I$(top_srcdir)/include $(BUILD_CFLAGS)
$(BUILD_CPPFLAGS) $(BUILD_LDFLAGS) -DGRUB_BUILD=1 -DGRUB_UTIL=1
-DGRUB_BUILD_PROGRAM_NAME=\"build-grub-module-verifier\" $^
+ $(BUILD_CC) -o $@ -I$(top_srcdir)/include $(BUILD_CFLAGS)
$(BUILD_CPPFLAGS) $(BUILD_LDFLAGS) -DGRUB_BUILD=1 -DGRUB_UTIL=1
-DGRUB_BUILD_PROGRAM_NAME=\"build-grub-module-verifier\"
-DPACKAGE_VERSION=\"$(PACKAGE_VERSION)\" $^
CLEANFILES += build-grub-module-verifier$(BUILD_EXEEXT)
# trigtables.c
diff --git a/grub-core/genmod.sh.in b/grub-core/genmod.sh.in
index e57c4d920..58a4cdcbe 100644
--- a/grub-core/genmod.sh.in
+++ b/grub-core/genmod.sh.in
@@ -36,22 +36,25 @@ deps=`grep ^$modname: $moddep | sed s@^.*:@@`
rm -f $tmpfile $outfile
if test x@TARGET_APPLE_LINKER@ != x1; then
-# stripout .modname and .moddeps sections from input module
-@TARGET_OBJCOPY@ -R .modname -R .moddeps $infile $tmpfile
+# stripout .modname and .moddeps and .modver sections from input module
+@TARGET_OBJCOPY@ -R .modname -R .moddeps -R .modver $infile $tmpfile
-# Attach .modname and .moddeps sections
+# Attach .modname and .moddeps and .modver sections
t1=`mktemp "${TMPDIR:-/tmp}/tmp.XX"` || exit 1
printf "$modname\0" >$t1
t2=`mktemp "${TMPDIR:-/tmp}/tmp.XX"` || exit 1
for dep in $deps; do printf "$dep\0" >> $t2; done
+t3=`mktemp "${TMPDIR:-/tmp}/tmp.XX"` || exit 1
+printf "@PACKAGE_VERSION@\0" >$t3
+
if test -n "$deps"; then
- @TARGET_OBJCOPY@ --add-section .modname=$t1 --add-section .moddeps=$t2
$tmpfile
+ @TARGET_OBJCOPY@ --add-section .modname=$t1 --add-section .moddeps=$t2
--add-section .modver=$t3 $tmpfile
else
- @TARGET_OBJCOPY@ --add-section .modname=$t1
Re: [RFC PATCH v3 0/1] kern/dl: Add module version check
I'm sorry but that's NO_GO for me. This is a major step back from v2, where the check was never applicable for BIOS and I believe I explained why a module check that can be enforced for BIOS is going to create a major headache for end-users. It seems pretty obvious that, if distro maintainers start to use --enable-modver-check=enforce for UEFI they are also going to use the same option for BIOS (because why should they use a different compilation options if it applies for both?), and you *ARE* going to leave Windows users stranded through them not trying to boot GRUB based media because they will be forced to use DD to create a workable media (even though, and this is the major point here, in the very vast majority of cases there is not issue with mixing versions if you're on the same major/minor base for GRUB), and consider that, because they cannot see its content, or can only see the ESP, their media was not properly created and is not worth trying to boot. This is a *VERY REAL* problem, for which I have provided many examples (see [1]), and could provide even more, which is why utilities like Rufus try to steer away from using DD mode where possible. However, if people can use --enable-modver-check=enforce for BIOS, then, under the aim of solving one minor issue (that from looking the Google links for "452: out of range pointer" seem to be a problem that is mostly encountered in UEFI environments rather than BIOS, or at least is not a BIOS-only issue, and therefore something that will be sorted out for the majority of folks experiencing it *even* if --enable-modver-check cannot apply for BIOS), you will be adding to an already existing a major one, and you are going to leave people, and especially Windows users who are trying to switch to Linux, stranded. In short, GRUB would be doing a major disservice to its end users if it is to allow module version checks to be enforceable for BIOS, and, considering how there is little benefit in enforcing this in an environment that cannot be secured in the first place, it's hard to see a justification for what is essentially a *MAJOR* design regression from the v2 of this patch. Or at the very least, a better approach is to introduce it for UEFI only, then, once that has been integrated with UEFI for a while, see if BIOS users are actually reporting issues due to module version mismatch, and reconvene on whether we really want to allow this check to also apply to BIOS. Thus, in my view, the proper approach for the sake of the end users is leave default BIOS boot behaviour as it currently stands, with no warning and no possibility of failing the boot process on version mismatch (because, and I am well placed to know, both from experience as well as the feedback I get from Rufus users, that in the very vast majority of cases, as long as you use modules and core.img based on the same major+minor, there is no issue, and producing a warning that hints that there may be is only going to be detrimental to the end user). And, once again, I have to ask this mailing list to please try not to consider issues only from a Linux/UNIX based maintainer's perspective, but also consider them from the end-users who are actually going to be creating GRUB based media on Windows, where using ISOHybrid as DD is far from the panacea it's supposed to be. Finally, if you are interested in seeing how end users actually struggle with boot media and boot media creation, I *strongly* invite you to spend some time on reddit, askubuntu, superuser and other end-user places, where you should be able to validate that, as I have pointed out, Windows end users really do struggle with ISOHybrdids that are written in DD mode, which will hopefully help you reach the conclusion that, if this v3 proposal gets integrated, it will cause more harm for BIOS users than it will benefit them on account that distro maintainers are bound to start using --enable-modver-check=enforce everywhere, if it can apply to both BIOS and UEFI. Regards, /Pete [1] https://lists.gnu.org/archive/html/grub-devel/2022-06/msg00024.html On 2022.12.22 16:38, Zhang Boyang wrote: Hi, This is the V3 of my patch. V2 is at: https://lists.gnu.org/archive/html/grub-devel/2022-12/msg00234.html V1 is at: https://lists.gnu.org/archive/html/grub-devel/2022-12/msg00213.html [ TD;LR ] 1) The check is always enforced when GRUB is locked down, i.e. modules will be refused to load if they have mismatched version 2) If built with "--disable-modver-check", modules can always be loaded even if they have mismatched version, and no message will be displayed. 3) If built with "--enable-modver-check=audit", modules can always be loaded even if they have mismatched version, but if mismatch is found, a warning message will be displayed. This is the default. 4) If built with "--enable-modver-check=enforce", the behavior is same as 1) [ Why this patch is useful, even f
Re: [PATCH v4 08/15] gdb: If enabled, print line used to load EFI kernel symbols when using gdb_grub script
On Wed, Dec 21, 2022 at 11:57:33AM -0600, Glenn Washburn wrote: > On Wed, 21 Dec 2022 16:20:17 +0100 > Daniel Kiper wrote: > > > Adding Robbie... > > > > Please CC him next time when you post these patches. I would want to > > hear his opinion too. Or at least he is aware what is happening > > here... > > Sure, I CC'd him and Peter on the first couple of ones. But there was Thank you! > never had a response in the 4 months since then, so I figured they > didn't care. Until somebody ask you to not include themselves in the thread please CC them. AFAICT many people read emails often, like me, but jump into discussion when something really important for them is happening. > > On Thu, Dec 15, 2022 at 11:29:31PM -0600, Glenn Washburn wrote: > > > If the macro PRINT_GDB_SYM_LOAD_CMD is non-zero, compile code which Why is this not a flag, like e.g. --enable-mm-debug, for the configure? > > > will print the command needed to load symbols for the GRUB EFI > > > kernel. This is needed because EFI firmware determines where to > > > load the GRUB EFI at runtime, and so the relevant addresses are not > > > known ahead of time. > > > > > > The command is a custom command defined in the gdb_grub GDB script. > > > So GDB should be started with the script as an argument to the -x > > > option or sourced into an active GDB session before running the > > > outputted command. > > > > I think this functionality should be disabled when lockdown is > > enforced, e.g. on UEFI platforms with Secure Boot enabled. > > Since this is off by default and must be enabled at build time, then if > the builder enabled it, they really did want it, regardless of > lockdown. What you're worried about seems highly improbable to me (but > then I don't know the inner workings of the distros). The concern as I > understand it, is that someone doing an official release of a distro > which will be secure boot ready will accidentally have this build time > macro enabled. That's almost inconceivable to me, but I'm curious what > the others have to say (especially since Robbie posted a similar patch > that always printed this info as a debug message[1]). Or is it more > about a regular user signing with their own keys accidentally shooting > themselves in the foot by forgetting to disable this (after having > already enabled it) and then some physical attacker getting extra info > to do an evil maid attack? I can imagine that a distro builds and signs GRUB with debug embedded and then somebody in the wild wants to enable this feature to debug a problem. Of course them cannot rebuild the GRUB image because it is signed. However, them can disable UEFI Secure Boot and enable debugging. Of course this probably will not work in all cases but should help solve most problems. Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v4 12/15] gdb: Allow running user-defined commands at GRUB start
On Thu, Dec 22, 2022 at 12:08:17AM -0600, Glenn Washburn wrote: > On Wed, 21 Dec 2022 12:19:16 -0600 > Glenn Washburn wrote: > > > On Wed, 21 Dec 2022 16:27:40 +0100 > > Daniel Kiper wrote: > > > > > On Thu, Dec 15, 2022 at 11:29:35PM -0600, Glenn Washburn wrote: > > > > A new command, run_on_start, is created which handles some > > > > complexities of the EFI platform when breaking on GRUB start. If > > > > GRUB start is hooked, run "onstart" command if it is defned. > > > > > > > > Signed-off-by: Glenn Washburn > > > > --- > > > > grub-core/gdb_grub.in | 38 ++ > > > > 1 file changed, 38 insertions(+) > > > > > > > > diff --git a/grub-core/gdb_grub.in b/grub-core/gdb_grub.in > > > > index 8ae6344edf..3b3cea1a4d 100644 > > > > --- a/grub-core/gdb_grub.in > > > > +++ b/grub-core/gdb_grub.in > > > > @@ -36,6 +36,8 @@ end > > > > define dynamic_load_symbols > > > > dynamic_load_kernel_exec_symbols $arg0 > > > > > > > > + run_on_start > > > > + > > > > # We may have been very late to loading the kernel.exec > > > > symbols and # and modules may already be loaded. So load symbols > > > > for any already # loaded. > > > > @@ -134,6 +136,41 @@ document runtime_load_module > > > > Load module symbols at runtime as they are loaded. > > > > end > > > > > > > > +define run_on_start > > > > + # TODO: Add check to see if _start symbol is defined, if > > > > not, then > > > > + # the symbols have not yet been loaded and this command > > > > will not work. > > > > + watch *_start > > > > + set $break_efi_start_bpnum = $bpnum > > > > + commands > > > > + silent > > > > + delete $break_efi_start_bpnum > > > > + break _start > > > > > > s/break/hbreak/? > > > > A regular break works here for me. I want to avoid hbreak at all > > costs, which is why I had a previous convoluted method using break > > (which I thought worked, and then found it didn't quite). My > > understanding is that the number of hardware breakpoints are limited > > and commonly its around 4. Specifically, my understanding is that on > > x86-64 the number is exactly 4, so I would prefer the user have > > usable as many as possible. > > > > Really, I'd like to figure out why sometimes break works and why > > sometimes not, and then figure out a way to make it work for these > > scripts. I recently had the idea that maybe the UEFI firmware sets up > > the pages where it loads the .text section of the GRUB UEFI binary to > > readonly in the page table structure. But I went through the structure > > when %eip is at _start and the R/W bit is set on the pages I checked. > > Even if the pages were set to readonly, I suspect the qemu gdb stub > > allows writing to that memory anyway. > > > > So I'm at a loss as to what could be preventing break from working. > > I'd love to hear some ideas if anyone has some. > > Okay so its been a while since I was deep in this and I forgot some > stuff I already knew (and which I should incorporate into the > documentation patch). The reason that software break doesn't always Great! > work, is that when its not working its because the breakpoint is being > set before the GRUB EFI app is loaded into memory. So happens is that > GDB sets a breakpoint (ie a 0xcc instruction where the symbol points > to), but it then gets over-written by the UEFI firmware as its loads > the GRUB EFI app. So the app loading effectively will clear all > software breakpoints. This doesn't affect the hardware breakpoints > because they are in CPU debug registers which are not affected by the > firmware loading the EFI app. Yeah, this is what I expected... > The reason that the above worked, and was written that way in the first > place, is that when the watch commands run the memory at _start has just > been modified. This happens when the UEFI firmware is loading the GRUB > EFI app. Then we can use software break without worrying about it being > cleared. Could you add this (whole) explanation to the commit message to make it clear why we can use "break" here? > Revisiting this has given me some ideas for improving the patch series. Cool! Thanks, Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v4 13/15] gdb: Add extra early initialization symbols for i386-pc
On Wed, Dec 21, 2022 at 12:21:26PM -0600, Glenn Washburn wrote: > On Wed, 21 Dec 2022 16:28:35 +0100 > Daniel Kiper wrote: > > > On Thu, Dec 15, 2022 at 11:29:36PM -0600, Glenn Washburn wrote: > > > Add symbols for boot.image, disk.image, and lzma_decompress.image > > > if the target is i386-pc. > > > > Why? > > Why not? I'm not clear on what your objection is. This is for debugging > the code that has those symbols, eg. to break on those symbols. This is not an objection. I want you to explain in the commit message why this patch is needed. This thing is missing... Sorry if I was to vague... Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: GRUB 2.12 release - update
Hi, Quick update below... On Wed, Oct 26, 2022 at 04:52:09PM +0200, Daniel Kiper wrote: > Hi, > > We are getting closer to the 2.12 release. Sadly we still do not have > many of important patch sets in the tree. So, I am going to spend more > time on reviews in the following weeks. Below you can find my list of > key patch sets which should land in the release: > - Dynamic allocation of memory regions and IBM vTPM v2, > - Unify ARM64 & RISC-V Linux Loader, > - Add support for LoongArch, > - plainmount: Support plain encryption mode, > - Glenn's tests fixes. > > Of course all patches which got my RB or are under review will be merged too. > > There is also "nice to have" list but I do not consider lack of this > patch sets as release blockers: > - Add support for EFI file system transposition, > - term/serial: Add support for PCI serial devices, > - Add MMIO serial ports support and SPCR detection, > - Glenn's gdb patch set. > > I hope I will be able to review and merge all patch sets from first list > during November. Then if everything goes well we will make code freeze > in December and release at the beginning of next year, preferably in January. Most of the patches from the list above are now under review or (almost) ready for merging. I will push all patches which got my RB recently after holidays. (Sadly) we have to add UEFI NX patches to the list above due to introduction of new requirements "for all applications to be signed by the Microsoft third-party Unified Extensible Firmware Interface (UEFI) Certificate Authority (CA)" [1]. I hope relevant patches will appear on the grub-devel in January or February. > I am considering to not block merges for tests and documentation during > code freeze. Nothing changes here... Happy holidays, Daniel [1] https://techcommunity.microsoft.com/t5/hardware-dev-center/new-uefi-ca-memory-mitigation-requirements-for-signing/ba-p/3608714 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v2 8/8] serial: Add ability to specify MMIO ports via 'serial'
On Thu, 2022-12-22 at 13:54 +0100, Daniel Kiper wrote:
> Thank you for quick turn around!
>
> Now patches looks much better but...
Hehe :-)
> >
> > #if (defined(__mips__) || defined (__i386__) || defined (__x86_64__)) &&
> > !defined(GRUB_MACHINE_EMU) && !defined(GRUB_MACHINE_ARC)
> > - if (!port && grub_memcmp (name, "port", sizeof ("port") - 1) == 0
> > - && grub_isxdigit (name [sizeof ("port") - 1]))
> > + if (!port && grub_strncmp (name, "port", grub_strlen ("port")) == 0
>
> Maybe I was a bit imprecise but I was thinking about converting
> grub_memcmp() to grub_strncmp() in your patches/changes only.
> Sorry about that.
Ok, that's fine, I wasn't sure what you wanted, it's easy enough for me
to respin and re-test.
> If you want still to do that I would prefer if you do this in separate
> patch. And please do not change "sizeof () - 1" to "grub_strlen ()".
I'll do a separate patch. I don't like sizeof () - 1 but it's indeed
more efficient at this point because grub_strlen() doesn't exploit
gcc's __builtin_strlen() (which would evaluate to a constant). So I'll
put them back in for now, but we should look at using more gcc
builtin's imho down the line.
> In the worst case I am OK with a sentence in the commit message saying
> you change grub_memcmp() to grub_strncmp() on the occasion. Otherwise it
> is confusing to see such changes in the patch. Though separate patch
> is preferred way to do this...
Nah, I'll do a separate patch. I'll put it in the same series though.
> And please replace grub_memcmp() with grub_strncmp() and 4 with
> 'sizeof ("mmio") - 1' in patch #3.
Ah I missed that one, thanks.
>
> > + && grub_isxdigit (name [grub_strlen ("port")]))
> > {
> > name = grub_serial_ns8250_add_port (grub_strtoul (&name[sizeof
> > ("port") - 1],
> > 0, 16), NULL);
> > @@ -164,6 +164,49 @@ grub_serial_find (const char *name)
> > if (grub_strcmp (port->name, name) == 0)
> > break;
> > }
> > + if (!port && grub_strncmp (name, "mmio,", grub_strlen ("mmio,")) == 0
> > + && grub_isxdigit (name [grub_strlen ("mmio,")]))
> > + {
> > + const char *p1, *p = &name[grub_strlen ("mmio,")];
>
> s/grub_strlen ("mmio,")/sizeof ("mmio,") - 1/
Ack.
> > + grub_addr_t addr = grub_strtoul (p, &p1, 16);
> > + unsigned int acc_size = 1;
> > + unsigned int nlen = p1 - p;
> > +
> > + /* Check address validity and general syntax */
> > + if (addr == 0 || (p1[0] != '\0' && p1[0] != '.'))
>
> This condition looks wrong. I think it should be
>
> if (*p == '\0' || (*p1 != '.' && *p1 != '\0') || addr == 0)
>
> I think the most important thing is lack of "*p == '\0'". Am I right?
> And I changed a logic a bit to make it more readable...
if *p is '\0' then addr is 0 (and *p1 is '\0'). But we wouldn't get
there in the first place because of the grub_isxdigit() in the previous
test. So we know *p is a digit at this point. In fact I don't even need
to test addr == 0, there could be universes where it's a valid MMIO
address :-) (not kidding, I've seen it, though nowhere we run grub
these days).
So the only thing we really care about at this point is the first non-
digit character which is *p1. It's either a dot or a '\0' for things to
be valild.
> Additionally, maybe we should print an error here to give a hint to
> a user what is wrong. The grub_error(..., N_()) is your friend...
Right, it would go to whatever console is still active at this point
(probably BIOS console) which is probably not going to be terribly
useful for people who are tyring to use a serial port (for a reason ..
:-) but it won't hurt either.
> > + return NULL;
> > + if (p1[0] == '.')
> > + switch(p1[1])
> > + {
> > + case 'w':
> > + acc_size = 2;
> > + break;
> > + case 'l':
> > + acc_size = 3;
> > + break;
> > + case 'q':
> > + acc_size = 4;
> > + break;
> > + case 'b':
> > + acc_size = 1;
> > + /* fallthrough */
> > + default:
> > + /*
> > + * Should we abort for an unknown size ? Let's just default
> > + * to 1 byte, it would increase the chance that the user who
> > + * did a typo can actually see the console.
> > + */
> > + acc_size = 1;
> > + }
> > + name = grub_serial_ns8250_add_mmio (addr, acc_size, NULL);
> > + if (!name)
> > + return NULL;
> > +
> > + FOR_SERIAL_PORTS (port)
> > + if (grub_strncmp (port->name, name, nlen) == 0) {
> > + break;
> > + }
> > + }
> > if (!port && grub_strcmp (name, "auto") == 0)
> > {
> > /* Look for an SPCR if any. If not, default to com0 */
> > @@ -178,9 +221,9 @@ grub_serial_find (const char *name)
> > #endif
> >
> > #ifdef GRUB_MACHINE_IEEE1275
> > - if (!port && gr
[PATCH v3 5/8] ns8250: Add configuration parameter when adding ports
From: Benjamin Herrenschmidt
This will allow ports to be added with a pre-set configuration
Signed-off-by: Benjamin Herrenschmidt
---
grub-core/term/ns8250.c | 27 ---
grub-core/term/serial.c | 2 +-
include/grub/serial.h | 4 ++--
3 files changed, 23 insertions(+), 10 deletions(-)
diff --git a/grub-core/term/ns8250.c b/grub-core/term/ns8250.c
index 9e0ebff5a..d9d93fcf8 100644
--- a/grub-core/term/ns8250.c
+++ b/grub-core/term/ns8250.c
@@ -304,7 +304,7 @@ grub_ns8250_hw_get_port (const unsigned int unit)
}
char *
-grub_serial_ns8250_add_port (grub_port_t port)
+grub_serial_ns8250_add_port (grub_port_t port, struct grub_serial_config
*config)
{
struct grub_serial_port *p;
unsigned i;
@@ -314,6 +314,9 @@ grub_serial_ns8250_add_port (grub_port_t port)
{
if (dead_ports & (1 << i))
return NULL;
+ /* give the opportunity for SPCR to configure a default com port */
+ if (config != NULL)
+ grub_serial_port_configure (&com_ports[i], config);
return com_names[i];
}
@@ -326,32 +329,39 @@ grub_serial_ns8250_add_port (grub_port_t port)
return NULL;
p = grub_malloc (sizeof (*p));
- if (!p)
+ if (p == NULL)
return NULL;
p->name = grub_xasprintf ("port%lx", (unsigned long) port);
- if (!p->name)
+ if (p->name == NULL)
{
grub_free (p);
return NULL;
}
p->driver = &grub_ns8250_driver;
- grub_serial_config_defaults (p);
p->mmio = false;
p->port = port;
+ if (config != NULL)
+grub_serial_port_configure (p, config);
+ else
+grub_serial_config_defaults (p);
grub_serial_register (p);
return p->name;
}
char *
-grub_serial_ns8250_add_mmio (grub_addr_t addr)
+grub_serial_ns8250_add_mmio (grub_addr_t addr, struct grub_serial_config
*config)
{
struct grub_serial_port *p;
unsigned i;
for (i = 0; i < GRUB_SERIAL_PORT_NUM; i++)
if (com_ports[i].mmio && com_ports[i].mmio_base == addr)
- return com_names[i];
+ {
+if (config != NULL)
+ grub_serial_port_configure (&com_ports[i], config);
+return com_names[i];
+ }
p = grub_malloc (sizeof (*p));
if (p == NULL)
@@ -363,9 +373,12 @@ grub_serial_ns8250_add_mmio (grub_addr_t addr)
return NULL;
}
p->driver = &grub_ns8250_driver;
- grub_serial_config_defaults (p);
p->mmio = true;
p->mmio_base = addr;
+ if (config != NULL)
+grub_serial_port_configure (p, config);
+ else
+grub_serial_config_defaults (p);
grub_serial_register (p);
return p->name;
diff --git a/grub-core/term/serial.c b/grub-core/term/serial.c
index 842322b1c..cdbbc54e3 100644
--- a/grub-core/term/serial.c
+++ b/grub-core/term/serial.c
@@ -156,7 +156,7 @@ grub_serial_find (const char *name)
&& grub_isxdigit (name [sizeof ("port") - 1]))
{
name = grub_serial_ns8250_add_port (grub_strtoul (&name[sizeof ("port")
- 1],
- 0, 16));
+ 0, 16), NULL);
if (!name)
return NULL;
diff --git a/include/grub/serial.h b/include/grub/serial.h
index 5e008f0f5..a94327140 100644
--- a/include/grub/serial.h
+++ b/include/grub/serial.h
@@ -185,8 +185,8 @@ grub_serial_config_defaults (struct grub_serial_port *port)
#if defined(__mips__) || defined (__i386__) || defined (__x86_64__)
void grub_ns8250_init (void);
-char *grub_serial_ns8250_add_port (grub_port_t port);
-char *grub_serial_ns8250_add_mmio (grub_addr_t addr);
+char *grub_serial_ns8250_add_port (grub_port_t port, struct grub_serial_config
*config);
+char *grub_serial_ns8250_add_mmio (grub_addr_t addr, struct grub_serial_config
*config);
#endif
#ifdef GRUB_MACHINE_IEEE1275
void grub_ofserial_init (void);
--
2.34.1
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH v3 7/8] ns8250: Support more MMIO access sizes
From: Benjamin Herrenschmidt
It is common for PCI based UARTs to use larger than one byte access
sizes. This adds support for this and uses the information present
in SPCR accordingly.
Signed-off-by: Benjamin Herrenschmidt
---
grub-core/term/ns8250-spcr.c | 3 +-
grub-core/term/ns8250.c | 67 ++--
include/grub/serial.h| 10 --
3 files changed, 67 insertions(+), 13 deletions(-)
diff --git a/grub-core/term/ns8250-spcr.c b/grub-core/term/ns8250-spcr.c
index 0b4417a30..f4b718833 100644
--- a/grub-core/term/ns8250-spcr.c
+++ b/grub-core/term/ns8250-spcr.c
@@ -74,7 +74,8 @@ grub_ns8250_spcr_init (void)
switch (spcr->base_addr.space_id)
{
case GRUB_ACPI_GENADDR_MEM_SPACE:
-return grub_serial_ns8250_add_mmio (spcr->base_addr.addr, &config);
+return grub_serial_ns8250_add_mmio (spcr->base_addr.addr,
+spcr->base_addr.access_size,
&config);
case GRUB_ACPI_GENADDR_IO_SPACE:
return grub_serial_ns8250_add_port (spcr->base_addr.addr, &config);
default:
diff --git a/grub-core/term/ns8250.c b/grub-core/term/ns8250.c
index d9d93fcf8..98f0b3bc3 100644
--- a/grub-core/term/ns8250.c
+++ b/grub-core/term/ns8250.c
@@ -42,16 +42,59 @@ ns8250_reg_read (struct grub_serial_port *port, grub_addr_t
reg)
{
asm volatile("" : : : "memory");
if (port->mmio)
-return *((volatile grub_uint8_t *) (port->mmio_base + reg));
+{
+ /*
+ * Note: we assume MMIO UARTs are little endian. This is not true of all
+ * embedded platforms but will do for now
+ */
+ switch(port->access_size)
+{
+default:
+ /* ACPI tables occasionally uses "0" (legacy) as equivalent to "1"
(byte) */
+case 1:
+ return *((volatile grub_uint8_t *) (port->mmio_base + reg));
+case 2:
+ return grub_le_to_cpu16 (*((volatile grub_uint16_t *)
(port->mmio_base + (reg << 1;
+case 3:
+ return grub_le_to_cpu32 (*((volatile grub_uint32_t *)
(port->mmio_base + (reg << 2;
+case 4:
+ /*
+ * This will only work properly on 64-bit systems since 64-bit
+ * accessors aren't atomic on 32-bit hardware. Thankfully the
+ * case of a UART with a 64-bit register spacing on 32-bit
+ * also probably doesn't exist.
+ */
+ return grub_le_to_cpu64 (*((volatile grub_uint64_t *)
(port->mmio_base + (reg << 3;
+}
+}
return grub_inb (port->port + reg);
}
static void
-ns8250_reg_write (struct grub_serial_port *port, grub_uint8_t, grub_addr_t reg)
+ns8250_reg_write (struct grub_serial_port *port, grub_uint8_t value,
grub_addr_t reg)
{
asm volatile("" : : : "memory");
if (port->mmio)
-*((volatile grub_uint8_t *) (port->mmio_base + reg)) = value;
+{
+ switch(port->access_size)
+{
+default:
+ /* ACPI tables occasionally uses "0" (legacy) as equivalent to "1"
(byte) */
+case 1:
+ *((volatile grub_uint8_t *) (port->mmio_base + reg)) = value;
+ break;
+case 2:
+ *((volatile grub_uint16_t *) (port->mmio_base + (reg << 1))) =
grub_cpu_to_le16 (value);
+ break;
+case 3:
+ *((volatile grub_uint32_t *) (port->mmio_base + (reg << 2))) =
grub_cpu_to_le32 (value);
+ break;
+case 4:
+ /* See commment in ns8250_reg_read() */
+ *((volatile grub_uint64_t *) (port->mmio_base + (reg << 3))) =
grub_cpu_to_le64 (value);
+ break;
+}
+}
else
grub_outb (value, port->port + reg);
}
@@ -286,6 +329,7 @@ grub_ns8250_init (void)
grub_print_error ();
grub_serial_register (&com_ports[i]);
+ com_ports[i].access_size = 1;
}
}
@@ -312,12 +356,12 @@ grub_serial_ns8250_add_port (grub_port_t port, struct
grub_serial_config *config
for (i = 0; i < GRUB_SERIAL_PORT_NUM; i++)
if (com_ports[i].port == port)
{
- if (dead_ports & (1 << i))
- return NULL;
- /* give the opportunity for SPCR to configure a default com port */
- if (config != NULL)
- grub_serial_port_configure (&com_ports[i], config);
- return com_names[i];
+if (dead_ports & (1 << i))
+ return NULL;
+/* give the opportunity for SPCR to configure a default com port */
+if (config != NULL)
+ grub_serial_port_configure (&com_ports[i], config);
+return com_names[i];
}
grub_outb (0x5a, port + UART_SR);
@@ -340,6 +384,7 @@ grub_serial_ns8250_add_port (grub_port_t port, struct
grub_serial_config *config
p->driver = &grub_ns8250_driver;
p->mmio = false;
p->port = port;
+ p->access_size = 1;
if (config != NULL)
grub_serial_port_configure (p, config);
else
@@ -350,7 +395,8 @@ grub_serial_ns8250_add_port (grub_port_t port, struct
grub_serial_config *config
}
char *
-g
[PATCH v2 0/8] serial: Add MMIO & SPCR support
This series adds support for MMIO serial ports and auto-configuration via ACPI SPCR. This is necessary for the serial port to work on AWS EC2 "metal" x86 systems. An MMIO serial port can be specified explicitely using the "mmio," prefix for the --port argument to the 'serial' command, the register access size can also be specified via a suffix, and when 'serial' is used without arguments, it will try to use an ACPI SPCR entry (if any) to add the default port and configure it appropriately (speed, parity etc...) This was tested using SPCR on an actual c5.metal instance, and using explicit instanciation via serial -p mmio on a modified qemu hacked to create MMIO PCI serial ports and to create a SPCR ACPI table. The insipiration was an original implementation by Matthias Lange ! matthias.lange at kernkonzept.com However, the code was rewritten pretty much from scratch. v2: This version (hopefully) addresses all the review commments and has been rebased on the latest master branch. v3: - Remove change to grub_cmd_serial() in patch 3, it serves no purpose at this stage in the series. - Rework string parsing so that patch 7 only affects new code and goes back to using sizeof (...) - 1. - Fix MMIO port matching to search again without the acccess size prefix before creating a new port - Move tentative changes to existing code to a separate series ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH v3 4/8] ns8250: Move base clock definition to a header
And while at it, unify it as clock frequency in HZ, to match the
value in "struct grub_serial_config" and do the division by
16 in one common place.
This will simplify adding SPCR support.
Signed-off-by: Benjamin Herrenschmidt
---
grub-core/term/ns8250.c | 15 ---
include/grub/ns8250.h | 13 +
2 files changed, 21 insertions(+), 7 deletions(-)
diff --git a/grub-core/term/ns8250.c b/grub-core/term/ns8250.c
index 43b9b3719..9e0ebff5a 100644
--- a/grub-core/term/ns8250.c
+++ b/grub-core/term/ns8250.c
@@ -37,12 +37,6 @@ static const grub_port_t serial_hw_io_addr[] =
GRUB_MACHINE_SERIAL_PORTS;
static int dead_ports = 0;
-#ifdef GRUB_MACHINE_MIPS_LOONGSON
-#define DEFAULT_BASE_CLOCK (2 * 115200)
-#else
-#define DEFAULT_BASE_CLOCK 115200
-#endif
-
static grub_uint8_t
ns8250_reg_read (struct grub_serial_port *port, grub_addr_t reg)
{
@@ -71,7 +65,14 @@ serial_get_divisor (const struct grub_serial_port *port
__attribute__ ((unused))
grub_uint32_t divisor;
grub_uint32_t actual_speed, error;
- base_clock = config->base_clock ? (config->base_clock >> 4) :
DEFAULT_BASE_CLOCK;
+ /* Get the UART input clock frequency */
+ base_clock = config->base_clock ? config->base_clock :
UART_DEFAULT_BASE_CLOCK;
+
+ /*
+ * The UART uses 16 times oversampling for the BRG, so adjust the value
+ * accordingly to calculate the divisor.
+ */
+ base_clock >>= 4;
divisor = (base_clock + (config->speed / 2)) / config->speed;
if (config->speed == 0)
diff --git a/include/grub/ns8250.h b/include/grub/ns8250.h
index 7947ba9c9..697637912 100644
--- a/include/grub/ns8250.h
+++ b/include/grub/ns8250.h
@@ -70,6 +70,19 @@
/* Turn on DTR, RTS, and OUT2. */
#define UART_ENABLE_OUT2 0x08
+/*
+ * Default clock input of the UART (feeds the baud rate generator)
+ *
+ * The standard value here is 1.8432 Mhz, which corresponds to
+ * 115200 bauds * 16 (16 times oversampling).
+ *
+ */
+#ifdef GRUB_MACHINE_MIPS_LOONGSON
+#define UART_DEFAULT_BASE_CLOCK ((2 * 115200) << 4)
+#else
+#define UART_DEFAULT_BASE_CLOCK (115200 << 4)
+#endif
+
#ifndef ASM_FILE
#include
--
2.34.1
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH v3 6/8] ns8250: Use ACPI SPCR table when available to configure serial
From: Benjamin Herrenschmidt
"serial auto" is now equivalent to just "serial" and will use the
SPCR to discover the port if present, otherwise defaults to "com0"
as before.
This allows to support MMIO ports specified by ACPI which is needed
on AWS EC2 "metal" instances, and will enable GRUB to pickup the
port configuration specified by ACPI in other cases.
Signed-off-by: Benjamin Herrenschmidt
---
docs/grub.texi | 9
grub-core/Makefile.core.def | 1 +
grub-core/term/ns8250-spcr.c | 83
grub-core/term/serial.c | 21 ++---
include/grub/serial.h| 1 +
5 files changed, 110 insertions(+), 5 deletions(-)
create mode 100644 grub-core/term/ns8250-spcr.c
diff --git a/docs/grub.texi b/docs/grub.texi
index 50c811a88..fd22a69fa 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -2719,6 +2719,10 @@ you want to use COM2, you must specify @samp{--unit=1}
instead. This
command accepts many other options, so please refer to @ref{serial},
for more details.
+Without argument or with @samp{--port=auto}, GRUB will attempt to use
+ACPI when available to auto-detect the default serial port and its
+configuration.
+
The commands @command{terminal_input} (@pxref{terminal_input}) and
@command{terminal_output} (@pxref{terminal_output}) choose which type of
terminal you want to use. In the case above, the terminal will be a
@@ -4172,6 +4176,11 @@ be in the range 5-8 and stop bits must be 1 or 2.
Default is 8 data
bits and one stop bit. @var{parity} is one of @samp{no}, @samp{odd},
@samp{even} and defaults to @samp{no}.
+If passed no @var{unit} nor @var{port}, or if @var{port} is set to
+@samp{auto} then GRUB will attempt to use ACPI to automatically detect
+the system default serial port and its configuration. If this information
+is not available, it will default to @var{unit} 0.
+
The serial port is not used as a communication channel unless the
@command{terminal_input} or @command{terminal_output} command is used
(@pxref{terminal_input}, @pxref{terminal_output}).
diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index 95942fc8c..b656bdb44 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -2048,6 +2048,7 @@ module = {
name = serial;
common = term/serial.c;
x86 = term/ns8250.c;
+ x86 = term/ns8250-spcr.c;
ieee1275 = term/ieee1275/serial.c;
mips_arc = term/arc/serial.c;
efi = term/efi/serial.c;
diff --git a/grub-core/term/ns8250-spcr.c b/grub-core/term/ns8250-spcr.c
new file mode 100644
index 0..0b4417a30
--- /dev/null
+++ b/grub-core/term/ns8250-spcr.c
@@ -0,0 +1,83 @@
+/*
+ * GRUB -- GRand Unified Bootloader
+ * Copyright (C) 2022 Free Software Foundation, Inc.
+ *
+ * GRUB is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * GRUB is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with GRUB. If not, see
[PATCH 2/3] serial: Avoid double lookup of serial ports
The various functions to add a port used to return port->name, and
the callers would immediately iterate all registered ports to "find"
the one just created by comparing that return value with ... port->name.
This is a waste of cycles and code. Instead, have those functions
return "port" directly.
Signed-off-by: Benjamin Herrenschmidt
---
grub-core/term/arc/serial.c | 4 ++--
grub-core/term/ieee1275/serial.c | 6 ++---
grub-core/term/ns8250-spcr.c | 2 +-
grub-core/term/ns8250.c | 12 +-
grub-core/term/serial.c | 41 +++-
include/grub/serial.h| 13 +-
6 files changed, 33 insertions(+), 45 deletions(-)
diff --git a/grub-core/term/arc/serial.c b/grub-core/term/arc/serial.c
index 87d1ce821..651f814ee 100644
--- a/grub-core/term/arc/serial.c
+++ b/grub-core/term/arc/serial.c
@@ -100,7 +100,7 @@ struct grub_serial_driver grub_arcserial_driver =
.put = serial_hw_put
};
-const char *
+struct grub_serial_port *
grub_arcserial_add_port (const char *path)
{
struct grub_serial_port *port;
@@ -120,7 +120,7 @@ grub_arcserial_add_port (const char *path)
grub_serial_register (port);
- return port->name;
+ return port;
}
static int
diff --git a/grub-core/term/ieee1275/serial.c b/grub-core/term/ieee1275/serial.c
index 7df2bee6f..b4aa9d5c5 100644
--- a/grub-core/term/ieee1275/serial.c
+++ b/grub-core/term/ieee1275/serial.c
@@ -217,7 +217,7 @@ dev_iterate (struct grub_ieee1275_devalias *alias)
return 0;
}
-static const char *
+static struct grub_serial_port *
add_port (struct ofserial_hash_ent *ent)
{
struct grub_serial_port *port;
@@ -245,10 +245,10 @@ add_port (struct ofserial_hash_ent *ent)
grub_serial_register (port);
- return port->name;
+ return port;
}
-const char *
+const struct grub_serial_port *
grub_ofserial_add_port (const char *path)
{
struct ofserial_hash_ent *ent;
diff --git a/grub-core/term/ns8250-spcr.c b/grub-core/term/ns8250-spcr.c
index f4b718833..9fe36733d 100644
--- a/grub-core/term/ns8250-spcr.c
+++ b/grub-core/term/ns8250-spcr.c
@@ -22,7 +22,7 @@
#include
#include
-char *
+struct grub_serial_port *
grub_ns8250_spcr_init (void)
{
struct grub_acpi_spcr *spcr;
diff --git a/grub-core/term/ns8250.c b/grub-core/term/ns8250.c
index 98f0b3bc3..1eb6a30b6 100644
--- a/grub-core/term/ns8250.c
+++ b/grub-core/term/ns8250.c
@@ -347,7 +347,7 @@ grub_ns8250_hw_get_port (const unsigned int unit)
return 0;
}
-char *
+struct grub_serial_port *
grub_serial_ns8250_add_port (grub_port_t port, struct grub_serial_config
*config)
{
struct grub_serial_port *p;
@@ -361,7 +361,7 @@ grub_serial_ns8250_add_port (grub_port_t port, struct
grub_serial_config *config
/* give the opportunity for SPCR to configure a default com port */
if (config != NULL)
grub_serial_port_configure (&com_ports[i], config);
-return com_names[i];
+return &com_ports[i];
}
grub_outb (0x5a, port + UART_SR);
@@ -391,10 +391,10 @@ grub_serial_ns8250_add_port (grub_port_t port, struct
grub_serial_config *config
grub_serial_config_defaults (p);
grub_serial_register (p);
- return p->name;
+ return p;
}
-char *
+struct grub_serial_port *
grub_serial_ns8250_add_mmio (grub_addr_t addr, unsigned int acc_size,
struct grub_serial_config *config)
{
@@ -406,7 +406,7 @@ grub_serial_ns8250_add_mmio (grub_addr_t addr, unsigned int
acc_size,
{
if (config != NULL)
grub_serial_port_configure (&com_ports[i], config);
-return com_names[i];
+return &com_ports[i];
}
p = grub_malloc (sizeof (*p));
@@ -428,5 +428,5 @@ grub_serial_ns8250_add_mmio (grub_addr_t addr, unsigned int
acc_size,
grub_serial_config_defaults (p);
grub_serial_register (p);
- return p->name;
+ return p;
}
diff --git a/grub-core/term/serial.c b/grub-core/term/serial.c
index 88bcd4032..275dd61af 100644
--- a/grub-core/term/serial.c
+++ b/grub-core/term/serial.c
@@ -155,14 +155,10 @@ grub_serial_find (const char *name)
if (!port && grub_strncmp (name, "port", sizeof ("port") - 1) == 0
&& grub_isxdigit (name [sizeof ("port") - 1]))
{
- name = grub_serial_ns8250_add_port (grub_strtoul (&name[sizeof ("port")
- 1],
+ port = grub_serial_ns8250_add_port (grub_strtoul (&name[sizeof ("port")
- 1],
0, 16), NULL);
- if (name == NULL)
+ if (port == NULL)
return NULL;
-
- FOR_SERIAL_PORTS (port)
-if (grub_strcmp (port->name, name) == 0)
- break;
}
if (!port && grub_strncmp (name, "mmio,", sizeof ("mmio,") - 1) == 0
&& grub_isxdigit (name [sizeof ("mmio,") - 1]))
@@ -219,38 +215,29 @@ grub_serial_find (const char *name)
break;
}
- name = grub_serial_ns8250_add_mmio (addr, acc_size, NULL);
- if (name == NULL)
+ p
[PATCH v3 8/8] serial: Add ability to specify MMIO ports via 'serial'
This adds the ability to explicitely add an MMIO based serial port
via the 'serial' command. The syntax is:
serial --port=mmio,{.b,.w,.l,.q}
Signed-off-by: Benjamin Herrenschmidt
# Conflicts:
# grub-core/term/serial.c
---
docs/grub.texi | 26 +--
grub-core/term/serial.c | 71 +++--
2 files changed, 93 insertions(+), 4 deletions(-)
diff --git a/docs/grub.texi b/docs/grub.texi
index fd22a69fa..502ca2ef7 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -4168,8 +4168,24 @@ Commands usable anywhere in the menu and in the
command-line.
@deffn Command serial [@option{--unit=unit}] [@option{--port=port}]
[@option{--speed=speed}] [@option{--word=word}] [@option{--parity=parity}]
[@option{--stop=stop}]
Initialize a serial device. @var{unit} is a number in the range 0-3
specifying which serial port to use; default is 0, which corresponds to
-the port often called COM1. @var{port} is the I/O port where the UART
-is to be found; if specified it takes precedence over @var{unit}.
+the port often called COM1.
+
+@var{port} is the I/O port where the UART is to be found or, if prefixed
+with @samp{mmio,}, the MMIO address of the UART. If specified it takes
+precedence over @var{unit}.
+
+Additionally, an MMIO address can be suffixed with:
+@itemize @bullet
+@item
+@samp{.b} for bytes access (default)
+@item
+@samp{.w} for 16-bit word access
+@item
+@samp{.l} for 32-bit long word access or
+@item
+@samp{.q} for 64-bit long long word access
+@end itemize
+
@var{speed} is the transmission speed; default is 9600. @var{word} and
@var{stop} are the number of data bits and stop bits. Data bits must
be in the range 5-8 and stop bits must be 1 or 2. Default is 8 data
@@ -4185,6 +4201,12 @@ The serial port is not used as a communication channel
unless the
@command{terminal_input} or @command{terminal_output} command is used
(@pxref{terminal_input}, @pxref{terminal_output}).
+Examples:
+@example
+serial --port=3f8 --speed=9600
+serial --port=mmio,fefb.l --speed=115200
+@end example
+
See also @ref{Serial terminal}.
@end deffn
diff --git a/grub-core/term/serial.c b/grub-core/term/serial.c
index d49261b48..d161e1d36 100644
--- a/grub-core/term/serial.c
+++ b/grub-core/term/serial.c
@@ -164,6 +164,70 @@ grub_serial_find (const char *name)
if (grub_strcmp (port->name, name) == 0)
break;
}
+ if (!port && grub_strncmp (name, "mmio,", sizeof ("mmio,") - 1) == 0
+ && grub_isxdigit (name [sizeof ("mmio,") - 1]))
+{
+ const char *p1, *p = &name[sizeof ("mmio,") - 1];
+ grub_addr_t addr = grub_strtoul (p, &p1, 16);
+ unsigned int acc_size = 1;
+ unsigned int nlen = p1 - p;
+
+ /*
+ * If we reach here, we know there's a digit after "mmio,", so
+ * all we need to check is the validity of the character following
+ * the number, which should be a termination, or a dot followed by
+ * an access size
+ */
+ if (p1[0] != '\0' && p1[0] != '.')
+{
+ grub_error (GRUB_ERR_BAD_ARGUMENT, N_("Incorrect MMIO address
syntax"));
+ return NULL;
+}
+ if (p1[0] == '.')
+switch(p1[1])
+ {
+ case 'w':
+acc_size = 2;
+break;
+ case 'l':
+acc_size = 3;
+break;
+ case 'q':
+acc_size = 4;
+break;
+ case 'b':
+acc_size = 1;
+break;
+ default:
+/*
+ * Should we abort for an unknown size ? Let's just default
+ * to 1 byte, it would increase the chance that the user who
+ * did a typo can actually see the console.
+ */
+grub_error (GRUB_ERR_BAD_ARGUMENT, N_("Incorrect MMIO access
size"));
+ }
+
+ /*
+ * Specifying the access size is optional an
grub_serial_ns8250_add_mmio()
+ * will not add it to the name. So the original loop trying to match an
+ * existing port above might have missed this one. Let's do another
+ * search ignoring the access size part of the string. At this point
+ * nlen contains the length of the name up to the end of the address.
+ */
+ FOR_SERIAL_PORTS (port)
+if (grub_strncmp (port->name, name, nlen) == 0) {
+ break;
+}
+
+ name = grub_serial_ns8250_add_mmio (addr, acc_size, NULL);
+ if (name == NULL)
+return NULL;
+
+ FOR_SERIAL_PORTS (port)
+if (grub_strcmp (port->name, name) == 0) {
+ break;
+}
+}
if (!port && grub_strcmp (name, "auto") == 0)
{
/* Look for an SPCR if any. If not, default to com0 */
@@ -212,8 +276,11 @@ grub_cmd_serial (grub_extcmd_context_t ctxt, int argc,
char **args)
if (state[OPTION_PORT].set)
{
- grub_snprintf (pname, sizeof (pname), "port%lx",
-grub_strtoul (state[1].arg, 0, 0));
+ if (gru
[PATCH 3/3] serial: Improve detection of duplicate serial ports
We currently rely on some pretty fragile comparison by name to
identify wether a serial port being configured is identical
---
grub-core/term/arc/serial.c | 4 +++
grub-core/term/ieee1275/serial.c | 4 +++
grub-core/term/ns8250.c | 16 +
grub-core/term/serial.c | 57 +---
include/grub/serial.h| 4 +++
5 files changed, 51 insertions(+), 34 deletions(-)
diff --git a/grub-core/term/arc/serial.c b/grub-core/term/arc/serial.c
index 651f814ee..487aa1b30 100644
--- a/grub-core/term/arc/serial.c
+++ b/grub-core/term/arc/serial.c
@@ -106,6 +106,10 @@ grub_arcserial_add_port (const char *path)
struct grub_serial_port *port;
grub_err_t err;
+ FOR_SERIAL_PORTS (port)
+if (grub_strcmp(path, port->name) == 0)
+ return port;
+
port = grub_zalloc (sizeof (*port));
if (!port)
return NULL;
diff --git a/grub-core/term/ieee1275/serial.c b/grub-core/term/ieee1275/serial.c
index b4aa9d5c5..afbe8dcda 100644
--- a/grub-core/term/ieee1275/serial.c
+++ b/grub-core/term/ieee1275/serial.c
@@ -227,6 +227,10 @@ add_port (struct ofserial_hash_ent *ent)
if (!ent->shortest)
return NULL;
+ FOR_SERIAL_PORTS (port)
+if (port->ent == ent)
+ return port;
+
port = grub_zalloc (sizeof (*port));
if (!port)
return NULL;
diff --git a/grub-core/term/ns8250.c b/grub-core/term/ns8250.c
index 1eb6a30b6..074bd08ca 100644
--- a/grub-core/term/ns8250.c
+++ b/grub-core/term/ns8250.c
@@ -364,6 +364,14 @@ grub_serial_ns8250_add_port (grub_port_t port, struct
grub_serial_config *config
return &com_ports[i];
}
+ FOR_SERIAL_PORTS (p)
+if (!p->mmio && p->port == port)
+ {
+if (config != NULL)
+ grub_serial_port_configure (p, config);
+return p;
+ }
+
grub_outb (0x5a, port + UART_SR);
if (grub_inb (port + UART_SR) != 0x5a)
return NULL;
@@ -409,6 +417,14 @@ grub_serial_ns8250_add_mmio (grub_addr_t addr, unsigned
int acc_size,
return &com_ports[i];
}
+ FOR_SERIAL_PORTS (p)
+if (p->mmio && p->mmio_base == addr)
+ {
+if (config != NULL)
+ grub_serial_port_configure (p, config);
+return p;
+ }
+
p = grub_malloc (sizeof (*p));
if (p == NULL)
return NULL;
diff --git a/grub-core/term/serial.c b/grub-core/term/serial.c
index 275dd61af..e214d773b 100644
--- a/grub-core/term/serial.c
+++ b/grub-core/term/serial.c
@@ -37,8 +37,6 @@
GRUB_MOD_LICENSE ("GPLv3+");
-#define FOR_SERIAL_PORTS(var) FOR_LIST_ELEMENTS((var), (grub_serial_ports))
-
enum
{
OPTION_UNIT,
@@ -65,7 +63,7 @@ static const struct grub_arg_option options[] =
{0, 0, 0, 0, 0, 0}
};
-static struct grub_serial_port *grub_serial_ports;
+struct grub_serial_port *grub_serial_ports;
struct grub_serial_output_state
{
@@ -147,26 +145,30 @@ grub_serial_find (const char *name)
{
struct grub_serial_port *port;
+ /*
+ * First look for an exact match by name, this will take care of
+ * things like "com0" which have already been created and that
+ * this function cannot re-create.
+ */
FOR_SERIAL_PORTS (port)
if (grub_strcmp (port->name, name) == 0)
- break;
+ return port;
#if (defined(__mips__) || defined (__i386__) || defined (__x86_64__)) &&
!defined(GRUB_MACHINE_EMU) && !defined(GRUB_MACHINE_ARC)
- if (!port && grub_strncmp (name, "port", sizeof ("port") - 1) == 0
+ if (grub_strncmp (name, "port", sizeof ("port") - 1) == 0
&& grub_isxdigit (name [sizeof ("port") - 1]))
{
port = grub_serial_ns8250_add_port (grub_strtoul (&name[sizeof ("port")
- 1],
0, 16), NULL);
- if (port == NULL)
-return NULL;
+ if (port != NULL)
+return port;
}
- if (!port && grub_strncmp (name, "mmio,", sizeof ("mmio,") - 1) == 0
+ if (grub_strncmp (name, "mmio,", sizeof ("mmio,") - 1) == 0
&& grub_isxdigit (name [sizeof ("mmio,") - 1]))
{
const char *p1, *p = &name[sizeof ("mmio,") - 1];
grub_addr_t addr = grub_strtoul (p, &p1, 16);
unsigned int acc_size = 1;
- unsigned int nlen = p1 - p;
/*
* If we reach here, we know there's a digit after "mmio,", so
@@ -203,45 +205,32 @@ grub_serial_find (const char *name)
grub_error (GRUB_ERR_BAD_ARGUMENT, N_("Incorrect MMIO access
size"));
}
- /*
- * Specifying the access size is optional an
grub_serial_ns8250_add_mmio()
- * will not add it to the name. So the original loop trying to match an
- * existing port above might have missed this one. Let's do another
- * search ignoring the access size part of the string. At this point
- * nlen contains the length of the name up to the end of the address.
- */
- FOR_SERIAL_PORTS (port)
-if (grub_strncmp (port->name, name, nlen) == 0) {
- break;
-}
-
port = grub_serial_ns82
[PATCH 1/3] serial: Replace usage of memcmp with strncmp
We are comparing strings after all
Signed-off-by: Benjamin Herrenschmidt
---
grub-core/term/serial.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/grub-core/term/serial.c b/grub-core/term/serial.c
index d161e1d36..88bcd4032 100644
--- a/grub-core/term/serial.c
+++ b/grub-core/term/serial.c
@@ -152,7 +152,7 @@ grub_serial_find (const char *name)
break;
#if (defined(__mips__) || defined (__i386__) || defined (__x86_64__)) &&
!defined(GRUB_MACHINE_EMU) && !defined(GRUB_MACHINE_ARC)
- if (!port && grub_memcmp (name, "port", sizeof ("port") - 1) == 0
+ if (!port && grub_strncmp (name, "port", sizeof ("port") - 1) == 0
&& grub_isxdigit (name [sizeof ("port") - 1]))
{
name = grub_serial_ns8250_add_port (grub_strtoul (&name[sizeof ("port")
- 1],
@@ -242,7 +242,7 @@ grub_serial_find (const char *name)
#endif
#ifdef GRUB_MACHINE_IEEE1275
- if (!port && grub_memcmp (name, "ieee1275/", sizeof ("ieee1275/") - 1) == 0)
+ if (!port && grub_strncmp (name, "ieee1275/", sizeof ("ieee1275/") - 1) == 0)
{
name = grub_ofserial_add_port (&name[sizeof ("ieee1275/") - 1]);
if (!name)
--
2.34.1
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH v3 1/8] acpi: Export a generic grub_acpi_find_table
And convert grub_acpi_find_fadt to use it
Signed-off-by: Benjamin Herrenschmidt
---
grub-core/kern/acpi.c | 43 +--
include/grub/acpi.h | 3 +++
2 files changed, 28 insertions(+), 18 deletions(-)
diff --git a/grub-core/kern/acpi.c b/grub-core/kern/acpi.c
index 70898ddcd..5d5b54b72 100644
--- a/grub-core/kern/acpi.c
+++ b/grub-core/kern/acpi.c
@@ -85,35 +85,42 @@ grub_acpi_xsdt_find_table (struct grub_acpi_table_header
*xsdt, const char *sig)
return 0;
}
-struct grub_acpi_fadt *
-grub_acpi_find_fadt (void)
+void *
+grub_acpi_find_table (const char *sig)
{
- struct grub_acpi_fadt *fadt = 0;
+ struct grub_acpi_fadt *r = 0;
struct grub_acpi_rsdp_v10 *rsdpv1;
struct grub_acpi_rsdp_v20 *rsdpv2;
+
rsdpv1 = grub_machine_acpi_get_rsdpv1 ();
if (rsdpv1)
-fadt = grub_acpi_rsdt_find_table ((struct grub_acpi_table_header *)
- (grub_addr_t) rsdpv1->rsdt_addr,
- GRUB_ACPI_FADT_SIGNATURE);
- if (fadt)
-return fadt;
+r = grub_acpi_rsdt_find_table ((struct grub_acpi_table_header *)
+ (grub_addr_t) rsdpv1->rsdt_addr,
+ sig);
+ if (r)
+return r;
rsdpv2 = grub_machine_acpi_get_rsdpv2 ();
if (rsdpv2)
-fadt = grub_acpi_rsdt_find_table ((struct grub_acpi_table_header *)
- (grub_addr_t) rsdpv2->rsdpv1.rsdt_addr,
- GRUB_ACPI_FADT_SIGNATURE);
- if (fadt)
-return fadt;
+r = grub_acpi_rsdt_find_table ((struct grub_acpi_table_header *)
+ (grub_addr_t) rsdpv2->rsdpv1.rsdt_addr,
+ sig);
+ if (r)
+return r;
if (rsdpv2
#if GRUB_CPU_SIZEOF_VOID_P != 8
&& !(rsdpv2->xsdt_addr >> 32)
#endif
)
-fadt = grub_acpi_xsdt_find_table ((struct grub_acpi_table_header *)
- (grub_addr_t) rsdpv2->xsdt_addr,
- GRUB_ACPI_FADT_SIGNATURE);
- if (fadt)
-return fadt;
+r = grub_acpi_xsdt_find_table ((struct grub_acpi_table_header *)
+ (grub_addr_t) rsdpv2->xsdt_addr,
+ sig);
+ if (r)
+return r;
return 0;
}
+
+struct grub_acpi_fadt *
+grub_acpi_find_fadt (void)
+{
+ return grub_acpi_find_table (GRUB_ACPI_FADT_SIGNATURE);
+}
diff --git a/include/grub/acpi.h b/include/grub/acpi.h
index 84f49487d..8c126b2b9 100644
--- a/include/grub/acpi.h
+++ b/include/grub/acpi.h
@@ -244,4 +244,7 @@ enum
struct grub_acpi_fadt *
EXPORT_FUNC(grub_acpi_find_fadt) (void);
+void *
+EXPORT_FUNC(grub_acpi_find_table) (const char *sig);
+
#endif /* ! GRUB_ACPI_HEADER */
--
2.34.1
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH v3 3/8] ns8250: Add base support for MMIO UARTs
From: Benjamin Herrenschmidt
This adds the ability for the driver to access UARTs via MMIO instead
of PIO selectively at runtime, and exposes a new function to add an
MMIO port.
In an ideal world, MMIO accessors would be generic and have architecture
specific memory barriers. However, existing drivers don't have them and
most of those "bare metal" drivers tend to be for x86 which doesn't need
them. If necessary, those can be added later.
Signed-off-by: Benjamin Herrenschmidt
---
grub-core/term/ns8250.c | 80 -
include/grub/serial.h | 11 +-
2 files changed, 74 insertions(+), 17 deletions(-)
diff --git a/grub-core/term/ns8250.c b/grub-core/term/ns8250.c
index 83b25990c..43b9b3719 100644
--- a/grub-core/term/ns8250.c
+++ b/grub-core/term/ns8250.c
@@ -43,6 +43,24 @@ static int dead_ports = 0;
#define DEFAULT_BASE_CLOCK 115200
#endif
+static grub_uint8_t
+ns8250_reg_read (struct grub_serial_port *port, grub_addr_t reg)
+{
+ asm volatile("" : : : "memory");
+ if (port->mmio)
+return *((volatile grub_uint8_t *) (port->mmio_base + reg));
+ return grub_inb (port->port + reg);
+}
+
+static void
+ns8250_reg_write (struct grub_serial_port *port, grub_uint8_t, grub_addr_t reg)
+{
+ asm volatile("" : : : "memory");
+ if (port->mmio)
+*((volatile grub_uint8_t *) (port->mmio_base + reg)) = value;
+ else
+grub_outb (value, port->port + reg);
+}
/* Convert speed to divisor. */
static unsigned short
@@ -93,43 +111,42 @@ do_real_config (struct grub_serial_port *port)
divisor = serial_get_divisor (port, &port->config);
/* Turn off the interrupt. */
- grub_outb (0, port->port + UART_IER);
+ ns8250_reg_write (port, 0, UART_IER);
/* Set DLAB. */
- grub_outb (UART_DLAB, port->port + UART_LCR);
+ ns8250_reg_write (port, UART_DLAB, UART_LCR);
- /* Set the baud rate. */
- grub_outb (divisor & 0xFF, port->port + UART_DLL);
- grub_outb (divisor >> 8, port->port + UART_DLH);
+ ns8250_reg_write (port, divisor & 0xFF, UART_DLL);
+ ns8250_reg_write (port, divisor >> 8, UART_DLH);
/* Set the line status. */
status |= (parities[port->config.parity]
| (port->config.word_len - 5)
| stop_bits[port->config.stop_bits]);
- grub_outb (status, port->port + UART_LCR);
+ ns8250_reg_write (port, status, UART_LCR);
if (port->config.rtscts)
{
/* Enable the FIFO. */
- grub_outb (UART_ENABLE_FIFO_TRIGGER1, port->port + UART_FCR);
+ ns8250_reg_write (port, UART_ENABLE_FIFO_TRIGGER1, UART_FCR);
/* Turn on DTR and RTS. */
- grub_outb (UART_ENABLE_DTRRTS, port->port + UART_MCR);
+ ns8250_reg_write (port, UART_ENABLE_DTRRTS, UART_MCR);
}
else
{
/* Enable the FIFO. */
- grub_outb (UART_ENABLE_FIFO_TRIGGER14, port->port + UART_FCR);
+ ns8250_reg_write (port, UART_ENABLE_FIFO_TRIGGER14, UART_FCR);
/* Turn on DTR, RTS, and OUT2. */
- grub_outb (UART_ENABLE_DTRRTS | UART_ENABLE_OUT2, port->port + UART_MCR);
+ ns8250_reg_write (port, UART_ENABLE_DTRRTS | UART_ENABLE_OUT2, UART_MCR);
}
/* Drain the input buffer. */
endtime = grub_get_time_ms () + 1000;
- while (grub_inb (port->port + UART_LSR) & UART_DATA_READY)
+ while (ns8250_reg_read (port, UART_LSR) & UART_DATA_READY)
{
- grub_inb (port->port + UART_RX);
+ ns8250_reg_read (port, UART_RX);
if (grub_get_time_ms () > endtime)
{
port->broken = 1;
@@ -145,8 +162,8 @@ static int
serial_hw_fetch (struct grub_serial_port *port)
{
do_real_config (port);
- if (grub_inb (port->port + UART_LSR) & UART_DATA_READY)
-return grub_inb (port->port + UART_RX);
+ if (ns8250_reg_read (port, UART_LSR) & UART_DATA_READY)
+return ns8250_reg_read (port, UART_RX);
return -1;
}
@@ -166,7 +183,7 @@ serial_hw_put (struct grub_serial_port *port, const int c)
else
endtime = grub_get_time_ms () + 200;
/* Wait until the transmitter holding register is empty. */
- while ((grub_inb (port->port + UART_LSR) & UART_EMPTY_TRANSMITTER) == 0)
+ while ((ns8250_reg_read (port, UART_LSR) & UART_EMPTY_TRANSMITTER) == 0)
{
if (grub_get_time_ms () > endtime)
{
@@ -179,7 +196,7 @@ serial_hw_put (struct grub_serial_port *port, const int c)
if (port->broken)
port->broken--;
- grub_outb (c, port->port + UART_TX);
+ ns8250_reg_write (port, c, UART_TX);
}
/* Initialize a serial device. PORT is the port number for a serial device.
@@ -262,6 +279,7 @@ grub_ns8250_init (void)
com_ports[i].name = com_names[i];
com_ports[i].driver = &grub_ns8250_driver;
com_ports[i].port = serial_hw_io_addr[i];
+ com_ports[i].mmio = false;
err = grub_serial_config_defaults (&com_ports[i]);
if (err)
grub_print_error ();
@@ -289,6 +307,7 @@ grub_serial_ns8250_add_port (grub_port_t port)
{
struct grub_serial_port *p;
unsigned i;
+
for (i = 0; i < GRUB_SERIAL_POR
[PATCH 0/3] serial: Cleanup & fix port discovery
This series cleans up a few things in serial port discovery and fixes detection of duplicates in some cases: - Existing usage of memcmp to compare strings is replaces with strncmp - Don't lookup ports by name right after creating them, just return the pointer instead. - Move the detection of duplicates *into* the various port creation functions. At this point, the various attributes such as port number and address have been properly parsed and there is no confusion between things like wether a 0x prefix is present or not for example, or wether hex digits are upper or lower case. ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH v3 2/8] acpi: Add SPCR and generic address definitions
This adds the definition of the two ACPI tables according to
the spec.
Signed-off-by: Benjamin Herrenschmidt
---
include/grub/acpi.h | 51 +
1 file changed, 51 insertions(+)
diff --git a/include/grub/acpi.h b/include/grub/acpi.h
index 8c126b2b9..17aadb802 100644
--- a/include/grub/acpi.h
+++ b/include/grub/acpi.h
@@ -179,6 +179,57 @@ enum
GRUB_ACPI_MADT_ENTRY_SAPIC_FLAGS_ENABLED = 1
};
+struct grub_acpi_genaddr {
+ grub_uint8_t space_id;
+#define GRUB_ACPI_GENADDR_MEM_SPACE 0x00
+#define GRUB_ACPI_GENADDR_IO_SPACE 0x01
+ grub_uint8_t bit_width;
+ grub_uint8_t bit_offset;
+ grub_uint8_t access_size;
+#define GRUB_ACPI_GENADDR_SIZE_LGCY 0x00
+#define GRUB_ACPI_GENADDR_SIZE_BYTE 0x01
+#define GRUB_ACPI_GENADDR_SIZE_WORD 0x02
+#define GRUB_ACPI_GENADDR_SIZE_DWORD 0x03
+#define GRUB_ACPI_GENADDR_SIZE_QWORD 0x04
+ grub_uint64_t addr;
+} GRUB_PACKED;
+
+#define GRUB_ACPI_SPCR_SIGNATURE "SPCR"
+
+struct grub_acpi_spcr {
+ struct grub_acpi_table_header hdr;
+ grub_uint8_t intf_type;
+#define GRUB_ACPI_SPCR_INTF_TYPE_16550 0x00
+#define GRUB_ACPI_SPCR_INTF_TYPE_16550X 0x01
+ grub_uint8_t reserved_0[3];
+ struct grub_acpi_genaddr base_addr;
+ grub_uint8_t interrupt_type;
+ grub_uint8_t irq;
+ grub_uint32_t gsi;
+ grub_uint8_t baud_rate;
+#define GRUB_ACPI_SPCR_BAUD_CURRENT 0x00
+#define GRUB_ACPI_SPCR_BAUD_9600 0x03
+#define GRUB_ACPI_SPCR_BAUD_192000x04
+#define GRUB_ACPI_SPCR_BAUD_576000x06
+#define GRUB_ACPI_SPCR_BAUD_115200 0x07
+ grub_uint8_t parity;
+ grub_uint8_t stop_bits;
+ grub_uint8_t flow_control;
+#define GRUB_ACPI_SPCR_FC_DCD_TX (1 << 0)
+#define GRUB_ACPI_SPCR_FC_RTSCTS (1 << 1)
+#define GRUB_ACPI_SPCR_FC_XONXOFF(1 << 2)
+ grub_uint8_t terminal_type;
+ grub_uint8_t language;
+ grub_uint16_tpci_device_id;
+ grub_uint16_t pci_vendor_id;
+ grub_uint8_t pci_bus;
+ grub_uint8_t pci_device;
+ grub_uint8_t pci_function;
+ grub_uint32_t pci_flags;
+ grub_uint8_t pci_segment;
+ grub_uint32_t reserved_1;
+} GRUB_PACKED;
+
#ifndef GRUB_DSDT_TEST
struct grub_acpi_rsdp_v10 *grub_acpi_get_rsdpv1 (void);
struct grub_acpi_rsdp_v20 *grub_acpi_get_rsdpv2 (void);
--
2.34.1
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [RFC PATCH v3 0/1] kern/dl: Add module version check
Hi, I think there are some misunderstandings. This feature is implemented in kern/dl.c in core.img, which is UNDER YOUR CONTROL. Let me analyze the worst case from your perspective: 1) Every distro is enforcing version check, even in BIOS mode. 2) Because this check is in kern/dl.c, there is no such code in any module (.mod) files. However, this modules are little fatter because they have version information (.modver section), which is harmless. 3) You can still build your own copy of core.img with this check disabled (or just revert this patch completely), then ship that core.img with Rufus. 4) Since there is no check in your core.img, it will load any module happily, regardless whether it has mismatched version information. Therefore, there is no problem at all. The only thing you need to do is build your core.img with this patch reverted in the future. Then things will "work" as usual. Let me analyze why I want to add version check to BIOS mode: 1) Because you can always remove this patch from your build for Rufus, there is no harm in having this patch in vanilla GRUB. 2) This patch can provide benefits to BIOS users, because it can diagnose improper installations. For example, user runs `grub-install /dev/sdb` to update GRUB but machine boot from `/dev/sda`. So there are benefits without any harm, why not have it in BIOS mode? By the way, I highly recommend you to test latest git GRUB (as a preview of upcoming 2.12 release) with Rufus and it will almost certainly break your existing Rufus build. e.g. use Rufus (with GRUB2.06) to process ISOs (with GRUB2.12); and the reverse, use Rufus (with GRUB2.12) to process ISOs (with GRUB2.06). You probably need to ship another bunch of core.img with Rufus. Also, I'd also like to advice you to create a patchset/proposal, which make it easy to convert the bootcode in ISO to the bootcode in USB. I think you are expert in this area. If that patch finally got merged (e.g. before 2.12 release window), you life will be definitely easier because you are free from shipping another bunch of core.img. However, since I'm not a expert of this area, please point out if I said something wrong. Best Regards, Zhang Boyang On 2022/12/23 02:16, Pete Batard wrote: I'm sorry but that's NO_GO for me. This is a major step back from v2, where the check was never applicable for BIOS and I believe I explained why a module check that can be enforced for BIOS is going to create a major headache for end-users. It seems pretty obvious that, if distro maintainers start to use --enable-modver-check=enforce for UEFI they are also going to use the same option for BIOS (because why should they use a different compilation options if it applies for both?), and you *ARE* going to leave Windows users stranded through them not trying to boot GRUB based media because they will be forced to use DD to create a workable media (even though, and this is the major point here, in the very vast majority of cases there is not issue with mixing versions if you're on the same major/minor base for GRUB), and consider that, because they cannot see its content, or can only see the ESP, their media was not properly created and is not worth trying to boot. This is a *VERY REAL* problem, for which I have provided many examples (see [1]), and could provide even more, which is why utilities like Rufus try to steer away from using DD mode where possible. However, if people can use --enable-modver-check=enforce for BIOS, then, under the aim of solving one minor issue (that from looking the Google links for "452: out of range pointer" seem to be a problem that is mostly encountered in UEFI environments rather than BIOS, or at least is not a BIOS-only issue, and therefore something that will be sorted out for the majority of folks experiencing it *even* if --enable-modver-check cannot apply for BIOS), you will be adding to an already existing a major one, and you are going to leave people, and especially Windows users who are trying to switch to Linux, stranded. In short, GRUB would be doing a major disservice to its end users if it is to allow module version checks to be enforceable for BIOS, and, considering how there is little benefit in enforcing this in an environment that cannot be secured in the first place, it's hard to see a justification for what is essentially a *MAJOR* design regression from the v2 of this patch. Or at the very least, a better approach is to introduce it for UEFI only, then, once that has been integrated with UEFI for a while, see if BIOS users are actually reporting issues due to module version mismatch, and reconvene on whether we really want to allow this check to also apply to BIOS. Thus, in my view, the proper approach for the sake of the end users is leave default BIOS boot behaviour as it currently stands, with no warning and no possibility of failing the boot process on version mismatch (because,
