| Message ID | 20200210033953.99692-3-boqun.feng@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: hv: Generify pci-hyperv.c | expand |
> From: linux-hyperv-owner@vger.kernel.org > <linux-hyperv-owner@vger.kernel.org> 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.h > b/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> 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.
On Thu, Feb 13, 2020 at 04:17:34AM +0000, Dexuan Cui wrote: > > From: linux-hyperv-owner@vger.kernel.org > > <linux-hyperv-owner@vger.kernel.org> 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.h > > b/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
> From: Boqun Feng <boqun.feng@gmail.com> > Sent: Wednesday, February 12, 2020 11:26 PM > > > 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 Makes sense to me. Thanks for the explanation! Thanks, -- Dexuan
diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h index dffed0e10a68..a0b6a88d2f05 100644 --- a/arch/x86/include/asm/hyperv-tlfs.h +++ b/arch/x86/include/asm/hyperv-tlfs.h @@ -912,4 +912,35 @@ struct hv_tlb_flush_ex { struct hv_partition_assist_pg { u32 tlb_lock_count; }; + +struct hv_interrupt_entry { + u32 source; /* 1 for MSI(-X) */ + u32 reserved1; + u32 address; + u32 data; +} __packed; + +/* + * flags for hv_device_interrupt_target.flags + */ +#define HV_DEVICE_INTERRUPT_TARGET_MULTICAST 1 +#define HV_DEVICE_INTERRUPT_TARGET_PROCESSOR_SET 2 + +struct hv_device_interrupt_target { + u32 vector; + u32 flags; + union { + u64 vp_mask; + struct hv_vpset vp_set; + }; +} __packed; + +/* HvRetargetDeviceInterrupt hypercall */ +struct hv_retarget_device_interrupt { + u64 partition_id; /* use "self" */ + u64 device_id; + struct hv_interrupt_entry int_entry; + u64 reserved2; + struct hv_device_interrupt_target int_target; +} __packed __aligned(8); #endif diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c index aacfcc90d929..0d9b74503577 100644 --- a/drivers/pci/controller/pci-hyperv.c +++ b/drivers/pci/controller/pci-hyperv.c @@ -406,36 +406,6 @@ struct pci_eject_response { static int pci_ring_size = (4 * PAGE_SIZE); -struct hv_interrupt_entry { - u32 source; /* 1 for MSI(-X) */ - u32 reserved1; - u32 address; - u32 data; -}; - -/* - * flags for hv_device_interrupt_target.flags - */ -#define HV_DEVICE_INTERRUPT_TARGET_MULTICAST 1 -#define HV_DEVICE_INTERRUPT_TARGET_PROCESSOR_SET 2 - -struct hv_device_interrupt_target { - u32 vector; - u32 flags; - union { - u64 vp_mask; - struct hv_vpset vp_set; - }; -}; - -struct retarget_msi_interrupt { - u64 partition_id; /* use "self" */ - u64 device_id; - struct hv_interrupt_entry int_entry; - u64 reserved2; - struct hv_device_interrupt_target int_target; -} __packed __aligned(8); - /* * Driver specific state. */ @@ -482,7 +452,7 @@ struct hv_pcibus_device { struct workqueue_struct *wq; /* hypercall arg, must not cross page boundary */ - struct retarget_msi_interrupt retarget_msi_interrupt_params; + struct hv_retarget_device_interrupt retarget_msi_interrupt_params; /* * Don't put anything here: retarget_msi_interrupt_params must be last @@ -1178,7 +1148,7 @@ static void hv_irq_unmask(struct irq_data *data) { struct msi_desc *msi_desc = irq_data_get_msi_desc(data); struct irq_cfg *cfg = irqd_cfg(data); - struct retarget_msi_interrupt *params; + struct hv_retarget_device_interrupt *params; struct hv_pcibus_device *hbus; struct cpumask *dest; cpumask_var_t tmp;
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. [1]: https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/reference/tlfs Signed-off-by: Boqun Feng (Microsoft) <boqun.feng@gmail.com> --- arch/x86/include/asm/hyperv-tlfs.h | 31 ++++++++++++++++++++++++++ drivers/pci/controller/pci-hyperv.c | 34 ++--------------------------- 2 files changed, 33 insertions(+), 32 deletions(-)