| Message ID | 20200627150428.2525192-4-npiggin@gmail.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | powerpc/pseries: IPI doorbell improvements | expand |
On Sun, Jun 28, 2020 at 01:04:28AM +1000, Nicholas Piggin wrote: > KVM guests have certain restrictions and performance quirks when > using doorbells. This patch tests for KVM environment in doorbell > setup, and optimises IPI performance: > > - PowerVM guests may now use doorbells even if they are secure. > > - KVM guests no longer use doorbells if XIVE is available. It seems, from the fact that you completely remove kvm_para_available(), that you perhaps haven't tried building with CONFIG_KVM_GUEST=y. Somewhat confusingly, that option is not used or needed when building for a PAPR guest (i.e. the "pseries" platform) but is used on non-IBM platforms using the "epapr" hypervisor interface. If you did intend to remove support for the epapr hypervisor interface then that should have been talked about in the commit message (and would I expect be controversial). So NAK on the kvm_para_available() removal. Paul.
Excerpts from Paul Mackerras's message of June 30, 2020 12:27 pm: > On Sun, Jun 28, 2020 at 01:04:28AM +1000, Nicholas Piggin wrote: >> KVM guests have certain restrictions and performance quirks when >> using doorbells. This patch tests for KVM environment in doorbell >> setup, and optimises IPI performance: >> >> - PowerVM guests may now use doorbells even if they are secure. >> >> - KVM guests no longer use doorbells if XIVE is available. > > It seems, from the fact that you completely remove > kvm_para_available(), that you perhaps haven't tried building with > CONFIG_KVM_GUEST=y. It's still there and builds: static inline int kvm_para_available(void) { return IS_ENABLED(CONFIG_KVM_GUEST) && is_kvm_guest(); } but... > Somewhat confusingly, that option is not used or > needed when building for a PAPR guest (i.e. the "pseries" platform) > but is used on non-IBM platforms using the "epapr" hypervisor > interface. ... is_kvm_guest() returns false on !PSERIES now. Not intended to break EPAPR. I'm not sure of a good way to share this between EPAPR and PSERIES, I might just make a copy of it but I'll see. Thanks, Nick
On Tue, Jun 30, 2020 at 03:35:08PM +1000, Nicholas Piggin wrote: > Excerpts from Paul Mackerras's message of June 30, 2020 12:27 pm: > > On Sun, Jun 28, 2020 at 01:04:28AM +1000, Nicholas Piggin wrote: > >> KVM guests have certain restrictions and performance quirks when > >> using doorbells. This patch tests for KVM environment in doorbell > >> setup, and optimises IPI performance: > >> > >> - PowerVM guests may now use doorbells even if they are secure. > >> > >> - KVM guests no longer use doorbells if XIVE is available. > > > > It seems, from the fact that you completely remove > > kvm_para_available(), that you perhaps haven't tried building with > > CONFIG_KVM_GUEST=y. > > It's still there and builds: OK, good, I missed that. > static inline int kvm_para_available(void) > { > return IS_ENABLED(CONFIG_KVM_GUEST) && is_kvm_guest(); > } > > but... > > > Somewhat confusingly, that option is not used or > > needed when building for a PAPR guest (i.e. the "pseries" platform) > > but is used on non-IBM platforms using the "epapr" hypervisor > > interface. > > ... is_kvm_guest() returns false on !PSERIES now. And therefore kvm_para_available() returns false on all the platforms where the code that depends on it could actually be used. It's not correct to assume that !PSERIES means not a KVM guest. > Not intended > to break EPAPR. I'm not sure of a good way to share this between > EPAPR and PSERIES, I might just make a copy of it but I'll see. OK, so you're doing a new version? Regards, Paul.
Excerpts from Paul Mackerras's message of June 30, 2020 6:26 pm: > On Tue, Jun 30, 2020 at 03:35:08PM +1000, Nicholas Piggin wrote: >> Excerpts from Paul Mackerras's message of June 30, 2020 12:27 pm: >> > On Sun, Jun 28, 2020 at 01:04:28AM +1000, Nicholas Piggin wrote: >> >> KVM guests have certain restrictions and performance quirks when >> >> using doorbells. This patch tests for KVM environment in doorbell >> >> setup, and optimises IPI performance: >> >> >> >> - PowerVM guests may now use doorbells even if they are secure. >> >> >> >> - KVM guests no longer use doorbells if XIVE is available. >> > >> > It seems, from the fact that you completely remove >> > kvm_para_available(), that you perhaps haven't tried building with >> > CONFIG_KVM_GUEST=y. >> >> It's still there and builds: > > OK, good, I missed that. > >> static inline int kvm_para_available(void) >> { >> return IS_ENABLED(CONFIG_KVM_GUEST) && is_kvm_guest(); >> } >> >> but... >> >> > Somewhat confusingly, that option is not used or >> > needed when building for a PAPR guest (i.e. the "pseries" platform) >> > but is used on non-IBM platforms using the "epapr" hypervisor >> > interface. >> >> ... is_kvm_guest() returns false on !PSERIES now. > > And therefore kvm_para_available() returns false on all the platforms > where the code that depends on it could actually be used. > > It's not correct to assume that !PSERIES means not a KVM guest. Yep, thanks for catching it. >> Not intended >> to break EPAPR. I'm not sure of a good way to share this between >> EPAPR and PSERIES, I might just make a copy of it but I'll see. > > OK, so you're doing a new version? Just sent. Thanks, Nick
diff --git a/arch/powerpc/include/asm/firmware.h b/arch/powerpc/include/asm/firmware.h index 6003c2e533a0..4dadb84ff2b2 100644 --- a/arch/powerpc/include/asm/firmware.h +++ b/arch/powerpc/include/asm/firmware.h @@ -134,7 +134,9 @@ extern unsigned int __start___fw_ftr_fixup, __stop___fw_ftr_fixup; #ifdef CONFIG_PPC_PSERIES void pseries_probe_fw_features(void); +bool is_kvm_guest(void); #else +static inline bool is_kvm_guest(void) { return false; } static inline void pseries_probe_fw_features(void) { }; #endif diff --git a/arch/powerpc/include/asm/kvm_para.h b/arch/powerpc/include/asm/kvm_para.h index 9c1f6b4b9bbf..744612054c94 100644 --- a/arch/powerpc/include/asm/kvm_para.h +++ b/arch/powerpc/include/asm/kvm_para.h @@ -8,35 +8,15 @@ #ifndef __POWERPC_KVM_PARA_H__ #define __POWERPC_KVM_PARA_H__ -#include <uapi/asm/kvm_para.h> - -#ifdef CONFIG_KVM_GUEST - -#include <linux/of.h> - -static inline int kvm_para_available(void) -{ - struct device_node *hyper_node; - - hyper_node = of_find_node_by_path("/hypervisor"); - if (!hyper_node) - return 0; +#include <asm/firmware.h> - if (!of_device_is_compatible(hyper_node, "linux,kvm")) - return 0; - - return 1; -} - -#else +#include <uapi/asm/kvm_para.h> static inline int kvm_para_available(void) { - return 0; + return IS_ENABLED(CONFIG_KVM_GUEST) && is_kvm_guest(); } -#endif - static inline unsigned int kvm_arch_para_features(void) { unsigned long r; diff --git a/arch/powerpc/platforms/pseries/firmware.c b/arch/powerpc/platforms/pseries/firmware.c index 3e49cc23a97a..f58eb10011dd 100644 --- a/arch/powerpc/platforms/pseries/firmware.c +++ b/arch/powerpc/platforms/pseries/firmware.c @@ -184,3 +184,17 @@ void __init pseries_probe_fw_features(void) { of_scan_flat_dt(probe_fw_features, NULL); } + +bool is_kvm_guest(void) +{ + struct device_node *hyper_node; + + hyper_node = of_find_node_by_path("/hypervisor"); + if (!hyper_node) + return 0; + + if (!of_device_is_compatible(hyper_node, "linux,kvm")) + return 0; + + return 1; +} diff --git a/arch/powerpc/platforms/pseries/smp.c b/arch/powerpc/platforms/pseries/smp.c index 67e6ad5076ce..7af0003b40b6 100644 --- a/arch/powerpc/platforms/pseries/smp.c +++ b/arch/powerpc/platforms/pseries/smp.c @@ -236,24 +236,32 @@ static __init void pSeries_smp_probe(void) if (!cpu_has_feature(CPU_FTR_SMT)) return; - /* - * KVM emulates doorbells by disabling FSCR[MSGP] so msgsndp faults - * to the hypervisor which then reads the instruction from guest - * memory. This can't be done if the guest is secure, so don't use - * doorbells in secure guests. - * - * Under PowerVM, FSCR[MSGP] is enabled so doorbells could be used - * by secure guests if we distinguished this from KVM. - */ - if (is_secure_guest()) - return; + if (is_kvm_guest()) { + /* + * KVM emulates doorbells by disabling FSCR[MSGP] so msgsndp + * faults to the hypervisor which then reads the instruction + * from guest memory, which tends to be slower than using XIVE. + */ + if (xive_enabled()) + return; + + /* + * XICS hcalls aren't as fast, so we can use msgsndp (which + * also helps exercise KVM emulation), however KVM can't + * emulate secure guests because it can't read the instruction + * out of their memory. + */ + if (is_secure_guest()) + return; + } /* - * The guest can use doobells for SMT sibling IPIs, which stay in - * the core rather than going to the interrupt controller. This - * tends to be slower under KVM where doorbells are emulated, but - * faster for PowerVM where they're enabled. + * Under PowerVM, FSCR[MSGP] is enabled as guest vCPU siblings are + * gang scheduled on the same physical core, so doorbells are always + * faster than the interrupt controller, and they can be used by + * secure guests. */ + ic_cause_ipi = smp_ops->cause_ipi; smp_ops->cause_ipi = dbell_or_ic_cause_ipi; }
KVM guests have certain restrictions and performance quirks when using doorbells. This patch tests for KVM environment in doorbell setup, and optimises IPI performance: - PowerVM guests may now use doorbells even if they are secure. - KVM guests no longer use doorbells if XIVE is available. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- arch/powerpc/include/asm/firmware.h | 2 ++ arch/powerpc/include/asm/kvm_para.h | 26 ++-------------- arch/powerpc/platforms/pseries/firmware.c | 14 +++++++++ arch/powerpc/platforms/pseries/smp.c | 38 ++++++++++++++--------- 4 files changed, 42 insertions(+), 38 deletions(-)