Michael Kelley <mikelley@xxxxxxxxxxxxx> writes:
> From: Michael Kelley Sent: Wednesday, February 10, 2021 9:09 AM
>>
>> From: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx> Sent: Tuesday, February 9, 2021 8:51 AM
>> >
>> > Michael Kelley <mikelley@xxxxxxxxxxxxx> writes:
>> >
>> > > As noted in a previous email, we don't have a consistent
>> > > pattern for checking Hyper-V hypercall status. Existing code and
>> > > recent new code uses a number of variants. The variants work, but
>> > > a consistent pattern would improve the readability of the code, and
>> > > be more conformant to what the Hyper-V TLFS says about hypercall
>> > > status. In particular, the 64 bit hypercall status contains fields that
>> > > the TLFS says should be ignored -- evidently they are not guaranteed
>> > > to be zero (though I've never seen otherwise).
>> > >
>> > > I'd propose the following helper functions to go in
>> > > asm-generic/mshyperv.h. The function names are relatively short
>> > > for readability:
>> > >
>> > > static inline u64 hv_result(u64 status)
>> > > {
>> > > return status & HV_HYPERCALL_RESULT_MASK;
>> > > }
>> > >
>> > > static inline bool hv_result_success(u64 status)
>> > > {
>> > > return hv_result(status) == HV_STATUS_SUCCESS;
>> > > }
>> > >
>> > > static inline unsigned int hv_repcomp(u64 status)
>> > > {
>> > > return (status & HV_HYPERCALL_REP_COMP_MASK) >>
>> > > HV_HYPERCALL_REP_COMP_OFFSET;
>> > > }
>> > >
>> > > The hv_do_hypercall() function (and its 'rep' and 'fast' variants) should
>> > > always assign the result to a u64 local variable, which is the return
>> > > type of the functions. Then the above functions can act on that local
>> > > variable. Here are some examples:
>> > >
>> > > u64 status;
>> > > unsigned int completed;
>> > >
>> > > status = hv_do_hypercall(<some args>);
>> > > if (!hv_result_success(status)) {
>> > > <handle error case>
>> > > }
>> > >
>> > > status = hv_do_rep_hypercall(<some args>);
>> > > if (hv_result(status) == HV_STATUS_INSUFFICIENT_MEMORY) {
>> > > <deposit more memory pages>
>> > > goto retry;
>> > > } else if (!hv_result_success(status)) {
>> > > <handle error case>
>> > > }
>> > > completed = hv_repcomp(status);
>> > >
>> > >
>> > > Thoughts?
>> >
>> > Personally, I like it and think it's going to be sufficient.
>> >
>> > Alternatively, I can suggest we introduce something like
>> >
>> > struct hv_result {
>> > u64 status:16;
>> > u64 rsvd1:16;
>> > u64 reps_comp:12;
>> > u64 rsvd1:20;
>> > };
>> >
>> > and make hv_do_rep_hypercall() return it. So the code above will look
>> > like:
>> >
>> > struct hv_result result;
>> >
>> > result = hv_do_rep_hypercall(<some args>);
>> > if (result.status) == HV_STATUS_INSUFFICIENT_MEMORY) {
>> > <deposit more memory pages>
>> > goto retry;
>> > } else if (result.status != HV_STATUS_SUCCESS) {
>> > <handle error case>
>> > }
>> > completed = result.reps_comp;
>> >
>> > --
>>
>> Your proposal is OK with me as well, though one downside is that it is
>> not compatible with current code. The return type of hv_do_hypercall()
>> and friends would change, so we would have to change all occurrences
>> in a single patch. With the helper functions, changing the code to use
>> them can be done incrementally.
>>
>> Back when I was first working on the patches for Linux on ARM64 on
>> Hyper-V, I went down the path of defining a structure for the hypercall
>> result, but ended up abandoning it, probably because of the compatibility
>> issue.
>>
>> But either works and is OK with me.
>>
>
> In thinking about this a few more days, having the hv_do_hypercall()
> functions return a struct rather than a scalar value seems a bit off
> the beaten path, even if the struct is a 64 bit quantity. I just wonder
> if currently unknown problems might arise later with other tooling
> (like sparse) in using that approach. So I'm leaning toward the
> helper function approach instead of bit fields in a struct.
>
No problem with me, let's stay conservative and use helpers.
--
Vitaly
