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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.10.14 at 18:03, <daniel.kiper@xxxxxxxxxx> wrote:
> 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?

Yes.

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

Patch 3 had quite a few more of them.

>> > +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;
> }

If it is guaranteed that whatever non-zero field(s) if and only if
original flag set - fine by me. But I guess it would be better if
you forwarded the flags values.

Jan


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