Login | Register For Free | Help
Search for: (Advanced)

Mailing List Archive: Xen: Devel

[PATCH RFC V6 1/5] kvm hypervisor : Add a hypercall to KVM hypervisor to support pv-ticketlocks

 

 

Xen devel RSS feed   Index | Next | Previous | View Threaded


raghavendra.kt at linux

Apr 23, 2012, 2:59 AM

Post #1 of 16 (530 views)
Permalink
[PATCH RFC V6 1/5] kvm hypervisor : Add a hypercall to KVM hypervisor to support pv-ticketlocks

From: Srivatsa Vaddagiri <vatsa [at] linux>

KVM_HC_KICK_CPU allows the calling vcpu to kick another vcpu out of halt state.

The presence of these hypercalls is indicated to guest via
KVM_FEATURE_PV_UNHALT/KVM_CAP_PV_UNHALT.

Signed-off-by: Srivatsa Vaddagiri <vatsa [at] linux>
Signed-off-by: Suzuki Poulose <suzuki [at] in>
Signed-off-by: Raghavendra K T <raghavendra.kt [at] linux>
---
diff --git a/arch/ia64/include/asm/kvm_host.h b/arch/ia64/include/asm/kvm_host.h
index e35b3a8..3252339 100644
--- a/arch/ia64/include/asm/kvm_host.h
+++ b/arch/ia64/include/asm/kvm_host.h
@@ -597,6 +597,9 @@ void kvm_sal_emul(struct kvm_vcpu *vcpu);
struct kvm *kvm_arch_alloc_vm(void);
void kvm_arch_free_vm(struct kvm *kvm);

+static inline void kvm_arch_vcpu_reset_pv_unhalted(struct kvm_vcpu *vcpu)
+{
+}
#endif /* __ASSEMBLY__*/

#endif
diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
index 52eb9c1..28446de 100644
--- a/arch/powerpc/include/asm/kvm_host.h
+++ b/arch/powerpc/include/asm/kvm_host.h
@@ -498,4 +498,8 @@ struct kvm_vcpu_arch {
#define KVM_MMIO_REG_QPR 0x0040
#define KVM_MMIO_REG_FQPR 0x0060

+static inline void kvm_arch_vcpu_reset_pv_unhalted(struct kvm_vcpu *vcpu)
+{
+}
+
#endif /* __POWERPC_KVM_HOST_H__ */
diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index 7343872..c47f874 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -256,4 +256,8 @@ struct kvm_arch{
};

extern int sie64a(struct kvm_s390_sie_block *, u64 *);
+static inline void kvm_arch_vcpu_reset_pv_unhalted(struct kvm_vcpu *vcpu)
+{
+}
+
#endif
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index e216ba0..dad475b 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -481,6 +481,10 @@ struct kvm_vcpu_arch {
u64 length;
u64 status;
} osvw;
+ /* pv related host specific info */
+ struct {
+ int pv_unhalted;
+ } pv;
};

struct kvm_lpage_info {
@@ -967,4 +971,6 @@ int kvm_pmu_read_pmc(struct kvm_vcpu *vcpu, unsigned pmc, u64 *data);
void kvm_handle_pmu_event(struct kvm_vcpu *vcpu);
void kvm_deliver_pmi(struct kvm_vcpu *vcpu);

+void kvm_arch_vcpu_reset_pv_unhalted(struct kvm_vcpu *vcpu);
+
#endif /* _ASM_X86_KVM_HOST_H */
diff --git a/arch/x86/include/asm/kvm_para.h b/arch/x86/include/asm/kvm_para.h
index 734c376..5b647ea 100644
--- a/arch/x86/include/asm/kvm_para.h
+++ b/arch/x86/include/asm/kvm_para.h
@@ -16,12 +16,14 @@
#define KVM_FEATURE_CLOCKSOURCE 0
#define KVM_FEATURE_NOP_IO_DELAY 1
#define KVM_FEATURE_MMU_OP 2
+
/* This indicates that the new set of kvmclock msrs
* are available. The use of 0x11 and 0x12 is deprecated
*/
#define KVM_FEATURE_CLOCKSOURCE2 3
#define KVM_FEATURE_ASYNC_PF 4
#define KVM_FEATURE_STEAL_TIME 5
+#define KVM_FEATURE_PV_UNHALT 6

/* The last 8 bits are used to indicate how to interpret the flags field
* in pvclock structure. If no bits are set, all flags are ignored.
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 9fed5be..7c93806 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -408,7 +408,8 @@ static int do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
(1 << KVM_FEATURE_NOP_IO_DELAY) |
(1 << KVM_FEATURE_CLOCKSOURCE2) |
(1 << KVM_FEATURE_ASYNC_PF) |
- (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT);
+ (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT) |
+ (1 << KVM_FEATURE_PV_UNHALT);

if (sched_info_on())
entry->eax |= (1 << KVM_FEATURE_STEAL_TIME);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 4044ce0..7fc9be6 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2147,6 +2147,7 @@ int kvm_dev_ioctl_check_extension(long ext)
case KVM_CAP_ASYNC_PF:
case KVM_CAP_GET_TSC_KHZ:
case KVM_CAP_PCI_2_3:
+ case KVM_CAP_PV_UNHALT:
r = 1;
break;
case KVM_CAP_COALESCED_MMIO:
@@ -4993,6 +4994,36 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu)
return 1;
}

+/*
+ * kvm_pv_kick_cpu_op: Kick a vcpu.
+ *
+ * @apicid - apicid of vcpu to be kicked.
+ */
+static void kvm_pv_kick_cpu_op(struct kvm *kvm, int apicid)
+{
+ struct kvm_vcpu *vcpu = NULL;
+ int i;
+
+ kvm_for_each_vcpu(i, vcpu, kvm) {
+ if (!kvm_apic_present(vcpu))
+ continue;
+
+ if (kvm_apic_match_dest(vcpu, 0, 0, apicid, 0))
+ break;
+ }
+ if (vcpu) {
+ /*
+ * Setting unhalt flag here can result in spurious runnable
+ * state when unhalt reset does not happen in vcpu_block.
+ * But that is harmless since that should soon result in halt.
+ */
+ vcpu->arch.pv.pv_unhalted = 1;
+ /* We need everybody see unhalt before vcpu unblocks */
+ smp_mb();
+ kvm_vcpu_kick(vcpu);
+ }
+}
+
int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
{
unsigned long nr, a0, a1, a2, a3, ret;
@@ -5026,6 +5057,10 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
case KVM_HC_VAPIC_POLL_IRQ:
ret = 0;
break;
+ case KVM_HC_KICK_CPU:
+ kvm_pv_kick_cpu_op(vcpu->kvm, a0);
+ ret = 0;
+ break;
default:
ret = -KVM_ENOSYS;
break;
@@ -6128,6 +6163,7 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
BUG_ON(vcpu->kvm == NULL);
kvm = vcpu->kvm;

+ kvm_arch_vcpu_reset_pv_unhalted(vcpu);
vcpu->arch.emulate_ctxt.ops = &emulate_ops;
if (!irqchip_in_kernel(kvm) || kvm_vcpu_is_bsp(vcpu))
vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
@@ -6398,11 +6434,17 @@ int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)
!vcpu->arch.apf.halted)
|| !list_empty_careful(&vcpu->async_pf.done)
|| vcpu->arch.mp_state == KVM_MP_STATE_SIPI_RECEIVED
+ || vcpu->arch.pv.pv_unhalted
|| atomic_read(&vcpu->arch.nmi_queued) ||
(kvm_arch_interrupt_allowed(vcpu) &&
kvm_cpu_has_interrupt(vcpu));
}

