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
_______________________________________________
kvm-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel
