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

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Sun, Aug 10, 2014 at 05:34:11PM +0100, Andrew Cooper wrote:
> 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@xxxxxxxxxx>
>
> 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.

OK, but after some investigation it looks that we should change also
xen/arch/x86/e820.c:init_e820() and friends due to type signedness
conflicts. I think that this is good idea but I would like to ask
you guys about your opinion. So?

> > +    /*
> > +     * 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?

Both of them should be UINTN because they are referenced
from EFI function GetMemoryMap(). However, this means that
we must include asm/efibind.h and efi/efidef.h. If we do
that then:

In file included from xen/include/acpi/acpi.h:57:0,
                 from xen/include/xen/acpi.h:34,
                 from xen/include/asm/boot_info.h:25,
                 from boot.c:27:
xen/include/acpi/actypes.h:130:35: error: conflicting types for âUINT64â

and

In file included from xen/include/acpi/acpi.h:57:0,
                 from xen/include/xen/acpi.h:34,
                 from xen/include/asm/boot_info.h:25,
                 from boot.c:27:
xen/include/acpi/actypes.h:131:34: error: conflicting types for âINT64â

That is why I used unsigned long here. It is a hack to avoid that issue.
I forgot about that. However, now if we wish to proceed further we should
do something with this conflict. Avoidance is not a solution in that case
because if we do that right now then this conflict will hit us once again
sooner or later somewhere else. So, I think that we should rename all
basic EFI types and use EFI_ prefix, e.g., EFI_UINT64. This way we will
avoid this conflict with ACPI types in the future. What do you think
about that? Do you have better solution for that problem?

> > +    /* Pointer to EFI memory map provided by preloader. */
> > +    void *efi_mmap;

EFI_MEMORY_DESCRIPTOR *efi_mmap;

That is why we need efi/efidef.h too.

> > +
> > +    /* Pointer to MPS. */
> > +    unsigned long mps;

paddr_t mps;

> > +    /* Pointer to ACPI RSDP. */
> > +    unsigned long acpi;

acpi_physical_address acpi;

...but #include <xen/acpi.h> generates conflict with
EFI types similar to mentioned above...

> > +    /* Pointer to ACPI 2.0 RSDP. */
> > +    unsigned long acpi20;

acpi_physical_address acpi20;

...but #include <xen/acpi.h> generates conflict with
EFI types similar to mentioned above...

> > +    /* Pointer to SMBIOS. */
> > +    unsigned long smbios;

paddr_t smbios;

> I presume these 4 are all physical addresses?  how about paddr_t ?
>
> > +
> > +    /* Pointer to EFI System Table. */
> > +    void *efi_system_table;

EFI_SYSTEM_TABLE *efi_system_table;

...but conflict with ACPI types as above...

Daniel

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