+void kvm_arch_vcpu_reset_pv_unhalted(struct kvm_vcpu *vcpu)
+{
+ vcpu->arch.pv.pv_unhalted = 0;
+}
+
void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
{
int me;
diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index 6c322a9..a189f02 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -589,6 +589,7 @@ struct kvm_ppc_pvinfo {
#define KVM_CAP_S390_UCONTROL 73
#define KVM_CAP_SYNC_REGS 74
#define KVM_CAP_PCI_2_3 75
+#define KVM_CAP_PV_UNHALT 76

#ifdef KVM_CAP_IRQ_ROUTING

diff --git a/include/linux/kvm_para.h b/include/linux/kvm_para.h
index ff476dd..38226e1 100644
--- a/include/linux/kvm_para.h
+++ b/include/linux/kvm_para.h
@@ -19,6 +19,7 @@
#define KVM_HC_MMU_OP 2
#define KVM_HC_FEATURES 3
#define KVM_HC_PPC_MAP_MAGIC_PAGE 4
+#define KVM_HC_KICK_CPU 5

/*
* hypercalls use architecture specific
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 42b7393..edf56d4 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1500,6 +1500,14 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)
prepare_to_wait(&vcpu->wq, &wait, TASK_INTERRUPTIBLE);

if (kvm_arch_vcpu_runnable(vcpu)) {
+ /*
+ * This is the only safe place to reset unhalt flag.
+ * otherwise it results in loosing the notification
+ * which eventually can result in vcpu hangs.
+ */
+ kvm_arch_vcpu_reset_pv_unhalted(vcpu);
+ /* preventing reordering should be enough here */
+ barrier();
kvm_make_request(KVM_REQ_UNHALT, vcpu);
break;
}


_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel


gleb at redhat

Apr 24, 2012, 2:59 AM

Post #2 of 16 (517 views)
Permalink
Re: [PATCH RFC V6 1/5] kvm hypervisor : Add a hypercall to KVM hypervisor to support pv-ticketlocks [In reply to]

On Mon, Apr 23, 2012 at 03:29:47PM +0530, Raghavendra K T wrote:
> From: Srivatsa Vaddagiri <vatsa [at] linux>
>
> KVM_HC_KICK_CPU allows the calling vcpu to kick another vcpu out of halt state.
>
> The presence of these hypercalls is indicated to guest via
> KVM_FEATURE_PV_UNHALT/KVM_CAP_PV_UNHALT.
>
> Signed-off-by: Srivatsa Vaddagiri <vatsa [at] linux>
> Signed-off-by: Suzuki Poulose <suzuki [at] in>
> Signed-off-by: Raghavendra K T <raghavendra.kt [at] linux>
> ---
> diff --git a/arch/ia64/include/asm/kvm_host.h b/arch/ia64/include/asm/kvm_host.h
> index e35b3a8..3252339 100644
> --- a/arch/ia64/include/asm/kvm_host.h
> +++ b/arch/ia64/include/asm/kvm_host.h
> @@ -597,6 +597,9 @@ void kvm_sal_emul(struct kvm_vcpu *vcpu);
> struct kvm *kvm_arch_alloc_vm(void);
> void kvm_arch_free_vm(struct kvm *kvm);
>
> +static inline void kvm_arch_vcpu_reset_pv_unhalted(struct kvm_vcpu *vcpu)
> +{
> +}
> #endif /* __ASSEMBLY__*/
>
> #endif
> diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
> index 52eb9c1..28446de 100644
> --- a/arch/powerpc/include/asm/kvm_host.h
> +++ b/arch/powerpc/include/asm/kvm_host.h
> @@ -498,4 +498,8 @@ struct kvm_vcpu_arch {
> #define KVM_MMIO_REG_QPR 0x0040
> #define KVM_MMIO_REG_FQPR 0x0060
>
> +static inline void kvm_arch_vcpu_reset_pv_unhalted(struct kvm_vcpu *vcpu)
> +{
> +}
> +
> #endif /* __POWERPC_KVM_HOST_H__ */
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index 7343872..c47f874 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -256,4 +256,8 @@ struct kvm_arch{
> };
>
> extern int sie64a(struct kvm_s390_sie_block *, u64 *);
> +static inline void kvm_arch_vcpu_reset_pv_unhalted(struct kvm_vcpu *vcpu)
> +{
> +}
> +
> #endif
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index e216ba0..dad475b 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -481,6 +481,10 @@ struct kvm_vcpu_arch {
> u64 length;
> u64 status;
> } osvw;
> + /* pv related host specific info */
> + struct {
> + int pv_unhalted;
> + } pv;
> };
>
> struct kvm_lpage_info {
> @@ -967,4 +971,6 @@ int kvm_pmu_read_pmc(struct kvm_vcpu *vcpu, unsigned pmc, u64 *data);
> void kvm_handle_pmu_event(struct kvm_vcpu *vcpu);
> void kvm_deliver_pmi(struct kvm_vcpu *vcpu);
>
> +void kvm_arch_vcpu_reset_pv_unhalted(struct kvm_vcpu *vcpu);
> +
> #endif /* _ASM_X86_KVM_HOST_H */
> diff --git a/arch/x86/include/asm/kvm_para.h b/arch/x86/include/asm/kvm_para.h
> index 734c376..5b647ea 100644
> --- a/arch/x86/include/asm/kvm_para.h
> +++ b/arch/x86/include/asm/kvm_para.h
> @@ -16,12 +16,14 @@
> #define KVM_FEATURE_CLOCKSOURCE 0
> #define KVM_FEATURE_NOP_IO_DELAY 1
> #define KVM_FEATURE_MMU_OP 2
> +
> /* This indicates that the new set of kvmclock msrs
> * are available. The use of 0x11 and 0x12 is deprecated
> */
> #define KVM_FEATURE_CLOCKSOURCE2 3
> #define KVM_FEATURE_ASYNC_PF 4
> #define KVM_FEATURE_STEAL_TIME 5
> +#define KVM_FEATURE_PV_UNHALT 6
>
> /* The last 8 bits are used to indicate how to interpret the flags field
> * in pvclock structure. If no bits are set, all flags are ignored.
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 9fed5be..7c93806 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -408,7 +408,8 @@ static int do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
> (1 << KVM_FEATURE_NOP_IO_DELAY) |
> (1 << KVM_FEATURE_CLOCKSOURCE2) |
> (1 << KVM_FEATURE_ASYNC_PF) |
> - (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT);
> + (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT) |
> + (1 << KVM_FEATURE_PV_UNHALT);
>
> if (sched_info_on())
> entry->eax |= (1 << KVM_FEATURE_STEAL_TIME);
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 4044ce0..7fc9be6 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2147,6 +2147,7 @@ int kvm_dev_ioctl_check_extension(long ext)
> case KVM_CAP_ASYNC_PF:
> case KVM_CAP_GET_TSC_KHZ:
> case KVM_CAP_PCI_2_3:
> + case KVM_CAP_PV_UNHALT:
> r = 1;
> break;
> case KVM_CAP_COALESCED_MMIO:
> @@ -4993,6 +4994,36 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu)
> return 1;
> }
>
> +/*
> + * kvm_pv_kick_cpu_op: Kick a vcpu.
> + *
> + * @apicid - apicid of vcpu to be kicked.
> + */
> +static void kvm_pv_kick_cpu_op(struct kvm *kvm, int apicid)
> +{
> + struct kvm_vcpu *vcpu = NULL;
> + int i;
> +
> + kvm_for_each_vcpu(i, vcpu, kvm) {
> + if (!kvm_apic_present(vcpu))
> + continue;
> +
> + if (kvm_apic_match_dest(vcpu, 0, 0, apicid, 0))
> + break;
> + }
> + if (vcpu) {
> + /*
> + * Setting unhalt flag here can result in spurious runnable
> + * state when unhalt reset does not happen in vcpu_block.
> + * But that is harmless since that should soon result in halt.
> + */
> + vcpu->arch.pv.pv_unhalted = 1;
> + /* We need everybody see unhalt before vcpu unblocks */
> + smp_mb();
> + kvm_vcpu_kick(vcpu);
> + }
> +}
This is too similar to kvm_irq_delivery_to_apic(). Why not reuse it. We
can use one of reserved delivery modes as PV delivery mode. We will
disallow guest to trigger it through apic interface, so this will not be
part of ABI and can be changed at will.

> +
> int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
> {
> unsigned long nr, a0, a1, a2, a3, ret;
> @@ -5026,6 +5057,10 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
> case KVM_HC_VAPIC_POLL_IRQ:
> ret = 0;
> break;
> + case KVM_HC_KICK_CPU:
> + kvm_pv_kick_cpu_op(vcpu->kvm, a0);
> + ret = 0;
> + break;
> default:
> ret = -KVM_ENOSYS;
> break;
> @@ -6128,6 +6163,7 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
> BUG_ON(vcpu->kvm == NULL);
> kvm = vcpu->kvm;
>
> + kvm_arch_vcpu_reset_pv_unhalted(vcpu);
> vcpu->arch.emulate_ctxt.ops = &emulate_ops;
> if (!irqchip_in_kernel(kvm) || kvm_vcpu_is_bsp(vcpu))
> vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
> @@ -6398,11 +6434,17 @@ int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)
> !vcpu->arch.apf.halted)
> || !list_empty_careful(&vcpu->async_pf.done)
> || vcpu->arch.mp_state == KVM_MP_STATE_SIPI_RECEIVED
> + || vcpu->arch.pv.pv_unhalted
> || atomic_read(&vcpu->arch.nmi_queued) ||
> (kvm_arch_interrupt_allowed(vcpu) &&
> kvm_cpu_has_interrupt(vcpu));
> }
>
> +void kvm_arch_vcpu_reset_pv_unhalted(struct kvm_vcpu *vcpu)
> +{
> + vcpu->arch.pv.pv_unhalted = 0;
> +}
> +
> void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
> {
> int me;
> diff --git a/include/linux/kvm.h b/include/linux/kvm.h
> index 6c322a9..a189f02 100644
> --- a/include/linux/kvm.h
> +++ b/include/linux/kvm.h
> @@ -589,6 +589,7 @@ struct kvm_ppc_pvinfo {
> #define KVM_CAP_S390_UCONTROL 73
> #define KVM_CAP_SYNC_REGS 74
> #define KVM_CAP_PCI_2_3 75
> +#define KVM_CAP_PV_UNHALT 76
>
> #ifdef KVM_CAP_IRQ_ROUTING
>
> diff --git a/include/linux/kvm_para.h b/include/linux/kvm_para.h
> index ff476dd..38226e1 100644
> --- a/include/linux/kvm_para.h
> +++ b/include/linux/kvm_para.h
> @@ -19,6 +19,7 @@
> #define KVM_HC_MMU_OP 2
> #define KVM_HC_FEATURES 3
> #define KVM_HC_PPC_MAP_MAGIC_PAGE 4
> +#define KVM_HC_KICK_CPU 5
>
> /*
> * hypercalls use architecture specific
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 42b7393..edf56d4 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -1500,6 +1500,14 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)
> prepare_to_wait(&vcpu->wq, &wait, TASK_INTERRUPTIBLE);
>
> if (kvm_arch_vcpu_runnable(vcpu)) {
> + /*
> + * This is the only safe place to reset unhalt flag.
> + * otherwise it results in loosing the notification
> + * which eventually can result in vcpu hangs.
> + */
Why this is the only safe place? Why clearing it in kvm/x86.c just after
call to kvm_vcpu_block() if KVM_REQ_UNHALT is set is not safe enough?

> + kvm_arch_vcpu_reset_pv_unhalted(vcpu);
> + /* preventing reordering should be enough here */
> + barrier();
> kvm_make_request(KVM_REQ_UNHALT, vcpu);
> break;
> }

--
Gleb.

_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel


raghavendra.kt at linux

Apr 26, 2012, 1:11 AM

Post #3 of 16 (515 views)
Permalink
Re: [PATCH RFC V6 1/5] kvm hypervisor : Add a hypercall to KVM hypervisor to support pv-ticketlocks [In reply to]

On 04/24/2012 03:29 PM, Gleb Natapov wrote:
> On Mon, Apr 23, 2012 at 03:29:47PM +0530, Raghavendra K T wrote:
>> From: Srivatsa Vaddagiri<vatsa [at] linux>
[...]
>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> index 42b7393..edf56d4 100644
>> --- a/virt/kvm/kvm_main.c
>> +++ b/virt/kvm/kvm_main.c
>> @@ -1500,6 +1500,14 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)
>> prepare_to_wait(&vcpu->wq,&wait, TASK_INTERRUPTIBLE);
>>
>> if (kvm_arch_vcpu_runnable(vcpu)) {
>> + /*
>> + * This is the only safe place to reset unhalt flag.
>> + * otherwise it results in loosing the notification
>> + * which eventually can result in vcpu hangs.
>> + */
> Why this is the only safe place? Why clearing it in kvm/x86.c just after
> call to kvm_vcpu_block() if KVM_REQ_UNHALT is set is not safe enough?
>

Yes, You are Right. The acceptable window to reset the flag can be
extended till there. and Good point about that is it removes the need
for having the stubs for other archs and simplifies everything a lot.
Thanks for that.

[. When I was experimenting with request bit, clearing request bit in the
same place was causing vm hang after some 16 iteration of stress test. I
had carried the same impression. Now I have done stress testing
to ensure that the change works. Basically as you know, my fear was
loosing kick(s) leads to vm hang eventually. ]


_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel


raghavendra.kt at linux

Apr 27, 2012, 3:45 AM

Post #4 of 16 (512 views)
Permalink
Re: [PATCH RFC V6 1/5] kvm hypervisor : Add a hypercall to KVM hypervisor to support pv-ticketlocks [In reply to]

On 04/24/2012 03:29 PM, Gleb Natapov wrote:
> On Mon, Apr 23, 2012 at 03:29:47PM +0530, Raghavendra K T wrote:
>> From: Srivatsa Vaddagiri<vatsa [at] linux>
>>
>> KVM_HC_KICK_CPU allows the calling vcpu to kick another vcpu out of halt state.
>>
>> The presence of these hypercalls is indicated to guest via
>> KVM_FEATURE_PV_UNHALT/KVM_CAP_PV_UNHALT.
>>
>> Signed-off-by: Srivatsa Vaddagiri<vatsa [at] linux>
>> Signed-off-by: Suzuki Poulose<suzuki [at] in>
>> Signed-off-by: Raghavendra K T<raghavendra.kt [at] linux>
>> ---
[...]
>> +/*
>> + * kvm_pv_kick_cpu_op: Kick a vcpu.
>> + *
>> + * @apicid - apicid of vcpu to be kicked.
>> + */
>> +static void kvm_pv_kick_cpu_op(struct kvm *kvm, int apicid)
>> +{
>> + struct kvm_vcpu *vcpu = NULL;
>> + int i;
>> +
>> + kvm_for_each_vcpu(i, vcpu, kvm) {
>> + if (!kvm_apic_present(vcpu))
>> + continue;
>> +
>> + if (kvm_apic_match_dest(vcpu, 0, 0, apicid, 0))
>> + break;
>> + }
>> + if (vcpu) {
>> + /*
>> + * Setting unhalt flag here can result in spurious runnable
>> + * state when unhalt reset does not happen in vcpu_block.
>> + * But that is harmless since that should soon result in halt.
>> + */
>> + vcpu->arch.pv.pv_unhalted = 1;
>> + /* We need everybody see unhalt before vcpu unblocks */
>> + smp_mb();
>> + kvm_vcpu_kick(vcpu);
>> + }
>> +}
> This is too similar to kvm_irq_delivery_to_apic(). Why not reuse it. We
> can use one of reserved delivery modes as PV delivery mode. We will
> disallow guest to trigger it through apic interface, so this will not be
> part of ABI and can be changed at will.
>

I think it is interesting ( Perhaps more reasonable way of doing it).
I am not too familiar with lapic source. So, pardon me if my questions
are stupid.

Something like below is what I deciphered from your suggestion which is
working.

kvm/x86.c
=========
kvm_pv_kick_cpu_op()
{

struct kvm_lapic_irq lapic_irq;

lapic_irq.shorthand = 0;
lapic_irq.dest_mode = 0;
lapic_irq.dest_id = apicid;

lapic_irq.delivery_mode = PV_DELIVERY_MODE;
kvm_irq_delivery_to_apic(kvm, 0, &lapic_irq );

}

kvm/lapic.c
==========
_apic_accept_irq()
{
...
case APIC_DM_REMRD:
result = 1;
vcpu->pv_unhalted = 1
smp_mb();
kvm_make_request(KVM_REQ_EVENT, vcpu);
kvm_vcpu_kick(vcpu);
break;

...
}

here using PV_DELIVERY_MODE = APIC_DM_REMRD, which was unused.

OR
1) Are you asking to remove vcpu->pv_unhalted flag and replace with an irq?
2) are you talking about some reserved fields in struct local_apic instead
of APIC_DM_REMRD what I have used above?
[. Honestly, arch/x86/include/asm/apicdef.h had too much of info to
digest :( ]

3) I am not sure about: disallow guest to trigger it through apic
interface part also.(mean howto?)
4) And one more question, So you think it takes care of migration part
(in case we are removing pv_unhalted flag)?

