| Message ID | 20181122031059.16338-3-kys@linuxonhyperv.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Hyper-V: Enable Linux guests on Hyper-V on ARM64 | expand |
On Thu, Nov 22, 2018 at 03:10:58AM +0000, kys@linuxonhyperv.com wrote: > From: Michael Kelley <mikelley@microsoft.com> > > Add hooks to enable/disable a per-CPU IRQ for VMbus. These hooks > are in the architecture independent setup and shutdown paths for > Hyper-V, and are needed by Linux guests on Hyper-V on ARM64. The > x86/x64 implementation is null because VMbus interrupts on x86/x64 > don't use an IRQ. > > Signed-off-by: Michael Kelley <mikelley@microsoft.com> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com> > --- > arch/x86/include/asm/mshyperv.h | 4 ++++ > drivers/hv/hv.c | 2 ++ > 2 files changed, 6 insertions(+) > > diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h > index 0d6271cce198..8d97bd3a13a6 100644 > --- a/arch/x86/include/asm/mshyperv.h > +++ b/arch/x86/include/asm/mshyperv.h > @@ -109,6 +109,10 @@ void hyperv_vector_handler(struct pt_regs *regs); > void hv_setup_vmbus_irq(void (*handler)(void)); > void hv_remove_vmbus_irq(void); > > +/* On x86/x64, there isn't a real IRQ to be enabled/disable */ > +static inline void hv_enable_vmbus_irq(void) {} > +static inline void hv_disable_vmbus_irq(void) {} > + > void hv_setup_kexec_handler(void (*handler)(void)); > void hv_remove_kexec_handler(void); > void hv_setup_crash_handler(void (*handler)(struct pt_regs *regs)); > diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c > index 166c2501de17..d0bb09a4bd73 100644 > --- a/drivers/hv/hv.c > +++ b/drivers/hv/hv.c > @@ -307,6 +307,7 @@ int hv_synic_init(unsigned int cpu) > hv_set_siefp(siefp.as_uint64); > > /* Setup the shared SINT. */ > + hv_enable_vmbus_irq(); > hv_get_synint_state(VMBUS_MESSAGE_SINT, shared_sint.as_uint64); > > shared_sint.vector = HYPERVISOR_CALLBACK_VECTOR; > @@ -434,6 +435,7 @@ int hv_synic_cleanup(unsigned int cpu) > /* Disable the global synic bit */ > sctrl.enable = 0; > hv_set_synic_state(sctrl.as_uint64); > + hv_disable_vmbus_irq(); > > return 0; > } > -- > 2.19.1 You created "null" hooks that do nothing, for no one in this patch series, why? Add them when they are needed not now. If I saw this code in the tree, I would just go delete it as it is because it is not used at all. thanks, greg k-h
From: Greg KH <gregkh@linuxfoundation.org> Monday, November 26, 2018 11:21 AM > > diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h > > index 0d6271cce198..8d97bd3a13a6 100644 > > --- a/arch/x86/include/asm/mshyperv.h > > +++ b/arch/x86/include/asm/mshyperv.h > > @@ -109,6 +109,10 @@ void hyperv_vector_handler(struct pt_regs *regs); > > void hv_setup_vmbus_irq(void (*handler)(void)); > > void hv_remove_vmbus_irq(void); > > > > +/* On x86/x64, there isn't a real IRQ to be enabled/disable */ > > +static inline void hv_enable_vmbus_irq(void) {} > > +static inline void hv_disable_vmbus_irq(void) {} > > + > > void hv_setup_kexec_handler(void (*handler)(void)); > > void hv_remove_kexec_handler(void); > > void hv_setup_crash_handler(void (*handler)(struct pt_regs *regs)); > > diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c > > index 166c2501de17..d0bb09a4bd73 100644 > > --- a/drivers/hv/hv.c > > +++ b/drivers/hv/hv.c > > @@ -307,6 +307,7 @@ int hv_synic_init(unsigned int cpu) > > hv_set_siefp(siefp.as_uint64); > > > > /* Setup the shared SINT. */ > > + hv_enable_vmbus_irq(); > > hv_get_synint_state(VMBUS_MESSAGE_SINT, shared_sint.as_uint64); > > > > shared_sint.vector = HYPERVISOR_CALLBACK_VECTOR; > > @@ -434,6 +435,7 @@ int hv_synic_cleanup(unsigned int cpu) > > /* Disable the global synic bit */ > > sctrl.enable = 0; > > hv_set_synic_state(sctrl.as_uint64); > > + hv_disable_vmbus_irq(); > > > > return 0; > > } > > -- > > 2.19.1 > > You created "null" hooks that do nothing, for no one in this patch > series, why? > hv_enable_vmbus_irq() and hv_disable_vmbus_irq() have non-null implementations in the ARM64 code in patch 2 of this series. The implementations are in the new file arch/arm64/hyperv/mshyperv.c. Or am I misunderstanding your point? Michael
On Mon, Nov 26, 2018 at 07:47:41PM +0000, Michael Kelley wrote: > From: Greg KH <gregkh@linuxfoundation.org> Monday, November 26, 2018 11:21 AM > > > > diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h > > > index 0d6271cce198..8d97bd3a13a6 100644 > > > --- a/arch/x86/include/asm/mshyperv.h > > > +++ b/arch/x86/include/asm/mshyperv.h > > > @@ -109,6 +109,10 @@ void hyperv_vector_handler(struct pt_regs *regs); > > > void hv_setup_vmbus_irq(void (*handler)(void)); > > > void hv_remove_vmbus_irq(void); > > > > > > +/* On x86/x64, there isn't a real IRQ to be enabled/disable */ > > > +static inline void hv_enable_vmbus_irq(void) {} > > > +static inline void hv_disable_vmbus_irq(void) {} > > > + > > > void hv_setup_kexec_handler(void (*handler)(void)); > > > void hv_remove_kexec_handler(void); > > > void hv_setup_crash_handler(void (*handler)(struct pt_regs *regs)); > > > diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c > > > index 166c2501de17..d0bb09a4bd73 100644 > > > --- a/drivers/hv/hv.c > > > +++ b/drivers/hv/hv.c > > > @@ -307,6 +307,7 @@ int hv_synic_init(unsigned int cpu) > > > hv_set_siefp(siefp.as_uint64); > > > > > > /* Setup the shared SINT. */ > > > + hv_enable_vmbus_irq(); > > > hv_get_synint_state(VMBUS_MESSAGE_SINT, shared_sint.as_uint64); > > > > > > shared_sint.vector = HYPERVISOR_CALLBACK_VECTOR; > > > @@ -434,6 +435,7 @@ int hv_synic_cleanup(unsigned int cpu) > > > /* Disable the global synic bit */ > > > sctrl.enable = 0; > > > hv_set_synic_state(sctrl.as_uint64); > > > + hv_disable_vmbus_irq(); > > > > > > return 0; > > > } > > > -- > > > 2.19.1 > > > > You created "null" hooks that do nothing, for no one in this patch > > series, why? > > > > hv_enable_vmbus_irq() and hv_disable_vmbus_irq() have non-null > implementations in the ARM64 code in patch 2 of this series. The > implementations are in the new file arch/arm64/hyperv/mshyperv.c. > Or am I misunderstanding your point? So you use a hook in an earlier patch and then add it in a later one? Shouldn't you do it the other way around? As it is, the earlier patch should not work properly, right? thanks, greg k-h
From: Greg KH <gregkh@linuxfoundation.org> Monday, November 26, 2018 11:57 AM > > > You created "null" hooks that do nothing, for no one in this patch > > > series, why? > > > > > > > hv_enable_vmbus_irq() and hv_disable_vmbus_irq() have non-null > > implementations in the ARM64 code in patch 2 of this series. The > > implementations are in the new file arch/arm64/hyperv/mshyperv.c. > > Or am I misunderstanding your point? > > So you use a hook in an earlier patch and then add it in a later one? > > Shouldn't you do it the other way around? As it is, the earlier patch > should not work properly, right? The earlier patch implements the hook on the ARM64 side but it is unused -- it's not called. The later patch then calls it. Wouldn't the other way around be backwards? The general approach is for patches 1 and 2 of the series to provide all the new code under arch/arm64 to enable Hyper-V. But the code won't get called (or even built) with just these two patches because CONFIG_HYPERV can't be selected. Patch 3 is separate because it applies to architecture independent code and arch/x86 code -- I thought there might be value in keeping the ARM64 and x86 patches distinct. Patch 4 applies to architecture independent code, and enables the ARM64 code in patches 1 and 2 to be compiled and run when CONFIG_HYPERV is selected. If combining some of the patches in the series is a better approach, I'm good with that. Michael
On Mon, Nov 26, 2018 at 08:56:50PM +0000, Michael Kelley wrote: > From: Greg KH <gregkh@linuxfoundation.org> Monday, November 26, 2018 11:57 AM > > > > > You created "null" hooks that do nothing, for no one in this patch > > > > series, why? > > > > > > > > > > hv_enable_vmbus_irq() and hv_disable_vmbus_irq() have non-null > > > implementations in the ARM64 code in patch 2 of this series. The > > > implementations are in the new file arch/arm64/hyperv/mshyperv.c. > > > Or am I misunderstanding your point? > > > > So you use a hook in an earlier patch and then add it in a later one? > > > > Shouldn't you do it the other way around? As it is, the earlier patch > > should not work properly, right? > > The earlier patch implements the hook on the ARM64 side but it is > unused -- it's not called. The later patch then calls it. Wouldn't the > other way around be backwards? Ah, it wasn't obvious that the previous patch added it at all, why not just make that addition part of this patch? > The general approach is for patches 1 and 2 of the series to provide > all the new code under arch/arm64 to enable Hyper-V. But the code > won't get called (or even built) with just these two patches because > CONFIG_HYPERV can't be selected. Patch 3 is separate because it > applies to architecture independent code and arch/x86 code -- I thought > there might be value in keeping the ARM64 and x86 patches distinct. > Patch 4 applies to architecture independent code, and enables the > ARM64 code in patches 1 and 2 to be compiled and run when > CONFIG_HYPERV is selected. > > If combining some of the patches in the series is a better approach, I'm > good with that. Ok, that makes more sense, if it is easier to get the ARM people to review this, that's fine. Doesn't seem like anyone did that yet :( sorry for the noise, greg k-h
On Tue, Nov 27, 2018 at 07:20:56AM +0100, Greg KH wrote: > On Mon, Nov 26, 2018 at 08:56:50PM +0000, Michael Kelley wrote: > > From: Greg KH <gregkh@linuxfoundation.org> Monday, November 26, 2018 11:57 AM > > > > > > > You created "null" hooks that do nothing, for no one in this patch > > > > > series, why? > > > > > > > > > > > > > hv_enable_vmbus_irq() and hv_disable_vmbus_irq() have non-null > > > > implementations in the ARM64 code in patch 2 of this series. The > > > > implementations are in the new file arch/arm64/hyperv/mshyperv.c. > > > > Or am I misunderstanding your point? > > > > > > So you use a hook in an earlier patch and then add it in a later one? > > > > > > Shouldn't you do it the other way around? As it is, the earlier patch > > > should not work properly, right? > > > > The earlier patch implements the hook on the ARM64 side but it is > > unused -- it's not called. The later patch then calls it. Wouldn't the > > other way around be backwards? > > Ah, it wasn't obvious that the previous patch added it at all, why not > just make that addition part of this patch? > > > The general approach is for patches 1 and 2 of the series to provide > > all the new code under arch/arm64 to enable Hyper-V. But the code > > won't get called (or even built) with just these two patches because > > CONFIG_HYPERV can't be selected. Patch 3 is separate because it > > applies to architecture independent code and arch/x86 code -- I thought > > there might be value in keeping the ARM64 and x86 patches distinct. > > Patch 4 applies to architecture independent code, and enables the > > ARM64 code in patches 1 and 2 to be compiled and run when > > CONFIG_HYPERV is selected. > > > > If combining some of the patches in the series is a better approach, I'm > > good with that. > > Ok, that makes more sense, if it is easier to get the ARM people to > review this, that's fine. Doesn't seem like anyone did that yet :( It's on the list, but thanks for having a look as well! Will
From: Will Deacon <will.deacon@arm.com> Sent: Tuesday, November 27, 2018 2:19 AM > > > The general approach is for patches 1 and 2 of the series to provide > > > all the new code under arch/arm64 to enable Hyper-V. But the code > > > won't get called (or even built) with just these two patches because > > > CONFIG_HYPERV can't be selected. Patch 3 is separate because it > > > applies to architecture independent code and arch/x86 code -- I thought > > > there might be value in keeping the ARM64 and x86 patches distinct. > > > Patch 4 applies to architecture independent code, and enables the > > > ARM64 code in patches 1 and 2 to be compiled and run when > > > CONFIG_HYPERV is selected. > > > > > > If combining some of the patches in the series is a better approach, I'm > > > good with that. > > > > Ok, that makes more sense, if it is easier to get the ARM people to > > review this, that's fine. Doesn't seem like anyone did that yet :( > > It's on the list, but thanks for having a look as well! > > Will Will -- I'll hold off on sending a new version, pending comments from the ARM64 maintainers. Let me know if you prefer that I do otherwise. Michael
diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h index 0d6271cce198..8d97bd3a13a6 100644 --- a/arch/x86/include/asm/mshyperv.h +++ b/arch/x86/include/asm/mshyperv.h @@ -109,6 +109,10 @@ void hyperv_vector_handler(struct pt_regs *regs); void hv_setup_vmbus_irq(void (*handler)(void)); void hv_remove_vmbus_irq(void); +/* On x86/x64, there isn't a real IRQ to be enabled/disable */ +static inline void hv_enable_vmbus_irq(void) {} +static inline void hv_disable_vmbus_irq(void) {} + void hv_setup_kexec_handler(void (*handler)(void)); void hv_remove_kexec_handler(void); void hv_setup_crash_handler(void (*handler)(struct pt_regs *regs)); diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c index 166c2501de17..d0bb09a4bd73 100644 --- a/drivers/hv/hv.c +++ b/drivers/hv/hv.c @@ -307,6 +307,7 @@ int hv_synic_init(unsigned int cpu) hv_set_siefp(siefp.as_uint64); /* Setup the shared SINT. */ + hv_enable_vmbus_irq(); hv_get_synint_state(VMBUS_MESSAGE_SINT, shared_sint.as_uint64); shared_sint.vector = HYPERVISOR_CALLBACK_VECTOR; @@ -434,6 +435,7 @@ int hv_synic_cleanup(unsigned int cpu) /* Disable the global synic bit */ sctrl.enable = 0; hv_set_synic_state(sctrl.as_uint64); + hv_disable_vmbus_irq(); return 0; }