Re: RFC: cache_regs in kvm_emulate_pio

[Marcelo Tosatti <mtosatti@redhat.com>]

On Thu, Jun 26, 2008 at 12:18:07PM +0300, Avi Kivity wrote:
>
>>   static int set_guest_debug(struct kvm_vcpu *vcpu, struct 
>> kvm_debug_guest *dbg)
>> @@ -2370,22 +2379,18 @@ static int handle_cr(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>>  			    (u32)((u64)vcpu->arch.regs[reg] >> 32), handler);
>>  		switch (cr) {
>>  		case 0:
>> -			vcpu_load_rsp_rip(vcpu);
>>  			kvm_set_cr0(vcpu, vcpu->arch.regs[reg]);
>>   
>
> What if reg points at rsp? You need to replace arch.regs[*] with the  
> accessor.

Catch!

>> @@ -2865,6 +2904,8 @@ again:
>>  	local_irq_enable();
>>   	++vcpu->stat.exits;
>> +	vcpu->arch.regs_available = KVM_CACHED_REGS;
>> +	vcpu->arch.regs_dirty = 0;
>>   
>
> How can you have a constant for this? Each subarch has different cached  
> regs. This ought to be set in $subarch_vcpu_run.

This is the intersection of registers cached by both architectures. But
I agree that moving it down to subarch code is saner.

>> diff --git a/include/asm-x86/kvm_host.h b/include/asm-x86/kvm_host.h
>> index 851184d..cc5c94b 100644
>> --- a/include/asm-x86/kvm_host.h
>> +++ b/include/asm-x86/kvm_host.h
>> @@ -87,7 +87,7 @@ extern struct list_head vm_list;
>>  struct kvm_vcpu;
>>  struct kvm;
>>  -enum {
>> +enum kvm_reg {
>>  	VCPU_REGS_RAX = 0,
>>  	VCPU_REGS_RCX = 1,
>>  	VCPU_REGS_RDX = 2,
>> @@ -106,9 +106,21 @@ enum {
>>  	VCPU_REGS_R14 = 14,
>>  	VCPU_REGS_R15 = 15,
>>  #endif
>> +	VCPU_REGS_RIP = 16,
>>  	NR_VCPU_REGS
>>  };
>>   
>
> No, rip is not a GPR.

We need the RIP index to be part of the kvm_reg space (to index in the
dirty/available bitmaps). Otherwise you have to special case it.

Sure it is not a GPR, but what is the problem storing RIP in regs array
instead of a separate variable ?

> Perhaps we ought to have a guest_rip_read() since rip is not truly a GPR.

#define kvm_guest_read_rip(vcpu) kvm_guest_register_read(vcpu, VCPU_REGS_RIP)

?

Will fix the remaining comments.