Re: [PATCH v7 04/20] xen/riscv: introduce guest riscv,isa string

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

On 8/13/26 9:19 AM, Jan Beulich wrote:
> On 04.08.2026 17:47, Oleksii Kurochko wrote:
>> @@ -120,29 +148,30 @@ static int __init dt_get_cpuid_from_node(const struct dt_device_node *cpu,
>>    * and strncmp() is used in match_isa_ext() to compare extension names instead
>>    * of strncasecmp().
>>    */
>> -const struct riscv_isa_ext_data __initconst riscv_isa_ext[] = {
>> -    RISCV_ISA_EXT_DATA(i),
>> -    RISCV_ISA_EXT_DATA(m),
>> -    RISCV_ISA_EXT_DATA(a),
>> -    RISCV_ISA_EXT_DATA(f),
>> -    RISCV_ISA_EXT_DATA(d),
>> -    RISCV_ISA_EXT_DATA(q),
>> -    RISCV_ISA_EXT_DATA(c),
>> -    RISCV_ISA_EXT_DATA(h),
>> -    RISCV_ISA_EXT_DATA(zicntr),
>> -    RISCV_ISA_EXT_DATA(zicsr),
>> -    RISCV_ISA_EXT_DATA(zifencei),
>> -    RISCV_ISA_EXT_DATA(zihintpause),
>> -    RISCV_ISA_EXT_DATA(zihpm),
>> -    RISCV_ISA_EXT_DATA(zba),
>> -    RISCV_ISA_EXT_DATA(zbb),
>> -    RISCV_ISA_EXT_DATA(zbs),
>> -    RISCV_ISA_EXT_DATA(smaia),
>> -    RISCV_ISA_EXT_DATA(smstateen),
>> -    RISCV_ISA_EXT_DATA(ssaia),
>> -    RISCV_ISA_EXT_DATA(sstc),
>> -    RISCV_ISA_EXT_DATA(svade),
>> -    RISCV_ISA_EXT_DATA(svpbmt),
>> +static const struct riscv_isa_ext_entry __initconstrel riscv_isa_ext[] = {
>> +    RISCV_ISA_EXT_ENTRY(i,            true),
>> +    RISCV_ISA_EXT_ENTRY(m,            true),
>> +    RISCV_ISA_EXT_ENTRY(a,            true),
>> +    RISCV_ISA_EXT_ENTRY(f,            false),
>> +    RISCV_ISA_EXT_ENTRY(d,            false),
>> +    RISCV_ISA_EXT_ENTRY(q,            false),
>> +    RISCV_ISA_EXT_ENTRY(c,            true),
>> +    RISCV_ISA_EXT_ENTRY(v,            false),
>> +    RISCV_ISA_EXT_ENTRY(h,            false),
>> +    RISCV_ISA_EXT_ENTRY(zicntr,       true),
>> +    RISCV_ISA_EXT_ENTRY(zicsr,        true),
>> +    RISCV_ISA_EXT_ENTRY(zifencei,     true),
>> +    RISCV_ISA_EXT_ENTRY(zihintpause,  true),
>> +    RISCV_ISA_EXT_ENTRY(zihpm,        true),
>> +    RISCV_ISA_EXT_ENTRY(zba,          true),
>> +    RISCV_ISA_EXT_ENTRY(zbb,          true),
>> +    RISCV_ISA_EXT_ENTRY(zbs,          true),
>> +    RISCV_ISA_EXT_ENTRY(smaia,        true),
>> +    RISCV_ISA_EXT_ENTRY(smstateen,    true),
>> +    RISCV_ISA_EXT_ENTRY(ssaia,        true),
>> +    RISCV_ISA_EXT_ENTRY(sstc,         false),
>> +    RISCV_ISA_EXT_ENTRY(svade,        false),
>> +    RISCV_ISA_EXT_ENTRY(svpbmt,       false),
>>   };
> 
> Just as an independent, up front remark after having looked at patch 16/17 of
> the other series: Is a mere boolean going to suffice in the longer run? I could
> see some extensions wanting exposing to only RV32 or only RV64 guests. E.g.
> Zilsd is RV32-only, while Zqinx quite likely would want restricting to RV64.

Good point generally.

Right now the distinction can't be observed: RV32 isn't buildable 
(#error "RV32 isn't supported" in asm/config.h), and guest XLEN is 
hard-wired to host XLEN — build_guest_isa_str() emits the rv32/rv64 
prefix from the Kconfig symbol, and riscv_isa_parse_string() rejects a 
host ISA string of the other width.

On top of that, extensions with an architectural XLEN restriction are 
already filtered out for free: compute_guest_isa() masks the table 
against the host bitmap, so an RV32-only extension like Zilsd can't have 
its bit set on an RV64 build regardless of what the table says. The 
boolean only ever subtracts from what the host actually reports.

That leaves purely policy-driven per-XLEN restrictions — e.g. exposing 
Zqinx to RV64 guests but not RV32 ones, since Zqinx is architecturally 
defined for both. Those only become meaningful once guest XLEN can 
differ from host XLEN (i.e. hstatus.VSXL support), and at that point the 
shared guest_isa bitmap has to become per-domain as well, as the comment 
above it already notes.

So I'd rather keep the plain bool for now and widen it to a flags field 
when there's an actual case; it's a mechanical change to the struct, the 
macro and the single test in compute_guest_isa(), all local to 
cpufeature.c. I can add a comment stating the "guest XLEN == host XLEN" 
assumption so the reason is on record. If you'd prefer it as flags from 
the start I don't mind doing it now either. I just don't have a way to 
give either value a meaning yet. So if to do that now I would suggest 
the following:


+/*
+ * Which guests an extension may be handed out to, by guest XLEN.
+ *
+ * These flags express Xen's policy, not the ISA's rules: extensions which
+ * are architecturally tied to one XLEN (Zilsd on RV32, say) need no 
special
+ * treatment here, as they can only ever appear in the "riscv,isa" of a 
host
+ * of that XLEN, and guest_isa is masked against the host ISA bitmap 
anyway.
+ * They are only of use for extensions Xen chooses not to expose to 
guests of
+ * a given width despite the hardware implementing them.
+ */
+#define RISCV_ISA_EXT_GUEST_NONE 0
+#define RISCV_ISA_EXT_GUEST_RV32 (1U << 0)
+#define RISCV_ISA_EXT_GUEST_RV64 (1U << 1)
+#define RISCV_ISA_EXT_GUEST_ANY  (RISCV_ISA_EXT_GUEST_RV32 | \
+                                  RISCV_ISA_EXT_GUEST_RV64)
+
+/*
+ * Guests are of the same width as Xen itself for the time being; once 
guest
+ * XLEN can differ from host XLEN (hstatus.VSXL), this becomes a per-domain
+ * property, just as guest_isa below does.
+ */
+#if defined(CONFIG_RISCV_32)
+#define RISCV_ISA_EXT_GUEST_XLEN RISCV_ISA_EXT_GUEST_RV32
+#elif defined(CONFIG_RISCV_64)
+#define RISCV_ISA_EXT_GUEST_XLEN RISCV_ISA_EXT_GUEST_RV64
+#else
+# error "Unsupported RISC-V bitness"
+#endif
+
  struct riscv_isa_ext_entry {
      unsigned int id;
      const char *name;
-    bool guest_supported;
+    unsigned int guest_flags;
  };

-#define RISCV_ISA_EXT_ENTRY(ext_name, guest_supp)       \
+#define RISCV_ISA_EXT_ENTRY(ext_name, guest_flgs)       \
  {                                                       \
-    .id              = RISCV_ISA_EXT_ ## ext_name,      \
-    .name            = #ext_name,                       \
-    .guest_supported = guest_supp,                      \
+    .id          = RISCV_ISA_EXT_ ## ext_name,          \
+    .name        = #ext_name,                           \
+    .guest_flags = guest_flgs,                          \
  }

  /* Host ISA bitmap */
@@ -149,29 +178,29 @@ static int __init dt_get_cpuid_from_node(const 
struct dt_device_node *cpu,
   * of strncasecmp().
   */
  static const struct riscv_isa_ext_entry __initconstrel riscv_isa_ext[] = {
-    RISCV_ISA_EXT_ENTRY(i,            true),
-    RISCV_ISA_EXT_ENTRY(m,            true),
-    RISCV_ISA_EXT_ENTRY(a,            true),
-    RISCV_ISA_EXT_ENTRY(f,            false),
-    RISCV_ISA_EXT_ENTRY(d,            false),
-    RISCV_ISA_EXT_ENTRY(q,            false),
-    RISCV_ISA_EXT_ENTRY(c,            true),
-    RISCV_ISA_EXT_ENTRY(v,            false),
-    RISCV_ISA_EXT_ENTRY(h,            false),
-    RISCV_ISA_EXT_ENTRY(zicntr,       true),
-    RISCV_ISA_EXT_ENTRY(zicsr,        true),
-    RISCV_ISA_EXT_ENTRY(zifencei,     true),
-    RISCV_ISA_EXT_ENTRY(zihintpause,  true),
-    RISCV_ISA_EXT_ENTRY(zihpm,        true),
-    RISCV_ISA_EXT_ENTRY(zba,          true),
-    RISCV_ISA_EXT_ENTRY(zbb,          true),
-    RISCV_ISA_EXT_ENTRY(zbs,          true),
-    RISCV_ISA_EXT_ENTRY(smaia,        true),
-    RISCV_ISA_EXT_ENTRY(smstateen,    true),
-    RISCV_ISA_EXT_ENTRY(ssaia,        true),
-    RISCV_ISA_EXT_ENTRY(sstc,         false),
-    RISCV_ISA_EXT_ENTRY(svade,        false),
-    RISCV_ISA_EXT_ENTRY(svpbmt,       false),
+    RISCV_ISA_EXT_ENTRY(i,            RISCV_ISA_EXT_GUEST_ANY),
+    RISCV_ISA_EXT_ENTRY(m,            RISCV_ISA_EXT_GUEST_ANY),
+    RISCV_ISA_EXT_ENTRY(a,            RISCV_ISA_EXT_GUEST_ANY),
+    RISCV_ISA_EXT_ENTRY(f,            RISCV_ISA_EXT_GUEST_NONE),
+    RISCV_ISA_EXT_ENTRY(d,            RISCV_ISA_EXT_GUEST_NONE),
+    RISCV_ISA_EXT_ENTRY(q,            RISCV_ISA_EXT_GUEST_NONE),
+    RISCV_ISA_EXT_ENTRY(c,            RISCV_ISA_EXT_GUEST_ANY),
+    RISCV_ISA_EXT_ENTRY(v,            RISCV_ISA_EXT_GUEST_NONE),
+    RISCV_ISA_EXT_ENTRY(h,            RISCV_ISA_EXT_GUEST_NONE),
+    RISCV_ISA_EXT_ENTRY(zicntr,       RISCV_ISA_EXT_GUEST_ANY),
+    RISCV_ISA_EXT_ENTRY(zicsr,        RISCV_ISA_EXT_GUEST_ANY),
+    RISCV_ISA_EXT_ENTRY(zifencei,     RISCV_ISA_EXT_GUEST_ANY),
+    RISCV_ISA_EXT_ENTRY(zihintpause,  RISCV_ISA_EXT_GUEST_ANY),
+    RISCV_ISA_EXT_ENTRY(zihpm,        RISCV_ISA_EXT_GUEST_ANY),
+    RISCV_ISA_EXT_ENTRY(zba,          RISCV_ISA_EXT_GUEST_ANY),
+    RISCV_ISA_EXT_ENTRY(zbb,          RISCV_ISA_EXT_GUEST_ANY),
+    RISCV_ISA_EXT_ENTRY(zbs,          RISCV_ISA_EXT_GUEST_ANY),
+    RISCV_ISA_EXT_ENTRY(smaia,        RISCV_ISA_EXT_GUEST_ANY),
+    RISCV_ISA_EXT_ENTRY(smstateen,    RISCV_ISA_EXT_GUEST_ANY),
+    RISCV_ISA_EXT_ENTRY(ssaia,        RISCV_ISA_EXT_GUEST_ANY),
+    RISCV_ISA_EXT_ENTRY(sstc,         RISCV_ISA_EXT_GUEST_NONE),
+    RISCV_ISA_EXT_ENTRY(svade,        RISCV_ISA_EXT_GUEST_NONE),
+    RISCV_ISA_EXT_ENTRY(svpbmt,       RISCV_ISA_EXT_GUEST_NONE),
  };

  static const struct riscv_isa_ext_data __initconst 
required_extensions[] = {
@@ -572,7 +601,7 @@ static void __init compute_guest_isa(void)
      {
          const struct riscv_isa_ext_entry *ext = &riscv_isa_ext[i];

-        if ( ext->guest_supported &&
+        if ( (ext->guest_flags & RISCV_ISA_EXT_GUEST_XLEN) &&
               riscv_isa_extension_available(NULL, ext->id) )
              __set_bit(ext->id, guest_isa);
      }

Will you be okay with such changes or it could be postponed to a time 
when it will be really needed?

~ Oleksii