Re: Checksummed environments

[Daniel Kiper]

On Fri, Apr 06, 2018 at 11:25:22AM +0200, Kristian Amlie wrote:
> Hey, I work for Northern.tech, developing update software for embedded
> Linux devices.
>
> I have a question about GRUB's environment block: This block is not

I am not sure what exactly you mean by "GRUB's environment block".
Could you send me some references to the code?

> checksummed, and hence I reckon it can become corrupt if power is lost
> in the middle of a write.

What about the other blocks?

> This is an important safety criterion for us, so we've been thinking of
> developing environment block checksumming as an extension to the
> existing save_env and load_env commands. The most likely approach will
> be to grab X amount of bytes at the end of the block and use these for
> the checksum.

Could you tell us more about that?

> This would also allow us to fall back to an earlier environment file if
> the current one is corrupt, hence implementing redundancy.
>
> Is this something that the GRUB project would be interested in? We want
> to upstream this if possible, since we think many people may benefit
> from this.

Great! However, first of all we need some more info about that.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: Add TPM measured boot support

[Daniel Kiper]

Hi Matthew,

On Sat, Apr 07, 2018 at 01:36:28AM +0100, Matthew Garrett wrote:
> On Tue, Jan 23, 2018 at 12:45:14PM +0100, Daniel Kiper wrote:
>
> > Sadly yes. Sorry about that. However, this is still on my radar. I hope that
> > I come back to work on this in a few weeks.
>
> Hi Daniel,
>
> Any news on this front? Thanks!

Good news is that my plans has not changed. Bad news is that
everything moves much slower than I expected. However, this
task is the second one on my TODO list. Right now I have to
post some Xen secure boot patches (this is also delayed a few
months) which depend on this work to some extent too. So,
I expect that I will take a stab at it in May or June. Anyway,
sorry for delays.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: stale comment in diskboot.S ?

[Daniel Kiper]

On Wed, Mar 28, 2018 at 04:09:00PM +0800, Cao jin wrote:
> Hi,
>
> I was reading the code, and I want to confirm is following comment is stale?
>
>   /*
>* _start is loaded at 0x2000 and is jumped to with
>* CS:IP 0:0x2000 in kernel.
>*/
>
> As I understand, it is loaded at 0x8000 and is jumped to with 0:0x8000.

It looks that you are right. Could you fix that comment?
I am happy to take a patch for it.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [GRUB PARTUUID PATCH V9 4/5] Update grub script template files

[Daniel Kiper]

On Sat, Apr 07, 2018 at 04:28:13PM -0700, Nicholas Vinson wrote:
> Update grub-mkconfig.in and 10_linux.in to support grub-probe's new
> partuuid target.  Update grub.texi documentation.  The following table
> shows how GRUB_DISABLE_LINUX_UUID, GRUB_DISABLE_LINUX_PARTUUID, and
> initramfs detection interact:
>
> Initramfs  GRUB_DISABLE_LINUX_PARTUUID  GRUB_DISABLE_LINUX_UUID  Linux Root
> detected   Set  Set  ID Method
>
> False  FalseFalsepart UUID
> False  FalseTrue part UUID
> False  True Falsedev name
> False  True True dev name
> True   FalseFalsefs UUID
> True   FalseTrue part UUID
> True   True Falsefs UUID
> True   True True dev name

What will happen if GRUB_DISABLE_LINUX_PARTUUID and/or GRUB_DISABLE_LINUX_UUID
are not set? I think that you can avoid that by setting defaults. You do that
for GRUB_DISABLE_LINUX_PARTUUID in next patch but GRUB_DISABLE_LINUX_UUID
does not have any default.

> Signed-off-by: Nicholas Vinson 
> ---
>  docs/grub.texi  | 10 ++
>  util/grub-mkconfig.in   |  3 +++
>  util/grub.d/10_linux.in | 18 +++---

Could you update util/grub.d/20_linux_xen.in too?

>  3 files changed, 28 insertions(+), 3 deletions(-)
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index 65b4bbeda..06f0afe45 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -1424,6 +1424,16 @@ the Linux kernel, using a @samp{root=UUID=...} kernel 
> parameter.  This is
>  usually more reliable, but in some cases it may not be appropriate.  To
>  disable the use of UUIDs, set this option to @samp{true}.
>
> +@item GRUB_DISABLE_LINUX_PARTUUID
> +If @command{grub-mkconfig} cannot identify the root filesystem via its
> +universally-unique indentifier (UUID), @command{grub-mkconfig} will use the 
> UUID
> +of the partition containing the filesystem to identify the root filesystem to
> +the Linux kernel via a @samp{root=PARTUUID=...} kernel parameter.  This is 
> not
> +as reliable as using the filesystem UUID, but is more reliable than using the
> +Linux device names.  When enabled, this option requires the Linux kernel 
> version

s/When enabled, /When enabled, GRUB_DISABLE_LINUX_PARTUUID set to false, /
or something like that. Otherwise it is unclear.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [GRUB PARTUUID PATCH V9 0/5] Add PARTUUID detection support

[Daniel Kiper]

On Sat, Apr 07, 2018 at 04:28:09PM -0700, Nicholas Vinson wrote:
> Changes from Patch v8:
> - Renamed GRUB_ENABLE_LINUX_PARTUUID to GRUB_DISABLE_LINUX_PARTUUID
> - Updated the 10_linux logic so GRUB_ENABLE_LINUX_PARTUUID and
> GRUB_ENABLE_LINUX_UUID would behave more independently.
> - Documented interactions of GRUB_DISABLE_LINUX_UUID,
> GRUB_DISABLE_LINUX_PARTUUID, and initramfs detection in commit
> message.
> - Added optional patch that sets GRUB_DISABLE_LINUX_PARTUUID to true
> by default.
> - Fixed merge conflicts between this patchset and upstream's master
> branch.

Could you provide summary for current version not only for previous ones?

Anyway, I think that most of the patches are now in good shape.
I am looking forward for next version.

Thank you for doing the work.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: Checksummed environments

[Daniel Kiper]

On Fri, Apr 06, 2018 at 03:08:23PM +0200, Kristian Amlie wrote:
> On 06/04/18 14:35, Daniel Kiper wrote:
> > On Fri, Apr 06, 2018 at 11:25:22AM +0200, Kristian Amlie wrote:
> >> Hey, I work for Northern.tech, developing update software for embedded
> >> Linux devices.
> >>
> >> I have a question about GRUB's environment block: This block is not
> >
> > I am not sure what exactly you mean by "GRUB's environment block".
> > Could you send me some references to the code?
>
> Of course, sorry if the context wasn't clear. I'm talking about the
> environment block mentioned here:
> https://www.gnu.org/software/grub/manual/grub/grub.html#Environment-block,
> which is used to store information from one boot to the next, using
> save_env and load_env.
>
> >> checksummed, and hence I reckon it can become corrupt if power is lost
> >> in the middle of a write.
> >
> > What about the other blocks?
>
> In theory, there is only one block, because it is written in-place,
> directly on disk, without changing any other filesystem blocks. Is that
> what you meant by "other blocks"?
>
> >> This is an important safety criterion for us, so we've been thinking of
> >> developing environment block checksumming as an extension to the
> >> existing save_env and load_env commands. The most likely approach will
> >> be to grab X amount of bytes at the end of the block and use these for
> >> the checksum.
> >
> > Could you tell us more about that?
>
> The idea comes from U-Boot [1], which uses a dedicated block on a
> storage device to store data, and uses a CRC32 checksum to make sure it
> is consistent. The motivation is to be able to detect that the block is
> corrupt, rather than accepting a block of data that may have incorrect
> data in if a write was interrupted midway by a powerloss. You can read
> more about it in the link.
>
> [2] https://www.denx.de/wiki/DULG/UBootEnvVariables

I am OK with the idea. However, I think that the feature should have
a kind of switch to turn it off/on. At first sight it looks that new
environment variable is sufficient for it.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] diskboot: trivial correction on stale comments

[Daniel Kiper]

On Wed, Apr 11, 2018 at 11:23:10AM +0800, Cao jin wrote:
> diskboot.img now is loaded at 0x8000 and is jumped to with 0:0x8000.
>
> Signed-off-by: Cao jin 

Reviewed-by: Daniel Kiper 

Thanks a lot! I will apply this patch together with other
patches in a week or so.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [GRUB PARTUUID PATCH V9 4/5] Update grub script template files

[Daniel Kiper]

On Tue, Apr 10, 2018 at 08:00:04PM -0700, Nick Vinson wrote:
> On 04/10/2018 01:52 PM, Daniel Kiper wrote:
> > On Sat, Apr 07, 2018 at 04:28:13PM -0700, Nicholas Vinson wrote:
> >> Update grub-mkconfig.in and 10_linux.in to support grub-probe's new
> >> partuuid target.  Update grub.texi documentation.  The following table
> >> shows how GRUB_DISABLE_LINUX_UUID, GRUB_DISABLE_LINUX_PARTUUID, and
> >> initramfs detection interact:
> >>
> >> Initramfs  GRUB_DISABLE_LINUX_PARTUUID  GRUB_DISABLE_LINUX_UUID  Linux Root
> >> detected   Set  Set  ID Method
> >>
> >> False  FalseFalsepart UUID
> >> False  FalseTrue part UUID
> >> False  True Falsedev name
> >> False  True True dev name
> >> True   FalseFalsefs UUID
> >> True   FalseTrue part UUID
> >> True   True Falsefs UUID
> >> True   True True dev name
> >
> > What will happen if GRUB_DISABLE_LINUX_PARTUUID and/or 
> > GRUB_DISABLE_LINUX_UUID
> > are not set? I think that you can avoid that by setting defaults. You do 
> > that
> > for GRUB_DISABLE_LINUX_PARTUUID in next patch but GRUB_DISABLE_LINUX_UUID
> > does not have any default.
> >
>
> If they're not set, then that's the same as them being set to 'False'.
> I should have worded my table above a bit differently and used Yes/No
> instead of True/False as that is really what it is trying to convey.

IMO it will be more confusing. I think that I would use lowercase
false/true as it is used in the script and below the table I would
add a note that  == false or something like that.

> I.E. If there is no initramfs detected and GRUB_DISABLE_LINUX_PARTUUID
> is not set and GRUB_DISABLE_LINUX_UUID is not set, set the kernel root
> variable to "root=PARTUUID=...".
>
> In the scripts, the only value explicitly checked for is 'true'.
> Anything else (including unset) is considered false.

OK, perfect!

> >> Signed-off-by: Nicholas Vinson 
> >> ---
> >>  docs/grub.texi  | 10 ++
> >>  util/grub-mkconfig.in   |  3 +++
> >>  util/grub.d/10_linux.in | 18 +++---
> >
> > Could you update util/grub.d/20_linux_xen.in too?
>
> Let me see what I can do.  I have never worked with xen before, so I do
> not know much about it.

This should be easy. However, if you are not sure please drop me a line.

> >>  3 files changed, 28 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/docs/grub.texi b/docs/grub.texi
> >> index 65b4bbeda..06f0afe45 100644
> >> --- a/docs/grub.texi
> >> +++ b/docs/grub.texi
> >> @@ -1424,6 +1424,16 @@ the Linux kernel, using a @samp{root=UUID=...} 
> >> kernel parameter.  This is
> >>  usually more reliable, but in some cases it may not be appropriate.  To
> >>  disable the use of UUIDs, set this option to @samp{true}.
> >>
> >> +@item GRUB_DISABLE_LINUX_PARTUUID
> >> +If @command{grub-mkconfig} cannot identify the root filesystem via its
> >> +universally-unique indentifier (UUID), @command{grub-mkconfig} will use 
> >> the UUID
> >> +of the partition containing the filesystem to identify the root 
> >> filesystem to
> >> +the Linux kernel via a @samp{root=PARTUUID=...} kernel parameter.  This 
> >> is not
> >> +as reliable as using the filesystem UUID, but is more reliable than using 
> >> the
> >> +Linux device names.  When enabled, this option requires the Linux kernel 
> >> version
> >
> > s/When enabled, /When enabled, GRUB_DISABLE_LINUX_PARTUUID set to false, /
> > or something like that. Otherwise it is unclear.
>
> I'll clean up the wording in the next release.

Thanks a lot!

By the way, should not we merge patches 4 and 5 into one patch?

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] ieee1275: obdisk driver

[Daniel Kiper]

On Thu, Apr 05, 2018 at 10:53:55AM -0700, Eric Snowberg wrote:
> Add a new disk driver called obdisk for IEEE1275 platforms.  Currently
> the only platform using this disk driver is SPARC, however other IEEE1275
> platforms could start using it if they so choose.  While the functionality
> within the other IEEE1275 ofdisk driver may be suitable for PPC and x86, it

s/other/current/?

> presented too many problems on SPARC hardware.
>
> Within the old ofdisk, there is not a way to determine the true canonical
> name for the disk.  Within Open Boot, the same disk can have multiple names
> but all reference the same disk.  For example the same disk can be referenced
> by its SAS WWN, using this form:
>
> /pci@302/pci@2/pci@0/pci@17/LSI,sas@0/disk@w5000cca02f037d6d,0
>
> It can also be referenced by its PHY identifier using this form:
>
> /pci@302/pci@2/pci@0/pci@17/LSI,sas@0/disk@p0
>
> It can also be referenced by its Target identifier using this form:
>
> /pci@302/pci@2/pci@0/pci@17/LSI,sas@0/disk@0
>
> Also, when the LUN=0, it is legal to omit the ,0 from the device name.  So 
> with
> the disk above, before taking into account the device aliases, there are 6 
> ways
> to reference the same disk.
>
> Then it is possible to have 0 .. n device aliases all representing the same 
> disk.
> Within this new driver the true canonical name is determined using the the
> IEEE1275 encode-unit and decode-unit commands when address_cells == 4.  This
> will determine the true single canonical name for the device so multiple 
> ihandles
> are not opened for the same device.  This is what frequently happens with the 
> old
> ofdisk driver.  With some devices when they are opened multiple times it 
> causes
> the entire system to hang.
>
> Another problem solved with this driver is devices that do not have a device
> alias can be booted and used within GRUB. Within the old ofdisk, this was not
> possible, unless it was the original boot device.  All devices behind a SAS
> or SCSI parent can be found.   Within the old ofdisk, finding these disks
> relied on there being an alias defined.  The alias requirement is not
> necessary with this new driver.  It can also find devices behind a parent
> after they have been hot-plugged.  This is something that is not possible
> with the old ofdisk driver.
>
> The old ofdisk driver also incorrectly assumes that the device pointing to by 
> a
> device alias is in its true canonical form. This assumption is never made with
> this new driver.
>
> Another issue solved with this driver is that it properly caches the ihandle
> for all open devices.  The old ofdisk tries to do this by caching the last
> opened ihandle.  However this does not work properly because the layer above
> does not use a consistent device name for the same disk when calling into the
> driver.  This is because the upper layer uses the bootpath value returned 
> within
> /chosen, other times it uses the device alias, and other times it uses the
> value within grub.cfg.  It does not have a way to figure out that these 
> devices
> are the same disk.  This is not a problem with this new driver.
>
> Due to the way GRUB repeatedly opens and closes the same disk. Caching the
> ihandle is important on SPARC.  Without caching, some SAS devices can take
> 15 - 20 minutes to get to the GRUB menu. This ihandle caching is not possible
> without correctly having the canonical disk name.
>
> When available, this driver also tries to use the deblocker #blocks and
> a way of determining the disk size.
>
> Finally and probably most importantly, this new driver is also capable of
> seeing all partitions on a GPT disk.  With the old driver, the GPT
> partition table can not be read and only the first partition on the disk
> can be seen.
>
> Signed-off-by: Eric Snowberg 
> ---
>  grub-core/Makefile.core.def  |1 +
>  grub-core/commands/nativedisk.c  |1 +
>  grub-core/disk/ieee1275/obdisk.c | 1093 
> ++
>  grub-core/kern/ieee1275/cmain.c  |3 +
>  grub-core/kern/ieee1275/init.c   |   12 +-
>  include/grub/disk.h  |1 +
>  include/grub/ieee1275/ieee1275.h |2 +
>  include/grub/ieee1275/obdisk.h   |   25 +
>  8 files changed, 1137 insertions(+), 1 deletions(-)
>  create mode 100644 grub-core/disk/ieee1275/obdisk.c
>  create mode 100644 include/grub/ieee1275/obdisk.h
>
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 2c1d62c..14471c0 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -292,6 +292,7 @@ kernel = {
>sparc64_ieee1275 = kern/sparc64/cache.S;
>sparc64_ieee1275 = kern/sparc64/dl.c;
>sparc64_ieee1275 = kern/sparc64/ieee1275/ieee1275.c;
> +  sparc64_ieee1275 = disk/ieee1275/obdisk.c;
>
>arm = kern/arm/dl.c;
>arm = kern/arm/dl_helper.c;
> diff --git a/grub-core/commands/nativedisk.c b/grub-core/commands/nativedisk.c
> index 2f56a87..2f2315d 100644
> --- a/grub-core/commands/nativedisk.c

Re: Checksummed environments

[Daniel Kiper]

On Tue, Apr 10, 2018 at 11:09:33PM +0200, Daniel Kiper wrote:
> On Fri, Apr 06, 2018 at 03:08:23PM +0200, Kristian Amlie wrote:
> > On 06/04/18 14:35, Daniel Kiper wrote:
> > > On Fri, Apr 06, 2018 at 11:25:22AM +0200, Kristian Amlie wrote:
> > >> Hey, I work for Northern.tech, developing update software for embedded
> > >> Linux devices.
> > >>
> > >> I have a question about GRUB's environment block: This block is not
> > >
> > > I am not sure what exactly you mean by "GRUB's environment block".
> > > Could you send me some references to the code?
> >
> > Of course, sorry if the context wasn't clear. I'm talking about the
> > environment block mentioned here:
> > https://www.gnu.org/software/grub/manual/grub/grub.html#Environment-block,
> > which is used to store information from one boot to the next, using
> > save_env and load_env.
> >
> > >> checksummed, and hence I reckon it can become corrupt if power is lost
> > >> in the middle of a write.
> > >
> > > What about the other blocks?
> >
> > In theory, there is only one block, because it is written in-place,
> > directly on disk, without changing any other filesystem blocks. Is that
> > what you meant by "other blocks"?
> >
> > >> This is an important safety criterion for us, so we've been thinking of
> > >> developing environment block checksumming as an extension to the
> > >> existing save_env and load_env commands. The most likely approach will
> > >> be to grab X amount of bytes at the end of the block and use these for
> > >> the checksum.
> > >
> > > Could you tell us more about that?
> >
> > The idea comes from U-Boot [1], which uses a dedicated block on a
> > storage device to store data, and uses a CRC32 checksum to make sure it
> > is consistent. The motivation is to be able to detect that the block is
> > corrupt, rather than accepting a block of data that may have incorrect
> > data in if a write was interrupted midway by a powerloss. You can read
> > more about it in the link.
> >
> > [2] https://www.denx.de/wiki/DULG/UBootEnvVariables
>
> I am OK with the idea. However, I think that the feature should have
> a kind of switch to turn it off/on. At first sight it looks that new
> environment variable is sufficient for it.

And/Or an argument for save_env/load_env...

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: Checksummed environments

[Daniel Kiper]

On Thu, Apr 12, 2018 at 10:35:06AM +0200, Kristian Amlie wrote:
> On 12/04/18 10:33, Daniel Kiper wrote:
> > On Tue, Apr 10, 2018 at 11:09:33PM +0200, Daniel Kiper wrote:
> >> On Fri, Apr 06, 2018 at 03:08:23PM +0200, Kristian Amlie wrote:
> >>> On 06/04/18 14:35, Daniel Kiper wrote:
> >>>> On Fri, Apr 06, 2018 at 11:25:22AM +0200, Kristian Amlie wrote:
> >>>>> Hey, I work for Northern.tech, developing update software for embedded
> >>>>> Linux devices.
> >>>>>
> >>>>> I have a question about GRUB's environment block: This block is not
> >>>>
> >>>> I am not sure what exactly you mean by "GRUB's environment block".
> >>>> Could you send me some references to the code?
> >>>
> >>> Of course, sorry if the context wasn't clear. I'm talking about the
> >>> environment block mentioned here:
> >>> https://www.gnu.org/software/grub/manual/grub/grub.html#Environment-block,
> >>> which is used to store information from one boot to the next, using
> >>> save_env and load_env.
> >>>
> >>>>> checksummed, and hence I reckon it can become corrupt if power is lost
> >>>>> in the middle of a write.
> >>>>
> >>>> What about the other blocks?
> >>>
> >>> In theory, there is only one block, because it is written in-place,
> >>> directly on disk, without changing any other filesystem blocks. Is that
> >>> what you meant by "other blocks"?
> >>>
> >>>>> This is an important safety criterion for us, so we've been thinking of
> >>>>> developing environment block checksumming as an extension to the
> >>>>> existing save_env and load_env commands. The most likely approach will
> >>>>> be to grab X amount of bytes at the end of the block and use these for
> >>>>> the checksum.
> >>>>
> >>>> Could you tell us more about that?
> >>>
> >>> The idea comes from U-Boot [1], which uses a dedicated block on a
> >>> storage device to store data, and uses a CRC32 checksum to make sure it
> >>> is consistent. The motivation is to be able to detect that the block is
> >>> corrupt, rather than accepting a block of data that may have incorrect
> >>> data in if a write was interrupted midway by a powerloss. You can read
> >>> more about it in the link.
> >>>
> >>> [2] https://www.denx.de/wiki/DULG/UBootEnvVariables
> >>
> >> I am OK with the idea. However, I think that the feature should have
> >> a kind of switch to turn it off/on. At first sight it looks that new
> >> environment variable is sufficient for it.
> >
> > And/Or an argument for save_env/load_env...
>
> Yes, I'm fine with either. How about a variable that determines the
> default, and you can override it with flags?

Works for me. However, I would assume that this feature is by default off.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [GRUB PARTUUID PATCH V9 4/5] Update grub script template files

[Daniel Kiper]

On Wed, Apr 11, 2018 at 07:45:01AM -0700, Nick Vinson wrote:
> On 04/11/2018 01:31 AM, Daniel Kiper wrote:
> > On Tue, Apr 10, 2018 at 08:00:04PM -0700, Nick Vinson wrote:
> >> On 04/10/2018 01:52 PM, Daniel Kiper wrote:
> >>> On Sat, Apr 07, 2018 at 04:28:13PM -0700, Nicholas Vinson wrote:
> >>>> Update grub-mkconfig.in and 10_linux.in to support grub-probe's new
> >>>> partuuid target.  Update grub.texi documentation.  The following table
> >>>> shows how GRUB_DISABLE_LINUX_UUID, GRUB_DISABLE_LINUX_PARTUUID, and
> >>>> initramfs detection interact:
> >>>>
> >>>> Initramfs  GRUB_DISABLE_LINUX_PARTUUID  GRUB_DISABLE_LINUX_UUID  Linux 
> >>>> Root
> >>>> detected   Set  Set  ID 
> >>>> Method
> >>>>
> >>>> False  FalseFalsepart 
> >>>> UUID
> >>>> False  FalseTrue part 
> >>>> UUID
> >>>> False  True Falsedev name
> >>>> False  True True dev name
> >>>> True   FalseFalsefs UUID
> >>>> True   FalseTrue part 
> >>>> UUID
> >>>> True   True Falsefs UUID
> >>>> True   True True dev name
> >>>
> >>> What will happen if GRUB_DISABLE_LINUX_PARTUUID and/or 
> >>> GRUB_DISABLE_LINUX_UUID
> >>> are not set? I think that you can avoid that by setting defaults. You do 
> >>> that
> >>> for GRUB_DISABLE_LINUX_PARTUUID in next patch but GRUB_DISABLE_LINUX_UUID
> >>> does not have any default.
> >>>
> >>
> >> If they're not set, then that's the same as them being set to 'False'.
> >> I should have worded my table above a bit differently and used Yes/No
> >> instead of True/False as that is really what it is trying to convey.
> >
> > IMO it will be more confusing. I think that I would use lowercase
> > false/true as it is used in the script and below the table I would
> > add a note that  == false or something like that.
>
> Ack.  I will update the commit comment.

Thanks. May I ask you to put similar table into docs/grub.texi?

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [GRUB PARTUUID PATCH V10 4/4] Update grub script template files

[Daniel Kiper]

On Mon, Apr 16, 2018 at 10:36:26PM -0700, Nicholas Vinson wrote:
> Update grub-mkconfig.in and 10_linux.in to support grub-probe's new
> partuuid target.  Update grub.texi documentation.  The following table
> shows how GRUB_DISABLE_LINUX_UUID, GRUB_DISABLE_LINUX_PARTUUID, and
> initramfs detection interact:
>
> Initramfs  GRUB_DISABLE_LINUX_PARTUUID  GRUB_DISABLE_LINUX_UUID  Linux Root
> detected   Set  Set  ID Method
>
> false  falsefalsepart UUID
> false  falsetrue part UUID
> false  true falsedev name
> false  true true dev name
> true   falsefalsefs UUID
> true   falsetrue part UUID
> true   true falsefs UUID
> true   true true dev name
>
> Note: GRUB_DISABLE_LINUX_PARTUUID and GRUB_DISABLE_LINUX_UUID equate to
>   'false' when unset or set to any value other than 'true'.
>   GRUB_DISABLE_LINUX_PARTUUID defaults to 'true'.
> Signed-off-by: Nicholas Vinson 
> ---
>  docs/grub.texi  | 67 ++---
>  util/grub-mkconfig.in   |  3 ++
>  util/grub.d/10_linux.in | 22 ++--
>  util/grub.d/20_linux_xen.in | 22 ++--
>  4 files changed, 104 insertions(+), 10 deletions(-)
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index 0f2ab91fc..6aa65552f 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -1214,10 +1214,11 @@ GRUB is configured using @file{grub.cfg}, usually 
> located under
>  need to write the whole thing by hand.
>
>  @menu
> -* Simple configuration::Recommended for most users
> -* Shell-like scripting::For power users and developers
> -* Multi-boot manual config::For non-standard multi-OS scenarios
> -* Embedded configuration::  Embedding a configuration file into GRUB
> +* Simple configuration::Recommended for most users
> +* Root Identifcation Heuristics::   Summary on how the root file system is 
> identified.
> +* Shell-like scripting::For power users and developers
> +* Multi-boot manual config::For non-standard multi-OS scenarios
> +* Embedded configuration::  Embedding a configuration file into GRUB
>  @end menu
>
>
> @@ -1425,6 +1426,17 @@ the Linux kernel, using a @samp{root=UUID=...} kernel 
> parameter.  This is
>  usually more reliable, but in some cases it may not be appropriate.  To
>  disable the use of UUIDs, set this option to @samp{true}.
>
> +@item GRUB_DISABLE_LINUX_PARTUUID
> +If @command{grub-mkconfig} cannot identify the root filesystem via its
> +universally-unique indentifier (UUID), @command{grub-mkconfig} can use the 
> UUID
> +of the partition containing the filesystem to identify the root filesystem to
> +the Linux kernel via a @samp{root=PARTUUID=...} kernel parameter.  This is 
> not
> +as reliable as using the filesystem UUID, but is more reliable than using the
> +Linux device names. When @samp{GRUB_DISABLE_LINUX_PARTUUID} is set to
> +@samp{false}, the Linux kernel version must be 2.6.37 (3.10 for systems using
> +the MSDOS partition scheme) or newer.  This option defaults to @samp{true}.  
> To
> +enable the use of partition UUIDs, set this option to @samp{false}.
> +
>  @item GRUB_DISABLE_RECOVERY
>  If this option is set to @samp{true}, disable the generation of recovery
>  mode menu entries.
> @@ -1556,6 +1568,53 @@ edit the scripts in @file{/etc/grub.d} directly.
>  menu entries; simply type the menu entries you want to add at the end of
>  that file, making sure to leave at least the first two lines intact.
>
> +@node Root Identifcation Heuristics
> +@section Root Identifcation Heuristics
> +If the target operating system uses the Linux kernel, @command{grub-mkconfig}
> +attempts to identify the root file system via a heuristic algoirthm.  This
> +algorithm selects the identification method of the root file system by
> +considering three factors.  The first is if an initrd for the target 
> operating
> +system is also present.  The second is @samp{GRUB_DISABLE_LINUX_UUID} and if 
> set
> +to @samp{true}, prevents @command{grub-mkconfig} from identifying the root 
> file
> +system by its UUID.  The third is @samp{GRUB_DISABLE_LINUX_PARTUUID} and if 
> set
> +to @samp{true}, prevents @command{grub-mkconfig} from identifying the root 
> file
> +system via the UUID of its enclosing partition.  If the variables are 
> assigned
> +any other value, that value is considered equivalent to @samp{false}.  The
> +variables are also considered to be set to @samp{false} if they are not set.
> +
> +When booting, the Linux kernel will delegate the task of mounting the root
> +filesystem to the initrd.  Most initrd i

Re: [PATCH] pass kernel command line as verbatim

[Daniel Kiper]

On Wed, Apr 11, 2018 at 05:17:03PM +0800, Michael Chang wrote:
> And this bug report seems relevant ..
>
> https://savannah.gnu.org/bugs/?49937
>
> On Wed, Apr 11, 2018 at 04:58:54PM +0800, Michael Chang wrote:
> > The command line has been processed by grub shell, then the result is 
> > expected
> > to be passed to kernel command line as verbatim according to the grub manual
> > [1][2].
> >
> > This patch removes extra escape character added as it helps nothing but only
> > creates trouble as you want them to be literal. Besides the surrounding
> > double-quotes added is kept as it used to protect space.

CC-ing Vladmir.

Hmmm... Have you tested this patch on all platforms supported by GRUB2?
I understand that current behavior is not accepted on some but I have
a feeling that somebody make it work in that way due to some reason.
Sadly I cannot find anything about that in git log. So, I have to think
how to cope with that. Or...

Vladmir, do you know why command line is processed in that way?

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] bufio: fix the next_buf calculation

