[Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup

[Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup

[Blue Swirl]

Clean up APIC and IOAPIC. Convert both devices to qdev.

v1->v2:
Remove apic.h reorganization.
Add IOAPIC and APIC qdev conversions.
Use CPUState also in 5/7. However on 6/7 we have to again use void *
because of VMState limitations. VMState gurus, please comment.

Blue Swirl (7):
  ioapic: unexport ioapic_set_irq
  ioapic: convert to qdev
  apic: avoid passing CPUState from devices
  apic: avoid passing CPUState from CPU code
  apic: avoid using CPUState internals
  apic: convert to qdev
  apic: qdev conversion cleanup

 hw/apic.c               |  174 +++++++++++++++++++++++-----------------------
 hw/apic.h               |   21 ++++--
 hw/ioapic.c             |   47 ++++++++----
 hw/pc.c                 |   74 ++++++++++++++++++--
 hw/pc.h                 |    4 +-
 hw/pc_piix.c            |   19 +++++-
 qemu-common.h           |    2 +-
 target-i386/cpu.h       |   28 +++++---
 target-i386/cpuid.c     |    6 ++
 target-i386/helper.c    |    4 +-
 target-i386/kvm.c       |   14 ++--
 target-i386/op_helper.c |    8 +-
 12 files changed, 258 insertions(+), 143 deletions(-)

[Qemu-devel] Re: [PATCH v2 0/7] APIC/IOAPIC cleanup

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1292 bytes --]

Blue Swirl wrote:
> Clean up APIC and IOAPIC. Convert both devices to qdev.

Wanted to give this a try, but patches are line-wrapped. Please fix.

Jan

> 
> v1->v2:
> Remove apic.h reorganization.
> Add IOAPIC and APIC qdev conversions.
> Use CPUState also in 5/7. However on 6/7 we have to again use void *
> because of VMState limitations. VMState gurus, please comment.
> 
> Blue Swirl (7):
>   ioapic: unexport ioapic_set_irq
>   ioapic: convert to qdev
>   apic: avoid passing CPUState from devices
>   apic: avoid passing CPUState from CPU code
>   apic: avoid using CPUState internals
>   apic: convert to qdev
>   apic: qdev conversion cleanup
> 
>  hw/apic.c               |  174 +++++++++++++++++++++++-----------------------
>  hw/apic.h               |   21 ++++--
>  hw/ioapic.c             |   47 ++++++++----
>  hw/pc.c                 |   74 ++++++++++++++++++--
>  hw/pc.h                 |    4 +-
>  hw/pc_piix.c            |   19 +++++-
>  qemu-common.h           |    2 +-
>  target-i386/cpu.h       |   28 +++++---
>  target-i386/cpuid.c     |    6 ++
>  target-i386/helper.c    |    4 +-
>  target-i386/kvm.c       |   14 ++--
>  target-i386/op_helper.c |    8 +-
>  12 files changed, 258 insertions(+), 143 deletions(-)
> 
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Qemu-devel] Re: [PATCH v2 0/7] APIC/IOAPIC cleanup

[Andreas Färber]

Am 13.06.2010 um 18:56 schrieb Jan Kiszka:

> Blue Swirl wrote:
>> Clean up APIC and IOAPIC. Convert both devices to qdev.
>
> Wanted to give this a try, but patches are line-wrapped. Please fix.

Threading the patches would also be appreciated.
Git's default changed at some point, this can be overridden  
in .gitconfig.

Andreas

[Qemu-devel] Re: [PATCH v2 0/7] APIC/IOAPIC cleanup

[Blue Swirl]

On Sun, Jun 13, 2010 at 4:56 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
> Blue Swirl wrote:
>> Clean up APIC and IOAPIC. Convert both devices to qdev.
>
> Wanted to give this a try, but patches are line-wrapped. Please fix.

I've pushed the patches here:
git://repo.or.cz/qemu/blueswirl.git
http://repo.or.cz/w/qemu/blueswirl.git

Re: [Qemu-devel] Re: [PATCH v2 0/7] APIC/IOAPIC cleanup

[Blue Swirl]

On Sun, Jun 13, 2010 at 5:03 PM, Andreas Färber <andreas.faerber@web.de> wrote:
> Am 13.06.2010 um 18:56 schrieb Jan Kiszka:
>
>> Blue Swirl wrote:
>>>
>>> Clean up APIC and IOAPIC. Convert both devices to qdev.
>>
>> Wanted to give this a try, but patches are line-wrapped. Please fix.
>
> Threading the patches would also be appreciated.
> Git's default changed at some point, this can be overridden in .gitconfig.

I have these lines in $HOME/.gitconfig
[format]
        thread = true

I'm also using --thread flag in git-format-patch command line.

git --version
git version 1.7.1

Re: [Qemu-devel] Re: [PATCH v2 0/7] APIC/IOAPIC cleanup

[Andreas Färber]

Hi,

Am 13.06.2010 um 19:53 schrieb Blue Swirl:

> On Sun, Jun 13, 2010 at 5:03 PM, Andreas Färber <andreas.faerber@web.de 
> > wrote:
>> Am 13.06.2010 um 18:56 schrieb Jan Kiszka:
>>
>>> Blue Swirl wrote:
>>>>
>>>> Clean up APIC and IOAPIC. Convert both devices to qdev.
>>>
>>> Wanted to give this a try, but patches are line-wrapped. Please fix.
>>
>> Threading the patches would also be appreciated.
>> Git's default changed at some point, this can be overridden  
>> in .gitconfig.
>
> I have these lines in $HOME/.gitconfig
> [format]
>        thread = true
>
> I'm also using --thread flag in git-format-patch command line.

git-send-email has --thread and --chain-reply-to options too:
http://www.kernel.org/pub/software/scm/git/docs/git-send-email.html

I've been using --chain-reply-to with no .gitconfig settings  
successfully in 1.7.0.4.

Andreas

Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup

[Anthony Liguori]

On 06/12/2010 04:14 PM, Blue Swirl wrote:
> Clean up APIC and IOAPIC. Convert both devices to qdev.
>
> v1->v2:
> Remove apic.h reorganization.
> Add IOAPIC and APIC qdev conversions.
> Use CPUState also in 5/7. However on 6/7 we have to again use void *
> because of VMState limitations. VMState gurus, please comment.
>    

I'm late to the game here, but I'm not sure converting the APIC to qdev 
makes a lot of sense conceptually.

qdev models devices that exist on a bus, but the local APIC actually 
lives on the processor core.  It's extremely unique in that it actually 
maps a physical memory region different depending on the actual core.  
It really belongs as part of the CPU emulation and not as part of the 
device emulation.

For now, the practical problem is that you can't hotplug a CPU because 
that creates an APIC which lives on the Sysbus which does not allow 
hotplug.  Making sysbus allow hotplug is definitely note the right 
answer though.

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.

> Blue Swirl (7):
>    ioapic: unexport ioapic_set_irq
>    ioapic: convert to qdev
>    apic: avoid passing CPUState from devices
>    apic: avoid passing CPUState from CPU code
>    apic: avoid using CPUState internals
>    apic: convert to qdev
>    apic: qdev conversion cleanup
>
>   hw/apic.c               |  174 +++++++++++++++++++++++-----------------------
>   hw/apic.h               |   21 ++++--
>   hw/ioapic.c             |   47 ++++++++----
>   hw/pc.c                 |   74 ++++++++++++++++++--
>   hw/pc.h                 |    4 +-
>   hw/pc_piix.c            |   19 +++++-
>   qemu-common.h           |    2 +-
>   target-i386/cpu.h       |   28 +++++---
>   target-i386/cpuid.c     |    6 ++
>   target-i386/helper.c    |    4 +-
>   target-i386/kvm.c       |   14 ++--
>   target-i386/op_helper.c |    8 +-
>   12 files changed, 258 insertions(+), 143 deletions(-)
>
>    

Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup

[Blue Swirl]

On Thu, Aug 19, 2010 at 7:33 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 06/12/2010 04:14 PM, Blue Swirl wrote:
>>
>> Clean up APIC and IOAPIC. Convert both devices to qdev.
>>
>> v1->v2:
>> Remove apic.h reorganization.
>> Add IOAPIC and APIC qdev conversions.
>> Use CPUState also in 5/7. However on 6/7 we have to again use void *
>> because of VMState limitations. VMState gurus, please comment.
>>
>
> I'm late to the game here, but I'm not sure converting the APIC to qdev
> makes a lot of sense conceptually.

Very late. I think it makes tons of sense, for example with 'info
qtree' you now see most of the QEMU devices. The CPUs are still
missing.

> qdev models devices that exist on a bus, but the local APIC actually lives
> on the processor core.  It's extremely unique in that it actually maps a
> physical memory region different depending on the actual core.

The bus does not need to have any connection to existence or
non-existence of real buses. In SoCs or ASICs, all devices and buses
may reside inside a chip. For example Sparc32 NCR89C105 contained
several devices, all of which are separate in QEMU. If APICs were
invented in i386 times, they would be separate chips. In NUMA systems
each CPU may see different physical memory layout.

But all we care in QEMU is logical entities.

>  It really
> belongs as part of the CPU emulation and not as part of the device
> emulation.

In that case it should be moved to target-i386.

> For now, the practical problem is that you can't hotplug a CPU because that
> creates an APIC which lives on the Sysbus which does not allow hotplug.
>  Making sysbus allow hotplug is definitely note the right answer though.

Why not?

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

Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup

[Anthony Liguori]

On 08/19/2010 03:09 PM, Blue Swirl wrote:
> On Thu, Aug 19, 2010 at 7:33 PM, Anthony Liguori<anthony@codemonkey.ws>  wrote:
>    
>> On 06/12/2010 04:14 PM, Blue Swirl wrote:
>>      
>>> Clean up APIC and IOAPIC. Convert both devices to qdev.
>>>
>>> v1->v2:
>>> Remove apic.h reorganization.
>>> Add IOAPIC and APIC qdev conversions.
>>> Use CPUState also in 5/7. However on 6/7 we have to again use void *
>>> because of VMState limitations. VMState gurus, please comment.
>>>
>>>        
>> I'm late to the game here, but I'm not sure converting the APIC to qdev
>> makes a lot of sense conceptually.
>>      
> Very late. I think it makes tons of sense, for example with 'info
> qtree' you now see most of the QEMU devices. The CPUs are still
> missing.
>    

