lkml.org 
[lkml]   [2009]   [Oct]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 2/4] KVM: introduce "xinterface" API for external interaction with guests
On 10/06/2009 01:57 AM, Gregory Haskins wrote:
> Avi Kivity wrote:
>
>> On 10/02/2009 10:19 PM, Gregory Haskins wrote:
>>
>>> What: xinterface is a mechanism that allows kernel modules external to
>>> the kvm.ko proper to interface with a running guest. It accomplishes
>>> this by creating an abstracted interface which does not expose any
>>> private details of the guest or its related KVM structures, and provides
>>> a mechanism to find and bind to this interface at run-time.
>>>
>>>
>> If this is needed, it should be done as a virt_address_space to which
>> kvm and other modules bind, instead of as something that kvm exports and
>> other modules import. The virt_address_space can be identified by an fd
>> and passed around to kvm and other modules.
>>
> IIUC, what you are proposing is something similar to generalizing the
> vbus::memctx object. I had considered doing something like that in the
> early design phase of vbus, but then decided it would be a hard-sell to
> the mm crowd, and difficult to generalize.
>
> What do you propose as the interface to program the object?
>

Something like the current kvm interfaces, de-warted. It will be a hard
sell indeed, for good reasons.

>> So, under my suggestion above, you'd call
>> sys_create_virt_address_space(), populate it, and pass the result to kvm
>> and to foo. This allows the use of virt_address_space without kvm and
>> doesn't require foo to interact with kvm.
>>
> The problem I see here is that the only way I can think to implement
> this generally is something that looks very kvm-esque (slots-to-pages
> kind of translation). Is there a way you can think of that does not
> involve a kvm.ko originated vtable that is also not kvm centric?
>

slots would be one implementation, if you can think of others then you'd
add them.

If you can't, I think it indicates that the whole thing isn't necessary
and we're better off with slots and virtual memory. The only thing
missing is dma, which you don't deal with anyway.

>>> +struct kvm_xinterface_ops {
>>> + unsigned long (*copy_to)(struct kvm_xinterface *intf,
>>> + unsigned long gpa, const void *src,
>>> + unsigned long len);
>>> + unsigned long (*copy_from)(struct kvm_xinterface *intf, void *dst,
>>> + unsigned long gpa, unsigned long len);
>>> + struct kvm_xvmap* (*vmap)(struct kvm_xinterface *intf,
>>> + unsigned long gpa,
>>> + unsigned long len);
>>>
>>>
>> How would vmap() work with live migration?
>>
> vmap represents shmem regions, and is a per-guest-instance resource. So
> my plan there is that the new and old guest instance would each have the
> vmap region instated at the same GPA location (assumption: gpas are
> stable across migration), and any state relevant data local to the shmem
> (like ring head/tail position) is conveyed in the serialized stream for
> the device model.
>

You'd have to copy the entire range since you don't know what the guest
might put there. I guess it's acceptable for small areas.

>>> +
>>> +static inline void
>>> +_kvm_xinterface_release(struct kref *kref)
>>> +{
>>> + struct kvm_xinterface *intf;
>>> + struct module *owner;
>>> +
>>> + intf = container_of(kref, struct kvm_xinterface, kref);
>>> +
>>> + owner = intf->owner;
>>> + rmb();
>>>
>>>
>> Why rmb?
>>
> the intf->ops->release() line may invalidate the intf pointer, so we
> want to ensure that the read completes before the release() is called.
>
> TBH: I'm not 100% its needed, but I was being conservative.
>

rmb()s are only needed if an external agent can issue writes, otherwise
you'd need one after every statement.




>>
>> A simple per-vcpu cache (in struct kvm_vcpu) is likely to give better
>> results.
>>
> per-vcpu will not work well here, unfortunately, since this is an
> external interface mechanism. The callers will generally be from a
> kthread or some other non-vcpu related context. Even if we could figure
> out a vcpu to use as a basis, we would require some kind of
> heavier-weight synchronization which would not be as desirable.
>
> Therefore, I opted to go per-cpu and use the presumably lighterweight
> get_cpu/put_cpu() instead.
>

This just assumes a low context switch rate.

How about a gfn_to_pfn_cached(..., struct gfn_to_pfn_cache *cache)?
Each user can place it in a natural place.

>>> +static unsigned long
>>> +xinterface_copy_to(struct kvm_xinterface *intf, unsigned long gpa,
>>> + const void *src, unsigned long n)
>>> +{
>>> + struct _xinterface *_intf = to_intf(intf);
>>> + unsigned long dst;
>>> + bool kthread = !current->mm;
>>> +
>>> + down_read(&_intf->kvm->slots_lock);
>>> +
>>> + dst = gpa_to_hva(_intf, gpa);
>>> + if (!dst)
>>> + goto out;
>>> +
>>> + if (kthread)
>>> + use_mm(_intf->mm);
>>> +
>>> + if (kthread || _intf->mm == current->mm)
>>> + n = copy_to_user((void *)dst, src, n);
>>> + else
>>> + n = _slow_copy_to_user(_intf, dst, src, n);
>>>
>>>
>> Can't you switch the mm temporarily instead of this?
>>
> Thats actually what I do for the fast-path (use_mm() does a switch_to()
> internally).
>
> The slow-path is only there for completeness for when switching is not
> possible (such as if called with an mm already active i.e.
> process-context).

Still, why can't you switch temporarily?

> In practice, however, this doesnt happen. Virtually
> 100% of the calls in vbus hit the fast-path here, and I suspect most
> xinterface clients would find the same conditions as well.
>

So you have 100% untested code here.

--
error compiling committee.c: too many arguments to function



\
 
 \ /
  Last update: 2009-10-06 11:37    [W:0.236 / U:0.044 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site