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

[Jan Kiszka]

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.

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.

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

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

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

signature.asc
Description: OpenPGP digital signature