Re: kernel device reset support

Dong, Eddie
2007-10-11T01:32:48Z

Avi Kivity wrote:
> Dong, Eddie wrote:
>> Avi Kivity wrote:
>>

>
> It's just like the guest kernel executing hlt. Why is there a
> difference?

Current VP wake up logic thru INIT/SIPI doesn't support this when
irqchip in kernel.

>
>>
>> Yes, halt all APs and let BSP do reset ops in user level.
>> Will post patch to Qemu to support SMP reboot some time later.
>>
>>
>
> Wait, that's a big change. Need to think about this...

Like talked in previous, we either let BSP do this, or let an AP
become BSP. Current Qemu doesn't support this.

>>
>> - if (vcpu->requests)
>> + if (vcpu->requests) {
>> if (test_and_clear_bit(KVM_TLB_FLUSH, &vcpu->requests))
>> kvm_x86_ops->tlb_flush(vcpu);
>> + if (test_and_clear_bit(KVM_FROZEN, &vcpu->requests)) {
>> + local_irq_enable(); +
preempt_enable();
>> + r = -EINTR;
>> + kvm_run->exit_reason = KVM_EXIT_FROZEN;
>> + goto out;
>> + }
>> + }
>>
>
>
> Why not just call vcpu_reset() here, then call vcpu_halt() if
> we're an AP?

Then you need to duplicate following code, which may bring extra issue
such as GUEST_CR3 update (kvm_mmu_reload) which is done after
this code but before vcpu->requests test. BTW, switch back to user level
to retry is cleaner IMO.

if (unlikely(vcpu->mp_state == VCPU_MP_STATE_UNINITIALIZED)) {
if (irqchip_in_kernel(vcpu->kvm) && vcpu->apic)
kvm_lapic_reset(vcpu);
kvm_vcpu_block(vcpu);
vcpu_put(vcpu);
return -EAGAIN;
}

>
>> + while (test_bit(KVM_FROZEN, &vcpu->requests))
>> + schedule();
>>
>
> I don't think we need to wait here.

The VCPU may be executing in kernel still, which may modify kernel
device state. E.g. A VCPU may be doing PIO emulating.

If BSP reset the kernel devices earlier than the VCPU modify the device
state,
we are in trouble.

>
>> + }
>> + else {
>> + vcpu->mp_state = VCPU_MP_STATE_RUNNABLE;
>> + kvm_lapic_reset(vcpu);
>> + }
>> + }
>> + /* Now only BSP is running... */
>> + kvm_reset_devices(kvm);
>>
>
> But now you're reseting the devices while vcpu 0 may be
> running. If in

No, VCPU0 (BSP) is current VCPU (though you don't have the current
vcpu parameter explicitly) like mentioned in previous mail and
as pre-requirement of user level change. Please refer my abswer above
of this mail.

> the first stage you halt all vcpus, and then restart vcpu 0 after the
> reset, you avoid the race.
>

No race here. The code is executing in VCPU0 context.

thx,eddie