Re: [RFC PATCH v3 4/7] xen/arm: scmi: introduce SCI SCMI SMC multi-agent driver

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

On Tue, Mar 11, 2025 at 01:16:15PM +0200, Grygorii Strashko wrote:
> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> index 7edf272386e3..fc6041724a13 100644
> --- a/docs/man/xl.cfg.5.pod.in
> +++ b/docs/man/xl.cfg.5.pod.in
> @@ -3126,6 +3126,21 @@ single SCMI OSPM agent support.
>  Should be used together with B<dom0_scmi_smc_passthrough> Xen command line
>  option.
>  
> +=item B<scmi_smc_multiagent>
> +
> +Enables ARM SCMI SMC multi-agent support for the guest by enabling SCMI over
> +SMC calls forwarding from domain to the EL3 firmware (like Trusted Firmware-A)
> +with a multi SCMI OSPM agent support. The SCMI B<agent_id> should be
> +specified for the guest.
> +
> +=back

This new =back shouldn't exist in this patch, because there's no =over
been added. This just fix a bug present in the previous patch.

> +
> +=item B<agent_id=NUMBER>
> +
> +Specifies a non-zero ARM SCI agent id for the guest. This option is mandatory
> +if the SCMI SMC support is enabled for the guest. The agent ids of domains
> +existing on a single host must be unique.

Are they other restriction on what agent_id value can be? I mean from
the description -4242 is a valid value.

>  =back
>  
>  =back
> diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
> index 8e50f6b7c7ac..bc3c64d6ec90 100644
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -1091,6 +1091,15 @@ which serves as Driver domain. The SCMI will be disabled for Dom0/hwdom and
>  SCMI nodes removed from Dom0/hwdom device tree.
>  (for example, thin Dom0 with Driver domain use-case).
>  
> +### dom0_scmi_agent_id (ARM)
> +> `= <integer>`
> +
> +The option is available when `CONFIG_SCMI_SMC_MA` is compiled in, and allows to
> +enable SCMI functionality for Dom0 by specifying a non-zero ARM SCMI agent id.
> +The SCMI will be disabled for Dom0 if this option is not specified
> +(for example, thin Dom0 or dom0less use-cases).
> +The agent ids of domains existing on a single host must be unique.
> +
>  ### dtuart (ARM)
>  > `= path [:options]`
>  
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index d41adea1cefd..cdf5edb299af 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -229,6 +229,10 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>      case LIBXL_ARM_SCI_TYPE_SCMI_SMC:
>          config->arch.arm_sci_type = XEN_DOMCTL_CONFIG_ARM_SCI_SCMI_SMC;
>          break;
> +    case LIBXL_ARM_SCI_TYPE_SCMI_SMC_MULTIAGENT:
> +        config->arch.arm_sci_type = XEN_DOMCTL_CONFIG_ARM_SCI_SCMI_SMC_MA;
> +        config->arch.arm_sci_agent_id = d_config->b_info.arm_sci.agent_id;
> +        break;
>      default:
>          LOG(ERROR, "Unknown ARM_SCI type %d",
>              d_config->b_info.arm_sci.type);
> diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
> index ea0d30654cdd..e6707c7ca9e7 100644
> --- a/tools/libs/light/libxl_types.idl
> +++ b/tools/libs/light/libxl_types.idl
> @@ -553,11 +553,13 @@ libxl_sve_type = Enumeration("sve_type", [
>  
>  libxl_arm_sci_type = Enumeration("arm_sci_type", [
>      (0, "none"),
> -    (1, "scmi_smc")
> +    (1, "scmi_smc"),
> +    (2, "scmi_smc_multiagent")
>      ], init_val = "LIBXL_ARM_SCI_TYPE_NONE")
>  
>  libxl_arm_sci = Struct("arm_sci", [
>      ("type", libxl_arm_sci_type),
> +    ("agent_id", uint8)

Is it necessary to limit this value to a 8-bit value here? Skimming
through the Xen code, it seems that that value can be 32-bits at times,
but just restricted to 8-bits in the hypercall.

>      ])
>  
>  libxl_rdm_reserve = Struct("rdm_reserve", [
> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> index ac9bf0b25c5a..011222ec55b9 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -1324,11 +1327,24 @@ static int parse_arm_sci_config(XLU_Config *cfg, libxl_arm_sci *arm_sci,
>                  tok = ptr + 1;
>              }
>              break;
> +        case STATE_AGENT_ID:
> +            if (*ptr == ',' || *ptr == '\0') {
> +                state = *ptr == ',' ? STATE_OPTION : STATE_TERMINAL;
> +                *ptr = '\0';
> +                arm_sci->agent_id = strtoul(tok, NULL, 0);

You should check that the value returned by strtoul() is actually valid
and does fit in `agent_id`.

> +                tok = ptr + 1;
> +            }
>          default:
>              break;
>          }
>      }
>  
> +    if (arm_sci->type == LIBXL_ARM_SCI_TYPE_SCMI_SMC_MULTIAGENT &&
> +        arm_sci->agent_id == 0) {
> +        fprintf(stderr, "A non-zero ARM_SCI agent_id must be specified\n");
> +        goto parse_error;
> +    }
> +
>      if (tok != ptr || state != STATE_TERMINAL)
>          goto parse_error;
>  
> @@ -3042,6 +3058,7 @@ skip_usbdev:
>          libxl_arm_sci arm_sci = { 0 };
>          if (!parse_arm_sci_config(config, &arm_sci, buf)) {
>              b_info->arm_sci.type = arm_sci.type;
> +            b_info->arm_sci.agent_id = arm_sci.agent_id;

I just realise that it's probably enough to call
parse_arm_sci_config(.., &b_info->arm_sci) instead of declaring
another local `arm_sci` variable. Or is it necessary to have a different
variable somehow?

>          } else {
>              exit(EXIT_FAILURE);
>          }

Thanks,

-- 

Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech