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