From: Paolo Bonzini <pbonzini@redhat.com>
To: Blue Swirl <blauwirbel@gmail.com>
Cc: Amit Shah <amit.shah@redhat.com>,
Isaku Yamahata <yamahata@valinux.co.jp>,
qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH 0/7] poison TARGET_xxx for compile once object and header file cleanups
Date: Mon, 28 Jun 2010 18:21:39 +0200 [thread overview]
Message-ID: <4C28CC13.3040304@redhat.com> (raw)
In-Reply-To: <AANLkTinJKJW7_XrEIIh8u29SKLJKD9RxZG78F5r9Uxc4@mail.gmail.com>
>> 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
prev parent reply other threads:[~2010-06-28 16:21 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-06-25 12:52 [Qemu-devel] [PATCH 0/7] poison TARGET_xxx for compile once object and header file cleanups Paolo Bonzini
2010-06-25 12:52 ` [Qemu-devel] [PATCH 1/7] rtc: Remove TARGET_I386 from qemu-config.c, enables driftfix Paolo Bonzini
2010-06-25 12:52 ` [Qemu-devel] [PATCH 2/7] include qemu-common.h when needed by the next patches Paolo Bonzini
2010-06-25 12:52 ` [Qemu-devel] [PATCH 3/7] include stdio.h freely, remove dyngen-exec.h hacks Paolo Bonzini
2010-06-25 12:52 ` [Qemu-devel] [PATCH 4/7] provide opaque CPUState to files that are compiled once Paolo Bonzini
2010-06-27 19:17 ` Blue Swirl
2010-06-28 8:04 ` Paolo Bonzini
2010-06-28 14:21 ` Blue Swirl
2010-06-25 12:52 ` [Qemu-devel] [PATCH 5/7] add qdev property type "cpu" Paolo Bonzini
2010-06-26 7:46 ` Markus Armbruster
2010-06-27 12:48 ` [Qemu-devel] " Paolo Bonzini
2010-06-29 11:42 ` Markus Armbruster
2010-06-29 13:52 ` Paolo Bonzini
2010-06-27 18:45 ` [Qemu-devel] " Blue Swirl
2010-06-25 12:52 ` [Qemu-devel] [PATCH 6/7] replace void* uses with opaque CPUState* Paolo Bonzini
2010-06-26 7:49 ` Markus Armbruster
2010-06-25 12:52 ` [Qemu-devel] [PATCH 7/7] poison TARGET_xxx for compile once object Paolo Bonzini
2010-06-25 22:47 ` [Qemu-devel] [PATCH 0/7] poison TARGET_xxx for compile once object and header file cleanups Richard Henderson
2010-06-27 12:39 ` [Qemu-devel] " Paolo Bonzini
2010-06-27 19:32 ` [Qemu-devel] " Blue Swirl
2010-06-28 8:20 ` Paolo Bonzini
2010-06-28 15:11 ` Blue Swirl
2010-06-28 16:21 ` Paolo Bonzini [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4C28CC13.3040304@redhat.com \
--to=pbonzini@redhat.com \
--cc=amit.shah@redhat.com \
--cc=blauwirbel@gmail.com \
--cc=qemu-devel@nongnu.org \
--cc=yamahata@valinux.co.jp \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).