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.
Michael
