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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
