Re: [PATCH V2 3/4] xen/unpopulated-alloc: Add mechanism to use Xen resource

[Oleksandr]

On 24.11.21 07:16, Juergen Gross wrote:

Hi Juergen


On 23.11.21 17:46, Oleksandr wrote:


On 20.11.21 04:19, Stefano Stabellini wrote:

Hi Stefano, Juergen, all



Juergen please see the bottom of the email

On Fri, 19 Nov 2021, Oleksandr wrote:

On 19.11.21 02:59, Stefano Stabellini wrote:

On Tue, 9 Nov 2021, Oleksandr wrote:

On 28.10.21 19:37, Stefano Stabellini wrote:

Hi Stefano

I am sorry for the late response.


On Tue, 26 Oct 2021, Oleksandr Tyshchenko wrote:

From: Oleksandr Tyshchenko 

The main reason of this change is that unpopulated-alloc
code cannot be used in its current form on Arm, but there
is a desire to reuse it to avoid wasting real RAM pages
for the grant/foreign mappings.

The problem is that system "iomem_resource" is used for
the address space allocation, but the really unallocated
space can't be figured out precisely by the domain on Arm
without hypervisor involvement. For example, not all device
I/O regions are known by the time domain starts creating
grant/foreign mappings. And following the advise from
"iomem_resource" we might end up reusing these regions by
a mistake. So, the hypervisor which maintains the P2M for
the domain is in the best position to provide unused regions
of guest physical address space which could be safely used
to create grant/foreign mappings.

Introduce new helper arch_xen_unpopulated_init() which purpose
is to create specific Xen resource based on the memory regions
provided by the hypervisor to be used as unused space for Xen
scratch pages.

If arch doesn't implement arch_xen_unpopulated_init() to
initialize Xen resource the default "iomem_resource" will be used.
So the behavior on x86 won't be changed.

Also fall back to allocate xenballooned pages (steal real RAM
pages) if we do not have any suitable resource to work with and
as the result we won't be able to provide unpopulated pages.

Signed-off-by: Oleksandr Tyshchenko 


---
Changes RFC -> V2:
  - new patch, instead of
   "[RFC PATCH 2/2] xen/unpopulated-alloc: Query hypervisor to
provide
unallocated space"
---
    drivers/xen/unpopulated-alloc.c | 89
+++--
    include/xen/xen.h   |  2 +
    2 files changed, 88 insertions(+), 3 deletions(-)

diff --git a/drivers/xen/unpopulated-alloc.c
b/drivers/xen/unpopulated-alloc.c
index a03dc5b..1f1d8d8 100644
--- a/drivers/xen/unpopulated-alloc.c
+++ b/drivers/xen/unpopulated-alloc.c
@@ -8,6 +8,7 @@
  #include 
    +#include 
    #include 
    #include 
    @@ -15,13 +16,29 @@ static DEFINE_MUTEX(list_lock);
    static struct page *page_list;
    static unsigned int list_count;
    +static struct resource *target_resource;
+static struct resource xen_resource = {
+    .name = "Xen unused space",
+};
+
+/*
+ * If arch is not happy with system "iomem_resource" being 
used for
+ * the region allocation it can provide it's own view by 
initializing
+ * "xen_resource" with unused regions of guest physical 
address space

+ * provided by the hypervisor.
+ */
+int __weak arch_xen_unpopulated_init(struct resource *res)
+{
+    return -ENOSYS;
+}
+
    static int fill_list(unsigned int nr_pages)
    {
    struct dev_pagemap *pgmap;
-    struct resource *res;
+    struct resource *res, *tmp_res = NULL;
    void *vaddr;
    unsigned int i, alloc_pages = round_up(nr_pages,
PAGES_PER_SECTION);
-    int ret = -ENOMEM;
+    int ret;
  res = kzalloc(sizeof(*res), GFP_KERNEL);
    if (!res)
@@ -30,7 +47,7 @@ static int fill_list(unsigned int nr_pages)
    res->name = "Xen scratch";
    res->flags = IORESOURCE_MEM | IORESOURCE_BUSY;
    -    ret = allocate_resource(_resource, res,
+    ret = allocate_resource(target_resource, res,
    alloc_pages * PAGE_SIZE, 0, -1,
    PAGES_PER_SECTION * PAGE_SIZE, NULL,
NULL);
    if (ret < 0) {
@@ -38,6 +55,31 @@ static int fill_list(unsigned int nr_pages)
    goto err_resource;
    }
    +    /*
+ * Reserve the region previously allocated from Xen resource
to avoid
+ * re-using it by someone else.
+ */
+    if (target_resource != _resource) {
+    tmp_res = kzalloc(sizeof(*tmp_res), GFP_KERNEL);
+    if (!res) {
+    ret = -ENOMEM;
+    goto err_insert;
+    }
+
+    tmp_res->name = res->name;
+    tmp_res->start = res->start;
+    tmp_res->end = res->end;
+    tmp_res->flags = res->flags;
+
+    ret = insert_resource(_resource, tmp_res);
+    if (ret < 0) {
+    pr_err("Cannot insert IOMEM resource [%llx -
%llx]\n",
+   tmp_res->start, tmp_res->end);
+    kfree(tmp_res);
+    goto err_insert;
+    }
+    }

I am a bit confused.. why do we need to do this? Who could be
erroneously re-using the region? Are you saying that the next time
allocate_resource is called it could find the same region again? It
doesn't seem possible?
No, as I understand the allocate_resource() being called for 

Re: [PATCH V2 3/4] xen/unpopulated-alloc: Add mechanism to use Xen resource

[Oleksandr]

On 23.11.21 23:25, Stefano Stabellini wrote:

Hi Stefano


On Tue, 23 Nov 2021, Oleksandr wrote:

Actually after reading your replies and explanation I changed opinion: I
think we do need the fallback because Linux cannot really assume that
it is running on "new Xen" so it definitely needs to keep working if
CONFIG_XEN_UNPOPULATED_ALLOC is enabled and the extended regions are not
advertised.

I think we'll have to roll back some of the changes introduced by
121f2faca2c0a. That's because even if CONFIG_XEN_UNPOPULATED_ALLOC is
enabled we cannot know if we can use unpopulated-alloc or whether we
have to use alloc_xenballooned_pages until we parse the /hypervisor node
in device tree at runtime.

Exactly!



In short, we cannot switch between unpopulated-alloc and
alloc_xenballooned_pages at build time, we have to do it at runtime
(boot time).

+1


I created a patch to partially revert 121f2faca2c0a "xen/balloon: rename
alloc/free_xenballooned_pages".

If there is no objections I will add it to V3 (which is almost ready, except
the fallback bits). Could you please tell me what do you think?
  
It makes sense to me. You can add my Reviewed-by.


Great, thank you!




  

 From dc79bcd425358596d95e715a8bd8b81deaaeb703 Mon Sep 17 00:00:00 2001
From: Oleksandr Tyshchenko 
Date: Tue, 23 Nov 2021 18:14:41 +0200
Subject: [PATCH] xen/balloon: Bring alloc(free)_xenballooned_pages helpers
  back

This patch rolls back some of the changes introduced by commit
121f2faca2c0a "xen/balloon: rename alloc/free_xenballooned_pages"
in order to make possible to still allocate xenballooned pages
if CONFIG_XEN_UNPOPULATED_ALLOC is enabled.

On Arm the unpopulated pages will be allocated on top of extended
regions provided by Xen via device-tree (the subsequent patches
will add required bits to support unpopulated-alloc feature on Arm).
The problem is that extended regions feature has been introduced
into Xen quite recently (during 4.16 release cycle). So this
effectively means that Linux must only use unpopulated-alloc on Arm
if it is running on "new Xen" which advertises these regions.
But, it will only be known after parsing the "hypervisor" node
at boot time, so before doing that we cannot assume anything.

In order to keep working if CONFIG_XEN_UNPOPULATED_ALLOC is enabled
and the extended regions are not advertised (Linux is running on
"old Xen", etc) we need the fallback to alloc_xenballooned_pages().

This way we wouldn't reduce the amount of memory usable (wasting
RAM pages) for any of the external mappings anymore (and eliminate
XSA-300) with "new Xen", but would be still functional ballooning
out RAM pages with "old Xen".

