Re: [Xen-devel] [PATCH v2 15/30] xen/x86: populate PVHv2 Dom0 physical memory map

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.10.16 at 16:01, <roger.pau@xxxxxxxxxx> wrote:
> On Tue, Oct 04, 2016 at 05:16:09AM -0600, Jan Beulich wrote:
>> >>> On 04.10.16 at 11:12, <roger.pau@xxxxxxxxxx> wrote:
>> > On Fri, Sep 30, 2016 at 09:52:56AM -0600, Jan Beulich wrote:
>> >> >>> On 27.09.16 at 17:57, <roger.pau@xxxxxxxxxx> wrote:
>> >> > @@ -336,7 +343,8 @@ static unsigned long __init compute_dom0_nr_pages(
>> >> >          avail -= dom0_paging_pages(d, nr_pages);
>> >> >      }
>> >> >  
>> >> > -    if ( (parms->p2m_base == UNSET_ADDR) && (dom0_nrpages <= 0) &&
>> >> > +    if ( is_pv_domain(d) &&
>> >> > +         (parms->p2m_base == UNSET_ADDR) && (dom0_nrpages <= 0) &&
>> >> 
>> >> Perhaps better to simply force parms->p2m_base to UNSET_ADDR
>> >> earlier on?
>> > 
>> > p2m_base is already unconditionally set to UNSET_ADDR for PVHv2 domains, 
>> > hence the added is_pv_domain check in order to make sure that PVHv2 guests 
>> > don't get into that branch, which AFAICT is only relevant to PV guests.
>> 
>> This reads contradictory: If it's set to UNSET_ADDR, why the extra
>> check?
> 
> On PVHv2 p2m_base == UNSET_ADDR already, so the extra check is to make sure 
> PVHv2 guests don't execute the code inside of the if condition, which is 
> for classic PV guests (note that the check is not against != UNSET_ADDR). Or 
> maybe I'm missing something?

No, I have to apologize - I read things the wrong way round.
Thanks for bearing with me.

>> >> > @@ -1657,15 +1679,238 @@ out:
>> >> >      return rc;
>> >> >  }
>> >> >  
>> >> > +/* Populate an HVM memory range using the biggest possible order. */
>> >> > +static void __init hvm_populate_memory_range(struct domain *d, 
>> >> > uint64_t start,
>> >> > +                                             uint64_t size)
>> >> > +{
>> >> > +    static unsigned int __initdata memflags = 
>> >> > MEMF_no_dma|MEMF_exact_node;
>> >> > +    unsigned int order;
>> >> > +    struct page_info *page;
>> >> > +    int rc;
>> >> > +
>> >> > +    ASSERT(IS_ALIGNED(size, PAGE_SIZE) && IS_ALIGNED(start, 
>> >> > PAGE_SIZE));
>> >> > +
>> >> > +    order = MAX_ORDER;
>> >> > +    while ( size != 0 )
>> >> > +    {
>> >> > +        order = min(get_order_from_bytes_floor(size), order);
>> >> > +        page = alloc_domheap_pages(d, order, memflags);
>> >> > +        if ( page == NULL )
>> >> > +        {
>> >> > +            if ( order == 0 && memflags )
>> >> > +            {
>> >> > +                /* Try again without any memflags. */
>> >> > +                memflags = 0;
>> >> > +                order = MAX_ORDER;
>> >> > +                continue;
>> >> > +            }
>> >> > +            if ( order == 0 )
>> >> > +                panic("Unable to allocate memory with order 0!\n");
>> >> > +            order--;
>> >> > +            continue;
>> >> > +        }
>> >> 
>> >> Is it not possible to utilize alloc_chunk() here?
>> > 
>> > Not really, alloc_chunk will do a ceil calculation of the number of needed 
>> > pages, which means we end up allocating bigger chunks than needed and then 
>> > free them. I prefer this approach, since we always request the exact 
>> > memory 
>> > that's required, so there's no need to free leftover pages.
>> 
>> Hmm, in that case at least some of the shared logic would be nice to
>> get abstracted out.
> 
> TBH, I don't see much benefit in that, alloc_chunk works fine for PV guests 
> because Xen ends up walking the list of returned pages and mapping them one 
> to one. This is IMHO not what should be done for PVH guests, and instead the 
> caller needs to know the actual order of the allocated chunk, so it can pass 
> it to guest_physmap_add_page. ATM it's a very simple function that's clear, 
> if I start mixing up bits of alloc_chunk it's going to get more complex, and 
> I would like to avoid that, so that PVH Dom0 build doesn't end up like 
> current PV Dom0 build.

Hmm - I did say "abstract out", not "re-use". The way how memflags
get set and decreasing orders get tried in your code looks suspiciously
similar to what alloc_chunk() does.

>> > In the RFC series we also spoke about placing the MADT in a different 
>> > position that the native one, which would mean that we would end up 
>> > stealing 
>> > some space from a RAM region in order to place it, so that we wouldn't 
>> > have 
>> > to do this accounting.
>> 
>> Putting the new MADT at the same address as the old one won't work
>> anyway, again because possibly vCPU-s > pCPU-s.
> 
> Yes, although from my tests doing CPU over-allocation on HVM guests works 
> very badly.

Interesting. I didn't try recently, but I recall having tried a longer
while ago without seeing severe issues.

Jan

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