On 03.02.2012, at 17:37, Anthony Liguori <anth...@codemonkey.ws> 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. > +} > + > #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? :) Alex > > Regards, > > Anthony Liguori > >> >> >> Alex >> >> >