Re: [Xen-devel] [v7][PATCH 07/16] hvmloader/e820: construct guest e820 table

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

> > Now we can use that memory map to build our final
> > e820 table but it may need to reorder all e820
> > entries.
> "it" being what? I'm afraid I can't really make sense of the second
> half of the sentence...
I hope the following can work for you,

...
but finally we should sort them into an increasing order since
we shouldn't assume the original order is always good.

> > --- a/tools/firmware/hvmloader/e820.c
> > +++ b/tools/firmware/hvmloader/e820.c
> > @@ -108,7 +108,9 @@ int build_e820_table(struct e820entry *e820,
> >                        unsigned int lowmem_reserved_base,
> >                        unsigned int bios_image_base)
[snip]

> > +     */
> > +    for ( i = 0; i < memory_map.nr_map; i++ )
> > +    {
> > +        uint64_t end = e820[i].addr + e820[i].size;
> Either loop index/boundary or used array are wrong here: In the
> earlier loop you copied memory_map[0...nr_map-1] to
> e820[n...n+nr_map-1], but here you're looping looking at
> e820[0...nr_map-1]
You're right. I should lookup all e820[] like this,

for ( i = 0; i < nr; i++ )

> > +        if ( e820[i].type == E820_RAM &&
> > +             low_mem_end > e820[i].addr && low_mem_end < end )
> Assuming you mean to look at the RDM e820[] entries here, this
> is not a correct check: You don't care about partly or fully
> contained, all you care about is whether low_mem_end extends
> beyond the start of the region.
Here I'm looking at the e820 entry indicating low memory. Because

low_mem_end = hvm_info->low_mem_pgend << PAGE_SHIFT;

and when we allocate MMIO in pci.c, its possible to populate RAM so 
hvm_info->low_mem_pgend would be changed over there. So we need to 
compensate this loss with high memory. Here memory_map[] also records 
the original low/high memory, so if low_mem_end is less-than the 
original we need this compensation.
So here we have two steps to address this issue,

#1. Calculate the loss

> > +        {
> > +            add_high_mem = end - low_mem_end;
> > +            e820[i].size = low_mem_end - e820[i].addr;
> > +        }
> > +    }
> > +
> > +    /*
> > +     * And then we also need to adjust highmem.
> > +     */
> A single line comment should use the respective comment style.
#2. Compensate the loss

> > +    if ( add_high_mem )
> > +    {
> > +        for ( i = 0; i < memory_map.nr_map; i++ )
s/memory_map.nr_map/nr

> > +        {
> > +            if ( e820[i].type == E820_RAM &&
> > +                 e820[i].addr > (1ull << 32))
> > +                e820[i].size += add_high_mem;
> > +        }
> > +    }
> But looking at the code I think the comment should be extended to
> state that we currently expect there to be exactly one such RAM
> region.
I can add this at the beginning of #1 loop,

Its possible to relocate RAM to allocate sufficient MMIO previously so
low_mem_pgend would be changed over there. And here memory_map[] records 
the original low/high memory, so if low_mem_end is less than the 
original we need to revise low/high memory range in e820.

Thanks
Tiejun

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