It would be helpful if you can give little more explanation/ pointer to
Documentation.

Ingo is keen to see whole ticketlock/Xen/KVM patch in one go.
and anyhow since this does not cause any ABI change, I hope you don't
mind if
I do only the vcpu->pv_unhalted change you suggested now [. having
pv_unhalted reset in vcpu_run if
you meant something else than code I have above ], so that whole series
get fair amount time for testing.


_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel


gleb at redhat

Apr 27, 2012, 8:53 AM

Post #5 of 16 (520 views)
Permalink
Re: [PATCH RFC V6 1/5] kvm hypervisor : Add a hypercall to KVM hypervisor to support pv-ticketlocks [In reply to]

On Fri, Apr 27, 2012 at 04:15:35PM +0530, Raghavendra K T wrote:
> On 04/24/2012 03:29 PM, Gleb Natapov wrote:
> >On Mon, Apr 23, 2012 at 03:29:47PM +0530, Raghavendra K T wrote:
> >>From: Srivatsa Vaddagiri<vatsa [at] linux>
> >>
> >>KVM_HC_KICK_CPU allows the calling vcpu to kick another vcpu out of halt state.
> >>
> >>The presence of these hypercalls is indicated to guest via
> >>KVM_FEATURE_PV_UNHALT/KVM_CAP_PV_UNHALT.
> >>
> >>Signed-off-by: Srivatsa Vaddagiri<vatsa [at] linux>
> >>Signed-off-by: Suzuki Poulose<suzuki [at] in>
> >>Signed-off-by: Raghavendra K T<raghavendra.kt [at] linux>
> >>---
> [...]
> >>+/*
> >>+ * kvm_pv_kick_cpu_op: Kick a vcpu.
> >>+ *
> >>+ * @apicid - apicid of vcpu to be kicked.
> >>+ */
> >>+static void kvm_pv_kick_cpu_op(struct kvm *kvm, int apicid)
> >>+{
> >>+ struct kvm_vcpu *vcpu = NULL;
> >>+ int i;
> >>+
> >>+ kvm_for_each_vcpu(i, vcpu, kvm) {
> >>+ if (!kvm_apic_present(vcpu))
> >>+ continue;
> >>+
> >>+ if (kvm_apic_match_dest(vcpu, 0, 0, apicid, 0))
> >>+ break;
> >>+ }
> >>+ if (vcpu) {
> >>+ /*
> >>+ * Setting unhalt flag here can result in spurious runnable
> >>+ * state when unhalt reset does not happen in vcpu_block.
> >>+ * But that is harmless since that should soon result in halt.
> >>+ */
> >>+ vcpu->arch.pv.pv_unhalted = 1;
> >>+ /* We need everybody see unhalt before vcpu unblocks */
> >>+ smp_mb();
> >>+ kvm_vcpu_kick(vcpu);
> >>+ }
> >>+}
> >This is too similar to kvm_irq_delivery_to_apic(). Why not reuse it. We
> >can use one of reserved delivery modes as PV delivery mode. We will
> >disallow guest to trigger it through apic interface, so this will not be
> >part of ABI and can be changed at will.
> >
>
> I think it is interesting ( Perhaps more reasonable way of doing it).
> I am not too familiar with lapic source. So, pardon me if my
> questions are stupid.
>
> Something like below is what I deciphered from your suggestion which
> is working.
>
> kvm/x86.c
> =========
> kvm_pv_kick_cpu_op()
> {
>
> struct kvm_lapic_irq lapic_irq;
>
> lapic_irq.shorthand = 0;
> lapic_irq.dest_mode = 0;
> lapic_irq.dest_id = apicid;
>
> lapic_irq.delivery_mode = PV_DELIVERY_MODE;
> kvm_irq_delivery_to_apic(kvm, 0, &lapic_irq );
>
> }
>
> kvm/lapic.c
> ==========
> _apic_accept_irq()
> {
> ...
> case APIC_DM_REMRD:
> result = 1;
> vcpu->pv_unhalted = 1
> smp_mb();
> kvm_make_request(KVM_REQ_EVENT, vcpu);
> kvm_vcpu_kick(vcpu);
> break;
>
> ...
> }
>
> here using PV_DELIVERY_MODE = APIC_DM_REMRD, which was unused.
>
Yes, this is what I mean except that PV_DELIVERY_MODE should be
number defined as reserved by Intel spec.

