Re: [PATCH v6 06/11] RISC-V: Add Linux load logic

[Daniel Kiper]

On Tue, Feb 19, 2019 at 02:25:11PM +0100, Alexander Graf wrote:
> On 02/18/2019 08:28 PM, Daniel Kiper wrote:
> > On Tue, Feb 12, 2019 at 11:31:03AM +0100, Alexander Graf wrote:
> > > We currently only support to run grub on RISC-V as UEFI payload. Ideally,
> > > we also only want to support running Linux underneath as UEFI payload.
> > >
> > > Prepare that with some Linux boot stub code. Once the arm64 target is
> > > generalized, we can hook into that one and gain boot functionality.
> > >
> > > Signed-off-by: Alexander Graf <address@hidden>
> > Reviewed-by: Daniel Kiper <address@hidden>
> >
> > But two nitpicks below...
> >
> > [...]
> >
> > > diff --git a/include/grub/riscv32/linux.h b/include/grub/riscv32/linux.h
> > > new file mode 100644
> > > index 000000000..b8ed39407
> > > --- /dev/null
> > > +++ b/include/grub/riscv32/linux.h
> > > @@ -0,0 +1,41 @@
> > > +/*
> > > + *  GRUB  --  GRand Unified Bootloader
> > > + *  Copyright (C) 2018  Free Software Foundation, Inc.
> > > + *
> > > + *  GRUB is free software: you can redistribute it and/or modify
> > > + *  it under the terms of the GNU General Public License as published by
> > > + *  the Free Software Foundation, either version 3 of the License, or
> > > + *  (at your option) any later version.
> > > + *
> > > + *  GRUB is distributed in the hope that it will be useful,
> > > + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > + *  GNU General Public License for more details.
> > > + *
> > > + *  You should have received a copy of the GNU General Public License
> > > + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> > > + */
> > > +
> > > +#ifndef GRUB_RISCV32_LINUX_HEADER
> > > +#define GRUB_RISCV32_LINUX_HEADER 1
> > > +
> > > +#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x52534356 /* 'RSCV' */
> > > +
> > > +/* From linux/Documentation/riscv/booting.txt */
> > > +struct linux_riscv_kernel_header
> > > +{
> > > +  grub_uint32_t code0;           /* Executable code */
> > > +  grub_uint32_t code1;           /* Executable code */
> > > +  grub_uint64_t text_offset;     /* Image load offset */
> > > +  grub_uint64_t res0;            /* reserved */
> > > +  grub_uint64_t res1;            /* reserved */
> > > +  grub_uint64_t res2;            /* reserved */
> > > +  grub_uint64_t res3;            /* reserved */
> > > +  grub_uint64_t res4;            /* reserved */
> > > +  grub_uint32_t magic;           /* Magic number, little endian, "RSCV" 
> > > */
> > > +  grub_uint32_t hdr_offset;      /* Offset of PE/COFF header */
> > > +};
> > > +
> > > +# define linux_arch_kernel_header linux_riscv_kernel_header
> > I do not think that we need a space between "#" and "define".
> >
> > > +#endif /* ! GRUB_RISCV32_LINUX_HEADER */
> > > diff --git a/include/grub/riscv64/linux.h b/include/grub/riscv64/linux.h
> > > new file mode 100644
> > > index 000000000..29140e45e
> > > --- /dev/null
> > > +++ b/include/grub/riscv64/linux.h
> > > @@ -0,0 +1,43 @@
> > > +/*
> > > + *  GRUB  --  GRand Unified Bootloader
> > > + *  Copyright (C) 2018  Free Software Foundation, Inc.
> > > + *
> > > + *  GRUB is free software: you can redistribute it and/or modify
> > > + *  it under the terms of the GNU General Public License as published by
> > > + *  the Free Software Foundation, either version 3 of the License, or
> > > + *  (at your option) any later version.
> > > + *
> > > + *  GRUB is distributed in the hope that it will be useful,
> > > + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > + *  GNU General Public License for more details.
> > > + *
> > > + *  You should have received a copy of the GNU General Public License
> > > + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> > > + */
> > > +
> > > +#ifndef GRUB_RISCV64_LINUX_HEADER
> > > +#define GRUB_RISCV64_LINUX_HEADER 1
> > > +
> > > +#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x52534356 /* 'RSCV' */
> > > +
> > > +#define GRUB_EFI_PE_MAGIC        0x5A4D
> > > +
> > > +/* From linux/Documentation/riscv/booting.txt */
> > > +struct linux_riscv_kernel_header
> > > +{
> > > +  grub_uint32_t code0;           /* Executable code */
> > > +  grub_uint32_t code1;           /* Executable code */
> > > +  grub_uint64_t text_offset;     /* Image load offset */
> > > +  grub_uint64_t res0;            /* reserved */
> > > +  grub_uint64_t res1;            /* reserved */
> > > +  grub_uint64_t res2;            /* reserved */
> > > +  grub_uint64_t res3;            /* reserved */
> > > +  grub_uint64_t res4;            /* reserved */
> > > +  grub_uint32_t magic;           /* Magic number, little endian, "RSCV" 
> > > */
> > > +  grub_uint32_t hdr_offset;      /* Offset of PE/COFF header */
> > > +};
> > > +
> > > +# define linux_arch_kernel_header linux_riscv_kernel_header
> > Ditto. I can fix it before committing this patch. Are you OK with that?
>
> Both again are 1:1 copies of the arm headers. If you think that this is bad
> coding style, please write a checkpatch script or add a travis check job for
> coding style problems. It will save everyone lots of work ;).

For sure but sadly I do not have a time to do that. If somebody here is
willing to do that then I can give some hints and review the patches.
However, I will do a favor for you. I will fix it before pushing the patches.

Daniel