[PATCH] Enable /dev/mapper/dm-[0-9]-* scanning
This code was modified because the original code skips any device with basename starts with /dm-[0-9]/ e.g. if the root device path is //dev/mapper/dm-0-luks/ it won't be detected by grub-probe and "root=" parameter in grub.cfg will be empty and the system will be unbootable unless you manually edit grub.cfg BUG: https://savannah.gnu.org/bugs/?53697 diff --git a/grub/grub-core/osdep/unix/getroot.c b/grub/grub-core/osdep/unix/getroot.c index 4bf37b0..2964dcd 100644 --- a/grub/grub-core/osdep/unix/getroot.c +++ b/grub/grub-core/osdep/unix/getroot.c @@ -433,7 +433,8 @@ grub_find_device (const char *dir, dev_t dev) ent->d_name[1] == 'm' && ent->d_name[2] == '-' && ent->d_name[3] >= '0' && - ent->d_name[3] <= '9') + ent->d_name[3] <= '9' && + ent->d_name[4] == '\0') continue; #endif PS I don't know what to do in case of //dev/dm-[0-9]+$/ yet ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH V2] Add support for BTRFS raid5/6 to GRUB
On Wed, May 09, 2018 at 09:36:55PM +0200, Goffredo Baroncelli wrote: > On 05/09/2018 07:00 PM, Daniel Kiper wrote: > > On Tue, Apr 24, 2018 at 09:13:09PM +0200, Goffredo Baroncelli wrote: > >> > >> Hi All, > >> > >> the aim of this patches set is to provide support for a BTRFS raid5/6 > >> filesystem in GRUB. > >> > >> The first patch, implements the basic support for raid5/6. I.e this works > >> when > >> all the disks > >> are present. > >> > >> The next 4 patches, are preparatory ones. > >> > >> The last two implements the support for raid5/6 recovery. It allow to read > >> from the filesystem even when 1 or 2 (for raid 6) disk(s) are missing. > >> > >> The last one is the more controversial. The code for the raid6 recovery is > >> copied from the GRUB file reaid6_recovery.c . I preferred this approach, > >> because the original code assumes that the read is GRUB_DISK_SECTOR_SIZE > >> bytes based (512 bytes). Instead the GRUB BTRFS implementation need to > >> read the disk with a granularity of the byte. > >> I tried to update the code (and the function which the code calls), but > >> the change was quite invasive. So for now I preferred to duplicating the > >> code, to get some feedback. > >> > >> I tested the code in grub-emu, and it works (for me) both with all the > >> disks, > >> and with some disks missing. I checked the sh1sums calculated from grub > >> and > >> from linux and these matched. > >> > >> Comments are welcome. > > > > Mostly nitpicks. However, if we can reuse reaid6_recovery.c somehow that > > will be great. Even if this require some more patches. > > I liked the idea too. I am not sure to be able to tests all the cases. > Do you know if it exists a list of tests about raid6_recovery and dm ? Sadly I know nothing about that. > Anyway, I update the patch. Tomorrow I will test these and then I issue the > patch set. I agree mostly with your replies. Please respin the patches and I will take a look at v2. Thank you for doing the work. Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] loader/i386/linux: calculate the size of the setup header
On Wed, May 09, 2018 at 10:46:47AM -0700, Andrew Jeddeloh wrote:
> This patch is prompted from a question I asked a while ago about why
> the disk read is necessary. See the thread here [1].
>
> This changes the disk read to use the length of the setup header as
> calculated by the x86 32 bit linux boot protocol [1]. I'm not 100%
> sure its patch that's wanted however. The idea was that grub should
There is no doubt. We want this patch.
> only read the amount specified by the boot protocol and not more, but
> it turns out the size of the linux_kernel_params struct is already
> sized such that grub reads the exact amount anyway (at least with the
> recent kernels I've tested with). This introduces two changes:
OK but earlier you have told that linux_kernel_params.e820_map[] is
overwritten. Is it still valid?
> - if a new version of linux makes the setup headers section larger,
> this will fail instead of only readiing the old fields. I'm not sure
> if this behavior is desired.
It is but math is wrong. Please look below.
> - If older versions have a smaller setup header section, less will be read.
Exactly.
> [1] http://lists.gnu.org/archive/html/grub-devel/2018-04/msg00073.html
> [2]
> https://raw.githubusercontent.com/torvalds/linux/master/Documentation/x86/boot.txt
>
>
> Previously the length was just assumed to be the size of the
> linux_params struct. The linux x86 32 bit boot protocol says the size of
> the setup header is 0x202 + the byte at 0x201 in the boot params.
> Calculate the size of the header, rather than assume it is the size of
> the linux_params struct.
>
> Signed-off-by: Andrew Jeddeloh
> ---
> grub-core/loader/i386/linux.c | 11 ++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/grub-core/loader/i386/linux.c b/grub-core/loader/i386/linux.c
> index 44301e126..9b4d33785 100644
> --- a/grub-core/loader/i386/linux.c
> +++ b/grub-core/loader/i386/linux.c
> @@ -805,7 +805,16 @@ grub_cmd_linux (grub_command_t cmd __attribute__
> ((unused)),
>linux_params.kernel_alignment = (1 << align);
>linux_params.ps_mouse = linux_params.padding10 = 0;
>
> - len = sizeof (linux_params) - sizeof (lh);
> + // The linux 32 bit boot protocol defines the setup header size to be
> 0x202 + the size of
> + // the jump at 0x200. We've already read sizeof(lh)
s/the size of the jump at 0x200/byte value at offset 0x201/
s/We've already read sizeof(lh)//
And please use /* ... */ for comments instead of //
> + len = *((char *)&linux_params.jump + 1) + 0x202 - sizeof(lh);
len = *((char *)&linux_params.jump + 1) + 0x202;
> + // Verify the struct is big enough so we do not write past the end
> + if (len + sizeof(lh) > sizeof(linux_params)) {
if (len > &linux_params.e820_map - &linux_params)
> +grub_error (GRUB_ERR_BAD_OS, "boot params setup header too big
> for linux_params struct");
s/boot params/Linux/
> +goto fail;
> + }
/* We've already read lh so there is not need to read it second time. */
len -= sizeof (lh);
> +
>if (grub_file_read (file, (char *) &linux_params + sizeof (lh), len) !=
> len)
> {
>if (!grub_errno)
I hope that helps.
Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Enable /dev/mapper/dm-[0-9]-* scanning
On Thu, May 10, 2018 at 04:32:46PM +0300, Oleg Solovyov wrote: > This code was modified because the original code skips any device with > basename starts with /dm-[0-9]/ > e.g. if the root device path is //dev/mapper/dm-0-luks/ it won't be > detected by grub-probe and "root=" parameter in grub.cfg will be empty > and the system will be unbootable unless you manually edit grub.cfg > > BUG: https://savannah.gnu.org/bugs/?53697 > > >diff --git a/grub/grub-core/osdep/unix/getroot.c > >b/grub/grub-core/osdep/unix/getroot.c > >index 4bf37b0..2964dcd 100644 > >--- a/grub/grub-core/osdep/unix/getroot.c > >+++ b/grub/grub-core/osdep/unix/getroot.c > >@@ -433,7 +433,8 @@ grub_find_device (const char *dir, dev_t dev) > > ?? ent->d_name[1] == 'm' && > > ?? ent->d_name[2] == '-' && > > ?? ent->d_name[3] >= '0' && > >-?? ?? ent->d_name[3] <= '9') > >+?? ?? ent->d_name[3] <= '9' && > >+?? ?? ent->d_name[4] == '\0') > > ?? continue; > >??#endif > > > PS > I don't know what to do in case of //dev/dm-[0-9]+$/ yet Your solution is not reliable. What about /dev/dm-10? I think that you should add check for /dev directory. This should work much better. Additionally, please use git format-patch/send-email to create and send patches. And do not forget about SOB (Signed-off-by). Good example is here: http://lists.gnu.org/archive/html/grub-devel/2018-04/msg00055.html Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: Multiboot ELF segment handling patch
On Thu, Apr 26, 2018 at 11:55:22PM +0200, Alexander Boettcher wrote:
> On 24.04.2018 12:42, Daniel Kiper wrote:
> > On Mon, Apr 23, 2018 at 08:26:34PM +0200, Alexander Boettcher wrote:
> >> On 17.04.2018 21:40, Daniel Kiper wrote
> The overriden memory may contain device memory (vga text mode e.g.),
> which
> leads to strange boot behaviour.
> >>>
> >>> Have you been able to take a look at memory allocator/relocator code why
> >>> this happened?
> >>
> >> for me it looks like, that either already
> >> grub_relocator_alloc_chunk_addr or at latest
> >> grub_relocator_prepare_relocs should bail out with an error, if the
> >> memory range of the VGA memory (0xa ++) is covered.
> >
> > Exactly!
> >
> >> What I don't know, respectively didn't found in the code, how and where
> >> this information about this VGA text memory area is excluded/reserved
> >> from the allocator/reallocator. Maybe you can give me a hint ?
> >
> > I have skimmed through the code and I think that you should take
> > a look at grub-core/kern/i386/pc/init.c, grub-core/kern/mm.c and
> > grub-core/lib/relocator.c. At first sight it looks that
> > grub-core/kern/i386/pc/init.c:grub_machine_init() is the
> > most interesting thing.
> >
> > I hope that helps. If you have further questions drop me a line.
> >
> > Thank you for taking a stab at this.
>
> The whole lower 1M physical memory is reserved and not available for use
> (done by grub-core/kern/i386/pc/init.c), but effectively ignored by
> grub_relocator_alloc_chunk_addr for the non relocatable case during
> multiboot segment load.
>
> The adjust_limits call in grub_relocator_alloc_chunk_addr mainly sets
> the min_addr to 0 and max_addr to end of physical memory. So
> grub_relocator_alloc_chunk_addr will ever find a piece of (higher)
> memory, mainly ignoring the target address (which is reserved, e.g.
> below 1M).
>
> The showcase code snippet below catches the case now. I don't know
> whether it is correct nor do I like it ...
>
> What do you think ?
>
>
> Alex
>
>
> --- a/grub-core/lib/relocator.c
> +++ b/grub-core/lib/relocator.c
> @@ -1226,7 +1237,7 @@ adjust_limits (struct grub_relocator *rel,
> grub_err_t
> grub_relocator_alloc_chunk_addr (struct grub_relocator *rel,
>grub_relocator_chunk_t *out,
> - grub_phys_addr_t target, grub_size_t size)
> + grub_phys_addr_t target, grub_size_t size, int
> relocatable)
> {
>struct grub_relocator_chunk *chunk;
>grub_phys_addr_t min_addr = 0, max_addr;
> @@ -1250,6 +1263,10 @@ grub_relocator_alloc_chunk_addr (struct
> grub_relocator *rel,
> (unsigned long long) min_addr, (unsigned long long) max_addr,
> (unsigned long long) target);
>
> + if (!relocatable &&
> + !malloc_in_range (rel, target, target + size, 1, size, chunk, 0, 1))
> +return grub_error (GRUB_ERR_BUG, "target memory not available");
> +
>do
> {
>/* A trick to improve Linux allocation. */
AIUI grub_relocator_alloc_chunk_addr() should allocate memory region
with exact properties, i.e, target and size. So, I think that relocatable
argument does not make sense here. However, I have a feeling that this
piece of code is a problem:
1255 /* A trick to improve Linux allocation. */
1256#if defined (__i386__) || defined (__x86_64__)
1257 if (target < 0x10)
1258if (malloc_in_range (rel, rel->highestnonpostaddr,
~(grub_addr_t)0, 1,
1259 size, chunk, 0, 1))
1260 {
1261if (rel->postchunks > chunk->src)
1262 rel->postchunks = chunk->src;
1263break;
1264 }
1265#endif
There is a chance that if you comment it out then Multiboot/Multiboot2
will work as expected. If it is true then we have to think how to
fix it and do not break Linux boot.
Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