Should CPUs appear in the QEMU device tree?

>> qdev models devices that exist on a bus, but the local APIC actually lives
>> on the processor core.  It's extremely unique in that it actually maps a
>> physical memory region different depending on the actual core.
>>      
> The bus does not need to have any connection to existence or
> non-existence of real buses. In SoCs or ASICs, all devices and buses
> may reside inside a chip.

Well, I think this is part of the trouble with the current qdev object 
model.  There are really two distinct types of devices.  There are chips 
that have pins whereas the meaning of those pins are defined by the chip 
itself.  For instance, a UART16650A is a chip that has a well defined 
pin layout.

Then there are buses which typically multiplex signals for many devices 
over a single set of wires.  Usually you need some type of logic that 
decodes the bus signals to the actual chips that sit on the card.

So really, I think this suggests that some devices shouldn't have any 
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.

> For example Sparc32 NCR89C105 contained
> several devices, all of which are separate in QEMU. If APICs were
> invented in i386 times, they would be separate chips. In NUMA systems
> each CPU may see different physical memory layout.
>    

The local APIC is an extreme special case.  There are special CPU 
instructions that return registers from the APIC (cr8 is the APIC TPR).

It's really part of the core and if there aren't objections, I'd like to 
move it to target-i386.

>>   It really
>> belongs as part of the CPU emulation and not as part of the device
>> emulation.
>>      
> In that case it should be moved to target-i386.
>
>    
>> For now, the practical problem is that you can't hotplug a CPU because that
>> creates an APIC which lives on the Sysbus which does not allow hotplug.
>>   Making sysbus allow hotplug is definitely note the right answer though.
>>      
> Why not?
>    

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.

>> 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.  It's like modelling a TLB as a 
separate device.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup

[Blue Swirl]

On Thu, Aug 19, 2010 at 8:49 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 08/19/2010 03:09 PM, Blue Swirl wrote:
>>
>> On Thu, Aug 19, 2010 at 7:33 PM, Anthony Liguori<anthony@codemonkey.ws>
>>  wrote:
>>
>>>
>>> On 06/12/2010 04:14 PM, Blue Swirl wrote:
>>>
>>>>
>>>> Clean up APIC and IOAPIC. Convert both devices to qdev.
>>>>
>>>> v1->v2:
>>>> Remove apic.h reorganization.
>>>> Add IOAPIC and APIC qdev conversions.
>>>> Use CPUState also in 5/7. However on 6/7 we have to again use void *
>>>> because of VMState limitations. VMState gurus, please comment.
>>>>
>>>>
>>>
>>> I'm late to the game here, but I'm not sure converting the APIC to qdev
>>> makes a lot of sense conceptually.
>>>
>>
>> Very late. I think it makes tons of sense, for example with 'info
>> qtree' you now see most of the QEMU devices. The CPUs are still
>> missing.
>>
>
> Should CPUs appear in the QEMU device tree?

They have several properties that should be user visible.

>>> qdev models devices that exist on a bus, but the local APIC actually
>>> lives
>>> on the processor core.  It's extremely unique in that it actually maps a
>>> physical memory region different depending on the actual core.
>>>
>>
>> The bus does not need to have any connection to existence or
>> non-existence of real buses. In SoCs or ASICs, all devices and buses
>> may reside inside a chip.
>
> Well, I think this is part of the trouble with the current qdev object
> model.  There are really two distinct types of devices.  There are chips
> that have pins whereas the meaning of those pins are defined by the chip
> itself.  For instance, a UART16650A is a chip that has a well defined pin
> layout.
>
> Then there are buses which typically multiplex signals for many devices over
> a single set of wires.  Usually you need some type of logic that decodes the
> bus signals to the actual chips that sit on the card.
>
> So really, I think this suggests that some devices shouldn't have any
> 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.

>> For example Sparc32 NCR89C105 contained
>> several devices, all of which are separate in QEMU. If APICs were
>> invented in i386 times, they would be separate chips. In NUMA systems
>> each CPU may see different physical memory layout.
>>
>
> The local APIC is an extreme special case.  There are special CPU
> instructions that return registers from the APIC (cr8 is the APIC TPR).
>
> It's really part of the core and if there aren't objections, I'd like to
> move it to target-i386.

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.

>>>  It really
>>> belongs as part of the CPU emulation and not as part of the device
>>> emulation.
>>>
>>
>> In that case it should be moved to target-i386.
>>
>>
>>>
>>> For now, the practical problem is that you can't hotplug a CPU because
>>> that
>>> creates an APIC which lives on the Sysbus which does not allow hotplug.
>>>  Making sysbus allow hotplug is definitely note the right answer though.
>>>
>>
>> Why not?
>>
>
> 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.

>>> 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?

>  It's like modelling a TLB as a
> separate device.

That has surely happened in ancient times.

Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup

[Anthony Liguori]

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.

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

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.

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

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.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup

[malc]

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

Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup

[Anthony Liguori]

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

[Qemu-devel] Re: [PATCH v2 0/7] APIC/IOAPIC cleanup

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup

[malc]

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

[Qemu-devel] Re: [PATCH v2 0/7] APIC/IOAPIC cleanup

[Jan Kiszka]

Anthony Liguori wrote:
> On 08/19/2010 03:09 PM, Blue Swirl wrote:
>> On Thu, Aug 19, 2010 at 7:33 PM, Anthony
>> Liguori<anthony@codemonkey.ws>  wrote:
>>   
>>> On 06/12/2010 04:14 PM, Blue Swirl wrote:
>>>     
>>>> Clean up APIC and IOAPIC. Convert both devices to qdev.
>>>>
>>>> v1->v2:
>>>> Remove apic.h reorganization.
>>>> Add IOAPIC and APIC qdev conversions.
>>>> Use CPUState also in 5/7. However on 6/7 we have to again use void *
>>>> because of VMState limitations. VMState gurus, please comment.
>>>>
>>>>        
>>> I'm late to the game here, but I'm not sure converting the APIC to qdev
>>> makes a lot of sense conceptually.
>>>      
>> Very late. I think it makes tons of sense, for example with 'info
>> qtree' you now see most of the QEMU devices. The CPUs are still
>> missing.
>>    
> 
> Should CPUs appear in the QEMU device tree?

Yes, we want to browse/display the whole virtual machine (one day). QMP
could then avoid an equivalent of "info registers" and expose the CPU
state via dumping qdev devices.

And note the the APIC was not part of the CPU core before Pentiums, and
we support that model as well.

Therefore, I like the idea of having a hotplug-capable (where
appropriate) CPU bus.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup

[Markus Armbruster]

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?

Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup

[Anthony Liguori]

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

Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup

[Blue Swirl]

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.

Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup

[Avi Kivity]

  On 08/19/2010 10:33 PM, Anthony Liguori wrote:
> On 06/12/2010 04:14 PM, Blue Swirl wrote:
>> Clean up APIC and IOAPIC. Convert both devices to qdev.
>>
>> v1->v2:
>> Remove apic.h reorganization.
>> Add IOAPIC and APIC qdev conversions.
>> Use CPUState also in 5/7. However on 6/7 we have to again use void *
>> because of VMState limitations. VMState gurus, please comment.
>
> I'm late to the game here, but I'm not sure converting the APIC to 
> qdev makes a lot of sense conceptually.
>
> qdev models devices that exist on a bus, but the local APIC actually 
> lives on the processor core.  It's extremely unique in that it 
> actually maps a physical memory region different depending on the 
> actual core.  It really belongs as part of the CPU emulation and not 
> as part of the device emulation.

The local APIC connects on one side to the processor core, and on the 
other side to the APIC bus (the system bus on modern processors).

>
> For now, the practical problem is that you can't hotplug a CPU because 
> that creates an APIC which lives on the Sysbus which does not allow 
> hotplug.  Making sysbus allow hotplug is definitely note the right 
> answer though.

Why not?

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

Every device connects to something else; otherwise you could optimize it 
out.  Currently the "something else" has to be a bus, which to me 
implies a 1:many connection.  Certainly it's true in the case of the 
APIC or the CPU.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup

[Avi Kivity]

  On 08/19/2010 11:49 PM, Anthony Liguori wrote:
>> The bus does not need to have any connection to existence or
>> non-existence of real buses. In SoCs or ASICs, all devices and buses
>> may reside inside a chip.
>
> Well, I think this is part of the trouble with the current qdev object 
> model.  There are really two distinct types of devices.  There are 
> chips that have pins whereas the meaning of those pins are defined by 
> the chip itself.  For instance, a UART16650A is a chip that has a well 
> defined pin layout.
>
> Then there are buses which typically multiplex signals for many 
> devices over a single set of wires.  Usually you need some type of 
> logic that decodes the bus signals to the actual chips that sit on the 
> card.
>
> So really, I think this suggests that some devices shouldn't have any 
> 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.

I don't think we want to model individual resistors on a serial card as 
separate qdev objects.  We want the serial card itself to be a qdev (as 
it is a hotpluggable entity) and the individual serial interfaces on 
that card (as they are duplicates of each other and of interest to the 
user).

>
>> For example Sparc32 NCR89C105 contained
>> several devices, all of which are separate in QEMU. If APICs were
>> invented in i386 times, they would be separate chips. In NUMA systems
>> each CPU may see different physical memory layout.
>
> The local APIC is an extreme special case.  There are special CPU 
> instructions that return registers from the APIC (cr8 is the APIC TPR).

