Re: [PATCH v2 3/4] hyperv: Add new Hyper-V headers in include/hyperv

[Nuno Das Neves <nunodasneves@linux.microsoft.com>]

On 11/11/2024 11:31 AM, Michael Kelley wrote:
> From: Nuno Das Neves <nunodasneves@linux.microsoft.com> Sent: Monday, November 11, 2024 10:45 AM
>>
>> On 11/10/2024 8:13 PM, Michael Kelley wrote:
>>> From: Nuno Das Neves <nunodasneves@linux.microsoft.com> Sent: Thursday,
>> November 7, 2024 2:32 PM
>>>>
>>>> These headers contain definitions for regular Hyper-V guests (as in
>>>> hyperv-tlfs.h), as well as interfaces for more privileged guests like
>>>> Dom0.
>>>
>>> See my comment on Patch 0/4 about use of "dom0" terminology.
>>>
>>
>> Thanks, noted.
>>
>>>>
>>>> These files are derived from headers exported from Hyper-V, rather than
>>>> being derived from the TLFS document. (Although, to preserve
>>>> compatibility with existing Linux code, some definitions are copied
>>>> directly from hyperv-tlfs.h too).
>>>>
>>>> The new files follow a naming convention according to their original
>>>> use:
>>>> - hdk "host development kit"
>>>> - gdk "guest development kit"
>>>> With postfix "_mini" implying userspace-only headers, and "_ext" for
>>>> extended hypercalls.
>>>>
>>>> These names should be considered a rough guide only - since there are
>>>> many places already where both host and guest code are in the same
>>>> place, hvhdk.h (which includes everything) can be used most of the time.
>>>
>>> Just curious -- are there really cases where hvhdk.h can't be used?
>>> If so, could you summarize why?
>>>
>>
>> No, there aren't cases where it "can't" be used. I suppose if someone
>> doesn't want to include everything, perhaps they could just include
>> hvgdk.h, for example. It doesn't really matter though.
>>
>>> I ask because it would be nice to expand slightly on your paragraph
>>> below, as follows:  (if indeed what I've added is correct)
>>>
>>> The use of multiple files and their original names is primarily to
>>> keep the provenance of exactly where they came from in Hyper-V
>>> code, which is helpful for manual maintenance and extension
>>> of these definitions. Microsoft maintainers importing new definitions
>>> should take care to put them in the right file. However, Linux kernel code
>>> that uses any of the definitions need not be aware of the multiple files
>>> or assign any meaning to the new names. Linux kernel uses should
>>> always just include hvhdk.h
>>>
>>
>> Thanks, I think that additional sentence helps clarify things. I'll
>> include it in the next version, and I think I can probably omit the prior
>> paragraph: "These names should be considered a rough guide only...".
>>
> 
> Omitting that prior paragraph is OK with me.  The key thoughts from my
> standpoint are:
> * The separation into multiple files and the file names come from
>    the Windows Hyper-V world and are maintained to ease bringing
>    the definitions over from that world
>    
> * Linux code can ignore the multiple files and their names. Just
>    #include hvhdk.h.
> 

Agreed, thanks for helping clarify the points.

>>>>
>>>> The original names are kept intact primarily to keep the provenance of
>>>> exactly where they came from in Hyper-V code, which is helpful for
>>>> manual maintenance and extension of these definitions. Microsoft
>>>> maintainers importing new definitions should take care to put them in
>>>> the right file.
>>>>
>>>> Note also that the files contain both arm64 and x86_64 code guarded by
>>>> \#ifdefs, which is how the definitions originally appear in Hyper-V.
>>>
>>> Spurious backslash?
>>>
>>
>> Indeed, thanks.
>>
>>> I would suggest some additional clarification:  The #ifdef guards are
>>> employed minimally where necessary to prevent conflicts due to
>>> different definitions for the same thing on x86_64 and arm64. Where
>>> there are no conflicts, the union of x86_64 definitions and arm64
>>> definitions is visible when building for either architecture. In other
>>> words, not all definitions specific to x86_64 are protected by #ifdef
>>> x86_64. Such unprotected definitions may be visible when building
>>> for arm64. And vice versa.
>>>
>>
>> Is there a reason you specifically want to point out that "Such
>> unprotected definitions may be visible when building for arm64. And vice
>> versa."? I think, in all the cases where #ifdefs are not used, an
>> arch-specific prefix is used - hv_x64_ or hv_arm64_.
>>
>> The main thing I wanted to call out here was the reasoning for not
>> splitting arch-specific definitions into separate files in arch/x86/
>> and arch/arm64/ as is typical in Linux.
>>
>> Maybe this is a bit clearer:
>> "
>> Note the new headers contain both arm64 and x86_64 definitions. Some are
>> guarded by #ifdefs, and some are instead prefixed with the architecture,
>> e.g. hv_x64_*. These conventions are kept from Hyper-V code as another
>> tactic to simplify the process of importing and maintaining the
>> definitions, rather than splitting them up into their own files in
>> arch/x86/ and arch/arm64/.
>> "
> 
> Yes, your new paragraph works for me. Your original statement was
> "the files contain both arm64 and x86_64 code guarded by #ifdefs",
> which sounds like the more typical Linux approach of using #ifdefs
> to segregate into x86-specific, arm64-specific, and common. I was
> just trying to be explicit that full segregation isn't done, and isn't a
> goal, because of wanting to maintain alignment with the original
> Hyper-V definitions.
> 
> It's "Hey, we know we're not handling this in the typical Linux way,
> and here's why". Your revised paragraph covers that in a less
> heavyweight way than what I wrote. :-)
> 

Ok, great. I'll use that for the next version then.

Thanks again!
Nuno

> Michael
> 
>>
>> I hope it's reasonably clear that it's a good tradeoff to go against
>> Linux convention in this case, to make it easy to import and maintain
>> Hyper-V definitions.
>>
>> Thanks
>> Nuno
>>