[linux-linus test] 185824: regressions - FAIL

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-armhf-armhf-xl-credit1   8 xen-boot fail REGR. vs. 185802

Tests which are failing intermittently (not blocking):
 test-armhf-armhf-libvirt  8 xen-bootfail pass in 185827-retest
 test-armhf-armhf-libvirt-vhd  8 xen-bootfail pass in 185827-retest

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-libvirt 16 saverestore-support-check fail in 185827 like 
185802
 test-armhf-armhf-libvirt15 migrate-support-check fail in 185827 never pass
 test-armhf-armhf-libvirt-vhd 14 migrate-support-check fail in 185827 never pass
 test-armhf-armhf-libvirt-vhd 15 saverestore-support-check fail in 185827 never 
pass
 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 185802
 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 185802
 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 185802
 test-armhf-armhf-examine  8 reboot   fail  like 185802
 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 185802
 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 185802
 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt 15 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-arm64-arm64-xl-credit1  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  16 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-qcow2 14 migrate-support-checkfail never pass
 test-armhf-armhf-xl  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail  never pass
 test-armhf-armhf-xl-arndale  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  16 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-raw 14 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-vhd  14 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-vhd  15 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-qcow214 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-qcow215 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-raw  14 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-raw  15 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  16 saverestore-support-checkfail   never pass

version targeted for testing:
 linux3022bf37da50ce0ee3ba443ec5f86fa8c28aacd0
baseline version:
 linuxc942a0cd3603e34dd2d7237e064d9318cb7f9654

Last test of basis   185802  2024-04-26 01:58:59 Z1 days
Testing same since   185824  2024-04-26 18:40:13 Z0 days1 attempts


People who touched revisions under test:
  Alex Deucher 
  Andy Shevchenko 
  Arnd Bergmann 
  Baoquan He 
  Bartosz Golaszewski 
  Bibo Mao 
  Christian Brauner 
  Christian Gmeiner 
  Christian König 
  Dan Williams 
  Dave Airlie 
  Dave Jiang 
  David Hildenbrand 
  

[PATCH] libxl: Fix handling XenStore errors in device creation

[Demi Marie Obenour]

If xenstored runs out of memory it is possible for it to fail operations
that should succeed.  libxl wasn't robust against this, and could fail
to ensure that the TTY path of a non-initial console was created and
read-only for guests.  This doesn't qualify for an XSA because guests
should not be able to run xenstored out of memory, but it still needs to
be fixed.

Add the missing error checks to ensure that all errors are properly
handled and that at no point can a guest make the TTY path of its
frontend directory writable.

Signed-off-by: Demi Marie Obenour 
---
 tools/libs/light/libxl_console.c | 10 ++---
 tools/libs/light/libxl_device.c  | 72 
 tools/libs/light/libxl_xshelp.c  | 13 --
 3 files changed, 59 insertions(+), 36 deletions(-)

diff --git a/tools/libs/light/libxl_console.c b/tools/libs/light/libxl_console.c
index 
cd7412a3272a2faf4b9dab0ef4dd077e55472546..adf82aa844a4f4989111bfc8a94af18ad8e114f1
 100644
--- a/tools/libs/light/libxl_console.c
+++ b/tools/libs/light/libxl_console.c
@@ -351,11 +351,10 @@ int libxl__device_console_add(libxl__gc *gc, uint32_t 
domid,
 flexarray_append(front, "protocol");
 flexarray_append(front, LIBXL_XENCONSOLE_PROTOCOL);
 }
-libxl__device_generic_add(gc, XBT_NULL, device,
-  libxl__xs_kvs_of_flexarray(gc, back),
-  libxl__xs_kvs_of_flexarray(gc, front),
-  libxl__xs_kvs_of_flexarray(gc, ro_front));
-rc = 0;
+rc = libxl__device_generic_add(gc, XBT_NULL, device,
+   libxl__xs_kvs_of_flexarray(gc, back),
+   libxl__xs_kvs_of_flexarray(gc, front),
+   libxl__xs_kvs_of_flexarray(gc, ro_front));
 out:
 return rc;
 }
