Re: [kvmtool PATCH 10/10] riscv: Allow including extensions in the min CPU type using command-line

[Andrew Jones <ajones@ventanamicro.com>]

On Wed, Mar 26, 2025 at 12:26:44PM +0530, Anup Patel wrote:
> It is useful to allow including extensions in the min CPU type on need
> basis via command-line. To achieve this, parse extension names as comma
> separated values appended to the "min" CPU type using command-line.
> 
> For example, to include Sstc and Ssaia in the min CPU type use
> "--cpu-type min,sstc,ssaia" command-line option.
> 
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> ---
>  riscv/fdt.c | 41 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 39 insertions(+), 2 deletions(-)
> 
> diff --git a/riscv/fdt.c b/riscv/fdt.c
> index 4c018c8..9cefd2f 100644
> --- a/riscv/fdt.c
> +++ b/riscv/fdt.c
> @@ -108,6 +108,20 @@ static bool __isa_ext_warn_disable_failure(struct kvm *kvm, struct isa_ext_info
>  	return true;
>  }
>  
> +static void __min_cpu_include(const char *ext, size_t ext_len)

s/include/enable/

> +{
> +	struct isa_ext_info *info;
> +	unsigned long i;
> +
> +	for (i = 0; i < ARRAY_SIZE(isa_info_arr); i++) {
> +		info = &isa_info_arr[i];
> +		if (strlen(info->name) != ext_len)
> +			continue;
> +		if (!strncmp(ext, info->name, ext_len))

strcmp should be fine here since we already checked length.

> +			info->min_cpu_included = true;
> +	}
> +}
> +
>  bool riscv__isa_extension_disabled(struct kvm *kvm, unsigned long isa_ext_id)
>  {
>  	struct isa_ext_info *info = NULL;
> @@ -128,16 +142,39 @@ bool riscv__isa_extension_disabled(struct kvm *kvm, unsigned long isa_ext_id)
>  int riscv__cpu_type_parser(const struct option *opt, const char *arg, int unset)
>  {
>  	struct kvm *kvm = opt->ptr;
> +	const char *str, *nstr;
> +	int len;
>  
> -	if ((strncmp(arg, "min", 3) && strncmp(arg, "max", 3)) || strlen(arg) != 3)
> +	if ((strncmp(arg, "min", 3) || strlen(arg) < 3) &&

If arg == 'min', then it can't be less than 3 so the '|| strlen(arg) < 3'
is dead code.

> +	    (strncmp(arg, "max", 3) || strlen(arg) != 3))

I think we want

 if (strlen(arg) < 3 ||
     (strlen(arg) == 3 && strcmp(arg, "min") && strcmp(arg, "max")) ||
     strncmp(arg, "min", 3))

>  		die("Invalid CPU type %s\n", arg);
>  
>  	if (!strncmp(arg, "max", 3))
>  		kvm->cfg.arch.cpu_type = "max";
>  
> -	if (!strncmp(arg, "min", 3))
> +	if (!strncmp(arg, "min", 3)) {
>  		kvm->cfg.arch.cpu_type = "min";
>  
> +		str = arg;
> +		str += 3;
> +		while (*str) {
> +			if (*str == ',') {
> +				str++;
> +				continue;
> +			}
> +
> +			nstr = strchr(str, ',');
> +			if (!nstr)
> +				nstr = str + strlen(str);
> +
> +			len = nstr - str;
> +			if (len) {

I think len will always be nonzero since *str isn't \0 and we ate all
consecutive ,'s above. __min_cpu_include() is also safe to call with
len=0, so we could drop this check.

> +				__min_cpu_include(str, len);
> +				str += len;
> +			}
> +		}
> +	}
> +
>  	return 0;
>  }
>  
> -- 
> 2.43.0
>

Thanks,
drew