[Daniel Kiper]

CC-ing Vladimir.

On Mon, Apr 16, 2018 at 06:05:04PM +0800, Michael Chang wrote:
> The next_buf is the offset to the next cached block rounded to the size of
> bufio->block_size. However the calculation needs the block_size to be in power
> of 2 is not always valid. As an example, files with smaller size than
> block_size will have the block_size leveled to the size of file which can be
> set arbitrary value.
>
> This patch fixes the next_buf calculation to accept any integers.
>
> Signed-off-by: Michael Chang 
> ---
>  grub-core/io/bufio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/grub-core/io/bufio.c b/grub-core/io/bufio.c
> index 22438277d..d0b0f71b6 100644
> --- a/grub-core/io/bufio.c
> +++ b/grub-core/io/bufio.c
> @@ -132,7 +132,7 @@ grub_bufio_read (grub_file_t file, char *buf, grub_size_t 
> len)
>  return res;
>
>/* Need to read some more.  */
> -  next_buf = (file->offset + res + len - 1) & ~((grub_off_t) 
> bufio->block_size - 1);
> +  next_buf = (grub_divmod64 (file->offset + res + len - 1, 
> bufio->block_size, NULL)) * bufio->block_size;

Should not you fix this in grub_bufio_open()? I think that
you should look for closest power of 2 in it.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [GRUB PARTUUID PATCH V10 4/4] Update grub script template files

[Daniel Kiper]

On Tue, Apr 17, 2018 at 08:20:45AM -0700, Nick Vinson wrote:
> On 04/17/2018 06:36 AM, Daniel Kiper wrote:
> > On Mon, Apr 16, 2018 at 10:36:26PM -0700, Nicholas Vinson wrote:
> >> Update grub-mkconfig.in and 10_linux.in to support grub-probe's new
> >> partuuid target.  Update grub.texi documentation.  The following table
> >> shows how GRUB_DISABLE_LINUX_UUID, GRUB_DISABLE_LINUX_PARTUUID, and
> >> initramfs detection interact:
> >>
> >> Initramfs  GRUB_DISABLE_LINUX_PARTUUID  GRUB_DISABLE_LINUX_UUID  Linux Root
> >> detected   Set  Set  ID Method
> >>
> >> false  falsefalsepart UUID
> >> false  falsetrue part UUID
> >> false  true falsedev name
> >> false  true true dev name
> >> true   falsefalsefs UUID
> >> true   falsetrue part UUID
> >> true   true falsefs UUID
> >> true   true true dev name
> >>
> >> Note: GRUB_DISABLE_LINUX_PARTUUID and GRUB_DISABLE_LINUX_UUID equate to
> >>   'false' when unset or set to any value other than 'true'.
> >>   GRUB_DISABLE_LINUX_PARTUUID defaults to 'true'.
> >> Signed-off-by: Nicholas Vinson 
> >> ---
> >>  docs/grub.texi  | 67 ++---
> >>  util/grub-mkconfig.in   |  3 ++
> >>  util/grub.d/10_linux.in | 22 ++--
> >>  util/grub.d/20_linux_xen.in | 22 ++--
> >>  4 files changed, 104 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/docs/grub.texi b/docs/grub.texi
> >> index 0f2ab91fc..6aa65552f 100644
> >> --- a/docs/grub.texi
> >> +++ b/docs/grub.texi
> >> @@ -1214,10 +1214,11 @@ GRUB is configured using @file{grub.cfg}, usually 
> >> located under
> >>  need to write the whole thing by hand.
> >>
> >>  @menu
> >> -* Simple configuration::Recommended for most users
> >> -* Shell-like scripting::For power users and developers
> >> -* Multi-boot manual config::For non-standard multi-OS scenarios
> >> -* Embedded configuration::  Embedding a configuration file into GRUB
> >> +* Simple configuration::Recommended for most users
> >> +* Root Identifcation Heuristics::   Summary on how the root file system 
> >> is identified.
> >> +* Shell-like scripting::For power users and developers
> >> +* Multi-boot manual config::For non-standard multi-OS scenarios
> >> +* Embedded configuration::  Embedding a configuration file into 
> >> GRUB
> >>  @end menu
> >>
> >>
> >> @@ -1425,6 +1426,17 @@ the Linux kernel, using a @samp{root=UUID=...} 
> >> kernel parameter.  This is
> >>  usually more reliable, but in some cases it may not be appropriate.  To
> >>  disable the use of UUIDs, set this option to @samp{true}.
> >>
> >> +@item GRUB_DISABLE_LINUX_PARTUUID
> >> +If @command{grub-mkconfig} cannot identify the root filesystem via its
> >> +universally-unique indentifier (UUID), @command{grub-mkconfig} can use 
> >> the UUID
> >> +of the partition containing the filesystem to identify the root 
> >> filesystem to
> >> +the Linux kernel via a @samp{root=PARTUUID=...} kernel parameter.  This 
> >> is not
> >> +as reliable as using the filesystem UUID, but is more reliable than using 
> >> the
> >> +Linux device names. When @samp{GRUB_DISABLE_LINUX_PARTUUID} is set to
> >> +@samp{false}, the Linux kernel version must be 2.6.37 (3.10 for systems 
> >> using
> >> +the MSDOS partition scheme) or newer.  This option defaults to 
> >> @samp{true}.  To
> >> +enable the use of partition UUIDs, set this option to @samp{false}.
> >> +
> >>  @item GRUB_DISABLE_RECOVERY
> >>  If this option is set to @samp{true}, disable the generation of recovery
> >>  mode menu entries.
> >> @@ -1556,6 +1568,53 @@ edit the scripts in @file{/etc/grub.d} directly.
> >>  menu entries; simply type the menu entries you want to add at the end of
> >>  that file, making sure to leave at least the first two lines intac

Re: [PATCH v3] grub-install: locale depends on nls

[Daniel Kiper]

On Fri, Apr 13, 2018 at 11:36:49PM +0200, Olaf Hering wrote:
> With --disable-nls no locales exist.
>
> Avoid runtime error by moving code that copies locales into its own
> function. Return early in case nls was disabled. That way the compiler
> will throw away unreachable code, no need to put preprocessor
> conditionals everywhere to avoid warnings about unused code.
>
> Fix memleak by freeing dstf.

s/dstf/srcf and dstf/?

> Convert tabs to spaces in moved code.
>
> Signed-off-by: Olaf Hering 

Otherwise LGTM. If there are no objections I will commit
this patch in a week or so.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH RFC] Proposition of a --auto-nvram option for grub-install

[Daniel Kiper]

On Thu, Apr 12, 2018 at 07:09:58PM +0200, Lukasz Zemczak wrote:
> Hello everyone,
>
> I'm writing to this list since I would like to get some feedback on an
> additional option to the grub-install tool we would find very
> convenient to have. The diff is attached to the e-mail (also available
> as a pastebin [1]).
>
> The idea of the --auto-nvram (the name is just a proposition) flag is
> a bit similar to what --no-nvram does. After providing the option
> during grub-install, the tool will attempt to guess if there is access
> to NVRAM variables for EFI and/or IEEE1275 and, if yes, perform the
> usual variable updates. If no access to the NVRAM is available the
> whole thing is handled somewhat similar to --no-nvram + a warning

Make sense for me but I am not sure about "a warning". Hmmm...
Is it really needed?

> message displayed. Rationale:
>
> We would like to use this in Ubuntu for cases of dual BIOS/EFI
> bootloaders installed (at the same time), helpful for the situation of
> calling grub-install --target=x86_64-efi from the shim-efi package on
> a BIOS legacy-mode booted machine. For this legacy-mode case when
> running on a EFI-enabled device, currently this causes grub-install to
> fail as obviously there is no access to the NVRAM and no --no-nvram is
> given. We don't want to unconditionally append --no-nvram as this is
> not what we want to happen for the case of a system that is actually
> booted in EFI-mode. With this flag, we would be simply performing a
> grub-install --target=x86_64-efi --auto-nvram unconditionally which
> would do the right thing in both cases, allowing for a much simpler
> handling of this dual-bootloader case in Ubuntu. Having it being done
> inside grub-installer makes everything much cleaner.
>
> This is of course just a proposition about which I wanted to get some
> feedback from people that know the codebase the most. It's my first
> time working on the grub project so apologies for any flukes or
> silliness in the code or the idea itself.
>
> Thank you!
>
> Best regards,
>
> [1] http://paste.ubuntu.com/p/cWR3k3NZgF/

Next time please use git format-patch/send-email to send the patches.

> diff --git a/grub-core/osdep/basic/no_platform.c 
> b/grub-core/osdep/basic/no_platform.c
> index d76c34c14..b39e97f48 100644
> --- a/grub-core/osdep/basic/no_platform.c
> +++ b/grub-core/osdep/basic/no_platform.c
> @@ -25,7 +25,7 @@
>
>  void
>  grub_install_register_ieee1275 (int is_prep, const char *install_device,
> - int partno, const char *relpath)
> + int partno, const char *relpath, int 
> detect_nvram)
>  {
>grub_util_error ("%s", _("no IEEE1275 routines are available for your 
> platform"));
>  }
> @@ -33,7 +33,8 @@ grub_install_register_ieee1275 (int is_prep, const char 
> *install_device,
>  void
>  grub_install_register_efi (grub_device_t efidir_grub_dev,
>  const char *efifile_path,
> -const char *efi_distributor)
> +const char *efi_distributor,
> +int detect_nvram)
>  {
>grub_util_error ("%s", _("no EFI routines are available for your 
> platform"));
>  }
> diff --git a/grub-core/osdep/unix/platform.c b/grub-core/osdep/unix/platform.c
> index ca448bc11..4eb2c11c9 100644
> --- a/grub-core/osdep/unix/platform.c
> +++ b/grub-core/osdep/unix/platform.c
> @@ -134,7 +134,8 @@ grub_install_remove_efi_entries_by_distributor (const 
> char *efi_distributor)
>  int
>  grub_install_register_efi (grub_device_t efidir_grub_dev,
>  const char *efifile_path,
> -const char *efi_distributor)
> +const char *efi_distributor,
> +int detect_nvram)
>  {
>const char * efidir_disk;
>int efidir_part;
> @@ -153,6 +154,21 @@ grub_install_register_efi (grub_device_t efidir_grub_dev,
>  #ifdef __linux__
>grub_util_exec ((const char * []){ "modprobe", "-q", "efivars", NULL });
>  #endif
> +
> +  /* If requested, we try to detect if NVRAM access is available and if not,
> + warn the user and resume normal operation.  */
> +  if (detect_nvram)
> +{
> +  error = grub_util_exec_redirect_null ((const char * []){ "efibootmgr", 
> NULL });
> +  if (error == 2)
> + {
> +   grub_util_warn ("%s", _("Auto-NVRAM selected and no EFI variable 
> support detected on the system."));

Wait, I have a feeling that you should fail hard here. In general
I think that if somebody passed --auto-nvram on platforms with NVRAM
and something fails then everything should fail. If somebody passed
--auto-nvram on platforms without NVRAM then any attempt to access
NVRAM should be skipped (silently?). Does it make sense?

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: Multiboot ELF segment handling patch

[Daniel Kiper]

Hi Alexander,

On Sat, Apr 14, 2018 at 12:07:29AM +0200, Alexander Boettcher wrote:
> Hello,
>
> On 06.04.2018 14:28, Daniel Kiper wrote:
> > On Thu, Mar 29, 2018 at 11:20:37AM +0200, Alexander Boettcher wrote:
> >
> >> Can you please have a look and check regarding what should/could be
> >> changed to get it upstream? We did not test the dynamic relocation part,
> >
> > Sure thing, please take a look below...
> >
> >> since we have no such (kernel) setup. Thanks in advance.
> >
> > You can use Xen (git://xenbits.xen.org/xen.git) for tests.
> > Just compile hypervisor without any tools and use xen.gz.
> > It produces nice output and you can see it is relocated or not.
> > Of course you have to use Multiboot2 protocol.
>
> Thanks, I managed to setup it. Based on your comments and on the Xen
> test case I had to re-work the patch, so that it now works for
> relocation and non-relocation kernels.
>
> Please review again.
> > However, this does not mean that I will not take this patch. Though
> > it requires some tweaking.
> >
> > First of all, lack of SOB.
>
> What is a SOB ?

Signed-off-by: Alexander Boettcher 

Next time please use git format-patch/send-email to send the patches.

> From c37727840be8aeb81ea4bbc95ac09a1b1e3d3658 Mon Sep 17 00:00:00 2001
> From: Alexander Boettcher 
> Date: Fri, 13 Apr 2018 23:37:01 +0200
> Subject: [PATCH] mbi: use per segment a separate relocator chunk
>
> if elf is non relocatable.
>
> If the segments are not dense packed, the original code set up a huge
> relocator chunk comprising all segments.
>
> During the final relocation in grub_relocator_prepare_relocs, the chunk
> is moved to its desired place and overrides memory which are actually not
> covered/touched by the specified segments.
>
> 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?

> ---
>  grub-core/loader/multiboot_elfxx.c | 56 
> --
>  1 file changed, 35 insertions(+), 21 deletions(-)
>
> diff --git a/grub-core/loader/multiboot_elfxx.c 
> b/grub-core/loader/multiboot_elfxx.c
> index 67daf59..d936223 100644
> --- a/grub-core/loader/multiboot_elfxx.c
> +++ b/grub-core/loader/multiboot_elfxx.c
> @@ -57,9 +57,9 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
>char *phdr_base;
>grub_err_t err;
>grub_relocator_chunk_t ch;
> -  grub_uint32_t load_offset, load_size;
> +  grub_uint32_t load_offset = 0, load_size;
>int i;
> -  void *source;
> +  void *source = NULL;
>
>if (ehdr->e_ident[EI_MAG0] != ELFMAG0
>|| ehdr->e_ident[EI_MAG1] != ELFMAG1
> @@ -83,6 +83,7 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
>  #define phdr(i)  ((Elf_Phdr *) (phdr_base + (i) * 
> ehdr->e_phentsize))
>
>mld->link_base_addr = ~0;
> +  mld->load_base_addr = ~0;
>
>/* Calculate lowest and highest load address.  */
>for (i = 0; i < ehdr->e_phnum; i++)
> @@ -108,27 +109,19 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t 
> *mld)
> mld->min_addr, mld->max_addr - 
> load_size,
> load_size, mld->align ? 
> mld->align : 1,
> mld->preference, 
> mld->avoid_efi_boot_services);
> -}
> -  else
> -err = grub_relocator_alloc_chunk_addr (GRUB_MULTIBOOT (relocator), &ch,
> -mld->link_base_addr, load_size);
>
> -  if (err)
> -{
> -  grub_dprintf ("multiboot_loader", "Cannot allocate memory for OS 
> image\n");
> -  return err;
> -}
> +  if (err)
> +{
> +  grub_dprintf ("multiboot_loader", "Cannot allocate memory for OS 
> image\n");
> +  return err;
> +}
>
> -  mld->load_base_addr = get_physical_target_address (ch);
> -  source = get_virtual_current_address (ch);
> +  mld->load_base_addr = get_physical_target_address (ch);
> +  source = get_virtual_current_address (ch);
>
> -  grub_dprintf ("multiboot_loader", "link_base_addr=0x%x, 
> load_base_addr=0x%x, "
> - "load_size=0x%x, relocatable=%d\n", mld->link_base_addr,
> - mld->load_base_addr, load_size, mld->relocatable);

I would not drop it completely from ~here. I think that you can display
link_base_addr and relocatable just before current line 10

Re: [PATCH] bufio: fix the next_buf calculation

[Daniel Kiper]

On Wed, Apr 18, 2018 at 03:28:20PM +0800, Michael Chang wrote:
> On Tue, Apr 17, 2018 at 07:11:34PM +0200, Daniel Kiper wrote:
> > CC-ing Vladimir.
> >
> > On Mon, Apr 16, 2018 at 06:05:04PM +0800, Michael Chang wrote:
> > > The next_buf is the offset to the next cached block rounded to the size of
> > > bufio->block_size. However the calculation needs the block_size to be in 
> > > power
> > > of 2 is not always valid. As an example, files with smaller size than
> > > block_size will have the block_size leveled to the size of file which can 
> > > be
> > > set arbitrary value.
> > >
> > > This patch fixes the next_buf calculation to accept any integers.
> > >
> > > Signed-off-by: Michael Chang 
> > > ---
> > >  grub-core/io/bufio.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/grub-core/io/bufio.c b/grub-core/io/bufio.c
> > > index 22438277d..d0b0f71b6 100644
> > > --- a/grub-core/io/bufio.c
> > > +++ b/grub-core/io/bufio.c
> > > @@ -132,7 +132,7 @@ grub_bufio_read (grub_file_t file, char *buf, 
> > > grub_size_t len)
> > >  return res;
> > >
> > >/* Need to read some more.  */
> > > -  next_buf = (file->offset + res + len - 1) & ~((grub_off_t) 
> > > bufio->block_size - 1);
> > > +  next_buf = (grub_divmod64 (file->offset + res + len - 1, 
> > > bufio->block_size, NULL)) * bufio->block_size;
> >
> > Should not you fix this in grub_bufio_open()? I think that
> > you should look for closest power of 2 in it.
>
> Of course here we can round up or down the bufio->block_size to meet power of
> 2. The down side of round-down is inefficient for small files as it can't 
> cache
> entire file in one go. The round-up will have to allocate (slightly) larger
> than needed buffer to hold small files which is not a big deal ...
>
> If you insist, I can work a new patch to round up the block_size in
> grub_bufio_open, just let me know.

Please do.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [RFC] Add support for BTRFS raid5/6 to GRUB

[Daniel Kiper]

On Tue, Apr 17, 2018 at 09:57:40PM +0200, Goffredo Baroncelli wrote:
> Hi All,
>
> Below you can find a patch to add support for accessing files from
> grub in a RAID5/6 btrfs filesystem. This is a RFC because it is
> missing the support for recovery (i.e. if some devices are missed). In
> the next days (weeks ?) I will extend this patch to support also this
> case.
>
> Comments are welcome.

More or less LGTM. Just a nitpick below... I am happy to take full blown
patch into GRUB if it is ready.

> BR
> G.Baroncelli
>
>
> ---
>
> commit 8c80a1b7c913faf50f95c5c76b4666ed17685666
> Author: Goffredo Baroncelli 
> Date:   Tue Apr 17 21:40:31 2018 +0200
>
> Add initial support for btrfs raid5/6 chunk
>
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index be195448d..4c5632acb 100644
> --- a/grub-core/fs/btrfs.c
> +++ b/grub-core/fs/btrfs.c
> @@ -119,6 +119,8 @@ struct grub_btrfs_chunk_item
>  #define GRUB_BTRFS_CHUNK_TYPE_RAID1 0x10
>  #define GRUB_BTRFS_CHUNK_TYPE_DUPLICATED0x20
>  #define GRUB_BTRFS_CHUNK_TYPE_RAID100x40
> +#define GRUB_BTRFS_CHUNK_TYPE_RAID5 0x80
> +#define GRUB_BTRFS_CHUNK_TYPE_RAID60x100
>grub_uint8_t dummy2[0xc];
>grub_uint16_t nstripes;
>grub_uint16_t nsubstripes;
> @@ -764,6 +766,39 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, 
> grub_disk_addr_t addr,
> stripe_offset = low + chunk_stripe_length
>   * high;
> csize = chunk_stripe_length - low;
> +   break;
> + }
> +   case GRUB_BTRFS_CHUNK_TYPE_RAID5:
> +   case GRUB_BTRFS_CHUNK_TYPE_RAID6:
> + {
> +   grub_uint64_t nparities;
> +   grub_uint64_t parity_pos;
> +   grub_uint64_t stripe_nr, high;
> +   grub_uint64_t low;
> +
> +   redundancy = 1;   /* no redundancy for now */
> +
> +   if (grub_le_to_cpu64 (chunk->type) & GRUB_BTRFS_CHUNK_TYPE_RAID5)
> + {
> +   grub_dprintf ("btrfs", "RAID5\n");
> +   nparities = 1;
> + }
> +   else
> + {
> +   grub_dprintf ("btrfs", "RAID6\n");
> +   nparities = 2;
> + }
> +
> +   stripe_nr = grub_divmod64 (off, chunk_stripe_length, &low);
> +
> +   high = grub_divmod64 (stripe_nr, nstripes - nparities, &stripen);
> +   grub_divmod64 (high+nstripes-nparities, nstripes, &parity_pos);
> +   grub_divmod64 (parity_pos+nparities+stripen, nstripes, &stripen);

Missing spaces around "+" and "-".

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH RFC] Proposition of a --auto-nvram option for grub-install

[Daniel Kiper]

Hey Lukasz,

On Thu, Apr 19, 2018 at 11:29:28AM +0200, Lukasz Zemczak wrote:
> Hey Daniel!
>
> Thank you for your feedback! I can drop the warning message, sure, I
> added it only for verbosity anyway so there's no real reason for
> keeping it around.

Thanks!

> As for your second comment: generally this is what I want to achieve
> by checking the return value of the efibootmgr command. Whenever
> efibootmgr exits with an error code of 2 this basically means "EFI
> variables are not supported on this system." - i.e. that the system
> we're running on has no NVRAM (at least in efibootmgr's mind). In all
> other cases, if there is any other error returned by efiboomgr, fail
> the grub_install_register_efi() as we did before. Therefore if
> something fails on a platform with NVRAM, we fail hard as previously.

What will happen if efibootmgr is not installed? I think that grub-install
should do nothing silently if --auto-nvram is passed. And should not you
check if /sys/firmware/efi exits instead of blindly trying efibootmgr?
Additionally, please do not forget to update docs: man, grub.texi, --help
for grub-install, etc.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] bufio: round up block size to power of 2

[Daniel Kiper]

On Fri, Apr 20, 2018 at 03:21:30PM +0800, Michael Chang wrote:
> Rounding up the bufio->block_size to meet power of 2 to facilitate next_buf
> calcuation in grub_bufio_read.
>
> Signed-off-by: Michael Chang 
> ---
>  grub-core/io/bufio.c | 8 
>  1 file changed, 8 insertions(+)
>
> diff --git a/grub-core/io/bufio.c b/grub-core/io/bufio.c
> index 22438277d..4d66b65a3 100644
> --- a/grub-core/io/bufio.c
> +++ b/grub-core/io/bufio.c
> @@ -61,6 +61,14 @@ grub_bufio_open (grub_file_t io, int size)
>  size = ((io->size > GRUB_BUFIO_MAX_SIZE) ? GRUB_BUFIO_MAX_SIZE :
>  io->size);
>
> +  /* round up size to power of 2 */

May I ask you to say why here?

> +  if (size & (size - 1))
> +{
> +  int round_up;
> +  for (round_up = 1; round_up < size; round_up <<= 1);
> +  size = round_up;
> +}

Please use this (stolen from linux/drivers/md/raid5.c):
  while (size & (size - 1))
size = (size | (size - 1)) + 1;

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2] bufio: round up block size to power of 2

[Daniel Kiper]

On Tue, Apr 24, 2018 at 02:13:04PM +0800, Michael Chang wrote:
> Rounding up the bufio->block_size to meet power of 2 to facilitate next_buf
> calcuation in grub_bufio_read.
>
> Signed-off-by: Michael Chang 
>
> v2:

Next time please put "---" before "v2:" just after SOB.
This way git am will not pull v2 changes description into
commit message. Otherwise it looks good to me. If there
are no objections I will commit this in a week or so.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] pass kernel command line as verbatim

[Daniel Kiper]

On Wed, Apr 18, 2018 at 03:07:15PM +0800, Michael Chang wrote:
> On Tue, Apr 17, 2018 at 06:37:06PM +0200, Daniel Kiper wrote:
> > On Wed, Apr 11, 2018 at 05:17:03PM +0800, Michael Chang wrote:
> > > And this bug report seems relevant ..
> > >
> > > https://savannah.gnu.org/bugs/?49937
> > >
> > > On Wed, Apr 11, 2018 at 04:58:54PM +0800, Michael Chang wrote:
> > > > The command line has been processed by grub shell, then the result is 
> > > > expected
> > > > to be passed to kernel command line as verbatim according to the grub 
> > > > manual
> > > > [1][2].
> > > >
> > > > This patch removes extra escape character added as it helps nothing but 
> > > > only
> > > > creates trouble as you want them to be literal. Besides the surrounding
> > > > double-quotes added is kept as it used to protect space.
> >
> > CC-ing Vladmir.
> >
> > Hmmm... Have you tested this patch on all platforms supported by GRUB2?
>
> AFAICS, it affects boot protocols of which grub supports in commands like
> linux, multiboot, xen_hypervisor and so on. Non of them will intepret escape
> sequence and are pretty straightforward in composition -- In general, use 
> space
> to delimit options and quote the options by double quotes once they contain 
> any
> space.
>
> > I understand that current behavior is not accepted on some but I have
> > a feeling that somebody make it work in that way due to some reason.
> > Sadly I cannot find anything about that in git log. So, I have to think
> > how to cope with that. Or...
>
> As a side note, we had a down stream bug report that udev generates symlink
> names containing '\' character, and with the current grub command processing
> it's never possible to use those symlinks as kernel command line option, as 
> the
> character is either stripped or doubled.

I understand your problem but I am afraid about compatibility. I would
agree if you change the behavior here and introduce a shell variable to
disable that change. If nobody will complain for some time then we can
drop that variable a leave new desired behavior. Does it make sense?

And I have a question about space (what about tab?): why it is quoted
if, AIUI, kernel command line should not be processed at this point?
Should not it be created earlier properly? I mean with required quotes.

Hmmm... What about modules command lines?

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2] Makefile.am: Add `all_video` to `default_payload.elf`

[Daniel Kiper]

On Mon, Apr 23, 2018 at 09:45:02PM +0200, Paul Menzel wrote:
> Dear GRUB folks,
>
>
> Am Mittwoch, den 21.03.2018, 09:28 +0100 schrieb Paul Menzel:
> > From a199bc1f64e33aa942b23fe6d16670cc6002bb6c Mon Sep 17 00:00:00 2001
> > From: Paul Menzel 
> > Date: Sun, 16 Apr 2017 21:02:58 +0200
> > Subject: [PATCH] Makefile.am: Add `all_video` to default_payload.elf
> >
> > The module `all_video` is used in `util/grub.d/00_header.in`, and
> > included for grub-pc by default.
> >
> > To make it easier to load such a GRUB configuration from disk with a GRUB
> > coreboot payload, add this module also to `default_payload.elf` by
> > default. That avoids a missing module error by GRUB, forcing the user to
> > hit enter during the start process.
> > ---
> > v2: Resend and fix typo in commit message.
> >
> >  Makefile.am | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/Makefile.am b/Makefile.am
> > index 7795baeb6..80a787e57 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -424,7 +424,7 @@ bootcheck: $(BOOTCHECKS)
> >  if COND_i386_coreboot
> >  default_payload.elf: grub-mkstandalone grub-mkimage FORCE
> > test -f $@ && rm $@ || true
> > -   pkgdatadir=. ./grub-mkstandalone --grub-mkimage=./grub-mkimage -O 
> > i386-coreboot -o $@ --modules='ahci pata ehci uhci ohci usb_keyboard usbms 
> > part_msdos ext2 fat at_keyboard part_gpt usbserial_usbdebug cbfs' 
> > --install-modules='ls linux search configfile normal cbtime cbls memrw iorw 
> > minicmd lsmmap lspci halt reboot hexdump pcidump regexp setpci lsacpi chain 
> > test serial multiboot cbmemc linux16 gzio echo help syslinuxcfg xnu $(shell 
> > cat grub-core/fs.lst) password_pbkdf2 $(EXTRA_PAYLOAD_MODULES)' --fonts= 
> > --themes= --locales= -d grub-core/ 
> > /boot/grub/grub.cfg=$(srcdir)/coreboot.cfg
> > +   pkgdatadir=. ./grub-mkstandalone --grub-mkimage=./grub-mkimage -O 
> > i386-coreboot -o $@ --modules='ahci pata ehci uhci ohci usb_keyboard usbms 
> > part_msdos ext2 fat at_keyboard part_gpt usbserial_usbdebug cbfs' 
> > --install-modules='ls linux search configfile normal cbtime cbls memrw iorw 
> > minicmd lsmmap lspci halt reboot hexdump pcidump regexp setpci lsacpi chain 
> > test serial multiboot cbmemc linux16 gzio echo help syslinuxcfg xnu 
> > all_video $(shell cat grub-core/fs.lst) password_pbkdf2 
> > $(EXTRA_PAYLOAD_MODULES)' --fonts= --themes= --locales= -d grub-core/ 
> > /boot/grub/grub.cfg=$(srcdir)/coreboot.cfg
> >  endif
> >
> >  endif
>
> Can you please commit this to the master branch?

LGTM. Could you rebase and repost it?

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: Multiboot ELF segment handling patch

[Daniel Kiper]

On Mon, Apr 23, 2018 at 08:26:34PM +0200, Alexander Boettcher wrote:
> Hello,
>
> 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.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: What is this grub_disk_read doing in the i386 linux loader?

[Daniel Kiper]

On Thu, Apr 19, 2018 at 03:22:55PM -0700, Andrew Jeddeloh wrote:
> While solving a bug in the coreos fork of grub I came across this disk
> read in the i386 linux loader [1]. It looks like its reading whatever
> is after the boot param header in the kernel file (defined by the
> linux x86 boot protocol [2]) into the rest of the `linux_params`
> struct. In practice this means overwriting part of the padding and the
> e820 map. As far as I can tell, this is not necessary or a useful
> thing to do. Am I missing something?
>
> The bug we were hitting on our fork was miscalculating
> (char*)&linux_params + sizeoh(lh) as &linux_params + sizeof(lh), which
> (in addition to corrupting memory) means the contents wasn't being
> written to (char*)&linux_params + sizeof(lh). However the machines
> seem to boot just fine when the memory corruption didn't cause
> problems. If I nop out the call to read that chunk into
> (char*)linux_params + sizeof(lh) it also seems to boot fine.
>
> Is this intended? If so what is it doing? It dates back to the
> original i386 linux loader support [3], but I can't figure out why
> this would be intended.

This is intended. Look at [2], 32-bit BOOT PROTOCOL paragraph. However,
at least since commit 2001169 math is wrong. I think that len should be
calculated according to Linux boot protocol spec. The destination for
read should be &linux_params.setup_sects.

May I ask you to provide relevant patch for upstream?

Daniel

[1] 
http://git.savannah.gnu.org/cgit/grub.git/tree/grub-core/loader/i386/linux.c#n809
[2] https://www.kernel.org/doc/Documentation/x86/boot.txt
[3] 
http://git.savannah.gnu.org/cgit/grub.git/commit/grub-core/loader/i386/linux.c?id=8c411768822a75c8c15108872191a05e84befa6e

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: What is this grub_disk_read doing in the i386 linux loader?

[Daniel Kiper]

On Tue, Apr 24, 2018 at 04:08:57PM -0700, Andrew Jeddeloh wrote:
> Thanks for the reply.
>
> I'm not sure I follow. Looking over the 32 bit boot spec, it looks
> like the process is:
>
> 1) zero out linux_params
>  - grub does this
> 2) copy the linux boot params (from 0x1f1) into linux params
>  - grub does this by reading from 0x0 until the end of lh, then
> copying lh+0x1f1 til the end of lh into linux_headers [4][5]
> 3) Do the read/write/modify operations that would happen for a 16 bit boot
> 4) calculate the end of the the setup header
>  - grub does not do this and I think assumes its just the end of the
> linux_params struct

As Michael said, "0x0202 + byte value at offset 0x0201" is your friend.

> 5) fill out the rest of the setup header things from the zero page doc [6]
>  - AFAICT this isn't meant to be read from the kernel image, but
> instead filled out by bootloader or left as zero

AIUI it is. Please look above.

>- It looks like the current code is trying to do this but is just
> assuming the end is at the end of linux_params.

This is wrong due to grub_e820_mmap.

>  - Looking over the items in the zero page doc, there doesn't seem to
> be anything that the kernel could supply a useful "default" value.

GRUB have to load the rest of Linux header into linux_params.
Current math is wrong and have to be fixed. Please look above
and think about newer versions of Linux headers which GRUB is
not aware of.

> As I understand it, the read is not harmful (as it just gets
> overwritten by the bootloader later) but also not needed. Is this your
> understanding?

This read is needed, however, math has to be fixed. Additionally,
I think that you can take a look at SYSLINUX and do something
similar in GRUB.

> I'll definitely submit a patch upstream once I figure out what should
> be going on.

Great! Thanks!

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: What is this grub_disk_read doing in the i386 linux loader?

[Daniel Kiper]

On Wed, Apr 25, 2018 at 12:45:15PM -0700, Andrew Jeddeloh wrote:
> > As Michael said, "0x0202 + byte value at offset 0x0201" is your friend.
>
> I think Michael's message got lost; I think he replied to just you? I
> didn't receive it and I don't see it in the archives.
>
> > AIUI it is. Please look above.
>
> I don't know what you're refering to here by "above".

I am referring to the math above and in general to 32-bit BOOT PROTOCOL
paragraph from https://www.kernel.org/doc/Documentation/x86/boot.txt

> > This is wrong due to grub_e820_mmap.
>
> My understanding is wrong or grub's behavior is wrong? Can you elaborate 
> please?

GRUB behavior is wrong due to grub_e820_mmap.

> > This read is needed
>
> I don't understand why. Again, looking at the zero page documentation,
> there's nothing after the 16 bit boot params that the linux image

I am not sure what do you mean by "16 bit boot params". Anyway, again 32-bit
BOOT PROTOCOL paragraph is pretty clear: ... Then the setup header from
offset 0x01f1 of kernel image on should be loaded into struct boot_params
and examined. The end of setup header can be calculated as follow:
0x0202 + byte value at offset 0x0201 ...

> could know ahead of time. Is your concern that future fields could be
> added that do need to be read?

Exactly.

> Regardless, we can't calculate then length then blindly use it reading
> into a struct. If the length grows larger than the size of the struct
> we'll start corrupting memory when doing the read. Assuming the read

Yep, the end of linux_params.pad2 is the limit. We should not go
over it. So, just simply check that we are not going to read more
than that. Otherwise fail and print an error message.

> is needed, we'll need to malloc the amount that still needs to be read
> then probably copy that into a struct to modify (and ignore the bits
> we don't understand in the case of a new Linux release adding fields
> grub doesn't have yet).
>
> Sorry for all the questions, I want to ensure that I understand what's

No problem.

> supposed to be happening first.

That is good way to go.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: grub-mkconfig does not fsync() or sync() - can result in boot failure

[Daniel Kiper]

On Tue, Apr 24, 2018 at 08:48:05PM -0600, Chris Murphy wrote:
> Hi,
>
> I've done a 'UB's docs then it would be perfect. -ff -o stracegrub.out 
> grub-mkconfig' and there is no
> fsync() or sync() at all, so this does not seem crash safe for either
> non-journaled or journaled file systems.
>
> The most typical result is the grub.cfg is stale, the grub.cfg.new is
> truncated. This isn't in a state a lot of users will even understand, it's
> going to be a silent failure by booting an older kernel. And many distros
> hide the menu by default so a user would not know unless they go looking
> for a problem.
>
> Note that fsync() is not adequate for journaled file systems, changes might
> only be in the journal which GRUB does not read, so it can still see stale
> data until kernel code does log replay and fixes up the file system for
> GRUB to read at the next reboot.
>
> And there's a further adorable aspect of XFS where sync() behaves the same
> way, only log replay makes the file system consistent. ext4 and Btrfs
> appear to flush everything on sync().
>
> XFS devs have insisted this is correct behavior, that the file system
> metadata is all on disk, and it's not their problem GRUB can't do log
> replay, if GRUB requires the log flushed, then it needs to call
> FIFREEZE/FITHAW. No joke.
>
> Hence I filed this bug a few months ago.
> https://savannah.gnu.org/bugs/index.php?52657
>
> Anyway, in my limited testing, just adding sync on the 3rd to last line of
> grub-mkconfig, right above
>
> gettext "done" >&2
>
> fixes this problem on all file systems except XFS, which is a lot better
> than nothing. I really don't think it's ok for grub-mkconfig to assume
> something else is going to properly sync, unmount, or remount-ro whatever
> file system the grub.cfg is being written to. grub-mkconfig needs to take
> responsibility for its own action, and ensure complete and full commit of
> its changes before exit.

May I ask you to post a patch here? If you could add this nice description
into GRUB's docs then it would be perfect.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] pass kernel command line as verbatim

[Daniel Kiper]

On Thu, Apr 26, 2018 at 04:42:30PM +0800, Michael Chang wrote:
> On Tue, Apr 24, 2018 at 12:03:21PM +0200, Daniel Kiper wrote:
> > On Wed, Apr 18, 2018 at 03:07:15PM +0800, Michael Chang wrote:
> > > On Tue, Apr 17, 2018 at 06:37:06PM +0200, Daniel Kiper wrote:
> > > > On Wed, Apr 11, 2018 at 05:17:03PM +0800, Michael Chang wrote:
> > > > > And this bug report seems relevant ..
> > > > >
> > > > > https://savannah.gnu.org/bugs/?49937
> > > > >
> > > > > On Wed, Apr 11, 2018 at 04:58:54PM +0800, Michael Chang wrote:
> > > > > > The command line has been processed by grub shell, then the result 
> > > > > > is expected
> > > > > > to be passed to kernel command line as verbatim according to the 
> > > > > > grub manual
> > > > > > [1][2].
> > > > > >
> > > > > > This patch removes extra escape character added as it helps nothing 
> > > > > > but only
> > > > > > creates trouble as you want them to be literal. Besides the 
> > > > > > surrounding
> > > > > > double-quotes added is kept as it used to protect space.
> > > >
> > > > CC-ing Vladmir.
> > > >
> > > > Hmmm... Have you tested this patch on all platforms supported by GRUB2?
> > >
> > > AFAICS, it affects boot protocols of which grub supports in commands like
> > > linux, multiboot, xen_hypervisor and so on. Non of them will intepret 
> > > escape
> > > sequence and are pretty straightforward in composition -- In general, use 
> > > space
> > > to delimit options and quote the options by double quotes once they 
> > > contain any
> > > space.
> > >
> > > > I understand that current behavior is not accepted on some but I have
> > > > a feeling that somebody make it work in that way due to some reason.
> > > > Sadly I cannot find anything about that in git log. So, I have to think
> > > > how to cope with that. Or...
> > >
> > > As a side note, we had a down stream bug report that udev generates 
> > > symlink
> > > names containing '\' character, and with the current grub command 
> > > processing
> > > it's never possible to use those symlinks as kernel command line option, 
> > > as the
> > > character is either stripped or doubled.
> >
> > I understand your problem but I am afraid about compatibility. I would
> > agree if you change the behavior here and introduce a shell variable to
> > disable that change. If nobody will complain for some time then we can
> > drop that variable a leave new desired behavior. Does it make sense?
>
> Yes, it makes sense. But here I have another idea that can be helpful if we
> want to have one grub.cfg working on all versions. With the shell variable is
> not possible as the result is either breaking in one way or not.
>
> It borrows the same idea of using feature variable to determine what current
> build can support at run time, and thus allow to use another set of settings
> like GRUB_CMDLINE_LINUX_V2 to override cmdline for the new sytax. It is fine 
> to
> not specify GRUB_CMDLINE_LINUX_V2, in that we will use GRUB_CMDLINE_LINUX.

I prefer something which is common for all boot protocols instead of
something specific for a given one, in this case Linux one.

> > And I have a question about space (what about tab?): why it is quoted
> > if, AIUI, kernel command line should not be processed at this point?
> > Should not it be created earlier properly? I mean with required quotes.
>
> The command line arguments will be processed by grub shell before passing them
> to kernel command line. They will evaluate quotes (among other things) and
> extract string from them. As a helper, the result can be observed by "echo"
> command.
>
>   grub> echo abc="foo bar"
>   abc=foo bar

Sure thing. So, I think that you have to escape all special characters
for GRUB shell before putting a given command line into grub.cfg.
I understand that it can be difficult in current shell based config
generator but you should try...

> As you can see, we have to add the quotes back here then it can be passed to
> kernel command line as one option.
>
>   "abc=foo bar"

"abc=foo bar" != abc="foo bar". This behavior can potentially create
issues on systems which use measurement/signing to protect the configs,
etc. So, I think that command lines should be properly escaped before
putting them into grub.cfg and GRUB sh

Re: [PATCH 1/7] Add support for reading a filesystem with a raid5 or raid6 profile.

[Daniel Kiper]

On Tue, Apr 24, 2018 at 09:13:10PM +0200, Goffredo Baroncelli wrote:
> Signed-off-by: Goffredo Baroncelli 
> ---
>  grub-core/fs/btrfs.c | 32 
>  1 file changed, 32 insertions(+)
>
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index be195448d..b0032ea46 100644
> --- a/grub-core/fs/btrfs.c
> +++ b/grub-core/fs/btrfs.c
> @@ -119,6 +119,8 @@ struct grub_btrfs_chunk_item
>  #define GRUB_BTRFS_CHUNK_TYPE_RAID1 0x10
>  #define GRUB_BTRFS_CHUNK_TYPE_DUPLICATED0x20
>  #define GRUB_BTRFS_CHUNK_TYPE_RAID100x40
> +#define GRUB_BTRFS_CHUNK_TYPE_RAID5 0x80
> +#define GRUB_BTRFS_CHUNK_TYPE_RAID60x100

#define GRUB_BTRFS_CHUNK_TYPE_RAID5 0x80
#define GRUB_BTRFS_CHUNK_TYPE_RAID6 0x100
 Please start in one column... >^

>grub_uint8_t dummy2[0xc];
>grub_uint16_t nstripes;
>grub_uint16_t nsubstripes;
> @@ -764,6 +766,36 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, 
> grub_disk_addr_t addr,
> stripe_offset = low + chunk_stripe_length
>   * high;
> csize = chunk_stripe_length - low;
> +   break;
> + }
> +   case GRUB_BTRFS_CHUNK_TYPE_RAID5:
> +   case GRUB_BTRFS_CHUNK_TYPE_RAID6:
> + {
> +   grub_uint64_t nparities;
> +   grub_uint64_t stripe_nr, high;
> +   grub_uint64_t low;

Could you define all in one line?

> +   redundancy = 1;   /* no redundancy for now */
> +
> +   if (grub_le_to_cpu64 (chunk->type) & GRUB_BTRFS_CHUNK_TYPE_RAID5)
> + {
> +   grub_dprintf ("btrfs", "RAID5\n");
> +   nparities = 1;
> + }
> +   else
> + {
> +   grub_dprintf ("btrfs", "RAID6\n");
> +   nparities = 2;
> + }
> +
> +   stripe_nr = grub_divmod64 (off, chunk_stripe_length, &low);

This is clear for me...

> +   high = grub_divmod64 (stripe_nr, nstripes - nparities, &stripen);
> +   grub_divmod64 (high + stripen, nstripes, &stripen);

... but not these two. Could you enlighten me?

> +   stripe_offset = low + chunk_stripe_length * high;

???

> +   csize = chunk_stripe_length - low;

Chunk size to read?

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 2/7] Add helper to check the btrfs header.

[Daniel Kiper]

On Tue, Apr 24, 2018 at 09:13:11PM +0200, Goffredo Baroncelli wrote:
> This helper was used in few places to help the debugging. As conservative
> approach, in case of error it is only logged.

Could you explain in the commit message why we are so conservative here?

> Signed-off-by: Goffredo Baroncelli 
> ---
>  grub-core/fs/btrfs.c | 22 +-
>  1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index b0032ea46..01a1fc7a1 100644
> --- a/grub-core/fs/btrfs.c
> +++ b/grub-core/fs/btrfs.c
> @@ -77,7 +77,8 @@ struct btrfs_header
>  {
>grub_btrfs_checksum_t checksum;
>grub_btrfs_uuid_t uuid;
> -  grub_uint8_t dummy[0x30];
> +  grub_uint64_t bytenr;
> +  grub_uint8_t dummy[0x28];
>grub_uint32_t nitems;
>grub_uint8_t level;
>  } GRUB_PACKED;
> @@ -286,6 +287,23 @@ free_iterator (struct grub_btrfs_leaf_descriptor *desc)
>grub_free (desc->data);
>  }
>
> +static grub_err_t
> +check_btrfs_header (struct grub_btrfs_data *data, struct btrfs_header 
> *header,
> +grub_disk_addr_t addr)
> +{
> +  if (grub_le_to_cpu64 (header->bytenr) != addr)
> +{
> +  grub_dprintf ("btrfs", "btrfs_header.bytenr is not addr\n");
> +  return grub_error (GRUB_ERR_BAD_FS, "header bytenr is not addr");

s/is not addr/is not equal node addr/?

> +}
> +  if (grub_memcmp (data->sblock.uuid, header->uuid, 
> sizeof(grub_btrfs_uuid_t)))
> +{
> +  grub_dprintf ("btrfs", "btrfs_header.uuid doesn't match\n");
> +  return grub_error (GRUB_ERR_BAD_FS, "header uuid doesn't match");

s/doesn't match/doesn't match sblock uuid/?

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 3/7] Move from the find_device the error logging logic to the callee.

[Daniel Kiper]

On Tue, Apr 24, 2018 at 09:13:12PM +0200, Goffredo Baroncelli wrote:
> Signed-off-by: Goffredo Baroncelli 
> ---
>  grub-core/fs/btrfs.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index 01a1fc7a1..745bb854e 100644
> --- a/grub-core/fs/btrfs.c
> +++ b/grub-core/fs/btrfs.c
> @@ -602,9 +602,6 @@ find_device (struct grub_btrfs_data *data, grub_uint64_t 
> id, int do_rescan)
>  grub_device_iterate (find_device_iter, &ctx);
>if (!ctx.dev_found)
>  {
> -  grub_error (GRUB_ERR_BAD_FS,
> -   N_("couldn't find a necessary member device "
> -  "of multi-device filesystem"));
>return NULL;
>  }
>data->n_devices_attached++;
> @@ -862,6 +859,9 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, 
> grub_disk_addr_t addr,
>   dev = find_device (data, stripe->device_id, j);
>   if (!dev)
> {
> + grub_dprintf ("btrfs",
> +   "couldn't find a necessary member device "
> +   "of multi-device filesystem\n");

May I ask you to explain in the commit message why are you doing that?

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 4/7] Avoiding scanning for an already not found device.

[Daniel Kiper]

On Tue, Apr 24, 2018 at 09:13:13PM +0200, Goffredo Baroncelli wrote:
> If a device is missing, create the entry in data->devices_attached[] array.

What kind of entry? Invalid one? Please clarify this.

> This avoid un-necessary devices rescan.
>
> Signed-off-by: Goffredo Baroncelli 

Otherwise LGTM.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 5/7] Refactor the code of read from disk

[Daniel Kiper]

On Tue, Apr 24, 2018 at 09:13:14PM +0200, Goffredo Baroncelli wrote:
> This is a preparatory patch, to help the adding of the raid5/6 recovery code

OK, but please tell us why this patch is needed?