@@ -665,6 +664,7 @@ int libxl_device_channel_getinfo(libxl_ctx *ctx, uint32_t 
domid,
   */
  if (!val) val = "/NO-SUCH-PATH";
  channelinfo->u.pty.path = strdup(val);
+ if (channelinfo->u.pty.path == NULL) abort();
  break;
  default:
  break;
diff --git a/tools/libs/light/libxl_device.c b/tools/libs/light/libxl_device.c
index 
a3d9f6f7df24b6ce1241c9cf0394a01a42c31b41..4faa5fa3bd115354af0a7ff9785acccd848a51bf
 100644
--- a/tools/libs/light/libxl_device.c
+++ b/tools/libs/light/libxl_device.c
@@ -177,8 +177,13 @@ int libxl__device_generic_add(libxl__gc *gc, 
xs_transaction_t t,
 ro_frontend_perms[1].perms = backend_perms[1].perms = XS_PERM_READ;
 
 retry_transaction:
-if (create_transaction)
+if (create_transaction) {
 t = xs_transaction_start(ctx->xsh);
+if (t == XBT_NULL) {
+LOGED(ERROR, device->domid, "xs_transaction_start failed");
+return ERROR_FAIL;
+}
+}
 
 /* FIXME: read frontend_path and check state before removing stuff */
 
@@ -195,42 +200,55 @@ retry_transaction:
 if (rc) goto out;
 }
 
-/* xxx much of this function lacks error checks! */
-
 if (fents || ro_fents) {
-xs_rm(ctx->xsh, t, frontend_path);
-xs_mkdir(ctx->xsh, t, frontend_path);
+if (!xs_rm(ctx->xsh, t, frontend_path) && errno != ENOENT)
+goto out;
+if (!xs_mkdir(ctx->xsh, t, frontend_path))
+goto out;
 /* Console 0 is a special case. It doesn't use the regular PV
  * state machine but also the frontend directory has
  * historically contained other information, such as the
  * vnc-port, which we don't want the guest fiddling with.
  */
 if ((device->kind == LIBXL__DEVICE_KIND_CONSOLE && device->devid == 0) 
||
-(device->kind == LIBXL__DEVICE_KIND_VUART))
-xs_set_permissions(ctx->xsh, t, frontend_path,
-   ro_frontend_perms, 
ARRAY_SIZE(ro_frontend_perms));
-else
-xs_set_permissions(ctx->xsh, t, frontend_path,
-   frontend_perms, ARRAY_SIZE(frontend_perms));
-xs_write(ctx->xsh, t, GCSPRINTF("%s/backend", frontend_path),
- backend_path, strlen(backend_path));
-if (fents)
-libxl__xs_writev_perms(gc, t, frontend_path, fents,
-   frontend_perms, ARRAY_SIZE(frontend_perms));
-if (ro_fents)
-libxl__xs_writev_perms(gc, t, frontend_path, ro_fents,
-   ro_frontend_perms, 
ARRAY_SIZE(ro_frontend_perms));
+(device->kind == LIBXL__DEVICE_KIND_VUART)) {
+if (!xs_set_permissions(ctx->xsh, t, frontend_path,
+ro_frontend_perms, 
ARRAY_SIZE(ro_frontend_perms)))
+goto out;
+} else {
+if (!xs_set_permissions(ctx->xsh, t, frontend_path,
+frontend_perms, 
ARRAY_SIZE(frontend_perms)))
+goto out;
+}
+if 

Re: [XEN PATCH v1 7/7] x86/MCE: optional build of AMD/Intel MCE code

[Stefano Stabellini]

On Tue, 23 Apr 2024, Sergiy Kibrik wrote:
> Separate Intel/AMD-specific MCE code using CONFIG_{INTEL,AMD} config options.
> Now we can avoid build of mcheck code if support for specific platform is
> intentionally disabled by configuration.
> 
> Signed-off-by: Sergiy Kibrik 
> ---
>  xen/arch/x86/cpu/mcheck/Makefile| 6 ++
>  xen/arch/x86/cpu/mcheck/non-fatal.c | 6 ++
>  xen/arch/x86/cpu/mcheck/vmce.h  | 1 +
>  3 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/x86/cpu/mcheck/Makefile 
> b/xen/arch/x86/cpu/mcheck/Makefile
> index f927f10b4d..5b3f6d875c 100644
> --- a/xen/arch/x86/cpu/mcheck/Makefile
> +++ b/xen/arch/x86/cpu/mcheck/Makefile
> @@ -1,12 +1,10 @@
> -obj-y += amd_nonfatal.o
> -obj-y += mce_amd.o
>  obj-y += mcaction.o
>  obj-y += barrier.o
> -obj-y += intel-nonfatal.o
>  obj-y += mctelem.o
>  obj-y += mce.o
>  obj-y += mce-apei.o
> -obj-y += mce_intel.o
> +obj-$(CONFIG_AMD) += mce_amd.o amd_nonfatal.o
> +obj-$(CONFIG_INTEL) += mce_intel.o intel-nonfatal.o
>  obj-y += non-fatal.o
>  obj-y += util.o
>  obj-y += vmce.o

Awesome!

Reviewed-by: Stefano Stabellini 


> diff --git a/xen/arch/x86/cpu/mcheck/non-fatal.c 
> b/xen/arch/x86/cpu/mcheck/non-fatal.c
> index 33cacd15c2..2d91a3b1e0 100644
> --- a/xen/arch/x86/cpu/mcheck/non-fatal.c
> +++ b/xen/arch/x86/cpu/mcheck/non-fatal.c
> @@ -24,14 +24,20 @@ static int __init cf_check init_nonfatal_mce_checker(void)
>* Check for non-fatal errors every MCE_RATE s
>*/
>   switch (c->x86_vendor) {
> +#ifdef CONFIG_AMD
>   case X86_VENDOR_AMD:
>   case X86_VENDOR_HYGON:
>   /* Assume we are on K8 or newer AMD or Hygon CPU here */
>   amd_nonfatal_mcheck_init(c);
>   break;
> +#endif
> +#ifdef CONFIG_INTEL
>   case X86_VENDOR_INTEL:
>   intel_nonfatal_mcheck_init(c);
>   break;
> +#endif
> + default:
> + return -ENODEV;
>   }
>   printk(KERN_INFO "mcheck_poll: Machine check polling timer started.\n");
>   return 0;

For consistency in all other cases this patch series uses IS_ENABLED
checks. They could be used here as well.

Re: [XEN PATCH v1 6/7] x86/MCE: guard call to Intel-specific intel_get_extended_msrs()

[Stefano Stabellini]

On Tue, 23 Apr 2024, Sergiy Kibrik wrote:
> Add check for CONFIG_INTEL build option to conditional call of this routine,
> so that if Intel support is disabled the call would be eliminated.
> 
> No functional change intended.
> 
> Signed-off-by: Sergiy Kibrik 

Reviewed-by: Stefano Stabellini 

Re: [XEN PATCH v1 5/7] x86/MCE: guard {intel/amd}_mcheck_init() calls

[Stefano Stabellini]

On Tue, 23 Apr 2024, Sergiy Kibrik wrote:
> Guard calls to CPU-specific mcheck init routines in common MCE code
> using new INTEL/AMD config options.
> 
> The purpose is not to build platform-specific mcheck code and calls to it,
> if this platform is disabled in config.
> 
> Signed-off-by: Sergiy Kibrik 

Reviewed-by: Stefano Stabellini 

Re: [XEN PATCH v1 4/7] x86/MCE: guard lmce_support/cmci_support

[Stefano Stabellini]

On Tue, 23 Apr 2024, Sergiy Kibrik wrote:
> Guard access to Intel-specific lmce_support & cmci_support variables in
> common MCE/VMCE code. These are set in Intel-specific parts of mcheck code
> and can potentially be skipped if building for non-intel platform by
> disabling CONFIG_INTEL option.
> 
> Signed-off-by: Sergiy Kibrik 

Reviewed-by: Stefano Stabellini 

Re: [XEN PATCH v1 3/7] x86/MCE: guard access to Intel/AMD-specific MCA MSRs

[Stefano Stabellini]

On Tue, 23 Apr 2024, Sergiy Kibrik wrote:
> Add build-time checks for newly introduced INTEL/AMD config options when
> calling vmce_{intel/amd}_{rdmsr/wrmsr}() routines.
> This way a platform-specific code can be omitted in vmce code, if this
> platform is disabled in config.
> 
> Signed-off-by: Sergiy Kibrik 

Reviewed-by: Stefano Stabellini 

Re: [XEN PATCH v1 2/7] x86/intel: guard vmce_has_lmce() with INTEL option

[Stefano Stabellini]

On Tue, 23 Apr 2024, Sergiy Kibrik wrote:
> Since MCG_LMCE_P bit is specific to Intel CPUs the code to check it can
> possibly be excluded from build if !CONFIG_INTEL. With these guards
> calls to vmce_has_lmce() are eliminated and mce_intel.c can end up
> not being built.
> 
> Also replace boilerplate code that checks for MCG_LMCE_P flag with
> vmce_has_lmce(), which might contribute to readability a bit.
> 
> Signed-off-by: Sergiy Kibrik 

Reviewed-by: Stefano Stabellini 

Re: [XEN PATCH v1 1/7] x86/vpmu: separate amd/intel vPMU code

[Stefano Stabellini]

On Tue, 23 Apr 2024, Sergiy Kibrik wrote:
> Build AMD vPMU when CONFIG_AMD is on, and Intel vPMU when CONFIG_INTEL
> is on respectively, allowing for a plaftorm-specific build. Also separate
> arch_vpmu_ops initializers using these options and static inline stubs.
> 
> No functional change intended.
> 
> Signed-off-by: Sergiy Kibrik 
> CC: Andrew Cooper 
> CC: Jan Beulich 
> 
> ---
> changes in v1:
>  - switch to CONFIG_{AMD,INTEL} instead of CONFIG_{SVM,VMX}
> 
> 
>  xen/arch/x86/cpu/Makefile   |  4 +++-
>  xen/arch/x86/include/asm/vpmu.h | 19 +++
>  2 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/cpu/Makefile b/xen/arch/x86/cpu/Makefile
> index 35561fe51d..eafce5f204 100644
> --- a/xen/arch/x86/cpu/Makefile
> +++ b/xen/arch/x86/cpu/Makefile
> @@ -10,4 +10,6 @@ obj-y += intel.o
>  obj-y += intel_cacheinfo.o
>  obj-y += mwait-idle.o
>  obj-y += shanghai.o
> -obj-y += vpmu.o vpmu_amd.o vpmu_intel.o
> +obj-y += vpmu.o
> +obj-$(CONFIG_AMD) += vpmu_amd.o
> +obj-$(CONFIG_INTEL) += vpmu_intel.o
> diff --git a/xen/arch/x86/include/asm/vpmu.h b/xen/arch/x86/include/asm/vpmu.h
> index dae9b43dac..e7a8f211f8 100644
> --- a/xen/arch/x86/include/asm/vpmu.h
> +++ b/xen/arch/x86/include/asm/vpmu.h
> @@ -11,6 +11,7 @@
>  #define __ASM_X86_HVM_VPMU_H_
>  
>  #include 
> +#include 
>  
>  #define vcpu_vpmu(vcpu)   (&(vcpu)->arch.vpmu)
>  #define vpmu_vcpu(vpmu)   container_of((vpmu), struct vcpu, arch.vpmu)
> @@ -42,9 +43,27 @@ struct arch_vpmu_ops {
>  #endif
>  };
>  
> +#ifdef CONFIG_INTEL
>  const struct arch_vpmu_ops *core2_vpmu_init(void);
> +#else
> +static inline const struct arch_vpmu_ops *core2_vpmu_init(void)
> +{
> +return ERR_PTR(-ENODEV);
> +}
> +#endif
> +#ifdef CONFIG_AMD
>  const struct arch_vpmu_ops *amd_vpmu_init(void);
>  const struct arch_vpmu_ops *hygon_vpmu_init(void);
> +#else
> +static inline const struct arch_vpmu_ops *amd_vpmu_init(void)
> +{
> +return ERR_PTR(-ENODEV);
> +}
> +static inline const struct arch_vpmu_ops *hygon_vpmu_init(void)
> +{
> +return ERR_PTR(-ENODEV);
> +}
> +#endif

At some point we'll need to discuss how to separate out hygon as well.
But for now:

Reviewed-by: Stefano Stabellini 

[PATCH v3] docs/misra: add R21.6 R21.9 R21.10 R21.14 R21.15 R21.16

[Stefano Stabellini]

Signed-off-by: Stefano Stabellini 
---

Changes in v3:
- add explanation in footnote
- remove comment from 21.14, 21.15, 21.16

 docs/misra/rules.rst | 42 ++
 1 file changed, 42 insertions(+)

diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
index b7b447e152..5ba7394f05 100644
--- a/docs/misra/rules.rst
+++ b/docs/misra/rules.rst
@@ -652,12 +652,48 @@ maintainers if you want to suggest a change.
declared
  - See comment for Rule 21.1
 
+   * - `Rule 21.6 
`_
+ - Required
+ - The Standard Library input/output routines shall not be used
+ - Xen doesn't provide, use, or link against a Standard Library [1]_
+
+   * - `Rule 21.9 
`_
+ - Required
+ - The library functions bsearch and qsort of  shall not be used
+ - Xen doesn't provide, use, or link against a Standard Library [1]_
+
+   * - `Rule 21.10 
`_
+ - Required
+ - The Standard Library time and date routines shall not be used
+ - Xen doesn't provide, use, or link against a Standard Library [1]_
+
* - `Rule 21.13 
`_
  - Mandatory
  - Any value passed to a function in  shall be representable as an
unsigned char or be the value EOF
  -
 
+   * - `Rule 21.14 
`_
+ - Required
+ - The Standard Library function memcmp shall not be used to compare
+   null terminated strings
+ -
+
+   * - `Rule 21.15 
`_
+ - Required
+ - The pointer arguments to the Standard Library functions memcpy,
+   memmove and memcmp shall be pointers to qualified or unqualified
+   versions of compatible types
+ -
+
+   * - `Rule 21.16 
`_
+ - Required
+ - The pointer arguments to the Standard Library function memcmp
+   shall point to either a pointer type, an essentially signed type,
+   an essentially unsigned type, an essentially Boolean type or an
+   essentially enum type
+ - void* arguments are allowed
+
* - `Rule 21.17 
`_
  - Mandatory
  - Use of the string handling functions from  shall not result in
@@ -712,3 +748,9 @@ maintainers if you want to suggest a change.
  - The value of a pointer to a FILE shall not be used after the associated
stream has been closed
  -
+
+
+.. [1] Xen implements itself a few functions with names that match the
+   corresponding function names of the Standard Library for developers'
+   convenience. These functions are part of the Xen code and subject to
+   analysis.
-- 
2.25.1

Re: [PATCH v2] docs/misra: add R21.6 R21.9 R21.10 R21.14 R21.15 R21.16

[Stefano Stabellini]

On Fri, 26 Apr 2024, Jan Beulich wrote:
> On 26.04.2024 01:31, Stefano Stabellini wrote:
> > --- a/docs/misra/rules.rst
> > +++ b/docs/misra/rules.rst
> > @@ -652,12 +652,72 @@ maintainers if you want to suggest a change.
> > declared
> >   - See comment for Rule 21.1
> >  
> > +   * - `Rule 21.6 
> > `_
> > + - Required
> > + - The Standard Library input/output routines shall not be used
> > + - Xen doesn't provide, use, or link against any Standard Library.
> > +   Xen implements itself a few functions with names that match the
> > +   corresponding function names of the Standard Library for
> > +   developers' convenience. These functions are part of the Xen code
> > +   and subject to analysis.
> > +
> > +   * - `Rule 21.9 
> > `_
> > + - Required
> > + - The library functions bsearch and qsort of  shall not be 
> > used
> > + - Xen doesn't provide, use, or link against any Standard Library.
> > +   Xen implements itself a few functions with names that match the
> > +   corresponding function names of the Standard Library for
> > +   developers' convenience. These functions are part of the Xen code
> > +   and subject to analysis.
> > +
> > +   * - `Rule 21.10 
> > `_
> > + - Required
> > + - The Standard Library time and date routines shall not be used
> > + - Xen doesn't provide, use, or link against any Standard Library.
> > +   Xen implements itself a few functions with names that match the
> > +   corresponding function names of the Standard Library for
> > +   developers' convenience. These functions are part of the Xen code
> > +   and subject to analysis.
> > +
> > * - `Rule 21.13 
> > `_
> >   - Mandatory
> >   - Any value passed to a function in  shall be representable 
> > as an
> > unsigned char or be the value EOF
> >   -
> 
> Up to here, did you consider adding a short reference to some common blob
> (footnote or alike), rather than repeating the same text verbatim several
> times?

I can look into it


> > +   * - `Rule 21.14 
> > `_
> > + - Required
> > + - The Standard Library function memcmp shall not be used to compare
> > +   null terminated strings
> > + - Xen doesn't provide, use, or link against any Standard Library.
> > +   Xen implements itself a few functions with names that match the
> > +   corresponding function names of the Standard Library for
> > +   developers' convenience. These functions are part of the Xen code
> > +   and subject to analysis.
> > +
> > +   * - `Rule 21.15 
> > `_
> > + - Required
> > + - The pointer arguments to the Standard Library functions memcpy,
> > +   memmove and memcmp shall be pointers to qualified or unqualified
> > +   versions of compatible types
> > + - Xen doesn't provide, use, or link against any Standard Library.
> > +   Xen implements itself a few functions with names that match the
> > +   corresponding function names of the Standard Library for
> > +   developers' convenience. These functions are part of the Xen code
> > +   and subject to analysis.
> > +
> > +   * - `Rule 21.16 
> > `_
> > + - Required
> > + - The pointer arguments to the Standard Library function memcmp
> > +   shall point to either a pointer type, an essentially signed type,
> > +   an essentially unsigned type, an essentially Boolean type or an
> > +   essentially enum type
> > + - void* arguments are allowed. Xen doesn't provide, use, or link
> > +   against any Standard Library.  Xen implements itself a few
> > +   functions with names that match the corresponding function names
> > +   of the Standard Library for developers' convenience. These
> > +   functions are part of the Xen code and subject to analysis.
> 
> For all three of these I'm not convinced the remark is appropriate. These
> talk about specific properties of the functions, which aren't related to
> risks associated with particular (and hence potentially varying) library
> implementations.

Good point

Re: [PATCH] public: xen: Define missing guest handle for int32_t

[Stefano Stabellini]

On Fri, 26 Apr 2024, Jan Beulich wrote:
> On 26.04.2024 00:39, Stefano Stabellini wrote:
> > On Mon, 22 Apr 2024, Julien Grall wrote:
> >> Hi Stefano,
> >>
> >> On 17/04/2024 19:49, Stefano Stabellini wrote:
> >>> On Wed, 17 Apr 2024, Julien Grall wrote:
>  Hi Michal,
> 
>  On 17/04/2024 13:14, Michal Orzel wrote:
> > Commit afab29d0882f ("public: s/int/int32_t") replaced int with int32_t
> > in XEN_GUEST_HANDLE() in memory.h but there is no guest handle defined
> > for it. This results in a build failure. Example on Arm:
> >
> > ./include/public/arch-arm.h:205:41: error: unknown type name
> > ‘__guest_handle_64_int32_t’
> > 205 | #define __XEN_GUEST_HANDLE(name)__guest_handle_64_ ##
> > name
> > | ^~
> > ./include/public/arch-arm.h:206:41: note: in expansion of macro
> > ‘__XEN_GUEST_HANDLE’
> > 206 | #define XEN_GUEST_HANDLE(name)
> > __XEN_GUEST_HANDLE(name)
> > | ^~
> > ./include/public/memory.h:277:5: note: in expansion of macro
> > ‘XEN_GUEST_HANDLE’
> > 277 | XEN_GUEST_HANDLE(int32_t) errs;
> >
> > Fix it. Also, drop guest handle definition for int given no further use.
> >
> > Fixes: afab29d0882f ("public: s/int/int32_t")
> > Signed-off-by: Michal Orzel 
> >>>
> >>> Reviewed-by: Stefano Stabellini 
> >>>
> >>>
>  So it turned out that I committed v1 from Stefano. I was meant to commit
>  the
>  patch at all, but I think I started with a dirty staging :(. Sorry for
>  that.
> 
>  I have reverted Stefano's commit for now so we can take the correct 
>  patch.
> 
>  Now, from my understanding, Andrew suggested on Matrix that this solution
>  may
>  actually be a good way to handle GUEST_HANLDEs (they were removed in v2).
>  Maybe this can be folded in Stefano's patch?
> >>>
> >>> v1 together with Michal's fix is correct. Also v2 alone is correct, or
> >>> v2 with Michal's fix is also correct.
> >>
> >> I am slightly confused, v2 + Michal's fix means that XEN_GUEST_HANDLE(int) 
> >> is
> >> removed and we introduce XEN_GUEST_INT(int32_t) with no user. So wouldn't 
> >> this
> > 
> > You are right I apologize. I looked at Michal's patch too quickly and
> > I thought it was just adding XEN_GUEST_INT(int32_t) without removing
> > anything.
> > 
> > In that case, if you are OK with it, please ack and commit v2 only.
> 
> Just to mention it: Committing would apparently be premature, as I can't spot
> any response to comments I gave to the patch. I'm okay with those being
> addressed verbally only, but imo they cannot be dropped on the floor.

I agree with your comments but I prefer to keep this patch smaller and
focused on doing one thing only. I don't want to mix non-mechanical
changes with the mechanical substitutions. For sure, there will be
follow ups to address your comments and other outstanding issues.

Re: [PATCH 3/3] tools/xentrace: Remove xentrace_format

[Olaf Hering]

Fri, 26 Apr 2024 15:32:31 +0100 George Dunlap :

> Simple remove xentrace_format, and point people to xenalyze instead.

Acked-by: Olaf Hering 


Olaf


pgpysdnaunS_g.pgp
Description: Digitale Signatur von OpenPGP

[PULL 01/38] exec: Rename NEED_CPU_H -> COMPILING_PER_TARGET

[Philippe Mathieu-Daudé]

'NEED_CPU_H' guard target-specific code; it is defined by meson
altogether with the 'CONFIG_TARGET' definition. Rename NEED_CPU_H
as COMPILING_PER_TARGET to clarify its meaning.

Mechanical change running:

 $ sed -i s/NEED_CPU_H/COMPILING_PER_TARGET/g $(git grep -l NEED_CPU_H)

then manually add a /* COMPILING_PER_TARGET */ comment
after the '#endif' when the block is large.

Inspired-by: Peter Maydell 
Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Richard Henderson 
Message-Id: <20240322161439.6448-4-phi...@linaro.org>
---
 meson.build| 4 ++--
 include/exec/cpu-defs.h| 2 +-
 include/exec/helper-head.h | 4 ++--
 include/exec/memop.h   | 4 ++--
 include/exec/memory.h  | 4 ++--
 include/exec/tswap.h   | 4 ++--
 include/gdbstub/helpers.h  | 2 +-
 include/hw/core/cpu.h  | 4 ++--
 include/qemu/osdep.h   | 2 +-
 include/sysemu/hvf.h   | 8 
 include/sysemu/kvm.h   | 6 +++---
 include/sysemu/nvmm.h  | 4 ++--
 include/sysemu/whpx.h  | 4 ++--
 include/sysemu/xen.h   | 4 ++--
 target/arm/kvm-consts.h| 4 ++--
 scripts/analyze-inclusions | 6 +++---
 16 files changed, 33 insertions(+), 33 deletions(-)

diff --git a/meson.build b/meson.build
index 553b940999..96fdc6dfd2 100644
--- a/meson.build
+++ b/meson.build
@@ -3610,7 +3610,7 @@ foreach d, list : target_modules
 if target.endswith('-softmmu')
   config_target = config_target_mak[target]
   target_inc = [include_directories('target' / 
config_target['TARGET_BASE_ARCH'])]
-  c_args = ['-DNEED_CPU_H',
+  c_args = ['-DCOMPILING_PER_TARGET',
 '-DCONFIG_TARGET="@0@-config-target.h"'.format(target),
 '-DCONFIG_DEVICES="@0@-config-devices.h"'.format(target)]
   target_module_ss = module_ss.apply(config_target, strict: false)
@@ -3793,7 +3793,7 @@ foreach target : target_dirs
   target_base_arch = config_target['TARGET_BASE_ARCH']
   arch_srcs = [config_target_h[target]]
   arch_deps = []
-  c_args = ['-DNEED_CPU_H',
+  c_args = ['-DCOMPILING_PER_TARGET',
 '-DCONFIG_TARGET="@0@-config-target.h"'.format(target),
 '-DCONFIG_DEVICES="@0@-config-devices.h"'.format(target)]
   link_args = emulator_link_args
diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h
index 3915438b83..0dbef3010c 100644
--- a/include/exec/cpu-defs.h
+++ b/include/exec/cpu-defs.h
@@ -19,7 +19,7 @@
 #ifndef CPU_DEFS_H
 #define CPU_DEFS_H
 
-#ifndef NEED_CPU_H
+#ifndef COMPILING_PER_TARGET
 #error cpu.h included from common code
 #endif
 
diff --git a/include/exec/helper-head.h b/include/exec/helper-head.h
index 28ceab0a46..5ef467a79d 100644
--- a/include/exec/helper-head.h
+++ b/include/exec/helper-head.h
@@ -43,7 +43,7 @@
 #define dh_ctype_noreturn G_NORETURN void
 #define dh_ctype(t) dh_ctype_##t
 
-#ifdef NEED_CPU_H
+#ifdef COMPILING_PER_TARGET
 # ifdef TARGET_LONG_BITS
 #  if TARGET_LONG_BITS == 32
 #   define dh_alias_tl i32
@@ -54,7 +54,7 @@
 #  endif
 # endif
 # define dh_ctype_tl target_ulong
-#endif
+#endif /* COMPILING_PER_TARGET */
 
 /* We can't use glue() here because it falls foul of C preprocessor
recursive expansion rules.  */
diff --git a/include/exec/memop.h b/include/exec/memop.h
index a86dc6743a..06417ff361 100644
--- a/include/exec/memop.h
+++ b/include/exec/memop.h
@@ -35,7 +35,7 @@ typedef enum MemOp {
 MO_LE= 0,
 MO_BE= MO_BSWAP,
 #endif
-#ifdef NEED_CPU_H
+#ifdef COMPILING_PER_TARGET
 #if TARGET_BIG_ENDIAN
 MO_TE= MO_BE,
 #else
@@ -135,7 +135,7 @@ typedef enum MemOp {
 MO_BESL  = MO_BE | MO_SL,
 MO_BESQ  = MO_BE | MO_SQ,
 
-#ifdef NEED_CPU_H
+#ifdef COMPILING_PER_TARGET
 MO_TEUW  = MO_TE | MO_UW,
 MO_TEUL  = MO_TE | MO_UL,
 MO_TEUQ  = MO_TE | MO_UQ,
diff --git a/include/exec/memory.h b/include/exec/memory.h
index dbb1bad72f..dadb5cd65a 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -3107,7 +3107,7 @@ address_space_write_cached(MemoryRegionCache *cache, 
hwaddr addr,
 MemTxResult address_space_set(AddressSpace *as, hwaddr addr,
   uint8_t c, hwaddr len, MemTxAttrs attrs);
 
-#ifdef NEED_CPU_H
+#ifdef COMPILING_PER_TARGET
 /* enum device_endian to MemOp.  */
 static inline MemOp devend_memop(enum device_endian end)
 {
@@ -3125,7 +3125,7 @@ static inline MemOp devend_memop(enum device_endian end)
 return (end == non_host_endianness) ? MO_BSWAP : 0;
 #endif
 }
-#endif
+#endif /* COMPILING_PER_TARGET */
 
 /*
  * Inhibit technologies that require discarding of pages in RAM blocks, e.g.,
diff --git a/include/exec/tswap.h b/include/exec/tswap.h
index 68944a880b..5089cd6a4c 100644
--- a/include/exec/tswap.h
+++ b/include/exec/tswap.h
@@ -15,11 +15,11 @@
  * If we're in target-specific code, we can hard-code the swapping
  * condition, otherwise we have to do (slower) run-time checks.
  */
-#ifdef NEED_CPU_H
+#ifdef COMPILING_PER_TARGET
 #define target_needs_bswap()  (HOST_BIG_ENDIAN != 

[xen-unstable bisection] complete test-amd64-amd64-livepatch

[osstest service owner]

branch xen-unstable
xenbranch xen-unstable
job test-amd64-amd64-livepatch
testid livepatch-run

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

*** Found and reproduced problem changeset ***

  Bug is in tree:  xen git://xenbits.xen.org/xen.git
  Bug introduced:  62a72092a51792ab74d64ad7454c11e0c22629a2
  Bug not present: fb2716a19190201ffb8d1b20cd9002f166000478
  Last fail repro: http://logs.test-lab.xenproject.org/osstest/logs/185825/


  commit 62a72092a51792ab74d64ad7454c11e0c22629a2
  Author: Roger Pau Monné 
  Date:   Thu Apr 25 09:52:16 2024 +0200
  
  livepatch: introduce --force option
  
  Introduce a xen-livepatch tool --force option, that's propagated into the
  hyerpvisor for livepatch operations.  The intention is for the option to 
be
  used to bypass some checks that would otherwise prevent the patch from 
being
  loaded.
  
  Re purpose the pad field in xen_sysctl_livepatch_op to be a flags field 
that
  applies to all livepatch operations.  The flag is currently only set by 
the
  hypercall wrappers for the XEN_SYSCTL_LIVEPATCH_UPLOAD operation, as 
that's so
  far the only one where it will be used initially.  Other uses can be 
added as
  required.
  
  Note that helpers would set the .pad field to 0, that's been removed 
since the
  structure is already zero initialized at definition.
  
  No functional usages of the new flag introduced in this patch.
  
  Signed-off-by: Roger Pau Monné 
  Acked-by: Jan Beulich 
  Acked-by: Anthony PERARD 


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


Running cs-bisection-step 
--graph-out=/home/logs/results/bisect/xen-unstable/test-amd64-amd64-livepatch.livepatch-run
 --summary-out=tmp/185825.bisection-summary --basis-template=185794 
--blessings=real,real-bisect,real-retry xen-unstable test-amd64-amd64-livepatch 
livepatch-run
Searching for failure / basis pass:
 185806 fail [host=fiano1] / 185794 [host=debina1] 185786 [host=italia1] 185780 
[host=rimava0] 185767 [host=albana0] 185762 [host=nobling1] 185754 
[host=sabro1] 185748 [host=septiner1] 185744 [host=rimava1] 185742 
[host=nobling0] 185741 [host=godello0] 185737 [host=huxelrebe1] 185731 
[host=albana1] 185712 [host=italia0] 185674 [host=godello1] 185635 
[host=fiano0] 185622 [host=septiner0] 185457 [host=debina0] 185386 
[host=himrod0] 185310 [host=huxelrebe0] 185294 [host=sabro0] 185281 
[host=italia1\
 ] 185277 [host=debina1] 185274 [host=albana0] 185271 [host=pinot1] 185268 ok.
Failure / basis pass flights: 185806 / 185268
(tree with no url: minios)
(tree with no url: ovmf)
(tree with no url: seabios)
Tree: linux git://xenbits.xen.org/linux-pvops.git
Tree: linuxfirmware git://xenbits.xen.org/osstest/linux-firmware.git
Tree: qemu git://xenbits.xen.org/qemu-xen-traditional.git
Tree: qemuu git://xenbits.xen.org/qemu-xen.git
Tree: xen git://xenbits.xen.org/xen.git
Latest 6741e066ec7633450d3186946035c1f80c4226b8 
c530a75c1e6a472b0eb9558310b518f0dfcd8860 
3d273dd05e51e5a1ffba3d98c7437ee84e8f8764 
0df9387c8983e1b1e72d8c574356f572342c03e6 
232ee07c23b23fbbafbbf27e475dbbc5b27e4bbb
Basis pass 347385861c50adc8d4801d4b899eded38a2f04cd 
c530a75c1e6a472b0eb9558310b518f0dfcd8860 
3d273dd05e51e5a1ffba3d98c7437ee84e8f8764 
0df9387c8983e1b1e72d8c574356f572342c03e6 
402c2d3e66a6bc9481dcabfc8697750dc4beabed
Generating revisions with ./adhoc-revtuple-generator  
git://xenbits.xen.org/linux-pvops.git#347385861c50adc8d4801d4b899eded38a2f04cd-6741e066ec7633450d3186946035c1f80c4226b8
 
git://xenbits.xen.org/osstest/linux-firmware.git#c530a75c1e6a472b0eb9558310b518f0dfcd8860-c530a75c1e6a472b0eb9558310b518f0dfcd8860
 
git://xenbits.xen.org/qemu-xen-traditional.git#3d273dd05e51e5a1ffba3d98c7437ee84e8f8764-3d273dd05e51e5a1ffba3d98c7437ee84e8f8764
 git://xenbits.xen.org/qemu-xen.git#0df9387c8983e1b1e72d8c574356f57\
 2342c03e6-0df9387c8983e1b1e72d8c574356f572342c03e6 
git://xenbits.xen.org/xen.git#402c2d3e66a6bc9481dcabfc8697750dc4beabed-232ee07c23b23fbbafbbf27e475dbbc5b27e4bbb
Loaded 10001 nodes in revision graph
Searching for test results:
 185253 [host=rimava1]
 185262 [host=italia0]
 185268 pass 347385861c50adc8d4801d4b899eded38a2f04cd 
c530a75c1e6a472b0eb9558310b518f0dfcd8860 
3d273dd05e51e5a1ffba3d98c7437ee84e8f8764 
0df9387c8983e1b1e72d8c574356f572342c03e6 
402c2d3e66a6bc9481dcabfc8697750dc4beabed
 185271 [host=pinot1]
 185274 [host=albana0]
 185277 [host=debina1]
 185281 [host=italia1]
 185294 [host=sabro0]
 185310 [host=huxelrebe0]
 185386 [host=himrod0]
 185457 [host=debina0]
 185508 

Re: MISRA and -Wextra-semi

[Stefano Stabellini]

On Fri, 26 Apr 2024, Andrew Cooper wrote:
> So, while -Wextra-semi is definitely useful to find some hidden
> problems, it seems like using it fully might be very complicated.  How
> much do we care?

I'll let Roberto confirm, but I wouldn't think -Wextra-semi is high
priority

Re: [PATCH v2 1/1] xen/arm64: entry: Add missing code symbol annotations

[Stefano Stabellini]

On Fri, 26 Apr 2024, Jan Beulich wrote:
> On 26.04.2024 01:13, Stefano Stabellini wrote:
> > On Tue, 16 Apr 2024, Edgar E. Iglesias wrote:
> >> From: "Edgar E. Iglesias" 
> >>
> >> Use the generic xen/linkage.h macros when and add missing
> > ^ when what?
> > 
> >> code symbol annotations.
> >>
> >> Signed-off-by: Edgar E. Iglesias 
> > 
> > I am looking at the implementation of FUNC and as far as I can tell
> > there is no change compared to ENTRY. So from that point of view we are
> > good. I wonder if we should keep using "ENTRY" because it is nice to
> > mark explicitely the entry points as such but at the same time I am also
> > OK with this. I'll let the other ARM maintainers decide.
> 
> Just to mention it: ENTRY should go away (and hence why PPC and RISC-V had
> it dropped already, while x86 has patches pending to reduce its scope
> enough), not the least to finally allow the oddity near the top of xen.lds.S
> to go away.

I didn't realize that. OK, understood.

Re: [XEN PATCH v3 5/5] xen/arm: ffa: support notification

[Julien Grall]

Hi Jens,

On 26/04/2024 09:47, Jens Wiklander wrote:

diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index d7306aa64d50..5224898265a5 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -155,7 +155,7 @@ void __init init_IRQ(void)
  {
  /* SGIs are always edge-triggered */
  if ( irq < NR_GIC_SGI )
-local_irqs_type[irq] = DT_IRQ_TYPE_EDGE_RISING;
+local_irqs_type[irq] = IRQ_TYPE_EDGE_RISING;


This changes seems unrelated to this patch. This wants to be separate 
(if you actually intended to change it).



  else
  local_irqs_type[irq] = IRQ_TYPE_INVALID;
  }
diff --git a/xen/arch/arm/tee/Makefile b/xen/arch/arm/tee/Makefile
index f0112a2f922d..7c0f46f7f446 100644
--- a/xen/arch/arm/tee/Makefile
+++ b/xen/arch/arm/tee/Makefile
@@ -2,5 +2,6 @@ obj-$(CONFIG_FFA) += ffa.o
  obj-$(CONFIG_FFA) += ffa_shm.o
  obj-$(CONFIG_FFA) += ffa_partinfo.o
  obj-$(CONFIG_FFA) += ffa_rxtx.o
+obj-$(CONFIG_FFA) += ffa_notif.o
  obj-y += tee.o
  obj-$(CONFIG_OPTEE) += optee.o
diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
index 5209612963e1..912e905a27bd 100644
--- a/xen/arch/arm/tee/ffa.c
+++ b/xen/arch/arm/tee/ffa.c
@@ -39,6 +39,9 @@
   *   - at most 32 shared memory regions per guest
   * o FFA_MSG_SEND_DIRECT_REQ:
   *   - only supported from a VM to an SP
+ * o FFA_NOTIFICATION_*:
+ *   - only supports global notifications, that is, per vCPU notifications
+ * are not supported
   *
   * There are some large locked sections with ffa_tx_buffer_lock and
   * ffa_rx_buffer_lock. Especially the ffa_tx_buffer_lock spinlock used
@@ -194,6 +197,8 @@ out:
  
  static void handle_features(struct cpu_user_regs *regs)

  {
+struct domain *d = current->domain;
+struct ffa_ctx *ctx = d->arch.tee;
  uint32_t a1 = get_user_reg(regs, 1);
  unsigned int n;
  
@@ -240,6 +245,30 @@ static void handle_features(struct cpu_user_regs *regs)

  BUILD_BUG_ON(PAGE_SIZE != FFA_PAGE_SIZE);
  ffa_set_regs_success(regs, 0, 0);
  break;
+case FFA_FEATURE_NOTIF_PEND_INTR:
+if ( ctx->notif.enabled )
+ffa_set_regs_success(regs, GUEST_FFA_NOTIF_PEND_INTR_ID, 0);
+else
+ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
+break;
+case FFA_FEATURE_SCHEDULE_RECV_INTR:
+if ( ctx->notif.enabled )
+ffa_set_regs_success(regs, GUEST_FFA_SCHEDULE_RECV_INTR_ID, 0);
+else
+ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
+break;
+
+case FFA_NOTIFICATION_BIND:
+case FFA_NOTIFICATION_UNBIND:
+case FFA_NOTIFICATION_GET:
+case FFA_NOTIFICATION_SET:
+case FFA_NOTIFICATION_INFO_GET_32:
+case FFA_NOTIFICATION_INFO_GET_64:
+if ( ctx->notif.enabled )
+ffa_set_regs_success(regs, 0, 0);
+else
+ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
+break;
  default:
  ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
  break;
@@ -305,6 +334,22 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
   get_user_reg(regs, 1)),
 get_user_reg(regs, 3));
  break;
+case FFA_NOTIFICATION_BIND:
+e = ffa_handle_notification_bind(regs);
+break;
+case FFA_NOTIFICATION_UNBIND:
+e = ffa_handle_notification_unbind(regs);
+break;
+case FFA_NOTIFICATION_INFO_GET_32:
+case FFA_NOTIFICATION_INFO_GET_64:
+ffa_handle_notification_info_get(regs);
+return true;
+case FFA_NOTIFICATION_GET:
+ffa_handle_notification_get(regs);
+return true;
+case FFA_NOTIFICATION_SET:
+e = ffa_handle_notification_set(regs);
+break;
  
  default:

  gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
@@ -322,6 +367,7 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
  static int ffa_domain_init(struct domain *d)
  {
  struct ffa_ctx *ctx;
+int ret;
  
  if ( !ffa_version )

  return -ENODEV;
@@ -345,10 +391,11 @@ static int ffa_domain_init(struct domain *d)
   * error, so no need for cleanup in this function.
   */
  
-if ( !ffa_partinfo_domain_init(d) )

-return -EIO;
+ret = ffa_partinfo_domain_init(d);
+if ( ret )
+return ret;
  
-return 0;

+return ffa_notif_domain_init(d);
  }
  
  static void ffa_domain_teardown_continue(struct ffa_ctx *ctx, bool first_time)

@@ -423,6 +470,7 @@ static int ffa_domain_teardown(struct domain *d)
  return 0;
  
  ffa_rxtx_domain_destroy(d);

+ffa_notif_domain_destroy(d);
  
  ffa_domain_teardown_continue(ctx, true /* first_time */);
  
@@ -502,6 +550,7 @@ static bool ffa_probe(void)

  if ( !ffa_partinfo_init() )
  goto err_rxtx_destroy;
  
+ffa_notif_init();

  INIT_LIST_HEAD(_teardown_head);
  init_timer(_teardown_timer, 

Re: [PATCH] xen/x86: Fix Syntax warning in gen-cpuid.py

[Andrew Cooper]

On 26/04/2024 5:07 am, Jason Andryuk wrote:
> Python 3.12.2 warns:
>
> xen/tools/gen-cpuid.py:50: SyntaxWarning: invalid escape sequence '\s'
>   "\s+([\s\d]+\*[\s\d]+\+[\s\d]+)\)"
> xen/tools/gen-cpuid.py:51: SyntaxWarning: invalid escape sequence '\s'
>   "\s+/\*([\w!]*) .*$")
>
> Specify the strings as raw strings so '\s' is read as literal '\' + 's'.
> This avoids escaping all the '\'s in the strings.
>
> Signed-off-by: Jason Andryuk 

That's something I didn't know about how python does string
concatenation.  I was expecting the whole string to be considered raw,
not just the first line.

Acked-by: Andrew Cooper 

I'll rebase my pending change altering the regex over this.

[xen-unstable test] 185806: regressions - FAIL

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-armhf-armhf-xl-raw  broken  in 185821
 test-amd64-amd64-livepatch   13 livepatch-runfail REGR. vs. 185794
 test-armhf-armhf-xl-raw  12 debian-di-installfail REGR. vs. 185794

Tests which are failing intermittently (not blocking):
 test-armhf-armhf-xl-raw  5 host-install(5) broken in 185821 pass in 185806
 test-amd64-amd64-xl-qemut-debianhvm-i386-xsm 12 debian-hvm-install fail pass 
in 185821-retest

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-libvirt 16 saverestore-support-checkfail  like 185794
 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 185794
 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 185794
 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 185794
 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 185794
 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 185794
 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  16 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail  never pass
 test-arm64-arm64-xl-credit2  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-qcow2 14 migrate-support-checkfail never pass
 test-armhf-armhf-xl-credit2  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  16 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-raw 14 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt 15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  16 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-vhd  14 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-vhd  15 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-qcow214 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-qcow215 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt-vhd 14 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-vhd 15 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail   never pass

version targeted for testing:
 xen  232ee07c23b23fbbafbbf27e475dbbc5b27e4bbb
baseline version:
 xen  31d6b5b4a7c84818294dacca7a07e92f52393c9d

Last test of basis   185794  2024-04-25 05:57:18 Z1 days
Failing since185800  2024-04-25 20:10:40 Z0 days2 attempts
Testing same since   185806  2024-04-26 06:40:39 Z0 days1 attempts


People who touched revisions under test:
  Alessandro Zucchelli 
  Andrew Cooper 
  Anthony PERARD 
  Christian Lindig 
 

[PATCH] xen/x86: Fix Syntax warning in gen-cpuid.py

[Jason Andryuk]

Python 3.12.2 warns:

xen/tools/gen-cpuid.py:50: SyntaxWarning: invalid escape sequence '\s'
  "\s+([\s\d]+\*[\s\d]+\+[\s\d]+)\)"
xen/tools/gen-cpuid.py:51: SyntaxWarning: invalid escape sequence '\s'
  "\s+/\*([\w!]*) .*$")

Specify the strings as raw strings so '\s' is read as literal '\' + 's'.
This avoids escaping all the '\'s in the strings.

Signed-off-by: Jason Andryuk 
---
 xen/tools/gen-cpuid.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/tools/gen-cpuid.py b/xen/tools/gen-cpuid.py
index bf3f9ec01e..dadeb33080 100755
--- a/xen/tools/gen-cpuid.py
+++ b/xen/tools/gen-cpuid.py
@@ -47,8 +47,8 @@ def parse_definitions(state):
 """
 feat_regex = re.compile(
 r"^XEN_CPUFEATURE\(([A-Z0-9_]+),"
-"\s+([\s\d]+\*[\s\d]+\+[\s\d]+)\)"
-"\s+/\*([\w!]*) .*$")
+r"\s+([\s\d]+\*[\s\d]+\+[\s\d]+)\)"
+r"\s+/\*([\w!]*) .*$")
 
 word_regex = re.compile(
 r"^/\* .* word (\d*) \*/$")
-- 
2.44.0

[PATCH] xen/xsm: Wire up get_dom0_console

[Jason Andryuk]

An XSM hook for get_dom0_console is currently missing.  Using XSM with
a PVH dom0 shows:
(XEN) FLASK: Denying unknown platform_op: 64.

Wire up the hook, and allow it for dom0.

Fixes: 4dd160583c ("x86/platform: introduce hypercall to get initial video 
console settings")
Signed-off-by: Jason Andryuk 
---
 tools/flask/policy/modules/dom0.te  | 2 +-
 xen/xsm/flask/hooks.c   | 4 
 xen/xsm/flask/policy/access_vectors | 2 ++
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/flask/policy/modules/dom0.te 
b/tools/flask/policy/modules/dom0.te
index f1dcff48e2..16b8c9646d 100644
--- a/tools/flask/policy/modules/dom0.te
+++ b/tools/flask/policy/modules/dom0.te
@@ -16,7 +16,7 @@ allow dom0_t xen_t:xen {
 allow dom0_t xen_t:xen2 {
resource_op psr_cmt_op psr_alloc pmu_ctrl get_symbol
get_cpu_levelling_caps get_cpu_featureset livepatch_op
-   coverage_op
+   coverage_op get_dom0_console
 };
 
 # Allow dom0 to use all XENVER_ subops that have checks.
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 78225f68c1..5e88c71b8e 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -1558,6 +1558,10 @@ static int cf_check flask_platform_op(uint32_t op)
 return avc_has_perm(domain_sid(current->domain), SECINITSID_XEN,
 SECCLASS_XEN2, XEN2__GET_SYMBOL, NULL);
 
+case XENPF_get_dom0_console:
+return avc_has_perm(domain_sid(current->domain), SECINITSID_XEN,
+SECCLASS_XEN2, XEN2__GET_DOM0_CONSOLE, NULL);
+
 default:
 return avc_unknown_permission("platform_op", op);
 }
diff --git a/xen/xsm/flask/policy/access_vectors 
b/xen/xsm/flask/policy/access_vectors
index 4e6710a63e..a35e3d4c51 100644
--- a/xen/xsm/flask/policy/access_vectors
+++ b/xen/xsm/flask/policy/access_vectors
@@ -99,6 +99,8 @@ class xen2
 livepatch_op
 # XEN_SYSCTL_coverage_op
 coverage_op
+# XENPF_get_dom0_console
+get_dom0_console
 }
 
 # Classes domain and domain2 consist of operations that a domain performs on
-- 
2.44.0

Re: [XEN PATCH v3 5/5] xen/arm: ffa: support notification

[Julien Grall]

Hi Bertrand,

On 26/04/2024 10:20, Bertrand Marquis wrote:

+static inline struct domain *ffa_get_domain_by_vm_id(uint16_t vm_id)
+{
+/* -1 to match ffa_get_vm_id() */
+return get_domain_by_id(vm_id - 1);
+}
+


I think there is a global discussion to have on using get_domain_by_vm_id
or rcu_lock_live_remote_domain_by_id to make sure the domain is not
dying when we try to do something with it.

It does not need to be done here as there are other places to adapt but
i wanted to raise the question.

I would like to know what you and also other maintainers think here.
@Julien/Michal/Stefano ?


Good question. I think the intention is get_domain_by_id() should be 
called when you need a reference for longer.


I would consider to involve the REST in this discussion.

Cheers,

--
Julien Grall

Re: [XEN PATCH v3 5/5] xen/arm: ffa: support notification

[Julien Grall]

Hi Jens,

On 26/04/2024 09:47, Jens Wiklander wrote:

+static void notif_irq_enable(void *info)
+{
+struct notif_irq_info *irq_info = info;
+
+irq_info->ret = setup_irq(irq_info->irq, 0, irq_info->action);
In v2, you were using request_irq(). But now you seem to be open-coding 
it. Can you explain why?



+if ( irq_info->ret )
+printk(XENLOG_ERR "ffa: request_irq irq %u failed: error %d\n",
+   irq_info->irq, irq_info->ret);
+}
+
+void ffa_notif_init(void)
+{
+const struct arm_smccc_1_2_regs arg = {
+.a0 = FFA_FEATURES,
+.a1 = FFA_FEATURE_SCHEDULE_RECV_INTR,
+};
+struct notif_irq_info irq_info = { };
+struct arm_smccc_1_2_regs resp;
+unsigned int cpu;
+
+arm_smccc_1_2_smc(, );
+if ( resp.a0 != FFA_SUCCESS_32 )
+return;
+
+irq_info.irq = resp.a2;
+if ( irq_info.irq < GIC_SGI_STATIC_MAX || irq_info.irq >= NR_GIC_SGI )
+{
+printk(XENLOG_ERR "ffa: notification initialization failed: conflicting SGI 
%u\n",
+   irq_info.irq);
+return;
+}
+
+/*
+ * SGIs are per-CPU so we must enable the IRQ on each CPU. We use an
+ * IPI to call notif_irq_enable() on each CPU including the current
+ * CPU. The struct irqaction is preallocated since we can't allocate
+ * memory while in interrupt context.
+ */
+for_each_online_cpu(cpu)
Even though we currently don't support CPU hotplug, you want to add a 
CPU Notifier to also register the IRQ when a CPU is onlined 
ffa_notif_init().


For an example, see time.c. We may also want to consider to enable TEE 
in presmp_initcalls() so we don't need to have a for_each_online_cpu().


Cheers,

--
Julien Grall

[PATCH v6 6/7] [DO NOT APPLY] switch to qemu fork

[Marek Marczykowski-Górecki]

This makes tests to use patched QEMU, to actually test the new behavior.
---
 Config.mk | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Config.mk b/Config.mk
index a962f095ca16..5e220a1284e4 100644
--- a/Config.mk
+++ b/Config.mk
@@ -220,8 +220,8 @@ endif
 OVMF_UPSTREAM_URL ?= https://xenbits.xen.org/git-http/ovmf.git
 OVMF_UPSTREAM_REVISION ?= ba91d0292e593df8528b66f99c1b0b14fadc8e16
 
-QEMU_UPSTREAM_URL ?= https://xenbits.xen.org/git-http/qemu-xen.git
-QEMU_UPSTREAM_REVISION ?= master
+QEMU_UPSTREAM_URL ?= https://github.com/marmarek/qemu
+QEMU_UPSTREAM_REVISION ?= origin/msix
 
 MINIOS_UPSTREAM_URL ?= https://xenbits.xen.org/git-http/mini-os.git
 MINIOS_UPSTREAM_REVISION ?= b6a5b4d72b88e5c4faed01f5a44505de022860fc
-- 
git-series 0.9.1

[PATCH v6 3/7] x86/hvm: Allow access to registers on the same page as MSI-X table

[Marek Marczykowski-Górecki]

Some devices (notably Intel Wifi 6 AX210 card) keep auxiliary registers
on the same page as MSI-X table. Device model (especially one in
stubdomain) cannot really handle those, as direct writes to that page is
refused (page is on the mmio_ro_ranges list). Instead, extend
msixtbl_mmio_ops to handle such accesses too.

Doing this, requires correlating read/write location with guest
of MSI-X table address. Since QEMU doesn't map MSI-X table to the guest,
it requires msixtbl_entry->gtable, which is HVM-only. Similar feature
for PV would need to be done separately.

This will be also used to read Pending Bit Array, if it lives on the same
page, making QEMU not needing /dev/mem access at all (especially helpful
with lockdown enabled in dom0). If PBA lives on another page, QEMU will
map it to the guest directly.
If PBA lives on the same page, discard writes and log a message.
Technically, writes outside of PBA could be allowed, but at this moment
the precise location of PBA isn't saved, and also no known device abuses
the spec in this way (at least yet).

To access those registers, msixtbl_mmio_ops need the relevant page
mapped. MSI handling already has infrastructure for that, using fixmap,
so try to map first/last page of the MSI-X table (if necessary) and save
their fixmap indexes. Note that msix_get_fixmap() does reference
counting and reuses existing mapping, so just call it directly, even if
the page was mapped before. Also, it uses a specific range of fixmap
indexes which doesn't include 0, so use 0 as default ("not mapped")
value - which simplifies code a bit.

GCC 12.2.1 gets confused about 'desc' variable:

arch/x86/hvm/vmsi.c: In function ‘msixtbl_range’:
arch/x86/hvm/vmsi.c:553:8: error: ‘desc’ may be used uninitialized 
[-Werror=maybe-uninitialized]
  553 | if ( desc )
  |^
arch/x86/hvm/vmsi.c:537:28: note: ‘desc’ was declared here
  537 | const struct msi_desc *desc;
  |^~~~

It's conditional initialization is actually correct (in the case where
it isn't initialized, function returns early), but to avoid
build failure initialize it explicitly to NULL anyway.

Signed-off-by: Marek Marczykowski-Górecki 
---
Changes in v6:
- use MSIX_CHECK_WARN macro
- extend assert on fixmap_idx
- add break in default label, after ASSERT_UNREACHABLE(), and move
  setting default there
- style fixes
Changes in v5:
- style fixes
- include GCC version in the commit message
- warn only once (per domain, per device) about failed adjacent access
Changes in v4:
- drop same_page parameter of msixtbl_find_entry(), distinguish two
  cases in relevant callers
- rename adj_access_table_idx to adj_access_idx
- code style fixes
- drop alignment check in adjacent_{read,write}() - all callers already
  have it earlier
- delay mapping first/last MSI-X pages until preparing device for a
  passthrough
v3:
 - merge handling into msixtbl_mmio_ops
 - extend commit message
v2:
 - adjust commit message
 - pass struct domain to msixtbl_page_handler_get_hwaddr()
 - reduce local variables used only once
 - log a warning if write is forbidden if MSI-X and PBA lives on the same
   page
 - do not passthrough unaligned accesses
 - handle accesses both before and after MSI-X table
---
 xen/arch/x86/hvm/vmsi.c| 200 --
 xen/arch/x86/include/asm/msi.h |   5 +-
 xen/arch/x86/msi.c |  41 +++-
 3 files changed, 236 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
index 17983789..230e3a5dee3f 100644
--- a/xen/arch/x86/hvm/vmsi.c
+++ b/xen/arch/x86/hvm/vmsi.c
@@ -180,6 +180,10 @@ static bool msixtbl_initialised(const struct domain *d)
 return d->arch.hvm.msixtbl_list.next;
 }
 
+/*
+ * Lookup an msixtbl_entry on the same page as given addr. It's up to the
+ * caller to check if address is strictly part of the table - if relevant.
+ */
 static struct msixtbl_entry *msixtbl_find_entry(
 struct vcpu *v, unsigned long addr)
 {
@@ -187,8 +191,8 @@ static struct msixtbl_entry *msixtbl_find_entry(
 struct domain *d = v->domain;
 
 list_for_each_entry( entry, >arch.hvm.msixtbl_list, list )
-if ( addr >= entry->gtable &&
- addr < entry->gtable + entry->table_len )
+if ( PFN_DOWN(addr) >= PFN_DOWN(entry->gtable) &&
+ PFN_DOWN(addr) <= PFN_DOWN(entry->gtable + entry->table_len - 1) )
 return entry;
 
 return NULL;
@@ -213,6 +217,138 @@ static struct msi_desc *msixtbl_addr_to_desc(
 return NULL;
 }
 
+/*
+ * Returns:
+ *  - ADJACENT_DONT_HANDLE if no handling should be done
+ *  - ADJACENT_DISCARD_WRITE if write should be discarded
+ *  - a fixmap idx to use for handling
+ */
+#define ADJACENT_DONT_HANDLE UINT_MAX
+#define ADJACENT_DISCARD_WRITE (UINT_MAX - 1)
+static unsigned int adjacent_handle(
+const struct msixtbl_entry *entry, unsigned long addr, bool write)
+{
+unsigned int adj_type;
+struct 

[PATCH v6 0/7] MSI-X support with qemu in stubdomain, and other related changes

[Marek Marczykowski-Górecki]

This series includes changes to make MSI-X working with Linux stubdomain and
especially Intel Wifi 6 AX210 card. This takes care of remaining reasons for
QEMU to access /dev/mem, but also the Intel Wifi card violating spec by putting
some registers on the same page as the MSI-X table.
Besides the stubdomain case (of which I care more), this is also necessary for
PCI-passthrough to work with lockdown enabled in dom0 (when QEMU runs in dom0).

See individual patches for details.

This series include also tests for MSI-X using new approach (by preventing QEMU
access to /dev/mem). But for it to work, it needs QEMU change that
makes use of the changes introduced here. It can be seen at
https://github.com/marmarek/qemu/commits/msix

Here is the pipeline that used the QEMU fork above:
https://gitlab.com/xen-project/people/marmarek/xen/-/pipelines/1269664578

Marek Marczykowski-Górecki (7):
  x86/msi: passthrough all MSI-X vector ctrl writes to device model
  x86/msi: Extend per-domain/device warning mechanism
  x86/hvm: Allow access to registers on the same page as MSI-X table
  automation: prevent QEMU access to /dev/mem in PCI passthrough tests
  automation: switch to a wifi card on ADL system
  [DO NOT APPLY] switch to qemu fork
  [DO NOT APPLY] switch to alternative artifact repo

 Config.mk   |   4 +-
 automation/gitlab-ci/build.yaml |   4 +-
 automation/gitlab-ci/test.yaml  |   4 +-
 automation/scripts/qubes-x86-64.sh  |   9 +-
 automation/tests-artifacts/alpine/3.18.dockerfile   |   7 +-
 automation/tests-artifacts/kernel/6.1.19.dockerfile |   2 +-
 xen/arch/x86/hvm/vmsi.c | 215 -
 xen/arch/x86/include/asm/msi.h  |  22 +-
 xen/arch/x86/msi.c  |  46 ++-
 xen/common/kernel.c |   1 +-
 xen/include/public/features.h   |   8 +-
 11 files changed, 300 insertions(+), 22 deletions(-)

base-commit: 7846f7699fea25502061a05ea847e942ea624f12
-- 
git-series 0.9.1

[PATCH v6 7/7] [DO NOT APPLY] switch to alternative artifact repo

[Marek Marczykowski-Górecki]

For testing, switch to my containers registry that includes containers
rebuilt with changes in this series.
---
 automation/gitlab-ci/build.yaml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/automation/gitlab-ci/build.yaml b/automation/gitlab-ci/build.yaml
index aac29ee13ab6..6957f06016b7 100644
--- a/automation/gitlab-ci/build.yaml
+++ b/automation/gitlab-ci/build.yaml
@@ -320,7 +320,7 @@ qemu-system-ppc64-8.1.0-ppc64-export:
 
 alpine-3.18-rootfs-export:
   extends: .test-jobs-artifact-common
-  image: registry.gitlab.com/xen-project/xen/tests-artifacts/alpine:3.18
+  image: 
registry.gitlab.com/xen-project/people/marmarek/xen/tests-artifacts/alpine:3.18
   script:
 - mkdir binaries && cp /initrd.tar.gz binaries/initrd.tar.gz
   artifacts:
@@ -331,7 +331,7 @@ alpine-3.18-rootfs-export:
 
 kernel-6.1.19-export:
   extends: .test-jobs-artifact-common
-  image: registry.gitlab.com/xen-project/xen/tests-artifacts/kernel:6.1.19
+  image: 
registry.gitlab.com/xen-project/people/marmarek/xen/tests-artifacts/kernel:6.1.19
   script:
 - mkdir binaries && cp /bzImage binaries/bzImage
   artifacts:
-- 
git-series 0.9.1

[PATCH v6 2/7] x86/msi: Extend per-domain/device warning mechanism

[Marek Marczykowski-Górecki]

The arch_msix struct had a single "warned" field with a domid for which
warning was issued. Upcoming patch will need similar mechanism for few
more warnings, so change it to save a bit field of issued warnings.

Signed-off-by: Marek Marczykowski-Górecki 
---
Changes in v6:
- add MSIX_CHECK_WARN macro (Jan)
- drop struct name from warned_kind union (Jan)

New in v5
---
 xen/arch/x86/include/asm/msi.h | 17 -
 xen/arch/x86/msi.c |  5 +
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/include/asm/msi.h b/xen/arch/x86/include/asm/msi.h
index 997ccb87be0c..bcfdfd35345d 100644
--- a/xen/arch/x86/include/asm/msi.h
+++ b/xen/arch/x86/include/asm/msi.h
@@ -208,6 +208,15 @@ struct msg_address {
PCI_MSIX_ENTRY_SIZE + \
(~PCI_MSIX_BIRMASK & (PAGE_SIZE - 1)))
 
+#define MSIX_CHECK_WARN(msix, domid, which) ({ \
+if ( (msix)->warned_domid != (domid) ) \
+{ \
+(msix)->warned_domid = (domid); \
+(msix)->warned_kind.all = 0; \
+} \
+(msix)->warned_kind.which ? false : ((msix)->warned_kind.which = true); \
+})
+
 struct arch_msix {
 unsigned int nr_entries, used_entries;
 struct {
@@ -217,7 +226,13 @@ struct arch_msix {
 int table_idx[MAX_MSIX_TABLE_PAGES];
 spinlock_t table_lock;
 bool host_maskall, guest_maskall;
-domid_t warned;
+domid_t warned_domid;
+union {
+uint8_t all;
+struct {
+bool maskall   : 1;
+};
+} warned_kind;
 };
 
 void early_msi_init(void);
diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
index e721aaf5c001..42c793426da3 100644
--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -364,13 +364,10 @@ static bool msi_set_mask_bit(struct irq_desc *desc, bool 
host, bool guest)
 domid_t domid = pdev->domain->domain_id;
 
 maskall = true;
-if ( pdev->msix->warned != domid )
-{
-pdev->msix->warned = domid;
+if ( MSIX_CHECK_WARN(pdev->msix, domid, maskall) )
 printk(XENLOG_G_WARNING
"cannot mask IRQ %d: masking MSI-X on Dom%d's %pp\n",
desc->irq, domid, >sbdf);
-}
 }
 pdev->msix->host_maskall = maskall;
 if ( maskall || pdev->msix->guest_maskall )
-- 
git-series 0.9.1

[PATCH v6 5/7] automation: switch to a wifi card on ADL system

[Marek Marczykowski-Górecki]

Switch to a wifi card that has registers on a MSI-X page. This tests the
"x86/hvm: Allow writes to registers on the same page as MSI-X table"
feature. Switch it only for HVM test, because MSI-X adjacent write is
not supported on PV.

This requires also including drivers and firmware in system for tests.
Remove firmware unrelated to the test, to not increase initrd size too
much (all firmware takes over 100MB compressed).
And finally adjusts test script to handle not only eth0 as a test device,
but also wlan0 and connect it to the wifi network.

Signed-off-by: Marek Marczykowski-Górecki 
Reviewed-by: Stefano Stabellini 
---
This needs two new gitlab variables: WIFI_HW2_SSID and WIFI_HW2_PSK. I'll
provide them in private.

This change requires rebuilding test containers.

This can be applied only after QEMU change is committed. Otherwise the
test will fail.
---
 automation/gitlab-ci/test.yaml  | 4 
 automation/scripts/qubes-x86-64.sh  | 7 +++
 automation/tests-artifacts/alpine/3.18.dockerfile   | 7 +++
 automation/tests-artifacts/kernel/6.1.19.dockerfile | 2 ++
 4 files changed, 20 insertions(+)

diff --git a/automation/gitlab-ci/test.yaml b/automation/gitlab-ci/test.yaml
index 8b7b2e4da92d..23a183fad967 100644
--- a/automation/gitlab-ci/test.yaml
+++ b/automation/gitlab-ci/test.yaml
@@ -193,6 +193,10 @@ adl-pci-pv-x86-64-gcc-debug:
 
 adl-pci-hvm-x86-64-gcc-debug:
   extends: .adl-x86-64
+  variables:
+PCIDEV: "00:14.3"
+WIFI_SSID: "$WIFI_HW2_SSID"
+WIFI_PSK: "$WIFI_HW2_PSK"
   script:
 - ./automation/scripts/qubes-x86-64.sh pci-hvm 2>&1 | tee ${LOGFILE}
   needs:
diff --git a/automation/scripts/qubes-x86-64.sh 
b/automation/scripts/qubes-x86-64.sh
index 7eabc1bd6ad4..60498ef1e89a 100755
--- a/automation/scripts/qubes-x86-64.sh
+++ b/automation/scripts/qubes-x86-64.sh
@@ -94,6 +94,13 @@ on_reboot = "destroy"
 domU_check="
 set -x -e
 interface=eth0
+if [ -e /sys/class/net/wlan0 ]; then
+interface=wlan0
+set +x
+wpa_passphrase "$WIFI_SSID" "$WIFI_PSK" > /etc/wpa_supplicant.conf
+set -x
+wpa_supplicant -B -iwlan0 -c /etc/wpa_supplicant.conf
+fi
 ip link set \"\$interface\" up
 timeout 30s udhcpc -i \"\$interface\"
 pingip=\$(ip -o -4 r show default|cut -f 3 -d ' ')
diff --git a/automation/tests-artifacts/alpine/3.18.dockerfile 
b/automation/tests-artifacts/alpine/3.18.dockerfile
index 9cde6c9ad4da..c323e266c7da 100644
--- a/automation/tests-artifacts/alpine/3.18.dockerfile
+++ b/automation/tests-artifacts/alpine/3.18.dockerfile
@@ -34,6 +34,13 @@ RUN \
   apk add udev && \
   apk add pciutils && \
   apk add libelf && \
+  apk add wpa_supplicant && \
+  # Select firmware for hardware tests
+  apk add linux-firmware-other && \
+  mkdir /lib/firmware-preserve && \
+  mv /lib/firmware/iwlwifi-so-a0-gf-a0* /lib/firmware-preserve/ && \
+  rm -rf /lib/firmware && \
+  mv /lib/firmware-preserve /lib/firmware && \
   \
   # Xen
   cd / && \
diff --git a/automation/tests-artifacts/kernel/6.1.19.dockerfile 
b/automation/tests-artifacts/kernel/6.1.19.dockerfile
index 3a4096780d20..84ed5dff23ae 100644
--- a/automation/tests-artifacts/kernel/6.1.19.dockerfile
+++ b/automation/tests-artifacts/kernel/6.1.19.dockerfile
@@ -32,6 +32,8 @@ RUN curl -fsSLO 
https://cdn.kernel.org/pub/linux/kernel/v6.x/linux-"$LINUX_VERSI
 make xen.config && \
 scripts/config --enable BRIDGE && \
 scripts/config --enable IGC && \
+scripts/config --enable IWLWIFI && \
+scripts/config --enable IWLMVM && \
 cp .config .config.orig && \
 cat .config.orig | grep XEN | grep =m |sed 's/=m/=y/g' >> .config && \
 make -j$(nproc) bzImage && \
-- 
git-series 0.9.1

[PATCH v6 4/7] automation: prevent QEMU access to /dev/mem in PCI passthrough tests

[Marek Marczykowski-Górecki]

/dev/mem access doesn't work in dom0 in lockdown and in stubdomain.
Simulate this environment with removing /dev/mem device node. Full test
for lockdown and stubdomain will come later, when all requirements will
be in place.

Signed-off-by: Marek Marczykowski-Górecki 
Acked-by: Stefano Stabellini 
---
This can be applied only after QEMU change is committed. Otherwise the
test will fail.
---
 automation/scripts/qubes-x86-64.sh | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/automation/scripts/qubes-x86-64.sh 
b/automation/scripts/qubes-x86-64.sh
index d81ed7b931cf..7eabc1bd6ad4 100755
--- a/automation/scripts/qubes-x86-64.sh
+++ b/automation/scripts/qubes-x86-64.sh
@@ -163,6 +163,8 @@ ifconfig eth0 up
 ifconfig xenbr0 up
 ifconfig xenbr0 192.168.0.1
 
+# ensure QEMU wont have access /dev/mem
+rm -f /dev/mem
 # get domU console content into test log
 tail -F /var/log/xen/console/guest-domU.log 2>/dev/null | sed -e \"s/^/(domU) 
/\" &
 xl create /etc/xen/domU.cfg
-- 
git-series 0.9.1

[PATCH v6 1/7] x86/msi: passthrough all MSI-X vector ctrl writes to device model

[Marek Marczykowski-Górecki]

QEMU needs to know whether clearing maskbit of a vector is really
clearing, or was already cleared before. Currently Xen sends only
clearing that bit to the device model, but not setting it, so QEMU
cannot detect it. Because of that, QEMU is working this around by
checking via /dev/mem, but that isn't the proper approach.

Give all necessary information to QEMU by passing all ctrl writes,
including masking a vector. Advertise the new behavior via
XENVER_get_features, so QEMU can know it doesn't need to access /dev/mem
anymore.

While this commit doesn't move the whole maskbit handling to QEMU (as
discussed on xen-devel as one of the possibilities), it is a necessary
first step anyway. Including telling QEMU it will get all the required
information to do so. The actual implementation would need to include:
 - a hypercall for QEMU to control just maskbit (without (re)binding the
   interrupt again
 - a methor for QEMU to tell Xen it will actually do the work
Those are not part of this series.

Signed-off-by: Marek Marczykowski-Górecki 
Reviewed-by: Jan Beulich 
---
I did not added any control to enable/disable this new behavior (as
Roger have suggested for possible non-QEMU ioreqs). I don't see how the
new behavior could be problematic for some existing ioreq server (they
already received writes to those addresses, just not all of them),
but if that's really necessary, I can probably add a command line option
to restore previous behavior system-wide.

Changes in v5:
- announce the feature only on x86
- style fixes

Changes in v4:
- ignore unaligned writes with X86EMUL_OKAY
- restructure the code to forward all writes in _msixtbl_write() instead
  of manipulating return value of msixtbl_write() - this makes
  WRITE_LEN4_COMPLETION special case unnecessary
- advertise the changed behavior via XENVER_get_features instead of DMOP
v3:
 - advertise changed behavior in XEN_DMOP_get_ioreq_server_info - make
   "flags" parameter IN/OUT
 - move len check back to msixtbl_write() - will be needed there anyway
   in a later patch
v2:
 - passthrough quad writes to emulator too (Jan)
 - (ab)use len==0 for write len=4 completion (Jan), but add descriptive
   #define for this magic value
---
 xen/arch/x86/hvm/vmsi.c   | 19 ++-
 xen/common/kernel.c   |  1 +
 xen/include/public/features.h |  8 
 3 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
index adbac965f9f7..17983789 100644
--- a/xen/arch/x86/hvm/vmsi.c
+++ b/xen/arch/x86/hvm/vmsi.c
@@ -283,8 +283,8 @@ static int msixtbl_write(struct vcpu *v, unsigned long 
address,
 unsigned long flags;
 struct irq_desc *desc;
 
-if ( (len != 4 && len != 8) || (address & (len - 1)) )
-return r;
+if ( !IS_ALIGNED(address, len) )
+return X86EMUL_OKAY;
 
 rcu_read_lock(_rcu_lock);
 
@@ -345,8 +345,7 @@ static int msixtbl_write(struct vcpu *v, unsigned long 
address,
 
 unlock:
 spin_unlock_irqrestore(>lock, flags);
-if ( len == 4 )
-r = X86EMUL_OKAY;
+r = X86EMUL_OKAY;
 
 out:
 rcu_read_unlock(_rcu_lock);
@@ -357,7 +356,17 @@ static int cf_check _msixtbl_write(
 const struct hvm_io_handler *handler, uint64_t address, uint32_t len,
 uint64_t val)
 {
-return msixtbl_write(current, address, len, val);
+/* Ignore invalid length or unaligned writes. */
+if ( (len != 4 && len != 8) || !IS_ALIGNED(address, len) )
+return X86EMUL_OKAY;
+
+/*
+ * This function returns X86EMUL_UNHANDLEABLE even if write is properly
+ * handled, to propagate it to the device model (so it can keep its
+ * internal state in sync).
+ */
+msixtbl_write(current, address, len, val);
+return X86EMUL_UNHANDLEABLE;
 }
 
 static bool cf_check msixtbl_range(
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index 08dbaa2a054c..b44b2439ca8e 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -637,6 +637,7 @@ long do_xen_version(int cmd, XEN_GUEST_HANDLE_PARAM(void) 
arg)
 fi.submap |= (1U << XENFEAT_hvm_safe_pvclock) |
  (1U << XENFEAT_hvm_callback_vector) |
  (has_pirq(d) ? (1U << XENFEAT_hvm_pirqs) : 0);
+fi.submap |= (1U << XENFEAT_dm_msix_all_writes);
 #endif
 if ( !paging_mode_translate(d) || is_domain_direct_mapped(d) )
 fi.submap |= (1U << XENFEAT_direct_mapped);
diff --git a/xen/include/public/features.h b/xen/include/public/features.h
index 4437f25d2532..880193094713 100644
--- a/xen/include/public/features.h
+++ b/xen/include/public/features.h
@@ -120,6 +120,14 @@
 #define XENFEAT_runstate_phys_area18
 #define XENFEAT_vcpu_time_phys_area   19
 
+/*
+ * If set, Xen will passthrough all MSI-X vector ctrl writes to device model,
+ * not only those unmasking an entry. This allows device model to properly keep
+ * track of the MSI-X table without having to read it from 

[PATCH] x86/cpu-policy: Annotate the accumulated features

[Andrew Cooper]

Some features need accumulating rather than intersecting to make migration
safe.  Introduce the new '|' attribute for this purpose.

Right now, it's only used by the Xapi toolstack, but it will be used by
xl/libxl when the full policy-object work is complete, and until then it's
still a useful hint for hand-crafted cpuid= lines in vm.cfg files.

Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 
CC: Roger Pau Monné 
---
 xen/include/public/arch-x86/cpufeatureset.h | 15 ++-
 xen/tools/gen-cpuid.py  |  7 +--
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/xen/include/public/arch-x86/cpufeatureset.h 
b/xen/include/public/arch-x86/cpufeatureset.h
index 53f13dec31f7..6627453e3985 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -72,6 +72,11 @@ enum {
  *   'H' = HVM HAP guests (not PV or HVM Shadow guests).
  *   Upper case => Available by default
  *   Lower case => Can be opted-in to, but not available by default.
+ *
+ * Migration: '|'
+ *   This bit should be visible to a guest if any anywhere it might run has
+ *   the bit set.  i.e. it needs accumulating across the migration pool,
+ *   rather than intersecting.
  */
 
 /* Intel-defined CPU features, CPUID level 0x0001.edx, word 0 */
@@ -248,7 +253,7 @@ XEN_CPUFEATURE(IBRS_ALWAYS,   8*32+16) /*S  IBRS preferred 
always on */
 XEN_CPUFEATURE(STIBP_ALWAYS,  8*32+17) /*S  STIBP preferred always on */
 XEN_CPUFEATURE(IBRS_FAST, 8*32+18) /*S  IBRS preferred over software 
options */
 XEN_CPUFEATURE(IBRS_SAME_MODE, 8*32+19) /*S  IBRS provides same-mode 
protection */
-XEN_CPUFEATURE(NO_LMSL,   8*32+20) /*S  EFER.LMSLE no longer supported. */
+XEN_CPUFEATURE(NO_LMSL,   8*32+20) /*S| EFER.LMSLE no longer supported. */
 XEN_CPUFEATURE(AMD_PPIN,  8*32+23) /*   Protected Processor Inventory 
Number */
 XEN_CPUFEATURE(AMD_SSBD,  8*32+24) /*S  MSR_SPEC_CTRL.SSBD available */
 XEN_CPUFEATURE(VIRT_SSBD, 8*32+25) /*!  MSR_VIRT_SPEC_CTRL.SSBD */
@@ -263,7 +268,7 @@ XEN_CPUFEATURE(AVX512_4FMAPS, 9*32+ 3) /*A  AVX512 Multiply 
Accumulation Single
 XEN_CPUFEATURE(FSRM,  9*32+ 4) /*A  Fast Short REP MOVS */
 XEN_CPUFEATURE(AVX512_VP2INTERSECT, 9*32+8) /*a  VP2INTERSECT{D,Q} insns */
 XEN_CPUFEATURE(SRBDS_CTRL,9*32+ 9) /*   MSR_MCU_OPT_CTRL and 
RNGDS_MITG_DIS. */
-XEN_CPUFEATURE(MD_CLEAR,  9*32+10) /*!A VERW clears microarchitectural 
buffers */
+XEN_CPUFEATURE(MD_CLEAR,  9*32+10) /*!A| VERW clears microarchitectural 
buffers */
 XEN_CPUFEATURE(RTM_ALWAYS_ABORT, 9*32+11) /*! RTM disabled (but XBEGIN wont 
fault) */
 XEN_CPUFEATURE(TSX_FORCE_ABORT, 9*32+13) /* MSR_TSX_FORCE_ABORT.RTM_ABORT */
 XEN_CPUFEATURE(SERIALIZE, 9*32+14) /*A  SERIALIZE insn */
@@ -292,7 +297,7 @@ XEN_CPUFEATURE(AVX_IFMA, 10*32+23) /*A  AVX-IFMA 
Instructions */
 
 /* AMD-defined CPU features, CPUID level 0x8021.eax, word 11 */
 XEN_CPUFEATURE(NO_NEST_BP, 11*32+ 0) /*A  No Nested Data Breakpoints */
-XEN_CPUFEATURE(FS_GS_NS,   11*32+ 1) /*S  FS/GS base MSRs 
non-serialising */
+XEN_CPUFEATURE(FS_GS_NS,   11*32+ 1) /*S| FS/GS base MSRs 
non-serialising */
 XEN_CPUFEATURE(LFENCE_DISPATCH,11*32+ 2) /*A  LFENCE always serializing */
 XEN_CPUFEATURE(NSCB,   11*32+ 6) /*A  Null Selector Clears Base 
(and limit too) */
 XEN_CPUFEATURE(AUTO_IBRS,  11*32+ 8) /*S  Automatic IBRS */
@@ -343,7 +348,7 @@ XEN_CPUFEATURE(DOITM,  16*32+12) /*   Data 
Operand Invariant Timing
 XEN_CPUFEATURE(SBDR_SSDP_NO,   16*32+13) /*A  No Shared Buffer Data Read 
or Sideband Stale Data Propagation */
 XEN_CPUFEATURE(FBSDP_NO,   16*32+14) /*A  No Fill Buffer Stale Data 
Propagation */
 XEN_CPUFEATURE(PSDP_NO,16*32+15) /*A  No Primary Stale Data 
Propagation */
-XEN_CPUFEATURE(FB_CLEAR,   16*32+17) /*!A Fill Buffers cleared by VERW 
*/
+XEN_CPUFEATURE(FB_CLEAR,   16*32+17) /*!A| Fill Buffers cleared by 
VERW */
 XEN_CPUFEATURE(FB_CLEAR_CTRL,  16*32+18) /*   
MSR_OPT_CPU_CTRL.FB_CLEAR_DIS */
 XEN_CPUFEATURE(RRSBA,  16*32+19) /*!  Restricted RSB Alternative */
 XEN_CPUFEATURE(BHI_NO, 16*32+20) /*A  No Branch History Injection  
*/
@@ -353,7 +358,7 @@ XEN_CPUFEATURE(PBRSB_NO,   16*32+24) /*A  No 
Post-Barrier RSB prediction
 XEN_CPUFEATURE(GDS_CTRL,   16*32+25) /*   
MCU_OPT_CTRL.GDS_MIT_{DIS,LOCK} */
 XEN_CPUFEATURE(GDS_NO, 16*32+26) /*A  No Gather Data Sampling */
 XEN_CPUFEATURE(RFDS_NO,16*32+27) /*A  No Register File Data 
Sampling */
-XEN_CPUFEATURE(RFDS_CLEAR, 16*32+28) /*!A Register File(s) cleared by 
VERW */
+XEN_CPUFEATURE(RFDS_CLEAR, 16*32+28) /*!A| Register File(s) cleared by 
VERW */
 
 /* Intel-defined CPU features, MSR_ARCH_CAPS 0x10a.edx, word 17 */
 
diff --git a/xen/tools/gen-cpuid.py b/xen/tools/gen-cpuid.py
index bf3f9ec01e6e..1fb76f664529 100755
--- 

Re: [PATCH v2 1/2] net: Provide MemReentrancyGuard * to qemu_new_nic()

[BALATON Zoltan]

On Fri, 26 Apr 2024, Philippe Mathieu-Daudé wrote:

On 26/4/24 14:37, Akihiko Odaki wrote:

On 2024/04/24 21:32, Thomas Huth wrote:

On 24/04/2024 12.41, Prasad Pandit wrote:
On Wednesday, 24 April, 2024 at 03:36:01 pm IST, Philippe Mathieu-Daudé 
wrote:

On 1/6/23 05:18, Akihiko Odaki wrote:

Recently MemReentrancyGuard was added to DeviceState to record that the
device is engaging in I/O. The network device backend needs to update 
it

when delivering a packet to a device.
In preparation for such a change, add MemReentrancyGuard * as a
parameter of qemu_new_nic().


An user on IRC asked if this patch is related/fixing CVE-2021-20255,
any clue?


* CVE-2021-20255 bug: infinite recursion is pointing at a different fix 
patch.

   -> https://bugzilla.redhat.com/show_bug.cgi?id=CVE-2021-20255

* And the this patch below has different issue tagged
-> https://lists.nongnu.org/archive/html/qemu-devel/2023-05/msg08312.html
   Fixes: CVE-2023-3019


* They look different, former is an infinite recursion issue and the 
latter is a use-after-free one.


I assume the eepro reentrancy issue has been fixed with:

  https://gitlab.com/qemu-project/qemu/-/issues/556
  i.e.:
  https://gitlab.com/qemu-project/qemu/-/commit/c40ca2301c7603524eaddb5308a3


I agree. Commit c40ca2301c7603524eaddb5308a3 should be what fixed 
CVE-2021-20255, not this patch.


Thank you all for clarifying!


$ git log -p c40ca2301c7603524eaddb5308a3 --
fatal: bad revision 'c40ca2301c7603524eaddb5308a3'

It seems to actually be commit a2e1753b8054344f32cf94f31c6399a58794a380

Regards,
BALATON Zoltan

Re: [PATCH 1/3] x86/hvm/trace: Use a different trace type for AMD processors

[Andrew Cooper]

On 26/04/2024 4:29 pm, George Dunlap wrote:
> On Fri, Apr 26, 2024 at 4:18 PM Andrew Cooper  
> wrote:
>> On 26/04/2024 3:32 pm, George Dunlap wrote:
>>> In xenalyze, first remove the redundant call to init_hvm_data();
>>> there's no way to get to hvm_vmexit_process() without it being already
>>> initialized by the set_vcpu_type call in hvm_process().
>>>
>>> Replace this with set_hvm_exit_reson_data(), and move setting of
>>> hvm->exit_reason_* into that function.
>>>
>>> Modify hvm_process and hvm_vmexit_process to handle all four potential
>>> values appropriately.
>>>
>>> If SVM entries are encountered, set opt.svm_mode so that other
>>> SVM-specific functionality is triggered.
>> Given that xenalyze is now closely tied to Xen, and that we're
>> technically changing the ABI here, is there any point keeping `--svm-mode` ?
>>
>> I'm unsure of the utility of reading the buggy trace records from an
>> older version of Xen.
> Yeah, I thought about that.  If nobody argues to keep it, I guess I'll
> rip it out for v2.

That's the way I'd suggest going.
>>> Signed-off-by: George Dunlap 
>>> ---
>>> NB that this patch goes on top of Andrew's trace cleanup series:
>>>
>>> https://lore.kernel.org/xen-devel/20240318163552.3808695-1-andrew.coop...@citrix.com/
>> The delta in Xen is trivial.  I'm happy if you want to commit this, and
>> I can rebase over it.
> It's trivial in part *because of* your series.  Without your series I
> would have had to do a bunch of munging around with DO_TRC_BLAH_BLAH
> to make it compile.  In fact, I started doing it directly on staging,
> but quickly moved onto your series to save myself some time. :-)

Ah.  I'll be refreshing mine next week.  Given that it's missed
everything since 4.16, I'm not intending to let it miss this one...

But I've still got a few things which need to go out before the
past-post deadline.

~Andrew

Re: [PATCH 1/3] x86/hvm/trace: Use a different trace type for AMD processors

[George Dunlap]

On Fri, Apr 26, 2024 at 4:18 PM Andrew Cooper  wrote:
>
> On 26/04/2024 3:32 pm, George Dunlap wrote:
> > A long-standing usability sub-optimality with xenalyze is the
> > necessity to specify `--svm-mode` when analyzing AMD processors.  This
> > fundamentally comes about because the same trace event ID is used for
> > both VMX and SVM, but the contents of the trace must be interpreted
> > differently.
> >
> > Instead, allocate separate trace events for VMX and SVM vmexits in
> > Xen; this will allow all readers to properly intrepret the meaning of
>
> interpret ?

Oops, yes.

> > In xenalyze, first remove the redundant call to init_hvm_data();
> > there's no way to get to hvm_vmexit_process() without it being already
> > initialized by the set_vcpu_type call in hvm_process().
> >
> > Replace this with set_hvm_exit_reson_data(), and move setting of
> > hvm->exit_reason_* into that function.
> >
> > Modify hvm_process and hvm_vmexit_process to handle all four potential
> > values appropriately.
> >
> > If SVM entries are encountered, set opt.svm_mode so that other
> > SVM-specific functionality is triggered.
>
> Given that xenalyze is now closely tied to Xen, and that we're
> technically changing the ABI here, is there any point keeping `--svm-mode` ?
>
> I'm unsure of the utility of reading the buggy trace records from an
> older version of Xen.

Yeah, I thought about that.  If nobody argues to keep it, I guess I'll
rip it out for v2.

> > Also add lines in `formats` for xentrace_format.
>
> Personally I'd have put patch 3 first, and reduced the churn here.

I actually meant to add an `RFC` to the third patch.  I'd already
implemented this by the time you suggested ripping it out.  Doing it
this way doesn't add much overhead if we do end up ripping out
xentrace_format, and does save time if we don't.

> > Signed-off-by: George Dunlap 
> > ---
> > NB that this patch goes on top of Andrew's trace cleanup series:
> >
> > https://lore.kernel.org/xen-devel/20240318163552.3808695-1-andrew.coop...@citrix.com/
>
> The delta in Xen is trivial.  I'm happy if you want to commit this, and
> I can rebase over it.

It's trivial in part *because of* your series.  Without your series I
would have had to do a bunch of munging around with DO_TRC_BLAH_BLAH
to make it compile.  In fact, I started doing it directly on staging,
but quickly moved onto your series to save myself some time. :-)

Since I'm planning on re-submitting your series if you don't (unless
you really object), this way should minimize churn.

I'm happy to post a branch to gitlab if anyone wants to see the combined result.

 -George

Re: [PATCH v5 3/7] x86/hvm: Allow access to registers on the same page as MSI-X table

[Marek Marczykowski-Górecki]

On Thu, Apr 25, 2024 at 01:15:34PM +0200, Jan Beulich wrote:
> On 13.03.2024 16:16, Marek Marczykowski-Górecki wrote:
> > Some devices (notably Intel Wifi 6 AX210 card) keep auxiliary registers
> > on the same page as MSI-X table. Device model (especially one in
> > stubdomain) cannot really handle those, as direct writes to that page is
> > refused (page is on the mmio_ro_ranges list). Instead, extend
> > msixtbl_mmio_ops to handle such accesses too.
> > 
> > Doing this, requires correlating read/write location with guest
> > of MSI-X table address. Since QEMU doesn't map MSI-X table to the guest,
> > it requires msixtbl_entry->gtable, which is HVM-only. Similar feature
> > for PV would need to be done separately.
> > 
> > This will be also used to read Pending Bit Array, if it lives on the same
> > page, making QEMU not needing /dev/mem access at all (especially helpful
> > with lockdown enabled in dom0). If PBA lives on another page, QEMU will
> > map it to the guest directly.
> > If PBA lives on the same page, discard writes and log a message.
> > Technically, writes outside of PBA could be allowed, but at this moment
> > the precise location of PBA isn't saved, and also no known device abuses
> > the spec in this way (at least yet).
> > 
> > To access those registers, msixtbl_mmio_ops need the relevant page
> > mapped. MSI handling already has infrastructure for that, using fixmap,
> > so try to map first/last page of the MSI-X table (if necessary) and save
> > their fixmap indexes. Note that msix_get_fixmap() does reference
> > counting and reuses existing mapping, so just call it directly, even if
> > the page was mapped before. Also, it uses a specific range of fixmap
> > indexes which doesn't include 0, so use 0 as default ("not mapped")
> > value - which simplifies code a bit.
> > 
> > GCC 12.2.1 gets confused about 'desc' variable:
> > 
> > arch/x86/hvm/vmsi.c: In function ‘msixtbl_range’:
> > arch/x86/hvm/vmsi.c:553:8: error: ‘desc’ may be used uninitialized 
> > [-Werror=maybe-uninitialized]
> >   553 | if ( desc )
> >   |^
> > arch/x86/hvm/vmsi.c:537:28: note: ‘desc’ was declared here
> >   537 | const struct msi_desc *desc;
> >   |^~~~
> > 
> > It's conditional initialization is actually correct (in the case where
> > it isn't initialized, function returns early), but to avoid
> > build failure initialize it explicitly to NULL anyway.
> > 
> > Signed-off-by: Marek Marczykowski-Górecki 
> 
> Sadly there are further more or less cosmetic issues. Plus, as indicated
> before, I'm not really happy for us to gain all of this extra code. In
> the end I may eventually give an R-b not including the usually implied
> A-b, to indicate the code (then) looks okay to me but I still want
> someone else to actually ack it to allow it going in.

I understand. Given similar code is committed for vPCI already, I hope
somebody will be comfortable with acking this one too (yes, I do realize
the vPCI one is much less exposed, but still).

> > +static int adjacent_read(
> > +unsigned int fixmap_idx,
> > +paddr_t address, unsigned int len, uint64_t *pval)
> > +{
> > +const void __iomem *hwaddr;
> > +
> > +*pval = ~0UL;
> > +
> > +ASSERT(fixmap_idx != ADJACENT_DISCARD_WRITE);
> 
> Why only one of the special values? And before you add the other here:
> Why not simply ASSERT(fixmap_idx <= FIX_MSIX_IO_RESERV_END)? (Could of
> course bound at the other end, too, i.e. against FIX_MSIX_IO_RESERV_BASE.)

That's the most likely bug that could happen, but indeed broader assert
would be better.

> > +hwaddr = fix_to_virt(fixmap_idx) + PAGE_OFFSET(address);
> > +
> > +switch ( len )
> > +{
> > +case 1:
> > +*pval = readb(hwaddr);
> > +break;
> > +
> > +case 2:
> > +*pval = readw(hwaddr);
> > +break;
> > +
> > +case 4:
> > +*pval = readl(hwaddr);
> > +break;
> > +
> > +case 8:
> > +*pval = readq(hwaddr);
> > +break;
> > +
> > +default:
> > +ASSERT_UNREACHABLE();
> 
> Misra demands "break;" to be here for release builds. In fact I wonder
> why "*pval = ~0UL;" isn't put here, too. Question of course is whether
> in such a case a true error indicator wouldn't be yet better.

I don't think it possible for the msixtbl_read() (that calls
adjacent_read()) to be called with other sizes. The default label is
here exactly to make it obvious for the reader.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab


signature.asc
Description: PGP signature

[PATCH v1] xen/riscv: improve check-extension() macro

[Oleksii Kurochko]

Now, the check-extension() macro has 1 argument instead of 2.
This change helps to reduce redundancy around usage of extensions
name (in the case of the zbb extension, the name was used 3 times).

To implement this, a new variable was introduced:
  -insn
which represents the instruction support that is being checked.

Additionally, zbb-insn is updated to use $(comma) instead of ",".

Signed-off-by: Oleksii Kurochko 
Suggested-by: Jan Beulich 
---
 xen/arch/riscv/arch.mk | 21 +++--
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/xen/arch/riscv/arch.mk b/xen/arch/riscv/arch.mk
index dd242c91d1..17827c302c 100644
--- a/xen/arch/riscv/arch.mk
+++ b/xen/arch/riscv/arch.mk
@@ -13,12 +13,21 @@ riscv-generic-flags := $(riscv-abi-y) 
-march=$(riscv-march-y)
 
 # check-extension: Check whether extenstion is supported by a compiler and
 #  an assembler.
-# Usage: $(call check-extension,extension_name,"instr")
-check-extension = $(call as-insn,$(CC) $(riscv-generic-flags)_$(1),$(2),_$(1))
-
-zbb-insn := "andn t0, t0, t0"
-zbb := $(call check-extension,zbb,$(zbb-insn))
-zihintpause := $(call check-extension,zihintpause,"pause")
+# Usage: $(call check-extension,extension_name).
+#it should be defined variable with following name:
+#  -insn := "insn"
+#which represents an instruction of extension support of which is
+#going to be checked.
+define check-extension =
+$(eval $(1) := \
+   $(call as-insn,$(CC) $(riscv-generic-flags)_$(1),$(value 
$(1)-insn),_$(1)))
+endef
+
+zbb-insn := "andn t0$(comma)t0$(comma)t0"
+$(call check-extension,zbb)
+
+zihintpause-insn := "pause"
+$(call check-extension,zihintpause)
 
 extensions := $(zbb) $(zihintpause)
 
-- 
2.44.0

Re: [PATCH 1/3] x86/hvm/trace: Use a different trace type for AMD processors

[Andrew Cooper]

On 26/04/2024 3:32 pm, George Dunlap wrote:
> A long-standing usability sub-optimality with xenalyze is the
> necessity to specify `--svm-mode` when analyzing AMD processors.  This
> fundamentally comes about because the same trace event ID is used for
> both VMX and SVM, but the contents of the trace must be interpreted
> differently.
>
> Instead, allocate separate trace events for VMX and SVM vmexits in
> Xen; this will allow all readers to properly intrepret the meaning of

interpret ?

> the vmexit reason.
>
> In xenalyze, first remove the redundant call to init_hvm_data();
> there's no way to get to hvm_vmexit_process() without it being already
> initialized by the set_vcpu_type call in hvm_process().
>
> Replace this with set_hvm_exit_reson_data(), and move setting of
> hvm->exit_reason_* into that function.
>
> Modify hvm_process and hvm_vmexit_process to handle all four potential
> values appropriately.
>
> If SVM entries are encountered, set opt.svm_mode so that other
> SVM-specific functionality is triggered.

Given that xenalyze is now closely tied to Xen, and that we're
technically changing the ABI here, is there any point keeping `--svm-mode` ?

I'm unsure of the utility of reading the buggy trace records from an
older version of Xen.

> Also add lines in `formats` for xentrace_format.

Personally I'd have put patch 3 first, and reduced the churn here.

> Signed-off-by: George Dunlap 
> ---
> NB that this patch goes on top of Andrew's trace cleanup series:
>
> https://lore.kernel.org/xen-devel/20240318163552.3808695-1-andrew.coop...@citrix.com/

The delta in Xen is trivial.  I'm happy if you want to commit this, and
I can rebase over it.

~Andrew

Re: [PATCH 2/3] tools/xenalyze: Ignore HVM_EMUL events harder

[George Dunlap]

On Fri, Apr 26, 2024 at 4:06 PM Andrew Cooper  wrote:
>
> On 26/04/2024 3:32 pm, George Dunlap wrote:
> > To unify certain common sanity checks, checks are done very early in
> > processing based only on the top-level type.
> >
> > Unfortunately, when TRC_HVM_EMUL was introduced, it broke some of the
> > assumptions about how the top-level types worked.  Namely, traces of
> > this type will show up outside of HVM contexts: in idle domains and in
> > PV domains.
> >
> > Make an explicit exception for TRC_HVM_EMUL types in a number of places:
> >
> >  - Pass the record info pointer to toplevel_assert_check, so that it
> >can exclude TRC_HVM_EMUL records from idle and vcpu data_mode
> >checks
> >
> >  - Don't attempt to set the vcpu data_type in hvm_process for
> >TRC_HVM_EMUL records.
> >
> > Signed-off-by: George Dunlap 
>
> Acked-by: Andrew Cooper 
>
> Although I'm tempted to say that if records of this type show up outside
> of HVM context, then it's misnamed or we've got an error in Xen.  Any
> view on which?

I didn't add them; they seem to be about doing emulation of devices on
behalf of an HVM domain.  But since some of these are things like
timers and such, the actual code ends up being run in random contexts;
probably interrupt contexts (which of course can occur when a non-HVM
guest is running).

One example of an event that showed up this way was
TRC_HVM_EMUL_RTC_STOP_TIMER, which is traced from
xen/arch/x86/hvm/rtc.c:rtc_pf_callback().  I didn't trace back how it
came to be called while a PV guest was running, but it was pretty
obvious that it was legit.

It would certainly make the xenalyze code cleaner to make a separate
top-level tracing category for them; but they certainly do seem to be
directly related to HVM, so doing so would seem to be putting the cart
before the horse, as they say (although I could be convinced
otherwise).

 -George

Re: [XEN PATCH v3 5/5] xen/arm: ffa: support notification

[Bertrand Marquis]

Hi Jens,

> On 26 Apr 2024, at 15:02, Jens Wiklander  wrote:
> 
> On Fri, Apr 26, 2024 at 2:41 PM Bertrand Marquis
>  wrote:
>> 
>> Hi Jens,
>> 
>>> On 26 Apr 2024, at 14:32, Jens Wiklander  wrote:
>>> 
>>> Hi Bertrand,
>>> 
>>> On Fri, Apr 26, 2024 at 2:19 PM Bertrand Marquis
>>>  wrote:
 
 Hi Jens,
 
> On 26 Apr 2024, at 14:11, Jens Wiklander  
> wrote:
> 
> Hi Bertrand,
> 
> On Fri, Apr 26, 2024 at 11:20 AM Bertrand Marquis
>  wrote:
>> 
>> Hi Jens,
>> 
>>> On 26 Apr 2024, at 10:47, Jens Wiklander  
>>> wrote:
>>> 
>>> [...]
>>> +struct notif_irq_info {
>>> +unsigned int irq;
>>> +int ret;
>>> +struct irqaction *action;
>>> +};
>>> +
>>> +static void notif_irq_enable(void *info)
>>> +{
>>> +struct notif_irq_info *irq_info = info;
>>> +
>>> +irq_info->ret = setup_irq(irq_info->irq, 0, irq_info->action);
>>> +if ( irq_info->ret )
>>> +printk(XENLOG_ERR "ffa: request_irq irq %u failed: error %d\n",
>>> +   irq_info->irq, irq_info->ret);
>>> +}
>>> +
>>> +void ffa_notif_init(void)
>>> +{
>>> +const struct arm_smccc_1_2_regs arg = {
>>> +.a0 = FFA_FEATURES,
>>> +.a1 = FFA_FEATURE_SCHEDULE_RECV_INTR,
>>> +};
>>> +struct notif_irq_info irq_info = { };
>>> +struct arm_smccc_1_2_regs resp;
>>> +unsigned int cpu;
>>> +
>>> +arm_smccc_1_2_smc(, );
>>> +if ( resp.a0 != FFA_SUCCESS_32 )
>>> +return;
>>> +
>>> +irq_info.irq = resp.a2;
>>> +if ( irq_info.irq < GIC_SGI_STATIC_MAX || irq_info.irq >= 
>>> NR_GIC_SGI )
>>> +{
>>> +printk(XENLOG_ERR "ffa: notification initialization failed: 
>>> conflicting SGI %u\n",
>>> +   irq_info.irq);
>>> +return;
>>> +}
>>> +
>>> +/*
>>> + * SGIs are per-CPU so we must enable the IRQ on each CPU. We use 
>>> an
>>> + * IPI to call notif_irq_enable() on each CPU including the current
>>> + * CPU. The struct irqaction is preallocated since we can't 
>>> allocate
>>> + * memory while in interrupt context.
>>> + */
>>> +for_each_online_cpu(cpu)
>>> +{
>>> +irq_info.action = xmalloc(struct irqaction);
>> 
>> You allocate one action per cpu but you have only one action pointer in 
>> your structure
>> which means you will overload the previously allocated one and lose 
>> track.
>> 
>> You should have a table of actions in your structure instead unless one 
>> action is
>> enough and can be reused on all cpus and in this case you should move 
>> out of
>> your loop the allocation part.
> 
> That shouldn't be needed because this is done in sequence only one CPU
> at a time.
 
 Sorry i do not understand here.
 You have a loop over each online cpu and on each loop you are assigning
 irq_info.action with a newly allocated struct irqaction so you are in 
 practice
 overloading on cpu 2 the action that was allocated for cpu 1.
 
 What do you mean by sequence here ?
 
>>> 
>>> My understanding is that for_each_online_cpu(cpu) loops over each cpu,
>>> one at a time. The call
>>> on_selected_cpus(cpumask_of(cpu), notif_irq_enable, _info, 1);
>>> returns after notif_irq_enable() has returned on the CPU in question
>>> thanks to the "1" (wait) parameter. So once it has returned _info
>>> isn't used by the other CPU any longer and we can assign a new value
>>> to irq_info.action.
>> 
>> Right so you loose track of what was assigned so you are not able to
>> free it.
>> If that is wanted then why saving this in irq.action as you will only have
>> there the one allocated for the last online cpu.
> 
> Wouldn't release_irq() free it? An error here is unlikely, but we may
> be left with a few installed struct irqaction if it occurs. I can add
> a more elaborate error path if it's worth the added complexity.

I think just add a comment saying that the irqaction will be freed
upon release_irq so we do not keep a reference to it or something
like that and this will be ok.

The code is in fact a bit misleading because the irqaction is used
inside the function called on other cores through the IPI and there
you actually pass the action. Your structure is only there to transport
the information for the IPI handler.
So please add a comment on top of the notif_irq_info to say that
this structure is used to pass information to and back the notif_irq_enable
executed using an IPI on other cores.

Cheers
Bertrand


> 
> Thanks,
> Jens

Re: [PATCH 2/3] tools/xenalyze: Ignore HVM_EMUL events harder

[Andrew Cooper]

On 26/04/2024 3:32 pm, George Dunlap wrote:
> To unify certain common sanity checks, checks are done very early in
> processing based only on the top-level type.
>
> Unfortunately, when TRC_HVM_EMUL was introduced, it broke some of the
> assumptions about how the top-level types worked.  Namely, traces of
> this type will show up outside of HVM contexts: in idle domains and in
> PV domains.
>
> Make an explicit exception for TRC_HVM_EMUL types in a number of places:
>
>  - Pass the record info pointer to toplevel_assert_check, so that it
>can exclude TRC_HVM_EMUL records from idle and vcpu data_mode
>checks
>
>  - Don't attempt to set the vcpu data_type in hvm_process for
>TRC_HVM_EMUL records.
>
> Signed-off-by: George Dunlap 

Acked-by: Andrew Cooper 

Although I'm tempted to say that if records of this type show up outside
of HVM context, then it's misnamed or we've got an error in Xen.  Any
view on which?

~Andrew

Re: [PATCH 3/3] tools/xentrace: Remove xentrace_format

[Andrew Cooper]

On 26/04/2024 3:32 pm, George Dunlap wrote:
> xentrace_format was always of limited utility, since trace records
> across pcpus were processed out of order; it was superseded by xenalyze
> over a decade ago.
>
> But for several releases, the `formats` file it has depended on for
> proper operation has not even been included in `make install` (which
> generally means it doesn't get picked up by distros either); yet
> nobody has seemed to complain.
>
> Simple remove xentrace_format, and point people to xenalyze instead.
>
> NB that there is no man page for xenalyze, so the "see also" on the
> xentrace man page is simply removed for now.
>
> Signed-off-by: George Dunlap 

Acked-by: Andrew Cooper 

[PATCH 3/3] tools/xentrace: Remove xentrace_format

[George Dunlap]

xentrace_format was always of limited utility, since trace records
across pcpus were processed out of order; it was superseded by xenalyze
over a decade ago.

But for several releases, the `formats` file it has depended on for
proper operation has not even been included in `make install` (which
generally means it doesn't get picked up by distros either); yet
nobody has seemed to complain.

Simple remove xentrace_format, and point people to xenalyze instead.

NB that there is no man page for xenalyze, so the "see also" on the
xentrace man page is simply removed for now.

Signed-off-by: George Dunlap 
---
CC: Andrew Cooper 
CC: Jan Beulich 
CC: Roger Pau Monne 
CC: Anthony Perard 
CC: Olaf Hering 
---
 docs/man/xentrace.8.pod|   5 +-
 docs/man/xentrace_format.1.pod |  46 --
 tools/xentrace/Makefile|   3 -
 tools/xentrace/formats | 231 -
 tools/xentrace/xentrace.c  |   2 +-
 tools/xentrace/xentrace_format | 264 -
 6 files changed, 2 insertions(+), 549 deletions(-)

diff --git a/docs/man/xentrace.8.pod b/docs/man/xentrace.8.pod
index 4c174a84c0..7c9f69c67f 100644
--- a/docs/man/xentrace.8.pod
+++ b/docs/man/xentrace.8.pod
@@ -20,7 +20,7 @@ D1...D5 are the trace data.
 Data is dumped onto the standard output (which must not be a TTY) or a
 I specified on the command line.
 
-The output should be parsed using the tool xentrace_format, which can
+The output should be parsed using the tool xenalyze, which can
 produce human-readable output in ASCII format.
 
 
@@ -157,6 +157,3 @@ B collects the following events from the trace 
buffer:
 
 Mark A. Williamson 
 
-=head1 SEE ALSO
-
-xentrace_format(1)
diff --git a/docs/man/xentrace_format.1.pod b/docs/man/xentrace_format.1.pod
deleted file mode 100644
index e05479a83b..00
--- a/docs/man/xentrace_format.1.pod
+++ /dev/null
@@ -1,46 +0,0 @@
-=head1 NAME
-
-xentrace_format - pretty-print Xen trace data
-
-=head1 SYNOPSIS
-
-B [ I ]
-
-=head1 DESCRIPTION
-
-B parses trace data in B binary format from
-standard input and reformats it according to the rules in a file of
-definitions (I), printing to standard output.
-
-The rules in I should have the format shown below:
-
-I I I
-
-Each rule should start on a new line.
-
-The format string may include format specifiers, such as:
-%(cpu)d, %(tsc)d, %(event)d, %(1)d, %(2)d, %(3)d, %(4)d, %(5)d
-
-[ the `d' format specifier output in decimal, alternatively `x'
-  will output in hexadecimal and `o' will output in octal ]
-
-These correspond to the CPU number, event ID, timestamp counter and
-the 5 data fields from the trace record.  There should be one such
-rule for each type of event to be pretty-printed (events which do not
-have formatting rules are ignored).
-
-A sample format file for Xen's predefined trace events is available
-in the file tools/xentrace/formats in the Xen source tree.
-
-Depending on your system and the rate at which trace data is produced,
-this script may not be able to keep up with the output of
-B if it is piped directly.  In these circumstances you
-should have B output to a file for processing off-line.
-
-=head1 AUTHOR
-
-Mark A. Williamson 
-
-=head1 SEE ALSO
-
-xentrace(8)
diff --git a/tools/xentrace/Makefile b/tools/xentrace/Makefile
index d50d400472..bf960c0867 100644
--- a/tools/xentrace/Makefile
+++ b/tools/xentrace/Makefile
@@ -10,7 +10,6 @@ LDLIBS += $(ARGP_LDFLAGS)
 BIN := xenalyze
 SBIN:= xentrace xentrace_setsize
 LIBBIN  := xenctx
-SCRIPTS := xentrace_format
 
 TARGETS := $(BIN) $(SBIN) $(LIBBIN)
 
@@ -24,13 +23,11 @@ install: all
$(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)
$(INSTALL_PROG) $(BIN) $(DESTDIR)$(bindir)
$(INSTALL_PROG) $(SBIN) $(DESTDIR)$(sbindir)
-   $(INSTALL_PYTHON_PROG) $(SCRIPTS) $(DESTDIR)$(bindir)
$(INSTALL_PROG) $(LIBBIN) $(DESTDIR)$(LIBEXEC_BIN)
 
 .PHONY: uninstall
 uninstall:
rm -f $(addprefix $(DESTDIR)$(LIBEXEC_BIN)/, $(LIBBIN))
-   rm -f $(addprefix $(DESTDIR)$(bindir)/, $(SCRIPTS))
rm -f $(addprefix $(DESTDIR)$(sbindir)/, $(SBIN))
rm -f $(addprefix $(DESTDIR)$(bindir)/, $(BIN))
 
diff --git a/tools/xentrace/formats b/tools/xentrace/formats
deleted file mode 100644
index 3381c1cda5..00
--- a/tools/xentrace/formats
+++ /dev/null
@@ -1,231 +0,0 @@
-0x  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  unknown (0x%(event)016x)  [ 
0x%(1)08x 0x%(2)08x 0x%(3)08x 0x%(4)08x 0x%(5)08x 0x%(6)08x 0x%(7)08x ]
-
-0x0001f001  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  lost_records  0x%(1)08x
-0x0001f002  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  wrap_buffer   0x%(1)08x
-0x0001f003  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  cpu_change0x%(1)08x
-0x0001f004  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  trace_irq[ vector = %(1)d, 
count = %(2)d, tot_cycles = 0x%(3)08x, max_cycles = 0x%(4)08x ]
-
-0x00021002  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  continue_running[ dom:vcpu 
= 0x%(1)08x ]
-0x00021011  CPU%(cpu)d  

[PATCH 1/3] x86/hvm/trace: Use a different trace type for AMD processors

[George Dunlap]

A long-standing usability sub-optimality with xenalyze is the
necessity to specify `--svm-mode` when analyzing AMD processors.  This
fundamentally comes about because the same trace event ID is used for
both VMX and SVM, but the contents of the trace must be interpreted
differently.

Instead, allocate separate trace events for VMX and SVM vmexits in
Xen; this will allow all readers to properly intrepret the meaning of
the vmexit reason.

In xenalyze, first remove the redundant call to init_hvm_data();
there's no way to get to hvm_vmexit_process() without it being already
initialized by the set_vcpu_type call in hvm_process().

Replace this with set_hvm_exit_reson_data(), and move setting of
hvm->exit_reason_* into that function.

Modify hvm_process and hvm_vmexit_process to handle all four potential
values appropriately.

If SVM entries are encountered, set opt.svm_mode so that other
SVM-specific functionality is triggered.

Also add lines in `formats` for xentrace_format.

Signed-off-by: George Dunlap 
---
NB that this patch goes on top of Andrew's trace cleanup series:

https://lore.kernel.org/xen-devel/20240318163552.3808695-1-andrew.coop...@citrix.com/

CC: Andrew Cooper 
CC: Jan Beulich 
CC: Roger Pau Monne 
CC: Anthony Perard 
CC: Stefano Stabellini 
CC: Julien Grall 
CC: Olaf Hering 
---
 tools/xentrace/formats |  6 --
 tools/xentrace/xenalyze.c  | 32 +++-
 xen/arch/x86/hvm/svm/svm.c |  4 ++--
 xen/arch/x86/hvm/vmx/vmx.c |  4 ++--
 xen/include/public/trace.h |  6 --
 5 files changed, 31 insertions(+), 21 deletions(-)

diff --git a/tools/xentrace/formats b/tools/xentrace/formats
index afb5ee0112..3381c1cda5 100644
--- a/tools/xentrace/formats
+++ b/tools/xentrace/formats
@@ -90,8 +90,10 @@
 0x00041002  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  domain_destroy  [ dom = 
0x%(1)08x ]
 
 0x00081001  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  VMENTRY
-0x00081002  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  VMEXIT  [ exitcode = 
0x%(1)08x, rIP  = 0x%(2)08x ]
-0x00081102  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  VMEXIT  [ exitcode = 
0x%(1)08x, rIP  = 0x%(3)08x%(2)08x ]
+0x00081002  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  VMX_EXIT [ exitcode = 
0x%(1)08x, rIP  = 0x%(2)08x ]
+0x00081102  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  VMX_EXIT [ exitcode = 
0x%(1)08x, rIP  = 0x%(3)08x%(2)08x ]
+0x00081003  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  SVM_EXIT [ exitcode = 
0x%(1)08x, rIP  = 0x%(2)08x ]
+0x00081103  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  SVM_EXIT [ exitcode = 
0x%(1)08x, rIP  = 0x%(3)08x%(2)08x ]
 0x00081401  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  nVMENTRY
 0x00081402  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  nVMEXIT [ exitcode = 
0x%(1)08x, rIP  = 0x%(2)08x ]
 0x00081502  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  nVMEXIT [ exitcode = 
0x%(1)08x, rIP  = 0x%(3)08x%(2)08x ]
diff --git a/tools/xentrace/xenalyze.c b/tools/xentrace/xenalyze.c
index ce6a85d50b..ceb07229d1 100644
--- a/tools/xentrace/xenalyze.c
+++ b/tools/xentrace/xenalyze.c
@@ -1437,14 +1437,6 @@ void init_hvm_data(struct hvm_data *h, struct vcpu_data 
*v) {
 
 h->init = 1;
 
-if(opt.svm_mode) {
-h->exit_reason_max = HVM_SVM_EXIT_REASON_MAX;
-h->exit_reason_name = hvm_svm_exit_reason_name;
-} else {
-h->exit_reason_max = HVM_VMX_EXIT_REASON_MAX;
-h->exit_reason_name = hvm_vmx_exit_reason_name;
-}
-
 if(opt.histogram_interrupt_eip) {
 int count = 
((1ULLexit_reason_max = HVM_SVM_EXIT_REASON_MAX;
+h->exit_reason_name = hvm_svm_exit_reason_name;
+} else {
+h->exit_reason_max = HVM_VMX_EXIT_REASON_MAX;
+h->exit_reason_name = hvm_vmx_exit_reason_name;
+}
+}
+
 /* PV data */
 enum {
 PV_HYPERCALL=1,
@@ -5088,13 +5092,13 @@ void hvm_vmexit_process(struct record_info *ri, struct 
hvm_data *h,
 
 r = (typeof(r))ri->d;
 
-if(!h->init)
-init_hvm_data(h, v);
+if(!h->exit_reason_name)
+set_hvm_exit_reason_data(h, ri->event);
 
 h->vmexit_valid=1;
 bzero(>inflight, sizeof(h->inflight));
 
-if(ri->event == TRC_HVM_VMEXIT64) {
+if(ri->event & TRC_64_FLAG) {
 if(v->guest_paging_levels != 4)
 {
 if ( verbosity >= 6 )
@@ -5316,8 +5320,10 @@ void hvm_process(struct pcpu_info *p)
 break;
 default:
 switch(ri->event) {
-case TRC_HVM_VMEXIT:
-case TRC_HVM_VMEXIT64:
+case TRC_HVM_VMX_EXIT:
+case TRC_HVM_VMX_EXIT64:
+case TRC_HVM_SVM_EXIT:
+case TRC_HVM_SVM_EXIT64:
 UPDATE_VOLUME(p, hvm[HVM_VOL_VMEXIT], ri->size);
 hvm_vmexit_process(ri, h, v);
 break;
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index db530d55f2..988250dbc1 100644

[PATCH 2/3] tools/xenalyze: Ignore HVM_EMUL events harder

[George Dunlap]

To unify certain common sanity checks, checks are done very early in
processing based only on the top-level type.

Unfortunately, when TRC_HVM_EMUL was introduced, it broke some of the
assumptions about how the top-level types worked.  Namely, traces of
this type will show up outside of HVM contexts: in idle domains and in
PV domains.

Make an explicit exception for TRC_HVM_EMUL types in a number of places:

 - Pass the record info pointer to toplevel_assert_check, so that it
   can exclude TRC_HVM_EMUL records from idle and vcpu data_mode
   checks

 - Don't attempt to set the vcpu data_type in hvm_process for
   TRC_HVM_EMUL records.

Signed-off-by: George Dunlap 
---
CC: Andrew Cooper 
CC: Anthony Perard 
CC: Olaf Hering 
---
 tools/xentrace/xenalyze.c | 18 --
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/tools/xentrace/xenalyze.c b/tools/xentrace/xenalyze.c
index ceb07229d1..8fb45dec76 100644
--- a/tools/xentrace/xenalyze.c
+++ b/tools/xentrace/xenalyze.c
@@ -21,6 +21,7 @@
 #define _XOPEN_SOURCE 600
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -5305,8 +5306,11 @@ void hvm_process(struct pcpu_info *p)
 
 assert(p->current);
 
-if(vcpu_set_data_type(p->current, VCPU_DATA_HVM))
-return;
+/* HVM_EMUL types show up in all contexts */
+if(ri->evt.sub != 0x4) {
+if(vcpu_set_data_type(p->current, VCPU_DATA_HVM))
+return;
+}
 
 switch ( ri->evt.sub ) {
 case 2: /* HVM_HANDLER */
@@ -9447,9 +9451,10 @@ static struct tl_assert_mask 
tl_assert_checks[TOPLEVEL_MAX] = {
 /* There are a lot of common assumptions for the various processing
  * routines.  Check them all in one place, doing something else if
  * they don't pass. */
-int toplevel_assert_check(int toplevel, struct pcpu_info *p)
+int toplevel_assert_check(int toplevel, struct record_info *ri, struct 
pcpu_info *p)
 {
 struct tl_assert_mask mask;
+bool is_hvm_emul = (toplevel == TOPLEVEL_HVM) && (ri->evt.sub == 0x4);
 
 mask = tl_assert_checks[toplevel];
 
@@ -9459,7 +9464,7 @@ int toplevel_assert_check(int toplevel, struct pcpu_info 
*p)
 goto fail;
 }
 
-if( mask.not_idle_domain )
+if( mask.not_idle_domain && !is_hvm_emul)
 {
 /* Can't do this check w/o first doing above check */
 assert(mask.p_current);
@@ -9478,7 +9483,8 @@ int toplevel_assert_check(int toplevel, struct pcpu_info 
*p)
 v = p->current;
 
 if ( ! (v->data_type == VCPU_DATA_NONE
-|| v->data_type == mask.vcpu_data_mode) )
+|| v->data_type == mask.vcpu_data_mode
+|| is_hvm_emul) )
 {
 /* This may happen for track_dirty_vram, which causes a 
SHADOW_WRMAP_BF trace f/ dom0 */
 fprintf(warn, "WARNING: Unexpected vcpu data type for d%dv%d on 
proc %d! Expected %d got %d. Not processing\n",
@@ -9525,7 +9531,7 @@ void process_record(struct pcpu_info *p) {
 return;
 
 /* Unify toplevel assertions */
-if ( toplevel_assert_check(toplevel, p) )
+if ( toplevel_assert_check(toplevel, ri, p) )
 {
 switch(toplevel) {
 case TRC_GEN_MAIN:
-- 
2.25.1

[PATCH 0/3] Further trace improvements

[George Dunlap]

Some further trace improvements:

 - Remove the need to specify `--svm-mode` every time running xenalyze
   on a trace generated on an AMD box.

 - Get rid of warnings due to unhandled HVM_EMUL traces

 - Completely remove obsolete xentrace_format

This series is meant to be applied on top of Andy's series "xen/trace:
Treewide API cleanup":

https://lore.kernel.org/xen-devel/20240318163552.3808695-1-andrew.coop...@citrix.com/

George Dunlap (3):
  x86/hvm/trace: Use a different trace type for AMD processors
  tools/xenalyze: Ignore HVM_EMUL events harder
  tools/xentrace: Remove xentrace_format

 docs/man/xentrace.8.pod|   5 +-
 docs/man/xentrace_format.1.pod |  46 --
 tools/xentrace/Makefile|   3 -
 tools/xentrace/formats | 229 
 tools/xentrace/xenalyze.c  |  50 ---
 tools/xentrace/xentrace.c  |   2 +-
 tools/xentrace/xentrace_format | 264 -
 xen/arch/x86/hvm/svm/svm.c |   4 +-
 xen/arch/x86/hvm/vmx/vmx.c |   4 +-
 xen/include/public/trace.h |   6 +-
 10 files changed, 41 insertions(+), 572 deletions(-)
 delete mode 100644 docs/man/xentrace_format.1.pod
 delete mode 100644 tools/xentrace/formats
 delete mode 100644 tools/xentrace/xentrace_format

-- 
2.25.1

Re: [PATCH] xen/spinlock: use correct pointer

[Stewart Hildebrand]

On 4/26/24 02:31, Jan Beulich wrote:
> On 25.04.2024 22:45, Stewart Hildebrand wrote:
>> The ->profile member is at different offsets in struct rspinlock and
>> struct spinlock. When initializing the profiling bits of an rspinlock,
>> an unrelated member in struct rspinlock was being overwritten, leading
>> to mild havoc. Use the correct pointer.
>>
>> Fixes: b053075d1a7b ("xen/spinlock: make struct lock_profile rspinlock_t 
>> aware")
>> Signed-off-by: Stewart Hildebrand 
> 
> Reviewed-by: Jan Beulich 

Thanks!

> 
>> --- a/xen/common/spinlock.c
>> +++ b/xen/common/spinlock.c
>> @@ -789,7 +789,11 @@ static int __init cf_check lock_prof_init(void)
>>  {
>>  (*q)->next = lock_profile_glb_q.elem_q;
>>  lock_profile_glb_q.elem_q = *q;
>> -(*q)->ptr.lock->profile = *q;
>> +
>> +if ( (*q)->is_rlock )
>> +(*q)->ptr.rlock->profile = *q;
>> +else
>> +(*q)->ptr.lock->profile = *q;
>>  }
>>  
>>  _lock_profile_register_struct(LOCKPROF_TYPE_GLOBAL,
> 
> Just to mention it: Strictly speaking spinlock_profile_print_elem()'s
> 
> printk("%s: addr=%p, lockval=%08x, ", data->name, data->ptr.lock, 
> lockval);
> 
> isn't quite right either (and I would be surprised if Misra didn't have
> to say something about it).
> 
> Jan

I'd be happy to send a patch for that instance, too. Would you like a
Reported-by: tag?

That patch would look something like:

--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -637,22 +637,25 @@ static void cf_check spinlock_profile_print_elem(struct 
lock_profile *data,
 {
 unsigned int cpu;
 unsigned int lockval;
+void *lockaddr;
 
 if ( data->is_rlock )
 {
 cpu = data->ptr.rlock->debug.cpu;
 lockval = data->ptr.rlock->tickets.head_tail;
+lockaddr = data->ptr.rlock;
 }
 else
 {
 cpu = data->ptr.lock->debug.cpu;
 lockval = data->ptr.lock->tickets.head_tail;
+lockaddr = data->ptr.lock;
 }
 
 printk("%s ", lock_profile_ancs[type].name);
 if ( type != LOCKPROF_TYPE_GLOBAL )
 printk("%d ", idx);
-printk("%s: addr=%p, lockval=%08x, ", data->name, data->ptr.lock, lockval);
+printk("%s: addr=%p, lockval=%08x, ", data->name, lockaddr, lockval);
 if ( cpu == SPINLOCK_NO_CPU )
 printk("not locked\n");
 else


That case is benign since the pointer is not dereferenced. So the
rationale would primarily be for consistency (and possibly satisfying
Misra).

[libvirt test] 185804: tolerable all pass - PUSHED

[osstest service owner]

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

Failures :-/ but no regressions.

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-libvirt 16 saverestore-support-checkfail  like 185793
 test-amd64-amd64-libvirt 15 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt 15 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-qcow2 14 migrate-support-checkfail never pass
 test-arm64-arm64-libvirt 16 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-raw 14 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt 15 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-qcow2 14 migrate-support-checkfail never pass
 test-arm64-arm64-libvirt-qcow2 15 saverestore-support-checkfail never pass
 test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt-vhd 14 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-vhd 15 saverestore-support-checkfail   never pass

version targeted for testing:
 libvirt  5540c3d2415c194b206f8946cf74b13648163332
baseline version:
 libvirt  948d496d2546807e8c3af634fa982615b2a3153b

Last test of basis   185793  2024-04-25 04:22:04 Z1 days
Testing same since   185804  2024-04-26 04:20:29 Z0 days1 attempts


People who touched revisions under test:
  Michal Privoznik 
  Peter Krempa 

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  pass
 build-arm64-libvirt  pass
 build-armhf-libvirt  pass
 build-i386-libvirt   pass
 build-amd64-pvopspass
 build-arm64-pvopspass
 build-armhf-pvopspass
 build-i386-pvops pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm   pass
 test-amd64-amd64-libvirt-xsm pass
 test-arm64-arm64-libvirt-xsm pass
 test-amd64-amd64-libvirt pass
 test-arm64-arm64-libvirt pass
 test-armhf-armhf-libvirt pass
 test-amd64-amd64-libvirt-pairpass
 test-amd64-amd64-libvirt-qcow2   pass
 test-arm64-arm64-libvirt-qcow2   pass
 test-amd64-amd64-libvirt-raw pass
 test-arm64-arm64-libvirt-raw pass
 test-amd64-amd64-libvirt-vhd pass
 test-armhf-armhf-libvirt-vhd 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/libvirt.git
   948d496d25..5540c3d241  5540c3d2415c194b206f8946cf74b13648163332 -> 
xen-tested-master

[PATCH 0/3] x86/boot: Untangling

[Andrew Cooper]

This started out as another series trying to fix CPUID handling for SEV.

I'm still trying to sort out the SEV side of things, but I might need to
resort to something *far* more unpleasant in order to hit 4.19.

This series is some cleanup of module handling from the work I've done so far.
It interacts with the HyperLaunch Boot Module cleanup, but should make it
simpler overall.

Andrew Cooper (3):
  x86/boot: Explain how moving mod[0] works
  x86/boot: Explain discard_initial_images() and untangle PV initrd handling
  x86/boot: Refactor pvh_load_kernel() to have an initrd_len local

 xen/arch/x86/hvm/dom0_build.c |  9 +
 xen/arch/x86/pv/dom0_build.c  | 22 +++---
 xen/arch/x86/setup.c  | 14 +-
 3 files changed, 29 insertions(+), 16 deletions(-)

-- 
2.30.2

[PATCH 2/3] x86/boot: Explain discard_initial_images() and untangle PV initrd handling

[Andrew Cooper]

discard_initial_images() frees the memory backing the boot modules.  It is
called by dom0_construct_pv{,h}(), but obtains the module list by global
pointer which is why there is no apparent link with the initrd pointer.

Generally, module contents are copied into dom0 as it's being built, but the
initrd for PV dom0 might be directly mapped instead of copied.

dom0_construct_pv() does it's own ad-hoc freeing of the module in the copy
case, and points the global reference at the new copy, then sets the size to
0.  This only functions because init_domheap_pages(x, x) happens to be a nop.

Delete the ad-hoc freeing, and leave it to discard_initial_images().  This
requires (not) adjusting initd->mod_start in the copy case, and only setting
the size to 0 in the mapping case.

Alter discard_initial_images() to explicitly check for an ignored module, and
explain what's going on.  This is more robust and will allow for fixing other
problems with module handling.

The later logic in dom0_construct_pv() now cannot use initrd->mod_start, but
that's fine because initrd_mfn is already a duplicate of the information
wanted, and is more consistent with initrd_{pfn,len} used elsewhere.

Invalidate the initrd pointer with LIST_POISON1 to make it clearer that it
shouldn't be used.

No practical change in behaviour, but a substantial reduction in the
complexity of how this works.

Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 
CC: Roger Pau Monné 
CC: Stefano Stabellini 
CC: Daniel Smith 
CC: Christopher Clark 

In other akward questions, why does initial_images_nrpages() account for all
modules when only 1 or 2 are relevant for how we construct dom0 ?
---
 xen/arch/x86/pv/dom0_build.c | 22 +++---
 xen/arch/x86/setup.c |  9 -
 2 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
index d8043fa58a27..64d9984b8308 100644
--- a/xen/arch/x86/pv/dom0_build.c
+++ b/xen/arch/x86/pv/dom0_build.c
@@ -630,18 +630,20 @@ int __init dom0_construct_pv(struct domain *d,
 }
 memcpy(page_to_virt(page), mfn_to_virt(initrd->mod_start),
initrd_len);
-mpt_alloc = (paddr_t)initrd->mod_start << PAGE_SHIFT;
-init_domheap_pages(mpt_alloc,
-   mpt_alloc + PAGE_ALIGN(initrd_len));
-initrd->mod_start = initrd_mfn = mfn_x(page_to_mfn(page));
+initrd_mfn = mfn_x(page_to_mfn(page));
 }
 else
 {
 while ( count-- )
 if ( assign_pages(mfn_to_page(_mfn(mfn++)), 1, d, 0) )
 BUG();
+/*
+ * Mapped rather than copied.  Tell discard_initial_images() to
+ * ignore it.
+ */
+initrd->mod_end = 0;
 }
-initrd->mod_end = 0;
+initrd = LIST_POISON1; /* No longer valid to use. */
 
 iommu_memory_setup(d, "initrd", mfn_to_page(_mfn(initrd_mfn)),
PFN_UP(initrd_len), _flags);
@@ -653,12 +655,10 @@ int __init dom0_construct_pv(struct domain *d,
 if ( domain_tot_pages(d) < nr_pages )
 printk(" (%lu pages to be allocated)",
nr_pages - domain_tot_pages(d));
-if ( initrd )
-{
-mpt_alloc = (paddr_t)initrd->mod_start << PAGE_SHIFT;
+if ( initrd_len )
 printk("\n Init. ramdisk: %"PRIpaddr"->%"PRIpaddr,
-   mpt_alloc, mpt_alloc + initrd_len);
-}
+   pfn_to_paddr(initrd_mfn),
+   pfn_to_paddr(initrd_mfn) + initrd_len);
 
 printk("\nVIRTUAL MEMORY ARRANGEMENT:\n");
 printk(" Loaded kernel: %p->%p\n", _p(vkern_start), _p(vkern_end));
@@ -881,7 +881,7 @@ int __init dom0_construct_pv(struct domain *d,
 if ( pfn >= initrd_pfn )
 {
 if ( pfn < initrd_pfn + PFN_UP(initrd_len) )
-mfn = initrd->mod_start + (pfn - initrd_pfn);
+mfn = initrd_mfn + (pfn - initrd_pfn);
 else
 mfn -= PFN_UP(initrd_len);
 }
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 59907fae095f..785a46a44995 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -294,7 +294,7 @@ unsigned long __init initial_images_nrpages(nodeid_t node)
 return nr;
 }
 
-void __init discard_initial_images(void)
+void __init discard_initial_images(void) /* a.k.a. free multiboot modules */
 {
 unsigned int i;
 
@@ -302,6 +302,13 @@ void __init discard_initial_images(void)
 {
 uint64_t start = (uint64_t)initial_images[i].mod_start << PAGE_SHIFT;
 
+/*
+ * Sometimes the initrd is mapped, rather than copied, into dom0.
+ * end=0 signifies that we should leave it alone.
+ */
+if ( initial_images[i].mod_end == 0 )
+continue;
+
 init_domheap_pages(start,
start + PAGE_ALIGN(initial_images[i].mod_end));
 }
-- 

[PATCH 3/3] x86/boot: Refactor pvh_load_kernel() to have an initrd_len local

[Andrew Cooper]

The expression get more complicated when ->mod_end isn't being abused as a
size field.  Introduce and use a initrd_len local variable.

No functional change.

Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 
CC: Roger Pau Monné 
CC: Stefano Stabellini 
CC: Daniel Smith 
CC: Christopher Clark 
---
 xen/arch/x86/hvm/dom0_build.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index ac71d43a6b3f..b0cb96c3bc76 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -650,6 +650,7 @@ static int __init pvh_load_kernel(struct domain *d, const 
module_t *image,
 {
 void *image_start = image_base + image_headroom;
 unsigned long image_len = image->mod_end;
+unsigned long initrd_len = initrd ? initrd->mod_end : 0;
 struct elf_binary elf;
 struct elf_dom_parms parms;
 paddr_t last_addr;
@@ -710,7 +711,7 @@ static int __init pvh_load_kernel(struct domain *d, const 
module_t *image,
  * simplify it.
  */
 last_addr = find_memory(d, , sizeof(start_info) +
-(initrd ? ROUNDUP(initrd->mod_end, PAGE_SIZE) +
+(initrd ? ROUNDUP(initrd_len, PAGE_SIZE) +
   sizeof(mod)
 : 0) +
 (cmdline ? ROUNDUP(strlen(cmdline) + 1,
@@ -725,7 +726,7 @@ static int __init pvh_load_kernel(struct domain *d, const 
module_t *image,
 if ( initrd != NULL )
 {
 rc = hvm_copy_to_guest_phys(last_addr, mfn_to_virt(initrd->mod_start),
-initrd->mod_end, v);
+initrd_len, v);
 if ( rc )
 {
 printk("Unable to copy initrd to guest\n");
@@ -733,8 +734,8 @@ static int __init pvh_load_kernel(struct domain *d, const 
module_t *image,
 }
 
 mod.paddr = last_addr;
-mod.size = initrd->mod_end;
-last_addr += ROUNDUP(initrd->mod_end, elf_64bit() ? 8 : 4);
+mod.size = initrd_len;
+last_addr += ROUNDUP(initrd_len, elf_64bit() ? 8 : 4);
 if ( initrd->string )
 {
 char *str = __va(initrd->string);
-- 
2.30.2

[PATCH 1/3] x86/boot: Explain how moving mod[0] works

[Andrew Cooper]

modules_headroom is a misleading name as it applies strictly to mod[0] only,
and the movement loop is deeply unintuitive and completely undocumented.

Provide help to whomever needs to look at this code next.

Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 
CC: Roger Pau Monné 
CC: Stefano Stabellini 
CC: Daniel Smith 
CC: Christopher Clark 
---
 xen/arch/x86/setup.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index caf235c6286d..59907fae095f 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1432,6 +1432,11 @@ void asmlinkage __init noreturn __start_xen(unsigned 
long mbi_p)
 /* Is the region suitable for relocating the multiboot modules? */
 for ( j = mbi->mods_count - 1; j >= 0; j-- )
 {
+/*
+ * 'headroom' is a guess for the decompressed size and
+ * decompressor overheads of mod[0] (the dom0 kernel).  When we
+ * move mod[0], we incorperate this as extra space at the start.
+ */
 unsigned long headroom = j ? 0 : modules_headroom;
 unsigned long size = PAGE_ALIGN(headroom + mod[j].mod_end);
 
-- 
2.30.2

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

[osstest service owner]

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

Failures :-/ but no regressions.

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

version targeted for testing:
 xen  be5b08dd6ea6ef0f01caf537bdae125fa66a2230
baseline version:
 xen  232ee07c23b23fbbafbbf27e475dbbc5b27e4bbb

Last test of basis   185801  2024-04-25 21:02:05 Z0 days
Testing same since   185812  2024-04-26 11:01:45 Z0 days1 attempts


People who touched revisions under test:
  Andrew Cooper 
  Anthony PERARD 
  Daniel P. Smith 
  Jan Beulich 
  Roger Pau Monné 
  Stewart Hildebrand 

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
   232ee07c23..be5b08dd6e  be5b08dd6ea6ef0f01caf537bdae125fa66a2230 -> smoke

Re: [PATCH v2 1/2] net: Provide MemReentrancyGuard * to qemu_new_nic()

[Philippe Mathieu-Daudé]

On 26/4/24 14:37, Akihiko Odaki wrote:

On 2024/04/24 21:32, Thomas Huth wrote:

On 24/04/2024 12.41, Prasad Pandit wrote:
On Wednesday, 24 April, 2024 at 03:36:01 pm IST, Philippe 
Mathieu-Daudé wrote:

On 1/6/23 05:18, Akihiko Odaki wrote:
Recently MemReentrancyGuard was added to DeviceState to record that 
the
device is engaging in I/O. The network device backend needs to 
update it

when delivering a packet to a device.
In preparation for such a change, add MemReentrancyGuard * as a
parameter of qemu_new_nic().


An user on IRC asked if this patch is related/fixing CVE-2021-20255,
any clue?


* CVE-2021-20255 bug: infinite recursion is pointing at a different 
fix patch.

   -> https://bugzilla.redhat.com/show_bug.cgi?id=CVE-2021-20255

* And the this patch below has different issue tagged
-> https://lists.nongnu.org/archive/html/qemu-devel/2023-05/msg08312.html
   Fixes: CVE-2023-3019


* They look different, former is an infinite recursion issue and the 
latter is a use-after-free one.


I assume the eepro reentrancy issue has been fixed with:

  https://gitlab.com/qemu-project/qemu/-/issues/556
  i.e.:
  https://gitlab.com/qemu-project/qemu/-/commit/c40ca2301c7603524eaddb5308a3


I agree. Commit c40ca2301c7603524eaddb5308a3 should be what fixed 
CVE-2021-20255, not this patch.


Thank you all for clarifying!

Re: [XEN PATCH v3 5/5] xen/arm: ffa: support notification

[Jens Wiklander]

On Fri, Apr 26, 2024 at 2:41 PM Bertrand Marquis
 wrote:
>
> Hi Jens,
>
> > On 26 Apr 2024, at 14:32, Jens Wiklander  wrote:
> >
> > Hi Bertrand,
> >
> > On Fri, Apr 26, 2024 at 2:19 PM Bertrand Marquis
> >  wrote:
> >>
> >> Hi Jens,
> >>
> >>> On 26 Apr 2024, at 14:11, Jens Wiklander  
> >>> wrote:
> >>>
> >>> Hi Bertrand,
> >>>
> >>> On Fri, Apr 26, 2024 at 11:20 AM Bertrand Marquis
> >>>  wrote:
> 
>  Hi Jens,
> 
> > On 26 Apr 2024, at 10:47, Jens Wiklander  
> > wrote:
> >
> > [...]
> > +struct notif_irq_info {
> > +unsigned int irq;
> > +int ret;
> > +struct irqaction *action;
> > +};
> > +
> > +static void notif_irq_enable(void *info)
> > +{
> > +struct notif_irq_info *irq_info = info;
> > +
> > +irq_info->ret = setup_irq(irq_info->irq, 0, irq_info->action);
> > +if ( irq_info->ret )
> > +printk(XENLOG_ERR "ffa: request_irq irq %u failed: error %d\n",
> > +   irq_info->irq, irq_info->ret);
> > +}
> > +
> > +void ffa_notif_init(void)
> > +{
> > +const struct arm_smccc_1_2_regs arg = {
> > +.a0 = FFA_FEATURES,
> > +.a1 = FFA_FEATURE_SCHEDULE_RECV_INTR,
> > +};
> > +struct notif_irq_info irq_info = { };
> > +struct arm_smccc_1_2_regs resp;
> > +unsigned int cpu;
> > +
> > +arm_smccc_1_2_smc(, );
> > +if ( resp.a0 != FFA_SUCCESS_32 )
> > +return;
> > +
> > +irq_info.irq = resp.a2;
> > +if ( irq_info.irq < GIC_SGI_STATIC_MAX || irq_info.irq >= 
> > NR_GIC_SGI )
> > +{
> > +printk(XENLOG_ERR "ffa: notification initialization failed: 
> > conflicting SGI %u\n",
> > +   irq_info.irq);
> > +return;
> > +}
> > +
> > +/*
> > + * SGIs are per-CPU so we must enable the IRQ on each CPU. We use 
> > an
> > + * IPI to call notif_irq_enable() on each CPU including the current
> > + * CPU. The struct irqaction is preallocated since we can't 
> > allocate
> > + * memory while in interrupt context.
> > + */
> > +for_each_online_cpu(cpu)
> > +{
> > +irq_info.action = xmalloc(struct irqaction);
> 
>  You allocate one action per cpu but you have only one action pointer in 
>  your structure
>  which means you will overload the previously allocated one and lose 
>  track.
> 
>  You should have a table of actions in your structure instead unless one 
>  action is
>  enough and can be reused on all cpus and in this case you should move 
>  out of
>  your loop the allocation part.
> >>>
> >>> That shouldn't be needed because this is done in sequence only one CPU
> >>> at a time.
> >>
> >> Sorry i do not understand here.
> >> You have a loop over each online cpu and on each loop you are assigning
> >> irq_info.action with a newly allocated struct irqaction so you are in 
> >> practice
> >> overloading on cpu 2 the action that was allocated for cpu 1.
> >>
> >> What do you mean by sequence here ?
> >>
> >
> > My understanding is that for_each_online_cpu(cpu) loops over each cpu,
> > one at a time. The call
> > on_selected_cpus(cpumask_of(cpu), notif_irq_enable, _info, 1);
> > returns after notif_irq_enable() has returned on the CPU in question
> > thanks to the "1" (wait) parameter. So once it has returned _info
> > isn't used by the other CPU any longer and we can assign a new value
> > to irq_info.action.
>
> Right so you loose track of what was assigned so you are not able to
> free it.
> If that is wanted then why saving this in irq.action as you will only have
> there the one allocated for the last online cpu.

Wouldn't release_irq() free it? An error here is unlikely, but we may
be left with a few installed struct irqaction if it occurs. I can add
a more elaborate error path if it's worth the added complexity.

Thanks,
Jens

Re: [XEN PATCH v3 5/5] xen/arm: ffa: support notification

[Bertrand Marquis]

Hi Jens,

> On 26 Apr 2024, at 14:32, Jens Wiklander  wrote:
> 
> Hi Bertrand,
> 
> On Fri, Apr 26, 2024 at 2:19 PM Bertrand Marquis
>  wrote:
>> 
>> Hi Jens,
>> 
>>> On 26 Apr 2024, at 14:11, Jens Wiklander  wrote:
>>> 
>>> Hi Bertrand,
>>> 
>>> On Fri, Apr 26, 2024 at 11:20 AM Bertrand Marquis
>>>  wrote:
 
 Hi Jens,
 
> On 26 Apr 2024, at 10:47, Jens Wiklander  
> wrote:
> 
> [...]
> +struct notif_irq_info {
> +unsigned int irq;
> +int ret;
> +struct irqaction *action;
> +};
> +
> +static void notif_irq_enable(void *info)
> +{
> +struct notif_irq_info *irq_info = info;
> +
> +irq_info->ret = setup_irq(irq_info->irq, 0, irq_info->action);
> +if ( irq_info->ret )
> +printk(XENLOG_ERR "ffa: request_irq irq %u failed: error %d\n",
> +   irq_info->irq, irq_info->ret);
> +}
> +
> +void ffa_notif_init(void)
> +{
> +const struct arm_smccc_1_2_regs arg = {
> +.a0 = FFA_FEATURES,
> +.a1 = FFA_FEATURE_SCHEDULE_RECV_INTR,
> +};
> +struct notif_irq_info irq_info = { };
> +struct arm_smccc_1_2_regs resp;
> +unsigned int cpu;
> +
> +arm_smccc_1_2_smc(, );
> +if ( resp.a0 != FFA_SUCCESS_32 )
> +return;
> +
> +irq_info.irq = resp.a2;
> +if ( irq_info.irq < GIC_SGI_STATIC_MAX || irq_info.irq >= NR_GIC_SGI 
> )
> +{
> +printk(XENLOG_ERR "ffa: notification initialization failed: 
> conflicting SGI %u\n",
> +   irq_info.irq);
> +return;
> +}
> +
> +/*
> + * SGIs are per-CPU so we must enable the IRQ on each CPU. We use an
> + * IPI to call notif_irq_enable() on each CPU including the current
> + * CPU. The struct irqaction is preallocated since we can't allocate
> + * memory while in interrupt context.
> + */
> +for_each_online_cpu(cpu)
> +{
> +irq_info.action = xmalloc(struct irqaction);
 
 You allocate one action per cpu but you have only one action pointer in 
 your structure
 which means you will overload the previously allocated one and lose track.
 
 You should have a table of actions in your structure instead unless one 
 action is
 enough and can be reused on all cpus and in this case you should move out 
 of
 your loop the allocation part.
>>> 
>>> That shouldn't be needed because this is done in sequence only one CPU
>>> at a time.
>> 
>> Sorry i do not understand here.
>> You have a loop over each online cpu and on each loop you are assigning
>> irq_info.action with a newly allocated struct irqaction so you are in 
>> practice
>> overloading on cpu 2 the action that was allocated for cpu 1.
>> 
>> What do you mean by sequence here ?
>> 
> 
> My understanding is that for_each_online_cpu(cpu) loops over each cpu,
> one at a time. The call
> on_selected_cpus(cpumask_of(cpu), notif_irq_enable, _info, 1);
> returns after notif_irq_enable() has returned on the CPU in question
> thanks to the "1" (wait) parameter. So once it has returned _info
> isn't used by the other CPU any longer and we can assign a new value
> to irq_info.action.

Right so you loose track of what was assigned so you are not able to
free it.
If that is wanted then why saving this in irq.action as you will only have
there the one allocated for the last online cpu.

Cheers
Bertrand

> 
> Thanks,
> Jens

Re: [PATCH v2 1/2] net: Provide MemReentrancyGuard * to qemu_new_nic()

[Akihiko Odaki]

On 2024/04/24 21:32, Thomas Huth wrote:

On 24/04/2024 12.41, Prasad Pandit wrote:
On Wednesday, 24 April, 2024 at 03:36:01 pm IST, Philippe 
Mathieu-Daudé wrote:

On 1/6/23 05:18, Akihiko Odaki wrote:

Recently MemReentrancyGuard was added to DeviceState to record that the
device is engaging in I/O. The network device backend needs to 
update it

when delivering a packet to a device.
In preparation for such a change, add MemReentrancyGuard * as a
parameter of qemu_new_nic().


An user on IRC asked if this patch is related/fixing CVE-2021-20255,
any clue?


* CVE-2021-20255 bug: infinite recursion is pointing at a different 
fix patch.

   -> https://bugzilla.redhat.com/show_bug.cgi?id=CVE-2021-20255

* And the this patch below has different issue tagged
   
-> https://lists.nongnu.org/archive/html/qemu-devel/2023-05/msg08312.html

   Fixes: CVE-2023-3019


* They look different, former is an infinite recursion issue and the 
latter is a use-after-free one.


I assume the eepro reentrancy issue has been fixed with:

  https://gitlab.com/qemu-project/qemu/-/issues/556
  i.e.:
  https://gitlab.com/qemu-project/qemu/-/commit/c40ca2301c7603524eaddb5308a3


I agree. Commit c40ca2301c7603524eaddb5308a3 should be what fixed 
CVE-2021-20255, not this patch.


Regards,
Akihiko Odaki

Re: [PATCH v3 7/8] gzip: move bitbuffer into gunzip state

[Andrew Cooper]

On 26/04/2024 6:57 am, Jan Beulich wrote:
> On 26.04.2024 07:55, Jan Beulich wrote:
>> On 25.04.2024 21:23, Andrew Cooper wrote:
>>> On 24/04/2024 5:34 pm, Daniel P. Smith wrote:
 --- a/xen/common/gzip/inflate.c
 +++ b/xen/common/gzip/inflate.c
 @@ -1017,8 +1014,8 @@ static int __init inflate(struct gunzip_state *s)
  /* Undo too much lookahead. The next read will be byte aligned so we
   * can discard unused bits in the last meaningful byte.
   */
 -while (bk >= 8) {
 -bk -= 8;
 +while (s->bk >= 8) {
 +s->bk -= 8;
  s->inptr--;
  }
>>> Isn't it just me, but isn't this just:
>>>
>>>     s->inptr -= (s->bk >> 3);
>>>     s->bk &= 7;
>>>
>>> ?
>> I'd say yes, if only there wasn't the comment talking of just a single byte,
>> and even there supposedly some of the bits.

The comments in this file leave a lot to be desired...

I'm reasonably confident they were not adjusted when a piece of
userspace code was imported into Linux.

This cannot ever have been just a byte's worth of bits, seeing as the
bit count is from 8 or more.  Right now it's an unsigned long's worth of
bits.
> Talking of the comment: Considering what patch 1 supposedly does, how come
> that isn't Xen-style (yet)?

I had been collecting some minor fixes like this to fold into patch 1
when I committed the whole series.

I'll see if I can fold them elsewhere.

~Andrew

Re: [XEN PATCH v3 5/5] xen/arm: ffa: support notification

[Jens Wiklander]

Hi Bertrand,

On Fri, Apr 26, 2024 at 2:19 PM Bertrand Marquis
 wrote:
>
> Hi Jens,
>
> > On 26 Apr 2024, at 14:11, Jens Wiklander  wrote:
> >
> > Hi Bertrand,
> >
> > On Fri, Apr 26, 2024 at 11:20 AM Bertrand Marquis
> >  wrote:
> >>
> >> Hi Jens,
> >>
> >>> On 26 Apr 2024, at 10:47, Jens Wiklander  
> >>> wrote:
> >>>
[...]
> >>> +struct notif_irq_info {
> >>> +unsigned int irq;
> >>> +int ret;
> >>> +struct irqaction *action;
> >>> +};
> >>> +
> >>> +static void notif_irq_enable(void *info)
> >>> +{
> >>> +struct notif_irq_info *irq_info = info;
> >>> +
> >>> +irq_info->ret = setup_irq(irq_info->irq, 0, irq_info->action);
> >>> +if ( irq_info->ret )
> >>> +printk(XENLOG_ERR "ffa: request_irq irq %u failed: error %d\n",
> >>> +   irq_info->irq, irq_info->ret);
> >>> +}
> >>> +
> >>> +void ffa_notif_init(void)
> >>> +{
> >>> +const struct arm_smccc_1_2_regs arg = {
> >>> +.a0 = FFA_FEATURES,
> >>> +.a1 = FFA_FEATURE_SCHEDULE_RECV_INTR,
> >>> +};
> >>> +struct notif_irq_info irq_info = { };
> >>> +struct arm_smccc_1_2_regs resp;
> >>> +unsigned int cpu;
> >>> +
> >>> +arm_smccc_1_2_smc(, );
> >>> +if ( resp.a0 != FFA_SUCCESS_32 )
> >>> +return;
> >>> +
> >>> +irq_info.irq = resp.a2;
> >>> +if ( irq_info.irq < GIC_SGI_STATIC_MAX || irq_info.irq >= NR_GIC_SGI 
> >>> )
> >>> +{
> >>> +printk(XENLOG_ERR "ffa: notification initialization failed: 
> >>> conflicting SGI %u\n",
> >>> +   irq_info.irq);
> >>> +return;
> >>> +}
> >>> +
> >>> +/*
> >>> + * SGIs are per-CPU so we must enable the IRQ on each CPU. We use an
> >>> + * IPI to call notif_irq_enable() on each CPU including the current
> >>> + * CPU. The struct irqaction is preallocated since we can't allocate
> >>> + * memory while in interrupt context.
> >>> + */
> >>> +for_each_online_cpu(cpu)
> >>> +{
> >>> +irq_info.action = xmalloc(struct irqaction);
> >>
> >> You allocate one action per cpu but you have only one action pointer in 
> >> your structure
> >> which means you will overload the previously allocated one and lose track.
> >>
> >> You should have a table of actions in your structure instead unless one 
> >> action is
> >> enough and can be reused on all cpus and in this case you should move out 
> >> of
> >> your loop the allocation part.
> >
> > That shouldn't be needed because this is done in sequence only one CPU
> > at a time.
>
> Sorry i do not understand here.
> You have a loop over each online cpu and on each loop you are assigning
> irq_info.action with a newly allocated struct irqaction so you are in practice
> overloading on cpu 2 the action that was allocated for cpu 1.
>
> What do you mean by sequence here ?
>

My understanding is that for_each_online_cpu(cpu) loops over each cpu,
one at a time. The call
on_selected_cpus(cpumask_of(cpu), notif_irq_enable, _info, 1);
returns after notif_irq_enable() has returned on the CPU in question
thanks to the "1" (wait) parameter. So once it has returned _info
isn't used by the other CPU any longer and we can assign a new value
to irq_info.action.

Thanks,
Jens

Re: [PATCH v8 03/17] xen/bitops: implement fls{l}() in common logic

[Jan Beulich]

On 26.04.2024 14:09, Oleksii wrote:
> On Fri, 2024-04-26 at 12:51 +0200, Jan Beulich wrote:
>> On 26.04.2024 10:21, Oleksii wrote:
>>> On Thu, 2024-04-25 at 17:44 +0200, Jan Beulich wrote:
 On 17.04.2024 12:04, Oleksii Kurochko wrote:
> Return type was left 'int' because of the following compilation
> error:
>
> ./include/xen/kernel.h:18:21: error: comparison of distinct
> pointer
> types lacks a cast [-Werror]
>    18 | (void) (&_x == &_y);    \
>   | ^~
>     common/page_alloc.c:1843:34: note: in expansion of macro
> 'min'
>  1843 | unsigned int inc_order = min(MAX_ORDER,
> flsl(e
> - s) - 1);

 Apart from this I'm okay with this patch, assuming Andrew's won't
 change in
 any conflicting way. As to the above - no, I don't see us having
 ffs() / ffsl()
 returning unsigned int, fls() / flsl() returning plain int. Even
 more
 so that,
 given the LHS variable's type, an unsigned quantity is really
 meant
 in the
 quoted code.
>>> If I understand you correctly, it's acceptable for fls() / flsl()
>>> to
>>> return 'int'. Therefore, I can update the commit message by
>>> removing
>>> the part mentioning the compilation error, as it's expected for
>>> fls() /
>>> flsl() to return 'int'. Is my understanding correct?
>>
>> No. I firmly object to ffs() and fls() being different in their
>> return
>> types. I'm sorry, I realize now that my earlier wording was ambiguous
>> (at least missing "but" after the comma).
> Thanks for clarifying.
> 
> I can change return type of fls() / flsl() to 'unsingned int' to be the
> same as return type of ffs() / ffsl(), but then it will be needed to
> add a cast in two places:

Except that no, it doesn't really need casts there.

>--- a/xen/common/page_alloc.c
>+++ b/xen/common/page_alloc.c
>@@ -1842,7 +1842,7 @@ static void _init_heap_pages(const struct
>page_info *pg,
>  * Note that the value of ffsl() and flsl() starts from 1
>so we need
>  * to decrement it by 1.
>  */
>-unsigned int inc_order = min(MAX_ORDER, flsl(e - s) - 1);
>+unsigned int inc_order = min((unsigned int)MAX_ORDER,
>flsl(e - s) - 1);

The preferred course of action would want to be to simply make MAX_ORDER
expand to an unsigned constant. Depending on the amount of fallout, an
alternative would be to use _AC(MAX_ORDER, U) here. Yet another
alternative would be to use MAX_ORDER + 0U here, as iirc we do in a few
other places, for similar purposes.

Avoiding a cast here is not only shorter, but - see statements elsewhere -
generally preferable.

Jan

Re: [XEN PATCH v3 5/5] xen/arm: ffa: support notification

[Bertrand Marquis]

Hi Jens,

> On 26 Apr 2024, at 14:11, Jens Wiklander  wrote:
> 
> Hi Bertrand,
> 
> On Fri, Apr 26, 2024 at 11:20 AM Bertrand Marquis
>  wrote:
>> 
>> Hi Jens,
>> 
>>> On 26 Apr 2024, at 10:47, Jens Wiklander  wrote:
>>> 
>>> Add support for FF-A notifications, currently limited to an SP (Secure
>>> Partition) sending an asynchronous notification to a guest.
>>> 
>>> Guests and Xen itself are made aware of pending notifications with an
>>> interrupt. The interrupt handler retrieves the notifications using the
>>> FF-A ABI and deliver them to their destinations.
>>> 
>>> Update ffa_partinfo_domain_init() to return error code like
>>> ffa_notif_domain_init().
>>> 
>>> Signed-off-by: Jens Wiklander 
>>> ---
>>> v2->v3:
>>> - Add a GUEST_ prefix and move FFA_NOTIF_PEND_INTR_ID and
>>> FFA_SCHEDULE_RECV_INTR_ID to public/arch-arm.h
>>> - Register the Xen SRI handler on each CPU using on_selected_cpus() and
>>> setup_irq()
>>> - Check that the SGI ID retrieved with FFA_FEATURE_SCHEDULE_RECV_INTR
>>> doesn't conflict with static SGI handlers
>>> 
>>> v1->v2:
>>> - Addressing review comments
>>> - Change ffa_handle_notification_{bind,unbind,set}() to take struct
>>> cpu_user_regs *regs as argument.
>>> - Update ffa_partinfo_domain_init() and ffa_notif_domain_init() to return
>>> an error code.
>>> - Fixing a bug in handle_features() for FFA_FEATURE_SCHEDULE_RECV_INTR.
>>> ---
>>> xen/arch/arm/irq.c  |   2 +-
>>> xen/arch/arm/tee/Makefile   |   1 +
>>> xen/arch/arm/tee/ffa.c  |  55 -
>>> xen/arch/arm/tee/ffa_notif.c| 378 
>>> xen/arch/arm/tee/ffa_partinfo.c |   9 +-
>>> xen/arch/arm/tee/ffa_private.h  |  56 -
>>> xen/include/public/arch-arm.h   |  14 ++
>>> 7 files changed, 507 insertions(+), 8 deletions(-)
>>> create mode 100644 xen/arch/arm/tee/ffa_notif.c
>>> 
>>> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
>>> index d7306aa64d50..5224898265a5 100644
>>> --- a/xen/arch/arm/irq.c
>>> +++ b/xen/arch/arm/irq.c
>>> @@ -155,7 +155,7 @@ void __init init_IRQ(void)
>>>{
>>>/* SGIs are always edge-triggered */
>>>if ( irq < NR_GIC_SGI )
>>> -local_irqs_type[irq] = DT_IRQ_TYPE_EDGE_RISING;
>>> +local_irqs_type[irq] = IRQ_TYPE_EDGE_RISING;
>>>else
>>>local_irqs_type[irq] = IRQ_TYPE_INVALID;
>>>}
>>> diff --git a/xen/arch/arm/tee/Makefile b/xen/arch/arm/tee/Makefile
>>> index f0112a2f922d..7c0f46f7f446 100644
>>> --- a/xen/arch/arm/tee/Makefile
>>> +++ b/xen/arch/arm/tee/Makefile
>>> @@ -2,5 +2,6 @@ obj-$(CONFIG_FFA) += ffa.o
>>> obj-$(CONFIG_FFA) += ffa_shm.o
>>> obj-$(CONFIG_FFA) += ffa_partinfo.o
>>> obj-$(CONFIG_FFA) += ffa_rxtx.o
>>> +obj-$(CONFIG_FFA) += ffa_notif.o
>>> obj-y += tee.o
>>> obj-$(CONFIG_OPTEE) += optee.o
>>> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
>>> index 5209612963e1..912e905a27bd 100644
>>> --- a/xen/arch/arm/tee/ffa.c
>>> +++ b/xen/arch/arm/tee/ffa.c
>>> @@ -39,6 +39,9 @@
>>> *   - at most 32 shared memory regions per guest
>>> * o FFA_MSG_SEND_DIRECT_REQ:
>>> *   - only supported from a VM to an SP
>>> + * o FFA_NOTIFICATION_*:
>>> + *   - only supports global notifications, that is, per vCPU notifications
>>> + * are not supported
>>> *
>>> * There are some large locked sections with ffa_tx_buffer_lock and
>>> * ffa_rx_buffer_lock. Especially the ffa_tx_buffer_lock spinlock used
>>> @@ -194,6 +197,8 @@ out:
>>> 
>>> static void handle_features(struct cpu_user_regs *regs)
>>> {
>>> +struct domain *d = current->domain;
>>> +struct ffa_ctx *ctx = d->arch.tee;
>>>uint32_t a1 = get_user_reg(regs, 1);
>>>unsigned int n;
>>> 
>>> @@ -240,6 +245,30 @@ static void handle_features(struct cpu_user_regs *regs)
>>>BUILD_BUG_ON(PAGE_SIZE != FFA_PAGE_SIZE);
>>>ffa_set_regs_success(regs, 0, 0);
>>>break;
>>> +case FFA_FEATURE_NOTIF_PEND_INTR:
>>> +if ( ctx->notif.enabled )
>>> +ffa_set_regs_success(regs, GUEST_FFA_NOTIF_PEND_INTR_ID, 0);
>>> +else
>>> +ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
>>> +break;
>>> +case FFA_FEATURE_SCHEDULE_RECV_INTR:
>>> +if ( ctx->notif.enabled )
>>> +ffa_set_regs_success(regs, GUEST_FFA_SCHEDULE_RECV_INTR_ID, 0);
>>> +else
>>> +ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
>>> +break;
>>> +
>>> +case FFA_NOTIFICATION_BIND:
>>> +case FFA_NOTIFICATION_UNBIND:
>>> +case FFA_NOTIFICATION_GET:
>>> +case FFA_NOTIFICATION_SET:
>>> +case FFA_NOTIFICATION_INFO_GET_32:
>>> +case FFA_NOTIFICATION_INFO_GET_64:
>>> +if ( ctx->notif.enabled )
>>> +ffa_set_regs_success(regs, 0, 0);
>>> +else
>>> +ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
>>> +break;
>>>default:
>>>ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
>>>break;
>>> @@ -305,6 +334,22 @@ static bool 

Re: [XEN PATCH v3 5/5] xen/arm: ffa: support notification

[Jens Wiklander]

Hi Bertrand,

On Fri, Apr 26, 2024 at 11:20 AM Bertrand Marquis
 wrote:
>
> Hi Jens,
>
> > On 26 Apr 2024, at 10:47, Jens Wiklander  wrote:
> >
> > Add support for FF-A notifications, currently limited to an SP (Secure
> > Partition) sending an asynchronous notification to a guest.
> >
> > Guests and Xen itself are made aware of pending notifications with an
> > interrupt. The interrupt handler retrieves the notifications using the
> > FF-A ABI and deliver them to their destinations.
> >
> > Update ffa_partinfo_domain_init() to return error code like
> > ffa_notif_domain_init().
> >
> > Signed-off-by: Jens Wiklander 
> > ---
> > v2->v3:
> > - Add a GUEST_ prefix and move FFA_NOTIF_PEND_INTR_ID and
> >  FFA_SCHEDULE_RECV_INTR_ID to public/arch-arm.h
> > - Register the Xen SRI handler on each CPU using on_selected_cpus() and
> >  setup_irq()
> > - Check that the SGI ID retrieved with FFA_FEATURE_SCHEDULE_RECV_INTR
> >  doesn't conflict with static SGI handlers
> >
> > v1->v2:
> > - Addressing review comments
> > - Change ffa_handle_notification_{bind,unbind,set}() to take struct
> >  cpu_user_regs *regs as argument.
> > - Update ffa_partinfo_domain_init() and ffa_notif_domain_init() to return
> >  an error code.
> > - Fixing a bug in handle_features() for FFA_FEATURE_SCHEDULE_RECV_INTR.
> > ---
> > xen/arch/arm/irq.c  |   2 +-
> > xen/arch/arm/tee/Makefile   |   1 +
> > xen/arch/arm/tee/ffa.c  |  55 -
> > xen/arch/arm/tee/ffa_notif.c| 378 
> > xen/arch/arm/tee/ffa_partinfo.c |   9 +-
> > xen/arch/arm/tee/ffa_private.h  |  56 -
> > xen/include/public/arch-arm.h   |  14 ++
> > 7 files changed, 507 insertions(+), 8 deletions(-)
> > create mode 100644 xen/arch/arm/tee/ffa_notif.c
> >
> > diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> > index d7306aa64d50..5224898265a5 100644
> > --- a/xen/arch/arm/irq.c
> > +++ b/xen/arch/arm/irq.c
> > @@ -155,7 +155,7 @@ void __init init_IRQ(void)
> > {
> > /* SGIs are always edge-triggered */
> > if ( irq < NR_GIC_SGI )
> > -local_irqs_type[irq] = DT_IRQ_TYPE_EDGE_RISING;
> > +local_irqs_type[irq] = IRQ_TYPE_EDGE_RISING;
> > else
> > local_irqs_type[irq] = IRQ_TYPE_INVALID;
> > }
> > diff --git a/xen/arch/arm/tee/Makefile b/xen/arch/arm/tee/Makefile
> > index f0112a2f922d..7c0f46f7f446 100644
> > --- a/xen/arch/arm/tee/Makefile
> > +++ b/xen/arch/arm/tee/Makefile
> > @@ -2,5 +2,6 @@ obj-$(CONFIG_FFA) += ffa.o
> > obj-$(CONFIG_FFA) += ffa_shm.o
> > obj-$(CONFIG_FFA) += ffa_partinfo.o
> > obj-$(CONFIG_FFA) += ffa_rxtx.o
> > +obj-$(CONFIG_FFA) += ffa_notif.o
> > obj-y += tee.o
> > obj-$(CONFIG_OPTEE) += optee.o
> > diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> > index 5209612963e1..912e905a27bd 100644
> > --- a/xen/arch/arm/tee/ffa.c
> > +++ b/xen/arch/arm/tee/ffa.c
> > @@ -39,6 +39,9 @@
> >  *   - at most 32 shared memory regions per guest
> >  * o FFA_MSG_SEND_DIRECT_REQ:
> >  *   - only supported from a VM to an SP
> > + * o FFA_NOTIFICATION_*:
> > + *   - only supports global notifications, that is, per vCPU notifications
> > + * are not supported
> >  *
> >  * There are some large locked sections with ffa_tx_buffer_lock and
> >  * ffa_rx_buffer_lock. Especially the ffa_tx_buffer_lock spinlock used
> > @@ -194,6 +197,8 @@ out:
> >
> > static void handle_features(struct cpu_user_regs *regs)
> > {
> > +struct domain *d = current->domain;
> > +struct ffa_ctx *ctx = d->arch.tee;
> > uint32_t a1 = get_user_reg(regs, 1);
> > unsigned int n;
> >
> > @@ -240,6 +245,30 @@ static void handle_features(struct cpu_user_regs *regs)
> > BUILD_BUG_ON(PAGE_SIZE != FFA_PAGE_SIZE);
> > ffa_set_regs_success(regs, 0, 0);
> > break;
> > +case FFA_FEATURE_NOTIF_PEND_INTR:
> > +if ( ctx->notif.enabled )
> > +ffa_set_regs_success(regs, GUEST_FFA_NOTIF_PEND_INTR_ID, 0);
> > +else
> > +ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
> > +break;
> > +case FFA_FEATURE_SCHEDULE_RECV_INTR:
> > +if ( ctx->notif.enabled )
> > +ffa_set_regs_success(regs, GUEST_FFA_SCHEDULE_RECV_INTR_ID, 0);
> > +else
> > +ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
> > +break;
> > +
> > +case FFA_NOTIFICATION_BIND:
> > +case FFA_NOTIFICATION_UNBIND:
> > +case FFA_NOTIFICATION_GET:
> > +case FFA_NOTIFICATION_SET:
> > +case FFA_NOTIFICATION_INFO_GET_32:
> > +case FFA_NOTIFICATION_INFO_GET_64:
> > +if ( ctx->notif.enabled )
> > +ffa_set_regs_success(regs, 0, 0);
> > +else
> > +ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
> > +break;
> > default:
> > ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
> > break;
> > @@ -305,6 +334,22 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
> >   

Re: [PATCH v8 03/17] xen/bitops: implement fls{l}() in common logic

[Oleksii]

On Fri, 2024-04-26 at 12:51 +0200, Jan Beulich wrote:
> On 26.04.2024 10:21, Oleksii wrote:
> > On Thu, 2024-04-25 at 17:44 +0200, Jan Beulich wrote:
> > > On 17.04.2024 12:04, Oleksii Kurochko wrote:
> > > > Return type was left 'int' because of the following compilation
> > > > error:
> > > > 
> > > > ./include/xen/kernel.h:18:21: error: comparison of distinct
> > > > pointer
> > > > types lacks a cast [-Werror]
> > > >    18 | (void) (&_x == &_y);    \
> > > >   | ^~
> > > >     common/page_alloc.c:1843:34: note: in expansion of macro
> > > > 'min'
> > > >  1843 | unsigned int inc_order = min(MAX_ORDER,
> > > > flsl(e
> > > > - s) - 1);
> > > 
> > > Apart from this I'm okay with this patch, assuming Andrew's won't
> > > change in
> > > any conflicting way. As to the above - no, I don't see us having
> > > ffs() / ffsl()
> > > returning unsigned int, fls() / flsl() returning plain int. Even
> > > more
> > > so that,
> > > given the LHS variable's type, an unsigned quantity is really
> > > meant
> > > in the
> > > quoted code.
> > If I understand you correctly, it's acceptable for fls() / flsl()
> > to
> > return 'int'. Therefore, I can update the commit message by
> > removing
> > the part mentioning the compilation error, as it's expected for
> > fls() /
> > flsl() to return 'int'. Is my understanding correct?
> 
> No. I firmly object to ffs() and fls() being different in their
> return
> types. I'm sorry, I realize now that my earlier wording was ambiguous
> (at least missing "but" after the comma).
Thanks for clarifying.

I can change return type of fls() / flsl() to 'unsingned int' to be the
same as return type of ffs() / ffsl(), but then it will be needed to
add a cast in two places:
   --- a/xen/common/page_alloc.c
   +++ b/xen/common/page_alloc.c
   @@ -1842,7 +1842,7 @@ static void _init_heap_pages(const struct
   page_info *pg,
 * Note that the value of ffsl() and flsl() starts from 1
   so we need
 * to decrement it by 1.
 */
   -unsigned int inc_order = min(MAX_ORDER, flsl(e - s) - 1);
   +unsigned int inc_order = min((unsigned int)MAX_ORDER,
   flsl(e - s) - 1);

if ( s )
inc_order = min(inc_order, ffsl(s) - 1U);
   @@ -2266,7 +2266,7 @@ void __init xenheap_max_mfn(unsigned long mfn)
ASSERT(!first_node_initialised);
ASSERT(!xenheap_bits);
BUILD_BUG_ON((PADDR_BITS - PAGE_SHIFT) >= BITS_PER_LONG);
   -xenheap_bits = min(flsl(mfn + 1) - 1 + PAGE_SHIFT, PADDR_BITS);
   +xenheap_bits = min(flsl(mfn + 1) - 1 + PAGE_SHIFT, (unsigned
   int)PADDR_BITS);
printk(XENLOG_INFO "Xen heap: %u bits\n", xenheap_bits);
}
   
If it looks okay, then I'll do that in the next patch version.

~ Oleksii

MISRA and -Wextra-semi

[Andrew Cooper]

Hi,

Based on a call a long while back, I experimented with -Wextra-semi. 
This is what lead to 8e36c668ca107 "xen: Drop superfluous semi-colons".

However, there are a number of problems with getting this working
fully.  First, we need workarounds like this:

diff --git a/xen/include/xen/config.h b/xen/include/xen/config.h
index d888b2314daf..12e99c6dded4 100644
--- a/xen/include/xen/config.h
+++ b/xen/include/xen/config.h
@@ -26,7 +26,7 @@
 
 #include 
 
-#define EXPORT_SYMBOL(var)
+#define EXPORT_SYMBOL(var) typedef int var##_ignore_t
 
 /*
  * The following log levels are as follows:

to avoid a failure for users, which do legitimately have a semi-colon. 
It occurs to me that we could swap the typedef for as asm("") which
might be a little less unpleasant.

But with the simple cases taken care of, we then hit:

In file included from common/grant_table.c:3813:
common/compat/grant_table.c:10:1: error: extra ';' outside of a function
[-Werror,-Wextra-semi]
CHECK_grant_entry_v1;
^

which is quickly starting to unravel.

Finally, while Clang does have -Wextra-semi working properly for C, GCC
does this:

cc1: warning: command-line option ‘-Wextra-semi’ is valid for C++/ObjC++
but not for C

instead, which passes the cc-option-add check (which doesn't contain
-Werror), but causes the real build to fail.


So, while -Wextra-semi is definitely useful to find some hidden
problems, it seems like using it fully might be very complicated.  How
much do we care?

~Andrew

[linux-linus test] 185802: tolerable FAIL - PUSHED

[osstest service owner]

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

Failures :-/ but no regressions.

Tests which are failing intermittently (not blocking):
 test-armhf-armhf-examine  8 reboot  fail pass in 185810-retest

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-libvirt 16 saverestore-support-checkfail  like 185768
 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 185768
 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 185768
 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 185768
 test-amd64-amd64-xl-qemut-debianhvm-i386-xsm 12 debian-hvm-install fail like 
185768
 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 185768
 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 185768
 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-arm64-arm64-xl-credit1  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt 15 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-qcow2 14 migrate-support-checkfail never pass
 test-armhf-armhf-xl  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail  never pass
 test-armhf-armhf-xl-arndale  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  16 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-raw 14 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-vhd  14 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-vhd  15 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-qcow214 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-qcow215 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-raw  14 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-raw  15 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt-vhd 14 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-vhd 15 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  16 saverestore-support-checkfail   never pass

version targeted for testing:
 linuxc942a0cd3603e34dd2d7237e064d9318cb7f9654
baseline version:
 linux71b1543c83d65af8215d7558d70fc2ecbee77dcf

Last test of basis   185768  2024-04-23 07:46:10 Z3 days
Failing since185779  2024-04-24 00:13:50 Z2 days5 attempts
Testing same since   185802  2024-04-26 01:58:59 Z0 days1 attempts


People who touched revisions under test:
  Abdelrahman Morsy 
  Aleksandr Loktionov 
  Alex Elder 
  Alexander Zubkov 
  Andreas Taschner 
  Arkadiusz Kubalewski 
  Arınç ÜNAL 
  Avraham Stern 
  Bartosz Golaszewski 
  Benjamin Tissoires 
  Chuck Lever 
  Chun-Yi Lee 
  

[ANNOUNCE] Xen Project Summit 2024 Design Sessions

[Kelly Choi]

Hello Xen Community,

*Our design sessions are now open for Xen Summit! *

If you've attended Xen Summit before, you might be familiar with the
process.

For anyone who hasn't done so before, please follow the instructions below,
using the link to create an account
.
Once you've created your account, you'll be asked to verify using the code
below. That's it, you're in!

We aim to have design sessions with a virtual element (using Jitsi/Zoom).
This is free for the community to join in and listen, and will be hosted by
an attendee in the session.

If you're going to be an *in-person attendee*, you can propose a session
and vote on sessions you would like to see discussed.
If you're going to be a *virtual attendee*, you can vote on sessions you
would like to see discussed. You can still propose a session, but please
note there will be no professional audio or visual equipment in person for
these sessions.

The final schedule will be allocated and arranged by the highest-voted
sessions.

Virtual links for the community to join Xen Summit design sessions will be
shared closer to the event.

If you have any questions, please let me know.

For in-person tickets, click here
.

--




We look forward to holding the Design Sessions at the upcoming Xen Project
Summit. The design sessions will be on Wednesday, 5 June, and Thursday, 6
June 2024.

We encourage everyone to submit a Design Session, the verification code is:
“*LFXEN24*”.

*SUBMIT A DESIGN SESSION* 



The process involves the following steps:

   - Anyone interested can propose
    a topic.
   - All participants review the list of sessions
   , indicating
   their interest in attending each one.
   - The session scheduler optimizes the schedule
    to accommodate as many
   preferences as possible.

Participants can also propose long-form talks by adding [TALK] to the
session title.

For suggested topics, sample Design Session submissions, and more tips
check out the Xen Design Session page

for more information.


Best Regards,
Xen Project Events Team


--

Re: [PATCH v3] xen/riscv: check whether the assembler has Zbb extension support

[Jan Beulich]

On 26.04.2024 10:26, Oleksii wrote:
> On Mon, 2024-04-22 at 17:41 +0200, Oleksii wrote:
>> On Mon, 2024-04-22 at 11:43 +0200, Jan Beulich wrote:
>>> On 19.04.2024 16:23, Oleksii Kurochko wrote:
 Update the argument of the as-insn for the Zbb case to verify
 that
 Zbb is supported not only by a compiler, but also by an
 assembler.

 Also, check-extenstion(ext_name, "insn") helper macro is
 introduced
 to check whether extension is supported by a compiler and an
 assembler.

 Signed-off-by: Oleksii Kurochko 
>>>
>>> Acked-by: Jan Beulich 
>>> despite ...
>>>
 --- a/xen/arch/riscv/arch.mk
 +++ b/xen/arch/riscv/arch.mk
 @@ -11,9 +11,14 @@ riscv-march-$(CONFIG_RISCV_ISA_C)   :=
 $(riscv-march-y)c
  
  riscv-generic-flags := $(riscv-abi-y) -march=$(riscv-march-y)
  
 -zbb := $(call as-insn,$(CC) $(riscv-generic-flags)_zbb,"",_zbb)
 -zihintpause := $(call as-insn, \
 -  $(CC) $(riscv-generic-
 flags)_zihintpause,"pause",_zihintpause)
 +# check-extension: Check whether extenstion is supported by a
 compiler and
 +#  an assembler.
 +# Usage: $(call check-extension,extension_name,"instr")
 +check-extension = $(call as-insn,$(CC) $(riscv-generic-
 flags)_$(1),$(2),_$(1))
 +
 +zbb-insn := "andn t0, t0, t0"
 +zbb := $(call check-extension,zbb,$(zbb-insn))
 +zihintpause := $(call check-extension,zihintpause,"pause")
>>>
>>> ... still not really being happy with this: Either, as said before,
>>> zbb-insn
>>> would better be avoided (by using $(comma) as necessary), or you
>>> should have
>>> gone yet a step further to fully address my "redundancy" concern.
>>> Note how
>>> the two ultimate lines still have 3 (zbb) and 2 (zihintpause)
>>> references
>>> respectively, when the goal ought to be to have exactly one. E.g.
>>> along the
>>> lines of
>>>
>>> $(call check-extension,zbb)
>>> $(call check-extension,zihintpause)
>>>
>>> suitably using $(eval ...) (to effect the variable definitions) and
>>> defining
>>> both zbb-insn and zihintpause-insn.
>>>
>>> But I'll nevertheless put this in as is, unless you tell me you're
>>> up
>>> to
>>> going the extra step.

Well, as per this v3 went in already. Hence ...

>> I am okay with all your suggestions. So the final version will look
>> like ( if I understood you correctly ):
> Jan,
> 
> Could you please review the changes below? I just want to ensure that
> you are okay with them. If you are, I'll add your Acked-by that you
> gave to this patch in previous answers.

... if you now want to further update the logic, it'll be a new patch
anyway. The adjustments below look okay to me, but I'm not going to
insist at this point that this be further fiddled with.

Jan

>> --- a/xen/arch/riscv/arch.mk
>> +++ b/xen/arch/riscv/arch.mk
>> @@ -13,12 +13,19 @@ riscv-generic-flags := $(riscv-abi-y) -
>> march=$(riscv-march-y)
>>  
>>  # check-extension: Check whether extenstion is supported by a
>> compiler
>> and
>>  #  an assembler.
>> -# Usage: $(call check-extension,extension_name,"instr")
>> -check-extension = $(call as-insn,$(CC) $(riscv-generic-
>> flags)_$(1),$(2),_$(1))
>> +# Usage: $(call check-extension,extension_name).
>> +#    it should be defined variable with name: extension-name :=
>> "insn"
>>  
>> -zbb-insn := "andn t0, t0, t0"
>> -zbb := $(call check-extension,zbb,$(zbb-insn))
>> -zihintpause := $(call check-extension,zihintpause,"pause")
>> +define check-extension =
>> +$(eval $(1) := \
>> +   $(call as-insn,$(CC) $(riscv-generic-flags)_$(1),$(value
>> $(1)-
>> insn),_$(1)))
>> +endef
>> +
>> +zbb-insn := "andn t0$(comma)t0$(comma)t0"
>> +$(call check-extension,zbb)
>> +
>> +zihintpause-insn := "pause"
>> +$(call check-extension,zihintpause)
>>  
>>  extensions := $(zbb) $(zihintpause)
>>
>> If the diff above looks good, I'll sent a new patch version.
>>
>> Thanks.
>>
>> ~ Oleksii
> 

Re: [PATCH v8 03/17] xen/bitops: implement fls{l}() in common logic

[Jan Beulich]

On 26.04.2024 10:21, Oleksii wrote:
> On Thu, 2024-04-25 at 17:44 +0200, Jan Beulich wrote:
>> On 17.04.2024 12:04, Oleksii Kurochko wrote:
>>> Return type was left 'int' because of the following compilation
>>> error:
>>>
>>> ./include/xen/kernel.h:18:21: error: comparison of distinct pointer
>>> types lacks a cast [-Werror]
>>>    18 | (void) (&_x == &_y);    \
>>>   | ^~
>>>     common/page_alloc.c:1843:34: note: in expansion of macro 'min'
>>>  1843 | unsigned int inc_order = min(MAX_ORDER, flsl(e
>>> - s) - 1);
>>
>> Apart from this I'm okay with this patch, assuming Andrew's won't
>> change in
>> any conflicting way. As to the above - no, I don't see us having
>> ffs() / ffsl()
>> returning unsigned int, fls() / flsl() returning plain int. Even more
>> so that,
>> given the LHS variable's type, an unsigned quantity is really meant
>> in the
>> quoted code.
> If I understand you correctly, it's acceptable for fls() / flsl() to
> return 'int'. Therefore, I can update the commit message by removing
> the part mentioning the compilation error, as it's expected for fls() /
> flsl() to return 'int'. Is my understanding correct?

No. I firmly object to ffs() and fls() being different in their return
types. I'm sorry, I realize now that my earlier wording was ambiguous
(at least missing "but" after the comma).

Jan

Re: [PATCH v8 02/17] xen: introduce generic non-atomic test_*bit()

[Jan Beulich]

On 26.04.2024 10:14, Oleksii wrote:
> On Thu, 2024-04-25 at 17:35 +0200, Jan Beulich wrote:
>>>   /* - Please tidy above here -
>>>  */
>>>   
>>>   #include 
>>>   
>>> +#ifndef arch_check_bitop_size
>>> +#define arch_check_bitop_size(addr)
>>
>> Can this really do nothing? Passing the address of an object smaller
>> than
>> bitop_uint_t will read past the object in the generic__*_bit()
>> functions.
> Agree, in generic case it would be better to add:
> #define arch_check_bitop_size(addr) (sizeof(*(addr)) <
> sizeof(bitop_uint_t))

At which point x86 won't need any special casing anymore, I think.

Jan

Re: [PATCH] xen-livepatch: fix --force option comparison

[Ross Lagerwall]

On Fri, Apr 26, 2024 at 8:21 AM Roger Pau Monne  wrote:
>
> The check for --force option shouldn't be against 0.
>
> Reported-by: Jan Beulich 
> Fixes: 62a72092a517 ('livepatch: introduce --force option')
> Signed-off-by: Roger Pau Monné 
> ---

Reviewed-by: Ross Lagerwall 

Re: [PATCH 0/2] Drop libsystemd

[Christian Lindig]

> On 25 Apr 2024, at 18:32, Andrew Cooper  wrote:
> 
> On advise from the systemd leadership.  See patch 1 for details.
> 
> Andrew Cooper (2):
>  tools/{c,o}xenstored: Don't link against libsystemd
>  tools: Drop libsystemd as a dependency

Acked-by: Christian Lindig 

I agree with the general direction of limiting exposure to systemd libraries. 
The exact way is currently being debated.

— C

Re: [PATCH 1/2] tools/{c,o}xenstored: Don't link against libsystemd

[Andrew Cooper]

On 26/04/2024 9:51 am, Anthony PERARD wrote:
> On Thu, Apr 25, 2024 at 07:16:23PM +0100, Andrew Cooper wrote:
>> On 25/04/2024 7:06 pm, Anthony PERARD wrote:
>>> On Thu, Apr 25, 2024 at 06:32:15PM +0100, Andrew Cooper wrote:
 libsystemd is a giant dependency for one single function, but in the wake 
 of
 the xz backdoor, it turns out that even systemd leadership recommend 
 against
 linking against libsystemd for sd_notify().

 Since commit 7b61011e1450 ("tools: make xenstore domain easy 
 configurable") in
 Xen 4.8, the launch-xenstore script invokes systemd-notify directly, so its
>>> That's not enough, it's needs to be `systemd-notify --ready` to be a
>>> replacement for sd_notify(READY=1).
>>>
 not even necessary for the xenstored's to call sd_notify() themselves.
>>> So, sd_notify() or equivalent is still necessary.
>>>
 Therefore, just drop the calls to sd_notify() and stop linking against
 libsystemd.
>>> Sounds good, be we need to replace the call by something like:
>>> echo READY=1 > $NOTIFY_SOCKET
>>> implemented in C and ocaml. Detail to be checked.
>>>
>>> Otherwise, things won't work.
>> Hmm.  It worked in XenRT when stripping this all out, but that is
> I don't know how XenServer is setup, maybe it doesn't matter?

In theory it's straight systemd, but I could also believe that Xapi
checks for the pidfile too.

>  Anyway...
>
>> extremely unintuitive behaviour for `systemd-notify --booted`, seeing as
>> it's entirely different to --ready.
> Yes, this --booted option should probably not exist, and there's
> `systemctl is-system-running` that does something similar.
>
>> I've got no interest in keeping the C around, but if:
>>
>> [ -n "$NOTIFY_SOCKET" ] && echo READY=1 > $NOTIFY_SOCKET
>>
>> works, then can't we just use that after waiting for the the pidfile ?
> Run `systemd-notify --ready` instead. Hopefully, that will be enough.
> ($NOTIFY_SOCKET is a socket, and a bit more complicated that I though,
> it can start with "@" for example)

I'll do a prep patch to adjust launch-xenstore after which this patch
should be fine in this form (modulo a tweak in the commit message).

~Andrew

Re: [PATCH 2/2] tools: Drop libsystemd as a dependency

[Anthony PERARD]

On Thu, Apr 25, 2024 at 06:32:16PM +0100, Andrew Cooper wrote:
> diff --git a/m4/systemd.m4 b/m4/systemd.m4
> index 112dc11b5e05..aa1ebe94f56c 100644
> --- a/m4/systemd.m4
> +++ b/m4/systemd.m4
> @@ -41,15 +41,6 @@ AC_DEFUN([AX_ALLOW_SYSTEMD_OPTS], [
>  ])
>  
>  AC_DEFUN([AX_CHECK_SYSTEMD_LIBS], [
> - PKG_CHECK_MODULES([SYSTEMD], [libsystemd-daemon],,
> - [PKG_CHECK_MODULES([SYSTEMD], [libsystemd >= 209])]
> -)
> - dnl pkg-config older than 0.24 does not set these for
> - dnl PKG_CHECK_MODULES() worth also noting is that as of version 208
> - dnl of systemd pkg-config --cflags currently yields no extra flags yet.
> - AC_SUBST([SYSTEMD_CFLAGS])
> - AC_SUBST([SYSTEMD_LIBS])
> -
>   AS_IF([test "x$SYSTEMD_DIR" = x], [
>   dnl In order to use the line below we need to fix upstream systemd
>   dnl to properly ${prefix} for child variables in
> @@ -83,23 +74,11 @@ AC_DEFUN([AX_CHECK_SYSTEMD_LIBS], [
>  AC_DEFUN([AX_CHECK_SYSTEMD], [
>   dnl Respect user override to disable
>   AS_IF([test "x$enable_systemd" != "xno"], [
> -  AS_IF([test "x$systemd" = "xy" ], [
> - AC_DEFINE([HAVE_SYSTEMD], [1], [Systemd available and enabled])
> - systemd=y
> - AX_CHECK_SYSTEMD_LIBS()

I think you need to keep calling AX_CHECK_SYSTEMD_LIBS() here,
otherwise, nothing sets $SYSTEMD_DIR or $SYSTEMD_MODULES_LOAD.

> - ],[
> - AS_IF([test "x$enable_systemd" = "xyes"],
> - [AC_MSG_ERROR([Unable to find systemd development 
> library])],
> - [systemd=n])
> - ])
> + systemd=y
>   ],[systemd=n])
>  ])
>  
>  AC_DEFUN([AX_CHECK_SYSTEMD_ENABLE_AVAILABLE], [
> - PKG_CHECK_MODULES([SYSTEMD], [libsystemd-daemon], [systemd="y"],[
> - PKG_CHECK_MODULES([SYSTEMD], [libsystemd >= 209],
> -   [systemd="y"],[systemd="n"])
> - ])

Instead, or in addition, you could AX_AVAILABLE_SYSTEMD() in
configure.ac by AX_ENABLE_SYSTEMD(). (Or AX_ALLOW_SYSTEMD()).

With the current patch, AX_CHECK_SYSTEMD() will enable systemd
"support", even if it supposed to be disabled by default. So it's better
to use AX_ENABLE_SYSTEMD() as this will set the correct help message.

And can you add an entry in CHANGELOG? As systemd support isn't
automatically enabled with the presence of the libs anymore.

Thanks,

-- 
Anthony PERARD

Re: [XEN PATCH v3 0/5] FF-A notifications

[Bertrand Marquis]

Hi Jens

> On 26 Apr 2024, at 10:47, Jens Wiklander  wrote:
> 
> Hi,
> 
> This patch set adds support for FF-A notifications. We only support
> global notifications, per vCPU notifications remain unsupported.
> 
> The first three patches are further cleanup and can be merged before the
> rest if desired.
> 
> A physical SGI is used to make Xen aware of pending FF-A notifications. The
> physical SGI is selected by the SPMC in the secure world. Since it must not
> already be used by Xen the SPMC is in practice forced to donate one of the
> secure SGIs, but that's normally not a problem. The SGI handling in Xen is
> updated to support registration of handlers for SGIs that aren't statically
> assigned, that is, SGI IDs above GIC_SGI_MAX.

>From my point of view:
- patches 1 to 3 are ready to be commited.
- patch 4 will need a R-b from an other maintainer.
- patch 5 has still some stuff to be checked or fixed but could be
handled as a single patch if the rest or the serie is merged.


Regards
Bertrand


> 
> Thanks,
> Jens
> 
> v2->v3:
> - "xen/arm: ffa: support notification" and
>  "xen/arm: allow dynamically assigned SGI handlers" updated as requestsed,
>  details in each patch.
> 
> v1->v2:
> - "xen/arm: ffa: support notification" and
>  "xen/arm: allow dynamically assigned SGI handlers" updated as requestsed,
>  details in each patch.
> - Added Bertrands R-B for "xen/arm: ffa: refactor ffa_handle_call()",
>  "xen/arm: ffa: use ACCESS_ONCE()", and
>  "xen/arm: ffa: simplify ffa_handle_mem_share()"
> 
> Jens Wiklander (5):
>  xen/arm: ffa: refactor ffa_handle_call()
>  xen/arm: ffa: use ACCESS_ONCE()
>  xen/arm: ffa: simplify ffa_handle_mem_share()
>  xen/arm: allow dynamically assigned SGI handlers
>  xen/arm: ffa: support notification
> 
> xen/arch/arm/gic.c  |  12 +-
> xen/arch/arm/include/asm/gic.h  |   2 +-
> xen/arch/arm/irq.c  |  18 +-
> xen/arch/arm/tee/Makefile   |   1 +
> xen/arch/arm/tee/ffa.c  |  83 +--
> xen/arch/arm/tee/ffa_notif.c| 378 
> xen/arch/arm/tee/ffa_partinfo.c |   9 +-
> xen/arch/arm/tee/ffa_private.h  |  56 -
> xen/arch/arm/tee/ffa_shm.c  |  33 ++-
> xen/include/public/arch-arm.h   |  14 ++
> 10 files changed, 551 insertions(+), 55 deletions(-)
> create mode 100644 xen/arch/arm/tee/ffa_notif.c
> 
> -- 
> 2.34.1
> 

Re: [XEN PATCH v3 5/5] xen/arm: ffa: support notification

[Bertrand Marquis]

Hi Jens,

> On 26 Apr 2024, at 10:47, Jens Wiklander  wrote:
> 
> Add support for FF-A notifications, currently limited to an SP (Secure
> Partition) sending an asynchronous notification to a guest.
> 
> Guests and Xen itself are made aware of pending notifications with an
> interrupt. The interrupt handler retrieves the notifications using the
> FF-A ABI and deliver them to their destinations.
> 
> Update ffa_partinfo_domain_init() to return error code like
> ffa_notif_domain_init().
> 
> Signed-off-by: Jens Wiklander 
> ---
> v2->v3:
> - Add a GUEST_ prefix and move FFA_NOTIF_PEND_INTR_ID and
>  FFA_SCHEDULE_RECV_INTR_ID to public/arch-arm.h
> - Register the Xen SRI handler on each CPU using on_selected_cpus() and
>  setup_irq()
> - Check that the SGI ID retrieved with FFA_FEATURE_SCHEDULE_RECV_INTR
>  doesn't conflict with static SGI handlers
> 
> v1->v2:
> - Addressing review comments
> - Change ffa_handle_notification_{bind,unbind,set}() to take struct
>  cpu_user_regs *regs as argument.
> - Update ffa_partinfo_domain_init() and ffa_notif_domain_init() to return
>  an error code.
> - Fixing a bug in handle_features() for FFA_FEATURE_SCHEDULE_RECV_INTR.
> ---
> xen/arch/arm/irq.c  |   2 +-
> xen/arch/arm/tee/Makefile   |   1 +
> xen/arch/arm/tee/ffa.c  |  55 -
> xen/arch/arm/tee/ffa_notif.c| 378 
> xen/arch/arm/tee/ffa_partinfo.c |   9 +-
> xen/arch/arm/tee/ffa_private.h  |  56 -
> xen/include/public/arch-arm.h   |  14 ++
> 7 files changed, 507 insertions(+), 8 deletions(-)
> create mode 100644 xen/arch/arm/tee/ffa_notif.c
> 
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index d7306aa64d50..5224898265a5 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -155,7 +155,7 @@ void __init init_IRQ(void)
> {
> /* SGIs are always edge-triggered */
> if ( irq < NR_GIC_SGI )
> -local_irqs_type[irq] = DT_IRQ_TYPE_EDGE_RISING;
> +local_irqs_type[irq] = IRQ_TYPE_EDGE_RISING;
> else
> local_irqs_type[irq] = IRQ_TYPE_INVALID;
> }
> diff --git a/xen/arch/arm/tee/Makefile b/xen/arch/arm/tee/Makefile
> index f0112a2f922d..7c0f46f7f446 100644
> --- a/xen/arch/arm/tee/Makefile
> +++ b/xen/arch/arm/tee/Makefile
> @@ -2,5 +2,6 @@ obj-$(CONFIG_FFA) += ffa.o
> obj-$(CONFIG_FFA) += ffa_shm.o
> obj-$(CONFIG_FFA) += ffa_partinfo.o
> obj-$(CONFIG_FFA) += ffa_rxtx.o
> +obj-$(CONFIG_FFA) += ffa_notif.o
> obj-y += tee.o
> obj-$(CONFIG_OPTEE) += optee.o
> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> index 5209612963e1..912e905a27bd 100644
> --- a/xen/arch/arm/tee/ffa.c
> +++ b/xen/arch/arm/tee/ffa.c
> @@ -39,6 +39,9 @@
>  *   - at most 32 shared memory regions per guest
>  * o FFA_MSG_SEND_DIRECT_REQ:
>  *   - only supported from a VM to an SP
> + * o FFA_NOTIFICATION_*:
> + *   - only supports global notifications, that is, per vCPU notifications
> + * are not supported
>  *
>  * There are some large locked sections with ffa_tx_buffer_lock and
>  * ffa_rx_buffer_lock. Especially the ffa_tx_buffer_lock spinlock used
> @@ -194,6 +197,8 @@ out:
> 
> static void handle_features(struct cpu_user_regs *regs)
> {
> +struct domain *d = current->domain;
> +struct ffa_ctx *ctx = d->arch.tee;
> uint32_t a1 = get_user_reg(regs, 1);
> unsigned int n;
> 
> @@ -240,6 +245,30 @@ static void handle_features(struct cpu_user_regs *regs)
> BUILD_BUG_ON(PAGE_SIZE != FFA_PAGE_SIZE);
> ffa_set_regs_success(regs, 0, 0);
> break;
> +case FFA_FEATURE_NOTIF_PEND_INTR:
> +if ( ctx->notif.enabled )
> +ffa_set_regs_success(regs, GUEST_FFA_NOTIF_PEND_INTR_ID, 0);
> +else
> +ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
> +break;
> +case FFA_FEATURE_SCHEDULE_RECV_INTR:
> +if ( ctx->notif.enabled )
> +ffa_set_regs_success(regs, GUEST_FFA_SCHEDULE_RECV_INTR_ID, 0);
> +else
> +ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
> +break;
> +
> +case FFA_NOTIFICATION_BIND:
> +case FFA_NOTIFICATION_UNBIND:
> +case FFA_NOTIFICATION_GET:
> +case FFA_NOTIFICATION_SET:
> +case FFA_NOTIFICATION_INFO_GET_32:
> +case FFA_NOTIFICATION_INFO_GET_64:
> +if ( ctx->notif.enabled )
> +ffa_set_regs_success(regs, 0, 0);
> +else
> +ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
> +break;
> default:
> ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
> break;
> @@ -305,6 +334,22 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
>  get_user_reg(regs, 1)),
>get_user_reg(regs, 3));
> break;
> +case FFA_NOTIFICATION_BIND:
> +e = ffa_handle_notification_bind(regs);
> +break;
> +case FFA_NOTIFICATION_UNBIND:
> +e = 

Re: [PATCH] xen-livepatch: fix --force option comparison

[Anthony PERARD]

On Fri, Apr 26, 2024 at 09:21:44AM +0200, Roger Pau Monne wrote:
> The check for --force option shouldn't be against 0.
> 
> Reported-by: Jan Beulich 
> Fixes: 62a72092a517 ('livepatch: introduce --force option')
> Signed-off-by: Roger Pau Monné 

Acked-by: Anthony PERARD 

Thanks,

-- 
Anthony PERARD

Re: [PATCH 1/2] tools/{c,o}xenstored: Don't link against libsystemd

[Anthony PERARD]

On Thu, Apr 25, 2024 at 07:16:23PM +0100, Andrew Cooper wrote:
> On 25/04/2024 7:06 pm, Anthony PERARD wrote:
> > On Thu, Apr 25, 2024 at 06:32:15PM +0100, Andrew Cooper wrote:
> >> libsystemd is a giant dependency for one single function, but in the wake 
> >> of
> >> the xz backdoor, it turns out that even systemd leadership recommend 
> >> against
> >> linking against libsystemd for sd_notify().
> >>
> >> Since commit 7b61011e1450 ("tools: make xenstore domain easy 
> >> configurable") in
> >> Xen 4.8, the launch-xenstore script invokes systemd-notify directly, so its
> > That's not enough, it's needs to be `systemd-notify --ready` to be a
> > replacement for sd_notify(READY=1).
> >
> >> not even necessary for the xenstored's to call sd_notify() themselves.
> > So, sd_notify() or equivalent is still necessary.
> >
> >> Therefore, just drop the calls to sd_notify() and stop linking against
> >> libsystemd.
> > Sounds good, be we need to replace the call by something like:
> > echo READY=1 > $NOTIFY_SOCKET
> > implemented in C and ocaml. Detail to be checked.
> >
> > Otherwise, things won't work.
> 
> Hmm.  It worked in XenRT when stripping this all out, but that is

I don't know how XenServer is setup, maybe it doesn't matter? Anyway...

> extremely unintuitive behaviour for `systemd-notify --booted`, seeing as
> it's entirely different to --ready.

Yes, this --booted option should probably not exist, and there's
`systemctl is-system-running` that does something similar.

> 
> I've got no interest in keeping the C around, but if:
> 
> [ -n "$NOTIFY_SOCKET" ] && echo READY=1 > $NOTIFY_SOCKET
> 
> works, then can't we just use that after waiting for the the pidfile ?

Run `systemd-notify --ready` instead. Hopefully, that will be enough.
($NOTIFY_SOCKET is a socket, and a bit more complicated that I though,
it can start with "@" for example)

Cheers,

-- 
Anthony PERARD

[XEN PATCH v3 5/5] xen/arm: ffa: support notification

[Jens Wiklander]

Add support for FF-A notifications, currently limited to an SP (Secure
Partition) sending an asynchronous notification to a guest.

Guests and Xen itself are made aware of pending notifications with an
interrupt. The interrupt handler retrieves the notifications using the
FF-A ABI and deliver them to their destinations.

Update ffa_partinfo_domain_init() to return error code like
ffa_notif_domain_init().

Signed-off-by: Jens Wiklander 
---
v2->v3:
- Add a GUEST_ prefix and move FFA_NOTIF_PEND_INTR_ID and
  FFA_SCHEDULE_RECV_INTR_ID to public/arch-arm.h
- Register the Xen SRI handler on each CPU using on_selected_cpus() and
  setup_irq()
- Check that the SGI ID retrieved with FFA_FEATURE_SCHEDULE_RECV_INTR
  doesn't conflict with static SGI handlers

v1->v2:
- Addressing review comments
- Change ffa_handle_notification_{bind,unbind,set}() to take struct
  cpu_user_regs *regs as argument.
- Update ffa_partinfo_domain_init() and ffa_notif_domain_init() to return
  an error code.
- Fixing a bug in handle_features() for FFA_FEATURE_SCHEDULE_RECV_INTR.
---
 xen/arch/arm/irq.c  |   2 +-
 xen/arch/arm/tee/Makefile   |   1 +
 xen/arch/arm/tee/ffa.c  |  55 -
 xen/arch/arm/tee/ffa_notif.c| 378 
 xen/arch/arm/tee/ffa_partinfo.c |   9 +-
 xen/arch/arm/tee/ffa_private.h  |  56 -
 xen/include/public/arch-arm.h   |  14 ++
 7 files changed, 507 insertions(+), 8 deletions(-)
 create mode 100644 xen/arch/arm/tee/ffa_notif.c

diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index d7306aa64d50..5224898265a5 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -155,7 +155,7 @@ void __init init_IRQ(void)
 {
 /* SGIs are always edge-triggered */
 if ( irq < NR_GIC_SGI )
-local_irqs_type[irq] = DT_IRQ_TYPE_EDGE_RISING;
+local_irqs_type[irq] = IRQ_TYPE_EDGE_RISING;
 else
 local_irqs_type[irq] = IRQ_TYPE_INVALID;
 }
diff --git a/xen/arch/arm/tee/Makefile b/xen/arch/arm/tee/Makefile
index f0112a2f922d..7c0f46f7f446 100644
--- a/xen/arch/arm/tee/Makefile
+++ b/xen/arch/arm/tee/Makefile
@@ -2,5 +2,6 @@ obj-$(CONFIG_FFA) += ffa.o
 obj-$(CONFIG_FFA) += ffa_shm.o
 obj-$(CONFIG_FFA) += ffa_partinfo.o
 obj-$(CONFIG_FFA) += ffa_rxtx.o
+obj-$(CONFIG_FFA) += ffa_notif.o
 obj-y += tee.o
 obj-$(CONFIG_OPTEE) += optee.o
diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
index 5209612963e1..912e905a27bd 100644
--- a/xen/arch/arm/tee/ffa.c
+++ b/xen/arch/arm/tee/ffa.c
@@ -39,6 +39,9 @@
  *   - at most 32 shared memory regions per guest
  * o FFA_MSG_SEND_DIRECT_REQ:
  *   - only supported from a VM to an SP
+ * o FFA_NOTIFICATION_*:
+ *   - only supports global notifications, that is, per vCPU notifications
+ * are not supported
  *
  * There are some large locked sections with ffa_tx_buffer_lock and
  * ffa_rx_buffer_lock. Especially the ffa_tx_buffer_lock spinlock used
@@ -194,6 +197,8 @@ out:
 
 static void handle_features(struct cpu_user_regs *regs)
 {
+struct domain *d = current->domain;
+struct ffa_ctx *ctx = d->arch.tee;
 uint32_t a1 = get_user_reg(regs, 1);
 unsigned int n;
 
@@ -240,6 +245,30 @@ static void handle_features(struct cpu_user_regs *regs)
 BUILD_BUG_ON(PAGE_SIZE != FFA_PAGE_SIZE);
 ffa_set_regs_success(regs, 0, 0);
 break;
+case FFA_FEATURE_NOTIF_PEND_INTR:
+if ( ctx->notif.enabled )
+ffa_set_regs_success(regs, GUEST_FFA_NOTIF_PEND_INTR_ID, 0);
+else
+ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
+break;
+case FFA_FEATURE_SCHEDULE_RECV_INTR:
+if ( ctx->notif.enabled )
+ffa_set_regs_success(regs, GUEST_FFA_SCHEDULE_RECV_INTR_ID, 0);
+else
+ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
+break;
+
+case FFA_NOTIFICATION_BIND:
+case FFA_NOTIFICATION_UNBIND:
+case FFA_NOTIFICATION_GET:
+case FFA_NOTIFICATION_SET:
+case FFA_NOTIFICATION_INFO_GET_32:
+case FFA_NOTIFICATION_INFO_GET_64:
+if ( ctx->notif.enabled )
+ffa_set_regs_success(regs, 0, 0);
+else
+ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
+break;
 default:
 ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
 break;
@@ -305,6 +334,22 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
  get_user_reg(regs, 1)),
get_user_reg(regs, 3));
 break;
+case FFA_NOTIFICATION_BIND:
+e = ffa_handle_notification_bind(regs);
+break;
+case FFA_NOTIFICATION_UNBIND:
+e = ffa_handle_notification_unbind(regs);
+break;
+case FFA_NOTIFICATION_INFO_GET_32:
+case FFA_NOTIFICATION_INFO_GET_64:
+ffa_handle_notification_info_get(regs);
+return true;
+case FFA_NOTIFICATION_GET:
+ffa_handle_notification_get(regs);
+return 

[XEN PATCH v3 1/5] xen/arm: ffa: refactor ffa_handle_call()

[Jens Wiklander]

Refactors the large switch block in ffa_handle_call() to use common code
for the simple case where it's either an error code or success with no
further parameters.

Signed-off-by: Jens Wiklander 
Reviewed-by: Bertrand Marquis 
---
 xen/arch/arm/tee/ffa.c | 30 ++
 1 file changed, 10 insertions(+), 20 deletions(-)

diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
index 8665201e34a9..5209612963e1 100644
--- a/xen/arch/arm/tee/ffa.c
+++ b/xen/arch/arm/tee/ffa.c
@@ -273,18 +273,10 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
 case FFA_RXTX_MAP_64:
 e = ffa_handle_rxtx_map(fid, get_user_reg(regs, 1),
get_user_reg(regs, 2), get_user_reg(regs, 3));
-if ( e )
-ffa_set_regs_error(regs, e);
-else
-ffa_set_regs_success(regs, 0, 0);
-return true;
+break;
 case FFA_RXTX_UNMAP:
 e = ffa_handle_rxtx_unmap();
-if ( e )
-ffa_set_regs_error(regs, e);
-else
-ffa_set_regs_success(regs, 0, 0);
-return true;
+break;
 case FFA_PARTITION_INFO_GET:
 e = ffa_handle_partition_info_get(get_user_reg(regs, 1),
   get_user_reg(regs, 2),
@@ -299,11 +291,7 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
 return true;
 case FFA_RX_RELEASE:
 e = ffa_handle_rx_release();
-if ( e )
-ffa_set_regs_error(regs, e);
-else
-ffa_set_regs_success(regs, 0, 0);
-return true;
+break;
 case FFA_MSG_SEND_DIRECT_REQ_32:
 case FFA_MSG_SEND_DIRECT_REQ_64:
 handle_msg_send_direct_req(regs, fid);
@@ -316,17 +304,19 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
 e = ffa_handle_mem_reclaim(regpair_to_uint64(get_user_reg(regs, 2),
  get_user_reg(regs, 1)),
get_user_reg(regs, 3));
-if ( e )
-ffa_set_regs_error(regs, e);
-else
-ffa_set_regs_success(regs, 0, 0);
-return true;
+break;
 
 default:
 gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
 ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
 return true;
 }
+
+if ( e )
+ffa_set_regs_error(regs, e);
+else
+ffa_set_regs_success(regs, 0, 0);
+return true;
 }
 
 static int ffa_domain_init(struct domain *d)
-- 
2.34.1

[XEN PATCH v3 3/5] xen/arm: ffa: simplify ffa_handle_mem_share()

[Jens Wiklander]

Simplify ffa_handle_mem_share() by removing the start_page_idx and
last_page_idx parameters from get_shm_pages() and check that the number
of pages matches expectations at the end of get_shm_pages().

Signed-off-by: Jens Wiklander 
Reviewed-by: Bertrand Marquis 
---
 xen/arch/arm/tee/ffa_shm.c | 18 ++
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/xen/arch/arm/tee/ffa_shm.c b/xen/arch/arm/tee/ffa_shm.c
index 75a5b66aeb4c..370d83ec5cf8 100644
--- a/xen/arch/arm/tee/ffa_shm.c
+++ b/xen/arch/arm/tee/ffa_shm.c
@@ -159,10 +159,9 @@ static int32_t ffa_mem_reclaim(uint32_t handle_lo, 
uint32_t handle_hi,
  */
 static int get_shm_pages(struct domain *d, struct ffa_shm_mem *shm,
  const struct ffa_address_range *range,
- uint32_t range_count, unsigned int start_page_idx,
- unsigned int *last_page_idx)
+ uint32_t range_count)
 {
-unsigned int pg_idx = start_page_idx;
+unsigned int pg_idx = 0;
 gfn_t gfn;
 unsigned int n;
 unsigned int m;
@@ -191,7 +190,9 @@ static int get_shm_pages(struct domain *d, struct 
ffa_shm_mem *shm,
 }
 }
 
-*last_page_idx = pg_idx;
+/* The ranges must add up */
+if ( pg_idx < shm->page_count )
+return FFA_RET_INVALID_PARAMETERS;
 
 return FFA_RET_OK;
 }
@@ -460,7 +461,6 @@ void ffa_handle_mem_share(struct cpu_user_regs *regs)
 struct domain *d = current->domain;
 struct ffa_ctx *ctx = d->arch.tee;
 struct ffa_shm_mem *shm = NULL;
-unsigned int last_page_idx = 0;
 register_t handle_hi = 0;
 register_t handle_lo = 0;
 int ret = FFA_RET_DENIED;
@@ -570,15 +570,9 @@ void ffa_handle_mem_share(struct cpu_user_regs *regs)
 goto out;
 }
 
-ret = get_shm_pages(d, shm, region_descr->address_range_array, range_count,
-0, _page_idx);
+ret = get_shm_pages(d, shm, region_descr->address_range_array, 
range_count);
 if ( ret )
 goto out;
-if ( last_page_idx != shm->page_count )
-{
-ret = FFA_RET_INVALID_PARAMETERS;
-goto out;
-}
 
 /* Note that share_shm() uses our tx buffer */
 spin_lock(_tx_buffer_lock);
-- 
2.34.1

[XEN PATCH v3 0/5] FF-A notifications

[Jens Wiklander]

Hi,

This patch set adds support for FF-A notifications. We only support
global notifications, per vCPU notifications remain unsupported.

The first three patches are further cleanup and can be merged before the
rest if desired.

A physical SGI is used to make Xen aware of pending FF-A notifications. The
physical SGI is selected by the SPMC in the secure world. Since it must not
already be used by Xen the SPMC is in practice forced to donate one of the
secure SGIs, but that's normally not a problem. The SGI handling in Xen is
updated to support registration of handlers for SGIs that aren't statically
assigned, that is, SGI IDs above GIC_SGI_MAX.

Thanks,
Jens

v2->v3:
- "xen/arm: ffa: support notification" and
  "xen/arm: allow dynamically assigned SGI handlers" updated as requestsed,
  details in each patch.

v1->v2:
- "xen/arm: ffa: support notification" and
  "xen/arm: allow dynamically assigned SGI handlers" updated as requestsed,
  details in each patch.
- Added Bertrands R-B for "xen/arm: ffa: refactor ffa_handle_call()",
  "xen/arm: ffa: use ACCESS_ONCE()", and
  "xen/arm: ffa: simplify ffa_handle_mem_share()"

Jens Wiklander (5):
  xen/arm: ffa: refactor ffa_handle_call()
  xen/arm: ffa: use ACCESS_ONCE()
  xen/arm: ffa: simplify ffa_handle_mem_share()
  xen/arm: allow dynamically assigned SGI handlers
  xen/arm: ffa: support notification

 xen/arch/arm/gic.c  |  12 +-
 xen/arch/arm/include/asm/gic.h  |   2 +-
 xen/arch/arm/irq.c  |  18 +-
 xen/arch/arm/tee/Makefile   |   1 +
 xen/arch/arm/tee/ffa.c  |  83 +--
 xen/arch/arm/tee/ffa_notif.c| 378 
 xen/arch/arm/tee/ffa_partinfo.c |   9 +-
 xen/arch/arm/tee/ffa_private.h  |  56 -
 xen/arch/arm/tee/ffa_shm.c  |  33 ++-
 xen/include/public/arch-arm.h   |  14 ++
 10 files changed, 551 insertions(+), 55 deletions(-)
 create mode 100644 xen/arch/arm/tee/ffa_notif.c

-- 
2.34.1

[XEN PATCH v3 4/5] xen/arm: allow dynamically assigned SGI handlers

[Jens Wiklander]

Updates so request_irq() can be used with a dynamically assigned SGI irq
as input. This prepares for a later patch where an FF-A schedule
receiver interrupt handler is installed for an SGI generated by the
secure world.

>From the Arm Base System Architecture v1.0C [1]:
"The system shall implement at least eight Non-secure SGIs, assigned to
interrupt IDs 0-7."

gic_route_irq_to_xen() don't gic_set_irq_type() for SGIs since they are
always edge triggered.

gic_interrupt() is updated to route the dynamically assigned SGIs to
do_IRQ() instead of do_sgi(). The latter still handles the statically
assigned SGI handlers like for instance GIC_SGI_CALL_FUNCTION.

[1] https://developer.arm.com/documentation/den0094/

Signed-off-by: Jens Wiklander 
---
v2->v3
- Rename GIC_SGI_MAX to GIC_SGI_STATIC_MAX and rename do_sgi() to
  do_static_sgi()
- Update comment in setup_irq() to mention that SGI irq_desc is banked
- Add ASSERT() in do_IRQ() that the irq isn't an SGI before injecting
  calling vgic_inject_irq()
- Initialize local_irqs_type[] range for SGIs as IRQ_TYPE_EDGE_RISING
- Adding link to the Arm Base System Architecture v1.0C

v1->v2
- Update patch description as requested
---
 xen/arch/arm/gic.c | 12 +++-
 xen/arch/arm/include/asm/gic.h |  2 +-
 xen/arch/arm/irq.c | 18 ++
 3 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 44c40e86defe..882768252740 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -38,7 +38,7 @@ const struct gic_hw_operations *gic_hw_ops;
 static void __init __maybe_unused build_assertions(void)
 {
 /* Check our enum gic_sgi only covers SGIs */
-BUILD_BUG_ON(GIC_SGI_MAX > NR_GIC_SGI);
+BUILD_BUG_ON(GIC_SGI_STATIC_MAX > NR_GIC_SGI);
 }
 
 void register_gic_ops(const struct gic_hw_operations *ops)
@@ -117,7 +117,9 @@ void gic_route_irq_to_xen(struct irq_desc *desc, unsigned 
int priority)
 
 desc->handler = gic_hw_ops->gic_host_irq_type;
 
-gic_set_irq_type(desc, desc->arch.type);
+/* SGIs are always edge-triggered, so there is need to set it */
+if ( desc->irq >= NR_GIC_SGI)
+gic_set_irq_type(desc, desc->arch.type);
 gic_set_irq_priority(desc, priority);
 }
 
@@ -330,7 +332,7 @@ void gic_disable_cpu(void)
 gic_hw_ops->disable_interface();
 }
 
-static void do_sgi(struct cpu_user_regs *regs, enum gic_sgi sgi)
+static void do_static_sgi(struct cpu_user_regs *regs, enum gic_sgi sgi)
 {
 struct irq_desc *desc = irq_to_desc(sgi);
 
@@ -375,7 +377,7 @@ void gic_interrupt(struct cpu_user_regs *regs, int is_fiq)
 /* Reading IRQ will ACK it */
 irq = gic_hw_ops->read_irq();
 
-if ( likely(irq >= 16 && irq < 1020) )
+if ( likely(irq >= GIC_SGI_STATIC_MAX && irq < 1020) )
 {
 isb();
 do_IRQ(regs, irq, is_fiq);
@@ -387,7 +389,7 @@ void gic_interrupt(struct cpu_user_regs *regs, int is_fiq)
 }
 else if ( unlikely(irq < 16) )
 {
-do_sgi(regs, irq);
+do_static_sgi(regs, irq);
 }
 else
 {
diff --git a/xen/arch/arm/include/asm/gic.h b/xen/arch/arm/include/asm/gic.h
index 03f209529b13..541f0eeb808a 100644
--- a/xen/arch/arm/include/asm/gic.h
+++ b/xen/arch/arm/include/asm/gic.h
@@ -285,7 +285,7 @@ enum gic_sgi {
 GIC_SGI_EVENT_CHECK,
 GIC_SGI_DUMP_STATE,
 GIC_SGI_CALL_FUNCTION,
-GIC_SGI_MAX,
+GIC_SGI_STATIC_MAX,
 };
 
 /* SGI irq mode types */
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index bcce80a4d624..d7306aa64d50 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -152,7 +152,13 @@ void __init init_IRQ(void)
 
 spin_lock(_irqs_type_lock);
 for ( irq = 0; irq < NR_LOCAL_IRQS; irq++ )
-local_irqs_type[irq] = IRQ_TYPE_INVALID;
+{
+/* SGIs are always edge-triggered */
+if ( irq < NR_GIC_SGI )
+local_irqs_type[irq] = DT_IRQ_TYPE_EDGE_RISING;
+else
+local_irqs_type[irq] = IRQ_TYPE_INVALID;
+}
 spin_unlock(_irqs_type_lock);
 
 BUG_ON(init_local_irq_data(smp_processor_id()) < 0);
@@ -224,9 +230,12 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, 
int is_fiq)
 
 perfc_incr(irqs);
 
-ASSERT(irq >= 16); /* SGIs do not come down this path */
+/* Statically assigned SGIs do not come down this path */
+ASSERT(irq >= GIC_SGI_STATIC_MAX);
 
-if ( irq < 32 )
+if ( irq < NR_GIC_SGI )
+perfc_incr(ipis);
+else if ( irq < NR_GIC_LOCAL_IRQS )
 perfc_incr(ppis);
 else
 perfc_incr(spis);
@@ -260,6 +269,7 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, 
int is_fiq)
  * The irq cannot be a PPI, we only support delivery of SPIs to
  * guests.
  */
+ASSERT(irq >= NR_GIC_SGI);
 vgic_inject_irq(info->d, NULL, info->virq, true);
 goto out_no_end;
 }
@@ -396,7 +406,7 @@ int setup_irq(unsigned int 

[XEN PATCH v3 2/5] xen/arm: ffa: use ACCESS_ONCE()

[Jens Wiklander]

Replace read_atomic() with ACCESS_ONCE() to match the intended use, that
is, to prevent the compiler from (via optimization) reading shared
memory more than once.

Signed-off-by: Jens Wiklander 
Reviewed-by: Bertrand Marquis 
---
 xen/arch/arm/tee/ffa_shm.c | 15 ---
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/tee/ffa_shm.c b/xen/arch/arm/tee/ffa_shm.c
index eed9ad2d2986..75a5b66aeb4c 100644
--- a/xen/arch/arm/tee/ffa_shm.c
+++ b/xen/arch/arm/tee/ffa_shm.c
@@ -7,6 +7,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -171,8 +172,8 @@ static int get_shm_pages(struct domain *d, struct 
ffa_shm_mem *shm,
 
 for ( n = 0; n < range_count; n++ )
 {
-page_count = read_atomic([n].page_count);
-addr = read_atomic([n].address);
+page_count = ACCESS_ONCE(range[n].page_count);
+addr = ACCESS_ONCE(range[n].address);
 for ( m = 0; m < page_count; m++ )
 {
 if ( pg_idx >= shm->page_count )
@@ -527,13 +528,13 @@ void ffa_handle_mem_share(struct cpu_user_regs *regs)
 goto out_unlock;
 
 mem_access = ctx->tx + trans.mem_access_offs;
-if ( read_atomic(_access->access_perm.perm) != FFA_MEM_ACC_RW )
+if ( ACCESS_ONCE(mem_access->access_perm.perm) != FFA_MEM_ACC_RW )
 {
 ret = FFA_RET_NOT_SUPPORTED;
 goto out_unlock;
 }
 
-region_offs = read_atomic(_access->region_offs);
+region_offs = ACCESS_ONCE(mem_access->region_offs);
 if ( sizeof(*region_descr) + region_offs > frag_len )
 {
 ret = FFA_RET_NOT_SUPPORTED;
@@ -541,8 +542,8 @@ void ffa_handle_mem_share(struct cpu_user_regs *regs)
 }
 
 region_descr = ctx->tx + region_offs;
-range_count = read_atomic(_descr->address_range_count);
-page_count = read_atomic(_descr->total_page_count);
+range_count = ACCESS_ONCE(region_descr->address_range_count);
+page_count = ACCESS_ONCE(region_descr->total_page_count);
 
 if ( !page_count )
 {
@@ -557,7 +558,7 @@ void ffa_handle_mem_share(struct cpu_user_regs *regs)
 goto out_unlock;
 }
 shm->sender_id = trans.sender_id;
-shm->ep_id = read_atomic(_access->access_perm.endpoint_id);
+shm->ep_id = ACCESS_ONCE(mem_access->access_perm.endpoint_id);
 
 /*
  * Check that the Composite memory region descriptor fits.
-- 
2.34.1

Re: [XEN PATCH] automation/eclair: reorganize pipelines

[Luca Fancellu]

> On 25 Apr 2024, at 14:32, Nicola Vetrini  wrote:
> 
> On 2024-04-25 11:40, Federico Serafini wrote:
>> On 25/04/24 02:06, Stefano Stabellini wrote:
>>> On Tue, 23 Apr 2024, Federico Serafini wrote:
 From: Simone Ballarin 
 Introduce accepted_guidelines.sh: a script to autogenerate the
 configuration file accepted.ecl from docs/misra/rules.rst which enables
 all accepted guidelines.
 Introduce monitored.ecl: a manual selection of accepted guidelines
 which are clean or almost clean, it is intended to be used for the
 analyses triggered by commits.
 Reorganize tagging.ecl:
   -Remove "accepted" tags: keeping track of accepted guidelines tagging
them as "accepted" in the configuration file tagging.ecl is no
longer needed since docs/rules.rst is keeping track of them.
   -Tag more guidelines as clean.
 Reorganize eclair pipelines:
   - Set1, Set2, Set3 are now obsolete: remove the corresponding
 pipelines and ecl files.
   - Amend scheduled eclair pipeline to use accepted.ecl.
   - Amend triggered eclair pipeline to use monitored.ecl.
 Rename and improve action_check_clean_regressions.sh to print a
 diagnostic in case a commit introduces a violation of a clean guideline.
 An example of diagnostic is the following:
 Failure: 13 regressions found for clean guidelines
   service MC3R1.R8.2: (required) Function types shall be in prototype form 
 with named parameters:
violation: 13
 Signed-off-by: Simone Ballarin 
 Signed-off-by: Federico Serafini 
 Signed-off-by: Alessandro Zucchelli 
>>> Fantastic work, thank you!
>>> Reviewed-by: Stefano Stabellini 
>>> Is this patch safe to commit now? Or would it cause gitlab-ci breakage?
>> Yes, it is safe because the ECLAIR analysis is still allowed to fail.
>> Committing this patch wouldn't break the CI but it will highlight some 
>> regressions with the orange badge and the following messages:
>> arm:
>> Failure: 5 regressions found for clean guidelines
>>  service MC3R1.R1.1: (required) The program shall contain no violations of 
>> the standard C syntax and constraints, and shall not exceed the 
>> implementation's translation limits:
>>   violation: 5
> 

Hi Nicola,

> +Cc ARM maintainers, Luca Fancellu
> 
> after some investigation on these regressions on R1.1, there are some 
> concerns with the current code:
> 2209c1e35b479dff8ed3d3161001ffdefa0a704e introduced the struct
> 
> struct membanks {
>unsigned int nr_banks;
>unsigned int max_banks;
>struct membank bank[];
> };
> 
> with a flexible array member at the end; this is well-defined in C99, but 
> what is non-standard (a GNU extension) is having this struct as a member to 
> another struct/union in a position other than the last.
> 
> This is still supported by GNU (albeit reliance on this is undocumented for 
> Xen), but what is concerning here is the following (taken from [1]):
> 
> "A structure containing a C99 flexible array member, or a union containing 
> such a structure, is not the last field of another structure, for example:
> 
> struct flex  { int length; char data[]; };
> 
> struct mid_flex { int m; struct flex flex_data; int n; };
> 
> In the above, accessing a member of the array mid_flex.flex_data.data[] might 
> have undefined behavior. Compilers do not handle such a case consistently. 
> Any code relying on this case should be modified to ensure that flexible 
> array members only end up at the ends of structures.
> Please use the warning option -Wflex-array-member-not-at-end to identify all 
> such cases in the source code and modify them. This extension is now 
> deprecated."
> 
> It looks like this option, and the corresponding deprecation of the 
> extension, will be available starting from GCC 14, so this might impact 
> future compiler updates for Xen builds.
> 
> Linux is also concerned about this (see [2]), so I think the changes in 
> struct layout introduced by the series [3] should be reviewed to determine 
> whether this change was deliberate or happened as a byproduct. If this is 
> determined not to be a legitimate concern, then this can be documented as an 
> use of the GNU extension.

Thanks for letting us know, so the change was deliberate, the effect probably 
not, I guess we need to find a way to fix this in order to use this interface.

Cheers,
Luca

Re: [PATCH] svm: Fix MISRA 8.2 violation

[George Dunlap]

On Thu, Apr 25, 2024 at 5:00 PM Andrew Cooper  wrote:
>
> On 25/04/2024 10:12 am, George Dunlap wrote:
> > Misra 8.2 requires named parameters in prototypes.  Use the name from
> > the implementaiton.
> >
> > Fixes 0d19d3aab0 ("svm/nestedsvm: Introduce nested capabilities bit").
>
> Nit.  We treat Fixes: as a tag.

Oops -- I'll fix that on check-in.  Gmail is also underlining my
misspelling of "implementation" in red, so I'll fix that as well.

> Also you might want to include the Reported-by's.

I thought about adding a Reported-by; but this was reported by Eclair,
which is run automatically, right?  We typically don't give a
"Reported-by" to the person who flagged up the osstest or gitlab
failure, so I figured this would be similar.

I'm happy to give credit where it's due, however.  You were the first
one who reported it to me, but Nicola also raised it; and I guess it's
someone else who's paying for the Eclair runs.  So what "Reported-by"
did you have in mind?

> Otherwise, Acked-by: Andrew Cooper  although
> I have a strong wish to shorten the parameter name.  That can be done at
> a later point.

Thanks,
 -George

Re: [PATCH v3] xen/riscv: check whether the assembler has Zbb extension support

[Oleksii]

On Mon, 2024-04-22 at 17:41 +0200, Oleksii wrote:
> On Mon, 2024-04-22 at 11:43 +0200, Jan Beulich wrote:
> > On 19.04.2024 16:23, Oleksii Kurochko wrote:
> > > Update the argument of the as-insn for the Zbb case to verify
> > > that
> > > Zbb is supported not only by a compiler, but also by an
> > > assembler.
> > > 
> > > Also, check-extenstion(ext_name, "insn") helper macro is
> > > introduced
> > > to check whether extension is supported by a compiler and an
> > > assembler.
> > > 
> > > Signed-off-by: Oleksii Kurochko 
> > 
> > Acked-by: Jan Beulich 
> > despite ...
> > 
> > > --- a/xen/arch/riscv/arch.mk
> > > +++ b/xen/arch/riscv/arch.mk
> > > @@ -11,9 +11,14 @@ riscv-march-$(CONFIG_RISCV_ISA_C)   :=
> > > $(riscv-march-y)c
> > >  
> > >  riscv-generic-flags := $(riscv-abi-y) -march=$(riscv-march-y)
> > >  
> > > -zbb := $(call as-insn,$(CC) $(riscv-generic-flags)_zbb,"",_zbb)
> > > -zihintpause := $(call as-insn, \
> > > -  $(CC) $(riscv-generic-
> > > flags)_zihintpause,"pause",_zihintpause)
> > > +# check-extension: Check whether extenstion is supported by a
> > > compiler and
> > > +#  an assembler.
> > > +# Usage: $(call check-extension,extension_name,"instr")
> > > +check-extension = $(call as-insn,$(CC) $(riscv-generic-
> > > flags)_$(1),$(2),_$(1))
> > > +
> > > +zbb-insn := "andn t0, t0, t0"
> > > +zbb := $(call check-extension,zbb,$(zbb-insn))
> > > +zihintpause := $(call check-extension,zihintpause,"pause")
> > 
> > ... still not really being happy with this: Either, as said before,
> > zbb-insn
> > would better be avoided (by using $(comma) as necessary), or you
> > should have
> > gone yet a step further to fully address my "redundancy" concern.
> > Note how
> > the two ultimate lines still have 3 (zbb) and 2 (zihintpause)
> > references
> > respectively, when the goal ought to be to have exactly one. E.g.
> > along the
> > lines of
> > 
> > $(call check-extension,zbb)
> > $(call check-extension,zihintpause)
> > 
> > suitably using $(eval ...) (to effect the variable definitions) and
> > defining
> > both zbb-insn and zihintpause-insn.
> > 
> > But I'll nevertheless put this in as is, unless you tell me you're
> > up
> > to
> > going the extra step.
> I am okay with all your suggestions. So the final version will look
> like ( if I understood you correctly ):
Jan,

Could you please review the changes below? I just want to ensure that
you are okay with them. If you are, I'll add your Acked-by that you
gave to this patch in previous answers.

Thanks in advance.

~ Oleksii
> diff --git a/xen/arch/riscv/arch.mk b/xen/arch/riscv/arch.mk
> index dd242c91d1..f172187144 100644
> --- a/xen/arch/riscv/arch.mk
> +++ b/xen/arch/riscv/arch.mk
> @@ -13,12 +13,19 @@ riscv-generic-flags := $(riscv-abi-y) -
> march=$(riscv-march-y)
>  
>  # check-extension: Check whether extenstion is supported by a
> compiler
> and
>  #  an assembler.
> -# Usage: $(call check-extension,extension_name,"instr")
> -check-extension = $(call as-insn,$(CC) $(riscv-generic-
> flags)_$(1),$(2),_$(1))
> +# Usage: $(call check-extension,extension_name).
> +#    it should be defined variable with name: extension-name :=
> "insn"
>  
> -zbb-insn := "andn t0, t0, t0"
> -zbb := $(call check-extension,zbb,$(zbb-insn))
> -zihintpause := $(call check-extension,zihintpause,"pause")
> +define check-extension =
> +$(eval $(1) := \
> +   $(call as-insn,$(CC) $(riscv-generic-flags)_$(1),$(value
> $(1)-
> insn),_$(1)))
> +endef
> +
> +zbb-insn := "andn t0$(comma)t0$(comma)t0"
> +$(call check-extension,zbb)
> +
> +zihintpause-insn := "pause"
> +$(call check-extension,zihintpause)
>  
>  extensions := $(zbb) $(zihintpause)
> 
> If the diff above looks good, I'll sent a new patch version.
> 
> Thanks.
> 
> ~ Oleksii

Re: [PATCH v8 03/17] xen/bitops: implement fls{l}() in common logic

[Oleksii]

On Thu, 2024-04-25 at 17:44 +0200, Jan Beulich wrote:
> On 17.04.2024 12:04, Oleksii Kurochko wrote:
> > Return type was left 'int' because of the following compilation
> > error:
> > 
> > ./include/xen/kernel.h:18:21: error: comparison of distinct pointer
> > types lacks a cast [-Werror]
> >    18 | (void) (&_x == &_y);    \
> >   | ^~
> >     common/page_alloc.c:1843:34: note: in expansion of macro 'min'
> >  1843 | unsigned int inc_order = min(MAX_ORDER, flsl(e
> > - s) - 1);
> 
> Apart from this I'm okay with this patch, assuming Andrew's won't
> change in
> any conflicting way. As to the above - no, I don't see us having
> ffs() / ffsl()
> returning unsigned int, fls() / flsl() returning plain int. Even more
> so that,
> given the LHS variable's type, an unsigned quantity is really meant
> in the
> quoted code.
If I understand you correctly, it's acceptable for fls() / flsl() to
return 'int'. Therefore, I can update the commit message by removing
the part mentioning the compilation error, as it's expected for fls() /
flsl() to return 'int'. Is my understanding correct?

~ Oleksii

Re: [PATCH v8 02/17] xen: introduce generic non-atomic test_*bit()

[Oleksii]

On Thu, 2024-04-25 at 17:35 +0200, Jan Beulich wrote:
> >   /* - Please tidy above here -
> >  */
> >   
> >   #include 
> >   
> > +#ifndef arch_check_bitop_size
> > +#define arch_check_bitop_size(addr)
> 
> Can this really do nothing? Passing the address of an object smaller
> than
> bitop_uint_t will read past the object in the generic__*_bit()
> functions.
Agree, in generic case it would be better to add:
#define arch_check_bitop_size(addr) (sizeof(*(addr)) <
sizeof(bitop_uint_t))

Originally, it was defined as empty becuase majority of supported
architectures by Xen don't do this check and I decided to use this
definition as generic.

Thanks.

~ Oleksii

[PATCH] xen-livepatch: fix --force option comparison

[Roger Pau Monne]

The check for --force option shouldn't be against 0.

Reported-by: Jan Beulich 
Fixes: 62a72092a517 ('livepatch: introduce --force option')
Signed-off-by: Roger Pau Monné 
---
 tools/misc/xen-livepatch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/misc/xen-livepatch.c b/tools/misc/xen-livepatch.c
index c16fb6862d6c..f406ea1373ae 100644
--- a/tools/misc/xen-livepatch.c
+++ b/tools/misc/xen-livepatch.c
@@ -576,7 +576,7 @@ int main(int argc, char *argv[])
 return 0;
 }
 
-if ( strcmp("--force", argv[1]) )
+if ( !strcmp("--force", argv[1]) )
 {
 if ( argc <= 2 )
 {
-- 
2.44.0

Re: [PATCH v4 2/4] livepatch: introduce --force option

[Roger Pau Monné]

On Fri, Apr 26, 2024 at 08:41:48AM +0200, Jan Beulich wrote:
> On 24.04.2024 10:19, Roger Pau Monne wrote:
> > @@ -571,6 +575,19 @@ int main(int argc, char *argv[])
> >  show_help();
> >  return 0;
> >  }
> > +
> > +if ( strcmp("--force", argv[1]) )
> 
> I guess this missing ! or "== 0" is the reason for osstest reporting a
> livepatch-run failure.

Bah, seems like I dropped it when changing from strncmp to strcmp, as
it's present in v3.

Will send a fix ASAP.

Thanks, Roger.

Re: [PATCH 2/3] xen/arm, tools: Add a new HVM_PARAM_MAGIC_BASE_PFN key in HVMOP

[Henry Wang]

Hi Jan,

On 4/26/2024 2:50 PM, Jan Beulich wrote:

On 26.04.2024 08:30, Henry Wang wrote:

On 4/26/2024 2:21 PM, Jan Beulich wrote:

On 26.04.2024 05:14, Henry Wang wrote:

--- a/xen/include/public/hvm/params.h
+++ b/xen/include/public/hvm/params.h
@@ -76,6 +76,7 @@
*/
   #define HVM_PARAM_STORE_PFN1
   #define HVM_PARAM_STORE_EVTCHN 2
+#define HVM_PARAM_MAGIC_BASE_PFN3
   
   #define HVM_PARAM_IOREQ_PFN5

Considering all adjacent values are used, it is overwhelmingly likely that
3 was once used, too. Such re-use needs to be done carefully. Since you
need this for Arm only, that's likely okay, but doesn't go without (a)
saying and (b) considering the possible future case of dom0less becoming
arch-agnostic, or hyperlaunch wanting to extend the scope. Plus (c) imo
this also needs at least a comment, maybe even an #ifdef, seeing how x86-
focused most of the rest of this header is.

Thanks for the feedback. These make sense. I think probably
dom0less/hyperlaunch will have similar use cases so the number 3 can be
reused at that time. Therefore, in v2, I will add more description in
commit message, a comment on top of this macro and protect it with
#ifdef. Hope this will address your concern. Thanks.

FTAOD: If you foresee re-use by hyperlaunch, re-using a previously used
number may need re-considering. Which isn't to say that number re-use is
excluded here, but it would need at least figuring out (and then stating)
what exactly the number was used for and until when.


I just did a bit search and noticed that the number 3 was used to be
#define HVM_PARAM_APIC_ENABLED 3

and it was removed 18 years ago in commit: 
6bc01e4efd50e1986a9391f75980d45691f42b74


So I think we are likely to be ok if reuse 3 on Arm with proper #ifdef.

Kind regards,
Henry


Jan

Re: [PATCH 2/3] xen/arm, tools: Add a new HVM_PARAM_MAGIC_BASE_PFN key in HVMOP

[Jan Beulich]

On 26.04.2024 08:30, Henry Wang wrote:
> On 4/26/2024 2:21 PM, Jan Beulich wrote:
>> On 26.04.2024 05:14, Henry Wang wrote:
>>> --- a/xen/include/public/hvm/params.h
>>> +++ b/xen/include/public/hvm/params.h
>>> @@ -76,6 +76,7 @@
>>>*/
>>>   #define HVM_PARAM_STORE_PFN1
>>>   #define HVM_PARAM_STORE_EVTCHN 2
>>> +#define HVM_PARAM_MAGIC_BASE_PFN3
>>>   
>>>   #define HVM_PARAM_IOREQ_PFN5
>> Considering all adjacent values are used, it is overwhelmingly likely that
>> 3 was once used, too. Such re-use needs to be done carefully. Since you
>> need this for Arm only, that's likely okay, but doesn't go without (a)
>> saying and (b) considering the possible future case of dom0less becoming
>> arch-agnostic, or hyperlaunch wanting to extend the scope. Plus (c) imo
>> this also needs at least a comment, maybe even an #ifdef, seeing how x86-
>> focused most of the rest of this header is.
> 
> Thanks for the feedback. These make sense. I think probably 
> dom0less/hyperlaunch will have similar use cases so the number 3 can be 
> reused at that time. Therefore, in v2, I will add more description in 
> commit message, a comment on top of this macro and protect it with 
> #ifdef. Hope this will address your concern. Thanks.

FTAOD: If you foresee re-use by hyperlaunch, re-using a previously used
number may need re-considering. Which isn't to say that number re-use is
excluded here, but it would need at least figuring out (and then stating)
what exactly the number was used for and until when.

Jan

Re: [PATCH v4 2/4] livepatch: introduce --force option

[Jan Beulich]

On 24.04.2024 10:19, Roger Pau Monne wrote:
> @@ -571,6 +575,19 @@ int main(int argc, char *argv[])
>  show_help();
>  return 0;
>  }
> +
> +if ( strcmp("--force", argv[1]) )

I guess this missing ! or "== 0" is the reason for osstest reporting a
livepatch-run failure.

Jan

> +{
> +if ( argc <= 2 )
> +{
> +show_help();
> +return EXIT_FAILURE;
> +}
> +force = true;
> +argv++;
> +argc--;
> +}
> +
>  for ( i = 0; i < ARRAY_SIZE(main_options); i++ )
>  if (!strcmp(main_options[i].name, argv[1]))
>  break;

Re: [PATCH] CI: Drop glibc-i386 from the build containers

[Jan Beulich]

On 25.04.2024 19:47, Andrew Cooper wrote:
> Xen 4.14 no longer runs in Gitlab CI.  Drop the dependency to shrink the build
> containers a little.
> 
> Signed-off-by: Andrew Cooper 

This is probably okay seeing that 4.15 shouldn't need to be tested anymore
either, for having gone out of support. Otherwise I would have asked whether
this isn't premature, as at some point there will want to be N-1 ==> N
migration tests, like osstest has them. Extending the description a little
to this effect may be desirable.

Jan

[xen-unstable test] 185800: regressions - FAIL

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-amd64-livepatch   13 livepatch-runfail REGR. vs. 185794

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-libvirt 16 saverestore-support-checkfail  like 185794
 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 185794
 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 185794
 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 185794
 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 185794
 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 185794
 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  16 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-qcow2 14 migrate-support-checkfail never pass
 test-armhf-armhf-xl-credit2  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail  never pass
 test-armhf-armhf-libvirt 15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  16 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-raw 14 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  16 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-vhd  14 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-vhd  15 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-qcow214 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-qcow215 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt-vhd 14 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-vhd 15 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-raw  14 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-raw  15 saverestore-support-checkfail   never pass

version targeted for testing:
 xen  6d5111b10e084d841284a56e962c61ad274f589e
baseline version:
 xen  31d6b5b4a7c84818294dacca7a07e92f52393c9d

Last test of basis   185794  2024-04-25 05:57:18 Z1 days
Testing same since   185800  2024-04-25 20:10:40 Z0 days1 attempts


People who touched revisions under test:
  Alessandro Zucchelli 
  Andrew Cooper 
  Anthony PERARD 
  Christian Lindig 
  Edwin Török 
  Jan Beulich 
  Jason Andryuk 
  Jason Andryuk 
  Nicola Vetrini 
  Petr Beneš 
  Roger Pau Monné 

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

Re: [PATCH] xen/spinlock: use correct pointer

[Jan Beulich]

On 25.04.2024 22:45, Stewart Hildebrand wrote:
> The ->profile member is at different offsets in struct rspinlock and
> struct spinlock. When initializing the profiling bits of an rspinlock,
> an unrelated member in struct rspinlock was being overwritten, leading
> to mild havoc. Use the correct pointer.
> 
> Fixes: b053075d1a7b ("xen/spinlock: make struct lock_profile rspinlock_t 
> aware")
> Signed-off-by: Stewart Hildebrand 

Reviewed-by: Jan Beulich 

> --- a/xen/common/spinlock.c
> +++ b/xen/common/spinlock.c
> @@ -789,7 +789,11 @@ static int __init cf_check lock_prof_init(void)
>  {
>  (*q)->next = lock_profile_glb_q.elem_q;
>  lock_profile_glb_q.elem_q = *q;
> -(*q)->ptr.lock->profile = *q;
> +
> +if ( (*q)->is_rlock )
> +(*q)->ptr.rlock->profile = *q;
> +else
> +(*q)->ptr.lock->profile = *q;
>  }
>  
>  _lock_profile_register_struct(LOCKPROF_TYPE_GLOBAL,

Just to mention it: Strictly speaking spinlock_profile_print_elem()'s

printk("%s: addr=%p, lockval=%08x, ", data->name, data->ptr.lock, lockval);

isn't quite right either (and I would be surprised if Misra didn't have
to say something about it).

Jan

Re: [PATCH 2/3] xen/arm, tools: Add a new HVM_PARAM_MAGIC_BASE_PFN key in HVMOP

[Henry Wang]

Hi Jan,

On 4/26/2024 2:21 PM, Jan Beulich wrote:

On 26.04.2024 05:14, Henry Wang wrote:

--- a/xen/include/public/hvm/params.h
+++ b/xen/include/public/hvm/params.h
@@ -76,6 +76,7 @@
   */
  #define HVM_PARAM_STORE_PFN1
  #define HVM_PARAM_STORE_EVTCHN 2
+#define HVM_PARAM_MAGIC_BASE_PFN3
  
  #define HVM_PARAM_IOREQ_PFN5

Considering all adjacent values are used, it is overwhelmingly likely that
3 was once used, too. Such re-use needs to be done carefully. Since you
need this for Arm only, that's likely okay, but doesn't go without (a)
saying and (b) considering the possible future case of dom0less becoming
arch-agnostic, or hyperlaunch wanting to extend the scope. Plus (c) imo
this also needs at least a comment, maybe even an #ifdef, seeing how x86-
focused most of the rest of this header is.


Thanks for the feedback. These make sense. I think probably 
dom0less/hyperlaunch will have similar use cases so the number 3 can be 
reused at that time. Therefore, in v2, I will add more description in 
commit message, a comment on top of this macro and protect it with 
#ifdef. Hope this will address your concern. Thanks.


Kind regards,
Henry


Jan

Re: [PATCH v2] docs/misra: add R21.6 R21.9 R21.10 R21.14 R21.15 R21.16

[Jan Beulich]

On 26.04.2024 01:31, Stefano Stabellini wrote:
> --- a/docs/misra/rules.rst
> +++ b/docs/misra/rules.rst
> @@ -652,12 +652,72 @@ maintainers if you want to suggest a change.
> declared
>   - See comment for Rule 21.1
>  
> +   * - `Rule 21.6 
> `_
> + - Required
> + - The Standard Library input/output routines shall not be used
> + - Xen doesn't provide, use, or link against any Standard Library.
> +   Xen implements itself a few functions with names that match the
> +   corresponding function names of the Standard Library for
> +   developers' convenience. These functions are part of the Xen code
> +   and subject to analysis.
> +
> +   * - `Rule 21.9 
> `_
> + - Required
> + - The library functions bsearch and qsort of  shall not be 
> used
> + - Xen doesn't provide, use, or link against any Standard Library.
> +   Xen implements itself a few functions with names that match the
> +   corresponding function names of the Standard Library for
> +   developers' convenience. These functions are part of the Xen code
> +   and subject to analysis.
> +
> +   * - `Rule 21.10 
> `_
> + - Required
> + - The Standard Library time and date routines shall not be used
> + - Xen doesn't provide, use, or link against any Standard Library.
> +   Xen implements itself a few functions with names that match the
> +   corresponding function names of the Standard Library for
> +   developers' convenience. These functions are part of the Xen code
> +   and subject to analysis.
> +
> * - `Rule 21.13 
> `_
>   - Mandatory
>   - Any value passed to a function in  shall be representable as 
> an
> unsigned char or be the value EOF
>   -

Up to here, did you consider adding a short reference to some common blob
(footnote or alike), rather than repeating the same text verbatim several
times?

> +   * - `Rule 21.14 
> `_
> + - Required
> + - The Standard Library function memcmp shall not be used to compare
> +   null terminated strings
> + - Xen doesn't provide, use, or link against any Standard Library.
> +   Xen implements itself a few functions with names that match the
> +   corresponding function names of the Standard Library for
> +   developers' convenience. These functions are part of the Xen code
> +   and subject to analysis.
> +
> +   * - `Rule 21.15 
> `_
> + - Required
> + - The pointer arguments to the Standard Library functions memcpy,
> +   memmove and memcmp shall be pointers to qualified or unqualified
> +   versions of compatible types
> + - Xen doesn't provide, use, or link against any Standard Library.
> +   Xen implements itself a few functions with names that match the
> +   corresponding function names of the Standard Library for
> +   developers' convenience. These functions are part of the Xen code
> +   and subject to analysis.
> +
> +   * - `Rule 21.16 
> `_
> + - Required
> + - The pointer arguments to the Standard Library function memcmp
> +   shall point to either a pointer type, an essentially signed type,
> +   an essentially unsigned type, an essentially Boolean type or an
> +   essentially enum type
> + - void* arguments are allowed. Xen doesn't provide, use, or link
> +   against any Standard Library.  Xen implements itself a few
> +   functions with names that match the corresponding function names
> +   of the Standard Library for developers' convenience. These
> +   functions are part of the Xen code and subject to analysis.

For all three of these I'm not convinced the remark is appropriate. These
talk about specific properties of the functions, which aren't related to
risks associated with particular (and hence potentially varying) library
implementations.

Jan

Re: [PATCH 2/3] xen/arm, tools: Add a new HVM_PARAM_MAGIC_BASE_PFN key in HVMOP

[Jan Beulich]

On 26.04.2024 05:14, Henry Wang wrote:
> --- a/xen/include/public/hvm/params.h
> +++ b/xen/include/public/hvm/params.h
> @@ -76,6 +76,7 @@
>   */
>  #define HVM_PARAM_STORE_PFN1
>  #define HVM_PARAM_STORE_EVTCHN 2
> +#define HVM_PARAM_MAGIC_BASE_PFN3
>  
>  #define HVM_PARAM_IOREQ_PFN5

Considering all adjacent values are used, it is overwhelmingly likely that
3 was once used, too. Such re-use needs to be done carefully. Since you
need this for Arm only, that's likely okay, but doesn't go without (a)
saying and (b) considering the possible future case of dom0less becoming
arch-agnostic, or hyperlaunch wanting to extend the scope. Plus (c) imo
this also needs at least a comment, maybe even an #ifdef, seeing how x86-
focused most of the rest of this header is.

Jan