[PATCH 1/2] KVM: x86: fix ordering of cr0 initialization code in vmx_cpu_reset

[PATCH 1/2] KVM: x86: fix ordering of cr0 initialization code in vmx_cpu_reset

[Bruce Rogers]

Commit d28bc9dd25ce reversed the order of two lines which initialize cr0,
allowing the current (old) cr0 value to mess up vcpu initialization.
This was observed in the checks for cr0 X86_CR0_WP bit in the context of
kvm_mmu_reset_context().  Besides, setting vcpu->arch.cr0 after vmx_set_cr0()
is completely redundant. Change the order back to ensure proper vcpu
intiialization.

Signed-off-by: Bruce Rogers <brogers@suse.com>
---
 arch/x86/kvm/vmx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index e2951b6..21507b4 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -4993,8 +4993,8 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
 		vmcs_write16(VIRTUAL_PROCESSOR_ID, vmx->vpid);
 
 	cr0 = X86_CR0_NW | X86_CR0_CD | X86_CR0_ET;
-	vmx_set_cr0(vcpu, cr0); /* enter rmode */
 	vmx->vcpu.arch.cr0 = cr0;
+	vmx_set_cr0(vcpu, cr0); /* enter rmode */
 	vmx_set_cr4(vcpu, 0);
 	vmx_set_efer(vcpu, 0);
 	vmx_fpu_activate(vcpu);
-- 
1.9.0

[PATCH 2/2] KVM: x86: allow BSP to handle INIT IPIs like APs do

[Bruce Rogers]

The INIT IPI event handler special cases the boot-strap processor
(BSP) handling, avoiding the same mp state handling which is done for
the other (AP) processors. Debugging a linux guest usage scenario of
avoiding a reboot through the bios for a crash on any processor via eg:
kexec -p /boot/vmlinuz --initrd=/boot/initrd --append="$(cat /proc/cmdline)\
maxcpus=1" led to identifying this change as the needed fix.

With this change, an AP can now startup the BSP without error.

Signed-off-by: Bruce Rogers <brogers@suse.com>
---
 arch/x86/kvm/lapic.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 36591fa..eda6bfb 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -2170,10 +2170,7 @@ void kvm_apic_accept_events(struct kvm_vcpu *vcpu)
 	if (test_bit(KVM_APIC_INIT, &pe)) {
 		kvm_lapic_reset(vcpu, true);
 		kvm_vcpu_reset(vcpu, true);
-		if (kvm_vcpu_is_bsp(apic->vcpu))
-			vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
-		else
-			vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
+		vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
 	}
 	if (test_bit(KVM_APIC_SIPI, &pe) &&
 	    vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
-- 
1.9.0

Re: [PATCH 1/2] KVM: x86: fix ordering of cr0 initialization code in vmx_cpu_reset

[Nadav Amit]

Oops.

Anyhow, I see my patch has done a similar change in init_vmcb() , so you may
want to revert it as well.

Nadav

Bruce Rogers <brogers@suse.com> wrote:

> Commit d28bc9dd25ce reversed the order of two lines which initialize cr0,
> allowing the current (old) cr0 value to mess up vcpu initialization.
> This was observed in the checks for cr0 X86_CR0_WP bit in the context of
> kvm_mmu_reset_context().  Besides, setting vcpu->arch.cr0 after vmx_set_cr0()
> is completely redundant. Change the order back to ensure proper vcpu
> intiialization.
> 
> Signed-off-by: Bruce Rogers <brogers@suse.com>
> ---
> arch/x86/kvm/vmx.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index e2951b6..21507b4 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -4993,8 +4993,8 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
> 		vmcs_write16(VIRTUAL_PROCESSOR_ID, vmx->vpid);
> 
> 	cr0 = X86_CR0_NW | X86_CR0_CD | X86_CR0_ET;
> -	vmx_set_cr0(vcpu, cr0); /* enter rmode */
> 	vmx->vcpu.arch.cr0 = cr0;
> +	vmx_set_cr0(vcpu, cr0); /* enter rmode */
> 	vmx_set_cr4(vcpu, 0);
> 	vmx_set_efer(vcpu, 0);
> 	vmx_fpu_activate(vcpu);
> -- 
> 1.9.0

Re: [PATCH 1/2] KVM: x86: fix ordering of cr0 initialization code in vmx_cpu_reset

[Bruce Rogers]

I hadn't noticed that one - my testcase didn't encounter any issues on
AMD. Anyways, it's probably best to revert that change as well. I'll add
that in for a v2.

Bruce
>>> On 2/3/2016 at 04:18 PM, Nadav Amit <nadav.amit@gmail.com> wrote: 
> Oops.
> 
> Anyhow, I see my patch has done a similar change in init_vmcb() , so you may
> want to revert it as well.
> 
> Nadav
> 
> Bruce Rogers <brogers@suse.com> wrote:
> 
>> Commit d28bc9dd25ce reversed the order of two lines which initialize cr0,
>> allowing the current (old) cr0 value to mess up vcpu initialization.
>> This was observed in the checks for cr0 X86_CR0_WP bit in the context of
>> kvm_mmu_reset_context().  Besides, setting vcpu->arch.cr0 after vmx_set_cr0()
>> is completely redundant. Change the order back to ensure proper vcpu
>> intiialization.
>> 
>> Signed-off-by: Bruce Rogers <brogers@suse.com>
>> ---
>> arch/x86/kvm/vmx.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index e2951b6..21507b4 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -4993,8 +4993,8 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool 
> init_event)
>> 		vmcs_write16(VIRTUAL_PROCESSOR_ID, vmx->vpid);
>> 
>> 	cr0 = X86_CR0_NW | X86_CR0_CD | X86_CR0_ET;
>> -	vmx_set_cr0(vcpu, cr0); /* enter rmode */
>> 	vmx->vcpu.arch.cr0 = cr0;
>> +	vmx_set_cr0(vcpu, cr0); /* enter rmode */
>> 	vmx_set_cr4(vcpu, 0);
>> 	vmx_set_efer(vcpu, 0);
>> 	vmx_fpu_activate(vcpu);
>> -- 
>> 1.9.0
> 
> 
> --
> 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: [PATCH 1/2] KVM: x86: fix ordering of cr0 initialization code in vmx_cpu_reset

