Re: [PATCH v5 01/10] hyperv: Convert Hyper-V status codes to strings

[Nuno Das Neves <nunodasneves@linux.microsoft.com>]

On 3/6/2025 10:09 AM, Michael Kelley wrote:
> From: Michael Kelley <mhklinux@outlook.com> Sent: Thursday, March 6, 2025 9:58 AM
> 
>>
>> From: Nuno Das Neves <nunodasneves@linux.microsoft.com> Sent: Wednesday, February
>> 26, 2025 3:08 PM
>>>
>>> Introduce hv_result_to_string() for this purpose. This allows
>>> hypercall failures to be debugged more easily with dmesg.
>>>
>>> Signed-off-by: Nuno Das Neves <nunodasneves@linux.microsoft.com>
>>> ---
>>>  drivers/hv/hv_common.c         | 65 ++++++++++++++++++++++++++++++++++
>>>  drivers/hv/hv_proc.c           | 13 ++++---
>>>  include/asm-generic/mshyperv.h |  1 +
>>>  3 files changed, 74 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
>>> index 9804adb4cc56..ce20818688fe 100644
>>> --- a/drivers/hv/hv_common.c
>>> +++ b/drivers/hv/hv_common.c
>>> @@ -740,3 +740,68 @@ void hv_identify_partition_type(void)
>>>  			pr_crit("Hyper-V: CONFIG_MSHV_ROOT not enabled!\n");
>>>  	}
>>>  }
>>> +
>>> +const char *hv_result_to_string(u64 hv_status)
>>> +{
>>> +	switch (hv_result(hv_status)) {
>>> +	case HV_STATUS_SUCCESS:
>>> +		return "HV_STATUS_SUCCESS";
>>> +	case HV_STATUS_INVALID_HYPERCALL_CODE:
>>> +		return "HV_STATUS_INVALID_HYPERCALL_CODE";
>>> +	case HV_STATUS_INVALID_HYPERCALL_INPUT:
>>> +		return "HV_STATUS_INVALID_HYPERCALL_INPUT";
>>> +	case HV_STATUS_INVALID_ALIGNMENT:
>>> +		return "HV_STATUS_INVALID_ALIGNMENT";
>>> +	case HV_STATUS_INVALID_PARAMETER:
>>> +		return "HV_STATUS_INVALID_PARAMETER";
>>> +	case HV_STATUS_ACCESS_DENIED:
>>> +		return "HV_STATUS_ACCESS_DENIED";
>>> +	case HV_STATUS_INVALID_PARTITION_STATE:
>>> +		return "HV_STATUS_INVALID_PARTITION_STATE";
>>> +	case HV_STATUS_OPERATION_DENIED:
>>> +		return "HV_STATUS_OPERATION_DENIED";
>>> +	case HV_STATUS_UNKNOWN_PROPERTY:
>>> +		return "HV_STATUS_UNKNOWN_PROPERTY";
>>> +	case HV_STATUS_PROPERTY_VALUE_OUT_OF_RANGE:
>>> +		return "HV_STATUS_PROPERTY_VALUE_OUT_OF_RANGE";
>>> +	case HV_STATUS_INSUFFICIENT_MEMORY:
>>> +		return "HV_STATUS_INSUFFICIENT_MEMORY";
>>> +	case HV_STATUS_INVALID_PARTITION_ID:
>>> +		return "HV_STATUS_INVALID_PARTITION_ID";
>>> +	case HV_STATUS_INVALID_VP_INDEX:
>>> +		return "HV_STATUS_INVALID_VP_INDEX";
>>> +	case HV_STATUS_NOT_FOUND:
>>> +		return "HV_STATUS_NOT_FOUND";
>>> +	case HV_STATUS_INVALID_PORT_ID:
>>> +		return "HV_STATUS_INVALID_PORT_ID";
>>> +	case HV_STATUS_INVALID_CONNECTION_ID:
>>> +		return "HV_STATUS_INVALID_CONNECTION_ID";
>>> +	case HV_STATUS_INSUFFICIENT_BUFFERS:
>>> +		return "HV_STATUS_INSUFFICIENT_BUFFERS";
>>> +	case HV_STATUS_NOT_ACKNOWLEDGED:
>>> +		return "HV_STATUS_NOT_ACKNOWLEDGED";
>>> +	case HV_STATUS_INVALID_VP_STATE:
>>> +		return "HV_STATUS_INVALID_VP_STATE";
>>> +	case HV_STATUS_NO_RESOURCES:
>>> +		return "HV_STATUS_NO_RESOURCES";
>>> +	case HV_STATUS_PROCESSOR_FEATURE_NOT_SUPPORTED:
>>> +		return "HV_STATUS_PROCESSOR_FEATURE_NOT_SUPPORTED";
>>> +	case HV_STATUS_INVALID_LP_INDEX:
>>> +		return "HV_STATUS_INVALID_LP_INDEX";
>>> +	case HV_STATUS_INVALID_REGISTER_VALUE:
>>> +		return "HV_STATUS_INVALID_REGISTER_VALUE";
>>> +	case HV_STATUS_OPERATION_FAILED:
>>> +		return "HV_STATUS_OPERATION_FAILED";
>>> +	case HV_STATUS_TIME_OUT:
>>> +		return "HV_STATUS_TIME_OUT";
>>> +	case HV_STATUS_CALL_PENDING:
>>> +		return "HV_STATUS_CALL_PENDING";
>>> +	case HV_STATUS_VTL_ALREADY_ENABLED:
>>> +		return "HV_STATUS_VTL_ALREADY_ENABLED";
>>> +	default:
>>> +		return "Unknown";
>>> +	};
>>> +	return "Unknown";
>>> +}
>>> +EXPORT_SYMBOL_GPL(hv_result_to_string);
>>> +
>>> diff --git a/drivers/hv/hv_proc.c b/drivers/hv/hv_proc.c
>>> index 2fae18e4f7d2..8fc30f509fa7 100644
>>> --- a/drivers/hv/hv_proc.c
>>> +++ b/drivers/hv/hv_proc.c
>>> @@ -87,7 +87,8 @@ int hv_call_deposit_pages(int node, u64 partition_id, u32
>>> num_pages)
>>>  				     page_count, 0, input_page, NULL);
>>>  	local_irq_restore(flags);
>>>  	if (!hv_result_success(status)) {
>>> -		pr_err("Failed to deposit pages: %lld\n", status);
>>> +		pr_err("%s: Failed to deposit pages: %s\n", __func__,
>>> +		       hv_result_to_string(status));
>>>  		ret = hv_result_to_errno(status);
>>>  		goto err_free_allocations;
>>>  	}
>>> @@ -137,8 +138,9 @@ int hv_call_add_logical_proc(int node, u32 lp_index, u32 apic_id)
>>>
>>>  		if (hv_result(status) != HV_STATUS_INSUFFICIENT_MEMORY) {
>>>  			if (!hv_result_success(status)) {
>>> -				pr_err("%s: cpu %u apic ID %u, %lld\n", __func__,
>>> -				       lp_index, apic_id, status);
>>> +				pr_err("%s: cpu %u apic ID %u, %s\n",
>>> +				       __func__, lp_index, apic_id,
>>> +				       hv_result_to_string(status));
>>>  				ret = hv_result_to_errno(status);
>>>  			}
>>>  			break;
>>> @@ -179,8 +181,9 @@ int hv_call_create_vp(int node, u64 partition_id, u32 vp_index,
>>> u32 flags)
>>>
>>>  		if (hv_result(status) != HV_STATUS_INSUFFICIENT_MEMORY) {
>>>  			if (!hv_result_success(status)) {
>>> -				pr_err("%s: vcpu %u, lp %u, %lld\n", __func__,
>>> -				       vp_index, flags, status);
>>> +				pr_err("%s: vcpu %u, lp %u, %s\n",
>>> +				       __func__, vp_index, flags,
>>> +				       hv_result_to_string(status));
>>>  				ret = hv_result_to_errno(status);
>>>  			}
>>>  			break;
>>> diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
>>> index b13b0cda4ac8..dc4729dba9ef 100644
>>> --- a/include/asm-generic/mshyperv.h
>>> +++ b/include/asm-generic/mshyperv.h
>>> @@ -298,6 +298,7 @@ static inline int cpumask_to_vpset_skip(struct hv_vpset *vpset,
>>>  	return __cpumask_to_vpset(vpset, cpus, func);
>>>  }
>>>
>>> +const char *hv_result_to_string(u64 hv_status);
>>>  int hv_result_to_errno(u64 status);
>>>  void hyperv_report_panic(struct pt_regs *regs, long err, bool in_die);
>>>  bool hv_is_hyperv_initialized(void);
>>> --
>>> 2.34.1
>>
>> I've read through the other comments on this patch. I definitely vote
>> for outputting both the hex code along with a string translation, which
>> could be empty if the hex code is unrecognized by the translation code.
>>
>> I can see providing something like hv_hvcall_err() as Nuno proposed, since
>> that standardizes the text output. But I wonder if it would be too limiting.
>> For example, in the changes above, both hv_call_add_logical_proc() and
>> hv_call_create_vp() output additional debugging values, which we probably
>> don't want to give up.
>>
>> Lastly, from an implementation standpoint, rather than using a big
>> switch statement, build a static array of entries that each have the
>> hex code and string equivalent. Then hv_result_to_string() loops through
>> the array looking for a match. This won't be any slower than the big switch
>> statement. I've seen other places in the kernel where string names are
>> output, and looking up the strings in a static array is the typical approach.
>> You'll have to work through the details and see if avoids being too clumsy,
>> but I think it will be OK.
>>
> 
> Better yet, also include the translated errno in each static array entry.
> Then hv_result_to_errno() can do the same kind of lookup instead of
> having its own switch statement. I did a quick look to see if the two
> functions might be combined to do only a single lookup, but that looks
> somewhat clumsy unless someone else spots a better way to handle it.
> The cost of doing two lookups doesn't really matter in an error case.
> 
> FWIW, hv_result_to_errno() and the new hv_result_to_string() are both
> slightly misnamed. The input argument is a full 64-bit hv_status, not the
> smaller 16-bit result field. hv_status_to_errno() and hv_status_to_string()
> would be more precise.
> 
Hmm, well I'll admit I was and still am rather confused on this point.

In the TLFS (section 3.8) the entire 64-bit return value is called the
"hypercall result value".
The 16-bit HV_STATUS part is *also* called the "result" in this section.
Later, in section 3.12, the 16-bit field is referred to as a "status value
field".
Furthermore, the name of the 16-bit value, itself, is HV_STATUS.

Despite the inconsistency, in my mind it makes the most sense that the
16-bit HV_STATUS part the "status" and the entire 64-bit return value the
"result". I am aware that elsewhere (and in the driver patches in this
series), the name "status" is used to refer to the entire 64-bit return
value.

These functions were actually called hv_status_to_errno() and hv_status_to_string()
in the past, but I changed them to use "result" by following my own logic, and I
thought this also matched the naming of hv_result() and hv_result_success().
However I now realize that the "result" in these names refers to the *output* of
these functions... they take a u64 status as a parameter after all..

So in the end I'm rather bothered by this whole situation. I can change these
names back to "status" (although hv_result_to_errno() is already merged, I
could send a fixup), or I could keep "result", which I think is a more
logical name for the 64-bit value, even though it somewhat contradicts how
the term is already used in the kernel.

Given it doesn't seem to be well-defined in the first place, I'm not really
sure the best route.

Nuno

> Michael