Message-ID: <6d2a6bd4-a7cf-4672-9fb0-975acdc8ed31@linux.microsoft.com>
Date: Wed, 13 Nov 2024 15:32:32 -0800
From: Nuno Das Neves <nunodasneves@...ux.microsoft.com>
To: Michael Kelley <mhklinux@...look.com>,
 "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
 "linux-arm-kernel@...ts.infradead.org"
 <linux-arm-kernel@...ts.infradead.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
 "iommu@...ts.linux.dev" <iommu@...ts.linux.dev>,
 "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
 "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
 "linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>,
 "virtualization@...ts.linux.dev" <virtualization@...ts.linux.dev>
Cc: "kys@...rosoft.com" <kys@...rosoft.com>,
 "haiyangz@...rosoft.com" <haiyangz@...rosoft.com>,
 "wei.liu@...nel.org" <wei.liu@...nel.org>,
 "decui@...rosoft.com" <decui@...rosoft.com>,
 "catalin.marinas@....com" <catalin.marinas@....com>,
 "will@...nel.org" <will@...nel.org>, "luto@...nel.org" <luto@...nel.org>,
 "tglx@...utronix.de" <tglx@...utronix.de>,
 "mingo@...hat.com" <mingo@...hat.com>, "bp@...en8.de" <bp@...en8.de>,
 "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
 "x86@...nel.org" <x86@...nel.org>, "hpa@...or.com" <hpa@...or.com>,
 "seanjc@...gle.com" <seanjc@...gle.com>,
 "pbonzini@...hat.com" <pbonzini@...hat.com>,
 "peterz@...radead.org" <peterz@...radead.org>,
 "daniel.lezcano@...aro.org" <daniel.lezcano@...aro.org>,
 "joro@...tes.org" <joro@...tes.org>,
 "robin.murphy@....com" <robin.murphy@....com>,
 "davem@...emloft.net" <davem@...emloft.net>,
 "edumazet@...gle.com" <edumazet@...gle.com>,
 "kuba@...nel.org" <kuba@...nel.org>, "pabeni@...hat.com"
 <pabeni@...hat.com>, "lpieralisi@...nel.org" <lpieralisi@...nel.org>,
 "kw@...ux.com" <kw@...ux.com>, "robh@...nel.org" <robh@...nel.org>,
 "bhelgaas@...gle.com" <bhelgaas@...gle.com>, "arnd@...db.de"
 <arnd@...db.de>, "sgarzare@...hat.com" <sgarzare@...hat.com>,
 "jinankjain@...ux.microsoft.com" <jinankjain@...ux.microsoft.com>,
 "muminulrussell@...il.com" <muminulrussell@...il.com>,
 "skinsburskii@...ux.microsoft.com" <skinsburskii@...ux.microsoft.com>,
 "mukeshrathor@...rosoft.com" <mukeshrathor@...rosoft.com>,
 "vkuznets@...hat.com" <vkuznets@...hat.com>,
 "ssengar@...ux.microsoft.com" <ssengar@...ux.microsoft.com>,
 "apais@...ux.microsoft.com" <apais@...ux.microsoft.com>
Subject: Re: [PATCH v2 3/4] hyperv: Add new Hyper-V headers in include/hyperv

On 11/11/2024 11:31 AM, Michael Kelley wrote:
> From: Nuno Das Neves <nunodasneves@...ux.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@...ux.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
>>

Powered by blists - more mailing lists