From: Nuno Das Neves <redacted> Sent: Thursday, March 6, 2025 10:41 AM

On 3/6/2025 10:09 AM, Michael Kelley wrote:

quoted

From: Michael Kelley <redacted> Sent: Thursday, March 6, 2025 9:58 AM

[snip]

quoted

quoted

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.

Hmmm. You are right. I had in my mind that "status" is the full 64-bit
value, and "result" is the 16-bit error code. But that's certainly not
always the case. And as you point out, it doesn't comport with the TLFS,
and the TLFS itself is not consistent in the terminology.

Ignore my comment. The difference doesn't have any real impact. Leave
the sorting out for some other time. :-)

Michael