On Mon, Feb 03, 2020 at 01:03:12PM +0800, Boqun Feng wrote: > 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. Therefore, this patch moves those definitions into x86's Nit: Rather than 'Therefore, this patch moves ...' - how about 'Let's move ...'? > tlfs header file to support virtual PCI on non-x86 architectures in the > future. > > Besides, while I'm at it, rename retarget_msi_interrupt to Nit: 'Besides, while I'm at it' - this type of wording describes what *you've* done rather than what the patch is doing. You could replace that quoted text with 'Additionally, ' > hv_retarget_msi_interrupt for the consistent name convention, also Nit: s/name/naming > 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@xxxxxxxxx> > --- > arch/x86/include/asm/hyperv-tlfs.h | 31 ++++++++++++++++++++++++++ > drivers/pci/controller/pci-hyperv.c | 34 ++--------------------------- > 2 files changed, 33 insertions(+), 32 deletions(-) > > diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h > index 739bd89226a5..4a76e442481a 100644 > --- a/arch/x86/include/asm/hyperv-tlfs.h > +++ b/arch/x86/include/asm/hyperv-tlfs.h > @@ -911,4 +911,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; Why have you added __packed here? There is no mention of this change in the commit log? Is it needed? > + > +/* > + * 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; Same here. > + > +/* HvRetargetDeviceInterrupt hypercall */ > +struct hv_retarget_device_interrupt { > + u64 partition_id; Why drop the 'self' comment? > + 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; pci-hyperv.c also makes use of retarget_msi_interrupt_lock - it's really clear from this name what it protects, however your rename now makes this more confusing. Likewise there is a comment in hv_pci_probe that refers to retarget_msi_interrupt_params which is now stale. It may be helpful to rename hv_retarget_device_interrupt for consistency with the docs - however please make sure you catch all the references - I'd suggest that the move and the rename are in different patches. Thanks, Andrew Murray > struct hv_pcibus_device *hbus; > struct cpumask *dest; > cpumask_var_t tmp; > -- > 2.24.1 >