[Paolo Bonzini]

On 03/02/2016 23:51, Bruce Rogers wrote:
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index e2951b6..21507b4 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -4993,8 +4993,8 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>  		vmcs_write16(VIRTUAL_PROCESSOR_ID, vmx->vpid);
>  
>  	cr0 = X86_CR0_NW | X86_CR0_CD | X86_CR0_ET;
> -	vmx_set_cr0(vcpu, cr0); /* enter rmode */
>  	vmx->vcpu.arch.cr0 = cr0;
> +	vmx_set_cr0(vcpu, cr0); /* enter rmode */

Your comment that the assignment is redundant is correct, but I am
afraid that this fix is also wrong.  In particular, it would not cause
exit_lmode and enter_rmode to be called.

You are not describing which call to kvm_mmu_reset_context was messed
up, so I'm not sure how your patch is fixing things.

Paolo

Re: [PATCH 2/2] KVM: x86: allow BSP to handle INIT IPIs like APs do

[Paolo Bonzini]

On 03/02/2016 23:51, Bruce Rogers wrote:
> The INIT IPI event handler special cases the boot-strap processor
> (BSP) handling, avoiding the same mp state handling which is done for
> the other (AP) processors. Debugging a linux guest usage scenario of
> avoiding a reboot through the bios for a crash on any processor via eg:
> kexec -p /boot/vmlinuz --initrd=/boot/initrd --append="$(cat /proc/cmdline)\
> maxcpus=1" led to identifying this change as the needed fix.
> 
> With this change, an AP can now startup the BSP without error.
> 
> Signed-off-by: Bruce Rogers <brogers@suse.com>
> ---
>  arch/x86/kvm/lapic.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 36591fa..eda6bfb 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -2170,10 +2170,7 @@ void kvm_apic_accept_events(struct kvm_vcpu *vcpu)
>  	if (test_bit(KVM_APIC_INIT, &pe)) {
>  		kvm_lapic_reset(vcpu, true);
>  		kvm_vcpu_reset(vcpu, true);
> -		if (kvm_vcpu_is_bsp(apic->vcpu))
> -			vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
> -		else
> -			vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
> +		vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
>  	}
>  	if (test_bit(KVM_APIC_SIPI, &pe) &&
>  	    vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
> 

KVM_MP_STATE_INIT_RECEIVED is what Intel calls the "wait for SIPI"
state.  The BSP never gets a SIPI, it goes straight to 0xFFFFFFF0
instead.  Can you explain the problem more in detail?

Paolo

Re: [PATCH 2/2] KVM: x86: allow BSP to handle INIT IPIs like APs do

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1835 bytes --]

