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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.09.14 at 18:30, <daniel.kiper@xxxxxxxxxx> wrote:
> On Sun, Aug 10, 2014 at 05:34:11PM +0100, Andrew Cooper wrote:
>> On 09/08/14 00:04, Daniel Kiper wrote:
>> > +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?

The general direction here is to not let new code in that's not
sufficiently sign- or const-correct, but not bother cleaning up
old code unless touching it anyway. But if you feel like submitting
a patch correcting signedness there, just go ahead.

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

Absolutely not - these headers should stay as closely in sync with
their originals as possible to ease eventual updating.

> Do you have better solution for that problem?

Don't include ACPI and EFI headers at the same time, or use
compatible types (as you did, and as - I think - size_t would also
do). In the latter case a comment explaining why the original
types can't be used would perhaps avoid questions like Andrew's.

Jan

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