[qemu-mainline test] 154583: regressions - FAIL

[osstest service owner]

flight 154583 qemu-mainline real [real]
http://logs.test-lab.xenproject.org/osstest/logs/154583/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-amd64-xl-qcow210 debian-di-installfail REGR. vs. 152631
 test-amd64-i386-xl-qemuu-dmrestrict-amd64-dmrestrict 10 debian-hvm-install 
fail REGR. vs. 152631
 test-amd64-amd64-xl-qemuu-win7-amd64 10 windows-install  fail REGR. vs. 152631
 test-amd64-amd64-qemuu-nested-intel 10 debian-hvm-install fail REGR. vs. 152631
 test-amd64-amd64-xl-qemuu-ovmf-amd64 10 debian-hvm-install fail REGR. vs. 
152631
 test-amd64-amd64-xl-qemuu-debianhvm-amd64-shadow 10 debian-hvm-install fail 
REGR. vs. 152631
 test-amd64-amd64-xl-qemuu-dmrestrict-amd64-dmrestrict 10 debian-hvm-install 
fail REGR. vs. 152631
 test-amd64-i386-xl-qemuu-win7-amd64 10 windows-install   fail REGR. vs. 152631
 test-amd64-i386-xl-qemuu-debianhvm-i386-xsm 10 debian-hvm-install fail REGR. 
vs. 152631
 test-amd64-amd64-qemuu-nested-amd 10 debian-hvm-install  fail REGR. vs. 152631
 test-amd64-amd64-xl-qemuu-ws16-amd64 10 windows-install  fail REGR. vs. 152631
 test-amd64-amd64-xl-qemuu-debianhvm-i386-xsm 10 debian-hvm-install fail REGR. 
vs. 152631
 test-amd64-amd64-xl-qemuu-debianhvm-amd64 10 debian-hvm-install fail REGR. vs. 
152631
 test-amd64-i386-qemuu-rhel6hvm-amd 10 redhat-install fail REGR. vs. 152631
 test-amd64-i386-xl-qemuu-debianhvm-amd64 10 debian-hvm-install fail REGR. vs. 
152631
 test-amd64-i386-xl-qemuu-ws16-amd64 10 windows-install   fail REGR. vs. 152631
 test-amd64-i386-qemuu-rhel6hvm-intel 10 redhat-install   fail REGR. vs. 152631
 test-amd64-i386-xl-qemuu-ovmf-amd64 10 debian-hvm-install fail REGR. vs. 152631
 test-amd64-i386-xl-qemuu-debianhvm-amd64-shadow 10 debian-hvm-install fail 
REGR. vs. 152631
 test-arm64-arm64-libvirt-xsm 12 guest-start  fail REGR. vs. 152631
 test-armhf-armhf-libvirt 12 guest-start  fail REGR. vs. 152631
 test-amd64-amd64-libvirt-vhd 11 guest-start  fail REGR. vs. 152631
 test-armhf-armhf-libvirt-raw 10 debian-di-installfail REGR. vs. 152631
 test-amd64-amd64-qemuu-freebsd11-amd64 19 guest-start/freebsd.repeat fail 
REGR. vs. 152631
 test-armhf-armhf-xl-vhd  10 debian-di-installfail REGR. vs. 152631

Regressions which are regarded as allowable (not blocking):
 test-amd64-amd64-xl-rtds 18 guest-localmigrate/x10   fail REGR. vs. 152631

Tests which did not succeed, but are not blocking:
 test-amd64-i386-xl-pvshim12 guest-start  fail   never pass
 test-amd64-amd64-libvirt 13 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-xsm  13 migrate-support-checkfail   never pass
 test-amd64-i386-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-arm64-arm64-xl-xsm  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  14 saverestore-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-credit2  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  14 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check 
fail never pass
 test-amd64-i386-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-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  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail  never pass
 test-armhf-armhf-xl-credit2  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-cubietruck 13 

[linux-linus test] 154582: regressions - FAIL

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-i386-qemut-rhel6hvm-intel  6 xen-install  fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-dmrestrict-amd64-dmrestrict 6 xen-install fail REGR. 
vs. 152332
 test-amd64-i386-xl-qemuu-ws16-amd64  6 xen-install   fail REGR. vs. 152332
 test-amd64-i386-xl-xsm6 xen-install  fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-debianhvm-amd64-shadow 6 xen-install fail REGR. vs. 
152332
 test-amd64-i386-qemuu-rhel6hvm-intel  6 xen-install  fail REGR. vs. 152332
 test-amd64-i386-xl-qemut-debianhvm-amd64  6 xen-install  fail REGR. vs. 152332
 test-amd64-i386-examine   6 xen-install  fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-debianhvm-i386-xsm 6 xen-install fail REGR. vs. 152332
 test-amd64-i386-libvirt   6 xen-install  fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-debianhvm-amd64  6 xen-install  fail REGR. vs. 152332
 test-amd64-i386-qemut-rhel6hvm-amd  6 xen-installfail REGR. vs. 152332
 test-amd64-i386-xl-qemut-ws16-amd64  6 xen-install   fail REGR. vs. 152332
 test-amd64-i386-pair  8 xen-install/src_host fail REGR. vs. 152332
 test-amd64-i386-qemuu-rhel6hvm-amd  6 xen-installfail REGR. vs. 152332
 test-amd64-i386-pair  9 xen-install/dst_host fail REGR. vs. 152332
 test-amd64-i386-xl6 xen-install  fail REGR. vs. 152332
 test-amd64-i386-libvirt-xsm   6 xen-install  fail REGR. vs. 152332
 test-amd64-coresched-i386-xl  6 xen-install  fail REGR. vs. 152332
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 6 xen-install fail REGR. vs. 
152332
 test-amd64-i386-xl-pvshim 6 xen-install  fail REGR. vs. 152332
 test-amd64-i386-freebsd10-amd64  6 xen-install   fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-win7-amd64  6 xen-install   fail REGR. vs. 152332
 test-amd64-i386-xl-qemut-debianhvm-i386-xsm 6 xen-install fail REGR. vs. 152332
 test-amd64-i386-xl-shadow 6 xen-install  fail REGR. vs. 152332
 test-amd64-i386-freebsd10-i386  6 xen-installfail REGR. vs. 152332
 test-amd64-i386-xl-raw6 xen-install  fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-ovmf-amd64  6 xen-install   fail REGR. vs. 152332
 test-amd64-i386-xl-qemut-win7-amd64  6 xen-install   fail REGR. vs. 152332
 test-amd64-i386-libvirt-pair  8 xen-install/src_host fail REGR. vs. 152332
 test-amd64-i386-libvirt-pair  9 xen-install/dst_host fail REGR. vs. 152332
 test-arm64-arm64-libvirt-xsm 16 guest-start/debian.repeat fail REGR. vs. 152332
 test-amd64-i386-xl-qemut-stubdom-debianhvm-amd64-xsm 6 xen-install fail REGR. 
vs. 152332
 test-armhf-armhf-xl-vhd 15 guest-start/debian.repeat 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-armhf-armhf-libvirt 14 saverestore-support-checkfail  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-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-credit2  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  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-arm64-arm64-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 14 saverestore-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-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-arm64-arm64-xl-thunderx 13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 14 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-multivcpu 13 

Re: [PATCH v4 4/4] xen/mm: Provide dummy M2P-related helpers when !CONFIG_HAVE_M2P

[Andrew Cooper]

On 21/09/2020 19:02, Julien Grall wrote:
> diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
> index 4536a62940a1..15bb0aa30d22 100644
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -685,4 +685,17 @@ static inline void put_page_alloc_ref(struct page_info 
> *page)
>  }
>  }
>  
> +/*
> + * Dummy implementation of M2P-related helpers for common code when
> + * the architecture doesn't have an M2P.
> + */
> +#ifndef CONFIG_HAS_M2P
> +
> +#define INVALID_M2P_ENTRY(~0UL)
> +#define SHARED_M2P(_e)   false

((void)_e, false)

Otherwise, Acked-by: Andrew Cooper 

> +
> +static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned long pfn) {}
> +
> +#endif
> +
>  #endif /* __XEN_MM_H__ */

Re: [PATCH v4 3/4] xen: Remove mfn_to_gmfn macro

[Andrew Cooper]

On 21/09/2020 19:02, Julien Grall wrote:
> From: Julien Grall 
>
> On x86, mfn_to_gmfn can be replaced with mfn_to_gfn. On Arm, there are
> no more call to mfn_to_gmfn, so the helper can be dropped.

The previous patch dropped the mfn_to_gmfn() call from the domain shared
info path, but without a hunk adjusting the innards of
memory_exchange(), this is going to break the x86 build.

> At the same time rework a comment in Arm code that does not make sense.
>
> Signed-off-by: Julien Grall 

To save a round trip, Acked-by: Andrew Cooper
 with the appropriate hunk to memory_exchange().

Alternatively, it might make sense to fold the adjustment into patch 1
which is perhaps more obvious, given the insertion of an is_pv_domain()
check.

~Andrew

Re: [PATCH v4 2/4] xen: Introduce HAS_M2P config and use to protect mfn_to_gmfn call

[Andrew Cooper]

On 21/09/2020 19:02, Julien Grall wrote:
> From: Julien Grall 
>
> While Arm never had a M2P, the implementation of mfn_to_gmfn is pretty
> bogus as we directly return the MFN passed in parameter.
>
> Thankfully, the use of mfn_to_gmfn is pretty limited on Arm today. There
> are only 2 callers in the common code:
> - memory_exchange: Arm cannot not use it because steal_page is not
> implemented. Note this is already protected by !CONFIG_PV.
> - getdomaininfo: Toolstack may map the shared page. It looks like
> this is mostly used for mapping the P2M of PV guest. Therefore the
> issue might be minor.
>
> Implementing the M2P on Arm is not planned. The M2P would require significant
> amount of VA address (very tough on 32-bit) that can hardly be justified with
> the current use of mfn_to_gmfn.

If anyone cares, access to the shared info page should be wired up
through acquire_resource, not through this foreign mapping bodge,
because ...

> - memory_exchange: This does not work and I haven't seen any
> request for it so far.
> - getdomaininfo: The structure on Arm does not seem to contain a lot
> of useful information on Arm. It is unclear whether we want to
> allow the toolstack mapping it on Arm.
>
> This patch introduces a config option HAS_M2P to tell whether an
> architecture implements the M2P.
> - memory_exchange: This is PV only (not supported on Arm) but take
> the opportunity to gate with HAS_M2P as well in case someone decide
> to implement PV without M2P.
> - getdomaininfo: A new helper is introduced to wrap the call to
> mfn_to_gfn/mfn_to_gmfn. For Arm, a fixed value will be provided that will
> fail on mapping if used.
>
> Signed-off-by Julien Grall 

... possibly the single best reason for returning -1 on ARM is that this
is already the behaviour for x86 HVM guests, until the guest has mapped
the shared info frame for the first time.

(XEN) *** d0v6 getdomaininfo(d1) d->shared_info 83007cffe000,
shared_info_frame 0x7cffe
(XEN) *** d0v6 getdomaininfo(d2) d->shared_info 83007a401000,
shared_info_frame 0x

(d1 is PV, d2 is HVM.  This is the result of creating domains at the
python shell, then querying them for their info, without any further
construction or execution).

Furthermore, both PV and HVM guests can invalidate the M2P mappings for
their shared_info page, which in the HVM case would cause -1 to be
returned again.

> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index 5ac6e9c5cafa..2bf3e6659018 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -68,6 +68,7 @@ void getdomaininfo(struct domain *d, struct 
> xen_domctl_getdomaininfo *info)
>  u64 cpu_time = 0;
>  int flags = XEN_DOMINF_blocked;
>  struct vcpu_runstate_info runstate;
> +gfn_t shared_info_frame;
>  
>  info->domain = d->domain_id;
>  info->max_vcpu_id = XEN_INVALID_MAX_VCPU_ID;
> @@ -111,8 +112,12 @@ void getdomaininfo(struct domain *d, struct 
> xen_domctl_getdomaininfo *info)
>  info->outstanding_pages = d->outstanding_pages;
>  info->shr_pages = atomic_read(>shr_pages);
>  info->paged_pages   = atomic_read(>paged_pages);
> -info->shared_info_frame = mfn_to_gmfn(d, virt_to_mfn(d->shared_info));
> -BUG_ON(SHARED_M2P(info->shared_info_frame));
> +
> +shared_info_frame = domain_shared_info_gfn(d);
> +if ( gfn_eq(shared_info_frame, INVALID_GFN) )
> +info->shared_info_frame = XEN_INVALID_SHARED_INFO_FRAME;
> +else
> +info->shared_info_frame = gfn_x(shared_info_frame);

This can be simplified to just  info->shared_info_frame =
gfn_x(arch_shared_info_gfn(d)) based on the suggestion at the bottom.

>  
>  info->cpupool = cpupool_get_id(d);
>  
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index 9300104943b0..c698e6872596 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -504,7 +504,7 @@ static bool propagate_node(unsigned int xmf, unsigned int 
> *memflags)
>  
>  static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) 
> arg)
>  {
> -#ifdef CONFIG_PV
> +#if defined(CONFIG_PV) && defined(CONFIG_M2P)

There is no such thing as PV && !M2P.  The M2P table is part of the PV
ABI with guests.

These two hunks should be dropped.

>  struct xen_memory_exchange exch;
>  PAGE_LIST_HEAD(in_chunk_list);
>  PAGE_LIST_HEAD(out_chunk_list);
> @@ -801,7 +801,7 @@ static long 
> memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
>  if ( __copy_field_to_guest(arg, , nr_exchanged) )
>  rc = -EFAULT;
>  return rc;
> -#else /* !CONFIG_PV */
> +#else /* !(CONFIG_PV && CONFIG_M2P) */
>  return -EOPNOTSUPP;
>  #endif
>  }
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 6819a3bf382f..90161dd079a1 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -262,6 +262,11 @@ static inline void 

Re: [PATCH V2] SUPPORT.md: Update status of Renesas IPMMU-VMSA (Arm)

[Stefano Stabellini]

On Sat, 19 Sep 2020, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko 
> 
> Mark Renesas IPMMU-VMSA as "Supported, not security supported"
> and remove dependencies on CONFIG_EXPERT.
> 
> We can't treat the IOMMU driver as "Supported" since the device
> passthrough feature is not security supported on Arm today.
> 
> Signed-off-by: Oleksandr Tyshchenko 

Acked-by: Stefano Stabellini 


> ---
> Was "SUPPORT.md: Mark Renesas IPMMU-VMSA (Arm) as supported"
> https://lists.xenproject.org/archives/html/xen-devel/2020-09/msg00967.html
> 
> Changes V1 -> V2:
>- Update patch subject/description
>- Use "Supported, not security supported"
> 
> ---
>  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..25987ec 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, not security 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 v4 1/4] xen: XENMEM_exchange should only be used/compiled for arch supporting PV guest

[Andrew Cooper]

On 21/09/2020 19:02, Julien Grall wrote:
> From: Julien Grall 
>
> XENMEM_exchange can only be used by PV guest but the check is well
> hidden in steal_page(). This is because paging_model_external() will
> return false only for PV domain.
>
> To make clearer this is PV only, add a check at the beginning of the
> implementation. Take the opportunity to compile out the code if
> CONFIG_PV is not set.
>
> This change will also help a follow-up patch where the gmfn_mfn() will
> be completely removed on arch not supporting the M2P.
>
> Signed-off-by: Julien Grall 
>
> ---
>
> Jan suggested to #ifdef anything after the check to is_pv_domain().
> However, it means to have two block of #ifdef as we can't mix
> declaration and code.
>
> I am actually thinking to move the implementation outside of mm.c in
> possibly arch/x86 or a pv specific directory under common. Any opinion?

arch/x86/pv/mm.c, with the case XENMEM_exchange: moving into
arch_memory_op().

However, making this happen is incredibly tangled, and we're years
overdue a fix here.

Lets go with this for now, and tidying up can come later.

Acked-by: Andrew Cooper , however...

>
> Changes in v4:
> - Patch added
> ---
>  xen/common/memory.c | 7 +++
>  1 file changed, 7 insertions(+)
>
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index 714077c1e597..9300104943b0 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -504,6 +504,7 @@ static bool propagate_node(unsigned int xmf, unsigned int 
> *memflags)
>  
>  static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) 
> arg)
>  {
> +#ifdef CONFIG_PV
>  struct xen_memory_exchange exch;
>  PAGE_LIST_HEAD(in_chunk_list);
>  PAGE_LIST_HEAD(out_chunk_list);
> @@ -516,6 +517,9 @@ static long 
> memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
>  struct domain *d;
>  struct page_info *page;
>  
> +if ( !is_pv_domain(d) )
> +return -EOPNOTSUPP;
> +
>  if ( copy_from_guest(, arg, 1) )
>  return -EFAULT;
>  
> @@ -797,6 +801,9 @@ static long 
> memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)

... there are now a load of #ifdef CONFIG_X86 between these two hunks
which can be dropped.

~Andrew

>  if ( __copy_field_to_guest(arg, , nr_exchanged) )
>  rc = -EFAULT;
>  return rc;
> +#else /* !CONFIG_PV */
> +return -EOPNOTSUPP;
> +#endif
>  }
>  
>  int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp,

Re: [PATCH] xen/arm: bootfdt: Ignore empty memory bank

[Stefano Stabellini]

On Mon, 21 Sep 2020, Bertrand Marquis wrote:
> > On 18 Sep 2020, at 18:11, Julien Grall  wrote:
> > 
> > From: Julien Grall 
> > 
> > At the moment, Xen will stop processing the Device Tree if a memory
> > bank is empty (size == 0).
> > 
> > Unfortunately, some of the Device Tree (such as on Colibri imx8qxp)
> > may contain such a bank. This means Xen will not be able to boot
> > properly.
> > 
> > Relax the check to just ignore the banks. FWIW this also seems to be the
> > behavior adopted by Linux.
> > 
> > Reported-by: Daniel Wagner 
> > Signed-off-by: Julien Grall 
> 
> Reviewed-by: Bertrand Marquis 

Acked-by: Stefano Stabellini 


