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

[Gregory Haskins <ghaskins@novell.com>]

[-- Attachment #1: Type: text/plain, Size: 3711 bytes --]

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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 266 bytes --]