Re: [PATCH 1/3] Split paravirt ops by functionality
Jeremy Fitzhardinge wrote: > On 11/18/09 08:13, Alexander Graf wrote: > >> Currently when using paravirt ops it's an all-or-nothing option. We can >> either >> use pv-ops for CPU, MMU, timing, etc. or not at all. >> >> Now there are some use cases where we don't need the full feature set, but >> only >> a small chunk of it. KVM is a pretty prominent example for this. >> >> So let's make everything a bit more fine-grained. We already have a splitting >> by function groups, namely "cpu", "mmu", "time", "irq", "apic" and >> "spinlock". >> >> Taking that existing splitting and extending it to only compile in the PV >> capable bits sounded like a natural fit. That way we don't get performance >> hits >> in MMU code from using the KVM PV clock which only needs the TIME parts of >> pv-ops. >> >> We define a new CONFIG_PARAVIRT_ALL option that basically does the same thing >> the CONFIG_PARAVIRT did before this splitting. We move all users of >> CONFIG_PARAVIRT to CONFIG_PARAVIRT_ALL, so they behave the same way they did >> before. >> >> So here it is - the splitting! I would have made the patch smaller, but this >> was the closest I could get to atomic (for bisect) while staying sane. >> >> > > The basic idea seems pretty sane. I'm wondering how much compile test > coverage you've given all these extra config options; there are now a > lot more combinations, and your use of select is particularly worrying > because they don't propagate dependencies properly. > Uh - I don't see where there should be any dependencies. > For example, does this actually work? > >> config XEN >> bool "Xen guest support" >> -select PARAVIRT >> +select PARAVIRT_ALL >> select PARAVIRT_CLOCK >> depends on X86_64 || (X86_32 && X86_PAE && !X86_VISWS) >> depends on X86_CMPXCHG && X86_TSC >> >> > Does selecting PARAVIRT_ALL end up selecting all the other PARAVIRT_*? > > Can you reassure me? > > +config PARAVIRT_ALL > + bool > + select PARAVIRT_CPU > + select PARAVIRT_TIME > + select PARAVIRT_IRQ > + select PARAVIRT_APIC > + select PARAVIRT_MMU > + default n > + So selecting PARAVIRT_ALL selects all the other split PARAVIRT parts that in turn select PARAVIRT. I tested a compile on x86_64 with Xen DomU support enabled and it compiled fine :-). > Also, I think VMI is the only serious user of PARAVIRT_APIC, so we can > mark that to go when VMI does. > Sounds good :-). So the patch even serves as a helper for anyone who'll remove that support later. > What ends up using plain CONFIG_PARAVIRT? Do we still need it? > It's used for an info field that says which hypervisor we're running on and as config option to know if we need to include the binary patch magic. As soon as a single sub-paravirt option is selected, we need that to make sure we have the framework in place. Alex -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] Split paravirt ops by functionality
On 11/18/09 08:13, Alexander Graf wrote: > Currently when using paravirt ops it's an all-or-nothing option. We can either > use pv-ops for CPU, MMU, timing, etc. or not at all. > > Now there are some use cases where we don't need the full feature set, but > only > a small chunk of it. KVM is a pretty prominent example for this. > > So let's make everything a bit more fine-grained. We already have a splitting > by function groups, namely "cpu", "mmu", "time", "irq", "apic" and "spinlock". > > Taking that existing splitting and extending it to only compile in the PV > capable bits sounded like a natural fit. That way we don't get performance > hits > in MMU code from using the KVM PV clock which only needs the TIME parts of > pv-ops. > > We define a new CONFIG_PARAVIRT_ALL option that basically does the same thing > the CONFIG_PARAVIRT did before this splitting. We move all users of > CONFIG_PARAVIRT to CONFIG_PARAVIRT_ALL, so they behave the same way they did > before. > > So here it is - the splitting! I would have made the patch smaller, but this > was the closest I could get to atomic (for bisect) while staying sane. > The basic idea seems pretty sane. I'm wondering how much compile test coverage you've given all these extra config options; there are now a lot more combinations, and your use of select is particularly worrying because they don't propagate dependencies properly. For example, does this actually work? > config XEN > bool "Xen guest support" > - select PARAVIRT > + select PARAVIRT_ALL > select PARAVIRT_CLOCK > depends on X86_64 || (X86_32 && X86_PAE && !X86_VISWS) > depends on X86_CMPXCHG && X86_TSC > Does selecting PARAVIRT_ALL end up selecting all the other PARAVIRT_*? Can you reassure me? Also, I think VMI is the only serious user of PARAVIRT_APIC, so we can mark that to go when VMI does. What ends up using plain CONFIG_PARAVIRT? Do we still need it? J > Signed-off-by: Alexander Graf > --- > arch/x86/Kconfig| 47 -- > arch/x86/include/asm/apic.h |2 +- > arch/x86/include/asm/desc.h |4 +- > arch/x86/include/asm/fixmap.h |2 +- > arch/x86/include/asm/highmem.h |2 +- > arch/x86/include/asm/io_32.h|4 ++- > arch/x86/include/asm/io_64.h|2 +- > arch/x86/include/asm/irqflags.h | 21 +--- > arch/x86/include/asm/mmu_context.h |4 +- > arch/x86/include/asm/msr.h |4 +- > arch/x86/include/asm/paravirt.h | 44 - > arch/x86/include/asm/paravirt_types.h | 12 +++ > arch/x86/include/asm/pgalloc.h |2 +- > arch/x86/include/asm/pgtable-3level_types.h |2 +- > arch/x86/include/asm/pgtable.h |2 +- > arch/x86/include/asm/processor.h|2 +- > arch/x86/include/asm/required-features.h|2 +- > arch/x86/include/asm/smp.h |2 +- > arch/x86/include/asm/system.h | 13 +-- > arch/x86/include/asm/tlbflush.h |4 +- > arch/x86/kernel/head_64.S |2 +- > arch/x86/kernel/paravirt.c |2 + > arch/x86/kernel/tsc.c |2 +- > arch/x86/kernel/vsmp_64.c |2 +- > arch/x86/xen/Kconfig|2 +- > 25 files changed, 149 insertions(+), 38 deletions(-) > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > index 0c7b699..8c150b6 100644 > --- a/arch/x86/Kconfig > +++ b/arch/x86/Kconfig > @@ -350,7 +350,7 @@ endif > > config X86_VSMP > bool "ScaleMP vSMP" > - select PARAVIRT > + select PARAVIRT_ALL > depends on X86_64 && PCI > depends on X86_EXTENDED_PLATFORM > ---help--- > @@ -493,7 +493,7 @@ source "arch/x86/xen/Kconfig" > > config VMI > bool "VMI Guest support (DEPRECATED)" > - select PARAVIRT > + select PARAVIRT_ALL > depends on X86_32 > ---help--- > VMI provides a paravirtualized interface to the VMware ESX server > @@ -512,7 +512,6 @@ config VMI > > config KVM_CLOCK > bool "KVM paravirtualized clock" > - select PARAVIRT > select PARAVIRT_CLOCK > ---help--- > Turning on this option will allow you to run a paravirtualized clock > @@ -523,7 +522,7 @@ config KVM_CLOCK > > config KVM_GUEST > bool "KVM Guest support" > - select PARAVIRT > + select PARAVIRT_ALL > ---help--- > This option enables various optimizations for running under the KVM > hypervisor. > @@ -551,8 +550,48 @@ config PARAVIRT_SPINLOCKS > > If you are unsure how to answer this question, answer N. > > +config PARAVIRT_CPU > + bool > + select PARAVIRT > + default n > + > +config PARAVIRT_TIME > + bool > + select PARAVIRT > + default n > + >
[PATCH 1/3] Split paravirt ops by functionality
Currently when using paravirt ops it's an all-or-nothing option. We can either use pv-ops for CPU, MMU, timing, etc. or not at all. Now there are some use cases where we don't need the full feature set, but only a small chunk of it. KVM is a pretty prominent example for this. So let's make everything a bit more fine-grained. We already have a splitting by function groups, namely "cpu", "mmu", "time", "irq", "apic" and "spinlock". Taking that existing splitting and extending it to only compile in the PV capable bits sounded like a natural fit. That way we don't get performance hits in MMU code from using the KVM PV clock which only needs the TIME parts of pv-ops. We define a new CONFIG_PARAVIRT_ALL option that basically does the same thing the CONFIG_PARAVIRT did before this splitting. We move all users of CONFIG_PARAVIRT to CONFIG_PARAVIRT_ALL, so they behave the same way they did before. So here it is - the splitting! I would have made the patch smaller, but this was the closest I could get to atomic (for bisect) while staying sane. Signed-off-by: Alexander Graf --- arch/x86/Kconfig| 47 -- arch/x86/include/asm/apic.h |2 +- arch/x86/include/asm/desc.h |4 +- arch/x86/include/asm/fixmap.h |2 +- arch/x86/include/asm/highmem.h |2 +- arch/x86/include/asm/io_32.h|4 ++- arch/x86/include/asm/io_64.h|2 +- arch/x86/include/asm/irqflags.h | 21 +--- arch/x86/include/asm/mmu_context.h |4 +- arch/x86/include/asm/msr.h |4 +- arch/x86/include/asm/paravirt.h | 44 - arch/x86/include/asm/paravirt_types.h | 12 +++ arch/x86/include/asm/pgalloc.h |2 +- arch/x86/include/asm/pgtable-3level_types.h |2 +- arch/x86/include/asm/pgtable.h |2 +- arch/x86/include/asm/processor.h|2 +- arch/x86/include/asm/required-features.h|2 +- arch/x86/include/asm/smp.h |2 +- arch/x86/include/asm/system.h | 13 +-- arch/x86/include/asm/tlbflush.h |4 +- arch/x86/kernel/head_64.S |2 +- arch/x86/kernel/paravirt.c |2 + arch/x86/kernel/tsc.c |2 +- arch/x86/kernel/vsmp_64.c |2 +- arch/x86/xen/Kconfig|2 +- 25 files changed, 149 insertions(+), 38 deletions(-) diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index 0c7b699..8c150b6 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -350,7 +350,7 @@ endif config X86_VSMP bool "ScaleMP vSMP" - select PARAVIRT + select PARAVIRT_ALL depends on X86_64 && PCI depends on X86_EXTENDED_PLATFORM ---help--- @@ -493,7 +493,7 @@ source "arch/x86/xen/Kconfig" config VMI bool "VMI Guest support (DEPRECATED)" - select PARAVIRT + select PARAVIRT_ALL depends on X86_32 ---help--- VMI provides a paravirtualized interface to the VMware ESX server @@ -512,7 +512,6 @@ config VMI config KVM_CLOCK bool "KVM paravirtualized clock" - select PARAVIRT select PARAVIRT_CLOCK ---help--- Turning on this option will allow you to run a paravirtualized clock @@ -523,7 +522,7 @@ config KVM_CLOCK config KVM_GUEST bool "KVM Guest support" - select PARAVIRT + select PARAVIRT_ALL ---help--- This option enables various optimizations for running under the KVM hypervisor. @@ -551,8 +550,48 @@ config PARAVIRT_SPINLOCKS If you are unsure how to answer this question, answer N. +config PARAVIRT_CPU + bool + select PARAVIRT + default n + +config PARAVIRT_TIME + bool + select PARAVIRT + default n + +config PARAVIRT_IRQ + bool + select PARAVIRT + default n + +config PARAVIRT_APIC + bool + select PARAVIRT + default n + +config PARAVIRT_MMU + bool + select PARAVIRT + default n + +# +# This is a placeholder to activate the old "include all pv-ops functionality" +# behavior. If you're using this I'd recommend looking through your code to see +# if you can be more specific. It probably saves you a few cycles! +# +config PARAVIRT_ALL + bool + select PARAVIRT_CPU + select PARAVIRT_TIME + select PARAVIRT_IRQ + select PARAVIRT_APIC + select PARAVIRT_MMU + default n + config PARAVIRT_CLOCK bool + select PARAVIRT_TIME default n endif diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h index 474d80d..b54c24a 100644 --- a/arch/x86/include/asm/apic.h +++ b/arch/x86/include/asm/apic.h @@ -81,7 +81,7 @@ static inline bool apic_from_smp_config(void) /* * Basic