> OR
> 1) Are you asking to remove vcpu->pv_unhalted flag and replace with an irq?
I would like to remove vcpu->pv_unhalted, but do not see how to do that,
so I do not asking that :)

> 2) are you talking about some reserved fields in struct local_apic instead
> of APIC_DM_REMRD what I have used above?
Delivery modes 011b and 111b are reserved. We can use one if them.

> [. Honestly, arch/x86/include/asm/apicdef.h had too much of info to
> digest :( ]
>
> 3) I am not sure about: disallow guest to trigger it through apic
> interface part also.(mean howto?)
I mean we should disallow guest to set delivery mode to reserved values
through apic interface.

> 4) And one more question, So you think it takes care of migration part
> (in case we are removing pv_unhalted flag)?
No, since I am not asking for removing pv_unhalted flag. I want to reuse
code that we already have to deliver the unhalt event.

>
> It would be helpful if you can give little more explanation/ pointer
> to Documentation.
>
> Ingo is keen to see whole ticketlock/Xen/KVM patch in one go.
> and anyhow since this does not cause any ABI change, I hope you
> don't mind if
> I do only the vcpu->pv_unhalted change you suggested now [. having
> pv_unhalted reset in vcpu_run if
> you meant something else than code I have above ], so that whole
> series get fair amount time for testing.

