From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH] Support for in-kernel mmio handlers Date: Sun, 08 Apr 2007 10:38:27 +0300 Message-ID: <46189BF3.6010106@qumranet.com> References: <4613C73F.BA47.005A.0@novell.com> <4614A03C.2050707@qumranet.com> <4614C844.BA47.005A.0@novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org To: Gregory Haskins Return-path: In-reply-to: <4614C844.BA47.005A.0-Et1tbQHTxzrQT0dZR+AlfA@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org Errors-To: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org List-Id: kvm.vger.kernel.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