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

[Markus Armbruster <armbru@redhat.com>]

Actually [PATCH v3].

Alexander Graf <agraf@suse.de> writes:

> When we have 2 separate qdev devices that both create a qbus of the
> same type without specifying a bus name or device name, we end up
> with two buses of the same name, such as ide.0 on the Mac machines:
>
>   dev: macio-ide, id ""
>     bus: ide.0
>       type IDE
>   dev: macio-ide, id ""
>     bus: ide.0
>       type IDE
>
> If we now spawn a device that connects to a ide.0 the last created
> bus gets the device, with the first created bus inaccessible to the
> command line.
>
> After some discussion on IRC we concluded that the best quick fix way
> forward for this is to make automated bus-class type based allocation
> count a global counter. That's what this patch implements. With this
> we instead get
>
>   dev: macio-ide, id ""
>     bus: ide.1
>       type IDE
>   dev: macio-ide, id ""
>     bus: ide.0
>       type IDE
>
> on the example mentioned above.
>
> This also means that if you did -device ...,bus=ide.0 you got a device
> on the first bus (the last created one) before this patch and get that
> device on the second one (the first created one) now.

Please add:

    This breaks migration unless you change bus=ide.0 to bus=ide.1 on
    the destination.

Should be mentioned in release notes.  Do we have a place where we
collect release notes as we go?

> This is intended and makes the bus enumeration work as expected.
>
> As per review request follows a list of otherwise affected boards and
> the reasoning for the conclusion that they are ok:
>
>    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.

Yes.  Bus ID "i2c-bus.0" isn't exposed to the user or the guest, so
changing its interpretation is harmless.

>    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
>
> -> Migration drivers need to access virtio-mmio-bus.4/32 rather
>    than .0 on the destination from old->new. Bugfix as it allows
>    coldplug for specific buses.

"from old->new"?  Do you mean "for old->new"?

Suggest

    -> Makes -device bus= work for all virtio-mmio buses.  Breaks
       migration.  Workaround for migration from old to new: specify
       virtio-mmio-bus.4 or .32 respectively rather than .0 on the
       destination.

>    aarch64     xilinx-zynq-a9  usb-bus.0           2
>    arm         xilinx-zynq-a9  usb-bus.0           2
>    mips64el    fulong2e        usb-bus.0           2
>
> -> Normal USB operation not affected. Migration driver needs command
>    line to use the other bus.
>
>    i386        isapc           ide.0               2
>    x86_64      isapc           ide.0               2
>
> -> Fix part of this patch.

Err, what's part of this patch is adapting pc_init1() for the changed
bus name, so it doesn't break.  No need to mention what you're not
breaking.  But do mention what you're breaking: migration with
bus=ide.0, exactly like in your Mac example.

Please replace by something like this:

    -> Makes -device bus= work for all IDE buses.  Breaks migration.
       Workaround for migration from old to new: specify ide.1 rather
       than ide.0 on the destination.

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

Are you sure?  I believe these are affected exactly like isapc.

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

Again, just like isapc.

> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Anthony Liguori <aliguori@amazon.com>
> Signed-off-by: Alexander Graf <agraf@suse.de>

Patch looks good.