Re: [KVM PATCH] KVM: introduce "xinterface" API for external interaction with guests

[Gregory Haskins <ghaskins@novell.com>]

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

Arnd Bergmann wrote:
> On Thursday 16 July 2009, Gregory Haskins wrote:
>   
>> Arnd Bergmann wrote:  
>>     
>
>   
>>> Your approach allows passing the vmid from a process that does
>>> not own the kvm context. This looks like an intentional feature,
>>> but I can't see what this gains us.
>>>       
>> This work is towards the implementation of lockless-shared-memory
>> subsystems, which includes ring constructs such as virtio-ring,
>> VJ-netchannels, and vbus-ioq.   I find that these designs perform
>> optimally when you allow two distinct contexts (producer + consumer) to
>> process the ring concurrently, which implies a disparate context from
>> the guest in question.  Note that the infrastructure we are discussing
>> does not impose a requirement for the contexts to be unique: it will
>> work equally well from the same or a different process.
>>
>> For an example of this "producer/consumer" dynamic over shared memory in
>> action, please refer to my previous posting re: "vbus"
>>
>> http://lkml.org/lkml/2009/4/21/408
>>
>> I am working on v4 now, and this patch is part of the required support.
>>     
>
> Ok. I can see how your approach gives you more flexibility in this
> regard, but it does not seem critical.
>   

Yeah, and I think I missed your point the first time around.  I thought
you were asking about how the resulting memory API was used, but now I
see you were referring to the scope of the vmid namespace (vs
file-descriptors).

On that topic, yes the vmid implementation here differs from
file-descriptors in the sense that the namespace is global to the
kernel, instead of per-process.  And yes, I also agree with you that
this is not critical to the design, per se.  For instance, the use cases
I have in mind would work fine with the per-task fd namespace as well
since I will be within the same QEMU instance.

So ultimately, I think that means the fd idea you presented would work
equally well. 

>   
>> But to your point, I suppose the dependency lifetime thing is not a huge
>> deal.  I could therefore modify the patch to simply link xinterface.o
>> into kvm.ko and still achieve the primary objective by retaining ops->owner.
>>     
>
> Right. And even if it's a separate module, holding an extra reference
> on kvm.ko will not cause any harm.
>   

Yep, agreed.
>   
>>> Can't you simply provide a function call to lookup the kvm context
>>> pointer from the file descriptor to achieve the same functionality?
>>>   
>>>       
>> You mean so have: struct kvm_xinterface *kvm_xinterface_find(int fd)
>>
>> (instead of creating our own vmid namespace) ?
>>
>> Or are you suggesting using fget() instead of kvm_xinterface_find()?
>>     
>
> I guess they are roughly equivalent. Either you pass a fd to
> kvm_xinterface_find, or pass the struct file pointer you get
> from fget. The latter is probably more convenient because it
> allows you to pass around the struct file in kernel contexts
> that don't have that file descriptor open.
>   

Interesting.  I like it.
>   
>>> To take that thought further, maybe the dependency can be turned
>>> around: If every user (pci-uio, virtio-net, ...) exposes a file
>>> descriptor based interface to user space, you can have a kvm
>>> ioctl to register the object behind that file descriptor with
>>> an existing kvm context to associate it with a guest.
>>>       
>> FWIW: We do that already for the signaling path (see irqfd and ioeventfd
>> in kvm.git).  Each side exposes interfaces that accept eventfds, and the
>> fds are passed around that way.
>>
>> However, for the functions we are talking about now, I don't think it
>> really works well to go the other way.  I could be misunderstanding what
>> you mean, though.  What I mean is that it's KVM that is providing a
>> service to the other modules (in this case, translating memory
>> pointers), so what would an inverse interface look like for that?  And
>> even if you came up with one, it seems to me that its just "6 of one,
>> half-dozen of the other" kind of thing.
>>     
>
> I mean something like 
>
> int kvm_ioctl_register_service(struct file *filp, unsigned long arg)
> {
> 	struct file *service = fget(arg);
> 	struct kvm *kvm = filp->private_data;
>
> 	if (!service->f_ops->new_xinterface_register)
> 		return -EINVAL;
>
> 	return service->f_ops->new_xinterface_register(service, (void*)kvm,
> 				&kvm_xinterface_ops);
> }
>
> This would assume that we define a new file_operation specifically for this,
> which would simplify the code, but there are other ways to achieve the same.
> It would even mean that you don't need any static code as an interface layer.
>   

I suspect I will have a much harder time getting that type of
modification accepted in mainline ;)

I think I will go with your suggestion for the fd/file interface, tho. 
I can get rid of the rbtree logic and re-use the lookup via fget, which
is a nice win.

Thanks Arnd!
-Greg


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