Re: [PATCH 2/2] xen/version: Remove xen_build_id() and export the variable instead

[Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>]

On Thu, Jul 31, 2025 at 12:02 PM Andrew Cooper
<andrew.cooper3@xxxxxxxxxx> wrote:
>
> The API is unergonomic to use, and leads to poor code generation because of
> unnecessary forcing of data to the stack.
>
> Rename build_id_p to xen_build_id, and build_id_len to xen_build_id_len, make
> them __ro_after_init, and export the variables directly.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> CC: Michal Orzel <michal.orzel@xxxxxxx>
> CC: Jan Beulich <jbeulich@xxxxxxxx>
> CC: Julien Grall <julien@xxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
>
> Both the code diffstat, and the binary on x86 speaks for themselves:
>
>   add/remove: 1/2 grow/shrink: 0/6 up/down: 4/-348 (-344)
>   Function                                     old     new   delta
>   xen_build_id_len                               -       4      +4
>   build_id_len                                   4       -      -4
>   build_id_p                                     8       -      -8
>   xen_build_id                                  42       8     -34
>   livepatch_printall                           470     432     -38
>   build_id_dep                                 152     113     -39
>   livepatch_op                                7930    7866     -64
>   do_xen_version                              2436    2363     -73
>   livepatch_op.cold                           1420    1332     -88
> ---
>  xen/common/kernel.c       | 14 ++++----------
>  xen/common/livepatch.c    | 23 ++++++++++-------------
>  xen/common/version.c      | 25 +++++++------------------
>  xen/include/xen/version.h |  4 +++-
>  4 files changed, 24 insertions(+), 42 deletions(-)
>
> diff --git a/xen/common/kernel.c b/xen/common/kernel.c
> index 5be668ba855a..e6979352e100 100644
> --- a/xen/common/kernel.c
> +++ b/xen/common/kernel.c
> @@ -510,21 +510,15 @@ static long xenver_varbuf_op(int cmd, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>      struct xen_varbuf user_str;
>      const char *str = NULL;
>      size_t sz;
> -    int rc;
>
>      switch ( cmd )
>      {
>      case XENVER_build_id:
> -    {
> -        unsigned int local_sz;
> -
> -        rc = xen_build_id((const void **)&str, &local_sz);
> -        if ( rc )
> -            return rc;
> -
> -        sz = local_sz;
> +        str = xen_build_id;
> +        sz  = xen_build_id_len;
> +        if ( !sz )
> +            return -ENODATA;
>          goto have_len;
> -    }
>
>      case XENVER_extraversion2:
>          str = xen_extra_version();
> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> index 9a0df5363b59..9285f88644f4 100644
> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -475,8 +475,8 @@ static int parse_buildid(const struct livepatch_elf_sec 
> *sec,
>
>  static int check_xen_buildid(const struct livepatch_elf *elf)
>  {
> -    const void *id;
> -    unsigned int len;
> +    const void *id = xen_build_id;
> +    unsigned int len = xen_build_id_len;

Is it worth making this const? Or you could just remove these variables
entirely and use xen_build_id / xen_build_id_len directly in the rest of
the function.

In any case,

Reviewed-by: Ross Lagerwall <Ross.lagerwall@xxxxxxxxxx>