PVH dom0 memory setup

Commit Message

Roger Pau Monné Sept. 8, 2017, 5:11 p.m. UTC

On Fri, Sep 08, 2017 at 10:56:33AM -0400, Boris Ostrovsky wrote:
> I am slightly confused by the use of 'need_paging' variable in
> dom0_compute_nr_pages().
> 
> Because paging_mode_hap() and iommu_hap_pt_share are (almost?) always
> true, we are not reducing available memory for PVH dom0 by page tables
> size. But then in pvh_setup_p2m() we do use this memory by
> paging_set_allocation(). And from what I've seen we then may run our of
> heap pages when populating memory map (in the 'for' loop below).
> 
> Am I not reading this correctly?

Yes, I think you are reading this correctly. dom0_compute_nr_pages
should set need_paging if the domain type is hvm AFAICT, because hap
also consumes memory for it's page tables. Do you have a reliable way
to trigger this?

I was thinking of a fix along the lines of:

---8<---

Comments

Boris Ostrovsky Sept. 8, 2017, 7:04 p.m. UTC | #1

On 09/08/2017 01:11 PM, Roger Pau Monné wrote:
> On Fri, Sep 08, 2017 at 10:56:33AM -0400, Boris Ostrovsky wrote:
>> I am slightly confused by the use of 'need_paging' variable in
>> dom0_compute_nr_pages().
>>
>> Because paging_mode_hap() and iommu_hap_pt_share are (almost?) always
>> true, we are not reducing available memory for PVH dom0 by page tables
>> size. But then in pvh_setup_p2m() we do use this memory by
>> paging_set_allocation(). And from what I've seen we then may run our of
>> heap pages when populating memory map (in the 'for' loop below).
>>
>> Am I not reading this correctly?
> Yes, I think you are reading this correctly. dom0_compute_nr_pages
> should set need_paging if the domain type is hvm AFAICT, because hap
> also consumes memory for it's page tables. Do you have a reliable way
> to trigger this?
>
> I was thinking of a fix along the lines of:

Yes, this is essentially what I ended up doing and yes it does fix this
problem.

I wasn't sure whether this would work for !HAP case (which we still
support).

-boris


>
> ---8<---
> diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
> index f616b99ddc..424192a7c4 100644
> --- a/xen/arch/x86/dom0_build.c
> +++ b/xen/arch/x86/dom0_build.c
> @@ -263,8 +263,7 @@ unsigned long __init dom0_compute_nr_pages(
>              avail -= max_pdx >> s;
>      }
>  
> -    need_paging = is_hvm_domain(d) &&
> -        (!iommu_hap_pt_share || !paging_mode_hap(d));
> +    need_paging = is_hvm_domain(d);
>      for ( ; ; need_paging = false )
>      {
>          nr_pages = dom0_nrpages;
>

Roger Pau Monné Sept. 11, 2017, 8:49 a.m. UTC | #2

On Fri, Sep 08, 2017 at 03:04:30PM -0400, Boris Ostrovsky wrote:
> On 09/08/2017 01:11 PM, Roger Pau Monné wrote:
> > On Fri, Sep 08, 2017 at 10:56:33AM -0400, Boris Ostrovsky wrote:
> >> I am slightly confused by the use of 'need_paging' variable in
> >> dom0_compute_nr_pages().
> >>
> >> Because paging_mode_hap() and iommu_hap_pt_share are (almost?) always
> >> true, we are not reducing available memory for PVH dom0 by page tables
> >> size. But then in pvh_setup_p2m() we do use this memory by
> >> paging_set_allocation(). And from what I've seen we then may run our of
> >> heap pages when populating memory map (in the 'for' loop below).
> >>
> >> Am I not reading this correctly?
> > Yes, I think you are reading this correctly. dom0_compute_nr_pages
> > should set need_paging if the domain type is hvm AFAICT, because hap
> > also consumes memory for it's page tables. Do you have a reliable way
> > to trigger this?
> >
> > I was thinking of a fix along the lines of:
> 
> Yes, this is essentially what I ended up doing and yes it does fix this
> problem.
> 
> I wasn't sure whether this would work for !HAP case (which we still
> support).

It should work fine in both cases, but we should check whether the
calculation of the reservation is done in the same way as libxl.

AFAICT even with the change proposed I don't think
dom0_compute_nr_pages will handle the !iommu_hap_pt_share case
properly (ie: I don't see specific memory for the IOMMU pages tables
being reserved anywhere).

Roger.

Jan Beulich Sept. 11, 2017, 9:35 a.m. UTC | #3

>>> On 11.09.17 at 10:49, <[e-mail]> wrote:
> AFAICT even with the change proposed I don't think
> dom0_compute_nr_pages will handle the !iommu_hap_pt_share case
> properly (ie: I don't see specific memory for the IOMMU pages tables
> being reserved anywhere).

Which (sadly) is in line with what the tool stack does (or really fails
to do), despite me having pointed this out a few times before.

Jan

Patch

diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
index f616b99ddc..424192a7c4 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -263,8 +263,7 @@  unsigned long __init dom0_compute_nr_pages(
             avail -= max_pdx >> s;
     }
 
-    need_paging = is_hvm_domain(d) &&
-        (!iommu_hap_pt_share || !paging_mode_hap(d));
+    need_paging = is_hvm_domain(d);
     for ( ; ; need_paging = false )
     {
         nr_pages = dom0_nrpages;