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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Mon, 11 Aug 2014, Stefano Stabellini wrote:
> On Sat, 9 Aug 2014, 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@xxxxxxxxxx>
> > ---
> >  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. */
> > +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;
> > +
> > +    /*
> > +     * 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;

I would use explicitly sized integers here. Moreover ARM32 supports
LPAE.


> > +    /* Size (in bytes) of EFI memory map descriptor provided by Xen 
> > preloader. */
> > +    unsigned long efi_mmap_desc_size;
> > +
> > +    /* Pointer to EFI memory map provided by preloader. */
> > +    void *efi_mmap;
> > +
> > +    /* Pointer to MPS. */
> > +    unsigned long mps;

Why some pointers are void* and some others are unsigned long?
If this contains a physical address you should say so in the comment.
Also you should use xen_ulong_t or u64.


> > +    /* Pointer to ACPI RSDP. */
> > +    unsigned long acpi;
> > +
> > +    /* Pointer to ACPI 2.0 RSDP. */
> > +    unsigned long acpi20;
> > +
> > +    /* Pointer to SMBIOS. */
> > +    unsigned long smbios;
> > +
> > +    /* 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;
> > +#endif /* __XBI_H__ */
>  
> If this struct is to become part of an internal cross-arch interface in
> Xen, it needs to have an arch specific field. You could move the e820
> stuff to the x86 version of it. The only memory map available on ARM is
> the UEFI memory map.
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel