Re: [PATCH v4 04/19] xen: prepare common code for Xen PVH support

[Daniel Kiper]

On Fri, Nov 02, 2018 at 01:37:23PM +0100, Juergen Gross wrote:
> Some common code needs to be special cased for Xen PVH mode. This hits
> mostly Xen PV mode specific areas.
>
> Split include/grub/i386/pc/int_types.h off from
> include/grub/i386/pc/int.h to support including this file later from
> xen_pvh code without the grub_bios_interrupt definition.
>
> Move definition of struct grub_e820_mmap_entry from
> grub-core/mmap/i386/pc/mmap.c to include/grub/i386/memory.h in order
> to make it usable from xen_pvh code.
>
> Signed-off-by: Juergen Gross <address@hidden>

If you fix two nitpicks below you can add
  Reviewed-by: Daniel Kiper <address@hidden>

> ---
> V3: GRUB_MACHINE_XENPVH -> GRUB_MACHINE_XEN_PVH (Daniel Kiper)
>     split include/grub/i386/pc/int.h (Daniel Kiper)
>     move struct grub_e820_mmap_entry definition to header file
> ---
>  grub-core/kern/i386/tsc.c         |  2 +-
>  grub-core/mmap/i386/pc/mmap.c     |  7 -----
>  include/grub/i386/memory.h        |  7 +++++
>  include/grub/i386/pc/int.h        | 36 +-----------------------
>  include/grub/i386/pc/int_types.h  | 59 
> +++++++++++++++++++++++++++++++++++++++
>  include/grub/i386/tsc.h           |  2 +-
>  include/grub/i386/xen/hypercall.h |  5 +++-
>  include/grub/kernel.h             |  4 ++-
>  8 files changed, 76 insertions(+), 46 deletions(-)
>  create mode 100644 include/grub/i386/pc/int_types.h
>
> diff --git a/grub-core/kern/i386/tsc.c b/grub-core/kern/i386/tsc.c
> index f266eb131..9293b161d 100644
> --- a/grub-core/kern/i386/tsc.c
> +++ b/grub-core/kern/i386/tsc.c
> @@ -65,7 +65,7 @@ grub_tsc_init (void)
>
>    tsc_boot_time = grub_get_tsc ();
>
> -#ifdef GRUB_MACHINE_XEN
> +#if defined (GRUB_MACHINE_XEN) || defined (GRUB_MACHINE_XEN_PVH)
>    (void) (grub_tsc_calibrate_from_xen () || calibrate_tsc_hardcode());
>  #elif defined (GRUB_MACHINE_EFI)
>    (void) (grub_tsc_calibrate_from_pmtimer () || grub_tsc_calibrate_from_pit 
> () || grub_tsc_calibrate_from_efi() || calibrate_tsc_hardcode());
> diff --git a/grub-core/mmap/i386/pc/mmap.c b/grub-core/mmap/i386/pc/mmap.c
> index 609994516..bcb097c38 100644
> --- a/grub-core/mmap/i386/pc/mmap.c
> +++ b/grub-core/mmap/i386/pc/mmap.c
> @@ -42,13 +42,6 @@ extern grub_uint16_t grub_machine_mmaphook_kblow;
>  extern grub_uint16_t grub_machine_mmaphook_kbin16mb;
>  extern grub_uint16_t grub_machine_mmaphook_64kbin4gb;
>
> -struct grub_e820_mmap_entry
> -{
> -  grub_uint64_t addr;
> -  grub_uint64_t len;
> -  grub_uint32_t type;
> -} GRUB_PACKED;
> -
>

Please drop this extra empty line too.

[...]

> diff --git a/include/grub/i386/pc/int_types.h 
> b/include/grub/i386/pc/int_types.h
> new file mode 100644
> index 000000000..35a4b5087
> --- /dev/null
> +++ b/include/grub/i386/pc/int_types.h
> @@ -0,0 +1,59 @@
> +/*
> + *  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_INTERRUPT_TYPES_MACHINE_HEADER
> +#define GRUB_INTERRUPT_TYPES_MACHINE_HEADER  1
> +
> +#include <grub/types.h>
> +
> +struct grub_bios_int_registers
> +{
> +  grub_uint32_t eax;
> +  grub_uint16_t es;
> +  grub_uint16_t ds;
> +  grub_uint16_t flags;
> +  grub_uint16_t dummy;
> +  grub_uint32_t ebx;
> +  grub_uint32_t ecx;
> +  grub_uint32_t edi;
> +  grub_uint32_t esi;
> +  grub_uint32_t edx;
> +};

Please move this struct behind constants definitions below.

> +#define  GRUB_CPU_INT_FLAGS_CARRY     0x1
> +#define  GRUB_CPU_INT_FLAGS_PARITY    0x4
> +#define  GRUB_CPU_INT_FLAGS_ADJUST    0x10
> +#define  GRUB_CPU_INT_FLAGS_ZERO      0x40
> +#define  GRUB_CPU_INT_FLAGS_SIGN      0x80
> +#define  GRUB_CPU_INT_FLAGS_TRAP      0x100
> +#define  GRUB_CPU_INT_FLAGS_INTERRUPT 0x200
> +#define  GRUB_CPU_INT_FLAGS_DIRECTION 0x400
> +#define  GRUB_CPU_INT_FLAGS_OVERFLOW  0x800
> +#ifdef GRUB_MACHINE_PCBIOS
> +#define  GRUB_CPU_INT_FLAGS_DEFAULT   GRUB_CPU_INT_FLAGS_INTERRUPT
> +#else
> +#define  GRUB_CPU_INT_FLAGS_DEFAULT   0
> +#endif
> +
> +struct grub_i386_idt
> +{
> +  grub_uint16_t limit;
> +  grub_uint32_t base;
> +} GRUB_PACKED;
> +
> +#endif

Daniel