Re: [Qemu-devel] [PATCH v2] qdev: Keep global allocation counter per bus

[Markus Armbruster]

Paolo Bonzini <address@hidden> writes:

> Il 08/01/2014 15:35, Markus Armbruster ha scritto:
>> Paolo Bonzini <address@hidden> writes:
>> 
>>> Leaving only those that will be affected by the patch:
>> 
>> You omitted akita, borzoi, connex, mainstone, nuri, smdkc210, spitz,
>> terrier, tosa, verdex, z2, s390-virtio.  Why won't they be affected?
>
> Because the dup bus names are hardcoded in the board:
>
>     i2cbus = i2c_init_bus(dev, "dummy");
>
> or in the device:
>
>     s->spi = g_new(SSIBus *, s->num_busses);
>     for (i = 0; i < s->num_busses; ++i) {
>         char bus_name[16];
>         snprintf(bus_name, 16, "spi%d", i);
>         s->spi[i] = ssi_create_bus(dev, bus_name);
>     }
>
> Only dups of the "xyzzy.NN" form have their bus names created by qdev
> core.  For other buses this patch changes nothing (neither for better,
> nor for worse).

Ah, right.

These duplicates are just as wrong.  But demanding everything gets fixed
usually gets us nothing fixed, so I'm not going to do that.

>> You also omitted the machines that I can't get to start, but I'm not
>> overly worried by them, because they're all either Xen, where I don't
>> expect differences to plain x86, or ppcemb, where Alex gets to clean up
>> any mess he might make.
>
> Right.  In particular xenfv is just a PIIX PC, plus the (non qdev) Xen
> PV bus.  And xenpv is just the Xen PV bus.
>
>>> Il 07/01/2014 18:34, Markus Armbruster ha scritto:
>>>>     target      machine         bus id              times
>>>>     aarch64     n800            i2c-bus.0           2
>>>>     aarch64     n810            i2c-bus.0           2
>>>>     arm         n800            i2c-bus.0           2
>>>>     arm         n810            i2c-bus.0           2
>>>
>>> Devices are created explicitly on one of the two buses, using
>>> s->mpu->i2c[0], so no change to the guest.
>>>
>>>>     aarch64     vexpress-a15    virtio-mmio-bus.0   4
>>>>     aarch64     vexpress-a9     virtio-mmio-bus.0   4
>>>>     aarch64     virt            virtio-mmio-bus.0   32
>>>>     arm         vexpress-a15    virtio-mmio-bus.0   4
>>>>     arm         vexpress-a9     virtio-mmio-bus.0   4
>>>>     arm         virt            virtio-mmio-bus.0   32
>>>
>>> With Alex's patch we get the ability to plug the device in a particular
>>> slot.  If anyone was using virtio-mmio-bus.0 explicitly, they get the
>>> first slot instead of the 4th or 32nd.  Bugfix.
>> 
>> Doesn't this break migration?  If yes, do we care?
>
> I don't know for sure, but probably not.  sysbus doesn't implement
> get_dev_path, so it relies on the old instance_id mechanism to
> distinguish devices.  instance_id is unreliable in general (e.g. with
> hotplug), but for command-lines and no hot-plug/hot-unplug it should
> work.  You do have to be careful and specify bus=virtio-mmio-bus.31 on
> the destination if you used bus=virtio-mmio-bus.0 on the source.

That's enough breakage to require documenting the issue in the commit
message, and possibly release notes.  Can be done as part of a single
list of affected machines and buses, with a general note on how the
interpretation of bus=FOO.0 changes, how that can affect migration, and
a hint on possible work-arounds.

> BTW if you didn't use bus=virtio-mmio-bus.0, nothing changes because the
> logic in qbus_find_recursive is unaffected.
>
>>>>     mips        mips            ide.0               2
>>>>     mips64      mips            ide.0               2
>>>>     mips64el    mips            ide.0               2
>>>>     mipsel      mips            ide.0               2
>>>
>>> Not affected, the bus is not stored anywhere.
>> 
>> Isn't command line use and migration affected, just like everywhere
>> else?
>
> Right, command-line use of ide.0.  Bugfix as in Alex's PPC case, because
> makes everything else consistent with PCI IDE which is the only place
> where bus=ide.N worked.
>
> Migration is not affected unless you used ide.0 on the command line.  In
> other words, migration from old "-drive if=ide,bus=N" to new "-drive
> if=none ... -device ...,bus=ide.N" should work.
>
>>>>     ppc         g3beige         ide.0               2
>>>>     ppc         mac99           ide.0               2
>>>>     ppc         prep            ide.0               2
>>>>     ppc64       g3beige         ide.0               2
>>>>     ppc64       mac99           ide.0               2
>>>>     ppc64       prep            ide.0               2
>>>
>>> Trusting Alex's tests here.
>> 
>> Our analysis should be recorded in the commit message.  With that done,
>> I could R-by the patch.
>
> Alex, can you spin v3 with a new commit message?

Yes, please.