On 2011-01-20 22:40, Blue Swirl wrote: > On Thu, Jan 20, 2011 at 9:22 PM, Jan Kiszka <jan.kiszka@xxxxxx> wrote: >> On 2011-01-20 20:27, Blue Swirl wrote: >>> On Thu, Jan 20, 2011 at 9:33 AM, Jan Kiszka <jan.kiszka@xxxxxxxxxxx> wrote: >>>> On 2011-01-19 20:32, Blue Swirl wrote: >>>>> On Wed, Jan 19, 2011 at 4:57 PM, Anthony Liguori >>>>> <aliguori@xxxxxxxxxxxxxxxxxx> 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:
signature.asc
Description: OpenPGP digital signature