It's a special case, but nothing is extreme about it.  Hardware often 
breaks abstraction layers.  For example, on newer systems the memory 
controller is programmed using MSRs.  This affects not just the cpu, but 
also devices writing to memory that is attached to that cpu.

>>
>>> For now, the practical problem is that you can't hotplug a CPU 
>>> because that
>>> creates an APIC which lives on the Sysbus which does not allow hotplug.
>>>   Making sysbus allow hotplug is definitely note the right answer 
>>> though.
>> Why not?
>
> 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.

Even on PCI, some devices will be hot-pluggable and some won't be.

>
>>> 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.  It's like modelling a 
> TLB as a separate device.

As was mentioned, all three exist.  Some processors had the MMU as a 
separate chip.  Even the FPU was once a separate chip, so you not only 
had instructions reach out to state in another device, you had another 
device actually executing instructions on behalf of a cpu.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup

[Anthony Liguori]

On 08/22/2010 04:37 AM, Avi Kivity wrote:
>  On 08/19/2010 11:49 PM, Anthony Liguori wrote:
>>> The bus does not need to have any connection to existence or
>>> non-existence of real buses. In SoCs or ASICs, all devices and buses
>>> may reside inside a chip.
>>
>> Well, I think this is part of the trouble with the current qdev 
>> object model.  There are really two distinct types of devices.  There 
>> are chips that have pins whereas the meaning of those pins are 
>> defined by the chip itself.  For instance, a UART16650A is a chip 
>> that has a well defined pin layout.
>>
>> Then there are buses which typically multiplex signals for many 
>> devices over a single set of wires.  Usually you need some type of 
>> logic that decodes the bus signals to the actual chips that sit on 
>> the card.
>>
>> So really, I think this suggests that some devices shouldn't have any 
>> 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.
>
> I don't think we want to model individual resistors on a serial card 
> as separate qdev objects.  We want the serial card itself to be a qdev 
> (as it is a hotpluggable entity) and the individual serial interfaces 
> on that card (as they are duplicates of each other and of interest to 
> the user).

You're missing the fundamental problem which arises because we've 
introduced an object model without thinking through how devices ought to 
be modelled.

All devices should have a DeviceState associated with them.  Otherwise, 
there's really no point in having qdev at all.

We have lots of devices today that don't have DeviceState's associated 
with them because the have a separate qdev representation with a 
reference to the non-DeviceState object.

We have non-DeviceState objects because otherwise we end up with an 
inheritance diamond.  We have this problem because we want to have 
relationships like: DeviceState <- SystemDeviceState <- ISADevice <- 
ISASerialDevice.

But ISASerialDevice is not the only type of serial device.  You can also 
have a SystemSerialDevice that's directly attached to the System bus.  
That means you'd have to have:

SerialDevice -> ISASerialDevice -> SystemDeviceState -> DeviceState
                      -> SystemSerialDevice -> SystemDeviceState -> 
DeviceState

Which is a classic MI diamond.  The only way to resolve this modelling 
problem is to split out the common code and rely on a has-a relationship 
instead of an is-a.  That gives you:

ISASerialDevice->SystemDeviceState->DeviceState
SystemSerialDevice->SystemDeviceState->DeviceState

ISASerialDevice has-a SerialDevice
SystemSerialDevice has-a SerialDevice

And since we want SerialDevice inherit from a DeviceState (recall, all 
devices should have DeviceStates):

SerialDevice->DeviceState

No more MI diamond and all devices have DeviceStates.  Coincidentally, 
it matches more closely how hardware works..

Generally speaking, any time we have one device that needs to sit on 
multiple busses, we're going to have to model it in this fashion.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup

[Avi Kivity]

  On 08/22/2010 09:52 PM, Anthony Liguori wrote:
>>> So really, I think this suggests that some devices shouldn't have 
>>> any 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.
>>
>> I don't think we want to model individual resistors on a serial card 
>> as separate qdev objects.  We want the serial card itself to be a 
>> qdev (as it is a hotpluggable entity) and the individual serial 
>> interfaces on that card (as they are duplicates of each other and of 
>> interest to the user).
>
>
> You're missing the fundamental problem which arises because we've 
> introduced an object model without thinking through how devices ought 
> to be modelled.
>
> All devices should have a DeviceState associated with them.  
> Otherwise, there's really no point in having qdev at all.
>
> We have lots of devices today that don't have DeviceState's associated 
> with them because the have a separate qdev representation with a 
> reference to the non-DeviceState object.
>
> We have non-DeviceState objects because otherwise we end up with an 
> inheritance diamond.  We have this problem because we want to have 
> relationships like: DeviceState <- SystemDeviceState <- ISADevice <- 
> ISASerialDevice.
>
> But ISASerialDevice is not the only type of serial device.  You can 
> also have a SystemSerialDevice that's directly attached to the System 
> bus.  That means you'd have to have:
>
> SerialDevice -> ISASerialDevice -> SystemDeviceState -> DeviceState
>                      -> SystemSerialDevice -> SystemDeviceState -> 
> DeviceState
>
> Which is a classic MI diamond.  The only way to resolve this modelling 
> problem is to split out the common code and rely on a has-a 
> relationship instead of an is-a.  That gives you:
>
> ISASerialDevice->SystemDeviceState->DeviceState
> SystemSerialDevice->SystemDeviceState->DeviceState
>
> ISASerialDevice has-a SerialDevice
> SystemSerialDevice has-a SerialDevice
>
> And since we want SerialDevice inherit from a DeviceState (recall, all 
> devices should have DeviceStates):
>
> SerialDevice->DeviceState
>
> No more MI diamond and all devices have DeviceStates.  Coincidentally, 
> it matches more closely how hardware works..
>

