Re: [PATCH v3 2/3] PCI: hv: Move retarget related structures into tlfs header
From: Boqun Feng <hidden>
Date: 2020-02-13 07:26:30
Also in:
linux-arm-kernel, linux-pci, lkml
On Thu, Feb 13, 2020 at 04:17:34AM +0000, Dexuan Cui wrote:
quoted
From: linux-hyperv-owner@vger.kernel.org [off-list ref] On Behalf Of Boqun Feng Sent: Sunday, February 9, 2020 7:40 PM Currently, retarget_msi_interrupt and other structures it relys on are defined in pci-hyperv.c. However, those structures are actually defined in Hypervisor Top-Level Functional Specification [1] and may be different in sizes of fields or layout from architecture to architecture. Let's move those definitions into x86's tlfs header file to support virtual PCI on non-x86 architectures in the future. Note that "__packed" attribute is added to these structures during the movement for the same reason as we use the attribute for other TLFS structures in the header file: make sure the structures meet the specification and avoid anything unexpected from the compilers. Additionally, rename struct retarget_msi_interrupt to hv_retarget_msi_interrupt for the consistent naming convention, also mirroring the name in TLFS.diff --git a/arch/x86/include/asm/hyperv-tlfs.hb/arch/x86/include/asm/hyperv-tlfs.h + +struct hv_device_interrupt_target { + u32 vector; + u32 flags; + union { + u64 vp_mask; + struct hv_vpset vp_set; + }; +} __packed; + +/* HvRetargetDeviceInterrupt hypercall */Reviewed-by: Dexuan Cui <decui@microsoft.com>
Thanks!
Just a small thing: would it be slightly better if we change the name in the above line to HVCALL_RETARGET_INTERRUPT ? HVCALL_RETARGET_INTERRUPT is a define, so it may help to locate the actual value of the define here. And, HVCALL_RETARGET_INTERRUPT is used several times in the patchset so IMO we'd better always use the same name.
This might be a good suggestion, however, throughout the TLFS header, camel case is more commonly used for referencing hypercall. For example: /* HvCallSendSyntheticClusterIpi hypercall */ So I think it's better to let it as it is for this patch, and later on, if we reach a consensus, we can convert the names all together. Thoughts? Regards, Boqun