Dong, Eddie wrote:
> 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?
>
>
No, wierd constants from Linux are okay.
>>> +
>>> +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.
>
>
>
Okay.
>>> +
>>> +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.
>
>
Okay.
>>> --- 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.
>
>
Rebased and pushed.
>>>
>>> +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?
>
A separate patch, please.
>
>>> +
>>> +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.
Sure.
--
error compiling committee.c: too many arguments to function
-------------------------------------------------------------------------
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
