Re: [PATCH v4 08/25] xen/riscv: introduce guest riscv,isa string

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.06.2026 17:46, Oleksii Kurochko wrote:
> Changes in v4:
>  - Add an explicit overflow guard in build_guest_isa_str(): return
>    -ENOSPC when buf is non-NULL and total >= size, to avoid the
>    size - total underflow being passed to snprintf().

How does ...

> @@ -480,6 +489,81 @@ bool riscv_isa_extension_available(const unsigned long 
> *isa_bitmap,
>      return test_bit(id, isa_bitmap);
>  }
>  
> +static int build_guest_isa_str(char *buf, size_t size,
> +                               const unsigned long *isa_bitmap)
> +{
> +    int total;
> +
> +#if defined(CONFIG_RISCV_32)
> +    total = snprintf(buf, size, "rv32");
> +#elif defined(CONFIG_RISCV_64)
> +    total = snprintf(buf, size, "rv64");
> +#else
> +#   error "Unsupported RISC-V bitness"
> +#endif
> +
> +    if ( total < 0 )
> +        return total;
> +
> +    if ( buf && ((size_t)total >= size) )
> +        return -ENOSPC;

... this help an underflow ...

> +    for ( unsigned int i = 0; i < ARRAY_SIZE(riscv_isa_ext); i++ )
> +    {
> +        const struct riscv_isa_ext_data *ext = &riscv_isa_ext[i];
> +        int ret;
> +
> +        if ( !riscv_isa_extension_available(isa_bitmap, ext->id) )
> +            continue;
> +
> +        ret = snprintf(buf ? buf + total : NULL,
> +                       buf ? size - total : 0, "%s%s",

... on any but the first iteration here?

> +static void __init init_guest_unsupp(void)
> +{
> +    __set_bit(RISCV_ISA_EXT_f, guest_unsupp);
> +    __set_bit(RISCV_ISA_EXT_d, guest_unsupp);
> +    __set_bit(RISCV_ISA_EXT_q, guest_unsupp);
> +    __set_bit(RISCV_ISA_EXT_v, guest_unsupp);
> +    __set_bit(RISCV_ISA_EXT_h, guest_unsupp);
> +    __set_bit(RISCV_ISA_EXT_sstc, guest_unsupp);
> +    __set_bit(RISCV_ISA_EXT_svade, guest_unsupp);
> +    __set_bit(RISCV_ISA_EXT_svpbmt, guest_unsupp);
> +}

Wouldn't riscv_isa_ext[] better get a prominent reminder that additions there
may need mirroring here (unless guest support is implemented at the same time)?
(As before, yet better would of course be to make sure this is consistent
right from build time, i.e. without the need to have this separate function.
Or minimally have the info right in that array, so that while adding one needs
to think how to set that separate field.)

> --- a/xen/arch/riscv/include/asm/cpufeature.h
> +++ b/xen/arch/riscv/include/asm/cpufeature.h
> @@ -17,6 +17,7 @@
>   */
>  #define RISCV_ISA_EXT_BASE  26
>  
> +
>  enum riscv_isa_ext_id {
>      RISCV_ISA_EXT_a,
>      RISCV_ISA_EXT_c,

???

> @@ -94,6 +95,9 @@ struct arch_domain {
>      struct p2m_domain p2m;
>  
>      struct paging_domain paging;
> +
> +    DECLARE_BITMAP(isa, RISCV_ISA_EXT_MAX);
> +    char *isa_str;
>  };

Why is it again that both the bitmap and its string representation need
storing? In the end they provide two different sources of truth, as there's
no guarantee that they'll remain in sync.

Jan