Re: [KVM PATCH v7 2/2] KVM: add iosignalfd support

Avi Kivity
2009-06-18T12:21:22Z

On 06/18/2009 03:09 PM, Gregory Haskins wrote:
>>> +config KVM_MAX_IOSIGNALFD_ITEMS
>>> + int "Maximum IOSIGNALFD items per address"
>>> + depends on KVM
>>> + default "32"
>>> + ---help---
>>> + This option influences the maximum number of fd's per PIO/MMIO
>>> + address that are allowed to register
>>> +
>>>
>>>
>> Is there a per-vm limit on iosignalfds? if not, userspace can exhaust
>> kernel memory in that way.
>>
>
> Yeah, its already naturally limited by the maximum number of MMIO/PIO
> devices we can register (today this is 6 per VM). I should have
> documented that fact somewhere, tho.
>

We need to raise this limit drastically and to expose it. I suggest
counting an all iosignalfd_items as part of the iodevice limit, so we
don't have a bunch of little limits which no one understands.

>>> +struct _iosignalfd_item {
>>> + struct list_head list;
>>> + struct file *file;
>>> + unsigned char *match;
>>> + struct rcu_head rcu;
>>> +};
>>>
>>>
>> Why not u64 match?
>>
>
> Well, tbh it was primarily because it was starting to make my head hurt
> w.r.t. endianness ;). For instance, if someone wanted a u16 match, I
> would presumably have to understand the relevant endianess of the u64 so
> I compare the appropriate bytes against the data-register coming in from
> the [MM|P]IO. Using a pointer, I simply copy/memcmp the specified
> number of bytes and never have to worry about endianness.
>

No, a u16 will naturally expand to a u64, and the emulator will generate
the correct value. As long as we don't allow mismatched access sizes,
we should be fine.

> As a minor bonus, item->match == NULL tells me its a wildcard. If I had
> item->match as a u64, I'd need a different state flag for "wildcard".
> NBD, but thought I would point it out.
>

True, a pointer also supplies extra information. But until we get
garbage collection as part of the Java rewrite, resource management is a
pain and I prefer as few pointers as possible.

>>> +static int
>>> +iosignalfd_is_match(struct _iosignalfd_group *group,
>>> + struct _iosignalfd_item *item,
>>> + const void *val,
>>> + int len)
>>> +{
>>> + if (!item->match)
>>> + /* wildcard is a hit */
>>> + return true;
>>> +
>>> + if (len != group->length)
>>> + /* mis-matched length is a miss */
>>> + return false;
>>>
>>>
>> Should check length before match (i.e. require correctly sized access).
>>
>
> Perhaps, but my thinking is that group->length only matters for
> data-matching. You could conceivably have a larger window registered if
> you are using all wildcards. Not sure if this is really useful, but its
> the reason the code is that way today.
>

My thinking is to have the code behave the same way. If you require
matching lengths on data match, require it on wildcard as well.

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