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

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Tue, Oct 14, 2014 at 04:27:13PM +0100, Jan Beulich wrote:
> >>> 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

You mean alloc_struct(sizeof(*mbd)) in this case?

> the myriad of casts you're adding - why can't you use void *,
> following what the old code did?

Most of mbi, mbi2 and mbd members are u32. So, that is why
all basic functions use u32 arguments and returns u32. There
are only two needed casts related to this functions and you can
see them above.

> > +static multiboot_info_t __read_mostly mbi;
>
> Is this really used post-init?

Yep.

> > +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.

Good point. I think that this should work:

if ( mbd->mem_lower || mbd->mem_upper )
{
    mbi.flags |= MBI_MEMLIMITS;
    mbi.mem_lower = mbd->mem_lower;
    mbi.mem_upper = mbd->mem_upper;
}

...

> > @@ -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?

I will try to call it from EFI boot code.

Daniel

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