Message-ID: <4A3A315A.5090204@redhat.com>
Date:	Thu, 18 Jun 2009 15:21:46 +0300
From:	Avi Kivity <avi@...hat.com>
To:	Gregory Haskins <ghaskins@...ell.com>
CC:	kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
	davidel@...ilserver.org, mtosatti@...hat.com,
	paulmck@...ux.vnet.ibm.com, markmc@...hat.com
Subject: Re: [KVM PATCH v7 2/2] KVM: add iosignalfd support

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists