Re: [Qemu-devel] [PATCH 7/7] KVM regsync: Fix do_kvm_cpu_synchronize_state data integrity issue

[Alexander Graf]

On 07.01.2013, at 16:43, Jason J. Herne wrote:

> On 01/04/2013 11:27 AM, Alexander Graf wrote:
>> 
>> On 04.01.2013, at 16:25, Jason J. Herne wrote:
>> 
>>> If I've followed the conversation correctly this is what needs to be done:
>>> 
>>> 1. Remove the level parameters from kvm_arch_get_registers and 
>>> kvm_arch_put_registers.
>>> 
>>> 2. Add a new bitmap parameter to kvm_arch_get_registers and 
>>> kvm_arch_put_registers.
>> 
>> I would combine these into "replace levels with bitmap".
>> 
>>> 3. Define a bit that correlates to our current notion of "all runtime 
>>> registers".  This bit, and all bits in this bitmap, would be architecture 
>>> specific.
>> 
>> Why would that bit be architecture specific? "All runtime registers" == 
>> "registers that gdb can access" IIRC. The implementation on what exactly 
>> that means obviously is architecture specific, but the bit itself would not 
>> be, as the gdbstub wants to be able to synchronize in arch independent code.
>> 
> 
> How do we want to define these bits?  is it logical to break up the registers 
> into smaller categories and then use masks to create RUNTIME_STATE, 
> FULL_STATE, RESET_STATE?  If so, how should we define them?  Would they be 
> arch specific and then we'd create the _STATE masks for each architecture?

I see. So you only want to make the name arch independent, but keep its actual 
backing bits arch specific. I can see how that'd end up being a useful thing to 
do, yes.

So we could have archs that just define RUNTIME_STATE as ARCH_RUNTIME_STATE and 
others that define it as ARCH_STATE_REGx | ARCH_STATE_REGy. That way other code 
may only synchronize less than the full runtime state. Works for me :).

> If we do simply define a bit for each of the above three states instead, they 
> should probably be 100% mutually exclusive to provide the best protection 
> against complicated data synchronization issues (like the original 7/7 patch 
> was trying to prevent).  Also, if we can assume 100% mutual exclusion the 
> sync logic becomes trivial:
> 
> static void do_kvm_cpu_synchronize_state(void *arg)
> {
>    struct kvm_cpu_syncstate_args *args = arg;
> 
>    /* Do not sync regs that are already dirty */
>    int regs_to_get = args->regmap & ~cpu->kvm_vcpu_dirty;
> 
>    kvm_arch_get_registers(args->cpu, regs_to_get);
>    args->cpu->kvm_vcpu_dirty |= regs_to_get;
> }
> 
> Thoughts?

I like the idea of making the bits 100% mutually exclusive.


Alex