[PATCH 0/5] x86/dom0: minor fixes and improvements to PVH builder

[PATCH 0/5] x86/dom0: minor fixes and improvements to PVH builder

[Roger Pau Monne]

Hello,

This series contains some trivial bug fixes for the PVH dom0 builder and
an improvement when filling the p2m so that alignment is taken into
account when allocating and populating the p2m.

Thanks, Roger.

Roger Pau Monne (5):
  x86/dom0: fix wording of PVH Dom0 error message
  x86/dom0: allow stealing RAM from a region that starts in the low 1MB
  x86/dom0: add missing flag to printf format for PVH
  x86/dom0: propagate guest_physmap_add_page error code
  x86/dom0: take alignment into account when populating p2m in PVH mode

 xen/arch/x86/hvm/dom0_build.c | 66 +++++++++++++++++++++++++----------
 1 file changed, 48 insertions(+), 18 deletions(-)

-- 
2.17.2 (Apple Git-113)


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

[PATCH 1/5] x86/dom0: fix wording of PVH Dom0 error message

[Roger Pau Monne]

No functional change.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/hvm/dom0_build.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index 12c20a4b66..91dc27dc3e 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -739,7 +739,7 @@ static int __init pvh_setup_acpi_madt(struct domain *d, paddr_t *addr)
     /* Place the new MADT in guest memory space. */
     if ( pvh_steal_ram(d, size, 0, GB(4), addr) )
     {
-        printk("Unable to find allocate guest RAM for MADT\n");
+        printk("Unable to steal guest RAM for MADT\n");
         rc = -ENOMEM;
         goto out;
     }
-- 
2.17.2 (Apple Git-113)


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

[PATCH 2/5] x86/dom0: allow stealing RAM from a region that starts in the low 1MB

[Roger Pau Monne]

