Re: [PATCH v6 10/12] xen/tools: add sve parameter in XL configuration

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Mon, Apr 24, 2023 at 07:02:46AM +0100, Luca Fancellu wrote:
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index ddc7b2a15975..1e69dac2c4fa 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -211,6 +213,12 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>          return ERROR_FAIL;
>      }
>  
> +    /* Parameter is sanitised in libxl__arch_domain_build_info_setdefault */
> +    if (d_config->b_info.arch_arm.sve_vl) {
> +        /* Vector length is divided by 128 in struct xen_domctl_createdomain 
> */
> +        config->arch.sve_vl = d_config->b_info.arch_arm.sve_vl / 128U;
> +    }
> +
>      return 0;
>  }
>  
> @@ -1681,6 +1689,26 @@ int libxl__arch_domain_build_info_setdefault(libxl__gc 
> *gc,
>      /* ACPI is disabled by default */
>      libxl_defbool_setdefault(&b_info->acpi, false);
>  
> +    /* Sanitise SVE parameter */
> +    if (b_info->arch_arm.sve_vl) {
> +        unsigned int max_sve_vl =
> +            arch_capabilities_arm_sve(physinfo->arch_capabilities);
> +
> +        if (!max_sve_vl) {
> +            LOG(ERROR, "SVE is unsupported on this machine.");
> +            return ERROR_FAIL;
> +        }
> +
> +        if (LIBXL_SVE_TYPE_HW == b_info->arch_arm.sve_vl) {
> +            b_info->arch_arm.sve_vl = max_sve_vl;
> +        } else if (b_info->arch_arm.sve_vl > max_sve_vl) {
> +            LOG(ERROR,
> +                "Invalid sve value: %d. Platform supports up to %u bits",
> +                b_info->arch_arm.sve_vl, max_sve_vl);
> +            return ERROR_FAIL;
> +        }

You still need to check that sve_vl is one of the value from the enum,
or that the value is divisible by 128.

> +    }
> +
>      if (b_info->type != LIBXL_DOMAIN_TYPE_PV)
>          return 0;
>  
> diff --git a/tools/libs/light/libxl_types.idl 
> b/tools/libs/light/libxl_types.idl
> index fd31dacf7d5a..9e48bb772646 100644
> --- a/tools/libs/light/libxl_types.idl
> +++ b/tools/libs/light/libxl_types.idl
> @@ -523,6 +523,27 @@ libxl_tee_type = Enumeration("tee_type", [
>      (1, "optee")
>      ], init_val = "LIBXL_TEE_TYPE_NONE")
>  
> +libxl_sve_type = Enumeration("sve_type", [
> +    (-1, "hw"),
> +    (0, "disabled"),
> +    (128, "128"),
> +    (256, "256"),
> +    (384, "384"),
> +    (512, "512"),
> +    (640, "640"),
> +    (768, "768"),
> +    (896, "896"),
> +    (1024, "1024"),
> +    (1152, "1152"),
> +    (1280, "1280"),
> +    (1408, "1408"),
> +    (1536, "1536"),
> +    (1664, "1664"),
> +    (1792, "1792"),
> +    (1920, "1920"),
> +    (2048, "2048")
> +    ], init_val = "LIBXL_SVE_TYPE_DISABLED")

I'm not sure if I like that or not. Is there a reason to stop at 2048?
It is possible that there will be more value available in the future?

Also this mean that users of libxl (like libvirt) would be supposed to
use LIBXL_SVE_TYPE_1024 for e.g., or use libxl_sve_type_from_string().

Also, it feels weird to me to mostly use numerical value of the enum
rather than the enum itself.

Anyway, hopefully that enum will work fine.

>  libxl_rdm_reserve = Struct("rdm_reserve", [
>      ("strategy",    libxl_rdm_reserve_strategy),
>      ("policy",      libxl_rdm_reserve_policy),

Thanks,

-- 
Anthony PERARD