* 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@vger.kernel.org
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* RE: Checking Hyper-V hypercall status
2021-02-10 17:08 Checking Hyper-V hypercall status Michael Kelley
@ 2021-02-16 14:25 ` Michael Kelley
2021-02-16 14:55 ` ** POTENTIAL FRAUD ALERT - RED HAT ** " Vitaly Kuznetsov
0 siblings, 1 reply; 3+ messages in thread
From: Michael Kelley @ 2021-02-16 14:25 UTC (permalink / raw)
To: vkuznets; +Cc: linux-hyperv@vger.kernel.org
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
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: ** POTENTIAL FRAUD ALERT - RED HAT ** RE: Checking Hyper-V hypercall status
2021-02-16 14:25 ` Michael Kelley
@ 2021-02-16 14:55 ` Vitaly Kuznetsov
0 siblings, 0 replies; 3+ messages in thread
From: Vitaly Kuznetsov @ 2021-02-16 14:55 UTC (permalink / raw)
To: Michael Kelley; +Cc: linux-hyperv@vger.kernel.org
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-02-16 14:57 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-02-10 17:08 Checking Hyper-V hypercall status Michael Kelley
2021-02-16 14:25 ` Michael Kelley
2021-02-16 14:55 ` ** POTENTIAL FRAUD ALERT - RED HAT ** " Vitaly Kuznetsov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).