Re: [RFC PATCH v3 3/7] xen/arm: scmi-smc: passthrough SCMI SMC to guest domain

["Anthony PERARD" <anthony.perard@vates.tech>]

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;

Thanks,

-- 

Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech