[Qemu-devel] [PATCH uq/master 0/2] CPU reset (INIT#) support for KVM

[Qemu-devel] [PATCH uq/master 0/2] CPU reset (INIT#) support for KVM

[Paolo Bonzini]

This is a follow up to the series I posted earlier this week to
support CPU soft reset.  It is a no-op without that series, but
it can be applied independently.

The combined series is available at branch x86-soft-reset of
my github repository (git://github.com/bonzini/qemu.git).  With
this series and the corresponding hypervisor patches, the init
test of kvm-unit-tests now passes:

    $ x86_64-softmmu/qemu-system-x86_64 \
          -kernel ../../kvm-unit-tests/x86/init.flat \
          -serial mon:stdio -display none \
          -device isa-debug-exit,iobase=0xf4 \
          --enable-kvm -machine kernel_irqchip=on
    enabling apic
    testing port 92 init... enabling apic
    testing kbd controller reset... enabling apic
    testing kbd controller init... enabling apic
    testing 0xcf9h init... enabling apic
    testing init to BSP... enabling apic

Paolo

Paolo Bonzini (2):
  kvm: report errors from kvm_arch_process_async_events    
  kvm: forward INIT signals coming from the chipset

 kvm-all.c         |  8 +++++++-
 target-i386/kvm.c | 34 +++++++++++++++++++++++-----------
 2 files changed, 30 insertions(+), 12 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH uq/master 1/2] kvm: detect errors from kvm_arch_process_async_events

[Paolo Bonzini]

The next patch will call a ioctl from kvm_arch_process_async_events.
Trap errors and abort the program if one comes.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 kvm-all.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/kvm-all.c b/kvm-all.c
index 4decfdc..bc1534c 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1536,7 +1536,13 @@ int kvm_cpu_exec(CPUArchState *env)
 
     DPRINTF("kvm_cpu_exec()\n");
 
-    if (kvm_arch_process_async_events(cpu)) {
+    ret = kvm_arch_process_async_events(cpu);
+    if (ret) {
+        if (ret < 0) {
+            fprintf(stderr, "error: kvm process events failed %s\n",
+                    strerror(-ret));
+            abort();
+        }
         cpu->exit_request = 0;
         return EXCP_HLT;
     }
-- 
1.8.1.4

[Qemu-devel] [PATCH uq/master 2/2] kvm: forward INIT signals coming from the chipset

[Paolo Bonzini]

CPU_INTERRUPT_INIT can also be generated if you have an internal APIC,
since the keyboard controller and the southbridge can also pulse the
CPU's INIT# pin.

Exit the VCPU is one is received, and process it by changing the
mp_state to KVM_MP_STATE_INIT_RECEIVED.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target-i386/kvm.c | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 0cf413d..56de77c 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -1772,14 +1772,15 @@ void kvm_arch_pre_run(CPUState *cpu, struct kvm_run *run)
         }
     }
 
-    if (!kvm_irqchip_in_kernel()) {
-        /* Force the VCPU out of its inner loop to process any INIT requests
-         * or pending TPR access reports. */
-        if (env->interrupt_request &
-            (CPU_INTERRUPT_INIT | CPU_INTERRUPT_TPR)) {
-            cpu->exit_request = 1;
-        }
+    /* Force the VCPU out of its inner loop to process any INIT requests
+     * or (for userspace APIC, but it is cheap to combine the checks here)
+     * pending TPR access reports.
+     */
+    if (env->interrupt_request & (CPU_INTERRUPT_INIT | CPU_INTERRUPT_TPR)) {
+        cpu->exit_request = 1;
+    }
 
+    if (!kvm_irqchip_in_kernel()) {
         /* Try to inject an interrupt if the guest can accept it */
         if (run->ready_for_interrupt_injection &&
             (env->interrupt_request & CPU_INTERRUPT_HARD) &&
@@ -1835,6 +1836,7 @@ int kvm_arch_process_async_events(CPUState *cs)
 {
     X86CPU *cpu = X86_CPU(cs);
     CPUX86State *env = &cpu->env;
+    int ret;
 
     if (env->interrupt_request & CPU_INTERRUPT_MCE) {
         /* We must not raise CPU_INTERRUPT_MCE if it's not supported. */
@@ -1859,6 +1861,20 @@ int kvm_arch_process_async_events(CPUState *cs)
         }
     }
 
+    if (env->interrupt_request & CPU_INTERRUPT_INIT) {
+        kvm_cpu_synchronize_state(env);
+        if (kvm_irqchip_in_kernel()) {
+            env->mp_state = KVM_MP_STATE_INIT_RECEIVED;
+            env->interrupt_request = 0;
+            ret = kvm_put_mp_state(cpu);
+            if (ret < 0) {
+                return ret;
+            }
+        } else {
+            do_cpu_init(cpu);
+        }
+    }
+
     if (kvm_irqchip_in_kernel()) {
         return 0;
     }
@@ -1872,10 +1888,6 @@ int kvm_arch_process_async_events(CPUState *cs)
         (env->interrupt_request & CPU_INTERRUPT_NMI)) {
         env->halted = 0;
     }
-    if (env->interrupt_request & CPU_INTERRUPT_INIT) {
-        kvm_cpu_synchronize_state(env);
-        do_cpu_init(cpu);
-    }
     if (env->interrupt_request & CPU_INTERRUPT_SIPI) {
         kvm_cpu_synchronize_state(env);
         do_cpu_sipi(cpu);
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH uq/master 2/2] kvm: forward INIT signals coming from the chipset

[Gleb Natapov]

On Sat, Mar 09, 2013 at 07:48:50AM +0100, Paolo Bonzini wrote:
> CPU_INTERRUPT_INIT can also be generated if you have an internal APIC,
> since the keyboard controller and the southbridge can also pulse the
> CPU's INIT# pin.
> 
> Exit the VCPU is one is received, and process it by changing the
> mp_state to KVM_MP_STATE_INIT_RECEIVED.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  target-i386/kvm.c | 34 +++++++++++++++++++++++-----------
>  1 file changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 0cf413d..56de77c 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -1772,14 +1772,15 @@ void kvm_arch_pre_run(CPUState *cpu, struct kvm_run *run)
>          }
>      }
>  
> -    if (!kvm_irqchip_in_kernel()) {
> -        /* Force the VCPU out of its inner loop to process any INIT requests
> -         * or pending TPR access reports. */
> -        if (env->interrupt_request &
> -            (CPU_INTERRUPT_INIT | CPU_INTERRUPT_TPR)) {
> -            cpu->exit_request = 1;
> -        }
> +    /* Force the VCPU out of its inner loop to process any INIT requests
> +     * or (for userspace APIC, but it is cheap to combine the checks here)
> +     * pending TPR access reports.
> +     */
> +    if (env->interrupt_request & (CPU_INTERRUPT_INIT | CPU_INTERRUPT_TPR)) {
> +        cpu->exit_request = 1;
> +    }
>  
> +    if (!kvm_irqchip_in_kernel()) {
>          /* Try to inject an interrupt if the guest can accept it */
>          if (run->ready_for_interrupt_injection &&
>              (env->interrupt_request & CPU_INTERRUPT_HARD) &&
> @@ -1835,6 +1836,7 @@ int kvm_arch_process_async_events(CPUState *cs)
>  {
>      X86CPU *cpu = X86_CPU(cs);
>      CPUX86State *env = &cpu->env;
> +    int ret;
>  
>      if (env->interrupt_request & CPU_INTERRUPT_MCE) {
>          /* We must not raise CPU_INTERRUPT_MCE if it's not supported. */
> @@ -1859,6 +1861,20 @@ int kvm_arch_process_async_events(CPUState *cs)
>          }
>      }
>  
> +    if (env->interrupt_request & CPU_INTERRUPT_INIT) {
> +        kvm_cpu_synchronize_state(env);
> +        if (kvm_irqchip_in_kernel()) {
> +            env->mp_state = KVM_MP_STATE_INIT_RECEIVED;
> +            env->interrupt_request = 0;
> +            ret = kvm_put_mp_state(cpu);
> +            if (ret < 0) {
> +                return ret;
> +            }
> +        } else {
> +            do_cpu_init(cpu);
> +        }
> +    }
> +
Why not move INIT case from below as is? Vcpu is reset to correct sate
by QEMU just like during system_reset.

>      if (kvm_irqchip_in_kernel()) {
>          return 0;
>      }
> @@ -1872,10 +1888,6 @@ int kvm_arch_process_async_events(CPUState *cs)
>          (env->interrupt_request & CPU_INTERRUPT_NMI)) {
>          env->halted = 0;
>      }
> -    if (env->interrupt_request & CPU_INTERRUPT_INIT) {
> -        kvm_cpu_synchronize_state(env);
> -        do_cpu_init(cpu);
> -    }
>      if (env->interrupt_request & CPU_INTERRUPT_SIPI) {
>          kvm_cpu_synchronize_state(env);
>          do_cpu_sipi(cpu);
> -- 
> 1.8.1.4


--
			Gleb.

Re: [Qemu-devel] [PATCH uq/master 2/2] kvm: forward INIT signals coming from the chipset

[Paolo Bonzini]

Il 10/03/2013 12:54, Gleb Natapov ha scritto:
> On Sat, Mar 09, 2013 at 07:48:50AM +0100, Paolo Bonzini wrote:
>> CPU_INTERRUPT_INIT can also be generated if you have an internal APIC,
>> since the keyboard controller and the southbridge can also pulse the
>> CPU's INIT# pin.
>>
>> Exit the VCPU is one is received, and process it by changing the
>> mp_state to KVM_MP_STATE_INIT_RECEIVED.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  target-i386/kvm.c | 34 +++++++++++++++++++++++-----------
>>  1 file changed, 23 insertions(+), 11 deletions(-)
>>
>> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
>> index 0cf413d..56de77c 100644
>> --- a/target-i386/kvm.c
>> +++ b/target-i386/kvm.c
>> @@ -1772,14 +1772,15 @@ void kvm_arch_pre_run(CPUState *cpu, struct kvm_run *run)
>>          }
>>      }
>>  
>> -    if (!kvm_irqchip_in_kernel()) {
>> -        /* Force the VCPU out of its inner loop to process any INIT requests
>> -         * or pending TPR access reports. */
>> -        if (env->interrupt_request &
>> -            (CPU_INTERRUPT_INIT | CPU_INTERRUPT_TPR)) {
>> -            cpu->exit_request = 1;
>> -        }
>> +    /* Force the VCPU out of its inner loop to process any INIT requests
>> +     * or (for userspace APIC, but it is cheap to combine the checks here)
>> +     * pending TPR access reports.
>> +     */
>> +    if (env->interrupt_request & (CPU_INTERRUPT_INIT | CPU_INTERRUPT_TPR)) {
>> +        cpu->exit_request = 1;
>> +    }
>>  
>> +    if (!kvm_irqchip_in_kernel()) {
>>          /* Try to inject an interrupt if the guest can accept it */
>>          if (run->ready_for_interrupt_injection &&
>>              (env->interrupt_request & CPU_INTERRUPT_HARD) &&
>> @@ -1835,6 +1836,7 @@ int kvm_arch_process_async_events(CPUState *cs)
>>  {
>>      X86CPU *cpu = X86_CPU(cs);
>>      CPUX86State *env = &cpu->env;
>> +    int ret;
>>  
>>      if (env->interrupt_request & CPU_INTERRUPT_MCE) {
>>          /* We must not raise CPU_INTERRUPT_MCE if it's not supported. */
>> @@ -1859,6 +1861,20 @@ int kvm_arch_process_async_events(CPUState *cs)
>>          }
>>      }
>>  
>> +    if (env->interrupt_request & CPU_INTERRUPT_INIT) {
>> +        kvm_cpu_synchronize_state(env);
>> +        if (kvm_irqchip_in_kernel()) {
>> +            env->mp_state = KVM_MP_STATE_INIT_RECEIVED;
>> +            env->interrupt_request = 0;
>> +            ret = kvm_put_mp_state(cpu);
>> +            if (ret < 0) {
>> +                return ret;
>> +            }
>> +        } else {
>> +            do_cpu_init(cpu);
>> +        }
>> +    }
>> +
> Why not move INIT case from below as is? Vcpu is reset to correct sate
> by QEMU just like during system_reset.

APs would not be able to receive SIPIs after executing do_cpu_init,
because they would stay in KVM_MP_STATE_RUNNABLE state.

Paolo

>>      if (kvm_irqchip_in_kernel()) {
>>          return 0;
>>      }
>> @@ -1872,10 +1888,6 @@ int kvm_arch_process_async_events(CPUState *cs)
>>          (env->interrupt_request & CPU_INTERRUPT_NMI)) {
>>          env->halted = 0;
>>      }
>> -    if (env->interrupt_request & CPU_INTERRUPT_INIT) {
>> -        kvm_cpu_synchronize_state(env);
>> -        do_cpu_init(cpu);
>> -    }
>>      if (env->interrupt_request & CPU_INTERRUPT_SIPI) {
>>          kvm_cpu_synchronize_state(env);
>>          do_cpu_sipi(cpu);
>> -- 
>> 1.8.1.4
> 
> 
> --
> 			Gleb.
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [Qemu-devel] [PATCH uq/master 2/2] kvm: forward INIT signals coming from the chipset

[Gleb Natapov]

On Sun, Mar 10, 2013 at 03:28:06PM +0100, Paolo Bonzini wrote:
> Il 10/03/2013 12:54, Gleb Natapov ha scritto:
> > On Sat, Mar 09, 2013 at 07:48:50AM +0100, Paolo Bonzini wrote:
> >> CPU_INTERRUPT_INIT can also be generated if you have an internal APIC,
> >> since the keyboard controller and the southbridge can also pulse the
> >> CPU's INIT# pin.
> >>
> >> Exit the VCPU is one is received, and process it by changing the
> >> mp_state to KVM_MP_STATE_INIT_RECEIVED.
> >>
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >> ---
> >>  target-i386/kvm.c | 34 +++++++++++++++++++++++-----------
> >>  1 file changed, 23 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> >> index 0cf413d..56de77c 100644
> >> --- a/target-i386/kvm.c
> >> +++ b/target-i386/kvm.c
> >> @@ -1772,14 +1772,15 @@ void kvm_arch_pre_run(CPUState *cpu, struct kvm_run *run)
> >>          }
> >>      }
> >>  
> >> -    if (!kvm_irqchip_in_kernel()) {
> >> -        /* Force the VCPU out of its inner loop to process any INIT requests
> >> -         * or pending TPR access reports. */
> >> -        if (env->interrupt_request &
> >> -            (CPU_INTERRUPT_INIT | CPU_INTERRUPT_TPR)) {
> >> -            cpu->exit_request = 1;
> >> -        }
> >> +    /* Force the VCPU out of its inner loop to process any INIT requests
> >> +     * or (for userspace APIC, but it is cheap to combine the checks here)
> >> +     * pending TPR access reports.
> >> +     */
> >> +    if (env->interrupt_request & (CPU_INTERRUPT_INIT | CPU_INTERRUPT_TPR)) {
> >> +        cpu->exit_request = 1;
> >> +    }
> >>  
> >> +    if (!kvm_irqchip_in_kernel()) {
> >>          /* Try to inject an interrupt if the guest can accept it */
> >>          if (run->ready_for_interrupt_injection &&
> >>              (env->interrupt_request & CPU_INTERRUPT_HARD) &&
> >> @@ -1835,6 +1836,7 @@ int kvm_arch_process_async_events(CPUState *cs)
> >>  {
> >>      X86CPU *cpu = X86_CPU(cs);
> >>      CPUX86State *env = &cpu->env;
> >> +    int ret;
> >>  
> >>      if (env->interrupt_request & CPU_INTERRUPT_MCE) {
> >>          /* We must not raise CPU_INTERRUPT_MCE if it's not supported. */
> >> @@ -1859,6 +1861,20 @@ int kvm_arch_process_async_events(CPUState *cs)
> >>          }
> >>      }
> >>  
> >> +    if (env->interrupt_request & CPU_INTERRUPT_INIT) {
> >> +        kvm_cpu_synchronize_state(env);
> >> +        if (kvm_irqchip_in_kernel()) {
> >> +            env->mp_state = KVM_MP_STATE_INIT_RECEIVED;
> >> +            env->interrupt_request = 0;
> >> +            ret = kvm_put_mp_state(cpu);
> >> +            if (ret < 0) {
> >> +                return ret;
> >> +            }
> >> +        } else {
> >> +            do_cpu_init(cpu);
> >> +        }
> >> +    }
> >> +
> > Why not move INIT case from below as is? Vcpu is reset to correct sate
> > by QEMU just like during system_reset.
> 
> APs would not be able to receive SIPIs after executing do_cpu_init,
> because they would stay in KVM_MP_STATE_RUNNABLE state.
> 
If APs are in runnable state after reset with in kernel irq chip we
have a bug somewhere. Should AP be able to get SIPI without INIT after
trigger of INIT# line?

--
			Gleb.

Re: [Qemu-devel] [PATCH uq/master 2/2] kvm: forward INIT signals coming from the chipset

[Paolo Bonzini]

Il 10/03/2013 15:55, Gleb Natapov ha scritto:
> > > Why not move INIT case from below as is? Vcpu is reset to correct sate
> > > by QEMU just like during system_reset.
> > 
> > APs would not be able to receive SIPIs after executing do_cpu_init,
> > because they would stay in KVM_MP_STATE_RUNNABLE state.
> 
> If APs are in runnable state after reset with in kernel irq chip we
> have a bug somewhere.

Here is where we are resetting the processor.  After clearing
CPU_INTERRUPT_INIT, no matter what else we do (such as resetting the
APIC and CPU), we need to set the mp_state to KVM_MP_STATE_INIT_RECEIVED.

Or if we go with your simpler hypervisor patch, we need to go to either
KVM_MP_STATE_INIT_RECEIVED for APs (wait for SIPI) or
KVM_MP_STATE_SIPI_RECEIVED for the BSP (restart running from the reset
vector).

> Should AP be able to get SIPI without INIT after trigger of INIT# line?

Yes, the effect is the same for an INIT interrupt and the triggering of
INIT#.

Paolo

Re: [Qemu-devel] [PATCH uq/master 2/2] kvm: forward INIT signals coming from the chipset

[Gleb Natapov]

On Sun, Mar 10, 2013 at 04:04:39PM +0100, Paolo Bonzini wrote:
> Il 10/03/2013 15:55, Gleb Natapov ha scritto:
> > > > Why not move INIT case from below as is? Vcpu is reset to correct sate
> > > > by QEMU just like during system_reset.
> > > 
> > > APs would not be able to receive SIPIs after executing do_cpu_init,
> > > because they would stay in KVM_MP_STATE_RUNNABLE state.
> > 
> > If APs are in runnable state after reset with in kernel irq chip we
> > have a bug somewhere.
> 
> Here is where we are resetting the processor.  After clearing
> CPU_INTERRUPT_INIT, no matter what else we do (such as resetting the
> APIC and CPU), we need to set the mp_state to KVM_MP_STATE_INIT_RECEIVED.
> 
> Or if we go with your simpler hypervisor patch, we need to go to either
> KVM_MP_STATE_INIT_RECEIVED for APs (wait for SIPI) or
> KVM_MP_STATE_SIPI_RECEIVED for the BSP (restart running from the reset
> vector).
> 
No need for KVM_MP_STATE_SIPI_RECEIVED. Just make it RUNNING. This is similar to
system_reset path, not? UNINIT for AP, RUNNING for BSP.

> > Should AP be able to get SIPI without INIT after trigger of INIT# line?
> 
> Yes, the effect is the same for an INIT interrupt and the triggering of
> INIT#.
> 
Can you give me SDM pointer?

--
			Gleb.

Re: [Qemu-devel] [PATCH uq/master 2/2] kvm: forward INIT signals coming from the chipset

[Paolo Bonzini]

Il 10/03/2013 16:24, Gleb Natapov ha scritto:
> On Sun, Mar 10, 2013 at 04:04:39PM +0100, Paolo Bonzini wrote:
>> Il 10/03/2013 15:55, Gleb Natapov ha scritto:
>>>>> Why not move INIT case from below as is? Vcpu is reset to correct sate
>>>>> by QEMU just like during system_reset.
>>>>
>>>> APs would not be able to receive SIPIs after executing do_cpu_init,
>>>> because they would stay in KVM_MP_STATE_RUNNABLE state.
>>>
>>> If APs are in runnable state after reset with in kernel irq chip we
>>> have a bug somewhere.
>>
>> Here is where we are resetting the processor.  After clearing
>> CPU_INTERRUPT_INIT, no matter what else we do (such as resetting the
>> APIC and CPU), we need to set the mp_state to KVM_MP_STATE_INIT_RECEIVED.
>>
>> Or if we go with your simpler hypervisor patch, we need to go to either
>> KVM_MP_STATE_INIT_RECEIVED for APs (wait for SIPI) or
>> KVM_MP_STATE_SIPI_RECEIVED for the BSP (restart running from the reset
>> vector).
>>
> No need for KVM_MP_STATE_SIPI_RECEIVED. Just make it RUNNING. This is similar to
> system_reset path, not? UNINIT for AP, RUNNING for BSP.
> 
>>> Should AP be able to get SIPI without INIT after trigger of INIT# line?
>>
>> Yes, the effect is the same for an INIT interrupt and the triggering of
>> INIT#.
>>
> Can you give me SDM pointer?

10.4.7.3 Local APIC State After an INIT Reset (“Wait-for-SIPI” State)

An INIT reset of the processor can be initiated in either of two ways:

• By asserting the processor’s INIT# pin.

• By sending the processor an INIT IPI (an IPI with the delivery mode
set to INIT).

Upon receiving an INIT through either of these mechanisms, the processor
responds by beginning the initialization process of the processor core
and the local APIC. The state of the local APIC following an INIT reset
is the same as it is after a power-up or
hardware RESET, except that the APIC ID and arbitration ID registers are
not affected. This state is also referred to at the “wait-for-SIPI”
state (see also: Section 8.4.2, “MP Initialization Protocol Requirements
and Restrictions”).

Paolo