--
Gleb.

_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel


avi at redhat

Apr 29, 2012, 6:18 AM

Post #6 of 16 (513 views)
Permalink
Re: [PATCH RFC V6 1/5] kvm hypervisor : Add a hypercall to KVM hypervisor to support pv-ticketlocks [In reply to]

On 04/24/2012 12:59 PM, Gleb Natapov wrote:
> >
> > +/*
> > + * kvm_pv_kick_cpu_op: Kick a vcpu.
> > + *
> > + * @apicid - apicid of vcpu to be kicked.
> > + */
> > +static void kvm_pv_kick_cpu_op(struct kvm *kvm, int apicid)
> > +{
> > + struct kvm_vcpu *vcpu = NULL;
> > + int i;
> > +
> > + kvm_for_each_vcpu(i, vcpu, kvm) {
> > + if (!kvm_apic_present(vcpu))
> > + continue;
> > +
> > + if (kvm_apic_match_dest(vcpu, 0, 0, apicid, 0))
> > + break;
> > + }
> > + if (vcpu) {
> > + /*
> > + * Setting unhalt flag here can result in spurious runnable
> > + * state when unhalt reset does not happen in vcpu_block.
> > + * But that is harmless since that should soon result in halt.
> > + */
> > + vcpu->arch.pv.pv_unhalted = 1;
> > + /* We need everybody see unhalt before vcpu unblocks */
> > + smp_mb();
> > + kvm_vcpu_kick(vcpu);
> > + }
> > +}
> This is too similar to kvm_irq_delivery_to_apic(). Why not reuse it. We
> can use one of reserved delivery modes as PV delivery mode. We will
> disallow guest to trigger it through apic interface, so this will not be
> part of ABI and can be changed at will.
>

I'm not thrilled about this. Those delivery modes will eventually
become unreserved. We can have a kvm_lookup_apic_id() that is shared
among implementations.

--
error compiling committee.c: too many arguments to function


_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel


gleb at redhat

Apr 29, 2012, 6:20 AM

Post #7 of 16 (511 views)
Permalink
Re: [PATCH RFC V6 1/5] kvm hypervisor : Add a hypercall to KVM hypervisor to support pv-ticketlocks [In reply to]

On Sun, Apr 29, 2012 at 04:18:03PM +0300, Avi Kivity wrote:
> On 04/24/2012 12:59 PM, Gleb Natapov wrote:
> > >
> > > +/*
> > > + * kvm_pv_kick_cpu_op: Kick a vcpu.
> > > + *
> > > + * @apicid - apicid of vcpu to be kicked.
> > > + */
> > > +static void kvm_pv_kick_cpu_op(struct kvm *kvm, int apicid)
> > > +{
> > > + struct kvm_vcpu *vcpu = NULL;
> > > + int i;
> > > +
> > > + kvm_for_each_vcpu(i, vcpu, kvm) {
> > > + if (!kvm_apic_present(vcpu))
> > > + continue;
> > > +
> > > + if (kvm_apic_match_dest(vcpu, 0, 0, apicid, 0))
> > > + break;
> > > + }
> > > + if (vcpu) {
> > > + /*
> > > + * Setting unhalt flag here can result in spurious runnable
> > > + * state when unhalt reset does not happen in vcpu_block.
> > > + * But that is harmless since that should soon result in halt.
> > > + */
> > > + vcpu->arch.pv.pv_unhalted = 1;
> > > + /* We need everybody see unhalt before vcpu unblocks */
> > > + smp_mb();
> > > + kvm_vcpu_kick(vcpu);
> > > + }
> > > +}
> > This is too similar to kvm_irq_delivery_to_apic(). Why not reuse it. We
> > can use one of reserved delivery modes as PV delivery mode. We will
> > disallow guest to trigger it through apic interface, so this will not be
> > part of ABI and can be changed at will.
> >
>
> I'm not thrilled about this. Those delivery modes will eventually
> become unreserved. We can have a kvm_lookup_apic_id() that is shared
> among implementations.
>
This is only internal implementation. If they become unreserved we will
use something else.

--
Gleb.

_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel


avi at redhat

Apr 29, 2012, 6:25 AM

