Re: KVM: x86: accessors for guest registers

[Marcelo Tosatti <mtosatti@redhat.com>]

On Sun, Jun 29, 2008 at 04:14:37PM +0300, Avi Kivity wrote:
> Marcelo Tosatti wrote:
>> As suggested by Avi, introduce accessors to read/write guest registers.
>> This simplifies the ->cache_regs/->decache_regs interface, and improves
>> register caching which is important for VMX, where the cost of
>> vmcs_read/vmcs_write is significant.
>>   
>
> I made some changes, which I think simplify things:

Stripped down, looks great.

> - 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.

> - kvm_register_write() sets the avail bit to avoid read-after-write  
> corruption (like forwarding in a real cpu :)

Eek.

> - made rip a GPR in spite of my own objections, it does simplify things
> - init avail and dirty bitmasks on cpu reset
> - convert a couple mode GUEST_RIP references
> - remove decache_all_regs
> - inline decache_reg() into vmx/svm
>
> How does the attached look?

> -	.cache_regs = svm_cache_regs,
> -	.decache_regs = svm_decache_regs,
>  	.get_rflags = svm_get_rflags,
>  	.set_rflags = svm_set_rflags,

>  		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.