| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <44f2032c-557d-4b11-aab8-1dd93646b05f@linux.microsoft.com>
Date: Fri, 7 Mar 2025 11:38:23 -0800
From: Nuno Das Neves <nunodasneves@...ux.microsoft.com>
To: Michael Kelley <mhklinux@...look.com>,
"linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
"x86@...nel.org" <x86@...nel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>,
"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>
Cc: "kys@...rosoft.com" <kys@...rosoft.com>,
"haiyangz@...rosoft.com" <haiyangz@...rosoft.com>,
"wei.liu@...nel.org" <wei.liu@...nel.org>,
"decui@...rosoft.com" <decui@...rosoft.com>,
"catalin.marinas@....com" <catalin.marinas@....com>,
"will@...nel.org" <will@...nel.org>, "tglx@...utronix.de"
<tglx@...utronix.de>, "mingo@...hat.com" <mingo@...hat.com>,
"bp@...en8.de" <bp@...en8.de>,
"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
"hpa@...or.com" <hpa@...or.com>,
"daniel.lezcano@...aro.org" <daniel.lezcano@...aro.org>,
"joro@...tes.org" <joro@...tes.org>,
"robin.murphy@....com" <robin.murphy@....com>, "arnd@...db.de"
<arnd@...db.de>,
"jinankjain@...ux.microsoft.com" <jinankjain@...ux.microsoft.com>,
"muminulrussell@...il.com" <muminulrussell@...il.com>,
"skinsburskii@...ux.microsoft.com" <skinsburskii@...ux.microsoft.com>,
"mrathor@...ux.microsoft.com" <mrathor@...ux.microsoft.com>,
"ssengar@...ux.microsoft.com" <ssengar@...ux.microsoft.com>,
"apais@...ux.microsoft.com" <apais@...ux.microsoft.com>,
"Tianyu.Lan@...rosoft.com" <Tianyu.Lan@...rosoft.com>,
"stanislav.kinsburskiy@...il.com" <stanislav.kinsburskiy@...il.com>,
"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
"vkuznets@...hat.com" <vkuznets@...hat.com>,
"prapal@...ux.microsoft.com" <prapal@...ux.microsoft.com>,
"muislam@...rosoft.com" <muislam@...rosoft.com>,
"anrayabh@...ux.microsoft.com" <anrayabh@...ux.microsoft.com>,
"rafael@...nel.org" <rafael@...nel.org>, "lenb@...nel.org"
<lenb@...nel.org>, "corbet@....net" <corbet@....net>
Subject: Re: [PATCH v5 01/10] hyperv: Convert Hyper-V status codes to strings
On 3/6/2025 9:57 AM, Michael Kelley wrote:
> From: Nuno Das Neves <nunodasneves@...ux.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@...ux.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.
>
Good point - that is easy though, I'll add a __VA_ARGS__ to the macro so any
custom message can be printed alongside the other info.
> 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.
I'll try it out, agree the perf difference probably won't be significant or
even matter much.
Thanks
Nuno
>
> Michael
Powered by blists - more mailing lists