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

[Jan Kiszka]

On 2011-01-24 22:35, Blue Swirl wrote:
> On Mon, Jan 24, 2011 at 2:08 PM, Jan Kiszka <address@hidden> wrote:
>> On 2011-01-21 19:49, Blue Swirl wrote:
>>>>> I'd add fourth possible class:
>>>>>  - device, CPU and machine configuration, like nographic,
>>>>> win2k_install_hack, no_hpet, smp_cpus etc. Maybe also
>>>>> irqchip_in_kernel could fit here, though it obviously depends on a
>>>>> host capability too.
>>>>
>>>> I would count everything that cannot be assigned to a concrete device
>>>> upfront to the dynamic state of a machine, thus class 2. The point is,
>>>> (potentially) every device of that machine requires access to it, just
>>>> like (indirectly, via the KVM core services) to some KVM VM state bits.
>>>
>>> The machine class should not be a catch-all, it would be like
>>> QEMUState or KVMState then. Perhaps each field or variable should be
>>> listed and given more thought.
>>
>> Let's start with what is most urgent:
>>
>>  - vmfd: file descriptor required for any KVM request that has VM scope
>>   (in-kernel device creation, device state synchronizations, IRQ
>>   routing etc.)
> 
> I'd say VM state.

Good. That's +1 for introducing and distributing it.

> 
>>  - irqchip_in_kernel: VM uses in-kernel irqchip acceleration
>>   (some devices will have to adjust their behavior depending on this)
> 
> Since QEMU version is useless, I peeked at qemu-kvm version.
> 
> There are a lot of lines like:
> if (kvm_enabled() && !kvm_irqchip_in_kernel())
>     kvm_just_do_it();
> 
> Perhaps these would be cleaner with stub functions.

Probably. I guess there is quite some room left for cleanups in this area.

> 
> The device cases are obvious: the devices need a flag, passed to them
> by pc.c, which combines kvm_enabled && kvm_irqchip_in_kernel(). This
> gets stored in device state.

Not all devices are only instantiated by the machine init code. Even if
we are lucky that all those we need on x86 are created that way, we
shouldn't rely on this for future use case, including other KVM archs.

> 
> But exec.c case, where kvm_update_interrupt_request() is called, is
> more interesting. CPU init could set up function pointer to either
> stub/NULL or kvm_update_interrupt_request().
> 

Yes, callbacks are the way to go long term. Here we could also define
one for VCPU interrupt handling and set it according to the VCPU mode.

> I didn't look at kvm*.c, qemu-kvm*.c or stuff in kvm/.
> 
> So I'd eliminate kvm_irqchip_in_kernel() from outside of KVM and pc.c.
> The information could be stored in a MachineState, where pc.c could
> grab it for device and CPU setup.

I still don't see how we can distribute the information to all
interested devices. It's basically the same issue as with current kvm_state.

Jan

signature.asc
Description: OpenPGP digital signature