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

From: Jan Kiszka <jan.kiszka@web.de>
To: Anthony Liguori <aliguori@linux.vnet.ibm.com>
Cc: Blue Swirl <blauwirbel@gmail.com>,
	Markus Armbruster <armbru@redhat.com>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	Glauber Costa <glommer@redhat.com>,
	Marcelo Tosatti <mtosatti@redhat.com>,
	"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
	Avi Kivity <avi@redhat.com>
Subject: Re: [Qemu-devel] [PATCH 28/35] kvm: x86: Introduce kvmclock device to save/restore its state
Date: Thu, 20 Jan 2011 22:27:59 +0100	[thread overview]
Message-ID: <4D38A8DF.4000200@web.de> (raw)
In-Reply-To: <4D388EE8.3000004@linux.vnet.ibm.com>

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

On 2011-01-20 20:37, Anthony Liguori wrote:
> On 01/20/2011 03:33 AM, Jan Kiszka 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. It may
>> be a good idea to introduce an arch-specific kvm_state and move related
>> bits over. 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.
>>
>> 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.
>>    
> 
> The debate is really:
> 
> 1) should we remove all passing of kvm_state and just assume it's static
> 
> 2) deal with a couple places in the code where we need to figure out how
> to get at kvm_state
> 
> I think we've only identified 1 real instance of (2) and it's resulted
> in some good discussions about how to model KVM devices vs. emulated
> devices.  Honestly, (1) just stinks.  I see absolutely no advantage to
> it at all.   In the very worst case scenario, the thing we need to do is
> just reference an extern variable in a few places.  That completely
> avoids all of the modelling discussions for now (while leaving for
> placeholder FIXMEs so the problem can be tackled later).

The PCI bus discussion is surely an interesting outcome, but now almost
completely off-topic to the original, way less critical issue (as we
were discussing internals).

> 
> I don't understand the resistance here.

IMHO, most suggestions on the table are still over-designed (like a
KVMBus that only passes a kvm_state - or do you have more features for
it in mind?). The idea I love most so far is establishing a machine
state that also carries those few KVM bits which correspond to the KVM
extension of CPUState.

But in the end I want an implementable consensus that helps moving
forward with main topic: the overdue KVM upstream merge. I just do not
have a clear picture yet.

Jan

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