RE: Checking Hyper-V hypercall status

[Michael Kelley <mikelley@microsoft.com>]

From: Michael Kelley  Sent: Wednesday, February 10, 2021 9:09 AM
> 
> From: Vitaly Kuznetsov <vkuznets@redhat.com> Sent: Tuesday, February 9, 2021 8:51 AM
> >
> > Michael Kelley <mikelley@microsoft.com> 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.

Michael