| Message ID | 20210901173357.3183658-1-farosas@linux.ibm.com |
|---|---|
| Headers | show |
| Series | KVM: PPC: Book3S: Modules cleanup and unification | expand |
On Wed, Sep 01, 2021 at 02:33:52PM -0300, Fabiano Rosas wrote: > This series merges our three kvm modules kvm.ko, kvm-hv.ko and > kvm-pr.ko into one kvm.ko module. That doesn't sound like a good idea to me. People who aren't on BookS servers don't want - and can't use - kvm-hv. Almost nobody wants kvm-pr. It's also kind of inconsistent with x86, which has the separate AMD and Intel modules. > The main reason for this is to deal with the issue that kvm.ko can be > loaded on its own without any of the other modules present. This can > happen if one or both of the modules fail to init or if the user loads > kvm.ko only. > > With only kvm.ko loaded, the userspace can call any of the KVM ioctls > which will fail more or less gracefully depending on what kind of > verification we do in powerpc.c. I see that that's awkward, but I'm not sure it justifies compromising the actual natural structure of the dependencies. > Instead of adding a check to every entry point or finding a hack to > link the modules so that when one fails (hv/pr), the other (kvm) > exits, I think it is cleaner to just make them all a single module. > > The two KVM implementations are already selected by Kconfig options, > so the only thing that changes is that they are now in the same > module. I also kept kvm-hv and kvm-pr as aliases to kvm, so that > people don't get too surprised with the change. > > There is a possible issue with the larger module size for kernel > builds that should support both HV-only and PR-only environments, but > PR is usually not used in production so I'm not sure if that is a real > issue. > > Patches 1,2,3 are standalone cleanups. > Patches 4,5 are the unification work. > > Fabiano Rosas (5): > KVM: PPC: Book3S HV: Check return value of kvmppc_radix_init > KVM: PPC: Book3S HV: Delay setting of kvm ops > KVM: PPC: Book3S HV: Free allocated memory if module init fails > KVM: PPC: Book3S: Unify kvm-hv and kvm-pr modules > KVM: PPC: Book3S: Stop exporting non-builtin symbols > > arch/powerpc/configs/powernv_defconfig | 2 +- > arch/powerpc/configs/ppc64_defconfig | 2 +- > arch/powerpc/configs/pseries_defconfig | 2 +- > arch/powerpc/kvm/Kconfig | 72 ++++++++++++-------------- > arch/powerpc/kvm/Makefile | 11 ++-- > arch/powerpc/kvm/book3s.c | 61 ++++++++++++++-------- > arch/powerpc/kvm/book3s.h | 19 +++++++ > arch/powerpc/kvm/book3s_64_mmu_radix.c | 3 -- > arch/powerpc/kvm/book3s_64_vio.c | 3 -- > arch/powerpc/kvm/book3s_hv.c | 38 ++++++++------ > arch/powerpc/kvm/book3s_pr.c | 13 ----- > arch/powerpc/kvm/book3s_rtas.c | 1 - > arch/powerpc/kvm/book3s_xics.c | 4 -- > arch/powerpc/kvm/book3s_xive.c | 6 --- > arch/powerpc/kvm/emulate.c | 1 - > arch/powerpc/kvm/powerpc.c | 14 ----- > kernel/irq/irqdesc.c | 2 +- > 17 files changed, 125 insertions(+), 129 deletions(-) >
David Gibson <david@gibson.dropbear.id.au> writes: > On Wed, Sep 01, 2021 at 02:33:52PM -0300, Fabiano Rosas wrote: >> This series merges our three kvm modules kvm.ko, kvm-hv.ko and >> kvm-pr.ko into one kvm.ko module. > > That doesn't sound like a good idea to me. People who aren't on BookS > servers don't want - and can't use - kvm-hv. Almost nobody wants > kvm-pr. It's also kind of inconsistent with x86, which has the > separate AMD and Intel modules. But this is not altering the ability of having only kvm-hv or only kvm-pr. I'm taking the Kconfig options that used to produce separate modules and using them to select which code gets built into the one kvm.ko module. Currently: CONFIG_KVM_BOOK3S_64=m <-- produces kvm.ko CONFIG_KVM_BOOK3S_64_HV=m <-- produces kvm-hv.ko CONFIG_KVM_BOOK3S_64_PR=m <-- produces kvm-pr.ko I'm making it so we now have one kvm.ko everywhere, but there is still: CONFIG_KVM_BOOK3S_64=m <-- produces kvm.ko CONFIG_KVM_BOOK3S_HV_POSSIBLE=y <-- includes HV in kvm.ko CONFIG_KVM_BOOK3S_PR_POSSIBLE=y <-- includes PR in kvm.ko In other words, if you are going to have at least two modules loaded at all times (kvm + kvm-hv or kvm + kvm-pr), why not put all that into one module? No one needs to build code they are not going to use, this is not changing. About consistency with x86, this situation is not analogous because we need to be able to load both modules at the same time, which means kvm.ko needs to stick around when one module goes away in case we want to load the other module. The KVM common code states that it expects to have at most one implementation: /* * kvm_arch_init makes sure there's at most one caller * for architectures that support multiple implementations, * like intel and amd on x86. (...) which is not true in our case due to this requirement of having two separate modules loading independently. (tangent) We are already quite different from other architectures since we're not making use of kvm_arch_init and some other KVM hooks, such as kvm_arch_check_processor_compat. So while other archs have their init dispatched by kvm common code, our init and cleanup happens independently in the ppc-specific modules, which obviously works but is needlessly different and has subtleties in the ordering of operations wrt. the kvm common code. (tangent)
On Thu, Sep 02, 2021 at 11:32:41AM -0300, Fabiano Rosas wrote: > David Gibson <david@gibson.dropbear.id.au> writes: > > > On Wed, Sep 01, 2021 at 02:33:52PM -0300, Fabiano Rosas wrote: > >> This series merges our three kvm modules kvm.ko, kvm-hv.ko and > >> kvm-pr.ko into one kvm.ko module. > > > > That doesn't sound like a good idea to me. People who aren't on BookS > > servers don't want - and can't use - kvm-hv. Almost nobody wants > > kvm-pr. It's also kind of inconsistent with x86, which has the > > separate AMD and Intel modules. > > But this is not altering the ability of having only kvm-hv or only > kvm-pr. I'm taking the Kconfig options that used to produce separate > modules and using them to select which code gets built into the one > kvm.ko module. > > Currently: > > CONFIG_KVM_BOOK3S_64=m <-- produces kvm.ko > CONFIG_KVM_BOOK3S_64_HV=m <-- produces kvm-hv.ko > CONFIG_KVM_BOOK3S_64_PR=m <-- produces kvm-pr.ko > > I'm making it so we now have one kvm.ko everywhere, but there is still: > > CONFIG_KVM_BOOK3S_64=m <-- produces kvm.ko > CONFIG_KVM_BOOK3S_HV_POSSIBLE=y <-- includes HV in kvm.ko > CONFIG_KVM_BOOK3S_PR_POSSIBLE=y <-- includes PR in kvm.ko > > In other words, if you are going to have at least two modules loaded at > all times (kvm + kvm-hv or kvm + kvm-pr), why not put all that into one > module? No one needs to build code they are not going to use, this is > not changing. Ah.. I see, you're removing the runtime switch from one to the other at the same time as having just a single one loaded, but leaving the ability to compile time switch. And compile time is arguably good enough for the cases I've described. Ok, I see your point. I still think it's conceptually not ideal, but the practical benefit is more important. Objection withdrawn. > About consistency with x86, this situation is not analogous because we > need to be able to load both modules at the same time, which means > kvm.ko needs to stick around when one module goes away in case we want > to load the other module. The KVM common code states that it expects to > have at most one implementation: > > /* > * kvm_arch_init makes sure there's at most one caller > * for architectures that support multiple implementations, > * like intel and amd on x86. > (...) > > which is not true in our case due to this requirement of having two > separate modules loading independently. > > (tangent) We are already quite different from other architectures since > we're not making use of kvm_arch_init and some other KVM hooks, such as > kvm_arch_check_processor_compat. So while other archs have their init > dispatched by kvm common code, our init and cleanup happens > independently in the ppc-specific modules, which obviously works but is > needlessly different and has subtleties in the ordering of operations > wrt. the kvm common code. (tangent) >