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@xxxxxxxxxx>
> ---
> 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