Post #8 of 16 (514 views)
Permalink
Re: [PATCH RFC V6 1/5] kvm hypervisor : Add a hypercall to KVM hypervisor to support pv-ticketlocks [In reply to]

On 04/23/2012 12:59 PM, Raghavendra K T wrote:
> From: Srivatsa Vaddagiri <vatsa [at] linux>
>
> KVM_HC_KICK_CPU allows the calling vcpu to kick another vcpu out of halt state.
>
> The presence of these hypercalls is indicated to guest via
> KVM_FEATURE_PV_UNHALT/KVM_CAP_PV_UNHALT.
>
> #endif
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index e216ba0..dad475b 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -481,6 +481,10 @@ struct kvm_vcpu_arch {
> u64 length;
> u64 status;
> } osvw;
> + /* pv related host specific info */
> + struct {
> + int pv_unhalted;
> + } pv;
> };

'bool'. Or maybe push into vcpu->requests.

>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 4044ce0..7fc9be6 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2147,6 +2147,7 @@ int kvm_dev_ioctl_check_extension(long ext)
> case KVM_CAP_ASYNC_PF:
> case KVM_CAP_GET_TSC_KHZ:
> case KVM_CAP_PCI_2_3:
> + case KVM_CAP_PV_UNHALT:
> r = 1;
> break;
> case KVM_CAP_COALESCED_MMIO:

Redundant, since we can infer this from KVM_GET_SUPPORTED_CPUID. But
please indicate this in the documentation.

>
> +/*
> + * kvm_pv_kick_cpu_op: Kick a vcpu.
> + *
> + * @apicid - apicid of vcpu to be kicked.
> + */
> +static void kvm_pv_kick_cpu_op(struct kvm *kvm, int apicid)
> +{
> + struct kvm_vcpu *vcpu = NULL;
> + int i;
> +
> + kvm_for_each_vcpu(i, vcpu, kvm) {
> + if (!kvm_apic_present(vcpu))
> + continue;
> +
> + if (kvm_apic_match_dest(vcpu, 0, 0, apicid, 0))
> + break;
> + }
> + if (vcpu) {
> + /*
> + * Setting unhalt flag here can result in spurious runnable
> + * state when unhalt reset does not happen in vcpu_block.
> + * But that is harmless since that should soon result in halt.
> + */
> + vcpu->arch.pv.pv_unhalted = 1;
> + /* We need everybody see unhalt before vcpu unblocks */
> + smp_mb();

smp_wmb().

> + kvm_vcpu_kick(vcpu);
> + }
> +}
> +
>
> /*
> * hypercalls use architecture specific
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 42b7393..edf56d4 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -1500,6 +1500,14 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)
> prepare_to_wait(&vcpu->wq, &wait, TASK_INTERRUPTIBLE);
>
> if (kvm_arch_vcpu_runnable(vcpu)) {
> + /*
> + * This is the only safe place to reset unhalt flag.
> + * otherwise it results in loosing the notification
> + * which eventually can result in vcpu hangs.
> + */
> + kvm_arch_vcpu_reset_pv_unhalted(vcpu);
> + /* preventing reordering should be enough here */
> + barrier();
> kvm_make_request(KVM_REQ_UNHALT, vcpu);
> break;
> }
>

Hm, what about reusing KVM_REQ_UNHALT?

--
error compiling committee.c: too many arguments to function


_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel


avi at redhat

Apr 29, 2012, 6:26 AM

Post #9 of 16 (514 views)
Permalink
Re: [PATCH RFC V6 1/5] kvm hypervisor : Add a hypercall to KVM hypervisor to support pv-ticketlocks [In reply to]

On 04/29/2012 04:20 PM, Gleb Natapov wrote:
> > > This is too similar to kvm_irq_delivery_to_apic(). Why not reuse it. We
> > > can use one of reserved delivery modes as PV delivery mode. We will
> > > disallow guest to trigger it through apic interface, so this will not be
> > > part of ABI and can be changed at will.
> > >
> >
> > I'm not thrilled about this. Those delivery modes will eventually
> > become unreserved. We can have a kvm_lookup_apic_id() that is shared
> > among implementations.
> >
> This is only internal implementation. If they become unreserved we will
> use something else.
>

Yeah, I'm thinking of that time. Why do something temporary and fragile?

--
error compiling committee.c: too many arguments to function


_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel


gleb at redhat

Apr 29, 2012, 6:52 AM

Post #10 of 16 (513 views)
Permalink
Re: [PATCH RFC V6 1/5] kvm hypervisor : Add a hypercall to KVM hypervisor to support pv-ticketlocks [In reply to]

On Sun, Apr 29, 2012 at 04:26:21PM +0300, Avi Kivity wrote:
> On 04/29/2012 04:20 PM, Gleb Natapov wrote:
> > > > This is too similar to kvm_irq_delivery_to_apic(). Why not reuse it. We
> > > > can use one of reserved delivery modes as PV delivery mode. We will
> > > > disallow guest to trigger it through apic interface, so this will not be
> > > > part of ABI and can be changed at will.
> > > >
> > >
> > > I'm not thrilled about this. Those delivery modes will eventually
> > > become unreserved. We can have a kvm_lookup_apic_id() that is shared
> > > among implementations.
> > >
> > This is only internal implementation. If they become unreserved we will
> > use something else.
> >
>
> Yeah, I'm thinking of that time. Why do something temporary and fragile?
>
Why is it fragile? Just by unreserving the value Intel will not break
KVM. Only when KVM will implement apic feature that unreserves the value
we will have to change internal implementation and use another value,
but this will be done by the same patch that does unreserving. The
unreserving may even never happen. Meanwhile kvm_irq_delivery_to_apic()
will likely be optimized to use hash for unicast delivery and unhalt
hypercall will benefit from it immediately.

--
Gleb.

_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel


raghavendra.kt at linux

Apr 30, 2012, 12:44 AM

Post #11 of 16 (512 views)
Permalink
Re: [PATCH RFC V6 1/5] kvm hypervisor : Add a hypercall to KVM hypervisor to support pv-ticketlocks [In reply to]

Thanks Avi, for the review.

On 04/29/2012 06:55 PM, Avi Kivity wrote:
> On 04/23/2012 12:59 PM, Raghavendra K T wrote:
>> From: Srivatsa Vaddagiri<vatsa [at] linux>
>>
>> KVM_HC_KICK_CPU allows the calling vcpu to kick another vcpu out of halt state.
>>
>> The presence of these hypercalls is indicated to guest via
>> KVM_FEATURE_PV_UNHALT/KVM_CAP_PV_UNHALT.
>>
>> #endif
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index e216ba0..dad475b 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -481,6 +481,10 @@ struct kvm_vcpu_arch {
>> u64 length;
>> u64 status;
>> } osvw;
>> + /* pv related host specific info */
>> + struct {
>> + int pv_unhalted;
>> + } pv;
>> };
>
> 'bool'. Or maybe push into vcpu->requests.

Ok. I think you meant
+ struct {
+ bool pv_unhalted;
+ } pv;