On 2016-02-08 16:12, Paolo Bonzini wrote:
> 
> 
> On 03/02/2016 23:51, Bruce Rogers wrote:
>> The INIT IPI event handler special cases the boot-strap processor
>> (BSP) handling, avoiding the same mp state handling which is done for
>> the other (AP) processors. Debugging a linux guest usage scenario of
>> avoiding a reboot through the bios for a crash on any processor via eg:
>> kexec -p /boot/vmlinuz --initrd=/boot/initrd --append="$(cat /proc/cmdline)\
>> maxcpus=1" led to identifying this change as the needed fix.
>>
>> With this change, an AP can now startup the BSP without error.
>>
>> Signed-off-by: Bruce Rogers <brogers@suse.com>
>> ---
>>  arch/x86/kvm/lapic.c | 5 +----
>>  1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>> index 36591fa..eda6bfb 100644
>> --- a/arch/x86/kvm/lapic.c
>> +++ b/arch/x86/kvm/lapic.c
>> @@ -2170,10 +2170,7 @@ void kvm_apic_accept_events(struct kvm_vcpu *vcpu)
>>  	if (test_bit(KVM_APIC_INIT, &pe)) {
>>  		kvm_lapic_reset(vcpu, true);
>>  		kvm_vcpu_reset(vcpu, true);
>> -		if (kvm_vcpu_is_bsp(apic->vcpu))
>> -			vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
>> -		else
>> -			vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
>> +		vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
>>  	}
>>  	if (test_bit(KVM_APIC_SIPI, &pe) &&
>>  	    vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
>>
> 
> KVM_MP_STATE_INIT_RECEIVED is what Intel calls the "wait for SIPI"
> state.  The BSP never gets a SIPI, it goes straight to 0xFFFFFFF0
> instead.  Can you explain the problem more in detail?

I suspect this is about sending INIT-SIPI from another CPU, directed to
the BSP, isn't it? We may have to differentiate between CPU (including
system) reset and that IPI case.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 1/2] KVM: x86: fix ordering of cr0 initialization code in vmx_cpu_reset

[Bruce Rogers]

>>> On 2/8/2016 at 08:09 AM, Paolo Bonzini <pbonzini@redhat.com> wrote: 

> 
> On 03/02/2016 23:51, Bruce Rogers wrote:
>> 
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index e2951b6..21507b4 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -4993,8 +4993,8 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool 
> init_event)
>>  		vmcs_write16(VIRTUAL_PROCESSOR_ID, vmx->vpid);
>>  
>>  	cr0 = X86_CR0_NW | X86_CR0_CD | X86_CR0_ET;
>> -	vmx_set_cr0(vcpu, cr0); /* enter rmode */
>>  	vmx->vcpu.arch.cr0 = cr0;
>> +	vmx_set_cr0(vcpu, cr0); /* enter rmode */
> 
> Your comment that the assignment is redundant is correct, but I am
> afraid that this fix is also wrong.  In particular, it would not cause
> exit_lmode and enter_rmode to be called.
> 
> You are not describing which call to kvm_mmu_reset_context was messed
> up, so I'm not sure how your patch is fixing things.

This is in the context of AP sending INIT to BSP with unrestricted_guest=N.

So the call sequence where I see the issue is: kvm_apic_accept_events() ->
kvm_vcpu_reset() -> vmx_vcpu_reset() -> vmx_set_cr0() -> enter_rmode() ->
kvm_mmu_reset_context().

enter_rmode is called in the case I am testing.

Bruce

Re: [PATCH 2/2] KVM: x86: allow BSP to handle INIT IPIs like APs do

[Bruce Rogers]

>>> On 2/8/2016 at 08:22 AM, Jan Kiszka <jan.kiszka@web.de> wrote: 
> On 2016-02-08 16:12, Paolo Bonzini wrote:
>> 
>> 
>> On 03/02/2016 23:51, Bruce Rogers wrote:
>>> The INIT IPI event handler special cases the boot-strap processor
>>> (BSP) handling, avoiding the same mp state handling which is done for
>>> the other (AP) processors. Debugging a linux guest usage scenario of
>>> avoiding a reboot through the bios for a crash on any processor via eg:
>>> kexec -p /boot/vmlinuz --initrd=/boot/initrd --append="$(cat /proc/cmdline)\
>>> maxcpus=1" led to identifying this change as the needed fix.
>>>
>>> With this change, an AP can now startup the BSP without error.
>>>
>>> Signed-off-by: Bruce Rogers <brogers@suse.com>
>>> ---
>>>  arch/x86/kvm/lapic.c | 5 +----
>>>  1 file changed, 1 insertion(+), 4 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>>> index 36591fa..eda6bfb 100644
>>> --- a/arch/x86/kvm/lapic.c
>>> +++ b/arch/x86/kvm/lapic.c
>>> @@ -2170,10 +2170,7 @@ void kvm_apic_accept_events(struct kvm_vcpu *vcpu)
>>>  	if (test_bit(KVM_APIC_INIT, &pe)) {
>>>  		kvm_lapic_reset(vcpu, true);
>>>  		kvm_vcpu_reset(vcpu, true);
>>> -		if (kvm_vcpu_is_bsp(apic->vcpu))
>>> -			vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
>>> -		else
>>> -			vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
>>> +		vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
>>>  	}
>>>  	if (test_bit(KVM_APIC_SIPI, &pe) &&
>>>  	    vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
>>>
>> 
>> KVM_MP_STATE_INIT_RECEIVED is what Intel calls the "wait for SIPI"
>> state.  The BSP never gets a SIPI, it goes straight to 0xFFFFFFF0
>> instead.  Can you explain the problem more in detail?
> 
> I suspect this is about sending INIT-SIPI from another CPU, directed to
> the BSP, isn't it? We may have to differentiate between CPU (including
> system) reset and that IPI case.

