Avi Kivity wrote:
> 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.
Any suggestions on a target #? 512?
> 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.
Yeah, I like this idea.
>
>>>> +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.
Right, I understand that part. What I mean specifically is at run-time
when the IO comes in. I was thinking I would need to do a memcmp
against the u64 and the data-register and it was hurting my head trying
to figure out what pointer to pass to memcmp.
<lightbulb turns on>
Duh, I can just load the data-register into a u64 and check equality.
Nevermind, I am a dumbass ;)
> 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.
Oh man! And I was so looking forward to that rewrite.....
>
>>>> +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.
Ack
Thanks Avi,
-Greg
Attachment:
signature.asc
Description: OpenPGP digital signature
