qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
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

      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).