[linux-linus test] 154349: regressions - trouble: broken/fail/pass

[osstest service owner]

flight 154349 linux-linus real [real]
http://logs.test-lab.xenproject.org/osstest/logs/154349/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-coresched-amd64-xl broken
 test-amd64-amd64-xl-xsm  broken
 test-amd64-amd64-xl-rtds broken
 test-amd64-amd64-xl-pvhv2-amd broken
 test-amd64-amd64-libvirt-vhd broken
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm   broken
 test-amd64-amd64-xl-pvhv2-amd  4 host-install(4)   broken REGR. vs. 152332
 test-amd64-coresched-amd64-xl  4 host-install(4)   broken REGR. vs. 152332
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 4 host-install(4) broken 
REGR. vs. 152332
 test-amd64-amd64-libvirt-vhd  4 host-install(4)broken REGR. vs. 152332
 test-amd64-amd64-xl-xsm   4 host-install(4)broken REGR. vs. 152332
 test-amd64-i386-qemut-rhel6hvm-intel  7 xen-boot fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-ws16-amd64  7 xen-boot  fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-dmrestrict-amd64-dmrestrict 7 xen-boot fail REGR. vs. 
152332
 test-amd64-i386-xl-xsm7 xen-boot fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-debianhvm-amd64-shadow 7 xen-boot fail REGR. vs. 
152332
 test-amd64-i386-qemuu-rhel6hvm-intel  7 xen-boot fail REGR. vs. 152332
 test-amd64-i386-xl-qemut-debianhvm-amd64  7 xen-boot fail REGR. vs. 152332
 test-amd64-i386-examine   8 reboot   fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-debianhvm-i386-xsm  7 xen-boot  fail REGR. vs. 152332
 test-amd64-i386-pair 10 xen-boot/src_hostfail REGR. vs. 152332
 test-amd64-i386-pair 11 xen-boot/dst_hostfail REGR. vs. 152332
 test-amd64-i386-libvirt   7 xen-boot fail REGR. vs. 152332
 test-amd64-i386-xl-qemut-ws16-amd64  7 xen-boot  fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-debianhvm-amd64  7 xen-boot fail REGR. vs. 152332
 test-amd64-i386-libvirt-xsm   7 xen-boot fail REGR. vs. 152332
 test-amd64-i386-qemut-rhel6hvm-amd  7 xen-boot   fail REGR. vs. 152332
 test-amd64-i386-qemuu-rhel6hvm-amd  7 xen-boot   fail REGR. vs. 152332
 test-amd64-i386-xl7 xen-boot fail REGR. vs. 152332
 test-amd64-coresched-i386-xl  7 xen-boot fail REGR. vs. 152332
 test-amd64-i386-xl-raw7 xen-boot fail REGR. vs. 152332
 test-amd64-i386-freebsd10-amd64  7 xen-boot  fail REGR. vs. 152332
 test-amd64-i386-xl-pvshim 7 xen-boot fail REGR. vs. 152332
 test-amd64-i386-xl-qemut-debianhvm-i386-xsm  7 xen-boot  fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-win7-amd64  7 xen-boot  fail REGR. vs. 152332
 test-amd64-i386-xl-shadow 7 xen-boot fail REGR. vs. 152332
 test-amd64-i386-freebsd10-i386  7 xen-boot   fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-ovmf-amd64  7 xen-boot  fail REGR. vs. 152332
 test-amd64-i386-xl-qemut-win7-amd64  7 xen-boot  fail REGR. vs. 152332
 test-amd64-i386-libvirt-pair 10 xen-boot/src_hostfail REGR. vs. 152332
 test-amd64-i386-libvirt-pair 11 xen-boot/dst_hostfail REGR. vs. 152332
 test-arm64-arm64-libvirt-xsm 17 guest-start.2fail REGR. vs. 152332
 test-amd64-i386-xl-qemut-stubdom-debianhvm-amd64-xsm 7 xen-boot fail REGR. vs. 
152332

Regressions which are regarded as allowable (not blocking):
 test-amd64-amd64-xl-rtds  4 host-install(4)broken REGR. vs. 152332

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stopfail like 152332
 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 152332
 test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stopfail like 152332
 test-armhf-armhf-libvirt 14 saverestore-support-checkfail  like 152332
 test-armhf-armhf-xl-rtds 16 guest-start/debian.repeatfail  like 152332
 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail  like 152332
 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stopfail like 152332
 test-amd64-amd64-libvirt 13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-seattle  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-seattle  14 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  14 saverestore-support-check 

libxenguest and xenguest.h

[Jürgen Groß]

Andy has reported a libxenguest related build failure of qemu when
building qemu outside the Xen build environment. Problem is xenguest.h
now including xenctrl_dom.h, which is including xen/libelf/libelf.h.

The underlying problem is that libxenguest is basically offering some
"official" functions via xenguest.h, while some other functions are
only Xen internally usable and are defined in xenctrl_dom.h.

This is a rather weird construction and I'm seeing the following
solutions:

1. Make xen/include/xen/libelf.h a public header (or split the parts
   needed by xenguest.h into a public header)

2. Reflect the two parts of libxenguest by carving out the xenctrl_dom.h
   defined parts into a new library not made public

3. Make the xenctrl_dom.h interfaces internal again by not adding it to
   the installed headers

While variant 3 seems to be the easiest one I'd prefer variant 1.
Variant 2 seems to add complexity without any real gain IMO.

Thoughts?


Juergen

Re: [PATCH v2 1/7] kernel/resource: make release_mem_region_adjustable() never fail

[Wei Yang]

