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
