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 kvm-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/kvm-devel