* [PATCH v2 0/3] PCI: hv: Generify pci-hyperv.c @ 2020-02-03 5:03 Boqun Feng 2020-02-03 5:03 ` [PATCH v2 1/3] PCI: hv: Move hypercall related definitions into tlfs header Boqun Feng ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: Boqun Feng @ 2020-02-03 5:03 UTC (permalink / raw) To: linux-pci, linux-hyperv, linux-kernel, linux-arm-kernel Cc: Michael Kelley, K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Sasha Levin, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86, Lorenzo Pieralisi, Andrew Murray, Bjorn Helgaas, Boqun Feng Hi, This is the first part for virtual PCI support of Hyper-V guest on ARM64. The whole patchset doesn't have any functional change, but only refactors the pci-hyperv.c code to make it more arch-independent. Previous version: v1: https://lore.kernel.org/lkml/20200121015713.69691-1-boqun.feng@gmail.com/ Changes since v1: * Reword the commit log and adjust the title as per Bjorn's suggestion * Split patch #2 into two patches (one for moving and one for adding new structure) as per Bjorn's suggestion * Remove unnecesarry #if guard as per Vitaly's suggestion. * Add explanation for adding hv_set_msi_address_from_desc(). I've done compile and boot test of this patchset, also done some tests with a pass-through NVMe device. Suggestions and comments are welcome! Regards, Boqun Boqun Feng (3): PCI: hv: Move hypercall related definitions into tlfs header PCI: hv: Move retarget related structures into tlfs header PCI: hv: Introduce hv_msi_entry arch/x86/include/asm/hyperv-tlfs.h | 41 +++++++++++++++++++++++++++ arch/x86/include/asm/mshyperv.h | 5 ++++ drivers/pci/controller/pci-hyperv.c | 44 +++-------------------------- 3 files changed, 50 insertions(+), 40 deletions(-) -- 2.24.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 1/3] PCI: hv: Move hypercall related definitions into tlfs header 2020-02-03 5:03 [PATCH v2 0/3] PCI: hv: Generify pci-hyperv.c Boqun Feng @ 2020-02-03 5:03 ` Boqun Feng 2020-02-03 9:25 ` Andrew Murray 2020-02-03 5:03 ` [PATCH v2 2/3] PCI: hv: Move retarget related structures " Boqun Feng 2020-02-03 5:03 ` [PATCH v2 3/3] PCI: hv: Introduce hv_msi_entry Boqun Feng 2 siblings, 1 reply; 13+ messages in thread From: Boqun Feng @ 2020-02-03 5:03 UTC (permalink / raw) To: linux-pci, linux-hyperv, linux-kernel, linux-arm-kernel Cc: Michael Kelley, K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Sasha Levin, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86, Lorenzo Pieralisi, Andrew Murray, Bjorn Helgaas, Boqun Feng Currently HVCALL_RETARGET_INTERRUPT and HV_PARTITION_ID_SELF are defined in pci-hyperv.c. However, similar to other hypercall related definitions , it makes more sense to put them in the tlfs header file. Besides, these definitions are arch-dependent, so for the support of virtual PCI on non-x86 archs in the future, move them into arch-specific tlfs header file. Signed-off-by: Boqun Feng (Microsoft) <boqun.feng@gmail.com> --- arch/x86/include/asm/hyperv-tlfs.h | 3 +++ drivers/pci/controller/pci-hyperv.c | 6 ------ 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h index 5f10f7f2098d..739bd89226a5 100644 --- a/arch/x86/include/asm/hyperv-tlfs.h +++ b/arch/x86/include/asm/hyperv-tlfs.h @@ -376,6 +376,7 @@ struct hv_tsc_emulation_status { #define HVCALL_SEND_IPI_EX 0x0015 #define HVCALL_POST_MESSAGE 0x005c #define HVCALL_SIGNAL_EVENT 0x005d +#define HVCALL_RETARGET_INTERRUPT 0x007e #define HVCALL_FLUSH_GUEST_PHYSICAL_ADDRESS_SPACE 0x00af #define HVCALL_FLUSH_GUEST_PHYSICAL_ADDRESS_LIST 0x00b0 @@ -405,6 +406,8 @@ enum HV_GENERIC_SET_FORMAT { HV_GENERIC_SET_ALL, }; +#define HV_PARTITION_ID_SELF ((u64)-1) + #define HV_HYPERCALL_RESULT_MASK GENMASK_ULL(15, 0) #define HV_HYPERCALL_FAST_BIT BIT(16) #define HV_HYPERCALL_VARHEAD_OFFSET 17 diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c index 9977abff92fc..aacfcc90d929 100644 --- a/drivers/pci/controller/pci-hyperv.c +++ b/drivers/pci/controller/pci-hyperv.c @@ -406,12 +406,6 @@ struct pci_eject_response { static int pci_ring_size = (4 * PAGE_SIZE); -/* - * Definitions or interrupt steering hypercall. - */ -#define HV_PARTITION_ID_SELF ((u64)-1) -#define HVCALL_RETARGET_INTERRUPT 0x7e - struct hv_interrupt_entry { u32 source; /* 1 for MSI(-X) */ u32 reserved1; -- 2.24.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/3] PCI: hv: Move hypercall related definitions into tlfs header 2020-02-03 5:03 ` [PATCH v2 1/3] PCI: hv: Move hypercall related definitions into tlfs header Boqun Feng @ 2020-02-03 9:25 ` Andrew Murray 2020-02-04 2:22 ` Boqun Feng 0 siblings, 1 reply; 13+ messages in thread From: Andrew Murray @ 2020-02-03 9:25 UTC (permalink / raw) To: Boqun Feng Cc: linux-pci, linux-hyperv, linux-kernel, linux-arm-kernel, Sasha Levin, Lorenzo Pieralisi, Stephen Hemminger, Haiyang Zhang, x86, Michael Kelley, Ingo Molnar, Borislav Petkov, H. Peter Anvin, Bjorn Helgaas, Andrew Murray, Thomas Gleixner, K. Y. Srinivasan On Mon, Feb 03, 2020 at 01:03:11PM +0800, Boqun Feng wrote: > Currently HVCALL_RETARGET_INTERRUPT and HV_PARTITION_ID_SELF are defined > in pci-hyperv.c. However, similar to other hypercall related definitions > , it makes more sense to put them in the tlfs header file. Nit: please keep the comma attached to the previous word - even if that means you need to move the word with it to the next line to maintain line limits. > > Besides, these definitions are arch-dependent, so for the support of > virtual PCI on non-x86 archs in the future, move them into arch-specific > tlfs header file. > > Signed-off-by: Boqun Feng (Microsoft) <boqun.feng@gmail.com> > --- > arch/x86/include/asm/hyperv-tlfs.h | 3 +++ > drivers/pci/controller/pci-hyperv.c | 6 ------ > 2 files changed, 3 insertions(+), 6 deletions(-) > > diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h > index 5f10f7f2098d..739bd89226a5 100644 > --- a/arch/x86/include/asm/hyperv-tlfs.h > +++ b/arch/x86/include/asm/hyperv-tlfs.h > @@ -376,6 +376,7 @@ struct hv_tsc_emulation_status { > #define HVCALL_SEND_IPI_EX 0x0015 > #define HVCALL_POST_MESSAGE 0x005c > #define HVCALL_SIGNAL_EVENT 0x005d > +#define HVCALL_RETARGET_INTERRUPT 0x007e > #define HVCALL_FLUSH_GUEST_PHYSICAL_ADDRESS_SPACE 0x00af > #define HVCALL_FLUSH_GUEST_PHYSICAL_ADDRESS_LIST 0x00b0 > > @@ -405,6 +406,8 @@ enum HV_GENERIC_SET_FORMAT { > HV_GENERIC_SET_ALL, > }; > > +#define HV_PARTITION_ID_SELF ((u64)-1) > + > #define HV_HYPERCALL_RESULT_MASK GENMASK_ULL(15, 0) > #define HV_HYPERCALL_FAST_BIT BIT(16) > #define HV_HYPERCALL_VARHEAD_OFFSET 17 > diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c > index 9977abff92fc..aacfcc90d929 100644 > --- a/drivers/pci/controller/pci-hyperv.c > +++ b/drivers/pci/controller/pci-hyperv.c > @@ -406,12 +406,6 @@ struct pci_eject_response { > > static int pci_ring_size = (4 * PAGE_SIZE); > > -/* > - * Definitions or interrupt steering hypercall. > - */ > -#define HV_PARTITION_ID_SELF ((u64)-1) > -#define HVCALL_RETARGET_INTERRUPT 0x7e > - Reviewed-by: Andrew Murray <amurray@thegoodpenguin.co.uk> > struct hv_interrupt_entry { > u32 source; /* 1 for MSI(-X) */ > u32 reserved1; > -- > 2.24.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/3] PCI: hv: Move hypercall related definitions into tlfs header 2020-02-03 9:25 ` Andrew Murray @ 2020-02-04 2:22 ` Boqun Feng 0 siblings, 0 replies; 13+ messages in thread From: Boqun Feng @ 2020-02-04 2:22 UTC (permalink / raw) To: Andrew Murray Cc: linux-pci, linux-hyperv, linux-kernel, linux-arm-kernel, Sasha Levin, Lorenzo Pieralisi, Stephen Hemminger, Haiyang Zhang, x86, Michael Kelley, Ingo Molnar, Borislav Petkov, H. Peter Anvin, Bjorn Helgaas, Andrew Murray, Thomas Gleixner, K. Y. Srinivasan On Mon, Feb 03, 2020 at 09:25:25AM +0000, Andrew Murray wrote: > On Mon, Feb 03, 2020 at 01:03:11PM +0800, Boqun Feng wrote: > > Currently HVCALL_RETARGET_INTERRUPT and HV_PARTITION_ID_SELF are defined > > in pci-hyperv.c. However, similar to other hypercall related definitions > > , it makes more sense to put them in the tlfs header file. > > Nit: please keep the comma attached to the previous word - even if that > means you need to move the word with it to the next line to maintain line > limits. > > > > > Besides, these definitions are arch-dependent, so for the support of > > virtual PCI on non-x86 archs in the future, move them into arch-specific > > tlfs header file. > > > > Signed-off-by: Boqun Feng (Microsoft) <boqun.feng@gmail.com> > > --- > > arch/x86/include/asm/hyperv-tlfs.h | 3 +++ > > drivers/pci/controller/pci-hyperv.c | 6 ------ > > 2 files changed, 3 insertions(+), 6 deletions(-) > > > > diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h > > index 5f10f7f2098d..739bd89226a5 100644 > > --- a/arch/x86/include/asm/hyperv-tlfs.h > > +++ b/arch/x86/include/asm/hyperv-tlfs.h > > @@ -376,6 +376,7 @@ struct hv_tsc_emulation_status { > > #define HVCALL_SEND_IPI_EX 0x0015 > > #define HVCALL_POST_MESSAGE 0x005c > > #define HVCALL_SIGNAL_EVENT 0x005d > > +#define HVCALL_RETARGET_INTERRUPT 0x007e > > #define HVCALL_FLUSH_GUEST_PHYSICAL_ADDRESS_SPACE 0x00af > > #define HVCALL_FLUSH_GUEST_PHYSICAL_ADDRESS_LIST 0x00b0 > > > > @@ -405,6 +406,8 @@ enum HV_GENERIC_SET_FORMAT { > > HV_GENERIC_SET_ALL, > > }; > > > > +#define HV_PARTITION_ID_SELF ((u64)-1) > > + > > #define HV_HYPERCALL_RESULT_MASK GENMASK_ULL(15, 0) > > #define HV_HYPERCALL_FAST_BIT BIT(16) > > #define HV_HYPERCALL_VARHEAD_OFFSET 17 > > diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c > > index 9977abff92fc..aacfcc90d929 100644 > > --- a/drivers/pci/controller/pci-hyperv.c > > +++ b/drivers/pci/controller/pci-hyperv.c > > @@ -406,12 +406,6 @@ struct pci_eject_response { > > > > static int pci_ring_size = (4 * PAGE_SIZE); > > > > -/* > > - * Definitions or interrupt steering hypercall. > > - */ > > -#define HV_PARTITION_ID_SELF ((u64)-1) > > -#define HVCALL_RETARGET_INTERRUPT 0x7e > > - > > Reviewed-by: Andrew Murray <amurray@thegoodpenguin.co.uk> > Thanks! I will fix the comma thing and add your Reviewed-by in next version. Regards, Boqun > > struct hv_interrupt_entry { > > u32 source; /* 1 for MSI(-X) */ > > u32 reserved1; > > -- > > 2.24.1 > > > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 2/3] PCI: hv: Move retarget related structures into tlfs header 2020-02-03 5:03 [PATCH v2 0/3] PCI: hv: Generify pci-hyperv.c Boqun Feng 2020-02-03 5:03 ` [PATCH v2 1/3] PCI: hv: Move hypercall related definitions into tlfs header Boqun Feng @ 2020-02-03 5:03 ` Boqun Feng 2020-02-03 9:41 ` Andrew Murray 2020-02-03 5:03 ` [PATCH v2 3/3] PCI: hv: Introduce hv_msi_entry Boqun Feng 2 siblings, 1 reply; 13+ messages in thread From: Boqun Feng @ 2020-02-03 5:03 UTC (permalink / raw) To: linux-pci, linux-hyperv, linux-kernel, linux-arm-kernel Cc: Michael Kelley, K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Sasha Levin, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86, Lorenzo Pieralisi, Andrew Murray, Bjorn Helgaas, Boqun Feng 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 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 hv_retarget_msi_interrupt for the consistent name 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(-) 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; + +/* + * 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; + 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; -- 2.24.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/3] PCI: hv: Move retarget related structures into tlfs header 2020-02-03 5:03 ` [PATCH v2 2/3] PCI: hv: Move retarget related structures " Boqun Feng @ 2020-02-03 9:41 ` Andrew Murray 2020-02-03 14:09 ` Boqun Feng 0 siblings, 1 reply; 13+ messages in thread From: Andrew Murray @ 2020-02-03 9:41 UTC (permalink / raw) To: Boqun Feng Cc: linux-pci, linux-hyperv, linux-kernel, linux-arm-kernel, Michael Kelley, K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Sasha Levin, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86, Lorenzo Pieralisi, Andrew Murray, Bjorn Helgaas 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@gmail.com> > --- > 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 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/3] PCI: hv: Move retarget related structures into tlfs header 2020-02-03 9:41 ` Andrew Murray @ 2020-02-03 14:09 ` Boqun Feng 2020-02-07 7:58 ` Boqun Feng 0 siblings, 1 reply; 13+ messages in thread From: Boqun Feng @ 2020-02-03 14:09 UTC (permalink / raw) To: Andrew Murray Cc: linux-pci, linux-hyperv, linux-kernel, linux-arm-kernel, Michael Kelley, K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Sasha Levin, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86, Lorenzo Pieralisi, Andrew Murray, Bjorn Helgaas 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@gmail.com> > > --- > > 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 > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/3] PCI: hv: Move retarget related structures into tlfs header 2020-02-03 14:09 ` Boqun Feng @ 2020-02-07 7:58 ` Boqun Feng 0 siblings, 0 replies; 13+ messages in thread From: Boqun Feng @ 2020-02-07 7:58 UTC (permalink / raw) To: Andrew Murray Cc: linux-pci, linux-hyperv, linux-kernel, linux-arm-kernel, Michael Kelley, K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Sasha Levin, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86, Lorenzo Pieralisi, Andrew Murray, Bjorn Helgaas 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@gmail.com> > > > --- > > > 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@redhat.com/ 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 > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 3/3] PCI: hv: Introduce hv_msi_entry 2020-02-03 5:03 [PATCH v2 0/3] PCI: hv: Generify pci-hyperv.c Boqun Feng 2020-02-03 5:03 ` [PATCH v2 1/3] PCI: hv: Move hypercall related definitions into tlfs header Boqun Feng 2020-02-03 5:03 ` [PATCH v2 2/3] PCI: hv: Move retarget related structures " Boqun Feng @ 2020-02-03 5:03 ` Boqun Feng 2020-02-03 9:51 ` Andrew Murray 2020-02-03 14:41 ` Thomas Gleixner 2 siblings, 2 replies; 13+ messages in thread From: Boqun Feng @ 2020-02-03 5:03 UTC (permalink / raw) To: linux-pci, linux-hyperv, linux-kernel, linux-arm-kernel Cc: Michael Kelley, K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Sasha Levin, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86, Lorenzo Pieralisi, Andrew Murray, Bjorn Helgaas, Boqun Feng Add a new structure (hv_msi_entry), which is also defined int tlfs, to describe the msi entry for HVCALL_RETARGET_INTERRUPT. The structure is needed because its layout may be different from architecture to architecture. Also add a new generic interface hv_set_msi_address_from_desc() to allow different archs to set the msi address from msi_desc. No functional change, only preparation for the future support of virtual PCI on non-x86 architectures. Signed-off-by: Boqun Feng (Microsoft) <boqun.feng@gmail.com> --- arch/x86/include/asm/hyperv-tlfs.h | 11 +++++++++-- arch/x86/include/asm/mshyperv.h | 5 +++++ drivers/pci/controller/pci-hyperv.c | 4 ++-- 3 files changed, 16 insertions(+), 4 deletions(-) diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h index 4a76e442481a..953b3ad38746 100644 --- a/arch/x86/include/asm/hyperv-tlfs.h +++ b/arch/x86/include/asm/hyperv-tlfs.h @@ -912,11 +912,18 @@ struct hv_partition_assist_pg { u32 tlb_lock_count; }; +union hv_msi_entry { + u64 as_uint64; + struct { + u32 address; + u32 data; + } __packed; +}; + struct hv_interrupt_entry { u32 source; /* 1 for MSI(-X) */ u32 reserved1; - u32 address; - u32 data; + union hv_msi_entry msi_entry; } __packed; /* diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h index 6b79515abb82..3bdaa3b6e68f 100644 --- a/arch/x86/include/asm/mshyperv.h +++ b/arch/x86/include/asm/mshyperv.h @@ -240,6 +240,11 @@ bool hv_vcpu_is_preempted(int vcpu); static inline void hv_apic_init(void) {} #endif +#define hv_set_msi_address_from_desc(msi_entry, msi_desc) \ +do { \ + (msi_entry)->address = (msi_desc)->msg.address_lo; \ +} while (0) + #else /* CONFIG_HYPERV */ static inline void hyperv_init(void) {} static inline void hyperv_setup_mmu_ops(void) {} diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c index 0d9b74503577..2240f2b3643e 100644 --- a/drivers/pci/controller/pci-hyperv.c +++ b/drivers/pci/controller/pci-hyperv.c @@ -1170,8 +1170,8 @@ static void hv_irq_unmask(struct irq_data *data) memset(params, 0, sizeof(*params)); params->partition_id = HV_PARTITION_ID_SELF; params->int_entry.source = 1; /* MSI(-X) */ - params->int_entry.address = msi_desc->msg.address_lo; - params->int_entry.data = msi_desc->msg.data; + hv_set_msi_address_from_desc(¶ms->int_entry.msi_entry, msi_desc); + params->int_entry.msi_entry.data = msi_desc->msg.data; params->device_id = (hbus->hdev->dev_instance.b[5] << 24) | (hbus->hdev->dev_instance.b[4] << 16) | (hbus->hdev->dev_instance.b[7] << 8) | -- 2.24.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/3] PCI: hv: Introduce hv_msi_entry 2020-02-03 5:03 ` [PATCH v2 3/3] PCI: hv: Introduce hv_msi_entry Boqun Feng @ 2020-02-03 9:51 ` Andrew Murray 2020-02-03 14:35 ` Boqun Feng 2020-02-03 14:41 ` Thomas Gleixner 1 sibling, 1 reply; 13+ messages in thread From: Andrew Murray @ 2020-02-03 9:51 UTC (permalink / raw) To: Boqun Feng Cc: linux-pci, linux-hyperv, linux-kernel, linux-arm-kernel, Michael Kelley, K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Sasha Levin, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86, Lorenzo Pieralisi, Andrew Murray, Bjorn Helgaas On Mon, Feb 03, 2020 at 01:03:13PM +0800, Boqun Feng wrote: > Add a new structure (hv_msi_entry), which is also defined int tlfs, to s/int/in the/ ? > describe the msi entry for HVCALL_RETARGET_INTERRUPT. The structure is > needed because its layout may be different from architecture to > architecture. > > Also add a new generic interface hv_set_msi_address_from_desc() to allow > different archs to set the msi address from msi_desc. > > No functional change, only preparation for the future support of virtual > PCI on non-x86 architectures. > > Signed-off-by: Boqun Feng (Microsoft) <boqun.feng@gmail.com> > --- > arch/x86/include/asm/hyperv-tlfs.h | 11 +++++++++-- > arch/x86/include/asm/mshyperv.h | 5 +++++ > drivers/pci/controller/pci-hyperv.c | 4 ++-- > 3 files changed, 16 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h > index 4a76e442481a..953b3ad38746 100644 > --- a/arch/x86/include/asm/hyperv-tlfs.h > +++ b/arch/x86/include/asm/hyperv-tlfs.h > @@ -912,11 +912,18 @@ struct hv_partition_assist_pg { > u32 tlb_lock_count; > }; > > +union hv_msi_entry { > + u64 as_uint64; > + struct { > + u32 address; > + u32 data; > + } __packed; > +}; > + > struct hv_interrupt_entry { > u32 source; /* 1 for MSI(-X) */ > u32 reserved1; > - u32 address; > - u32 data; > + union hv_msi_entry msi_entry; > } __packed; > > /* > diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h > index 6b79515abb82..3bdaa3b6e68f 100644 > --- a/arch/x86/include/asm/mshyperv.h > +++ b/arch/x86/include/asm/mshyperv.h > @@ -240,6 +240,11 @@ bool hv_vcpu_is_preempted(int vcpu); > static inline void hv_apic_init(void) {} > #endif > > +#define hv_set_msi_address_from_desc(msi_entry, msi_desc) \ > +do { \ > + (msi_entry)->address = (msi_desc)->msg.address_lo; \ > +} while (0) Given that this is a single statement, is there really a need for the do ; while(0) ? > + > #else /* CONFIG_HYPERV */ > static inline void hyperv_init(void) {} > static inline void hyperv_setup_mmu_ops(void) {} > diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c > index 0d9b74503577..2240f2b3643e 100644 > --- a/drivers/pci/controller/pci-hyperv.c > +++ b/drivers/pci/controller/pci-hyperv.c > @@ -1170,8 +1170,8 @@ static void hv_irq_unmask(struct irq_data *data) > memset(params, 0, sizeof(*params)); > params->partition_id = HV_PARTITION_ID_SELF; > params->int_entry.source = 1; /* MSI(-X) */ > - params->int_entry.address = msi_desc->msg.address_lo; > - params->int_entry.data = msi_desc->msg.data; > + hv_set_msi_address_from_desc(¶ms->int_entry.msi_entry, msi_desc); > + params->int_entry.msi_entry.data = msi_desc->msg.data; If the layout may differ, then don't we also need a wrapper for data? Thanks, Andrew Murray > params->device_id = (hbus->hdev->dev_instance.b[5] << 24) | > (hbus->hdev->dev_instance.b[4] << 16) | > (hbus->hdev->dev_instance.b[7] << 8) | > -- > 2.24.1 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/3] PCI: hv: Introduce hv_msi_entry 2020-02-03 9:51 ` Andrew Murray @ 2020-02-03 14:35 ` Boqun Feng 0 siblings, 0 replies; 13+ messages in thread From: Boqun Feng @ 2020-02-03 14:35 UTC (permalink / raw) To: Andrew Murray Cc: linux-pci, linux-hyperv, linux-kernel, linux-arm-kernel, Michael Kelley, K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Sasha Levin, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86, Lorenzo Pieralisi, Andrew Murray, Bjorn Helgaas On Mon, Feb 03, 2020 at 09:51:40AM +0000, Andrew Murray wrote: > On Mon, Feb 03, 2020 at 01:03:13PM +0800, Boqun Feng wrote: > > Add a new structure (hv_msi_entry), which is also defined int tlfs, to > > s/int/in the/ ? > Good catch, will fix. > > describe the msi entry for HVCALL_RETARGET_INTERRUPT. The structure is > > needed because its layout may be different from architecture to > > architecture. > > > > Also add a new generic interface hv_set_msi_address_from_desc() to allow > > different archs to set the msi address from msi_desc. > > > > No functional change, only preparation for the future support of virtual > > PCI on non-x86 architectures. > > > > Signed-off-by: Boqun Feng (Microsoft) <boqun.feng@gmail.com> > > --- > > arch/x86/include/asm/hyperv-tlfs.h | 11 +++++++++-- > > arch/x86/include/asm/mshyperv.h | 5 +++++ > > drivers/pci/controller/pci-hyperv.c | 4 ++-- > > 3 files changed, 16 insertions(+), 4 deletions(-) > > > > diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h > > index 4a76e442481a..953b3ad38746 100644 > > --- a/arch/x86/include/asm/hyperv-tlfs.h > > +++ b/arch/x86/include/asm/hyperv-tlfs.h > > @@ -912,11 +912,18 @@ struct hv_partition_assist_pg { > > u32 tlb_lock_count; > > }; > > > > +union hv_msi_entry { > > + u64 as_uint64; > > + struct { > > + u32 address; > > + u32 data; > > + } __packed; > > +}; > > + > > struct hv_interrupt_entry { > > u32 source; /* 1 for MSI(-X) */ > > u32 reserved1; > > - u32 address; > > - u32 data; > > + union hv_msi_entry msi_entry; > > } __packed; > > > > /* > > diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h > > index 6b79515abb82..3bdaa3b6e68f 100644 > > --- a/arch/x86/include/asm/mshyperv.h > > +++ b/arch/x86/include/asm/mshyperv.h > > @@ -240,6 +240,11 @@ bool hv_vcpu_is_preempted(int vcpu); > > static inline void hv_apic_init(void) {} > > #endif > > > > +#define hv_set_msi_address_from_desc(msi_entry, msi_desc) \ > > +do { \ > > + (msi_entry)->address = (msi_desc)->msg.address_lo; \ > > +} while (0) > > Given that this is a single statement, is there really a need for the do ; while(0) ? > I choose to use do ; while (0) because I don't want code like the following to be able to compile: hv_set_msi_address_from_desc(...) /* semicolon is missing */ a = b; But now think more about this, I think it's probably better to define this as a function.. > > > + > > #else /* CONFIG_HYPERV */ > > static inline void hyperv_init(void) {} > > static inline void hyperv_setup_mmu_ops(void) {} > > diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c > > index 0d9b74503577..2240f2b3643e 100644 > > --- a/drivers/pci/controller/pci-hyperv.c > > +++ b/drivers/pci/controller/pci-hyperv.c > > @@ -1170,8 +1170,8 @@ static void hv_irq_unmask(struct irq_data *data) > > memset(params, 0, sizeof(*params)); > > params->partition_id = HV_PARTITION_ID_SELF; > > params->int_entry.source = 1; /* MSI(-X) */ > > - params->int_entry.address = msi_desc->msg.address_lo; > > - params->int_entry.data = msi_desc->msg.data; > > + hv_set_msi_address_from_desc(¶ms->int_entry.msi_entry, msi_desc); > > + params->int_entry.msi_entry.data = msi_desc->msg.data; > > If the layout may differ, then don't we also need a wrapper for data? > On x86 hv_msi_entry is: { u32 address; u32 data; } and on ARM64 it is: { u64 address; u32 data; u32 reserved; } So currently, setting msi_entry.data doesn't need a wrapper for different archs. But now you mention it, probably a better way is to provide a wrapper hv_set_msi_entry_from_desc(), which sets both address and data instead of hv_set_msi_address_from_desc(). Thanks for looking into the whole patchset! Regards, Boqun > Thanks, > > Andrew Murray > > > params->device_id = (hbus->hdev->dev_instance.b[5] << 24) | > > (hbus->hdev->dev_instance.b[4] << 16) | > > (hbus->hdev->dev_instance.b[7] << 8) | > > -- > > 2.24.1 > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/3] PCI: hv: Introduce hv_msi_entry 2020-02-03 5:03 ` [PATCH v2 3/3] PCI: hv: Introduce hv_msi_entry Boqun Feng 2020-02-03 9:51 ` Andrew Murray @ 2020-02-03 14:41 ` Thomas Gleixner 2020-02-04 2:13 ` Boqun Feng 1 sibling, 1 reply; 13+ messages in thread From: Thomas Gleixner @ 2020-02-03 14:41 UTC (permalink / raw) To: Boqun Feng, linux-pci, linux-hyperv, linux-kernel, linux-arm-kernel Cc: Michael Kelley, K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Sasha Levin, Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86, Lorenzo Pieralisi, Andrew Murray, Bjorn Helgaas, Boqun Feng Boqun Feng <boqun.feng@gmail.com> writes: > /* > diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h > index 6b79515abb82..3bdaa3b6e68f 100644 > --- a/arch/x86/include/asm/mshyperv.h > +++ b/arch/x86/include/asm/mshyperv.h > @@ -240,6 +240,11 @@ bool hv_vcpu_is_preempted(int vcpu); > static inline void hv_apic_init(void) {} > #endif > > +#define hv_set_msi_address_from_desc(msi_entry, msi_desc) \ > +do { \ > + (msi_entry)->address = (msi_desc)->msg.address_lo; \ > +} while (0) Any reason why this needs to be a macro? inlines are preferrred. They are typesafe and readable. Thanks, tglx ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/3] PCI: hv: Introduce hv_msi_entry 2020-02-03 14:41 ` Thomas Gleixner @ 2020-02-04 2:13 ` Boqun Feng 0 siblings, 0 replies; 13+ messages in thread From: Boqun Feng @ 2020-02-04 2:13 UTC (permalink / raw) To: Thomas Gleixner Cc: linux-pci, linux-hyperv, linux-kernel, linux-arm-kernel, Michael Kelley, K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Sasha Levin, Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86, Lorenzo Pieralisi, Andrew Murray, Bjorn Helgaas On Mon, Feb 03, 2020 at 03:41:52PM +0100, Thomas Gleixner wrote: > Boqun Feng <boqun.feng@gmail.com> writes: > > /* > > diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h > > index 6b79515abb82..3bdaa3b6e68f 100644 > > --- a/arch/x86/include/asm/mshyperv.h > > +++ b/arch/x86/include/asm/mshyperv.h > > @@ -240,6 +240,11 @@ bool hv_vcpu_is_preempted(int vcpu); > > static inline void hv_apic_init(void) {} > > #endif > > > > +#define hv_set_msi_address_from_desc(msi_entry, msi_desc) \ > > +do { \ > > + (msi_entry)->address = (msi_desc)->msg.address_lo; \ > > +} while (0) > > Any reason why this needs to be a macro? inlines are preferrred. They Making it an inline function will require #include <linux/msi.h> in mshyperv.h, which I was trying to avoid. But now it seems pointless. I will make it an inline in next version. Regards, Boqun > are typesafe and readable. > > Thanks, > > tglx ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-02-07 7:58 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-02-03 5:03 [PATCH v2 0/3] PCI: hv: Generify pci-hyperv.c Boqun Feng 2020-02-03 5:03 ` [PATCH v2 1/3] PCI: hv: Move hypercall related definitions into tlfs header Boqun Feng 2020-02-03 9:25 ` Andrew Murray 2020-02-04 2:22 ` Boqun Feng 2020-02-03 5:03 ` [PATCH v2 2/3] PCI: hv: Move retarget related structures " Boqun Feng 2020-02-03 9:41 ` Andrew Murray 2020-02-03 14:09 ` Boqun Feng 2020-02-07 7:58 ` Boqun Feng 2020-02-03 5:03 ` [PATCH v2 3/3] PCI: hv: Introduce hv_msi_entry Boqun Feng 2020-02-03 9:51 ` Andrew Murray 2020-02-03 14:35 ` Boqun Feng 2020-02-03 14:41 ` Thomas Gleixner 2020-02-04 2:13 ` Boqun Feng
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).