> Signed-off-by: Goffredo Baroncelli 
> ---
>  grub-core/fs/btrfs.c | 111 ---
>  1 file changed, 62 insertions(+), 49 deletions(-)
>
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index d6c3adbe4..697322125 100644
> --- a/grub-core/fs/btrfs.c
> +++ b/grub-core/fs/btrfs.c
> @@ -623,6 +623,47 @@ find_device (struct grub_btrfs_data *data, grub_uint64_t 
> id)
>return ctx.dev_found;
>  }
>
> +static grub_err_t
> +btrfs_read_from_chunk (struct grub_btrfs_data *data,
> +struct grub_btrfs_chunk_item *chunk,
> +grub_uint64_t stripen, grub_uint64_t stripe_offset,
> +int redundancy, grub_uint64_t csize,
> +void *buf)
> +{
> +
> +struct grub_btrfs_chunk_stripe *stripe;
> +grub_disk_addr_t paddr;
> +grub_device_t dev;
> +grub_err_t err;
> +
> +stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1);
> +/* Right now the redundancy handling is easy.
> +   With RAID5-like it will be more difficult.  */
> +stripe += stripen + redundancy;
> +
> +paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset;
> +
> +grub_dprintf ("btrfs", "stripe %" PRIxGRUB_UINT64_T
> +   " maps to 0x%" PRIxGRUB_UINT64_T "\n",
> +   stripen, stripe->offset);
> +grub_dprintf ("btrfs", "reading paddr 0x%" PRIxGRUB_UINT64_T "\n", 
> paddr);
> +
> +dev = find_device (data, stripe->device_id);
> +if (!dev)
> +  {
> + grub_dprintf ("btrfs",
> +   "couldn't find a necessary member device "
> +   "of multi-device filesystem\n");
> + grub_errno = GRUB_ERR_NONE;
> + return GRUB_ERR_READ_ERROR;
> +  }
> +
> +err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS,
> +   paddr & (GRUB_DISK_SECTOR_SIZE - 1),
> +   csize, buf);
> +return err;
> +}
> +
>  static grub_err_t
>  grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
>void *buf, grub_size_t size, int recursion_depth)
> @@ -636,7 +677,6 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, 
> grub_disk_addr_t addr,
>grub_err_t err = 0;
>struct grub_btrfs_key key_out;
>int challoc = 0;
> -  grub_device_t dev;
>struct grub_btrfs_key key_in;
>grub_size_t chsize;
>grub_disk_addr_t chaddr;
> @@ -825,55 +865,28 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, 
> grub_disk_addr_t addr,
>   if (csize > (grub_uint64_t) size)
> csize = size;
>
> - for (j = 0; j < 2; j++)
> + err = GRUB_ERR_NONE + 1;
> +
> + for (j = 0; j < 2 && err != GRUB_ERR_NONE; j++)
> {
> - for (i = 0; i < redundancy; i++)
> -   {
> - struct grub_btrfs_chunk_stripe *stripe;
> - grub_disk_addr_t paddr;
> -
> - stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1);
> - /* Right now the redundancy handling is easy.
> -With RAID5-like it will be more difficult.  */
> - stripe += stripen + i;
> -
> - paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset;
> -
> - grub_dprintf ("btrfs", "chunk 0x%" PRIxGRUB_UINT64_T
> -   "+0x%" PRIxGRUB_UINT64_T
> -   " (%d stripes (%d substripes) of %"
> -   PRIxGRUB_UINT64_T ") stripe %" PRIxGRUB_UINT64_T
> -   " maps to 0x%" PRIxGRUB_UINT64_T "\n",
> -   grub_le_to_cpu64 (key->offset),
> -   grub_le_to_cpu64 (chunk->size),
> -   grub_le_to_cpu16 (chunk->nstripes),
> -   grub_le_to_cpu16 (chunk->nsubstripes),
> -   grub_le_to_cpu64 (chunk->stripe_length),
> -   stripen, stripe->offset);
> - grub_dprintf ("btrfs", "reading paddr 0x%" PRIxGRUB_UINT64_T
> -   " for laddr 0x%" PRIxGRUB_UINT64_T "\n", paddr,
> -   addr);
> -
> - dev = find_device (data, stripe->device_id);
> - if (!dev)
> -   {
> - grub_dprintf ("btrfs",
> -   "couldn't find a necessary member device "
> -   "of multi-device filesystem\n");
> - err = grub_errno;
> - grub_errno = GRUB_ERR_NONE;
> - continue;
> -   }
> -
> - err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS,
> -   paddr & (GRUB_DISK_SECTOR_SIZE - 1),
> -   csiz

Re: [PATCH 6/7] Add support for recovery for a raid5 btrfs profiles.

[Daniel Kiper]

On Tue, Apr 24, 2018 at 09:13:15PM +0200, Goffredo Baroncelli wrote:
> Signed-off-by: Goffredo Baroncelli 
> ---
>  grub-core/fs/btrfs.c | 206 +--
>  1 file changed, 199 insertions(+), 7 deletions(-)
>
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index 697322125..5c76a68f3 100644
> --- a/grub-core/fs/btrfs.c
> +++ b/grub-core/fs/btrfs.c
> @@ -29,6 +29,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  GRUB_MOD_LICENSE ("GPLv3+");
>
> @@ -661,9 +662,180 @@ btrfs_read_from_chunk (struct grub_btrfs_data *data,
>  err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS,
> paddr & (GRUB_DISK_SECTOR_SIZE - 1),
> csize, buf);
> +grub_dprintf ("btrfs", "reading paddr 0x%" PRIxGRUB_UINT64_T "\n", 
> paddr);
>  return err;
>  }
>
> +struct raid56_buffer {
> +  void *buf;
> +  int  ok;

What ok is?

> +};
> +
> +static void
> +rebuild_raid5 (struct raid56_buffer *buffers, grub_uint64_t nstripes,
> +   grub_uint64_t csize)
> +{
> +  grub_uint64_t target, i;

grub_uint64_t target = 0, i;

> +  target = 0;

...then you can drop this.

> +  while (buffers[target].ok && target < nstripes)
> +++target;
> +
> +  if (target == nstripes)
> +{
> +  grub_dprintf ("btrfs", "called rebuild_raid5(), but all disks are 
> OK\n");
> +  return;
> +}
> +
> +  grub_dprintf ("btrfs", "rebuilding raid5 stripe #%" PRIuGRUB_UINT64_T "\n",
> + target);
> +  for (i = 0; i < nstripes ; i++)
> +if (i != target)
> +  grub_crypto_xor (buffers[target].buf, buffers[target].buf, 
> buffers[i].buf,
> +   csize);
> +}
> +
> +static void
> +rebuild_raid6 (struct raid56_buffer *buffers, grub_uint64_t nstripes,
> +   grub_uint64_t csize, grub_uint64_t parities_pos, void *dest,
> +   grub_uint64_t stripen)
> +
> +{
> +  (void)buffers;
> +  (void)nstripes;
> +  (void)csize;
> +  (void)parities_pos;
> +  (void)dest;
> +  (void)stripen;
> +  grub_dprintf ("btrfs", "called rebuild_raid6(), NOT IMPLEMENTED\n");

Could you add this function in next patch and print the relevant message
below directly instead from rebuild_raid6()?

> +}
> +
> +static grub_err_t
> +raid56_read_retry (struct grub_btrfs_data *data,
> +struct grub_btrfs_chunk_item *chunk,
> +grub_uint64_t stripe_offset, grub_uint64_t stripen,
> +grub_uint64_t csize, void *buf, grub_uint64_t parities_pos)
> +{
> +
> +  struct raid56_buffer *buffers = NULL;
> +  grub_uint64_t nstripes = grub_le_to_cpu16 (chunk->nstripes);
> +  grub_uint64_t chunk_type = grub_le_to_cpu64 (chunk->type);
> +  grub_err_t ret = GRUB_ERR_NONE;
> +  grub_uint64_t i, failed_devices;
> +
> +  buffers = grub_zalloc (sizeof(struct raid56_buffer)* nstripes);

... sizeof (*buffers) * nstripes ...

> +  if (!buffers)
> +{
> +  ret = GRUB_ERR_OUT_OF_MEMORY;
> +  goto cleanup;
> +}
> +
> +  for (i = 0 ; i < nstripes ; i++)

for (i = 0; i < nstripes; i++)

> +{
> +  buffers[i].buf = grub_zalloc(csize);

... grub_zalloc (csize);

> +  if (!buffers[i].buf)
> + {
> +   ret = GRUB_ERR_OUT_OF_MEMORY;
> +   goto cleanup;
> + }
> +}
> +
> +  for (i = 0 ; i < nstripes ; i++)

Ditto.

> +{
> +  struct grub_btrfs_chunk_stripe *stripe;
> +  grub_disk_addr_t paddr;
> +  grub_device_t dev;
> +  grub_err_t err2;
> +
> +  stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1);
> +  stripe += i;
> +
> +  paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset;
> +  grub_dprintf ("btrfs", "reading paddr %" PRIxGRUB_UINT64_T
> +" from stripe ID %" PRIxGRUB_UINT64_T "\n", paddr,
> +stripe->device_id);
> +
> +  /* FIXME: rescan the devices */
> +  dev = find_device (data, stripe->device_id);
> +  if (!dev)
> + {
> +   buffers[i].ok = 0;
> +   grub_dprintf ("btrfs", "stripe %" PRIuGRUB_UINT64_T " FAILED (dev ID 
> %"
> + PRIxGRUB_UINT64_T ")\n", i, stripe->device_id);
> +   continue;
> +}

Something is wrong with aligment.

> +  err2 = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS,
> +  paddr & (GRUB_DISK_SECTOR_SIZE - 1),
> +  csize, buffers[i].buf);
> +  if (err2 == GRUB_ERR_NONE)
> + {
> +   buffers[i].ok = 1;
> +   grub_dprintf ("btrfs", "stripe %" PRIuGRUB_UINT64_T " Ok (dev ID %"
> + PRIxGRUB_UINT64_T ")\n", i, stripe->device_id);
> + }
> +  else
> + {
> +   buffers[i].ok = 0;
> +   grub_dprintf ("btrfs", "stripe %" PRIuGRUB_UINT64_T
> + " FAILED (dev ID %" PRIxGRUB_UINT64_T ")\n", i,
> + stripe->device_id);
> + }
> +}
> +
> +  failed_devices = 0;
> +  for (i = 0 ; i < nstripes ; i++)

Ditto.

> +if (!buffers[i].ok)
> +  ++f

Re: [PATCH V2] Add support for BTRFS raid5/6 to GRUB

[Daniel Kiper]

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.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 7/7] Add raid6 recovery.

[Daniel Kiper]

On Tue, Apr 24, 2018 at 09:13:16PM +0200, Goffredo Baroncelli wrote:
> The code origins from "raid6_recovery.c". The code was changed because the
> original one assumed that both the disk address and size are multiple of
> GRUB_DISK_SECTOR_SIZE. This is not true for grub btrfs driver.
>
> The code was made more generalized using a function pointer for reading
> the data from the memory or disk.
>
> Signed-off-by: Goffredo Baroncelli 
> ---
>  grub-core/fs/btrfs.c | 211 +--
>  1 file changed, 204 insertions(+), 7 deletions(-)
>
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index 5c76a68f3..195313a31 100644
> --- a/grub-core/fs/btrfs.c
> +++ b/grub-core/fs/btrfs.c
> @@ -30,6 +30,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  GRUB_MOD_LICENSE ("GPLv3+");
>
> @@ -695,19 +696,215 @@ rebuild_raid5 (struct raid56_buffer *buffers, 
> grub_uint64_t nstripes,
> csize);
>  }
>
> +
> +/* copied from raid6_recover */
> +/* x**y.  */
> +static grub_uint8_t powx[255 * 2];
> +/* Such an s that x**s = y */
> +static unsigned powx_inv[256];
> +static const grub_uint8_t poly = 0x1d;

Could you define this as a constant?

> +static void
> +grub_raid_block_mulx (unsigned mul, char *buf, grub_size_t size)
> +{
> +  grub_size_t i;
> +  grub_uint8_t *p;
> +
> +  p = (grub_uint8_t *) buf;
> +  for (i = 0; i < size; i++, p++)
> +if (*p)
> +  *p = powx[mul + powx_inv[*p]];
> +}
> +
> +static void
> +grub_raid6_init_table (void)
> +{
> +  unsigned i;
> +
> +  grub_uint8_t cur = 1;
> +  for (i = 0; i < 255; i++)
> +{
> +  powx[i] = cur;
> +  powx[i + 255] = cur;
> +  powx_inv[cur] = i;
> +  if (cur & 0x80)
> + cur = (cur << 1) ^ poly;
> +  else
> + cur <<= 1;
> +}
> +}

Could not we do this as a static? It is initialized only once.

> +static unsigned
> +mod_255 (unsigned x)
> +{
> +  while (x > 0xff)
> +x = (x >> 8) + (x & 0xff);
> +  if (x == 0xff)
> +return 0;
> +  return x;
> +}
> +
> +typedef grub_err_t (* raid_recover_read_t)(void *data, int disk_nr,
> +grub_uint64_t addr, void *dest,
> +grub_size_t size);
> +#if 0
> +/*
> + * code example to be used in raid6_recover.
> + */
> +static grub_err_t
> +raid_recover_read_diskfilter_array (void *data, int disk_nr,
> + grub_uint64_t sector,
> + void *buf, grub_size_t size)
> +{
> +struct grub_diskfilter_segment *array = data;
> +return grub_diskfilter_read_node (&array->nodes[disk_nr],
> +   (grub_disk_addr_t)sector,
> +   size >> GRUB_DISK_SECTOR_BITS, buf);
> +}
> +#endif

Please drop this.

> +
> +static grub_err_t
> +raid_recover_read_raid56_buffer (void *data, int disk_nr, grub_uint64_t addr,
> + void *dest, grub_size_t size)
> +{
> +struct raid56_buffer *buffers = data;
> +
> +(void)addr;

"grub_uint64_t addr __attribute__ ((unused))" in prototype definition?

> +grub_memcpy(dest, buffers[disk_nr].buf, size);
> +
> +grub_errno = buffers[disk_nr].ok ? GRUB_ERR_NONE : GRUB_ERR_READ_ERROR;
> +return grub_errno;
> +}
> +
> +static grub_err_t
> +grub_raid6_recover (void *data, grub_uint64_t nstripes, int disknr, int p,
> +char *buf, grub_uint64_t sector, grub_size_t size,
> + raid_recover_read_t read_func, int layout)
> +{
> +  int i, q, pos;
> +  int bad1 = -1, bad2 = -1;
> +  char *pbuf = 0, *qbuf = 0;
> +  static int table_initializated = 0;
> +
> +  if (!table_initializated)
> +{
> +  grub_raid6_init_table();
> +  table_initializated = 1;
> +}
> +
> +  pbuf = grub_zalloc (size);
> +  if (!pbuf)
> +goto quit;
> +
> +  qbuf = grub_zalloc (size);
> +  if (!qbuf)
> +goto quit;
> +
> +  q = p + 1;
> +  if (q == (int) nstripes)
> +q = 0;
> +
> +  pos = q + 1;
> +  if (pos == (int) nstripes)
> +pos = 0;
> +
> +  for (i = 0; i < (int) nstripes - 2; i++)
> +{
> +  int c;
> +  if (layout & GRUB_RAID_LAYOUT_MUL_FROM_POS)
> + c = pos;
> +  else
> + c = i;
> +  if (pos == disknr)
> +bad1 = c;
> +  else
> +{
> +   if (!read_func(data, pos, sector, buf, size))
> +{
> +  grub_crypto_xor (pbuf, pbuf, buf, size);
> +  grub_raid_block_mulx (c, buf, size);
> +  grub_crypto_xor (qbuf, qbuf, buf, size);
> +}
> +  else
> +{
> +  /* Too many bad devices */
> +  if (bad2 >= 0)
> +goto quit;
> +
> +  bad2 = c;
> +  grub_errno = GRUB_ERR_NONE;
> +}
> +}
> +
> +  pos++;
> +  if (pos == (int) nstripes)
> +pos = 0;
> +}
> +
> +  /* Invalid disknr or p */
> +  if (bad1 < 0)
> +goto quit;
> +
> +  if (ba

Re: [PATCH V2] Add support for BTRFS raid5/6 to GRUB

[Daniel Kiper]

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

[Daniel Kiper]

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

[Daniel Kiper]

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

[Daniel Kiper]

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

Re: [PATCH] Enable /dev/mapper/dm-[0-9]-* scanning

[Daniel Kiper]

On Fri, May 11, 2018 at 02:32:48PM +0300, Oleg Solovyov wrote:
> Thanks for review
> New version is attached
>
> From 19e3f13632a20a0b1be12b6d6ff4c52ba4f3b4d6 Mon Sep 17 00:00:00 2001
> From: Oleg Solovyov 
> Date: Fri, 11 May 2018 13:55:46 +0300
> Subject: [PATCH] Don't skip /dev/mapper/dm-* devices
>
> This patch ensures that grub-probe will find the root device placed in
> /dev/mapper/dm-[0-9]+-.*
> e.g. device named /dev/mapper/dm-0-luks will be found and grub.cfg will
> be updated properly, enabling the system to boot.
>
> Signed-off-by: Oleg Solovyov 

Reviewed-by: Daniel Kiper 

If there are no objections I will apply this patch in a week or so.

Next time please use "git send-email" to send the patches.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 1/8] Add support for reading a filesystem with a raid5 or raid6 profile.

[Daniel Kiper]

On Fri, May 11, 2018 at 09:24:39PM +0200, Goffredo Baroncelli wrote:
> Signed-off-by: Goffredo Baroncelli 
> ---
>  grub-core/fs/btrfs.c | 61 
>  1 file changed, 61 insertions(+)
>
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index be195448d..7e287d0ec 100644
> --- a/grub-core/fs/btrfs.c
> +++ b/grub-core/fs/btrfs.c
> @@ -119,6 +119,8 @@ struct grub_btrfs_chunk_item
>  #define GRUB_BTRFS_CHUNK_TYPE_RAID1 0x10
>  #define GRUB_BTRFS_CHUNK_TYPE_DUPLICATED0x20
>  #define GRUB_BTRFS_CHUNK_TYPE_RAID100x40
> +#define GRUB_BTRFS_CHUNK_TYPE_RAID5 0x80
> +#define GRUB_BTRFS_CHUNK_TYPE_RAID6 0x100
>grub_uint8_t dummy2[0xc];
>grub_uint16_t nstripes;
>grub_uint16_t nsubstripes;
> @@ -764,6 +766,65 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, 
> grub_disk_addr_t addr,
> stripe_offset = low + chunk_stripe_length
>   * high;
> csize = chunk_stripe_length - low;
> +   break;
> + }
> +   case GRUB_BTRFS_CHUNK_TYPE_RAID5:
> +   case GRUB_BTRFS_CHUNK_TYPE_RAID6:
> + {
> +   grub_uint64_t nparities, stripe_nr, high, low;
> +
> +   redundancy = 1;   /* no redundancy for now */
> +
> +   if (grub_le_to_cpu64 (chunk->type) & GRUB_BTRFS_CHUNK_TYPE_RAID5)
> + {
> +   grub_dprintf ("btrfs", "RAID5\n");
> +   nparities = 1;
> + }
> +   else
> + {
> +   grub_dprintf ("btrfs", "RAID6\n");
> +   nparities = 2;
> + }
> +
> +   /*
> +* A raid 6 layout consists in several stripes spread

This line is confusing if you compare it with both cases a few lines above.
I think that you should explain somewhere why this math works for RAID 5
and RAID 6.

> +* on the disks, following a layout like the one below
> +*
> +*   Disk1  Disk2  Disk3  Ddisk4
> +*
> +*A1 B1 P1 Q1
> +*Q2 A2 B2 P2
> +*P3 Q3 A3 B3
> +*  [...]
> +*
> +*  Note that the placement of the parities depends by the row

s/by the/on/?

> +*  index;
> +*  In the code below:
> +*  stripe_nr -> is the stripe number not considering the parities
> +*   (A1=0, B1=1, A2 = 2, B2 = 3, ...)
> +*  high -> is the row number (0 for A1...Q1, 1 for Q2..P2, ...)
> +*  stripen -> is the column number (or number of disk)

s/number of disk/number of disk in row/?

> +*  off -> logical address to read (from teh beginning of the

What is "teh"? nth? And it seems to me that off is the offset from the
beginning of the array.

> +* chunk space)

I am not sure what you mean by "chunk space" here.

> +*  chunk_stripe_length -> size of a stripe (typically 64k)

I assume that stripe does not cover P1 and Q1 (parity disks)?

> +*  nstripes -> number of disks
> +*
> +*/
> +   stripe_nr = grub_divmod64 (off, chunk_stripe_length, &low);

What is the low?

> +   high = grub_divmod64 (stripe_nr, nstripes - nparities, &stripen);
> +
> +   /*
> +* until now stripen is evaluated without the parities (0 for A1,
> +* A2, A3... 1 for B1, B2...); now consider also the parities (0
> +* for A1, 1 for A2, 2 for A3); the math is done "modulo"
> +* number of disk

May I ask you to use proper English sentences in the comments?

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 2/8] Add helper to check the btrfs header.

[Daniel Kiper]

On Fri, May 11, 2018 at 09:24:40PM +0200, Goffredo Baroncelli wrote:
> This helper was used in few places to help the debugging. As conservative

s/was/will be/?

> approach, in case of error it is only logged. This because I was unaware

s/This because I was unaware/This is because I am not sure/

> if this could change something the error handling of the previous

s/could/can/
s/the error/in the error/

> code.

s/previous code./currently existing code./

Otherwise LGTM.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 3/8] Move the error logging logic from find_device() to the callee.

[Daniel Kiper]

On Fri, May 11, 2018 at 09:24:41PM +0200, Goffredo Baroncelli wrote:
> This is a preparatory patch. The callee knows better if this
> error is fatal, or if it exists another available disk.

WRT the subject:
  s/Move the error logging logic from find_device() to the callee./
Move the error logging from find_device() to its caller/?

And it seems to me that every email's subject should start with "btrfs: ",
e.g. "btrfs: Move the error logging from find_device() to its caller".

s/error is fatal, or if it exists another available disk/
  error is fatal or not, i.e. another disk is available./

Otherwise LGTM.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 5/8] Refactor the code that read from disk

[Daniel Kiper]

It looks that patch 4 is missing in this series.

On Fri, May 11, 2018 at 09:24:43PM +0200, Goffredo Baroncelli wrote:
> This is a preparatory patch, to help the adding of the raid5/6 recovery code.
> In case of availability of all disks, this code is good for all the RAID
> profiles. However in case of failure, the error handling is quite different.
> Refactoring this code increases the general readability.

s/raid/RAID/ in all patches please. And use "RAID 5" and "RAID 6" (with
space between RAID and number) respectively.

> Signed-off-by: Goffredo Baroncelli 
> ---
>  grub-core/fs/btrfs.c | 106 +--
>  1 file changed, 61 insertions(+), 45 deletions(-)
>
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index 32c4382e5..fc4198e39 100644
> --- a/grub-core/fs/btrfs.c
> +++ b/grub-core/fs/btrfs.c
> @@ -625,6 +625,47 @@ find_device (struct grub_btrfs_data *data, grub_uint64_t 
> id)
>return ctx.dev_found;
>  }
>
> +static grub_err_t
> +btrfs_read_from_chunk (struct grub_btrfs_data *data,
> +struct grub_btrfs_chunk_item *chunk,
> +grub_uint64_t stripen, grub_uint64_t stripe_offset,
> +int redundancy, grub_uint64_t csize,
> +void *buf)
> +{
> +
> +struct grub_btrfs_chunk_stripe *stripe;
> +grub_disk_addr_t paddr;
> +grub_device_t dev;
> +grub_err_t err;
> +
> +stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1);
> +/* Right now the redundancy handling is easy.
> +   With RAID5-like it will be more difficult.  */
> +stripe += stripen + redundancy;
> +
> +paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset;
> +
> +grub_dprintf ("btrfs", "stripe %" PRIxGRUB_UINT64_T
> +   " maps to 0x%" PRIxGRUB_UINT64_T "\n",
> +   stripen, stripe->offset);
> +grub_dprintf ("btrfs", "reading paddr 0x%" PRIxGRUB_UINT64_T "\n", 
> paddr);
> +
> +dev = find_device (data, stripe->device_id);
> +if (!dev)
> +  {
> + grub_dprintf ("btrfs",
> +   "couldn't find a necessary member device "
> +   "of multi-device filesystem\n");
> + grub_errno = GRUB_ERR_NONE;
> + return GRUB_ERR_READ_ERROR;
> +  }
> +
> +err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS,
> +   paddr & (GRUB_DISK_SECTOR_SIZE - 1),
> +   csize, buf);
> +return err;
> +}
> +
>  static grub_err_t
>  grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
>void *buf, grub_size_t size, int recursion_depth)
> @@ -638,7 +679,6 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, 
> grub_disk_addr_t addr,
>grub_err_t err = 0;
>struct grub_btrfs_key key_out;
>int challoc = 0;
> -  grub_device_t dev;
>struct grub_btrfs_key key_in;
>grub_size_t chsize;
>grub_disk_addr_t chaddr;
> @@ -858,53 +898,29 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, 
> grub_disk_addr_t addr,
>
>   for (j = 0; j < 2; j++)
> {
> + grub_dprintf ("btrfs", "chunk 0x%" PRIxGRUB_UINT64_T
> +   "+0x%" PRIxGRUB_UINT64_T
> +   " (%d stripes (%d substripes) of %"
> +   PRIxGRUB_UINT64_T ") \n",
> +   grub_le_to_cpu64 (key->offset),
> +   grub_le_to_cpu64 (chunk->size),
> +   grub_le_to_cpu16 (chunk->nstripes),
> +   grub_le_to_cpu16 (chunk->nsubstripes),
> +   grub_le_to_cpu64 (chunk->stripe_length));
> + grub_dprintf ("btrfs", "reading laddr 0x%" PRIxGRUB_UINT64_T "\n",
> +   addr);
> +
>   for (i = 0; i < redundancy; i++)
> {
> - struct grub_btrfs_chunk_stripe *stripe;
> - grub_disk_addr_t paddr;
> -
> - stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1);
> - /* Right now the redundancy handling is easy.
> -With RAID5-like it will be more difficult.  */
> - stripe += stripen + i;
> -
> - paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset;
> -
> - grub_dprintf ("btrfs", "chunk 0x%" PRIxGRUB_UINT64_T
> -   "+0x%" PRIxGRUB_UINT64_T
> -   " (%d stripes (%d substripes) of %"
> -   PRIxGRUB_UINT64_T ") stripe %" PRIxGRUB_UINT64_T
> -   " maps to 0x%" PRIxGRUB_UINT64_T "\n",
> -   grub_le_to_cpu64 (key->offset),
> -   grub_le_to_cpu64 (chunk->size),
> -   grub_le_to_cpu16 (chunk->nstripes),
> -   grub_le_to_cpu16 (chunk->nsubstripes),
> -   grub_le_to_cpu64 (chunk->stripe_length),
> -   stripen, stripe->offset)

Re: [PATCH 6/8] Add support for recovery for a raid5 btrfs profiles.

[Daniel Kiper]

On Fri, May 11, 2018 at 09:24:44PM +0200, Goffredo Baroncelli wrote:
> Signed-off-by: Goffredo Baroncelli 
> ---
>  grub-core/fs/btrfs.c | 178 +--
>  1 file changed, 173 insertions(+), 5 deletions(-)
>
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index fc4198e39..8d72607d1 100644
> --- a/grub-core/fs/btrfs.c
> +++ b/grub-core/fs/btrfs.c
> @@ -29,6 +29,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  GRUB_MOD_LICENSE ("GPLv3+");
>
> @@ -663,9 +664,156 @@ btrfs_read_from_chunk (struct grub_btrfs_data *data,
>  err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS,
> paddr & (GRUB_DISK_SECTOR_SIZE - 1),
> csize, buf);
> +grub_dprintf ("btrfs", "reading paddr 0x%" PRIxGRUB_UINT64_T "\n", 
> paddr);
>  return err;
>  }
>
> +struct raid56_buffer {
> +  void *buf;
> +  int  data_is_valid;
> +};
> +
> +static void
> +rebuild_raid5 (struct raid56_buffer *buffers, grub_uint64_t nstripes,
> +   grub_uint64_t csize)
> +{
> +  grub_uint64_t target = 0, i;
> +
> +  while (buffers[target].data_is_valid && target < nstripes)
> +++target;
> +
> +  if (target == nstripes)
> +{
> +  grub_dprintf ("btrfs", "called rebuild_raid5(), but all disks are 
> OK\n");
> +  return;
> +}
> +
> +  grub_dprintf ("btrfs", "rebuilding raid5 stripe #%" PRIuGRUB_UINT64_T "\n",
> + target);
> +  for (i = 0; i < nstripes ; i++)

Please drop extra space behind nstripes. I have found
similar issues below too. Please fix all of them.

> +if (i != target)
> +  grub_crypto_xor (buffers[target].buf, buffers[target].buf, 
> buffers[i].buf,
> +   csize);
> +}
> +
> +static grub_err_t
> +raid56_read_retry (struct grub_btrfs_data *data,
> +struct grub_btrfs_chunk_item *chunk,
> +grub_uint64_t stripe_offset, grub_uint64_t stripen,
> +grub_uint64_t csize, void *buf)
> +{
> +
> +  struct raid56_buffer *buffers = NULL;
> +  grub_uint64_t nstripes = grub_le_to_cpu16 (chunk->nstripes);
> +  grub_uint64_t chunk_type = grub_le_to_cpu64 (chunk->type);
> +  grub_err_t ret = GRUB_ERR_NONE;
> +  grub_uint64_t i, failed_devices;
> +
> +  buffers = grub_zalloc (sizeof(*buffers) * nstripes);
> +  if (!buffers)
> +{
> +  ret = GRUB_ERR_OUT_OF_MEMORY;
> +  goto cleanup;
> +}
> +
> +  for (i = 0; i < nstripes ; i++)

Ditto.

> +{
> +  buffers[i].buf = grub_zalloc (csize);
> +  if (!buffers[i].buf)
> + {
> +   ret = GRUB_ERR_OUT_OF_MEMORY;
> +   goto cleanup;
> + }
> +}
> +
> +  for (i = 0; i < nstripes ; i++)

Ditto.

> +{
> +  struct grub_btrfs_chunk_stripe *stripe;
> +  grub_disk_addr_t paddr;
> +  grub_device_t dev;
> +  grub_err_t err2;
> +
> +  stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1);
> +  stripe += i;
> +
> +  paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset;
> +  grub_dprintf ("btrfs", "reading paddr %" PRIxGRUB_UINT64_T
> +" from stripe ID %" PRIxGRUB_UINT64_T "\n", paddr,
> +stripe->device_id);
> +
> +  /* FIXME: rescan the devices */

Could you be more verbose here?

> +  dev = find_device (data, stripe->device_id);
> +  if (!dev)
> + {
> +   buffers[i].data_is_valid = 0;
> +   grub_dprintf ("btrfs", "stripe %" PRIuGRUB_UINT64_T " FAILED (dev ID 
> %"
> + PRIxGRUB_UINT64_T ")\n", i, stripe->device_id);
> +   continue;
> + }
> +
> +  err2 = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS,
> +  paddr & (GRUB_DISK_SECTOR_SIZE - 1),
> +  csize, buffers[i].buf);
> +  if (err2 == GRUB_ERR_NONE)
> + {
> +   buffers[i].data_is_valid = 1;
> +   grub_dprintf ("btrfs", "stripe %" PRIuGRUB_UINT64_T " Ok (dev ID %"
> + PRIxGRUB_UINT64_T ")\n", i, stripe->device_id);
> + }
> +  else
> + {
> +   buffers[i].data_is_valid = 0;
> +   grub_dprintf ("btrfs", "stripe %" PRIuGRUB_UINT64_T
> + " FAILED (dev ID %" PRIxGRUB_UINT64_T ")\n", i,
> + stripe->device_id);
> + }
> +}
> +
> +  failed_devices = 0;
> +  for (i = 0; i < nstripes ; i++)
> +if (!buffers[i].data_is_valid)
> +  ++failed_devices;
> +  if (failed_devices > 1 && (chunk_type & GRUB_BTRFS_CHUNK_TYPE_RAID5))
> +{
> +  grub_dprintf ("btrfs",
> + "not enough disks for raid5: total %" PRIuGRUB_UINT64_T
> + ", missing %" PRIuGRUB_UINT64_T "\n",
> + nstripes, failed_devices);
> +  ret = GRUB_ERR_READ_ERROR;
> +  goto cleanup;
> +}
> +  else
> +{
> +  grub_dprintf ("btrfs",
> +"enough disks for raid5/6 rebuilding: total %"
> + PRIuGRUB_UINT64_T ", missing %" PRIuGRUB_UINT64_T "\n",
> + 

Re: [PATCH 5/8] Refactor the code that read from disk

[Daniel Kiper]

On Mon, May 14, 2018 at 08:26:40PM +0200, Daniel Kiper wrote:
> It looks that patch 4 is missing in this series.
>
> On Fri, May 11, 2018 at 09:24:43PM +0200, Goffredo Baroncelli wrote:
> > This is a preparatory patch, to help the adding of the raid5/6 recovery 
> > code.
> > In case of availability of all disks, this code is good for all the RAID
> > profiles. However in case of failure, the error handling is quite different.
> > Refactoring this code increases the general readability.
>
> s/raid/RAID/ in all patches please. And use "RAID 5" and "RAID 6" (with
> space between RAID and number) respectively.

Of course I thought about the commit messages and comments. Code you
can leave as is.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 7/8] Make more generic the code for raid6 rebuilding

[Daniel Kiper]

On Fri, May 11, 2018 at 09:24:45PM +0200, Goffredo Baroncelli wrote:
> The original grub code which handles the recovery of a raid6 disks array
> assumes that all the read are multiple of 1 << GRUB_DISK_SECTOR_BITS and

s/the read/reads/

> it also assumes that all the I/O is done via the struct
> grub_diskfilter_segment.
> This is not true for the btrfs code. In order to reuse the native
> grub_raid6_recover() code, it is modified to not call
> grub_diskfilter_read_node directly, but to call an handler passed
> as argument.

Could you fix the formating? Currently it is difficult to read.

> Signed-off-by: Goffredo Baroncelli 
> ---
>  grub-core/disk/raid6_recover.c | 52 ++
>  include/grub/diskfilter.h  |  9 ++
>  2 files changed, 43 insertions(+), 18 deletions(-)
>
> diff --git a/grub-core/disk/raid6_recover.c b/grub-core/disk/raid6_recover.c
> index aa674f6ca..1b37d0552 100644
> --- a/grub-core/disk/raid6_recover.c
> +++ b/grub-core/disk/raid6_recover.c
> @@ -74,14 +74,25 @@ mod_255 (unsigned x)
>  }
>
>  static grub_err_t
> -grub_raid6_recover (struct grub_diskfilter_segment *array, int disknr, int p,
> -char *buf, grub_disk_addr_t sector, grub_size_t size)
> +raid_recover_read_diskfilter_array (void *data, int disk_nr,

s/raid_recover_read_diskfilter_array/raid6_read_node_gen/

> + grub_uint64_t sector,
> + void *buf, grub_size_t size)
> +{
> +struct grub_diskfilter_segment *array = data;

Please add empty line here.

> +return grub_diskfilter_read_node (&array->nodes[disk_nr],
> +   (grub_disk_addr_t)sector,
> +   size >> GRUB_DISK_SECTOR_BITS, buf);
> +}
> +
> +grub_err_t
> +grub_raid6_recover_generic (void *data, grub_uint64_t nstripes, int disknr, 
> int p,

s/grub_raid6_recover_generic/grub_raid6_recover_gen/

> + char *buf, grub_uint64_t sector, grub_size_t size,
> + raid_recover_read_t read_func, int layout)

s/read_func/read_node/

Hmmm... Could not you create a struct for this call?

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 8/8] Add raid6 recovery for a btrfs filesystem.

[Daniel Kiper]

On Fri, May 11, 2018 at 09:24:46PM +0200, Goffredo Baroncelli wrote:
> Add the raid6 recovery, in order to use a raid6 filesystem even if some
> disks (up to two) are missing.
> This code use the old md raid6 code already present in grub.

Please fix the commit message formating.

> Signed-off-by: Goffredo Baroncelli 
> ---
>  grub-core/fs/btrfs.c | 43 +++
>  1 file changed, 39 insertions(+), 4 deletions(-)
>
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index 8d72607d1..07e9db910 100644
> --- a/grub-core/fs/btrfs.c
> +++ b/grub-core/fs/btrfs.c
> @@ -30,6 +30,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  GRUB_MOD_LICENSE ("GPLv3+");
>
> @@ -696,11 +697,36 @@ rebuild_raid5 (struct raid56_buffer *buffers, 
> grub_uint64_t nstripes,
> csize);
>  }
>
> +static grub_err_t
> +raid_recover_read_raid56_buffer (void *data, int disk_nr, grub_uint64_t addr,
> + void *dest, grub_size_t size)

s/raid_recover_read_raid56_buffer/raid6_recover_read_node/

> +{
> +struct raid56_buffer *buffers = data;
> +
> +(void)addr;

I do not like this. grub_uint64_t addr __attribute__ ((unused))" in
prototype definition please.

> +grub_memcpy(dest, buffers[disk_nr].buf, size);
> +
> +grub_errno = buffers[disk_nr].data_is_valid ? GRUB_ERR_NONE :
> +  GRUB_ERR_READ_ERROR;
> +return grub_errno;
> +}
> +
> +static void
> +rebuild_raid6 (struct raid56_buffer *buffers, grub_uint64_t nstripes,
> +   grub_uint64_t csize, grub_uint64_t parities_pos, void *dest,
> +   grub_uint64_t stripen)

struct as a argument?

> +
> +{
> +  grub_raid6_recover_generic (buffers, nstripes, stripen, parities_pos,
> +  dest, 0, csize,
> +  raid_recover_read_raid56_buffer, 0);
> +}
> +
>  static grub_err_t
>  raid56_read_retry (struct grub_btrfs_data *data,
>  struct grub_btrfs_chunk_item *chunk,
>  grub_uint64_t stripe_offset, grub_uint64_t stripen,
> -grub_uint64_t csize, void *buf)
> +grub_uint64_t csize, void *buf, grub_uint64_t parities_pos)

struct as a argument?

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 7/8] Make more generic the code for raid6 rebuilding

[Daniel Kiper]

On Mon, May 14, 2018 at 08:57:10PM +0200, Daniel Kiper wrote:
> On Fri, May 11, 2018 at 09:24:45PM +0200, Goffredo Baroncelli wrote:
> > The original grub code which handles the recovery of a raid6 disks array
> > assumes that all the read are multiple of 1 << GRUB_DISK_SECTOR_BITS and
>
> s/the read/reads/
>
> > it also assumes that all the I/O is done via the struct
> > grub_diskfilter_segment.
> > This is not true for the btrfs code. In order to reuse the native
> > grub_raid6_recover() code, it is modified to not call
> > grub_diskfilter_read_node directly, but to call an handler passed
> > as argument.
>
> Could you fix the formating? Currently it is difficult to read.
>
> > Signed-off-by: Goffredo Baroncelli 
> > ---
> >  grub-core/disk/raid6_recover.c | 52 ++
> >  include/grub/diskfilter.h  |  9 ++
> >  2 files changed, 43 insertions(+), 18 deletions(-)
> >
> > diff --git a/grub-core/disk/raid6_recover.c b/grub-core/disk/raid6_recover.c
> > index aa674f6ca..1b37d0552 100644
> > --- a/grub-core/disk/raid6_recover.c
> > +++ b/grub-core/disk/raid6_recover.c
> > @@ -74,14 +74,25 @@ mod_255 (unsigned x)
> >  }
> >
> >  static grub_err_t
> > -grub_raid6_recover (struct grub_diskfilter_segment *array, int disknr, int 
> > p,
> > -char *buf, grub_disk_addr_t sector, grub_size_t size)
> > +raid_recover_read_diskfilter_array (void *data, int disk_nr,
>
> s/raid_recover_read_diskfilter_array/raid6_read_node_gen/

s/raid_recover_read_diskfilter_array/raid6_recover_read_node_gen/

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 7/8] Make more generic the code for raid6 rebuilding

[Daniel Kiper]

On Mon, May 14, 2018 at 10:52:11PM +0200, Goffredo Baroncelli wrote:
> On 05/14/2018 08:57 PM, Daniel Kiper wrote:
> > On Fri, May 11, 2018 at 09:24:45PM +0200, Goffredo Baroncelli wrote:
> [...]
> >
> >> +  char *buf, grub_uint64_t sector, grub_size_t size,
> >> +  raid_recover_read_t read_func, int layout)
> >
> > s/read_func/read_node/
> >
> > Hmmm... Could not you create a struct for this call?
>
> What do you mean ? Do you mean passing a (pointer of a) struct instead of 
> passing all these arguments ?

Exactly!

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: xfsprogs 4.16.0 format change, GRUB error unknown filesystem

[Daniel Kiper]

Hi,

On Mon, May 14, 2018 at 01:12:27PM -0600, Chris Murphy wrote:
> Hi,
>
> xfsprogs 4.16.0 defaults to enabling "sparse inode" which is not
> compatible with GRUB.
>
> Downstream bug in Fedora, grub-probe fails so grub-install can't work;
> and on systems with an already properly installed core.img with XFS
> support, it can't read an XFS file system with this feature set.
> https://bugzilla.redhat.com/show_bug.cgi?id=1575797
>
> Make them default
> https://www.spinics.net/lists/linux-xfs/msg16598.html
>
> Change description, page 48
> https://mirrors.edge.kernel.org/pub/linux/utils/fs/xfs/docs/xfs_filesystem_structure.pdf
>
> It'd be helpful to know the approximate time frame for getting this
> fixed, to know whether for Fedora 29 /boot on XFS should be inhibited
> until a proper tested patch for GRUB exists.

Sorry, I do not have resources right now to do that. However,
if somebody will post the patches I am happy to review them.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] mbi: use per segment a separate relocator chunk

[Daniel Kiper]

On Mon, May 14, 2018 at 09:02:00PM +0200, Alexander Boettcher wrote:
> >From 8a0296d040a42950cd64e733f7997203975bc184 Mon Sep 17 00:00:00 2001
> From: Alexander Boettcher 
> Date: Fri, 13 Apr 2018 23:37:01 +0200
> Subject: [PATCH] mbi: use per segment a separate relocator chunk
>
> if elf is non relocatable.

s/elf/ELF file/

> If the segments are not dense packed, the original code set up a huge
> relocator chunk comprising all segments.
>
> During the final relocation in grub_relocator_prepare_relocs, the chunk
> is moved to its desired place and overrides memory which are actually not
> covered/touched by the specified segments.
>
> The overriden memory may contain device memory (vga text mode e.g.), which
> leads to strange boot behaviour.

I assume that a given ELF PHDR address/size does not cover VGA memory or
anything like that, so, I am not sure what exactly overwrites this region.
grub_memset() in current line 161 at some point?

> Signed-off-by: Alexander Boettcher 
> ---
>  grub-core/loader/multiboot_elfxx.c | 57 
> +++---
>  1 file changed, 35 insertions(+), 22 deletions(-)
>
> diff --git a/grub-core/loader/multiboot_elfxx.c 
> b/grub-core/loader/multiboot_elfxx.c
> index 67daf59..539c94a 100644
> --- a/grub-core/loader/multiboot_elfxx.c
> +++ b/grub-core/loader/multiboot_elfxx.c
> @@ -57,9 +57,9 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
>char *phdr_base;
>grub_err_t err;
>grub_relocator_chunk_t ch;
> -  grub_uint32_t load_offset, load_size;
> +  grub_uint32_t load_offset = 0, load_size;
>int i;
> -  void *source;
> +  void *source = NULL;

It seems to me that this change is not needed.
I am thinking about "void *source = NULL;".

>if (ehdr->e_ident[EI_MAG0] != ELFMAG0
>|| ehdr->e_ident[EI_MAG1] != ELFMAG1
> @@ -83,6 +83,7 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
>  #define phdr(i)  ((Elf_Phdr *) (phdr_base + (i) * 
> ehdr->e_phentsize))
>
>mld->link_base_addr = ~0;
> +  mld->load_base_addr = ~0;
>
>/* Calculate lowest and highest load address.  */
>for (i = 0; i < ehdr->e_phnum; i++)
> @@ -97,10 +98,14 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
>  return grub_error (GRUB_ERR_BAD_OS, "segment crosses 4 GiB border");
>  #endif
>
> -  load_size = highest_load - mld->link_base_addr;
> +  grub_dprintf ("multiboot_loader", "link_base_addr=0x%x, 
> load_base_addr=0x%x, "
> + "relocatable=%d\n", mld->link_base_addr,
> + mld->load_base_addr, mld->relocatable);

mld->load_base_addr is always ~0 here, so, it does not make sense
to display it here. Though I think that mld->link_base_addr and
mld->relocatable should be shown here. Maybe other values from mld
which are know here, i.e. align, preference, avoid_efi_boot_services
too...

>if (mld->relocatable)
>  {
> +  load_size = highest_load - mld->link_base_addr;
> +
>if (load_size > mld->max_addr || mld->min_addr > mld->max_addr - 
> load_size)
>   return grub_error (GRUB_ERR_BAD_OS, "invalid min/max address and/or 
> load size");
>
> @@ -108,27 +113,19 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t 
> *mld)
> mld->min_addr, mld->max_addr - 
> load_size,
> load_size, mld->align ? 
> mld->align : 1,
> mld->preference, 
> mld->avoid_efi_boot_services);
> -}
> -  else
> -err = grub_relocator_alloc_chunk_addr (GRUB_MULTIBOOT (relocator), &ch,
> -mld->link_base_addr, load_size);
>
> -  if (err)
> -{
> -  grub_dprintf ("multiboot_loader", "Cannot allocate memory for OS 
> image\n");
> -  return err;
> -}
> -
> -  mld->load_base_addr = get_physical_target_address (ch);
> -  source = get_virtual_current_address (ch);
> +  if (err)
> +{
> +  grub_dprintf ("multiboot_loader", "Cannot allocate memory for OS 
> image\n");
> +  return err;
> +}
>
> -  grub_dprintf ("multiboot_loader", "link_base_addr=0x%x, 
> load_base_addr=0x%x, "
> - "load_size=0x%x, relocatable=%d\n", mld->link_base_addr,
> - mld->load_base_addr, load_size, mld->relocatable);
> +  mld->load_base_addr = get_physical_target_address (ch);
> +  source = get_virtual_current_address (ch);
>
> -  if (mld->relocatable)
> -grub_dprintf ("multiboot_loader", "align=0x%lx, preference=0x%x, 
> avoid_efi_boot_services=%d\n",
> -   (long) mld->align, mld->preference, 
> mld->avoid_efi_boot_services);
> +  grub_dprintf ("multiboot_loader", "align=0x%lx, preference=0x%x, 
> avoid_efi_boot_services=%d\n",
> +  (long) mld->align, mld->preference, 
> mld->avoid_efi_boot_services);
> +}

I think that this grub_dprintf() should be moved...

>/* Load every loadable segment in memory.  */
>for (i = 0; i < e

Re: FTDI serial console support?

[Daniel Kiper]

On Fri, May 11, 2018 at 03:55:36PM +0200, Petter Gustad wrote:
>
> Is there a way to specify a FTDI based serial console using
> GRUB_TERMINAL, GRUB_SERIAL_COMMAND, or some other variable? If yes,
> what is the syntax?
>
> BTW my serial port adapter matches the vendor id and device id given
> in grub-core/bus/usb/serial/ftdi.c:
>
> Bus 001 Device 008: ID 0403:6001 Future Technology Devices International, Ltd 
> FT232 Serial (UART) IC

Have you tried usbserial_ftdi, e.g. GRUB_TERMINAL=usbserial_ftdi?
Just guessing...

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [GRUB PATCH] xfs: accept filesystem with sparse inodes

[Daniel Kiper]

On Tue, May 15, 2018 at 02:55:55PM -0500, Eric Sandeen wrote:
> The sparse inode metadata format became a mkfs.xfs default in
> xfsprogs-4.16.0, and such filesystems are now rejected by grub as
> containing an incompatible feature.
>
> In essence, this feature allows xfs to allocate inodes into fragmented
> freespace.  (Without this feature, if xfs could not allocate contiguous
> space for 64 new inodes, inode creation would fail.)
>
> In practice, the disk format change is restricted to the inode btree,
> which as far as I can tell is not used by grub.  If all you're doing
> today is parsing a directory, reading an inode number, and converting
> that inode number to a disk location, then ignoring this feature
> should be fine, so I've added it to XFS_SB_FEAT_INCOMPAT_SUPPORTED
>
> I did some brief testing of this patch by hacking up the regression
> tests to completely fragment freespace on the test xfs filesystem, and
> then write a large-ish number of inodes to consume any existing
> contiguous 64-inode chunk.  This way any files the grub tests add and
> traverse would be in such a fragmented inode allocation.  Tests passed,
> but I'm not sure how to cleanly integrate that into the test harness.
>
> Signed-off-by: Eric Sandeen 

Eric, thank you for posting the patch. LGTM.

Chris, may I ask you to test it and add your "Tested-by:" if it works?

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: FTDI serial console support?

[Daniel Kiper]

On Wed, May 16, 2018 at 06:21:17AM +0200, Petter Gustad wrote:
> From: Daniel Kiper 
> Subject: Re: FTDI serial console support?
> Date: Tue, 15 May 2018 22:00:06 +0200
>
> > On Fri, May 11, 2018 at 03:55:36PM +0200, Petter Gustad wrote:
> >>
> >> Is there a way to specify a FTDI based serial console using
> >> GRUB_TERMINAL, GRUB_SERIAL_COMMAND, or some other variable? If yes,
> >> what is the syntax?
> >>
> >> BTW my serial port adapter matches the vendor id and device id given
> >> in grub-core/bus/usb/serial/ftdi.c:
> >>
> >> Bus 001 Device 008: ID 0403:6001 Future Technology Devices International, 
> >> Ltd FT232 Serial (UART) IC
> >
> > Have you tried usbserial_ftdi, e.g. GRUB_TERMINAL=usbserial_ftdi?
> > Just guessing...
>
> Yes, I tried (as I found the "usbserial_ftdi" string in the source)
> the but grub-mkconfig does not accecpt it:
>
> grub-mkconfig -o /boot/grub/grub.cfg
> Invalid output terminal "usbserial_ftdi"
>
> Currently I have this:
>
> GRUB_CMDLINE_LINUX_DEFAULT="console=tty0 console=ttyUSB0,115200n8"
> #GRUB_TERMINAL="console serial"
> GRUB_TERMINAL="serial"
> GRUB_SERIAL_COMMAND="usbserial_ftdi --speed=115200 --unit=0 --word=8 
> --parity=no --stop=1"
>
> But this does not seem to work either, even though I do get console
> output on my USB serial after the kernel has loaded (due to
> GRUB_CMDLINE_LINUX being passed to the kernel), e.g. I can do "echo
> hello > /dev/console" and read "hello" on the USB serial output. When
> I do a "reboot" I observe messages about unmounting filesystems etc.
>
> I was hoping to be able to select kernels and options in grub using
> the USB serial line.

Please take a look here:
  https://www.coreboot.org/GRUB2#On_a_USB_serial_or_USB_debug_adapter

I hope that helps.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] mbi: use per segment a separate relocator chunk

[Daniel Kiper]

On Tue, May 15, 2018 at 09:18:18PM +0200, Alexander Boettcher wrote:
> On 15.05.2018 21:10, Alexander Boettcher wrote:
> >>I assume that a given ELF PHDR address/size does not cover VGA memory or
> >>anything like that,
> >
> >No.
> >
> >>so, I am not sure what exactly overwrites this region.
> >>grub_memset() in current line 161 at some point?
> >
> >No. During grub_relocator_prepare_reloc the overwrite happens, if i'm
> >not wrong.
> >
> >An (artificial) example, imagine two ELF PHDRs, e.g.
> >
> > ??[0x8000-0x9000) and
> > ??[0x200-0x210).
> >
> >Without this patch grub calculates one relocator chunk of size 0x20f8000
> >(0x210 - 0x8000) and places it at some higher memory, e.g.
> >[0x300 - 0x30f8000). During the invocation of
>
> Must be [0x300-0x50f8000)
>
> >grub_relocator_prepare_reloc the memory gets copied from
> >
> >[0x300-0x30f8000) to [0x8000-0x210)
>
> Must be [0x300-0x50f8000) to [0x8000-0x210)

It seems to me that it happens a bit later. AIUI grub_relocator_prepare_reloc()
prepare movers which are executed when main GRUB code is left,
e.g relst() call from grub_relocator32_boot(). And the movers
do bad job. Well, they were told to do so... Anyway, I think
that the comment should be a bit more clear about it.

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

[Daniel Kiper]

Re-added grub-devel. Next time please do not drop GRUB ML from the addresses.

On Thu, May 17, 2018 at 03:40:32PM -0700, Andrew Jeddeloh wrote:
> Sorry about the long delay, I agree with all the sugguestions, except 
> shouldn't

No problem.

> if (len > &linux_params.e820_map - &linux_params)
>
> be
>
> if (len > sizeof(linux_params))
>
> since at that point len is the total size of the header which
> linux_params represents?

Please take a look at arch/x86/include/uapi/asm/bootparam.h in latest
Linux kernel source. My proposal is better but it seems to me right now
that it is still too much. I have a feeling that we should not go beyond
the end of boot_params._pad7. Am I right?

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: FTDI serial console support?

[Daniel Kiper]

On Fri, May 18, 2018 at 01:01:41PM +0200, Petter Gustad wrote:
> From: Daniel Kiper 
> Subject: Re: FTDI serial console support?
> Date: Wed, 16 May 2018 11:32:19 +0200
>
> > On Wed, May 16, 2018 at 06:21:17AM +0200, Petter Gustad wrote:
> >> From: Daniel Kiper 
> >> Subject: Re: FTDI serial console support?
> >> Date: Tue, 15 May 2018 22:00:06 +0200
> >>
> >> > On Fri, May 11, 2018 at 03:55:36PM +0200, Petter Gustad wrote:
> >> >>
> >> >> Is there a way to specify a FTDI based serial console using
> >> >> GRUB_TERMINAL, GRUB_SERIAL_COMMAND, or some other variable? If yes,
> >> >> what is the syntax?
> >> >>
> >> >> BTW my serial port adapter matches the vendor id and device id given
> >> >> in grub-core/bus/usb/serial/ftdi.c:
> >> >>
> >> >> Bus 001 Device 008: ID 0403:6001 Future Technology Devices 
> >> >> International, Ltd FT232 Serial (UART) IC
> >> >
> >> > Have you tried usbserial_ftdi, e.g. GRUB_TERMINAL=usbserial_ftdi?
> >> > Just guessing...
> >>
> >> Yes, I tried (as I found the "usbserial_ftdi" string in the source)
> >> the but grub-mkconfig does not accecpt it:
> >>
> >> grub-mkconfig -o /boot/grub/grub.cfg
> >> Invalid output terminal "usbserial_ftdi"
> >>
> >> Currently I have this:
> >>
> >> GRUB_CMDLINE_LINUX_DEFAULT="console=tty0 console=ttyUSB0,115200n8"
> >> #GRUB_TERMINAL="console serial"
> >> GRUB_TERMINAL="serial"
> >> GRUB_SERIAL_COMMAND="usbserial_ftdi --speed=115200 --unit=0 --word=8 
> >> --parity=no --stop=1"
> >>
> >> But this does not seem to work either, even though I do get console
> >> output on my USB serial after the kernel has loaded (due to
> >> GRUB_CMDLINE_LINUX being passed to the kernel), e.g. I can do "echo
> >> hello > /dev/console" and read "hello" on the USB serial output. When
> >> I do a "reboot" I observe messages about unmounting filesystems etc.
> >>
> >> I was hoping to be able to select kernels and options in grub using
> >> the USB serial line.
> >
> > Please take a look here:
> >   https://www.coreboot.org/GRUB2#On_a_USB_serial_or_USB_debug_adapter
> >
> > I hope that helps.
>
> Thank you for the link.
>
> Unfortunately, it seems like my system can't access the hard disk
> after running "insmod ehci" despite doing "insmod nativedisk" first.
> Some of subsequent insmod commands will fail when accessing the disk
> and I'm not able to boot linux either.

Could you just do "insmod usbserial_ftdi"? There is a chance that it
will pull in all needed dependencies without breaking disk access.

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

[Daniel Kiper]

On Tue, May 29, 2018 at 04:55:10PM +0300, Oleg Solovyov wrote:
> 29.05.2018 16:50, Vladimir 'phcoder' Serbinenko ??:
> >
> >
> > On Tue, 29 May 2018, 13:58 Oleg Solovyov,  > > wrote:
> >
> > Anybody?
> >
> > Why do you need this? Normally the same devices should be under
> > /dev/mapper. Using dm-X will mess up some other code most likely
> What happens if you have /dev/mapper/dm-0-luks symlinked to /dev/dm-0?
> This device will be skipped because basename starts with dm-0
> There's no checking whether device is under /dev/mapper or not yet.

I have just pushed the patch. Sorry for delay but I was traveling recently.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 1/9] btrfs: add support for reading a filesystem with a RAID 5 or RAID 6 profile.

[Daniel Kiper]

