Re: [PATCH RFC 2/7] xen/x86: Add xbi.h header file

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 09/08/14 00:04, Daniel Kiper wrote:
> Define Xen Boot Info (XBI) type. This will be used to define variable
> used to store data collected by bootloader and preloader. This way
> we are able to make most of Xen code bootloader agnostic.
>
> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>

This looks ok in principle, and the end goal seems like a good idea.

> ---
>  xen/include/asm-x86/xbi.h |  117 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 117 insertions(+)
>  create mode 100644 xen/include/asm-x86/xbi.h
>
> diff --git a/xen/include/asm-x86/xbi.h b/xen/include/asm-x86/xbi.h
> new file mode 100644
> index 0000000..ca9e615
> --- /dev/null
> +++ b/xen/include/asm-x86/xbi.h
> @@ -0,0 +1,117 @@
> +/*
> + * Copyright (c) 2013, 2014 Oracle Co., Daniel Kiper
> + *
> + * This program 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 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program 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 this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef __XBI_H__
> +#define __XBI_H__
> +
> +#include <xen/types.h>
> +#include <xen/vga.h>
> +
> +#include <asm/e820.h>
> +#include <asm/edd.h>
> +#include <asm/mbd.h>
> +
> +/* Xen Boot Info (XBI) type. */

Probably need a rather larger comment here, reiterating that this
structure is a collection of information provided/found by the
bootloader/preloader, and presented in a common place for the Xen boot code.

> +typedef struct {
> +    /* Boot loader name. */
> +    char *boot_loader_name;
> +
> +    /* Xen command line. */
> +    char *cmdline;
> +
> +    /* Memory map type (source of memory map). */
> +    char *mmap_type;
> +
> +    /*
> +     * Amount of upper memory (in KiB) accordingly to The Multiboot
> +     * Specification version 0.6.96.
> +     */
> +    u32 mem_upper;
> +
> +    /* Number of memory map entries provided by Xen preloader. */
> +    int e820map_nr;

Count of e820 entries is inherently an unsigned quantity.

> +
> +    /*
> +     * Memory map provided by Xen preloader. It should always point
> +     * to an area able to accommodate at least E820MAX entries.
> +     */
> +    struct e820entry *e820map;
> +
> +    /* Size (in bytes) of EFI memory map provided by Xen preloader. */
> +    unsigned long efi_mmap_size;
> +
> +    /* Size (in bytes) of EFI memory map descriptor provided by Xen preloader. */
> +    unsigned long efi_mmap_desc_size;

size_t for these two?

> +
> +    /* Pointer to EFI memory map provided by preloader. */
> +    void *efi_mmap;
> +
> +    /* Pointer to MPS. */
> +    unsigned long mps;
> +
> +    /* Pointer to ACPI RSDP. */
> +    unsigned long acpi;
> +
> +    /* Pointer to ACPI 2.0 RSDP. */
> +    unsigned long acpi20;
> +
> +    /* Pointer to SMBIOS. */
> +    unsigned long smbios;

I presume these 4 are all physical addresses?  how about paddr_t ?

> +
> +    /* Pointer to EFI System Table. */
> +    void *efi_system_table;
> +
> +    /* VGA console info. */
> +    struct xen_vga_console_info vga_console_info;
> +
> +    /* EDID info. */
> +    unsigned short edid_caps;
> +    unsigned char *edid_info;
> +
> +    /* Number of EDD entries provided by Xen preloader. */
> +    u8 edd_info_nr;
> +
> +    /* Pointer to EDD info. */
> +    struct edd_info *edd_info;
> +
> +    /* Number of MBR entries provided by Xen preloader. */
> +    u8 mbr_signature_nr;
> +
> +    /* Pointer to MBR info. */
> +    struct mbr_signature *mbr_signature;
> +
> +    /* Number of modules. */
> +    unsigned int mods_nr;
> +
> +    /* Pointer to modules description. */
> +    boot_module_t *mods;
> +
> +    /*
> +     * Info about warning occurred during XBI initialization.
> +     * NULL if everything went OK.
> +     */
> +    char *warn_msg;
> +
> +    /*
> +     * Info about error occurred during XBI initialization. NULL if everything
> +     * went OK. Otherwise XBI is not fully/properly initialized.
> +     */
> +    char *err_msg;
> +} xbi_t;
> +
> +extern xbi_t *xbi;

I realise this is very subjective, but I quite dislike this name.  The X
is redundant, this being the Xen source tree, and BI could perfectly
easily be an object called "boot_into", which would be rather clearer
when used in the code.

~Andrew

> +#endif /* __XBI_H__ */