and as discussed in old series (V4), cleaner implementation having
vcpu request, would still need a flag to prevent vcpu hang, so back to
having one flag.

>
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 4044ce0..7fc9be6 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -2147,6 +2147,7 @@ int kvm_dev_ioctl_check_extension(long ext)
>> case KVM_CAP_ASYNC_PF:
>> case KVM_CAP_GET_TSC_KHZ:
>> case KVM_CAP_PCI_2_3:
>> + case KVM_CAP_PV_UNHALT:
>> r = 1;
>> break;
>> case KVM_CAP_COALESCED_MMIO:
>
> Redundant, since we can infer this from KVM_GET_SUPPORTED_CPUID. But
> please indicate this in the documentation.
>

Ok. will mention that in documentation added for KVM_CAP_PV_UNHALT.

>>
>> +/*
>> + * kvm_pv_kick_cpu_op: Kick a vcpu.
>> + *
>> + * @apicid - apicid of vcpu to be kicked.
>> + */
>> +static void kvm_pv_kick_cpu_op(struct kvm *kvm, int apicid)
>> +{
>> + struct kvm_vcpu *vcpu = NULL;
>> + int i;
>> +
>> + kvm_for_each_vcpu(i, vcpu, kvm) {
>> + if (!kvm_apic_present(vcpu))
>> + continue;
>> +
>> + if (kvm_apic_match_dest(vcpu, 0, 0, apicid, 0))
>> + break;
>> + }
>> + if (vcpu) {
>> + /*
>> + * Setting unhalt flag here can result in spurious runnable
>> + * state when unhalt reset does not happen in vcpu_block.
>> + * But that is harmless since that should soon result in halt.
>> + */
>> + vcpu->arch.pv.pv_unhalted = 1;
>> + /* We need everybody see unhalt before vcpu unblocks */
>> + smp_mb();
>
> smp_wmb().
>

Done.

>> + kvm_vcpu_kick(vcpu);
>> + }
>> +}
>> +
>>
>> /*
>> * hypercalls use architecture specific
>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> index 42b7393..edf56d4 100644
>> --- a/virt/kvm/kvm_main.c
>> +++ b/virt/kvm/kvm_main.c
>> @@ -1500,6 +1500,14 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)
>> prepare_to_wait(&vcpu->wq,&wait, TASK_INTERRUPTIBLE);
>>
>> if (kvm_arch_vcpu_runnable(vcpu)) {
>> + /*
>> + * This is the only safe place to reset unhalt flag.
>> + * otherwise it results in loosing the notification
>> + * which eventually can result in vcpu hangs.
>> + */
>> + kvm_arch_vcpu_reset_pv_unhalted(vcpu);
>> + /* preventing reordering should be enough here */
>> + barrier();
>> kvm_make_request(KVM_REQ_UNHALT, vcpu);
>> break;
>> }
>>
>
> Hm, what about reusing KVM_REQ_UNHALT?
>

Yes, I had experimented this for some time without success.
For e.g. having
make_request(KVM_REQ_UNHALT, vcpu) directly from kick hypercall.

