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

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

> > +    /*
> > +     * And then we also need to adjust highmem.
> > +     */
> > +    if ( add_high_mem )
> > +    {
> > +        for ( i = 0; i < nr; i++ )
> > +        {
> > +            if ( e820[i].type == E820_RAM &&
> > +                 e820[i].addr == (1ull << 32))
> > +            {
> > +                e820[i].size += add_high_mem;
> > +                add_high_mem = 0;
> > +                break;
> > +            }
> > +        }
> > +    }
> > +
> > +    /* Or this is just populated by hvmloader itself. */
> This should probably say something like:
>
> "If there was no highmem region, we need to create one."
Okay, "If there was no highmem entry, we need to create one."

> > +    if ( add_high_mem )
> > +    {
> > +        /*
> > +         * hvmloader should always update hvm_info->high_mem_pgend
> > +         * when it relocates RAM anywhere.
> > +         */
> > +        BUG_ON( !hvm_info->high_mem_pgend );
> > +
> >           e820[nr].addr = ((uint64_t)1 << 32);
> >           e820[nr].size =
> >               ((uint64_t)hvm_info->high_mem_pgend << PAGE_SHIFT) - 
> > e820[nr].addr;
> In theory add_high_mem and hvm_info->high_mem_pgend << PAGE_SHIFT -
> 4GiB is the same, but it seems like asking for trouble to assume so
No, its not true in the first if( add_high_mem ) conditional.

Before we enter hvmloader, there are two cases:

#1. hvm_info->high_mem_pgend == 0

So we wouldn't have a highmem entry in e820. But hvmloader may relocate 
RAM upward highmem (add_high_mem) to get sufficient mmio, so 
hvm_info->high_mem_pgend is expanded somewhere (4GiB + add_high_mem).
Then we would fall into the second if( add_high_mem ) conditional.

#2. hvm_info->high_mem_pgend != 0

We always walk into the first if( add_high_mem ) conditional. But here 
"add_high_mem" just represents that highmem section expanded by 
hvmloader, its really not the whole higmem:(hvm_info->high_mem_pgend << 
PAGE_SHIFT - 4GiB).
Thanks
Tiejun

> without checking.
>
> Perhaps in the first if( add_high_mem ) conditional, you can
> BUG_ON(add_high_mem != ((hvm_info->high_mem_pgend << PAGE_SHIFT) -
> (ull1 << 32))) ?
>
> Other than that, this looks good, thanks.
>
>   -George
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel