Re: [Xen-devel] [PATCH v4 08/19] xen: add basic hooks for PVH in current code

[Daniel Kiper]

On Thu, Nov 08, 2018 at 08:23:20PM +0100, Juergen Gross wrote:
> On 08/11/2018 16:45, Daniel Kiper wrote:
> > On Fri, Nov 02, 2018 at 01:37:27PM +0100, Juergen Gross wrote:
> >> Add the hooks to current code needed for Xen PVH. They will be filled
> >> with code later when the related functionality is being added.
> >>
> >> Signed-off-by: Juergen Gross <address@hidden>
> >> ---
> >> V3: xenpvh->xen_pvh (Daniel Kiper)
> >>     adjust copyright date (Roger Pau Monné)
> >> ---
> >>  grub-core/kern/i386/xen/pvh.c         | 36 
> >> +++++++++++++++++++++++++++++++++++
> >>  grub-core/kern/i386/xen/startup_pvh.S | 29 ++++++++++++++++++++++++++++
> >>  grub-core/kern/xen/init.c             |  6 ++++++
> >>  grub-core/loader/i386/linux.c         |  1 +
> >>  include/grub/i386/xen_pvh/kernel.h    | 30 +++++++++++++++++++++++++++++
> >>  include/grub/xen.h                    |  6 ++++++
> >>  6 files changed, 108 insertions(+)
> >>  create mode 100644 grub-core/kern/i386/xen/pvh.c
> >>  create mode 100644 grub-core/kern/i386/xen/startup_pvh.S
> >>  create mode 100644 include/grub/i386/xen_pvh/kernel.h
> >>
> >> diff --git a/grub-core/kern/i386/xen/pvh.c b/grub-core/kern/i386/xen/pvh.c
> >> new file mode 100644
> >> index 000000000..ac6181f4e
> >> --- /dev/null
> >> +++ b/grub-core/kern/i386/xen/pvh.c
> >> @@ -0,0 +1,36 @@
> >> +/*
> >> + *  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/kernel.h>
> >> +#include <grub/misc.h>
> >> +#include <grub/memory.h>
> >> +#include <grub/mm.h>
> >> +#include <grub/xen.h>
> >> +#include <grub/machine/kernel.h>
> >> +
> >> +grub_uint64_t grub_rsdp_addr;
> >> +
> >> +void
> >> +grub_xen_setup_pvh (void)
> >> +{
> >> +}
> >> +
> >> +grub_err_t
> >> +grub_machine_mmap_iterate (grub_memory_hook_t hook, void *hook_data)
> >> +{
> >> +}
> >> diff --git a/grub-core/kern/i386/xen/startup_pvh.S 
> >> b/grub-core/kern/i386/xen/startup_pvh.S
> >> new file mode 100644
> >> index 000000000..69b8fdcca
> >> --- /dev/null
> >> +++ b/grub-core/kern/i386/xen/startup_pvh.S
> >> @@ -0,0 +1,29 @@
> >> +/* startup.S - bootstrap GRUB itself */
> >> +/*
> >> + *  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 <config.h>
> >> +#include <grub/symbol.h>
> >> +
> >> +  .file   "startup_pvh.S"
> >> +  .text
> >> +
> >> +/* Saved pointer to start info structure. */
> >> +  .globl  pvh_start_info
> >> +pvh_start_info:
> >> +  .long   0
> >> diff --git a/grub-core/kern/xen/init.c b/grub-core/kern/xen/init.c
> >> index 10007b411..782ca7295 100644
> >> --- a/grub-core/kern/xen/init.c
> >> +++ b/grub-core/kern/xen/init.c
> >> @@ -45,6 +45,8 @@ grub_xen_ptr2mfn (void *ptr)
> >>    grub_xen_mfn_t *mfn_list =
> >>      (grub_xen_mfn_t *) grub_xen_start_page_addr->mfn_list;
> >>    return mfn_list[(grub_addr_t) ptr >> GRUB_XEN_LOG_PAGE_SIZE];
> >> +#else
> >> +  return (grub_addr_t) ptr >> GRUB_XEN_LOG_PAGE_SIZE;
> >
> > It seems to me that this change does not belong to this patch.
>
> Hmm, in which patch would you put it?

Separate one? Or at least say something in the commit message.
Otherwise it is misleading. Hmmm... But probably I prefer the former...

> I wanted to have all modifications to current grub-core code in place
> after this patch. This includes all needed #ifdefs.
>
> >
> >>  #endif
> >>  }
> >>
> >> @@ -562,6 +564,10 @@ grub_machine_init (void)
> >>                       + GRUB_KERNEL_MACHINE_MOD_GAP,
> >>                       GRUB_KERNEL_MACHINE_MOD_ALIGN);
> >>
> >> +#ifdef GRUB_MACHINE_XEN_PVH
> >> +  grub_xen_setup_pvh ();
> >> +#endif
> >> +
> >>    grub_xen_setup_gnttab ();
> >>
> >>  #ifdef GRUB_MACHINE_XEN
> >> diff --git a/grub-core/loader/i386/linux.c b/grub-core/loader/i386/linux.c
> >> index 51920896e..f96309476 100644
> >> --- a/grub-core/loader/i386/linux.c
> >> +++ b/grub-core/loader/i386/linux.c
> >> @@ -35,6 +35,7 @@
> >>  #include <grub/i18n.h>
> >>  #include <grub/lib/cmdline.h>
> >>  #include <grub/linux.h>
> >> +#include <grub/machine/kernel.h>
> >
> > Please say in the commit message why you include grub/machine/kernel.h here.
>
> Okay.
>
> >
> >>  GRUB_MOD_LICENSE ("GPLv3+");
> >>
> >> diff --git a/include/grub/i386/xen_pvh/kernel.h 
> >> b/include/grub/i386/xen_pvh/kernel.h
> >> new file mode 100644
> >> index 000000000..2b7b8a129
> >> --- /dev/null
> >> +++ b/include/grub/i386/xen_pvh/kernel.h
> >> @@ -0,0 +1,30 @@
> >> +/*
> >> + *  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_KERNEL_MACHINE_HEADER
> >> +#define GRUB_KERNEL_MACHINE_HEADER        1
> >> +
> >> +#ifndef ASM_FILE
> >> +
> >> +#define GRUB_KERNEL_USE_RSDP_ADDR 1
> >> +
> >> +extern grub_uint64_t EXPORT_VAR(grub_rsdp_addr);
> >> +
> >> +#endif /* ! ASM_FILE */
> >> +
> >> +#endif /* GRUB_KERNEL_MACHINE_HEADER */
> >> diff --git a/include/grub/xen.h b/include/grub/xen.h
> >> index c31cc10c7..4cb2c8f43 100644
> >> --- a/include/grub/xen.h
> >> +++ b/include/grub/xen.h
> >> @@ -43,6 +43,7 @@ typedef grub_uint64_t uint64_t;
> >>
> >>  #include <xen/sched.h>
> >>  #include <xen/grant_table.h>
> >> +#include <xen/hvm/start_info.h>
> >
> > Hmmm... Why?
>
> This header contains all #includes of xen interface headers.
>
> I can omit this one and add it to the source files where it is needed
> if you like that better.

Yes, please.

Daniel