Re: ** POTENTIAL FRAUD ALERT - RED HAT ** RE: Field names inside ms_hyperv_info

[Vitaly Kuznetsov <vkuznets@redhat.com>]

Michael Kelley <mikelley@microsoft.com> writes:

> From: Vitaly Kuznetsov <vkuznets@redhat.com>
>> 
>> Wei Liu <wei.liu@kernel.org> writes:
>> 
>> > Hi all
>> >
>> > During my work to make Linux boot as root partition on MSHV, I found out
>> > that a privilege mask was not collected in ms_hyperv_info.
>> >
>> > Looking at the code, the field names of ms_hyperv_info are not
>> > consistent with the names defined in TLFS. That makes it difficult to
>> > choose a name for the field that stores the privilege mask.
>> >
>> > I've got a local patch to make the existing fields closer to TLFS. The
>> > suffix "_a" means the value comes from EAX.
>> >
>> > Given that this structure is also used on ARM, so having x86 suffix is
>> > probably not the best idea. Do people care?
>> >
>> > diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
>> > index c57799684170..913af5e93cce 100644
>> > --- a/include/asm-generic/mshyperv.h
>> > +++ b/include/asm-generic/mshyperv.h
>> > @@ -26,9 +26,9 @@
>> >  #include <asm/hyperv-tlfs.h>
>> >
>> >  struct ms_hyperv_info {
>> > -       u32 features;
>> > -       u32 misc_features;
>> > -       u32 hints;
>> > +       u32 features_a;
>> > +       u32 features_d;
>> > +       u32 recommendations;
>> >         u32 nested_features;
>> >         u32 max_vp_index;
>> >         u32 max_lp_index;
>> >
>> > Any comment on this? I'm normally bad at naming things so any suggestion
>> > is welcomed.
>> 
>> My take: let's avoid ambiguous '_a', '_d' and use full register names,
>> it's only three letters after all. Let's also avoid suffix-less names as
>> eventually we'll need to add non-eax parts. That is:
>> 
>>        u32 features_eax;
>>        u32 features_edx;
>>        u32 recommendations_eax;
>>        u32 nested_features_eax;
>> ...
>> 
>> I would also feel comfortable with these names sortened,
>> 
>>        u32 feat_eax;
>>        u32 feat_edx;
>>        u32 recomm_eax;
>>        u32 nested_feat_eax;
>> ...
>> 
>
> This is in the asm-generic portion of mshyperv.h, so it is shared
> across the x86 and ARM64 architectures.  So I don't think we
> want x86 register names.  On ARM64, the eax/ebx/ecx/edx
> portions are retrieved all together in a single 128-bit register
> read.  I abstracted this into four 32-bit parts labeled
> "a", "b", "c", and "d" with the obvious mapping to
> eax/ebx/ecx/edx on the x86 side, but without using those names.
>

Oh, yea, I completely forgot about ARM! Using '_a'..'_d' suffixes is
justified then. 

> We don't have a TLFS for ARM64 (should be coming soon).  Might
> be worth seeing what naming, if any, will be used there.

In that case we should probably wait untill the naming there is clear as
I'm afraid we'll be renaming again.

-- 
Vitaly