* [PATCH] KVM: x86: Convert INIT and SIPI signals into synchronously handled requests
@ 2013-03-03 20:21 Jan Kiszka
2013-03-04 14:28 ` Paolo Bonzini
2013-03-04 18:08 ` Gleb Natapov
0 siblings, 2 replies; 6+ messages in thread
From: Jan Kiszka @ 2013-03-03 20:21 UTC (permalink / raw)
To: Gleb Natapov, Marcelo Tosatti; +Cc: kvm
From: Jan Kiszka <jan.kiszka@siemens.com>
A VCPU sending INIT or SIPI to some other VCPU races for setting the
remote VCPU's mp_state. When we were unlucky, KVM_MP_STATE_INIT_RECEIVED
was overwritten by kvm_emulate_halt and, thus, got lost.
Fix this by raising requests on the sender side that will then be
handled synchronously over the target VCPU context.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
Turned out to be simpler than expected. I'm no longer able to reproduce
the race I saw before.
arch/x86/kvm/lapic.c | 9 ++++-----
arch/x86/kvm/x86.c | 16 +++++++++++++++-
include/linux/kvm_host.h | 2 ++
3 files changed, 21 insertions(+), 6 deletions(-)
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 02b51dd..be1e37a 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -731,8 +731,7 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
case APIC_DM_INIT:
if (!trig_mode || level) {
result = 1;
- vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
- kvm_make_request(KVM_REQ_EVENT, vcpu);
+ kvm_make_request(KVM_REQ_INIT, vcpu);
kvm_vcpu_kick(vcpu);
} else {
apic_debug("Ignoring de-assert INIT to vcpu %d\n",
@@ -743,11 +742,11 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
case APIC_DM_STARTUP:
apic_debug("SIPI to vcpu %d vector 0x%02x\n",
vcpu->vcpu_id, vector);
- if (vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
+ if (vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED ||
+ test_bit(KVM_REQ_INIT, &vcpu->requests)) {
result = 1;
vcpu->arch.sipi_vector = vector;
- vcpu->arch.mp_state = KVM_MP_STATE_SIPI_RECEIVED;
- kvm_make_request(KVM_REQ_EVENT, vcpu);
+ kvm_make_request(KVM_REQ_SIPI, vcpu);
kvm_vcpu_kick(vcpu);
}
break;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index d0cf737..8c8843c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5641,6 +5641,18 @@ static void update_eoi_exitmap(struct kvm_vcpu *vcpu)
kvm_x86_ops->load_eoi_exitmap(vcpu, eoi_exit_bitmap);
}
+static bool kvm_check_init_and_sipi(struct kvm_vcpu *vcpu)
+{
+ if (kvm_check_request(KVM_REQ_INIT, vcpu))
+ vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
+ if (kvm_check_request(KVM_REQ_SIPI, vcpu) &&
+ vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
+ vcpu->arch.mp_state = KVM_MP_STATE_SIPI_RECEIVED;
+ return true;
+ }
+ return false;
+}
+
static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
{
int r;
@@ -5649,6 +5661,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
bool req_immediate_exit = 0;
if (vcpu->requests) {
+ kvm_check_init_and_sipi(vcpu);
if (kvm_check_request(KVM_REQ_MMU_RELOAD, vcpu))
kvm_mmu_unload(vcpu);
if (kvm_check_request(KVM_REQ_MIGRATE_TIMER, vcpu))
@@ -6977,10 +6990,11 @@ void kvm_arch_flush_shadow_memslot(struct kvm *kvm,
int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)
{
+ if (kvm_check_init_and_sipi(vcpu))
+ return 1;
return (vcpu->arch.mp_state == KVM_MP_STATE_RUNNABLE &&
!vcpu->arch.apf.halted)
|| !list_empty_careful(&vcpu->async_pf.done)
- || vcpu->arch.mp_state == KVM_MP_STATE_SIPI_RECEIVED
|| atomic_read(&vcpu->arch.nmi_queued) ||
(kvm_arch_interrupt_allowed(vcpu) &&
kvm_cpu_has_interrupt(vcpu));
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 722cae7..1a191c9 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -124,6 +124,8 @@ static inline bool is_error_page(struct page *page)
#define KVM_REQ_MCLOCK_INPROGRESS 20
#define KVM_REQ_EPR_EXIT 21
#define KVM_REQ_EOIBITMAP 22
+#define KVM_REQ_INIT 23
+#define KVM_REQ_SIPI 24
#define KVM_USERSPACE_IRQ_SOURCE_ID 0
#define KVM_IRQFD_RESAMPLE_IRQ_SOURCE_ID 1
--
1.7.3.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: x86: Convert INIT and SIPI signals into synchronously handled requests
2013-03-03 20:21 [PATCH] KVM: x86: Convert INIT and SIPI signals into synchronously handled requests Jan Kiszka
@ 2013-03-04 14:28 ` Paolo Bonzini
2013-03-04 14:38 ` Jan Kiszka
2013-03-04 20:50 ` Jan Kiszka
2013-03-04 18:08 ` Gleb Natapov
1 sibling, 2 replies; 6+ messages in thread
From: Paolo Bonzini @ 2013-03-04 14:28 UTC (permalink / raw)
To: Jan Kiszka; +Cc: Gleb Natapov, Marcelo Tosatti, kvm
Il 03/03/2013 21:21, Jan Kiszka ha scritto:
> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> A VCPU sending INIT or SIPI to some other VCPU races for setting the
> remote VCPU's mp_state. When we were unlucky, KVM_MP_STATE_INIT_RECEIVED
> was overwritten by kvm_emulate_halt and, thus, got lost.
>
> Fix this by raising requests on the sender side that will then be
> handled synchronously over the target VCPU context.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>
> Turned out to be simpler than expected. I'm no longer able to reproduce
> the race I saw before.
>
> arch/x86/kvm/lapic.c | 9 ++++-----
> arch/x86/kvm/x86.c | 16 +++++++++++++++-
> include/linux/kvm_host.h | 2 ++
> 3 files changed, 21 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 02b51dd..be1e37a 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -731,8 +731,7 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
> case APIC_DM_INIT:
> if (!trig_mode || level) {
> result = 1;
> - vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
> - kvm_make_request(KVM_REQ_EVENT, vcpu);
> + kvm_make_request(KVM_REQ_INIT, vcpu);
> kvm_vcpu_kick(vcpu);
> } else {
> apic_debug("Ignoring de-assert INIT to vcpu %d\n",
> @@ -743,11 +742,11 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
> case APIC_DM_STARTUP:
> apic_debug("SIPI to vcpu %d vector 0x%02x\n",
> vcpu->vcpu_id, vector);
> - if (vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
> + if (vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED ||
> + test_bit(KVM_REQ_INIT, &vcpu->requests)) {
> result = 1;
> vcpu->arch.sipi_vector = vector;
> - vcpu->arch.mp_state = KVM_MP_STATE_SIPI_RECEIVED;
> - kvm_make_request(KVM_REQ_EVENT, vcpu);
> + kvm_make_request(KVM_REQ_SIPI, vcpu);
> kvm_vcpu_kick(vcpu);
> }
> break;
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index d0cf737..8c8843c 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5641,6 +5641,18 @@ static void update_eoi_exitmap(struct kvm_vcpu *vcpu)
> kvm_x86_ops->load_eoi_exitmap(vcpu, eoi_exit_bitmap);
> }
>
> +static bool kvm_check_init_and_sipi(struct kvm_vcpu *vcpu)
> +{
> + if (kvm_check_request(KVM_REQ_INIT, vcpu))
> + vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
> + if (kvm_check_request(KVM_REQ_SIPI, vcpu) &&
> + vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
> + vcpu->arch.mp_state = KVM_MP_STATE_SIPI_RECEIVED;
Do you need KVM_MP_STATE_SIPI_RECEIVED at all anymore? Perhaps you can
call kvm_check_init_and_sipi from __vcpu_run, before the call to
kvm_vcpu_block (and move the reset from __vcpu_run to
kvm_check_init_and_sipi too)? Then you do not even need to touch
kvm_arch_vcpu_runnable.
> + return true;
> + }
> + return false;
> +}
> +
> static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> {
> int r;
> @@ -5649,6 +5661,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> bool req_immediate_exit = 0;
>
> if (vcpu->requests) {
> + kvm_check_init_and_sipi(vcpu);
Does this need to return 1 if kvm_check_init_and_sipi returns 1?
Otherwise the guest is entered in INIT state. I think.
Paolo
> if (kvm_check_request(KVM_REQ_MMU_RELOAD, vcpu))
> kvm_mmu_unload(vcpu);
> if (kvm_check_request(KVM_REQ_MIGRATE_TIMER, vcpu))
> @@ -6977,10 +6990,11 @@ void kvm_arch_flush_shadow_memslot(struct kvm *kvm,
>
> int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)
> {
> + if (kvm_check_init_and_sipi(vcpu))
> + return 1;
> return (vcpu->arch.mp_state == KVM_MP_STATE_RUNNABLE &&
> !vcpu->arch.apf.halted)
> || !list_empty_careful(&vcpu->async_pf.done)
> - || vcpu->arch.mp_state == KVM_MP_STATE_SIPI_RECEIVED
> || atomic_read(&vcpu->arch.nmi_queued) ||
> (kvm_arch_interrupt_allowed(vcpu) &&
> kvm_cpu_has_interrupt(vcpu));
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 722cae7..1a191c9 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -124,6 +124,8 @@ static inline bool is_error_page(struct page *page)
> #define KVM_REQ_MCLOCK_INPROGRESS 20
> #define KVM_REQ_EPR_EXIT 21
> #define KVM_REQ_EOIBITMAP 22
> +#define KVM_REQ_INIT 23
> +#define KVM_REQ_SIPI 24
>
> #define KVM_USERSPACE_IRQ_SOURCE_ID 0
> #define KVM_IRQFD_RESAMPLE_IRQ_SOURCE_ID 1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: x86: Convert INIT and SIPI signals into synchronously handled requests
2013-03-04 14:28 ` Paolo Bonzini
@ 2013-03-04 14:38 ` Jan Kiszka
2013-03-04 20:50 ` Jan Kiszka
1 sibling, 0 replies; 6+ messages in thread
From: Jan Kiszka @ 2013-03-04 14:38 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Gleb Natapov, Marcelo Tosatti, kvm
On 2013-03-04 15:28, Paolo Bonzini wrote:
> Il 03/03/2013 21:21, Jan Kiszka ha scritto:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> A VCPU sending INIT or SIPI to some other VCPU races for setting the
>> remote VCPU's mp_state. When we were unlucky, KVM_MP_STATE_INIT_RECEIVED
>> was overwritten by kvm_emulate_halt and, thus, got lost.
>>
>> Fix this by raising requests on the sender side that will then be
>> handled synchronously over the target VCPU context.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>
>> Turned out to be simpler than expected. I'm no longer able to reproduce
>> the race I saw before.
>>
>> arch/x86/kvm/lapic.c | 9 ++++-----
>> arch/x86/kvm/x86.c | 16 +++++++++++++++-
>> include/linux/kvm_host.h | 2 ++
>> 3 files changed, 21 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>> index 02b51dd..be1e37a 100644
>> --- a/arch/x86/kvm/lapic.c
>> +++ b/arch/x86/kvm/lapic.c
>> @@ -731,8 +731,7 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
>> case APIC_DM_INIT:
>> if (!trig_mode || level) {
>> result = 1;
>> - vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
>> - kvm_make_request(KVM_REQ_EVENT, vcpu);
>> + kvm_make_request(KVM_REQ_INIT, vcpu);
>> kvm_vcpu_kick(vcpu);
>> } else {
>> apic_debug("Ignoring de-assert INIT to vcpu %d\n",
>> @@ -743,11 +742,11 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
>> case APIC_DM_STARTUP:
>> apic_debug("SIPI to vcpu %d vector 0x%02x\n",
>> vcpu->vcpu_id, vector);
>> - if (vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
>> + if (vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED ||
>> + test_bit(KVM_REQ_INIT, &vcpu->requests)) {
>> result = 1;
>> vcpu->arch.sipi_vector = vector;
>> - vcpu->arch.mp_state = KVM_MP_STATE_SIPI_RECEIVED;
>> - kvm_make_request(KVM_REQ_EVENT, vcpu);
>> + kvm_make_request(KVM_REQ_SIPI, vcpu);
>> kvm_vcpu_kick(vcpu);
>> }
>> break;
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index d0cf737..8c8843c 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -5641,6 +5641,18 @@ static void update_eoi_exitmap(struct kvm_vcpu *vcpu)
>> kvm_x86_ops->load_eoi_exitmap(vcpu, eoi_exit_bitmap);
>> }
>>
>> +static bool kvm_check_init_and_sipi(struct kvm_vcpu *vcpu)
>> +{
>> + if (kvm_check_request(KVM_REQ_INIT, vcpu))
>> + vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
>> + if (kvm_check_request(KVM_REQ_SIPI, vcpu) &&
>> + vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
>> + vcpu->arch.mp_state = KVM_MP_STATE_SIPI_RECEIVED;
>
> Do you need KVM_MP_STATE_SIPI_RECEIVED at all anymore? Perhaps you can
> call kvm_check_init_and_sipi from __vcpu_run, before the call to
> kvm_vcpu_block (and move the reset from __vcpu_run to
> kvm_check_init_and_sipi too)? Then you do not even need to touch
> kvm_arch_vcpu_runnable.
Haven't thought about this in details yet as I first wanted to fix
within the existing infrastructure. But maybe the change below requires
more refactoring anyway. Let's see.
>
>> + return true;
>> + }
>> + return false;
>> +}
>> +
>> static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>> {
>> int r;
>> @@ -5649,6 +5661,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>> bool req_immediate_exit = 0;
>>
>> if (vcpu->requests) {
>> + kvm_check_init_and_sipi(vcpu);
>
> Does this need to return 1 if kvm_check_init_and_sipi returns 1?
> Otherwise the guest is entered in INIT state. I think.
Hmm, true... Need to refactor things a bit more as
kvm_check_init_and_sipi is designed to return true only for
wait-on-sipi->runnable transition.
Thanks,
Jan
--
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: x86: Convert INIT and SIPI signals into synchronously handled requests
2013-03-03 20:21 [PATCH] KVM: x86: Convert INIT and SIPI signals into synchronously handled requests Jan Kiszka
2013-03-04 14:28 ` Paolo Bonzini
@ 2013-03-04 18:08 ` Gleb Natapov
2013-03-04 18:13 ` Jan Kiszka
1 sibling, 1 reply; 6+ messages in thread
From: Gleb Natapov @ 2013-03-04 18:08 UTC (permalink / raw)
To: Jan Kiszka; +Cc: Marcelo Tosatti, kvm
On Sun, Mar 03, 2013 at 09:21:43PM +0100, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> A VCPU sending INIT or SIPI to some other VCPU races for setting the
> remote VCPU's mp_state. When we were unlucky, KVM_MP_STATE_INIT_RECEIVED
> was overwritten by kvm_emulate_halt and, thus, got lost.
>
> Fix this by raising requests on the sender side that will then be
> handled synchronously over the target VCPU context.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>
> Turned out to be simpler than expected. I'm no longer able to reproduce
> the race I saw before.
>
> arch/x86/kvm/lapic.c | 9 ++++-----
> arch/x86/kvm/x86.c | 16 +++++++++++++++-
> include/linux/kvm_host.h | 2 ++
> 3 files changed, 21 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 02b51dd..be1e37a 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -731,8 +731,7 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
> case APIC_DM_INIT:
> if (!trig_mode || level) {
> result = 1;
> - vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
> - kvm_make_request(KVM_REQ_EVENT, vcpu);
> + kvm_make_request(KVM_REQ_INIT, vcpu);
> kvm_vcpu_kick(vcpu);
> } else {
> apic_debug("Ignoring de-assert INIT to vcpu %d\n",
> @@ -743,11 +742,11 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
> case APIC_DM_STARTUP:
> apic_debug("SIPI to vcpu %d vector 0x%02x\n",
> vcpu->vcpu_id, vector);
> - if (vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
> + if (vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED ||
> + test_bit(KVM_REQ_INIT, &vcpu->requests)) {
> result = 1;
> vcpu->arch.sipi_vector = vector;
> - vcpu->arch.mp_state = KVM_MP_STATE_SIPI_RECEIVED;
> - kvm_make_request(KVM_REQ_EVENT, vcpu);
> + kvm_make_request(KVM_REQ_SIPI, vcpu);
> kvm_vcpu_kick(vcpu);
> }
> break;
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index d0cf737..8c8843c 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5641,6 +5641,18 @@ static void update_eoi_exitmap(struct kvm_vcpu *vcpu)
> kvm_x86_ops->load_eoi_exitmap(vcpu, eoi_exit_bitmap);
> }
>
> +static bool kvm_check_init_and_sipi(struct kvm_vcpu *vcpu)
> +{
> + if (kvm_check_request(KVM_REQ_INIT, vcpu))
> + vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
> + if (kvm_check_request(KVM_REQ_SIPI, vcpu) &&
> + vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
> + vcpu->arch.mp_state = KVM_MP_STATE_SIPI_RECEIVED;
> + return true;
> + }
> + return false;
> +}
> +
> static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> {
> int r;
> @@ -5649,6 +5661,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> bool req_immediate_exit = 0;
>
> if (vcpu->requests) {
> + kvm_check_init_and_sipi(vcpu);
> if (kvm_check_request(KVM_REQ_MMU_RELOAD, vcpu))
> kvm_mmu_unload(vcpu);
> if (kvm_check_request(KVM_REQ_MIGRATE_TIMER, vcpu))
> @@ -6977,10 +6990,11 @@ void kvm_arch_flush_shadow_memslot(struct kvm *kvm,
>
> int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)
> {
> + if (kvm_check_init_and_sipi(vcpu))
> + return 1;
> return (vcpu->arch.mp_state == KVM_MP_STATE_RUNNABLE &&
> !vcpu->arch.apf.halted)
> || !list_empty_careful(&vcpu->async_pf.done)
> - || vcpu->arch.mp_state == KVM_MP_STATE_SIPI_RECEIVED
> || atomic_read(&vcpu->arch.nmi_queued) ||
> (kvm_arch_interrupt_allowed(vcpu) &&
> kvm_cpu_has_interrupt(vcpu));
This makes two subsequent calls to kvm_arch_vcpu_runnable() return
different values if SIPI is pending. While it may not cause problem to
current code (I haven't thought it through) with such semantics you
gonna have a bad time.
--
Gleb.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: x86: Convert INIT and SIPI signals into synchronously handled requests
2013-03-04 18:08 ` Gleb Natapov
@ 2013-03-04 18:13 ` Jan Kiszka
0 siblings, 0 replies; 6+ messages in thread
From: Jan Kiszka @ 2013-03-04 18:13 UTC (permalink / raw)
To: Gleb Natapov; +Cc: Marcelo Tosatti, kvm
On 2013-03-04 19:08, Gleb Natapov wrote:
> On Sun, Mar 03, 2013 at 09:21:43PM +0100, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> A VCPU sending INIT or SIPI to some other VCPU races for setting the
>> remote VCPU's mp_state. When we were unlucky, KVM_MP_STATE_INIT_RECEIVED
>> was overwritten by kvm_emulate_halt and, thus, got lost.
>>
>> Fix this by raising requests on the sender side that will then be
>> handled synchronously over the target VCPU context.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>
>> Turned out to be simpler than expected. I'm no longer able to reproduce
>> the race I saw before.
>>
>> arch/x86/kvm/lapic.c | 9 ++++-----
>> arch/x86/kvm/x86.c | 16 +++++++++++++++-
>> include/linux/kvm_host.h | 2 ++
>> 3 files changed, 21 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>> index 02b51dd..be1e37a 100644
>> --- a/arch/x86/kvm/lapic.c
>> +++ b/arch/x86/kvm/lapic.c
>> @@ -731,8 +731,7 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
>> case APIC_DM_INIT:
>> if (!trig_mode || level) {
>> result = 1;
>> - vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
>> - kvm_make_request(KVM_REQ_EVENT, vcpu);
>> + kvm_make_request(KVM_REQ_INIT, vcpu);
>> kvm_vcpu_kick(vcpu);
>> } else {
>> apic_debug("Ignoring de-assert INIT to vcpu %d\n",
>> @@ -743,11 +742,11 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
>> case APIC_DM_STARTUP:
>> apic_debug("SIPI to vcpu %d vector 0x%02x\n",
>> vcpu->vcpu_id, vector);
>> - if (vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
>> + if (vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED ||
>> + test_bit(KVM_REQ_INIT, &vcpu->requests)) {
>> result = 1;
>> vcpu->arch.sipi_vector = vector;
>> - vcpu->arch.mp_state = KVM_MP_STATE_SIPI_RECEIVED;
>> - kvm_make_request(KVM_REQ_EVENT, vcpu);
>> + kvm_make_request(KVM_REQ_SIPI, vcpu);
>> kvm_vcpu_kick(vcpu);
>> }
>> break;
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index d0cf737..8c8843c 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -5641,6 +5641,18 @@ static void update_eoi_exitmap(struct kvm_vcpu *vcpu)
>> kvm_x86_ops->load_eoi_exitmap(vcpu, eoi_exit_bitmap);
>> }
>>
>> +static bool kvm_check_init_and_sipi(struct kvm_vcpu *vcpu)
>> +{
>> + if (kvm_check_request(KVM_REQ_INIT, vcpu))
>> + vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
>> + if (kvm_check_request(KVM_REQ_SIPI, vcpu) &&
>> + vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
>> + vcpu->arch.mp_state = KVM_MP_STATE_SIPI_RECEIVED;
>> + return true;
>> + }
>> + return false;
>> +}
>> +
>> static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>> {
>> int r;
>> @@ -5649,6 +5661,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>> bool req_immediate_exit = 0;
>>
>> if (vcpu->requests) {
>> + kvm_check_init_and_sipi(vcpu);
>> if (kvm_check_request(KVM_REQ_MMU_RELOAD, vcpu))
>> kvm_mmu_unload(vcpu);
>> if (kvm_check_request(KVM_REQ_MIGRATE_TIMER, vcpu))
>> @@ -6977,10 +6990,11 @@ void kvm_arch_flush_shadow_memslot(struct kvm *kvm,
>>
>> int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)
>> {
>> + if (kvm_check_init_and_sipi(vcpu))
>> + return 1;
>> return (vcpu->arch.mp_state == KVM_MP_STATE_RUNNABLE &&
>> !vcpu->arch.apf.halted)
>> || !list_empty_careful(&vcpu->async_pf.done)
>> - || vcpu->arch.mp_state == KVM_MP_STATE_SIPI_RECEIVED
>> || atomic_read(&vcpu->arch.nmi_queued) ||
>> (kvm_arch_interrupt_allowed(vcpu) &&
>> kvm_cpu_has_interrupt(vcpu));
> This makes two subsequent calls to kvm_arch_vcpu_runnable() return
> different values if SIPI is pending. While it may not cause problem to
> current code (I haven't thought it through) with such semantics you
> gonna have a bad time.
If I manage to follow Paolo's suggestion to eliminate the SIPI_RECEIVED
state and all the staged logic around it, that might change. Will be
more invasive but likely cleaner in its result.
Jan
--
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: x86: Convert INIT and SIPI signals into synchronously handled requests
2013-03-04 14:28 ` Paolo Bonzini
2013-03-04 14:38 ` Jan Kiszka
@ 2013-03-04 20:50 ` Jan Kiszka
1 sibling, 0 replies; 6+ messages in thread
From: Jan Kiszka @ 2013-03-04 20:50 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Gleb Natapov, Marcelo Tosatti, kvm
[-- Attachment #1: Type: text/plain, Size: 3319 bytes --]
On 2013-03-04 15:28, Paolo Bonzini wrote:
> Il 03/03/2013 21:21, Jan Kiszka ha scritto:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> A VCPU sending INIT or SIPI to some other VCPU races for setting the
>> remote VCPU's mp_state. When we were unlucky, KVM_MP_STATE_INIT_RECEIVED
>> was overwritten by kvm_emulate_halt and, thus, got lost.
>>
>> Fix this by raising requests on the sender side that will then be
>> handled synchronously over the target VCPU context.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>
>> Turned out to be simpler than expected. I'm no longer able to reproduce
>> the race I saw before.
>>
>> arch/x86/kvm/lapic.c | 9 ++++-----
>> arch/x86/kvm/x86.c | 16 +++++++++++++++-
>> include/linux/kvm_host.h | 2 ++
>> 3 files changed, 21 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>> index 02b51dd..be1e37a 100644
>> --- a/arch/x86/kvm/lapic.c
>> +++ b/arch/x86/kvm/lapic.c
>> @@ -731,8 +731,7 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
>> case APIC_DM_INIT:
>> if (!trig_mode || level) {
>> result = 1;
>> - vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
>> - kvm_make_request(KVM_REQ_EVENT, vcpu);
>> + kvm_make_request(KVM_REQ_INIT, vcpu);
>> kvm_vcpu_kick(vcpu);
>> } else {
>> apic_debug("Ignoring de-assert INIT to vcpu %d\n",
>> @@ -743,11 +742,11 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
>> case APIC_DM_STARTUP:
>> apic_debug("SIPI to vcpu %d vector 0x%02x\n",
>> vcpu->vcpu_id, vector);
>> - if (vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
>> + if (vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED ||
>> + test_bit(KVM_REQ_INIT, &vcpu->requests)) {
>> result = 1;
>> vcpu->arch.sipi_vector = vector;
>> - vcpu->arch.mp_state = KVM_MP_STATE_SIPI_RECEIVED;
>> - kvm_make_request(KVM_REQ_EVENT, vcpu);
>> + kvm_make_request(KVM_REQ_SIPI, vcpu);
>> kvm_vcpu_kick(vcpu);
>> }
>> break;
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index d0cf737..8c8843c 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -5641,6 +5641,18 @@ static void update_eoi_exitmap(struct kvm_vcpu *vcpu)
>> kvm_x86_ops->load_eoi_exitmap(vcpu, eoi_exit_bitmap);
>> }
>>
>> +static bool kvm_check_init_and_sipi(struct kvm_vcpu *vcpu)
>> +{
>> + if (kvm_check_request(KVM_REQ_INIT, vcpu))
>> + vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
>> + if (kvm_check_request(KVM_REQ_SIPI, vcpu) &&
>> + vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
>> + vcpu->arch.mp_state = KVM_MP_STATE_SIPI_RECEIVED;
>
> Do you need KVM_MP_STATE_SIPI_RECEIVED at all anymore?
Unfortunately, we cannot kill it as it was user-visible:
When a VCPU receives KVM_MP_STATE_SIPI_RECEIVED, it leaves __vcpu_run
with -EINTR and, thus, KVM_RUN. We actually return to userspace,
allowing it to see this mp_state and also migrate the guest in this state.
I could avoid this userspace exit (not sure what it is good for) but we
will have to keep the logic to accept and convert the state into
KVM_MP_STATE_RUNNABLE. So there is not much to simplify here, I'm afraid.
Jan
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-03-04 20:50 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-03-03 20:21 [PATCH] KVM: x86: Convert INIT and SIPI signals into synchronously handled requests Jan Kiszka
2013-03-04 14:28 ` Paolo Bonzini
2013-03-04 14:38 ` Jan Kiszka
2013-03-04 20:50 ` Jan Kiszka
2013-03-04 18:08 ` Gleb Natapov
2013-03-04 18:13 ` Jan Kiszka
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox