RE: Checking Hyper-V hypercall status

* RE: Checking Hyper-V hypercall status
@ 2021-02-10 17:08 Michael Kelley
  2021-02-16 14:25 ` Michael Kelley
  0 siblings, 1 reply; 3+ messages in thread
From: Michael Kelley @ 2021-02-10 17:08 UTC (permalink / raw)
  To: vkuznets; +Cc: linux-hyperv

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.

Michael

^ permalink raw reply	[flat|nested] 3+ messages in thread