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

Avi Kivity
2007-04-08T07:38:27Z

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.