Re: [Qemu-devel] [PATCH 07/12] target-i386: Add ICC_BUS and attach apic, kvmvapic and cpu to it

[Igor Mammedov]

On Thu, 04 Apr 2013 13:10:54 +0200
Andreas Färber <address@hidden> wrote:

> Am 28.03.2013 11:55, schrieb Igor Mammedov:
> > On Wed, 27 Mar 2013 11:57:53 +0100
> > Paolo Bonzini <address@hidden> wrote:
> > 
> >> Il 21/03/2013 15:28, Igor Mammedov ha scritto:  
> >>> +static BusState *icc_bus;
> >>> +
> >>> +BusState *get_icc_bus(void)
> >>> +{
> >>> +    if (icc_bus == NULL) {
> >>> +        icc_bus = g_malloc0(icc_bus_info.instance_size);
> >>> +        qbus_create_inplace(icc_bus, TYPE_ICC_BUS, NULL, "icc-bus");
> >>> +        icc_bus->allow_hotplug = 1;
> >>> +        OBJECT(icc_bus)->free = g_free;  
> >>
> >> You can just use qbus_create.
> >>   
> >>> +        object_property_add_child(container_get(qdev_get_machine(),
> >>> +                                                "/unattached"),  
> >>
> >> Please put it straight under /machine, not /unattached.  
> > 
> > sure
> > 
> >>
> >> But actually, you lack a device that instantiates the bus.  That device
> >> would be a simple container device similar hw/a15mpcore.c, and would  
> > 
> > About a year ago something like that device was proposed "cpu_sockets", but
> > there was suggestion to drop it:
> > http://lists.gnu.org/archive/html/qemu-devel/2012-02/msg02157.html
> > So I've opted in favor of parent-less bus, but I could create it
> > explicitly at board level as you suggest.
> > 
> > However having parent sysbus device for icc-bus will allow to see it via
> > monitor info qtree
> 
> As I said on IRC, info qtree should not influence our design decisions.
> All that's missing is a convenient script that pretty-prints the output
> from the QOM graph using qom-list and qom-get in a non-interactive way.
> Whether CPUs or busses are shown in info qtree is totally irrelevant IMO
> by now.
> 
> > and reset on SysBus could be propagated through it as well,
> > it may be good to add it later.
> 
> What exactly is this ICC bus needed for? Is it just a workaround for
> qdev_init() or something making assumptions about busses and falling
> back to SysBus otherwise? Or about avoiding SysBus?
SysBus doesn't allow hotplug on it, I've asked Anthony on IRC and short answer
was do not touch what doesn't exists. Hence hotplug-able bus that exists
on real hw.
x86 hw had ICC bus some time ago through which APICs, IOAPICs and CPUs
communicated. So here it is.

More close to the code:
 - device_add assigns SysBus to any bus-less device, therefore CPU hits
   assertion because hotplug is not allowed on bus.
 - when x86 CPU is realizing, it creates APIC and possible kvmvapic, which are
   SysBus devices presently, it triggers the same assert as above.

> 
> And where would it sit if be hosted you think of something like Qseven
> modules - on the board or on the module? I don't see pc.c being touched
> here at all...
I've reworked it quite a bit to Paolo's suggestions. now board creates
following QOM tree:
  /machine/icc-bridge<SYS_BUS_DEVICE>/--->child<icc-bus>/->link<apic[..]>
                                       |                |   
                                       |                 ->link<kvmvapic[..]>
                                       |                |
                                       |                 ->link<cpu[..]
                                       |                |
                                       |                 ->link<ioapic>
                                        ->child<ioapic>

where icc-bridge is created by board and serves as a parent, which creates
icc-bus, ioapic and takes care about mapping memory regions registered by
*apic* devices.

>

> 
> Andreas
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg


-- 
Regards,
  Igor