Re: RFC: cache_regs in kvm_emulate_pio

[Avi Kivity <avi@qumranet.com>]

Marcelo Tosatti wrote:
>>
>> ugh, another callback.  how about instead
>>
>> /* in vcpu structure */
>> u16 regs_available;
>> u16 regs_dirty;
>>
>> /* read from cache if possible */
>> if (!test_bit(VCPU_REG_RAX, &regs_available))
>>       ->cache_regs();
>> printk("%d\n", regs[VCPU_REGS_RAX]);
>>
>> /* write to cache, ->vcpu_run() will flush */
>> regs[VCPU_REGS_RAX] = 17;
>> __set_bit(VCPU_REGS_RAX, &regs_dirty);
>>     
>
> I think that hiding whether registers are cached or not behing wrappers
> makes a lot of sense, but having the ->cache_regs interface split can
> also result in gains. An index argument to ->cache_regs() would do the
> trick.
>
>   

Yes and yes.

> For example, there's no need to read GUEST_RSP for
> skip_emulated_instruction, thats another 50+ cycles.
>
> Unless there's something obscure that means you need to read RSP/RIP
> before accessing the now in-memory guest registers saved with "mov"
> in vmx_vcpu_run(). The comment on vcpu_load_rsp_rip seems a little
> ambiguous to me:
>
> /*
>  * Sync the rsp and rip registers into the vcpu structure.  This allows
>  * registers to be accessed by indexing vcpu->arch.regs.
>  */
>
> But I think it just refers to the interface in general, so that nobody
> would try to access RSP or RIP (and RAX in AMD's case) before calling
> ->cache_regs().
>   

It refers to the fact that sometimes you don't know which registers you 
refer to, e.g. in the emulator.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.