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

[Grygorii Strashko <grygorii_strashko@xxxxxxxx>]

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