Well, I agree, but I honestly lost the context.  How does this relate to 
the APIC and cpu hotplug?

I'll take the opportunity to say that we should be using a language that 
has first-class (...) support for these concepts instead of having to 
divine them from the code.

> Generally speaking, any time we have one device that needs to sit on 
> multiple busses, we're going to have to model it in this fashion.

We'll just have to address them one by one then.  Perhaps if many come 
up we can try a generic solution.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup

[Anthony Liguori]

On 08/22/2010 02:44 PM, Avi Kivity wrote:
>> No more MI diamond and all devices have DeviceStates.  
>> Coincidentally, it matches more closely how hardware works..
>>
>
>
> Well, I agree, but I honestly lost the context.  How does this relate 
> to the APIC and cpu hotplug?

My original assertion was that the local APIC is not a DeviceState, but 
rather it's a CPU feature.

If you look at some of the magic that apic.c has to do in the IO 
callbacks, it should be clear that it's special.  In the not too distant 
future, I'd like to move apic.c to target-i386.  There should be no need 
to explicitly instantiate it when you instantiate a CPU.

BTW, this gets a bit funky with KVM.  If we declare that the lapic is 
part of the CPU, then in an ideal world we would have interfaces for the 
I/O APIC as part of the KVM interface so we could implement the I/O APIC 
in userspace with the lapic implemented in the kernel.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup

[Avi Kivity]

  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.

Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup

[Avi Kivity]

  On 08/22/2010 11:03 PM, Anthony Liguori wrote:
> On 08/22/2010 02:44 PM, Avi Kivity wrote:
>>> No more MI diamond and all devices have DeviceStates.  
>>> Coincidentally, it matches more closely how hardware works..
>>>
>>
>>
>> Well, I agree, but I honestly lost the context.  How does this relate 
>> to the APIC and cpu hotplug?
>
> My original assertion was that the local APIC is not a DeviceState, 
> but rather it's a CPU feature.
>
> If you look at some of the magic that apic.c has to do in the IO 
> callbacks, it should be clear that it's special. 

It's special in that it is connected to a cpu core.  So's the RTL8139 
device, on one hand connected to a PCI bus, on the other hand connected 
to a PHY (netdev in qemu).

> In the not too distant future, I'd like to move apic.c to 
> target-i386.  There should be no need to explicitly instantiate it 
> when you instantiate a CPU.

But then there's a need explicitly not to instantiate it when using -isapc.

>
> BTW, this gets a bit funky with KVM.  If we declare that the lapic is 
> part of the CPU, then in an ideal world we would have interfaces for 
> the I/O APIC as part of the KVM interface so we could implement the 
> I/O APIC in userspace with the lapic implemented in the kernel.

We discussed that once (or rather, twice), but the conclusion was it was 
too much work for the gain, especially as we'd have to keep supporting 
the old interface forever.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup

[Anthony Liguori]

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

Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup

[Anthony Liguori]

On 08/22/2010 03:33 PM, Avi Kivity wrote:
>  On 08/22/2010 11:03 PM, Anthony Liguori wrote:
>> On 08/22/2010 02:44 PM, Avi Kivity wrote:
>>>> No more MI diamond and all devices have DeviceStates.  
>>>> Coincidentally, it matches more closely how hardware works..
>>>>
>>>
>>>
>>> Well, I agree, but I honestly lost the context.  How does this 
>>> relate to the APIC and cpu hotplug?
>>
>> My original assertion was that the local APIC is not a DeviceState, 
>> but rather it's a CPU feature.
>>
>> If you look at some of the magic that apic.c has to do in the IO 
>> callbacks, it should be clear that it's special. 
>
> It's special in that it is connected to a cpu core.  So's the RTL8139 
> device, on one hand connected to a PCI bus, on the other hand 
> connected to a PHY (netdev in qemu).
>
>> In the not too distant future, I'd like to move apic.c to 
>> target-i386.  There should be no need to explicitly instantiate it 
>> when you instantiate a CPU.
>
> But then there's a need explicitly not to instantiate it when using 
> -isapc.

No.  isapc has nothing to do with whether a CPU has a local APIC.

You don't instantiate the local APIC if you create a 486 CPU.  If you 
create an isapc with a pentium processor, it's going to have a local 
APIC although it's irrelevant as it's fully compatible with the 486.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup

[Anthony Liguori]

