* Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup
2010-08-19 21:51 ` Anthony Liguori
@ 2010-08-19 22:52 ` malc
2010-08-20 1:01 ` Anthony Liguori
2010-08-20 8:42 ` [Qemu-devel] " Paolo Bonzini
` (2 subsequent siblings)
3 siblings, 1 reply; 54+ messages in thread
From: malc @ 2010-08-19 22:52 UTC (permalink / raw)
To: Anthony Liguori
Cc: Blue Swirl, Liu >> "Liu, Jinsong",
Paul Brook, Avi Kivity, qemu-devel
On Thu, 19 Aug 2010, Anthony Liguori wrote:
> On 08/19/2010 04:21 PM, Blue Swirl wrote:
> > > Should CPUs appear in the QEMU device tree?
> > >
> > They have several properties that should be user visible.
> >
>
> Sure, but that's an argument for having some of the qdev features (like
> variant properties) be consumable outside of qdev.
>
[..snip..]
>
> Yes, but the programming model was different.
>
> Look at the PIC compared to the lapic. The PIC is programmed via pio at a
> fixed location. There is only one PIC and it interacts with the system just
> like all other devices. IOW, there is no reference to CPUState.
There are two PICs actually there's a cascade..
>
> When you look at the local APIC (apic.c) however, you see that it's the only
> device in the tree that actually interacts with a CPUState. The notion of
> cpu_get_current_apic() is a good example of the tricks needed to make this
> work.
>
> The local APIC is a cpu feature, not a device.
>
> Regards,
>
> Anthony Liguori
>
--
mailto:av1474@comtv.ru
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup
2010-08-19 22:52 ` malc
@ 2010-08-20 1:01 ` Anthony Liguori
2010-08-20 10:00 ` malc
0 siblings, 1 reply; 54+ messages in thread
From: Anthony Liguori @ 2010-08-20 1:01 UTC (permalink / raw)
To: malc
Cc: Blue Swirl, Liu >> "Liu, Jinsong",
Paul Brook, Avi Kivity, qemu-devel
On 08/19/2010 05:52 PM, malc wrote:
>> Yes, but the programming model was different.
>>
>> Look at the PIC compared to the lapic. The PIC is programmed via pio at a
>> fixed location. There is only one PIC and it interacts with the system just
>> like all other devices. IOW, there is no reference to CPUState.
>>
> There are two PICs actually there's a cascade..
>
Technically speaking, originally there was just one and then more IRQs
were supported by cascading and reserving an IRQ line for supporting
cascading. I think you can technical cascade with any IRQ and have more
than one slave but the historic PC architecture only had one slave AFAIK.
Regards,
Anthon Liguori
>> When you look at the local APIC (apic.c) however, you see that it's the only
>> device in the tree that actually interacts with a CPUState. The notion of
>> cpu_get_current_apic() is a good example of the tricks needed to make this
>> work.
>>
>> The local APIC is a cpu feature, not a device.
>>
>> Regards,
>>
>> Anthony Liguori
>>
>>
>
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup
2010-08-20 1:01 ` Anthony Liguori
@ 2010-08-20 10:00 ` malc
0 siblings, 0 replies; 54+ messages in thread
From: malc @ 2010-08-20 10:00 UTC (permalink / raw)
To: Anthony Liguori
Cc: Blue Swirl, Liu >> "Liu, Jinsong",
Paul Brook, Avi Kivity, qemu-devel
On Thu, 19 Aug 2010, Anthony Liguori wrote:
> On 08/19/2010 05:52 PM, malc wrote:
> > > Yes, but the programming model was different.
> > >
> > > Look at the PIC compared to the lapic. The PIC is programmed via pio at a
> > > fixed location. There is only one PIC and it interacts with the system
> > > just
> > > like all other devices. IOW, there is no reference to CPUState.
> > >
> > There are two PICs actually there's a cascade..
> >
>
> Technically speaking, originally there was just one and then more IRQs were
> supported by cascading and reserving an IRQ line for supporting cascading. I
> think you can technical cascade with any IRQ and have more than one slave but
> the historic PC architecture only had one slave AFAIK.
One master + one slave = 2 PICs, i guess you've meant that originally
there was no slave.
[..snip..]
--
mailto:av1474@comtv.ru
^ permalink raw reply [flat|nested] 54+ messages in thread
* [Qemu-devel] Re: [PATCH v2 0/7] APIC/IOAPIC cleanup
2010-08-19 21:51 ` Anthony Liguori
2010-08-19 22:52 ` malc
@ 2010-08-20 8:42 ` Paolo Bonzini
2010-08-20 17:01 ` [Qemu-devel] " Markus Armbruster
2010-08-20 19:26 ` Blue Swirl
3 siblings, 0 replies; 54+ messages in thread
From: Paolo Bonzini @ 2010-08-20 8:42 UTC (permalink / raw)
To: Anthony Liguori
Cc: Blue Swirl, Liu, Jinsong, Paul Brook, Avi Kivity, qemu-devel
On 08/19/2010 11:51 PM, Anthony Liguori wrote:
>>> Neither approach maps well to real hardware. An x86 CPU cannot exist
>>> without a local APIC and a local APIC cannot exist without an x86
>>> CPU. The
>>> two are fundamentally tied together.
>>
>> What about 486? Or 82489?
>
> Don't confuse the local APIC with the PIC or the I/O APIC.
>
> The local APIC has always existed in the CPU core. There is also an I/O
> APIC which exists outside of the CPU core. The local APIC was introduced
> with SMP support.
In theory it's possible to have a discrete local APIC on the 486 or the
Pentium. See figure 5-1 in the Intel multiprocessor specification.
> When you look at the local APIC (apic.c) however, you see that it's the
> only device in the tree that actually interacts with a CPUState.
Even worse, it does it through an opaque pointer even though the only
CPUState that makes sense there is the i386 CPUState. (BTW, there is
exactly one other example of a CPUState property, and it's in the CRIS PIC).
Paolo
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup
2010-08-19 21:51 ` Anthony Liguori
2010-08-19 22:52 ` malc
2010-08-20 8:42 ` [Qemu-devel] " Paolo Bonzini
@ 2010-08-20 17:01 ` Markus Armbruster
2010-08-20 18:38 ` Anthony Liguori
2010-08-20 19:26 ` Blue Swirl
3 siblings, 1 reply; 54+ messages in thread
From: Markus Armbruster @ 2010-08-20 17:01 UTC (permalink / raw)
To: Anthony Liguori
Cc: Blue Swirl, Liu >> "Liu, Jinsong",
Paul Brook, Avi Kivity, qemu-devel
Anthony Liguori <anthony@codemonkey.ws> writes:
> On 08/19/2010 04:21 PM, Blue Swirl wrote:
>>> Should CPUs appear in the QEMU device tree?
>>>
>> They have several properties that should be user visible.
>>
>
> Sure, but that's an argument for having some of the qdev features
> (like variant properties) be consumable outside of qdev.
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.
>>> requirement to sit on a bus. A UART16650A does not sit on bus. It sits on
>>> a card and is wired to the ISA bus or is sometimes wired directly to pins on
>>> a CPU on a SoC.
>>>
>> Or all buses should be unified, so all devices could be wired into any board.
>
> But that defeats the notion of having bus level interfaces. You
> really want the virtio PCI layer to only use PCI functions and
> specifically to interact with PCI concepts that don't exist in other
> busses (like IO regions and config space). However, you also want the
> ability to interact with a virtio device through a well defined
> interface that isn't PCI specific.
>
> The only way to do this right now is having a bus with a single
> device. I've been working on the serial device, and what this
> requires is having an ISASerialDevice which is-a ISADevice. The
> ISASerialDevice has-a
> SerialBus and the SerialBus contain a single SerialDevice which is-a
> DeviceState.
It needs to be a SerialBusDevice, not just a DeviceState, actually.
> But the ISASerialDevice has to assume that the SerialBus contains only
> a single child because it needs to connect the GPIO out from the
> SerialDevice to the ISA irq that's assigned.
>
> It would be a whole lot simpler to break the "all devices sit on
> busses" assumption that we have today and simply have the
> ISASerialDevice has-a
> SerialDevice with no additional layers of bus indirection.
What's the *practical* benefit of splitting up ISASerialDevice into
UART-ISA adapter and the UART proper?
The "bus" sitting between the two would be trivial. If we do that
everywhere, we'll end up with many different, trivial buses.
>> IIRC I tried also that approach when I worked on this patch set but
>> there were some problems. Maybe with header file conflicts, or
>> qemu_irq was not available to CPU code.
>>
>
> Okay, I'll queue it up for a future day.
>
>>> Because not all devices on the sysbus can be hot added so if you made the
>>> bus hotpluggable, it would basically defeat the point of even marking a bus
>>> as not supporting hot plug.
>>>
>>> IOW, the whole bus is either hot pluggable or not. You cannot just say that
>>> device X can be hotplugged but nothing else.
>>>
>> Perhaps the hotplugging system in QEMU needs some rethinking instead.
>> Many real devices are not hot pluggable.
>>
>
> That's probably fair. I don't think hot plugging should exist at the
> qdev level. Hot plugging is a very bus specific concept with very bus
> specific restrictions. For instance, with PCI, you can only plug at
> slot granularities whereas today, we don't really model multi-function
> devices as anything more than a set of unrelated devices. So there's
> really no way you could conceptually hot plug a multi-function virtio
> adapter although you can pretty trivially create one statically.
I think it's fair to say that qdev doesn't currently get hot-plugging
right.
>>>>> I think the options are to allow non-bus devices (like the APIC) or make
>>>>> the
>>>>> APIC a special case that's part of the CPU emulation.
>>>>>
>>>>>
>>>> No. There could also be a new hotpluggable bus type, CPUBus, one
>>>> instance between each CPU and APIC. Or SysBusWithHotPlug. But I don't
>>>> see how that would be different from SysBus.
>>>>
>>>>
>>> Neither approach maps well to real hardware. An x86 CPU cannot exist
>>> without a local APIC and a local APIC cannot exist without an x86 CPU. The
>>> two are fundamentally tied together.
That's a pretty convincing argument for why a LAPIC should not be
separated from its CPU in qdev. It should be part of the CPU qdev, or
maybe a child of it.
>> What about 486? Or 82489?
>>
>
> Don't confuse the local APIC with the PIC or the I/O APIC.
>
> The local APIC has always existed in the CPU core. There is also an
> I/O APIC which exists outside of the CPU core. The local APIC was
> introduced with SMP support.
>
> The PIC and IO APIC totally make sense to be modelled as qdev.
>
>>> It's like modelling a TLB as a
>>> separate device.
>>>
>> That has surely happened in ancient times.
>>
>
> Yes, but the programming model was different.
>
> Look at the PIC compared to the lapic. The PIC is programmed via pio
> at a fixed location. There is only one PIC and it interacts with the
> system just like all other devices. IOW, there is no reference to
> CPUState.
>
> When you look at the local APIC (apic.c) however, you see that it's
> the only device in the tree that actually interacts with a CPUState.
> The notion of cpu_get_current_apic() is a good example of the tricks
> needed to make this work.
>
> The local APIC is a cpu feature, not a device.
A bit like the UART is a ISASerialDevice feature?
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup
2010-08-20 17:01 ` [Qemu-devel] " Markus Armbruster
@ 2010-08-20 18:38 ` Anthony Liguori
2010-08-22 20:28 ` Avi Kivity
0 siblings, 1 reply; 54+ messages in thread
From: Anthony Liguori @ 2010-08-20 18:38 UTC (permalink / raw)
To: Markus Armbruster
Cc: Blue Swirl, Liu >> "Liu, Jinsong",
Paul Brook, Avi Kivity, qemu-devel
On 08/20/2010 12:01 PM, Markus Armbruster wrote:
> Anthony Liguori<anthony@codemonkey.ws> writes:
>
>
>> On 08/19/2010 04:21 PM, Blue Swirl wrote:
>>
>>>> Should CPUs appear in the QEMU device tree?
>>>>
>>>>
>>> They have several properties that should be user visible.
>>>
>>>
>> Sure, but that's an argument for having some of the qdev features
>> (like variant properties) be consumable outside of qdev.
>>
> 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. But do we really want to model memory chipsets, a
north and south bridge, and long with cache hierarchies?
It's the trade off between a functional simulator and a cycle accurate
simulator.
>> The only way to do this right now is having a bus with a single
>> device. I've been working on the serial device, and what this
>> requires is having an ISASerialDevice which is-a ISADevice. The
>> ISASerialDevice has-a
>> SerialBus and the SerialBus contain a single SerialDevice which is-a
>> DeviceState.
>>
> It needs to be a SerialBusDevice, not just a DeviceState, actually.
>
It doesn't "need to be" actually. Like it or not, we're still upcast
from DeviceState so there's no practical reason to have a
SerialBusDevice unless you're wrapping an interface behind function
pointers. But if you're only ever going to have one SerialDevice,
what's the point?
>> But the ISASerialDevice has to assume that the SerialBus contains only
>> a single child because it needs to connect the GPIO out from the
>> SerialDevice to the ISA irq that's assigned.
>>
>> It would be a whole lot simpler to break the "all devices sit on
>> busses" assumption that we have today and simply have the
>> ISASerialDevice has-a
>> SerialDevice with no additional layers of bus indirection.
>>
> What's the *practical* benefit of splitting up ISASerialDevice into
> UART-ISA adapter and the UART proper?
>
> The "bus" sitting between the two would be trivial. If we do that
> everywhere, we'll end up with many different, trivial buses.
>
If we wanted to add per-device locking based on putting a lock in
DeviceState that was acquired and released whenever you executed a PIO,
how would you do that today?
You would convert cpu_register_ioport_* to take a DeviceState in
serial_init_core. Sure, you could add a layer of indirection in
ISASerialDevice, but what about timers? We would want to implement
device based timers to do the same thing but again, when we register the
timers we don't have a DeviceState.
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.
>> qdev level. Hot plugging is a very bus specific concept with very bus
>> specific restrictions. For instance, with PCI, you can only plug at
>> slot granularities whereas today, we don't really model multi-function
>> devices as anything more than a set of unrelated devices. So there's
>> really no way you could conceptually hot plug a multi-function virtio
>> adapter although you can pretty trivially create one statically.
>>
> I think it's fair to say that qdev doesn't currently get hot-plugging
> right.
>
I think we can fix it pretty reasonably though. I think we can get
pretty far by adding an interface whereas when creating a device,
qdev_create calls into the parent Bus to "plug" it in which gives the
parent bus an opportunity to reject the creation.
We need to indicate to the devices when they have been "realized".
Devices have (at least) three explicit states, constructing,
initialized, and realized. Realized is the point where it can expect
I/O operations from the guest. Each bus can then decide when it is
willing to accept new devices and whether the devices can be hotplugged
meaningfully.
For instance, sysbus would reject any devices added in the realized
state whereas the PCI bus would accept devices in either the initialized
state or the realized state but in the realized state, it would trigger
a hotplug notification to the guest. If the PCI address of the device
didn't make sense (for instance, it was a function on a multi-function
device), it would be rejected.
>>> What about 486? Or 82489?
>>>
>>>
>> Don't confuse the local APIC with the PIC or the I/O APIC.
>>
>> The local APIC has always existed in the CPU core. There is also an
>> I/O APIC which exists outside of the CPU core. The local APIC was
>> introduced with SMP support.
>>
>> The PIC and IO APIC totally make sense to be modelled as qdev.
>>
>>
>>>> It's like modelling a TLB as a
>>>> separate device.
>>>>
>>>>
>>> That has surely happened in ancient times.
>>>
>>>
>> Yes, but the programming model was different.
>>
>> Look at the PIC compared to the lapic. The PIC is programmed via pio
>> at a fixed location. There is only one PIC and it interacts with the
>> system just like all other devices. IOW, there is no reference to
>> CPUState.
>>
>> When you look at the local APIC (apic.c) however, you see that it's
>> the only device in the tree that actually interacts with a CPUState.
>> The notion of cpu_get_current_apic() is a good example of the tricks
>> needed to make this work.
>>
>> The local APIC is a cpu feature, not a device.
>>
> A bit like the UART is a ISASerialDevice feature?
>
Not at all. A UART is a chip that exists on a physical board. The fact
that the board is a daughter board connected via a signal multiplexing
bus is an implementation detail that is oblivious to the UART. It can
be the same chip regardless of whether it's on an ISA card or directly
attached to a PIC microcontroller.
A local APIC is transistors and microcode in modern processors. You
cannot take a modern local APIC and interface it to a PIC
microcontroller in any meaningful way.
Regards,
Anthony Liguori
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup
2010-08-20 18:38 ` Anthony Liguori
@ 2010-08-22 20:28 ` Avi Kivity
2010-08-22 21:02 ` Anthony Liguori
0 siblings, 1 reply; 54+ messages in thread
From: Avi Kivity @ 2010-08-22 20:28 UTC (permalink / raw)
To: Anthony Liguori
Cc: Blue Swirl, Liu >> "Liu, Jinsong",
qemu-devel, Markus Armbruster, Paul Brook
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.
> But do we really want to model memory chipsets, a north and south
> bridge, and long with cache hierarchies?
We do model devices within the north and south bridges. The aggregation
into two chips is largely meaningless from a functional point of view,
as are cache hierarchies.
> It's the trade off between a functional simulator and a cycle accurate
> simulator.
>
Exactly. A cpu is a functional unit since it's hot-pluggable and has
user-visible state. A north bridge is not hot-pluggable and has no
user-visible state (apart from its component devices). A PCI device
does not have user visible state (except as an aggregation of
functions), but it is hot-pluggable, thus it should be modelled.
> If we wanted to add per-device locking based on putting a lock in
> DeviceState that was acquired and released whenever you executed a
> PIO, how would you do that today?
>
> You would convert cpu_register_ioport_* to take a DeviceState in
> serial_init_core. Sure, you could add a layer of indirection in
> ISASerialDevice, but what about timers? We would want to implement
> device based timers to do the same thing but again, when we register
> the timers we don't have a DeviceState.
>
> 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?
Much better to call a traditional callback which then uses
container_of() to locate its state.
--
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup
2010-08-22 20:28 ` Avi Kivity
@ 2010-08-22 21:02 ` Anthony Liguori
2010-08-23 5:46 ` Avi Kivity
0 siblings, 1 reply; 54+ messages in thread
From: Anthony Liguori @ 2010-08-22 21:02 UTC (permalink / raw)
To: Avi Kivity
Cc: Blue Swirl, Liu >> "Liu, Jinsong",
qemu-devel, Markus Armbruster, Paul Brook
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.
But we really need to spend some more energy on how we model the device
tree because it's important to work out the interface that we want to
represent without going too low.
>> But do we really want to model memory chipsets, a north and south
>> bridge, and long with cache hierarchies?
>
> We do model devices within the north and south bridges. The
> aggregation into two chips is largely meaningless from a functional
> point of view, as are cache hierarchies.
Right, but it's an issue of how we want devices to interact.
This is the important thing to figure out. Implementations languages
don't matter compared to getting the object model right. If we tried to
model the device tree based on pin-outs being the interface between
devices at the lowest level, we're going to fail in any language.
>> If we wanted to add per-device locking based on putting a lock in
>> DeviceState that was acquired and released whenever you executed a
>> PIO, how would you do that today?
>>
>> You would convert cpu_register_ioport_* to take a DeviceState in
>> serial_init_core. Sure, you could add a layer of indirection in
>> ISASerialDevice, but what about timers? We would want to implement
>> device based timers to do the same thing but again, when we register
>> the timers we don't have a DeviceState.
>>
>> 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.
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.
Regards,
Anthony Liguori
> Much better to call a traditional callback which then uses
> container_of() to locate its state.
>
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup
2010-08-22 21:02 ` Anthony Liguori
@ 2010-08-23 5:46 ` Avi Kivity
2010-08-23 13:23 ` Anthony Liguori
0 siblings, 1 reply; 54+ messages in thread
From: Avi Kivity @ 2010-08-23 5:46 UTC (permalink / raw)
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.
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup
2010-08-23 5:46 ` Avi Kivity
@ 2010-08-23 13:23 ` Anthony Liguori
2010-08-23 13:42 ` Avi Kivity
0 siblings, 1 reply; 54+ messages in thread
From: Anthony Liguori @ 2010-08-23 13:23 UTC (permalink / raw)
To: Avi Kivity
Cc: Blue Swirl, Liu >> "Liu, Jinsong",
qemu-devel, Markus Armbruster, Paul Brook
On 08/23/2010 12:46 AM, Avi Kivity wrote:
> 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?
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.
> 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 :-)
>> 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.
>> 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.
Regards,
Anthony Liguori
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup
2010-08-23 13:23 ` Anthony Liguori
@ 2010-08-23 13:42 ` Avi Kivity
2010-08-23 13:48 ` Anthony Liguori
0 siblings, 1 reply; 54+ messages in thread
From: Avi Kivity @ 2010-08-23 13:42 UTC (permalink / raw)
To: Anthony Liguori
Cc: Blue Swirl, Liu >> "Liu, Jinsong",
qemu-devel, Markus Armbruster, Paul Brook
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
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup
2010-08-23 13:42 ` Avi Kivity
@ 2010-08-23 13:48 ` Anthony Liguori
2010-08-23 14:00 ` Avi Kivity
0 siblings, 1 reply; 54+ messages in thread
From: Anthony Liguori @ 2010-08-23 13:48 UTC (permalink / raw)
To: Avi Kivity
Cc: Blue Swirl, Liu >> "Liu, Jinsong",
qemu-devel, Markus Armbruster, Paul Brook
On 08/23/2010 08:42 AM, Avi Kivity wrote:
>>> 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.
I think you're reading too much into my late night rantings ;-)
>>>> 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)?
No. If this was C++, then the statement would be: device have to be
implemented in terms of objects that inherit from Device. Device is our
common base object.
> Isn't "save/restore works" an equivalent statement to "device state is
> reachable from the DeviceState"?
I'm not sure I can connect the dots here as I'm not sure what follows if
your assertion is true.
Regards,
Anthony Liguori
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup
2010-08-23 13:48 ` Anthony Liguori
@ 2010-08-23 14:00 ` Avi Kivity
2010-08-23 14:26 ` Anthony Liguori
0 siblings, 1 reply; 54+ messages in thread
From: Avi Kivity @ 2010-08-23 14:00 UTC (permalink / raw)
To: Anthony Liguori
Cc: Blue Swirl, Liu >> "Liu, Jinsong",
qemu-devel, Markus Armbruster, Paul Brook
On 08/23/2010 04:48 PM, Anthony Liguori wrote:
>>> 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)?
>
>
> No. If this was C++, then the statement would be: device have to be
> implemented in terms of objects that inherit from Device. Device is
> our common base object.
so,
struct MyDevicestate {
struct DeviceState device_state;
bool *some_bit;
};
bad, while
struct MyDevicestate {
struct DeviceState device_state;
bool some_bit;
};
good?
>
>> Isn't "save/restore works" an equivalent statement to "device state
>> is reachable from the DeviceState"?
>
> I'm not sure I can connect the dots here as I'm not sure what follows
> if your assertion is true.
If save/restore works then all state is reachable from one point?
Presumable DeviceState?
I really don't see why the state has to be in the DeviceState subclass.
We're probably talking past each other here due to some confusion in terms.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup
2010-08-23 14:00 ` Avi Kivity
@ 2010-08-23 14:26 ` Anthony Liguori
2010-08-23 14:32 ` Avi Kivity
0 siblings, 1 reply; 54+ messages in thread
From: Anthony Liguori @ 2010-08-23 14:26 UTC (permalink / raw)
To: Avi Kivity
Cc: Blue Swirl, Liu >> "Liu, Jinsong",
qemu-devel, Markus Armbruster, Paul Brook
On 08/23/2010 09:00 AM, Avi Kivity wrote:
> On 08/23/2010 04:48 PM, Anthony Liguori wrote:
>>>> 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)?
>>
>>
>> No. If this was C++, then the statement would be: device have to be
>> implemented in terms of objects that inherit from Device. Device is
>> our common base object.
>
> so,
>
> struct MyDevicestate {
> struct DeviceState device_state;
> bool *some_bit;
> };
>
> bad, while
>
> struct MyDevicestate {
> struct DeviceState device_state;
> bool some_bit;
> };
>
> good?
And the next logical question is whether:
struct MyDeviceState {
DeviceState qdev;
OtherState *s;
};
Is bad? The answer would depend on whether OtherState implemented
methods or not. If OtherState has no methods, it's fine. If it has
methods, it's bad.
>
>
>>
>>> Isn't "save/restore works" an equivalent statement to "device state
>>> is reachable from the DeviceState"?
>>
>> I'm not sure I can connect the dots here as I'm not sure what follows
>> if your assertion is true.
>
> If save/restore works then all state is reachable from one point?
> Presumable DeviceState?
>
> I really don't see why the state has to be in the DeviceState
> subclass. We're probably talking past each other here due to some
> confusion in terms.
Probably. Let's say that 'structs' are containers of primatives that
have no methods associated with them. 'objects' are structs that have
methods and potentially constructors/destructors.
Devices can contain references to structs and objects. If a Device
contains a reference to an object, the object should be stored in a
BusState which is a container of Devices. Therefore, the object should
inherit from Device.
Regards,
Anthony Liguori
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup
2010-08-23 14:26 ` Anthony Liguori
@ 2010-08-23 14:32 ` Avi Kivity
2010-08-23 14:47 ` Anthony Liguori
0 siblings, 1 reply; 54+ messages in thread
From: Avi Kivity @ 2010-08-23 14:32 UTC (permalink / raw)
To: Anthony Liguori
Cc: Blue Swirl, Liu >> "Liu, Jinsong",
qemu-devel, Markus Armbruster, Paul Brook
On 08/23/2010 05:26 PM, Anthony Liguori wrote:
> On 08/23/2010 09:00 AM, Avi Kivity wrote:
>> On 08/23/2010 04:48 PM, Anthony Liguori wrote:
>>>>> 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)?
>>>
>>>
>>> No. If this was C++, then the statement would be: device have to be
>>> implemented in terms of objects that inherit from Device. Device is
>>> our common base object.
>>
>> so,
>>
>> struct MyDevicestate {
>> struct DeviceState device_state;
>> bool *some_bit;
>> };
>>
>> bad, while
>>
>> struct MyDevicestate {
>> struct DeviceState device_state;
>> bool some_bit;
>> };
>>
>> good?
>
> And the next logical question is whether:
>
> struct MyDeviceState {
> DeviceState qdev;
> OtherState *s;
> };
>
> Is bad? The answer would depend on whether OtherState implemented
> methods or not. If OtherState has no methods, it's fine. If it has
> methods, it's bad.
I don't see why. As long as you can manipulate all of MyDevice's state
via MyDeviceState methods, why do you care about OtherState at all?
>
>>
>>>
>>>> Isn't "save/restore works" an equivalent statement to "device state
>>>> is reachable from the DeviceState"?
>>>
>>> I'm not sure I can connect the dots here as I'm not sure what
>>> follows if your assertion is true.
>>
>> If save/restore works then all state is reachable from one point?
>> Presumable DeviceState?
>>
>> I really don't see why the state has to be in the DeviceState
>> subclass. We're probably talking past each other here due to some
>> confusion in terms.
>
> Probably. Let's say that 'structs' are containers of primatives that
> have no methods associated with them. 'objects' are structs that have
> methods and potentially constructors/destructors.
>
> Devices can contain references to structs and objects. If a Device
> contains a reference to an object, the object should be stored in a
> BusState which is a container of Devices. Therefore, the object
> should inherit from Device.
I disagree. It's up to the author to decide whether to split a Device
into 1 or 15 objects.
If one of the other objects is also a subclass of DeviceState, then
you're probably violating that DeviceState's contract. But that's a
different (and trivial) matter.
(side point: in C no objects have constructors and methods. in C++ all
objects have constructors and methods, even PODs)
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup
2010-08-23 14:32 ` Avi Kivity
@ 2010-08-23 14:47 ` Anthony Liguori
2010-08-23 15:10 ` Markus Armbruster
2010-08-23 15:14 ` [Qemu-devel] " Avi Kivity
0 siblings, 2 replies; 54+ messages in thread
From: Anthony Liguori @ 2010-08-23 14:47 UTC (permalink / raw)
To: Avi Kivity
Cc: Blue Swirl, Liu >> "Liu, Jinsong",
qemu-devel, Markus Armbruster, Paul Brook
On 08/23/2010 09:32 AM, Avi Kivity wrote:
>> Is bad? The answer would depend on whether OtherState implemented
>> methods or not. If OtherState has no methods, it's fine. If it has
>> methods, it's bad.
>
>
> I don't see why. As long as you can manipulate all of MyDevice's
> state via MyDeviceState methods, why do you care about OtherState at all?
See below.
>>
>> Devices can contain references to structs and objects. If a Device
>> contains a reference to an object, the object should be stored in a
>> BusState which is a container of Devices. Therefore, the object
>> should inherit from Device.
>
> I disagree. It's up to the author to decide whether to split a Device
> into 1 or 15 objects.
>
> If one of the other objects is also a subclass of DeviceState, then
> you're probably violating that DeviceState's contract. But that's a
> different (and trivial) matter.
>
> (side point: in C no objects have constructors and methods. in C++
> all objects have constructors and methods, even PODs)
Things that inherit from DeviceState have ctors/dtors. Things that
don't end up inventing their own and probably will do so poorly.
When implementing a C object model, you really need to have a base
object that everything inherits from. In glib, it's GObject.
Regards,
Anthony Liguori
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup
2010-08-23 14:47 ` Anthony Liguori
@ 2010-08-23 15:10 ` Markus Armbruster
2010-08-23 16:05 ` Anthony Liguori
2010-08-23 15:14 ` [Qemu-devel] " Avi Kivity
1 sibling, 1 reply; 54+ messages in thread
From: Markus Armbruster @ 2010-08-23 15:10 UTC (permalink / raw)
To: Anthony Liguori
Cc: Blue Swirl, Liu @mail.corp.redhat.com>> "Liu,
Jinsong",
qemu-devel, Avi Kivity, Paul Brook
You lost me. A few messages upthread.
What's the *practical* problem again?
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup
2010-08-23 15:10 ` Markus Armbruster
@ 2010-08-23 16:05 ` Anthony Liguori
2010-08-23 17:36 ` Markus Armbruster
2010-08-23 18:24 ` [Qemu-devel] " Jan Kiszka
0 siblings, 2 replies; 54+ messages in thread
From: Anthony Liguori @ 2010-08-23 16:05 UTC (permalink / raw)
To: Markus Armbruster
Cc: Blue Swirl, Liu @mail.corp.redhat.com>> "Liu,
Jinsong",
qemu-devel, Avi Kivity, Paul Brook
On 08/23/2010 10:10 AM, Markus Armbruster wrote:
> You lost me. A few messages upthread.
>
> What's the *practical* problem again?
>
CPU hotplug adds a local APIC to Sysbus but Sysbus does not allow hot plug.
I believe the right short term way to fix this is to take the local APIC
off of Sysbus. The right long term fix is to not make the local APIC a
qdev device and instead fold it into CPUX86State.
Regards,
Anthony Liguori
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup
2010-08-23 16:05 ` Anthony Liguori
@ 2010-08-23 17:36 ` Markus Armbruster
2010-08-23 17:47 ` Anthony Liguori
2010-08-23 18:24 ` [Qemu-devel] " Jan Kiszka
1 sibling, 1 reply; 54+ messages in thread
From: Markus Armbruster @ 2010-08-23 17:36 UTC (permalink / raw)
To: Anthony Liguori
Cc: Blue Swirl, Liu @mail.corp.redhat.com>> "Liu,
Jinsong",
qemu-devel, Avi Kivity, Paul Brook
Anthony Liguori <anthony@codemonkey.ws> writes:
> On 08/23/2010 10:10 AM, Markus Armbruster wrote:
>> You lost me. A few messages upthread.
>>
>> What's the *practical* problem again?
>>
>
> CPU hotplug adds a local APIC to Sysbus but Sysbus does not allow hot plug.
>
> I believe the right short term way to fix this is to take the local
> APIC off of Sysbus. The right long term fix is to not make the local
> APIC a qdev device and instead fold it into CPUX86State.
Treating the LAPIC as a part of the CPU makes sense to me. I can't see,
however, how all the OO-talk in this thread is related to that, and I
can't connect the OO-talk to a practical problem. Do I miss anything
important?
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup
2010-08-23 17:36 ` Markus Armbruster
@ 2010-08-23 17:47 ` Anthony Liguori
0 siblings, 0 replies; 54+ messages in thread
From: Anthony Liguori @ 2010-08-23 17:47 UTC (permalink / raw)
To: Markus Armbruster
Cc: Blue Swirl, Liu @mail.corp.redhat.com>> "Liu,
Jinsong",
qemu-devel, Avi Kivity, Paul Brook
On 08/23/2010 12:36 PM, Markus Armbruster wrote:
> Anthony Liguori<anthony@codemonkey.ws> writes:
>
>
>> On 08/23/2010 10:10 AM, Markus Armbruster wrote:
>>
>>> You lost me. A few messages upthread.
>>>
>>> What's the *practical* problem again?
>>>
>>>
>> CPU hotplug adds a local APIC to Sysbus but Sysbus does not allow hot plug.
>>
>> I believe the right short term way to fix this is to take the local
>> APIC off of Sysbus. The right long term fix is to not make the local
>> APIC a qdev device and instead fold it into CPUX86State.
>>
> Treating the LAPIC as a part of the CPU makes sense to me. I can't see,
> however, how all the OO-talk in this thread is related to that, and I
> can't connect the OO-talk to a practical problem. Do I miss anything
> important?
>
It's abstract talk about what the qdev object module includes, what it
doesn't include, and what the interfaces are.
It started because I claimed that the lapic doesn't fit the qdev object
module and Avi disagrees.
Regards,
Anthony Liguori
^ permalink raw reply [flat|nested] 54+ messages in thread
* [Qemu-devel] Re: [PATCH v2 0/7] APIC/IOAPIC cleanup
2010-08-23 16:05 ` Anthony Liguori
2010-08-23 17:36 ` Markus Armbruster
@ 2010-08-23 18:24 ` Jan Kiszka
2010-08-23 18:29 ` Anthony Liguori
1 sibling, 1 reply; 54+ messages in thread
From: Jan Kiszka @ 2010-08-23 18:24 UTC (permalink / raw)
To: Anthony Liguori
Cc: Liu @mail.corp.redhat.com>> "Liu, Jinsong",
Markus Armbruster, qemu-devel, Blue Swirl, Avi Kivity,
Paul Brook
Anthony Liguori wrote:
> On 08/23/2010 10:10 AM, Markus Armbruster wrote:
>> You lost me. A few messages upthread.
>>
>> What's the *practical* problem again?
>>
>
> CPU hotplug adds a local APIC to Sysbus but Sysbus does not allow hot plug.
>
> I believe the right short term way to fix this is to take the local APIC
> off of Sysbus. The right long term fix is to not make the local APIC a
> qdev device and instead fold it into CPUX86State.
This pushes the issue to the CPU. I'm convinced CPUs should become qdevs
as well, so we need a hotplug-capable bus anyway.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 54+ messages in thread
* [Qemu-devel] Re: [PATCH v2 0/7] APIC/IOAPIC cleanup
2010-08-23 18:24 ` [Qemu-devel] " Jan Kiszka
@ 2010-08-23 18:29 ` Anthony Liguori
0 siblings, 0 replies; 54+ messages in thread
From: Anthony Liguori @ 2010-08-23 18:29 UTC (permalink / raw)
To: Jan Kiszka
Cc: Liu @mail.corp.redhat.com>> "Liu, Jinsong",
Markus Armbruster, qemu-devel, Blue Swirl, Avi Kivity,
Paul Brook
On 08/23/2010 01:24 PM, Jan Kiszka wrote:
> Anthony Liguori wrote:
>
>> On 08/23/2010 10:10 AM, Markus Armbruster wrote:
>>
>>> You lost me. A few messages upthread.
>>>
>>> What's the *practical* problem again?
>>>
>>>
>> CPU hotplug adds a local APIC to Sysbus but Sysbus does not allow hot plug.
>>
>> I believe the right short term way to fix this is to take the local APIC
>> off of Sysbus. The right long term fix is to not make the local APIC a
>> qdev device and instead fold it into CPUX86State.
>>
> This pushes the issue to the CPU. I'm convinced CPUs should become qdevs
> as well, so we need a hotplug-capable bus anyway.
>
For the CPU, yes. That implies making a CPUBus and making it hot pluggable.
Regards,
Anthony Liguori
> Jan
>
>
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup
2010-08-23 14:47 ` Anthony Liguori
2010-08-23 15:10 ` Markus Armbruster
@ 2010-08-23 15:14 ` Avi Kivity
2010-08-23 16:02 ` Anthony Liguori
1 sibling, 1 reply; 54+ messages in thread
From: Avi Kivity @ 2010-08-23 15:14 UTC (permalink / raw)
To: Anthony Liguori
Cc: Blue Swirl, Liu >> "Liu, Jinsong",
qemu-devel, Markus Armbruster, Paul Brook
On 08/23/2010 05:47 PM, Anthony Liguori wrote:
>
>>>
>>> Devices can contain references to structs and objects. If a Device
>>> contains a reference to an object, the object should be stored in a
>>> BusState which is a container of Devices. Therefore, the object
>>> should inherit from Device.
>>
>> I disagree. It's up to the author to decide whether to split a
>> Device into 1 or 15 objects.
>>
>> If one of the other objects is also a subclass of DeviceState, then
>> you're probably violating that DeviceState's contract. But that's a
>> different (and trivial) matter.
>>
>> (side point: in C no objects have constructors and methods. in C++
>> all objects have constructors and methods, even PODs)
>
> Things that inherit from DeviceState have ctors/dtors. Things that
> don't end up inventing their own and probably will do so poorly.
you mean misimplement other_state_init(OtherState *) and
other_state_destroy(OtherState *)?
> When implementing a C object model, you really need to have a base
> object that everything inherits from.
Why? While it's true for Java, I don't see why it needs to be true for C.
> In glib, it's GObject.
We aren't using glib.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup
2010-08-23 15:14 ` [Qemu-devel] " Avi Kivity
@ 2010-08-23 16:02 ` Anthony Liguori
2010-08-24 9:51 ` Avi Kivity
0 siblings, 1 reply; 54+ messages in thread
From: Anthony Liguori @ 2010-08-23 16:02 UTC (permalink / raw)
To: Avi Kivity
Cc: Blue Swirl, Liu >> "Liu, Jinsong",
qemu-devel, Markus Armbruster, Paul Brook
On 08/23/2010 10:14 AM, Avi Kivity wrote:
> On 08/23/2010 05:47 PM, Anthony Liguori wrote:
>>
>>>>
>>>> Devices can contain references to structs and objects. If a Device
>>>> contains a reference to an object, the object should be stored in a
>>>> BusState which is a container of Devices. Therefore, the object
>>>> should inherit from Device.
>>>
>>> I disagree. It's up to the author to decide whether to split a
>>> Device into 1 or 15 objects.
>>>
>>> If one of the other objects is also a subclass of DeviceState, then
>>> you're probably violating that DeviceState's contract. But that's a
>>> different (and trivial) matter.
>>>
>>> (side point: in C no objects have constructors and methods. in C++
>>> all objects have constructors and methods, even PODs)
>>
>> Things that inherit from DeviceState have ctors/dtors. Things that
>> don't end up inventing their own and probably will do so poorly.
>
> you mean misimplement other_state_init(OtherState *) and
> other_state_destroy(OtherState *)?
And hot plug, save/restore, properties, and all of the other things that
go along with qdev.
Hot plug is a good example of how easy it is to screw this up by not
having the right hooks being propagated through the device model.
Regards,
Anthony Liguori
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup
2010-08-23 16:02 ` Anthony Liguori
@ 2010-08-24 9:51 ` Avi Kivity
0 siblings, 0 replies; 54+ messages in thread
From: Avi Kivity @ 2010-08-24 9:51 UTC (permalink / raw)
To: Anthony Liguori
Cc: Blue Swirl, Liu >> "Liu, Jinsong",
qemu-devel, Markus Armbruster, Paul Brook
On 08/23/2010 07:02 PM, Anthony Liguori wrote:
> On 08/23/2010 10:14 AM, Avi Kivity wrote:
>> On 08/23/2010 05:47 PM, Anthony Liguori wrote:
>>>
>>>>>
>>>>> Devices can contain references to structs and objects. If a
>>>>> Device contains a reference to an object, the object should be
>>>>> stored in a BusState which is a container of Devices. Therefore,
>>>>> the object should inherit from Device.
>>>>
>>>> I disagree. It's up to the author to decide whether to split a
>>>> Device into 1 or 15 objects.
>>>>
>>>> If one of the other objects is also a subclass of DeviceState, then
>>>> you're probably violating that DeviceState's contract. But that's
>>>> a different (and trivial) matter.
>>>>
>>>> (side point: in C no objects have constructors and methods. in C++
>>>> all objects have constructors and methods, even PODs)
>>>
>>> Things that inherit from DeviceState have ctors/dtors. Things that
>>> don't end up inventing their own and probably will do so poorly.
>>
>> you mean misimplement other_state_init(OtherState *) and
>> other_state_destroy(OtherState *)?
>
> And hot plug, save/restore, properties, and all of the other things
> that go along with qdev.
>
> Hot plug is a good example of how easy it is to screw this up by not
> having the right hooks being propagated through the device model.
Come on now, this is all trivial. If you get a qdev callback you just
propagate it to all objects that need it.
Practically all non-trivial devices need to do it. For example if you
remove gpio from qdev, then a device that has an array of gpios will
have un-embedded objects in its DeviceState.
Look at ivshmem.c with its non-embedded Peer objects.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup
2010-08-19 21:51 ` Anthony Liguori
` (2 preceding siblings ...)
2010-08-20 17:01 ` [Qemu-devel] " Markus Armbruster
@ 2010-08-20 19:26 ` Blue Swirl
3 siblings, 0 replies; 54+ messages in thread
From: Blue Swirl @ 2010-08-20 19:26 UTC (permalink / raw)
To: Anthony Liguori
Cc: Liu >> "Liu, Jinsong", Avi Kivity, qemu-devel, Paul Brook
On Thu, Aug 19, 2010 at 9:51 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 08/19/2010 04:21 PM, Blue Swirl wrote:
>>>
>>> Should CPUs appear in the QEMU device tree?
>>>
>>
>> They have several properties that should be user visible.
>>
>
> Sure, but that's an argument for having some of the qdev features (like
> variant properties) be consumable outside of qdev.
Or rather, separate qdev from SysBus. I don't think either makes much sense.
>>> requirement to sit on a bus. A UART16650A does not sit on bus. It sits
>>> on
>>> a card and is wired to the ISA bus or is sometimes wired directly to pins
>>> on
>>> a CPU on a SoC.
>>>
>>
>> Or all buses should be unified, so all devices could be wired into any
>> board.
>>
>
> But that defeats the notion of having bus level interfaces. You really want
> the virtio PCI layer to only use PCI functions and specifically to interact
> with PCI concepts that don't exist in other busses (like IO regions and
> config space). However, you also want the ability to interact with a virtio
> device through a well defined interface that isn't PCI specific.
True, but if a device uses dedicated bus interfaces, it can't be used
on other buses. It might be possible to construct the devices so that
most of the code is bus agnostic and only a shim interfaces with the
bus of the day, which should be close to what you proposed in the
earlier serial example.
One way to model pure devices could be something like chip select
line, address bus (possibly truncated) and data bus (also possibly
narrowed).
Any way, I think the current model is pretty much fine as is.
> The only way to do this right now is having a bus with a single device.
> I've been working on the serial device, and what this requires is having an
> ISASerialDevice which is-a ISADevice. The ISASerialDevice has-a SerialBus
> and the SerialBus contain a single SerialDevice which is-a DeviceState.
>
> But the ISASerialDevice has to assume that the SerialBus contains only a
> single child because it needs to connect the GPIO out from the SerialDevice
> to the ISA irq that's assigned.
>
> It would be a whole lot simpler to break the "all devices sit on busses"
> assumption that we have today and simply have the ISASerialDevice has-a
> SerialDevice with no additional layers of bus indirection.
>
>> IIRC I tried also that approach when I worked on this patch set but
>> there were some problems. Maybe with header file conflicts, or
>> qemu_irq was not available to CPU code.
>>
>
> Okay, I'll queue it up for a future day.
>
>>> Because not all devices on the sysbus can be hot added so if you made the
>>> bus hotpluggable, it would basically defeat the point of even marking a
>>> bus
>>> as not supporting hot plug.
>>>
>>> IOW, the whole bus is either hot pluggable or not. You cannot just say
>>> that
>>> device X can be hotplugged but nothing else.
>>>
>>
>> Perhaps the hotplugging system in QEMU needs some rethinking instead.
>> Many real devices are not hot pluggable.
>>
>
> That's probably fair. I don't think hot plugging should exist at the qdev
> level. Hot plugging is a very bus specific concept with very bus specific
> restrictions. For instance, with PCI, you can only plug at slot
> granularities whereas today, we don't really model multi-function devices as
> anything more than a set of unrelated devices. So there's really no way you
> could conceptually hot plug a multi-function virtio adapter although you can
> pretty trivially create one statically.
Maybe PCI could introduce an indirection: cards. They could contain
any number of qdev devices.
>>>>> I think the options are to allow non-bus devices (like the APIC) or
>>>>> make
>>>>> the
>>>>> APIC a special case that's part of the CPU emulation.
>>>>>
>>>>>
>>>>
>>>> No. There could also be a new hotpluggable bus type, CPUBus, one
>>>> instance between each CPU and APIC. Or SysBusWithHotPlug. But I don't
>>>> see how that would be different from SysBus.
>>>>
>>>>
>>>
>>> Neither approach maps well to real hardware. An x86 CPU cannot exist
>>> without a local APIC and a local APIC cannot exist without an x86 CPU.
>>> The
>>> two are fundamentally tied together.
>>>
>>
>> What about 486? Or 82489?
>>
>
> Don't confuse the local APIC with the PIC or the I/O APIC.
>
> The local APIC has always existed in the CPU core. There is also an I/O
> APIC which exists outside of the CPU core. The local APIC was introduced
> with SMP support.
No, 82489 was an external local APIC for 486DX. See for example Intel
Multiprocessor Specification, page 3-5.
> The PIC and IO APIC totally make sense to be modelled as qdev.
>
>>> It's like modelling a TLB as a
>>> separate device.
>>>
>>
>> That has surely happened in ancient times.
>>
>
> Yes, but the programming model was different.
>
> Look at the PIC compared to the lapic. The PIC is programmed via pio at a
> fixed location. There is only one PIC and it interacts with the system just
> like all other devices. IOW, there is no reference to CPUState.
>
> When you look at the local APIC (apic.c) however, you see that it's the only
> device in the tree that actually interacts with a CPUState. The notion of
> cpu_get_current_apic() is a good example of the tricks needed to make this
> work.
>
> The local APIC is a cpu feature, not a device.
That is certainly one way how to handle it, but not the only one and
not the most flexible. In contrast, CPUBus could have other uses like
north/south bridges, caches etc. On Sparc, a CPUBus could fuse ASIs
and addresses and these could be decoded by an external device. That
could simplify target-sparc/op_helper.c while not messing physical
address space order too much.
But in my mind, what in this specific case binds local APIC tighter to
the CPU than to the devices is the CR register access. We probably
don't want a CRBus.
^ permalink raw reply [flat|nested] 54+ messages in thread