On Mon, Feb 03, 2020 at 10:09:02PM +0800, Boqun Feng wrote: [...] > > > 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. > I find this patch: https://lore.kernel.org/lkml/20181212175701.18754-1-vkuznets@xxxxxxxxxx/ The reason why the "__packed" attribute is needed is to protect the hypervisor-guet communication structures from unexpected behaviors of compilers. I will keep the code as it is and add some words in the commit log. Regards, Boqun > > > + > > > +/* > > > + * 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 > > >
