Checking Hyper-V hypercall status

Checking Hyper-V hypercall status

[Michael Kelley]

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?

Michael

Re: ** POTENTIAL FRAUD ALERT - RED HAT ** Checking Hyper-V hypercall status

[Vitaly Kuznetsov]

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;

-- 
Vitaly

Re: Checking Hyper-V hypercall status

[Wei Liu]

On Tue, Feb 09, 2021 at 03:45:23PM +0000, Michael Kelley wrote:
> 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?

I think this is definitely an improvement over open-coding everywhere.

Wei.