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

[Vitaly Kuznetsov <vkuznets@redhat.com>]

Michael Kelley <mikelley@microsoft.com> writes:

> 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.
>

No problem with me, let's stay conservative and use helpers.

-- 
Vitaly