From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=42247 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OnPrW-0001Re-96 for qemu-devel@nongnu.org; Mon, 23 Aug 2010 01:46:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OnPrU-0002FO-U8 for qemu-devel@nongnu.org; Mon, 23 Aug 2010 01:46:25 -0400 Received: from mx1.redhat.com ([209.132.183.28]:31006) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OnPrU-0002FH-MX for qemu-devel@nongnu.org; Mon, 23 Aug 2010 01:46:24 -0400 Message-ID: <4C720B1F.3030206@redhat.com> Date: Mon, 23 Aug 2010 08:46:07 +0300 From: Avi Kivity MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup References: <4C6D86F9.3010602@codemonkey.ws> <4C6D98E7.9020109@codemonkey.ws> <4C6DA75D.40303@codemonkey.ws> <4C6ECBB7.7060608@codemonkey.ws> <4C718865.7010807@redhat.com> <4C719080.4030202@codemonkey.ws> In-Reply-To: <4C719080.4030202@codemonkey.ws> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: Blue Swirl , "Liu >> \"Liu, Jinsong\"" , qemu-devel , Markus Armbruster , Paul Brook On 08/23/2010 12:02 AM, Anthony Liguori wrote: > On 08/22/2010 03:28 PM, Avi Kivity wrote: >> On 08/20/2010 09:38 PM, Anthony Liguori wrote: >>>> While that might be useful, I don't quite see what makes CPUs so >>>> special >>>> that they need to be kept out of qdev. Could be just my ignorance, of >>>> course. >>> >>> CPUs have special relationships with things like memory in QEMU. >>> You can argue that a device is anything with pins and that CPUs are >>> just like any other chip. >> >> We're not modelling chips! If we declare something a device, we do >> it because it's functionally a device. It could be part of a chip, >> or spread along multiple chips. > > This is really a fundamental discussion. If you look closely at qdev > in it's current form, what it actually models is a device with GPIO > input and output whereas the GPIO input and output correspond to > qemu_irqs which really model pins that can be raised and lowered. > > To me, this is insane and I'm looking to move the GPIO stuff out of > qdev. There are some devices where it makes sense to model the > interactions between pins but not for the vast majority of devices. I agree, but I don't see the burning need or why it's "insane". Seems like a minor design issue, can't you just ignore GPIO when you don't need it? GPIO is just one way for a device to talk, same as (*bus)_phys_memory_rw() or its netdev or its chardev or its timers. It doesn't need to have special status within DeviceState, but it doesn't hurt so much that I can tell. >>> All device callbacks should be based on DeviceState * pointers which >>> means if we want to share device code between multiple interfaces >>> (be it ISA, PCI, or a SysBus device), we need to have a bus in between. >> >> How can you do that? Do you mean that a timer calls >> DeviceState::ops->timer(DeviceState *)? How do you handle multiple >> timers then? > > > No. We have two types of timers today. vm_clock based timers and > rt_clock based timers. It's always a bug for a device model to use an > rt_clock based timer. We ought to have a separate API for vm_clock > based timers and it makes sense to tie that API to DeviceState. For > instance: > > typedef struct Timer Timer; > > void timer_init(DeviceState *, void (*fn)(Timer *)); > void timer_update_rel_ns(Timer *); > void timer_cancel(Timer *); > void timer_release(Timer *); > > Timer objects get embedded into the device's state and container_of > can be used to get to the original device state. We could also pass > DeviceState. It's not clear to me which is better. Not embedding the DeviceState is more generic. For example, a device with a variable number of timers wouldn't be able to embed them in DeviceState. > But being able to associate timers with devices seems like a very good > idea to me because it means that you can see which devices are > registering timers. You might also have the timers auto-cancelled and auto-destroyed on device removal. But the whole thing seems like a minor coding issue rather than something fundamental. -- I have a truly marvellous patch that fixes the bug which this signature is too narrow to contain.