Re: apic+ioapiuc patch

[Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org>]

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/