Hi Anthony,

Thanks a lot for your comments.

On 14.03.25 18:23, Anthony PERARD wrote:
On Tue, Mar 11, 2025 at 01:16:14PM +0200, Grygorii Strashko wrote:
diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index 8e1422104e50..7edf272386e3 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -3094,6 +3094,42 @@ assigned to the domain.
=back +=over 4

This =over 4 is unnecessary, but you need to move that existing =back to
the end of the Arm section, just before "=head3 x86" that we can see in
the context of this patch. (And doing that will fix a compiliation
failure ;-))

+=item B<arm_sci="ARM_SCI_STRING">
+
+Set ARM_SCI specific options for the guest. ARM SCI is System
+Control Protocol allows domain to manage various functions that are provided
+by HW platform firmware.
+
+B<ARM_SCI_STRING> is a comma separated list of C<KEY=VALUE> settings,
+from the following list:
+
+=over 4
+
+=item B<type=STRING>
+
+Specifies an ARM SCI type for the guest.
+
+=over 4
+
+=item B<none>
+
+Don't allow guest to use ARM SCI if present on the platform. This is the
+default value.
+
+=item B<scmi_smc>
+
+Enables ARM SCMI SMC support for the guest by enabling SCMI over SMC calls
+forwarding from domain to the EL3 firmware (like Trusted Firmware-A) with a
+single SCMI OSPM agent support.
+Should be used together with B<dom0_scmi_smc_passthrough> Xen command line
+option.
+
+=back
+
+=back
+
  =head3 x86
=over 4
diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index f8fe4afd7dca..5fa43637ab76 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -313,6 +313,11 @@
   */
  #define LIBXL_HAVE_BUILDINFO_ARCH_NR_SPIS 1
+/*
+ * libxl_domain_build_info has the arch_arm.sci* fields.

The new field seems to be called `arm_sci`. Did you intend to add `sci`
in `arch_arm` instead? Also, there's only `type` been added to
`arm_sci`, with the possibility to add more field in the future, so it
would be better to say that only "type" exist.

+ */
+#define LIBXL_HAVE_BUILDINFO_ARCH_ARM_SCI 1
+
  /*
   * LIBXL_HAVE_SOFT_RESET indicates that libxl supports performing
   * 'soft reset' for domains and there is 'soft_reset' shutdown reason
diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index 33c9cfc1a267..ea0d30654cdd 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -551,6 +551,15 @@ libxl_sve_type = Enumeration("sve_type", [
      (2048, "2048")
      ], init_val = "LIBXL_SVE_TYPE_DISABLED")
+libxl_arm_sci_type = Enumeration("arm_sci_type", [
+    (0, "none"),
+    (1, "scmi_smc")
+    ], init_val = "LIBXL_ARM_SCI_TYPE_NONE")
+
+libxl_arm_sci = Struct("arm_sci", [
+    ("type", libxl_arm_sci_type),
+    ])
+
  libxl_rdm_reserve = Struct("rdm_reserve", [
      ("strategy",    libxl_rdm_reserve_strategy),
      ("policy",      libxl_rdm_reserve_policy),
@@ -639,6 +648,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
      ("apic",             libxl_defbool),
      ("dm_restrict",      libxl_defbool),
      ("tee",              libxl_tee_type),
+    ("arm_sci",          libxl_arm_sci),
      ("u", KeyedUnion(None, libxl_domain_type, "type",
                  [("hvm", Struct(None, [("firmware",         string),
                                         ("bios",             libxl_bios_type),
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 9a3679c02325..ac9bf0b25c5a 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -1284,6 +1284,63 @@ out:
      if (rc) exit(EXIT_FAILURE);
  }
+static int parse_arm_sci_config(XLU_Config *cfg, libxl_arm_sci *arm_sci,
+                                const char *str)
+{
+    enum {
+        STATE_OPTION,
+        STATE_TYPE,
+        STATE_TERMINAL,
+    };
+    int ret, state = STATE_OPTION;
+    char *buf2, *tok, *ptr, *end;
+
+    if (NULL == (buf2 = ptr = strdup(str)))
+        return ERROR_NOMEM;
+
+    for (tok = ptr, end = ptr + strlen(ptr) + 1; ptr < end; ptr++) {
+        switch(state) {
+        case STATE_OPTION:
+            if (*ptr == '=') {
+                *ptr = '\0';
+                if (!strcmp(tok, "type")) {
+                    state = STATE_TYPE;
+                } else {
+                    fprintf(stderr, "Unknown ARM_SCI option: %s\n", tok);
+                    goto parse_error;
+                }
+                tok = ptr + 1;
+            }
+            break;
+        case STATE_TYPE:
+            if (*ptr == '\0' || *ptr == ',') {
+                state = *ptr == ',' ? STATE_OPTION : STATE_TERMINAL;
+                *ptr = '\0';
+                ret = libxl_arm_sci_type_from_string(tok, &arm_sci->type);
+                if (ret) {
+                    fprintf(stderr, "Unknown ARM_SCI type: %s\n", tok);
+                    goto parse_error;
+                }
+                tok = ptr + 1;
+            }
+            break;
+        default:
+            break;

Instead of rolling your own parsing algo, could you do something similar
to the code that parse VIRTIO_DEVICE_STRING just above? It's basically a
loop with strtok() and a bunch of MATCH_OPTION() call (see
parse_virtio_config(), not the MATCH_OPTION for "type") which seems it
would be enough for parsing the SCI string. It would make
parse_arm_sci_config() much smaller and avoid a lot of repetition in the
code.

+        }
+    }
+
+    if (tok != ptr || state != STATE_TERMINAL)
+        goto parse_error;
+
+    free(buf2);
+
+    return 0;
+
+parse_error:
+    free(buf2);
+    return ERROR_INVAL;
+}
+
  void parse_config_data(const char *config_source,
                         const char *config_data,
                         int config_len,
@@ -2981,6 +3038,15 @@ skip_usbdev:
      if (!xlu_cfg_get_long (config, "nr_spis", &l, 0))
          b_info->arch_arm.nr_spis = l;
+ if (!xlu_cfg_get_string(config, "arm_sci", &buf, 1)) {
+        libxl_arm_sci arm_sci = { 0 };

Please use libxl_arm_sci_init() to initialise `arm_sci` instead. And add
a call to libxl_arm_sci_dispose() at the end of this context.

+        if (!parse_arm_sci_config(config, &arm_sci, buf)) {
+            b_info->arm_sci.type = arm_sci.type;
+        } else {
+            exit(EXIT_FAILURE);
+        }
+    }
+
      parse_vkb_list(config, d_config);
d_config->virtios = NULL;

I think I understood you comments and will update accordingly.

--
Best regards,
-grygorii

Reply via email to