From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marcelo Tosatti <mtosatti@redhat.com>
Subject: Re: RFC: cache_regs in kvm_emulate_pio
Date: Thu, 26 Jun 2008 11:52:43 -0300
Message-ID: <20080626145243.GB14336@dmt.cnet>
References: <20080621194639.GA15032@dmt.cnet> <485DE023.4060006@qumranet.com> <20080624193316.GA12618@dmt.cnet> <48635ECF.7030601@qumranet.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: kvm-devel <kvm@vger.kernel.org>
To: Avi Kivity <avi@qumranet.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([66.187.233.31]:40193 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1754057AbYFZOxH (ORCPT <rfc822;kvm@vger.kernel.org>);
	Thu, 26 Jun 2008 10:53:07 -0400
Content-Disposition: inline
In-Reply-To: <48635ECF.7030601@qumranet.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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.