Re: [Qemu-devel] [PATCH 01/16] qdev: fix hot-unplug

[Anthony Liguori]

On 02/03/2012 10:57 AM, Alexander Graf wrote:
> On 03.02.2012, at 17:37, Anthony Liguori<address@hidden>  wrote:
>
> > On 02/02/2012 01:07 PM, Alexander Graf wrote:
> > > On 02.02.2012, at 20:01, Anthony Liguori wrote:
> > >
> > > > On 02/02/2012 11:29 AM, Paolo Bonzini wrote:
> > > > > On 02/02/2012 06:03 PM, Anthony Liguori wrote:
> > > > > > >
> > > > > >
> > > > > > Is this still needed with qom-upstream.14? I fixed a bug on .14 that
> > > > > > involved child properties that was making device-del sometimes fail.
> > > > >
> > > > > Not sure, I tried with .13 but, from the look of it, it should still be there.
> > > > > Regarding the .13->.14 diff:
> > > > >
> > > > > - you need QTAILQ_FOREACH_SAFE in object_property_del_child.
> > > >
> > > > Ack.
> > > >
> > > > > - you need to check for the existence of the non-aliased name when accessing the
> > > > > alias table, because s390 does not have PCI.
> > > >
> > > > I don't think that's the right strategy as it means that s390 only works if we 
> > > > don't include the PCI objects in the build (regardless of whether it uses PCI). 
> > > >  This would be defeated if/when we move to having all device objects in a 
> > > > single shared library used by all of the qemu executables.
> > > >
> > > > I'd prefer to just drop the aliases for s390.  I don't see a lot of value in it 
> > > > and I don't think there are tons of s390 users that will be affected.
> > >
> > > The reason for the aliases is to make -drive and -net work. If you have 
> > > alternatives to aliases there, I'm happy to go with them.
> >
> > Um, but I see (in s390-virtio.c):
> >
> >
> >     for(i = 0; i<  nb_nics; i++) {
> >         NICInfo *nd =&nd_table[i];
> >         DeviceState *dev;
> >
> >         if (!nd->model) {
> >             nd->model = g_strdup("virtio");
> >         }
> >
> >         if (strcmp(nd->model, "virtio")) {
> >             fprintf(stderr, "S390 only supports VirtIO nics\n");
> >             exit(1);
> >         }
> >
> >         dev = qdev_create((BusState *)s390_bus, "virtio-net-s390");
> >         qdev_set_nic_properties(dev, nd);
> >         qdev_init_nofail(dev);
> >     }
> >
> >     /* Create VirtIO disk drives */
> >     for(i = 0; i<  MAX_BLK_DEVS; i++) {
> >         DriveInfo *dinfo;
> >         DeviceState *dev;
> >
> >         dinfo = drive_get(IF_IDE, 0, i);
> >         if (!dinfo) {
> >             continue;
> >         }
> >
> >         dev = qdev_create((BusState *)s390_bus, "virtio-blk-s390");
> >         qdev_prop_set_drive_nofail(dev, "drive", dinfo->bdrv);
> >         qdev_init_nofail(dev);
> >     }
> >
> > So s390 totally ignores the -drive if
>
> Nope, since virtio drives aren't handled through the IF_ legacy stuff but 
> through qden instantiation. We only fake virtio disks for -hda here (which 
> should be replaced by a default_virtio option in the machine config).
>
> > parameter and will only accept virtio for -net.
>
> It only supports virtio at all, yes. No MMIO there ;).
>
> >  From what I can tell, it's not an issue.  But if we need it, we can do:
> >
> > diff --git a/arch_init.h b/arch_init.h
> > index 828256c..bfbd9e1 100644
> > --- a/arch_init.h
> > +++ b/arch_init.h
> > @@ -32,4 +32,9 @@ int tcg_available(void);
> > int kvm_available(void);
> > int xen_available(void);
> >
> > +static inline int target_get_arch(void)
> > +{
> > +    return arch_type;
>
> We could also have a machine type field that could be PCI or S390, right? I 
> somehow don't like globals.
>
> And just because it's s390 doesn't tell us anything. Maybe someone clever will 
> find a way to expose PCI in a later machine type and use that for all devices?
>
> The same thing goes for arm and their mmio virtio too btw.

Right, you're just pointing out though that the current code is dumb.  I full 
heartedly agree with you :-)

If you added PCI to the build for s390, aliases wouldn't do what you expect 
anymore.

> > +}
> > +
> > #endif
> > diff --git a/blockdev.c b/blockdev.c
> > index 7e4c548..caa9205 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -565,7 +565,15 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
> >      case IF_VIRTIO:
> >          /* add virtio block device */
> >          opts = qemu_opts_create(qemu_find_opts("device"), NULL, 0);
> > -        qemu_opt_set(opts, "driver", "virtio-blk");
> > +        switch(target_get_arch()) {
> > +        case QEMU_ARCH_S390X:
> > +            qemu_opt_set(opts, "driver", "virtio-blk-s390");
> > +            break;
> > +        default:
> > +            qemu_opt_set(opts, "driver", "virtio-blk-pci");
> > +            break;
> > +        }
> > +
> >          qemu_opt_set(opts, "drive", dinfo->id);
> >          if (devaddr)
> >              qemu_opt_set(opts, "addr", devaddr);
> > diff --git a/blockdev.c b/blockdev.c
> > index 7e4c548..caa9205 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -565,7 +565,15 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
> >      case IF_VIRTIO:
> >          /* add virtio block device */
> >          opts = qemu_opts_create(qemu_find_opts("device"), NULL, 0);
> > -        qemu_opt_set(opts, "driver", "virtio-blk");
> > +        switch(target_get_arch()) {
> > +        case QEMU_ARCH_S390X:
> > +            qemu_opt_set(opts, "driver", "virtio-blk-s390");
> > +            break;
> > +        default:
> > +            qemu_opt_set(opts, "driver", "virtio-blk-pci");
> > +            break;
> > +        }
> > +
> >          qemu_opt_set(opts, "drive", dinfo->id);
> >          if (devaddr)
> >              qemu_opt_set(opts, "addr", devaddr);
> >
> >
> > Can you confirm what we actually need here?
>
> Every time you grep for virtio-xxx-pci in the code without an s390 branch it's 
> wrong. Pretty simple, eh? :)


Okay, I'm going to go with this change as this is the only place that it seems 
to exist.

Regards,

Anthony Liguori

> Alex
>
> > Regards,
> >
> > Anthony Liguori
> >
> > > Alex