Re: [PATCH] Support for in-kernel mmio handlers

[Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org>]

Gregory Haskins wrote:
>   
>>> +	int (*in_range)(struct kvm_io_device *this, gpa_t addr);
>>>   
>>>       
>> Do you see any reason to have this as a callback and not a pair of gpas?
>>     
>
> I believe Dor replied earlier stating the reason of being able to support holes.  Another reason that I can think of that I particularly like about this design (which I am not claiming as my own) is that the device can relocate (e.g. LAPIC base addr) without worrying about reprogramming the bus.
>
>   

I don't like either reasons much, but okay.  We can address any 
performance concerns later (I doubt we'll see any with current hardware).


>>> +
>>> +	void             *private;
>>> +	struct list_head  link;
>>>   
>>>       
>> Having these in an array would be much more efficient.  A fixed size 
>> array of moderate size should suffice.
>>     
>
> Done.  Maximum # devices is currently 6, because anything beyond that and I think we need to revisit the linear alg ;)
>
>   


You'll be surprised.  Processors are so efficient at processing arrays 
that you'll need a much longer list before a better algorithm starts to 
gain.

Anyway 6 is as good a number as any.


>> function declarations on one line please.
>>     
>
> Done (though I hate lines that runneth over 80 ;)
>
>   

A newline usually answers :)

  

>> The per- vcpu I/O bus is special in that it has exactly one component, 
>> and one which can change its address.  I think we can special case it 
>> and just check for apic addresses explicitly when searching the bus.
>>     
>
> I am loath to make special cases if they can be avoided.  I think performance wise a kvm_io_bus with one device wont be much different than having a special case check against apicbase.  And the advantage that this buys us is future platforms (e.g. IA64?) may have more than one per-cpu MMIO address.   I also realize that future platforms may be divergent from the entire in-kernel code base altogether, but I think the general and flexible way is better if there are no compromising tradeoffs, even if its only for example/reference.  In this case I dont think there are any tradeoffs, so I left it.  If you insist, I will pull it ;)
>
>   

I think it unlikely that we'll see another local mmio device, it's so 
counter to the spirit of mmio (which is global by its nature).


>>>  
>>> +static struct kvm_io_device* vcpu_find_mmio_dev(struct kvm_vcpu *vcpu, 
>>> +						gpa_t addr)
>>> +{
>>> +	struct kvm_io_device *mmio_dev;
>>> +
>>> +	/* First check the local CPU addresses */
>>> +	mmio_dev = kvm_io_bus_find_dev(&vcpu- >mmio_bus, addr);
>>> +	if(!mmio_dev) {
>>> +		/* Then check the entire VM */
>>> +		mmio_dev = kvm_io_bus_find_dev(&vcpu- >kvm- >mmio_bus, addr);
>>> +	}
>>>   
>>>       
>> space, comment, braces
>>     
>
> I believe I fixed this, but I am a little confused about what you were pointing out.  The space is obvious.  I believe you were pointing out that the braces weren't needed because its technically a single-line, and that the comment is fine.  If I needed to change the comment too, let me know.
>   

/*
 * comment
 */

Do re-read Documentation/CodingStyle.  Coding practices die hard, and 
the kernel is especially sensitive to coding style.  There are some 
instances of nonconforming comments in the updated patches too.

>>>   
>>>       
>> Please fix and *test*.  Boot at least 32- bit Windows with ACPI HAL and 
>> 64- bit Linux, the more the better of course.
>>     
>
>
> I have confirmed that my 64 bit linux guest boots fine.  I don't currently have any other guests.  Careful review of the code leads me to believe this should be an inert change, so I wont go through the effort of finding an XP CD to install unless you insist ;)
>
>   

Please do test.  Even if the changes have no effect, you might expose 
some latent bug.  In any case you'll need Windows to do the apic stuff 
-- it's much more sensitive to apic problems than Linux.



-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV