Re: [Xen-devel] [PATCH v3 2/6] x86 / p2m: remove page_list check in p2m_alloc_table
> -Original Message-
> From: Jan Beulich
> Sent: 06 March 2020 12:47
> To: Durrant, Paul
> Cc: pdurr...@amzn.com; xen-devel@lists.xenproject.org; Andrew Cooper
> ;
> George Dunlap ; Wei Liu ; Roger Pau
> Monné
> 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
> >> Sent: 06 March 2020 11:46
> >> To: pdurr...@amzn.com
> >> Cc: xen-devel@lists.xenproject.org; Durrant, Paul ;
> >> Andrew Cooper
> >> ; George Dunlap ; Wei
> >> Liu ; Roger
> Pau
> >> Monné
> >> Subject: Re: [PATCH v3 2/6] x86 / p2m: remove page_list check in
> >> p2m_alloc_table
> >>
> >> On 05.03.2020 13:45, pdurr...@amzn.com wrote:
> >>> From: Paul Durrant
> >>>
> >>> 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(>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
Re: [Xen-devel] [PATCH v3 2/6] x86 / p2m: remove page_list check in p2m_alloc_table
On 06.03.2020 13:50, Durrant, Paul wrote: >> -Original Message- >> From: Jan Beulich >> Sent: 06 March 2020 12:47 >> To: Durrant, Paul >> Cc: pdurr...@amzn.com; xen-devel@lists.xenproject.org; Andrew Cooper >> ; >> George Dunlap ; Wei Liu ; Roger Pau >> Monné >> 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 Sent: 06 March 2020 11:46 To: pdurr...@amzn.com Cc: xen-devel@lists.xenproject.org; Durrant, Paul ; Andrew Cooper ; George Dunlap ; Wei Liu ; Roger >> Pau Monné Subject: Re: [PATCH v3 2/6] x86 / p2m: remove page_list check in p2m_alloc_table On 05.03.2020 13:45, pdurr...@amzn.com wrote: > From: Paul Durrant > > 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? How that? populate-physmap can be called any number of times. We can't refuse a 2nd call there just because a 1st one had happened already. Or did you mean the inverse check (i.e. that there already is a p2m)? This surely wouldn't be a bad idea, as otherwise both ept_get_entry() and p2m_pt_get_entry() would blindly map MFN 0. But adding such a check wouldn't eliminate the reason to also have the check that you're proposing to drop. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [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
>> Sent: 06 March 2020 11:46
>> To: pdurr...@amzn.com
>> Cc: xen-devel@lists.xenproject.org; Durrant, Paul ;
>> Andrew Cooper
>> ; George Dunlap ; Wei
>> Liu ; Roger Pau
>> Monné
>> Subject: Re: [PATCH v3 2/6] x86 / p2m: remove page_list check in
>> p2m_alloc_table
>>
>> On 05.03.2020 13:45, pdurr...@amzn.com wrote:
>>> From: Paul Durrant
>>>
>>> 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.
>>> 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(>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.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 2/6] x86 / p2m: remove page_list check in p2m_alloc_table
> -Original Message-
> From: Jan Beulich
> Sent: 06 March 2020 11:46
> To: pdurr...@amzn.com
> Cc: xen-devel@lists.xenproject.org; Durrant, Paul ;
> Andrew Cooper
> ; George Dunlap ; Wei
> Liu ; Roger Pau
> Monné
> Subject: Re: [PATCH v3 2/6] x86 / p2m: remove page_list check in
> p2m_alloc_table
>
> On 05.03.2020 13:45, pdurr...@amzn.com wrote:
> > From: Paul Durrant
> >
> > 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?
>
> > 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(>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.
Paul
>
> Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 2/6] x86 / p2m: remove page_list check in p2m_alloc_table
On 05.03.2020 13:45, pdurr...@amzn.com wrote:
> From: Paul Durrant
>
> 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?
> 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(>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?
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