That is correct. In looking over the KVM code which deals with BSP, this was
the only place which seemed wrong to me wrt special casing for BSP outside the
context of initial system initialization / reset. As far as I understand the BSP shouldn't
be treated differently in this case.

Bruce

Re: [PATCH 2/2] KVM: x86: allow BSP to handle INIT IPIs like APs do

[Paolo Bonzini]

On 08/02/2016 17:33, Bruce Rogers wrote:
>>> >> 
>>> >> KVM_MP_STATE_INIT_RECEIVED is what Intel calls the "wait for SIPI"
>>> >> state.  The BSP never gets a SIPI, it goes straight to 0xFFFFFFF0
>>> >> instead.  Can you explain the problem more in detail?
>> > 
>> > I suspect this is about sending INIT-SIPI from another CPU, directed to
>> > the BSP, isn't it? We may have to differentiate between CPU (including
>> > system) reset and that IPI case.
> That is correct. In looking over the KVM code which deals with BSP, this was
> the only place which seemed wrong to me wrt special casing for BSP outside the
> context of initial system initialization / reset. As far as I understand the
> BSP shouldn't be treated differently in this case.

See 8.4.2 of the SDM:

If the MP protocol has completed and a BSP is chosen, subsequent INITs
(either to a specific processor or system wide) do not cause the MP
protocol to be repeated. Instead, each logical processor examines its
BSP flag (in the IA32_APIC_BASE MSR) to determine whether it should
execute the BIOS boot-strap code (if it is the BSP) or enter a
wait-for-SIPI state (if it is an AP).

So it is correct to treat the BSP differently here, I think.

Paolo

Re: [PATCH 1/2] KVM: x86: fix ordering of cr0 initialization code in vmx_cpu_reset

[Paolo Bonzini]

On 08/02/2016 17:29, Bruce Rogers wrote:
>>>> On 2/8/2016 at 08:09 AM, Paolo Bonzini <pbonzini@redhat.com> wrote: 
> 
>>
>> On 03/02/2016 23:51, Bruce Rogers wrote:
>>>
>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>> index e2951b6..21507b4 100644
>>> --- a/arch/x86/kvm/vmx.c
>>> +++ b/arch/x86/kvm/vmx.c
>>> @@ -4993,8 +4993,8 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool 
>> init_event)
>>>  		vmcs_write16(VIRTUAL_PROCESSOR_ID, vmx->vpid);
>>>  
>>>  	cr0 = X86_CR0_NW | X86_CR0_CD | X86_CR0_ET;
>>> -	vmx_set_cr0(vcpu, cr0); /* enter rmode */
>>>  	vmx->vcpu.arch.cr0 = cr0;
>>> +	vmx_set_cr0(vcpu, cr0); /* enter rmode */
>>
>> Your comment that the assignment is redundant is correct, but I am
>> afraid that this fix is also wrong.  In particular, it would not cause
>> exit_lmode and enter_rmode to be called.
>>
>> You are not describing which call to kvm_mmu_reset_context was messed
>> up, so I'm not sure how your patch is fixing things.
> 
> This is in the context of AP sending INIT to BSP with unrestricted_guest=N.
> 
> So the call sequence where I see the issue is: kvm_apic_accept_events() ->
> kvm_vcpu_reset() -> vmx_vcpu_reset() -> vmx_set_cr0() -> enter_rmode() ->
> kvm_mmu_reset_context().
> 
> enter_rmode is called in the case I am testing.

Please describe the bug as thoroughly as possible, especially the
initial state of the BSP and AP and how the bug manifests after the INIT
IPI.  It would be great to write a kvm-unit-tests testcase for it, but I
can do it too if you provide enough information.