On 08/22/2010 02:44 PM, Avi Kivity wrote:
>> No more MI diamond and all devices have DeviceStates.  
>> Coincidentally, it matches more closely how hardware works..
>>
>
>
> Well, I agree, but I honestly lost the context.  How does this relate 
> to the APIC and cpu hotplug?
>
> I'll take the opportunity to say that we should be using a language 
> that has first-class (...) support for these concepts instead of 
> having to divine them from the code.

At the risk of sounding uncool, I think what we really need is a UML 
diagram describing the device tree.  It's really not that much more 
difficult to create a similar mess in C++.

Regards,

Anthony Liguori

>> Generally speaking, any time we have one device that needs to sit on 
>> multiple busses, we're going to have to model it in this fashion.
>
> We'll just have to address them one by one then.  Perhaps if many come 
> up we can try a generic solution.
>

Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup

[Avi Kivity]

  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.

Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup

[Avi Kivity]

  On 08/23/2010 12:07 AM, Anthony Liguori wrote:
> On 08/22/2010 02:44 PM, Avi Kivity wrote:
>>> No more MI diamond and all devices have DeviceStates.  
>>> Coincidentally, it matches more closely how hardware works..
>>>
>>
>>
>> Well, I agree, but I honestly lost the context.  How does this relate 
>> to the APIC and cpu hotplug?
>>
>> I'll take the opportunity to say that we should be using a language 
>> that has first-class (...) support for these concepts instead of 
>> having to divine them from the code.
>
> At the risk of sounding uncool, I think what we really need is a UML 
> diagram describing the device tree.  It's really not that much more 
> difficult to create a similar mess in C++.
>

That's fine near a whiteboard with some airspace for handwaving.  
Doesn't work well through patches and email.

The advantage of better language support is that it's easier to 
resurrect the model from the code; it doesn't give you a better or worse 
model by itself.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup

[Avi Kivity]

  On 08/23/2010 12:06 AM, Anthony Liguori wrote:
> On 08/22/2010 03:33 PM, Avi Kivity wrote:
>>  On 08/22/2010 11:03 PM, Anthony Liguori wrote:
>>> On 08/22/2010 02:44 PM, Avi Kivity wrote:
>>>>> No more MI diamond and all devices have DeviceStates.  
>>>>> Coincidentally, it matches more closely how hardware works..
>>>>>
>>>>
>>>>
>>>> Well, I agree, but I honestly lost the context.  How does this 
>>>> relate to the APIC and cpu hotplug?
>>>
>>> My original assertion was that the local APIC is not a DeviceState, 
>>> but rather it's a CPU feature.
>>>
>>> If you look at some of the magic that apic.c has to do in the IO 
>>> callbacks, it should be clear that it's special. 
>>
>> It's special in that it is connected to a cpu core.  So's the RTL8139 
>> device, on one hand connected to a PCI bus, on the other hand 
>> connected to a PHY (netdev in qemu).
>>
>>> In the not too distant future, I'd like to move apic.c to 
>>> target-i386.  There should be no need to explicitly instantiate it 
>>> when you instantiate a CPU.
>>
>> But then there's a need explicitly not to instantiate it when using 
>> -isapc.
>
> No.  isapc has nothing to do with whether a CPU has a local APIC.
>
> You don't instantiate the local APIC if you create a 486 CPU.  If you 
> create an isapc with a pentium processor, it's going to have a local 
> APIC although it's irrelevant as it's fully compatible with the 486.
>

Okay, okay.  "But then there's a need explicitly not to instantiate it 
when modelling a 486 or lower".


-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

[Qemu-devel] Re: [PATCH v2 0/7] APIC/IOAPIC cleanup

[Jan Kiszka]

Avi Kivity wrote:
>  On 08/23/2010 12:06 AM, Anthony Liguori wrote:
>> On 08/22/2010 03:33 PM, Avi Kivity wrote:
>>>  On 08/22/2010 11:03 PM, Anthony Liguori wrote:
>>>> On 08/22/2010 02:44 PM, Avi Kivity wrote:
>>>>>> No more MI diamond and all devices have DeviceStates. 
>>>>>> Coincidentally, it matches more closely how hardware works..
>>>>>>
>>>>>
>>>>>
>>>>> Well, I agree, but I honestly lost the context.  How does this
>>>>> relate to the APIC and cpu hotplug?
>>>>
>>>> My original assertion was that the local APIC is not a DeviceState,
>>>> but rather it's a CPU feature.
>>>>
>>>> If you look at some of the magic that apic.c has to do in the IO
>>>> callbacks, it should be clear that it's special. 
>>>
>>> It's special in that it is connected to a cpu core.  So's the RTL8139
>>> device, on one hand connected to a PCI bus, on the other hand
>>> connected to a PHY (netdev in qemu).
>>>
>>>> In the not too distant future, I'd like to move apic.c to
>>>> target-i386.  There should be no need to explicitly instantiate it
>>>> when you instantiate a CPU.
>>>
>>> But then there's a need explicitly not to instantiate it when using
>>> -isapc.
>>
>> No.  isapc has nothing to do with whether a CPU has a local APIC.
>>
>> You don't instantiate the local APIC if you create a 486 CPU.  If you
>> create an isapc with a pentium processor, it's going to have a local
>> APIC although it's irrelevant as it's fully compatible with the 486.
>>
> 
> Okay, okay.  "But then there's a need explicitly not to instantiate it
> when modelling a 486 or lower".

