Re: [PATCH] KVM: x86 emulator: access GPRs on demand

[Avi Kivity <avi@redhat.com>]

On 08/08/2012 12:23 AM, Marcelo Tosatti wrote:
>> @@ -281,8 +294,10 @@ struct x86_emulate_ctxt {
>>  	bool rip_relative;
>>  	unsigned long _eip;
>>  	struct operand memop;
>> +	u32 regs_valid;  /* bitmaps of registers in _regs[] that can be read */
>> +	u32 regs_dirty;  /* bitmaps of registers in _regs[] that have been written */
> 
> emul_regs_dirty (to avoid with the other regs_dirty).

Well, it's in x86_emulate_ctxt, and there are other fields that can be
confused here.  I think it's okay as they're never used together.

>>  
>> @@ -4453,7 +4447,6 @@ int kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq, int inc_eip)
>>  		return EMULATE_FAIL;
>>  
>>  	ctxt->eip = ctxt->_eip;
>> -	memcpy(vcpu->arch.regs, ctxt->regs, sizeof ctxt->regs);
>>  	kvm_rip_write(vcpu, ctxt->eip);
>>  	kvm_set_rflags(vcpu, ctxt->eflags);
> 
> 
> Need to writeback here?

In emulate_in_real().  Good catch.

> 
>> @@ -4601,7 +4594,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
>>  	   changes registers values  during IO operation */
>>  	if (vcpu->arch.emulate_regs_need_sync_from_vcpu) {
>>  		vcpu->arch.emulate_regs_need_sync_from_vcpu = false;
>> -		memcpy(ctxt->regs, vcpu->arch.regs, sizeof ctxt->regs);
>> +		ctxt->regs_valid = 0;
>>  	}
> 
> I think you can improve this hack now (perhaps invalidate the emulator
> cache on SET_REGS?).

This is dangerous, if someone does a GET_REGS/SET_REGS pair within an
mmio transaction.

The documentation implies it should not be done, but it doesn't say so
explicitly.  It's likely qemu allows it if the mmio transaction drops
the lock, for example.

I agree that doing it in SET_REGS is better conceptually.

> 
>>  		vcpu->arch.emulate_regs_need_sync_to_vcpu = false;
>>  	}
>>  	regs->rax = kvm_register_read(vcpu, VCPU_REGS_RAX);
>> @@ -5724,6 +5720,7 @@ int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int idt_index,
>>  {
>>  	struct x86_emulate_ctxt *ctxt = &vcpu->arch.emulate_ctxt;
>>  	int ret;
>> +	unsigned reg;
>>  
>>  	init_emulate_ctxt(vcpu);
>>  
>> @@ -5733,7 +5730,8 @@ int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int idt_index,
>>  	if (ret)
>>  		return EMULATE_FAIL;
>>  
>> -	memcpy(vcpu->arch.regs, ctxt->regs, sizeof ctxt->regs);
>> +	for_each_set_bit(reg, (ulong *)&ctxt->regs_dirty, 16)
>> +		kvm_register_write(vcpu, reg, ctxt->_regs[reg]);
> 
> Should update regs_avail/regs_dirty? Better to do any of that in
> emulator.c via interfaces.

emulator_task_switch() should do it, yes.

> 
>>  	kvm_rip_write(vcpu, ctxt->eip);
>>  	kvm_set_rflags(vcpu, ctxt->eflags);
>>  	kvm_make_request(KVM_REQ_EVENT, vcpu);
>> -- 
>> 1.7.11.2
> 
> Did not double check that emulator.c convertion is complete with your
> patch (which should be done).

I did s/regs/_regs/ for that.  Or did you mean something else?

Regarding emulator interfaces, perhaps we should make all entry points
regular:

  emulator_init(ctxt, ops, vcpu); // just once

  emulator_reset(ctxt)
  while ((ret = emulator_entry_point(ctxt)) == EMULATOR_NEED_DATA)
       get that data from somewhere

emulator_reset() simply resets the state machine, can be as simple as
clearing a bool flag. emulator_entry_point (can be x86_emulate_insn(),
x86_emulate_int(), x86_emulate_task_switch(), etc.) runs until one of
the callbacks tells it to stop to get more data.

(of course the while loop is actually in userspace)


-- 
error compiling committee.c: too many arguments to function