From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=56617 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OTH5i-0000UW-7C for qemu-devel@nongnu.org; Mon, 28 Jun 2010 12:21:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OTH5c-00025r-Uq for qemu-devel@nongnu.org; Mon, 28 Jun 2010 12:21:50 -0400 Received: from mx1.redhat.com ([209.132.183.28]:24639) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OTH5c-00025j-JF for qemu-devel@nongnu.org; Mon, 28 Jun 2010 12:21:44 -0400 Message-ID: <4C28CC13.3040304@redhat.com> Date: Mon, 28 Jun 2010 18:21:39 +0200 From: Paolo Bonzini MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 0/7] poison TARGET_xxx for compile once object and header file cleanups References: <1277470342-5861-1-git-send-email-pbonzini@redhat.com> <4C285B42.1030309@redhat.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Blue Swirl Cc: Amit Shah , Isaku Yamahata , qemu-devel@nongnu.org >> I don't see it at all as a temporary measure. In theory, only devices >> compiled per-target would need access to CPUState, that's true. > > There is no need for any device to have access to CPUState fields. That's why this patch defines CPUState as opaque (i.e. incomplete). Granted, an opaque CPUState plus a bunch of accessors is not any better than a fully-visible CPUState, if the accessors are very much tied to that particular CPUState type. (But then, we also agree it is also a tiny little bit better than void*). >> First, converting devices such as APIC to qdev requires knowledge of >> CPUState in qdev, unless you want to keep DEFINE_PROP_PTR (whose removal is >> much more interesting) or sweep it under the void* blanket. PICs are likely >> to have CPUState members, e.g. hw/pxa2xx_pic.c (and BTW indirect access via >> functions to these fields is making emulation a little bit slower). > > One way to clean up PICs would be to use qemu_irq to signal CPU > interrupts, but there are probably others. In the end the devices will be wired to a particular CPU and these "wires" will likely be more complex than a 1-bit IRQ, so qemu_irq only makes limited sense as it is now. What you have to pass may be simply a 32-bit value as in hw/etraxfs_pic, or a more complicated payload as in apic_bus_deliver, but anyway at some point you'll be coupling either: - the device to the CPU, as it happens now. As above, the CPU may be accessed via void* and functions, or via fields directly, but in any case the coupling is there. - the CPU to the device. If you used qemu_irq that would be the most likely outcome: the CPU knows about some DeviceState, and it downcasts it to an expected device type in the IRQ handler. Again, you only have an illusion of decoupling and, given the lengthy discussion about IRQ payloads on qemu-devel only a few weeks ago, I'm not optimist it's going to go away. >> Also, for things compiled in libhw that are not part of device code, >> requiring knowledge of CPUState is absolutely not problematic and the only >> alternative loses type-safety and so it is inferior. > > Completely untrue. Devices (whether part of libhw etc. or not) have no > need (from architectural point of view) to access CPUState contents. > It's clearly problematic. It's also possible to get type safety > without CPUState references in the devices. I explicitly said "things compiled in libhw that are not part of device code", most of which you added. Anyway, thanks for the review and the (partial) ack. I'll rework the patch series and resend. Paolo