On 3/6/2025 10:09 AM, Michael Kelley wrote:
> From: Michael Kelley <mhklinux@xxxxxxxxxxx> Sent: Thursday, March 6, 2025 9:58 AM
>
>>
>> From: Nuno Das Neves <nunodasneves@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx>
>>> ---
>>> 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
