Re: [Xen-devel] [PATCH v3 2/6] x86 / p2m: remove page_list check in p2m_alloc_table

From: "Durrant, Paul" <pdurrant@amazon.co.uk>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Wei Liu" <wl@xen.org>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"George Dunlap" <george.dunlap@citrix.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	"pdurrant@amzn.com" <pdurrant@amzn.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH v3 2/6] x86 / p2m: remove page_list check in p2m_alloc_table
Date: Fri, 6 Mar 2020 12:50:49 +0000	[thread overview]
Message-ID: <cddc7c8d65b6444a9c44f075fdca3b93@EX13D32EUC003.ant.amazon.com> (raw)
In-Reply-To: <9beb5129-68c0-ce68-2778-748a4485f6fb@suse.com>

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 06 March 2020 12:47
> To: Durrant, Paul <pdurrant@amazon.co.uk>
> Cc: pdurrant@amzn.com; xen-devel@lists.xenproject.org; Andrew Cooper <andrew.cooper3@citrix.com>;
> George Dunlap <george.dunlap@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau Monné <roger.pau@citrix.com>
> Subject: Re: [PATCH v3 2/6] x86 / p2m: remove page_list check in p2m_alloc_table
> 
> On 06.03.2020 13:07, Durrant, Paul wrote:
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: 06 March 2020 11:46
> >> To: pdurrant@amzn.com
> >> Cc: xen-devel@lists.xenproject.org; Durrant, Paul <pdurrant@amazon.co.uk>; Andrew Cooper
> >> <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>; Wei Liu <wl@xen.org>; Roger
> Pau
> >> Monné <roger.pau@citrix.com>
> >> Subject: Re: [PATCH v3 2/6] x86 / p2m: remove page_list check in p2m_alloc_table
> >>
> >> On 05.03.2020 13:45, pdurrant@amzn.com wrote:
> >>> From: Paul Durrant <pdurrant@amazon.com>
> >>>
> >>> There does not seem to be any justification for refusing to create the
> >>> domain's p2m table simply because it may have assigned pages.
> >>
> >> I think there is: If any such allocation had happened before, how
> >> would it be represented in the domain's p2m?
> >
> > Insertion into the p2m is a separate action from page allocation. Why should they be linked?
> 
> They are, because of how XENMEM_populate_physmap works. Yes,
> they _could_ be separate steps, but that's only a theoretical
> consideration.

Then surely the check should be in the XENMEM_populate_physmap code?

> 
> >>> Particularly
> >>> it prevents the prior allocation of PGC_extra pages.
> >>
> >> That's unfortunate, but will need taking care of differently then:
> >>
> >>> --- a/xen/arch/x86/mm/p2m.c
> >>> +++ b/xen/arch/x86/mm/p2m.c
> >>> @@ -695,14 +695,6 @@ int p2m_alloc_table(struct p2m_domain *p2m)
> >>>
> >>>      p2m_lock(p2m);
> >>>
> >>> -    if ( p2m_is_hostp2m(p2m)
> >>> -         && !page_list_empty(&d->page_list) )
> >>> -    {
> >>> -        P2M_ERROR("dom %d already has memory allocated\n", d->domain_id);
> >>> -        p2m_unlock(p2m);
> >>> -        return -EINVAL;
> >>> -    }
> >>
> >> Instead of checking the list to be empty, how about checking
> >> domain_tot_pages() to return zero?
> >
> > I could do that, and in fact my original code did, but with more
> > consideration the whole test just didn't make sense to me. Yes,
> > clearly the p2m has to be there before pages can be added into it,
> > but I can't see any reason why you couldn't even allocate the
> > entire guest RAM, then create the p2m and then add the pages into
> > it.
> 
> Right - more hypercalls (XENMEM_increase_reservation + operations
> like XENMAPSPACE_gmfn), and hence slower overall domain creation.
> Plus - XENMEM_increase_reservation is not very useful for
> translated domains, as it won't return the MFNs allocated, and
> there's no way to specify where they should appear in GFN space.
> Hence in practice I don't see how this whole operation could
> work without XENMEM_populate_physmap.
> 

Oh, it would mean a big change in the tools etc. so I'm not saying it's a good idea or even possible at the moment. I was just pointing out that, as far as the lower layers of code in Xen go, page allocation and p2m insertion are distinct actions.

  Paul

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel