Field names inside ms_hyperv_info

Field names inside ms_hyperv_info

[Wei Liu]

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.

Thanks,
Wei.

Re: Field names inside ms_hyperv_info

[Vitaly Kuznetsov]

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

-- 
Vitaly

RE: Field names inside ms_hyperv_info

[Michael Kelley]

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.

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

Michael

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

[Vitaly Kuznetsov]

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

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

[Wei Liu]

On Fri, Oct 30, 2020 at 06:09:57PM +0100, Vitaly Kuznetsov wrote:
[...]
> > 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.
> 

Sure. I'm fine with this too.

Wei.

> -- 
> Vitaly
>