...plus the need to instantiate it (as a dedicated device) when modeling
486 SMP.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [PATCH v2 0/7] APIC/IOAPIC cleanup

[Avi Kivity]

  On 08/23/2010 12:09 PM, Jan Kiszka wrote:
>
>> Okay, okay.  "But then there's a need explicitly not to instantiate it
>> when modelling a 486 or lower".
> ...plus the need to instantiate it (as a dedicated device) when modeling
> 486 SMP.

In short, if we want our model to be perfect, we have to fix reality first.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] Re: [PATCH v2 0/7] APIC/IOAPIC cleanup

[Alexander Graf]

On 23.08.2010, at 11:25, Avi Kivity wrote:

> On 08/23/2010 12:09 PM, Jan Kiszka wrote:
>> 
>>> Okay, okay.  "But then there's a need explicitly not to instantiate it
>>> when modelling a 486 or lower".
>> ...plus the need to instantiate it (as a dedicated device) when modeling
>> 486 SMP.
> 
> In short, if we want our model to be perfect, we have to fix reality first.

How about we don't care about the 486 case?


Alex

Re: [Qemu-devel] Re: [PATCH v2 0/7] APIC/IOAPIC cleanup

[Avi Kivity]

  On 08/23/2010 01:11 PM, Alexander Graf wrote:
> On 23.08.2010, at 11:25, Avi Kivity wrote:
>
>> On 08/23/2010 12:09 PM, Jan Kiszka wrote:
>>>> Okay, okay.  "But then there's a need explicitly not to instantiate it
>>>> when modelling a 486 or lower".
>>> ...plus the need to instantiate it (as a dedicated device) when modeling
>>> 486 SMP.
>> In short, if we want our model to be perfect, we have to fix reality first.
> How about we don't care about the 486 case?
>

I certainly don't, but others may.

However, the problem remains: every time real hardware doesn't fit our 
pretty model we'll drop support for that hardware?

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] Re: [PATCH v2 0/7] APIC/IOAPIC cleanup

[Alexander Graf]

On 23.08.2010, at 12:15, Avi Kivity wrote:

> On 08/23/2010 01:11 PM, Alexander Graf wrote:
>> On 23.08.2010, at 11:25, Avi Kivity wrote:
>> 
>>> On 08/23/2010 12:09 PM, Jan Kiszka wrote:
>>>>> Okay, okay.  "But then there's a need explicitly not to instantiate it
>>>>> when modelling a 486 or lower".
>>>> ...plus the need to instantiate it (as a dedicated device) when modeling
>>>> 486 SMP.
>>> In short, if we want our model to be perfect, we have to fix reality first.
>> How about we don't care about the 486 case?
>> 
> 
> I certainly don't, but others may.
> 
> However, the problem remains: every time real hardware doesn't fit our pretty model we'll drop support for that hardware?

No, every time real hardware doesn't fit out pretty model and nobody really cares, we don't care.

Seriously - would you want to start off modeling everything so that EISA and MCA fit in too?


Alex

Re: [Qemu-devel] Re: [PATCH v2 0/7] APIC/IOAPIC cleanup

[Avi Kivity]

  On 08/23/2010 01:18 PM, Alexander Graf wrote:
>
>> I certainly don't, but others may.
>>
>> However, the problem remains: every time real hardware doesn't fit our pretty model we'll drop support for that hardware?
> No, every time real hardware doesn't fit out pretty model and nobody really cares, we don't care.
>
> Seriously - would you want to start off modeling everything so that EISA and MCA fit in too?

Look how this developed:

1. apic.c has an incestuous relationship with the cpu
2. let's make them one and the same
3. but in real life they're not one and the same in some cases
4. including some we support
5. let's drop support for those

vs

1. hardware isn't a tree
2. we have to live with that

Dropping support for 486 is fine, but let's not do it because we want to 
model hardware in a way that's different from reality

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup

[Anthony Liguori]

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

Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup

[Avi Kivity]

  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

[Anthony Liguori]

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

Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup

[Avi Kivity]

  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

Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup

[Anthony Liguori]

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

Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup

[Avi Kivity]

  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

Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup

[Anthony Liguori]

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

Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup

[Markus Armbruster]

You lost me.  A few messages upthread.

What's the *practical* problem again?

Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup

[Avi Kivity]

  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

Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup

[Anthony Liguori]

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

Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup

[Anthony Liguori]

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

Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup

[Markus Armbruster]

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?

Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup

[Anthony Liguori]

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

[Qemu-devel] Re: [PATCH v2 0/7] APIC/IOAPIC cleanup

[Jan Kiszka]

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

[Qemu-devel] Re: [PATCH v2 0/7] APIC/IOAPIC cleanup

[Anthony Liguori]

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

Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup

[Avi Kivity]

  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