On Wed, May 16, 2018 at 08:48:11PM +0200, Goffredo Baroncelli wrote:
> Signed-off-by: Goffredo Baroncelli 
> ---
>  grub-core/fs/btrfs.c | 68 
>  1 file changed, 68 insertions(+)
>
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index be195448d..394611cbb 100644
> --- a/grub-core/fs/btrfs.c
> +++ b/grub-core/fs/btrfs.c
> @@ -119,6 +119,8 @@ struct grub_btrfs_chunk_item
>  #define GRUB_BTRFS_CHUNK_TYPE_RAID1 0x10
>  #define GRUB_BTRFS_CHUNK_TYPE_DUPLICATED0x20
>  #define GRUB_BTRFS_CHUNK_TYPE_RAID100x40
> +#define GRUB_BTRFS_CHUNK_TYPE_RAID5 0x80
> +#define GRUB_BTRFS_CHUNK_TYPE_RAID6 0x100
>grub_uint8_t dummy2[0xc];
>grub_uint16_t nstripes;
>grub_uint16_t nsubstripes;
> @@ -764,6 +766,72 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, 
> grub_disk_addr_t addr,
> stripe_offset = low + chunk_stripe_length
>   * high;
> csize = chunk_stripe_length - low;
> +   break;
> + }
> +   case GRUB_BTRFS_CHUNK_TYPE_RAID5:
> +   case GRUB_BTRFS_CHUNK_TYPE_RAID6:
> + {
> +   grub_uint64_t nparities, stripe_nr, high, low;
> +
> +   redundancy = 1;   /* no redundancy for now */
> +
> +   if (grub_le_to_cpu64 (chunk->type) & GRUB_BTRFS_CHUNK_TYPE_RAID5)
> + {
> +   grub_dprintf ("btrfs", "RAID5\n");
> +   nparities = 1;
> + }
> +   else
> + {
> +   grub_dprintf ("btrfs", "RAID6\n");
> +   nparities = 2;
> + }
> +
> +   /*
> +* Below an example of a RAID6 layout and the meaning of the

s/an example of a RAID6/is an example of a RAID 6/

> +* variables. The same apply to RAID5. The only differences is 
> that

s/The same apply to RAID5/The same applies to RAID 5/

> +* there is only one parity instead of two.

s/parity/parity disk/

> +*
> +* A RAID6 6 layout consists in several stripes spread

s/RAID6 6/RAID 6/
s/consists in/consists of/

> +* on the disks, following a layout like the one below

s/on the disks/over the disks/

> +*
> +*   Disk1  Disk2  Disk3  Ddisk4
> +*
> +*A1 B1 P1 Q1
> +*Q2 A2 B2 P2
> +*P3 Q3 A3 B3
> +*  [...]
> +*
> +*  Note that the placement of the parities depends on row index;

Please, please, please, do not abuse ";". Full stop is preffered at the
end of sentence. Please fix this in all patches.

> +*  In the code below:
> +*  stripe_nr -> is the stripe number not considering the parities
> +*   (A1=0, B1=1, A2 = 2, B2 = 3, ...)

May I ask you to do this like that.

The variables below have following meaning:
  - stripe_nr is the stripe number not considering the parities
(A1 = 0, B1 = 1, A2 = 2, B2 = 3, ...),
  - high is the row number (0 for A1...Q1, 1 for Q2...P2, ...),
...
  - low is the offset of the data inside a stripe.

> +*  high -> is the row number (0 for A1...Q1, 1 for Q2..P2, ...)
> +*  stripen -> is the column number (or disk number)
> +*  off -> logical address to read (from the beginning of the
> +* chunk space)

What do you mean by "chunk"? Is it e.g. A1 + B1 region? Please make it
clear what do you mean by "chunk".

> +*  chunk_stripe_length -> size of a stripe (typically 64k)
> +*  nstripes -> number of disks
> +*  low -> offset of the data inside a stripe

It looks that stripe_offset and csize explanation is missing here.

> +*
> +*/
> +   stripe_nr = grub_divmod64 (off, chunk_stripe_length, &low);
> +
> +   /*
> +* In the line below stripen is evaluated without considering

s/In the line below //

> +* the parities (0 for A1, A2, A3... 1 for B1, B2...);

s/;/./

> +*/
> +   high = grub_divmod64 (stripe_nr, nstripes - nparities, &stripen);
> +
> +   /*
> +* In the line below stripen, now consider also the parities (0

s/In the line below stripen, now/Now/

> +* for A1, 1 for A2, 2 for A3); the math is done "modulo"

s/; the/. The/
s/"modulo"/modulo/

> +* number of disks

Full stop at the end of sentence please.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 2/9] btrfs: add helper to check the btrfs header.

[Daniel Kiper]

On Wed, May 16, 2018 at 08:48:12PM +0200, Goffredo Baroncelli wrote:
> This helper will be used in few places to help the debugging. As

s/in few/in a few/

> conservative approach, in case of error it is only logged. This

s/This/This is/

Otherwise LGTM.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 3/9] btrfs: move the error logging from find_device() to its callee.

[Daniel Kiper]

On Wed, May 16, 2018 at 08:48:13PM +0200, Goffredo Baroncelli wrote:
> This is a preparatory patch. The callee knows better if this

s/callee/caller/

> error is fatal or not, i.e. another available disk.

s/i.e. another available disk./e.g. another disk is available or not./

And I think that you can reverse order of the sentences above.

WRT the subject:
  s/Move the error logging logic from find_device() to the callee./
Move the error logging from find_device() to its caller/

Otherwise LGTM.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 4/9] btrfs: avoiding a rescan for a device which was already not founded.

[Daniel Kiper]

WRT the subject:
s/btrfs: avoiding a rescan for a device which was already not founded./
  btrfs: Avoid a rescan for a device which was already not found/

On Wed, May 16, 2018 at 08:48:14PM +0200, Goffredo Baroncelli wrote:
> If a device is not found, record this failure storing NULL in

s/failure storing/failure by storing/

> data->devices_attached[]. This avoid un-necessary devices rescan,
> speeding the reading in case of a degraded array.

Last sentence should sound more or less like that:

  This way we avoid unnecessary devices rescan and speedup the reads
  in case of degraded array.

Otherwise LGTM.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 5/9] btrfs: move logging code in grub_btrfs_read_logical()

[Daniel Kiper]

On Wed, May 16, 2018 at 08:48:15PM +0200, Goffredo Baroncelli wrote:
> A portion of the logging code is moved outside a for(;;). The part that is

s/outside a for/outside of for/

> left inside is the one which depends by the for(;;) index.

s/depends by the for/depends on the for/

By the way, there are 2 for(;;). You have to be clear which one.

> This is a preparatory patch: in the next one it will be possible to refactor
> the code inside the for(;;) in an another function.

I think that you can drop these sentences. However, it seems to me that you
are no giving the reason why this move is needed for subsequent patches.
Please explain that in the commit message.

And missing SOB.

The code itself LGTM.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 6/9] btrfs: refactor the code that read from disk

[Daniel Kiper]

On Wed, May 16, 2018 at 08:48:16PM +0200, Goffredo Baroncelli wrote:
> This is a preparatory patch, to help the adding of the RAID 5/6 recovery

Please move "This is a preparatory patch" sentence below...

> code. In case of availability of all disks, this code is good for all the
> RAID profiles. However in case of failure, the error handling is quite
> different. Refactoring this code increases the general readability.

s/readability/readability too/?

You can put "This is a preparatory patch" in separate line here.
Same for the other patches.

> Signed-off-by: Goffredo Baroncelli 
> ---
>  grub-core/fs/btrfs.c | 85 +---
>  1 file changed, 49 insertions(+), 36 deletions(-)
>
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index 51f300829..63651928b 100644
> --- a/grub-core/fs/btrfs.c
> +++ b/grub-core/fs/btrfs.c
> @@ -625,6 +625,47 @@ find_device (struct grub_btrfs_data *data, grub_uint64_t 
> id)
>return ctx.dev_found;
>  }
>
> +static grub_err_t
> +btrfs_read_from_chunk (struct grub_btrfs_data *data,
> +struct grub_btrfs_chunk_item *chunk,
> +grub_uint64_t stripen, grub_uint64_t stripe_offset,
> +int redundancy, grub_uint64_t csize,
> +void *buf)
> +{
> +
> +struct grub_btrfs_chunk_stripe *stripe;
> +grub_disk_addr_t paddr;
> +grub_device_t dev;
> +grub_err_t err;
> +
> +stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1);
> +/* Right now the redundancy handling is easy.
> +   With RAID5-like it will be more difficult.  */
> +stripe += stripen + redundancy;
> +
> +paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset;
> +
> +grub_dprintf ("btrfs", "stripe %" PRIxGRUB_UINT64_T
> +   " maps to 0x%" PRIxGRUB_UINT64_T "\n",
> +   stripen, stripe->offset);
> +grub_dprintf ("btrfs", "reading paddr 0x%" PRIxGRUB_UINT64_T "\n", 
> paddr);
> +
> +dev = find_device (data, stripe->device_id);
> +if (!dev)
> +  {
> + grub_dprintf ("btrfs",
> +   "couldn't find a necessary member device "
> +   "of multi-device filesystem\n");
> + grub_errno = GRUB_ERR_NONE;
> + return GRUB_ERR_READ_ERROR;
> +  }
> +
> +err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS,
> +   paddr & (GRUB_DISK_SECTOR_SIZE - 1),
> +   csize, buf);
> +return err;
> +}
> +
>  static grub_err_t
>  grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
>void *buf, grub_size_t size, int recursion_depth)
> @@ -638,7 +679,6 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, 
> grub_disk_addr_t addr,
>grub_err_t err = 0;
>struct grub_btrfs_key key_out;
>int challoc = 0;
> -  grub_device_t dev;
>struct grub_btrfs_key key_in;
>grub_size_t chsize;
>grub_disk_addr_t chaddr;
> @@ -879,42 +919,15 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, 
> grub_disk_addr_t addr,
>
>   for (i = 0; i < redundancy; i++)
> {
> - struct grub_btrfs_chunk_stripe *stripe;
> - grub_disk_addr_t paddr;
> -
> - stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1);
> - /* Right now the redundancy handling is easy.
> -With RAID5-like it will be more difficult.  */
> - stripe += stripen + i;
> -
> - paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset;
> -
> - grub_dprintf ("btrfs", "stripe %" PRIxGRUB_UINT64_T
> -   " maps to 0x%" PRIxGRUB_UINT64_T "\n",
> -   stripen, stripe->offset);
> - grub_dprintf ("btrfs", "reading paddr 0x%" PRIxGRUB_UINT64_T
> -   "\n", paddr);
> -
> - dev = find_device (data, stripe->device_id);
> - if (!dev)
> -   {
> - grub_dprintf ("btrfs",
> -   "couldn't find a necessary member device "
> -   "of multi-device filesystem\n");
> - err = grub_errno;
> - grub_errno = GRUB_ERR_NONE;
> - continue;
> -   }
> -
> - err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS,
> -   paddr & (GRUB_DISK_SECTOR_SIZE - 1),
> -   csize, buf);
> - if (!err)
> -   break;
> - grub_errno = GRUB_ERR_NONE;

If you drop this line then you change behavior of this function.
I have a feeling that this should stay as is. At least for now.
If you need this change then probably it should be a part of the
other patch.