Also rename alloc(free)_xenballooned_pages to xen_alloc(free)_ballooned_pages.

Signed-off-by: Oleksandr Tyshchenko 
---
  drivers/xen/balloon.c | 20 +---
  include/xen/balloon.h |  3 +++
  include/xen/xen.h |  6 ++
  3 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
index ba2ea11..a2c4fc49 100644
--- a/drivers/xen/balloon.c
+++ b/drivers/xen/balloon.c
@@ -581,7 +581,6 @@ void balloon_set_new_target(unsigned long target)
  }
  EXPORT_SYMBOL_GPL(balloon_set_new_target);

-#ifndef CONFIG_XEN_UNPOPULATED_ALLOC
  static int add_ballooned_pages(unsigned int nr_pages)
  {
  enum bp_state st;
@@ -610,12 +609,12 @@ static int add_ballooned_pages(unsigned int nr_pages)
  }

  /**
- * xen_alloc_unpopulated_pages - get pages that have been ballooned out
+ * xen_alloc_ballooned_pages - get pages that have been ballooned out
   * @nr_pages: Number of pages to get
   * @pages: pages returned
   * @return 0 on success, error otherwise
   */
-int xen_alloc_unpopulated_pages(unsigned int nr_pages, struct page **pages)
+int xen_alloc_ballooned_pages(unsigned int nr_pages, struct page **pages)
  {
  unsigned int pgno = 0;
  struct page *page;
@@ -652,23 +651,23 @@ int xen_alloc_unpopulated_pages(unsigned int nr_pages,
struct page **pages)
  return 0;
   out_undo:
  mutex_unlock(_mutex);
-    xen_free_unpopulated_pages(pgno, pages);
+    xen_free_ballooned_pages(pgno, pages);
  /*
-     * NB: free_xenballooned_pages will only subtract pgno pages, but since
+     * NB: xen_free_ballooned_pages will only subtract pgno pages, but since
   * target_unpopulated is incremented with nr_pages at the start we need
   * to remove the remaining ones also, or accounting will be screwed.
   */
  balloon_stats.target_unpopulated -= nr_pages - pgno;
  return ret;
  }
-EXPORT_SYMBOL(xen_alloc_unpopulated_pages);
+EXPORT_SYMBOL(xen_alloc_ballooned_pages);

  /**
- * xen_free_unpopulated_pages - return pages retrieved with
get_ballooned_pages
+ * xen_free_ballooned_pages - return pages retrieved with get_ballooned_pages
   * @nr_pages: Number of pages
   * @pages: pages to return
   */
