On Mon, Feb 03, 2020 at 09:41:18AM +0000, Andrew Murray wrote: > 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 > Thanks for the suggestion on wording ;-) > > 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? > I'm simply following the convention of hyperv-tlfs.h: most of the structures have this "__packed" attribute. I personally don't think this attribute is necessary, but I was afraid that I was missing something subtle. So a question for folks working on Hyper-V: why we need this attribute on TLFS-defined structures? Most of those will have no difference with or without this attribute, IIUC. > > + > > +/* > > + * 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? > Good catch, TLFS does say this field must be 'self'. I will add it in next version. > > + 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. > But 'retarget_msi_interrupt_params' is the name of field in hv_pcibus_device, so is 'retarget_msi_interrupt_lock'. And what I change is the name of type. I believe people can tell the relationship from the name of the fields, and the comment of hv_pci_probe actually refers to the field rather than the type. > 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. > If the renaming requires a lot of work (e.g. need to change multiple references), I will follow your suggestion. But seems it's not the case for this renaming. Regards, Boqun > Thanks, > > Andrew Murray > > > struct hv_pcibus_device *hbus; > > struct cpumask *dest; > > cpumask_var_t tmp; > > -- > > 2.24.1 > >