> + err = btrfs_read_from_chunk (data, chunk, stripen,
> +  stripe_offset,
> +

Re: [PATCH 7/9] btrfs: add support for recovery for a RAID 5 btrfs profiles.

[Daniel Kiper]

On Wed, May 16, 2018 at 08:48:17PM +0200, Goffredo Baroncelli wrote:
> Signed-off-by: Goffredo Baroncelli 
> ---
>  grub-core/fs/btrfs.c | 174 ++-
>  1 file changed, 170 insertions(+), 4 deletions(-)
>
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index 63651928b..5fcaad86f 100644
> --- a/grub-core/fs/btrfs.c
> +++ b/grub-core/fs/btrfs.c
> @@ -29,6 +29,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  GRUB_MOD_LICENSE ("GPLv3+");
>
> @@ -666,6 +667,150 @@ btrfs_read_from_chunk (struct grub_btrfs_data *data,
>  return err;
>  }
>
> +struct raid56_buffer {
> +  void *buf;
> +  int  data_is_valid;
> +};
> +
> +static void
> +rebuild_raid5 (struct raid56_buffer *buffers, grub_uint64_t nstripes,
> +   grub_uint64_t csize)
> +{
> +  grub_uint64_t target = 0, i;
> +
> +  while (buffers[target].data_is_valid && target < nstripes)
> +++target;
> +
> +  if (target == nstripes)
> +{
> +  grub_dprintf ("btrfs", "called rebuild_raid5(), but all disks are 
> OK\n");
> +  return;
> +}
> +
> +  grub_dprintf ("btrfs", "rebuilding raid5 stripe #%" PRIuGRUB_UINT64_T "\n",
> + target);
> +  for (i = 0; i < nstripes; i++)
> +if (i != target)
> +  grub_crypto_xor (buffers[target].buf, buffers[target].buf, 
> buffers[i].buf,
> +   csize);
> +}
> +
> +static grub_err_t
> +raid56_read_retry (struct grub_btrfs_data *data,
> +struct grub_btrfs_chunk_item *chunk,
> +grub_uint64_t stripe_offset, grub_uint64_t stripen,
> +grub_uint64_t csize, void *buf)
> +{
> +
> +  struct raid56_buffer *buffers = NULL;
> +  grub_uint64_t nstripes = grub_le_to_cpu16 (chunk->nstripes);
> +  grub_uint64_t chunk_type = grub_le_to_cpu64 (chunk->type);
> +  grub_err_t ret = GRUB_ERR_NONE;
> +  grub_uint64_t i, failed_devices;
> +
> +  buffers = grub_zalloc (sizeof(*buffers) * nstripes);
> +  if (!buffers)
> +{
> +  ret = GRUB_ERR_OUT_OF_MEMORY;
> +  goto cleanup;
> +}
> +
> +  for (i = 0; i < nstripes; i++)
> +{
> +  buffers[i].buf = grub_zalloc (csize);
> +  if (!buffers[i].buf)
> + {
> +   ret = GRUB_ERR_OUT_OF_MEMORY;
> +   goto cleanup;
> + }
> +}
> +
> +  for (i = 0; i < nstripes; i++)
> +{
> +  struct grub_btrfs_chunk_stripe *stripe;
> +  grub_disk_addr_t paddr;
> +  grub_device_t dev;
> +  grub_err_t err2;
> +
> +  stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1);
> +  stripe += i;
> +
> +  paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset;
> +  grub_dprintf ("btrfs", "reading paddr %" PRIxGRUB_UINT64_T
> +" from stripe ID %" PRIxGRUB_UINT64_T "\n", paddr,
> +stripe->device_id);
> +
> +  dev = find_device (data, stripe->device_id);
> +  if (!dev)
> + {
> +   buffers[i].data_is_valid = 0;
> +   grub_dprintf ("btrfs", "stripe %" PRIuGRUB_UINT64_T " FAILED (dev ID 
> %"
> + PRIxGRUB_UINT64_T ")\n", i, stripe->device_id);
> +   continue;

What will happen if more than one stripe is broken?

> + }
> +
> +  err2 = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS,
> +  paddr & (GRUB_DISK_SECTOR_SIZE - 1),
> +  csize, buffers[i].buf);
> +  if (err2 == GRUB_ERR_NONE)
> + {
> +   buffers[i].data_is_valid = 1;
> +   grub_dprintf ("btrfs", "stripe %" PRIuGRUB_UINT64_T " Ok (dev ID %"
> + PRIxGRUB_UINT64_T ")\n", i, stripe->device_id);
> + }
> +  else
> + {
> +   buffers[i].data_is_valid = 0;
> +   grub_dprintf ("btrfs", "stripe %" PRIuGRUB_UINT64_T
> + " FAILED (dev ID %" PRIxGRUB_UINT64_T ")\n", i,
> + stripe->device_id);

Ditto?

> + }
> +}
> +
> +  failed_devices = 0;
> +  for (i = 0; i < nstripes; i++)
> +if (!buffers[i].data_is_valid)
> +  ++failed_devices;
> +  if (failed_devices > 1 && (chunk_type & GRUB_BTRFS_CHUNK_TYPE_RAID5))
> +{
> +  grub_dprintf ("btrfs",
> + "not enough disks for raid5: total %" PRIuGRUB_UINT64_T

s/raid5/RAID5/ Please fix this and the rest of messages in the other patches.

> + ", missing %" PRIuGRUB_UINT64_T "\n",
> + nstripes, failed_devices);
> +  ret = GRUB_ERR_READ_ERROR;
> +  goto cleanup;

Ahhh... Here it is! Perfect!

> +}
> +  else
> +{
> +  grub_dprintf ("btrfs",
> +"enough disks for raid5/6 rebuilding: total %"

s#raid5/6#RAID5# This is the patch for RAID 5. Right?
Please do not mix RAID 5 changes with RAID 6 changes.

> + PRIuGRUB_UINT64_T ", missing %" PRIuGRUB_UINT64_T "\n",
> +nstripes, failed_devices);
> +}
> +
> +  /* if these are enough, try to rebuild the data */
> +  if (chunk_type & GRUB_BTRFS_CHUNK_TYPE_RAID5)
> +{
> +  rebuil

Re: [PATCH 8/9] btrfs: make more generic the code for RAID 6 rebuilding

[Daniel Kiper]

On Wed, May 16, 2018 at 08:48:18PM +0200, Goffredo Baroncelli wrote:
> The original code which handles the recovery of a RAID 6 disks array
> assumes that all reads are multiple of 1 << GRUB_DISK_SECTOR_BITS and it
> assumes that all the I/O is done via the struct grub_diskfilter_segment.
> This is not true for the btrfs code. In order to reuse the native
> grub_raid6_recover() code, it is modified to not call
> grub_diskfilter_read_node() directly, but to call an handler passed
> as argument.

s/as argument/as an argument/

> Signed-off-by: Goffredo Baroncelli 
> ---
>  grub-core/disk/raid6_recover.c | 53 ++
>  include/grub/diskfilter.h  |  9 ++
>  2 files changed, 44 insertions(+), 18 deletions(-)
>
> diff --git a/grub-core/disk/raid6_recover.c b/grub-core/disk/raid6_recover.c
> index aa674f6ca..10ee495e2 100644
> --- a/grub-core/disk/raid6_recover.c
> +++ b/grub-core/disk/raid6_recover.c
> @@ -74,14 +74,25 @@ mod_255 (unsigned x)
>  }
>
>  static grub_err_t
> -grub_raid6_recover (struct grub_diskfilter_segment *array, int disknr, int p,
> -char *buf, grub_disk_addr_t sector, grub_size_t size)
> +raid6_recover_read_node (void *data, int disk_nr,

s/disk_nr/disknr/

> + grub_uint64_t sector,
> + void *buf, grub_size_t size)
> +{
> +struct grub_diskfilter_segment *array = data;

Please add one empty line here.

> +return grub_diskfilter_read_node (&array->nodes[disk_nr],
> +   (grub_disk_addr_t)sector,
> +   size >> GRUB_DISK_SECTOR_BITS, buf);
> +}
> +
> +grub_err_t
> +grub_raid6_recover_gen (void *data, grub_uint64_t nstripes, int disknr, int 
> p,
> + char *buf, grub_uint64_t sector, grub_size_t size,
> + raid_recover_read_t read_func, int layout)

Could you change the order of read_func and layout arguments?
I mean make read_func the last one and put the layout before it.

>  {
>int i, q, pos;
>int bad1 = -1, bad2 = -1;
>char *pbuf = 0, *qbuf = 0;
>
> -  size <<= GRUB_DISK_SECTOR_BITS;
>pbuf = grub_zalloc (size);
>if (!pbuf)
>  goto quit;
> @@ -91,17 +102,17 @@ grub_raid6_recover (struct grub_diskfilter_segment 
> *array, int disknr, int p,
>  goto quit;
>
>q = p + 1;
> -  if (q == (int) array->node_count)
> +  if (q == (int) nstripes)
>  q = 0;
>
>pos = q + 1;
> -  if (pos == (int) array->node_count)
> +  if (pos == (int) nstripes)
>  pos = 0;
>
> -  for (i = 0; i < (int) array->node_count - 2; i++)
> +  for (i = 0; i < (int) nstripes - 2; i++)
>  {
>int c;
> -  if (array->layout & GRUB_RAID_LAYOUT_MUL_FROM_POS)
> +  if (layout & GRUB_RAID_LAYOUT_MUL_FROM_POS)
>   c = pos;
>else
>   c = i;
> @@ -109,8 +120,7 @@ grub_raid6_recover (struct grub_diskfilter_segment 
> *array, int disknr, int p,
>  bad1 = c;
>else
>  {
> -  if (! grub_diskfilter_read_node (&array->nodes[pos], sector,
> -size >> GRUB_DISK_SECTOR_BITS, buf))
> +   if (!read_func(data, pos, sector, buf, size))
>  {
>grub_crypto_xor (pbuf, pbuf, buf, size);
>grub_raid_block_mulx (c, buf, size);
> @@ -128,7 +138,7 @@ grub_raid6_recover (struct grub_diskfilter_segment 
> *array, int disknr, int p,
>  }
>
>pos++;
> -  if (pos == (int) array->node_count)
> +  if (pos == (int) nstripes)
>  pos = 0;
>  }
>
> @@ -139,16 +149,14 @@ grub_raid6_recover (struct grub_diskfilter_segment 
> *array, int disknr, int p,
>if (bad2 < 0)
>  {
>/* One bad device */
> -  if ((! grub_diskfilter_read_node (&array->nodes[p], sector,
> - size >> GRUB_DISK_SECTOR_BITS, buf)))
> +  if (!read_func(data, p, sector, buf, size))
>  {
>grub_crypto_xor (buf, buf, pbuf, size);
>goto quit;
>  }
>
>grub_errno = GRUB_ERR_NONE;
> -  if (grub_diskfilter_read_node (&array->nodes[q], sector,
> -  size >> GRUB_DISK_SECTOR_BITS, buf))
> +  if (read_func(data, q, sector, buf, size))
>  goto quit;
>
>grub_crypto_xor (buf, buf, qbuf, size);
> @@ -160,14 +168,12 @@ grub_raid6_recover (struct grub_diskfilter_segment 
> *array, int disknr, int p,
>/* Two bad devices */
>unsigned c;
>
> -  if (grub_diskfilter_read_node (&array->nodes[p], sector,
> -  size >> GRUB_DISK_SECTOR_BITS, buf))
> +  if (read_func(data, p, sector, buf, size))
>  goto quit;
>
>grub_crypto_xor (pbuf, pbuf, buf, size);
>
> -  if (grub_diskfilter_read_node (&array->nodes[q], sector,
> -  size >> GRUB_DISK_SECTOR_BITS, buf))
> +  if (read_func(data, q, sector, buf, size))
> 

Re: [PATCH 9/9] btrfs: add RAID 6 recovery for a btrfs filesystem.

[Daniel Kiper]

On Wed, May 16, 2018 at 08:48:19PM +0200, Goffredo Baroncelli wrote:
> Add the RAID 6 recovery, in order to use a RAID 6 filesystem even if some
> disks (up to two) are missing. This code use the old md RAID 6 code already

s/the old md/the md/

> present in grub.
>
> Signed-off-by: Goffredo Baroncelli 
> ---
>  grub-core/fs/btrfs.c | 45 +++-
>  1 file changed, 40 insertions(+), 5 deletions(-)
>
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index 5fcaad86f..3d71b954e 100644
> --- a/grub-core/fs/btrfs.c
> +++ b/grub-core/fs/btrfs.c
> @@ -30,6 +30,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  GRUB_MOD_LICENSE ("GPLv3+");
>
> @@ -695,11 +696,35 @@ rebuild_raid5 (struct raid56_buffer *buffers, 
> grub_uint64_t nstripes,
> csize);
>  }
>
> +static grub_err_t
> +raid6_recover_read_buffer (void *data, int disk_nr,
> +grub_uint64_t addr __attribute__ ((unused)),
> +void *dest, grub_size_t size)
> +{
> +struct raid56_buffer *buffers = data;
> +
> +grub_memcpy(dest, buffers[disk_nr].buf, size);

Could we avoid this grub_memcpy() call somehow?

> +grub_errno = buffers[disk_nr].data_is_valid ? GRUB_ERR_NONE :
> +  GRUB_ERR_READ_ERROR;
> +return grub_errno;
> +}
> +
> +static void
> +rebuild_raid6 (struct raid56_buffer *buffers, grub_uint64_t nstripes,
> +   grub_uint64_t csize, grub_uint64_t parities_pos, void *dest,
> +   grub_uint64_t stripen)
> +
> +{
> +  grub_raid6_recover_gen (buffers, nstripes, stripen, parities_pos,
> +  dest, 0, csize, raid6_recover_read_buffer, 0);
> +}
> +
>  static grub_err_t
>  raid56_read_retry (struct grub_btrfs_data *data,
>  struct grub_btrfs_chunk_item *chunk,
>  grub_uint64_t stripe_offset, grub_uint64_t stripen,
> -grub_uint64_t csize, void *buf)
> +grub_uint64_t csize, void *buf, grub_uint64_t parities_pos)
>  {
>
>struct raid56_buffer *buffers = NULL;
> @@ -780,6 +805,15 @@ raid56_read_retry (struct grub_btrfs_data *data,
>ret = GRUB_ERR_READ_ERROR;
>goto cleanup;
>  }
> +  else if (failed_devices > 2 && (chunk_type & GRUB_BTRFS_CHUNK_TYPE_RAID6))
> +{
> +  grub_dprintf ("btrfs",
> + "not enough disks for raid6: total %" PRIuGRUB_UINT64_T
> + ", missing %" PRIuGRUB_UINT64_T "\n",
> + nstripes, failed_devices);
> +  ret = GRUB_ERR_READ_ERROR;
> +  goto cleanup;
> +}
>else
>  {
>grub_dprintf ("btrfs",
> @@ -796,7 +830,7 @@ raid56_read_retry (struct grub_btrfs_data *data,
>  }
>else
>  {
> -  grub_dprintf ("btrfs", "called rebuild_raid6(), NOT IMPLEMENTED\n");
> +  rebuild_raid6 (buffers, nstripes, csize, parities_pos, buf, stripen);
>  }
>
>  cleanup:
> @@ -891,8 +925,9 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, 
> grub_disk_addr_t addr,
>   int is_raid56;
>   grub_uint64_t parities_pos = 0;
>
> - is_raid56 = !!(grub_le_to_cpu64 (chunk->type) &
> -GRUB_BTRFS_CHUNK_TYPE_RAID5);
> +is_raid56 = !!(grub_le_to_cpu64 (chunk->type) &
> +(GRUB_BTRFS_CHUNK_TYPE_RAID5|

Space before "|" please?

> + GRUB_BTRFS_CHUNK_TYPE_RAID6));
>
>   if (grub_le_to_cpu64 (chunk->size) <= off)
> {
> @@ -1089,7 +1124,7 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, 
> grub_disk_addr_t addr,
>csize, buf);
>   if (err != GRUB_ERR_NONE)
> err = raid56_read_retry (data, chunk, stripe_offset,
> -stripen, csize, buf);
> +stripen, csize, buf, parities_pos);

I am not sure what is parities_pos and where it is initialized.
If it is a part of earlier patch but it is not used then I think
that it should be a part of this patch.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: online doc currency?

[Daniel Kiper]

On Mon, May 21, 2018 at 01:26:25AM -0400, Felix Miata wrote:
> https://www.gnu.org/software/grub/manual/grub/html_node/Command_002dline-and-menu-entry-commands.html
> shows only a small fraction of what I see at a grub> prompt, where there are 
> 19
> items starting with the string "net_" alone. Is there a more up-to-date list
> online somewhere else?
>
> The reason I ask, is because at a grub> prompt, only a small fraction of
> commands listed by the help command can be seen, starting with net_add_route 
> and
> ending with zfskey. Is there a way at a grub> prompt to get at the rest of the
> help listing?

I think that "pager=1" at GRUB prompt should help.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: FTDI serial console support?

[Daniel Kiper]

On Sat, May 26, 2018 at 07:21:24AM +0200, Petter Gustad wrote:
> From: Andrew Worsley 
> Subject: Re: FTDI serial console support?
> Date: Sat, 19 May 2018 23:12:42 +1000
>
> > On 19 May 2018 at 00:32, Petter Gustad  wrote:
> >> From: Daniel Kiper 
> >> Subject: Re: FTDI serial console support?
> >> Date: Fri, 18 May 2018 13:55:55 +0200
> >>
> >>> On Fri, May 18, 2018 at 01:01:41PM +0200, Petter Gustad wrote:
> >>>> From: Daniel Kiper 
> >>>> Subject: Re: FTDI serial console support?
> >>>> Date: Wed, 16 May 2018 11:32:19 +0200
> >>>>
> >>>> > On Wed, May 16, 2018 at 06:21:17AM +0200, Petter Gustad wrote:
> >>>> >> From: Daniel Kiper 
> >>>> >> Subject: Re: FTDI serial console support?
> >>>> >> Date: Tue, 15 May 2018 22:00:06 +0200
> >>>> >>
> >>>> >> > On Fri, May 11, 2018 at 03:55:36PM +0200, Petter Gustad wrote:
> >>>> >> >>
> >>>> >> >> Is there a way to specify a FTDI based serial console using
> >>>> >> >> GRUB_TERMINAL, GRUB_SERIAL_COMMAND, or some other variable? If yes,
> >>>> >> >> what is the syntax?
> >>>> >> >>
> >>>> >> >> BTW my serial port adapter matches the vendor id and device id 
> >>>> >> >> given
> >>>> >> >> in grub-core/bus/usb/serial/ftdi.c:
> >>>> >> >>
> > 
> >>> Could you just do "insmod usbserial_ftdi"? There is a chance that it
> >>> will pull in all needed dependencies without breaking disk access.
> >>
> >> Thanks. Only "insmod usbserial_ftdi" does work in the sense that it
> >> will not break disk access. I also managed to load grub using PXE/tftp
> >> where I was not dependend upon disk access.
> >>
> >> But in both cases it does not work like in the coreboot URL above.
> >> After do insmod on usbserial_ftdi and usbserial_usbdebug the
> >> terminal_output command will only return console as its active output
> >> terminal. It will list serial_* as available output terminals, but I
> >> can't select usb0, usb1, etc as argument to the serial command.
> >
> > I think GRUB relies on the BIOS/UEIF to provide drivers and typically
> > USB  was only
> > used to emulated old hardware like keyboard/mouse  and CD-ROMs.
> >
> > Coreboot - is a BIOS/UEIF replacement and so it can do anything they
> > care to implement - but then it has to be
> > customised for each individual hardware case where as GRUB can rely on
> > the standardised routines.
> >
> > But perhaps things have moved on - it would be great if UEIF provided
> > some standard USB drivers for serial.
>
> Thank you Andrew and Daniel for information and suggestions. Seems

You are welcome!

> like I have to figure out what drivers are provided with UEIF. Or
> simply just get another motherboard with a 8250/16550 compatible
> serial port.

I think that this is the simplest solution at this point. Anyway,
I will try to take a look at this issue when I less busy.

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

[Daniel Kiper]

On Fri, May 25, 2018 at 12:02:03PM -0700, Andrew Jeddeloh wrote:
> Oops, my bad, didn't mean to drop the ML.

No problem. Sometimes it happens.

> So you're saying the setup header could expand to include some of pad7
> in the future, but we error if it goes beyond that (into the e820

Yep!

> map)? Looking at bootparam.h it looks like the _most_ correct thing
> would be to stop at edd_mbr_sig_buffer, but grub doesn't have a

Exactly!

> corresponding field so e820_map seems good enough. Thanks for working

I would suggest to add edd_mbr_sig_buffer[] to linux_kernel_params.
It does not cost a lot.

> through this.
>
> Here's the revised patch

Next time please post the patch in separate email.

> >From d46c75b4a9151c10e7f7809637f9f42a2c393c25 Mon Sep 17 00:00:00 2001
> From: Andrew Jeddeloh 
> Date: Tue, 8 May 2018 14:39:01 -0700
> Subject: [PATCH] loader/i386/linux: calculate setup header length
>
> 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 | 14 +-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/grub-core/loader/i386/linux.c b/grub-core/loader/i386/linux.c
> index 44301e126..b5b65ea36 100644
> --- a/grub-core/loader/i386/linux.c
> +++ b/grub-core/loader/i386/linux.c
> @@ -805,7 +805,19 @@ 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 byte value
> +   * at 0x201. */
> +  len = *((char *)&linux_params.jump + 1) + 0x202;

Let's be in line with the comment:
  len = 0x202 + *((char *) &linux_params.jump + 1);

> +  /* Verify the struct is big enough so we do not write past the end */
> +  if (len > (char *)&linux_params.e820_map - (char *)&linux_params) {

Lack of space between "(char *)" and "&".

> +grub_error (GRUB_ERR_BAD_OS, "boot params setup header too big");
> +goto fail;
> +  }
> +
> +  /* We've already read lh so there is no need to read it second time. */
> +  len -= sizeof(lh);
> +
>if (grub_file_read (file, (char *) &linux_params + sizeof (lh), len) != 
> len)
>  {
>if (!grub_errno)

Otherwise, +/- edd_mbr_sig_buffer[], the patch LGTM.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] multiboot2: clarify usage of the address tag

[Daniel Kiper]

On Tue, Jun 05, 2018 at 11:55:36AM +0200, Roger Pau Monne wrote:
> Add a note to spell out that if the address tag is not present the
> file should be loaded using the elf header.
>
> Signed-off-by: Roger Pau Monné 
> ---
> Cc: Daniel Kiper 
> Cc: xen-de...@lists.xenproject.org
> ---
>  doc/multiboot.texi | 6 ++
>  1 file changed, 6 insertions(+)
>
> diff --git a/doc/multiboot.texi b/doc/multiboot.texi
> index 2e2d7e74a..196f9c17a 100644
> --- a/doc/multiboot.texi
> +++ b/doc/multiboot.texi
> @@ -509,6 +509,12 @@ assumes that no bss segment is present.
>
>  @end table
>
> +Note: This information does not need to be provided if the kernel
> +image is in elf format, but it must be provided if the image is in

s/elf/@sc{elf}/

> +a.out format or in some other format. Compliant boot loaders must be
> +able to load images that are either in elf format or contain the

Ditto.

> +address tag embedded in the Multiboot header.

s/Multiboot/Multiboot2/

I think that it is also worth mentioning that the address tag has
preference over relevant data provided in ELF header.

Additionally, may I ask you to provide similar patch for Multiboot spec?
You can find it in multiboot branch. Please look for "The address fields
of Multiboot header" paragraph.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] multiboot2: clarify usage of the address tag

[Daniel Kiper]

On Thu, Jun 07, 2018 at 04:46:37PM +0200, Roger Pau Monné wrote:
> On Wed, Jun 06, 2018 at 07:28:20PM +0200, Daniel Kiper wrote:
> > On Tue, Jun 05, 2018 at 11:55:36AM +0200, Roger Pau Monne wrote:
> > > Add a note to spell out that if the address tag is not present the
> > > file should be loaded using the elf header.
> > >
> > > Signed-off-by: Roger Pau Monné 
> > > ---
> > > Cc: Daniel Kiper 
> > > Cc: xen-de...@lists.xenproject.org
> > > ---
> > >  doc/multiboot.texi | 6 ++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/doc/multiboot.texi b/doc/multiboot.texi
> > > index 2e2d7e74a..196f9c17a 100644
> > > --- a/doc/multiboot.texi
> > > +++ b/doc/multiboot.texi
> > > @@ -509,6 +509,12 @@ assumes that no bss segment is present.
> > >
> > >  @end table
> > >
> > > +Note: This information does not need to be provided if the kernel
> > > +image is in elf format, but it must be provided if the image is in
> >
> > s/elf/@sc{elf}/
> >
> > > +a.out format or in some other format. Compliant boot loaders must be
> > > +able to load images that are either in elf format or contain the
> >
> > Ditto.
> >
> > > +address tag embedded in the Multiboot header.
> >
> > s/Multiboot/Multiboot2/
> >
> > I think that it is also worth mentioning that the address tag has
> > preference over relevant data provided in ELF header.
> >
> > Additionally, may I ask you to provide similar patch for Multiboot spec?
> > You can find it in multiboot branch. Please look for "The address fields
> > of Multiboot header" paragraph.
>
> Multiboot1 already has such paragraph in the "3.1.2 The magic fields
> of Multiboot header" section.

Yep, you are right. So, please just send me updated Multiboot2 spec patch.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2] multiboot2: clarify usage of the address tag

[Daniel Kiper]

On Thu, Jun 07, 2018 at 05:59:06PM +0200, Roger Pau Monne wrote:
> Add a note to spell out that if the address tag is not present the
> file should be loaded using the elf header.
>
> Signed-off-by: Roger Pau Monné 
> ---
> Cc: Daniel Kiper 
> Cc: xen-de...@lists.xenproject.org
> ---
> Changes since v1:
>  - s/elf/@sc{elf}/
>  - s/Multiboot/Multiboot2/
> ---
>  doc/multiboot.texi | 6 ++
>  1 file changed, 6 insertions(+)
>
> diff --git a/doc/multiboot.texi b/doc/multiboot.texi
> index 2e2d7e74a..3c797787c 100644
> --- a/doc/multiboot.texi
> +++ b/doc/multiboot.texi
> @@ -509,6 +509,12 @@ assumes that no bss segment is present.
>
>  @end table
>
> +Note: This information does not need to be provided if the kernel image
> +is in @sc{elf} format, but it must be provided if the image is in a.out
> +format or in some other format. Compliant boot loaders must be able to
> +load images that are either in @sc{elf} format or contain the address
> +tag embedded in the Multiboot2 header.
> +

Now it is better. However, there is a lack of information that this tag
should be preferred over the relevant data provided in the ELF header if
both are available in the image. This have to be clear like it is in
Multiboot spec.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2] multiboot2: clarify usage of the address tag

[Daniel Kiper]

On Fri, Jun 08, 2018 at 12:08:22PM +0200, Roger Pau Monné wrote:
> On Fri, Jun 08, 2018 at 11:35:52AM +0200, Daniel Kiper wrote:
> > On Thu, Jun 07, 2018 at 05:59:06PM +0200, Roger Pau Monne wrote:
> > > Add a note to spell out that if the address tag is not present the
> > > file should be loaded using the elf header.
> > >
> > > Signed-off-by: Roger Pau Monné 
> > > ---
> > > Cc: Daniel Kiper 
> > > Cc: xen-de...@lists.xenproject.org
> > > ---
> > > Changes since v1:
> > >  - s/elf/@sc{elf}/
> > >  - s/Multiboot/Multiboot2/
> > > ---
> > >  doc/multiboot.texi | 6 ++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/doc/multiboot.texi b/doc/multiboot.texi
> > > index 2e2d7e74a..3c797787c 100644
> > > --- a/doc/multiboot.texi
> > > +++ b/doc/multiboot.texi
> > > @@ -509,6 +509,12 @@ assumes that no bss segment is present.
> > >
> > >  @end table
> > >
> > > +Note: This information does not need to be provided if the kernel image
> > > +is in @sc{elf} format, but it must be provided if the image is in a.out
> > > +format or in some other format. Compliant boot loaders must be able to
> > > +load images that are either in @sc{elf} format or contain the address
> > > +tag embedded in the Multiboot2 header.
> > > +
> >
> > Now it is better. However, there is a lack of information that this tag
> > should be preferred over the relevant data provided in the ELF header if
> > both are available in the image. This have to be clear like it is in
> > Multiboot spec.
>
> Would you be OK with adding the following sentence at the end:
>
> "When the address tag is present it must be used in order to load the
> image, regardless of whether an @sc{elf} header is also present."

I would put this as a second sentence in note, just after "... some
other format." However, then probably last sentence should be rephrased
a bit.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2] multiboot2: clarify usage of the address tag

[Daniel Kiper]

On Mon, Jun 11, 2018 at 11:30:16AM +0200, Roger Pau Monné wrote:

[...]

> I think the following is clear enough:
>
> "Note: This information does not need to be provided if the kernel image is in
> @sc{elf} format, but it must be provided if the image is in a.out format or in
> some other format. When the address tag is present it must be used in order to
> load the image, regardless of whether an @sc{elf} header is also present.
> Compliant boot loaders must be able to load images that are either in @sc{elf}
> format or contain the address tag embedded in the Multiboot2 header."

LGTM. Thanks!

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v3] multiboot2: clarify usage of the address tag

[Daniel Kiper]

On Mon, Jun 11, 2018 at 01:49:02PM +0200, Roger Pau Monne wrote:
> Add a note to spell out that if the address tag is not present the
> file should be loaded using the elf header.
>
> Signed-off-by: Roger Pau Monné 

Reviewed-by: Daniel Kiper 

If there are no objections I will apply this in a week or so.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2] mbi: use per segment a separate relocator chunk

[Daniel Kiper]

On Tue, Jun 05, 2018 at 10:14:49PM +0200, Alexander Boettcher wrote:
> Instead of setting up a all comprising relocator chunk for all segments,
> use per segment a separate relocator chunk.
>
> If the ELF is non-relocatable, a single relocator chunk will comprise memory
> (between the segments) which gets overridden by the relst() invocation of the
> movers code in grub_relocator16/32/64_boot().
>
> The overridden memory may contain reserved ranges like VGA memory or ACPI
> tables, which leads to strange boot behaviour.
>
> Signed-off-by: Alexander Boettcher 
> ---
>  grub-core/loader/multiboot_elfxx.c | 60 
> +++---
>  1 file changed, 37 insertions(+), 23 deletions(-)
>
> diff --git a/grub-core/loader/multiboot_elfxx.c 
> b/grub-core/loader/multiboot_elfxx.c
> index 67daf59..6565b50 100644
> --- a/grub-core/loader/multiboot_elfxx.c
> +++ b/grub-core/loader/multiboot_elfxx.c
> @@ -57,9 +57,9 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
>char *phdr_base;
>grub_err_t err;
>grub_relocator_chunk_t ch;
> -  grub_uint32_t load_offset, load_size;
> +  grub_uint32_t load_offset = 0, load_size;
>int i;
> -  void *source;
> +  void *source = NULL;
>
>if (ehdr->e_ident[EI_MAG0] != ELFMAG0
>|| ehdr->e_ident[EI_MAG1] != ELFMAG1
> @@ -83,6 +83,7 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
>  #define phdr(i)  ((Elf_Phdr *) (phdr_base + (i) * 
> ehdr->e_phentsize))
>
>mld->link_base_addr = ~0;
> +  mld->load_base_addr = ~0;

We do not need it. Please look below.

>/* Calculate lowest and highest load address.  */
>for (i = 0; i < ehdr->e_phnum; i++)
> @@ -97,10 +98,15 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
>  return grub_error (GRUB_ERR_BAD_OS, "segment crosses 4 GiB border");
>  #endif
>
> -  load_size = highest_load - mld->link_base_addr;
> +  grub_dprintf ("multiboot_loader", "link_base_addr=0x%x, relocatable=%d, "
> + "align=0x%lx, preference=0x%x, avoid_efi_boot_services=%d\n",
> + mld->link_base_addr, mld->relocatable, (long) mld->align,
> + mld->preference, mld->avoid_efi_boot_services);

I have just realized that it does not make sense to print align,
preference and avoid_efi_boot_services if mld->relocatable is not
set. Please take a look at grub-core/loader/multiboot_mbi2.c for
more details. Sorry about that. In this case you can probably drop
this grub_dprintf() and...

>if (mld->relocatable)
>  {
> +  load_size = highest_load - mld->link_base_addr;
> +
>if (load_size > mld->max_addr || mld->min_addr > mld->max_addr - 
> load_size)
>   return grub_error (GRUB_ERR_BAD_OS, "invalid min/max address and/or 
> load size");
>
> @@ -108,27 +114,16 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t 
> *mld)
> mld->min_addr, mld->max_addr - 
> load_size,
> load_size, mld->align ? 
> mld->align : 1,
> mld->preference, 
> mld->avoid_efi_boot_services);
> -}
> -  else

Please do not drop this else. You can put this here:
  mld->load_base_addr = mld->link_base_addr;

load_base_addr == link_base_addr in non relocatable case.
Am I right?

> -err = grub_relocator_alloc_chunk_addr (GRUB_MULTIBOOT (relocator), &ch,
> -mld->link_base_addr, load_size);
> -
> -  if (err)
> -{
> -  grub_dprintf ("multiboot_loader", "Cannot allocate memory for OS 
> image\n");
> -  return err;
> -}
>
> -  mld->load_base_addr = get_physical_target_address (ch);
> -  source = get_virtual_current_address (ch);
> -
> -  grub_dprintf ("multiboot_loader", "link_base_addr=0x%x, 
> load_base_addr=0x%x, "
> - "load_size=0x%x, relocatable=%d\n", mld->link_base_addr,
> - mld->load_base_addr, load_size, mld->relocatable);

...you can leave this excluding load_size and...

> +  if (err)
> +{
> +  grub_dprintf ("multiboot_loader", "Cannot allocate memory for OS 
> image\n");
> +  return err;
> +}
>
> -  if (mld->relocatable)
> -grub_dprintf ("multiboot_loader", "align=0x%lx, preference=0x%x, 
> avoid_efi_boot_services=%d\n",
> -   (long) mld->align, mld->preference, 
> mld->avoid_efi_boot_services);

...you can leave this grub_dprintf() and move load_size from above.
You can put it after preference.

> +  mld->load_base_addr = get_physical_target_address (ch);
> +  source = get_virtual_current_address (ch);
> +}
>
>/* Load every loadable segment in memory.  */
>for (i = 0; i < ehdr->e_phnum; i++)
> @@ -139,7 +134,26 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t 
> *mld)
> grub_dprintf ("multiboot_loader", "segment %d: paddr=0x%lx, 
> memsz=0x%lx, vaddr=0x%lx\n",
>   i, (long) phdr(i)->p_paddr, (long) phdr(i)->p_memsz, 
> (lon

Re: [PATCH v3] mbi: use per segment a separate relocator chunk

[Daniel Kiper]

On Tue, Jun 12, 2018 at 08:19:35PM +0200, Alexander Boettcher wrote:
> Instead of setting up a all comprising relocator chunk for all segments,
> use per segment a separate relocator chunk.
>
> If the ELF is non-relocatable, a single relocator chunk will comprise memory

s/If/Currently, if/

> (between the segments) which gets overridden by the relst() invocation of the
> movers code in grub_relocator16/32/64_boot().
>

Please drop this empty line and start next sentence immediately behind previous 
one.

> The overridden memory may contain reserved ranges like VGA memory or ACPI
> tables, which leads to strange boot behaviour.

s/leads/may lead to crashes or at least/

> Signed-off-by: Alexander Boettcher 
> ---
>  grub-core/loader/multiboot_elfxx.c | 58 
> +++---
>  1 file changed, 36 insertions(+), 22 deletions(-)
>
> diff --git a/grub-core/loader/multiboot_elfxx.c 
> b/grub-core/loader/multiboot_elfxx.c
> index 67daf59..f493b0a 100644
> --- a/grub-core/loader/multiboot_elfxx.c
> +++ b/grub-core/loader/multiboot_elfxx.c
> @@ -57,9 +57,9 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
>char *phdr_base;
>grub_err_t err;
>grub_relocator_chunk_t ch;
> -  grub_uint32_t load_offset, load_size;
> +  grub_uint32_t load_offset = 0, load_size;
>int i;
> -  void *source;
> +  void *source = NULL;
>
>if (ehdr->e_ident[EI_MAG0] != ELFMAG0
>|| ehdr->e_ident[EI_MAG1] != ELFMAG1
> @@ -97,10 +97,13 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
>  return grub_error (GRUB_ERR_BAD_OS, "segment crosses 4 GiB border");
>  #endif
>
> -  load_size = highest_load - mld->link_base_addr;
> -
>if (mld->relocatable)
>  {
> +  load_size = highest_load - mld->link_base_addr;
> +
> +  grub_dprintf ("multiboot_loader", "align=0x%lx, preference=0x%x, 
> load_size=0x%x avoid_efi_boot_services=%d\n",
> + (long) mld->align, mld->preference, load_size, 
> mld->avoid_efi_boot_services);

Too long line and lack of ",". I would like to see this:

 grub_dprintf ("multiboot_loader", "align=0x%lx, preference=0x%x, "
   "load_size=0x%x, avoid_efi_boot_services=%d\n",
   (long) mld->align, mld->preference, load_size,
   mld->avoid_efi_boot_services);

>if (load_size > mld->max_addr || mld->min_addr > mld->max_addr - 
> load_size)
>   return grub_error (GRUB_ERR_BAD_OS, "invalid min/max address and/or 
> load size");
>
> @@ -108,27 +111,21 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t 
> *mld)
> mld->min_addr, mld->max_addr - 
> load_size,
> load_size, mld->align ? 
> mld->align : 1,
> mld->preference, 
> mld->avoid_efi_boot_services);
> -}
> -  else
> -err = grub_relocator_alloc_chunk_addr (GRUB_MULTIBOOT (relocator), &ch,
> -mld->link_base_addr, load_size);
>
> -  if (err)
> -{
> -  grub_dprintf ("multiboot_loader", "Cannot allocate memory for OS 
> image\n");
> -  return err;
> -}
> +  if (err)
> +{
> +  grub_dprintf ("multiboot_loader", "Cannot allocate memory for OS 
> image\n");
> +  return err;
> +}
>
> -  mld->load_base_addr = get_physical_target_address (ch);
> -  source = get_virtual_current_address (ch);
> +  mld->load_base_addr = get_physical_target_address (ch);
> +  source = get_virtual_current_address (ch);
> +}
> +  else
> +mld->load_base_addr = mld->link_base_addr;
>
>grub_dprintf ("multiboot_loader", "link_base_addr=0x%x, 
> load_base_addr=0x%x, "
> - "load_size=0x%x, relocatable=%d\n", mld->link_base_addr,
> - mld->load_base_addr, load_size, mld->relocatable);
> -
> -  if (mld->relocatable)
> -grub_dprintf ("multiboot_loader", "align=0x%lx, preference=0x%x, 
> avoid_efi_boot_services=%d\n",
> -   (long) mld->align, mld->preference, 
> mld->avoid_efi_boot_services);
> + "relocatable=%d\n", mld->link_base_addr, mld->load_base_addr, 
> mld->relocatable);

I think that this would be much better:
   grub_dprintf ("multiboot_loader", "relocatable=%d, link_base_addr=0x%x, "
 "load_base_addr=0x%x\n", mld->relocatable,
 mld->link_base_addr, mld->load_base_addr);

As I you can see mostly nitpicks. So, I think that after next
iteration everything will be OK.

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 1/9] btrfs: Add support for reading a filesystem with a RAID 5 or RAID 6 profile.

[Daniel Kiper]

On Sun, Jun 03, 2018 at 08:53:40PM +0200, Goffredo Baroncelli wrote:
> Signed-off-by: Goffredo Baroncelli 
> ---
>  grub-core/fs/btrfs.c | 70 
>  1 file changed, 70 insertions(+)
>
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index be195448d..4d418859b 100644
> --- a/grub-core/fs/btrfs.c
> +++ b/grub-core/fs/btrfs.c
> @@ -119,6 +119,8 @@ struct grub_btrfs_chunk_item
>  #define GRUB_BTRFS_CHUNK_TYPE_RAID1 0x10
>  #define GRUB_BTRFS_CHUNK_TYPE_DUPLICATED0x20
>  #define GRUB_BTRFS_CHUNK_TYPE_RAID100x40
> +#define GRUB_BTRFS_CHUNK_TYPE_RAID5 0x80
> +#define GRUB_BTRFS_CHUNK_TYPE_RAID6 0x100
>grub_uint8_t dummy2[0xc];
>grub_uint16_t nstripes;
>grub_uint16_t nsubstripes;
> @@ -764,6 +766,74 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, 
> grub_disk_addr_t addr,
> stripe_offset = low + chunk_stripe_length
>   * high;
> csize = chunk_stripe_length - low;
> +   break;
> + }
> +   case GRUB_BTRFS_CHUNK_TYPE_RAID5:
> +   case GRUB_BTRFS_CHUNK_TYPE_RAID6:
> + {
> +   grub_uint64_t nparities, stripe_nr, high, low;
> +
> +   redundancy = 1;   /* no redundancy for now */
> +
> +   if (grub_le_to_cpu64 (chunk->type) & GRUB_BTRFS_CHUNK_TYPE_RAID5)
> + {
> +   grub_dprintf ("btrfs", "RAID5\n");
> +   nparities = 1;
> + }
> +   else
> + {
> +   grub_dprintf ("btrfs", "RAID6\n");
> +   nparities = 2;
> + }
> +
> +   /*
> +* Below is an example of a RAID 6 layout and the meaning of the
> +* variables. The same applies to RAID 5. The only differences is
> +* that there is only one parity disk instead of two.
> +*
> +* A RAID 6 layout consists of several stripes spread
> +* on the disks, following a layout like the one below
> +*
> +*   Disk1  Disk2  Disk3  Ddisk4

Numbering seems confusing to me. I think that it should be
   Disk0  Disk1  Disk2  Disk3

> +*
> +*A1 B1 P1 Q1
> +*Q2 A2 B2 P2
> +*P3 Q3 A3 B3
> +*  [...]
> +*
> +*  Note that the placement of the parities depends on row index.
> +*  In the code below:
> +*  - stripe_nr is the stripe number not considering the parities
> +*(A1=0, B1=1, A2 = 2, B2 = 3, ...),

Please be consistent. A1 = 0, B1 = 1, A2 = 2, B2 = 3, ...

> +*  - high is the row number (0 for A1...Q1, 1 for Q2..P2, ...),

Ditto. Please always use "..." not "..".

> +*  - stripen is the column number (or disk number),

AIUI starting from 0. Right? If yes then I think that
drawing above requires disks/columns renumbering.

> +*  - off is the logical address to read (from the beginning of
> +*the chunk space),

s/chunk space/chunk/?

> +*  - chunk_stripe_length is the size of a stripe (typically 64k),
> +*  - nstripes is the number of disks,
> +*  - low is the offset of the data inside a stripe,
> +*  - stripe_offset is the offset from the beginning of the chunk
> +*disks physical address,

I am not sure that I understand. Could clarify this?

> +*  - csize is the "potential" data to read. It will be reduced to
> +*size if the latter is smaller.
> +*/
> +   stripe_nr = grub_divmod64 (off, chunk_stripe_length, &low);

OK.

> +   /*
> +* stripen is evaluated without considering
> +* the parities (0 for A1, A2, A3... 1 for B1, B2...).
> +*/
> +   high = grub_divmod64 (stripe_nr, nstripes - nparities, &stripen);

OK.

> +   /*
> +* stripen now considers also the parities (0 for A1, 1 for A2,
> +* 2 for A3). The math is performed modulo number of disks.
> +*/
> +   grub_divmod64 (high + stripen, nstripes, &stripen);

OK.

> +   stripe_offset = low + chunk_stripe_length * high;

Hmmm... I am confused. What does it mean?

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 2/9] btrfs: Add helper to check the btrfs header.

[Daniel Kiper]

On Sun, Jun 03, 2018 at 08:53:41PM +0200, Goffredo Baroncelli wrote:
> This helper will be used in a few places to help the debugging. As
> conservative approach, in case of error it is only logged. This is

s/, in case of error it/ the error/

> because I am not sure if this can change something in the error
> handling of the currently existing code.

s/code/code or not/

Otherwise LGTM.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 3/9] btrfs: Move the error logging from find_device() to its caller.

[Daniel Kiper]

On Sun, Jun 03, 2018 at 08:53:42PM +0200, Goffredo Baroncelli wrote:
> This is a preparatory patch. The caller knows better if this
> error is fatal or not, i.e. another disk is available or not.

Please make first sentence last one in separate line.
The same applies to other patches.

Otherwise LGTM.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: Fwd: [PATCH 4/9] btrfs: Avoid a rescan for a device which was already not found.

[Daniel Kiper]

On Mon, Jun 04, 2018 at 09:26:06PM +0200, Goffredo Baroncelli wrote:
> Resend this patch because I am not sure that all received it.

It looks that #4 is a bit unfortunate for you and/or patch series... :-)))

> BR
> G.Baroncelli
>
>
>  Forwarded Message 
> Subject: [PATCH 4/9] btrfs: Avoid a rescan for a device which was already not 
> found.
> Date: Sun,  3 Jun 2018 20:53:43 +0200
> From: Goffredo Baroncelli 
> To: grub-devel@gnu.org
> CC: Goffredo Baroncelli 
>
> If a device is not found, record this failure by storing NULL in
> data->devices_attached[]. This way we avoid unnecessary devices rescan,

Hmmm... Could you point me out where this store happens below?

Daniel

> and speedup the reads in case of a degraded array.
>
> Signed-off-by: Goffredo Baroncelli 
> ---
>  grub-core/fs/btrfs.c | 19 +--
>  1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index 9b3a22772..b64b692f8 100644
> --- a/grub-core/fs/btrfs.c
> +++ b/grub-core/fs/btrfs.c
> @@ -588,7 +588,7 @@ find_device_iter (const char *name, void *data)
>  }
>
>  static grub_device_t
> -find_device (struct grub_btrfs_data *data, grub_uint64_t id, int do_rescan)
> +find_device (struct grub_btrfs_data *data, grub_uint64_t id)
>  {
>struct find_device_ctx ctx = {
>  .data = data,
> @@ -600,12 +600,9 @@ find_device (struct grub_btrfs_data *data, grub_uint64_t 
> id, int do_rescan)
>for (i = 0; i < data->n_devices_attached; i++)
>  if (id == data->devices_attached[i].id)
>return data->devices_attached[i].dev;
> -  if (do_rescan)
> -grub_device_iterate (find_device_iter, &ctx);
> -  if (!ctx.dev_found)
> -{
> -  return NULL;
> -}
> +
> +  grub_device_iterate (find_device_iter, &ctx);
> +
>data->n_devices_attached++;
>if (data->n_devices_attached > data->n_devices_allocated)
>  {
> @@ -617,7 +614,8 @@ find_device (struct grub_btrfs_data *data, grub_uint64_t 
> id, int do_rescan)
>   * sizeof (data->devices_attached[0]));
>if (!data->devices_attached)
>   {
> -   grub_device_close (ctx.dev_found);
> +   if (ctx.dev_found)
> + grub_device_close (ctx.dev_found);
> data->devices_attached = tmp;
> return NULL;
>   }
> @@ -896,7 +894,7 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, 
> grub_disk_addr_t addr,
> " for laddr 0x%" PRIxGRUB_UINT64_T "\n", paddr,
> addr);
>
> - dev = find_device (data, stripe->device_id, j);
> + dev = find_device (data, stripe->device_id);
>   if (!dev)
> {
>   grub_dprintf ("btrfs",
> @@ -973,7 +971,8 @@ grub_btrfs_unmount (struct grub_btrfs_data *data)
>unsigned i;
>/* The device 0 is closed one layer upper.  */
>for (i = 1; i < data->n_devices_attached; i++)
> -grub_device_close (data->devices_attached[i].dev);
> +if (data->devices_attached[i].dev)
> +grub_device_close (data->devices_attached[i].dev);
>grub_free (data->devices_attached);
>grub_free (data->extent);
>grub_free (data);
> --
> 2.17.1
>
>
>
> ___
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 5/9] btrfs: Move logging code in grub_btrfs_read_logical()

[Daniel Kiper]

On Sun, Jun 03, 2018 at 08:53:44PM +0200, Goffredo Baroncelli wrote:
> A portion of the logging code is moved outside of internal for(;;). The part
> that is left inside is the one which depends by the internal for(;;) index.

s/depends by/depends on/

> This is a preparatory patch: in the next one it will be possible to refactor

s/: in the next one it will be possible to/. The next one will/

> the code inside the for(;;) in an another function.

s/in/into/

> Signed-off-by: Goffredo Baroncelli 
> ---
>  grub-core/fs/btrfs.c | 25 ++---
>  1 file changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index b64b692f8..e2baed665 100644
> --- a/grub-core/fs/btrfs.c
> +++ b/grub-core/fs/btrfs.c
> @@ -867,6 +867,18 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, 
> grub_disk_addr_t addr,
>
>   for (j = 0; j < 2; j++)
> {
> + grub_dprintf ("btrfs", "chunk 0x%" PRIxGRUB_UINT64_T
> +   "+0x%" PRIxGRUB_UINT64_T
> +   " (%d stripes (%d substripes) of %"
> +   PRIxGRUB_UINT64_T ") \n",

s/") \n"/")\n"/

Otherwise LGTM.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 6/9] btrfs: Refactor the code that read from disk

[Daniel Kiper]

On Sun, Jun 03, 2018 at 08:53:45PM +0200, Goffredo Baroncelli wrote:
> Move the code in charge to read the data from disk in a separate

s/in a/into a/

> function. This helps to separate the error handling logic (which depend by

s/depend by/depends on/ Please fix this here and in other patches too.

> the different raid profiles) from the read from disk logic.
> Refactoring this code increases the general readability too.
>
> This is a preparatory patch, to help the adding of the RAID 5/6 recovery
> code.
>
> Signed-off-by: Goffredo Baroncelli 
> ---
>  grub-core/fs/btrfs.c | 76 ++--
>  1 file changed, 45 insertions(+), 31 deletions(-)
>
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index e2baed665..9cdbfe792 100644
> --- a/grub-core/fs/btrfs.c
> +++ b/grub-core/fs/btrfs.c
> @@ -625,6 +625,47 @@ find_device (struct grub_btrfs_data *data, grub_uint64_t 
> id)
>return ctx.dev_found;
>  }
>
> +static grub_err_t
> +btrfs_read_from_chunk (struct grub_btrfs_data *data,
> +struct grub_btrfs_chunk_item *chunk,
> +grub_uint64_t stripen, grub_uint64_t stripe_offset,
> +int redundancy, grub_uint64_t csize,
> +void *buf)
> +{
> +

Please drop this empty line.

> +struct grub_btrfs_chunk_stripe *stripe;
> +grub_disk_addr_t paddr;
> +grub_device_t dev;
> +grub_err_t err;
> +
> +stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1);
> +/* Right now the redundancy handling is easy.
> +   With RAID5-like it will be more difficult.  */
> +stripe += stripen + redundancy;
> +
> +paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset;
> +
> +grub_dprintf ("btrfs", "stripe %" PRIxGRUB_UINT64_T
> +   " maps to 0x%" PRIxGRUB_UINT64_T "\n",
> +   stripen, stripe->offset);
> +grub_dprintf ("btrfs", "reading paddr 0x%" PRIxGRUB_UINT64_T "\n", 
> paddr);

Could you put this into one grub_dprintf() call?

> +
> +dev = find_device (data, stripe->device_id);
> +if (!dev)
> +  {
> + grub_dprintf ("btrfs",
> +   "couldn't find a necessary member device "
> +   "of multi-device filesystem\n");
> + grub_errno = GRUB_ERR_NONE;
> + return GRUB_ERR_READ_ERROR;

Why grub_errno = GRUB_ERR_NONE and then return GRUB_ERR_READ_ERROR?
If you do things like that I think that you should add a few words
of comment before such code. Otherwise it is confusing.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 7/9] btrfs: Add support for recovery for a RAID 5 btrfs profiles.

[Daniel Kiper]

On Sun, Jun 03, 2018 at 08:53:46PM +0200, Goffredo Baroncelli wrote:
> Add support for recovery fo a RAID 5 btrfs profile. In addition

s/fo /for /

> it is added some code as preparatory work for RAID 6 recovery code.
>
> Signed-off-by: Goffredo Baroncelli 
> ---
>  grub-core/fs/btrfs.c | 180 +--
>  1 file changed, 175 insertions(+), 5 deletions(-)
>
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index 9cdbfe792..c8f034641 100644
> --- a/grub-core/fs/btrfs.c
> +++ b/grub-core/fs/btrfs.c
> @@ -29,6 +29,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  GRUB_MOD_LICENSE ("GPLv3+");
>
> @@ -666,6 +667,157 @@ btrfs_read_from_chunk (struct grub_btrfs_data *data,
>  return err;
>  }
>
> +struct raid56_buffer {
> +  void *buf;
> +  int  data_is_valid;
> +};
> +
> +static void
> +rebuild_raid5 (char *dest, struct raid56_buffer *buffers,
> +grub_uint64_t nstripes, grub_uint64_t csize)
> +{
> +  grub_uint64_t i;

grub_uint64_t i = 0;
int first = 1;

> +  int first;
> +
> +  i = 0;

Then you can drop this assignment.

> +  while (buffers[i].data_is_valid && i < nstripes)
> +++i;
> +
> +  if (i == nstripes)
> +{
> +  grub_dprintf ("btrfs", "called rebuild_raid5(), but all disks are 
> OK\n");
> +  return;
> +}
> +
> +  grub_dprintf ("btrfs", "rebuilding RAID 5 stripe #%" PRIuGRUB_UINT64_T 
> "\n",
> + i);

This can be in one line.

> +  first = 1;

And you can drop this assignment too.

> +  for (i = 0; i < nstripes; i++)
> +{
> +  if (!buffers[i].data_is_valid)
> + continue;
> +
> +  if (first)
> + grub_memcpy(dest, buffers[i].buf, csize);
> +  else
> + grub_crypto_xor (dest, dest, buffers[i].buf, csize);

I am not sure why at first you use grub_memcpy() and
then move to grub_crypto_xor(). Could you explain this?
Why do not use grub_crypto_xor() in all cases?

> +
> +  first = 0;
> +}
> +}
> +
> +static grub_err_t
> +raid56_read_retry (struct grub_btrfs_data *data,
> +struct grub_btrfs_chunk_item *chunk,
> +grub_uint64_t stripe_offset,
> +grub_uint64_t csize, void *buf)
> +{
> +

Please drop this empty line. I have asked about that earlier.

> +  struct raid56_buffer *buffers = NULL;
> +  grub_uint64_t nstripes = grub_le_to_cpu16 (chunk->nstripes);
> +  grub_uint64_t chunk_type = grub_le_to_cpu64 (chunk->type);
> +  grub_err_t ret = GRUB_ERR_NONE;
> +  grub_uint64_t i, failed_devices;
> +
> +  buffers = grub_zalloc (sizeof(*buffers) * nstripes);

How often this function is called? Maybe you should consider
doing memory allocation for this function only once and free
it at btrfs module unload.

> +  if (!buffers)
> +{
> +  ret = GRUB_ERR_OUT_OF_MEMORY;
> +  goto cleanup;
> +}
> +
> +  for (i = 0; i < nstripes; i++)
> +{
> +  buffers[i].buf = grub_zalloc (csize);

Ditto.

> +  if (!buffers[i].buf)
> + {
> +   ret = GRUB_ERR_OUT_OF_MEMORY;
> +   goto cleanup;
> + }
> +}
> +
> +  for (i = 0; i < nstripes; i++)
> +{
> +  struct grub_btrfs_chunk_stripe *stripe;
> +  grub_disk_addr_t paddr;
> +  grub_device_t dev;
> +  grub_err_t err2;
> +
> +  stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1);
> +  stripe += i;
> +
> +  paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset;
> +  grub_dprintf ("btrfs", "reading paddr %" PRIxGRUB_UINT64_T
> +" from stripe ID %" PRIxGRUB_UINT64_T "\n", paddr,
> +stripe->device_id);
> +
> +  dev = find_device (data, stripe->device_id);
> +  if (!dev)
> + {
> +   buffers[i].data_is_valid = 0;
> +   grub_dprintf ("btrfs", "stripe %" PRIuGRUB_UINT64_T " FAILED (dev ID 
> %"
> + PRIxGRUB_UINT64_T ")\n", i, stripe->device_id);
> +   continue;
> + }
> +
> +  err2 = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS,
> +  paddr & (GRUB_DISK_SECTOR_SIZE - 1),
> +  csize, buffers[i].buf);
> +  if (err2 == GRUB_ERR_NONE)
> + {
> +   buffers[i].data_is_valid = 1;
> +   grub_dprintf ("btrfs", "stripe %" PRIuGRUB_UINT64_T " Ok (dev ID %"
> + PRIxGRUB_UINT64_T ")\n", i, stripe->device_id);
> + }
> +  else
> + {
> +   buffers[i].data_is_valid = 0;
> +   grub_dprintf ("btrfs", "stripe %" PRIuGRUB_UINT64_T
> + " FAILED (dev ID %" PRIxGRUB_UINT64_T ")\n", i,
> + stripe->device_id);
> + }
> +}
> +
> +  failed_devices = 0;
> +  for (i = 0; i < nstripes; i++)

for (failed_devices = i = 0; i < nstripes; i++)

> +if (!buffers[i].data_is_valid)
> +  ++failed_devices;
> +  if (failed_devices > 1 && (chunk_type & GRUB_BTRFS_CHUNK_TYPE_RAID5))
> +{
> +  grub_dprintf ("btrfs",
> + "not enough disks for RAID 5: total %" PRIuGRUB_UINT64_T
> + 

Re: [PATCH 9/9] btrfs: Add RAID 6 recovery for a btrfs filesystem.

[Daniel Kiper]

On Sun, Jun 03, 2018 at 08:53:48PM +0200, Goffredo Baroncelli wrote:
> Add the RAID 6 recovery, in order to use a RAID 6 filesystem even if some
> disks (up to two) are missing. This code use the md RAID 6 code already
> present in grub.
>
> Signed-off-by: Goffredo Baroncelli 
> ---
>  grub-core/fs/btrfs.c | 50 ++--
>  1 file changed, 44 insertions(+), 6 deletions(-)
>
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index c8f034641..0b6557ce3 100644
> --- a/grub-core/fs/btrfs.c
> +++ b/grub-core/fs/btrfs.c
> @@ -30,6 +30,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  GRUB_MOD_LICENSE ("GPLv3+");
>
> @@ -706,11 +707,35 @@ rebuild_raid5 (char *dest, struct raid56_buffer 
> *buffers,
>  }
>  }
>
> +static grub_err_t
> +raid6_recover_read_buffer (void *data, int disk_nr,
> +grub_uint64_t addr __attribute__ ((unused)),
> +void *dest, grub_size_t size)
> +{
> +struct raid56_buffer *buffers = data;
> +
> +grub_memcpy(dest, buffers[disk_nr].buf, size);
> +
> +grub_errno = buffers[disk_nr].data_is_valid ? GRUB_ERR_NONE :
> +  GRUB_ERR_READ_ERROR;
> +return grub_errno;

  if (!buffers[disk_nr].data_is_valid)
  return GRUB_ERR_READ_ERROR;

  grub_memcpy(dest, buffers[disk_nr].buf, size);

  return GRUB_ERR_NONE;

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH V5] Add support for BTRFS raid5/6 to GRUB

[Daniel Kiper]

Hi Goffredo,

On Sun, Jun 03, 2018 at 08:53:39PM +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 5 patches, are preparatory ones.
>
> The 7th patch implements the raid5 recovery for btrfs (i.e. handling the
> disappearing of 1 disk).
> The 8th patch makes the code for handling the raid6 recovery more generic.
> The last one implements the raid6 recovery for btrfs (i.e. handling the
> disappearing up to two disks).
>
> I tested the code in grub-emu, and it works both with all the disks,
> and with some disks missing. I checked the crc32 calculated from grub and
> from linux and these matched. Finally I checked if the support for md raid6
> still works properly, and it does (with all drives and with up to 2 drives
> missing)
>
> Comments are welcome.

In general I am happy that you are doing this work. However, I have just
realized that in some cases you are agreeing with my comments and then
you do not incorporate the changes which I was asking for. So, I would
be more happy if you instead of saying OK just do requested changes.
Otherwise you lose your and my time. Hence, I would like ask you to
check carefully all my comments for v4 and v5 (at least), apply all
requested changes with which you agree and then post v6.

Sorry for being blunt.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 0/2] efi/fdt: set #address-cells/#size-cells on empty tree

[Daniel Kiper]

On Mon, Jun 11, 2018 at 05:24:57PM +0100, Leif Lindholm wrote:
> Set #address-cells and #size-cells properties (to 2) for ARM*/UEFI
> systems when creating an empty DT at boot time. This resolves an issue
> seen in the wild with kexec on certain 64-bit ARM systems.
>
> First part is moving out the prop_entry_size macro from lib/fdt.c
> and make it available in  (with the required name
> change).
>
> Second part is adding the two properties to the empty tree.

In general LGTM. Though I would prefer "?" instead of "if" in patch #2.
Or at least drop curly braces. Anyway, if you are OK with proposed
changes I can incorporate them before push.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 0/2] efi/fdt: set #address-cells/#size-cells on empty tree

[Daniel Kiper]

On Thu, Jun 14, 2018 at 04:37:34PM +0100, Leif Lindholm wrote:
> On Thu, Jun 14, 2018 at 03:38:13PM +0200, Daniel Kiper wrote:
> > On Mon, Jun 11, 2018 at 05:24:57PM +0100, Leif Lindholm wrote:
> > > Set #address-cells and #size-cells properties (to 2) for ARM*/UEFI
> > > systems when creating an empty DT at boot time. This resolves an issue
> > > seen in the wild with kexec on certain 64-bit ARM systems.
> > >
> > > First part is moving out the prop_entry_size macro from lib/fdt.c
> > > and make it available in  (with the required name
> > > change).
> > >
> > > Second part is adding the two properties to the empty tree.
> >
> > In general LGTM. Though I would prefer "?" instead of "if" in patch #2.
> > Or at least drop curly braces. Anyway, if you are OK with proposed
> > changes I can incorporate them before push.
>
> I thought the added additional bits made the ternary a bit painful,
> but I'd be happy for you to get rid of the braces (TianoCore coding

Granted! I will drop the braces.

> style is making me use those more heavily :)

