Re: [PATCH v1 06/13] mshv: detect and report SEV-SNP support at init

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Failing to query optional SEV-SNP support aborts the entire `mshv` module initialization, preventing non-SNP virtual machines from running on unsupported platforms.
- [Low] The helper function `hv_snp_status_to_string` is defined unconditionally but only used inside an x86-specific block, causing a build failure on non-x86 architectures.
--

--- Patch [6]: [PATCH v1 06/13] mshv: detect and report SEV-SNP support at init ---
commit e3193a7357d34307ce8275f28a3eb193fd3183cf
Author: Wei Liu <[email protected]>

mshv: detect and report SEV-SNP support at init
    
Query Hyper-V dynamic processor features during mshv initialization to
determine SEV-SNP availability, and report the result.

> diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c
> index 3590a8ea24b5f..cc38507570ac4 100644
> --- a/drivers/hv/mshv_root_main.c
> +++ b/drivers/hv/mshv_root_main.c
> @@ -2660,6 +2660,109 @@ static int mshv_root_scheduler_cleanup(unsigned int cpu)
>  }
>  
>  /* Must be called after retrieving the scheduler type */
> +#if defined(__x86_64__)
> +static int mshv_print_max_sev_snp_partitions(struct device *dev)
> +{

[ ... ]

> +	status = hv_do_hypercall(HVCALL_GET_SYSTEM_PROPERTY, input, output);
> +	if (!hv_result_success(status)) {
> +		local_irq_restore(flags);
> +		dev_err(dev, "Failed to get max SNP partitions: %s\n",
> +			hv_result_to_string(status));
> +		return hv_result_to_errno(status);
> +	}

[Severity: High]
Does propagating this error cause driver initialization to abort on platforms
that do not support SEV-SNP? 

If the hypervisor does not support this dynamic processor feature, returning
an error here will cause the entire mshv module to fail to load because it
is propagated up to mshv_parent_partition_init().

[ ... ]

> +static const char *hv_snp_status_to_string(enum hv_snp_status status)
> +{
> +	switch (status) {
> +	case HV_SNP_STATUS_NONE:
> +		return "not available";

[Severity: Low]
This function is defined unconditionally, but it is only called from within
an `#if defined(__x86_64__)` block in mshv_check_sev_snp_support(). 

Does this cause an unused function warning on non-x86 architectures (such as
ARM64) that could break the build when CONFIG_WERROR is enabled?

[ ... ]

> +static int __init mshv_check_sev_snp_support(struct device *dev)
> +{
> +#if defined(__x86_64__)
> +	struct hv_input_get_system_property *input;
> +	struct hv_output_get_system_property *output;
> +	unsigned long flags;
> +	u64 status;
> +	enum hv_snp_status snp_status;
> +
> +	local_irq_save(flags);
> +	input = *this_cpu_ptr(hyperv_pcpu_input_arg);
> +	output = *this_cpu_ptr(hyperv_pcpu_output_arg);
> +
> +	memset(input, 0, sizeof(*input));
> +	input->property_id = HV_DYNAMIC_PROCESSOR_FEATURE_PROPERTY;
> +	input->hv_processor_feature = HV_X64_DYNAMIC_PROCESSOR_FEATURE_SNP_STATUS;
> +
> +	status = hv_do_hypercall(HVCALL_GET_SYSTEM_PROPERTY, input, output);
> +	if (!hv_result_success(status)) {
> +		local_irq_restore(flags);
> +		dev_err(dev, "Failed to get SNP support: %s\n",
> +			hv_result_to_string(status));
> +		return hv_result_to_errno(status);
> +	}

[Severity: High]
Similar to above, does returning an error here prevent the mshv module from
loading on systems without SEV-SNP support (like Intel CPUs) or on older
Hyper-V versions? 

It appears this will abort module initialization if the hypercall fails.

> +
> +	snp_status = output->hv_processor_feature_value;
> +	local_irq_restore(flags);
> +
> +	dev_info(dev, "SEV-SNP support status: %s (%u)\n",
> +		 hv_snp_status_to_string(snp_status), snp_status);
> +
> +	if (snp_status == HV_SNP_STATUS_AVAILABLE)
> +		return mshv_print_max_sev_snp_partitions(dev);
> +#endif
> +
> +	return 0;
> +}
> +
>  static int
>  root_scheduler_init(struct device *dev)
>  {

[ ... ]

> @@ -2763,6 +2866,10 @@ static int __init mshv_parent_partition_init(void)
>  	if (ret)
>  		goto synic_cleanup;
>  
> +	ret = mshv_check_sev_snp_support(dev);
> +	if (ret)
> +		goto synic_cleanup;
> +

[Severity: High]
This is where the error from the informational SEV-SNP checks causes module
initialization to abort. Should these functions handle unsupported features
gracefully instead of failing the init?

>  	ret = root_scheduler_init(dev);
>  	if (ret)
>  		goto synic_cleanup;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6