Message-ID: <4A3FB156.3030301@novell.com>
Date:	Mon, 22 Jun 2009 12:29:10 -0400
From:	Gregory Haskins <ghaskins@...ell.com>
To:	"Michael S. Tsirkin" <mst@...hat.com>
CC:	Gregory Haskins <ghaskins@...ell.com>, avi@...hat.com,
	kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
	mtosatti@...hat.com, paulmck@...ux.vnet.ibm.com, markmc@...hat.com
Subject: Re: [PATCH RFC] pass write value to in_range pointers

Michael S. Tsirkin wrote:
> On Mon, Jun 22, 2009 at 11:45:00AM -0400, Gregory Haskins wrote:
>   
>> Michael S. Tsirkin wrote:
>>     
>>> It seems that a lot of complexity and trickiness with iosignalfd is
>>> handling the group/item relationship, which comes about because kvm does
>>> not currently let a device on the bus claim a write transaction based on the
>>> value written.  This could be greatly simplified if the value written
>>> was passed to the in_range check for write operation.  We could then
>>> simply make each kvm_iosignalfd a device on the bus.
>>>
>>> What does everyone think of the following lightly tested patch?
>>>   
>>>       
>> Hi Michael,
>>   Its interesting, but I am not convinced its necessary.  We created the
>> group/item layout because iosignalfds are unique in that they are
>> probably the only IO device that wants to do some kind of address
>> aliasing.
>>     
>
> We actually already have aliasing: is_write flag is used for this
> purpose.

Yes, but read/write address aliasing is not the same thing is
multi-match data aliasing.  Besides, your proposal also breaks some of
the natural relationship models (e.g. all the aliased iosignal_items
always belong to the same underlying device.  io_bus entries have an
arbitrary topology).


>  Actually, it's possible to remove is_write by passing
> a null pointer in write_val for reads. I like this a bit less as
> the code generated is less compact ... Avi, what do you think?
>
>   
>>  With what you are proposing here, you are adding aliasing
>> support to the general infrastructure which I am not (yet) convinced is
>> necessary.
>>     
>
> Infrastructure is a big name for something that adds a total of 10 lines to kvm.
> And it should at least halve the size of your 450-line patch.
>   

Your patch isn't complete until some critical missing features are added
to io_bus, though, so its not really just 10 lines.  For one, it will
need to support much more than 6 devices.  It will also need to support
multiple matches.  Also you are proposing an general interface change
that doesn't make sense to all but one device type.  So now every
io-device developer that comes along will scratch their head at what to
do with that field.

None of these are insurmountable hurdles, but my point is that today the
complexity is encapsulated in the proper place IMO.  E.g. The one and
only device that cares to do this "weird" thing handles it behind an
interface that makes sense to all parties involved.
>   
>> If there isn't a use case for other devices to have
>> aliasing, I would think the logic is best contained in iosignalfd.  Do
>> you have something in mind?
>>     
>
> One is enough :)
>   

I am not convinced yet. ;)  It appears to me that we are leaking
iosignalfd-isms into the general code.  If there is another device that
wants to do something similar, ok.  But I can't think of any.

> Seriously, do you see that this saves you all of RCU, linked lists and
> counters?

Well, also keep in mind we will probably be converting io_bus to RCU
very soon, so we are going the opposite direction ;)

Kind Regards,
-Greg


Download attachment "signature.asc" of type "application/pgp-signature" (267 bytes)

Powered by blists - more mailing lists