On Tue, Sep 08, 2020 at 10:10:06PM +0200, David Hildenbrand wrote:
>Let's make sure splitting a resource on memory hotunplug will never fail.
>This will become more relevant once we merge selected System RAM
>resources - then, we'll trigger that case more often on memory hotunplug.
>
>In general, this function is already unlikely to fail. When we remove
>memory, we free up quite a lot of metadata (memmap, page tables, memory
>block device, etc.). The only reason it could really fail would be when
>injecting allocation errors.
>
>All other error cases inside release_mem_region_adjustable() seem to be
>sanity checks if the function would be abused in different context -
>let's add WARN_ON_ONCE() in these cases so we can catch them.
>
>Cc: Andrew Morton 
>Cc: Michal Hocko 
>Cc: Dan Williams 
>Cc: Jason Gunthorpe 
>Cc: Kees Cook 
>Cc: Ard Biesheuvel 
>Cc: Pankaj Gupta 
>Cc: Baoquan He 
>Cc: Wei Yang 
>Signed-off-by: David Hildenbrand 
>---
> include/linux/ioport.h |  4 ++--
> kernel/resource.c  | 49 --
> mm/memory_hotplug.c| 22 +--
> 3 files changed, 31 insertions(+), 44 deletions(-)
>
>diff --git a/include/linux/ioport.h b/include/linux/ioport.h
>index 6c2b06fe8beb7..52a91f5fa1a36 100644
>--- a/include/linux/ioport.h
>+++ b/include/linux/ioport.h
>@@ -248,8 +248,8 @@ extern struct resource * __request_region(struct resource 
>*,
> extern void __release_region(struct resource *, resource_size_t,
>   resource_size_t);
> #ifdef CONFIG_MEMORY_HOTREMOVE
>-extern int release_mem_region_adjustable(struct resource *, resource_size_t,
>-  resource_size_t);
>+extern void release_mem_region_adjustable(struct resource *, resource_size_t,
>+resource_size_t);
> #endif
> 
> /* Wrappers for managed devices */
>diff --git a/kernel/resource.c b/kernel/resource.c
>index f1175ce93a1d5..36b3552210120 100644
>--- a/kernel/resource.c
>+++ b/kernel/resource.c
>@@ -1258,21 +1258,28 @@ EXPORT_SYMBOL(__release_region);
>  *   assumes that all children remain in the lower address entry for
>  *   simplicity.  Enhance this logic when necessary.
>  */
>-int release_mem_region_adjustable(struct resource *parent,
>-resource_size_t start, resource_size_t size)
>+void release_mem_region_adjustable(struct resource *parent,
>+ resource_size_t start, resource_size_t size)
> {
>+  struct resource *new_res = NULL;
>+  bool alloc_nofail = false;
>   struct resource **p;
>   struct resource *res;
>-  struct resource *new_res;
>   resource_size_t end;
>-  int ret = -EINVAL;
> 
>   end = start + size - 1;
>-  if ((start < parent->start) || (end > parent->end))
>-  return ret;
>+  if (WARN_ON_ONCE((start < parent->start) || (end > parent->end)))
>+  return;
> 
>-  /* The alloc_resource() result gets checked later */
>-  new_res = alloc_resource(GFP_KERNEL);
>+  /*
>+   * We free up quite a lot of memory on memory hotunplug (esp., memap),
>+   * just before releasing the region. This is highly unlikely to
>+   * fail - let's play save and make it never fail as the caller cannot
>+   * perform any error handling (e.g., trying to re-add memory will fail
>+   * similarly).
>+   */
>+retry:
>+  new_res = alloc_resource(GFP_KERNEL | alloc_nofail ? __GFP_NOFAIL : 0);
> 

It looks like a bold change, while I don't find a reason to object it.

>   p = &parent->child;
>   write_lock(&resource_lock);
>@@ -1298,7 +1305,6 @@ int release_mem_region_adjustable(struct resource 
>*parent,
>* so if we are dealing with them, let us just back off here.
>*/
>   if (!(res->flags & IORESOURCE_SYSRAM)) {
>-  ret = 0;
>   break;
>   }
> 
>@@ -1315,20 +1321,23 @@ int release_mem_region_adjustable(struct resource 
>*parent,
>   /* free the whole entry */
>   *p = res->sibling;
>   free_resource(res);
>-  ret = 0;
>   } else if (res->start == start && res->end != end) {
>   /* adjust the start */
>-  ret = __adjust_resource(res, end + 1,
>-  res->end - end);
>+  WARN_ON_ONCE(__adjust_resource(res, end + 1,
>+ res->end - end));
>   } else if (res->start != start && res->end == end) {
>   /* adjust the end */
>-  ret = __adjust_resource(res, res->start,
>-  start - res->start);
>+  WARN_ON_ONCE(__adjust_resource(res, res->start,
>+ start - res->start

Re: [PATCH v2 2/7] kernel/resource: move and rename IORESOURCE_MEM_DRIVER_MANAGED

[Wei Yang]

On Tue, Sep 08, 2020 at 10:10:07PM +0200, David Hildenbrand wrote:
>IORESOURCE_MEM_DRIVER_MANAGED currently uses an unused PnP bit, which is
>always set to 0 by hardware. This is far from beautiful (and confusing),
>and the bit only applies to SYSRAM. So let's move it out of the
>bus-specific (PnP) defined bits.
>
>We'll add another SYSRAM specific bit soon. If we ever need more bits for
>other purposes, we can steal some from "desc", or reshuffle/regroup what we
>have.

I think you make this definition because we use IORESOURCE_SYSRAM_RAM for
hotpluged memory? So we make them all in IORESOURCE_SYSRAM_XXX family?

>
>Cc: Andrew Morton 
>Cc: Michal Hocko 
>Cc: Dan Williams 
>Cc: Jason Gunthorpe 
>Cc: Kees Cook 
>Cc: Ard Biesheuvel 
>Cc: Pankaj Gupta 
>Cc: Baoquan He 
>Cc: Wei Yang 
>Cc: Eric Biederman 
>Cc: Thomas Gleixner 
>Cc: Greg Kroah-Hartman 
>Cc: ke...@lists.infradead.org
>Signed-off-by: David Hildenbrand 
>---
> include/linux/ioport.h | 4 +++-
> kernel/kexec_file.c| 2 +-
> mm/memory_hotplug.c| 4 ++--
> 3 files changed, 6 insertions(+), 4 deletions(-)
>
>diff --git a/include/linux/ioport.h b/include/linux/ioport.h
>index 52a91f5fa1a36..d7620d7c941a0 100644
>--- a/include/linux/ioport.h
>+++ b/include/linux/ioport.h
>@@ -58,6 +58,9 @@ struct resource {
> #define IORESOURCE_EXT_TYPE_BITS 0x0100   /* Resource extended types */
> #define IORESOURCE_SYSRAM 0x0100  /* System RAM (modifier) */
> 
>+/* IORESOURCE_SYSRAM specific bits. */
>+#define IORESOURCE_SYSRAM_DRIVER_MANAGED  0x0200 /* Always detected 
>via a driver. */
>+
> #define IORESOURCE_EXCLUSIVE  0x0800  /* Userland may not map this 
> resource */
> 
> #define IORESOURCE_DISABLED   0x1000
>@@ -103,7 +106,6 @@ struct resource {
> #define IORESOURCE_MEM_32BIT  (3<<3)
> #define IORESOURCE_MEM_SHADOWABLE (1<<5)  /* dup: IORESOURCE_SHADOWABLE */
> #define IORESOURCE_MEM_EXPANSIONROM   (1<<6)
>-#define IORESOURCE_MEM_DRIVER_MANAGED (1<<7)
> 
> /* PnP I/O specific bits (IORESOURCE_BITS) */
> #define IORESOURCE_IO_16BIT_ADDR  (1<<0)
>diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
>index ca40bef75a616..dfeeed1aed084 100644
>--- a/kernel/kexec_file.c
>+++ b/kernel/kexec_file.c
>@@ -520,7 +520,7 @@ static int locate_mem_hole_callback(struct resource *res, 
>void *arg)
>   /* Returning 0 will take to next memory range */
> 
>   /* Don't use memory that will be detected and handled by a driver. */
>-  if (res->flags & IORESOURCE_MEM_DRIVER_MANAGED)
>+  if (res->flags & IORESOURCE_SYSRAM_DRIVER_MANAGED)
>   return 0;
> 
>   if (sz < kbuf->memsz)
>diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>index 4c47b68a9f4b5..8e1cd18b5cf14 100644
>--- a/mm/memory_hotplug.c
>+++ b/mm/memory_hotplug.c
>@@ -105,7 +105,7 @@ static struct resource *register_memory_resource(u64 
>start, u64 size,
>   unsigned long flags =  IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY;
> 
>   if (strcmp(resource_name, "System RAM"))
>-  flags |= IORESOURCE_MEM_DRIVER_MANAGED;
>+  flags |= IORESOURCE_SYSRAM_DRIVER_MANAGED;
> 
>   /*
>* Make sure value parsed from 'mem=' only restricts memory adding
>@@ -1160,7 +1160,7 @@ EXPORT_SYMBOL_GPL(add_memory);
>  *
>  * For this memory, no entries in /sys/firmware/memmap ("raw firmware-provided
>  * memory map") are created. Also, the created memory resource is flagged
>- * with IORESOURCE_MEM_DRIVER_MANAGED, so in-kernel users can special-case
>+ * with IORESOURCE_SYSRAM_DRIVER_MANAGED, so in-kernel users can special-case
>  * this memory as well (esp., not place kexec images onto it).
>  *
>  * The resource_name (visible via /proc/iomem) has to have the format
>-- 
>2.26.2

-- 
Wei Yang
Help you, Help me

Re: [PATCH v2 1/7] kernel/resource: make release_mem_region_adjustable() never fail

[Wei Yang]

On Tue, Sep 08, 2020 at 10:10:06PM +0200, David Hildenbrand wrote:
>Let's make sure splitting a resource on memory hotunplug will never fail.
>This will become more relevant once we merge selected System RAM
>resources - then, we'll trigger that case more often on memory hotunplug.
>
>In general, this function is already unlikely to fail. When we remove
>memory, we free up quite a lot of metadata (memmap, page tables, memory
>block device, etc.). The only reason it could really fail would be when
>injecting allocation errors.
>
>All other error cases inside release_mem_region_adjustable() seem to be
>sanity checks if the function would be abused in different context -
>let's add WARN_ON_ONCE() in these cases so we can catch them.
>
>Cc: Andrew Morton 
>Cc: Michal Hocko 
>Cc: Dan Williams 
>Cc: Jason Gunthorpe 
>Cc: Kees Cook 
>Cc: Ard Biesheuvel 
>Cc: Pankaj Gupta 
>Cc: Baoquan He 
>Cc: Wei Yang 
>Signed-off-by: David Hildenbrand 
>---
> include/linux/ioport.h |  4 ++--
> kernel/resource.c  | 49 --
> mm/memory_hotplug.c| 22 +--
> 3 files changed, 31 insertions(+), 44 deletions(-)
>
>diff --git a/include/linux/ioport.h b/include/linux/ioport.h
>index 6c2b06fe8beb7..52a91f5fa1a36 100644
>--- a/include/linux/ioport.h
>+++ b/include/linux/ioport.h
>@@ -248,8 +248,8 @@ extern struct resource * __request_region(struct resource 
>*,
> extern void __release_region(struct resource *, resource_size_t,
>   resource_size_t);
> #ifdef CONFIG_MEMORY_HOTREMOVE
>-extern int release_mem_region_adjustable(struct resource *, resource_size_t,
>-  resource_size_t);
>+extern void release_mem_region_adjustable(struct resource *, resource_size_t,
>+resource_size_t);
> #endif
> 
> /* Wrappers for managed devices */
>diff --git a/kernel/resource.c b/kernel/resource.c
>index f1175ce93a1d5..36b3552210120 100644
>--- a/kernel/resource.c
>+++ b/kernel/resource.c
>@@ -1258,21 +1258,28 @@ EXPORT_SYMBOL(__release_region);
>  *   assumes that all children remain in the lower address entry for
>  *   simplicity.  Enhance this logic when necessary.
>  */
>-int release_mem_region_adjustable(struct resource *parent,
>-resource_size_t start, resource_size_t size)
>+void release_mem_region_adjustable(struct resource *parent,
>+ resource_size_t start, resource_size_t size)
> {
>+  struct resource *new_res = NULL;
>+  bool alloc_nofail = false;
>   struct resource **p;
>   struct resource *res;
>-  struct resource *new_res;
>   resource_size_t end;
>-  int ret = -EINVAL;
> 
>   end = start + size - 1;
>-  if ((start < parent->start) || (end > parent->end))
>-  return ret;
>+  if (WARN_ON_ONCE((start < parent->start) || (end > parent->end)))
>+  return;
> 
>-  /* The alloc_resource() result gets checked later */
>-  new_res = alloc_resource(GFP_KERNEL);
>+  /*
>+   * We free up quite a lot of memory on memory hotunplug (esp., memap),
>+   * just before releasing the region. This is highly unlikely to
>+   * fail - let's play save and make it never fail as the caller cannot
>+   * perform any error handling (e.g., trying to re-add memory will fail
>+   * similarly).
>+   */
>+retry:
>+  new_res = alloc_resource(GFP_KERNEL | alloc_nofail ? __GFP_NOFAIL : 0);
> 
>   p = &parent->child;
>   write_lock(&resource_lock);
>@@ -1298,7 +1305,6 @@ int release_mem_region_adjustable(struct resource 
>*parent,
>* so if we are dealing with them, let us just back off here.
>*/
>   if (!(res->flags & IORESOURCE_SYSRAM)) {
>-  ret = 0;
>   break;
>   }
> 
>@@ -1315,20 +1321,23 @@ int release_mem_region_adjustable(struct resource 
>*parent,
>   /* free the whole entry */
>   *p = res->sibling;
>   free_resource(res);
>-  ret = 0;
>   } else if (res->start == start && res->end != end) {
>   /* adjust the start */
>-  ret = __adjust_resource(res, end + 1,
>-  res->end - end);
>+  WARN_ON_ONCE(__adjust_resource(res, end + 1,
>+ res->end - end));
>   } else if (res->start != start && res->end == end) {
>   /* adjust the end */
>-  ret = __adjust_resource(res, res->start,
>-  start - res->start);
>+  WARN_ON_ONCE(__adjust_resource(res, res->start,
>+ start - res->start));
>   } else {
>-  /* split into two en

Re: [PATCH v4 5/8] mm/memory_hotplug: MEMHP_MERGE_RESOURCE to specify merging of System RAM resources

[Wei Yang]

On Fri, Sep 11, 2020 at 12:34:56PM +0200, David Hildenbrand wrote:
>Some add_memory*() users add memory in small, contiguous memory blocks.
>Examples include virtio-mem, hyper-v balloon, and the XEN balloon.
>
>This can quickly result in a lot of memory resources, whereby the actual
>resource boundaries are not of interest (e.g., it might be relevant for
>DIMMs, exposed via /proc/iomem to user space). We really want to merge
>added resources in this scenario where possible.
>
>Let's provide a flag (MEMHP_MERGE_RESOURCE) to specify that a resource
>either created within add_memory*() or passed via add_memory_resource()
>shall be marked mergeable and merged with applicable siblings.
>
>To implement that, we need a kernel/resource interface to mark selected
>System RAM resources mergeable (IORESOURCE_SYSRAM_MERGEABLE) and trigger
>merging.
>
>Note: We really want to merge after the whole operation succeeded, not
>directly when adding a resource to the resource tree (it would break
>add_memory_resource() and require splitting resources again when the
>operation failed - e.g., due to -ENOMEM).

Oops, the latest version is here.

BTW, I don't see patch 4. Not sure it is junked by my mail system?

>
>Reviewed-by: Pankaj Gupta 
>Cc: Andrew Morton 
>Cc: Michal Hocko 
>Cc: Dan Williams 
>Cc: Jason Gunthorpe 
>Cc: Kees Cook 
>Cc: Ard Biesheuvel 
>Cc: Thomas Gleixner 
>Cc: "K. Y. Srinivasan" 
>Cc: Haiyang Zhang 
>Cc: Stephen Hemminger 
>Cc: Wei Liu 
>Cc: Boris Ostrovsky 
>Cc: Juergen Gross 
>Cc: Stefano Stabellini 
>Cc: Roger Pau Monné 
>Cc: Julien Grall 
>Cc: Pankaj Gupta 
>Cc: Baoquan He 
>Cc: Wei Yang 
>Signed-off-by: David Hildenbrand 
>---
> include/linux/ioport.h |  4 +++
> include/linux/memory_hotplug.h |  7 
> kernel/resource.c  | 60 ++
> mm/memory_hotplug.c|  7 
> 4 files changed, 78 insertions(+)
>
>diff --git a/include/linux/ioport.h b/include/linux/ioport.h
>index d7620d7c941a0..7e61389dcb017 100644
>--- a/include/linux/ioport.h
>+++ b/include/linux/ioport.h
>@@ -60,6 +60,7 @@ struct resource {
> 
> /* IORESOURCE_SYSRAM specific bits. */
> #define IORESOURCE_SYSRAM_DRIVER_MANAGED  0x0200 /* Always detected 
> via a driver. */
>+#define IORESOURCE_SYSRAM_MERGEABLE   0x0400 /* Resource can be 
>merged. */
> 
> #define IORESOURCE_EXCLUSIVE  0x0800  /* Userland may not map this 
> resource */
> 
>@@ -253,6 +254,9 @@ extern void __release_region(struct resource *, 
>resource_size_t,
> extern void release_mem_region_adjustable(struct resource *, resource_size_t,
> resource_size_t);
> #endif
>+#ifdef CONFIG_MEMORY_HOTPLUG
>+extern void merge_system_ram_resource(struct resource *res);
>+#endif
> 
> /* Wrappers for managed devices */
> struct device;
>diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
>index 33eb80fdba22f..d65c6fdc5cfc3 100644
>--- a/include/linux/memory_hotplug.h
>+++ b/include/linux/memory_hotplug.h
>@@ -62,6 +62,13 @@ typedef int __bitwise mhp_t;
> 
> /* No special request */
> #define MHP_NONE  ((__force mhp_t)0)
>+/*
>+ * Allow merging of the added System RAM resource with adjacent,
>+ * mergeable resources. After a successful call to add_memory_resource()
>+ * with this flag set, the resource pointer must no longer be used as it
>+ * might be stale, or the resource might have changed.
>+ */
>+#define MEMHP_MERGE_RESOURCE  ((__force mhp_t)BIT(0))
> 
> /*
>  * Extended parameters for memory hotplug:
>diff --git a/kernel/resource.c b/kernel/resource.c
>index 36b3552210120..7a91b935f4c20 100644
>--- a/kernel/resource.c
>+++ b/kernel/resource.c
>@@ -1363,6 +1363,66 @@ void release_mem_region_adjustable(struct resource 
>*parent,
> }
> #endif/* CONFIG_MEMORY_HOTREMOVE */
> 
>+#ifdef CONFIG_MEMORY_HOTPLUG
>+static bool system_ram_resources_mergeable(struct resource *r1,
>+ struct resource *r2)
>+{
>+  /* We assume either r1 or r2 is IORESOURCE_SYSRAM_MERGEABLE. */
>+  return r1->flags == r2->flags && r1->end + 1 == r2->start &&
>+ r1->name == r2->name && r1->desc == r2->desc &&
>+ !r1->child && !r2->child;
>+}
>+
>+/*
>+ * merge_system_ram_resource - mark the System RAM resource mergeable and try 
>to
>+ * merge it with adjacent, mergeable resources
>+ * @res: resource descriptor
>+ *
>+ * This interface is intended for memory hotplug, whereby lots of contiguous
>+ * system ram resources are added (e.g., via add_memory*()) by a driver, and
>+ * the actual resource boundaries are not of interest (e.g., it might be
>+ * relevant for DIMMs). Only resources that are marked mergeable, that have 
>the
>+ * same parent, and that don't have any children are considered. All mergeable
>+ * resources must be immutable during the request.
>+ *
>+ * Note:
>+ * - The caller has to make sure that no pointers to resources that are
>+ *   marked mergeable are use

RE: [PATCH 1/5] x86/asm: Rename FS/GS base helpers

[Tian, Kevin]

> From: Andrew Cooper 
> Sent: Wednesday, September 9, 2020 5:59 PM
> 
> They are currently named after the FSGSBASE instructions, but are not thin
> wrappers around said instructions, and therefore do not accurately reflect
> the
> logic they perform, especially when it comes to functioning safely in non
> FSGSBASE context.
> 
> Rename them to {read,write}_{fs,gs}_base() to avoid confusion.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper 

Reviewed-by: Kevin Tian 

> ---
> CC: Jan Beulich 
> CC: Roger Pau Monné 
> CC: Wei Liu 
> CC: Jun Nakajima 
> CC: Kevin Tian 
> ---
>  xen/arch/x86/domain.c  | 10 +-
>  xen/arch/x86/hvm/vmx/vmx.c |  8 
>  xen/arch/x86/pv/domain.c   |  2 +-
>  xen/arch/x86/pv/emul-priv-op.c | 14 +++---
>  xen/arch/x86/x86_64/mm.c   |  8 
>  xen/arch/x86/x86_64/traps.c|  6 +++---
>  xen/include/asm-x86/msr.h  | 12 ++--
>  7 files changed, 30 insertions(+), 30 deletions(-)
> 
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index e8e91cf080..2271bee36a 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1581,9 +1581,9 @@ static void load_segments(struct vcpu *n)
> 
>  if ( !fs_gs_done && !compat )
>  {
> -wrfsbase(n->arch.pv.fs_base);
> -wrgsshadow(n->arch.pv.gs_base_kernel);
> -wrgsbase(n->arch.pv.gs_base_user);
> +write_fs_base(n->arch.pv.fs_base);
> +write_gs_shadow(n->arch.pv.gs_base_kernel);
> +write_gs_base(n->arch.pv.gs_base_user);
> 
>  /* If in kernel mode then switch the GS bases around. */
>  if ( (n->arch.flags & TF_kernel_mode) )
> @@ -1710,9 +1710,9 @@ static void save_segments(struct vcpu *v)
> 
>  if ( !is_pv_32bit_vcpu(v) )
>  {
> -unsigned long gs_base = rdgsbase();
> +unsigned long gs_base = read_gs_base();
> 
> -v->arch.pv.fs_base = rdfsbase();
> +v->arch.pv.fs_base = read_fs_base();
>  if ( v->arch.flags & TF_kernel_mode )
>  v->arch.pv.gs_base_kernel = gs_base;
>  else
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index c4b40bf3cb..d26e102970 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -512,12 +512,12 @@ static void vmx_save_guest_msrs(struct vcpu *v)
>   * We cannot cache SHADOW_GS_BASE while the VCPU runs, as it can
>   * be updated at any time via SWAPGS, which we cannot trap.
>   */
> -v->arch.hvm.vmx.shadow_gs = rdgsshadow();
> +v->arch.hvm.vmx.shadow_gs = read_gs_shadow();
>  }
> 
>  static void vmx_restore_guest_msrs(struct vcpu *v)
>  {
> -wrgsshadow(v->arch.hvm.vmx.shadow_gs);
> +write_gs_shadow(v->arch.hvm.vmx.shadow_gs);
>  wrmsrl(MSR_STAR,   v->arch.hvm.vmx.star);
>  wrmsrl(MSR_LSTAR,  v->arch.hvm.vmx.lstar);
>  wrmsrl(MSR_SYSCALL_MASK,   v->arch.hvm.vmx.sfmask);
> @@ -2958,7 +2958,7 @@ static int vmx_msr_read_intercept(unsigned int
> msr, uint64_t *msr_content)
>  break;
> 
>  case MSR_SHADOW_GS_BASE:
> -*msr_content = rdgsshadow();
> +*msr_content = read_gs_shadow();
>  break;
> 
>  case MSR_STAR:
> @@ -3190,7 +3190,7 @@ static int vmx_msr_write_intercept(unsigned int
> msr, uint64_t msr_content)
>  else if ( msr == MSR_GS_BASE )
>  __vmwrite(GUEST_GS_BASE, msr_content);
>  else
> -wrgsshadow(msr_content);
> +write_gs_shadow(msr_content);
> 
>  break;
> 
> diff --git a/xen/arch/x86/pv/domain.c b/xen/arch/x86/pv/domain.c
> index 44e4ea2582..663e76c773 100644
> --- a/xen/arch/x86/pv/domain.c
> +++ b/xen/arch/x86/pv/domain.c
> @@ -452,7 +452,7 @@ void toggle_guest_mode(struct vcpu *v)
>   * Update the cached value of the GS base about to become inactive, as a
>   * subsequent context switch won't bother re-reading it.
>   */
> -gs_base = rdgsbase();
> +gs_base = read_gs_base();
>  if ( v->arch.flags & TF_kernel_mode )
>  v->arch.pv.gs_base_kernel = gs_base;
>  else
> diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-
> op.c
> index a192160f84..9dd1d59423 100644
> --- a/xen/arch/x86/pv/emul-priv-op.c
> +++ b/xen/arch/x86/pv/emul-priv-op.c
> @@ -511,10 +511,10 @@ static int read_segment(enum x86_segment seg,
>  reg->base = 0;
>  break;
>  case x86_seg_fs:
> -reg->base = rdfsbase();
> +reg->base = read_fs_base();
>  break;
>  case x86_seg_gs:
> -reg->base = rdgsbase();
> +reg->base = read_gs_base();
>  break;
>  }
> 
> @@ -871,13 +871,13 @@ static int read_msr(unsigned int reg, uint64_t *val,
>  case MSR_FS_BASE:
>  if ( is_pv_32bit_domain(currd) )
>  break;
> -*val = rdfsbase();
> +*val = read_fs_base();
>  return X86EMUL_OKAY;
> 
>  case MSR_GS_BASE:
>  if

RE: [PATCH v4 2/5] x86/pv: allow reading FEATURE_CONTROL MSR

[Tian, Kevin]

> From: Roger Pau Monne 
> Sent: Monday, September 7, 2020 6:32 PM
> 
> Linux PV guests will attempt to read the FEATURE_CONTROL MSR, so move
> the handling done in VMX code into guest_rdmsr as it can be shared
> between PV and HVM guests that way.
> 
> Note that there's a slight behavior change and attempting to read the
> MSR when no features are available will result in a fault.
> 
> Signed-off-by: Roger Pau Monné 

Reviewed-by: Kevin Tian 

> ---
> Changes since v3:
>  - Only allow reading the MSR when there are bits available (different
>than bit 0).
> 
> Changes from v1:
>  - Move the VMX implementation into guest_rdmsr.
> ---
>  xen/arch/x86/hvm/vmx/vmx.c |  8 +---
>  xen/arch/x86/msr.c | 12 
>  2 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index c4b40bf3cb..709ea149d1 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2980,13 +2980,7 @@ static int vmx_msr_read_intercept(unsigned int
> msr, uint64_t *msr_content)
>  case MSR_IA32_DEBUGCTLMSR:
>  __vmread(GUEST_IA32_DEBUGCTL, msr_content);
>  break;
> -case MSR_IA32_FEATURE_CONTROL:
> -*msr_content = IA32_FEATURE_CONTROL_LOCK;
> -if ( vmce_has_lmce(curr) )
> -*msr_content |= IA32_FEATURE_CONTROL_LMCE_ON;
> -if ( nestedhvm_enabled(curr->domain) )
> -*msr_content |=
> IA32_FEATURE_CONTROL_ENABLE_VMXON_OUTSIDE_SMX;
> -break;
> +
>  case MSR_IA32_VMX_BASIC...MSR_IA32_VMX_VMFUNC:
>  if ( !nvmx_msr_read_intercept(msr, msr_content) )
>  goto gp_fault;
> diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
> index 74bf7d9589..79fbb9e940 100644
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -25,6 +25,7 @@
>  #include 
> 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -197,6 +198,17 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr,
> uint64_t *val)
>  /* Not offered to guests. */
>  goto gp_fault;
> 
> +case MSR_IA32_FEATURE_CONTROL:
> +if ( !cp->basic.vmx && !vmce_has_lmce(v) )
> +goto gp_fault;
> +
> +*val = IA32_FEATURE_CONTROL_LOCK;
> +if ( vmce_has_lmce(v) )
> +*val |= IA32_FEATURE_CONTROL_LMCE_ON;
> +if ( cp->basic.vmx )
> +*val |= IA32_FEATURE_CONTROL_ENABLE_VMXON_OUTSIDE_SMX;
> +break;
> +
>  case MSR_IA32_PLATFORM_ID:
>  if ( !(cp->x86_vendor & X86_VENDOR_INTEL) ||
>   !(boot_cpu_data.x86_vendor & X86_VENDOR_INTEL) )
> --
> 2.28.0

RE: [PATCH v3] x86/HVM: more consistently set I/O completion

[Tian, Kevin]

> From: Jan Beulich 
> Sent: Thursday, August 27, 2020 3:09 PM
> 
> Doing this just in hvm_emulate_one_insn() is not enough.
> hvm_ud_intercept() and hvm_emulate_one_vm_event() can get invoked for
> insns requiring one or more continuations, and at least in principle
> hvm_emulate_one_mmio() could, too. Without proper setting of the field,
> handle_hvm_io_completion() will do nothing completion-wise, and in
> particular the missing re-invocation of the insn emulation paths will
> lead to emulation caching not getting disabled in due course, causing
> the ASSERT() in {svm,vmx}_vmenter_helper() to trigger.
> 
> Reported-by: Don Slutz 
> 
> Similar considerations go for the clearing of vio->mmio_access, which
> gets moved as well.
> 
> Additionally all updating of vio->mmio_* now gets done dependent upon
> the new completion value, rather than hvm_ioreq_needs_completion()'s
> return value. This is because it is the completion chosen which controls
> what path will be taken when handling the completion, not the simple
> boolean return value. In particular, PIO completion doesn't involve
> going through the insn emulator, and hence emulator state ought to get
> cleared early (or it won't get cleared at all).
> 
> The new logic, besides allowing for a caller override for the
> continuation type to be set (for VMX real mode emulation), will also
> avoid setting an MMIO completion when a simpler PIO one will do. This
> is a minor optimization only as a side effect - the behavior is strictly
> needed at least for hvm_ud_intercept(), as only memory accesses can
> successfully complete through handle_mmio(). Care of course needs to be
> taken to correctly deal with "mixed" insns (doing both MMIO and PIO at
> the same time, i.e. INS/OUTS). For this, hvmemul_validate() now latches
> whether the insn being emulated is a memory access, as this information
> is no longer easily available at the point where we want to consume it.
> 
> Note that the presence of non-NULL .validate fields in the two ops
> structures in hvm_emulate_one_mmio() was really necessary even before
> the changes here: Without this, passing non-NULL as middle argument to
> hvm_emulate_init_once() is meaningless.
> 
> The restrictions on when the #UD intercept gets actually enabled are why
> it was decided that this is not a security issue:
> - the "hvm_fep" option to enable its use is a debugging option only,
> - for the cross-vendor case is considered experimental, even if
>   unfortunately SUPPORT.md doesn't have an explicit statement about
>   this.
> The other two affected functions are
> - hvm_emulate_one_vm_event(), used for introspection,
> - hvm_emulate_one_mmio(), used for Dom0 only,
> which aren't qualifying this as needing an XSA either.
> 
> Signed-off-by: Jan Beulich 
> Tested-by: Don Slutz 

Reviewed-by: Kevin Tian 

> ---
> v3: Add comment ahead of _hvm_emulate_one(). Add parentheses in a
> conditional expr. Justify why this does not need an XSA.
> v2: Make updating of vio->mmio_* fields fully driven by the new
> completion value.
> ---
> I further think that the entire tail of _hvm_emulate_one() (everything
> past the code changed/added there by this patch) wants skipping in case
> a completion is needed, at the very least for the mmio and realmode
> cases, where we know we'll come back here.
> 
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -1683,9 +1683,11 @@ static int hvmemul_validate(
>  const struct x86_emulate_state *state,
>  struct x86_emulate_ctxt *ctxt)
>  {
> -const struct hvm_emulate_ctxt *hvmemul_ctxt =
> +struct hvm_emulate_ctxt *hvmemul_ctxt =
>  container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
> 
> +hvmemul_ctxt->is_mem_access = x86_insn_is_mem_access(state, ctxt);
> +
>  return !hvmemul_ctxt->validate || hvmemul_ctxt->validate(state, ctxt)
> ? X86EMUL_OKAY : X86EMUL_UNHANDLEABLE;
>  }
> @@ -2610,8 +2612,13 @@ static const struct x86_emulate_ops hvm_
>  .vmfunc= hvmemul_vmfunc,
>  };
> 
> +/*
> + * Note that passing HVMIO_no_completion into this function serves as kind
> + * of (but not fully) an "auto select completion" indicator.
> + */
>  static int _hvm_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt,
> -const struct x86_emulate_ops *ops)
> +const struct x86_emulate_ops *ops,
> +enum hvm_io_completion completion)
>  {
>  const struct cpu_user_regs *regs = hvmemul_ctxt->ctxt.regs;
>  struct vcpu *curr = current;
> @@ -2642,16 +2649,31 @@ static int _hvm_emulate_one(struct hvm_e
>  rc = X86EMUL_RETRY;
> 
>  if ( !hvm_ioreq_needs_completion(&vio->io_req) )
> +completion = HVMIO_no_completion;
> +else if ( completion == HVMIO_no_completion )
> +completion = (vio->io_req.type != IOREQ_TYPE_PIO ||
> +  hvmemul_ctxt->is_mem_access) ? HVMIO_mmio_completion
> +   : HVMIO_pio_completion;
> +
> +switch ( vio->

[xen-unstable bisection] complete test-amd64-i386-xl-raw

[osstest service owner]

branch xen-unstable
xenbranch xen-unstable
job test-amd64-i386-xl-raw
testid guest-start

Tree: linux git://xenbits.xen.org/linux-pvops.git
Tree: linuxfirmware git://xenbits.xen.org/osstest/linux-firmware.git
Tree: qemu git://xenbits.xen.org/qemu-xen-traditional.git
Tree: qemuu git://xenbits.xen.org/qemu-xen.git
Tree: xen git://xenbits.xen.org/xen.git

*** Found and reproduced problem changeset ***

  Bug is in tree:  xen git://xenbits.xen.org/xen.git
  Bug introduced:  7c273ffdd0e91d9eeb975b7d531a4ed235931bb1
  Bug not present: 0b77395ef2f20058305240f2395883b1d961982a
  Last fail repro: http://logs.test-lab.xenproject.org/osstest/logs/154348/


  commit 7c273ffdd0e91d9eeb975b7d531a4ed235931bb1
  Author: Juergen Gross 
  Date:   Fri Aug 28 17:07:19 2020 +0200
  
  tools/python: drop libxenguest from setup.py
  
  There is not a single wrapper for a libxenguest function defined.
  So drop libxenguest from tools/python/setup.py.
  
  Signed-off-by: Juergen Gross 
  Acked-by: Marek Marczykowski-Górecki 


For bisection revision-tuple graph see:
   
http://logs.test-lab.xenproject.org/osstest/results/bisect/xen-unstable/test-amd64-i386-xl-raw.guest-start.html
Revision IDs in each graph node refer, respectively, to the Trees above.


Running cs-bisection-step 
--graph-out=/home/logs/results/bisect/xen-unstable/test-amd64-i386-xl-raw.guest-start
 --summary-out=tmp/154348.bisection-summary --basis-template=154016 
--blessings=real,real-bisect xen-unstable test-amd64-i386-xl-raw guest-start
Searching for failure / basis pass:
 154090 fail [host=chardonnay1] / 154016 [host=pinot0] 153983 [host=elbling1] 
153957 [host=huxelrebe0] 153931 [host=albana0] 153906 [host=fiano1] 153882 
[host=fiano0] 153845 [host=fiano0] 153813 [host=fiano0] 153788 [host=fiano0] 
153770 [host=fiano0] 153758 [host=fiano0] 153653 [host=chardonnay0] 153619 
[host=rimava1] 153602 ok.
Failure / basis pass flights: 154090 / 153602
(tree with no url: minios)
(tree with no url: ovmf)
(tree with no url: seabios)
Tree: linux git://xenbits.xen.org/linux-pvops.git
Tree: linuxfirmware git://xenbits.xen.org/osstest/linux-firmware.git
Tree: qemu git://xenbits.xen.org/qemu-xen-traditional.git
Tree: qemuu git://xenbits.xen.org/qemu-xen.git
Tree: xen git://xenbits.xen.org/xen.git
Latest c3038e718a19fc596f7b1baba0f83d5146dc7784 
c530a75c1e6a472b0eb9558310b518f0dfcd8860 
3d273dd05e51e5a1ffba3d98c7437ee84e8f8764 
ea6d3cd1ed79d824e605a70c3626bc437c386260 
cc13835377debe4e300c5f5f11f8f78920778c4e
Basis pass c3038e718a19fc596f7b1baba0f83d5146dc7784 
c530a75c1e6a472b0eb9558310b518f0dfcd8860 
3d273dd05e51e5a1ffba3d98c7437ee84e8f8764 
ea6d3cd1ed79d824e605a70c3626bc437c386260 
afe018e041ec112d90a8b4e6ed607d22aa06f280
Generating revisions with ./adhoc-revtuple-generator  
git://xenbits.xen.org/linux-pvops.git#c3038e718a19fc596f7b1baba0f83d5146dc7784-c3038e718a19fc596f7b1baba0f83d5146dc7784
 
git://xenbits.xen.org/osstest/linux-firmware.git#c530a75c1e6a472b0eb9558310b518f0dfcd8860-c530a75c1e6a472b0eb9558310b518f0dfcd8860
 
git://xenbits.xen.org/qemu-xen-traditional.git#3d273dd05e51e5a1ffba3d98c7437ee84e8f8764-3d273dd05e51e5a1ffba3d98c7437ee84e8f8764
 git://xenbits.xen.org/qemu-xen.git#ea6d3cd1ed79d824e605a70c3626bc4\
 37c386260-ea6d3cd1ed79d824e605a70c3626bc437c386260 
git://xenbits.xen.org/xen.git#afe018e041ec112d90a8b4e6ed607d22aa06f280-cc13835377debe4e300c5f5f11f8f78920778c4e
Loaded 5001 nodes in revision graph
Searching for test results:
 153400 [host=elbling1]
 153437 [host=elbling0]
 153468 [host=albana0]
 153494 [host=pinot1]
 153526 [host=pinot0]
 153551 [host=fiano1]
 153591 [host=huxelrebe1]
 153602 pass c3038e718a19fc596f7b1baba0f83d5146dc7784 
c530a75c1e6a472b0eb9558310b518f0dfcd8860 
3d273dd05e51e5a1ffba3d98c7437ee84e8f8764 
ea6d3cd1ed79d824e605a70c3626bc437c386260 
afe018e041ec112d90a8b4e6ed607d22aa06f280
 153619 [host=rimava1]
 153653 [host=chardonnay0]
 153758 [host=fiano0]
 153770 [host=fiano0]
 153788 [host=fiano0]
 153813 [host=fiano0]
 153845 [host=fiano0]
 153882 [host=fiano0]
 153906 [host=fiano1]
 153931 [host=albana0]
 153957 [host=huxelrebe0]
 153983 [host=elbling1]
 154016 [host=pinot0]
 154036 blocked c3038e718a19fc596f7b1baba0f83d5146dc7784 
c530a75c1e6a472b0eb9558310b518f0dfcd8860 
3d273dd05e51e5a1ffba3d98c7437ee84e8f8764 
ea6d3cd1ed79d824e605a70c3626bc437c386260 
89002866bb6c6f26024f015820c8f52012f95cf2
 154058 blocked c3038e718a19fc596f7b1baba0f83d5146dc7784 
c530a75c1e6a472b0eb9558310b518f0dfcd8860 
3d273dd05e51e5a1ffba3d98c7437ee84e8f8764 
ea6d3cd1ed79d824e605a70c3626bc437c386260 
89002866bb6c6f26024f015820c8f52012f95cf2
 154090 fail c3038e718a19fc596f7b1baba0f83d5146dc7784 
c530a75c1e6a472b0eb9558310b518f0dfcd8860 
3d273dd05e51e5a1ffba3d98c7437ee84e8f8764 
ea6d3cd1ed79d824e605a70c3626bc437c386260 
cc13835377debe4e300c5f5f11f8f78920778c4e
 154314 pass c3038e718a19fc596f7b1baba0f83d5146dc7784 
c530a75c1e6a472b0eb9558310b518f0dfcd8860 
3d273dd05e51e5a1ffba3d98c7437ee84e8

Re: preparations for 4.13.2 and 4.12.4

[Stefano Stabellini]

On Fri, 11 Sep 2020, Julien Grall wrote:
> Hi Bertrand,
> 
> On 11/09/2020 14:56, Bertrand Marquis wrote:
> > 
> > 
> > > On 11 Sep 2020, at 14:51, Julien Grall  wrote:
> > > 
> > > Hi Bertrand,
> > > 
> > > On 11/09/2020 14:32, Bertrand Marquis wrote:
> > > > > On 11 Sep 2020, at 14:11, Jan Beulich  wrote:
> > > > > 
> > > > > All,
> > > > > 
> > > > > the releases are about due, but will of course want to wait for the
> > > > > XSA fixes going public on the 22nd. Please point out backports
> > > > > you find missing from the respective staging branches, but which
> > > > > you consider relevant. (Ian, Julien, Stefano: I notice there once
> > > > > again haven't been any tools or Arm side backports at all so far
> > > > > since the most recent stable releases from these branches. But
> > > > > maybe there simply aren't any.)
> > > > > 
> > > > > One that I have queued already, but which first need to at least
> > > > > pass the push gate to master, are
> > > > > 
> > > > > 8efa46516c5f hvmloader: indicate ACPI tables with "ACPI data" type in
> > > > > e820
> > > > > e5a1b6f0d207 x86/mm: do not mark IO regions as Xen heap
> > > > > b4e41b1750d5 b4e41b1750d5 [4.14 only]
> > > > > 
> > > > > On the Arm side I'd also like to ask for
> > > > > 
> > > > > 5d45ecabe3c0 xen/arm64: force gcc 10+ to always inline generic atomics
> > > > > helpers
> > > > +1
> > > > Could those fixes also be considered:
> > > > 3b418b3326 arm: Add Neoverse N1 processor identification
> > > > 858c0be8c2 xen/arm: Enable CPU Erratum 1165522 for Neoverse
> > > > 1814a626fb xen/arm: Update silicon-errata.txt with the Neovers AT
> > > > erratum
> > > The processor is quite new so may I ask why we would want to backport on
> > > older Xen?
> > 
> > 4.14 at least would be good as it is the current stable and N1SDP is support
> > in Yocto which is based on 4.14.
> While I understand external project are often based on stable release, I don't
> want to always backport errata. Some of them are quite involved and this is a
> risk for others.

Yeah, I very much agree with this. Some of them are **very** involved,
we don't really want to backport them. Speaking of which, maybe we
should add some wording to SUPPORT.md about it? Currently it doesn't
say anything specific about errata.


> In this case, the erratum has already been implemented for other processors.
> So the risk is minimal.
> 
> > But as the official one will be on next Yocto release this would be ok to
> > consider only 4.14 here.
> 
> 4.14 only would be my preference.

These ones are so trivial that apply straight away to all trees. I
understand it is not your preference, but I'll backport them up until
4.12 unless you are strongly opposed to it.

Re: preparations for 4.13.2 and 4.12.4

[Stefano Stabellini]

On Fri, 11 Sep 2020, Julien Grall wrote:
> On 11/09/2020 14:11, Jan Beulich wrote:
> > All,
> > 
> > the releases are about due, but will of course want to wait for the
> > XSA fixes going public on the 22nd. Please point out backports
> > you find missing from the respective staging branches, but which
> > you consider relevant. (Ian, Julien, Stefano: I notice there once
> > again haven't been any tools or Arm side backports at all so far
> > since the most recent stable releases from these branches. But
> > maybe there simply aren't any.)
> > 
> > One that I have queued already, but which first need to at least
> > pass the push gate to master, are
> > 
> > 8efa46516c5f hvmloader: indicate ACPI tables with "ACPI data" type in e820
> > e5a1b6f0d207 x86/mm: do not mark IO regions as Xen heap
> > b4e41b1750d5 b4e41b1750d5 [4.14 only]
> > 
> > On the Arm side I'd also like to ask for
> > 
> > 5d45ecabe3c0 xen/arm64: force gcc 10+ to always inline generic atomics
> > helpers
> 
> Sounds good to me.
> 
> I would also add:
> 
> d501ef90ae7f xen/arm: cmpxchg: Add missing memory barriers in
> __cmpxchg_mb_timeout() [4.12+]

OK

Re: [PATCH v3 01/11] xen/manage: keep track of the on-going suspend mode

[boris . ostrovsky]

On 9/14/20 5:47 PM, Anchal Agarwal wrote:
> On Sun, Sep 13, 2020 at 11:43:30AM -0400, boris.ostrov...@oracle.com wrote:
>> CAUTION: This email originated from outside of the organization. Do not 
>> click links or open attachments unless you can confirm the sender and know 
>> the content is safe.
>>
>>
>>
>> On 8/21/20 6:25 PM, Anchal Agarwal wrote:
>>> Though, accquirng pm_mutex is still right thing to do, we may
>>> see deadlock if PM hibernation is interrupted by Xen suspend.
>>> PM hibernation depends on xenwatch thread to process xenbus state
>>> transactions, but the thread will sleep to wait pm_mutex which is
>>> already held by PM hibernation context in the scenario. Xen shutdown
>>> code may need some changes to avoid the issue.
>>
>>
>> Is it Xen's shutdown or suspend code that needs to address this? (Or I
>> may not understand what the problem is that you are describing)
>>
> Its Xen suspend code I think. If we do not take the system_transition_mutex
> in do_suspend then if hibernation is triggered in parallel to xen suspend 
> there
> could be issues. 


But you *are* taking this mutex to avoid this exact race, aren't you?


> Now this is still theoretical in my case and I havent been able
> to reproduce such a race. So the approach the original author took was to take
> this lock which to me seems right.
> And its Xen suspend and not Xen Shutdown. So basically if this scenario
> happens I am of the view one of other will fail to occur then how do we 
> recover
> or avoid this at all.
>
> Does that answer your question?
>


>>> +
>>> +static int xen_setup_pm_notifier(void)
>>> +{
>>> + if (!xen_hvm_domain() || xen_initial_domain())
>>> + return -ENODEV;
>>
>> I don't think this works anymore.
> What do you mean?
> The first check is for xen domain types and other is for architecture 
> support. 
> The reason I put this check here is because I wanted to segregate the two.
> I do not want to register this notifier at all for !hmv guest and also if its
> an initial control domain.
> The arm check only lands in notifier because once hibernate() api is called ->
> calls pm_notifier_call_chain for PM_HIBERNATION_PREPARE this will fail for
> aarch64. 
> Once we have support for aarch64 this notifier can go away altogether. 
>
> Is there any other reason I may be missing why we should move this check to
> notifier?


Not registering this notifier is equivalent to having it return NOTIFY_OK.


In your earlier versions just returning NOTIFY_OK was not sufficient for
hibernation to proceed since the notifier would also need to set
suspend_mode appropriately. But now your notifier essentially filters
out unsupported configurations. And so if it is not called your
configuration (e.g. PV domain) will be considered supported.


>> In the past your notifier would set suspend_mode (or something) but now
>> it really doesn't do anything except reports an error in some (ARM) cases.
>>
>> So I think you should move this check into the notifier.
>> (And BTW I still think PM_SUSPEND_PREPARE should return an error too.
>> The fact that we are using "suspend" in xen routine names is irrelevant)
>>
> I may have send "not-updated" version of the notifier's function change.
>
> +switch (pm_event) {
> +   case PM_HIBERNATION_PREPARE:
> +/* Guest hibernation is not supported for aarch64 currently*/
> +if (IS_ENABLED(CONFIG_ARM64)) {
> + ret = NOTIFY_BAD;   
>   
>   
>  
> + break;  
>   
>   
>  
> + }   
> +   case PM_RESTORE_PREPARE:
> +   case PM_POST_RESTORE:
> +   case PM_POST_HIBERNATION:
> +   default:
> +   ret = NOTIFY_OK;
> +}


There is no difference on x86 between this code and what you sent
earlier. In both instances PM_SUSPEND_PREPARE will return NOTIFY_OK.


On ARM this code will allow suspend to proceed (which is not what we want).


-boris


>
> With the above path PM_SUSPEND_PREPARE will go all together. Does that
> resolves this issue? I wanted to get rid of all SUSPEND_* as they are not 
> needed
> here clearly.
> The only reason I kept it there is if someone tries to trigger hibernation on
> ARM instances they should get an error. As I am not sure about the current
> behavior. There may be a better way to not invoke hibernation on ARM DomU's 
> and
> get rid of this block all together.
>
> Again, sorry for sending in the half baked fix. My workspace switch may have
> caused the error.
>>
>>
>> -boris
>>
> Anchal
>>
>>

Re: [PATCH v2 2/2] xen: Introduce cmpxchg64() and guest_cmpxchg64()

[Stefano Stabellini]

On Fri, 11 Sep 2020, Julien Grall wrote:
> From: Julien Grall 
> 
> The IOREQ code is using cmpxchg() with 64-bit value. At the moment, this
> is x86 code, but there is plan to make it common.
> 
> To cater 32-bit arch, introduce two new helpers to deal with 64-bit
> cmpxchg().
> 
> The Arm 32-bit implementation of cmpxchg64() is based on the __cmpxchg64
> in Linux v5.8 (arch/arm/include/asm/cmpxchg.h).
> 
> Note that only guest_cmpxchg64() is introduced on x86 so far.
> 
> Signed-off-by: Julien Grall 

Reviewed-by: Stefano Stabellini 


> ---
> 
> I have looked at whether we can extend cmpxchg() to support 64-bit on
> arm32 bit. However the resulting code is much worse as the compiler will
> use the stack more often to spill. Therefore the introduction of
> cmpxchg64() is better option.
> 
> Changes in v2:
> - Remove extra teq in the arm32 cmpxchg implementation
> - Don't define cmpxchg64() on x86
> - Drop _mb from the helpers name
> - Add missing barrier in the arm32 implementation
> ---
>  xen/include/asm-arm/arm32/cmpxchg.h | 68 +
>  xen/include/asm-arm/arm64/cmpxchg.h |  5 +++
>  xen/include/asm-arm/guest_atomics.h | 22 ++
>  xen/include/asm-x86/guest_atomics.h |  1 +
>  4 files changed, 96 insertions(+)
> 
> diff --git a/xen/include/asm-arm/arm32/cmpxchg.h 
> b/xen/include/asm-arm/arm32/cmpxchg.h
> index 3ef1e5c63276..b0bd1d8b685e 100644
> --- a/xen/include/asm-arm/arm32/cmpxchg.h
> +++ b/xen/include/asm-arm/arm32/cmpxchg.h
> @@ -87,6 +87,37 @@ __CMPXCHG_CASE(b, 1)
>  __CMPXCHG_CASE(h, 2)
>  __CMPXCHG_CASE( , 4)
>  
> +static inline bool __cmpxchg_case_8(volatile uint64_t *ptr,
> + uint64_t *old,
> + uint64_t new,
> + bool timeout,
> + unsigned int max_try)
> +{
> + uint64_t oldval;
> + uint64_t res;
> +
> + do {
> + asm volatile(
> + "   ldrexd  %1, %H1, [%3]\n"
> + "   teq %1, %4\n"
> + "   teqeq   %H1, %H4\n"
> + "   movne   %0, #0\n"
> + "   movne   %H0, #0\n"
> + "   bne 2f\n"
> + "   strexd  %0, %5, %H5, [%3]\n"
> + "2:"
> + : "=&r" (res), "=&r" (oldval), "+Qo" (*ptr)
> + : "r" (ptr), "r" (*old), "r" (new)
> + : "memory", "cc");
> + if (!res)
> + break;
> + } while (!timeout || ((--max_try) > 0));
> +
> + *old = oldval;
> +
> + return !res;
> +}
> +
>  static always_inline bool __int_cmpxchg(volatile void *ptr, unsigned long 
> *old,
>   unsigned long new, int size,
>   bool timeout, unsigned int max_try)
> @@ -145,11 +176,48 @@ static always_inline bool __cmpxchg_timeout(volatile 
> void *ptr,
>   return ret;
>  }
>  
> +/*
> + * The helper may fail to update the memory if the action takes too long.
> + *
> + * @old: On call the value pointed contains the expected old value. It will 
> be
> + * updated to the actual old value.
> + * @max_try: Maximum number of iterations
> + *
> + * The helper will return true when the update has succeeded (i.e no
> + * timeout) and false if the update has failed.
> + */
> +static always_inline bool __cmpxchg64_timeout(volatile uint64_t *ptr,
> +   uint64_t *old,
> +   uint64_t new,
> +   unsigned int max_try)
> +{
> + bool ret;
> +
> + smp_mb();
> + ret = __cmpxchg_case_8(ptr, old, new, true, max_try);
> + smp_mb();
> +
> + return ret;
> +}
> +
>  #define cmpxchg(ptr,o,n) \
>   ((__typeof__(*(ptr)))__cmpxchg((ptr),   \
>  (unsigned long)(o),  \
>  (unsigned long)(n),  \
>  sizeof(*(ptr
> +
> +static inline uint64_t cmpxchg64(volatile uint64_t *ptr,
> +  uint64_t old,
> +  uint64_t new)
> +{
> + smp_mb();
> + if (!__cmpxchg_case_8(ptr, &old, new, false, 0))
> + ASSERT_UNREACHABLE();
> + smp_mb();
> +
> + return old;
> +}
> +
>  #endif
>  /*
>   * Local variables:
> diff --git a/xen/include/asm-arm/arm64/cmpxchg.h 
> b/xen/include/asm-arm/arm64/cmpxchg.h
> index f4a8c1ccba80..10e4edc022b7 100644
> --- a/xen/include/asm-arm/arm64/cmpxchg.h
> +++ b/xen/include/asm-arm/arm64/cmpxchg.h
> @@ -167,6 +167,11 @@ static always_inline bool __cmpxchg_timeout(volatile 
> void *ptr,
>   __ret; \
>  })
>  
> +#define cmpxchg64(ptr, o, n) cmpxchg(ptr, o, n)
> +
> +#define __cmpxchg64_timeout

Re: [PATCH v2 1/2] xen/arm: Remove cmpxchg_local() and drop _mb from the other helpers

[Stefano Stabellini]

On Fri, 11 Sep 2020, Julien Grall wrote:
> From: Julien Grall 
> 
> The current set of helpers are quite confusing to follow as the naming
> is not very consistent.
> 
> Given that cmpxchg_local() is not used in Xen, drop it completely.
> Furthermore, adopt a naming with _mb so all names are now consistent.
> 
> Signed-off-by: Julien Grall 

Reviewed-by: Stefano Stabellini 


> ---
> Changes in v2:
> - Patch added
> ---
>  xen/include/asm-arm/arm32/cmpxchg.h | 31 ++-
>  xen/include/asm-arm/arm64/cmpxchg.h | 38 +++--
>  xen/include/asm-arm/guest_atomics.h |  6 ++---
>  3 files changed, 19 insertions(+), 56 deletions(-)
> 
> diff --git a/xen/include/asm-arm/arm32/cmpxchg.h 
> b/xen/include/asm-arm/arm32/cmpxchg.h
> index 0770f272ee99..3ef1e5c63276 100644
> --- a/xen/include/asm-arm/arm32/cmpxchg.h
> +++ b/xen/include/asm-arm/arm32/cmpxchg.h
> @@ -112,23 +112,12 @@ static always_inline unsigned long __cmpxchg(volatile 
> void *ptr,
>unsigned long new,
>int size)
>  {
> + smp_mb();
>   if (!__int_cmpxchg(ptr, &old, new, size, false, 0))
>   ASSERT_UNREACHABLE();
> -
> - return old;
> -}
> -
> -static always_inline unsigned long __cmpxchg_mb(volatile void *ptr,
> -unsigned long old,
> -unsigned long new, int size)
> -{
> - unsigned long ret;
> -
> - smp_mb();
> - ret = __cmpxchg(ptr, old, new, size);
>   smp_mb();
>  
> - return ret;
> + return old;
>  }
>  
>  /*
> @@ -141,11 +130,11 @@ static always_inline unsigned long 
> __cmpxchg_mb(volatile void *ptr,
>   * The helper will return true when the update has succeeded (i.e no
>   * timeout) and false if the update has failed.
>   */
> -static always_inline bool __cmpxchg_mb_timeout(volatile void *ptr,
> -unsigned long *old,
> -unsigned long new,
> -int size,
> -unsigned int max_try)
> +static always_inline bool __cmpxchg_timeout(volatile void *ptr,
> + unsigned long *old,
> + unsigned long new,
> + int size,
> + unsigned int max_try)
>  {
>   bool ret;
>  
> @@ -157,12 +146,6 @@ static always_inline bool __cmpxchg_mb_timeout(volatile 
> void *ptr,
>  }
>  
>  #define cmpxchg(ptr,o,n) \
> - ((__typeof__(*(ptr)))__cmpxchg_mb((ptr),\
> -   (unsigned long)(o),   \
> -   (unsigned long)(n),   \
> -   sizeof(*(ptr
> -
> -#define cmpxchg_local(ptr,o,n)   
> \
>   ((__typeof__(*(ptr)))__cmpxchg((ptr),   \
>  (unsigned long)(o),  \
>  (unsigned long)(n),  \
> diff --git a/xen/include/asm-arm/arm64/cmpxchg.h 
> b/xen/include/asm-arm/arm64/cmpxchg.h
> index fc5c60f0bd74..f4a8c1ccba80 100644
> --- a/xen/include/asm-arm/arm64/cmpxchg.h
> +++ b/xen/include/asm-arm/arm64/cmpxchg.h
> @@ -125,23 +125,12 @@ static always_inline unsigned long __cmpxchg(volatile 
> void *ptr,
>unsigned long new,
>int size)
>  {
> + smp_mb();
>   if (!__int_cmpxchg(ptr, &old, new, size, false, 0))
>   ASSERT_UNREACHABLE();
> -
> - return old;
> -}
> -
> -static always_inline unsigned long __cmpxchg_mb(volatile void *ptr,
> - unsigned long old,
> - unsigned long new, int size)
> -{
> - unsigned long ret;
> -
> - smp_mb();
> - ret = __cmpxchg(ptr, old, new, size);
>   smp_mb();
>  
> - return ret;
> + return old;
>  }
>  
>  /*
> @@ -154,11 +143,11 @@ static always_inline unsigned long 
> __cmpxchg_mb(volatile void *ptr,
>   * The helper will return true when the update has succeeded (i.e no
>   * timeout) and false if the update has failed.
>   */
> -static always_inline bool __cmpxchg_mb_timeout(volatile void *ptr,
> -unsigned long *old,
> -unsigned long new,
> -int size,
> -unsigned int max_try)
> +static always_inline bool __cmpxchg_timeout(volatile void *ptr,
> + unsigned long *old,
> 

[linux-linus test] 154336: regressions - FAIL

[osstest service owner]

flight 154336 linux-linus real [real]
http://logs.test-lab.xenproject.org/osstest/logs/154336/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-i386-xl-qemuu-ws16-amd64  7 xen-boot  fail REGR. vs. 152332
 test-amd64-i386-qemut-rhel6hvm-intel  7 xen-boot fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-dmrestrict-amd64-dmrestrict 7 xen-boot fail REGR. vs. 
152332
 test-amd64-i386-qemuu-rhel6hvm-intel  7 xen-boot fail REGR. vs. 152332
 test-amd64-i386-xl-xsm7 xen-boot fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-debianhvm-amd64-shadow 7 xen-boot fail REGR. vs. 
152332
 test-amd64-i386-xl-qemut-debianhvm-amd64  7 xen-boot fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-debianhvm-i386-xsm  7 xen-boot  fail REGR. vs. 152332
 test-amd64-i386-examine   8 reboot   fail REGR. vs. 152332
 test-amd64-i386-pair 10 xen-boot/src_hostfail REGR. vs. 152332
 test-amd64-i386-pair 11 xen-boot/dst_hostfail REGR. vs. 152332
 test-amd64-i386-libvirt   7 xen-boot fail REGR. vs. 152332
 test-amd64-i386-xl-qemut-ws16-amd64  7 xen-boot  fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-debianhvm-amd64  7 xen-boot fail REGR. vs. 152332
 test-amd64-i386-libvirt-xsm   7 xen-boot fail REGR. vs. 152332
 test-amd64-i386-qemut-rhel6hvm-amd  7 xen-boot   fail REGR. vs. 152332
 test-amd64-i386-qemuu-rhel6hvm-amd  7 xen-boot   fail REGR. vs. 152332
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 7 xen-boot fail REGR. vs. 
152332
 test-amd64-i386-xl7 xen-boot fail REGR. vs. 152332
 test-amd64-coresched-i386-xl  7 xen-boot fail REGR. vs. 152332
 test-amd64-i386-xl-raw7 xen-boot fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-win7-amd64  7 xen-boot  fail REGR. vs. 152332
 test-amd64-i386-freebsd10-amd64  7 xen-boot  fail REGR. vs. 152332
 test-amd64-i386-xl-pvshim 7 xen-boot fail REGR. vs. 152332
 test-amd64-i386-xl-qemut-debianhvm-i386-xsm  7 xen-boot  fail REGR. vs. 152332
 test-amd64-i386-xl-shadow 7 xen-boot fail REGR. vs. 152332
 test-amd64-i386-freebsd10-i386  7 xen-boot   fail REGR. vs. 152332
 test-amd64-i386-xl-qemut-win7-amd64  7 xen-boot  fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-ovmf-amd64  7 xen-boot  fail REGR. vs. 152332
 test-amd64-i386-libvirt-pair 10 xen-boot/src_hostfail REGR. vs. 152332
 test-amd64-i386-libvirt-pair 11 xen-boot/dst_hostfail REGR. vs. 152332
 test-amd64-i386-xl-qemut-stubdom-debianhvm-amd64-xsm 7 xen-boot fail REGR. vs. 
152332

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stopfail like 152332
 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 152332
 test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stopfail like 152332
 test-armhf-armhf-libvirt 14 saverestore-support-checkfail  like 152332
 test-armhf-armhf-xl-rtds 16 guest-start/debian.repeatfail  like 152332
 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail  like 152332
 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stopfail like 152332
 test-amd64-amd64-libvirt 13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-seattle  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-seattle  14 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  14 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check 
fail never pass
 test-arm64-arm64-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 13 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  14 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail   n

Re: [PATCH 1/3] x86/shim: fix build with PV_SHIM_EXCLUSIVE and SHADOW_PAGING

[Roger Pau Monné]

On Mon, Sep 14, 2020 at 02:38:49PM +0200, Jan Beulich wrote:
> While there's little point in enabling both, the combination ought to at
> least build correctly. Drop the direct PV_SHIM_EXCLUSIVE conditionals
> and instead zap PG_log_dirty to zero under the right conditions, and key
> other #ifdef-s off of that.
> 
> While there also expand on ded576ce07e9 ("x86/shadow: dirty VRAM
> tracking is needed for HVM only"): There was yet another is_hvm_domain()
> missing, and code touching the struct fields needs to be guarded by
> suitable #ifdef-s as well. While there also guard shadow-mode-only
> fields accordingly.
> 
> Fixes: 8b5b49ceb3d9 ("x86: don't include domctl and alike in shim-exclusive 
> builds")
> Reported-by: Andrew Cooper 
> Signed-off-by: Jan Beulich 

Reviewed-by: Roger Pau Monné 

We seem to be growing more and more ifdefs which is not ideal IMO, we
should rather aim to remove them by splitting code into separate
compilation units. There doesn't seem to be much option to split
stuff in this case, so be it.

Thanks, Roger.

Re: [PATCH v2 2/2] EFI: free unused boot mem in at least some cases

[Roger Pau Monné]

On Mon, Sep 14, 2020 at 05:26:57PM +0200, Jan Beulich wrote:
> On 14.09.2020 17:16, Roger Pau Monné wrote:
> > On Mon, Aug 24, 2020 at 02:08:11PM +0200, Jan Beulich wrote:
> >> Address at least the primary reason why 52bba67f8b87 ("efi/boot: Don't
> >> free ebmalloc area at all") was put in place: Make xen_in_range() aware
> >> of the freed range. This is in particular relevant for EFI-enabled
> >> builds not actually running on EFI, as the entire range will be unused
> >> in this case.
> >>
> >> Signed-off-by: Jan Beulich 
> > 
> > Acked-by: Roger Pau Monné 
> 
> Thanks much.
> 
> >> @@ -1145,7 +1146,8 @@ void __init noreturn __start_xen(unsigne
> >>  
> >>  /*
> >>   * This needs to remain in sync with xen_in_range() and the
> >> - * respective reserve_e820_ram() invocation below.
> >> + * respective reserve_e820_ram() invocation below. No need to
> >> + * query efi_boot_mem_unused() here, though.
> >>   */
> >>  mod[mbi->mods_count].mod_start = virt_to_mfn(_stext);
> >>  mod[mbi->mods_count].mod_end = __2M_rwdata_end - _stext;
> > 
> > I find this extremely confusing, we reuse mod_start/mod_end to contain
> > a mfn and a size (in bytes) instead of a start and end address (not
> > something that should be fixed here, but seeing this I assumed it was
> > wrong).
> 
> While perhaps somewhat confusing, I still think it was a fair thing
> to do in favor of introducing a completely new way of propagating
> respective information, and then having the consumer of this data
> look at two different places.

Maybe we could add a union there so that mod_end would alias with
mod_size or something.

> >> +bool efi_boot_mem_unused(unsigned long *start, unsigned long *end)
> >> +{
> >> +*start = (unsigned long)ebmalloc_mem + PAGE_ALIGN(ebmalloc_allocated);
> >> +*end = (unsigned long)ebmalloc_mem + sizeof(ebmalloc_mem);
> >> +
> >> +return *start < *end;
> >> +}
> >> +
> >>  void __init free_ebmalloc_unused_mem(void)
> >>  {
> >> -#if 0 /* FIXME: Putting a hole in the BSS breaks the IOMMU mappings for 
> >> dom0. */
> >>  unsigned long start, end;
> >>  
> >> -start = (unsigned long)ebmalloc_mem + PAGE_ALIGN(ebmalloc_allocated);
> >> -end = (unsigned long)ebmalloc_mem + sizeof(ebmalloc_mem);
> >> +#ifdef CONFIG_X86
> >> +/* FIXME: Putting a hole in .bss would shatter the large page 
> >> mapping. */
> > 
> > Could you make the ebmalloc size (EBMALLOC_SIZE) 2MB (and aligned), so
> > that you would only shatter the malloc'ed pages but not the
> > surrounding mappings?
> > 
> > That would be a good compromise IMO.
> 
> Yes, that's what I've been considering as a compromise as well. In
> fact I was further thinking whether to allocate the space from the
> linker script instead of having a global/static object. Maybe by
> extending into the .pad section, which is already 2Mb aligned anyway.

We would have to adjust the calculations then, but would be fine IMO
as it won't require poking a hole in the bss section. I assume we
would need to then move _end to cover it.

> Another option is to not further align the whole blob at all and
> merely free whatever comes past the next 2Mb boundary (and is not
> in use). This would avoid having an up to 2Mb block of unused, not
> freed memory ahead of the ebmalloc one.

I would just place it at the end of the loaded image (after bss) as it
seems simpler.

Roger.

[PATCH] SUPPORT.md: Mark Renesas IPMMU-VMSA (Arm) as supported

[Oleksandr Tyshchenko]

From: Oleksandr Tyshchenko 

And remove dependencies on CONFIG_EXPERT.

Signed-off-by: Oleksandr Tyshchenko 
---
 SUPPORT.md  | 2 +-
 xen/arch/arm/platforms/Kconfig  | 2 +-
 xen/drivers/passthrough/Kconfig | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/SUPPORT.md b/SUPPORT.md
index 1479055..5a96a12 100644
--- a/SUPPORT.md
+++ b/SUPPORT.md
@@ -64,7 +64,7 @@ supported in this document.
 Status, Intel VT-d: Supported
 Status, ARM SMMUv1: Supported
 Status, ARM SMMUv2: Supported
-Status, Renesas IPMMU-VMSA: Tech Preview
+Status, Renesas IPMMU-VMSA: Supported
 
 ### ARM/GICv3 ITS
 
diff --git a/xen/arch/arm/platforms/Kconfig b/xen/arch/arm/platforms/Kconfig
index 4bb7319..c93a6b2 100644
--- a/xen/arch/arm/platforms/Kconfig
+++ b/xen/arch/arm/platforms/Kconfig
@@ -25,7 +25,7 @@ config RCAR3
bool "Renesas RCar3 support"
depends on ARM_64
select HAS_SCIF
-   select IPMMU_VMSA if EXPERT
+   select IPMMU_VMSA
---help---
Enable all the required drivers for Renesas RCar3
 
diff --git a/xen/drivers/passthrough/Kconfig b/xen/drivers/passthrough/Kconfig
index 73f4ad8..0036007 100644
--- a/xen/drivers/passthrough/Kconfig
+++ b/xen/drivers/passthrough/Kconfig
@@ -14,7 +14,7 @@ config ARM_SMMU
  ARM SMMU architecture.
 
 config IPMMU_VMSA
-   bool "Renesas IPMMU-VMSA found in R-Car Gen3 SoCs" if EXPERT
+   bool "Renesas IPMMU-VMSA found in R-Car Gen3 SoCs"
depends on ARM_64
---help---
  Support for implementations of the Renesas IPMMU-VMSA found
-- 
2.7.4

Re: [PATCH] libxl: User defined max_maptrack_frames in a stub domain

[Andrew Cooper]

On 14/09/2020 15:50, Dmitry Fedorov wrote:
> Hi,
>
> Implementing qrexec+usbip+qemu in Linux-based stub domain leads me to
> an issue where a device model stub domain doesn't have maptrack entries.
>
> Would it be possible to apply a user defined max_maptrack_frames value
> to dm_config in the same way as for max_grant_frames?
>
> Signed-off-by: Dmitry Fedorov 

This looks entirely reasonable.

CC'ing the maintainers for their opinion.

~Andrew

> ---
>  tools/libxl/libxl_dm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index f2dc5696b9..f044f2566c 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -2292,7 +2292,7 @@ void libxl__spawn_stub_dm(libxl__egc *egc,
> libxl__stub_dm_spawn_state *sdss)
>  dm_config->b_info.target_memkb = dm_config->b_info.max_memkb;
>
>  dm_config->b_info.max_grant_frames =
> guest_config->b_info.max_grant_frames;
> -    dm_config->b_info.max_maptrack_frames = 0;
> +    dm_config->b_info.max_maptrack_frames =
> guest_config->b_info.max_maptrack_frames;
>
>  dm_config->b_info.u.pv.features = "";
>
> -- 
> 2.26.2
>
>

Re: libxl - b_info.{acpi,apic} behaves differently than b_info.u.hvm.{acpi,apic}

[Roger Pau Monné]

On Sun, Sep 13, 2020 at 01:12:39PM +0200, Marek Marczykowski-Górecki wrote:
> On Thu, Sep 10, 2020 at 12:58:57PM +0200, Marek Marczykowski-Górecki wrote:
> > On Thu, Sep 10, 2020 at 12:41:04PM +0200, Roger Pau Monné wrote:
> > > Adding toolstack maintainers.
> > > 
> > > On Thu, Sep 10, 2020 at 12:29:21PM +0200, Marek Marczykowski-Górecki 
> > > wrote:
> > > > On Thu, Sep 10, 2020 at 10:51:48AM +0200, Roger Pau Monné wrote:
> > > > > On Thu, Sep 10, 2020 at 05:57:23AM +0200, Marek Marczykowski-Górecki 
> > > > > wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > After updating from Xen 4.13 to Xen 4.14 I have troubles starting 
> > > > > > any
> > > > > > HVM: just after hvmloader saying "Invoking SeaBIOS" I get "(XEN) 
> > > > > > MMIO
> > > > > > emulation failed (1): d29v0 32bit @ 0008:fffeedf d -> "
> > > > > > 
> > > > > > I come to a situation where seemingly the same domU started via xl
> > > > > > works, while when started via libvirt it crashes. This seems to be
> > > > > > related to xl setting b_info.{acpi,apic}, while libvirt setting
> > > > > > b_info.u.hvm.{acpi,apic}. Modifying libvirt to use the former fixes 
> > > > > > the
> > > > > > issue.
> > > > > 
> > > > > Could you print the values of the involved fields at the end of
> > > > > libxl__domain_build_info_setdefault in both cases?
> > > > > 
> > > > > I'm not able to spot what changed between 4.13 and 4.14 that could
> > > > > alter the behavior, but knowing the values at that point might make
> > > > > it easier.
> > > > 
> > > > Sure, will do.
> > > > It may be also something else: maybe it acpi/apic settings were broken
> > > > before, but did not results in a domU crash this way.
> > > > FWIW when looking into /var/lib/xen/*-libxl-json I clearly see
> > > > difference between b_info.{acpi,apic} and b_info.u.hvm.{acpi,apic}.
> > > 
> > > I think libxl__domain_build_info_setdefault should check whether
> > > b_info.u.hvm.{acpi,apic} is set and copy those into b_info.{acpi,apic}
> > > if those are not set?
> > 
> > Looking at libxl__domain_build_info_setdefault this is not the case.
> > Instead there is libxl__acpi_defbool_val which looks at both.
> > Oh, and there is no similar thing for apic -> b_info.u.hvm.apic is
> > ignored!
> > 
> > > Toolstack people is more likely to have an opinion here, or to help
> > > debug the issue.
> 
> Ok, The crash reported initially was caused by a different thing: using
> seabios.bin instead of seabios-256k.bin (should that really cause the
> crash? shouldn't 128k seabios build work too?). But in any case, I think
> the b_info.u.hvm.{acpi,apic} is also not in a good shape.

Does 128K SeaBIOS have Xen support enabled?

Roger.

Re: [PATCH v2 2/2] EFI: free unused boot mem in at least some cases

[Roger Pau Monné]

On Mon, Aug 24, 2020 at 02:08:11PM +0200, Jan Beulich wrote:
> Address at least the primary reason why 52bba67f8b87 ("efi/boot: Don't
> free ebmalloc area at all") was put in place: Make xen_in_range() aware
> of the freed range. This is in particular relevant for EFI-enabled
> builds not actually running on EFI, as the entire range will be unused
> in this case.
> 
> Signed-off-by: Jan Beulich 

Acked-by: Roger Pau Monné 

Just one comment below.

> ---
> v2: Also adjust the two places where comments point out that they need
> to remain in sync with xen_in_range(). Add assertions to
> xen_in_range().
> ---
> The remaining issue could be addressed too, by making the area 2M in
> size and 2M-aligned.
> 
> --- a/xen/arch/x86/efi/stub.c
> +++ b/xen/arch/x86/efi/stub.c
> @@ -52,6 +52,12 @@ bool efi_enabled(unsigned int feature)
>  
>  void __init efi_init_memory(void) { }
>  
> +bool efi_boot_mem_unused(unsigned long *start, unsigned long *end)
> +{
> +*start = *end = (unsigned long)_end;
> +return false;
> +}
> +
>  void efi_update_l4_pgtable(unsigned int l4idx, l4_pgentry_t l4e) { }
>  
>  bool efi_rs_using_pgtables(void)
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -608,7 +608,7 @@ static void __init kexec_reserve_area(st
>  #endif
>  }
>  
> -static inline bool using_2M_mapping(void)
> +bool using_2M_mapping(void)
>  {
>  return !l1_table_offset((unsigned long)__2M_text_end) &&
> !l1_table_offset((unsigned long)__2M_rodata_start) &&
> @@ -830,6 +830,7 @@ void __init noreturn __start_xen(unsigne
>  module_t *mod;
>  unsigned long nr_pages, raw_max_page, modules_headroom, module_map[1];
>  int i, j, e820_warn = 0, bytes = 0;
> +unsigned long eb_start, eb_end;
>  bool acpi_boot_table_init_done = false, relocated = false;
>  int ret;
>  struct ns16550_defaults ns16550 = {
> @@ -1145,7 +1146,8 @@ void __init noreturn __start_xen(unsigne
>  
>  /*
>   * This needs to remain in sync with xen_in_range() and the
> - * respective reserve_e820_ram() invocation below.
> + * respective reserve_e820_ram() invocation below. No need to
> + * query efi_boot_mem_unused() here, though.
>   */
>  mod[mbi->mods_count].mod_start = virt_to_mfn(_stext);
>  mod[mbi->mods_count].mod_end = __2M_rwdata_end - _stext;

I find this extremely confusing, we reuse mod_start/mod_end to contain
a mfn and a size (in bytes) instead of a start and end address (not
something that should be fixed here, but seeing this I assumed it was
wrong).

> +bool efi_boot_mem_unused(unsigned long *start, unsigned long *end)
> +{
> +*start = (unsigned long)ebmalloc_mem + PAGE_ALIGN(ebmalloc_allocated);
> +*end = (unsigned long)ebmalloc_mem + sizeof(ebmalloc_mem);
> +
> +return *start < *end;
> +}
> +
>  void __init free_ebmalloc_unused_mem(void)
>  {
> -#if 0 /* FIXME: Putting a hole in the BSS breaks the IOMMU mappings for 
> dom0. */
>  unsigned long start, end;
>  
> -start = (unsigned long)ebmalloc_mem + PAGE_ALIGN(ebmalloc_allocated);
> -end = (unsigned long)ebmalloc_mem + sizeof(ebmalloc_mem);
> +#ifdef CONFIG_X86
> +/* FIXME: Putting a hole in .bss would shatter the large page mapping. */

Could you make the ebmalloc size (EBMALLOC_SIZE) 2MB (and aligned), so
that you would only shatter the malloc'ed pages but not the
surrounding mappings?

That would be a good compromise IMO.

Roger.

Re: [PATCH 2/5] x86/asm: Split __wr{fs,gs}base() out of write_{fs,gs}_base()

[Andrew Cooper]

On 10/09/2020 15:47, Jan Beulich wrote:
> On 09.09.2020 11:59, Andrew Cooper wrote:
>> To match the read side which is already split out.  A future change will want
>> to use them.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper 
> Acked-by: Jan Beulich 
> Of course ...
>
>> --- a/xen/include/asm-x86/msr.h
>> +++ b/xen/include/asm-x86/msr.h
>> @@ -156,6 +156,24 @@ static inline unsigned long __rdgsbase(void)
>>  return base;
>>  }
>>  
>> +static inline void __wrfsbase(unsigned long base)
>> +{
>> +#ifdef HAVE_AS_FSGSBASE
>> +asm volatile ( "wrfsbase %0" :: "r" (base) );
>> +#else
>> +asm volatile ( ".byte 0xf3, 0x48, 0x0f, 0xae, 0xd0" :: "a" (base) );
>> +#endif
>> +}
>> +
>> +static inline void __wrgsbase(unsigned long base)
>> +{
>> +#ifdef HAVE_AS_FSGSBASE
>> +asm volatile ( "wrgsbase %0" :: "r" (base) );
>> +#else
>> +asm volatile ( ".byte 0xf3, 0x48, 0x0f, 0xae, 0xd8" :: "a" (base) );
>> +#endif
>> +}
> ... I'd have preferred if you had used just a single leading
> underscore, despite realizing this would introduce an inconsistency
> with the read sides.

You're welcome to change them if you wish.

As always, I value consistency far far higher than arbitrary rules which
don't impact us in practice.

~Andrew

Re: [PATCH] tools: Delete XEN_DOMCTL_disable_migrate

[Andrew Cooper]

On 14/09/2020 09:43, Jan Beulich wrote:
> On 11.09.2020 21:06, Andrew Cooper wrote:
>> It is conceptually wrong for this information to exist in the hypervisor in
>> the first place.  Only the toolstack is capable of correctly reasoning about
>> the non-migrateability of guests.
> But isn't it the purpose of the domctl to tell Xen about the tool
> stack's decision in this regard?

There is nothing (not even ITSC handling, which was buggy) which Xen can
legitimately do with the information.

It is conceptually wrong, and a layering violation, for Xen to have this
information, or to be making any assumptions about it.

>> This hypercall has only ever existed to control the visibility of the
>> Invariant TSC flag to the guest.  Now that we have properly disentanged that
>> and moved ITSC into the guests CPUID policy, delete this hypercall.
>>
>> Furthermore, this fixes a corner case where Xen would override the toolstacks
>> choice of ITSC for a xenstore stubdomain.
> I'm afraid I don't fully understand: A xenstore stubdom can't be
> migrated (or at least it isn't supposed to be), can it?

That is some very wild assumptions.

What if someone is trying to debug a time related issue, and wants to
turn itsc off?

> In which
> case - what's wrong with exposing to it even by default a feature
> it may be able to make use of?

Because silently trampling the configuration chosen by the toolstack
isn't acceptable.

>  IOW ...
>
>> --- a/xen/arch/x86/cpuid.c
>> +++ b/xen/arch/x86/cpuid.c
>> @@ -708,7 +708,8 @@ int init_domain_cpuid_policy(struct domain *d)
>>  if ( !p )
>>  return -ENOMEM;
>>  
>> -if ( d->disable_migrate )
>> +/* The hardware domain can't migrate.  Give it ITSC if available. */
>> +if ( is_hardware_domain(d) )
>>  p->extd.itsc = cpu_has_itsc;
> ... why not include is_xenstore_domain() here that you remove from
> ...
>
>> @@ -452,9 +451,6 @@ struct domain *domain_create(domid_t domid,
>>  watchdog_domain_init(d);
>>  init_status |= INIT_watchdog;
>>  
>> -if ( is_xenstore_domain(d) )
>> -d->disable_migrate = true;
> ... here? On the tool stack side the change here only deletes code,
> i.e. I don't see you taking care of the default enabling there
> either. Am I overlooking any logic that now causes the feature to
> be requested for the xenstore domain without you needing to add
> any code?

The toolstack (legitimately) has knowledge of nomigrate, and will even
implicitly turn on ITSC as a side effect, but will also honour explicit
requests to turn it on or off.

>> --- a/xen/include/public/domctl.h
>> +++ b/xen/include/public/domctl.h
>> @@ -730,11 +730,6 @@ struct xen_domctl_hvmcontext_partial {
>>  XEN_GUEST_HANDLE_64(uint8) buffer;  /* OUT: buffer to write record into 
>> */
>>  };
>>  
>> -/* XEN_DOMCTL_disable_migrate */
>> -struct xen_domctl_disable_migrate {
>> -uint32_t disable; /* IN: 1: disable migration and restore */
>> -};
>> -
>>  
>>  /* XEN_DOMCTL_gettscinfo */
>>  /* XEN_DOMCTL_settscinfo */
>> @@ -1191,7 +1186,7 @@ struct xen_domctl {
>>  #define XEN_DOMCTL_gethvmcontext_partial 55
>>  #define XEN_DOMCTL_vm_event_op   56
>>  #define XEN_DOMCTL_mem_sharing_op57
>> -#define XEN_DOMCTL_disable_migrate   58
>> +/* #define XEN_DOMCTL_disable_migrate58 - Obsolete */
>>  #define XEN_DOMCTL_gettscinfo59
>>  #define XEN_DOMCTL_settscinfo60
>>  #define XEN_DOMCTL_getpageframeinfo3 61
>> @@ -1242,7 +1237,6 @@ struct xen_domctl {
>>  struct xen_domctl_ioport_permission ioport_permission;
>>  struct xen_domctl_hypercall_inithypercall_init;
>>  struct xen_domctl_settimeoffset settimeoffset;
>> -struct xen_domctl_disable_migrate   disable_migrate;
>>  struct xen_domctl_tsc_info  tsc_info;
>>  struct xen_domctl_hvmcontexthvmcontext;
>>  struct xen_domctl_hvmcontext_partial hvmcontext_partial;
> Deletion of sub-ops, just like their modification, requires the
> interface version to get bumped if it wasn't already during a
> release cycle. I know you dislike the underlying concept, but as
> long as the interface version continues to exist (with its
> present meaning) I'm afraid it needs bumping for any backwards-
> incompatible change.

I can fix it on commit.  I don't waste time tracking whether it has had
its conditional bump.

~Andrew

[xen-unstable-smoke test] 154341: tolerable all pass - PUSHED

[osstest service owner]

flight 154341 xen-unstable-smoke real [real]
http://logs.test-lab.xenproject.org/osstest/logs/154341/

Failures :-/ but no regressions.

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-libvirt 13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  14 saverestore-support-checkfail   never pass

version targeted for testing:
 xen  fc4e79c3f77f4360064f3614e32557a105458bae
baseline version:
 xen  d16467b18e0c0a77743c3111bed2a833a77fbfe7

Last test of basis   154332  2020-09-14 11:00:26 Z0 days
Testing same since   154341  2020-09-14 16:00:26 Z0 days1 attempts


People who touched revisions under test:
  Andrew Cooper 
  Wei Liu 

jobs:
 build-arm64-xsm  pass
 build-amd64  pass
 build-armhf  pass
 build-amd64-libvirt  pass
 test-armhf-armhf-xl  pass
 test-arm64-arm64-xl-xsm  pass
 test-amd64-amd64-xl-qemuu-debianhvm-amd64pass
 test-amd64-amd64-libvirt pass



sg-report-flight on osstest.test-lab.xenproject.org
logs: /home/logs/logs
images: /home/logs/images

Logs, config files, etc. are available at
http://logs.test-lab.xenproject.org/osstest/logs

Explanation of these reports, and of osstest in general, is at
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master

Test harness code can be found at
http://xenbits.xen.org/gitweb?p=osstest.git;a=summary


Pushing revision :

To xenbits.xen.org:/home/xen/git/xen.git
   d16467b18e..fc4e79c3f7  fc4e79c3f77f4360064f3614e32557a105458bae -> smoke

Re: [PATCH 01/20] drm/amdgpu: Introduce GEM object functions

[Christian König]

Am 14.09.20 um 17:05 schrieb Thomas Zimmermann:

Hi

Am 13.08.20 um 12:22 schrieb Christian König:

Am 13.08.20 um 10:36 schrieb Thomas Zimmermann:

GEM object functions deprecate several similar callback interfaces in
struct drm_driver. This patch replaces the per-driver callbacks with
per-instance callbacks in amdgpu. The only exception is gem_prime_mmap,
which is non-trivial to convert.

Signed-off-by: Thomas Zimmermann 
---
   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c    |  6 --
   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 12 
   2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 81a79760ca61..51525b8774c9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -1468,19 +1468,13 @@ static struct drm_driver kms_driver = {
   .lastclose = amdgpu_driver_lastclose_kms,
   .irq_handler = amdgpu_irq_handler,
   .ioctls = amdgpu_ioctls_kms,
-    .gem_free_object_unlocked = amdgpu_gem_object_free,
-    .gem_open_object = amdgpu_gem_object_open,
-    .gem_close_object = amdgpu_gem_object_close,
   .dumb_create = amdgpu_mode_dumb_create,
   .dumb_map_offset = amdgpu_mode_dumb_mmap,
   .fops = &amdgpu_driver_kms_fops,
     .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
   .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
-    .gem_prime_export = amdgpu_gem_prime_export,
   .gem_prime_import = amdgpu_gem_prime_import,
-    .gem_prime_vmap = amdgpu_gem_prime_vmap,
-    .gem_prime_vunmap = amdgpu_gem_prime_vunmap,
   .gem_prime_mmap = amdgpu_gem_prime_mmap,
     .name = DRIVER_NAME,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 43f4966331dd..ca2b79f94e99 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -36,6 +36,7 @@
   #include 
   #include 
   #include "amdgpu.h"
+#include "amdgpu_dma_buf.h"
   #include "amdgpu_trace.h"
   #include "amdgpu_amdkfd.h"
   @@ -510,6 +511,15 @@ bool amdgpu_bo_support_uswc(u64 bo_flags)
   #endif
   }
   +static const struct drm_gem_object_funcs amdgpu_gem_object_funcs = {
+    .free = amdgpu_gem_object_free,
+    .open = amdgpu_gem_object_open,
+    .close = amdgpu_gem_object_close,
+    .export = amdgpu_gem_prime_export,
+    .vmap = amdgpu_gem_prime_vmap,
+    .vunmap = amdgpu_gem_prime_vunmap,
+};
+

Wrong file, this belongs into amdgpu_gem.c


   static int amdgpu_bo_do_create(struct amdgpu_device *adev,
  struct amdgpu_bo_param *bp,
  struct amdgpu_bo **bo_ptr)
@@ -552,6 +562,8 @@ static int amdgpu_bo_do_create(struct
amdgpu_device *adev,
   bo = kzalloc(sizeof(struct amdgpu_bo), GFP_KERNEL);
   if (bo == NULL)
   return -ENOMEM;
+
+    bo->tbo.base.funcs = &amdgpu_gem_object_funcs;

And this should probably go into amdgpu_gem_object_create().

I'm trying to understand what amdgpu does.  What about all the places
where amdgpu calls amdgpu_bo_create() internally? Wouldn't these miss
the free callback for the GEM object?


Those shouldn't have a GEM object in the first place.

Or otherwise we would have a reference counting issue.

Regards,
Christian.



Best regards
Thomas


Apart from that looks like a good idea to me.

Christian.


   drm_gem_private_object_init(adev->ddev, &bo->tbo.base, size);
   INIT_LIST_HEAD(&bo->shadow_list);
   bo->vm_bo = NULL;


___
amd-gfx mailing list
amd-...@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] travis: Fix build with newer Qemu

[Andrew Cooper]

Qemu requires a bleeding edge version of Python, not found in the current
travis environment.  Skip building Qemu in that case.

Signed-off-by: Andrew Cooper 
---
CC: Doug Goldstein 
CC: Wei Liu 
---
 scripts/travis-build | 5 +
 1 file changed, 5 insertions(+)

diff --git a/scripts/travis-build b/scripts/travis-build
index 0cb15a89e4..08a1f66b84 100755
--- a/scripts/travis-build
+++ b/scripts/travis-build
@@ -16,6 +16,11 @@ cfgargs+=("--disable-rombios")
 cfgargs+=("--enable-docs")
 cfgargs+=("--with-system-seabios=/usr/share/seabios/bios.bin")
 
+# Qemu requires Python 3.7 or later
+if ! type python3 || python3 -c "import sys; res = sys.version_info < (3, 7); 
exit(not(res))"; then
+cfgargs+=("--with-system-qemu=/bin/false")
+fi
+
 if [[ "${XEN_TARGET_ARCH}" == "x86_64" ]]; then
 cfgargs+=("--enable-tools")
 else
-- 
2.11.0

Re: [RFC PATCH] efi: const correct EFI functions

[Trammell Hudson]

On Monday, September 14, 2020 10:55 AM, Jan Beulich  wrote:
> On 14.09.2020 16:46, Trammell Hudson wrote:
> > Option 3 would be to write wrappers for the few functions that are
> > used in the EFI boot path that cast-away the constness of their
> > arguments (while also silently cursing the UEFI forum for not
> > writing const-correct code).
>
> This would be kind of a last resort fallback (except for the
> cursing, which of course we can do at any time).

Since you didn't like the time travel option, I checked to see
which functions would need to be wrapped.  It is a surprisingly
small number:

#define PrintStr(s) StdOut->OutputString(StdOut, (CHAR16 *)(s))
#define PrintErr(s) StdErr->OutputString(StdErr, (CHAR16 *)(s))
#define efi_file_open(file,handle,name,mode,attr) \
  (file)->Open(file, handle, (CHAR16 *)(name), mode, attr)
#define shim_verify(shim, ptr, len) \
  (shim)->Verify((void *)(ptr), len)

--
Trammell

Re: [PATCH V1 06/16] xen/ioreq: Make x86's hvm_ioreq_(page/vcpu/server) structs common

[Julien Grall]

Hi Jan,

On 14/09/2020 16:16, Jan Beulich wrote:

On 10.09.2020 22:22, Oleksandr Tyshchenko wrote:

--- a/xen/include/xen/ioreq.h
+++ b/xen/include/xen/ioreq.h
@@ -23,6 +23,40 @@
  
  #include 
  
+struct hvm_ioreq_page {

+gfn_t gfn;
+struct page_info *page;
+void *va;
+};
+
+struct hvm_ioreq_vcpu {
+struct list_head list_entry;
+struct vcpu  *vcpu;
+evtchn_port_tioreq_evtchn;
+bool pending;
+};
+
+#define NR_IO_RANGE_TYPES (XEN_DMOP_IO_RANGE_PCI + 1)
+#define MAX_NR_IO_RANGES  256
+
+struct hvm_ioreq_server {
+struct domain  *target, *emulator;
+
+/* Lock to serialize toolstack modifications */
+spinlock_t lock;
+
+struct hvm_ioreq_page  ioreq;
+struct list_head   ioreq_vcpu_list;
+struct hvm_ioreq_page  bufioreq;
+
+/* Lock to serialize access to buffered ioreq ring */
+spinlock_t bufioreq_lock;
+evtchn_port_t  bufioreq_evtchn;
+struct rangeset*range[NR_IO_RANGE_TYPES];
+bool   enabled;
+uint8_tbufioreq_handling;
+};


Besides there again being the question of hvm_ prefixes here,
is the bufioreq concept something Arm is meaning to make use
of? If not, this may want to become conditional ...


Yes, I would like to make use of it to optimize virtio notifications as 
we don't need to wait for them to be processed by the IOREQ server.


Cheers,

--
Julien Grall

Re: [PATCH V1 07/16] xen/dm: Make x86's DM feature common

[Jan Beulich]

On 10.09.2020 22:22, Oleksandr Tyshchenko wrote:
> --- a/xen/include/xen/hypercall.h
> +++ b/xen/include/xen/hypercall.h
> @@ -150,6 +150,18 @@ do_dm_op(
>  unsigned int nr_bufs,
>  XEN_GUEST_HANDLE_PARAM(xen_dm_op_buf_t) bufs);
>  
> +struct dmop_args {
> +domid_t domid;
> +unsigned int nr_bufs;
> +/* Reserve enough buf elements for all current hypercalls. */
> +struct xen_dm_op_buf buf[2];
> +};
> +
> +int arch_dm_op(struct xen_dm_op *op,
> +   struct domain *d,
> +   const struct dmop_args *op_args,
> +   bool *const_op);
> +
>  #ifdef CONFIG_HYPFS
>  extern long
>  do_hypfs_op(

There are exactly two CUs which need to see these two declarations.
Personally I think they should go into a new header, or at least
into one that half-way fits (from the pov of its other contents)
and doesn't get included by half the code base. But maybe it's
just me ...

Jan

[ovmf test] 154333: all pass - PUSHED

[osstest service owner]

flight 154333 ovmf real [real]
http://logs.test-lab.xenproject.org/osstest/logs/154333/

Perfect :-)
All tests in this flight passed as required
version targeted for testing:
 ovmf 1b461403ee723dab01d5828714cca0b9396a6b3c
baseline version:
 ovmf 067503a8c675ddd38b099a0c604bc1a565e83838

Last test of basis   154312  2020-09-14 04:09:48 Z0 days
Testing same since   154333  2020-09-14 13:11:13 Z0 days1 attempts


People who touched revisions under test:
  Chasel Chiu 

jobs:
 build-amd64-xsm  pass
 build-i386-xsm   pass
 build-amd64  pass
 build-i386   pass
 build-amd64-libvirt  pass
 build-i386-libvirt   pass
 build-amd64-pvopspass
 build-i386-pvops pass
 test-amd64-amd64-xl-qemuu-ovmf-amd64 pass
 test-amd64-i386-xl-qemuu-ovmf-amd64  pass



sg-report-flight on osstest.test-lab.xenproject.org
logs: /home/logs/logs
images: /home/logs/images

Logs, config files, etc. are available at
http://logs.test-lab.xenproject.org/osstest/logs

Explanation of these reports, and of osstest in general, is at
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master

Test harness code can be found at
http://xenbits.xen.org/gitweb?p=osstest.git;a=summary


Pushing revision :

To xenbits.xen.org:/home/xen/git/osstest/ovmf.git
   067503a8c6..1b461403ee  1b461403ee723dab01d5828714cca0b9396a6b3c -> 
xen-tested-master

Re: [PATCH] travis: Fix build with newer Qemu

[Wei Liu]

On Mon, Sep 14, 2020 at 02:23:55PM +0100, Andrew Cooper wrote:
> Qemu requires a bleeding edge version of Python, not found in the current
> travis environment.  Skip building Qemu in that case.
> 
> Signed-off-by: Andrew Cooper 

Acked-by: Wei Liu 

Re: libxl - b_info.{acpi,apic} behaves differently than b_info.u.hvm.{acpi,apic}

[Marek Marczykowski-Górecki]

On Mon, Sep 14, 2020 at 05:19:56PM +0200, Roger Pau Monné wrote:
> On Sun, Sep 13, 2020 at 01:12:39PM +0200, Marek Marczykowski-Górecki wrote:
> > Ok, The crash reported initially was caused by a different thing: using
> > seabios.bin instead of seabios-256k.bin (should that really cause the
> > crash? shouldn't 128k seabios build work too?). But in any case, I think
> > the b_info.u.hvm.{acpi,apic} is also not in a good shape.
> 
> Does 128K SeaBIOS have Xen support enabled?

This is a very good question. No:
https://src.fedoraproject.org/rpms/seabios/blob/f32/f/config.seabios-128k#_7

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?


signature.asc
Description: PGP signature

Re: [PATCH v2 2/2] EFI: free unused boot mem in at least some cases

[Jan Beulich]

On 14.09.2020 17:16, Roger Pau Monné wrote:
> On Mon, Aug 24, 2020 at 02:08:11PM +0200, Jan Beulich wrote:
>> Address at least the primary reason why 52bba67f8b87 ("efi/boot: Don't
>> free ebmalloc area at all") was put in place: Make xen_in_range() aware
>> of the freed range. This is in particular relevant for EFI-enabled
>> builds not actually running on EFI, as the entire range will be unused
>> in this case.
>>
>> Signed-off-by: Jan Beulich 
> 
> Acked-by: Roger Pau Monné 

Thanks much.

>> @@ -1145,7 +1146,8 @@ void __init noreturn __start_xen(unsigne
>>  
>>  /*
>>   * This needs to remain in sync with xen_in_range() and the
>> - * respective reserve_e820_ram() invocation below.
>> + * respective reserve_e820_ram() invocation below. No need to
>> + * query efi_boot_mem_unused() here, though.
>>   */
>>  mod[mbi->mods_count].mod_start = virt_to_mfn(_stext);
>>  mod[mbi->mods_count].mod_end = __2M_rwdata_end - _stext;
> 
> I find this extremely confusing, we reuse mod_start/mod_end to contain
> a mfn and a size (in bytes) instead of a start and end address (not
> something that should be fixed here, but seeing this I assumed it was
> wrong).

While perhaps somewhat confusing, I still think it was a fair thing
to do in favor of introducing a completely new way of propagating
respective information, and then having the consumer of this data
look at two different places.

>> +bool efi_boot_mem_unused(unsigned long *start, unsigned long *end)
>> +{
>> +*start = (unsigned long)ebmalloc_mem + PAGE_ALIGN(ebmalloc_allocated);
>> +*end = (unsigned long)ebmalloc_mem + sizeof(ebmalloc_mem);
>> +
>> +return *start < *end;
>> +}
>> +
>>  void __init free_ebmalloc_unused_mem(void)
>>  {
>> -#if 0 /* FIXME: Putting a hole in the BSS breaks the IOMMU mappings for 
>> dom0. */
>>  unsigned long start, end;
>>  
>> -start = (unsigned long)ebmalloc_mem + PAGE_ALIGN(ebmalloc_allocated);
>> -end = (unsigned long)ebmalloc_mem + sizeof(ebmalloc_mem);
>> +#ifdef CONFIG_X86
>> +/* FIXME: Putting a hole in .bss would shatter the large page mapping. 
>> */
> 
> Could you make the ebmalloc size (EBMALLOC_SIZE) 2MB (and aligned), so
> that you would only shatter the malloc'ed pages but not the
> surrounding mappings?
> 
> That would be a good compromise IMO.

Yes, that's what I've been considering as a compromise as well. In
fact I was further thinking whether to allocate the space from the
linker script instead of having a global/static object. Maybe by
extending into the .pad section, which is already 2Mb aligned anyway.

Another option is to not further align the whole blob at all and
merely free whatever comes past the next 2Mb boundary (and is not
in use). This would avoid having an up to 2Mb block of unused, not
freed memory ahead of the ebmalloc one.

Jan

Re: [PATCH V1 06/16] xen/ioreq: Make x86's hvm_ioreq_(page/vcpu/server) structs common

[Jan Beulich]

On 10.09.2020 22:22, Oleksandr Tyshchenko wrote:
> --- a/xen/include/xen/ioreq.h
> +++ b/xen/include/xen/ioreq.h
> @@ -23,6 +23,40 @@
>  
>  #include 
>  
> +struct hvm_ioreq_page {
> +gfn_t gfn;
> +struct page_info *page;
> +void *va;
> +};
> +
> +struct hvm_ioreq_vcpu {
> +struct list_head list_entry;
> +struct vcpu  *vcpu;
> +evtchn_port_tioreq_evtchn;
> +bool pending;
> +};
> +
> +#define NR_IO_RANGE_TYPES (XEN_DMOP_IO_RANGE_PCI + 1)
> +#define MAX_NR_IO_RANGES  256
> +
> +struct hvm_ioreq_server {
> +struct domain  *target, *emulator;
> +
> +/* Lock to serialize toolstack modifications */
> +spinlock_t lock;
> +
> +struct hvm_ioreq_page  ioreq;
> +struct list_head   ioreq_vcpu_list;
> +struct hvm_ioreq_page  bufioreq;
> +
> +/* Lock to serialize access to buffered ioreq ring */
> +spinlock_t bufioreq_lock;
> +evtchn_port_t  bufioreq_evtchn;
> +struct rangeset*range[NR_IO_RANGE_TYPES];
> +bool   enabled;
> +uint8_tbufioreq_handling;
> +};

Besides there again being the question of hvm_ prefixes here,
is the bufioreq concept something Arm is meaning to make use
of? If not, this may want to become conditional ...

Jan

Re: [PATCH V1 05/16] xen/ioreq: Make x86's hvm_mmio_first(last)_byte() common

[Jan Beulich]

On 10.09.2020 22:21, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko 
> 
> The IOREQ is a common feature now and these helpers will be used
> on Arm as is. Move them to include/xen/ioreq.h

I think you also want to renamed them to replace the hvm_
prefix by ioreq_.

Jan

Re: [PATCH V1 04/16] xen/ioreq: Provide alias for the handle_mmio()

[Jan Beulich]

On 10.09.2020 22:21, Oleksandr Tyshchenko wrote:
> --- a/xen/common/ioreq.c
> +++ b/xen/common/ioreq.c
> @@ -189,7 +189,7 @@ bool handle_hvm_io_completion(struct vcpu *v)
>  break;
>  
>  case HVMIO_mmio_completion:
> -return handle_mmio();
> +return ioreq_handle_complete_mmio();

Again the question: Any particular reason to have "handle" in here?
With the abstraction simply named ioreq_complete_mmio() feel free
to add
Acked-by: Jan Beulich 

Jan

[PATCH] libxl: User defined max_maptrack_frames in a stub domain

[Dmitry Fedorov]

Hi,

Implementing qrexec+usbip+qemu in Linux-based stub domain leads me to
an issue where a device model stub domain doesn't have maptrack entries.

Would it be possible to apply a user defined max_maptrack_frames value 
to dm_config in the same way as for max_grant_frames?


Signed-off-by: Dmitry Fedorov 
---
 tools/libxl/libxl_dm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index f2dc5696b9..f044f2566c 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -2292,7 +2292,7 @@ void libxl__spawn_stub_dm(libxl__egc *egc, 
libxl__stub_dm_spawn_state *sdss)

 dm_config->b_info.target_memkb = dm_config->b_info.max_memkb;

 dm_config->b_info.max_grant_frames = 
guest_config->b_info.max_grant_frames;

-    dm_config->b_info.max_maptrack_frames = 0;
+    dm_config->b_info.max_maptrack_frames = 
guest_config->b_info.max_maptrack_frames;


 dm_config->b_info.u.pv.features = "";

--
2.26.2

Re: [PATCH 01/20] drm/amdgpu: Introduce GEM object functions

[Thomas Zimmermann]

Hi

Am 13.08.20 um 12:22 schrieb Christian König:
> Am 13.08.20 um 10:36 schrieb Thomas Zimmermann:
>> GEM object functions deprecate several similar callback interfaces in
>> struct drm_driver. This patch replaces the per-driver callbacks with
>> per-instance callbacks in amdgpu. The only exception is gem_prime_mmap,
>> which is non-trivial to convert.
>>
>> Signed-off-by: Thomas Zimmermann 
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c    |  6 --
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 12 
>>   2 files changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> index 81a79760ca61..51525b8774c9 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> @@ -1468,19 +1468,13 @@ static struct drm_driver kms_driver = {
>>   .lastclose = amdgpu_driver_lastclose_kms,
>>   .irq_handler = amdgpu_irq_handler,
>>   .ioctls = amdgpu_ioctls_kms,
>> -    .gem_free_object_unlocked = amdgpu_gem_object_free,
>> -    .gem_open_object = amdgpu_gem_object_open,
>> -    .gem_close_object = amdgpu_gem_object_close,
>>   .dumb_create = amdgpu_mode_dumb_create,
>>   .dumb_map_offset = amdgpu_mode_dumb_mmap,
>>   .fops = &amdgpu_driver_kms_fops,
>>     .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>>   .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
>> -    .gem_prime_export = amdgpu_gem_prime_export,
>>   .gem_prime_import = amdgpu_gem_prime_import,
>> -    .gem_prime_vmap = amdgpu_gem_prime_vmap,
>> -    .gem_prime_vunmap = amdgpu_gem_prime_vunmap,
>>   .gem_prime_mmap = amdgpu_gem_prime_mmap,
>>     .name = DRIVER_NAME,
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> index 43f4966331dd..ca2b79f94e99 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> @@ -36,6 +36,7 @@
>>   #include 
>>   #include 
>>   #include "amdgpu.h"
>> +#include "amdgpu_dma_buf.h"
>>   #include "amdgpu_trace.h"
>>   #include "amdgpu_amdkfd.h"
>>   @@ -510,6 +511,15 @@ bool amdgpu_bo_support_uswc(u64 bo_flags)
>>   #endif
>>   }
>>   +static const struct drm_gem_object_funcs amdgpu_gem_object_funcs = {
>> +    .free = amdgpu_gem_object_free,
>> +    .open = amdgpu_gem_object_open,
>> +    .close = amdgpu_gem_object_close,
>> +    .export = amdgpu_gem_prime_export,
>> +    .vmap = amdgpu_gem_prime_vmap,
>> +    .vunmap = amdgpu_gem_prime_vunmap,
>> +};
>> +
> 
> Wrong file, this belongs into amdgpu_gem.c
> 
>>   static int amdgpu_bo_do_create(struct amdgpu_device *adev,
>>  struct amdgpu_bo_param *bp,
>>  struct amdgpu_bo **bo_ptr)
>> @@ -552,6 +562,8 @@ static int amdgpu_bo_do_create(struct
>> amdgpu_device *adev,
>>   bo = kzalloc(sizeof(struct amdgpu_bo), GFP_KERNEL);
>>   if (bo == NULL)
>>   return -ENOMEM;
>> +
>> +    bo->tbo.base.funcs = &amdgpu_gem_object_funcs;
> 
> And this should probably go into amdgpu_gem_object_create().

I'm trying to understand what amdgpu does.  What about all the places
where amdgpu calls amdgpu_bo_create() internally? Wouldn't these miss
the free callback for the GEM object?

Best regards
Thomas

> 
> Apart from that looks like a good idea to me.
> 
> Christian.
> 
>>   drm_gem_private_object_init(adev->ddev, &bo->tbo.base, size);
>>   INIT_LIST_HEAD(&bo->shadow_list);
>>   bo->vm_bo = NULL;
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer



signature.asc
Description: OpenPGP digital signature

Re: [PATCH V1 03/16] xen/ioreq: Make x86's hvm_ioreq_needs_completion() common

[Jan Beulich]

On 10.09.2020 22:21, Oleksandr Tyshchenko wrote:
> --- a/xen/include/xen/ioreq.h
> +++ b/xen/include/xen/ioreq.h
> @@ -35,6 +35,13 @@ static inline struct hvm_ioreq_server 
> *get_ioreq_server(const struct domain *d,
>  return GET_IOREQ_SERVER(d, id);
>  }
>  
> +static inline bool hvm_ioreq_needs_completion(const ioreq_t *ioreq)
> +{
> +return ioreq->state == STATE_IOREQ_READY &&
> +   !ioreq->data_is_ptr &&
> +   (ioreq->type != IOREQ_TYPE_PIO || ioreq->dir != IOREQ_WRITE);
> +}

While the PIO aspect has been discussed to some length, what about
the data_is_ptr concept? I didn't think there were Arm insns fitting
this? Instead I thought some other Arm-specific adjustments to the
protocol might be needed. At which point the question of course would
be in how far ioreq_t as a whole really fits Arm in its current shape.

Jan

Re: [RFC PATCH] efi: const correct EFI functions

[Jan Beulich]

On 14.09.2020 16:46, Trammell Hudson wrote:
> On Monday, September 14, 2020 10:30 AM, Jan Beulich  wrote:
>> On 14.09.2020 16:25, Trammell Hudson wrote:
>>> By defining IN as const, the EFI handler functions become almost
>>> const-correct and allow most of the rest of the EFI boot code to
>>> use constant strings.
>>
>> How does this work with combined "IN OUT"? I'm afraid there is a
>> reason why things aren't done the way you suggest.
> 
> WTF FFS UEFI. They really do continue to find new ways to disappoint me.
> 
> So I see three options:
> 
> Option 1 is to retroactively modify the C spec to allow us to have
> a "nonconst" that will override any prior "const" modifiers
> (last one wins, like Duck Season/Rabbit Season).

LoL

> Option 2 would be to modify the imported header to change
> the 30 uses of "IN OUT" to "INOUT" and define that to be an
> empty string.

This is something that one might hope even the gnu-efi folks
could be persuaded to do.

> Option 3 would be to write wrappers for the few functions that are
> used in the EFI boot path that cast-away the constness of their
> arguments (while also silently cursing the UEFI forum for not
> writing const-correct code).

This would be kind of a last resort fallback (except for the
cursing, which of course we can do at any time).

Jan

Re: [RFC PATCH] efi: const correct EFI functions

[Trammell Hudson]

On Monday, September 14, 2020 10:30 AM, Jan Beulich  wrote:
> On 14.09.2020 16:25, Trammell Hudson wrote:
> > By defining IN as const, the EFI handler functions become almost
> > const-correct and allow most of the rest of the EFI boot code to
> > use constant strings.
>
> How does this work with combined "IN OUT"? I'm afraid there is a
> reason why things aren't done the way you suggest.

WTF FFS UEFI. They really do continue to find new ways to disappoint me.

So I see three options:

Option 1 is to retroactively modify the C spec to allow us to have
a "nonconst" that will override any prior "const" modifiers
(last one wins, like Duck Season/Rabbit Season).

Option 2 would be to modify the imported header to change
the 30 uses of "IN OUT" to "INOUT" and define that to be an
empty string.

Option 3 would be to write wrappers for the few functions that are
used in the EFI boot path that cast-away the constness of their
arguments (while also silently cursing the UEFI forum for not
writing const-correct code).

--
Trammell

Re: [PATCH] tools: Delete XEN_DOMCTL_disable_migrate

[Jan Beulich]

On 14.09.2020 16:04, Andrew Cooper wrote:
> On 14/09/2020 09:43, Jan Beulich wrote:
>> On 11.09.2020 21:06, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/cpuid.c
>>> +++ b/xen/arch/x86/cpuid.c
>>> @@ -708,7 +708,8 @@ int init_domain_cpuid_policy(struct domain *d)
>>>  if ( !p )
>>>  return -ENOMEM;
>>>  
>>> -if ( d->disable_migrate )
>>> +/* The hardware domain can't migrate.  Give it ITSC if available. */
>>> +if ( is_hardware_domain(d) )
>>>  p->extd.itsc = cpu_has_itsc;
>> ... why not include is_xenstore_domain() here that you remove from
>> ...
>>
>>> @@ -452,9 +451,6 @@ struct domain *domain_create(domid_t domid,
>>>  watchdog_domain_init(d);
>>>  init_status |= INIT_watchdog;
>>>  
>>> -if ( is_xenstore_domain(d) )
>>> -d->disable_migrate = true;
>> ... here? On the tool stack side the change here only deletes code,
>> i.e. I don't see you taking care of the default enabling there
>> either. Am I overlooking any logic that now causes the feature to
>> be requested for the xenstore domain without you needing to add
>> any code?
> 
> The toolstack (legitimately) has knowledge of nomigrate, and will even
> implicitly turn on ITSC as a side effect, but will also honour explicit
> requests to turn it on or off.

Okay, it this is the case then ...

>>> --- a/xen/include/public/domctl.h
>>> +++ b/xen/include/public/domctl.h
>>> @@ -730,11 +730,6 @@ struct xen_domctl_hvmcontext_partial {
>>>  XEN_GUEST_HANDLE_64(uint8) buffer;  /* OUT: buffer to write record 
>>> into */
>>>  };
>>>  
>>> -/* XEN_DOMCTL_disable_migrate */
>>> -struct xen_domctl_disable_migrate {
>>> -uint32_t disable; /* IN: 1: disable migration and restore */
>>> -};
>>> -
>>>  
>>>  /* XEN_DOMCTL_gettscinfo */
>>>  /* XEN_DOMCTL_settscinfo */
>>> @@ -1191,7 +1186,7 @@ struct xen_domctl {
>>>  #define XEN_DOMCTL_gethvmcontext_partial 55
>>>  #define XEN_DOMCTL_vm_event_op   56
>>>  #define XEN_DOMCTL_mem_sharing_op57
>>> -#define XEN_DOMCTL_disable_migrate   58
>>> +/* #define XEN_DOMCTL_disable_migrate58 - Obsolete */
>>>  #define XEN_DOMCTL_gettscinfo59
>>>  #define XEN_DOMCTL_settscinfo60
>>>  #define XEN_DOMCTL_getpageframeinfo3 61
>>> @@ -1242,7 +1237,6 @@ struct xen_domctl {
>>>  struct xen_domctl_ioport_permission ioport_permission;
>>>  struct xen_domctl_hypercall_inithypercall_init;
>>>  struct xen_domctl_settimeoffset settimeoffset;
>>> -struct xen_domctl_disable_migrate   disable_migrate;
>>>  struct xen_domctl_tsc_info  tsc_info;
>>>  struct xen_domctl_hvmcontexthvmcontext;
>>>  struct xen_domctl_hvmcontext_partial hvmcontext_partial;
>> Deletion of sub-ops, just like their modification, requires the
>> interface version to get bumped if it wasn't already during a
>> release cycle. I know you dislike the underlying concept, but as
>> long as the interface version continues to exist (with its
>> present meaning) I'm afraid it needs bumping for any backwards-
>> incompatible change.
> 
> I can fix it on commit.  I don't waste time tracking whether it has had
> its conditional bump.

... with this taken care of
Acked-by: Jan Beulich 

Jan

Re: [PATCH 2/2] libxl: do not automatically force detach of block devices

[Roger Pau Monné]

On Tue, Sep 08, 2020 at 02:19:03PM +, Wei Liu wrote:
> On Thu, Sep 03, 2020 at 11:05:37AM +0100, Paul Durrant wrote:
> > From: Paul Durrant 
> > 
> > The manpage for 'xl' documents that guest co-operation is required for a 
> > (non-
> > forced) block-detach operation and that it may consequently fail. Currently,
> > however, the implementation of generic device removal means that a time-out
> > of a block-detach is being automatically re-tried with the force flag set
> > rather than failing. This patch stops such behaviour.
> > 
> > Signed-off-by: Paul Durrant 
> 
> I'm two-minded here.
> 
> On the one hand, special-casing VBD in libxl to conform to xl's
> behaviour seems wrong to me.
> 
> On the other hand, if we want to treat all devices the same in libxl,
> libxl should drop its force-removal behaviour all together, and the
> retry behaviour would need to be implemented in xl for all devices
> excepts for VBD. This requires a bit of code churn and, more
> importantly, changes how those device removal APIs behave. In the end
> this effort may not worth it.

I would be worried about those changes, as we would likely have to
also change libvirt or any other downstreams?

> If we go with the patch here, we should document this special case on
> libxl level somehow.
> 
> Anthony and Ian, do you have any opinion on this?

Maybe a new function should be introduced instead, that attempts to
remove a device gracefully and fail otherwise?

Then none of the current APIs would change, and xl could use this new
function to handle VBD removal?

Roger.

Re: [RFC PATCH] efi: const correct EFI functions

[Jan Beulich]

On 14.09.2020 16:25, Trammell Hudson wrote:
> By defining IN as const, the EFI handler functions become almost
> const-correct and allow most of the rest of the EFI boot code to
> use constant strings.

How does this work with combined "IN OUT"? I'm afraid there is a
reason why things aren't done the way you suggest.

> --- a/xen/include/efi/efidef.h
> +++ b/xen/include/efi/efidef.h
> @@ -50,7 +50,7 @@ typedef VOID*EFI_EVENT;
>  //
>  
>  #ifndef IN
> -#define IN
> +#define IN const
>  #define OUT
>  #define OPTIONAL
>  #endif

I think the #ifdef here is precisely so you can override the three
identifiers without having to touch this (imported) header. We try
to keep imported headers as little modified as possible.

Jan

[RFC PATCH] efi: const correct EFI functions

[Trammell Hudson]

By defining IN as const, the EFI handler functions become almost
const-correct and allow most of the rest of the EFI boot code to
use constant strings.

There are a few places in the code that casts away the const
that should be reconsidered. The config parser code modifies the
config file in place, which would not work if it were in a read-only
segment.

Signed-off-by: Trammell Hudson 
---
 xen/arch/arm/efi/efi-boot.h |  8 ++--
 xen/arch/x86/efi/efi-boot.h | 26 +--
 xen/common/efi/boot.c   | 92 +++--
 xen/include/efi/efidef.h|  2 +-
 4 files changed, 66 insertions(+), 62 deletions(-)

diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index 6527cb0bdf..13666bc065 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -418,9 +418,9 @@ static void __init efi_arch_memory_setup(void)
 {
 }
 
-static void __init efi_arch_handle_cmdline(CHAR16 *image_name,
-   CHAR16 *cmdline_options,
-   char *cfgfile_options)
+static void __init efi_arch_handle_cmdline(const CHAR16 *image_name,
+   const CHAR16 *cmdline_options,
+   const char *cfgfile_options)
 {
 union string name;
 char *buf;
@@ -482,7 +482,7 @@ static void __init efi_arch_handle_cmdline(CHAR16 
*image_name,
 }
 
 static void __init efi_arch_handle_module(struct file *file, const CHAR16 
*name,
-  char *options)
+  const char *options)
 {
 int node;
 int chosen;
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 7188c9a551..ce5577012b 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -280,10 +280,10 @@ static void __init efi_arch_cfg_file_late(EFI_FILE_HANDLE 
dir_handle, char *sect
 {
 union string name;
 
-name.s = get_value(&cfg, section, "ucode");
-if ( !name.s )
-name.s = get_value(&cfg, "global", "ucode");
-if ( name.s )
+name.cs = get_value(&cfg, section, "ucode");
+if ( !name.cs )
+name.cs = get_value(&cfg, "global", "ucode");
+if ( name.cs )
 {
 microcode_set_module(mbi.mods_count);
 split_string(name.s);
@@ -292,29 +292,29 @@ static void __init efi_arch_cfg_file_late(EFI_FILE_HANDLE 
dir_handle, char *sect
 }
 }
 
-static void __init efi_arch_handle_cmdline(CHAR16 *image_name,
-   CHAR16 *cmdline_options,
-   char *cfgfile_options)
+static void __init efi_arch_handle_cmdline(const CHAR16 *image_name,
+   const CHAR16 *cmdline_options,
+   const char *cfgfile_options)
 {
 union string name;
 
 if ( cmdline_options )
 {
-name.w = cmdline_options;
+name.cw = cmdline_options;
 w2s(&name);
-place_string(&mbi.cmdline, name.s);
+place_string(&mbi.cmdline, name.cs);
 }
 if ( cfgfile_options )
 place_string(&mbi.cmdline, cfgfile_options);
 /* Insert image name last, as it gets prefixed to the other options. */
 if ( image_name )
 {
-name.w = image_name;
+name.cw = image_name;
 w2s(&name);
 }
 else
-name.s = "xen";
-place_string(&mbi.cmdline, name.s);
+name.cs = "xen";
+place_string(&mbi.cmdline, name.cs);
 
 if ( mbi.cmdline )
 mbi.flags |= MBI_CMDLINE;
@@ -636,7 +636,7 @@ static void __init efi_arch_memory_setup(void)
 }
 
 static void __init efi_arch_handle_module(struct file *file, const CHAR16 
*name,
-  char *options)
+  const char *options)
 {
 union string local_name;
 void *ptr;
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 4022a672c9..147af81286 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -98,13 +98,14 @@ union string {
 CHAR16 *w;
 char *s;
 const char *cs;
+const CHAR16 *cw;
 };
 
 struct file {
 UINTN size;
 union {
 EFI_PHYSICAL_ADDRESS addr;
-void *ptr;
+const void *ptr;
 };
 };
 
@@ -113,13 +114,13 @@ static CHAR16 *FormatHex(UINT64 Val, UINTN Width, CHAR16 
*Buffer);
 static void  DisplayUint(UINT64 Val, INTN Width);
 static CHAR16 *wstrcpy(CHAR16 *d, const CHAR16 *s);
 static void PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode);
-static char *get_value(const struct file *cfg, const char *section,
-  const char *item);
+static const char *get_value(const struct file *cfg, const char *section,
+ const char *item);
 static char *split_string(char *s);
-static CHAR16 *s2w(union string *str);
-static char *w2s(const union string *str);
-static 

Re: [PATCH v3 4/4] efi: Do not use command line if secure boot is enabled.

[Roger Pau Monné]

On Mon, Sep 07, 2020 at 03:00:27PM -0400, Trammell Hudson wrote:
> From: Trammell hudson 
> 
> If secure boot is enabled, the Xen command line arguments are ignored.
> If a unified Xen image is used, then the bundled configuration, dom0
> kernel, and initrd are prefered over the ones listed in the config file.
> 
> Unlike the shim based verification, the PE signature on a unified image
> covers the all of the Xen+config+kernel+initrd modules linked into the
> unified image. This also ensures that properly configured platforms
> will measure the entire runtime into the TPM for unsealing secrets or
> remote attestation.
> 
> Signed-off-by: Trammell Hudson 
> ---
>  xen/common/efi/boot.c | 31 ---
>  1 file changed, 28 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index 452b5f4362..5aaebd5f20 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -947,6 +947,26 @@ static void __init setup_efi_pci(void)
>  efi_bs->FreePool(handles);
>  }
>  
> +/*
> + * Logic should remain sync'ed with linux/arch/x86/xen/efi.c
> + * Secure Boot is enabled iff 'SecureBoot' is set and the system is
> + * not in Setup Mode.
> + */
> +static bool __init efi_secure_boot(void)
> +{
> +static const __initconst EFI_GUID global_guid = EFI_GLOBAL_VARIABLE;
> +uint8_t secboot, setupmode;
> +UINTN secboot_size = sizeof(secboot);
> +UINTN setupmode_size = sizeof(setupmode);
> +
> +if ( efi_rs->GetVariable(L"SecureBoot", (EFI_GUID *)&global_guid, NULL, 
> &secboot_size, &secboot) != EFI_SUCCESS )

I'm slightly worried about the dropping of the const here, and the
fact that the variable is placed in initconst section. Isn't it
dangerous that the EFI services will try to write to it?

Line length also.

> +return false;
> +if ( efi_rs->GetVariable(L"SetupMode", (EFI_GUID *)&global_guid, NULL, 
> &setupmode_size, &setupmode) != EFI_SUCCESS )
> +return false;
> +
> +return secboot == 1 && setupmode == 0;

I would print a message if secboot is > 1, since those should be
reserved.

Roger.

Re: [PATCH V1 02/16] xen/ioreq: Make x86's IOREQ feature common

[Jan Beulich]

On 10.09.2020 22:21, Oleksandr Tyshchenko wrote:
> ---
>  MAINTAINERS |8 +-
>  xen/arch/x86/Kconfig|1 +
>  xen/arch/x86/hvm/dm.c   |2 +-
>  xen/arch/x86/hvm/emulate.c  |2 +-
>  xen/arch/x86/hvm/hvm.c  |2 +-
>  xen/arch/x86/hvm/io.c   |2 +-
>  xen/arch/x86/hvm/ioreq.c| 1425 
> +--
>  xen/arch/x86/hvm/stdvga.c   |2 +-
>  xen/arch/x86/hvm/vmx/vvmx.c |3 +-
>  xen/arch/x86/mm.c   |2 +-
>  xen/arch/x86/mm/shadow/common.c |2 +-
>  xen/common/Kconfig  |3 +
>  xen/common/Makefile |1 +
>  xen/common/ioreq.c  | 1410 ++

This suggests it was almost the entire file which got moved. It would
be really nice if you could convince git to show the diff here, rather
than removal and addition of 1400 lines.

Additionally I wonder whether what's left in the original file wouldn't
better become inline functions now. If this was done in the previous
patch, the change would be truly a rename then, I think.

> --- a/xen/include/asm-x86/hvm/ioreq.h
> +++ b/xen/include/asm-x86/hvm/ioreq.h
> @@ -19,41 +19,12 @@
>  #ifndef __ASM_X86_HVM_IOREQ_H__
>  #define __ASM_X86_HVM_IOREQ_H__
>  
> -bool hvm_io_pending(struct vcpu *v);
> -bool handle_hvm_io_completion(struct vcpu *v);
> -bool is_ioreq_server_page(struct domain *d, const struct page_info *page);
> +#include 
> +#include 
> +#include 

Are all three really needed here? Especially the last one strikes me as
odd.

> --- /dev/null
> +++ b/xen/include/xen/ioreq.h
> @@ -0,0 +1,82 @@
> +/*
> + * ioreq.h: Hardware virtual machine assist interface definitions.
> + *
> + * Copyright (c) 2016 Citrix Systems Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along 
> with
> + * this program; If not, see .
> + */
> +
> +#ifndef __IOREQ_H__
> +#define __IOREQ_H__

__XEN_IOREQ_H__ please.

> +#include 
> +
> +#include 

Is this include really needed here (i.e. by the code further down in
the file, and hence by everyone including this header), or rather
just in a few specific .c files?

> +#define GET_IOREQ_SERVER(d, id) \
> +(d)->arch.hvm.ioreq_server.server[id]

arch.hvm.* feels like a layering violation when used in this header.

Jan

Re: [PATCH v3 3/4] efi: Enable booting unified hypervisor/kernel/initrd images

[Roger Pau Monné]

On Mon, Sep 07, 2020 at 03:00:26PM -0400, Trammell Hudson wrote:
> From: Trammell hudson 
> 
> This patch adds support for bundling the xen.efi hypervisor, the xen.cfg
> configuration file, the Linux kernel and initrd, as well as the XSM,
> and architectural specific files into a single "unified" EFI executable.
> This allows an administrator to update the components independently
> without requiring rebuilding xen, as well as to replace the components
> in an existing image.
> 
> The resulting EFI executable can be invoked directly from the UEFI Boot
> Manager, removing the need to use a separate loader like grub as well
> as removing dependencies on local filesystem access.  And since it is
> a single file, it can be signed and validated by UEFI Secure Boot without
> requring the shim protocol.
> 
> It is inspired by systemd-boot's unified kernel technique and borrows the
> function to locate PE sections from systemd's LGPL'ed code.  During EFI
> boot, Xen looks at its own loaded image to locate the PE sections for
> the Xen configuration (`.config`), dom0 kernel (`.kernel`), dom0 initrd
> (`.initrd`), and XSM config (`.xsm`), which are included after building

Could we name this kernel_payload or maybe just payload instead of
initrd?

That's a Linux specific concept.

> xen.efi using objcopy to add named sections for each input file.
> 
> For x86, the CPU ucode can be included in a section named `.ucode`,
> which is loaded in the efi_arch_cfg_file_late() stage of the boot process.
> 
> On ARM systems the Device Tree can be included in a section named
> `.dtb`, which is loaded during the efi_arch_cfg_file_early() stage of
> the boot process.
> 
> Signed-off-by: Trammell hudson 
> ---
>  .gitignore  |   1 +
>  docs/misc/efi.pandoc|  47 
>  xen/arch/arm/efi/efi-boot.h |  22 --
>  xen/arch/x86/efi/Makefile   |   2 +-
>  xen/arch/x86/efi/efi-boot.h |   7 +-
>  xen/common/efi/boot.c   |  72 +-
>  xen/common/efi/efi.h|   3 +
>  xen/common/efi/pe.c | 141 
>  8 files changed, 266 insertions(+), 29 deletions(-)
>  create mode 100644 xen/common/efi/pe.c
> 
> diff --git a/.gitignore b/.gitignore
> index 823f4743dc..1c79b9f90a 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -296,6 +296,7 @@ xen/arch/x86/efi/mkreloc
>  xen/arch/*/xen.lds
>  xen/arch/*/asm-offsets.s
>  xen/arch/*/efi/boot.c
> +xen/arch/*/efi/pe.c

Please sort alphabetically like to rest of the entries in efi/.

>  xen/arch/*/efi/compat.c
>  xen/arch/*/efi/ebmalloc.c
>  xen/arch/*/efi/efi.h
> diff --git a/docs/misc/efi.pandoc b/docs/misc/efi.pandoc
> index 23c1a2732d..c882b1f8f8 100644
> --- a/docs/misc/efi.pandoc
> +++ b/docs/misc/efi.pandoc
> @@ -116,3 +116,50 @@ Filenames must be specified relative to the location of 
> the EFI binary.
>  
>  Extra options to be passed to Xen can also be specified on the command line,
>  following a `--` separator option.
> +
> +## Unified Xen kernel image
> +
> +The "Unified" kernel image can be generated by adding additional
> +sections to the Xen EFI executable with objcopy, similar to how
> +[systemd-boot uses the stub to add them to the Linux 
> kernel](https://wiki.archlinux.org/index.php/systemd-boot#Preparing_a_unified_kernel_image)
> +
> +The sections for the xen configuration file, the dom0 kernel, dom0 initrd,
> +XSM and CPU microcode should be added after the Xen `.pad` section, the
> +ending address of which can be located with:
> +
> +```
> +objdump -h xen.efi \
> + | perl -ane '/\.pad/ && printf "0x%016x\n", hex($F[2]) + hex($F[3])'
> +```
> +
> +All the sections are optional (`.config`, `.kernel`, `.initrd`, `.xsm`,
> +`.ucode` (x86) and `.dtb` (ARM)) and the order does not matter.
> +The addresses do not need to be contiguous, although they should not
> +be overlapping.
> +
> +```
> +objcopy \
> + --add-section .config=xen.cfg \
> + --change-section-vma .config=0x82d04100
> + --add-section .ucode=ucode.bin \
> + --change-section-vma .ucode=0x82d04101 \
> + --add-section .xsm=xsm.cfg \
> + --change-section-vma .xsm=0x82d04108 \
> + --add-section .kernel=vmlinux \
> + --change-section-vma .kernel=0x82d04110 \
> + --add-section .ramdisk=initrd.img \

This seems like a typo, you should use .initrd instead?

> + --change-section-vma .initrd=0x82d04200 \

Why do you need to adjust the VMA, is this not set to a suitable
default value?

How can users find a suitable VMA value?

> + xen.efi \
> + xen.unified.efi
> +```
> +
> +The unified executable can be signed with sbsigntool to make
> +it usable with UEFI secure boot:
> +
> +```
> +sbsign \
> + --key signing.key \
> + --cert cert.pem \
> + --output xen.signed.efi \
> + xen.unified.efi
> +```
> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
> index 6527cb0bdf..154e605fee 100644
> --- a/xen/arch/arm/efi/efi-boot.h
>

Re: [PATCH V1 01/16] x86/ioreq: Prepare IOREQ feature for making it common

[Jan Beulich]

On 10.09.2020 22:21, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko 
> 
> As a lot of x86 code can be re-used on Arm later on, this patch
> prepares IOREQ support before moving to the common code. This way
> we will get almost a verbatim copy for a code movement.
> 
> This support is going to be used on Arm to be able run device
> emulator outside of Xen hypervisor.

This is all fine, but you should then go on and explain what you're
doing, and why (at which point it may become obvious that it would
be more helpful to split this into a couple of steps). In particular
something as suspicious as ...

> Changes RFC -> V1:
>- new patch, was split from:
>  "[RFC PATCH V1 01/12] hvm/ioreq: Make x86's IOREQ feature common"
>- fold the check of p->type into hvm_get_ioreq_server_range_type()
>  and make it return success/failure
>- remove relocate_portio_handler() call from arch_hvm_ioreq_destroy()
>  in arch/x86/hvm/ioreq.c

... this (see below).

> --- a/xen/arch/x86/hvm/ioreq.c
> +++ b/xen/arch/x86/hvm/ioreq.c
> @@ -170,6 +170,29 @@ static bool hvm_wait_for_io(struct hvm_ioreq_vcpu *sv, 
> ioreq_t *p)
>  return true;
>  }
>  
> +bool arch_handle_hvm_io_completion(enum hvm_io_completion io_completion)

Do we need "handle" in here? Without it, I'd also not have to ask about
moving hvm further to the front of the name...

> @@ -836,6 +848,12 @@ int hvm_create_ioreq_server(struct domain *d, int 
> bufioreq_handling,
>  return rc;
>  }
>  
> +/* Called when target domain is paused */
> +int arch_hvm_destroy_ioreq_server(struct hvm_ioreq_server *s)
> +{
> +return p2m_set_ioreq_server(s->target, 0, s);
> +}

Why return "int" when ...

> @@ -855,7 +873,7 @@ int hvm_destroy_ioreq_server(struct domain *d, ioservid_t 
> id)
>  
>  domain_pause(d);
>  
> -p2m_set_ioreq_server(d, 0, s);
> +arch_hvm_destroy_ioreq_server(s);

... the result has been ignored anyway? Or otherwise I guess you'd
want to add error handling here (but then the result of
p2m_set_ioreq_server() should still get ignored, for backwards
compatibility).

> @@ -1215,8 +1233,7 @@ void hvm_destroy_all_ioreq_servers(struct domain *d)
>  struct hvm_ioreq_server *s;
>  unsigned int id;
>  
> -if ( !relocate_portio_handler(d, 0xcf8, 0xcf8, 4) )
> -return;
> +arch_hvm_ioreq_destroy(d);

So the call to relocate_portio_handler() simply goes away. No
replacement, no explanation?

> @@ -1239,19 +1256,15 @@ void hvm_destroy_all_ioreq_servers(struct domain *d)
>  spin_unlock_recursive(&d->arch.hvm.ioreq_server.lock);
>  }
>  
> -struct hvm_ioreq_server *hvm_select_ioreq_server(struct domain *d,
> - ioreq_t *p)
> +int hvm_get_ioreq_server_range_type(struct domain *d,
> +ioreq_t *p,

At least p, but perhaps also d can gain const?

> +uint8_t *type,
> +uint64_t *addr)

By the name the function returns a type for a range (albeit without
it being clear where the two bounds of such a range actually live).
By the implementation is looks more like you mean "range_and_type",
albeit still without there really being a range passed back to the
caller. Therefore I think I need some clarification on what's
intended before even being able to suggest something. From ...

> +struct hvm_ioreq_server *hvm_select_ioreq_server(struct domain *d,
> + ioreq_t *p)
> +{
> +struct hvm_ioreq_server *s;
> +uint8_t type;
> +uint64_t addr;
> +unsigned int id;
> +
> +if ( hvm_get_ioreq_server_range_type(d, p, &type, &addr) )
> +return NULL;

... this use - maybe hvm_ioreq_server_get_type_addr() (albeit I
don't like this name very much)?

> @@ -1351,7 +1378,7 @@ static int hvm_send_buffered_ioreq(struct 
> hvm_ioreq_server *s, ioreq_t *p)
>  pg = iorp->va;
>  
>  if ( !pg )
> -return X86EMUL_UNHANDLEABLE;
> +return IOREQ_IO_UNHANDLED;

At this example - why the IO infix, duplicating the prefix? I'd
suggest to either drop it (if the remaining identifiers are deemed
unambiguous enough) or use e.g. IOREQ_STATUS_*.

> @@ -1515,11 +1542,21 @@ static int hvm_access_cf8(
>  return X86EMUL_UNHANDLEABLE;
>  }
>  
> +void arch_hvm_ioreq_init(struct domain *d)
> +{
> +register_portio_handler(d, 0xcf8, 4, hvm_access_cf8);
> +}
> +
> +void arch_hvm_ioreq_destroy(struct domain *d)
> +{
> +
> +}

Stray blank line?

Jan

[xen-unstable-smoke test] 154332: tolerable all pass - PUSHED

[osstest service owner]

flight 154332 xen-unstable-smoke real [real]
http://logs.test-lab.xenproject.org/osstest/logs/154332/

Failures :-/ but no regressions.

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-libvirt 13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  14 saverestore-support-checkfail   never pass

version targeted for testing:
 xen  d16467b18e0c0a77743c3111bed2a833a77fbfe7
baseline version:
 xen  6c5fb129e88b9c06b5fd62a410163ebb8ef77ee6

Last test of basis   154129  2020-09-11 18:00:26 Z2 days
Testing same since   154332  2020-09-14 11:00:26 Z0 days1 attempts


People who touched revisions under test:
  Andrew Cooper 
  Juergen Gross 
  Marek Marczykowski-Górecki 
  Wei Liu 

jobs:
 build-arm64-xsm  pass
 build-amd64  pass
 build-armhf  pass
 build-amd64-libvirt  pass
 test-armhf-armhf-xl  pass
 test-arm64-arm64-xl-xsm  pass
 test-amd64-amd64-xl-qemuu-debianhvm-amd64pass
 test-amd64-amd64-libvirt pass



sg-report-flight on osstest.test-lab.xenproject.org
logs: /home/logs/logs
images: /home/logs/images

Logs, config files, etc. are available at
http://logs.test-lab.xenproject.org/osstest/logs

Explanation of these reports, and of osstest in general, is at
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master

Test harness code can be found at
http://xenbits.xen.org/gitweb?p=osstest.git;a=summary


Pushing revision :

To xenbits.xen.org:/home/xen/git/xen.git
   6c5fb129e8..d16467b18e  d16467b18e0c0a77743c3111bed2a833a77fbfe7 -> smoke

[linux-linus test] 154324: regressions - FAIL

[osstest service owner]

flight 154324 linux-linus real [real]
http://logs.test-lab.xenproject.org/osstest/logs/154324/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-i386-xl-qemuu-ws16-amd64  7 xen-boot  fail REGR. vs. 152332
 test-amd64-i386-qemut-rhel6hvm-intel  7 xen-boot fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-dmrestrict-amd64-dmrestrict 7 xen-boot fail REGR. vs. 
152332
 test-amd64-i386-qemuu-rhel6hvm-intel  7 xen-boot fail REGR. vs. 152332
 test-amd64-i386-xl-xsm7 xen-boot fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-debianhvm-amd64-shadow 7 xen-boot fail REGR. vs. 
152332
 test-amd64-i386-xl-qemut-debianhvm-amd64  7 xen-boot fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-debianhvm-i386-xsm  7 xen-boot  fail REGR. vs. 152332
 test-amd64-i386-examine   8 reboot   fail REGR. vs. 152332
 test-amd64-i386-pair 10 xen-boot/src_hostfail REGR. vs. 152332
 test-amd64-i386-pair 11 xen-boot/dst_hostfail REGR. vs. 152332
 test-amd64-i386-xl7 xen-boot fail REGR. vs. 152332
 test-amd64-i386-libvirt   7 xen-boot fail REGR. vs. 152332
 test-amd64-i386-qemut-rhel6hvm-amd  7 xen-boot   fail REGR. vs. 152332
 test-amd64-i386-xl-qemut-ws16-amd64  7 xen-boot  fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-debianhvm-amd64  7 xen-boot fail REGR. vs. 152332
 test-amd64-i386-libvirt-xsm   7 xen-boot fail REGR. vs. 152332
 test-amd64-coresched-i386-xl  7 xen-boot fail REGR. vs. 152332
 test-amd64-i386-qemuu-rhel6hvm-amd  7 xen-boot   fail REGR. vs. 152332
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 7 xen-boot fail REGR. vs. 
152332
 test-amd64-i386-xl-raw7 xen-boot fail REGR. vs. 152332
 test-amd64-i386-freebsd10-amd64  7 xen-boot  fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-win7-amd64  7 xen-boot  fail REGR. vs. 152332
 test-amd64-i386-xl-pvshim 7 xen-boot fail REGR. vs. 152332
 test-amd64-i386-xl-qemut-debianhvm-i386-xsm  7 xen-boot  fail REGR. vs. 152332
 test-amd64-i386-xl-shadow 7 xen-boot fail REGR. vs. 152332
 test-amd64-i386-freebsd10-i386  7 xen-boot   fail REGR. vs. 152332
 test-amd64-i386-xl-qemut-win7-amd64  7 xen-boot  fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-ovmf-amd64  7 xen-boot  fail REGR. vs. 152332
 test-amd64-i386-libvirt-pair 10 xen-boot/src_hostfail REGR. vs. 152332
 test-amd64-i386-libvirt-pair 11 xen-boot/dst_hostfail REGR. vs. 152332
 test-amd64-i386-xl-qemut-stubdom-debianhvm-amd64-xsm 7 xen-boot fail REGR. vs. 
152332
 build-arm64-pvops 6 kernel-build fail REGR. vs. 152332

Tests which did not succeed, but are not blocking:
 test-arm64-arm64-examine  1 build-check(1)   blocked  n/a
 test-arm64-arm64-libvirt-xsm  1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl   1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-credit1   1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-credit2   1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-seattle   1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-thunderx  1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-xsm   1 build-check(1)   blocked  n/a
 test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stopfail like 152332
 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 152332
 test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stopfail like 152332
 test-armhf-armhf-libvirt 14 saverestore-support-checkfail  like 152332
 test-armhf-armhf-xl-rtds 16 guest-start/debian.repeatfail  like 152332
 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail  like 152332
 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stopfail like 152332
 test-amd64-amd64-libvirt 13 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check 
fail never pass
 test-armhf-armhf-xl-arndale  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  14 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-cubietruck 13 migrate-support-checkfail never pass
 test-armhf-armhf-xl-cubietruc

Re: [PATCH 0/8] Finding a home for the Code of Conduct

[George Dunlap]

> On Sep 11, 2020, at 5:50 PM, Stefano Stabellini  
> wrote:
> 
> On Fri, 11 Sep 2020, George Dunlap wrote:
>> The Code of Conduct has been approved [1]; now we need to find it a
>> home.  Since we've started using sphinx for the hypervisor documents,
>> I propose doing the same for the project-wide governance documents, starting
>> with the Code of Conduct.
>> 
>> This series takes Lars' code of conduct tree, written as individual MD
>> files, and puts them into the sphinx documentation system.  After this
>> series, if you run "make html" in the top-level directory, you'll get
>> the generated sphinx documentation in the build/ directory.
>> 
>> The finalized Code of Conduct documentation can be found at:
>> 
>> https://xenbits.xen.org/git-http/people/gdunlap/governance.git
>> 
>> This series can be found on the branch out/move-to-sphinx/v1
>> 
>> And a rendered version of the governance can be found here:
>> 
>> https://xenbits.xenproject.org/people/gdunlap/governance/
>> 
>> If there are no objections to this setup, I propose the following URL
>> as a long-term home:
>> 
>> https://xenbits.xenproject.org/governance
>> 
>> And also moving both the main governance doc [2] and the security
>> policy [3] into that system, to make it easier to update.
>> 
>> Thoughts?
> 
> Sounds great.
> 
> I'd suggest to also move the governance.git repository to a more
> "official" location on xenbits.

Yes of course. :-)  The goal was first to get a draft up as quickly as possible 
for people to look at.

 -George

Re: [PATCH v3] efi: Always map EfiRuntimeServicesCode and EfiRuntimeServicesData

[Jan Beulich]

On 11.09.2020 16:43, Sergey Temerkhanov wrote:
> @@ -1510,6 +1517,24 @@ void __init efi_init_memory(void)
> desc->PhysicalStart, desc->PhysicalStart + len - 1,
> desc->Type, desc->Attribute);
>  
> +/*
> + * EfiRuntimeServicesCode and EfiRuntimeServicesData
> + * memory ranges are adjusted here. Any changes
> + * or adjustments must be kept in sync with efi_exit_boot()
> + */
> +if ( efi_enabled(EFI_RS) &&
> + (!(desc->Attribute & EFI_MEMORY_RUNTIME) &&
> +   (desc->Attribute & EFI_MEMORY_CACHEABILITY_MASK) &&
> +   (desc->Type == EfiRuntimeServicesCode ||
> +desc->Type == EfiRuntimeServicesData)) )
> +{
> +printk(XENLOG_WARNING
> +   "Setting EFI_RUNTIME memory attribute for area %013"
> +   PRIx64 "-%013" PRIx64 "\n",
> +   desc->PhysicalStart, desc->PhysicalStart + len - 1);
> +desc->Attribute |= EFI_MEMORY_RUNTIME;
> +}

So you've moved from always checking for EFI_MEMORY_WP to not
checking it at all. Neither is the way to go imo. Similarly, ...

> --- a/xen/include/efi/efidef.h
> +++ b/xen/include/efi/efidef.h
> @@ -158,6 +158,12 @@ typedef enum {
>  #define EFI_MEMORY_UCE  0x0010  
>  #define EFI_MEMORY_WP   0x1000
>  
> +#define EFI_MEMORY_CACHEABILITY_MASK  ( EFI_MEMORY_UC | \
> +EFI_MEMORY_WC | \
> +EFI_MEMORY_WT | \
> +EFI_MEMORY_WB | \
> +EFI_MEMORY_UCE )

... this now doesn't really cover what its name suggests. As
indicated before, without such a (questionable) #define having
appeared in the gnu-efi tree, I don't think we want it, at the
very least not in this imported header. But given that it
doesn't express what you want anyway, I can only repeat my
suggestion to drop this #define altogether.

In order to save further rounds, I would offer to finish this
patch to a shape that I'd feel comfortable with - if that's
okay with you.

Jan

[PATCH 2/2] tools/Makefile: Drop the use of $(file ...)

[Andrew Cooper]

It is only available in make 4.0 and later, and not for example in CentOS 7.

Rewrite the logic to use echo and shell redirection, using a single capture
group to avoid having 12 different processes in quick succession each
appending one line to the file.

Fixes: 52dbd6f07cea7a ("tools: generate pkg-config files from make variables")
Signed-off-by: Andrew Cooper 
---
CC: Juergen Gross 
CC: Ian Jackson 
CC: Wei Liu 
---
 tools/Rules.mk | 52 
 1 file changed, 28 insertions(+), 24 deletions(-)

diff --git a/tools/Rules.mk b/tools/Rules.mk
index 4fd91fa444..a71abb2e4f 100644
--- a/tools/Rules.mk
+++ b/tools/Rules.mk
@@ -186,29 +186,33 @@ $(PKG_CONFIG_DIR):
mkdir -p $(PKG_CONFIG_DIR)
 
 $(PKG_CONFIG_DIR)/%.pc: Makefile $(XEN_ROOT)/tools/Rules.mk $(PKG_CONFIG_DIR)
-   $(file >$@,prefix=$(PKG_CONFIG_PREFIX))
-   $(file >>$@,includedir=$(PKG_CONFIG_INCDIR))
-   $(file >>$@,libdir=$(PKG_CONFIG_LIBDIR))
-   $(foreach var,$(PKG_CONFIG_VARS),$(file >>$@,$(var)))
-   $(file >>$@,)
-   $(file >>$@,Name: $(PKG_CONFIG_NAME))
-   $(file >>$@,Description: $(PKG_CONFIG_DESC))
-   $(file >>$@,Version: $(PKG_CONFIG_VERSION))
-   $(file >>$@,Cflags: -I$${includedir} $(CFLAGS_xeninclude))
-   $(file >>$@,Libs: -L$${libdir} $(PKG_CONFIG_USELIBS) 
-l$(PKG_CONFIG_LIB))
-   $(file >>$@,Libs.private: $(PKG_CONFIG_LIBSPRIV))
-   $(file >>$@,Requires.private: $(PKG_CONFIG_REQPRIV))
+   { \
+   echo "prefix=$(PKG_CONFIG_PREFIX)"; \
+   echo "includedir=$(PKG_CONFIG_INCDIR)"; \
+   echo "libdir=$(PKG_CONFIG_LIBDIR)"; \
+   $(foreach var,$(PKG_CONFIG_VARS),echo $(var);) \
+   echo ""; \
+   echo "Name: $(PKG_CONFIG_NAME)"; \
+   echo "Description: $(PKG_CONFIG_DESC)"; \
+   echo "Version: $(PKG_CONFIG_VERSION)"; \
+   echo "Cflags: -I\$${includedir} $(CFLAGS_xeninclude)"; \
+   echo "Libs: -L\$${libdir} $(PKG_CONFIG_USELIBS) -l$(PKG_CONFIG_LIB)"; \
+   echo "Libs.private: $(PKG_CONFIG_LIBSPRIV)"; \
+   echo "Requires.private: $(PKG_CONFIG_REQPRIV)"; \
+   } > $@
 
 %.pc: Makefile $(XEN_ROOT)/tools/Rules.mk
-   $(file >$@,prefix=$(PKG_CONFIG_PREFIX))
-   $(file >>$@,includedir=$(PKG_CONFIG_INCDIR))
-   $(file >>$@,libdir=$(PKG_CONFIG_LIBDIR))
-   $(foreach var,$(PKG_CONFIG_VARS),$(file >>$@,$(var)))
-   $(file >>$@,)
-   $(file >>$@,Name: $(PKG_CONFIG_NAME))
-   $(file >>$@,Description: $(PKG_CONFIG_DESC))
-   $(file >>$@,Version: $(PKG_CONFIG_VERSION))
-   $(file >>$@,Cflags: -I$${includedir})
-   $(file >>$@,Libs: -L$${libdir} -l$(PKG_CONFIG_LIB))
-   $(file >>$@,Libs.private: $(PKG_CONFIG_LIBSPRIV))
-   $(file >>$@,Requires.private: $(PKG_CONFIG_REQPRIV))
+   { \
+   echo "prefix=$(PKG_CONFIG_PREFIX)"; \
+   echo "includedir=$(PKG_CONFIG_INCDIR)"; \
+   echo "libdir=$(PKG_CONFIG_LIBDIR)"; \
+   $(foreach var,$(PKG_CONFIG_VARS),echo $(var);) \
+   echo ""; \
+   echo "Name: $(PKG_CONFIG_NAME)"; \
+   echo "Description: $(PKG_CONFIG_DESC)"; \
+   echo "Version: $(PKG_CONFIG_VERSION)"; \
+   echo "Cflags: -I\$${includedir}"; \
+   echo "Libs: -L\$${libdir} -l$(PKG_CONFIG_LIB)"; \
+   echo "Libs.private: $(PKG_CONFIG_LIBSPRIV)"; \
+   echo "Requires.private: $(PKG_CONFIG_REQPRIV)"; \
+   } > $@
-- 
2.11.0

[PATCH 0/2] Build fixes

[Andrew Cooper]

As demonstrated by Gitlab CI.

Still pending is the xenstat fix, and a newly discovered randconfig issue.

Andrew Cooper (2):
  tools/libs/vchan: Don't run the headers check
  tools/Makefile: Drop the use of $(file ...)

 tools/Rules.mk| 52 +--
 tools/libs/vchan/Makefile |  2 ++
 2 files changed, 30 insertions(+), 24 deletions(-)

-- 
2.11.0

[PATCH 1/2] tools/libs/vchan: Don't run the headers check

[Andrew Cooper]

There was never a headers check previously, and CentOS 6 can't cope with the
anonymous union in struct libxenvchan.

  cc1: warnings being treated as errors
  ... tools/include/libxenvchan.h:75: error: declaration does not declare 
anything
  make[6]: *** [headers.chk] Error 1

Fixes: 8ab2429f12 ("tools: split libxenvchan into new tools/libs/vchan 
directory")
Signed-off-by: Andrew Cooper 
---
CC: Juergen Gross 
CC: Ian Jackson 
CC: Wei Liu 
---
 tools/libs/vchan/Makefile | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/libs/vchan/Makefile b/tools/libs/vchan/Makefile
index 87ff608f45..5e18d5b196 100644
--- a/tools/libs/vchan/Makefile
+++ b/tools/libs/vchan/Makefile
@@ -8,6 +8,8 @@ LIBHEADER := libxenvchan.h
 SRCS-y += init.c
 SRCS-y += io.c
 
+NO_HEADERS_CHK := y
+
 include $(XEN_ROOT)/tools/libs/libs.mk
 
 $(PKG_CONFIG_LOCAL): PKG_CONFIG_INCDIR = $(XEN_libxenvchan)/include
-- 
2.11.0

Re: [PATCH v8 4/8] iommu: make map and unmap take a page count, similar to flush

[Jan Beulich]

On 11.09.2020 10:20, Paul Durrant wrote:
> From: Paul Durrant 
> 
> At the moment iommu_map() and iommu_unmap() take a page order rather than a
> count, whereas iommu_iotlb_flush() takes a page count rather than an order.
> This patch makes them consistent with each other, opting for a page count 
> since
> CPU page orders are not necessarily the same as those of an IOMMU.
> 
> NOTE: The 'page_count' parameter is also made an unsigned long in all the
>   aforementioned functions.
> 
> Signed-off-by: Paul Durrant 

Reviewed-by: Jan Beulich 

Re: [PATCH v3 2/4] efi/boot.c: add file.need_to_free and split display_file_info()

[Roger Pau Monné]

Thanks! Being picky you likely wan to split this into two separate
commits: one for adding need_to_free and the other for
display_file_info.  There's no relation between the two that would
require them to be on the same commit.

On Mon, Sep 07, 2020 at 03:00:25PM -0400, Trammell Hudson wrote:
> From: Trammell hudson 
> 
> Signed-off-by: Trammell hudson 
> ---
>  xen/common/efi/boot.c | 36 ++--
>  1 file changed, 22 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index 4022a672c9..f5bdc4b1df 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -102,6 +102,7 @@ union string {
>  
>  struct file {
>  UINTN size;
> +bool need_to_free;
>  union {
>  EFI_PHYSICAL_ADDRESS addr;
>  void *ptr;
> @@ -279,13 +280,13 @@ void __init noreturn blexit(const CHAR16 *str)
>  if ( !efi_bs )
>  efi_arch_halt();
>  
> -if ( cfg.addr )
> +if ( cfg.addr && cfg.need_to_free )
>  efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
> -if ( kernel.addr )
> +if ( kernel.addr && kernel.need_to_free )
>  efi_bs->FreePages(kernel.addr, PFN_UP(kernel.size));
> -if ( ramdisk.addr )
> +if ( ramdisk.addr && ramdisk.need_to_free )
>  efi_bs->FreePages(ramdisk.addr, PFN_UP(ramdisk.size));
> -if ( xsm.addr )
> +if ( xsm.addr && xsm.need_to_free )
>  efi_bs->FreePages(xsm.addr, PFN_UP(xsm.size));
>  
>  efi_arch_blexit();
> @@ -538,6 +539,21 @@ static char * __init split_string(char *s)
>  return NULL;
>  }
>  
> +static void __init display_file_info(CHAR16 *name, struct file *file, char 
> *options)

I think name at least could be constified?

Also efi_arch_handle_module seem to do more than just printing file
info, hence I would likely rename this to handle_file_info to be more
representative of what it does.

Roger.

[PATCH 1/3] x86/shim: fix build with PV_SHIM_EXCLUSIVE and SHADOW_PAGING

[Jan Beulich]

While there's little point in enabling both, the combination ought to at
least build correctly. Drop the direct PV_SHIM_EXCLUSIVE conditionals
and instead zap PG_log_dirty to zero under the right conditions, and key
other #ifdef-s off of that.

While there also expand on ded576ce07e9 ("x86/shadow: dirty VRAM
tracking is needed for HVM only"): There was yet another is_hvm_domain()
missing, and code touching the struct fields needs to be guarded by
suitable #ifdef-s as well. While there also guard shadow-mode-only
fields accordingly.

Fixes: 8b5b49ceb3d9 ("x86: don't include domctl and alike in shim-exclusive 
builds")
Reported-by: Andrew Cooper 
Signed-off-by: Jan Beulich 

--- a/xen/arch/x86/mm/paging.c
+++ b/xen/arch/x86/mm/paging.c
@@ -47,7 +47,7 @@
 /* Per-CPU variable for enforcing the lock ordering */
 DEFINE_PER_CPU(int, mm_lock_level);
 
-#ifndef CONFIG_PV_SHIM_EXCLUSIVE
+#if PG_log_dirty
 
 //
 /*  LOG DIRTY SUPPORT   */
@@ -630,7 +630,7 @@ void paging_log_dirty_init(struct domain
 d->arch.paging.log_dirty.ops = ops;
 }
 
-#endif /* CONFIG_PV_SHIM_EXCLUSIVE */
+#endif /* PG_log_dirty */
 
 //
 /*   CODE FOR PAGING SUPPORT*/
@@ -671,7 +671,7 @@ void paging_vcpu_init(struct vcpu *v)
 shadow_vcpu_init(v);
 }
 
-#ifndef CONFIG_PV_SHIM_EXCLUSIVE
+#if PG_log_dirty
 int paging_domctl(struct domain *d, struct xen_domctl_shadow_op *sc,
   XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl,
   bool_t resuming)
@@ -792,7 +792,7 @@ long paging_domctl_continuation(XEN_GUES
 
 return ret;
 }
-#endif /* CONFIG_PV_SHIM_EXCLUSIVE */
+#endif /* PG_log_dirty */
 
 /* Call when destroying a domain */
 int paging_teardown(struct domain *d)
@@ -808,7 +808,7 @@ int paging_teardown(struct domain *d)
 if ( preempted )
 return -ERESTART;
 
-#ifndef CONFIG_PV_SHIM_EXCLUSIVE
+#if PG_log_dirty
 /* clean up log dirty resources. */
 rc = paging_free_log_dirty_bitmap(d, 0);
 if ( rc == -ERESTART )
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -2869,12 +2869,14 @@ void shadow_teardown(struct domain *d, b
  * calls now that we've torn down the bitmap */
 d->arch.paging.mode &= ~PG_log_dirty;
 
-if ( d->arch.hvm.dirty_vram )
+#ifdef CONFIG_HVM
+if ( is_hvm_domain(d) && d->arch.hvm.dirty_vram )
 {
 xfree(d->arch.hvm.dirty_vram->sl1ma);
 xfree(d->arch.hvm.dirty_vram->dirty_bitmap);
 XFREE(d->arch.hvm.dirty_vram);
 }
+#endif
 
 out:
 paging_unlock(d);
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -618,6 +618,7 @@ _sh_propagate(struct vcpu *v,
 }
 }
 
+#ifdef CONFIG_HVM
 if ( unlikely(level == 1) && is_hvm_domain(d) )
 {
 struct sh_dirty_vram *dirty_vram = d->arch.hvm.dirty_vram;
@@ -632,6 +633,7 @@ _sh_propagate(struct vcpu *v,
 sflags &= ~_PAGE_RW;
 }
 }
+#endif
 
 /* Read-only memory */
 if ( p2m_is_readonly(p2mt) )
@@ -1050,6 +1052,7 @@ static inline void shadow_vram_get_l1e(s
mfn_t sl1mfn,
struct domain *d)
 {
+#ifdef CONFIG_HVM
 mfn_t mfn = shadow_l1e_get_mfn(new_sl1e);
 int flags = shadow_l1e_get_flags(new_sl1e);
 unsigned long gfn;
@@ -1074,6 +1077,7 @@ static inline void shadow_vram_get_l1e(s
 dirty_vram->sl1ma[i] = mfn_to_maddr(sl1mfn)
 | ((unsigned long)sl1e & ~PAGE_MASK);
 }
+#endif
 }
 
 static inline void shadow_vram_put_l1e(shadow_l1e_t old_sl1e,
@@ -1081,6 +1085,7 @@ static inline void shadow_vram_put_l1e(s
mfn_t sl1mfn,
struct domain *d)
 {
+#ifdef CONFIG_HVM
 mfn_t mfn = shadow_l1e_get_mfn(old_sl1e);
 int flags = shadow_l1e_get_flags(old_sl1e);
 unsigned long gfn;
@@ -1140,6 +1145,7 @@ static inline void shadow_vram_put_l1e(s
 dirty_vram->last_dirty = NOW();
 }
 }
+#endif
 }
 
 static int shadow_set_l1e(struct domain *d,
--- a/xen/include/asm-x86/paging.h
+++ b/xen/include/asm-x86/paging.h
@@ -67,8 +67,12 @@
 #define PG_translate   0
 #define PG_external0
 #endif
+#if defined(CONFIG_HVM) || !defined(CONFIG_PV_SHIM_EXCLUSIVE)
 /* Enable log dirty mode */
 #define PG_log_dirty   (XEN_DOMCTL_SHADOW_ENABLE_LOG_DIRTY << PG_mode_shift)
+#else
+#define PG_log_dirty   0
+#endif
 
 /* All paging modes. */
 #define PG_MASK (PG_refcounts | PG_log_dirty | PG_translate | PG_external)
@@ -154,7 +158,7 @@ struct paging_mode {
 /*
  * Log dirty code */
 
-#ifndef CONFIG_PV_SHIM_EXCLUSIVE
+#if PG_log_dirty
 
 /* get the dirty bitmap for a specific range of pfns */
 void paging_log_dirty_range(struct domain *d,
@@ -195,23 +199,28 @@ int paging

[PATCH 3/3] x86/shim: don't permit HVM and PV_SHIM_EXCLUSIVE at the same time

[Jan Beulich]

This combination doesn't really make sense (and there likely are more).
The alternative here would be some presumably intrusive #ifdef-ary to
get this combination to actually build again.

Signed-off-by: Jan Beulich 

--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -23,7 +23,7 @@ config X86
select HAS_PDX
select HAS_SCHED_GRANULARITY
select HAS_UBSAN
-   select HAS_VPCI if !PV_SHIM_EXCLUSIVE && HVM
+   select HAS_VPCI if HVM
select NEEDS_LIBELF
select NUMA
 
@@ -90,8 +90,8 @@ config PV_LINEAR_PT
  If unsure, say Y.
 
 config HVM
-   def_bool !PV_SHIM_EXCLUSIVE
-   prompt "HVM support"
+   bool "HVM support"
+   depends on !PV_SHIM_EXCLUSIVE
---help---
  Interfaces to support HVM domains.  HVM domains require hardware
  virtualisation extensions (e.g. Intel VT-x, AMD SVM), but can boot

[PATCH 2/3] x86/shim: adjust Kconfig defaults

[Jan Beulich]

Just like HVM, defaulting SHADOW_PAGING and TBOOT to Yes in shim-
exclusive mode makes no sense, as the respective code is dead there.

Also adjust the shim default config file: It needs to specifiy values
only for settings where a non-default value is wanted.

Signed-off-by: Jan Beulich 

--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -116,9 +116,9 @@ config XEN_SHSTK
  compatiblity can be provided via the PV Shim mechanism.
 
 config SHADOW_PAGING
-bool "Shadow Paging"
-default y
----help---
+   bool "Shadow Paging"
+   default y if !PV_SHIM_EXCLUSIVE
+   ---help---
 
   Shadow paging is a software alternative to hardware paging support
   (Intel EPT, AMD NPT).
@@ -165,8 +165,8 @@ config HVM_FEP
  If unsure, say N.
 
 config TBOOT
-   def_bool y
-   prompt "Xen tboot support" if EXPERT
+   bool "Xen tboot support" if EXPERT
+   default y if !PV_SHIM_EXCLUSIVE
select CRYPTO
---help---
  Allows support for Trusted Boot using the Intel(R) Trusted Execution
--- a/xen/arch/x86/configs/pvshim_defconfig
+++ b/xen/arch/x86/configs/pvshim_defconfig
@@ -8,12 +8,9 @@ CONFIG_NR_CPUS=32
 CONFIG_EXPERT=y
 CONFIG_SCHED_NULL=y
 # Disable features not used by the PV shim
-# CONFIG_HVM is not set
 # CONFIG_XEN_SHSTK is not set
 # CONFIG_HYPFS is not set
-# CONFIG_SHADOW_PAGING is not set
 # CONFIG_BIGMEM is not set
-# CONFIG_TBOOT is not set
 # CONFIG_KEXEC is not set
 # CONFIG_XENOPROF is not set
 # CONFIG_XSM is not set

[PATCH 0/3] x86: shim building adjustments

[Jan Beulich]

The first change is simply addressing a build issue noticed by
Andrew. The build breakage goes beyond this specific combination
though - PV_SHIM_EXCLUSIVE plus HVM is similarly an issue. This
is what the last patch tries to take care of, in a shape already
on irc noticed to be controversial. I'm submitting the change
nevertheless because for the moment there looks to be a majority
in favor of going this route. One argument not voiced there yet:
What good does it do to allow a user to enable HVM when then on
the resulting hypervisor they still can't run HVM guests (for
the hypervisor still being a dedicated PV shim one). On top of
this, the alternative approach is likely going to get ugly.

1: fix build with PV_SHIM_EXCLUSIVE and SHADOW_PAGING
2: adjust Kconfig defaults
3: don't permit HVM and PV_SHIM_EXCLUSIVE at the same time

Jan

RE: [PATCH v8 5/8] remove remaining uses of iommu_legacy_map/unmap

[Paul Durrant]

> -Original Message-
> From: Julien Grall 
> Sent: 14 September 2020 11:47
> To: Paul Durrant ; xen-devel@lists.xenproject.org
> Cc: Paul Durrant ; Jan Beulich ; 
> Andrew Cooper
> ; Wei Liu ; Roger Pau Monné 
> ; George
> Dunlap ; Ian Jackson ; 
> Stefano Stabellini
> ; Jun Nakajima ; Kevin Tian 
> 
> Subject: Re: [PATCH v8 5/8] remove remaining uses of iommu_legacy_map/unmap
> 
> Hi Paul,
> 
> I am sorry for jumping very late in the discussion.
> 
> On 11/09/2020 09:20, Paul Durrant wrote:
> > From: Paul Durrant 
> >
> > The 'legacy' functions do implicit flushing so amend the callers to do the
> > appropriate flushing.
> >
> > Unfortunately, because of the structure of the P2M code, we cannot remove
> > the per-CPU 'iommu_dont_flush_iotlb' global and the optimization it
> > facilitates. It is now checked directly iommu_iotlb_flush(). This is safe
> > because callers of iommu_iotlb_flush() on another CPU will not be affected,
> > and hence a flush will be done. Also, 'iommu_dont_flush_iotlb' is now 
> > declared
> > as bool (rather than bool_t) and setting/clearing it are no longer 
> > pointlessly
> > gated on is_iommu_enabled() returning true. (Arguably it is also pointless 
> > to
> > gate the call to iommu_iotlb_flush() on that condition - since it is a no-op
> > in that case - but the if clause allows the scope of a stack variable to be
> > restricted).
> 
> Unfortunately, this change will re-open a potential security hole closed
> by commit 671878779741:
> 
>  xen/arm: p2m: Free the p2m entry after flushing the IOMMU TLBs
> 
>  When freeing a p2m entry, all the sub-tree behind it will also be
> freed.
>  This may include intermediate page-tables or any l3 entry requiring to
>  drop a reference (e.g for foreign pages). As soon as pages are freed,
>  they may be re-used by Xen or another domain. Therefore it is necessary
>  to flush *all* the TLBs beforehand.
> 
>  While CPU TLBs will be flushed before freeing the pages, this is not
>  the case for IOMMU TLBs. This can be solved by moving the IOMMU TLBs
>  flush earlier in the code.
> 
>  This wasn't considered as a security issue as device passthrough on Arm
>  is not security supported.
> 
>  Signed-off-by: Julien Grall 
>  Tested-by: Oleksandr Tyshchenko 
>  Reviewed-by: Stefano Stabellini 
>  Release-acked-by: Juergen Gross 
> 
> One possibility would be to treat iommu_dont_flush_iotlb as x86 only.
> 

Yes, it could be checked in the calling (and hence arch specific) code to deal 
with this.

  Paul

> Cheers,
> 
> --
> Julien Grall

Re: [PATCH 2/2] libxl: do not automatically force detach of block devices

[Wei Liu]

On Mon, Sep 14, 2020 at 12:41:09PM +0200, Roger Pau Monné wrote:
> On Tue, Sep 08, 2020 at 02:19:03PM +, Wei Liu wrote:
> > On Thu, Sep 03, 2020 at 11:05:37AM +0100, Paul Durrant wrote:
> > > From: Paul Durrant 
> > > 
> > > The manpage for 'xl' documents that guest co-operation is required for a 
> > > (non-
> > > forced) block-detach operation and that it may consequently fail. 
> > > Currently,
> > > however, the implementation of generic device removal means that a 
> > > time-out
> > > of a block-detach is being automatically re-tried with the force flag set
> > > rather than failing. This patch stops such behaviour.
> > > 
> > > Signed-off-by: Paul Durrant 
> > 
> > I'm two-minded here.
> > 
> > On the one hand, special-casing VBD in libxl to conform to xl's
> > behaviour seems wrong to me.
> > 
> > On the other hand, if we want to treat all devices the same in libxl,
> > libxl should drop its force-removal behaviour all together, and the
> > retry behaviour would need to be implemented in xl for all devices
> > excepts for VBD. This requires a bit of code churn and, more
> > importantly, changes how those device removal APIs behave. In the end
> > this effort may not worth it.
> 
> I would be worried about those changes, as we would likely have to
> also change libvirt or any other downstreams?
> 
> > If we go with the patch here, we should document this special case on
> > libxl level somehow.
> > 
> > Anthony and Ian, do you have any opinion on this?
> 
> Maybe a new function should be introduced instead, that attempts to
> remove a device gracefully and fail otherwise?
> 
> Then none of the current APIs would change, and xl could use this new
> function to handle VBD removal?

This sounds fine to me.

Wei.

> 
> Roger.

Re: [PATCH v3 4/4] efi: Do not use command line if secure boot is enabled.

[Jan Beulich]

On 14.09.2020 13:36, Trammell Hudson wrote:
> On Monday, September 14, 2020 6:24 AM, Roger Pau Monné  
> wrote:
>> On Mon, Sep 07, 2020 at 03:00:27PM -0400, Trammell Hudson wrote:
>> [...]
>>> -   static const __initconst EFI_GUID global_guid = EFI_GLOBAL_VARIABLE;
>>> -   uint8_t secboot, setupmode;
>>> -   UINTN secboot_size = sizeof(secboot);
>>> -   UINTN setupmode_size = sizeof(setupmode);
>>> -
>>> -   if ( efi_rs->GetVariable(L"SecureBoot", (EFI_GUID *)&global_guid, NULL, 
>>> &secboot_size, &secboot) != EFI_SUCCESS )
>>
>> I'm slightly worried about the dropping of the const here, and the
>> fact that the variable is placed in initconst section. Isn't it
>> dangerous that the EFI services will try to write to it?
> 
> The EFI services do not try to write to it; the API doesn't
> even bother with const-correctness.  The prototype has IN
> and OUT, but they are not used for constness:
> 
> typedef EFI_STATUS(EFIAPI * EFI_GET_VARIABLE) (
> IN CHAR16 *VariableName,
> IN EFI_GUID *VendorGuid,
> OUT UINT32 *Attributes,
> OPTIONAL IN OUT UINTN *DataSize,
> OUT VOID *Data OPTIONAL)

And I think this underlying aspect if the reason for a lot of apparently
missing const in our EFI interfacing code.

Jan

Re: [PATCH v3 3/4] efi: Enable booting unified hypervisor/kernel/initrd images

[Jan Beulich]

On 14.09.2020 13:19, Trammell Hudson wrote:
> On Monday, September 14, 2020 6:06 AM, Roger Pau Monné  
> wrote:
>> On Mon, Sep 07, 2020 at 03:00:26PM -0400, Trammell Hudson wrote:
>>> -   file->ptr = (void *)pe_find_section(image->ImageBase, image->ImageSize,
>>
>> This already returns a void *, so there's no need for the cast.
> 
> It returns const void *, but file has a non-const pointer.  Perhaps files 
> should
> be immutable and that could be addressed in a const-correctness patch series.

But casting away const-ness is really bad. If file->ptr can't
be adjusted accordingly, I'm afraid the function will need to
return a pointer to non-const for the time being.

Jan

[PATCH v4 1/4] efi/boot.c: add file.need_to_free

[Trammell Hudson]

The config file, kernel, initrd, etc should only be freed if they
are allocated with the UEFI allocator.

Signed-off-by: Trammell Hudson 
---
 xen/common/efi/boot.c | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 4022a672c9..7156139174 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -102,6 +102,7 @@ union string {
 
 struct file {
 UINTN size;
+bool need_to_free;
 union {
 EFI_PHYSICAL_ADDRESS addr;
 void *ptr;
@@ -279,13 +280,13 @@ void __init noreturn blexit(const CHAR16 *str)
 if ( !efi_bs )
 efi_arch_halt();
 
-if ( cfg.addr )
+if ( cfg.addr && cfg.need_to_free )
 efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
-if ( kernel.addr )
+if ( kernel.addr && kernel.need_to_free )
 efi_bs->FreePages(kernel.addr, PFN_UP(kernel.size));
-if ( ramdisk.addr )
+if ( ramdisk.addr && ramdisk.need_to_free )
 efi_bs->FreePages(ramdisk.addr, PFN_UP(ramdisk.size));
-if ( xsm.addr )
+if ( xsm.addr && xsm.need_to_free )
 efi_bs->FreePages(xsm.addr, PFN_UP(xsm.size));
 
 efi_arch_blexit();
@@ -572,6 +573,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, 
CHAR16 *name,
  HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START);
 ret = efi_bs->AllocatePages(AllocateMaxAddress, EfiLoaderData,
 PFN_UP(size), &file->addr);
+file->need_to_free = true;
 }
 if ( EFI_ERROR(ret) )
 {
-- 
2.25.1

[PATCH v4 0/4] efi: Unified Xen hypervisor/kernel/initrd images

[Trammell Hudson]

This patch series adds support for bundling the xen.efi hypervisor,
the xen.cfg configuration file, the Linux kernel and initrd, as well
as the XSM, and architectural specific files into a single "unified"
EFI executable.  This allows an administrator to update the components
independently without requiring rebuilding xen, as well as to replace
the components in an existing image.

The resulting EFI executable can be invoked directly from the UEFI Boot
Manager, removing the need to use a separate loader like grub as well
as removing dependencies on local filesystem access.  And since it is
a single file, it can be signed and validated by UEFI Secure Boot without
requring the shim protocol.

It is inspired by systemd-boot's unified kernel technique and borrows the
function to locate PE sections from systemd's LGPL'ed code.  During EFI
boot, Xen looks at its own loaded image to locate the PE sections for
the Xen configuration (`.config`), dom0 kernel (`.kernel`), dom0 initrd
(`.ramdisk`), and XSM config (`.xsm`), which are included after building
xen.efi using objcopy to add named sections for each input file.

Trammell Hudson (4):
  efi/boot.c: add file.need_to_free
  efi/boot.c: add handle_file_info()
  efi: Enable booting unified hypervisor/kernel/initrd images
  efi: Do not use command line if secure boot is enabled.

 .gitignore  |   1 +
 docs/misc/efi.pandoc|  49 
 xen/arch/arm/efi/efi-boot.h |  25 --
 xen/arch/x86/efi/Makefile   |   2 +-
 xen/arch/x86/efi/efi-boot.h |  11 ++-
 xen/common/efi/boot.c   | 154 
 xen/common/efi/efi.h|   3 +
 xen/common/efi/pe.c | 137 
 8 files changed, 336 insertions(+), 46 deletions(-)
 create mode 100644 xen/common/efi/pe.c

-- 
2.25.1

[PATCH v4 4/4] efi: Do not use command line if secure boot is enabled.

[Trammell Hudson]

If secure boot is enabled, the Xen command line arguments are ignored.
If a unified Xen image is used, then the bundled configuration, dom0
kernel, and initrd are prefered over the ones listed in the config file.

Unlike the shim based verification, the PE signature on a unified image
covers the all of the Xen+config+kernel+initrd modules linked into the
unified image. This also ensures that properly configured platforms
will measure the entire runtime into the TPM for unsealing secrets or
remote attestation.

Signed-off-by: Trammell Hudson 
---
 xen/common/efi/boot.c | 44 ---
 1 file changed, 41 insertions(+), 3 deletions(-)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 4b1fbc9643..e65c1f1a09 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -949,6 +949,39 @@ static void __init setup_efi_pci(void)
 efi_bs->FreePool(handles);
 }
 
+/*
+ * Logic should remain sync'ed with linux/arch/x86/xen/efi.c
+ * Secure Boot is enabled iff 'SecureBoot' is set and the system is
+ * not in Setup Mode.
+ */
+static bool __init efi_secure_boot(void)
+{
+static const __initconst EFI_GUID global_guid = EFI_GLOBAL_VARIABLE;
+uint8_t secboot, setupmode;
+UINTN secboot_size = sizeof(secboot);
+UINTN setupmode_size = sizeof(setupmode);
+EFI_STATUS rc;
+
+rc = efi_rs->GetVariable(L"SecureBoot", (EFI_GUID *)&global_guid,
+ NULL, &secboot_size, &secboot);
+if ( rc != EFI_SUCCESS )
+return false;
+
+rc = efi_rs->GetVariable(L"SetupMode", (EFI_GUID *)&global_guid,
+ NULL, &setupmode_size, &setupmode);
+if ( rc != EFI_SUCCESS )
+return false;
+
+if ( secboot > 1)
+{
+PrintStr(L"Invalid SecureBoot variable=0x");
+DisplayUint(secboot, 2);
+PrintStr(newline);
+}
+
+return secboot == 1 && setupmode == 0;
+}
+
 static void __init efi_variables(void)
 {
 EFI_STATUS status;
@@ -1125,8 +1158,8 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
*SystemTable)
 static EFI_GUID __initdata shim_lock_guid = SHIM_LOCK_PROTOCOL_GUID;
 EFI_LOADED_IMAGE *loaded_image;
 EFI_STATUS status;
-unsigned int i, argc;
-CHAR16 **argv, *file_name, *cfg_file_name = NULL, *options = NULL;
+unsigned int i, argc = 0;
+CHAR16 **argv = NULL, *file_name, *cfg_file_name = NULL, *options = NULL;
 UINTN gop_mode = ~0;
 EFI_SHIM_LOCK_PROTOCOL *shim_lock;
 EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL;
@@ -1134,6 +1167,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
*SystemTable)
 bool base_video = false;
 char *option_str;
 bool use_cfg_file;
+bool secure = false;
 
 __set_bit(EFI_BOOT, &efi_flags);
 __set_bit(EFI_LOADER, &efi_flags);
@@ -1152,8 +1186,10 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
*SystemTable)
 PrintErrMesg(L"No Loaded Image Protocol", status);
 
 efi_arch_load_addr_check(loaded_image);
+secure = efi_secure_boot();
 
-if ( use_cfg_file )
+/* If UEFI Secure Boot is enabled, do not parse the command line */
+if ( use_cfg_file && !secure )
 {
 UINTN offset = 0;
 
@@ -1211,6 +1247,8 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
*SystemTable)
 
 PrintStr(L"Xen " __stringify(XEN_VERSION) "." __stringify(XEN_SUBVERSION)
  XEN_EXTRAVERSION " (c/s " XEN_CHANGESET ") EFI loader\r\n");
+if ( secure )
+PrintStr(L"UEFI Secure Boot enabled\r\n");
 
 efi_arch_relocate_image(0);
 
-- 
2.25.1

[PATCH v4 2/4] efi/boot.c: add handle_file_info()

[Trammell Hudson]

Add a separate function to display the address ranges used by
the files and call `efi_arch_handle_module()` on the modules.

Signed-off-by: Trammell Hudson 
---
 xen/common/efi/boot.c | 27 +--
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 7156139174..57df89cacb 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -539,6 +539,22 @@ static char * __init split_string(char *s)
 return NULL;
 }
 
+static void __init handle_file_info(CHAR16 *name,
+struct file *file, char *options)
+{
+if ( file == &cfg )
+return;
+
+PrintStr(name);
+PrintStr(L": ");
+DisplayUint(file->addr, 2 * sizeof(file->addr));
+PrintStr(L"-");
+DisplayUint(file->addr + file->size, 2 * sizeof(file->addr));
+PrintStr(newline);
+
+efi_arch_handle_module(file, name, options);
+}
+
 static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
  struct file *file, char *options)
 {
@@ -583,16 +599,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, 
CHAR16 *name,
 else
 {
 file->size = size;
-if ( file != &cfg )
-{
-PrintStr(name);
-PrintStr(L": ");
-DisplayUint(file->addr, 2 * sizeof(file->addr));
-PrintStr(L"-");
-DisplayUint(file->addr + size, 2 * sizeof(file->addr));
-PrintStr(newline);
-efi_arch_handle_module(file, name, options);
-}
+handle_file_info(name, file, options);
 
 ret = FileHandle->Read(FileHandle, &file->size, file->ptr);
 if ( !EFI_ERROR(ret) && file->size != size )
-- 
2.25.1

[PATCH v4 3/4] efi: Enable booting unified hypervisor/kernel/initrd images

[Trammell Hudson]

This patch adds support for bundling the xen.efi hypervisor, the xen.cfg
configuration file, the Linux kernel and initrd, as well as the XSM,
and architectural specific files into a single "unified" EFI executable.
This allows an administrator to update the components independently
without requiring rebuilding xen, as well as to replace the components
in an existing image.

The resulting EFI executable can be invoked directly from the UEFI Boot
Manager, removing the need to use a separate loader like grub as well
as removing dependencies on local filesystem access.  And since it is
a single file, it can be signed and validated by UEFI Secure Boot without
requring the shim protocol.

It is inspired by systemd-boot's unified kernel technique and borrows the
function to locate PE sections from systemd's LGPL'ed code.  During EFI
boot, Xen looks at its own loaded image to locate the PE sections for
the Xen configuration (`.config`), dom0 kernel (`.kernel`), dom0 initrd
(`.ramdisk`), and XSM config (`.xsm`), which are included after building
xen.efi using objcopy to add named sections for each input file.

For x86, the CPU ucode can be included in a section named `.ucode`,
which is loaded in the efi_arch_cfg_file_late() stage of the boot process.

On ARM systems the Device Tree can be included in a section named
`.dtb`, which is loaded during the efi_arch_cfg_file_early() stage of
the boot process.

Signed-off-by: Trammell Hudson 
---
 .gitignore  |   1 +
 docs/misc/efi.pandoc|  49 +
 xen/arch/arm/efi/efi-boot.h |  25 ---
 xen/arch/x86/efi/Makefile   |   2 +-
 xen/arch/x86/efi/efi-boot.h |  11 ++-
 xen/common/efi/boot.c   |  73 ++-
 xen/common/efi/efi.h|   3 +
 xen/common/efi/pe.c | 137 
 8 files changed, 272 insertions(+), 29 deletions(-)
 create mode 100644 xen/common/efi/pe.c

diff --git a/.gitignore b/.gitignore
index 823f4743dc..d568017804 100644
--- a/.gitignore
+++ b/.gitignore
@@ -299,6 +299,7 @@ xen/arch/*/efi/boot.c
 xen/arch/*/efi/compat.c
 xen/arch/*/efi/ebmalloc.c
 xen/arch/*/efi/efi.h
+xen/arch/*/efi/pe.c
 xen/arch/*/efi/runtime.c
 xen/common/config_data.S
 xen/common/config.gz
diff --git a/docs/misc/efi.pandoc b/docs/misc/efi.pandoc
index 23c1a2732d..ac3cd58cae 100644
--- a/docs/misc/efi.pandoc
+++ b/docs/misc/efi.pandoc
@@ -116,3 +116,52 @@ Filenames must be specified relative to the location of 
the EFI binary.
 
 Extra options to be passed to Xen can also be specified on the command line,
 following a `--` separator option.
+
+## Unified Xen kernel image
+
+The "Unified" kernel image can be generated by adding additional
+sections to the Xen EFI executable with objcopy, similar to how
+[systemd-boot uses the stub to add them to the Linux 
kernel](https://wiki.archlinux.org/index.php/systemd-boot#Preparing_a_unified_kernel_image)
+
+The sections for the xen configuration file, the dom0 kernel, dom0 initrd,
+XSM and CPU microcode should be added after the Xen `.pad` section, the
+ending address of which can be located with:
+
+```
+objdump -h xen.efi \
+   | perl -ane '/\.pad/ && printf "0x%016x\n", hex($F[2]) + hex($F[3])'
+```
+
+For all the examples the `.pad` section ended at 0x82d04100.
+All the sections are optional (`.config`, `.kernel`, `.ramdisk`, `.xsm`,
+`.ucode` (x86) and `.dtb` (ARM)) and the order does not matter.
+The virtual addresses do not need to be contiguous, although they should not
+be overlapping and should all be greater than the last virtual address of the
+hypervisor components.
+
+```
+objcopy \
+   --add-section .config=xen.cfg \
+   --change-section-vma .config=0x82d04100
+   --add-section .ucode=ucode.bin \
+   --change-section-vma .ucode=0x82d04101 \
+   --add-section .xsm=xsm.cfg \
+   --change-section-vma .xsm=0x82d04108 \
+   --add-section .kernel=vmlinux \
+   --change-section-vma .kernel=0x82d04110 \
+   --add-section .ramdisk=initrd.img \
+   --change-section-vma .ramdisk=0x82d04200 \
+   xen.efi \
+   xen.unified.efi
+```
+
+The unified executable can be signed with sbsigntool to make
+it usable with UEFI secure boot:
+
+```
+sbsign \
+   --key signing.key \
+   --cert cert.pem \
+   --output xen.signed.efi \
+   xen.unified.efi
+```
diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index 6527cb0bdf..08bd4d7630 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -375,27 +375,36 @@ static void __init noreturn efi_arch_post_exit_boot(void)
 efi_xen_start(fdt, fdt_totalsize(fdt));
 }
 
-static void __init efi_arch_cfg_file_early(EFI_FILE_HANDLE dir_handle, char 
*section)
+static void __init efi_arch_cfg_file_early(const EFI_LOADED_IMAGE *image,
+   EFI_FILE_HANDLE dir_handle,
+   char *section)
 {
 unio

Re: [PATCH v3 4/4] efi: Do not use command line if secure boot is enabled.

[Trammell Hudson]

On Monday, September 14, 2020 6:24 AM, Roger Pau Monné  
wrote:
> On Mon, Sep 07, 2020 at 03:00:27PM -0400, Trammell Hudson wrote:
> [...]
> > -   static const __initconst EFI_GUID global_guid = EFI_GLOBAL_VARIABLE;
> > -   uint8_t secboot, setupmode;
> > -   UINTN secboot_size = sizeof(secboot);
> > -   UINTN setupmode_size = sizeof(setupmode);
> > -
> > -   if ( efi_rs->GetVariable(L"SecureBoot", (EFI_GUID *)&global_guid, NULL, 
> > &secboot_size, &secboot) != EFI_SUCCESS )
>
> I'm slightly worried about the dropping of the const here, and the
> fact that the variable is placed in initconst section. Isn't it
> dangerous that the EFI services will try to write to it?

The EFI services do not try to write to it; the API doesn't
even bother with const-correctness.  The prototype has IN
and OUT, but they are not used for constness:

typedef EFI_STATUS(EFIAPI * EFI_GET_VARIABLE) (
IN CHAR16 *VariableName,
IN EFI_GUID *VendorGuid,
OUT UINT32 *Attributes,
OPTIONAL IN OUT UINTN *DataSize,
OUT VOID *Data OPTIONAL)

(So the VariableName string is also silently being turned
into a non-const pointer as well, which is just ugh)

> [...]
> > -   return secboot == 1 && setupmode == 0;
>
> I would print a message if secboot is > 1, since those should be
> reserved.

Ok.  Addressed in v4, coming soon.

--
Trammell

Re: [PATCH v3 3/4] efi: Enable booting unified hypervisor/kernel/initrd images

[Trammell Hudson]

On Monday, September 14, 2020 6:06 AM, Roger Pau Monné  
wrote:
> On Mon, Sep 07, 2020 at 03:00:26PM -0400, Trammell Hudson wrote:
> > [...]
> > It is inspired by systemd-boot's unified kernel technique and borrows the
> > function to locate PE sections from systemd's LGPL'ed code. During EFI
> > boot, Xen looks at its own loaded image to locate the PE sections for
> > the Xen configuration (`.config`), dom0 kernel (`.kernel`), dom0 initrd
> > (`.initrd`), and XSM config (`.xsm`), which are included after building
>
> Could we name this kernel_payload or maybe just payload instead of
> initrd?
>
> That's a Linux specific concept.

Perhaps "ramdisk" is better, since that is the name of the module in the
Xen config file?  That was what I used elsewhere (and messed up in the docs,
as you had noticed).

> [...]
> > -   --change-section-vma .initrd=0x82d04200 \
>
> Why do you need to adjust the VMA, is this not set to a suitable
> default value?
>
> How can users find a suitable VMA value?

The default is to put the new sections at virtual address 0, which causes
load errors.  It seemed to be necessary to have it above the hypervisor
image, although I'm also borrowing that from the systemd-boot code.
I wish objcopy had an "--append-section" that would add after the last
section in the file...

An earlier version of the patch series had a shell script to do this process,
although it was not as general as people wanted, so it was dropped in favor of
documenting how to build the images with objcopy.

> [...]
> > -   file->ptr = (void *)pe_find_section(image->ImageBase, image->ImageSize,
>
> This already returns a void *, so there's no need for the cast.

It returns const void *, but file has a non-const pointer.  Perhaps files should
be immutable and that could be addressed in a const-correctness patch series.

The style guide issues will also be addressed in the v4 of the patch, to
be posted soon.

--
Trammell

Re: [PATCH v3] tools/libs/stat: fix broken build

[Bertrand Marquis]

> On 12 Sep 2020, at 14:08, Juergen Gross  wrote:
> 
> Making getBridge() static triggered a build error with some gcc versions:
> 
> error: 'strncpy' output may be truncated copying 15 bytes from a string of
> length 255 [-Werror=stringop-truncation]
> 
> Fix that by using a buffer with 256 bytes instead.
> 
> Fixes: 6d0ec053907794 ("tools: split libxenstat into new tools/libs/stat 
> directory")
> Signed-off-by: Juergen Gross 
Reviewed-by: Bertrand Marquis 

> ---
> tools/libs/stat/xenstat_linux.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libs/stat/xenstat_linux.c b/tools/libs/stat/xenstat_linux.c
> index 793263f2b6..d2ee6fda64 100644
> --- a/tools/libs/stat/xenstat_linux.c
> +++ b/tools/libs/stat/xenstat_linux.c
> @@ -78,7 +78,7 @@ static void getBridge(char *excludeName, char *result, 
> size_t resultLen)
>   sprintf(tmp, "/sys/class/net/%s/bridge", 
> de->d_name);
> 
>   if (access(tmp, F_OK) == 0) {
> - strncpy(result, de->d_name, resultLen - 
> 1);
> + strncpy(result, de->d_name, resultLen);
>   result[resultLen - 1] = 0;
>   }
>   }
> @@ -264,7 +264,7 @@ int xenstat_collect_networks(xenstat_node * node)
> {
>   /* Helper variables for parseNetDevLine() function defined above */
>   int i;
> - char line[512] = { 0 }, iface[16] = { 0 }, devBridge[16] = { 0 }, 
> devNoBridge[17] = { 0 };
> + char line[512] = { 0 }, iface[16] = { 0 }, devBridge[256] = { 0 }, 
> devNoBridge[257] = { 0 };
>   unsigned long long rxBytes, rxPackets, rxErrs, rxDrops, txBytes, 
> txPackets, txErrs, txDrops;
> 
>   struct priv_data *priv = get_priv_data(node->handle);
> -- 
> 2.26.2
> 
> 

Re: [PATCH v8 5/8] remove remaining uses of iommu_legacy_map/unmap

[Julien Grall]

Hi Paul,

I am sorry for jumping very late in the discussion.

On 11/09/2020 09:20, Paul Durrant wrote:

From: Paul Durrant 

The 'legacy' functions do implicit flushing so amend the callers to do the
appropriate flushing.

Unfortunately, because of the structure of the P2M code, we cannot remove
the per-CPU 'iommu_dont_flush_iotlb' global and the optimization it
facilitates. It is now checked directly iommu_iotlb_flush(). This is safe
because callers of iommu_iotlb_flush() on another CPU will not be affected,
and hence a flush will be done. Also, 'iommu_dont_flush_iotlb' is now declared
as bool (rather than bool_t) and setting/clearing it are no longer pointlessly
gated on is_iommu_enabled() returning true. (Arguably it is also pointless to
gate the call to iommu_iotlb_flush() on that condition - since it is a no-op
in that case - but the if clause allows the scope of a stack variable to be
restricted).


Unfortunately, this change will re-open a potential security hole closed 
by commit 671878779741:


xen/arm: p2m: Free the p2m entry after flushing the IOMMU TLBs

When freeing a p2m entry, all the sub-tree behind it will also be 
freed.

This may include intermediate page-tables or any l3 entry requiring to
drop a reference (e.g for foreign pages). As soon as pages are freed,
they may be re-used by Xen or another domain. Therefore it is necessary
to flush *all* the TLBs beforehand.

While CPU TLBs will be flushed before freeing the pages, this is not
the case for IOMMU TLBs. This can be solved by moving the IOMMU TLBs
flush earlier in the code.

This wasn't considered as a security issue as device passthrough on Arm
is not security supported.

Signed-off-by: Julien Grall 
Tested-by: Oleksandr Tyshchenko 
Reviewed-by: Stefano Stabellini 
Release-acked-by: Juergen Gross 

One possibility would be to treat iommu_dont_flush_iotlb as x86 only.

Cheers,

--
Julien Grall

Re: [PATCH v3 2/4] efi/boot.c: add file.need_to_free and split display_file_info()

[Trammell Hudson]

On Monday, September 14, 2020 5:05 AM, Roger Pau Monné  
wrote:

> Thanks! Being picky you likely wan to split this into two separate
> commits: one for adding need_to_free and the other for
> display_file_info. There's no relation between the two that would
> require them to be on the same commit.

Ok.  I'll address that in the v4 of the patch.

> [...]
> > +static void __init display_file_info(CHAR16 *name, struct file *file, char 
> > *options)
>
> I think name at least could be constified?

Oh, I wish.  There should be a major pass to constify the EFI functions.
So many of them are not const-correct and it worries me that
literal strings are passed to those functions.

> Also efi_arch_handle_module seem to do more than just printing file
> info, hence I would likely rename this to handle_file_info to be more
> representative of what it does.

Ok. Fixed in v4.

--
Trammell

Re: [PATCH v8 4/8] iommu: make map and unmap take a page count, similar to flush

[Julien Grall]

Hi Paul,

On 11/09/2020 09:20, Paul Durrant wrote:

From: Paul Durrant 

At the moment iommu_map() and iommu_unmap() take a page order rather than a
count, whereas iommu_iotlb_flush() takes a page count rather than an order.
This patch makes them consistent with each other, opting for a page count since
CPU page orders are not necessarily the same as those of an IOMMU.

NOTE: The 'page_count' parameter is also made an unsigned long in all the
   aforementioned functions.

Signed-off-by: Paul Durrant 


For Arm and common code:

Reviewed-by: Julien Grall 

Cheers,


---
Cc: Jan Beulich 
Cc: Andrew Cooper 
Cc: Wei Liu 
Cc: "Roger Pau Monné" 
Cc: George Dunlap 
Cc: Ian Jackson 
Cc: Julien Grall 
Cc: Stefano Stabellini 
Cc: Jun Nakajima 
Cc: Kevin Tian 
Cc: Bertrand Marquis 
Cc: Oleksandr Tyshchenko 

v8:
  - Fix IPMMU-VMSA code too

v7:
  - Fix ARM build

v6:
  - Fix missing conversion to unsigned long in AMD code
  - Fixed unconverted call to iommu_legacy_unmap() in x86/mm.c
  - s/1ul/1 in the grant table code

v5:
  - Re-worked to just use a page count, rather than both a count and an order

v2:
  - New in v2
---
  xen/arch/x86/mm.c|  8 --
  xen/arch/x86/mm/p2m-ept.c|  6 ++--
  xen/arch/x86/mm/p2m-pt.c |  4 +--
  xen/arch/x86/mm/p2m.c|  5 ++--
  xen/arch/x86/x86_64/mm.c |  4 +--
  xen/common/grant_table.c |  6 ++--
  xen/drivers/passthrough/amd/iommu.h  |  2 +-
  xen/drivers/passthrough/amd/iommu_map.c  |  4 +--
  xen/drivers/passthrough/arm/ipmmu-vmsa.c |  2 +-
  xen/drivers/passthrough/arm/smmu.c   |  2 +-
  xen/drivers/passthrough/iommu.c  | 35 +++-
  xen/drivers/passthrough/vtd/iommu.c  |  4 +--
  xen/drivers/passthrough/x86/iommu.c  |  2 +-
  xen/include/xen/iommu.h  | 12 
  14 files changed, 48 insertions(+), 48 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 56bf7add2b..095422024a 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2422,7 +2422,7 @@ static int cleanup_page_mappings(struct page_info *page)
  
  if ( d && unlikely(need_iommu_pt_sync(d)) && is_pv_domain(d) )

  {
-int rc2 = iommu_legacy_unmap(d, _dfn(mfn), PAGE_ORDER_4K);
+int rc2 = iommu_legacy_unmap(d, _dfn(mfn), 1u << PAGE_ORDER_4K);
  
  if ( !rc )

  rc = rc2;
@@ -2949,9 +2949,11 @@ static int _get_page_type(struct page_info *page, 
unsigned long type,
  mfn_t mfn = page_to_mfn(page);
  
  if ( (x & PGT_type_mask) == PGT_writable_page )

-rc = iommu_legacy_unmap(d, _dfn(mfn_x(mfn)), PAGE_ORDER_4K);
+rc = iommu_legacy_unmap(d, _dfn(mfn_x(mfn)),
+1ul << PAGE_ORDER_4K);
  else
-rc = iommu_legacy_map(d, _dfn(mfn_x(mfn)), mfn, PAGE_ORDER_4K,
+rc = iommu_legacy_map(d, _dfn(mfn_x(mfn)), mfn,
+  1ul << PAGE_ORDER_4K,
IOMMUF_readable | IOMMUF_writable);
  
  if ( unlikely(rc) )

diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index b8154a7ecc..12cf38f6eb 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -843,14 +843,14 @@ out:
   need_modify_vtd_table )
  {
  if ( iommu_use_hap_pt(d) )
-rc = iommu_iotlb_flush(d, _dfn(gfn), (1u << order),
+rc = iommu_iotlb_flush(d, _dfn(gfn), 1ul << order,
 (iommu_flags ? IOMMU_FLUSHF_added : 0) |
 (vtd_pte_present ? IOMMU_FLUSHF_modified
  : 0));
  else if ( need_iommu_pt_sync(d) )
  rc = iommu_flags ?
-iommu_legacy_map(d, _dfn(gfn), mfn, order, iommu_flags) :
-iommu_legacy_unmap(d, _dfn(gfn), order);
+iommu_legacy_map(d, _dfn(gfn), mfn, 1ul << order, iommu_flags) 
:
+iommu_legacy_unmap(d, _dfn(gfn), 1ul << order);
  }
  
  unmap_domain_page(table);

diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
index badb26bc34..3af51be78e 100644
--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -679,9 +679,9 @@ p2m_pt_set_entry(struct p2m_domain *p2m, gfn_t gfn_, mfn_t 
mfn,
  if ( need_iommu_pt_sync(p2m->domain) &&
   (iommu_old_flags != iommu_pte_flags || old_mfn != mfn_x(mfn)) )
  rc = iommu_pte_flags
- ? iommu_legacy_map(d, _dfn(gfn), mfn, page_order,
+ ? iommu_legacy_map(d, _dfn(gfn), mfn, 1ul << page_order,
  iommu_pte_flags)
- : iommu_legacy_unmap(d, _dfn(gfn), page_order);
+ : iommu_legacy_unmap(d, _dfn(gfn), 1ul << page_order);
  
  /*

   * Free old intermediate tables if ne

[PATCH 9/9] lib: move sort code

[Jan Beulich]

Build this code into an archive, to parallel bsearch().

Signed-off-by: Jan Beulich 
---
 xen/common/Makefile| 1 -
 xen/lib/Makefile   | 1 +
 xen/{common => lib}/sort.c | 0
 3 files changed, 1 insertion(+), 1 deletion(-)
 rename xen/{common => lib}/sort.c (100%)

diff --git a/xen/common/Makefile b/xen/common/Makefile
index 149466b473a8..efcfd4c34ecf 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -36,7 +36,6 @@ obj-y += rcupdate.o
 obj-y += rwlock.o
 obj-y += shutdown.o
 obj-y += softirq.o
-obj-y += sort.o
 obj-y += smp.o
 obj-y += spinlock.o
 obj-y += stop_machine.o
diff --git a/xen/lib/Makefile b/xen/lib/Makefile
index 122eeb3d327b..33ff322b1655 100644
--- a/xen/lib/Makefile
+++ b/xen/lib/Makefile
@@ -6,3 +6,4 @@ lib-y += ctype.o
 lib-y += list-sort.o
 lib-y += parse-size.o
 lib-y += rbtree.o
+lib-y += sort.o
diff --git a/xen/common/sort.c b/xen/lib/sort.c
similarity index 100%
rename from xen/common/sort.c
rename to xen/lib/sort.c
-- 
2.22.0

[PATCH 6/9] lib: move init_constructors()

[Jan Beulich]

... into its own CU, for being unrelated to other things in
common/lib.c. For now it gets compiled into built_in.o rather than
lib.a, as it gets used unconditionally by Arm's as well as x86'es
{,__}start_xen(). But this could be changed in principle, the more that
there typically aren't any constructors anyway. Then again it's just
__init code anyway.

Signed-off-by: Jan Beulich 
---
 xen/common/lib.c | 14 --
 xen/lib/Makefile |  1 +
 xen/lib/ctors.c  | 25 +
 3 files changed, 26 insertions(+), 14 deletions(-)
 create mode 100644 xen/lib/ctors.c

diff --git a/xen/common/lib.c b/xen/common/lib.c
index 6cfa332142a5..f5ca179a0af4 100644
--- a/xen/common/lib.c
+++ b/xen/common/lib.c
@@ -1,6 +1,5 @@
 #include 
 #include 
-#include 
 #include 
 
 /*
@@ -423,19 +422,6 @@ uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
 #endif
 }
 
-typedef void (*ctor_func_t)(void);
-extern const ctor_func_t __ctors_start[], __ctors_end[];
-
-void __init init_constructors(void)
-{
-const ctor_func_t *f;
-for ( f = __ctors_start; f < __ctors_end; ++f )
-(*f)();
-
-/* Putting this here seems as good (or bad) as any other place. */
-BUILD_BUG_ON(sizeof(size_t) != sizeof(ssize_t));
-}
-
 /*
  * Local variables:
  * mode: C
diff --git a/xen/lib/Makefile b/xen/lib/Makefile
index 99f857540c99..ba1fb7bcdee2 100644
--- a/xen/lib/Makefile
+++ b/xen/lib/Makefile
@@ -1,3 +1,4 @@
+obj-y += ctors.o
 obj-$(CONFIG_X86) += x86/
 
 lib-y += ctype.o
diff --git a/xen/lib/ctors.c b/xen/lib/ctors.c
new file mode 100644
index ..5bdc591cd50a
--- /dev/null
+++ b/xen/lib/ctors.c
@@ -0,0 +1,25 @@
+#include 
+#include 
+
+typedef void (*ctor_func_t)(void);
+extern const ctor_func_t __ctors_start[], __ctors_end[];
+
+void __init init_constructors(void)
+{
+const ctor_func_t *f;
+for ( f = __ctors_start; f < __ctors_end; ++f )
+(*f)();
+
+/* Putting this here seems as good (or bad) as any other place. */
+BUILD_BUG_ON(sizeof(size_t) != sizeof(ssize_t));
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.22.0

[PATCH 8/9] lib: move bsearch code

[Jan Beulich]

Build this code into an archive, which results in not linking it into
x86 final binaries. This saves a little bit of dead code.

Signed-off-by: Jan Beulich 
---
 xen/common/Makefile   | 1 -
 xen/lib/Makefile  | 1 +
 xen/{common => lib}/bsearch.c | 0
 3 files changed, 1 insertion(+), 1 deletion(-)
 rename xen/{common => lib}/bsearch.c (100%)

diff --git a/xen/common/Makefile b/xen/common/Makefile
index 46406dccdfd4..149466b473a8 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -1,6 +1,5 @@
 obj-$(CONFIG_ARGO) += argo.o
 obj-y += bitmap.o
-obj-y += bsearch.o
 obj-$(CONFIG_HYPFS_CONFIG) += config_data.o
 obj-$(CONFIG_CORE_PARKING) += core_parking.o
 obj-y += cpu.o
diff --git a/xen/lib/Makefile b/xen/lib/Makefile
index b469d2dff7b8..122eeb3d327b 100644
--- a/xen/lib/Makefile
+++ b/xen/lib/Makefile
@@ -1,6 +1,7 @@
 obj-y += ctors.o
 obj-$(CONFIG_X86) += x86/
 
+lib-y += bsearch.o
 lib-y += ctype.o
 lib-y += list-sort.o
 lib-y += parse-size.o
diff --git a/xen/common/bsearch.c b/xen/lib/bsearch.c
similarity index 100%
rename from xen/common/bsearch.c
rename to xen/lib/bsearch.c
-- 
2.22.0

[PATCH 7/9] lib: move rbtree code

[Jan Beulich]

Build this code into an archive, which results in not linking it into
x86 final binaries. This saves about 1.5k of dead code.

While moving the source file, take the opportunity and drop the
pointless EXPORT_SYMBOL().

Signed-off-by: Jan Beulich 
---
 xen/common/Makefile  | 1 -
 xen/lib/Makefile | 1 +
 xen/{common => lib}/rbtree.c | 9 +
 3 files changed, 2 insertions(+), 9 deletions(-)
 rename xen/{common => lib}/rbtree.c (98%)

diff --git a/xen/common/Makefile b/xen/common/Makefile
index 958ad8c7d946..46406dccdfd4 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -33,7 +33,6 @@ obj-y += preempt.o
 obj-y += random.o
 obj-y += rangeset.o
 obj-y += radix-tree.o
-obj-y += rbtree.o
 obj-y += rcupdate.o
 obj-y += rwlock.o
 obj-y += shutdown.o
diff --git a/xen/lib/Makefile b/xen/lib/Makefile
index ba1fb7bcdee2..b469d2dff7b8 100644
--- a/xen/lib/Makefile
+++ b/xen/lib/Makefile
@@ -4,3 +4,4 @@ obj-$(CONFIG_X86) += x86/
 lib-y += ctype.o
 lib-y += list-sort.o
 lib-y += parse-size.o
+lib-y += rbtree.o
diff --git a/xen/common/rbtree.c b/xen/lib/rbtree.c
similarity index 98%
rename from xen/common/rbtree.c
rename to xen/lib/rbtree.c
index 9f5498a89d4e..95e045d52461 100644
--- a/xen/common/rbtree.c
+++ b/xen/lib/rbtree.c
@@ -25,7 +25,7 @@
 #include 
 
 /*
- * red-black trees properties:  http://en.wikipedia.org/wiki/Rbtree 
+ * red-black trees properties:  http://en.wikipedia.org/wiki/Rbtree
  *
  *  1) A node is either red or black
  *  2) The root is black
@@ -223,7 +223,6 @@ void rb_insert_color(struct rb_node *node, struct rb_root 
*root)
}
}
 }
-EXPORT_SYMBOL(rb_insert_color);
 
 static void __rb_erase_color(struct rb_node *parent, struct rb_root *root)
 {
@@ -467,7 +466,6 @@ void rb_erase(struct rb_node *node, struct rb_root *root)
if (rebalance)
__rb_erase_color(rebalance, root);
 }
-EXPORT_SYMBOL(rb_erase);
 
 /*
  * This function returns the first node (in sort order) of the tree.
@@ -483,7 +481,6 @@ struct rb_node *rb_first(const struct rb_root *root)
n = n->rb_left;
return n;
 }
-EXPORT_SYMBOL(rb_first);
 
 struct rb_node *rb_last(const struct rb_root *root)
 {
@@ -496,7 +493,6 @@ struct rb_node *rb_last(const struct rb_root *root)
n = n->rb_right;
return n;
 }
-EXPORT_SYMBOL(rb_last);
 
 struct rb_node *rb_next(const struct rb_node *node)
 {
@@ -528,7 +524,6 @@ struct rb_node *rb_next(const struct rb_node *node)
 
return parent;
 }
-EXPORT_SYMBOL(rb_next);
 
 struct rb_node *rb_prev(const struct rb_node *node)
 {
@@ -557,7 +552,6 @@ struct rb_node *rb_prev(const struct rb_node *node)
 
return parent;
 }
-EXPORT_SYMBOL(rb_prev);
 
 void rb_replace_node(struct rb_node *victim, struct rb_node *new,
 struct rb_root *root)
@@ -574,4 +568,3 @@ void rb_replace_node(struct rb_node *victim, struct rb_node 
*new,
/* Copy the pointers/colour from the victim to the replacement */
*new = *victim;
 }
-EXPORT_SYMBOL(rb_replace_node);
-- 
2.22.0

[PATCH 4/9] lib: move list sorting code

[Jan Beulich]

Build the source file always, as by putting it into an archive it still
won't be linked into final binaries when not needed. This way possible
build breakage will be easier to notice, and it's more consistent with
us unconditionally building other library kind of code (e.g. sort() or
bsearch()).

While moving the source file, take the opportunity and drop the
pointless EXPORT_SYMBOL().

Signed-off-by: Jan Beulich 
---
 xen/arch/arm/Kconfig| 4 +---
 xen/common/Kconfig  | 3 ---
 xen/common/Makefile | 1 -
 xen/lib/Makefile| 1 +
 xen/{common/list_sort.c => lib/list-sort.c} | 2 --
 5 files changed, 2 insertions(+), 9 deletions(-)
 rename xen/{common/list_sort.c => lib/list-sort.c} (98%)

diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 277738826581..cb7e2523b6de 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -56,9 +56,7 @@ config HVM
 def_bool y
 
 config NEW_VGIC
-   bool
-   prompt "Use new VGIC implementation"
-   select NEEDS_LIST_SORT
+   bool "Use new VGIC implementation"
---help---
 
This is an alternative implementation of the ARM GIC interrupt
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 15e3b79ff57f..87e99d4ba2f7 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -66,9 +66,6 @@ config HAS_SCHED_GRANULARITY
 config NEEDS_LIBELF
bool
 
-config NEEDS_LIST_SORT
-   bool
-
 menu "Speculative hardening"
 
 config SPECULATIVE_HARDEN_ARRAY
diff --git a/xen/common/Makefile b/xen/common/Makefile
index b3b60a1ba25b..958ad8c7d946 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -21,7 +21,6 @@ obj-y += keyhandler.o
 obj-$(CONFIG_KEXEC) += kexec.o
 obj-$(CONFIG_KEXEC) += kimage.o
 obj-y += lib.o
-obj-$(CONFIG_NEEDS_LIST_SORT) += list_sort.o
 obj-$(CONFIG_LIVEPATCH) += livepatch.o livepatch_elf.o
 obj-$(CONFIG_MEM_ACCESS) += mem_access.o
 obj-y += memory.o
diff --git a/xen/lib/Makefile b/xen/lib/Makefile
index b8814361d63e..764f3624b5f9 100644
--- a/xen/lib/Makefile
+++ b/xen/lib/Makefile
@@ -1,3 +1,4 @@
 obj-$(CONFIG_X86) += x86/
 
 lib-y += ctype.o
+lib-y += list-sort.o
diff --git a/xen/common/list_sort.c b/xen/lib/list-sort.c
similarity index 98%
rename from xen/common/list_sort.c
rename to xen/lib/list-sort.c
index af2b2f6519f1..f8d8bbf28178 100644
--- a/xen/common/list_sort.c
+++ b/xen/lib/list-sort.c
@@ -15,7 +15,6 @@
  * this program; If not, see .
  */
 
-#include 
 #include 
 
 #define MAX_LIST_LENGTH_BITS 20
@@ -154,4 +153,3 @@ void list_sort(void *priv, struct list_head *head,
 
merge_and_restore_back_links(priv, cmp, head, part[max_lev], list);
 }
-EXPORT_SYMBOL(list_sort);
-- 
2.22.0

[PATCH 3/9] lib: collect library files in an archive

[Jan Beulich]

In order to (subsequently) drop odd things like CONFIG_NEEDS_LIST_SORT
just to avoid bloating binaries when only some arch-es and/or
configurations need generic library routines, combine objects under lib/
into an archive, which the linker then can pick the necessary objects
out of.

Note that we can't use thin archives just yet, until we've raised the
minimum required binutils version suitably.

Signed-off-by: Jan Beulich 
---
 xen/Rules.mk  | 33 +++--
 xen/arch/arm/Makefile |  6 +++---
 xen/arch/x86/Makefile |  8 
 xen/lib/Makefile  |  3 ++-
 4 files changed, 36 insertions(+), 14 deletions(-)

diff --git a/xen/Rules.mk b/xen/Rules.mk
index 333e19bec343..e59c7f213f77 100644
--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -41,12 +41,16 @@ ALL_OBJS-y   += $(BASEDIR)/xsm/built_in.o
 ALL_OBJS-y   += $(BASEDIR)/arch/$(TARGET_ARCH)/built_in.o
 ALL_OBJS-$(CONFIG_CRYPTO)   += $(BASEDIR)/crypto/built_in.o
 
+ALL_LIBS-y   := $(BASEDIR)/lib/lib.a
+
 # Initialise some variables
+lib-y :=
 targets :=
 CFLAGS-y :=
 AFLAGS-y :=
 
 ALL_OBJS := $(ALL_OBJS-y)
+ALL_LIBS := $(ALL_LIBS-y)
 
 SPECIAL_DATA_SECTIONS := rodata $(foreach a,1 2 4 8 16, \
 $(foreach w,1 2 4, \
@@ -60,7 +64,14 @@ include Makefile
 # ---
 
 quiet_cmd_ld = LD  $@
-cmd_ld = $(LD) $(XEN_LDFLAGS) -r -o $@ $(real-prereqs)
+cmd_ld = $(LD) $(XEN_LDFLAGS) -r -o $@ $(filter-out %.a,$(real-prereqs)) \
+   --start-group $(filter %.a,$(real-prereqs)) --end-group
+
+# Archive
+# ---
+
+quiet_cmd_ar = AR  $@
+cmd_ar = rm -f $@; $(AR) cPrs $@ $(real-prereqs)
 
 # Objcopy
 # ---
@@ -86,6 +97,10 @@ obj-y:= $(patsubst %/, %/built_in.o, $(obj-y))
 # tell kbuild to descend
 subdir-obj-y := $(filter %/built_in.o, $(obj-y))
 
+# Libraries are always collected in one lib file.
+# Filter out objects already built-in
+lib-y := $(filter-out $(obj-y), $(sort $(lib-y)))
+
 $(filter %.init.o,$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS-y += 
-DINIT_SECTIONS_ONLY
 
 ifeq ($(CONFIG_COVERAGE),y)
@@ -129,19 +144,25 @@ include $(BASEDIR)/arch/$(TARGET_ARCH)/Rules.mk
 c_flags += $(CFLAGS-y)
 a_flags += $(CFLAGS-y) $(AFLAGS-y)
 
-built_in.o: $(obj-y) $(extra-y)
+built_in.o: $(obj-y) $(if $(strip $(lib-y)),lib.a) $(extra-y)
 ifeq ($(obj-y),)
$(CC) $(c_flags) -c -x c /dev/null -o $@
 else
 ifeq ($(CONFIG_LTO),y)
-   $(LD_LTO) -r -o $@ $(filter-out $(extra-y),$^)
+   $(LD_LTO) -r -o $@ $(filter-out lib.a $(extra-y),$^)
 else
-   $(LD) $(XEN_LDFLAGS) -r -o $@ $(filter-out $(extra-y),$^)
+   $(LD) $(XEN_LDFLAGS) -r -o $@ $(filter-out lib.a $(extra-y),$^)
 endif
 endif
 
+lib.a: $(lib-y) FORCE
+   $(call if_changed,ar)
+
 targets += built_in.o
-targets += $(filter-out $(subdir-obj-y), $(obj-y)) $(extra-y)
+ifneq ($(strip $(lib-y)),)
+targets += lib.a
+endif
+targets += $(filter-out $(subdir-obj-y), $(obj-y) $(lib-y)) $(extra-y)
 targets += $(MAKECMDGOALS)
 
 built_in_bin.o: $(obj-bin-y) $(extra-y)
@@ -155,7 +176,7 @@ endif
 PHONY += FORCE
 FORCE:
 
-%/built_in.o: FORCE
+%/built_in.o %/lib.a: FORCE
$(MAKE) -f $(BASEDIR)/Rules.mk -C $* built_in.o
 
 %/built_in_bin.o: FORCE
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 296c5e68bbc3..612a83b315c8 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -90,14 +90,14 @@ endif
 
 ifeq ($(CONFIG_LTO),y)
 # Gather all LTO objects together
-prelink_lto.o: $(ALL_OBJS)
-   $(LD_LTO) -r -o $@ $^
+prelink_lto.o: $(ALL_OBJS) $(ALL_LIBS)
+   $(LD_LTO) -r -o $@ $(filter-out %.a,$^) --start-group $(filter %.a,$^) 
--end-group
 
 # Link it with all the binary objects
 prelink.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o
$(call if_changed,ld)
 else
-prelink.o: $(ALL_OBJS) FORCE
+prelink.o: $(ALL_OBJS) $(ALL_LIBS) FORCE
$(call if_changed,ld)
 endif
 
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 9b368632fb43..8f2180485b2b 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -132,8 +132,8 @@ EFI_OBJS-$(XEN_BUILD_EFI) := efi/relocs-dummy.o
 
 ifeq ($(CONFIG_LTO),y)
 # Gather all LTO objects together
-prelink_lto.o: $(ALL_OBJS)
-   $(LD_LTO) -r -o $@ $^
+prelink_lto.o: $(ALL_OBJS) $(ALL_LIBS)
+   $(LD_LTO) -r -o $@ $(filter-out %.a,$^) --start-group $(filter %.a,$^) 
--end-group
 
 # Link it with all the binary objects
 prelink.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o 
$(EFI_OBJS-y) FORCE
@@ -142,10 +142,10 @@ prelink.o: $(patsubst 
%/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o $
 prelink-efi.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) 
prelink_lto.o FORCE
$(call if_changed,ld)
 else
-prelink.o: $(ALL_

[PATCH 5/9] lib: move parse_size_and_unit()

[Jan Beulich]

... into its own CU, to build it into an archive.

Signed-off-by: Jan Beulich 
---
 xen/common/lib.c | 39 --
 xen/lib/Makefile |  1 +
 xen/lib/parse-size.c | 50 
 3 files changed, 51 insertions(+), 39 deletions(-)
 create mode 100644 xen/lib/parse-size.c

diff --git a/xen/common/lib.c b/xen/common/lib.c
index a224efa8f6e8..6cfa332142a5 100644
--- a/xen/common/lib.c
+++ b/xen/common/lib.c
@@ -423,45 +423,6 @@ uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
 #endif
 }
 
-unsigned long long parse_size_and_unit(const char *s, const char **ps)
-{
-unsigned long long ret;
-const char *s1;
-
-ret = simple_strtoull(s, &s1, 0);
-
-switch ( *s1 )
-{
-case 'T': case 't':
-ret <<= 10;
-/* fallthrough */
-case 'G': case 'g':
-ret <<= 10;
-/* fallthrough */
-case 'M': case 'm':
-ret <<= 10;
-/* fallthrough */
-case 'K': case 'k':
-ret <<= 10;
-/* fallthrough */
-case 'B': case 'b':
-s1++;
-break;
-case '%':
-if ( ps )
-break;
-/* fallthrough */
-default:
-ret <<= 10; /* default to kB */
-break;
-}
-
-if ( ps != NULL )
-*ps = s1;
-
-return ret;
-}
-
 typedef void (*ctor_func_t)(void);
 extern const ctor_func_t __ctors_start[], __ctors_end[];
 
diff --git a/xen/lib/Makefile b/xen/lib/Makefile
index 764f3624b5f9..99f857540c99 100644
--- a/xen/lib/Makefile
+++ b/xen/lib/Makefile
@@ -2,3 +2,4 @@ obj-$(CONFIG_X86) += x86/
 
 lib-y += ctype.o
 lib-y += list-sort.o
+lib-y += parse-size.o
diff --git a/xen/lib/parse-size.c b/xen/lib/parse-size.c
new file mode 100644
index ..ec980cadfff3
--- /dev/null
+++ b/xen/lib/parse-size.c
@@ -0,0 +1,50 @@
+#include 
+
+unsigned long long parse_size_and_unit(const char *s, const char **ps)
+{
+unsigned long long ret;
+const char *s1;
+
+ret = simple_strtoull(s, &s1, 0);
+
+switch ( *s1 )
+{
+case 'T': case 't':
+ret <<= 10;
+/* fallthrough */
+case 'G': case 'g':
+ret <<= 10;
+/* fallthrough */
+case 'M': case 'm':
+ret <<= 10;
+/* fallthrough */
+case 'K': case 'k':
+ret <<= 10;
+/* fallthrough */
+case 'B': case 'b':
+s1++;
+break;
+case '%':
+if ( ps )
+break;
+/* fallthrough */
+default:
+ret <<= 10; /* default to kB */
+break;
+}
+
+if ( ps != NULL )
+*ps = s1;
+
+return ret;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.22.0

[PATCH 2/9] lib: split _ctype[] into its own object, under lib/

[Jan Beulich]

This is, besides for tidying, in preparation of then starting to use an
archive rather than an object file for generic library code which
arch-es (or even specific configurations within a single arch) may or
may not need.

Signed-off-by: Jan Beulich 
---
 xen/Makefile |  3 ++-
 xen/Rules.mk |  2 +-
 xen/common/lib.c | 29 -
 xen/lib/Makefile |  1 +
 xen/lib/ctype.c  | 38 ++
 5 files changed, 42 insertions(+), 31 deletions(-)
 create mode 100644 xen/lib/ctype.c

diff --git a/xen/Makefile b/xen/Makefile
index bf0c804d4352..73bdc326c549 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -331,6 +331,7 @@ _clean: delete-unfresh-files
$(MAKE) $(clean) include
$(MAKE) $(clean) common
$(MAKE) $(clean) drivers
+   $(MAKE) $(clean) lib
$(MAKE) $(clean) xsm
$(MAKE) $(clean) crypto
$(MAKE) $(clean) arch/arm
@@ -414,7 +415,7 @@ include/asm-$(TARGET_ARCH)/asm-offsets.h: 
arch/$(TARGET_ARCH)/asm-offsets.s
  echo ""; \
  echo "#endif") <$< >$@
 
-SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers test
+SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test
 define all_sources
 ( find include/asm-$(TARGET_ARCH) -name '*.h' -print; \
   find include -name 'asm-*' -prune -o -name '*.h' -print; \
diff --git a/xen/Rules.mk b/xen/Rules.mk
index 891c94e6ad00..333e19bec343 100644
--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -36,7 +36,7 @@ TARGET := $(BASEDIR)/xen
 # Note that link order matters!
 ALL_OBJS-y   += $(BASEDIR)/common/built_in.o
 ALL_OBJS-y   += $(BASEDIR)/drivers/built_in.o
-ALL_OBJS-$(CONFIG_X86)   += $(BASEDIR)/lib/built_in.o
+ALL_OBJS-y   += $(BASEDIR)/lib/built_in.o
 ALL_OBJS-y   += $(BASEDIR)/xsm/built_in.o
 ALL_OBJS-y   += $(BASEDIR)/arch/$(TARGET_ARCH)/built_in.o
 ALL_OBJS-$(CONFIG_CRYPTO)   += $(BASEDIR)/crypto/built_in.o
diff --git a/xen/common/lib.c b/xen/common/lib.c
index b2b799da44c5..a224efa8f6e8 100644
--- a/xen/common/lib.c
+++ b/xen/common/lib.c
@@ -1,37 +1,8 @@
-
-#include 
 #include 
 #include 
 #include 
 #include 
 
-/* for ctype.h */
-const unsigned char _ctype[] = {
-_C,_C,_C,_C,_C,_C,_C,_C,/* 0-7 */
-_C,_C|_S,_C|_S,_C|_S,_C|_S,_C|_S,_C,_C, /* 8-15 */
-_C,_C,_C,_C,_C,_C,_C,_C,/* 16-23 */
-_C,_C,_C,_C,_C,_C,_C,_C,/* 24-31 */
-_S|_SP,_P,_P,_P,_P,_P,_P,_P,/* 32-39 */
-_P,_P,_P,_P,_P,_P,_P,_P,/* 40-47 */
-_D,_D,_D,_D,_D,_D,_D,_D,/* 48-55 */
-_D,_D,_P,_P,_P,_P,_P,_P,/* 56-63 */
-_P,_U|_X,_U|_X,_U|_X,_U|_X,_U|_X,_U|_X,_U,  /* 64-71 */
-_U,_U,_U,_U,_U,_U,_U,_U,/* 72-79 */
-_U,_U,_U,_U,_U,_U,_U,_U,/* 80-87 */
-_U,_U,_U,_P,_P,_P,_P,_P,/* 88-95 */
-_P,_L|_X,_L|_X,_L|_X,_L|_X,_L|_X,_L|_X,_L,  /* 96-103 */
-_L,_L,_L,_L,_L,_L,_L,_L,/* 104-111 */
-_L,_L,_L,_L,_L,_L,_L,_L,/* 112-119 */
-_L,_L,_L,_P,_P,_P,_P,_C,/* 120-127 */
-0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,/* 128-143 */
-0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,/* 144-159 */
-_S|_SP,_P,_P,_P,_P,_P,_P,_P,_P,_P,_P,_P,_P,_P,_P,_P,   /* 160-175 */
-_P,_P,_P,_P,_P,_P,_P,_P,_P,_P,_P,_P,_P,_P,_P,_P,   /* 176-191 */
-_U,_U,_U,_U,_U,_U,_U,_U,_U,_U,_U,_U,_U,_U,_U,_U,   /* 192-207 */
-_U,_U,_U,_U,_U,_U,_U,_P,_U,_U,_U,_U,_U,_U,_U,_L,   /* 208-223 */
-_L,_L,_L,_L,_L,_L,_L,_L,_L,_L,_L,_L,_L,_L,_L,_L,   /* 224-239 */
-_L,_L,_L,_L,_L,_L,_L,_P,_L,_L,_L,_L,_L,_L,_L,_L};  /* 240-255 */
-
 /*
  * A couple of 64 bit operations ported from FreeBSD.
  * The code within the '#if BITS_PER_LONG == 32' block below, and no other
diff --git a/xen/lib/Makefile b/xen/lib/Makefile
index 7019ca00e8fd..53b1da025e0d 100644
--- a/xen/lib/Makefile
+++ b/xen/lib/Makefile
@@ -1 +1,2 @@
+obj-y += ctype.o
 obj-$(CONFIG_X86) += x86/
diff --git a/xen/lib/ctype.c b/xen/lib/ctype.c
new file mode 100644
index ..7b233a335fdf
--- /dev/null
+++ b/xen/lib/ctype.c
@@ -0,0 +1,38 @@
+#include 
+
+/* for ctype.h */
+const unsigned char _ctype[] = {
+_C,_C,_C,_C,_C,_C,_C,_C,/* 0-7 */
+_C,_C|_S,_C|_S,_C|_S,_C|_S,_C|_S,_C,_C, /* 8-15 */
+_C,_C,_C,_C,_C,_C,_C,_C,/* 16-23 */
+_C,_C,_C,_C,_C,_C,_C,_C,/* 24-31 */
+_S|_SP,_P,_P,_P,_P,_P,_P,_P,/* 32-39 */
+_P,_P,_P,_P,_P,_P,_P,_P,/* 40-47 */
+_D,_D,_D,_D,_D,_D,_D,_D,/* 48-55 */
+_D,_D,_P,_P,_P,_P,_P,_P,/* 56-63 */
+_P,_U|_X,_U|_X,_U|_X,_U|_X,_U|_X,_U|_X,_U,  /* 64-71 */
+_U,_U,_U,_U,_U,_U,_U,_U, 

[PATCH 1/9] build: use if_changed more consistently (and correctly) for prelink*.o

[Jan Beulich]

Switch to $(call if_changed,ld) where possible; presumably not doing so
in e321576f4047 ("xen/build: start using if_changed") right away was an
oversight, as it did for Arm in (just) one case. It failed to add
prelink.o to $(targets), though, causing - judging from the observed
behavior on x86 - undue rebuilds of the final binary (because of
prelink.o getting rebuild for $(cmd_prelink.o) being empty, in turn
because of .prelink.o.cmd not getting read) during "make install-xen".

Signed-off-by: Jan Beulich 
---
 xen/arch/arm/Makefile |  4 +++-
 xen/arch/x86/Makefile | 18 ++
 2 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 51173d97127e..296c5e68bbc3 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -95,12 +95,14 @@ prelink_lto.o: $(ALL_OBJS)
 
 # Link it with all the binary objects
 prelink.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o
-   $(LD) $(XEN_LDFLAGS) -r -o $@ $^
+   $(call if_changed,ld)
 else
 prelink.o: $(ALL_OBJS) FORCE
$(call if_changed,ld)
 endif
 
+targets += prelink.o
+
 $(TARGET)-syms: prelink.o xen.lds
$(LD) $(XEN_LDFLAGS) -T xen.lds -N prelink.o \
$(BASEDIR)/common/symbols-dummy.o -o $(@D)/.$(@F).0
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 74152f2a0dad..9b368632fb43 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -136,19 +136,21 @@ prelink_lto.o: $(ALL_OBJS)
$(LD_LTO) -r -o $@ $^
 
 # Link it with all the binary objects
-prelink.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o 
$(EFI_OBJS-y)
-   $(LD) $(XEN_LDFLAGS) -r -o $@ $^
+prelink.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o 
$(EFI_OBJS-y) FORCE
+   $(call if_changed,ld)
 
-prelink-efi.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) 
prelink_lto.o
-   $(LD) $(XEN_LDFLAGS) -r -o $@ $^
+prelink-efi.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) 
prelink_lto.o FORCE
+   $(call if_changed,ld)
 else
-prelink.o: $(ALL_OBJS) $(EFI_OBJS-y)
-   $(LD) $(XEN_LDFLAGS) -r -o $@ $^
+prelink.o: $(ALL_OBJS) $(EFI_OBJS-y) FORCE
+   $(call if_changed,ld)
 
-prelink-efi.o: $(ALL_OBJS)
-   $(LD) $(XEN_LDFLAGS) -r -o $@ $^
+prelink-efi.o: $(ALL_OBJS) FORCE
+   $(call if_changed,ld)
 endif
 
+targets += prelink.o prelink-efi.o
+
 $(TARGET)-syms: prelink.o xen.lds
$(LD) $(XEN_LDFLAGS) -T xen.lds -N prelink.o $(build_id_linker) \
$(BASEDIR)/common/symbols-dummy.o -o $(@D)/.$(@F).0
-- 
2.22.0

[PATCH 0/9] xen: beginnings of moving library-like code into an archive

[Jan Beulich]

In a few cases we link in library-like functions when they're not
actually needed. While we could use Kconfig options for each one
of them, I think the better approach for such generic code is to
build it always (thus making sure a build issue can't be introduced
for these in any however exotic configuration) and then put it into
an archive, for the linker to pick up as needed. The series here
presents a first few tiny steps towards such a goal.

Not that we can't use thin archives yet, due to our tool chain
(binutils) baseline being too low.

The first patch actually isn't directly related to the rest of the
series, except that - to avoid undue redundancy - I ran into the
issue addressed there while (originally) making patch 3 convert to
using $(call if_changed,ld) "on the fly". IOW it's a full
(contextual and functional) prereq to the series.

Further almost immediate steps I'd like to take if the approach
meets no opposition are
- split and move the rest of common/lib.c,
- split and move common/string.c, dropping the need for all the
  __HAVE_ARCH_* (implying possible per-arch archives then need to
  be specified ahead of lib/lib.a on the linker command lines),
- move common/libelf/ and common/libfdt/.

1: build: use if_changed more consistently (and correctly) for prelink*.o
2: lib: split _ctype[] into its own object, under lib/
3: lib: collect library files in an archive
4: lib: move list sorting code
5: lib: move parse_size_and_unit()
6: lib: move init_constructors()
7: lib: move rbtree code
8: lib: move bsearch code
9: lib: move sort code

Jan

Re: [PATCH] tools/libs/guest: fix Makefile regarding zlib options

[Wei Liu]

On Mon, Sep 14, 2020 at 09:39:01AM +, Wei Liu wrote:
> On Thu, Sep 10, 2020 at 05:42:10PM +0200, Juergen Gross wrote:
> > When renaming the libxenguest sources to xg_*.c there was an omission
> > in the Makefile when setting the zlib related define for the related
> > sources. Fix that.
> > 
> > Signed-off-by: Juergen Gross 
> 
> Acked-by: Wei Liu 

Actually this is already applied. Thanks Andrew.

Re: Adopting the Linux Kernel Memory Model in Xen?

[Julien Grall]

Hi Jan,

On 14/09/2020 10:03, Jan Beulich wrote:

On 11.09.2020 18:33, Julien Grall wrote:

At the moment, Xen doesn't have a formal memory model. Instead, we are
relying on intuitions. This can lead to heated discussion on what can a
processor/compiler do or not.

We also have some helpers that nearly do the same (such as
{read,write}_atomic() vs ACCESS_ONCE()) with no clear understanding
where to use which.

In the past few years, Linux community spent a lot of time to write down
their memory model and make the compiler communities aware of it (see
[1], [2]).

There are a few reasons I can see for adopting LKMM:
 - Xen borrows a fair amount of code from Linux;
 - There are efforts to standardize it;
 - This will allow us to streamline the discussion.


While I agree with the goal, I'm uncertain about the last of the
three points above, at least as long as we're "blindly" taking
whatever they do or decide. Over the years they've changed their
implementation a number of time afaict, in order to deal with
"disagreements" between it and what compilers actually do and/or can
be expected to guarantee. Yes, the Linux community is much bigger
than ours, and hence chances are far better for someone there to
notice and correct flaws or oversights, yet I still think it cannot
be the goal to silence discussions on our side, even if they tend to
be unpleasant for (almost) everyone involved.


Xen-devel (or security@) is not suited for arguing on how a 
compiler/processor should behave (or not). We don't have the expertise 
for making a proper decision.


Don't get me wrong, I am not trying to silence discussion but rather 
move them to the correct forum.


If we adopt the LKMM, then all the discussions on Xen-devel could be 
reduced to whether the code match the formal model.


If there are any questions on the model, then they would be raised 
directly with the LKMM team. They can then assess if they need to update

the model.



One additional thing needs to be kept in mind imo, especially also
having seen Andrew's reply: If we more formally tie ourselves to
their model (and I agree with him that informally we already do so
anyway in sufficiently large a degree), we need to take measures to
make sure we also adjust our code when they adjust theirs.


This makes perfect sense. I would expect the effort to be quite minimal 
in long term.


Cheers,

--
Julien Grall

Re: [PATCH] tools/build: fix python xc bindings

[Wei Liu]

On Mon, Sep 14, 2020 at 11:38:19AM +0200, Marek Marczykowski-Górecki wrote:
> On Mon, Sep 14, 2020 at 09:35:42AM +, Wei Liu wrote:
> > On Sat, Sep 12, 2020 at 03:58:07PM +0200, Juergen Gross wrote:
> > > Commit 7c273ffdd0e91 ("tools/python: drop libxenguest from setup.py")
> > > was just wrong: there is one function from libxenguest used in the
> > > bindings, so readd the library again.
> > > 
> > > While at it remove the unused PATH_LIBXL setting.
> > > 
> > > Fixes: 7c273ffdd0e91 ("tools/python: drop libxenguest from setup.py")
> > > Signed-off-by: Juergen Gross 
> > 
> > Acked-by: Wei Liu 
> 
> Acked-by: Marek Marczykowski-Górecki 
> 

Thanks for the quick turnaround, Marek.

Re: [PATCH] tools/libs/guest: fix Makefile regarding zlib options

[Wei Liu]

On Thu, Sep 10, 2020 at 05:42:10PM +0200, Juergen Gross wrote:
> When renaming the libxenguest sources to xg_*.c there was an omission
> in the Makefile when setting the zlib related define for the related
> sources. Fix that.
> 
> Signed-off-by: Juergen Gross 

Acked-by: Wei Liu 

Re: [PATCH] tools/build: fix python xc bindings

[Marek Marczykowski-Górecki]

On Mon, Sep 14, 2020 at 09:35:42AM +, Wei Liu wrote:
> On Sat, Sep 12, 2020 at 03:58:07PM +0200, Juergen Gross wrote:
> > Commit 7c273ffdd0e91 ("tools/python: drop libxenguest from setup.py")
> > was just wrong: there is one function from libxenguest used in the
> > bindings, so readd the library again.
> > 
> > While at it remove the unused PATH_LIBXL setting.
> > 
> > Fixes: 7c273ffdd0e91 ("tools/python: drop libxenguest from setup.py")
> > Signed-off-by: Juergen Gross 
> 
> Acked-by: Wei Liu 

Acked-by: Marek Marczykowski-Górecki 

> Since this blocks osstest and is just putting back old code I want to
> commit it as quickly as possible.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?


signature.asc
Description: PGP signature

Re: [PATCH] tools/build: avoid $(file ...) directive in Makefile

[Wei Liu]

On Sat, Sep 12, 2020 at 10:49:13AM +0200, Juergen Gross wrote:
> Using $(file ...) breaks the build with make older than version 4.0.
> Replace it with echo.
> 
> Fixes: 52dbd6f07cea7a ("tools: generate pkg-config files from make variables")
> Signed-off-by: Juergen Gross 

This patch is superseded by Andrew's patch.

Wei.

Re: [PATCH] tools/build: fix python xc bindings

[Wei Liu]

On Sat, Sep 12, 2020 at 03:58:07PM +0200, Juergen Gross wrote:
> Commit 7c273ffdd0e91 ("tools/python: drop libxenguest from setup.py")
> was just wrong: there is one function from libxenguest used in the
> bindings, so readd the library again.
> 
> While at it remove the unused PATH_LIBXL setting.
> 
> Fixes: 7c273ffdd0e91 ("tools/python: drop libxenguest from setup.py")
> Signed-off-by: Juergen Gross 

Acked-by: Wei Liu 

Since this blocks osstest and is just putting back old code I want to
commit it as quickly as possible.

Re: [PATCH v3] tools/libs/stat: fix broken build

[Wei Liu]

On Sat, Sep 12, 2020 at 03:08:36PM +0200, Juergen Gross wrote:
> Making getBridge() static triggered a build error with some gcc versions:
> 
> error: 'strncpy' output may be truncated copying 15 bytes from a string of
> length 255 [-Werror=stringop-truncation]
> 
> Fix that by using a buffer with 256 bytes instead.
> 
> Fixes: 6d0ec053907794 ("tools: split libxenstat into new tools/libs/stat 
> directory")
> Signed-off-by: Juergen Gross 

Acked-by: Wei Liu 

Re: [PATCH 2/2] tools/Makefile: Drop the use of $(file ...)

[Wei Liu]

On Mon, Sep 14, 2020 at 10:24:20AM +0100, Andrew Cooper wrote:
> It is only available in make 4.0 and later, and not for example in CentOS 7.
> 
> Rewrite the logic to use echo and shell redirection, using a single capture
> group to avoid having 12 different processes in quick succession each
> appending one line to the file.
> 
> Fixes: 52dbd6f07cea7a ("tools: generate pkg-config files from make variables")
> Signed-off-by: Andrew Cooper 

Acked-by: Wei Liu 

Re: [PATCH 1/2] tools/libs/vchan: Don't run the headers check

[Wei Liu]

On Mon, Sep 14, 2020 at 10:24:19AM +0100, Andrew Cooper wrote:
> There was never a headers check previously, and CentOS 6 can't cope with the
> anonymous union in struct libxenvchan.
> 
>   cc1: warnings being treated as errors
>   ... tools/include/libxenvchan.h:75: error: declaration does not declare 
> anything
>   make[6]: *** [headers.chk] Error 1
> 
> Fixes: 8ab2429f12 ("tools: split libxenvchan into new tools/libs/vchan 
> directory")
> Signed-off-by: Andrew Cooper 

Acked-by: Wei Liu 

Re: [PATCH v3 1/4] x86/xen.lds.S: Work around binutils build id alignment bug

[Jan Beulich]

On 14.09.2020 11:14, Trammell Hudson wrote:
> On Tuesday, September 8, 2020 8:29 AM, Jan Beulich  wrote:
>> [...] As with, I think, the majority of new
>> features, distros would pick up your new functionality mainly for
>> use in new versions, and hence would likely run with new binutils
>> anyway by that time.
> 
> It also occurs to me that the binutils used to process the xen.efi
> image does not need to be the same as the one used to generate it,
> so there are no build-time dependencies on having this fix in place.
> 
> Dropping this patch from the series doesn't affect the ability of a
> distribution with a new binutils from being able to build unified
> images, so I'm fine with abandoning it.
> 
> Are there any further thoughts on the other parts of the series?

It's on my list of things to look at, yes. I'm sorry it's taking
some time to get there.

Jan

Re: [PATCH v3 1/4] x86/xen.lds.S: Work around binutils build id alignment bug

[Trammell Hudson]

On Tuesday, September 8, 2020 8:29 AM, Jan Beulich  wrote:
> [...] As with, I think, the majority of new
> features, distros would pick up your new functionality mainly for
> use in new versions, and hence would likely run with new binutils
> anyway by that time.

It also occurs to me that the binutils used to process the xen.efi
image does not need to be the same as the one used to generate it,
so there are no build-time dependencies on having this fix in place.

Dropping this patch from the series doesn't affect the ability of a
distribution with a new binutils from being able to build unified
images, so I'm fine with abandoning it.

Are there any further thoughts on the other parts of the series?

--
Trammell