grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v4 06/10] RISC-V: Add Linux load logic


From: Daniel Kiper
Subject: Re: [PATCH v4 06/10] RISC-V: Add Linux load logic
Date: Wed, 23 Jan 2019 11:41:17 +0100
User-agent: NeoMutt/20170113 (1.7.2)

On Tue, Jan 22, 2019 at 05:09:18PM +0100, Alexander Graf wrote:
> On 17.01.19 13:24, Daniel Kiper wrote:
> > On Mon, Nov 26, 2018 at 12:38:11AM +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 a Linux boot case that is not enabled in Linux yet. At
> >> least it will give people something to test against when they enable the
> >> Linux UEFI port.
> >>
> >> Signed-off-by: Alexander Graf <address@hidden>
> >> Reviewed-by: Alistair Francis <address@hidden>
> >>
> >> ---
> >>
> >> v1 -> v2:
> >>
> >>   - adapt to new grub_open_file() API
> >>   - adapt to new grub_create_loader_cmdline() API
> >>
> >> v3 -> v4:
> >>
> >>   - Change copyright from 2013 to 2018
> >>   - Coding style fixes
> >> ---
> >>  grub-core/loader/riscv/linux.c | 351 
> >> +++++++++++++++++++++++++++++++++++++++++
> >>  include/grub/riscv32/linux.h   |  41 +++++
> >>  include/grub/riscv64/linux.h   |  43 +++++
> >>  3 files changed, 435 insertions(+)
> >>  create mode 100644 grub-core/loader/riscv/linux.c
> >>  create mode 100644 include/grub/riscv32/linux.h
> >>  create mode 100644 include/grub/riscv64/linux.h
> >>
> >> diff --git a/grub-core/loader/riscv/linux.c 
> >> b/grub-core/loader/riscv/linux.c
> >> new file mode 100644
> >> index 000000000..fc8c508c8
> >> --- /dev/null
> >> +++ b/grub-core/loader/riscv/linux.c
> >> @@ -0,0 +1,351 @@
> >> +/*
> >> + *  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/>.
> >> + */
> >> +
> >> +#include <grub/charset.h>
> >> +#include <grub/command.h>
> >> +#include <grub/err.h>
> >> +#include <grub/file.h>
> >> +#include <grub/fdt.h>
> >> +#include <grub/linux.h>
> >> +#include <grub/loader.h>
> >> +#include <grub/mm.h>
> >> +#include <grub/types.h>
> >> +#include <grub/cpu/linux.h>
> >> +#include <grub/efi/efi.h>
> >> +#include <grub/efi/fdtload.h>
> >> +#include <grub/efi/memory.h>
> >> +#include <grub/efi/pe32.h>
> >> +#include <grub/i18n.h>
> >> +#include <grub/lib/cmdline.h>
> >> +
> >> +GRUB_MOD_LICENSE ("GPLv3+");
> >> +
> >> +static grub_dl_t my_mod;
> >> +static int loaded;
> >> +
> >> +static void *kernel_addr;
> >> +static grub_uint64_t kernel_size;
> >> +
> >> +static char *linux_args;
> >> +static grub_uint32_t cmdline_size;
> >> +
> >> +static grub_addr_t initrd_start;
> >> +static grub_addr_t initrd_end;
> >> +
> >> +grub_err_t
> >> +grub_arch_efi_linux_check_image (struct linux_riscv_kernel_header * lh)
> >> +{
> >> +  if (lh->magic != GRUB_LINUX_RISCV_MAGIC_SIGNATURE)
> >> +    return grub_error(GRUB_ERR_BAD_OS, "invalid magic number");
> >> +
> >> +  if ((lh->code0 & 0xffff) != GRUB_PE32_MAGIC)
> >> +    return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
> >> +                 N_("plain image kernel not supported - rebuild with 
> >> CONFIG_(U)EFI_STUB enabled"));
> >> +
> >> +  grub_dprintf ("linux", "UEFI stub kernel:\n");
> >> +  grub_dprintf ("linux", "PE/COFF header @ %08x\n", lh->hdr_offset);
> >> +
> >> +  return GRUB_ERR_NONE;
> >> +}
> >> +
> >> +static grub_err_t
> >> +finalize_params_linux (void)
> >> +{
> >> +  int node, retval;
> >> +
> >
> > Please drop this empty line.
> >
> >> +  void *fdt;
> >> +
> >> +  fdt = grub_fdt_load (0x400);
> >
> > Why this number? Please define constant or add a comment here.
> > Whichever is better. And I can see the same value in ARM64. So,
> > maybe it is worth using the same constant here and there. Anyway,
> > please fix it somehow.
>
> This file is a 1:1 copy from the arm version. Ideally, they should get
> merged eventually. But any issues you found here apply similarly to the
> arm copy.

ARM copy is fixed in the master right now. So, you can fix it here too.

> >> +  if (!fdt)
> >> +    goto failure;
> >> +
> >> +  node = grub_fdt_find_subnode (fdt, 0, "chosen");
> >> +  if (node < 0)
> >> +    node = grub_fdt_add_subnode (fdt, 0, "chosen");
> >> +
> >> +  if (node < 1)
> >> +    goto failure;
> >> +
> >> +  /* Set initrd info */
> >> +  if (initrd_start && initrd_end > initrd_start)
> >> +    {
> >> +      grub_dprintf ("linux", "Initrd @ %p-%p\n",
> >> +              (void *) initrd_start, (void *) initrd_end);
> >> +
> >> +      retval = grub_fdt_set_prop64 (fdt, node, "linux,initrd-start",
> >> +                              initrd_start);
> >> +      if (retval)
> >> +  goto failure;
> >> +      retval = grub_fdt_set_prop64 (fdt, node, "linux,initrd-end",
> >> +                              initrd_end);
> >> +      if (retval)
> >> +  goto failure;
> >> +    }
> >> +
> >> +  if (grub_fdt_install() != GRUB_ERR_NONE)
> >> +    goto failure;
> >> +
> >> +  return GRUB_ERR_NONE;
> >> +
> >> + failure:
> >
> > s/failure/fail/?
>
> Why? I mean, sure, I can change it. But why?

Yeah, this is rather cosmetic. However, AFAICT "fail" is more common in
the GRUB code. Or "err" would be even better if possible.

> >> +  grub_fdt_unload();
> >
> > s/grub_fdt_unload()/grub_fdt_unload ()/ May I ask you to fix similar
> > mistakes in the other patches too?
>
> Can you please write a checkpatch.pl or similar to catch them? At this

Sadly I am too busy right now to take a look at it. If somebody is willing
to work on it then I can review the patches. Anyway, I am adding this
to TODO list.

> point, all those coding style issues just add to frustration on both
> sides I think. To me, the GNU style comes very close in uglyness to the
> TianoCore one - and my fingers just simply refuse to naturally adhere to it.

Yep, I agree. Sadly I am afraid that we have to live with that. Maybe
checkpatch.pl will relieve some pain in the future.

[...]

> >> +  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 */
> >
> > Wrong number of tabs?
>
> Looks correct in my editor?

If yes then never mind.

Daniel



reply via email to

[Prev in Thread] Current Thread [Next in Thread]