[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup
From: |
Avi Kivity |
Subject: |
Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup |
Date: |
Mon, 23 Aug 2010 16:42:36 +0300 |
User-agent: |
Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.7) Gecko/20100720 Fedora/3.1.1-1.fc13 Lightning/1.0b2pre Thunderbird/3.1.1 |
On 08/23/2010 04:23 PM, Anthony Liguori wrote:
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?
In a sane object model, the expectation is that you can meaningfully
interact with base classes using the interfaces provided by the base
class.
If DeviceState has a GPIO interface, you should be able to use that
interface without knowledge of the subclasses. This implies that all
subclasses implement a GPIO interface and that it can be the primary
interface to interact and connect with devices. Modelling a PCI
device based on a GPIO interface is what I was referring to as insane.
The pci device has num_gpio_out == num_gpio_in == 0, so it all works
(trivially). Again I agree except for the sense of impending doom.
It's silly but not insane - it assumes most devices have >0 gpio pins,
which isn't the case for the devices we care about (so it's only
subjectively silly).
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.
Everything extra hurts when you're trying to move code in to a library
with unit tests covering the functionality :-)
Sure, it's a worthy cleanup. But it's not a reason to go to DEFCON 1.
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.
Where would they put them? Everything a device does has to be stored
in a DeviceState. It may put them in a container of some form if the
timers are dynamic.
Right. In any case, I don't see how passing a DeviceState helps.
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.
The fundamental issue is: every function (minus trivial ones) in the
device models code should have a state reference. That state
reference should inherit from a DeviceState. If this statement isn't
true, then the device has been modelled in qdev incorrectly.
Using this test, quite a lot of the "converted" devices are being
modelled incorrectly.
Is a "state reference" allowed to have a pointer to the state, or reach
it in some other way (for example, static storage for singleton devices)?
Isn't "save/restore works" an equivalent statement to "device state is
reachable from the DeviceState"?
--
error compiling committee.c: too many arguments to function
- Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup, (continued)
- [Qemu-devel] Re: [PATCH v2 0/7] APIC/IOAPIC cleanup, Paolo Bonzini, 2010/08/20
- Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup, Markus Armbruster, 2010/08/20
- Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup, Anthony Liguori, 2010/08/20
- Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup, Avi Kivity, 2010/08/22
- Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup, Anthony Liguori, 2010/08/22
- Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup, Avi Kivity, 2010/08/23
- Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup, Anthony Liguori, 2010/08/23
- Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup,
Avi Kivity <=
- Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup, Anthony Liguori, 2010/08/23
- Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup, Avi Kivity, 2010/08/23
- Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup, Anthony Liguori, 2010/08/23
- Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup, Avi Kivity, 2010/08/23
- Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup, Anthony Liguori, 2010/08/23
- Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup, Markus Armbruster, 2010/08/23
- Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup, Anthony Liguori, 2010/08/23
- Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup, Markus Armbruster, 2010/08/23
- Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup, Anthony Liguori, 2010/08/23
- [Qemu-devel] Re: [PATCH v2 0/7] APIC/IOAPIC cleanup, Jan Kiszka, 2010/08/23