Message ID | 20170426203227.12321-10-rkrcmar@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Wed, Apr 26, 2017 at 10:32:27PM +0200, Radim Krčmář wrote: > kvm_make_all_requests() provides a synchronization that waits until all > kicked VCPUs have acknowledged the kick. This is important for > KVM_REQ_MMU_RELOAD as it prevents freeing while lockless paging is > underway. > > This patch adds the synchronization property into all requests that are > currently being used with kvm_make_all_requests() in order to preserve > the current behavior and only introduce a new framework. Removing it > from requests where it is not necessary is left for future patches. > > A question is whether this propertly isn't better expressed as a > function of the caller. Yes, IMO, knowing that waiting is needed, and the expectation that a wait will occur, should be explicit to the caller. I guess we need to extend the VCPU request API in some way to do that. > > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> > --- > v2: replaces [v1 1/6] > Ugh, KVM_ARCH_REQ_WAIT_NO_WAKEUP looks a weird ... Assuming we stick with the request wait flag, rather than an API change, then maybe we should drop KVM_ARCH_REQ_NO_WAKEUP() and just use, e.g. KVM_ARCH_REQ_FLAGS(14, KVM_REQUEST_NO_WAKEUP) or KVM_ARCH_REQ_FLAGS(14, KVM_REQUEST_NO_WAKEUP | KVM_REQUEST_WAIT) > --- > arch/arm/include/asm/kvm_host.h | 2 +- > arch/arm64/include/asm/kvm_host.h | 2 +- > arch/x86/include/asm/kvm_host.h | 6 +++--- > include/linux/kvm_host.h | 8 ++++++-- > virt/kvm/kvm_main.c | 16 +++++++++++++--- > 5 files changed, 24 insertions(+), 10 deletions(-) > > diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h > index 1d48a4b65b86..2190a7ddd515 100644 > --- a/arch/arm/include/asm/kvm_host.h > +++ b/arch/arm/include/asm/kvm_host.h > @@ -44,7 +44,7 @@ > #define KVM_MAX_VCPUS VGIC_V2_MAX_CPUS > #endif > > -#define KVM_REQ_VCPU_EXIT KVM_ARCH_REQ_NO_WAKEUP(0) > +#define KVM_REQ_VCPU_EXIT KVM_ARCH_REQ_WAIT_NO_WAKEUP(0) > > u32 *kvm_vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num, u32 mode); > int __attribute_const__ kvm_target_cpu(void); > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index d3370b79660e..98f3d01ae91b 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -41,7 +41,7 @@ > > #define KVM_VCPU_MAX_FEATURES 4 > > -#define KVM_REQ_VCPU_EXIT KVM_ARCH_REQ_NO_WAKEUP(0) > +#define KVM_REQ_VCPU_EXIT KVM_ARCH_REQ_WAIT_NO_WAKEUP(0) > > int __attribute_const__ kvm_target_cpu(void); > int kvm_reset_vcpu(struct kvm_vcpu *vcpu); > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index 15eb7d3837e3..77083d7e9540 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -61,10 +61,10 @@ > #define KVM_REQ_PMI KVM_ARCH_REQ(11) > #define KVM_REQ_SMI KVM_ARCH_REQ(12) > #define KVM_REQ_MASTERCLOCK_UPDATE KVM_ARCH_REQ(13) > -#define KVM_REQ_MCLOCK_INPROGRESS KVM_ARCH_REQ_NO_WAKEUP(14) > -#define KVM_REQ_SCAN_IOAPIC KVM_ARCH_REQ_NO_WAKEUP(15) > +#define KVM_REQ_MCLOCK_INPROGRESS KVM_ARCH_REQ_WAIT_NO_WAKEUP(14) > +#define KVM_REQ_SCAN_IOAPIC KVM_ARCH_REQ_WAIT_NO_WAKEUP(15) > #define KVM_REQ_GLOBAL_CLOCK_UPDATE KVM_ARCH_REQ(16) > -#define KVM_REQ_APIC_PAGE_RELOAD KVM_ARCH_REQ_NO_WAKEUP(17) > +#define KVM_REQ_APIC_PAGE_RELOAD KVM_ARCH_REQ_WAIT_NO_WAKEUP(17) > #define KVM_REQ_HV_CRASH KVM_ARCH_REQ(18) > #define KVM_REQ_IOAPIC_EOI_EXIT KVM_ARCH_REQ(19) > #define KVM_REQ_HV_RESET KVM_ARCH_REQ(20) > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index 38cfe372918c..a668f33b20dc 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -117,12 +117,13 @@ static inline bool is_error_page(struct page *page) > > #define KVM_REQUEST_MASK GENMASK(7,0) > #define KVM_REQUEST_NO_WAKEUP BIT(8) > +#define KVM_REQUEST_WAIT BIT(9) > /* > * Architecture-independent vcpu->requests bit members > * Bits 4-7 are reserved for more arch-independent bits. > */ > -#define KVM_REQ_TLB_FLUSH (0 | KVM_REQUEST_NO_WAKEUP) > -#define KVM_REQ_MMU_RELOAD (1 | KVM_REQUEST_NO_WAKEUP) > +#define KVM_REQ_TLB_FLUSH (0 | KVM_REQUEST_NO_WAKEUP | KVM_REQUEST_WAIT) > +#define KVM_REQ_MMU_RELOAD (1 | KVM_REQUEST_NO_WAKEUP | KVM_REQUEST_WAIT) > #define KVM_REQ_PENDING_TIMER 2 > #define KVM_REQ_UNHALT 3 > #define KVM_REQUEST_ARCH_BASE 8 > @@ -133,6 +134,9 @@ static inline bool is_error_page(struct page *page) > }) > #define KVM_ARCH_REQ(nr) KVM_ARCH_REQ_FLAGS(nr, 0) > #define KVM_ARCH_REQ_NO_WAKEUP(nr) KVM_ARCH_REQ_FLAGS(nr, KVM_REQUEST_NO_WAKEUP) > +#define KVM_ARCH_REQ_WAIT(nr) KVM_ARCH_REQ_FLAGS(nr, KVM_REQUEST_NO_WAKEUP) copy+paste mistake: s/KVM_REQUEST_NO_WAKEUP/KVM_REQUEST_WAIT/ > +#define KVM_ARCH_REQ_WAIT_NO_WAKEUP(nr) \ > + KVM_ARCH_REQ_FLAGS(nr, KVM_REQUEST_NO_WAKEUP | KVM_REQUEST_WAIT) > > #define KVM_USERSPACE_IRQ_SOURCE_ID 0 > #define KVM_IRQFD_RESAMPLE_IRQ_SOURCE_ID 1 > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 632f7b3e198c..dbf0410cd8b2 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -165,6 +165,15 @@ void vcpu_put(struct kvm_vcpu *vcpu) > } > EXPORT_SYMBOL_GPL(vcpu_put); > > +/* TODO: merge with kvm_arch_vcpu_should_kick */ > +static bool kvm_should_kick_request(struct kvm_vcpu *vcpu, unsigned req) > +{ > + int mode = kvm_vcpu_exiting_guest_mode(vcpu); > + > + return req & KVM_REQUEST_WAIT ? > + mode != OUTSIDE_GUEST_MODE : mode == IN_GUEST_MODE; > +} > + > static void ack_flush(void *_completed) > { > } > @@ -174,6 +183,7 @@ bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req) > int i, cpu, me; > cpumask_var_t cpus; > bool called = true; > + bool wait = req & KVM_REQUEST_WAIT; > struct kvm_vcpu *vcpu; > > zalloc_cpumask_var(&cpus, GFP_ATOMIC); > @@ -187,13 +197,13 @@ bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req) > continue; > > if (cpus != NULL && cpu != -1 && cpu != me && > - kvm_vcpu_exiting_guest_mode(vcpu) != OUTSIDE_GUEST_MODE) > + kvm_should_kick_request(vcpu, req)) > cpumask_set_cpu(cpu, cpus); > } > if (unlikely(cpus == NULL)) > - smp_call_function_many(cpu_online_mask, ack_flush, NULL, 1); > + smp_call_function_many(cpu_online_mask, ack_flush, NULL, wait); > else if (!cpumask_empty(cpus)) > - smp_call_function_many(cpus, ack_flush, NULL, 1); > + smp_call_function_many(cpus, ack_flush, NULL, wait); > else > called = false; > put_cpu(); > -- > 2.12.2 > Thanks, drew
On 26/04/2017 22:32, Radim Krčmář wrote: > v2: replaces [v1 1/6] > Ugh, KVM_ARCH_REQ_WAIT_NO_WAKEUP looks a weird ... Yeah, let's drop patch 7 and just use bits for now. I think using KVM_ARCH_REQ_FLAGS directly should be fine, especially after the default is flipped from "no wakeup" to "wakeup", but for 4.12 this is the simplest incremental step. > +/* TODO: merge with kvm_arch_vcpu_should_kick */ > +static bool kvm_should_kick_request(struct kvm_vcpu *vcpu, unsigned req) I'm renaming this to kvm_request_needs_ipi; the point of the IPI for synchronous requests is not the "kick", but the "ack" that comes back. Paolo > +{ > + int mode = kvm_vcpu_exiting_guest_mode(vcpu); > + > + return req & KVM_REQUEST_WAIT ? > + mode != OUTSIDE_GUEST_MODE : mode == IN_GUEST_MODE; > +} > +
diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h index 1d48a4b65b86..2190a7ddd515 100644 --- a/arch/arm/include/asm/kvm_host.h +++ b/arch/arm/include/asm/kvm_host.h @@ -44,7 +44,7 @@ #define KVM_MAX_VCPUS VGIC_V2_MAX_CPUS #endif -#define KVM_REQ_VCPU_EXIT KVM_ARCH_REQ_NO_WAKEUP(0) +#define KVM_REQ_VCPU_EXIT KVM_ARCH_REQ_WAIT_NO_WAKEUP(0) u32 *kvm_vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num, u32 mode); int __attribute_const__ kvm_target_cpu(void); diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index d3370b79660e..98f3d01ae91b 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -41,7 +41,7 @@ #define KVM_VCPU_MAX_FEATURES 4 -#define KVM_REQ_VCPU_EXIT KVM_ARCH_REQ_NO_WAKEUP(0) +#define KVM_REQ_VCPU_EXIT KVM_ARCH_REQ_WAIT_NO_WAKEUP(0) int __attribute_const__ kvm_target_cpu(void); int kvm_reset_vcpu(struct kvm_vcpu *vcpu); diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 15eb7d3837e3..77083d7e9540 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -61,10 +61,10 @@ #define KVM_REQ_PMI KVM_ARCH_REQ(11) #define KVM_REQ_SMI KVM_ARCH_REQ(12) #define KVM_REQ_MASTERCLOCK_UPDATE KVM_ARCH_REQ(13) -#define KVM_REQ_MCLOCK_INPROGRESS KVM_ARCH_REQ_NO_WAKEUP(14) -#define KVM_REQ_SCAN_IOAPIC KVM_ARCH_REQ_NO_WAKEUP(15) +#define KVM_REQ_MCLOCK_INPROGRESS KVM_ARCH_REQ_WAIT_NO_WAKEUP(14) +#define KVM_REQ_SCAN_IOAPIC KVM_ARCH_REQ_WAIT_NO_WAKEUP(15) #define KVM_REQ_GLOBAL_CLOCK_UPDATE KVM_ARCH_REQ(16) -#define KVM_REQ_APIC_PAGE_RELOAD KVM_ARCH_REQ_NO_WAKEUP(17) +#define KVM_REQ_APIC_PAGE_RELOAD KVM_ARCH_REQ_WAIT_NO_WAKEUP(17) #define KVM_REQ_HV_CRASH KVM_ARCH_REQ(18) #define KVM_REQ_IOAPIC_EOI_EXIT KVM_ARCH_REQ(19) #define KVM_REQ_HV_RESET KVM_ARCH_REQ(20) diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index 38cfe372918c..a668f33b20dc 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -117,12 +117,13 @@ static inline bool is_error_page(struct page *page) #define KVM_REQUEST_MASK GENMASK(7,0) #define KVM_REQUEST_NO_WAKEUP BIT(8) +#define KVM_REQUEST_WAIT BIT(9) /* * Architecture-independent vcpu->requests bit members * Bits 4-7 are reserved for more arch-independent bits. */ -#define KVM_REQ_TLB_FLUSH (0 | KVM_REQUEST_NO_WAKEUP) -#define KVM_REQ_MMU_RELOAD (1 | KVM_REQUEST_NO_WAKEUP) +#define KVM_REQ_TLB_FLUSH (0 | KVM_REQUEST_NO_WAKEUP | KVM_REQUEST_WAIT) +#define KVM_REQ_MMU_RELOAD (1 | KVM_REQUEST_NO_WAKEUP | KVM_REQUEST_WAIT) #define KVM_REQ_PENDING_TIMER 2 #define KVM_REQ_UNHALT 3 #define KVM_REQUEST_ARCH_BASE 8 @@ -133,6 +134,9 @@ static inline bool is_error_page(struct page *page) }) #define KVM_ARCH_REQ(nr) KVM_ARCH_REQ_FLAGS(nr, 0) #define KVM_ARCH_REQ_NO_WAKEUP(nr) KVM_ARCH_REQ_FLAGS(nr, KVM_REQUEST_NO_WAKEUP) +#define KVM_ARCH_REQ_WAIT(nr) KVM_ARCH_REQ_FLAGS(nr, KVM_REQUEST_NO_WAKEUP) +#define KVM_ARCH_REQ_WAIT_NO_WAKEUP(nr) \ + KVM_ARCH_REQ_FLAGS(nr, KVM_REQUEST_NO_WAKEUP | KVM_REQUEST_WAIT) #define KVM_USERSPACE_IRQ_SOURCE_ID 0 #define KVM_IRQFD_RESAMPLE_IRQ_SOURCE_ID 1 diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 632f7b3e198c..dbf0410cd8b2 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -165,6 +165,15 @@ void vcpu_put(struct kvm_vcpu *vcpu) } EXPORT_SYMBOL_GPL(vcpu_put); +/* TODO: merge with kvm_arch_vcpu_should_kick */ +static bool kvm_should_kick_request(struct kvm_vcpu *vcpu, unsigned req) +{ + int mode = kvm_vcpu_exiting_guest_mode(vcpu); + + return req & KVM_REQUEST_WAIT ? + mode != OUTSIDE_GUEST_MODE : mode == IN_GUEST_MODE; +} + static void ack_flush(void *_completed) { } @@ -174,6 +183,7 @@ bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req) int i, cpu, me; cpumask_var_t cpus; bool called = true; + bool wait = req & KVM_REQUEST_WAIT; struct kvm_vcpu *vcpu; zalloc_cpumask_var(&cpus, GFP_ATOMIC); @@ -187,13 +197,13 @@ bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req) continue; if (cpus != NULL && cpu != -1 && cpu != me && - kvm_vcpu_exiting_guest_mode(vcpu) != OUTSIDE_GUEST_MODE) + kvm_should_kick_request(vcpu, req)) cpumask_set_cpu(cpu, cpus); } if (unlikely(cpus == NULL)) - smp_call_function_many(cpu_online_mask, ack_flush, NULL, 1); + smp_call_function_many(cpu_online_mask, ack_flush, NULL, wait); else if (!cpumask_empty(cpus)) - smp_call_function_many(cpus, ack_flush, NULL, 1); + smp_call_function_many(cpus, ack_flush, NULL, wait); else called = false; put_cpu();
kvm_make_all_requests() provides a synchronization that waits until all kicked VCPUs have acknowledged the kick. This is important for KVM_REQ_MMU_RELOAD as it prevents freeing while lockless paging is underway. This patch adds the synchronization property into all requests that are currently being used with kvm_make_all_requests() in order to preserve the current behavior and only introduce a new framework. Removing it from requests where it is not necessary is left for future patches. A question is whether this propertly isn't better expressed as a function of the caller. Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> --- v2: replaces [v1 1/6] Ugh, KVM_ARCH_REQ_WAIT_NO_WAKEUP looks a weird ... --- arch/arm/include/asm/kvm_host.h | 2 +- arch/arm64/include/asm/kvm_host.h | 2 +- arch/x86/include/asm/kvm_host.h | 6 +++--- include/linux/kvm_host.h | 8 ++++++-- virt/kvm/kvm_main.c | 16 +++++++++++++--- 5 files changed, 24 insertions(+), 10 deletions(-)