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

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 5204 bytes --]

On 2011-01-20 22:40, Blue Swirl wrote:
> On Thu, Jan 20, 2011 at 9:22 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>> On 2011-01-20 20:27, Blue Swirl wrote:
>>> On Thu, Jan 20, 2011 at 9:33 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>> On 2011-01-19 20:32, Blue Swirl wrote:
>>>>> On Wed, Jan 19, 2011 at 4:57 PM, Anthony Liguori
>>>>> <aliguori@linux.vnet.ibm.com> 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.

That already exists.

> 
>> 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.

No, they are practically static host 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.

Yeah, if we had a MachineState or if we could agree on introducing it,
I'm with you again. Improving the currently cumbersome KVM API
interaction was the main motivation for my original patch.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]