:-)))

If there are no objections I apply the patches in a week or so.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2] ieee1275: obdisk driver

[Daniel Kiper]

On Wed, May 30, 2018 at 04:55:22PM -0700, Eric Snowberg wrote:
> Add a new disk driver called obdisk for IEEE1275 platforms.  Currently
> the only platform using this disk driver is SPARC, however other IEEE1275
> platforms could start using it if they so choose.  While the functionality
> within the current IEEE1275 ofdisk driver may be suitable for PPC and x86, it
> presented too many problems on SPARC hardware.
>
> Within the old ofdisk, there is not a way to determine the true canonical
> name for the disk.  Within Open Boot, the same disk can have multiple names
> but all reference the same disk.  For example the same disk can be referenced
> by its SAS WWN, using this form:
>
> /pci@302/pci@2/pci@0/pci@17/LSI,sas@0/disk@w5000cca02f037d6d,0
>
> It can also be referenced by its PHY identifier using this form:
>
> /pci@302/pci@2/pci@0/pci@17/LSI,sas@0/disk@p0
>
> It can also be referenced by its Target identifier using this form:
>
> /pci@302/pci@2/pci@0/pci@17/LSI,sas@0/disk@0
>
> Also, when the LUN=0, it is legal to omit the ,0 from the device name.  So 
> with
> the disk above, before taking into account the device aliases, there are 6 
> ways
> to reference the same disk.
>
> Then it is possible to have 0 .. n device aliases all representing the same 
> disk.
> Within this new driver the true canonical name is determined using the the
> IEEE1275 encode-unit and decode-unit commands when address_cells == 4.  This
> will determine the true single canonical name for the device so multiple 
> ihandles
> are not opened for the same device.  This is what frequently happens with the 
> old
> ofdisk driver.  With some devices when they are opened multiple times it 
> causes
> the entire system to hang.
>
> Another problem solved with this driver is devices that do not have a device
> alias can be booted and used within GRUB. Within the old ofdisk, this was not
> possible, unless it was the original boot device.  All devices behind a SAS
> or SCSI parent can be found.   Within the old ofdisk, finding these disks
> relied on there being an alias defined.  The alias requirement is not
> necessary with this new driver.  It can also find devices behind a parent
> after they have been hot-plugged.  This is something that is not possible
> with the old ofdisk driver.
>
> The old ofdisk driver also incorrectly assumes that the device pointing to by 
> a
> device alias is in its true canonical form. This assumption is never made with
> this new driver.
>
> Another issue solved with this driver is that it properly caches the ihandle
> for all open devices.  The old ofdisk tries to do this by caching the last
> opened ihandle.  However this does not work properly because the layer above
> does not use a consistent device name for the same disk when calling into the
> driver.  This is because the upper layer uses the bootpath value returned 
> within
> /chosen, other times it uses the device alias, and other times it uses the
> value within grub.cfg.  It does not have a way to figure out that these 
> devices
> are the same disk.  This is not a problem with this new driver.
>
> Due to the way GRUB repeatedly opens and closes the same disk. Caching the
> ihandle is important on SPARC.  Without caching, some SAS devices can take
> 15 - 20 minutes to get to the GRUB menu. This ihandle caching is not possible
> without correctly having the canonical disk name.
>
> When available, this driver also tries to use the deblocker #blocks and
> a way of determining the disk size.
>
> Finally and probably most importantly, this new driver is also capable of
> seeing all partitions on a GPT disk.  With the old driver, the GPT
> partition table can not be read and only the first partition on the disk
> can be seen.
>
> Signed-off-by: Eric Snowberg 
> ---
> Changes from v1:
> - Addressed various coding style changes requested by Daniel Kipper
> ---
>  grub-core/Makefile.core.def  |1 +
>  grub-core/commands/nativedisk.c  |1 +
>  grub-core/disk/ieee1275/obdisk.c | 1106 
> ++
>  grub-core/kern/ieee1275/cmain.c  |3 +
>  grub-core/kern/ieee1275/init.c   |   12 +-
>  include/grub/disk.h  |1 +
>  include/grub/ieee1275/ieee1275.h |2 +
>  include/grub/ieee1275/obdisk.h   |   25 +
>  8 files changed, 1150 insertions(+), 1 deletions(-)
>  create mode 100644 grub-core/disk/ieee1275/obdisk.c
>  create mode 100644 include/grub/ieee1275/obdisk.h
>
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index fc4767f..ab84aa4 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -292,6 +292,7 @@ kernel = {
>sparc64_ieee1275 = kern/sparc64/cache.S;
>sparc64_ieee1275 = kern/sparc64/dl.c;
>sparc64_ieee1275 = kern/sparc64/ieee1275/ieee1275.c;
> +  sparc64_ieee1275 = disk/ieee1275/obdisk.c;
>
>arm = kern/arm/dl.c;
>arm = kern/arm/dl_helper.c;
> diff --git a/grub-core/commands/nativedisk.c b/grub-core/commands/natived

Re: Grub2 EFI: compatibility with wimboot

[Daniel Kiper]

Hi,

On Tue, Jun 05, 2018 at 08:25:53PM +0200, jame88f...@gmx.de wrote:
> Hello,
>
> I wanted to ask if it would be possible to make the compatibility with
> wimboot in Grub2 EFI? currently only legacy works.
>
> http://ipxe.org/wimboot
>
> that would be really nice.

We are happy to help but first of all we need more info
about your legacy BIOS and UEFI usage. Could you send us
your config files, error messages, etc.?

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2] ieee1275: obdisk driver

[Daniel Kiper]

On Fri, Jun 15, 2018 at 02:05:32PM +0200, John Paul Adrian Glaubitz wrote:
> On 06/15/2018 02:02 PM, Daniel Kiper wrote:
> >> +static int disks_enumerated = 0;
> >> +static struct disk_dev *disk_devs = NULL;
> >> +static struct parent_dev *parent_devs = NULL;
> >
> > I would drop all these 0/NULL initializations.
> > Compiler will do work for you. I asked about
> > that in earlier comments.
>
> Not sure about that though. I seem to remember that newer gcc versions
> were quite nitpicky about that when building with -Werror and I had
> to add a NULL initialization to get it to build.

If this is true I am not going to insist. Sadly I have asked about
that in the earlier comments and I have not seen any reply WRT nor
changes to the code. So, that is why I am repeating my question.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 1/1] grub-fs-tester: fix losetup race

[Daniel Kiper]

On Fri, Jun 15, 2018 at 06:34:04PM +0100, Will Thompson wrote:
> If something else on the system is using loopback devices, then the
> device that's free at the call to `losetup -f` may not be free in the
> following call to try to use it. Instead, find and use the first free
> loopback device in a single call to losetup.

In general LGTM except lack of SOB. I can add it (Signed-off-by: Will
Thompson ) before committing if you are OK with that.
Next time please do not forget about that.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v4] mbi: use per segment a separate relocator chunk

[Daniel Kiper]

On Thu, Jun 14, 2018 at 08:29:14PM +0200, Alexander Boettcher wrote:
> Instead of setting up a all comprising relocator chunk for all segments,
> use per segment a separate relocator chunk.
>
> Currently, if the ELF is non-relocatable, a single relocator chunk will
> comprise memory (between the segments) which gets overridden by the relst()
> invocation of the movers code in grub_relocator16/32/64_boot().
>
> The overridden memory may contain reserved ranges like VGA memory or ACPI
> tables, which may lead to crashes or at least to strange boot behaviour.
>
> Signed-off-by: Alexander Boettcher 

LGTM. If there are no objections I will apply this patch by the end of this 
week.

Thank you for doing the work.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel