Re: [PATCH] KVM: x86: Rework INIT and SIPI handling

[Jan Kiszka <jan.kiszka@siemens.com>]

On 2013-03-13 13:16, Gleb Natapov wrote:
> On Wed, Mar 13, 2013 at 12:16:13PM +0100, Jan Kiszka wrote:
>>>> @@ -1199,11 +1199,8 @@ static void svm_vcpu_reset(struct kvm_vcpu *vcpu)
>>>>
>>>>       init_vmcb(svm);
>>>>
>>>> -     if (!kvm_vcpu_is_bsp(vcpu)) {
>>>> +     if (!kvm_vcpu_is_bsp(vcpu))
>>>>               kvm_rip_write(vcpu, 0);
>>> Table 9-1 in SDM says that after INIT reset RIP is 0xfff0. Not
>>> mentioning AP or BSP. We should drop any mentioning of kvm_vcpu_is_bsp()
>>> in vmx and svm reset code and thing should just work.
>>
>> SDM says that APs start up at 0x000VV000 (with VV == SIPI vector) - this
>> implies RIP is 0. I suppose no SMP guest would boot if we change this.
>>
> Yes, correct. Setting RIP to 0 should be moved to SIPI handling.

Done.

> 
>>>
>>>> -             svm->vmcb->save.cs.base = svm->vcpu.arch.sipi_vector << 12;
>>>> -             svm->vmcb->save.cs.selector = svm->vcpu.arch.sipi_vector << 8;
>>>> -     }
>>>>
>>>>       kvm_cpuid(vcpu, &eax, &dummy, &dummy, &dummy);
>>>>       kvm_register_write(vcpu, VCPU_REGS_RDX, eax);
>>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>>> index f17cd2a..5b862ed 100644
>>>> --- a/arch/x86/kvm/vmx.c
>>>> +++ b/arch/x86/kvm/vmx.c
>>>> @@ -4121,10 +4121,6 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu)
>>>>       seg_setup(VCPU_SREG_CS);
>>>>       if (kvm_vcpu_is_bsp(&vmx->vcpu))
>>>>               vmcs_write16(GUEST_CS_SELECTOR, 0xf000);
>>>> -     else {
>>>> -             vmcs_write16(GUEST_CS_SELECTOR, vmx->vcpu.arch.sipi_vector << 8);
>>>> -             vmcs_writel(GUEST_CS_BASE, vmx->vcpu.arch.sipi_vector << 12);
>>>> -     }
>>>>
>>>>       seg_setup(VCPU_SREG_DS);
>>>>       seg_setup(VCPU_SREG_ES);
>>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>>> index b891ac3..37c0807 100644
>>>> --- a/arch/x86/kvm/x86.c
>>>> +++ b/arch/x86/kvm/x86.c
>>>> @@ -162,8 +162,6 @@ u64 __read_mostly host_xcr0;
>>>>
>>>>  static int emulator_fix_hypercall(struct x86_emulate_ctxt *ctxt);
>>>>
>>>> -static void kvm_vcpu_reset(struct kvm_vcpu *vcpu);
>>>> -
>>>>  static inline void kvm_async_pf_hash_reset(struct kvm_vcpu *vcpu)
>>>>  {
>>>>       int i;
>>>> @@ -2823,10 +2821,9 @@ static void kvm_vcpu_ioctl_x86_get_vcpu_events(struct kvm_vcpu *vcpu,
>>>>       events->nmi.masked = kvm_x86_ops->get_nmi_mask(vcpu);
>>>>       events->nmi.pad = 0;
>>>>
>>>> -     events->sipi_vector = vcpu->arch.sipi_vector;
>>>> +     events->sipi_vector = 0; /* never valid when reporting to user space */
>>>>
>>>>       events->flags = (KVM_VCPUEVENT_VALID_NMI_PENDING
>>>> -                      | KVM_VCPUEVENT_VALID_SIPI_VECTOR
>>>>                        | KVM_VCPUEVENT_VALID_SHADOW);
>>>>       memset(&events->reserved, 0, sizeof(events->reserved));
>>>>  }
>>>> @@ -2857,8 +2854,9 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu,
>>>>               vcpu->arch.nmi_pending = events->nmi.pending;
>>>>       kvm_x86_ops->set_nmi_mask(vcpu, events->nmi.masked);
>>>>
>>>> -     if (events->flags & KVM_VCPUEVENT_VALID_SIPI_VECTOR)
>>>> -             vcpu->arch.sipi_vector = events->sipi_vector;
>>>> +     if (events->flags & KVM_VCPUEVENT_VALID_SIPI_VECTOR &&
>>>> +         kvm_vcpu_has_lapic(vcpu))
>>>> +             vcpu->arch.apic->sipi_vector = events->sipi_vector;
>>> This looks out of place in this patch. Why is it needed?
>>
>> It is required as long as we support MP_STATE_SIPI_RECEIVED as input
>> from user space.
> What problem are fixing with adding kvm_vcpu_has_lapic() here?

A NULL pointer access when setting the mp_state of a VCPU without an APIC.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux