Re: [PATCH v6 01/10] hyperv: Log hypercall status codes as strings

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

On 3/18/2025 11:01 AM, Michael Kelley wrote:
> From: Nuno Das Neves <nunodasneves@linux.microsoft.com> Sent: Friday, March 14, 2025 12:29 PM
>>
>> Introduce hv_status_printk() macros as a convenience to log hypercall
>> errors, formatting them with the status code (HV_STATUS_*) as a raw hex
>> value and also as a string, which saves some time while debugging.
>>
>> Create a table of HV_STATUS_ codes with strings and mapped errnos, and
>> use it for hv_result_to_string() and hv_result_to_errno().
>>
>> Use the new hv_status_printk()s in hv_proc.c, hyperv-iommu.c, and
>> irqdomain.c hypercalls to aid debugging in the root partition.
>>
>> Signed-off-by: Nuno Das Neves <nunodasneves@linux.microsoft.com>
>> Reviewed-by: Stanislav Kinsburskii <skinsburskii@linux.microsoft.com>
>> ---
>>  arch/x86/hyperv/irqdomain.c    |   6 +-
>>  drivers/hv/hv_common.c         | 129 ++++++++++++++++++++++++---------
>>  drivers/hv/hv_proc.c           |  10 +--
>>  drivers/iommu/hyperv-iommu.c   |   4 +-
>>  include/asm-generic/mshyperv.h |  13 ++++
>>  5 files changed, 118 insertions(+), 44 deletions(-)
>>
> 
> [snip]
>  
>> +
>> +struct hv_status_info {
>> +	char *string;
>> +	int errno;
>> +	u16 code;
>> +};
>> +
>> +/*
>> + * Note on the errno mappings:
>> + * A failed hypercall is usually only recoverable (or loggable) near
>> + * the call site where the HV_STATUS_* code is known. So the errno
>> + * it gets converted to is not too useful further up the stack.
>> + * Provide a few mappings that could be useful, and revert to -EIO
>> + * as a fallback.
>> + */
>> +static const struct hv_status_info hv_status_infos[] = {
>> +#define _STATUS_INFO(status, errno) { #status, (errno), (status) }
>> +	_STATUS_INFO(HV_STATUS_SUCCESS,				0),
>> +	_STATUS_INFO(HV_STATUS_INVALID_HYPERCALL_CODE,		-EINVAL),
>> +	_STATUS_INFO(HV_STATUS_INVALID_HYPERCALL_INPUT,		-EINVAL),
>> +	_STATUS_INFO(HV_STATUS_INVALID_ALIGNMENT,		-EIO),
>> +	_STATUS_INFO(HV_STATUS_INVALID_PARAMETER,		-EINVAL),
>> +	_STATUS_INFO(HV_STATUS_ACCESS_DENIED,			-EIO),
>> +	_STATUS_INFO(HV_STATUS_INVALID_PARTITION_STATE,		-EIO),
>> +	_STATUS_INFO(HV_STATUS_OPERATION_DENIED,		-EIO),
>> +	_STATUS_INFO(HV_STATUS_UNKNOWN_PROPERTY,		-EIO),
>> +	_STATUS_INFO(HV_STATUS_PROPERTY_VALUE_OUT_OF_RANGE,	-EIO),
>> +	_STATUS_INFO(HV_STATUS_INSUFFICIENT_MEMORY,		-ENOMEM),
>> +	_STATUS_INFO(HV_STATUS_INVALID_PARTITION_ID,		-EINVAL),
>> +	_STATUS_INFO(HV_STATUS_INVALID_VP_INDEX,		-EINVAL),
>> +	_STATUS_INFO(HV_STATUS_NOT_FOUND,			-EIO),
>> +	_STATUS_INFO(HV_STATUS_INVALID_PORT_ID,			-EINVAL),
>> +	_STATUS_INFO(HV_STATUS_INVALID_CONNECTION_ID,		-EINVAL),
>> +	_STATUS_INFO(HV_STATUS_INSUFFICIENT_BUFFERS,		-EIO),
>> +	_STATUS_INFO(HV_STATUS_NOT_ACKNOWLEDGED,		-EIO),
>> +	_STATUS_INFO(HV_STATUS_INVALID_VP_STATE,		-EIO),
>> +	_STATUS_INFO(HV_STATUS_NO_RESOURCES,			-EIO),
>> +	_STATUS_INFO(HV_STATUS_PROCESSOR_FEATURE_NOT_SUPPORTED,	-EIO),
>> +	_STATUS_INFO(HV_STATUS_INVALID_LP_INDEX,		-EINVAL),
>> +	_STATUS_INFO(HV_STATUS_INVALID_REGISTER_VALUE,		-EINVAL),
>> +	_STATUS_INFO(HV_STATUS_INVALID_LP_INDEX,		-EIO),
>> +	_STATUS_INFO(HV_STATUS_INVALID_REGISTER_VALUE,		-EIO),
>> +	_STATUS_INFO(HV_STATUS_OPERATION_FAILED,		-EIO),
>> +	_STATUS_INFO(HV_STATUS_TIME_OUT,			-EIO),
>> +	_STATUS_INFO(HV_STATUS_CALL_PENDING,			-EIO),
>> +	_STATUS_INFO(HV_STATUS_VTL_ALREADY_ENABLED,		-EIO),
>> +#undef _STATUS_INFO
>> +};
>> +
>> +static inline const struct hv_status_info *find_hv_status_info(u64 hv_status)
>> +{
>> +	int i;
>> +	u16 code = hv_result(hv_status);
>> +
>> +	for (i = 0; i < ARRAY_SIZE(hv_status_infos); ++i) {
>> +		const struct hv_status_info *info = &hv_status_infos[i];
>> +
>> +		if (info->code == code)
>> +			return info;
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +/* Convert a hypercall result into a linux-friendly error code. */
>> +int hv_result_to_errno(u64 status)
>> +{
>> +	const struct hv_status_info *info;
>> +
>> +	/* hv_do_hypercall() may return U64_MAX, hypercalls aren't possible */
>> +	if (unlikely(status == U64_MAX))
>> +		return -EOPNOTSUPP;
>> +
>> +	info = find_hv_status_info(status);
>> +	if (info)
>> +		return info->errno;
>> +
>> +	return -EIO;
>> +}
>> +EXPORT_SYMBOL_GPL(hv_result_to_errno);
>> +
>> +const char *hv_result_to_string(u64 status)
>> +{
>> +	const struct hv_status_info *info;
>> +
>> +	if (unlikely(status == U64_MAX))
>> +		return "Hypercall page missing!";
>> +
>> +	info = find_hv_status_info(status);
>> +	if (info)
>> +		return info->string;
>> +
>> +	return "Unknown";
>> +}
>> +EXPORT_SYMBOL_GPL(hv_result_to_string);
> 
> I think the table-driven approach worked out pretty well. But here's a version that
> is even more compact, and avoids the duplicate testing for U64_MAX and having
> to special case both U64_MAX and not finding a match:
> 
> +
> +struct hv_status_info {
> +	char *string;
> +	int errno;
> +	int code;
> +};
> +
> +/*
> + * Note on the errno mappings:
> + * A failed hypercall is usually only recoverable (or loggable) near
> + * the call site where the HV_STATUS_* code is known. So the errno
> + * it gets converted to is not too useful further up the stack.
> + * Provide a few mappings that could be useful, and revert to -EIO
> + * as a fallback.
> + */
> +static const struct hv_status_info hv_status_infos[] = {
> +#define _STATUS_INFO(status, errno) { #status, (errno), (status) }
> +	_STATUS_INFO(HV_STATUS_SUCCESS,				0),
> +	_STATUS_INFO(HV_STATUS_INVALID_HYPERCALL_CODE,		-EINVAL),
> +	_STATUS_INFO(HV_STATUS_INVALID_HYPERCALL_INPUT,		-EINVAL),
> +	_STATUS_INFO(HV_STATUS_INVALID_ALIGNMENT,		-EIO),
> +	_STATUS_INFO(HV_STATUS_INVALID_PARAMETER,		-EINVAL),
> +	_STATUS_INFO(HV_STATUS_ACCESS_DENIED,			-EIO),
> +	_STATUS_INFO(HV_STATUS_INVALID_PARTITION_STATE,		-EIO),
> +	_STATUS_INFO(HV_STATUS_OPERATION_DENIED,		-EIO),
> +	_STATUS_INFO(HV_STATUS_UNKNOWN_PROPERTY,		-EIO),
> +	_STATUS_INFO(HV_STATUS_PROPERTY_VALUE_OUT_OF_RANGE,	-EIO),
> +	_STATUS_INFO(HV_STATUS_INSUFFICIENT_MEMORY,		-ENOMEM),
> +	_STATUS_INFO(HV_STATUS_INVALID_PARTITION_ID,		-EINVAL),
> +	_STATUS_INFO(HV_STATUS_INVALID_VP_INDEX,		-EINVAL),
> +	_STATUS_INFO(HV_STATUS_NOT_FOUND,			-EIO),
> +	_STATUS_INFO(HV_STATUS_INVALID_PORT_ID,			-EINVAL),
> +	_STATUS_INFO(HV_STATUS_INVALID_CONNECTION_ID,		-EINVAL),
> +	_STATUS_INFO(HV_STATUS_INSUFFICIENT_BUFFERS,		-EIO),
> +	_STATUS_INFO(HV_STATUS_NOT_ACKNOWLEDGED,		-EIO),
> +	_STATUS_INFO(HV_STATUS_INVALID_VP_STATE,		-EIO),
> +	_STATUS_INFO(HV_STATUS_NO_RESOURCES,			-EIO),
> +	_STATUS_INFO(HV_STATUS_PROCESSOR_FEATURE_NOT_SUPPORTED,	-EIO),
> +	_STATUS_INFO(HV_STATUS_INVALID_LP_INDEX,		-EINVAL),
> +	_STATUS_INFO(HV_STATUS_INVALID_REGISTER_VALUE,		-EINVAL),
> +	_STATUS_INFO(HV_STATUS_INVALID_LP_INDEX,		-EIO),
> +	_STATUS_INFO(HV_STATUS_INVALID_REGISTER_VALUE,		-EIO),
> +	_STATUS_INFO(HV_STATUS_OPERATION_FAILED,		-EIO),
> +	_STATUS_INFO(HV_STATUS_TIME_OUT,			-EIO),
> +	_STATUS_INFO(HV_STATUS_CALL_PENDING,			-EIO),
> +	_STATUS_INFO(HV_STATUS_VTL_ALREADY_ENABLED,		-EIO),
> +	{"Hypercall page missing!", -EOPNOTSUPP, -1}, /* code -1 is "no hypercall page" */
> +	{"Unknown", -EIO, -2},  /* code -2 is "Not found" entry; must be last */
> +#undef _STATUS_INFO
> +};
> +
> +static inline const struct hv_status_info *find_hv_status_info(u64 hv_status)
> +{
> +	int i, code;
> +	const struct hv_status_info *info;
> +
> +	/* hv_do_hypercall() may return U64_MAX, hypercalls aren't possible */
> +	if (unlikely(hv_status == U64_MAX))
> +		code = -1;
> +	else
> +		code = hv_result(hv_status);
> +
> +	for (i = 0; i < ARRAY_SIZE(hv_status_infos); ++i) {
> +		info = &hv_status_infos[i];
> +		if (info->code == code || info->code == -2)
> +			break;
> +	}
> +
> +	return info;
> +}
> +
> +/* Convert a hypercall result into a linux-friendly error code. */
> +int hv_result_to_errno(u64 status)
> +{
> +	return find_hv_status_info(status)->errno;
> +}
> +EXPORT_SYMBOL_GPL(hv_result_to_errno);
> +
> +const char *hv_result_to_string(u64 status)
> +{
> +	return find_hv_status_info(status)->string;
> +}
> +EXPORT_SYMBOL_GPL(hv_result_to_string);
> 
> It could be even more compact by exporting find_hv_status_info() and
> letting  hv_result_to_errno() and hv_result_to_string() be #defines to
> find_hv_status_info()->errno and find_hv_status_info()->string,
> respectively.
> 
> Note that in struct hv_status_info, the "code" field is defined as "int"
> instead of "u16" so that it can contain sentinel values -1 and -2 that
> won't overlap with HV_STATUS_* values.
> 
Played around with this some more.

I like your idea of making it more compact by dealing with U64_MAX and
unknown in find_hv_status_info(), however I'm not as keen on putting
these cases in the array and iterating over the whole array when they
could just be static constants or inline struct initializers. See below.

I also like the idea of making hv_result_to_*() functions into simple
macros and exporting find_hv_status_info(). However, if it gets used
elsewhere it makes more sense if the returned hv_status_info for the
"Unknown" case contains the actual status code instead of replacing
that information with -2, so then I'd want to return it by value
instead of pointer:

+static const struct hv_status_info hv_status_infos[] = {
+#define _STATUS_INFO(status, errno) { #status, (status), (errno) }
+       _STATUS_INFO(HV_STATUS_SUCCESS,                         0),
<snip>
+       _STATUS_INFO(HV_STATUS_VTL_ALREADY_ENABLED,             -EIO),
+#undef _STATUS_INFO
+};
+
+struct hv_status_info hv_get_status_info(u64 hv_status)
+{
+       int i;
+       const struct hv_status_info *info;
+       u16 code = hv_result(hv_status);
+       struct hv_status_info ret = {"Unknown", code, -EIO};
+
+       if (hv_status == U64_MAX)
+               ret = (struct hv_status_info){"Hypercall page missing!", -1,
+                                             -EOPNOTSUPP};
+       else
+               for (i = 0; i < ARRAY_SIZE(hv_status_infos); ++i) {
+                       info = &hv_status_infos[i];
+                       if (info->code == code) {
+                               ret = *info;
+                               break;
+                       }
+               }
+
+       return ret;
+}
+EXPORT_SYMBOL_GPL(hv_get_status_info);

and in mshyperv.h:

+#define hv_result_to_string(hv_status) hv_get_status_info(hv_status).string
+#define hv_result_to_errno(hv_status) hv_get_status_info(hv_status).errno
+
+struct hv_status_info {
+       char *string;
+       int code;
+       int errno;
+};
+
+struct hv_status_info hv_get_status_info(u64 hv_status);

Note also I switched the order of code and errno in hv_status_info,
mainly because I think the struct initializers for "Unknown" and
"Hypercall page missing!" are more readable with that order:
{string, code, errno}

Do you see any problems with the above?

> Anyway, just a suggestion. The current code works from what I can
> see.
Thanks, it's not a bad idea at all to make it as compact and readable
as possible on the first try, but not a big loss either way.

Thanks
Nuno

> 
> Michael