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

[Juergen Gross]

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?

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.


Juergen