> > ---
> > xen/arch/arm/bootfdt.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> > index 08fb59f4e7a9..dcff512648a0 100644
> > --- a/xen/arch/arm/bootfdt.c
> > +++ b/xen/arch/arm/bootfdt.c
> > @@ -163,8 +163,9 @@ static int __init process_memory_node(const void *fdt, 
> > int node,
> > for ( i = 0; i < banks && mem->nr_banks < NR_MEM_BANKS; i++ )
> > {
> > device_tree_get_reg(, address_cells, size_cells, , 
> > );
> > +/* Some DT may describe empty bank, ignore them */
> > if ( !size )
> > -return -EINVAL;
> > +continue;
> > mem->bank[mem->nr_banks].start = start;
> > mem->bank[mem->nr_banks].size = size;
> > mem->nr_banks++;
> > -- 
> > 2.17.1
> > 
> > 
> 

[xen-unstable test] 154576: regressions - FAIL

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-arm64-arm64-xl-seattle   7 xen-boot fail REGR. vs. 154556
 test-amd64-amd64-examine  4 memdisk-try-append   fail REGR. vs. 154556

Regressions which are regarded as allowable (not blocking):
 test-amd64-amd64-xl-rtds 18 guest-localmigrate/x10   fail REGR. vs. 154556

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 154556
 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stopfail like 154556
 test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stopfail like 154556
 test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop fail like 154556
 test-armhf-armhf-libvirt 14 saverestore-support-checkfail  like 154556
 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 154556
 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail  like 154556
 test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stopfail like 154556
 test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop fail like 154556
 test-amd64-i386-xl-pvshim12 guest-start  fail   never pass
 test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt 13 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt  13 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-xsm  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl  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-arm64-arm64-xl-thunderx 13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  14 saverestore-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 14 saverestore-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-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check 
fail 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-rtds 13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 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-amd64-i386-xl-qemut-ws16-amd64 17 guest-stop  fail never pass
 test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail  never pass
 test-armhf-armhf-xl-credit2  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-cubietruck 13 migrate-support-checkfail never pass
 test-armhf-armhf-xl-cubietruck 14 saverestore-support-checkfail never pass
 test-armhf-armhf-libvirt 13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  13 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail   never pass
 test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2  fail never pass

version targeted for testing:
 xen  c7e3021a71fdb4f2d5dbad90ba83ce35bc21cda6
baseline version:
 xen  baa4d064e91b6d2bcfe400bdf71f83b961e4c28e

Last test of basis   154556  2020-09-21 01:52:15 Z0 days
Testing same since   154576  2020-09-21 15:06:28 Z0 days1 attempts


People who touched revisions under test:
  Jan Beulich 
  Roger Pau Monné 
  Trammell Hudson 

jobs:
 build-amd64-xsm

Re: [PATCH] qemu/atomic.h: prefix qemu_ to solve collisions

[Eric Blake]

On 9/21/20 11:23 AM, Stefan Hajnoczi wrote:

clang's C11 atomic_fetch_*() functions only take a C11 atomic type
pointer argument. QEMU uses direct types (int, etc) and this causes a
compiler error when a QEMU code calls these functions in a source file
that also included  via a system header file:

   $ CC=clang CXX=clang++ ./configure ... && make
   ../util/async.c:79:17: error: address argument to atomic operation must be a 
pointer to _Atomic type ('unsigned int *' invalid)

Avoid using atomic_*() names in QEMU's atomic.h since that namespace is
used by . Prefix QEMU's APIs with qemu_ so that atomic.h
and  can co-exist.

This patch was generated using:

   $ git diff | grep -o '\/tmp/changed_identifiers


Missing a step in the recipe: namely, you probably modified 
include/qemu/atomic*.h prior to running 'git diff' (so that you actually 
had input to feed to grep -o).  But spelling it 'git diff HEAD^ 
include/qemu/atomic*.h | ...' does indeed give me a sane list of 
identifiers that looks like what you touched in the rest of the patch.



   $ for identifier in $(

Also not quite the right recipe, based on the file name used in the line 
above.



sed -i "s%\<$identifier\>%qemu_$identifier%" $(git grep -l 
"\<$identifier\>") \
 done



Fortunately, running "git grep -c '\pre-patch state of the tree gives me a list that is somewhat close to 
yours, where the obvious difference in line counts is explained by:



I manually fixed line-wrap issues and misaligned rST tables.

Signed-off-by: Stefan Hajnoczi 
---


First, focusing on the change summary:


  docs/devel/lockcnt.txt|  14 +-
  docs/devel/rcu.txt|  40 +--
  accel/tcg/atomic_template.h   |  20 +-
  include/block/aio-wait.h  |   4 +-
  include/block/aio.h   |   8 +-
  include/exec/cpu_ldst.h   |   2 +-
  include/exec/exec-all.h   |   6 +-
  include/exec/log.h|   6 +-
  include/exec/memory.h |   2 +-
  include/exec/ram_addr.h   |  27 +-
  include/exec/ramlist.h|   2 +-
  include/exec/tb-lookup.h  |   4 +-
  include/hw/core/cpu.h |   2 +-
  include/qemu/atomic.h | 258 +++---
  include/qemu/atomic128.h  |   6 +-


These two are the most important for the sake of this patch; perhaps 
it's worth a temporary override of your git orderfile if you have to 
respin, to list them first?



  include/qemu/bitops.h |   2 +-
  include/qemu/coroutine.h  |   2 +-
  include/qemu/log.h|   6 +-
  include/qemu/queue.h  |   8 +-
  include/qemu/rcu.h|  10 +-
  include/qemu/rcu_queue.h  | 103 +++---


Presumably, this and any other file with an odd number of changes was 
due to a difference in lines after reformatting long lines.



  include/qemu/seqlock.h|   8 +-

...


  util/stats64.c|  34 +-
  docs/devel/atomics.rst| 326 +-
  .../opensbi-riscv32-generic-fw_dynamic.elf| Bin 558668 -> 558698 bytes
  .../opensbi-riscv64-generic-fw_dynamic.elf| Bin 620424 -> 620454 bytes


Why are we regenerating .elf files in this patch?  Is your change even 
correct for those two files?



  scripts/kernel-doc|   2 +-
  tcg/aarch64/tcg-target.c.inc  |   2 +-
  tcg/mips/tcg-target.c.inc |   2 +-
  tcg/ppc/tcg-target.c.inc  |   6 +-
  tcg/sparc/tcg-target.c.inc|   5 +-
  135 files changed, 1195 insertions(+), 1130 deletions(-)


I don't spot accel/tcg/atomic_common.c.inc in the list (which declares 
functions such as atomic_trace_rmw_pre) - I guess that's intentional 
based on how you tried to edit only the identifiers you touched in 
include/qemu/atomic*.h.


For the rest of this patch, I only spot-checked in places, trusting the 
mechanical nature of this patch, and not spotting anything wrong in the 
places I checked.  But the two .elf files worry me enough to withhold 
R-b.  At the same time, because it's big, it will probably be a source 
of conflicts if we don't get it in soon, but can also be regenerated (if 
your recipe is corrected) without too much difficulty.  So I am in favor 
of the idea.



diff --git a/pc-bios/opensbi-riscv32-generic-fw_dynamic.elf 
b/pc-bios/opensbi-riscv32-generic-fw_dynamic.elf
index 
eb9ebf5674d3953ab25de6bf4db134abe904eb20..35b80512446dcf5c49424ae90caacf3c579be1b5
 100644
GIT binary patch
delta 98
zcmX@pqx7mrsiB3jg{g(Pg=Gt?!4uZP)ZEhe?LdZ@5QIJ5?Hg+mgxS918!HgAZQt>Y
ceSHN~X?i|K5fhYsvykI97nHrFhGPaN0Hp#a^8f$<

delta 62

Re: [PATCH 6/6] x86/xen: open code alloc_vm_area in arch_gnttab_valloc

[boris . ostrovsky]

On 9/18/20 12:37 PM, Christoph Hellwig wrote:
>  
> +static int gnttab_apply(pte_t *pte, unsigned long addr, void *data)
> +{
> + pte_t ***p = data;
> +
> + **p = pte;
> + (*p)++;
> + return 0;
> +}
> +
>  static int arch_gnttab_valloc(struct gnttab_vm_area *area, unsigned 
> nr_frames)
>  {
>   area->ptes = kmalloc_array(nr_frames, sizeof(*area->ptes), GFP_KERNEL);
>   if (area->ptes == NULL)
>   return -ENOMEM;
> -
> - area->area = alloc_vm_area(PAGE_SIZE * nr_frames, area->ptes);
> - if (area->area == NULL) {
> - kfree(area->ptes);
> - return -ENOMEM;
> - }
> -
> + area->area = get_vm_area(PAGE_SIZE * nr_frames, VM_IOREMAP);
> + if (!area->area)
> + goto out_free_ptes;
> + if (apply_to_page_range(_mm, (unsigned long)area->area->addr,
> + PAGE_SIZE * nr_frames, gnttab_apply, >ptes))


This will end up incrementing area->ptes pointer. So perhaps something like


pte_t **ptes = area->ptes;

if (apply_to_page_range(_mm, (unsigned long)area->area->addr,
    PAGE_SIZE * nr_frames, gnttab_apply, )) {

   ...

}


-boris

Re: [PATCH] x86/mm: do not mark IO regions as Xen heap

[Roger Pau Monné]

On Mon, Sep 21, 2020 at 04:22:26PM +0200, Jan Beulich wrote:
> On 10.09.2020 15:35, Roger Pau Monne wrote:
> > arch_init_memory will treat all the gaps on the physical memory map
> > between RAM regions as MMIO and use share_xen_page_with_guest in order
> > to assign them to dom_io. This has the side effect of setting the Xen
> > heap flag on such pages, and thus is_special_page would then return
> > true which is an issue in epte_get_entry_emt because such pages will
> > be forced to use write-back cache attributes.
> > 
> > Fix this by introducing a new helper to assign the MMIO regions to
> > dom_io without setting the Xen heap flag on the pages, so that
> > is_special_page will return false and the pages won't be forced to use
> > write-back cache attributes.
> > 
> > Fixes: 81fd0d3ca4b2cd ('x86/hvm: simplify 'mmio_direct' check in 
> > epte_get_entry_emt()')
> 
> Is this really the correct commit? I had this queued for backport,
> and ended up discarding it from the queue based on this tag, just
> to then noticing that I remembered correctly that I did backport
> ca24b2ffdbd9 ("x86/hvm: set 'ipat' in EPT for special pages") to
> the stable trees already. Isn't it _this_ commit which the change
> here fixes?

My bisection pointed to that exact commit as the one that broke my PVH
dom0 setup, so yes, I'm quite sure that's the commit at least on the
box that I've bisected it.

ca24b2ffdbd9 was still fine because previous to the is_special_page
check loop there was still the `if ( direct_mmio ) ...` handling,
which made all MMIO regions except for the APIC access page forcefully
have it's cache attributes set to UC.

Roger.

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

[osstest service owner]

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

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  d4ed1d4132f5825a795d5a78505811ecd2717b5e
baseline version:
 xen  c7e3021a71fdb4f2d5dbad90ba83ce35bc21cda6

Last test of basis   154569  2020-09-21 11:00:24 Z0 days
Testing same since   154581  2020-09-21 17:00:25 Z0 days1 attempts


People who touched revisions under test:
  Jan Beulich 
  Julien Grall 
  Paul Durrant 
  Stefano Stabellini 

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
   c7e3021a71..d4ed1d4132  d4ed1d4132f5825a795d5a78505811ecd2717b5e -> smoke

Re: [PATCH 3/6] drm/i915: use vmap in shmem_pin_map

[Matthew Wilcox]

On Fri, Sep 18, 2020 at 06:37:21PM +0200, Christoph Hellwig wrote:
>  void shmem_unpin_map(struct file *file, void *ptr)
>  {
> + long i = shmem_npages(file);
> +
>   mapping_clear_unevictable(file->f_mapping);
> - __shmem_unpin_map(file, ptr, shmem_npte(file));
> + vunmap(ptr);
> +
> + for (i = 0; i < shmem_npages(file); i++) {
> + struct page *page;
> +
> + page = shmem_read_mapping_page_gfp(file->f_mapping, i,
> +GFP_KERNEL);
> + if (!WARN_ON(IS_ERR(page))) {
> + put_page(page);
> + put_page(page);
> + }
> + }
>  }

This is awkward.  I'd like it if we had a vfree() variant which called
put_page() instead of __free_pages().  I'd like it even more if we
used release_pages() instead of our own loop that called put_page().

Perhaps something like this ...

+++ b/mm/vmalloc.c
@@ -2262,7 +2262,7 @@ static void __vunmap(const void *addr, int deallocate_page
s)
 
vm_remove_mappings(area, deallocate_pages);
 
-   if (deallocate_pages) {
+   if (deallocate_pages == 1) {
int i;
 
for (i = 0; i < area->nr_pages; i++) {
@@ -2271,8 +2271,12 @@ static void __vunmap(const void *addr, int 
deallocate_pages)
BUG_ON(!page);
__free_pages(page, 0);
}
-   atomic_long_sub(area->nr_pages, _vmalloc_pages);
+   } else if (deallocate_pages == 2) {
+   release_pages(area->pages, area->nr_pages);
+   }
 
+   if (deallocate_pages) {
+   atomic_long_sub(area->nr_pages, _vmalloc_pages);
kvfree(area->pages);
}
@@ -2369,6 +2373,14 @@ void vunmap(const void *addr)
 }
 EXPORT_SYMBOL(vunmap);
 
+void vunmap_put_pages(const void *addr)
+{
+   BUG_ON(in_interrupt());
+   might_sleep();
+   if (addr)
+   __vunmap(addr, 2);
+}
+
 /**
  * vmap - map an array of pages into virtually contiguous space
  * @pages: array of page pointers

only with kernel-doc and so on.  I bet somebody has a better idea for a name.

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

[Oleksandr]

On 14.09.20 17:17, Jan Beulich wrote:

Hi Jan


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.
Last time when trying to make something inline in arch files for the RFC 
series (I don't remember exactly for what it was)
I got completely stuck with the build issues due to the header 
(inter-?)dependencies, which I failed to resolve).

Anyway I got your point and will try to experiment with that.





--- 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.


We can leave only #include  here and move #include 
 to x86/hvm/ioreq.c.

Also #include  could be dropped.





--- /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.


ok





+#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?
I think, just in few specific .c files. Which are x86/hvm/ioreq.c and 
common/ioreq.c now and several other files later on (x86/hvm/dm.c, 
arm/io.c, etc)

Shall I include that header in these files instead?





+#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.
Got it. The only reason why GET_IOREQ_SERVER is here is inline 
get_ioreq_server(). I will make it non-inline and move both to 
common/ioreq.c.
I assume this layering violation issue applies for 
hvm_domain_has_ioreq_server() introduced in

[PATCH V1 11/16] xen/ioreq: Introduce hvm_domain_has_ioreq_server()


--
Regards,

Oleksandr Tyshchenko

[qemu-mainline test] 154566: regressions - FAIL

[osstest service owner]

flight 154566 qemu-mainline real [real]
http://logs.test-lab.xenproject.org/osstest/logs/154566/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-amd64-xl-qcow210 debian-di-installfail REGR. vs. 152631
 test-amd64-i386-xl-qemuu-dmrestrict-amd64-dmrestrict 10 debian-hvm-install 
fail REGR. vs. 152631
 test-amd64-amd64-xl-qemuu-win7-amd64 10 windows-install  fail REGR. vs. 152631
 test-amd64-amd64-qemuu-nested-intel 10 debian-hvm-install fail REGR. vs. 152631
 test-amd64-amd64-xl-qemuu-ovmf-amd64 10 debian-hvm-install fail REGR. vs. 
152631
 test-amd64-amd64-xl-qemuu-debianhvm-amd64-shadow 10 debian-hvm-install fail 
REGR. vs. 152631
 test-amd64-amd64-xl-qemuu-dmrestrict-amd64-dmrestrict 10 debian-hvm-install 
fail REGR. vs. 152631
 test-amd64-i386-xl-qemuu-win7-amd64 10 windows-install   fail REGR. vs. 152631
 test-amd64-i386-xl-qemuu-debianhvm-i386-xsm 10 debian-hvm-install fail REGR. 
vs. 152631
 test-amd64-amd64-qemuu-nested-amd 10 debian-hvm-install  fail REGR. vs. 152631
 test-amd64-amd64-xl-qemuu-ws16-amd64 10 windows-install  fail REGR. vs. 152631
 test-amd64-amd64-xl-qemuu-debianhvm-i386-xsm 10 debian-hvm-install fail REGR. 
vs. 152631
 test-amd64-amd64-xl-qemuu-debianhvm-amd64 10 debian-hvm-install fail REGR. vs. 
152631
 test-amd64-i386-qemuu-rhel6hvm-amd 10 redhat-install fail REGR. vs. 152631
 test-amd64-i386-xl-qemuu-debianhvm-amd64 10 debian-hvm-install fail REGR. vs. 
152631
 test-amd64-i386-xl-qemuu-ws16-amd64 10 windows-install   fail REGR. vs. 152631
 test-amd64-i386-qemuu-rhel6hvm-intel 10 redhat-install   fail REGR. vs. 152631
 test-amd64-i386-xl-qemuu-ovmf-amd64 10 debian-hvm-install fail REGR. vs. 152631
 test-amd64-i386-xl-qemuu-debianhvm-amd64-shadow 10 debian-hvm-install fail 
REGR. vs. 152631
 test-arm64-arm64-libvirt-xsm 12 guest-start  fail REGR. vs. 152631
 test-amd64-amd64-libvirt-vhd 11 guest-start  fail REGR. vs. 152631
 test-armhf-armhf-libvirt 12 guest-start  fail REGR. vs. 152631
 test-armhf-armhf-libvirt-raw 10 debian-di-installfail REGR. vs. 152631
 test-armhf-armhf-xl-vhd  10 debian-di-installfail REGR. vs. 152631

Tests which are failing intermittently (not blocking):
 test-amd64-amd64-qemuu-freebsd11-amd64 19 guest-start/freebsd.repeat fail pass 
in 154552

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-xl-rtds 16 guest-start/debian.repeat fail in 154552 like 
152631
 test-amd64-i386-xl-pvshim12 guest-start  fail   never pass
 test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt 13 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-xsm  13 migrate-support-checkfail   never pass
 test-amd64-i386-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-arm64-arm64-xl-xsm  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl  14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 13 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check 
fail never pass
 test-arm64-arm64-xl-thunderx 14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  14 saverestore-support-checkfail   never pass
 test-amd64-i386-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-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  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail  never pass
 test-armhf-armhf-xl-credit2  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-cubietruck 13 migrate-support-check

Re: [PATCH 1/6] zsmalloc: switch from alloc_vm_area to get_vm_area

[Christoph Hellwig]

On Mon, Sep 21, 2020 at 11:42:29AM -0700, Minchan Kim wrote:
> It seems zsmalloc is only customer the function so let's have the helper
> when we see another customer.
> 
> If we don't have objection, I'd like to ask to Andrew fold this up.

Fine with me.

Re: [PATCH 1/6] zsmalloc: switch from alloc_vm_area to get_vm_area

[Minchan Kim]

On Mon, Sep 21, 2020 at 08:17:08PM +0200, Christoph Hellwig wrote:
> On Mon, Sep 21, 2020 at 10:42:56AM -0700, Minchan Kim wrote:
> > IIRC, the problem was runtime pte popluating needs GFP_KERNEL but
> > zs_map_object API runs under non-preemtible section.
> 
> Make sense.
> 
> > > - area->vm = alloc_vm_area(PAGE_SIZE * 2, NULL);
> > > + area->vm = get_vm_area(PAGE_SIZE * 2, 0);
> > >   if (!area->vm)
> > >   return -ENOMEM;
> > >   return 0;
> > 
> > I think it shoud work.
> > 
> > diff --git a/mm/memory.c b/mm/memory.c
> > index 05789aa4af12..6a1e4d854593 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -2232,7 +2232,7 @@ static int apply_to_pte_range(struct mm_struct *mm, 
> > pmd_t *pmd,
> > arch_enter_lazy_mmu_mode();
> >  
> > do {
> > -   if (create || !pte_none(*pte)) {
> > +   if ((create || !pte_none(*pte)) && fn) {
> > err = fn(pte++, addr, data);
> > if (err)
> > break;
> > diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
> > index 3e4fe3259612..9ef7daf3d279 100644
> > --- a/mm/zsmalloc.c
> > +++ b/mm/zsmalloc.c
> > @@ -1116,6 +1116,8 @@ static struct zspage *find_get_zspage(struct 
> > size_class *class)
> >  #ifdef CONFIG_ZSMALLOC_PGTABLE_MAPPING
> >  static inline int __zs_cpu_up(struct mapping_area *area)
> >  {
> > +   int ret;
> > +
> > /*
> >  * Make sure we don't leak memory if a cpu UP notification
> >  * and zs_init() race and both call zs_cpu_up() on the same cpu
> > @@ -1125,7 +1127,13 @@ static inline int __zs_cpu_up(struct mapping_area 
> > *area)
> > area->vm = get_vm_area(PAGE_SIZE * 2, 0);
> > if (!area->vm)
> > return -ENOMEM;
> > -   return 0;
> > +
> > +   /*
> > +* Populate ptes in advance to avoid pte allocation with GFP_KERNEL
> > +* in non-preemtible context of zs_map_object.
> > +*/
> > +   ret = apply_to_page_range(_mm, NULL, PAGE_SIZE * 2, NULL, NULL);
> > +   return ret;
> 
> I think this needs the addr from the vm area somewhere..

Yeah, let's assign the addres we got get_vm_area.

> 
> We probaby want to add a trivial helper to prefault an area instead of
> the open coded variant.

It seems zsmalloc is only customer the function so let's have the helper
when we see another customer.

If we don't have objection, I'd like to ask to Andrew fold this up.

---
 mm/memory.c   | 2 +-
 mm/zsmalloc.c | 8 +++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/mm/memory.c b/mm/memory.c
index 05789aa4af12..6a1e4d854593 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -2232,7 +2232,7 @@ static int apply_to_pte_range(struct mm_struct *mm, pmd_t 
*pmd,
arch_enter_lazy_mmu_mode();
 
do {
-   if (create || !pte_none(*pte)) {
+   if ((create || !pte_none(*pte)) && fn) {
err = fn(pte++, addr, data);
if (err)
break;
diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index 3e4fe3259612..918c7b019b3d 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -1125,7 +1125,13 @@ static inline int __zs_cpu_up(struct mapping_area *area)
area->vm = get_vm_area(PAGE_SIZE * 2, 0);
if (!area->vm)
return -ENOMEM;
-   return 0;
+
+   /*
+* Populate ptes in advance to avoid pte allocation with GFP_KERNEL
+* in non-preemtible context of zs_map_object.
+*/
+   return apply_to_page_range(_mm, (unsigned long)area->vm->addr,
+   PAGE_SIZE * 2, NULL, NULL);
 }
 
 static inline void __zs_cpu_down(struct mapping_area *area)
-- 
2.28.0.681.g6f77f65b4e-goog

Re: [PATCH 1/6] zsmalloc: switch from alloc_vm_area to get_vm_area

[Christoph Hellwig]

On Mon, Sep 21, 2020 at 10:42:56AM -0700, Minchan Kim wrote:
> IIRC, the problem was runtime pte popluating needs GFP_KERNEL but
> zs_map_object API runs under non-preemtible section.

Make sense.

> > -   area->vm = alloc_vm_area(PAGE_SIZE * 2, NULL);
> > +   area->vm = get_vm_area(PAGE_SIZE * 2, 0);
> > if (!area->vm)
> > return -ENOMEM;
> > return 0;
> 
> I think it shoud work.
> 
> diff --git a/mm/memory.c b/mm/memory.c
> index 05789aa4af12..6a1e4d854593 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -2232,7 +2232,7 @@ static int apply_to_pte_range(struct mm_struct *mm, 
> pmd_t *pmd,
>   arch_enter_lazy_mmu_mode();
>  
>   do {
> - if (create || !pte_none(*pte)) {
> + if ((create || !pte_none(*pte)) && fn) {
>   err = fn(pte++, addr, data);
>   if (err)
>   break;
> diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
> index 3e4fe3259612..9ef7daf3d279 100644
> --- a/mm/zsmalloc.c
> +++ b/mm/zsmalloc.c
> @@ -1116,6 +1116,8 @@ static struct zspage *find_get_zspage(struct size_class 
> *class)
>  #ifdef CONFIG_ZSMALLOC_PGTABLE_MAPPING
>  static inline int __zs_cpu_up(struct mapping_area *area)
>  {
> + int ret;
> +
>   /*
>* Make sure we don't leak memory if a cpu UP notification
>* and zs_init() race and both call zs_cpu_up() on the same cpu
> @@ -1125,7 +1127,13 @@ static inline int __zs_cpu_up(struct mapping_area 
> *area)
>   area->vm = get_vm_area(PAGE_SIZE * 2, 0);
>   if (!area->vm)
>   return -ENOMEM;
> - return 0;
> +
> + /*
> +  * Populate ptes in advance to avoid pte allocation with GFP_KERNEL
> +  * in non-preemtible context of zs_map_object.
> +  */
> + ret = apply_to_page_range(_mm, NULL, PAGE_SIZE * 2, NULL, NULL);
> + return ret;

I think this needs the addr from the vm area somewhere..

We probaby want to add a trivial helper to prefault an area instead of
the open coded variant.

Re: [PATCH] xen/mm: Introduce CONFIG_M2P and provide common fallback logic

[Julien Grall]

Hi Jan,

On 25/08/2020 08:40, Jan Beulich wrote:

On 24.08.2020 20:15, Andrew Cooper wrote:

I'm pretty sure the mfn_to_gmfn() stub is bogus before and after this change.
The two uses in common code are getdomaininfo and in memory_exchange(), which
result in junk.


It's been a long time back that I think I did suggest to restrict
memory_exchange() to non-translated guests. I don't recall what
the arguments against this were, but I'm quite sure it wasn't
merely "it alters the ABI for such guests".


This was just a low priority for me. But I revived the series (see [1]).

Cheers,

[1] <20200921180214.4842-1-jul...@xen.org>

--
Julien Grall

[PATCH v4 0/4] xen/arm: Properly disable M2P on Arm

[Julien Grall]

From: Julien Grall 

Hi all,

Arm never supported a M2P yet there are some helpers implemented to deal with
the common code. However, the implementation of mfn_to_gmfn is completely
bogus.

This series aims to properly disable the M2P on Arm. See patch #2 for the
rationale regarding the lack of M2P on Arm.

I have dropped the typesafe patches from this series to focus on just the M2P.

Cheers,

Julien Grall (4):
  xen: XENMEM_exchange should only be used/compiled for arch supporting
PV guest
  xen: Introduce HAS_M2P config and use to protect mfn_to_gmfn call
  xen: Remove mfn_to_gmfn macro
  xen/mm: Provide dummy M2P-related helpers when !CONFIG_HAVE_M2P

 xen/arch/x86/Kconfig |  1 +
 xen/common/Kconfig   |  3 +++
 xen/common/domctl.c  |  9 +++--
 xen/common/memory.c  |  7 +++
 xen/include/asm-arm/domain.h |  5 +
 xen/include/asm-arm/mm.h | 13 -
 xen/include/asm-x86/mm.h |  5 -
 xen/include/public/domctl.h  |  6 ++
 xen/include/xen/domain.h | 12 
 xen/include/xen/mm.h | 13 +
 10 files changed, 54 insertions(+), 20 deletions(-)

-- 
2.17.1

[PATCH v4 2/4] xen: Introduce HAS_M2P config and use to protect mfn_to_gmfn call

[Julien Grall]

From: Julien Grall 

While Arm never had a M2P, the implementation of mfn_to_gmfn is pretty
bogus as we directly return the MFN passed in parameter.

Thankfully, the use of mfn_to_gmfn is pretty limited on Arm today. There
are only 2 callers in the common code:
- memory_exchange: Arm cannot not use it because steal_page is not
implemented. Note this is already protected by !CONFIG_PV.
- getdomaininfo: Toolstack may map the shared page. It looks like
this is mostly used for mapping the P2M of PV guest. Therefore the
issue might be minor.

Implementing the M2P on Arm is not planned. The M2P would require significant
amount of VA address (very tough on 32-bit) that can hardly be justified with
the current use of mfn_to_gmfn.
- memory_exchange: This does not work and I haven't seen any
request for it so far.
- getdomaininfo: The structure on Arm does not seem to contain a lot
of useful information on Arm. It is unclear whether we want to
allow the toolstack mapping it on Arm.

This patch introduces a config option HAS_M2P to tell whether an
architecture implements the M2P.
- memory_exchange: This is PV only (not supported on Arm) but take
the opportunity to gate with HAS_M2P as well in case someone decide
to implement PV without M2P.
- getdomaininfo: A new helper is introduced to wrap the call to
mfn_to_gfn/mfn_to_gmfn. For Arm, a fixed value will be provided that will
fail on mapping if used.

Signed-off-by Julien Grall 

---
Changes in v4:
- The IOMMU code doesn't use the M2P anymore, so this can be
dropped.
- Reword the commit message
- Fix rebase conflict

Changes in v3:
- Move the BUG_ON() in domain_shared_info_gfn()
- Use a fixed value when the field shared_info_frame is not
supported.
- Add an ASSERT_UNREACHABLE in iommu_hwdom_init + move printk
within the #ifdef.

Changes in v2:
- Add a warning in public headers
- Constify local variable in domain_shared_info_gfn
- Invert the naming (_d / d) in domain_shared_info_gfn
- Use -EOPNOTSUPP rather than -ENOSYS
- Rework how the memory_exchange hypercall is removed from Arm
---
 xen/arch/x86/Kconfig |  1 +
 xen/common/Kconfig   |  3 +++
 xen/common/domctl.c  |  9 +++--
 xen/common/memory.c  |  4 ++--
 xen/include/asm-arm/domain.h |  5 +
 xen/include/public/domctl.h  |  6 ++
 xen/include/xen/domain.h | 12 
 7 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index a636a4bb1e69..ad92e756 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -16,6 +16,7 @@ config X86
select HAS_IOPORTS
select HAS_KEXEC
select MEM_ACCESS_ALWAYS_ON
+   select HAS_M2P
select HAS_MEM_PAGING
select HAS_NS16550
select HAS_PASSTHROUGH
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 15e3b79ff57f..b6b4db92d700 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -63,6 +63,9 @@ config HAS_IOPORTS
 config HAS_SCHED_GRANULARITY
bool
 
+config HAS_M2P
+   bool
+
 config NEEDS_LIBELF
bool
 
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 5ac6e9c5cafa..2bf3e6659018 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -68,6 +68,7 @@ void getdomaininfo(struct domain *d, struct 
xen_domctl_getdomaininfo *info)
 u64 cpu_time = 0;
 int flags = XEN_DOMINF_blocked;
 struct vcpu_runstate_info runstate;
+gfn_t shared_info_frame;
 
 info->domain = d->domain_id;
 info->max_vcpu_id = XEN_INVALID_MAX_VCPU_ID;
@@ -111,8 +112,12 @@ void getdomaininfo(struct domain *d, struct 
xen_domctl_getdomaininfo *info)
 info->outstanding_pages = d->outstanding_pages;
 info->shr_pages = atomic_read(>shr_pages);
 info->paged_pages   = atomic_read(>paged_pages);
-info->shared_info_frame = mfn_to_gmfn(d, virt_to_mfn(d->shared_info));
-BUG_ON(SHARED_M2P(info->shared_info_frame));
+
+shared_info_frame = domain_shared_info_gfn(d);
+if ( gfn_eq(shared_info_frame, INVALID_GFN) )
+info->shared_info_frame = XEN_INVALID_SHARED_INFO_FRAME;
+else
+info->shared_info_frame = gfn_x(shared_info_frame);
 
 info->cpupool = cpupool_get_id(d);
 
diff --git a/xen/common/memory.c b/xen/common/memory.c
index 9300104943b0..c698e6872596 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -504,7 +504,7 @@ static bool propagate_node(unsigned int xmf, unsigned int 
*memflags)
 
 static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
 {
-#ifdef CONFIG_PV
+#if defined(CONFIG_PV) && defined(CONFIG_M2P)
 struct xen_memory_exchange exch;
 PAGE_LIST_HEAD(in_chunk_list);
 PAGE_LIST_HEAD(out_chunk_list);
@@ -801,7 +801,7 @@ static long 
memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
  

[PATCH v4 4/4] xen/mm: Provide dummy M2P-related helpers when !CONFIG_HAVE_M2P

[Julien Grall]

From: Julien Grall 

At the moment, Arm is providing a dummy implementation for the M2P
helpers used in common code. However, they are quite isolated and could
be used by other architecture in the future. So move all the helpers in
xen/mm.h.

Signed-off-by: Julien Grall 

---
Changes in v4:
- The tags were dropped as the previous version was sent a long
time ago.

Changes in v3:
- Add Stefano's reviewed-by
- Add George's acked-by

Changes in v2:
- Patch added
---
 xen/include/asm-arm/mm.h | 11 ---
 xen/include/xen/mm.h | 13 +
 2 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index 29489a3e1076..5929201d0299 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -318,17 +318,6 @@ static inline void *page_to_virt(const struct page_info 
*pg)
 struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va,
 unsigned long flags);
 
-/*
- * Arm does not have an M2P, but common code expects a handful of
- * M2P-related defines and functions. Provide dummy versions of these.
- */
-#define INVALID_M2P_ENTRY(~0UL)
-#define SHARED_M2P_ENTRY (~0UL - 1UL)
-#define SHARED_M2P(_e)   ((_e) == SHARED_M2P_ENTRY)
-
-/* We don't have a M2P on Arm */
-#define set_gpfn_from_mfn(mfn, pfn) do { (void) (mfn), (void)(pfn); } while (0)
-
 /* Arch-specific portion of memory_op hypercall. */
 long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg);
 
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 4536a62940a1..15bb0aa30d22 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -685,4 +685,17 @@ static inline void put_page_alloc_ref(struct page_info 
*page)
 }
 }
 
+/*
+ * Dummy implementation of M2P-related helpers for common code when
+ * the architecture doesn't have an M2P.
+ */
+#ifndef CONFIG_HAS_M2P
+
+#define INVALID_M2P_ENTRY(~0UL)
+#define SHARED_M2P(_e)   false
+
+static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned long pfn) {}
+
+#endif
+
 #endif /* __XEN_MM_H__ */
-- 
2.17.1

[PATCH v4 3/4] xen: Remove mfn_to_gmfn macro

[Julien Grall]

From: Julien Grall 

On x86, mfn_to_gmfn can be replaced with mfn_to_gfn. On Arm, there are
no more call to mfn_to_gmfn, so the helper can be dropped.

At the same time rework a comment in Arm code that does not make sense.

Signed-off-by: Julien Grall 

---
Changes in v4:
- Remove acks as the patch is old

Changes in v2:
- Add Jan's and Stefano's acked-by
---
 xen/include/asm-arm/mm.h | 4 +---
 xen/include/asm-x86/mm.h | 5 -
 2 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index f8ba49b1188f..29489a3e1076 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -326,10 +326,8 @@ struct page_info *get_page_from_gva(struct vcpu *v, 
vaddr_t va,
 #define SHARED_M2P_ENTRY (~0UL - 1UL)
 #define SHARED_M2P(_e)   ((_e) == SHARED_M2P_ENTRY)
 
-/* Xen always owns P2M on ARM */
+/* We don't have a M2P on Arm */
 #define set_gpfn_from_mfn(mfn, pfn) do { (void) (mfn), (void)(pfn); } while (0)
-#define mfn_to_gmfn(_d, mfn)  (mfn)
-
 
 /* Arch-specific portion of memory_op hypercall. */
 long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg);
diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index deeba75a1cbb..dfa71ce15ac3 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -508,11 +508,6 @@ extern struct rangeset *mmio_ro_ranges;
 
 #define get_gpfn_from_mfn(mfn)  (machine_to_phys_mapping[(mfn)])
 
-#define mfn_to_gmfn(_d, mfn)\
-( (paging_mode_translate(_d))   \
-  ? get_gpfn_from_mfn(mfn)  \
-  : (mfn) )
-
 #define compat_pfn_to_cr3(pfn) (((unsigned)(pfn) << 12) | ((unsigned)(pfn) >> 
20))
 #define compat_cr3_to_pfn(cr3) (((unsigned)(cr3) >> 12) | ((unsigned)(cr3) << 
20))
 
-- 
2.17.1

[PATCH v4 1/4] xen: XENMEM_exchange should only be used/compiled for arch supporting PV guest

[Julien Grall]

From: Julien Grall 

XENMEM_exchange can only be used by PV guest but the check is well
hidden in steal_page(). This is because paging_model_external() will
return false only for PV domain.

To make clearer this is PV only, add a check at the beginning of the
implementation. Take the opportunity to compile out the code if
CONFIG_PV is not set.

This change will also help a follow-up patch where the gmfn_mfn() will
be completely removed on arch not supporting the M2P.

Signed-off-by: Julien Grall 

---

Jan suggested to #ifdef anything after the check to is_pv_domain().
However, it means to have two block of #ifdef as we can't mix
declaration and code.

I am actually thinking to move the implementation outside of mm.c in
possibly arch/x86 or a pv specific directory under common. Any opinion?

Changes in v4:
- Patch added
---
 xen/common/memory.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/xen/common/memory.c b/xen/common/memory.c
index 714077c1e597..9300104943b0 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -504,6 +504,7 @@ static bool propagate_node(unsigned int xmf, unsigned int 
*memflags)
 
 static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
 {
+#ifdef CONFIG_PV
 struct xen_memory_exchange exch;
 PAGE_LIST_HEAD(in_chunk_list);
 PAGE_LIST_HEAD(out_chunk_list);
@@ -516,6 +517,9 @@ static long 
memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
 struct domain *d;
 struct page_info *page;
 
+if ( !is_pv_domain(d) )
+return -EOPNOTSUPP;
+
 if ( copy_from_guest(, arg, 1) )
 return -EFAULT;
 
@@ -797,6 +801,9 @@ static long 
memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
 if ( __copy_field_to_guest(arg, , nr_exchanged) )
 rc = -EFAULT;
 return rc;
+#else /* !CONFIG_PV */
+return -EOPNOTSUPP;
+#endif
 }
 
 int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp,
-- 
2.17.1

[linux-linus test] 154559: regressions - FAIL

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-i386-qemut-rhel6hvm-intel  6 xen-install  fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-dmrestrict-amd64-dmrestrict 6 xen-install fail REGR. 
vs. 152332
 test-amd64-i386-xl-qemuu-ws16-amd64  6 xen-install   fail REGR. vs. 152332
 test-amd64-i386-xl-xsm6 xen-install  fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-debianhvm-amd64-shadow 6 xen-install fail REGR. vs. 
152332
 test-amd64-i386-qemuu-rhel6hvm-intel  6 xen-install  fail REGR. vs. 152332
 test-amd64-i386-xl-qemut-debianhvm-amd64  6 xen-install  fail REGR. vs. 152332
 test-amd64-i386-examine   6 xen-install  fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-debianhvm-i386-xsm 6 xen-install fail REGR. vs. 152332
 test-amd64-i386-libvirt   6 xen-install  fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-debianhvm-amd64  6 xen-install  fail REGR. vs. 152332
 test-amd64-i386-qemut-rhel6hvm-amd  6 xen-installfail REGR. vs. 152332
 test-amd64-i386-xl-qemut-ws16-amd64  6 xen-install   fail REGR. vs. 152332
 test-amd64-i386-pair  8 xen-install/src_host fail REGR. vs. 152332
 test-amd64-i386-qemuu-rhel6hvm-amd  6 xen-installfail REGR. vs. 152332
 test-amd64-i386-pair  9 xen-install/dst_host fail REGR. vs. 152332
 test-amd64-i386-xl6 xen-install  fail REGR. vs. 152332
 test-amd64-i386-libvirt-xsm   6 xen-install  fail REGR. vs. 152332
 test-amd64-coresched-i386-xl  6 xen-install  fail REGR. vs. 152332
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 6 xen-install fail REGR. vs. 
152332
 test-amd64-i386-xl-pvshim 6 xen-install  fail REGR. vs. 152332
 test-amd64-i386-freebsd10-amd64  6 xen-install   fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-win7-amd64  6 xen-install   fail REGR. vs. 152332
 test-amd64-i386-xl-qemut-debianhvm-i386-xsm 6 xen-install fail REGR. vs. 152332
 test-amd64-i386-xl-shadow 6 xen-install  fail REGR. vs. 152332
 test-amd64-i386-freebsd10-i386  6 xen-installfail REGR. vs. 152332
 test-amd64-i386-xl-raw6 xen-install  fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-ovmf-amd64  6 xen-install   fail REGR. vs. 152332
 test-amd64-i386-xl-qemut-win7-amd64  6 xen-install   fail REGR. vs. 152332
 test-amd64-i386-libvirt-pair  8 xen-install/src_host fail REGR. vs. 152332
 test-amd64-i386-libvirt-pair  9 xen-install/dst_host fail REGR. vs. 152332
 test-amd64-i386-xl-qemut-stubdom-debianhvm-amd64-xsm 6 xen-install 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-armhf-armhf-libvirt 14 saverestore-support-checkfail  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-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-credit2  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  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-arm64-arm64-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 14 saverestore-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-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-arm64-arm64-xl-thunderx 13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 14 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-vhd 12 migrate-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 

Re: [PATCH 1/6] zsmalloc: switch from alloc_vm_area to get_vm_area

[Minchan Kim]

On Fri, Sep 18, 2020 at 06:37:19PM +0200, Christoph Hellwig wrote:
> There is no obvious reason why zsmalloc needs to pre-fault the PTEs
> given that it later uses map_kernel_range to just like vmap().

IIRC, the problem was runtime pte popluating needs GFP_KERNEL but
zs_map_object API runs under non-preemtible section.

> 
> Signed-off-by: Christoph Hellwig 
> ---
>  mm/zsmalloc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
> index c36fdff9a37131..3e4fe3259612fd 100644
> --- a/mm/zsmalloc.c
> +++ b/mm/zsmalloc.c
> @@ -1122,7 +1122,7 @@ static inline int __zs_cpu_up(struct mapping_area *area)
>*/
>   if (area->vm)
>   return 0;
> - area->vm = alloc_vm_area(PAGE_SIZE * 2, NULL);
> + area->vm = get_vm_area(PAGE_SIZE * 2, 0);
>   if (!area->vm)
>   return -ENOMEM;
>   return 0;

I think it shoud work.

diff --git a/mm/memory.c b/mm/memory.c
index 05789aa4af12..6a1e4d854593 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -2232,7 +2232,7 @@ static int apply_to_pte_range(struct mm_struct *mm, pmd_t 
*pmd,
arch_enter_lazy_mmu_mode();
 
do {
-   if (create || !pte_none(*pte)) {
+   if ((create || !pte_none(*pte)) && fn) {
err = fn(pte++, addr, data);
if (err)
break;
diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index 3e4fe3259612..9ef7daf3d279 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -1116,6 +1116,8 @@ static struct zspage *find_get_zspage(struct size_class 
*class)
 #ifdef CONFIG_ZSMALLOC_PGTABLE_MAPPING
 static inline int __zs_cpu_up(struct mapping_area *area)
 {
+   int ret;
+
/*
 * Make sure we don't leak memory if a cpu UP notification
 * and zs_init() race and both call zs_cpu_up() on the same cpu
@@ -1125,7 +1127,13 @@ static inline int __zs_cpu_up(struct mapping_area *area)
area->vm = get_vm_area(PAGE_SIZE * 2, 0);
if (!area->vm)
return -ENOMEM;
-   return 0;
+
+   /*
+* Populate ptes in advance to avoid pte allocation with GFP_KERNEL
+* in non-preemtible context of zs_map_object.
+*/
+   ret = apply_to_page_range(_mm, NULL, PAGE_SIZE * 2, NULL, NULL);
+   return ret;
 }
 
 static inline void __zs_cpu_down(struct mapping_area *area)

[PATCH] x86: Use LOCK ADD instead of MFENCE for smp_mb()

[Andrew Cooper]

MFENCE is overly heavyweight for SMP semantics on WB memory, because it also
orders weaker cached writes, and flushes the WC buffers.

This technique was used as an optimisation in Java[1], and later adopted by
Linux[2] where it was measured to have a 60% performance improvement in VirtIO
benchmarks.

The stack is used because it is hot in the L1 cache, and a -4 offset is used
to avoid creating a false data dependency on live data.  (For 64bit userspace,
the offset needs to be under the red zone to avoid false dependences).

Fix up the 32 bit definitions in HVMLoader and libxc to avoid a false data
dependency.

[1] https://shipilev.net/blog/2014/on-the-fence-with-dependencies/
[2] https://git.kernel.org/torvalds/c/450cbdd0125cfa5d7bbf9e2a6b6961cc48d29730

Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 
CC: Roger Pau Monné 
CC: Wei Liu 
CC: Ian Jackson 
---
 tools/firmware/hvmloader/util.h   | 2 +-
 tools/libs/ctrl/include/xenctrl.h | 4 ++--
 xen/include/asm-x86/system.h  | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
index 31889de634..4f0baade0e 100644
--- a/tools/firmware/hvmloader/util.h
+++ b/tools/firmware/hvmloader/util.h
@@ -133,7 +133,7 @@ static inline void cpu_relax(void)
 #define barrier() asm volatile ( "" : : : "memory" )
 #define rmb() barrier()
 #define wmb() barrier()
-#define mb()  asm volatile ( "lock; addl $0,0(%%esp)" : : : "memory" )
+#define mb()  asm volatile ( "lock addl $0, -4(%%esp)" ::: "memory" )
 
 /*
  * Divide a 64-bit dividend by a 32-bit divisor.
diff --git a/tools/libs/ctrl/include/xenctrl.h 
b/tools/libs/ctrl/include/xenctrl.h
index 73e9535fc8..1d9f514302 100644
--- a/tools/libs/ctrl/include/xenctrl.h
+++ b/tools/libs/ctrl/include/xenctrl.h
@@ -68,11 +68,11 @@
 #define xen_barrier() asm volatile ( "" : : : "memory")
 
 #if defined(__i386__)
-#define xen_mb()  asm volatile ( "lock; addl $0,0(%%esp)" : : : "memory" )
+#define xen_mb()  asm volatile ( "lock addl $0, -4(%%esp)" ::: "memory" )
 #define xen_rmb() xen_barrier()
 #define xen_wmb() xen_barrier()
 #elif defined(__x86_64__)
-#define xen_mb()  asm volatile ( "mfence" : : : "memory")
+#define xen_mb()  asm volatile ( "lock addl $0, -128(%%rsp)" ::: "memory" )
 #define xen_rmb() xen_barrier()
 #define xen_wmb() xen_barrier()
 #elif defined(__arm__)
diff --git a/xen/include/asm-x86/system.h b/xen/include/asm-x86/system.h
index 7e5891f3df..6474dd1243 100644
--- a/xen/include/asm-x86/system.h
+++ b/xen/include/asm-x86/system.h
@@ -226,7 +226,7 @@ static always_inline unsigned long __xadd(
  *
  * Refer to the vendor system programming manuals for further details.
  */
-#define smp_mb()mb()
+#define smp_mb()asm volatile ( "lock addl $0, -4(%%rsp)" ::: "memory" )
 #define smp_rmb()   barrier()
 #define smp_wmb()   barrier()
 
-- 
2.11.0

Re: [PATCH] x86/mm: do not mark IO regions as Xen heap

[Jan Beulich]

On 21.09.2020 17:35, Roger Pau Monné wrote:
> On Mon, Sep 21, 2020 at 04:22:26PM +0200, Jan Beulich wrote:
>> On 10.09.2020 15:35, Roger Pau Monne wrote:
>>> arch_init_memory will treat all the gaps on the physical memory map
>>> between RAM regions as MMIO and use share_xen_page_with_guest in order
>>> to assign them to dom_io. This has the side effect of setting the Xen
>>> heap flag on such pages, and thus is_special_page would then return
>>> true which is an issue in epte_get_entry_emt because such pages will
>>> be forced to use write-back cache attributes.
>>>
>>> Fix this by introducing a new helper to assign the MMIO regions to
>>> dom_io without setting the Xen heap flag on the pages, so that
>>> is_special_page will return false and the pages won't be forced to use
>>> write-back cache attributes.
>>>
>>> Fixes: 81fd0d3ca4b2cd ('x86/hvm: simplify 'mmio_direct' check in 
>>> epte_get_entry_emt()')
>>
>> Is this really the correct commit? I had this queued for backport,
>> and ended up discarding it from the queue based on this tag, just
>> to then noticing that I remembered correctly that I did backport
>> ca24b2ffdbd9 ("x86/hvm: set 'ipat' in EPT for special pages") to
>> the stable trees already. Isn't it _this_ commit which the change
>> here fixes?
> 
> My bisection pointed to that exact commit as the one that broke my PVH
> dom0 setup, so yes, I'm quite sure that's the commit at least on the
> box that I've bisected it.
> 
> ca24b2ffdbd9 was still fine because previous to the is_special_page
> check loop there was still the `if ( direct_mmio ) ...` handling,
> which made all MMIO regions except for the APIC access page forcefully
> have it's cache attributes set to UC.

Ah yes, I see - thanks. Makes me less sure whether the patch
here really wants backporting. It's certainly an improvement in
its own right to remove the difference between mfn_valid() and
!mfn_valid() MMIO pages, leading e.g. to different treatment by
_sh_propagate(). Will need to give this some more thought, and
of course your and others thoughts would also be appreciated.

Jan

RE: [RESEND][PATCH v2 5/7] xen: include xen/guest_access.h rather than asm/guest_access.h

[Paul Durrant]

> -Original Message-
> From: Julien Grall 
> Sent: 30 July 2020 19:18
> To: xen-devel@lists.xenproject.org
> Cc: jul...@xen.org; Julien Grall ; Stefano Stabellini 
> ;
> Volodymyr Babchuk ; Andrew Cooper 
> ; George
> Dunlap ; Ian Jackson ; 
> Jan Beulich
> ; Wei Liu ; Roger Pau Monné 
> ; Paul Durrant
> ; Jun Nakajima ; Kevin Tian 
> 
> Subject: [RESEND][PATCH v2 5/7] xen: include xen/guest_access.h rather than 
> asm/guest_access.h
> 
> From: Julien Grall 
> 
> Only a few places are actually including asm/guest_access.h. While this
> is fine today, a follow-up patch will want to move most of the helpers
> from asm/guest_access.h to xen/guest_access.h.
> 
> To prepare the move, everyone should include xen/guest_access.h rather
> than asm/guest_access.h.
> 
> Interestingly, asm-arm/guest_access.h includes xen/guest_access.h. The
> inclusion is now removed as no-one but the latter should include the
> former.
> 
> Signed-off-by: Julien Grall 

Viridian change...

Acked-by: Paul Durrant 

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

[Jan Beulich]

On 21.09.2020 16:43, Oleksandr wrote:
> On 21.09.20 16:29, Jan Beulich wrote:
>> On 21.09.2020 14:47, Oleksandr wrote:
>>> On 21.09.20 15:31, Jan Beulich wrote:
 On 21.09.2020 14:22, Oleksandr wrote:
> On 14.09.20 16:52, Jan Beulich wrote:
>> On 10.09.2020 22:21, Oleksandr Tyshchenko wrote:
>>> @@ -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?
> As I understand from the review comment on that for the RFC patch, there
> is no
> a lot of point of keeping this. Indeed, looking at the code I got the
> same opinion.
> I should have added an explanation in the commit description at least.
> Or shall I return the call back?
 If there's a reason to drop it (which I can't see, but I also
 don't recall seeing the discussion you're mentioning), then doing
 so should be a separate patch with suitable reasoning. In the
 patch here you definitely should only transform what's already
 there.
>>> Sounds reasonable. Please see the comment below
>>> relocate_portio_handler() here:
>>> https://www.mail-archive.com/xen-devel@lists.xenproject.org/msg78512.html
>>>
>>> However, I might interpret the request incorrectly.
>> I'm afraid you do: The way you've coded it the function was a no-op.
>> But that's because you broke the caller by not bailing from
>> hvm_destroy_all_ioreq_servers() if relocate_portio_handler() returned
>> false. IOW you did assume that moving the "return" statement into an
>> inline function would have an effect on its caller(s). For questions
>> like this one it also often helps to look at the commit introducing
>> the construct in question (b3344bb1cae0 in this case): Chances are
>> that the description helps, albeit I agree there are many cases
>> (particularly the farther you get into distant past) where it isn't
>> of much help.
> 
> 
> Hmm, now it's clear to me what I did wrong. By calling 
> relocate_portio_handler() here we don't really want to relocate 
> something, we just use it as a flag to indicate whether we need to 
> actually release IOREQ resources down the 
> hvm_destroy_all_ioreq_servers(). Thank you for the explanation, it 
> wasn't obvious to me at the beginning. But, now the question is how to 
> do it in a correct way and retain current behavior (to not break callers)?
> 
> I see two options here:
> 1. Place the check of relocate_portio_handler() in 
> arch_hvm_ioreq_destroy() and make the later returning bool.
>      The "common" hvm_destroy_all_ioreq_servers() will check for the 
> return value and bail out if false.
> 2. Don't use relocate_portio_handler(), instead introduce a flag into 
> struct hvm_domain's ioreq_server sub-structure.
> 
> 
> Personally I don't like much the option 1 and option 2 is a little bit 
> overhead.

Well, 1 is what matches current behavior, so I'd advocate for you
not changing the abstract model. Or else, again, change the abstract
model in a separate prereq patch.

Jan

[xen-unstable test] 154556: tolerable FAIL

[osstest service owner]

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

Failures :-/ but no regressions.

Tests which are failing intermittently (not blocking):
 test-armhf-armhf-xl-rtds 16 guest-start/debian.repeat fail in 154521 pass in 
154556
 test-amd64-i386-xl-qemuu-debianhvm-amd64 18 guest-start/debianhvm.repeat fail 
pass in 154521

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 154521
 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stopfail like 154521
 test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stopfail like 154521
 test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop fail like 154521
 test-armhf-armhf-libvirt 14 saverestore-support-checkfail  like 154521
 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 154521
 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail  like 154521
 test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stopfail like 154521
 test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop fail like 154521
 test-amd64-i386-xl-pvshim12 guest-start  fail   never pass
 test-amd64-amd64-libvirt 13 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt  13 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-xsm  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-arm64-arm64-xl-thunderx 13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl  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-arm64-arm64-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  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-qemuu-debianhvm-amd64-xsm 11 migrate-support-check 
fail never pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check 
fail 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-rtds 13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 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-amd64-i386-xl-qemut-ws16-amd64 17 guest-stop  fail never pass
 test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail  never pass
 test-armhf-armhf-xl-credit2  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-cubietruck 13 migrate-support-checkfail never pass
 test-armhf-armhf-xl-cubietruck 14 saverestore-support-checkfail never pass
 test-armhf-armhf-libvirt 13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  13 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail   never pass
 test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2  fail never pass

version targeted for testing:
 xen  baa4d064e91b6d2bcfe400bdf71f83b961e4c28e
baseline version:
 xen  baa4d064e91b6d2bcfe400bdf71f83b961e4c28e

Last test of basis   154556  2020-09-21 01:52:15 Z0 days
Testing same since  (not found) 0 attempts

jobs:
 build-amd64-xsm  pass
 build-arm64-xsm  pass
 

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

[Oleksandr]

On 21.09.20 16:29, Jan Beulich wrote:

Hi


On 21.09.2020 14:47, Oleksandr wrote:

On 21.09.20 15:31, Jan Beulich wrote:

On 21.09.2020 14:22, Oleksandr wrote:

On 14.09.20 16:52, Jan Beulich wrote:

On 10.09.2020 22:21, Oleksandr Tyshchenko wrote:

@@ -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?

As I understand from the review comment on that for the RFC patch, there
is no
a lot of point of keeping this. Indeed, looking at the code I got the
same opinion.
I should have added an explanation in the commit description at least.
Or shall I return the call back?

If there's a reason to drop it (which I can't see, but I also
don't recall seeing the discussion you're mentioning), then doing
so should be a separate patch with suitable reasoning. In the
patch here you definitely should only transform what's already
there.

Sounds reasonable. Please see the comment below
relocate_portio_handler() here:
https://www.mail-archive.com/xen-devel@lists.xenproject.org/msg78512.html

However, I might interpret the request incorrectly.

I'm afraid you do: The way you've coded it the function was a no-op.
But that's because you broke the caller by not bailing from
hvm_destroy_all_ioreq_servers() if relocate_portio_handler() returned
false. IOW you did assume that moving the "return" statement into an
inline function would have an effect on its caller(s). For questions
like this one it also often helps to look at the commit introducing
the construct in question (b3344bb1cae0 in this case): Chances are
that the description helps, albeit I agree there are many cases
(particularly the farther you get into distant past) where it isn't
of much help.



Hmm, now it's clear to me what I did wrong. By calling 
relocate_portio_handler() here we don't really want to relocate 
something, we just use it as a flag to indicate whether we need to 
actually release IOREQ resources down the 
hvm_destroy_all_ioreq_servers(). Thank you for the explanation, it 
wasn't obvious to me at the beginning. But, now the question is how to 
do it in a correct way and retain current behavior (to not break callers)?


I see two options here:
1. Place the check of relocate_portio_handler() in 
arch_hvm_ioreq_destroy() and make the later returning bool.
    The "common" hvm_destroy_all_ioreq_servers() will check for the 
return value and bail out if false.
2. Don't use relocate_portio_handler(), instead introduce a flag into 
struct hvm_domain's ioreq_server sub-structure.



Personally I don't like much the option 1 and option 2 is a little bit 
overhead.


What do you think?


--
Regards,

Oleksandr Tyshchenko

Re: Memory ordering question in the shutdown deferral code

[Julien Grall]

Hi Jan,

On 21/09/2020 14:11, Jan Beulich wrote:

On 21.09.2020 13:40, Julien Grall wrote:

(+ Xen-devel)

Sorry I forgot to CC xen-devel.

On 21/09/2020 12:38, Julien Grall wrote:

Hi all,

I have started to look at the deferral code (see
vcpu_start_shutdown_deferral()) because we need it for LiveUpdate and
Arm will soon use it.

The current implementation is using an smp_mb() to ensure ordering
between a write then a read. The code looks roughly (I have slightly
adapted it to make my question more obvious):

domain_shutdown()
      d->is_shutting_down = 1;
      smp_mb();
      if ( !vcpu0->defer_shutdown )
      {
    vcpu_pause_nosync(v);
    v->paused_for_shutdown = 1;
      }

vcpu_start_shutdown_deferral()
      vcpu0->defer_shutdown = 1;
      smp_mb();
      if ( unlikely(d->is_shutting_down) )
    vcpu_check_shutdown(v);

      return vcpu0->defer_shutdown;

smp_mb() should only guarantee ordering (this may be stronger on some
arch), so I think there is a race between the two functions.

It would be possible to pause the vCPU in domain_shutdown() because
vcpu0->defer_shutdown wasn't yet seen.

Equally, vcpu_start_shutdown_deferral() may not see d->is_shutting_down
and therefore Xen may continue to send the I/O. Yet the vCPU will be
paused so the I/O will never complete.


Individually for each of these I agree. But isn't the goal merely
to prevent both to enter their if()-s' bodies at the same time?
And isn't the combined effect of the two barriers preventing just
this?


The code should already be able to deal with that as 
vcpu_check_shutdown() will request to hold d->shutdown_lock and then 
check v->paused_for_shutdown.


So I am not sure why the barriers would matter here.




I am not fully familiar with the IOREQ code, but it sounds to me this is
not the behavior that was intended. Can someone more familiar with the
code confirm it?


As to original intentions, I'm afraid among the people still
listed as maintainers for any part of Xen it may only be Tim to
possibly have been involved in the original installation of
this model, and hence who may know of the precise intentions
and considerations back at the time.


It would be useful to know the original intentions, so I have CCed Tim.

However, I think it is more important to agree on what we want to 
achieve so we can decide whether the existing code is suitable.


Do you agree that we only want to shutdown (or pause it at an 
architecturally restartable bounday) a domain with no I/Os inflights?




As far as I'm concerned, to be honest I don't think I've ever
managed to fully convince myself of the correctness of the
model in the general case. But since it did look good enough
for x86 ...


Right, the memory model on x86 is quite simple compare to Arm :). I am 
pretty sure we need some sort of ordering, but I am not convinced we 
have the correct one in place if we want to cater architecture with more 
relaxed memory model.


Cheers,

--
Julien Grall

Re: [PATCH] x86/mm: do not mark IO regions as Xen heap

[Jan Beulich]

On 10.09.2020 15:35, Roger Pau Monne wrote:
> arch_init_memory will treat all the gaps on the physical memory map
> between RAM regions as MMIO and use share_xen_page_with_guest in order
> to assign them to dom_io. This has the side effect of setting the Xen
> heap flag on such pages, and thus is_special_page would then return
> true which is an issue in epte_get_entry_emt because such pages will
> be forced to use write-back cache attributes.
> 
> Fix this by introducing a new helper to assign the MMIO regions to
> dom_io without setting the Xen heap flag on the pages, so that
> is_special_page will return false and the pages won't be forced to use
> write-back cache attributes.
> 
> Fixes: 81fd0d3ca4b2cd ('x86/hvm: simplify 'mmio_direct' check in 
> epte_get_entry_emt()')

Is this really the correct commit? I had this queued for backport,
and ended up discarding it from the queue based on this tag, just
to then noticing that I remembered correctly that I did backport
ca24b2ffdbd9 ("x86/hvm: set 'ipat' in EPT for special pages") to
the stable trees already. Isn't it _this_ commit which the change
here fixes?

Jan

Re: qemu and Xen ABI-unstable libs

[Roger Pau Monné]

On Mon, Sep 21, 2020 at 08:36:55AM +0100, Paul Durrant wrote:
> > -Original Message-
> > From: Xen-devel  On Behalf Of Ian 
> > Jackson
> > Sent: 18 September 2020 17:39
> > To: Debian folks: Michael Tokarev ; Hans van Kranenburg 
> > ; Xen
> > upstream folks with an interest: Andrew Cooper ; 
> > Roger Pau Monné
> > 
> > Cc: pkg-xen-de...@lists.alioth.debian.org; xen-devel@lists.xenproject.org; 
> > My Xen upstream tools co-
> > maintainer: Wei Liu 
> > Subject: RFC: qemu and Xen ABI-unstable libs
> > 
> > Hi all.  Michael Tokarev has been looking into the problem that qemu
> > is using Xen libraries with usntable ABIs.  We did an experiment to
> > see which abi-unstable symbols qemu links to, by suppressing libxc
> > from the link line.  The results are below.[1]
> > 
> > Things are not looking too bad.  After some discussion on #xendevel I
> > have tried to summarise the situation for each of the troublesome
> > symbols.
> > 
> > Also, we discovered that upstream qemu does not link against any
> > abi-unstable Xen libraries if PCI passthrough is disabled.
> > 
> > Please would my Xen colleages correct me if I have made any mistakes.
> > Michael, I hope this is helpful and clear.
> > 
> > 
> > In order from easy to hard:
> > 
> > 
> > xc_domain_shutdown
> > 
> > This call in qemu needs to be replaced with a call to the existing
> > function xendevicemodel_shutdown in libxendevicemodel.  I think it is
> > likely that this call is fixed in qemu upstream.
> > 
> 
> I just pulled QEMU master and it appears that destroy_hvm_domain() is still 
> calling xc_domain_shutdown().
> 
> > 
> > xc_get_hvm_param
> > 
> > There are three references in qemu's
> > xen_get_default_ioreq_server_info, relating to ioreq servers.  These
> > uses (and perhaps surrounding code at this function's call site)
> > should be replaced by use of xendevicemodel_create_ioreq_server
> > etc. from libxendevicemodel.  I think it is likely that this call is
> > fixed in qemu upstream.
> > 
> 
> These references are in compat code for Xen < 4.6. Use of (non-default) ioreq 
> server has been present in the code for a long time.
> We can remove them by retiring the compat code.
> 
> > 
> > xc_physdev_map_pirq
> > xc_physdev_map_pirq_msi
> > xc_physdev_unmap_pirq
> > 
> > These are all small wrappers for the PHYSDEVOP_map_pirq hypercall.
> > PHYSDEVOP is already reasonably abi-stable at the hypervisor level (in
> > theory it's versioned, but changing it would break all dom0's).
> 
> The hypercalls are non-tools and directly called from the Linux kernel code 
> so they are ABI.
> 
> > These calls could just be provided as-is by a new stable abi
> > entrypoint.  We think this should probably go in libxendevicemodel.
> > 
> 
> Rather than simply moving this calls into libxendevicemodel, we should think 
> about their interactions with calls such as
> xc_domain_bind_pt_pci_irq() below and maybe have a stable library that 
> actually provides a better API/ABI for interrupt
> mapping/triggering although...

I've thought the same when speaking with Ian about this, as (for HVM
passthrough) we use the physdev op to obtain a pirq from a physical
device interrupt source (a MSI entry in the QEMU case, because the
legacy interrupt is bound by the toolstack IIRC) and then use that
pirq to bind it to a guest lapic vector.

I think in a sense such physical interrupt abstraction (the pirq) is
helpful in order to simplify the binding, as you don't end up with a
hypercall with a massive number of parameters to identify both the
source and destination interrupt data. It's also helpful when the
guest changes the interrupt binding, as you then only update the guest
side and keep using the same pirq.

We might want however to have an interface more specific to
passthrough, such that the pirqs (or maybe we could just call them
handles) returned by such interface can only be used with guest
specific bind hypercalls?

> I've long felt PCI pass-through should not be done by QEMU anyway (not least 
> because we currently
> have no mechanism for PCI pass-through to PVH domains).

Having xenpt in tree would be fine IMO. Now we have all the proper
infrastructure in place to allow different pci devices to be handled
by different emulators IIRC, which is all that's required for this to
work correctly.

Thanks, Roger.

Re: Memory ordering question in the shutdown deferral code

[Jan Beulich]

On 21.09.2020 15:35, Durrant, Paul wrote:
>> From: Jan Beulich 
>> Sent: 21 September 2020 14:32
>>
>> On 21.09.2020 15:27, Julien Grall wrote:
>>> I think this part is racy at least on non-x86 platform as x86 seems to
>>> implement smp_mb() with a strong memory barrier (mfence).
>>
>> The "strength" of the memory barrier doesn't matter here imo. It's
>> the fully coherent memory model (for WB type memory) which makes
>> this be fine on x86. The barrier still only guarantees ordering,
>> not visibility.
>>
> 
> In which case I misunderstood what the 'smp' means in this context then.

I find this confusing too, at times. But according to my reading
of the doc the "smp" in there really only means "simple compiler
barrier when UP".

Jan

Re: [PATCH] x86: Use LOCK ADD instead of MFENCE for smp_mb()

[Jan Beulich]

On 21.09.2020 15:04, Andrew Cooper wrote:
> MFENCE is overly heavyweight for SMP semantics on WB memory, because it also
> orders weaker cached writes, and flushes the WC buffers.
> 
> This technique was used as an optimisation in Java[1], and later adopted by
> Linux[2] where it was measured to have a 60% performance improvement in VirtIO
> benchmarks.
> 
> The stack is used because it is hot in the L1 cache, and a -4 offset is used
> to avoid creating a false data dependency on live data.  (For 64bit userspace,
> the offset needs to be under the red zone to avoid false dependences).
> 
> Fix up the 32 bit definitions in HVMLoader and libxc to avoid a false data
> dependency.
> 
> [1] https://shipilev.net/blog/2014/on-the-fence-with-dependencies/
> [2] https://git.kernel.org/torvalds/c/450cbdd0125cfa5d7bbf9e2a6b6961cc48d29730
> 
> Signed-off-by: Andrew Cooper 

For the hypervisor and hvmloader part:
Reviewed-by: Jan Beulich 

> --- a/tools/libs/ctrl/include/xenctrl.h
> +++ b/tools/libs/ctrl/include/xenctrl.h
> @@ -68,11 +68,11 @@
>  #define xen_barrier() asm volatile ( "" : : : "memory")
>  
>  #if defined(__i386__)
> -#define xen_mb()  asm volatile ( "lock; addl $0,0(%%esp)" : : : "memory" )

If this causes a false dependency (which I agree it does), how
come ...

> +#define xen_mb()  asm volatile ( "lock addl $0, -4(%%esp)" ::: "memory" )
>  #define xen_rmb() xen_barrier()
>  #define xen_wmb() xen_barrier()
>  #elif defined(__x86_64__)
> -#define xen_mb()  asm volatile ( "mfence" : : : "memory")
> +#define xen_mb()  asm volatile ( "lock addl $0, -128(%%rsp)" ::: "memory" )

... this doesn't? It accesses the bottom 4 bytes of the redzone,
doesn't it?

As a minor other thought for all of its incarnations: Is a 32-bit
memory access really the best choice? Wouldn't an 8-bit access
further reduce (albeit not eliminate) the risk of unnecessary
dependencies between this memory access and others in functions
called from the users of this barrier?

Or actually, in the hypervisor case, since the used stack slot
would typically hold the return address of the next level's
functions, would a 64-bit access or one further away from the
current stack pointer not help avoid partial dependencies?

And finally, already when Linux used this for just 32-bit I've
always been wondering why they bother preserving the contents of
this piece of memory. How about using NOT (saving the immediate
byte) or XCHG (requiring a dead register instead of the saved
arithmetic, immediate byte, and lock prefix)?

Jan

RE: Memory ordering question in the shutdown deferral code

[Durrant, Paul]

> -Original Message-
> From: Jan Beulich 
> Sent: 21 September 2020 14:32
> To: Julien Grall 
> Cc: Durrant, Paul ; Stefano Stabellini 
> ;
> andrew.coop...@citrix.com; George Dunlap ; Xia, 
> Hongyan
> ; xen-devel@lists.xenproject.org
> Subject: RE: [EXTERNAL] Memory ordering question in the shutdown deferral code
> 
> 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 21.09.2020 15:27, Julien Grall wrote:
> > I think this part is racy at least on non-x86 platform as x86 seems to
> > implement smp_mb() with a strong memory barrier (mfence).
> 
> The "strength" of the memory barrier doesn't matter here imo. It's
> the fully coherent memory model (for WB type memory) which makes
> this be fine on x86. The barrier still only guarantees ordering,
> not visibility.
> 

In which case I misunderstood what the 'smp' means in this context then.

  Paul

Re: Memory ordering question in the shutdown deferral code

[Jan Beulich]

On 21.09.2020 15:27, Julien Grall wrote:
> I think this part is racy at least on non-x86 platform as x86 seems to 
> implement smp_mb() with a strong memory barrier (mfence).

The "strength" of the memory barrier doesn't matter here imo. It's
the fully coherent memory model (for WB type memory) which makes
this be fine on x86. The barrier still only guarantees ordering,
not visibility.

Jan

Re: Memory ordering question in the shutdown deferral code

[Xia, Hongyan]

On Mon, 2020-09-21 at 12:55 +, Durrant, Paul wrote:
> > -Original Message-
> > From: Julien Grall 
> > Sent: 21 September 2020 12:41
> > To: Jan Beulich ; Stefano Stabellini <
> > sstabell...@kernel.org>;
> > andrew.coop...@citrix.com; George Dunlap 
> > ; Durrant, Paul
> > 
> > Cc: Xia, Hongyan ; 
> > xen-devel@lists.xenproject.org
> > Subject: RE: [EXTERNAL] Memory ordering question in the shutdown
> > deferral code
> > 
> > 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.
> > 
> > 
> > 
> > (+ Xen-devel)
> > 
> > Sorry I forgot to CC xen-devel.
> > 
> > On 21/09/2020 12:38, Julien Grall wrote:
> > > Hi all,
> > > 
> > > I have started to look at the deferral code (see
> > > vcpu_start_shutdown_deferral()) because we need it for LiveUpdate
> > > and
> > > Arm will soon use it.
> > > 
> > > The current implementation is using an smp_mb() to ensure
> > > ordering
> > > between a write then a read. The code looks roughly (I have
> > > slightly
> > > adapted it to make my question more obvious):
> > > 
> > > domain_shutdown()
> > >  d->is_shutting_down = 1;
> > >  smp_mb();
> > >  if ( !vcpu0->defer_shutdown )
> > >  {
> > >vcpu_pause_nosync(v);
> > >v->paused_for_shutdown = 1;
> > >  }
> > > 
> > > vcpu_start_shutdown_deferral()
> > >  vcpu0->defer_shutdown = 1;
> > >  smp_mb();
> > >  if ( unlikely(d->is_shutting_down) )
> > >vcpu_check_shutdown(v);
> > > 
> > >  return vcpu0->defer_shutdown;
> > > 
> > > smp_mb() should only guarantee ordering (this may be stronger on
> > > some
> > > arch), so I think there is a race between the two functions.
> > > 
> > > It would be possible to pause the vCPU in domain_shutdown()
> > > because
> > > vcpu0->defer_shutdown wasn't yet seen.
> > > 
> > > Equally, vcpu_start_shutdown_deferral() may not see d-
> > > >is_shutting_down
> > > and therefore Xen may continue to send the I/O. Yet the vCPU will
> > > be
> > > paused so the I/O will never complete.
> > > 
> 
> The barrier enforces global order, right? So, if domain_shutdown()
> pauses the vcpu then is_shutting_down must necessarily be visible all
> cpus. Thus vcpu_start_shutdown referral will execute
> vcpu_check_shutdown(), so I'm having a hard time seeing the race.

I think the question is whether smp_mb() can enforce immediate
visibility. It at least enforces ordering, so when other CPUs
eventually see the memory accesses they will be in the right order. On
the x86 side I think an mfence enforces that when the later load is
executed, the previous store is globally visible. But, if it is only
about the ordering seen by other cores, it could be possible that the
write is seen much later.

Hongyan

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

[Jan Beulich]

On 21.09.2020 14:47, Oleksandr wrote:
> On 21.09.20 15:31, Jan Beulich wrote:
>> On 21.09.2020 14:22, Oleksandr wrote:
>>> On 14.09.20 16:52, Jan Beulich wrote:
 On 10.09.2020 22:21, Oleksandr Tyshchenko wrote:
> @@ -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?
>>> As I understand from the review comment on that for the RFC patch, there
>>> is no
>>> a lot of point of keeping this. Indeed, looking at the code I got the
>>> same opinion.
>>> I should have added an explanation in the commit description at least.
>>> Or shall I return the call back?
>> If there's a reason to drop it (which I can't see, but I also
>> don't recall seeing the discussion you're mentioning), then doing
>> so should be a separate patch with suitable reasoning. In the
>> patch here you definitely should only transform what's already
>> there.
> Sounds reasonable. Please see the comment below 
> relocate_portio_handler() here:
> https://www.mail-archive.com/xen-devel@lists.xenproject.org/msg78512.html
> 
> However, I might interpret the request incorrectly.

I'm afraid you do: The way you've coded it the function was a no-op.
But that's because you broke the caller by not bailing from
hvm_destroy_all_ioreq_servers() if relocate_portio_handler() returned
false. IOW you did assume that moving the "return" statement into an
inline function would have an effect on its caller(s). For questions
like this one it also often helps to look at the commit introducing
the construct in question (b3344bb1cae0 in this case): Chances are
that the description helps, albeit I agree there are many cases
(particularly the farther you get into distant past) where it isn't
of much help.

Jan

Re: Memory ordering question in the shutdown deferral code

[Julien Grall]

On 21/09/2020 13:55, Durrant, Paul wrote:

-Original Message-
From: Julien Grall 
Sent: 21 September 2020 12:41
To: Jan Beulich ; Stefano Stabellini 
;
andrew.coop...@citrix.com; George Dunlap ; Durrant, 
Paul

Cc: Xia, Hongyan ; xen-devel@lists.xenproject.org
Subject: RE: [EXTERNAL] Memory ordering question in the shutdown deferral code

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.



(+ Xen-devel)

Sorry I forgot to CC xen-devel.

On 21/09/2020 12:38, Julien Grall wrote:

Hi all,

I have started to look at the deferral code (see
vcpu_start_shutdown_deferral()) because we need it for LiveUpdate and
Arm will soon use it.

The current implementation is using an smp_mb() to ensure ordering
between a write then a read. The code looks roughly (I have slightly
adapted it to make my question more obvious):

domain_shutdown()
  d->is_shutting_down = 1;
  smp_mb();
  if ( !vcpu0->defer_shutdown )
  {
vcpu_pause_nosync(v);
v->paused_for_shutdown = 1;
  }

vcpu_start_shutdown_deferral()
  vcpu0->defer_shutdown = 1;
  smp_mb();
  if ( unlikely(d->is_shutting_down) )
vcpu_check_shutdown(v);

  return vcpu0->defer_shutdown;

smp_mb() should only guarantee ordering (this may be stronger on some
arch), so I think there is a race between the two functions.

It would be possible to pause the vCPU in domain_shutdown() because
vcpu0->defer_shutdown wasn't yet seen.

Equally, vcpu_start_shutdown_deferral() may not see d->is_shutting_down
and therefore Xen may continue to send the I/O. Yet the vCPU will be
paused so the I/O will never complete.



The barrier enforces global order, right?


It is not clear to me what you mean by "global ordering". This seems to 
suggest a very expensive synchronization barrier between all the processors.


From an arch-agnostic PoV, smp_mb() will enforce an ordering between 
loads/stores but it doesn't guarantee *when* they will be observed.



So, if domain_shutdown() pauses the vcpu then is_shutting_down must necessarily 
be visible all cpus.


That's not the guarantee provided by smp_mb() (see above).


Thus vcpu_start_shutdown referral will execute vcpu_check_shutdown(), so I'm 
having a hard time seeing the race.


I am not fully familiar with the IOREQ code, but it sounds to me this is
not the behavior that was intended. Can someone more familiar with the
code confirm it?



No indeed. I think emulation should complete before the vcpu pauses.


I think this part is racy at least on non-x86 platform as x86 seems to 
implement smp_mb() with a strong memory barrier (mfence).


Cheers,

--
Julien Grall

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

[osstest service owner]

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

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  c7e3021a71fdb4f2d5dbad90ba83ce35bc21cda6
baseline version:
 xen  baa4d064e91b6d2bcfe400bdf71f83b961e4c28e

Last test of basis   154480  2020-09-18 20:00:24 Z2 days
Testing same since   154569  2020-09-21 11:00:24 Z0 days1 attempts


People who touched revisions under test:
  Jan Beulich 
  Roger Pau Monné 
  Trammell Hudson 

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
   baa4d064e9..c7e3021a71  c7e3021a71fdb4f2d5dbad90ba83ce35bc21cda6 -> smoke

Re: Memory ordering question in the shutdown deferral code

[Jan Beulich]

On 21.09.2020 13:40, Julien Grall wrote:
> (+ Xen-devel)
> 
> Sorry I forgot to CC xen-devel.
> 
> On 21/09/2020 12:38, Julien Grall wrote:
>> Hi all,
>>
>> I have started to look at the deferral code (see 
>> vcpu_start_shutdown_deferral()) because we need it for LiveUpdate and 
>> Arm will soon use it.
>>
>> The current implementation is using an smp_mb() to ensure ordering 
>> between a write then a read. The code looks roughly (I have slightly 
>> adapted it to make my question more obvious):
>>
>> domain_shutdown()
>>      d->is_shutting_down = 1;
>>      smp_mb();
>>      if ( !vcpu0->defer_shutdown )
>>      {
>>    vcpu_pause_nosync(v);
>>    v->paused_for_shutdown = 1;
>>      }
>>
>> vcpu_start_shutdown_deferral()
>>      vcpu0->defer_shutdown = 1;
>>      smp_mb();
>>      if ( unlikely(d->is_shutting_down) )
>>    vcpu_check_shutdown(v);
>>
>>      return vcpu0->defer_shutdown;
>>
>> smp_mb() should only guarantee ordering (this may be stronger on some 
>> arch), so I think there is a race between the two functions.
>>
>> It would be possible to pause the vCPU in domain_shutdown() because 
>> vcpu0->defer_shutdown wasn't yet seen.
>>
>> Equally, vcpu_start_shutdown_deferral() may not see d->is_shutting_down 
>> and therefore Xen may continue to send the I/O. Yet the vCPU will be 
>> paused so the I/O will never complete.

Individually for each of these I agree. But isn't the goal merely
to prevent both to enter their if()-s' bodies at the same time?
And isn't the combined effect of the two barriers preventing just
this?

>> I am not fully familiar with the IOREQ code, but it sounds to me this is 
>> not the behavior that was intended. Can someone more familiar with the 
>> code confirm it?

As to original intentions, I'm afraid among the people still
listed as maintainers for any part of Xen it may only be Tim to
possibly have been involved in the original installation of
this model, and hence who may know of the precise intentions
and considerations back at the time.

As far as I'm concerned, to be honest I don't think I've ever
managed to fully convince myself of the correctness of the
model in the general case. But since it did look good enough
for x86 ...

Jan

Re: [PATCH 2/2] qdev: Let BusRealize() return a boolean value to indicate error

[Markus Armbruster]

Philippe Mathieu-Daudé  writes:

> On 9/21/20 10:19 AM, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé  writes:
>> 
>>> Commit 9940b2cfbc0 introduced qdev_realize() and qbus_realize()
>>> with the ability to return a boolean value if an error occured,
>>> thus the caller does not need to check if the Error* pointer is
>>> set.
>>> Provide the same ability to the BusRealize type.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé 
>>> ---
>>>  include/hw/qdev-core.h | 14 +-
>>>  hw/hyperv/vmbus.c  |  5 +++--
>>>  hw/nubus/nubus-bus.c   |  5 +++--
>>>  hw/pci/pci.c   | 12 +---
>>>  hw/xen/xen-bus.c   |  5 +++--
>>>  5 files changed, 31 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>>> index 02ac1c50b7f..eecfe794a71 100644
>>> --- a/include/hw/qdev-core.h
>>> +++ b/include/hw/qdev-core.h
>>> @@ -32,7 +32,19 @@ typedef enum DeviceCategory {
>>>  typedef void (*DeviceRealize)(DeviceState *dev, Error **errp);
>>>  typedef void (*DeviceUnrealize)(DeviceState *dev);
>>>  typedef void (*DeviceReset)(DeviceState *dev);
>>> -typedef void (*BusRealize)(BusState *bus, Error **errp);
>>> +/**
>>> + * BusRealize: Realize @bus.
>>> + * @bus: bus to realize
>>> + * @errp: pointer to error object
>>> + *
>>> + * On success, return true.
>>> + * On failure, store an error through @errp and return false.
>>> + */
>>> +typedef bool (*BusRealize)(BusState *bus, Error **errp);
>>> +/**
>>> + * BusUnrealize: Unrealize @bus.
>>> + * @bus: bus to unrealize
>>> + */
>>>  typedef void (*BusUnrealize)(BusState *bus);
>>>  
>>>  /**
>>> diff --git a/hw/hyperv/vmbus.c b/hw/hyperv/vmbus.c
>>> index 6ef895bc352..8a0452b2464 100644
>>> --- a/hw/hyperv/vmbus.c
>>> +++ b/hw/hyperv/vmbus.c
>>> @@ -2487,7 +2487,7 @@ static const TypeInfo vmbus_dev_type_info = {
>>>  .instance_init = vmbus_dev_instance_init,
>>>  };
>>>  
>>> -static void vmbus_realize(BusState *bus, Error **errp)
>>> +static bool vmbus_realize(BusState *bus, Error **errp)
>>>  {
>>>  int ret = 0;
>>>  Error *local_err = NULL;
>>> @@ -2519,7 +2519,7 @@ static void vmbus_realize(BusState *bus, Error **errp)
>>>  goto clear_event_notifier;
>>>  }
>>>  
>>> -return;
>>> +return true;
>>>  
>>>  clear_event_notifier:
>>>  event_notifier_cleanup(>notifier);
>>> @@ -2528,6 +2528,7 @@ remove_msg_handler:
>>>  error_out:
>>>  qemu_mutex_destroy(>rx_queue_lock);
>>>  error_propagate(errp, local_err);
>>> +return false;
>>>  }
>>>  
>>>  static void vmbus_unrealize(BusState *bus)
>>> diff --git a/hw/nubus/nubus-bus.c b/hw/nubus/nubus-bus.c
>>> index 942a6d5342d..d20d9c0f72c 100644
>>> --- a/hw/nubus/nubus-bus.c
>>> +++ b/hw/nubus/nubus-bus.c
>>> @@ -65,12 +65,13 @@ static const MemoryRegionOps nubus_super_slot_ops = {
>>>  },
>>>  };
>>>  
>>> -static void nubus_realize(BusState *bus, Error **errp)
>>> +static bool nubus_realize(BusState *bus, Error **errp)
>>>  {
>>>  if (!nubus_find()) {
>>>  error_setg(errp, "at most one %s device is permitted", 
>>> TYPE_NUBUS_BUS);
>>> -return;
>>> +return false;
>>>  }
>>> +return true;
>>>  }
>>>  
>>>  static void nubus_init(Object *obj)
>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>> index de0fae10ab9..f535ebac847 100644
>>> --- a/hw/pci/pci.c
>>> +++ b/hw/pci/pci.c
>>> @@ -115,7 +115,7 @@ static void pcibus_machine_done(Notifier *notifier, 
>>> void *data)
>>>  }
>>>  }
>>>  
>>> -static void pci_bus_realize(BusState *qbus, Error **errp)
>>> +static bool pci_bus_realize(BusState *qbus, Error **errp)
>>>  {
>>>  PCIBus *bus = PCI_BUS(qbus);
>>>  
>>> @@ -123,13 +123,17 @@ static void pci_bus_realize(BusState *qbus, Error 
>>> **errp)
>>>  qemu_add_machine_init_done_notifier(>machine_done);
>>>  
>>>  vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY, _pcibus, bus);
>>> +
>>> +return true;
>>>  }
>>>  
>>> -static void pcie_bus_realize(BusState *qbus, Error **errp)
>>> +static bool pcie_bus_realize(BusState *qbus, Error **errp)
>>>  {
>>>  PCIBus *bus = PCI_BUS(qbus);
>>>  
>>> -pci_bus_realize(qbus, errp);
>>> +if (!pci_bus_realize(qbus, errp)) {
>>> +return false;
>>> +}
>> 
>> We now update bus->flags only when pci_bus_realize() succeeds.  Is this
>> a bug fix?
>
> Fortunate side effect :) I'll let the PCI maintainers
> have a look at it.

If it's an observable change, the commit message must mention it.  I'd
put it in its own commit then.

Even if it's not observable, explaining why in the commit message would
help, I think.

>> 
>>>  
>>>  /*
>>>   * A PCI-E bus can support extended config space if it's the root
>>> @@ -144,6 +148,8 @@ static void pcie_bus_realize(BusState *qbus, Error 
>>> **errp)
>>>  bus->flags |= PCI_BUS_EXTENDED_CONFIG_SPACE;
>>>  }
>>>  }
>>> +
>>> +return true;
>>>  }
>>>  
>>>  static void pci_bus_unrealize(BusState *qbus)
>>> diff --git a/hw/xen/xen-bus.c 

RE: Memory ordering question in the shutdown deferral code

[Durrant, Paul]

> -Original Message-
> From: Julien Grall 
> Sent: 21 September 2020 12:41
> To: Jan Beulich ; Stefano Stabellini 
> ;
> andrew.coop...@citrix.com; George Dunlap ; Durrant, 
> Paul
> 
> Cc: Xia, Hongyan ; xen-devel@lists.xenproject.org
> Subject: RE: [EXTERNAL] Memory ordering question in the shutdown deferral code
> 
> 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.
> 
> 
> 
> (+ Xen-devel)
> 
> Sorry I forgot to CC xen-devel.
> 
> On 21/09/2020 12:38, Julien Grall wrote:
> > Hi all,
> >
> > I have started to look at the deferral code (see
> > vcpu_start_shutdown_deferral()) because we need it for LiveUpdate and
> > Arm will soon use it.
> >
> > The current implementation is using an smp_mb() to ensure ordering
> > between a write then a read. The code looks roughly (I have slightly
> > adapted it to make my question more obvious):
> >
> > domain_shutdown()
> >  d->is_shutting_down = 1;
> >  smp_mb();
> >  if ( !vcpu0->defer_shutdown )
> >  {
> >vcpu_pause_nosync(v);
> >v->paused_for_shutdown = 1;
> >  }
> >
> > vcpu_start_shutdown_deferral()
> >  vcpu0->defer_shutdown = 1;
> >  smp_mb();
> >  if ( unlikely(d->is_shutting_down) )
> >vcpu_check_shutdown(v);
> >
> >  return vcpu0->defer_shutdown;
> >
> > smp_mb() should only guarantee ordering (this may be stronger on some
> > arch), so I think there is a race between the two functions.
> >
> > It would be possible to pause the vCPU in domain_shutdown() because
> > vcpu0->defer_shutdown wasn't yet seen.
> >
> > Equally, vcpu_start_shutdown_deferral() may not see d->is_shutting_down
> > and therefore Xen may continue to send the I/O. Yet the vCPU will be
> > paused so the I/O will never complete.
> >

The barrier enforces global order, right? So, if domain_shutdown() pauses the 
vcpu then is_shutting_down must necessarily be visible all cpus. Thus 
vcpu_start_shutdown referral will execute vcpu_check_shutdown(), so I'm having 
a hard time seeing the race.

> > I am not fully familiar with the IOREQ code, but it sounds to me this is
> > not the behavior that was intended. Can someone more familiar with the
> > code confirm it?
> >

No indeed. I think emulation should complete before the vcpu pauses.

  Paul

> > Cheers,
> >
> 
> --
> Julien Grall

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

[Oleksandr]

On 21.09.20 15:31, Jan Beulich wrote:

Hi


On 21.09.2020 14:22, Oleksandr wrote:

On 14.09.20 16:52, Jan Beulich wrote:

On 10.09.2020 22:21, Oleksandr Tyshchenko wrote:

--- 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...

For me without "handle" it will sound a bit confusing because of the
enum type which
has a similar name but without "arch" prefix:
bool arch_hvm_io_completion(enum hvm_io_completion io_completion)

Every function handles something; there's no point including
"handle" in every name. Or else we'd have handle_memset()
instead of just memset(), for example.


Got it. Will remove "handle" here.





Shall I then move "hvm" to the front of the name here?

As per comments on later patches, I think we want to consider dropping
hvm prefixes or infixes altogether from the functions involved here.

@@ -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?

As I understand from the review comment on that for the RFC patch, there
is no
a lot of point of keeping this. Indeed, looking at the code I got the
same opinion.
I should have added an explanation in the commit description at least.
Or shall I return the call back?

If there's a reason to drop it (which I can't see, but I also
don't recall seeing the discussion you're mentioning), then doing
so should be a separate patch with suitable reasoning. In the
patch here you definitely should only transform what's already
there.
Sounds reasonable. Please see the comment below 
relocate_portio_handler() here:

https://www.mail-archive.com/xen-devel@lists.xenproject.org/msg78512.html

However, I might interpret the request incorrectly.

--
Regards,

Oleksandr Tyshchenko

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

[Jan Beulich]

On 21.09.2020 14:22, Oleksandr wrote:
> On 14.09.20 16:52, Jan Beulich wrote:
>> On 10.09.2020 22:21, Oleksandr Tyshchenko wrote:
>>> --- 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...
> For me without "handle" it will sound a bit confusing because of the 
> enum type which
> has a similar name but without "arch" prefix:
> bool arch_hvm_io_completion(enum hvm_io_completion io_completion)

Every function handles something; there's no point including
"handle" in every name. Or else we'd have handle_memset()
instead of just memset(), for example.

> Shall I then move "hvm" to the front of the name here?

As per comments on later patches, I think we want to consider dropping
hvm prefixes or infixes altogether from the functions involved here.

>>> @@ -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?
> As I understand from the review comment on that for the RFC patch, there 
> is no
> a lot of point of keeping this. Indeed, looking at the code I got the 
> same opinion.
> I should have added an explanation in the commit description at least.
> Or shall I return the call back?

If there's a reason to drop it (which I can't see, but I also
don't recall seeing the discussion you're mentioning), then doing
so should be a separate patch with suitable reasoning. In the
patch here you definitely should only transform what's already
there.

Jan

Re: [PATCH] xen/arm: bootfdt: Ignore empty memory bank

[Bertrand Marquis]

> On 18 Sep 2020, at 18:11, Julien Grall  wrote:
> 
> From: Julien Grall 
> 
> At the moment, Xen will stop processing the Device Tree if a memory
> bank is empty (size == 0).
> 
> Unfortunately, some of the Device Tree (such as on Colibri imx8qxp)
> may contain such a bank. This means Xen will not be able to boot
> properly.
> 
> Relax the check to just ignore the banks. FWIW this also seems to be the
> behavior adopted by Linux.
> 
> Reported-by: Daniel Wagner 
> Signed-off-by: Julien Grall 

Reviewed-by: Bertrand Marquis 

> ---
> xen/arch/arm/bootfdt.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index 08fb59f4e7a9..dcff512648a0 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -163,8 +163,9 @@ static int __init process_memory_node(const void *fdt, 
> int node,
> for ( i = 0; i < banks && mem->nr_banks < NR_MEM_BANKS; i++ )
> {
> device_tree_get_reg(, address_cells, size_cells, , );
> +/* Some DT may describe empty bank, ignore them */
> if ( !size )
> -return -EINVAL;
> +continue;
> mem->bank[mem->nr_banks].start = start;
> mem->bank[mem->nr_banks].size = size;
> mem->nr_banks++;
> -- 
> 2.17.1
> 
> 

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

[Oleksandr]

On 14.09.20 16:52, Jan Beulich wrote:

Hi Jan


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).


Got it. Will add an explanation.



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...
For me without "handle" it will sound a bit confusing because of the 
enum type which

has a similar name but without "arch" prefix:
bool arch_hvm_io_completion(enum hvm_io_completion io_completion)


Shall I then move "hvm" to the front of the name here?





@@ -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).


I didn't have a plan to add error handling here. Agree, will make 
arch_hvm_destroy_ioreq_server() returning void.






@@ -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?
As I understand from the review comment on that for the RFC patch, there 
is no
a lot of point of keeping this. Indeed, looking at the code I got the 
same opinion.

I should have added an explanation in the commit description at least.
Or shall I return the call back?





@@ -1239,19 +1256,15 @@ void hvm_destroy_all_ioreq_servers(struct domain *d)
  spin_unlock_recursive(>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?


Agree, will add.





+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.


This function is just an attempt to split arch specific things (cf8 
handling) from "common" hvm_select_ioreq_server().




 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, , ) )
+return NULL;

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


Yes, hvm_ioreq_server_get_type_addr() better represents what function does.





@@ -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 

Re: [PATCH v5 4/5] efi: Enable booting unified hypervisor/kernel/initrd images

[Jan Beulich]

On 21.09.2020 13:47, Trammell Hudson wrote:
> On Friday, September 18, 2020 11:15 AM, Jan Beulich  wrote:
>> On 17.09.2020 17:40, Trammell Hudson wrote:
>> Instead of forcing the caller to pass in a dot-prefixed name
>> and you assuming it's a dot here, how about ...
>> ... pe_find_section() looking for '.' followed by ?
> 
> v6 adds a special name compare function to do this with a
> CHAR16 section name to avoid the extra s2w() you mentioned.
> 
> (btw, even if the EFI constness patches don't go in, just
> making PrintStr cast away the const would allow several of
> these startup functions to have const CHAR16* arguments since
> the only reason they are non-const is to be able to print)

And I'd be happy to approve a patch to this effect, provided
it uses an inline function and not a macro to do this wrapping.

>> [...]
>>> -  if ( read_section(loaded_image, ".config", , NULL) )
>>> -  PrintStr(L"Using unified config file\\r\\n");
>>
>> Maybe "embedded" instead of "unified"? The config file isn't unified
>> after all, it's the Xen binary which is.
> 
> How about "builtin"?

That's fine with me as well.

Jan

[PATCH v6 3/5] 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 
Acked-by: Jan Beulich 
---
 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 c2ce0c7294..93cfeba7e1 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -540,6 +540,22 @@ static char * __init split_string(char *s)
 return NULL;
 }
 
+static void __init handle_file_info(CHAR16 *name,
+const struct file *file, const char 
*options)
+{
+if ( file ==  )
+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, const char *options)
 {
@@ -584,16 +600,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, 
CHAR16 *name,
 {
 file->need_to_free = true;
 file->size = size;
-if ( file !=  )
-{
-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, >size, file->str);
 if ( !EFI_ERROR(ret) && file->size != size )
-- 
2.25.1

[PATCH v6 1/5] efi/boot.c: Make file->ptr const void*

[Trammell Hudson]

Other than the config file parser that edits the image inplace,
no other users of the file sections requires write access to the
data.

Signed-off-by: Trammell Hudson 
Reviewed-by: Jan Beulich 
Reviewed-by: Roger Pau Monné 
---
 xen/common/efi/boot.c | 11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 0ac80e47bb..157fe0e8c5 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -41,7 +41,7 @@
 
 typedef EFI_STATUS
 (/* _not_ EFIAPI */ *EFI_SHIM_LOCK_VERIFY) (
-IN VOID *Buffer,
+IN const VOID *Buffer,
 IN UINT32 Size);
 
 typedef struct {
@@ -104,7 +104,8 @@ struct file {
 UINTN size;
 union {
 EFI_PHYSICAL_ADDRESS addr;
-void *ptr;
+char *str;
+const void *ptr;
 };
 };
 
@@ -592,7 +593,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, 
CHAR16 *name,
 efi_arch_handle_module(file, name, options);
 }
 
-ret = FileHandle->Read(FileHandle, >size, file->ptr);
+ret = FileHandle->Read(FileHandle, >size, file->str);
 if ( !EFI_ERROR(ret) && file->size != size )
 ret = EFI_ABORTED;
 if ( EFI_ERROR(ret) )
@@ -616,7 +617,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, 
CHAR16 *name,
 
 static void __init pre_parse(const struct file *cfg)
 {
-char *ptr = cfg->ptr, *end = ptr + cfg->size;
+char *ptr = cfg->str, *end = ptr + cfg->size;
 bool start = true, comment = false;
 
 for ( ; ptr < end; ++ptr )
@@ -645,7 +646,7 @@ static void __init pre_parse(const struct file *cfg)
 static char *__init get_value(const struct file *cfg, const char *section,
   const char *item)
 {
-char *ptr = cfg->ptr, *end = ptr + cfg->size;
+char *ptr = cfg->str, *end = ptr + cfg->size;
 size_t slen = section ? strlen(section) : 0, ilen = strlen(item);
 bool match = !slen;
 
-- 
2.25.1

[PATCH v6 5/5] efi: Do not use command line if unified config is included

[Trammell Hudson]

If a unified Xen image is used, then the bundled configuration,
Xen command line, dom0 kernel, and ramdisk are prefered over
any files listed in the config file or provided on the command line.

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

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

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index dcf939a05e..28903d6013 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -945,6 +945,35 @@ 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 __initdata 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", _guid,
+ NULL, _size, );
+if ( rc != EFI_SUCCESS )
+return false;
+
+rc = efi_rs->GetVariable(L"SetupMode", _guid,
+ NULL, _size, );
+if ( rc != EFI_SUCCESS )
+return false;
+
+if ( secboot > 1 || setupmode > 1 )
+blexit(L"Invalid SecureBoot/SetupMode variables");
+
+return secboot == 1 && setupmode == 0;
+}
+
 static void __init efi_variables(void)
 {
 EFI_STATUS status;
@@ -1121,15 +1150,15 @@ 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;
 union string section = { NULL }, name;
 bool base_video = false;
 const char *option_str;
-bool use_cfg_file;
+bool use_cfg_file, secure;
 
 __set_bit(EFI_BOOT, _flags);
 __set_bit(EFI_LOADER, _flags);
@@ -1148,8 +1177,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 ( use_cfg_file &&
+ !read_section(loaded_image, L"config", , NULL) )
 {
 UINTN offset = 0;
 
@@ -1207,6 +1238,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);
 
@@ -1226,7 +1259,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
*SystemTable)
 /* Get the file system interface. */
 dir_handle = get_parent_handle(loaded_image, _name);
 
-if ( read_section(loaded_image, L"config", , NULL) )
+if ( cfg.ptr )
 PrintStr(L"Using builtin config file\r\n");
 else if ( !cfg_file_name )
 {
-- 
2.25.1

[PATCH v6 2/5] 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 
Reviewed-by: Roger Pau Monné 
---
 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 157fe0e8c5..c2ce0c7294 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;
 char *str;
@@ -280,13 +281,13 @@ void __init noreturn blexit(const CHAR16 *str)
 if ( !efi_bs )
 efi_arch_halt();
 
-if ( cfg.addr )
+if ( cfg.need_to_free )
 efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
-if ( kernel.addr )
+if ( kernel.need_to_free )
 efi_bs->FreePages(kernel.addr, PFN_UP(kernel.size));
-if ( ramdisk.addr )
+if ( ramdisk.need_to_free )
 efi_bs->FreePages(ramdisk.addr, PFN_UP(ramdisk.size));
-if ( xsm.addr )
+if ( xsm.need_to_free )
 efi_bs->FreePages(xsm.addr, PFN_UP(xsm.size));
 
 efi_arch_blexit();
@@ -581,6 +582,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, 
CHAR16 *name,
 }
 else
 {
+file->need_to_free = true;
 file->size = size;
 if ( file !=  )
 {
-- 
2.25.1

[PATCH v6 0/5] 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 (5):
  efi/boot.c: Make file->ptr const void*
  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 unified config is included

 .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   | 155 +--
 xen/common/efi/efi.h|   3 +
 xen/common/efi/pe.c | 159 
 8 files changed, 353 insertions(+), 52 deletions(-)
 create mode 100644 xen/common/efi/pe.c

-- 
2.25.1

[PATCH v6 4/5] 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.

Note that the system will fall back to loading files from disk if
the named sections do not exist. This allows distributions to continue
with the status quo if they want a signed kernel + config, while still
allowing a user provided initrd (which is how the shim protocol currently
works as well).

This patch also adds constness to the section parameter of
efi_arch_cfg_file_early() and efi_arch_cfg_file_late()
and changes pe_find_section() to use a CHAR16 section name.

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   |  66 +++
 xen/common/efi/efi.h|   3 +
 xen/common/efi/pe.c | 159 
 8 files changed, 287 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 27dd0b1a94..3653dfeabc 100644
--- a/xen/arch/arm/efi/efi-boot.h

Re: [PATCH v5 4/5] efi: Enable booting unified hypervisor/kernel/initrd images

[Trammell Hudson]

On Friday, September 18, 2020 11:15 AM, Jan Beulich  wrote:
> On 17.09.2020 17:40, Trammell Hudson wrote:
> Instead of forcing the caller to pass in a dot-prefixed name
> and you assuming it's a dot here, how about ...
> ... pe_find_section() looking for '.' followed by ?

v6 adds a special name compare function to do this with a
CHAR16 section name to avoid the extra s2w() you mentioned.

(btw, even if the EFI constness patches don't go in, just
making PrintStr cast away the const would allow several of
these startup functions to have const CHAR16* arguments since
the only reason they are non-const is to be able to print)

> [...]
> > -  if ( read_section(loaded_image, ".config", , NULL) )
> > -  PrintStr(L"Using unified config file\\r\\n");
>
> Maybe "embedded" instead of "unified"? The config file isn't unified
> after all, it's the Xen binary which is.

How about "builtin"?

I missed the reviews on the need_to_free patch; they are also incorporated into 
v6.

--
Trammell

Re: Memory ordering question in the shutdown deferral code

[Julien Grall]

(+ Xen-devel)

Sorry I forgot to CC xen-devel.

On 21/09/2020 12:38, Julien Grall wrote:

Hi all,

I have started to look at the deferral code (see 
vcpu_start_shutdown_deferral()) because we need it for LiveUpdate and 
Arm will soon use it.


The current implementation is using an smp_mb() to ensure ordering 
between a write then a read. The code looks roughly (I have slightly 
adapted it to make my question more obvious):


domain_shutdown()
     d->is_shutting_down = 1;
     smp_mb();
     if ( !vcpu0->defer_shutdown )
     {
   vcpu_pause_nosync(v);
   v->paused_for_shutdown = 1;
     }

vcpu_start_shutdown_deferral()
     vcpu0->defer_shutdown = 1;
     smp_mb();
     if ( unlikely(d->is_shutting_down) )
   vcpu_check_shutdown(v);

     return vcpu0->defer_shutdown;

smp_mb() should only guarantee ordering (this may be stronger on some 
arch), so I think there is a race between the two functions.


It would be possible to pause the vCPU in domain_shutdown() because 
vcpu0->defer_shutdown wasn't yet seen.


Equally, vcpu_start_shutdown_deferral() may not see d->is_shutting_down 
and therefore Xen may continue to send the I/O. Yet the vCPU will be 
paused so the I/O will never complete.


I am not fully familiar with the IOREQ code, but it sounds to me this is 
not the behavior that was intended. Can someone more familiar with the 
code confirm it?


Cheers,



--
Julien Grall

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

[Julien Grall]

Hi Jan,

On 21/09/2020 11:17, Jan Beulich wrote:

On 14.09.2020 12:15, Jan Beulich wrote:

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(-)


May I ask for an Arm-side ack (or otherwise) here, please?


Acked-by: Julien Grall 

Cheers,



Jan


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





--
Julien Grall

RE: qemu and Xen ABI-unstable libs

[Paul Durrant]

> -Original Message-
> From: Roger Pau Monné 
> Sent: 21 September 2020 11:16
> To: p...@xen.org
> Cc: 'Ian Jackson' ; 'Debian folks: Michael Tokarev' 
> ; 'Hans van
> Kranenburg' ; 'Xen upstream folks with an interest: Andrew 
> Cooper'
> ; pkg-xen-de...@lists.alioth.debian.org; 
> xen-devel@lists.xenproject.org;
> 'My Xen upstream tools co-maintainer: Wei Liu' 
> Subject: Re: qemu and Xen ABI-unstable libs
> 
> On Mon, Sep 21, 2020 at 08:36:55AM +0100, Paul Durrant wrote:
> > > -Original Message-
> > > From: Xen-devel  On Behalf Of Ian 
> > > Jackson
> > > Sent: 18 September 2020 17:39
> > > To: Debian folks: Michael Tokarev ; Hans van Kranenburg 
> > > ; Xen
> > > upstream folks with an interest: Andrew Cooper 
> > > ; Roger Pau Monné
> > > 
> > > Cc: pkg-xen-de...@lists.alioth.debian.org; 
> > > xen-devel@lists.xenproject.org; My Xen upstream tools
> co-
> > > maintainer: Wei Liu 
> > > Subject: RFC: qemu and Xen ABI-unstable libs
> > >
> > > Hi all.  Michael Tokarev has been looking into the problem that qemu
> > > is using Xen libraries with usntable ABIs.  We did an experiment to
> > > see which abi-unstable symbols qemu links to, by suppressing libxc
> > > from the link line.  The results are below.[1]
> > >
> > > Things are not looking too bad.  After some discussion on #xendevel I
> > > have tried to summarise the situation for each of the troublesome
> > > symbols.
> > >
> > > Also, we discovered that upstream qemu does not link against any
> > > abi-unstable Xen libraries if PCI passthrough is disabled.
> > >
> > > Please would my Xen colleages correct me if I have made any mistakes.
> > > Michael, I hope this is helpful and clear.
> > >
> > >
> > > In order from easy to hard:
> > >
> > >
> > > xc_domain_shutdown
> > >
> > > This call in qemu needs to be replaced with a call to the existing
> > > function xendevicemodel_shutdown in libxendevicemodel.  I think it is
> > > likely that this call is fixed in qemu upstream.
> > >
> >
> > I just pulled QEMU master and it appears that destroy_hvm_domain() is still 
> > calling
> xc_domain_shutdown().
> >
> > >
> > > xc_get_hvm_param
> > >
> > > There are three references in qemu's
> > > xen_get_default_ioreq_server_info, relating to ioreq servers.  These
> > > uses (and perhaps surrounding code at this function's call site)
> > > should be replaced by use of xendevicemodel_create_ioreq_server
> > > etc. from libxendevicemodel.  I think it is likely that this call is
> > > fixed in qemu upstream.
> > >
> >
> > These references are in compat code for Xen < 4.6. Use of (non-default) 
> > ioreq server has been
> present in the code for a long time.
> > We can remove them by retiring the compat code.
> >
> > >
> > > xc_physdev_map_pirq
> > > xc_physdev_map_pirq_msi
> > > xc_physdev_unmap_pirq
> > >
> > > These are all small wrappers for the PHYSDEVOP_map_pirq hypercall.
> > > PHYSDEVOP is already reasonably abi-stable at the hypervisor level (in
> > > theory it's versioned, but changing it would break all dom0's).
> >
> > The hypercalls are non-tools and directly called from the Linux kernel code 
> > so they are ABI.
> >
> > > These calls could just be provided as-is by a new stable abi
> > > entrypoint.  We think this should probably go in libxendevicemodel.
> > >
> >
> > Rather than simply moving this calls into libxendevicemodel, we should 
> > think about their
> interactions with calls such as
> > xc_domain_bind_pt_pci_irq() below and maybe have a stable library that 
> > actually provides a better
> API/ABI for interrupt
> > mapping/triggering although...
> 
> I've thought the same when speaking with Ian about this, as (for HVM
> passthrough) we use the physdev op to obtain a pirq from a physical
> device interrupt source (a MSI entry in the QEMU case, because the
> legacy interrupt is bound by the toolstack IIRC) and then use that
> pirq to bind it to a guest lapic vector.
> 
> I think in a sense such physical interrupt abstraction (the pirq) is
> helpful in order to simplify the binding, as you don't end up with a
> hypercall with a massive number of parameters to identify both the
> source and destination interrupt data. It's also helpful when the
> guest changes the interrupt binding, as you then only update the guest
> side and keep using the same pirq.
> 
> We might want however to have an interface more specific to
> passthrough, such that the pirqs (or maybe we could just call them
> handles) returned by such interface can only be used with guest
> specific bind hypercalls?

Yes, that sounds sensible: we have functions to allocare/free a pirq handle and 
then functions to bind/unbind such a handle to guest vector.

> 
> > I've long felt PCI pass-through should not be done by QEMU anyway (not 
> > least because we currently
> > have no mechanism for PCI pass-through to PVH domains).
> 
> Having xenpt in tree would be fine IMO. Now we have all the proper
> infrastructure in place to allow different pci devices to be handled
> 

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

[Jan Beulich]

On 14.09.2020 12:15, Jan Beulich wrote:
> 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(-)

May I ask for an Arm-side ack (or otherwise) here, please?

Jan

> 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
> 

Ping: [PATCH 2/2] x86/vpic: also execute dpci callback for non-specific EOI

[Jan Beulich]

On 21.08.2020 09:45, Jan Beulich wrote:
> On 20.08.2020 18:28, Andrew Cooper wrote:
>> On 20/08/2020 16:34, Roger Pau Monne wrote:
>>> Currently the dpci EOI callback is only executed for specific EOIs.
>>> This is wrong as non-specific EOIs will also clear the ISR bit and
>>> thus end the interrupt. Re-arrange the code a bit so that the common
>>> EOI handling path can be shared between all EOI modes.
>>>
>>> Signed-off-by: Roger Pau Monné 
>>> ---
>>>  xen/arch/x86/hvm/vpic.c | 10 +-
>>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/hvm/vpic.c b/xen/arch/x86/hvm/vpic.c
>>> index feb1db2ee3..3cf12581e9 100644
>>> --- a/xen/arch/x86/hvm/vpic.c
>>> +++ b/xen/arch/x86/hvm/vpic.c
>>> @@ -249,15 +249,15 @@ static void vpic_ioport_write(
>>>  if ( priority == VPIC_PRIO_NONE )
>>>  break;
>>>  pin = (priority + vpic->priority_add) & 7;
>>> -vpic->isr &= ~(1 << pin);
>>> -if ( cmd == 5 )
>>> -vpic->priority_add = (pin + 1) & 7;
>>> -break;
>>> +goto common_eoi;
>>> +
>>>  case 3: /* Specific EOI*/
>>>  case 7: /* Specific EOI & Rotate   */
>>>  pin = val & 7;
>>
>> You'll need a /* Fallthrough */ here to keep various things happy.
> 
> Are you sure? There's ...
> 
>> Otherwise, Acked-by: Andrew Cooper 
>>
>> Can fix on commit if you're happy.
>>
>>> +
>>> +common_eoi:
> 
> ... an ordinary label here, not a case one.

I would have wanted to commit this, but it's still not clear to me
whether the adjustment you ask for is really needed.

Thanks for following up,
Jan

[ovmf test] 154558: all pass - PUSHED

[osstest service owner]

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

Perfect :-)
All tests in this flight passed as required
version targeted for testing:
 ovmf ea9af51479fe04955443f0d366376a1008f07c94
baseline version:
 ovmf 7faece69854cbcc593643182581b5d7f99b7dab6

Last test of basis   154468  2020-09-18 12:10:39 Z2 days
Testing same since   154558  2020-09-21 03:10:34 Z0 days1 attempts


People who touched revisions under test:
  Divneil Rai Wadhawan 

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
   7faece6985..ea9af51479  ea9af51479fe04955443f0d366376a1008f07c94 -> 
xen-tested-master

RE: qemu and Xen ABI-unstable libs

[Paul Durrant]

> -Original Message-
> From: Jan Beulich 
> Sent: 21 September 2020 10:41
> To: p...@xen.org
> Cc: 'Ian Jackson' ; 'Debian folks: Michael Tokarev' 
> ; 'Hans van
> Kranenburg' ; 'Xen upstream folks with an interest: Andrew 
> Cooper'
> ; 'Roger Pau Monné' ; 
> pkg-xen-
> de...@lists.alioth.debian.org; xen-devel@lists.xenproject.org; 'My Xen 
> upstream tools co-maintainer:
> Wei Liu' 
> Subject: Re: qemu and Xen ABI-unstable libs
> 
> On 21.09.2020 09:36, Paul Durrant wrote:
> >> From: Xen-devel  On Behalf Of Ian 
> >> Jackson
> >> Sent: 18 September 2020 17:39
> >>
> >> xc_domain_iomem_permission
> >> xc_domain_populate_physmap_exact
> >> xc_domain_ioport_mapping
> >> xc_domain_memory_mapping
> >>
> >> The things done by these calls in qemu should be done by the Xen
> >> toolstack (libxl), during domain creation etc., instead.
> >
> > I don't think that is practical. E.g. if a guest re-programs a PCI I/O BAR 
> > then it may necessitate
> re-calling
> > xc_domain_ioport_mapping(); the tool-stack cannot know a priori where PCI 
> > BARs will end up in guest
> port/memory space.
> 
> In your reply I assume you meant just the latter two of the four?
> For these I agree, and as a result they shouldn't be domctl in
> the new model.
> 

Sorry if I wasn't clear. Yes, the latter two are what I was referring to.

  Paul

> Jan

Re: qemu and Xen ABI-unstable libs

[Jan Beulich]

On 21.09.2020 09:36, Paul Durrant wrote:
>> From: Xen-devel  On Behalf Of Ian 
>> Jackson
>> Sent: 18 September 2020 17:39
>>
>> xc_domain_iomem_permission
>> xc_domain_populate_physmap_exact
>> xc_domain_ioport_mapping
>> xc_domain_memory_mapping
>>
>> The things done by these calls in qemu should be done by the Xen
>> toolstack (libxl), during domain creation etc., instead.
> 
> I don't think that is practical. E.g. if a guest re-programs a PCI I/O BAR 
> then it may necessitate re-calling
> xc_domain_ioport_mapping(); the tool-stack cannot know a priori where PCI 
> BARs will end up in guest port/memory space.

In your reply I assume you meant just the latter two of the four?
For these I agree, and as a result they shouldn't be domctl in
the new model.

Jan

Re: [PATCH 2/2] qdev: Let BusRealize() return a boolean value to indicate error

[Philippe Mathieu-Daudé]

On 9/21/20 10:19 AM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé  writes:
> 
>> Commit 9940b2cfbc0 introduced qdev_realize() and qbus_realize()
>> with the ability to return a boolean value if an error occured,
>> thus the caller does not need to check if the Error* pointer is
>> set.
>> Provide the same ability to the BusRealize type.
>>
>> Signed-off-by: Philippe Mathieu-Daudé 
>> ---
>>  include/hw/qdev-core.h | 14 +-
>>  hw/hyperv/vmbus.c  |  5 +++--
>>  hw/nubus/nubus-bus.c   |  5 +++--
>>  hw/pci/pci.c   | 12 +---
>>  hw/xen/xen-bus.c   |  5 +++--
>>  5 files changed, 31 insertions(+), 10 deletions(-)
>>
>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>> index 02ac1c50b7f..eecfe794a71 100644
>> --- a/include/hw/qdev-core.h
>> +++ b/include/hw/qdev-core.h
>> @@ -32,7 +32,19 @@ typedef enum DeviceCategory {
>>  typedef void (*DeviceRealize)(DeviceState *dev, Error **errp);
>>  typedef void (*DeviceUnrealize)(DeviceState *dev);
>>  typedef void (*DeviceReset)(DeviceState *dev);
>> -typedef void (*BusRealize)(BusState *bus, Error **errp);
>> +/**
>> + * BusRealize: Realize @bus.
>> + * @bus: bus to realize
>> + * @errp: pointer to error object
>> + *
>> + * On success, return true.
>> + * On failure, store an error through @errp and return false.
>> + */
>> +typedef bool (*BusRealize)(BusState *bus, Error **errp);
>> +/**
>> + * BusUnrealize: Unrealize @bus.
>> + * @bus: bus to unrealize
>> + */
>>  typedef void (*BusUnrealize)(BusState *bus);
>>  
>>  /**
>> diff --git a/hw/hyperv/vmbus.c b/hw/hyperv/vmbus.c
>> index 6ef895bc352..8a0452b2464 100644
>> --- a/hw/hyperv/vmbus.c
>> +++ b/hw/hyperv/vmbus.c
>> @@ -2487,7 +2487,7 @@ static const TypeInfo vmbus_dev_type_info = {
>>  .instance_init = vmbus_dev_instance_init,
>>  };
>>  
>> -static void vmbus_realize(BusState *bus, Error **errp)
>> +static bool vmbus_realize(BusState *bus, Error **errp)
>>  {
>>  int ret = 0;
>>  Error *local_err = NULL;
>> @@ -2519,7 +2519,7 @@ static void vmbus_realize(BusState *bus, Error **errp)
>>  goto clear_event_notifier;
>>  }
>>  
>> -return;
>> +return true;
>>  
>>  clear_event_notifier:
>>  event_notifier_cleanup(>notifier);
>> @@ -2528,6 +2528,7 @@ remove_msg_handler:
>>  error_out:
>>  qemu_mutex_destroy(>rx_queue_lock);
>>  error_propagate(errp, local_err);
>> +return false;
>>  }
>>  
>>  static void vmbus_unrealize(BusState *bus)
>> diff --git a/hw/nubus/nubus-bus.c b/hw/nubus/nubus-bus.c
>> index 942a6d5342d..d20d9c0f72c 100644
>> --- a/hw/nubus/nubus-bus.c
>> +++ b/hw/nubus/nubus-bus.c
>> @@ -65,12 +65,13 @@ static const MemoryRegionOps nubus_super_slot_ops = {
>>  },
>>  };
>>  
>> -static void nubus_realize(BusState *bus, Error **errp)
>> +static bool nubus_realize(BusState *bus, Error **errp)
>>  {
>>  if (!nubus_find()) {
>>  error_setg(errp, "at most one %s device is permitted", 
>> TYPE_NUBUS_BUS);
>> -return;
>> +return false;
>>  }
>> +return true;
>>  }
>>  
>>  static void nubus_init(Object *obj)
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index de0fae10ab9..f535ebac847 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -115,7 +115,7 @@ static void pcibus_machine_done(Notifier *notifier, void 
>> *data)
>>  }
>>  }
>>  
>> -static void pci_bus_realize(BusState *qbus, Error **errp)
>> +static bool pci_bus_realize(BusState *qbus, Error **errp)
>>  {
>>  PCIBus *bus = PCI_BUS(qbus);
>>  
>> @@ -123,13 +123,17 @@ static void pci_bus_realize(BusState *qbus, Error 
>> **errp)
>>  qemu_add_machine_init_done_notifier(>machine_done);
>>  
>>  vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY, _pcibus, bus);
>> +
>> +return true;
>>  }
>>  
>> -static void pcie_bus_realize(BusState *qbus, Error **errp)
>> +static bool pcie_bus_realize(BusState *qbus, Error **errp)
>>  {
>>  PCIBus *bus = PCI_BUS(qbus);
>>  
>> -pci_bus_realize(qbus, errp);
>> +if (!pci_bus_realize(qbus, errp)) {
>> +return false;
>> +}
> 
> We now update bus->flags only when pci_bus_realize() succeeds.  Is this
> a bug fix?

Fortunate side effect :) I'll let the PCI maintainers
have a look at it.

> 
>>  
>>  /*
>>   * A PCI-E bus can support extended config space if it's the root
>> @@ -144,6 +148,8 @@ static void pcie_bus_realize(BusState *qbus, Error 
>> **errp)
>>  bus->flags |= PCI_BUS_EXTENDED_CONFIG_SPACE;
>>  }
>>  }
>> +
>> +return true;
>>  }
>>  
>>  static void pci_bus_unrealize(BusState *qbus)
>> diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
>> index 9ce1c9540b9..d7ef5d05e37 100644
>> --- a/hw/xen/xen-bus.c
>> +++ b/hw/xen/xen-bus.c
>> @@ -444,7 +444,7 @@ static void xen_bus_unrealize(BusState *bus)
>>  }
>>  }
>>  
>> -static void xen_bus_realize(BusState *bus, Error **errp)
>> +static bool xen_bus_realize(BusState *bus, Error **errp)
>>  {
>>  XenBus *xenbus = XEN_BUS(bus);
>>  

[qemu-mainline test] 154552: regressions - FAIL

[osstest service owner]

flight 154552 qemu-mainline real [real]
http://logs.test-lab.xenproject.org/osstest/logs/154552/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-amd64-xl-qcow210 debian-di-installfail REGR. vs. 152631
 test-amd64-i386-xl-qemuu-dmrestrict-amd64-dmrestrict 10 debian-hvm-install 
fail REGR. vs. 152631
 test-amd64-amd64-xl-qemuu-win7-amd64 10 windows-install  fail REGR. vs. 152631
 test-amd64-amd64-qemuu-nested-intel 10 debian-hvm-install fail REGR. vs. 152631
 test-amd64-amd64-xl-qemuu-ovmf-amd64 10 debian-hvm-install fail REGR. vs. 
152631
 test-amd64-amd64-xl-qemuu-debianhvm-amd64-shadow 10 debian-hvm-install fail 
REGR. vs. 152631
 test-amd64-amd64-xl-qemuu-dmrestrict-amd64-dmrestrict 10 debian-hvm-install 
fail REGR. vs. 152631
 test-amd64-i386-xl-qemuu-win7-amd64 10 windows-install   fail REGR. vs. 152631
 test-amd64-i386-xl-qemuu-debianhvm-i386-xsm 10 debian-hvm-install fail REGR. 
vs. 152631
 test-amd64-amd64-qemuu-nested-amd 10 debian-hvm-install  fail REGR. vs. 152631
 test-amd64-amd64-xl-qemuu-ws16-amd64 10 windows-install  fail REGR. vs. 152631
 test-amd64-amd64-xl-qemuu-debianhvm-i386-xsm 10 debian-hvm-install fail REGR. 
vs. 152631
 test-amd64-amd64-xl-qemuu-debianhvm-amd64 10 debian-hvm-install fail REGR. vs. 
152631
 test-amd64-i386-qemuu-rhel6hvm-amd 10 redhat-install fail REGR. vs. 152631
 test-amd64-i386-xl-qemuu-debianhvm-amd64 10 debian-hvm-install fail REGR. vs. 
152631
 test-amd64-i386-xl-qemuu-ws16-amd64 10 windows-install   fail REGR. vs. 152631
 test-amd64-i386-qemuu-rhel6hvm-intel 10 redhat-install   fail REGR. vs. 152631
 test-amd64-i386-xl-qemuu-ovmf-amd64 10 debian-hvm-install fail REGR. vs. 152631
 test-amd64-i386-xl-qemuu-debianhvm-amd64-shadow 10 debian-hvm-install fail 
REGR. vs. 152631
 test-arm64-arm64-libvirt-xsm 12 guest-start  fail REGR. vs. 152631
 test-amd64-amd64-libvirt-vhd 11 guest-start  fail REGR. vs. 152631
 test-armhf-armhf-libvirt 12 guest-start  fail REGR. vs. 152631
 test-armhf-armhf-libvirt-raw 10 debian-di-installfail REGR. vs. 152631
 test-armhf-armhf-xl-vhd  10 debian-di-installfail REGR. vs. 152631

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-xl-rtds 16 guest-start/debian.repeatfail  like 152631
 test-amd64-i386-xl-pvshim12 guest-start  fail   never pass
 test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt 13 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-xsm  13 migrate-support-checkfail   never pass
 test-amd64-i386-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-arm64-arm64-xl-xsm  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl  14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 13 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check 
fail never pass
 test-arm64-arm64-xl-thunderx 14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  14 saverestore-support-checkfail   never pass
 test-amd64-i386-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-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  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail  never pass
 test-armhf-armhf-xl-cubietruck 13 migrate-support-checkfail never pass
 test-armhf-armhf-xl-cubietruck 14 saverestore-support-checkfail never pass
 test-armhf-armhf-xl-credit2  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 14 

Re: [PATCH 2/2] qdev: Let BusRealize() return a boolean value to indicate error

[Markus Armbruster]

Philippe Mathieu-Daudé  writes:

> Commit 9940b2cfbc0 introduced qdev_realize() and qbus_realize()
> with the ability to return a boolean value if an error occured,
> thus the caller does not need to check if the Error* pointer is
> set.
> Provide the same ability to the BusRealize type.
>
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  include/hw/qdev-core.h | 14 +-
>  hw/hyperv/vmbus.c  |  5 +++--
>  hw/nubus/nubus-bus.c   |  5 +++--
>  hw/pci/pci.c   | 12 +---
>  hw/xen/xen-bus.c   |  5 +++--
>  5 files changed, 31 insertions(+), 10 deletions(-)
>
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 02ac1c50b7f..eecfe794a71 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -32,7 +32,19 @@ typedef enum DeviceCategory {
>  typedef void (*DeviceRealize)(DeviceState *dev, Error **errp);
>  typedef void (*DeviceUnrealize)(DeviceState *dev);
>  typedef void (*DeviceReset)(DeviceState *dev);
> -typedef void (*BusRealize)(BusState *bus, Error **errp);
> +/**
> + * BusRealize: Realize @bus.
> + * @bus: bus to realize
> + * @errp: pointer to error object
> + *
> + * On success, return true.
> + * On failure, store an error through @errp and return false.
> + */
> +typedef bool (*BusRealize)(BusState *bus, Error **errp);
> +/**
> + * BusUnrealize: Unrealize @bus.
> + * @bus: bus to unrealize
> + */
>  typedef void (*BusUnrealize)(BusState *bus);
>  
>  /**
> diff --git a/hw/hyperv/vmbus.c b/hw/hyperv/vmbus.c
> index 6ef895bc352..8a0452b2464 100644
> --- a/hw/hyperv/vmbus.c
> +++ b/hw/hyperv/vmbus.c
> @@ -2487,7 +2487,7 @@ static const TypeInfo vmbus_dev_type_info = {
>  .instance_init = vmbus_dev_instance_init,
>  };
>  
> -static void vmbus_realize(BusState *bus, Error **errp)
> +static bool vmbus_realize(BusState *bus, Error **errp)
>  {
>  int ret = 0;
>  Error *local_err = NULL;
> @@ -2519,7 +2519,7 @@ static void vmbus_realize(BusState *bus, Error **errp)
>  goto clear_event_notifier;
>  }
>  
> -return;
> +return true;
>  
>  clear_event_notifier:
>  event_notifier_cleanup(>notifier);
> @@ -2528,6 +2528,7 @@ remove_msg_handler:
>  error_out:
>  qemu_mutex_destroy(>rx_queue_lock);
>  error_propagate(errp, local_err);
> +return false;
>  }
>  
>  static void vmbus_unrealize(BusState *bus)
> diff --git a/hw/nubus/nubus-bus.c b/hw/nubus/nubus-bus.c
> index 942a6d5342d..d20d9c0f72c 100644
> --- a/hw/nubus/nubus-bus.c
> +++ b/hw/nubus/nubus-bus.c
> @@ -65,12 +65,13 @@ static const MemoryRegionOps nubus_super_slot_ops = {
>  },
>  };
>  
> -static void nubus_realize(BusState *bus, Error **errp)
> +static bool nubus_realize(BusState *bus, Error **errp)
>  {
>  if (!nubus_find()) {
>  error_setg(errp, "at most one %s device is permitted", 
> TYPE_NUBUS_BUS);
> -return;
> +return false;
>  }
> +return true;
>  }
>  
>  static void nubus_init(Object *obj)
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index de0fae10ab9..f535ebac847 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -115,7 +115,7 @@ static void pcibus_machine_done(Notifier *notifier, void 
> *data)
>  }
>  }
>  
> -static void pci_bus_realize(BusState *qbus, Error **errp)
> +static bool pci_bus_realize(BusState *qbus, Error **errp)
>  {
>  PCIBus *bus = PCI_BUS(qbus);
>  
> @@ -123,13 +123,17 @@ static void pci_bus_realize(BusState *qbus, Error 
> **errp)
>  qemu_add_machine_init_done_notifier(>machine_done);
>  
>  vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY, _pcibus, bus);
> +
> +return true;
>  }
>  
> -static void pcie_bus_realize(BusState *qbus, Error **errp)
> +static bool pcie_bus_realize(BusState *qbus, Error **errp)
>  {
>  PCIBus *bus = PCI_BUS(qbus);
>  
> -pci_bus_realize(qbus, errp);
> +if (!pci_bus_realize(qbus, errp)) {
> +return false;
> +}

We now update bus->flags only when pci_bus_realize() succeeds.  Is this
a bug fix?

>  
>  /*
>   * A PCI-E bus can support extended config space if it's the root
> @@ -144,6 +148,8 @@ static void pcie_bus_realize(BusState *qbus, Error **errp)
>  bus->flags |= PCI_BUS_EXTENDED_CONFIG_SPACE;
>  }
>  }
> +
> +return true;
>  }
>  
>  static void pci_bus_unrealize(BusState *qbus)
> diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
> index 9ce1c9540b9..d7ef5d05e37 100644
> --- a/hw/xen/xen-bus.c
> +++ b/hw/xen/xen-bus.c
> @@ -444,7 +444,7 @@ static void xen_bus_unrealize(BusState *bus)
>  }
>  }
>  
> -static void xen_bus_realize(BusState *bus, Error **errp)
> +static bool xen_bus_realize(BusState *bus, Error **errp)
>  {
>  XenBus *xenbus = XEN_BUS(bus);
>  unsigned int domid;
> @@ -478,10 +478,11 @@ static void xen_bus_realize(BusState *bus, Error **errp)
>"failed to set up enumeration watch: ");
>  }
>  
> -return;
> +return true;
>  
>  fail:
>  xen_bus_unrealize(bus);
> +return false;
>  }
>  

RE: qemu and Xen ABI-unstable libs

[Paul Durrant]

> -Original Message-
> From: Xen-devel  On Behalf Of Ian 
> Jackson
> Sent: 18 September 2020 17:39
> To: Debian folks: Michael Tokarev ; Hans van Kranenburg 
> ; Xen
> upstream folks with an interest: Andrew Cooper ; 
> Roger Pau Monné
> 
> Cc: pkg-xen-de...@lists.alioth.debian.org; xen-devel@lists.xenproject.org; My 
> Xen upstream tools co-
> maintainer: Wei Liu 
> Subject: RFC: qemu and Xen ABI-unstable libs
> 
> Hi all.  Michael Tokarev has been looking into the problem that qemu
> is using Xen libraries with usntable ABIs.  We did an experiment to
> see which abi-unstable symbols qemu links to, by suppressing libxc
> from the link line.  The results are below.[1]
> 
> Things are not looking too bad.  After some discussion on #xendevel I
> have tried to summarise the situation for each of the troublesome
> symbols.
> 
> Also, we discovered that upstream qemu does not link against any
> abi-unstable Xen libraries if PCI passthrough is disabled.
> 
> Please would my Xen colleages correct me if I have made any mistakes.
> Michael, I hope this is helpful and clear.
> 
> 
> In order from easy to hard:
> 
> 
> xc_domain_shutdown
> 
> This call in qemu needs to be replaced with a call to the existing
> function xendevicemodel_shutdown in libxendevicemodel.  I think it is
> likely that this call is fixed in qemu upstream.
> 

I just pulled QEMU master and it appears that destroy_hvm_domain() is still 
calling xc_domain_shutdown().

> 
> xc_get_hvm_param
> 
> There are three references in qemu's
> xen_get_default_ioreq_server_info, relating to ioreq servers.  These
> uses (and perhaps surrounding code at this function's call site)
> should be replaced by use of xendevicemodel_create_ioreq_server
> etc. from libxendevicemodel.  I think it is likely that this call is
> fixed in qemu upstream.
> 

These references are in compat code for Xen < 4.6. Use of (non-default) ioreq 
server has been present in the code for a long time.
We can remove them by retiring the compat code.

> 
> xc_physdev_map_pirq
> xc_physdev_map_pirq_msi
> xc_physdev_unmap_pirq
> 
> These are all small wrappers for the PHYSDEVOP_map_pirq hypercall.
> PHYSDEVOP is already reasonably abi-stable at the hypervisor level (in
> theory it's versioned, but changing it would break all dom0's).

The hypercalls are non-tools and directly called from the Linux kernel code so 
they are ABI.

> These calls could just be provided as-is by a new stable abi
> entrypoint.  We think this should probably go in libxendevicemodel.
> 

Rather than simply moving this calls into libxendevicemodel, we should think 
about their interactions with calls such as
xc_domain_bind_pt_pci_irq() below and maybe have a stable library that actually 
provides a better API/ABI for interrupt
mapping/triggering although... I've long felt PCI pass-through should not be 
done by QEMU anyway (not least because we currently
have no mechanism for PCI pass-through to PVH domains).

> So, what's needed is to make Xen upstream change to add versions of
> these three functions to tools/libs/devicemodel.  Change qemu to use
> them.
> 
> 
> xc_domain_iomem_permission
> xc_domain_populate_physmap_exact
> xc_domain_ioport_mapping
> xc_domain_memory_mapping
> 
> The things done by these calls in qemu should be done by the Xen
> toolstack (libxl), during domain creation etc., instead.

I don't think that is practical. E.g. if a guest re-programs a PCI I/O BAR then 
it may necessitate re-calling
xc_domain_ioport_mapping(); the tool-stack cannot know a priori where PCI BARs 
will end up in guest port/memory space.

> 
> For at least some of them, there are patches on xen-devel, see
>   From: Grzegorz Uriasz 
>   Subject: [PATCH 1/3] tools/libxl: Grant VGA IO port permission for
>stubdom/target domain
>   Date: Sun, 14 Jun 2020 23:12:01 +0100
> et seq.  These patches have been reviewed and as far as I can tell
> from the thread we are awaiting a resend.
> 

For legacy ranges, such as VGA, it is practical.

> 
> xc_set_hvm_param
> 
> Two calls both relating to HVM_PARAM_ACPI_S_STATE.
> 
> These need to be turned into DMOP hypercalls (ie, new hypercalls added
> to the hypervisor) and entrypoints provided in libxendevicemodel.
> 

Yes, this is certainly a candidate for a DM op.

> 
> xc_domain_bind_pt_pci_irq
> xc_domain_unbind_msi_irq
> xc_domain_unbind_pt_irq
> xc_domain_update_msi_irq
> 
> These are currently XEN_DOMCTL_* hypercalls.  These do not have a
> stable ABI at the hypervisor interface.  AIUI Xen hypervisor folks
> think they should be changed to use the DMOP or PHYSDEVOP hypercalls.
> 
> Additionally, we need calls for these in a userspace library with a
> stable ABI.  We think that should be libxendevicemodel.
> 

What I said above: This needs more consideration.

A while ago I hacked together xenpt 
(https://xenbits.xen.org/gitweb/?p=people/pauldu/xenpt.git), a stand-alone PCI 
pass-through
emulator. One option would be to get this into shape and pull it into the Xen 

RE: [PATCH 2/2] qdev: Let BusRealize() return a boolean value to indicate error

[Paul Durrant]

> -Original Message-
> From: Philippe Mathieu-Daudé  On Behalf Of 
> Philippe Mathieu-Daudé
> Sent: 20 September 2020 12:44
> To: Markus Armbruster ; qemu-de...@nongnu.org
> Cc: Laurent Vivier ; Paolo Bonzini ; 
> Anthony Perard
> ; Stefano Stabellini ; 
> Daniel P. Berrangé
> ; Eduardo Habkost ; Paul Durrant 
> ; Marcel
> Apfelbaum ; Michael S. Tsirkin ; 
> xen-
> de...@lists.xenproject.org; Philippe Mathieu-Daudé 
> Subject: [PATCH 2/2] qdev: Let BusRealize() return a boolean value to 
> indicate error
> 
> Commit 9940b2cfbc0 introduced qdev_realize() and qbus_realize()
> with the ability to return a boolean value if an error occured,
> thus the caller does not need to check if the Error* pointer is
> set.
> Provide the same ability to the BusRealize type.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  include/hw/qdev-core.h | 14 +-
>  hw/hyperv/vmbus.c  |  5 +++--
>  hw/nubus/nubus-bus.c   |  5 +++--
>  hw/pci/pci.c   | 12 +---
>  hw/xen/xen-bus.c   |  5 +++--

Acked-by: Paul Durrant 

[libvirt test] 154561: regressions - FAIL

[osstest service owner]

flight 154561 libvirt real [real]
http://logs.test-lab.xenproject.org/osstest/logs/154561/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 build-amd64-libvirt   6 libvirt-buildfail REGR. vs. 151777
 build-i386-libvirt6 libvirt-buildfail REGR. vs. 151777
 build-arm64-libvirt   6 libvirt-buildfail REGR. vs. 151777
 build-armhf-libvirt   6 libvirt-buildfail REGR. vs. 151777

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-libvirt  1 build-check(1)   blocked  n/a
 test-amd64-amd64-libvirt-pair  1 build-check(1)   blocked  n/a
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a
 test-amd64-amd64-libvirt-vhd  1 build-check(1)   blocked  n/a
 test-amd64-amd64-libvirt-xsm  1 build-check(1)   blocked  n/a
 test-amd64-i386-libvirt   1 build-check(1)   blocked  n/a
 test-amd64-i386-libvirt-pair  1 build-check(1)   blocked  n/a
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a
 test-amd64-i386-libvirt-xsm   1 build-check(1)   blocked  n/a
 test-arm64-arm64-libvirt  1 build-check(1)   blocked  n/a
 test-arm64-arm64-libvirt-qcow2  1 build-check(1)   blocked  n/a
 test-arm64-arm64-libvirt-xsm  1 build-check(1)   blocked  n/a
 test-armhf-armhf-libvirt  1 build-check(1)   blocked  n/a
 test-armhf-armhf-libvirt-raw  1 build-check(1)   blocked  n/a

version targeted for testing:
 libvirt  5be1b028cecb30e5b49cd0b8d06a67564e9128b5
baseline version:
 libvirt  2c846fa6bcc11929c9fb857a22430fb9945654ad

Last test of basis   151777  2020-07-10 04:19:19 Z   73 days
Failing since151818  2020-07-11 04:18:52 Z   72 days   68 attempts
Testing same since   154489  2020-09-19 04:23:06 Z2 days3 attempts


People who touched revisions under test:
  Andika Triwidada 
  Andrea Bolognani 
  Balázs Meskó 
  Bastien Orivel 
  Bihong Yu 
  Binfeng Wu 
  Boris Fiuczynski 
  Christian Ehrhardt 
  Côme Borsoi 
  Daniel Henrique Barboza 
  Daniel P. Berrange 
  Daniel P. Berrangé 
  Erik Skultety 
  Fangge Jin 
  Fedora Weblate Translation 
  Han Han 
  Hao Wang 
  Ian Wienand 
  Jamie Strandboge 
  Jamie Strandboge 
  Jean-Baptiste Holcroft 
  Jianan Gao 
  Jim Fehlig 
  Jin Yan 
  Jiri Denemark 
  Jonathon Jongsma 
  Ján Tomko 
  Kashyap Chamarthy 
  Kevin Locke 
  Laine Stump 
  Liao Pingfang 
  Lin Ma 
  Lin Ma 
  Marek Marczykowski-Górecki 
  Martin Kletzander 
  Matt Coleman 
  Matt Coleman 
  Michal Privoznik 
  Neal Gompa 
  Nikolay Shirokovskiy 
  Patrick Magauran 
  Paulo de Rezende Pinatti 
  Pavel Hrdina 
  Peter Krempa 
  Pino Toscano 
  Pino Toscano 
  Piotr Drąg 
  Prathamesh Chavan 
  Roman Bogorodskiy 
  Ryan Schmidt 
  Sam Hartman 
  Scott Shambarger 
  Sebastian Mitterle 
  Simon Gaiser 
  Stefan Bader 
  Stefan Berger 
  Szymon Scholz 
  Thomas Huth 
  Tim Wiederhake 
  Tomáš Golembiovský 
  Wang Xin 
  Weblate 
  Yang Hang 
  Yanqiu Zhang 
  Yi Li 
  Yi Wang 
  Yuri Chornoivan 
  Zheng Chuan 

jobs:
 build-amd64-xsm  pass
 build-arm64-xsm  pass
 build-i386-xsm   pass
 build-amd64  pass
 build-arm64  pass
 build-armhf  pass
 build-i386   pass
 build-amd64-libvirt  fail
 build-arm64-libvirt  fail
 build-armhf-libvirt  fail
 build-i386-libvirt   fail
 build-amd64-pvopspass
 build-arm64-pvopspass
 build-armhf-pvopspass
 build-i386-pvops pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm   blocked 
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsmblocked 
 test-amd64-amd64-libvirt-xsm blocked 
 test-arm64-arm64-libvirt-xsm blocked 
 test-amd64-i386-libvirt-xsm  blocked 
 test-amd64-amd64-libvirt blocked 
 test-arm64-arm64-libvirt blocked 
 test-armhf-armhf-libvirt blocked 
 test-amd64-i386-libvirt