Patchwork [uq/master,2/2] kvm: forward INIT signals coming from the chipset

login
register
mail settings
Submitter Paolo Bonzini
Date March 9, 2013, 6:48 a.m.
Message ID <1362811730-6716-3-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/226310/
State New
Headers show

Comments

Paolo Bonzini - March 9, 2013, 6:48 a.m.
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(-)
Gleb Natapov - March 10, 2013, 11:54 a.m.
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.
Paolo Bonzini - March 10, 2013, 2:28 p.m.
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
>
Gleb Natapov - March 10, 2013, 2:55 p.m.
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.
Paolo Bonzini - March 10, 2013, 3:04 p.m.
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
Gleb Natapov - March 10, 2013, 3:24 p.m.
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.
Paolo Bonzini - March 11, 2013, 7:35 a.m.
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

Patch

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);