Re: KVM: x86: accessors for guest registers

[Avi Kivity <avi@qumranet.com>]

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.