-void xen_free_unpopulated_pages(unsigned int nr_pages, struct page **pages)
+void xen_free_ballooned_pages(unsigned int 

Re: [PATCH V2 3/4] xen/unpopulated-alloc: Add mechanism to use Xen resource

[Juergen Gross]

On 23.11.21 17:46, Oleksandr wrote:


On 20.11.21 04:19, Stefano Stabellini wrote:

Hi Stefano, Juergen, all



Juergen please see the bottom of the email

On Fri, 19 Nov 2021, Oleksandr wrote:

On 19.11.21 02:59, Stefano Stabellini wrote:

On Tue, 9 Nov 2021, Oleksandr wrote:

On 28.10.21 19:37, Stefano Stabellini wrote:

Hi Stefano

I am sorry for the late response.


On Tue, 26 Oct 2021, Oleksandr Tyshchenko wrote:

From: Oleksandr Tyshchenko 

The main reason of this change is that unpopulated-alloc
code cannot be used in its current form on Arm, but there
is a desire to reuse it to avoid wasting real RAM pages
for the grant/foreign mappings.

The problem is that system "iomem_resource" is used for
the address space allocation, but the really unallocated
space can't be figured out precisely by the domain on Arm
without hypervisor involvement. For example, not all device
I/O regions are known by the time domain starts creating
grant/foreign mappings. And following the advise from
"iomem_resource" we might end up reusing these regions by
a mistake. So, the hypervisor which maintains the P2M for
the domain is in the best position to provide unused regions
of guest physical address space which could be safely used
to create grant/foreign mappings.

Introduce new helper arch_xen_unpopulated_init() which purpose
is to create specific Xen resource based on the memory regions
provided by the hypervisor to be used as unused space for Xen
scratch pages.

If arch doesn't implement arch_xen_unpopulated_init() to
initialize Xen resource the default "iomem_resource" will be used.
So the behavior on x86 won't be changed.

Also fall back to allocate xenballooned pages (steal real RAM
pages) if we do not have any suitable resource to work with and
as the result we won't be able to provide unpopulated pages.

Signed-off-by: Oleksandr Tyshchenko 
---
Changes RFC -> V2:
  - new patch, instead of
   "[RFC PATCH 2/2] xen/unpopulated-alloc: Query hypervisor to
provide
unallocated space"
---
    drivers/xen/unpopulated-alloc.c | 89
+++--
    include/xen/xen.h   |  2 +
    2 files changed, 88 insertions(+), 3 deletions(-)

diff --git a/drivers/xen/unpopulated-alloc.c
b/drivers/xen/unpopulated-alloc.c
index a03dc5b..1f1d8d8 100644
--- a/drivers/xen/unpopulated-alloc.c
+++ b/drivers/xen/unpopulated-alloc.c
@@ -8,6 +8,7 @@
  #include 
    +#include 
    #include 
    #include 
    @@ -15,13 +16,29 @@ static DEFINE_MUTEX(list_lock);
    static struct page *page_list;
    static unsigned int list_count;
    +static struct resource *target_resource;
+static struct resource xen_resource = {
+    .name = "Xen unused space",
+};
+
+/*
+ * If arch is not happy with system "iomem_resource" being used for
+ * the region allocation it can provide it's own view by 
initializing
+ * "xen_resource" with unused regions of guest physical address 
space

+ * provided by the hypervisor.
+ */
+int __weak arch_xen_unpopulated_init(struct resource *res)
+{
+    return -ENOSYS;
+}
+
    static int fill_list(unsigned int nr_pages)
    {
    struct dev_pagemap *pgmap;
-    struct resource *res;
+    struct resource *res, *tmp_res = NULL;
    void *vaddr;
    unsigned int i, alloc_pages = round_up(nr_pages,
PAGES_PER_SECTION);
-    int ret = -ENOMEM;
+    int ret;
  res = kzalloc(sizeof(*res), GFP_KERNEL);
    if (!res)
@@ -30,7 +47,7 @@ static int fill_list(unsigned int nr_pages)
    res->name = "Xen scratch";
    res->flags = IORESOURCE_MEM | IORESOURCE_BUSY;
    -    ret = allocate_resource(_resource, res,
+    ret = allocate_resource(target_resource, res,
    alloc_pages * PAGE_SIZE, 0, -1,
    PAGES_PER_SECTION * PAGE_SIZE, NULL,
NULL);
    if (ret < 0) {
@@ -38,6 +55,31 @@ static int fill_list(unsigned int nr_pages)
    goto err_resource;
    }
    +    /*
+ * Reserve the region previously allocated from Xen resource
to avoid
+ * re-using it by someone else.
+ */
+    if (target_resource != _resource) {
+    tmp_res = kzalloc(sizeof(*tmp_res), GFP_KERNEL);
+    if (!res) {
+    ret = -ENOMEM;
+    goto err_insert;
+    }
+
+    tmp_res->name = res->name;
+    tmp_res->start = res->start;
+    tmp_res->end = res->end;
+    tmp_res->flags = res->flags;
+
+    ret = insert_resource(_resource, tmp_res);
+    if (ret < 0) {
+    pr_err("Cannot insert IOMEM resource [%llx -
%llx]\n",
+   tmp_res->start, tmp_res->end);
+    kfree(tmp_res);
+    goto err_insert;
+    }
+    }

I am a bit confused.. why do we need to do this? Who could be
erroneously re-using the region? Are you saying that the next time
allocate_resource is called it could find the same region again? It
doesn't seem possible?
No, as I understand the allocate_resource() being called for the 
same root
resource won't provide the same region... 

Re: [PATCH V2 3/4] xen/unpopulated-alloc: Add mechanism to use Xen resource

[Stefano Stabellini]

On Tue, 23 Nov 2021, Oleksandr wrote:
> > Actually after reading your replies and explanation I changed opinion: I
> > think we do need the fallback because Linux cannot really assume that
> > it is running on "new Xen" so it definitely needs to keep working if
> > CONFIG_XEN_UNPOPULATED_ALLOC is enabled and the extended regions are not
> > advertised.
> > 
> > I think we'll have to roll back some of the changes introduced by
> > 121f2faca2c0a. That's because even if CONFIG_XEN_UNPOPULATED_ALLOC is
> > enabled we cannot know if we can use unpopulated-alloc or whether we
> > have to use alloc_xenballooned_pages until we parse the /hypervisor node
> > in device tree at runtime.
> 
> Exactly!
> 
> 
> > 
> > In short, we cannot switch between unpopulated-alloc and
> > alloc_xenballooned_pages at build time, we have to do it at runtime
> > (boot time).
> 
> +1
> 
> 
> I created a patch to partially revert 121f2faca2c0a "xen/balloon: rename
> alloc/free_xenballooned_pages".
> 
> If there is no objections I will add it to V3 (which is almost ready, except
> the fallback bits). Could you please tell me what do you think?
 
It makes sense to me. You can add my Reviewed-by.

 
> From dc79bcd425358596d95e715a8bd8b81deaaeb703 Mon Sep 17 00:00:00 2001
> From: Oleksandr Tyshchenko 
> Date: Tue, 23 Nov 2021 18:14:41 +0200
> Subject: [PATCH] xen/balloon: Bring alloc(free)_xenballooned_pages helpers
>  back
> 
> This patch rolls back some of the changes introduced by commit
> 121f2faca2c0a "xen/balloon: rename alloc/free_xenballooned_pages"
> in order to make possible to still allocate xenballooned pages
> if CONFIG_XEN_UNPOPULATED_ALLOC is enabled.
> 
> On Arm the unpopulated pages will be allocated on top of extended
> regions provided by Xen via device-tree (the subsequent patches
> will add required bits to support unpopulated-alloc feature on Arm).
> The problem is that extended regions feature has been introduced
> into Xen quite recently (during 4.16 release cycle). So this
> effectively means that Linux must only use unpopulated-alloc on Arm
> if it is running on "new Xen" which advertises these regions.
> But, it will only be known after parsing the "hypervisor" node
> at boot time, so before doing that we cannot assume anything.
> 
> In order to keep working if CONFIG_XEN_UNPOPULATED_ALLOC is enabled
> and the extended regions are not advertised (Linux is running on
> "old Xen", etc) we need the fallback to alloc_xenballooned_pages().
> 
> This way we wouldn't reduce the amount of memory usable (wasting
> RAM pages) for any of the external mappings anymore (and eliminate
> XSA-300) with "new Xen", but would be still functional ballooning
> out RAM pages with "old Xen".
> 
> Also rename alloc(free)_xenballooned_pages to xen_alloc(free)_ballooned_pages.
> 
> Signed-off-by: Oleksandr Tyshchenko 
> ---
>  drivers/xen/balloon.c | 20 +---
>  include/xen/balloon.h |  3 +++
>  include/xen/xen.h |  6 ++
>  3 files changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> index ba2ea11..a2c4fc49 100644
> --- a/drivers/xen/balloon.c
> +++ b/drivers/xen/balloon.c
> @@ -581,7 +581,6 @@ void balloon_set_new_target(unsigned long target)
>  }
>  EXPORT_SYMBOL_GPL(balloon_set_new_target);
> 
> -#ifndef CONFIG_XEN_UNPOPULATED_ALLOC
>  static int add_ballooned_pages(unsigned int nr_pages)
>  {
>  enum bp_state st;
> @@ -610,12 +609,12 @@ static int add_ballooned_pages(unsigned int nr_pages)
>  }
> 
>  /**
> - * xen_alloc_unpopulated_pages - get pages that have been ballooned out
> + * xen_alloc_ballooned_pages - get pages that have been ballooned out
>   * @nr_pages: Number of pages to get
>   * @pages: pages returned
>   * @return 0 on success, error otherwise
>   */
> -int xen_alloc_unpopulated_pages(unsigned int nr_pages, struct page **pages)
> +int xen_alloc_ballooned_pages(unsigned int nr_pages, struct page **pages)
>  {
>  unsigned int pgno = 0;
>  struct page *page;
> @@ -652,23 +651,23 @@ int xen_alloc_unpopulated_pages(unsigned int nr_pages,
> struct page **pages)
>  return 0;
>   out_undo:
>  mutex_unlock(_mutex);
> -    xen_free_unpopulated_pages(pgno, pages);
> +    xen_free_ballooned_pages(pgno, pages);
>  /*
> -     * NB: free_xenballooned_pages will only subtract pgno pages, but since
> +     * NB: xen_free_ballooned_pages will only subtract pgno pages, but since
>   * target_unpopulated is incremented with nr_pages at the start we need
>   * to remove the remaining ones also, or accounting will be screwed.
>   */
>  balloon_stats.target_unpopulated -= nr_pages - pgno;
>  return ret;
>  }
> -EXPORT_SYMBOL(xen_alloc_unpopulated_pages);
> +EXPORT_SYMBOL(xen_alloc_ballooned_pages);
> 
>  /**
> - * xen_free_unpopulated_pages - return pages retrieved with
> get_ballooned_pages
> + * xen_free_ballooned_pages - return pages retrieved with get_ballooned_pages
>   * @nr_pages: Number of pages
>   * @pages: 

Re: [PATCH V2 3/4] xen/unpopulated-alloc: Add mechanism to use Xen resource

[Oleksandr]

On 20.11.21 04:19, Stefano Stabellini wrote:

Hi Stefano, Juergen, all



Juergen please see the bottom of the email

On Fri, 19 Nov 2021, Oleksandr wrote:

On 19.11.21 02:59, Stefano Stabellini wrote:

On Tue, 9 Nov 2021, Oleksandr wrote:

On 28.10.21 19:37, Stefano Stabellini wrote:

Hi Stefano

I am sorry for the late response.


On Tue, 26 Oct 2021, Oleksandr Tyshchenko wrote:

From: Oleksandr Tyshchenko 

The main reason of this change is that unpopulated-alloc
code cannot be used in its current form on Arm, but there
is a desire to reuse it to avoid wasting real RAM pages
for the grant/foreign mappings.

The problem is that system "iomem_resource" is used for
the address space allocation, but the really unallocated
space can't be figured out precisely by the domain on Arm
without hypervisor involvement. For example, not all device
I/O regions are known by the time domain starts creating
grant/foreign mappings. And following the advise from
"iomem_resource" we might end up reusing these regions by
a mistake. So, the hypervisor which maintains the P2M for
the domain is in the best position to provide unused regions
of guest physical address space which could be safely used
to create grant/foreign mappings.

Introduce new helper arch_xen_unpopulated_init() which purpose
is to create specific Xen resource based on the memory regions
provided by the hypervisor to be used as unused space for Xen
scratch pages.

If arch doesn't implement arch_xen_unpopulated_init() to
initialize Xen resource the default "iomem_resource" will be used.
So the behavior on x86 won't be changed.

Also fall back to allocate xenballooned pages (steal real RAM
pages) if we do not have any suitable resource to work with and
as the result we won't be able to provide unpopulated pages.

Signed-off-by: Oleksandr Tyshchenko 
---
Changes RFC -> V2:
  - new patch, instead of
   "[RFC PATCH 2/2] xen/unpopulated-alloc: Query hypervisor to
provide
unallocated space"
---
drivers/xen/unpopulated-alloc.c | 89
+++--
include/xen/xen.h   |  2 +
2 files changed, 88 insertions(+), 3 deletions(-)

diff --git a/drivers/xen/unpopulated-alloc.c
b/drivers/xen/unpopulated-alloc.c
index a03dc5b..1f1d8d8 100644
--- a/drivers/xen/unpopulated-alloc.c
+++ b/drivers/xen/unpopulated-alloc.c
@@ -8,6 +8,7 @@
  #include 
+#include 
#include 
#include 
@@ -15,13 +16,29 @@ static DEFINE_MUTEX(list_lock);
static struct page *page_list;
static unsigned int list_count;
+static struct resource *target_resource;
+static struct resource xen_resource = {
+   .name = "Xen unused space",
+};
+
+/*
+ * If arch is not happy with system "iomem_resource" being used for
+ * the region allocation it can provide it's own view by initializing
+ * "xen_resource" with unused regions of guest physical address space
+ * provided by the hypervisor.
+ */
+int __weak arch_xen_unpopulated_init(struct resource *res)
+{
+   return -ENOSYS;
+}
+
static int fill_list(unsigned int nr_pages)
{
struct dev_pagemap *pgmap;
-   struct resource *res;
+   struct resource *res, *tmp_res = NULL;
void *vaddr;
unsigned int i, alloc_pages = round_up(nr_pages,
PAGES_PER_SECTION);
-   int ret = -ENOMEM;
+   int ret;
res = kzalloc(sizeof(*res), GFP_KERNEL);
if (!res)
@@ -30,7 +47,7 @@ static int fill_list(unsigned int nr_pages)
res->name = "Xen scratch";
res->flags = IORESOURCE_MEM | IORESOURCE_BUSY;
-   ret = allocate_resource(_resource, res,
+   ret = allocate_resource(target_resource, res,
alloc_pages * PAGE_SIZE, 0, -1,
PAGES_PER_SECTION * PAGE_SIZE, NULL,
NULL);
if (ret < 0) {
@@ -38,6 +55,31 @@ static int fill_list(unsigned int nr_pages)
goto err_resource;
}
+   /*
+* Reserve the region previously allocated from Xen resource
to avoid
+* re-using it by someone else.
+*/
+   if (target_resource != _resource) {
+   tmp_res = kzalloc(sizeof(*tmp_res), GFP_KERNEL);
+   if (!res) {
+   ret = -ENOMEM;
+   goto err_insert;
+   }
+
+   tmp_res->name = res->name;
+   tmp_res->start = res->start;
+   tmp_res->end = res->end;
+   tmp_res->flags = res->flags;
+
+   ret = insert_resource(_resource, tmp_res);
+   if (ret < 0) {
+   pr_err("Cannot insert IOMEM resource [%llx -
%llx]\n",
+  tmp_res->start, tmp_res->end);
+   kfree(tmp_res);
+   goto err_insert;
+   }
+   }

I am a bit confused.. why do we need to do this? Who could be
erroneously re-using the region? Are you saying that the next time
allocate_resource is called it could find the same 

Re: [PATCH V2 3/4] xen/unpopulated-alloc: Add mechanism to use Xen resource

[Stefano Stabellini]

Juergen please see the bottom of the email

On Fri, 19 Nov 2021, Oleksandr wrote:
> On 19.11.21 02:59, Stefano Stabellini wrote:
> > On Tue, 9 Nov 2021, Oleksandr wrote:
> > > On 28.10.21 19:37, Stefano Stabellini wrote:
> > > 
> > > Hi Stefano
> > > 
> > > I am sorry for the late response.
> > > 
> > > > On Tue, 26 Oct 2021, Oleksandr Tyshchenko wrote:
> > > > > From: Oleksandr Tyshchenko 
> > > > > 
> > > > > The main reason of this change is that unpopulated-alloc
> > > > > code cannot be used in its current form on Arm, but there
> > > > > is a desire to reuse it to avoid wasting real RAM pages
> > > > > for the grant/foreign mappings.
> > > > > 
> > > > > The problem is that system "iomem_resource" is used for
> > > > > the address space allocation, but the really unallocated
> > > > > space can't be figured out precisely by the domain on Arm
> > > > > without hypervisor involvement. For example, not all device
> > > > > I/O regions are known by the time domain starts creating
> > > > > grant/foreign mappings. And following the advise from
> > > > > "iomem_resource" we might end up reusing these regions by
> > > > > a mistake. So, the hypervisor which maintains the P2M for
> > > > > the domain is in the best position to provide unused regions
> > > > > of guest physical address space which could be safely used
> > > > > to create grant/foreign mappings.
> > > > > 
> > > > > Introduce new helper arch_xen_unpopulated_init() which purpose
> > > > > is to create specific Xen resource based on the memory regions
> > > > > provided by the hypervisor to be used as unused space for Xen
> > > > > scratch pages.
> > > > > 
> > > > > If arch doesn't implement arch_xen_unpopulated_init() to
> > > > > initialize Xen resource the default "iomem_resource" will be used.
> > > > > So the behavior on x86 won't be changed.
> > > > > 
> > > > > Also fall back to allocate xenballooned pages (steal real RAM
> > > > > pages) if we do not have any suitable resource to work with and
> > > > > as the result we won't be able to provide unpopulated pages.
> > > > > 
> > > > > Signed-off-by: Oleksandr Tyshchenko 
> > > > > ---
> > > > > Changes RFC -> V2:
> > > > >  - new patch, instead of
> > > > >   "[RFC PATCH 2/2] xen/unpopulated-alloc: Query hypervisor to
> > > > > provide
> > > > > unallocated space"
> > > > > ---
> > > > >drivers/xen/unpopulated-alloc.c | 89
> > > > > +++--
> > > > >include/xen/xen.h   |  2 +
> > > > >2 files changed, 88 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/xen/unpopulated-alloc.c
> > > > > b/drivers/xen/unpopulated-alloc.c
> > > > > index a03dc5b..1f1d8d8 100644
> > > > > --- a/drivers/xen/unpopulated-alloc.c
> > > > > +++ b/drivers/xen/unpopulated-alloc.c
> > > > > @@ -8,6 +8,7 @@
> > > > >  #include 
> > > > >+#include 
> > > > >#include 
> > > > >#include 
> > > > >@@ -15,13 +16,29 @@ static DEFINE_MUTEX(list_lock);
> > > > >static struct page *page_list;
> > > > >static unsigned int list_count;
> > > > >+static struct resource *target_resource;
> > > > > +static struct resource xen_resource = {
> > > > > + .name = "Xen unused space",
> > > > > +};
> > > > > +
> > > > > +/*
> > > > > + * If arch is not happy with system "iomem_resource" being used for
> > > > > + * the region allocation it can provide it's own view by initializing
> > > > > + * "xen_resource" with unused regions of guest physical address space
> > > > > + * provided by the hypervisor.
> > > > > + */
> > > > > +int __weak arch_xen_unpopulated_init(struct resource *res)
> > > > > +{
> > > > > + return -ENOSYS;
> > > > > +}
> > > > > +
> > > > >static int fill_list(unsigned int nr_pages)
> > > > >{
> > > > >   struct dev_pagemap *pgmap;
> > > > > - struct resource *res;
> > > > > + struct resource *res, *tmp_res = NULL;
> > > > >   void *vaddr;
> > > > >   unsigned int i, alloc_pages = round_up(nr_pages,
> > > > > PAGES_PER_SECTION);
> > > > > - int ret = -ENOMEM;
> > > > > + int ret;
> > > > >   res = kzalloc(sizeof(*res), GFP_KERNEL);
> > > > >   if (!res)
> > > > > @@ -30,7 +47,7 @@ static int fill_list(unsigned int nr_pages)
> > > > >   res->name = "Xen scratch";
> > > > >   res->flags = IORESOURCE_MEM | IORESOURCE_BUSY;
> > > > >-  ret = allocate_resource(_resource, res,
> > > > > + ret = allocate_resource(target_resource, res,
> > > > >   alloc_pages * PAGE_SIZE, 0, -1,
> > > > >   PAGES_PER_SECTION * PAGE_SIZE, NULL,
> > > > > NULL);
> > > > >   if (ret < 0) {
> > > > > @@ -38,6 +55,31 @@ static int fill_list(unsigned int nr_pages)
> > > > >   goto err_resource;
> > > > >   }
> > > > >+  /*
> > > > > +  * Reserve the region previously allocated from Xen resource
> > > > > to avoid
> > > > > +  * re-using it by someone else.
> > > > > +  */
> 

Re: [PATCH V2 3/4] xen/unpopulated-alloc: Add mechanism to use Xen resource

[Oleksandr]

On 19.11.21 02:59, Stefano Stabellini wrote:


Hi Stefano


On Tue, 9 Nov 2021, Oleksandr wrote:

On 28.10.21 19:37, Stefano Stabellini wrote:

Hi Stefano

I am sorry for the late response.


On Tue, 26 Oct 2021, Oleksandr Tyshchenko wrote:

From: Oleksandr Tyshchenko 

The main reason of this change is that unpopulated-alloc
code cannot be used in its current form on Arm, but there
is a desire to reuse it to avoid wasting real RAM pages
for the grant/foreign mappings.

The problem is that system "iomem_resource" is used for
the address space allocation, but the really unallocated
space can't be figured out precisely by the domain on Arm
without hypervisor involvement. For example, not all device
I/O regions are known by the time domain starts creating
grant/foreign mappings. And following the advise from
"iomem_resource" we might end up reusing these regions by
a mistake. So, the hypervisor which maintains the P2M for
the domain is in the best position to provide unused regions
of guest physical address space which could be safely used
to create grant/foreign mappings.

Introduce new helper arch_xen_unpopulated_init() which purpose
is to create specific Xen resource based on the memory regions
provided by the hypervisor to be used as unused space for Xen
scratch pages.

If arch doesn't implement arch_xen_unpopulated_init() to
initialize Xen resource the default "iomem_resource" will be used.
So the behavior on x86 won't be changed.

Also fall back to allocate xenballooned pages (steal real RAM
pages) if we do not have any suitable resource to work with and
as the result we won't be able to provide unpopulated pages.

Signed-off-by: Oleksandr Tyshchenko 
---
Changes RFC -> V2:
 - new patch, instead of
  "[RFC PATCH 2/2] xen/unpopulated-alloc: Query hypervisor to provide
unallocated space"
---
   drivers/xen/unpopulated-alloc.c | 89
+++--
   include/xen/xen.h   |  2 +
   2 files changed, 88 insertions(+), 3 deletions(-)

diff --git a/drivers/xen/unpopulated-alloc.c
b/drivers/xen/unpopulated-alloc.c
index a03dc5b..1f1d8d8 100644
--- a/drivers/xen/unpopulated-alloc.c
+++ b/drivers/xen/unpopulated-alloc.c
@@ -8,6 +8,7 @@
 #include 
   +#include 
   #include 
   #include 
   @@ -15,13 +16,29 @@ static DEFINE_MUTEX(list_lock);
   static struct page *page_list;
   static unsigned int list_count;
   +static struct resource *target_resource;
+static struct resource xen_resource = {
+   .name = "Xen unused space",
+};
+
+/*
+ * If arch is not happy with system "iomem_resource" being used for
+ * the region allocation it can provide it's own view by initializing
+ * "xen_resource" with unused regions of guest physical address space
+ * provided by the hypervisor.
+ */
+int __weak arch_xen_unpopulated_init(struct resource *res)
+{
+   return -ENOSYS;
+}
+
   static int fill_list(unsigned int nr_pages)
   {
struct dev_pagemap *pgmap;
-   struct resource *res;
+   struct resource *res, *tmp_res = NULL;
void *vaddr;
unsigned int i, alloc_pages = round_up(nr_pages, PAGES_PER_SECTION);
-   int ret = -ENOMEM;
+   int ret;
res = kzalloc(sizeof(*res), GFP_KERNEL);
if (!res)
@@ -30,7 +47,7 @@ static int fill_list(unsigned int nr_pages)
res->name = "Xen scratch";
res->flags = IORESOURCE_MEM | IORESOURCE_BUSY;
   -ret = allocate_resource(_resource, res,
+   ret = allocate_resource(target_resource, res,
alloc_pages * PAGE_SIZE, 0, -1,
PAGES_PER_SECTION * PAGE_SIZE, NULL, NULL);
if (ret < 0) {
@@ -38,6 +55,31 @@ static int fill_list(unsigned int nr_pages)
goto err_resource;
}
   +/*
+* Reserve the region previously allocated from Xen resource to avoid
+* re-using it by someone else.
+*/
+   if (target_resource != _resource) {
+   tmp_res = kzalloc(sizeof(*tmp_res), GFP_KERNEL);
+   if (!res) {
+   ret = -ENOMEM;
+   goto err_insert;
+   }
+
+   tmp_res->name = res->name;
+   tmp_res->start = res->start;
+   tmp_res->end = res->end;
+   tmp_res->flags = res->flags;
+
+   ret = insert_resource(_resource, tmp_res);
+   if (ret < 0) {
+   pr_err("Cannot insert IOMEM resource [%llx - %llx]\n",
+  tmp_res->start, tmp_res->end);
+   kfree(tmp_res);
+   goto err_insert;
+   }
+   }

I am a bit confused.. why do we need to do this? Who could be
erroneously re-using the region? Are you saying that the next time
allocate_resource is called it could find the same region again? It
doesn't seem possible?


No, as I understand the allocate_resource() being called for the same root
resource won't provide the same region... 

Re: [PATCH V2 3/4] xen/unpopulated-alloc: Add mechanism to use Xen resource

[Stefano Stabellini]

On Tue, 9 Nov 2021, Oleksandr wrote:
> On 28.10.21 19:37, Stefano Stabellini wrote:
> 
> Hi Stefano
> 
> I am sorry for the late response.
> 
> > On Tue, 26 Oct 2021, Oleksandr Tyshchenko wrote:
> > > From: Oleksandr Tyshchenko 
> > > 
> > > The main reason of this change is that unpopulated-alloc
> > > code cannot be used in its current form on Arm, but there
> > > is a desire to reuse it to avoid wasting real RAM pages
> > > for the grant/foreign mappings.
> > > 
> > > The problem is that system "iomem_resource" is used for
> > > the address space allocation, but the really unallocated
> > > space can't be figured out precisely by the domain on Arm
> > > without hypervisor involvement. For example, not all device
> > > I/O regions are known by the time domain starts creating
> > > grant/foreign mappings. And following the advise from
> > > "iomem_resource" we might end up reusing these regions by
> > > a mistake. So, the hypervisor which maintains the P2M for
> > > the domain is in the best position to provide unused regions
> > > of guest physical address space which could be safely used
> > > to create grant/foreign mappings.
> > > 
> > > Introduce new helper arch_xen_unpopulated_init() which purpose
> > > is to create specific Xen resource based on the memory regions
> > > provided by the hypervisor to be used as unused space for Xen
> > > scratch pages.
> > > 
> > > If arch doesn't implement arch_xen_unpopulated_init() to
> > > initialize Xen resource the default "iomem_resource" will be used.
> > > So the behavior on x86 won't be changed.
> > > 
> > > Also fall back to allocate xenballooned pages (steal real RAM
> > > pages) if we do not have any suitable resource to work with and
> > > as the result we won't be able to provide unpopulated pages.
> > > 
> > > Signed-off-by: Oleksandr Tyshchenko 
> > > ---
> > > Changes RFC -> V2:
> > > - new patch, instead of
> > >  "[RFC PATCH 2/2] xen/unpopulated-alloc: Query hypervisor to provide
> > > unallocated space"
> > > ---
> > >   drivers/xen/unpopulated-alloc.c | 89
> > > +++--
> > >   include/xen/xen.h   |  2 +
> > >   2 files changed, 88 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/xen/unpopulated-alloc.c
> > > b/drivers/xen/unpopulated-alloc.c
> > > index a03dc5b..1f1d8d8 100644
> > > --- a/drivers/xen/unpopulated-alloc.c
> > > +++ b/drivers/xen/unpopulated-alloc.c
> > > @@ -8,6 +8,7 @@
> > > #include 
> > >   +#include 
> > >   #include 
> > >   #include 
> > >   @@ -15,13 +16,29 @@ static DEFINE_MUTEX(list_lock);
> > >   static struct page *page_list;
> > >   static unsigned int list_count;
> > >   +static struct resource *target_resource;
> > > +static struct resource xen_resource = {
> > > + .name = "Xen unused space",
> > > +};
> > > +
> > > +/*
> > > + * If arch is not happy with system "iomem_resource" being used for
> > > + * the region allocation it can provide it's own view by initializing
> > > + * "xen_resource" with unused regions of guest physical address space
> > > + * provided by the hypervisor.
> > > + */
> > > +int __weak arch_xen_unpopulated_init(struct resource *res)
> > > +{
> > > + return -ENOSYS;
> > > +}
> > > +
> > >   static int fill_list(unsigned int nr_pages)
> > >   {
> > >   struct dev_pagemap *pgmap;
> > > - struct resource *res;
> > > + struct resource *res, *tmp_res = NULL;
> > >   void *vaddr;
> > >   unsigned int i, alloc_pages = round_up(nr_pages, 
> > > PAGES_PER_SECTION);
> > > - int ret = -ENOMEM;
> > > + int ret;
> > >   res = kzalloc(sizeof(*res), GFP_KERNEL);
> > >   if (!res)
> > > @@ -30,7 +47,7 @@ static int fill_list(unsigned int nr_pages)
> > >   res->name = "Xen scratch";
> > >   res->flags = IORESOURCE_MEM | IORESOURCE_BUSY;
> > >   -   ret = allocate_resource(_resource, res,
> > > + ret = allocate_resource(target_resource, res,
> > >   alloc_pages * PAGE_SIZE, 0, -1,
> > >   PAGES_PER_SECTION * PAGE_SIZE, NULL, 
> > > NULL);
> > >   if (ret < 0) {
> > > @@ -38,6 +55,31 @@ static int fill_list(unsigned int nr_pages)
> > >   goto err_resource;
> > >   }
> > >   +   /*
> > > +  * Reserve the region previously allocated from Xen resource to avoid
> > > +  * re-using it by someone else.
> > > +  */
> > > + if (target_resource != _resource) {
> > > + tmp_res = kzalloc(sizeof(*tmp_res), GFP_KERNEL);
> > > + if (!res) {
> > > + ret = -ENOMEM;
> > > + goto err_insert;
> > > + }
> > > +
> > > + tmp_res->name = res->name;
> > > + tmp_res->start = res->start;
> > > + tmp_res->end = res->end;
> > > + tmp_res->flags = res->flags;
> > > +
> > > + ret = insert_resource(_resource, tmp_res);
> > > + if (ret < 0) {
> > > + pr_err("Cannot insert IOMEM resource [%llx - 

Re: [PATCH V2 3/4] xen/unpopulated-alloc: Add mechanism to use Xen resource

[Oleksandr]

On 28.10.21 22:08, Boris Ostrovsky wrote:

Hi Boris

I am sorry for the late response.



On 10/26/21 12:05 PM, Oleksandr Tyshchenko wrote:

  +static void unpopulated_init(void)
+{
+    static bool inited = false;
+    int ret;
+
+    if (inited)
+    return;
+
+    /*
+ * Try to initialize Xen resource the first and fall back to 
default

+ * resource if arch doesn't offer one.
+ */
+    ret = arch_xen_unpopulated_init(_resource);
+    if (!ret)
+    target_resource = _resource;
+    else if (ret == -ENOSYS)
+    target_resource = _resource;
+    else
+    pr_err("Cannot initialize Xen resource\n");



I'd pass target_resource as a parameter to arch_xen_unpopulated_init() 
instead. Default routine will assign it iomem_resource and you won't 
have to deal with -ENOSYS.


That would be much better, thank you. Will do.






Also, what happens in case of error? Is it fatal? I don't think your 
changes in fill_list() will work.


The error is fatal as we don't have a suitable resource to allocate a 
region from, and yes, the fill_list() must not be called.








+
+    inited = true;



I agree with Stefano in that it would be better to call this from an 
init function, and you won't have t worry about multiple calls here.


Yes, that's good point, thank you. Will do.






-boris


--
Regards,

Oleksandr Tyshchenko

Re: [PATCH V2 3/4] xen/unpopulated-alloc: Add mechanism to use Xen resource

[Oleksandr]

On 28.10.21 19:37, Stefano Stabellini wrote:

Hi Stefano

I am sorry for the late response.


On Tue, 26 Oct 2021, Oleksandr Tyshchenko wrote:

From: Oleksandr Tyshchenko 

The main reason of this change is that unpopulated-alloc
code cannot be used in its current form on Arm, but there
is a desire to reuse it to avoid wasting real RAM pages
for the grant/foreign mappings.

The problem is that system "iomem_resource" is used for
the address space allocation, but the really unallocated
space can't be figured out precisely by the domain on Arm
without hypervisor involvement. For example, not all device
I/O regions are known by the time domain starts creating
grant/foreign mappings. And following the advise from
"iomem_resource" we might end up reusing these regions by
a mistake. So, the hypervisor which maintains the P2M for
the domain is in the best position to provide unused regions
of guest physical address space which could be safely used
to create grant/foreign mappings.

Introduce new helper arch_xen_unpopulated_init() which purpose
is to create specific Xen resource based on the memory regions
provided by the hypervisor to be used as unused space for Xen
scratch pages.

If arch doesn't implement arch_xen_unpopulated_init() to
initialize Xen resource the default "iomem_resource" will be used.
So the behavior on x86 won't be changed.

Also fall back to allocate xenballooned pages (steal real RAM
pages) if we do not have any suitable resource to work with and
as the result we won't be able to provide unpopulated pages.

Signed-off-by: Oleksandr Tyshchenko 
---
Changes RFC -> V2:
- new patch, instead of
 "[RFC PATCH 2/2] xen/unpopulated-alloc: Query hypervisor to provide unallocated 
space"
---
  drivers/xen/unpopulated-alloc.c | 89 +++--
  include/xen/xen.h   |  2 +
  2 files changed, 88 insertions(+), 3 deletions(-)

diff --git a/drivers/xen/unpopulated-alloc.c b/drivers/xen/unpopulated-alloc.c
index a03dc5b..1f1d8d8 100644
--- a/drivers/xen/unpopulated-alloc.c
+++ b/drivers/xen/unpopulated-alloc.c
@@ -8,6 +8,7 @@
  
  #include 
  
+#include 

  #include 
  #include 
  
@@ -15,13 +16,29 @@ static DEFINE_MUTEX(list_lock);

  static struct page *page_list;
  static unsigned int list_count;
  
+static struct resource *target_resource;

+static struct resource xen_resource = {
+   .name = "Xen unused space",
+};
+
+/*
+ * If arch is not happy with system "iomem_resource" being used for
+ * the region allocation it can provide it's own view by initializing
+ * "xen_resource" with unused regions of guest physical address space
+ * provided by the hypervisor.
+ */
+int __weak arch_xen_unpopulated_init(struct resource *res)
+{
+   return -ENOSYS;
+}
+
  static int fill_list(unsigned int nr_pages)
  {
struct dev_pagemap *pgmap;
-   struct resource *res;
+   struct resource *res, *tmp_res = NULL;
void *vaddr;
unsigned int i, alloc_pages = round_up(nr_pages, PAGES_PER_SECTION);
-   int ret = -ENOMEM;
+   int ret;
  
  	res = kzalloc(sizeof(*res), GFP_KERNEL);

if (!res)
@@ -30,7 +47,7 @@ static int fill_list(unsigned int nr_pages)
res->name = "Xen scratch";
res->flags = IORESOURCE_MEM | IORESOURCE_BUSY;
  
-	ret = allocate_resource(_resource, res,

+   ret = allocate_resource(target_resource, res,
alloc_pages * PAGE_SIZE, 0, -1,
PAGES_PER_SECTION * PAGE_SIZE, NULL, NULL);
if (ret < 0) {
@@ -38,6 +55,31 @@ static int fill_list(unsigned int nr_pages)
goto err_resource;
}
  
+	/*

+* Reserve the region previously allocated from Xen resource to avoid
+* re-using it by someone else.
+*/
+   if (target_resource != _resource) {
+   tmp_res = kzalloc(sizeof(*tmp_res), GFP_KERNEL);
+   if (!res) {
+   ret = -ENOMEM;
+   goto err_insert;
+   }
+
+   tmp_res->name = res->name;
+   tmp_res->start = res->start;
+   tmp_res->end = res->end;
+   tmp_res->flags = res->flags;
+
+   ret = insert_resource(_resource, tmp_res);
+   if (ret < 0) {
+   pr_err("Cannot insert IOMEM resource [%llx - %llx]\n",
+  tmp_res->start, tmp_res->end);
+   kfree(tmp_res);
+   goto err_insert;
+   }
+   }

I am a bit confused.. why do we need to do this? Who could be
erroneously re-using the region? Are you saying that the next time
allocate_resource is called it could find the same region again? It
doesn't seem possible?



No, as I understand the allocate_resource() being called for the same 
root resource won't provide the same region... We only need to do this 
(insert the region into "iomem_resource") if we allocated it from our 
*internal* 

Re: [PATCH V2 3/4] xen/unpopulated-alloc: Add mechanism to use Xen resource

[Boris Ostrovsky]

On 10/26/21 12:05 PM, Oleksandr Tyshchenko wrote:
  
+static void unpopulated_init(void)

+{
+   static bool inited = false;
+   int ret;
+
+   if (inited)
+   return;
+
+   /*
+* Try to initialize Xen resource the first and fall back to default
+* resource if arch doesn't offer one.
+*/
+   ret = arch_xen_unpopulated_init(_resource);
+   if (!ret)
+   target_resource = _resource;
+   else if (ret == -ENOSYS)
+   target_resource = _resource;
+   else
+   pr_err("Cannot initialize Xen resource\n");



I'd pass target_resource as a parameter to arch_xen_unpopulated_init() instead. 
Default routine will assign it iomem_resource and you won't have to deal with 
-ENOSYS.


Also, what happens in case of error? Is it fatal? I don't think your changes in 
fill_list() will work.



+
+   inited = true;



I agree with Stefano in that it would be better to call this from an init 
function, and you won't have t worry about multiple calls here.


-boris

Re: [PATCH V2 3/4] xen/unpopulated-alloc: Add mechanism to use Xen resource

[Stefano Stabellini]

On Tue, 26 Oct 2021, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko 
> 
> The main reason of this change is that unpopulated-alloc
> code cannot be used in its current form on Arm, but there
> is a desire to reuse it to avoid wasting real RAM pages
> for the grant/foreign mappings.
> 
> The problem is that system "iomem_resource" is used for
> the address space allocation, but the really unallocated
> space can't be figured out precisely by the domain on Arm
> without hypervisor involvement. For example, not all device
> I/O regions are known by the time domain starts creating
> grant/foreign mappings. And following the advise from
> "iomem_resource" we might end up reusing these regions by
> a mistake. So, the hypervisor which maintains the P2M for
> the domain is in the best position to provide unused regions
> of guest physical address space which could be safely used
> to create grant/foreign mappings.
> 
> Introduce new helper arch_xen_unpopulated_init() which purpose
> is to create specific Xen resource based on the memory regions
> provided by the hypervisor to be used as unused space for Xen
> scratch pages.
> 
> If arch doesn't implement arch_xen_unpopulated_init() to
> initialize Xen resource the default "iomem_resource" will be used.
> So the behavior on x86 won't be changed.
> 
> Also fall back to allocate xenballooned pages (steal real RAM
> pages) if we do not have any suitable resource to work with and
> as the result we won't be able to provide unpopulated pages.
> 
> Signed-off-by: Oleksandr Tyshchenko 
> ---
> Changes RFC -> V2:
>- new patch, instead of
> "[RFC PATCH 2/2] xen/unpopulated-alloc: Query hypervisor to provide 
> unallocated space"
> ---
>  drivers/xen/unpopulated-alloc.c | 89 
> +++--
>  include/xen/xen.h   |  2 +
>  2 files changed, 88 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/xen/unpopulated-alloc.c b/drivers/xen/unpopulated-alloc.c
> index a03dc5b..1f1d8d8 100644
> --- a/drivers/xen/unpopulated-alloc.c
> +++ b/drivers/xen/unpopulated-alloc.c
> @@ -8,6 +8,7 @@
>  
>  #include 
>  
> +#include 
>  #include 
>  #include 
>  
> @@ -15,13 +16,29 @@ static DEFINE_MUTEX(list_lock);
>  static struct page *page_list;
>  static unsigned int list_count;
>  
> +static struct resource *target_resource;
> +static struct resource xen_resource = {
> + .name = "Xen unused space",
> +};
> +
> +/*
> + * If arch is not happy with system "iomem_resource" being used for
> + * the region allocation it can provide it's own view by initializing
> + * "xen_resource" with unused regions of guest physical address space
> + * provided by the hypervisor.
> + */
> +int __weak arch_xen_unpopulated_init(struct resource *res)
> +{
> + return -ENOSYS;
> +}
> +
>  static int fill_list(unsigned int nr_pages)
>  {
>   struct dev_pagemap *pgmap;
> - struct resource *res;
> + struct resource *res, *tmp_res = NULL;
>   void *vaddr;
>   unsigned int i, alloc_pages = round_up(nr_pages, PAGES_PER_SECTION);
> - int ret = -ENOMEM;
> + int ret;
>  
>   res = kzalloc(sizeof(*res), GFP_KERNEL);
>   if (!res)
> @@ -30,7 +47,7 @@ static int fill_list(unsigned int nr_pages)
>   res->name = "Xen scratch";
>   res->flags = IORESOURCE_MEM | IORESOURCE_BUSY;
>  
> - ret = allocate_resource(_resource, res,
> + ret = allocate_resource(target_resource, res,
>   alloc_pages * PAGE_SIZE, 0, -1,
>   PAGES_PER_SECTION * PAGE_SIZE, NULL, NULL);
>   if (ret < 0) {
> @@ -38,6 +55,31 @@ static int fill_list(unsigned int nr_pages)
>   goto err_resource;
>   }
>  
> + /*
> +  * Reserve the region previously allocated from Xen resource to avoid
> +  * re-using it by someone else.
> +  */
> + if (target_resource != _resource) {
> + tmp_res = kzalloc(sizeof(*tmp_res), GFP_KERNEL);
> + if (!res) {
> + ret = -ENOMEM;
> + goto err_insert;
> + }
> +
> + tmp_res->name = res->name;
> + tmp_res->start = res->start;
> + tmp_res->end = res->end;
> + tmp_res->flags = res->flags;
> +
> + ret = insert_resource(_resource, tmp_res);
> + if (ret < 0) {
> + pr_err("Cannot insert IOMEM resource [%llx - %llx]\n",
> +tmp_res->start, tmp_res->end);
> + kfree(tmp_res);
> + goto err_insert;
> + }
> + }

I am a bit confused.. why do we need to do this? Who could be
erroneously re-using the region? Are you saying that the next time
allocate_resource is called it could find the same region again? It
doesn't seem possible?


>   pgmap = kzalloc(sizeof(*pgmap), GFP_KERNEL);
>   if (!pgmap) {
>   ret = -ENOMEM;
> @@ -95,12 +137,40 @@ static int fill_list(unsigned int 

[PATCH V2 3/4] xen/unpopulated-alloc: Add mechanism to use Xen resource

[Oleksandr Tyshchenko]

From: Oleksandr Tyshchenko 

The main reason of this change is that unpopulated-alloc
code cannot be used in its current form on Arm, but there
is a desire to reuse it to avoid wasting real RAM pages
for the grant/foreign mappings.

The problem is that system "iomem_resource" is used for
the address space allocation, but the really unallocated
space can't be figured out precisely by the domain on Arm
without hypervisor involvement. For example, not all device
I/O regions are known by the time domain starts creating
grant/foreign mappings. And following the advise from
"iomem_resource" we might end up reusing these regions by
a mistake. So, the hypervisor which maintains the P2M for
the domain is in the best position to provide unused regions
of guest physical address space which could be safely used
to create grant/foreign mappings.

Introduce new helper arch_xen_unpopulated_init() which purpose
is to create specific Xen resource based on the memory regions
provided by the hypervisor to be used as unused space for Xen
scratch pages.

If arch doesn't implement arch_xen_unpopulated_init() to
initialize Xen resource the default "iomem_resource" will be used.
So the behavior on x86 won't be changed.

Also fall back to allocate xenballooned pages (steal real RAM
pages) if we do not have any suitable resource to work with and
as the result we won't be able to provide unpopulated pages.

Signed-off-by: Oleksandr Tyshchenko 
---
Changes RFC -> V2:
   - new patch, instead of
"[RFC PATCH 2/2] xen/unpopulated-alloc: Query hypervisor to provide 
unallocated space"
---
 drivers/xen/unpopulated-alloc.c | 89 +++--
 include/xen/xen.h   |  2 +
 2 files changed, 88 insertions(+), 3 deletions(-)

diff --git a/drivers/xen/unpopulated-alloc.c b/drivers/xen/unpopulated-alloc.c
index a03dc5b..1f1d8d8 100644
--- a/drivers/xen/unpopulated-alloc.c
+++ b/drivers/xen/unpopulated-alloc.c
@@ -8,6 +8,7 @@
 
 #include 
 
+#include 
 #include 
 #include 
 
@@ -15,13 +16,29 @@ static DEFINE_MUTEX(list_lock);
 static struct page *page_list;
 static unsigned int list_count;
 
+static struct resource *target_resource;
+static struct resource xen_resource = {
+   .name = "Xen unused space",
+};
+
+/*
+ * If arch is not happy with system "iomem_resource" being used for
+ * the region allocation it can provide it's own view by initializing
+ * "xen_resource" with unused regions of guest physical address space
+ * provided by the hypervisor.
+ */
+int __weak arch_xen_unpopulated_init(struct resource *res)
+{
+   return -ENOSYS;
+}
+
 static int fill_list(unsigned int nr_pages)
 {
struct dev_pagemap *pgmap;
-   struct resource *res;
+   struct resource *res, *tmp_res = NULL;
void *vaddr;
unsigned int i, alloc_pages = round_up(nr_pages, PAGES_PER_SECTION);
-   int ret = -ENOMEM;
+   int ret;
 
res = kzalloc(sizeof(*res), GFP_KERNEL);
if (!res)
@@ -30,7 +47,7 @@ static int fill_list(unsigned int nr_pages)
res->name = "Xen scratch";
res->flags = IORESOURCE_MEM | IORESOURCE_BUSY;
 
-   ret = allocate_resource(_resource, res,
+   ret = allocate_resource(target_resource, res,
alloc_pages * PAGE_SIZE, 0, -1,
PAGES_PER_SECTION * PAGE_SIZE, NULL, NULL);
if (ret < 0) {
@@ -38,6 +55,31 @@ static int fill_list(unsigned int nr_pages)
goto err_resource;
}
 
+   /*
+* Reserve the region previously allocated from Xen resource to avoid
+* re-using it by someone else.
+*/
+   if (target_resource != _resource) {
+   tmp_res = kzalloc(sizeof(*tmp_res), GFP_KERNEL);
+   if (!res) {
+   ret = -ENOMEM;
+   goto err_insert;
+   }
+
+   tmp_res->name = res->name;
+   tmp_res->start = res->start;
+   tmp_res->end = res->end;
+   tmp_res->flags = res->flags;
+
+   ret = insert_resource(_resource, tmp_res);
+   if (ret < 0) {
+   pr_err("Cannot insert IOMEM resource [%llx - %llx]\n",
+  tmp_res->start, tmp_res->end);
+   kfree(tmp_res);
+   goto err_insert;
+   }
+   }
+
pgmap = kzalloc(sizeof(*pgmap), GFP_KERNEL);
if (!pgmap) {
ret = -ENOMEM;
@@ -95,12 +137,40 @@ static int fill_list(unsigned int nr_pages)
 err_memremap:
kfree(pgmap);
 err_pgmap:
+   if (tmp_res) {
+   release_resource(tmp_res);
+   kfree(tmp_res);
+   }
+err_insert:
release_resource(res);
 err_resource:
kfree(res);
return ret;
 }
 
+static void unpopulated_init(void)
+{
+   static bool inited = false;
+   int ret;
+
+   if (inited)
+   return;
+
+   /*
+* Try to