Paolo

Re: [PATCH 2/2] KVM: x86: allow BSP to handle INIT IPIs like APs do

[Bruce Rogers]

>>> On 2/8/2016 at 09:40 AM, Paolo Bonzini <pbonzini@redhat.com> wrote: 

> 
> On 08/02/2016 17:33, Bruce Rogers wrote:
>>>> >> 
>>>> >> KVM_MP_STATE_INIT_RECEIVED is what Intel calls the "wait for SIPI"
>>>> >> state.  The BSP never gets a SIPI, it goes straight to 0xFFFFFFF0
>>>> >> instead.  Can you explain the problem more in detail?
>>> > 
>>> > I suspect this is about sending INIT-SIPI from another CPU, directed to
>>> > the BSP, isn't it? We may have to differentiate between CPU (including
>>> > system) reset and that IPI case.
>> That is correct. In looking over the KVM code which deals with BSP, this was
>> the only place which seemed wrong to me wrt special casing for BSP outside 
> the
>> context of initial system initialization / reset. As far as I understand the
>> BSP shouldn't be treated differently in this case.
> 
> See 8.4.2 of the SDM:
> 
> If the MP protocol has completed and a BSP is chosen, subsequent INITs
> (either to a specific processor or system wide) do not cause the MP
> protocol to be repeated. Instead, each logical processor examines its
> BSP flag (in the IA32_APIC_BASE MSR) to determine whether it should
> execute the BIOS boot-strap code (if it is the BSP) or enter a
> wait-for-SIPI state (if it is an AP).
> 
> So it is correct to treat the BSP differently here, I think.

I had read that, but I though this was speaking from the perspective of the
SMP aware BIOS code only. In other words, the BIOS would sidetrack AP's
(based on BSP flag not being present), while BSP would be allowed to go through
the regular BIOS code, checking for reset case, etc. An OS on the other hand
would be free to treat all x86 processors equally, once it has booted into
fully symmetrical mode.
I certainly could be wrong about my above interpretation, but with these
changes I'm proposing, things work well for the test case of manually onlining
the BSP after the crash kernel has been started (via kexec -e on a AP processor
with maxcpus=1 on the crash kernel command line). From looking through the
kernel git history it appears this sequence of events was explicitly supported
quite a while ago, and we've got a customer who uses this for fast recovery from
a guest kernel crash.

Bruce

Re: [PATCH 2/2] KVM: x86: allow BSP to handle INIT IPIs like APs do

[Bruce Rogers]

>>> On 2/8/2016 at 10:27 AM, Bruce Rogers wrote: 
> >>> On 2/8/2016 at 09:40 AM, Paolo Bonzini <pbonzini@redhat.com> wrote: 
> 
>> 
>> On 08/02/2016 17:33, Bruce Rogers wrote:
>>>>> >> 
>>>>> >> KVM_MP_STATE_INIT_RECEIVED is what Intel calls the "wait for SIPI"
>>>>> >> state.  The BSP never gets a SIPI, it goes straight to 0xFFFFFFF0
>>>>> >> instead.  Can you explain the problem more in detail?
>>>> > 
>>>> > I suspect this is about sending INIT-SIPI from another CPU, directed to
>>>> > the BSP, isn't it? We may have to differentiate between CPU (including
>>>> > system) reset and that IPI case.
>>> That is correct. In looking over the KVM code which deals with BSP, this was
>>> the only place which seemed wrong to me wrt special casing for BSP outside 
>> the
>>> context of initial system initialization / reset. As far as I understand the
>>> BSP shouldn't be treated differently in this case.
>> 
>> See 8.4.2 of the SDM:
>> 
>> If the MP protocol has completed and a BSP is chosen, subsequent INITs
>> (either to a specific processor or system wide) do not cause the MP
>> protocol to be repeated. Instead, each logical processor examines its
>> BSP flag (in the IA32_APIC_BASE MSR) to determine whether it should
>> execute the BIOS boot-strap code (if it is the BSP) or enter a
>> wait-for-SIPI state (if it is an AP).
>> 
>> So it is correct to treat the BSP differently here, I think.
> 
> I had read that, but I though this was speaking from the perspective of the
> SMP aware BIOS code only. In other words, the BIOS would sidetrack AP's
> (based on BSP flag not being present), while BSP would be allowed to go 
> through
> the regular BIOS code, checking for reset case, etc. An OS on the other hand
> would be free to treat all x86 processors equally, once it has booted into
> fully symmetrical mode.
> I certainly could be wrong about my above interpretation, but with these
> changes I'm proposing, things work well for the test case of manually 
> onlining
> the BSP after the crash kernel has been started (via kexec -e on a AP 
> processor
> with maxcpus=1 on the crash kernel command line). From looking through the
> kernel git history it appears this sequence of events was explicitly 
> supported
> quite a while ago, and we've got a customer who uses this for fast recovery 
> from
> a guest kernel crash.
> 
> Bruce

