Re: [PATCH v3 01/10] xen/version: Calculate xen_capabilities_info once at boot

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 15 Aug 2023, Andrew Cooper wrote:
> The arch_get_xen_caps() infrastructure is horribly inefficient for something
> that is constant after features have been resolved on boot.
> 
> Every instance used snprintf() to format constants into a string (which gets
> shorter when %d gets resolved!), and which get double buffered on the stack.
> 
> Switch to using string literals with the "3.0" inserted - these numbers
> haven't changed in 18 years (The Xen 3.0 release was Dec 5th 2005).
> 
> Use initcalls to format the data into xen_cap_info, which is deliberately not
> of type xen_capabilities_info_t because a 1k array is a silly overhead for
> storing a maximum of 77 chars (the x86 version) and isn't liable to need any
> more space in the forseeable future.
> 
> This speeds up the the XENVER_capabilities hypercall, but the purpose of the
> change is to allow us to introduce a better XENVER_* API that doesn't force
> the use of a 1k buffer on the stack.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>


> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: Julien Grall <julien@xxxxxxx>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> CC: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> CC: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> CC: Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>
> CC: Henry Wang <Henry.Wang@xxxxxxx>
> 
> v3:
>  * CC new-arch folks so they're aware
> v2:
>  * New
> 
> Moving to __initcall() also allows new architectures to not implement this
> API, and I'm going to recommend strongly that they dont.  Its a very dubious
> way of signalling about 3 bits of info to the toolstack, and inefficient to
> use (the toolstack has to do string parsing on the result figure out if
> PV64/PV32/HVM is available).
> ---
>  xen/arch/arm/setup.c        | 20 +++++++-------------
>  xen/arch/x86/setup.c        | 31 ++++++++++---------------------
>  xen/common/kernel.c         |  3 ++-
>  xen/include/xen/hypercall.h |  2 --
>  xen/include/xen/version.h   |  2 ++
>  5 files changed, 21 insertions(+), 37 deletions(-)
> 
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 44ccea03ca14..3592c9ebc1cd 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -27,6 +27,7 @@
>  #include <xen/cpu.h>
>  #include <xen/pfn.h>
>  #include <xen/virtual_region.h>
> +#include <xen/version.h>
>  #include <xen/vmap.h>
>  #include <xen/trace.h>
>  #include <xen/libfdt/libfdt-xen.h>
> @@ -1286,24 +1287,17 @@ void __init start_xen(unsigned long boot_phys_offset,
>      switch_stack_and_jump(idle_vcpu[0]->arch.cpu_info, init_done);
>  }
>  
> -void arch_get_xen_caps(xen_capabilities_info_t *info)
> +static int __init init_xen_cap_info(void)
>  {
> -    /* Interface name is always xen-3.0-* for Xen-3.x. */
> -    int major = 3, minor = 0;
> -    char s[32];
> -
> -    (*info)[0] = '\0';
> -
>  #ifdef CONFIG_ARM_64
> -    snprintf(s, sizeof(s), "xen-%d.%d-aarch64 ", major, minor);
> -    safe_strcat(*info, s);
> +    safe_strcat(xen_cap_info, "xen-3.0-aarch64 ");
>  #endif
>      if ( cpu_has_aarch32 )
> -    {
> -        snprintf(s, sizeof(s), "xen-%d.%d-armv7l ", major, minor);
> -        safe_strcat(*info, s);
> -    }
> +        safe_strcat(xen_cap_info, "xen-3.0-armv7l ");
> +
> +    return 0;
>  }
> +__initcall(init_xen_cap_info);
>  
>  /*
>   * Local variables:
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 2bfc1fd00f8c..206f0c50a136 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -2049,35 +2049,24 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>      unreachable();
>  }
>  
> -void arch_get_xen_caps(xen_capabilities_info_t *info)
> +static int __init cf_check init_xen_cap_info(void)
>  {
> -    /* Interface name is always xen-3.0-* for Xen-3.x. */
> -    int major = 3, minor = 0;
> -    char s[32];
> -
> -    (*info)[0] = '\0';
> -
>      if ( IS_ENABLED(CONFIG_PV) )
>      {
> -        snprintf(s, sizeof(s), "xen-%d.%d-x86_64 ", major, minor);
> -        safe_strcat(*info, s);
> +        safe_strcat(xen_cap_info, "xen-3.0-x86_64 ");
>  
>          if ( opt_pv32 )
> -        {
> -            snprintf(s, sizeof(s), "xen-%d.%d-x86_32p ", major, minor);
> -            safe_strcat(*info, s);
> -        }
> +            safe_strcat(xen_cap_info, "xen-3.0-x86_32p ");
>      }
>      if ( hvm_enabled )
> -    {
> -        snprintf(s, sizeof(s), "hvm-%d.%d-x86_32 ", major, minor);
> -        safe_strcat(*info, s);
> -        snprintf(s, sizeof(s), "hvm-%d.%d-x86_32p ", major, minor);
> -        safe_strcat(*info, s);
> -        snprintf(s, sizeof(s), "hvm-%d.%d-x86_64 ", major, minor);
> -        safe_strcat(*info, s);
> -    }
> +        safe_strcat(xen_cap_info,
> +                    "hvm-3.0-x86_32 "
> +                    "hvm-3.0-x86_32p "
> +                    "hvm-3.0-x86_64 ");
> +
> +    return 0;
>  }
> +__initcall(init_xen_cap_info);
>  
>  int __hwdom_init xen_in_range(unsigned long mfn)
>  {
> diff --git a/xen/common/kernel.c b/xen/common/kernel.c
> index 52aa28762782..f822480a8ef3 100644
> --- a/xen/common/kernel.c
> +++ b/xen/common/kernel.c
> @@ -30,6 +30,7 @@ enum system_state system_state = SYS_STATE_early_boot;
>  
>  static xen_commandline_t saved_cmdline;
>  static const char __initconst opt_builtin_cmdline[] = CONFIG_CMDLINE;
> +char __ro_after_init xen_cap_info[128];
>  
>  static int assign_integer_param(const struct kernel_param *param, uint64_t 
> val)
>  {
> @@ -537,7 +538,7 @@ long do_xen_version(int cmd, XEN_GUEST_HANDLE_PARAM(void) 
> arg)
>  
>          memset(info, 0, sizeof(info));
>          if ( !deny )
> -            arch_get_xen_caps(&info);
> +            safe_strcpy(info, xen_cap_info);
>  
>          if ( copy_to_guest(arg, info, ARRAY_SIZE(info)) )
>              return -EFAULT;
> diff --git a/xen/include/xen/hypercall.h b/xen/include/xen/hypercall.h
> index f307dfb59760..15b6be6ec818 100644
> --- a/xen/include/xen/hypercall.h
> +++ b/xen/include/xen/hypercall.h
> @@ -56,6 +56,4 @@ common_vcpu_op(int cmd,
>      struct vcpu *v,
>      XEN_GUEST_HANDLE_PARAM(void) arg);
>  
> -void arch_get_xen_caps(xen_capabilities_info_t *info);
> -
>  #endif /* __XEN_HYPERCALL_H__ */
> diff --git a/xen/include/xen/version.h b/xen/include/xen/version.h
> index 93c58773630c..4856ad1b446d 100644
> --- a/xen/include/xen/version.h
> +++ b/xen/include/xen/version.h
> @@ -19,6 +19,8 @@ const char *xen_deny(void);
>  const char *xen_build_info(void);
>  int xen_build_id(const void **p, unsigned int *len);
>  
> +extern char xen_cap_info[128];
> +
>  #ifdef BUILD_ID
>  void xen_build_init(void);
>  int xen_build_id_check(const Elf_Note *n, unsigned int n_sz,
> -- 
> 2.30.2
>