From mboxrd@z Thu Jan  1 00:00:00 1970
From: Avi Kivity <avi@qumranet.com>
Subject: Re: KVM: x86: accessors for guest registers
Date: Mon, 30 Jun 2008 08:08:58 +0300
Message-ID: <48686A6A.6070302@qumranet.com>
References: <20080627175802.GA18541@dmt.cnet> <48678ABD.4070209@qumranet.com> <20080629193526.GA8337@dmt.cnet>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Cc: "Yang, Sheng" <sheng.yang@intel.com>,
	kvm-devel <kvm@vger.kernel.org>
To: Marcelo Tosatti <mtosatti@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from il.qumranet.com ([212.179.150.194]:33788 "EHLO il.qumranet.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1750878AbYF3FIk (ORCPT <rfc822;kvm@vger.kernel.org>);
	Mon, 30 Jun 2008 01:08:40 -0400
In-Reply-To: <20080629193526.GA8337@dmt.cnet>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

Marcelo Tosatti wrote:
>
>> - svm always caches registers, and all registers are dirty, since  
>> cache/decache is cheap
>>     
>
> Accurate regs_dirty information is useful for converting the emulator,
> so that you can do something like:
>
> emul_register_write(ctxt, reg, val)
> {
>     if (!__test_and_set_bit(reg, &ctxt->vcpu->regs_dirty))
>         ctxt->original_regs[reg] = kvm_register_read(ctxt->vcpu, reg);
>     ctxt->vcpu->regs[reg] = val;
> }
>
> Because restoring the original reg contents on failure is necessary.
> Otherwise you need to cache all regs on emulation entry. RIP is always
> read anyway, but RSP not so frequently.
>
> Well, might not be worth the complexity for saving just one vmcs_read().
> Or it can be changed later during conversion.
>
>   

That doesn't work, because some of the registers may already be dirty 
when the emulator is invoked (say, if we're emulating several 
instructions back-to-back).  I think the best way to change the emulator 
is to let it have its own set of dirty/available bits.

>>  		if (io->in) {
>>  			r = pio_copy_data(vcpu);
>>  			if (r) {
>> -				kvm_x86_ops->cache_regs(vcpu);
>> +				kvm_x86_ops->cache_reg(vcpu, VCPU_REGS_RAX);
>>  				return r;
>>  			}
>>     
>
> These two don't go well together. Apparently the intent of this
> ->cache_regs call on failure was to restore the original registers in
> case they were modified by pio_copy_data? But pio_copy_data does not
> write to any guest register (and even if it did, this ->cache_regs call
> assumes what registers are fetched from the guest's originals).
>
>         kvm_x86_ops->cache_regs(vcpu);
>
>         if (!io->string) {
>                 if (io->in)
>                         memcpy(&vcpu->arch.regs[VCPU_REGS_RAX], vcpu->arch.pio_data,
>                                io->size);
>         } else {
>                 if (io->in) {
>                         r = pio_copy_data(vcpu);
>                         if (r) {
>                                 kvm_x86_ops->cache_regs(vcpu);
>                                 return r;
>                         }
>                 }
>
> Unless I'm mistaken you can just remove it.
>
>   

Right.


-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.