As long as the memory stolen is always above 1MB. This allows the PVH
Dom0 builder to be used on a memory map that only has a single RAM
region starting at 0.

Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/hvm/dom0_build.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index 91dc27dc3e..24cc15f28b 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -154,12 +154,13 @@ static int __init pvh_steal_ram(struct domain *d, unsigned long size,
     {
         struct e820entry *entry = &d->arch.e820[i];
 
-        if ( entry->type != E820_RAM || entry->addr + entry->size > limit ||
-             entry->addr < MB(1) )
+        if ( entry->type != E820_RAM || entry->addr + entry->size > limit )
             continue;
 
         *addr = (entry->addr + entry->size - size) & ~(align - 1);
-        if ( *addr < entry->addr )
+        if ( *addr < entry->addr ||
+             /* Don't steal from the low 1MB due to the copying done there. */
+             *addr < MB(1) )
             continue;
 
         entry->size = *addr - entry->addr;
-- 
2.17.2 (Apple Git-113)


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

[PATCH 3/5] x86/dom0: add missing flag to printf format for PVH

[Roger Pau Monne]

No functional change.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/hvm/dom0_build.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index 24cc15f28b..c0eb9cb953 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -124,7 +124,7 @@ static int __init pvh_populate_memory_range(struct domain *d,
                                     order);
         if ( rc != 0 )
         {
-            printk("Failed to populate memory: [%#lx,%lx): %d\n",
+            printk("Failed to populate memory: [%#lx,%#lx): %d\n",
                    start, start + (1UL << order), rc);
             return -ENOMEM;
         }
-- 
2.17.2 (Apple Git-113)


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

[PATCH 4/5] x86/dom0: propagate guest_physmap_add_page error code

[Roger Pau Monne]

No functional change.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/hvm/dom0_build.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index c0eb9cb953..7ea29c443a 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -126,7 +126,7 @@ static int __init pvh_populate_memory_range(struct domain *d,
         {
             printk("Failed to populate memory: [%#lx,%#lx): %d\n",
                    start, start + (1UL << order), rc);
-            return -ENOMEM;
+            return rc;
         }
         start += 1UL << order;
         nr_pages -= 1UL << order;
-- 
2.17.2 (Apple Git-113)


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

[PATCH 5/5] x86/dom0: take alignment into account when populating p2m in PVH mode

[Roger Pau Monne]

Current code that allocates memory and populates the p2m for PVH Dom0
doesn't take the address alignment into account, this can lead to high
order allocations that start on a non-aligned address to be broken
down into lower order entries on the p2m page tables.

Fix this by taking into account the p2m page sizes and alignment
requirements when allocating the memory and populating the p2m.

Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/hvm/dom0_build.c | 53 +++++++++++++++++++++++++++--------
 1 file changed, 41 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index 7ea29c443a..2d85e808b6 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -91,32 +91,61 @@ static int __init pvh_populate_memory_range(struct domain *d,
                                             unsigned long start,
                                             unsigned long nr_pages)
 {
-    unsigned int order = MAX_ORDER, i = 0;
+    struct {
+        unsigned long align;
+        unsigned int order;
+    } static const __initconst orders[] = {
+        /* NB: must be sorted by decreasing size. */
+        { .align = PFN_DOWN(GB(1)), .order = PAGE_ORDER_1G },
+        { .align = PFN_DOWN(MB(2)), .order = PAGE_ORDER_2M },
+        { .align = PFN_DOWN(KB(4)), .order = PAGE_ORDER_4K },
+    };
+    unsigned int max_order = orders[0].order, i = 0;
     struct page_info *page;
     int rc;
 #define MAP_MAX_ITER 64
 
     while ( nr_pages != 0 )
     {
-        unsigned int range_order = get_order_from_pages(nr_pages + 1);
+        unsigned int order, j;
 
-        order = min(range_order ? range_order - 1 : 0, order);
+        for ( j = 0; j < ARRAY_SIZE(orders); j++ )
+            if ( IS_ALIGNED(start, orders[j].align) &&
+                 nr_pages >= (1UL << orders[j].order) )
+            {
+                order = orders[j].order;
+                break;
+            }
+
+        if ( j == ARRAY_SIZE(orders) )
+        {
+           printk("Unable to find allocation order for [%#lx,%#lx)\n",
+                  start, start + nr_pages);
+           return -EINVAL;
+        }
+
+        order = min(order, max_order);
         page = alloc_domheap_pages(d, order, dom0_memflags | MEMF_no_scrub);
         if ( page == NULL )
         {
-            if ( order == 0 && dom0_memflags )
-            {
-                /* Try again without any dom0_memflags. */
-                dom0_memflags = 0;
-                order = MAX_ORDER;
-                continue;
-            }
-            if ( order == 0 )
+            if ( order == orders[ARRAY_SIZE(orders) - 1].order )
             {
+                if ( dom0_memflags )
+                {
+                    /* Try again without any dom0_memflags. */
+                    max_order = orders[0].order;
+                    dom0_memflags = 0;
+                    continue;
+                }
                 printk("Unable to allocate memory with order 0!\n");
                 return -ENOMEM;
             }
-            order--;
+            for ( j = 0; j < ARRAY_SIZE(orders) - 1; j++ )
+                if ( order == orders[j].order )
+                {
+                    max_order = orders[j + 1].order;
+                    break;
+                }
             continue;
         }
 
-- 
2.17.2 (Apple Git-113)


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

Re: [PATCH 1/5] x86/dom0: fix wording of PVH Dom0 error message

[Andrew Cooper]

On 27/12/2018 15:26, Roger Pau Monne wrote:
> No functional change.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH 2/5] x86/dom0: allow stealing RAM from a region that starts in the low 1MB

[Andrew Cooper]

On 27/12/2018 15:26, Roger Pau Monne wrote:
> As long as the memory stolen is always above 1MB. This allows the PVH
> Dom0 builder to be used on a memory map that only has a single RAM
> region starting at 0.
>
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Tested-by:  Andrew Cooper <andrew.cooper3@citrix.com>


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

Re: [PATCH 3/5] x86/dom0: add missing flag to printf format for PVH

[Andrew Cooper]

On 27/12/2018 15:26, Roger Pau Monne wrote:
> No functional change.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH 4/5] x86/dom0: propagate guest_physmap_add_page error code

[Andrew Cooper]

On 27/12/2018 15:26, Roger Pau Monne wrote:
> No functional change.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH 5/5] x86/dom0: take alignment into account when populating p2m in PVH mode

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 1803 bytes --]

On 27/12/2018 15:26, Roger Pau Monne wrote:
> Current code that allocates memory and populates the p2m for PVH Dom0
> doesn't take the address alignment into account, this can lead to high
> order allocations that start on a non-aligned address to be broken
> down into lower order entries on the p2m page tables.
>
> Fix this by taking into account the p2m page sizes and alignment
> requirements when allocating the memory and populating the p2m.
>
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

I've committed patches 1-4 because they are trivial.

This patch does fix the alignment issue, but does so at a rather large cost.

The sequence (without any of the E820 series) is now:

(XEN) *** Building a PVH Dom0 ***
(XEN) pvh_populate_memory_range(d0, 0, a0)
(XEN) guest_physmap_add_page(d0, 00000, 100be1, 0 = 4kB)
...
(XEN) guest_physmap_add_page(d0, 0009f, 100b42, 0 = 4kB)
(XEN) pvh_populate_memory_range(d0, 100, 360)
(XEN) guest_physmap_add_page(d0, 00100, 100b41, 0 = 4kB)
...
(XEN) guest_physmap_add_page(d0, 001ff, 100a42, 0 = 4kB)
(XEN) guest_physmap_add_page(d0, 00200, 100800, 9 = 2048kB)
(XEN) guest_physmap_add_page(d0, 00400, 100a41, 0 = 4kB)
...
(XEN) guest_physmap_add_page(d0, 0045f, 1007e2, 0 = 4kB)


So overall, the 2M superpage is created in the middle, but all the 4k
entries are made with a single call to guest_physmap_add_page() at a
time, which is going to be very inefficient with the P2M lock.

Ideally, we should make a single order 8 allocation at 1M, then the
order 9 at 2M.

I think it might be better to try and crib from the HVM domainbuilder
code, which copes with collecting together the head and tail of
non-aligned allocations while still making an order > 0 requests where
possible.

~Andrew

[-- Attachment #1.2: Type: text/html, Size: 2479 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

Re: [PATCH 5/5] x86/dom0: take alignment into account when populating p2m in PVH mode

[Roger Pau Monné]

On Thu, Dec 27, 2018 at 08:14:40PM +0000, Andrew Cooper wrote:
> On 27/12/2018 15:26, Roger Pau Monne wrote:
> > Current code that allocates memory and populates the p2m for PVH Dom0
> > doesn't take the address alignment into account, this can lead to high
> > order allocations that start on a non-aligned address to be broken
> > down into lower order entries on the p2m page tables.
> >
> > Fix this by taking into account the p2m page sizes and alignment
> > requirements when allocating the memory and populating the p2m.
> >
> > Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> I've committed patches 1-4 because they are trivial.
> 
> This patch does fix the alignment issue, but does so at a rather large cost.
> 
> The sequence (without any of the E820 series) is now:
> 
> (XEN) *** Building a PVH Dom0 ***
> (XEN) pvh_populate_memory_range(d0, 0, a0)
> (XEN) guest_physmap_add_page(d0, 00000, 100be1, 0 = 4kB)
> ...
> (XEN) guest_physmap_add_page(d0, 0009f, 100b42, 0 = 4kB)
> (XEN) pvh_populate_memory_range(d0, 100, 360)
> (XEN) guest_physmap_add_page(d0, 00100, 100b41, 0 = 4kB)
> ...
> (XEN) guest_physmap_add_page(d0, 001ff, 100a42, 0 = 4kB)
> (XEN) guest_physmap_add_page(d0, 00200, 100800, 9 = 2048kB)
> (XEN) guest_physmap_add_page(d0, 00400, 100a41, 0 = 4kB)
> ...
> (XEN) guest_physmap_add_page(d0, 0045f, 1007e2, 0 = 4kB)
> 
> 
> So overall, the 2M superpage is created in the middle, but all the 4k
> entries are made with a single call to guest_physmap_add_page() at a
> time, which is going to be very inefficient with the P2M lock.
> 
> Ideally, we should make a single order 8 allocation at 1M, then the
> order 9 at 2M.

Right, this new approach limit the allocation orders to the orders of
the page, super page or huge pages, thus preventing using the orders
in the middle.

> I think it might be better to try and crib from the HVM domainbuilder
> code, which copes with collecting together the head and tail of
> non-aligned allocations while still making an order > 0 requests where
> possible.

I find the HVM p2m domain builder code extremely difficult to follow,
and hence I would like to avoid introducing such complexity here.

I've reworked the patch so that all possible orders are used when
populating the p2m, let me post that version.

Thanks, Roger.

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

Re: [PATCH 3/5] x86/dom0: add missing flag to printf format for PVH

[Jan Beulich]

>>> On 27.12.18 at 16:26, <roger.pau@citrix.com> wrote:
> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -124,7 +124,7 @@ static int __init pvh_populate_memory_range(struct domain *d,
>                                      order);
>          if ( rc != 0 )
>          {
> -            printk("Failed to populate memory: [%#lx,%lx): %d\n",
> +            printk("Failed to populate memory: [%#lx,%#lx): %d\n",
>                     start, start + (1UL << order), rc);

I think the change would better have been the other way around:
Serial console bandwidth considerations (like also mentioned
elsewhere on a few occasions) should lead to a preference of
shorter messages, as long as things don't become ambiguous.
Ambiguity is not an issue here - printing of MFNs in decimal is
entirely useless imo, and printing of error codes in hex (the more
that they're negative) should be considered bad practice at best.
Hence a mix of hex and dec here would be not an issue at all,
without any 0x prefixes.

But this is an error path, so my reply is not meant to be a call
for further action in this specific case.

Jan



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