I mean kexec - p ... above, not kexec -e. Sorry about that.

Bruce

Re: [PATCH 2/2] KVM: x86: allow BSP to handle INIT IPIs like APs do

[Paolo Bonzini]

On 08/02/2016 18:27, Bruce Rogers wrote:
> I had read that, but I though this was speaking from the perspective of the
> SMP aware BIOS code only.

It says "logical processor", so I cannot really see how it can be
interpreted that way.  The BSP jumps to 0xFFFFFFF0, the APs go into
wait-for-SIPI state.

> I certainly could be wrong about my above interpretation, but with these
> changes I'm proposing, things work well for the test case of manually onlining
> the BSP after the crash kernel has been started (via kexec -e on a AP processor
> with maxcpus=1 on the crash kernel command line). From looking through the
> kernel git history it appears this sequence of events was explicitly supported
> quite a while ago, and we've got a customer who uses this for fast recovery from
> a guest kernel crash.

You need to comment on the output of trace-cmd for KVM events, or
provide a full reproducer, or at the very least point me to the kernel
code that you're referring to.  Otherwise I just cannot understand what
you're talking about; sorry. :(

Paolo

Re: [PATCH 2/2] KVM: x86: allow BSP to handle INIT IPIs like APs do

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2621 bytes --]

On 2016-02-08 18:38, Bruce Rogers wrote:
>>>> On 2/8/2016 at 10:27 AM, Bruce Rogers wrote: 
>>>>> On 2/8/2016 at 09:40 AM, Paolo Bonzini <pbonzini@redhat.com> wrote: 
>>
>>>
>>> On 08/02/2016 17:33, Bruce Rogers wrote:
>>>>>>>>
>>>>>>>> KVM_MP_STATE_INIT_RECEIVED is what Intel calls the "wait for SIPI"
>>>>>>>> state.  The BSP never gets a SIPI, it goes straight to 0xFFFFFFF0
>>>>>>>> instead.  Can you explain the problem more in detail?
>>>>>>
>>>>>> I suspect this is about sending INIT-SIPI from another CPU, directed to
>>>>>> the BSP, isn't it? We may have to differentiate between CPU (including
>>>>>> system) reset and that IPI case.
>>>> That is correct. In looking over the KVM code which deals with BSP, this was
>>>> the only place which seemed wrong to me wrt special casing for BSP outside 
>>> the
>>>> context of initial system initialization / reset. As far as I understand the
>>>> BSP shouldn't be treated differently in this case.
>>>
>>> See 8.4.2 of the SDM:
>>>
>>> If the MP protocol has completed and a BSP is chosen, subsequent INITs
>>> (either to a specific processor or system wide) do not cause the MP
>>> protocol to be repeated. Instead, each logical processor examines its
>>> BSP flag (in the IA32_APIC_BASE MSR) to determine whether it should
>>> execute the BIOS boot-strap code (if it is the BSP) or enter a
>>> wait-for-SIPI state (if it is an AP).
>>>
>>> So it is correct to treat the BSP differently here, I think.
>>
>> I had read that, but I though this was speaking from the perspective of the
>> SMP aware BIOS code only. In other words, the BIOS would sidetrack AP's
>> (based on BSP flag not being present), while BSP would be allowed to go 
>> through
>> the regular BIOS code, checking for reset case, etc. An OS on the other hand
>> would be free to treat all x86 processors equally, once it has booted into
>> fully symmetrical mode.
>> I certainly could be wrong about my above interpretation, but with these
>> changes I'm proposing, things work well for the test case of manually 
>> onlining
>> the BSP after the crash kernel has been started (via kexec -e on a AP 
>> processor
>> with maxcpus=1 on the crash kernel command line). From looking through the
>> kernel git history it appears this sequence of events was explicitly 
>> supported
>> quite a while ago, and we've got a customer who uses this for fast recovery 
>> from
>> a guest kernel crash.
>>
>> Bruce
> 
> I mean kexec - p ... above, not kexec -e. Sorry about that.

How does real HW behave with your kexec case? Did you try this?

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 2/2] KVM: x86: allow BSP to handle INIT IPIs like APs do

[Bruce Rogers]

