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

[Alexander Graf]

On 07.01.2013, at 19:19, Jason J. Herne wrote:

> On 01/07/2013 10:49 AM, Alexander Graf wrote:
>> 
>> 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.
>> 
> 
> I've started writing the code to replace the kvm_arch_put_register level 
> parameter with a register bitmap and I'm hitting some problems with respect 
> to the Intel/PPC targets:
> 
> 1. target-i386/kvm,c : kvm_arch_put_registers() : This function syncs many 
> registers "all the time".  I'm not entirely convinced from reading the code 
> that I can easily group these registers into the default mutually exclusive 
> groupings (runtime, reset, full).

Runtime, reset and full are special, as they are levels.

#define SYNC_RESET (SYNC_RUNTIME | SYNC_RESET_EXTRA)
#define SYNC_FULL (SYNC_RESET | SYNC_FULL_EXTRA)

But SYNC_RUNTIME and the _EXTRA bits should be individual bits. That way the 
bitmap contains mutual bits, but we still have friendly helper bitmaps to 
convert from the past level based approach.

>  Some of the comments seem to imply an ordering.  Also, a massive data 
> structure is prepared and then a single IOCTL call is used to do the register 
> sync. So I'm not sure if the IOCTL will expect to always see certain 
> registers.  

The ioctl API is pretty well documented in the Linux kernel source in 
Documentation/virt/kvm/api.txt. But the synchronization function itself can 
still be one giant piece of code that simply disassembles the bitmap rather 
than a level, no?

> Things get messier when considering the msr's in kvm_put_msrs().
> 
> 2. Currently no architecture has code to selectively GET register state 
> (hence the initial patch set).  If we do not fully implement this now for all 
> KVM targets then the selective syncing implemented in 
> do_kvm_cpu_synchronize_state() will fail.  Consider the case where we sync 
> the reset group of registers and make some of them dirty.  If a separate task 
> syncs the full state at this point the reset regs will get pulled down and 
> local changes will be lost due to kvm_arch_get_registers.  Sure we could hack 
> around it but we would just be reverting to a "level" based system on top of 
> our bitmap.
> 
> It looks like this is going to be a decent amount of work. I do not believe I 
> have the platform specific knowledge (nor would I have the time) to make the 
> changes required to the x86 and PPC platform specific code.  Perhaps others 
> might volunteer if this is worth the effort :)? Else, perhaps we should 
> examine a simpler solution to the problem?  Any chance the originally 
> submitted patch set would suffice?

I'd like to hear some ideas from Jan first :).


Alex