It would still need a flag. (did not get any alternative so far except
the workaround posted in V4) :(


_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel


avi at redhat

Apr 30, 2012, 1:19 AM

Post #12 of 16 (513 views)
Permalink
Re: [PATCH RFC V6 1/5] kvm hypervisor : Add a hypercall to KVM hypervisor to support pv-ticketlocks [In reply to]

On 04/30/2012 10:44 AM, Raghavendra K T wrote:
>> Hm, what about reusing KVM_REQ_UNHALT?
>>
>
>
> Yes, I had experimented this for some time without success.
> For e.g. having
> make_request(KVM_REQ_UNHALT, vcpu) directly from kick hypercall.
>
> It would still need a flag. (did not get any alternative so far except
> the workaround posted in V4) :(
>

Okay.

--
error compiling committee.c: too many arguments to function


_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel


avi at redhat

Apr 30, 2012, 1:22 AM

Post #13 of 16 (515 views)
Permalink
Re: [PATCH RFC V6 1/5] kvm hypervisor : Add a hypercall to KVM hypervisor to support pv-ticketlocks [In reply to]

On 04/29/2012 04:52 PM, Gleb Natapov wrote:
> On Sun, Apr 29, 2012 at 04:26:21PM +0300, Avi Kivity wrote:
> > On 04/29/2012 04:20 PM, Gleb Natapov wrote:
> > > > > This is too similar to kvm_irq_delivery_to_apic(). Why not reuse it. We
> > > > > can use one of reserved delivery modes as PV delivery mode. We will
> > > > > disallow guest to trigger it through apic interface, so this will not be
> > > > > part of ABI and can be changed at will.
> > > > >
> > > >
> > > > I'm not thrilled about this. Those delivery modes will eventually
> > > > become unreserved. We can have a kvm_lookup_apic_id() that is shared
> > > > among implementations.
> > > >
> > > This is only internal implementation. If they become unreserved we will
> > > use something else.
> > >
> >
> > Yeah, I'm thinking of that time. Why do something temporary and fragile?
> >
> Why is it fragile? Just by unreserving the value Intel will not break
> KVM. Only when KVM will implement apic feature that unreserves the value
> we will have to change internal implementation and use another value,
> but this will be done by the same patch that does unreserving. The
> unreserving may even never happen.

Some remains of that may leak somewhere. Why not add an extra
parameter? Or do something like

kvm_for_each_apic_dest(vcpu, apic_destination) {
...
}

That can be reused in both the apic code and pv kick.

> Meanwhile kvm_irq_delivery_to_apic()
> will likely be optimized to use hash for unicast delivery and unhalt
> hypercall will benefit from it immediately.

Overloading delivery mode is not the only way to achieve sharing.

--
error compiling committee.c: too many arguments to function


_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel


gleb at redhat

Apr 30, 2012, 1:38 AM

Post #14 of 16 (512 views)
Permalink
Re: [PATCH RFC V6 1/5] kvm hypervisor : Add a hypercall to KVM hypervisor to support pv-ticketlocks [In reply to]

On Mon, Apr 30, 2012 at 11:22:34AM +0300, Avi Kivity wrote:
> On 04/29/2012 04:52 PM, Gleb Natapov wrote:
> > On Sun, Apr 29, 2012 at 04:26:21PM +0300, Avi Kivity wrote:
> > > On 04/29/2012 04:20 PM, Gleb Natapov wrote:
> > > > > > This is too similar to kvm_irq_delivery_to_apic(). Why not reuse it. We
> > > > > > can use one of reserved delivery modes as PV delivery mode. We will
> > > > > > disallow guest to trigger it through apic interface, so this will not be
> > > > > > part of ABI and can be changed at will.
> > > > > >
> > > > >
> > > > > I'm not thrilled about this. Those delivery modes will eventually
> > > > > become unreserved. We can have a kvm_lookup_apic_id() that is shared
> > > > > among implementations.
> > > > >
> > > > This is only internal implementation. If they become unreserved we will
> > > > use something else.
> > > >
> > >
> > > Yeah, I'm thinking of that time. Why do something temporary and fragile?
> > >
> > Why is it fragile? Just by unreserving the value Intel will not break
> > KVM. Only when KVM will implement apic feature that unreserves the value
> > we will have to change internal implementation and use another value,
> > but this will be done by the same patch that does unreserving. The
> > unreserving may even never happen.
>
> Some remains of that may leak somewhere.
I do not see where. APIC code should #GP if a guest attempts to set
reserved values through APIC interface, or at least ignore them.

> Why not add an extra
> parameter?
Yes, we can add extra parameter to "struct kvm_lapic_irq" and propagate it to
__apic_accept_irq(). We can do that now, or when Intel unreserve all
reserved values. I prefer the later since I do not believe it will
happen in observable feature.

> Or do something like
>
> kvm_for_each_apic_dest(vcpu, apic_destination) {
> ...
> }
>
> That can be reused in both the apic code and pv kick.
>
That's exactly what kvm_irq_delivery_to_apic() is.

> > Meanwhile kvm_irq_delivery_to_apic()
> > will likely be optimized to use hash for unicast delivery and unhalt
> > hypercall will benefit from it immediately.
>
> Overloading delivery mode is not the only way to achieve sharing.
>
It is simplest and most straightforward with no demonstratable drawbacks :)
Adding parameter to "struct kvm_lapic_irq" is next best thing.

--
Gleb.

_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel


raghavendra.kt at linux

May 1, 2012, 1:20 PM

Post #15 of 16 (486 views)
Permalink
Re: [PATCH RFC V6 1/5] kvm hypervisor : Add a hypercall to KVM hypervisor to support pv-ticketlocks [In reply to]

On 04/30/2012 01:14 PM, Raghavendra K T wrote:

>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>> index 4044ce0..7fc9be6 100644
>>> --- a/arch/x86/kvm/x86.c
>>> +++ b/arch/x86/kvm/x86.c
>>> @@ -2147,6 +2147,7 @@ int kvm_dev_ioctl_check_extension(long ext)
>>> case KVM_CAP_ASYNC_PF:
>>> case KVM_CAP_GET_TSC_KHZ:
>>> case KVM_CAP_PCI_2_3:
>>> + case KVM_CAP_PV_UNHALT:
>>> r = 1;
>>> break;
>>> case KVM_CAP_COALESCED_MMIO:
>>
>> Redundant, since we can infer this from KVM_GET_SUPPORTED_CPUID. But
>> please indicate this in the documentation.
>>
>
> Ok. will mention that in documentation added for KVM_CAP_PV_UNHALT.
>

I think it is better to remove KVM_CAP_PV_UNHALT itself and avoid
spamming CAP.. will do that in coming version.


_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel


raghavendra.kt at linux

Jun 28, 2012, 11:17 AM

Post #16 of 16 (435 views)
Permalink
Re: [PATCH RFC V6 1/5] kvm hypervisor : Add a hypercall to KVM hypervisor to support pv-ticketlocks [In reply to]

On 04/27/2012 09:23 PM, Gleb Natapov wrote:
> On Fri, Apr 27, 2012 at 04:15:35PM +0530, Raghavendra K T wrote:
>> On 04/24/2012 03:29 PM, Gleb Natapov wrote:
>>> On Mon, Apr 23, 2012 at 03:29:47PM +0530, Raghavendra K T wrote:
>>>> From: Srivatsa Vaddagiri<vatsa [at] linux>
>>>>
>>>> KVM_HC_KICK_CPU allows the calling vcpu to kick another vcpu out of halt state.
>>>>
>>>> The presence of these hypercalls is indicated to guest via
>>>> KVM_FEATURE_PV_UNHALT/KVM_CAP_PV_UNHALT.
>>>>
>>>> Signed-off-by: Srivatsa Vaddagiri<vatsa [at] linux>
>>>> Signed-off-by: Suzuki Poulose<suzuki [at] in>
>>>> Signed-off-by: Raghavendra K T<raghavendra.kt [at] linux>
>>>> ---
>> [...]
>>>> +/*
>>>> + * kvm_pv_kick_cpu_op: Kick a vcpu.
>>>> + *
>>>> + * @apicid - apicid of vcpu to be kicked.
>>>> + */
>>>> +static void kvm_pv_kick_cpu_op(struct kvm *kvm, int apicid)
>>>> +{
>>>> + struct kvm_vcpu *vcpu = NULL;
>>>> + int i;
>>>> +
>>>> + kvm_for_each_vcpu(i, vcpu, kvm) {
>>>> + if (!kvm_apic_present(vcpu))
>>>> + continue;
>>>> +
>>>> + if (kvm_apic_match_dest(vcpu, 0, 0, apicid, 0))
>>>> + break;
>>>> + }
>>>> + if (vcpu) {
>>>> + /*
>>>> + * Setting unhalt flag here can result in spurious runnable
>>>> + * state when unhalt reset does not happen in vcpu_block.
>>>> + * But that is harmless since that should soon result in halt.
>>>> + */
>>>> + vcpu->arch.pv.pv_unhalted = 1;
>>>> + /* We need everybody see unhalt before vcpu unblocks */
>>>> + smp_mb();
>>>> + kvm_vcpu_kick(vcpu);
>>>> + }
>>>> +}
>>> This is too similar to kvm_irq_delivery_to_apic(). Why not reuse it. We
>>> can use one of reserved delivery modes as PV delivery mode. We will
>>> disallow guest to trigger it through apic interface, so this will not be
>>> part of ABI and can be changed at will.
[...]
>> kvm/x86.c
>> =========
>> kvm_pv_kick_cpu_op()
>> {
>>
>> struct kvm_lapic_irq lapic_irq;
>>
>> lapic_irq.shorthand = 0;
>> lapic_irq.dest_mode = 0;
>> lapic_irq.dest_id = apicid;
>>
>> lapic_irq.delivery_mode = PV_DELIVERY_MODE;
>> kvm_irq_delivery_to_apic(kvm, 0,&lapic_irq );
>>
>> }
>>
>> kvm/lapic.c
>> ==========
>> _apic_accept_irq()
>> {
>> ...
>> case APIC_DM_REMRD:
>> result = 1;
>> vcpu->pv_unhalted = 1
>> smp_mb();
>> kvm_make_request(KVM_REQ_EVENT, vcpu);
>> kvm_vcpu_kick(vcpu);
>> break;
>>
>> ...
>> }
>>
>> here using PV_DELIVERY_MODE = APIC_DM_REMRD, which was unused.
>>
> Yes, this is what I mean except that PV_DELIVERY_MODE should be
> number defined as reserved by Intel spec.
>

Hi Gleb, Avi,

This had been TODO in my V8 patches.
I 'll fold this into V9 (while rebasing to
3.5-rc).
Please let me know if it is OK.


_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel

Xen devel RSS feed   Index | Next | Previous | View Threaded
 
 


Interested in having your list archived? Contact Gossamer Threads
 
  Web Applications & Managed Hosting Powered by Gossamer Threads Inc.