>>> On 2/8/2016 at 10:53 AM, Jan Kiszka <jan.kiszka@web.de> wrote: 
> On 2016-02-08 18:38, Bruce Rogers wrote:
>>>>> On 2/8/2016 at 10:27 AM, Bruce Rogers wrote: 
>>>>>> On 2/8/2016 at 09:40 AM, Paolo Bonzini <pbonzini@redhat.com> wrote: 
>>>
>>>>
>>>> On 08/02/2016 17:33, Bruce Rogers wrote:
>>>>>>>>>
>>>>>>>>> KVM_MP_STATE_INIT_RECEIVED is what Intel calls the "wait for SIPI"
>>>>>>>>> state.  The BSP never gets a SIPI, it goes straight to 0xFFFFFFF0
>>>>>>>>> instead.  Can you explain the problem more in detail?
>>>>>>>
>>>>>>> I suspect this is about sending INIT-SIPI from another CPU, directed to
>>>>>>> the BSP, isn't it? We may have to differentiate between CPU (including
>>>>>>> system) reset and that IPI case.
>>>>> That is correct. In looking over the KVM code which deals with BSP, this was
>>>>> the only place which seemed wrong to me wrt special casing for BSP outside 
>>>> the
>>>>> context of initial system initialization / reset. As far as I understand the
>>>>> BSP shouldn't be treated differently in this case.
>>>>
>>>> See 8.4.2 of the SDM:
>>>>
>>>> If the MP protocol has completed and a BSP is chosen, subsequent INITs
>>>> (either to a specific processor or system wide) do not cause the MP
>>>> protocol to be repeated. Instead, each logical processor examines its
>>>> BSP flag (in the IA32_APIC_BASE MSR) to determine whether it should
>>>> execute the BIOS boot-strap code (if it is the BSP) or enter a
>>>> wait-for-SIPI state (if it is an AP).
>>>>
>>>> So it is correct to treat the BSP differently here, I think.
>>>
>>> I had read that, but I though this was speaking from the perspective of the
>>> SMP aware BIOS code only. In other words, the BIOS would sidetrack AP's
>>> (based on BSP flag not being present), while BSP would be allowed to go 
>>> through
>>> the regular BIOS code, checking for reset case, etc. An OS on the other hand
>>> would be free to treat all x86 processors equally, once it has booted into
>>> fully symmetrical mode.
>>> I certainly could be wrong about my above interpretation, but with these
>>> changes I'm proposing, things work well for the test case of manually 
>>> onlining
>>> the BSP after the crash kernel has been started (via kexec -e on a AP 
>>> processor
>>> with maxcpus=1 on the crash kernel command line). From looking through the
>>> kernel git history it appears this sequence of events was explicitly 
>>> supported
>>> quite a while ago, and we've got a customer who uses this for fast recovery 
>>> from
>>> a guest kernel crash.
>>>
>>> Bruce
>> 
>> I mean kexec - p ... above, not kexec -e. Sorry about that.
> 
> How does real HW behave with your kexec case? Did you try this?


I appreciate the review of these patches. I think I was premature in sharing
these patches, not having more fully vetting our customer's claims, and by
trying to satisfy their request that it also work the same under KVM
Virtualization without more complete details.

I had initially tried it on real hardware I have and it didn't behave as the
customer claimed, but mistakenly I dropped that concern, More recent testing
on additional machines continues to show that this approach does not work
on real hardware, and I think we want KVM to act like real hardware here.

I've asked for more details from the customer on their hardware and the specifics
of their use case. I'm also taking more time to get familiar with how this part of
KVM works.

Thanks,

Bruce

Re: [PATCH 1/2] KVM: x86: fix ordering of cr0 initialization code in vmx_cpu_reset

[Bruce Rogers]

>>> On 2/3/2016 at 04:18 PM, Nadav Amit <nadav.amit@gmail.com> wrote: 
> Oops.
> 
> Anyhow, I see my patch has done a similar change in init_vmcb() , so you may
> want to revert it as well.
> 
> Nadav
> 
> Bruce Rogers <brogers@suse.com> wrote:
> 
>> Commit d28bc9dd25ce reversed the order of two lines which initialize cr0,
>> allowing the current (old) cr0 value to mess up vcpu initialization.
>> This was observed in the checks for cr0 X86_CR0_WP bit in the context of
>> kvm_mmu_reset_context().  Besides, setting vcpu->arch.cr0 after vmx_set_cr0()
>> is completely redundant. Change the order back to ensure proper vcpu
>> intiialization.
>> 
>> Signed-off-by: Bruce Rogers <brogers@suse.com>
>> ---
>> arch/x86/kvm/vmx.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index e2951b6..21507b4 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -4993,8 +4993,8 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool 
> init_event)
>> 		vmcs_write16(VIRTUAL_PROCESSOR_ID, vmx->vpid);
>> 
>> 	cr0 = X86_CR0_NW | X86_CR0_CD | X86_CR0_ET;
>> -	vmx_set_cr0(vcpu, cr0); /* enter rmode */
>> 	vmx->vcpu.arch.cr0 = cr0;
>> +	vmx_set_cr0(vcpu, cr0); /* enter rmode */
>> 	vmx_set_cr4(vcpu, 0);
>> 	vmx_set_efer(vcpu, 0);
>> 	vmx_fpu_activate(vcpu);
>> -- 
>> 1.9.0
> 

