Re: [Xen-devel] [PATCH for-xen-4.5 v3 04/16] x86: Introduce MultiBoot Data (MBD) type

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.10.14 at 19:52, <daniel.kiper@xxxxxxxxxx> wrote:
> +static mbd_t __used *__reloc(void *mbi)
> +{
> +    mbd_t *mbd;
>  
> -    /* Mask features we don't understand or don't relocate. */
> -    mbi->flags &= (MBI_MEMLIMITS |
> -                   MBI_BOOTDEV |
> -                   MBI_CMDLINE |
> -                   MBI_MODULES |
> -                   MBI_MEMMAP |
> -                   MBI_LOADERNAME);
> +    mbd = (mbd_t *)alloc_struct(sizeof(mbd_t));
> +    zero_struct((u32)mbd, sizeof(mbd_t));

Here and elsewhere - please prefer sizeof(<expression>) over
sizeof(<type>) where possible. Also I continue to be questioning
the myriad of casts you're adding - why can't you use void *,
following what the old code did?

> +static multiboot_info_t __read_mostly mbi;

Is this really used post-init?

> +unsigned long __init __init_mbi(u32 mbd_pa)
> +{
> +    mbd_t *mbd = __va(mbd_pa);
> +
> +    enable_exception_support();
> +
> +    if ( mbd->boot_loader_name ) {
> +        mbi.flags = MBI_LOADERNAME;
> +        mbi.boot_loader_name = mbd->boot_loader_name;
> +    }
> +
> +    if ( mbd->cmdline ) {
> +        mbi.flags |= MBI_CMDLINE;
> +        mbi.cmdline = mbd->cmdline;
> +    }
> +
> +    mbi.flags |= MBI_MEMLIMITS;
> +    mbi.mem_lower = mbd->mem_lower;
> +    mbi.mem_upper = mbd->mem_upper;
> +
> +    mbi.flags |= MBI_MEMMAP;
> +    mbi.mmap_length = mbd->mmap_size;
> +    mbi.mmap_addr = mbd->mmap;
> +
> +    mbi.flags |= MBI_MODULES;
> +    mbi.mods_count = mbd->mods_nr;
> +    mbi.mods_addr = mbd->mods;
> +
> +    return __pa(&mbi);
> +}

You shouldn't be blindly setting these flags - the incoming structure
has them, but you discard them in reloc.c instead of propagating
them here.

> @@ -546,21 +565,13 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>          .stop_bits = 1
>      };
>  
> -    /* Critical region without IDT or TSS.  Any fault is deadly! */
> -
> -    set_processor_id(0);
> -    set_current((struct vcpu *)0xfffff000); /* debug sanity. */
> -    idle_vcpu[0] = current;
> -
> -    percpu_init_areas();
> -
> -    init_idt_traps();
> -    load_system_tables();
> -
> -    smp_prepare_boot_cpu();
> -    sort_exception_tables();
> -
> -    /* Full exception support from here on in. */
> +    if ( efi_enabled ) {
> +        enable_exception_support();
> +    }
> +    else
> +    {
> +        /* Exception support was enabled before __start_xen() call. */
> +    }

Apart from the coding style issue (not just here) - what's the reason
this can't also be done in the EFI case?

Jan


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