Avi:
Most of the comments are fixed, but need to have a double check
on several point of them. Can u have a look?
Eddie
>> +
>> + switch (delivery_mode) {
>> + case dest_LowestPrio:
>>
>
>Wierd constant. How about IOAPIC_DEST_LOWEST_PRIO?
dest_LowestPrio is defined in native Linux asm-i386/io_apic.h &
asm-x86_64/io_apic.h. Do u want to add new definition?
>
>> +static void vcpu_kick_intr(void *info)
>> +{
>> +#ifdef DEBUG
>> + struct kvm_vcpu *vcpu = (struct kvm_vcpu *)info;
>> + printk(KERN_DEBUG "vcpu_kick_intr %p \n", vcpu);
>> +#endif
>> +}
>> +
>> +void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
>> +{
>> + int ipi_pcpu = vcpu->cpu;
>> +
>> + if (vcpu->guest_mode)
>> + smp_call_function_single(ipi_pcpu, vcpu_kick_intr, vcpu,
>> 0, 0);
>> +}
>>
>
>What if it's in hlt state?
If guest is in hlt time, vcpu->guest_mode=0. (VM Exit always clear this
one).
BTW, even we give a redunadant IPI, it is just a minor performance issue
only.
>> +
>> +struct kvm_ioapic {
>> + struct kvm_io_device dev;
>> + unsigned long base_address;
>> + struct kvm *kvm;
>> + u32 ioregsel;
>> + u32 id;
>> + u32 irr;
>> + union ioapic_redir_entry {
>> + u64 bits;
>> + struct {
>> + u8 vector;
>> + u8 delivery_mode:3;
>> + u8 dest_mode:1;
>> + u8 delivery_status:1;
>> + u8 polarity:1;
>> + u8 remote_irr:1;
>> + u8 trig_mode:1;
>> + u8 mask:1;
>> + u8 reserve:7;
>> + u8 reserved[4];
>> + u8 dest_id;
>> + } fields;
>> + } redirtbl[IOAPIC_NUM_PINS];
>> +};
>>
>
>Which lock protects this?
kvm->lock.
When guest do ioapic ops, it is in shadow page fault handler,
and can take kvm->lock for page fault.
If asynchronize Qemu do ioapic ops, it will take this lock too.
>
>> +
>> +struct kvm_lapic {
>> + spinlock_t lock; /* TODO: need? */
>>
>
>I think not. Maybe when we have msi support?
I want to use kvm->lock for lapic too in future. But leave as it is now.
The key thing is to cancel hrtimer when VP migrates.
>> --- a/drivers/kvm/kvm.h
>> +++ b/drivers/kvm/kvm.h
>> @@ -334,15 +334,13 @@ struct kvm_vcpu {
>> };
>> struct mutex mutex;
>> int cpu;
>> - int launched;
>> + char vcpu_id;
>>
>
>There is already a vcpu_id in kvm.git...
>
>> + char launched;
>>
>
>
>???
I saw you added vcpu_id in main steam, can u pull this to lapic2 too?
So I don't need to add redundantly.
>
>>
>> +unsigned long get_cr8(struct kvm_vcpu *vcpu)
>> +{
>> + if (irqchip_in_kernel(vcpu->kvm))
>> + return kvm_lapic_get_cr8(vcpu);
>> + else
>> + return vcpu->cr8;
>> +}
>> +EXPORT_SYMBOL_GPL(get_cr8);
>>
>
>How about keep vcpu->cr8 even with kernel lapic? then we
>don't need this.
We need to sync cr8 with vTPR, Are u suggesting to sync them every
VM_EXIT?
That means we sparse apic registers in different place and extra sync
issue.
I can seperate the patch as a preparation patch to wrap all cr8 access.
Which one is prefered?
>
>> +
>> +u64 kvm_get_apic_base(struct kvm_vcpu *vcpu)
>> +{
>> + if (irqchip_in_kernel(vcpu->kvm))
>> + return vcpu->apic->base_msr;
>> + else
>> + return vcpu->apic_base;
>> +}
>> +EXPORT_SYMBOL_GPL(kvm_get_apic_base);
>>
>
>ditto.
>
>Where are the state save/restore? they can be added later, so long as
>the capability is enabled only after everything is working.
>
this is only for branch check-in and we need to stay in branch for one
more week.
Eddie
-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
kvm-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel
