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

[Avi Kivity]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/