I had not pursued this as the initial problem I was chasing ended up including some
undefined behavior.

Since, I've run into another failure which this patch addresses (ovmf based booting with
vcpu count >1 on older hardware), so I'll resend this one patch with updated info.

Also, it seems to me  that the init_vmcb() svm issue Nadav mentioned is no longer an
issue in the current master branch so I won't be addressing that.

Bruce

Re: [PATCH 1/2] KVM: x86: fix ordering of cr0 initialization code in vmx_cpu_reset

[Wanpeng Li]

2016-02-09 0:29 GMT+08:00 Bruce Rogers <brogers@suse.com>:
>>>> On 2/8/2016 at 08:09 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>>
>> On 03/02/2016 23:51, Bruce Rogers wrote:
>>>
>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>> index e2951b6..21507b4 100644
>>> --- a/arch/x86/kvm/vmx.c
>>> +++ b/arch/x86/kvm/vmx.c
>>> @@ -4993,8 +4993,8 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool
>> init_event)
>>>              vmcs_write16(VIRTUAL_PROCESSOR_ID, vmx->vpid);
>>>
>>>      cr0 = X86_CR0_NW | X86_CR0_CD | X86_CR0_ET;
>>> -    vmx_set_cr0(vcpu, cr0); /* enter rmode */
>>>      vmx->vcpu.arch.cr0 = cr0;
>>> +    vmx_set_cr0(vcpu, cr0); /* enter rmode */
>>
>> Your comment that the assignment is redundant is correct, but I am
>> afraid that this fix is also wrong.  In particular, it would not cause
>> exit_lmode and enter_rmode to be called.
>>
>> You are not describing which call to kvm_mmu_reset_context was messed
>> up, so I'm not sure how your patch is fixing things.
>
> This is in the context of AP sending INIT to BSP with unrestricted_guest=N.

BSP will broadcast INIT-SIPI-SIPI sequence to APs during
initialization, could you point out when "AP sending INIT to BSP" as
you mentioned above in SDM?

Regards,
Wanpeng Li

Re: [PATCH 1/2] KVM: x86: fix ordering of cr0 initialization code in vmx_cpu_reset

[Bruce Rogers]

>>> On 4/26/2016 at 08:54 PM, Wanpeng Li <kernellwp@gmail.com> wrote: 
> 2016-02-09 0:29 GMT+08:00 Bruce Rogers <brogers@suse.com>:
>>>>> On 2/8/2016 at 08:09 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>>
>>> On 03/02/2016 23:51, Bruce Rogers wrote:
>>>>
>>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>>> index e2951b6..21507b4 100644
>>>> --- a/arch/x86/kvm/vmx.c
>>>> +++ b/arch/x86/kvm/vmx.c
>>>> @@ -4993,8 +4993,8 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool
>>> init_event)
>>>>              vmcs_write16(VIRTUAL_PROCESSOR_ID, vmx->vpid);
>>>>
>>>>      cr0 = X86_CR0_NW | X86_CR0_CD | X86_CR0_ET;
>>>> -    vmx_set_cr0(vcpu, cr0); /* enter rmode */
>>>>      vmx->vcpu.arch.cr0 = cr0;
>>>> +    vmx_set_cr0(vcpu, cr0); /* enter rmode */
>>>
>>> Your comment that the assignment is redundant is correct, but I am
>>> afraid that this fix is also wrong.  In particular, it would not cause
>>> exit_lmode and enter_rmode to be called.
>>>
>>> You are not describing which call to kvm_mmu_reset_context was messed
>>> up, so I'm not sure how your patch is fixing things.
>>
>> This is in the context of AP sending INIT to BSP with unrestricted_guest=N.
> 
> BSP will broadcast INIT-SIPI-SIPI sequence to APs during
> initialization, could you point out when "AP sending INIT to BSP" as
> you mentioned above in SDM?
> 

You should know that I abandoned this patch series as further investigation
revealed that this was not as cut and dried as I had first hoped.

Bruce