Re: [Xen-devel] [v3][PATCH 06/16] hvmloader: get guest memory map into memory_map[]

["Chen, Tiejun" <tiejun.chen@xxxxxxxxx>]

On 2015/6/17 18:22, Jan Beulich wrote:
> > > > On 11.06.15 at 03:15, <tiejun.chen@xxxxxxxxx> wrote:
> > --- a/tools/firmware/hvmloader/hvmloader.c
> > +++ b/tools/firmware/hvmloader/hvmloader.c
> > @@ -107,6 +107,8 @@ asm (
> >       "    .text                       \n"
> >       );
> >
> > +struct e820map memory_map;
> Imo this should live in e820.c.
Okay.

> > @@ -199,6 +201,39 @@ static void apic_setup(void)
> >       ioapic_write(0x11, SET_APIC_ID(LAPIC_ID(0)));
> >   }
> >
> > +void memory_map_setup(void)
> And perhaps this one too. Or if not, it should be static.
I'd like to move this into e820.c as well.

> > +{
> > +    unsigned int nr_entries = E820MAX, i;
> > +    int rc;
> > +    uint64_t alloc_addr = RESERVED_MEMORY_DYNAMIC_START - 1;
> > +    uint64_t alloc_size = RESERVED_MEMORY_DYNAMIC_END - alloc_addr;
> > +
> > +    rc = get_mem_mapping_layout(memory_map.map, &nr_entries);
> > +
> > +    if ( rc )
> > +    {
> > +        printf("Failed to get guest memory map.\n");
> > +        BUG();
> > +    }
> > +
> > +    BUG_ON(!nr_entries);
> Please be consistent: printf()+BUG() or BUG_ON(). Also I think the
> two (sanity) checks above could combined into one (and the
> printf() should then print both rc and nr_entries).
What about this?

    if ( rc || !nr_entries ) 

    { 

        printf("Get guest memory maps[%d] failed. (%d)\n", nr_entries, 
rc);
        BUG(); 

    }

> > @@ -368,6 +379,21 @@ uuid_to_string(char *dest, uint8_t *uuid)
> >       *p = '\0';
> >   }
> >
> > +int get_mem_mapping_layout(struct e820entry entries[], uint32_t *max_entries)
s/uint32_t/unsigned int/

> Again no need for a fixed width type here afaict.
Thanks
Tiejun

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