Re: [Qemu-devel] [PATCH 28/35] kvm: x86: Introduce kvmclock device to save/restore its state

[Blue Swirl]

On Thu, Jan 20, 2011 at 9:22 PM, Jan Kiszka <address@hidden> wrote:
> On 2011-01-20 20:27, Blue Swirl wrote:
>> On Thu, Jan 20, 2011 at 9:33 AM, Jan Kiszka <address@hidden> wrote:
>>> On 2011-01-19 20:32, Blue Swirl wrote:
>>>> On Wed, Jan 19, 2011 at 4:57 PM, Anthony Liguori
>>>> <address@hidden> wrote:
>>>>> On 01/19/2011 07:15 AM, Markus Armbruster wrote:
>>>>>>
>>>>>> So they interact with KVM (need kvm_state), and they interact with the
>>>>>> emulated PCI bus.  Could you elaborate on the fundamental difference
>>>>>> between the two interactions that makes you choose the (hypothetical)
>>>>>> KVM bus over the PCI bus as device parent?
>>>>>>
>>>>>
>>>>> It's almost arbitrary, but I would say it's the direction that I/Os flow.
>>>>>
>>>>> But if the underlying observation is that the device tree is not really a
>>>>> tree, you're 100% correct.  This is part of why a factory interface that
>>>>> just takes a parent bus is too simplistic.
>>>>>
>>>>> I think we ought to introduce a -pci-device option that is specifically 
>>>>> for
>>>>> creating PCI devices that doesn't require a parent bus argument but 
>>>>> provides
>>>>> a way to specify stable addressing (for instancing, using a linear index).
>>>>
>>>> I think kvm_state should not be a property of any device or bus. It
>>>> should be split to more logical pieces.
>>>>
>>>> Some parts of it could remain in CPUState, because they are associated
>>>> with a VCPU.
>>>>
>>>> Also, for example irqfd could be considered to be similar object to
>>>> char or block devices provided by QEMU to devices. Would it make sense
>>>> to introduce new host types for passing parts of kvm_state to devices?
>>>>
>>>> I'd also make coalesced MMIO stuff part of memory object. We are not
>>>> passing any state references when using cpu_physical_memory_rw(), but
>>>> that could be changed.
>>>
>>> There are currently no VCPU-specific bits remaining in kvm_state.
>>
>> I think fields vcpu_events, robust_singlestep, debugregs,
>> kvm_sw_breakpoints, xsave, xcrs belong to CPUX86State. They may be the
>> same for all VCPUs but still they are sort of CPU properties. I'm not
>> sure about fd field.
>
> They are all properties of the currently loaded KVM subsystem in the
> host kernel. They can't change while KVM's root fd is opened.
> Replicating this static information into each and every VCPU state would
> be crazy.

Then each CPUX86State could have a pointer to common structure.

> In fact, services like kvm_has_vcpu_events() already encode this: they
> are static functions without any kvm_state reference that simply return
> the content of those fields. Totally inconsistent to this, we force the
> caller of kvm_check_extension to pass a handle. This is part of my
> problem with the current situation and any halfhearted steps in this
> context. Either we work towards eliminating "static KVMState *kvm_state"
> in kvm-all.c or eliminating KVMState.

If the CPU related fields are accessible through CPUState, the handle
should be available.

>>> It may
>>> be a good idea to introduce an arch-specific kvm_state and move related
>>> bits over.
>>
>> This should probably contain only irqchip_in_kernel, pit_in_kernel and
>> many_ioeventfds, maybe fd.
>
> fd is that root file descriptor you need for a few KVM services that are
> not bound to a specific VM - e.g. feature queries. It's not arch
> specific. Arch specific are e.g. robust_singlestep or xsave feature states.

By arch you mean guest CPU architecture? They are not machine features.

>>
>>> It may also once be feasible to carve out memory management
>>> related fields if we have proper abstractions for that, but I'm not
>>> completely sure here.
>>
>> I'd put slots, vmfd, coalesced_mmio, broken_set_mem_region,
>> migration_log into the memory object.
>
> vmfd is the VM-scope file descriptor you need at machine-level. The rest
> logically belongs to a memory object, but I haven't looked at technical
> details yet.
>
>>
>>> Anyway, all these things are secondary. The primary topic here is how to
>>> deal with kvm_state and its fields that have VM-global scope.
>>
>> If it is an opaque blob which contains various unrelated stuff, no
>> clear place will be found.
>
> We aren't moving fields yet (and we shouldn't). We first of all need to
> establish the handle distribution (which apparently requires quite some
> work in areas beyond KVM).

But I think this is exactly  the problem. If the handle is for the
current KVMState, you'll indeed need it in various places and passing
it around will be cumbersome. By moving the fields around, the
information should  be available more naturally.

>> By the way, we don't have a QEMUState but instead use globals. Perhaps
>> this should be reorganized as well. For fd field, maybe even using a
>> global variable could be justified, since it is used for direct access
>> to kernel, not unlike a system call.
>
> The fd field is part of this discussion. Making it global (but local to
> the kvm subsystem) was an implicit part of my original suggestion.
>
> I've no problem with something like a QEMUState, or better a
> MachineState that would also include a few KVM-specific fields like the
> vmfd - just like we already do for CPUstate (or should we better
> introduce a KVM CPU bus... ;) ).
>
> Jan
>
>