Re: [Qemu-devel] [PATCH 4/7 v2] KVM regsync: Add register bitmap parameter to do_kvm_cpu_synchronize_state

[Alexander Graf]

On 16.01.2013, at 18:23, Marcelo Tosatti wrote:

> On Wed, Jan 16, 2013 at 05:00:52PM +0000, Bhushan Bharat-R65777 wrote:
>> I think above code should be:
>>            kvm_arch_put_registers(cpu, cpu->kvm_vcpu_dirty);
>>            cpu->kvm_vcpu_dirty = false;
>> 
>> so vcpu will not enter guest state with dirty registers in qemu.
> 
> Not so clear - currently PUT_FULL/PUT_RESET are performed on
> pre-defined points.
> 
>>> Unrelated:
>>> 
>>> 2) Also, what is the reason for specifying sets of registers in 
>>> arch-specific
>>> code? Is that because it allows PPC to fix their sync-timer register 
>>> problem?
>>> 
>>> When you are writing generic code, what does it mean to use
>>> 'KVM_REGSYNC_{RUNTIME,RESET,FULL}_STATE' ?
>>> Answer: it depends on the architecture.
>>> 
>>> 3) On x86, kvm_arch_get_registers(GET_FULL) must not imply
>>> kvm_arch_put_registers(PUT_FULL).
>>> 
>>> The S/390 problem, from
>>> http://lists.nongnu.org/archive/html/qemu-devel/2012-11/msg02213.html:
>>> 
>>> ">>> The kvm register sync needs to happen in the kvm register sync
>>>>>> function :)
>>>>> That would eliminate the whole purpose of sync regs and forces us to
>>>>> have an expensive ioctl on lots of exits (again). I would prefer to
>>>>> sync the registers that we never need in qemu just here.
>>>> 
>>>> That's why the register sync has different stages.
>>> 
>>> Not the get_register. Which is called on every synchronize_state. Which 
>>> happen
>>> quite often on s390."
>>> 
>>> But wait: on these S/390 codepaths, you do GET_REGS already, via
>>> cpu_synchronize_state.
>>> 
>>> So on S/390
>>> 
>>> - cpu_synchronize_state(env)
>>> - read any register from env
>>> 
>>> Is not valid? This is what generic code assumes.
>>> 
>>> 
>>> Bhushan Bharat, the PPC problem, can you describe it clearly: from what i
>>> understood, an in-kernel register cannot be read/written back because that
>>> register value can change in the meantime. When is it necessary to write it
>>> back? (there is a similar problem with TSC on x86, which is "fixed" by only
>>> writing TSC on FULL_STATE arch_put_registers).
>> 
>> There are two things:
>> 
>> First-)
>> For timer related changes on PowerPC, some registers needed to be changed 
>> from QEMU, so we have to get the registers via KVM_GET_SREGS and then set 
>> those registers back to KVM via KVM_SET_SREGS. cpu_synchronize_state() will 
>> get registers but kvm_arch_put_registers() works on level based mechanism 
>> and does not provide a good way of setting a register-set. So we wrote a 
>> separate function that will push these registers back to KVM and this also 
>> uses KVM_SET_SREGS ioctl. This solves what is needed for PPC.
> 
> Can you describe the problem in detail? You must sync a particular
> timer register only on special conditions, not during normal
> cpu_synchronize_state() runs?

We basically have a "core interrupt pending" register. This register can be

  * written from kernel space when a timer expires
  * written from user space on reset
  * written from user space on watchdog expiry

> What register is that and why it cannot be synced normally? When is it
> necessary to sync it?

We need to sync it on the above 2 occasions.

Thinking about this a bit more, we're trying to keep the synchronization window 
short to not get into conflicts with the kernel timer kicking in in between. 
Imagine this race:

  * user space reads TSR
  * kernel timer expires, sets bit in TSR
  * user space writes TSR

That's why we don't want this to be synced every time. We would only set TSR 
when we reset the counter. At that point in time it doesn't hurt to lose the 
kernel timer set, because we cleared the bit anyways.

But maybe the better solution would be a special "write to clear" ONE_REG 
register to clear specific bits and a big hammer "set" ONE_REG (which we have 
already) for reset only.

That would make things easier, right? Scott, any ideas on this?


Alex