| Message ID | CACVXFVMjKu+osfB63g3s-QDgA0XBbOB1fkD=2WdX=LRSXzG60w@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Tue, 17 Jun 2014 10:44:11 +0800 Ming Lei <ming.lei@canonical.com> wrote: > On Tue, Jun 17, 2014 at 12:04 AM, Cornelia Huck > <cornelia.huck@de.ibm.com> wrote: > > On Mon, 16 Jun 2014 23:40:50 +0800 > > Ming Lei <ming.lei@canonical.com> wrote: > > > >> The two common virtio features can be defined per bus, so move all > >> into virtio-s390 class device to make code more clean. > >> > >> Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > >> Signed-off-by: Ming Lei <ming.lei@canonical.com> > >> --- > >> hw/s390x/s390-virtio-bus.c | 15 ++++++--------- > >> 1 file changed, 6 insertions(+), 9 deletions(-) > > > > This one breaks for me: > > > > qemu-system-s390x: -device virtio-blk-s390,scsi=off,config-wce=off,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1: Property '.scsi' not found > > > >> > >> diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c > >> index 9c71afa..ab9758e 100644 > >> --- a/hw/s390x/s390-virtio-bus.c > >> +++ b/hw/s390x/s390-virtio-bus.c > >> @@ -526,18 +526,11 @@ static const TypeInfo s390_virtio_net = { > >> .class_init = s390_virtio_net_class_init, > >> }; > >> > >> -static Property s390_virtio_blk_properties[] = { > >> - DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkS390, blk), > >> - DEFINE_PROP_END_OF_LIST(), > >> -}; > >> - > >> static void s390_virtio_blk_class_init(ObjectClass *klass, void *data) > >> { > >> - DeviceClass *dc = DEVICE_CLASS(klass); > >> VirtIOS390DeviceClass *k = VIRTIO_S390_DEVICE_CLASS(klass); > >> > >> k->init = s390_virtio_blk_init; > >> - dc->props = s390_virtio_blk_properties; > > > > ...which is probably because you removed the block properties here. > > You are right, DEFINE_VIRTIO_BLK_PROPERTIES() should have been > kept. > > Could you test attached patch? > Hm, with the attached patch qemu starts, but the guest will not come up (same commandline + kernel comes up fine on master). Let me dig around a bit.
On Tue, 17 Jun 2014 09:07:41 +0200 Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > On Tue, 17 Jun 2014 10:44:11 +0800 > Ming Lei <ming.lei@canonical.com> wrote: > > > On Tue, Jun 17, 2014 at 12:04 AM, Cornelia Huck > > <cornelia.huck@de.ibm.com> wrote: > > > On Mon, 16 Jun 2014 23:40:50 +0800 > > > Ming Lei <ming.lei@canonical.com> wrote: > > > > > >> The two common virtio features can be defined per bus, so move all > > >> into virtio-s390 class device to make code more clean. > > >> > > >> Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > > >> Signed-off-by: Ming Lei <ming.lei@canonical.com> > > >> --- > > >> hw/s390x/s390-virtio-bus.c | 15 ++++++--------- > > >> 1 file changed, 6 insertions(+), 9 deletions(-) > > > > > > This one breaks for me: > > > > > > qemu-system-s390x: -device virtio-blk-s390,scsi=off,config-wce=off,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1: Property '.scsi' not found > > > > > >> > > >> diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c > > >> index 9c71afa..ab9758e 100644 > > >> --- a/hw/s390x/s390-virtio-bus.c > > >> +++ b/hw/s390x/s390-virtio-bus.c > > >> @@ -526,18 +526,11 @@ static const TypeInfo s390_virtio_net = { > > >> .class_init = s390_virtio_net_class_init, > > >> }; > > >> > > >> -static Property s390_virtio_blk_properties[] = { > > >> - DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkS390, blk), > > >> - DEFINE_PROP_END_OF_LIST(), > > >> -}; > > >> - > > >> static void s390_virtio_blk_class_init(ObjectClass *klass, void *data) > > >> { > > >> - DeviceClass *dc = DEVICE_CLASS(klass); > > >> VirtIOS390DeviceClass *k = VIRTIO_S390_DEVICE_CLASS(klass); > > >> > > >> k->init = s390_virtio_blk_init; > > >> - dc->props = s390_virtio_blk_properties; > > > > > > ...which is probably because you removed the block properties here. > > > > You are right, DEFINE_VIRTIO_BLK_PROPERTIES() should have been > > kept. > > > > Could you test attached patch? > > > Hm, with the attached patch qemu starts, but the guest will not come up > (same commandline + kernel comes up fine on master). Let me dig around > a bit. And the winner is: event_idx s390-virtio wants event_idx=false or we can't get I/O through. I guess we can't use DEFINE_VIRTIO_COMMON_FEATURES for this transport. (Probably nobody tested virtio-rng and vhost-scsi on this transport, either :)
On Tue, Jun 17, 2014 at 3:07 PM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > On Tue, 17 Jun 2014 10:44:11 +0800 > Ming Lei <ming.lei@canonical.com> wrote: > >> On Tue, Jun 17, 2014 at 12:04 AM, Cornelia Huck >> <cornelia.huck@de.ibm.com> wrote: >> > On Mon, 16 Jun 2014 23:40:50 +0800 >> > Ming Lei <ming.lei@canonical.com> wrote: >> > >> >> The two common virtio features can be defined per bus, so move all >> >> into virtio-s390 class device to make code more clean. >> >> >> >> Suggested-by: Paolo Bonzini <pbonzini@redhat.com> >> >> Signed-off-by: Ming Lei <ming.lei@canonical.com> >> >> --- >> >> hw/s390x/s390-virtio-bus.c | 15 ++++++--------- >> >> 1 file changed, 6 insertions(+), 9 deletions(-) >> > >> > This one breaks for me: >> > >> > qemu-system-s390x: -device virtio-blk-s390,scsi=off,config-wce=off,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1: Property '.scsi' not found >> > >> >> >> >> diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c >> >> index 9c71afa..ab9758e 100644 >> >> --- a/hw/s390x/s390-virtio-bus.c >> >> +++ b/hw/s390x/s390-virtio-bus.c >> >> @@ -526,18 +526,11 @@ static const TypeInfo s390_virtio_net = { >> >> .class_init = s390_virtio_net_class_init, >> >> }; >> >> >> >> -static Property s390_virtio_blk_properties[] = { >> >> - DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkS390, blk), >> >> - DEFINE_PROP_END_OF_LIST(), >> >> -}; >> >> - >> >> static void s390_virtio_blk_class_init(ObjectClass *klass, void *data) >> >> { >> >> - DeviceClass *dc = DEVICE_CLASS(klass); >> >> VirtIOS390DeviceClass *k = VIRTIO_S390_DEVICE_CLASS(klass); >> >> >> >> k->init = s390_virtio_blk_init; >> >> - dc->props = s390_virtio_blk_properties; >> > >> > ...which is probably because you removed the block properties here. >> >> You are right, DEFINE_VIRTIO_BLK_PROPERTIES() should have been >> kept. >> >> Could you test attached patch? >> > Hm, with the attached patch qemu starts, but the guest will not come up > (same commandline + kernel comes up fine on master). Let me dig around > a bit. Looks like s390 virtio-blk never enables the two common features, is there any reason the two features can't be supported by s390? Thanks, -- Ming Lei
On Tue, 17 Jun 2014 15:44:28 +0800 Ming Lei <ming.lei@canonical.com> wrote: > On Tue, Jun 17, 2014 at 3:07 PM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > > On Tue, 17 Jun 2014 10:44:11 +0800 > > Ming Lei <ming.lei@canonical.com> wrote: > > > >> On Tue, Jun 17, 2014 at 12:04 AM, Cornelia Huck > >> <cornelia.huck@de.ibm.com> wrote: > >> > On Mon, 16 Jun 2014 23:40:50 +0800 > >> > Ming Lei <ming.lei@canonical.com> wrote: > >> > > >> >> The two common virtio features can be defined per bus, so move all > >> >> into virtio-s390 class device to make code more clean. > >> >> > >> >> Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > >> >> Signed-off-by: Ming Lei <ming.lei@canonical.com> > >> >> --- > >> >> hw/s390x/s390-virtio-bus.c | 15 ++++++--------- > >> >> 1 file changed, 6 insertions(+), 9 deletions(-) > >> > > >> > This one breaks for me: > >> > > >> > qemu-system-s390x: -device virtio-blk-s390,scsi=off,config-wce=off,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1: Property '.scsi' not found > >> > > >> >> > >> >> diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c > >> >> index 9c71afa..ab9758e 100644 > >> >> --- a/hw/s390x/s390-virtio-bus.c > >> >> +++ b/hw/s390x/s390-virtio-bus.c > >> >> @@ -526,18 +526,11 @@ static const TypeInfo s390_virtio_net = { > >> >> .class_init = s390_virtio_net_class_init, > >> >> }; > >> >> > >> >> -static Property s390_virtio_blk_properties[] = { > >> >> - DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkS390, blk), > >> >> - DEFINE_PROP_END_OF_LIST(), > >> >> -}; > >> >> - > >> >> static void s390_virtio_blk_class_init(ObjectClass *klass, void *data) > >> >> { > >> >> - DeviceClass *dc = DEVICE_CLASS(klass); > >> >> VirtIOS390DeviceClass *k = VIRTIO_S390_DEVICE_CLASS(klass); > >> >> > >> >> k->init = s390_virtio_blk_init; > >> >> - dc->props = s390_virtio_blk_properties; > >> > > >> > ...which is probably because you removed the block properties here. > >> > >> You are right, DEFINE_VIRTIO_BLK_PROPERTIES() should have been > >> kept. > >> > >> Could you test attached patch? > >> > > Hm, with the attached patch qemu starts, but the guest will not come up > > (same commandline + kernel comes up fine on master). Let me dig around > > a bit. > > Looks like s390 virtio-blk never enables the two common features, is > there any reason the two features can't be supported by s390? Indirect descriptors are fine. event_idx will not work IIUC because we always need to do a sync before we see changes, and this needs an interrupt to trigger. The correct way, of course, is to unset VIRTIO_RING_F_EVENT_IDX in the guest driver, and I verified that this works - this does not help with old guests, however. Note that this applies only to the old s390-virtio transport - virtio-ccw is fine.
On Tue, Jun 17, 2014 at 4:46 PM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote: >> >> Looks like s390 virtio-blk never enables the two common features, is >> there any reason the two features can't be supported by s390? > > Indirect descriptors are fine. event_idx will not work IIUC because we > always need to do a sync before we see changes, and this needs an > interrupt to trigger. Sounds like the old s390 isn't cache coherent? Because you mean write in one side can only be observed from another side with an explicit notification or interrupt. On arm/arm64, we didn't see any problem with event_idx. > > The correct way, of course, is to unset VIRTIO_RING_F_EVENT_IDX in the > guest driver, and I verified that this works - this does not help with > old guests, however. Both sides should be ok since it is decided by negotiation. > > Note that this applies only to the old s390-virtio transport - > virtio-ccw is fine. Could you share me how s390 virtio is supported in kernel since I only see virtio-mmio and virtio-pci support under drivers/virtio/ of linux kernel? Thanks, -- Ming Lei
On Tue, 17 Jun 2014 18:15:39 +0800 Ming Lei <ming.lei@canonical.com> wrote: > On Tue, Jun 17, 2014 at 4:46 PM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > >> > >> Looks like s390 virtio-blk never enables the two common features, is > >> there any reason the two features can't be supported by s390? > > > > Indirect descriptors are fine. event_idx will not work IIUC because we > > always need to do a sync before we see changes, and this needs an > > interrupt to trigger. > > Sounds like the old s390 isn't cache coherent? Because you mean > write in one side can only be observed from another side with an > explicit notification or interrupt. > > On arm/arm64, we didn't see any problem with event_idx. But you probably have the queues in guest memory, as on other transports (including virtio-ccw)? The old s390-virtio transport keeps the devices and their virtqueues in a memory area behind the guest memory - the guest does not see that memory directly, but a sync has to be performed to see virtqueue movement (see s390_virtio_device_sync()). > > > > > The correct way, of course, is to unset VIRTIO_RING_F_EVENT_IDX in the > > guest driver, and I verified that this works - this does not help with > > old guests, however. > > Both sides should be ok since it is decided by negotiation. Yes, but the guest driver currently does not drop the feature bit, as it only asks about the features that the common code supports - and that does include VIRTIO_RING_F_EVENT_IDX. > > > > > Note that this applies only to the old s390-virtio transport - > > virtio-ccw is fine. > > Could you share me how s390 virtio is supported in kernel since > I only see virtio-mmio and virtio-pci support under drivers/virtio/ of > linux kernel? They're hiding under drivers/s390/kvm/. The old s390-virtio transport is supported by kvm_virtio.c, the virtio-ccw transport by virtio_ccw.c.
On Tue, Jun 17, 2014 at 6:25 PM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > On Tue, 17 Jun 2014 18:15:39 +0800 > Ming Lei <ming.lei@canonical.com> wrote: > >> On Tue, Jun 17, 2014 at 4:46 PM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote: >> >> >> >> Looks like s390 virtio-blk never enables the two common features, is >> >> there any reason the two features can't be supported by s390? >> > >> > Indirect descriptors are fine. event_idx will not work IIUC because we >> > always need to do a sync before we see changes, and this needs an >> > interrupt to trigger. >> >> Sounds like the old s390 isn't cache coherent? Because you mean >> write in one side can only be observed from another side with an >> explicit notification or interrupt. >> >> On arm/arm64, we didn't see any problem with event_idx. > > But you probably have the queues in guest memory, as on other > transports (including virtio-ccw)? The old s390-virtio transport keeps > the devices and their virtqueues in a memory area behind the guest > memory - the guest does not see that memory directly, but a sync has to > be performed to see virtqueue movement (see s390_virtio_device_sync()). OK, it looks like a real physical device, :-) I will keep s390-virtio as it is, thanks for your explanation. Thanks, -- Ming Lei
On Tue, Jun 17, 2014 at 6:40 PM, Ming Lei <ming.lei@canonical.com> wrote: > On Tue, Jun 17, 2014 at 6:25 PM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote: >> On Tue, 17 Jun 2014 18:15:39 +0800 >> Ming Lei <ming.lei@canonical.com> wrote: >> >>> On Tue, Jun 17, 2014 at 4:46 PM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote: >>> >> >>> >> Looks like s390 virtio-blk never enables the two common features, is >>> >> there any reason the two features can't be supported by s390? >>> > >>> > Indirect descriptors are fine. event_idx will not work IIUC because we >>> > always need to do a sync before we see changes, and this needs an >>> > interrupt to trigger. >>> >>> Sounds like the old s390 isn't cache coherent? Because you mean >>> write in one side can only be observed from another side with an >>> explicit notification or interrupt. >>> >>> On arm/arm64, we didn't see any problem with event_idx. >> >> But you probably have the queues in guest memory, as on other >> transports (including virtio-ccw)? The old s390-virtio transport keeps >> the devices and their virtqueues in a memory area behind the guest >> memory - the guest does not see that memory directly, but a sync has to >> be performed to see virtqueue movement (see s390_virtio_device_sync()). > > OK, it looks like a real physical device, :-) > > I will keep s390-virtio as it is, thanks for your explanation. BTW, do you want me to add DEFINE_VIRTIO_COMMON_FEATURES() to s390_virtio_net_properties and s390_virtio_scsi_properties since I remove them from their default properties? Thanks, -- Ming Lei
On Tue, 17 Jun 2014 18:45:09 +0800 Ming Lei <ming.lei@canonical.com> wrote: > On Tue, Jun 17, 2014 at 6:40 PM, Ming Lei <ming.lei@canonical.com> wrote: > > On Tue, Jun 17, 2014 at 6:25 PM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > >> On Tue, 17 Jun 2014 18:15:39 +0800 > >> Ming Lei <ming.lei@canonical.com> wrote: > >> > >>> On Tue, Jun 17, 2014 at 4:46 PM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > >>> >> > >>> >> Looks like s390 virtio-blk never enables the two common features, is > >>> >> there any reason the two features can't be supported by s390? > >>> > > >>> > Indirect descriptors are fine. event_idx will not work IIUC because we > >>> > always need to do a sync before we see changes, and this needs an > >>> > interrupt to trigger. > >>> > >>> Sounds like the old s390 isn't cache coherent? Because you mean > >>> write in one side can only be observed from another side with an > >>> explicit notification or interrupt. > >>> > >>> On arm/arm64, we didn't see any problem with event_idx. > >> > >> But you probably have the queues in guest memory, as on other > >> transports (including virtio-ccw)? The old s390-virtio transport keeps > >> the devices and their virtqueues in a memory area behind the guest > >> memory - the guest does not see that memory directly, but a sync has to > >> be performed to see virtqueue movement (see s390_virtio_device_sync()). > > > > OK, it looks like a real physical device, :-) > > > > I will keep s390-virtio as it is, thanks for your explanation. > > BTW, do you want me to add DEFINE_VIRTIO_COMMON_FEATURES() > to s390_virtio_net_properties and s390_virtio_scsi_properties since > I remove them from their default properties? It might be better to remove them for good :) Nobody has probably tried to use them for some time... I just start a very minimal guest with a virtio-console and a virtio-blk device. Don't know whether Alex has a more advanced setup at hand?
On 17.06.14 14:04, Cornelia Huck wrote: > On Tue, 17 Jun 2014 18:45:09 +0800 > Ming Lei <ming.lei@canonical.com> wrote: > >> On Tue, Jun 17, 2014 at 6:40 PM, Ming Lei <ming.lei@canonical.com> wrote: >>> On Tue, Jun 17, 2014 at 6:25 PM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote: >>>> On Tue, 17 Jun 2014 18:15:39 +0800 >>>> Ming Lei <ming.lei@canonical.com> wrote: >>>> >>>>> On Tue, Jun 17, 2014 at 4:46 PM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote: >>>>>>> Looks like s390 virtio-blk never enables the two common features, is >>>>>>> there any reason the two features can't be supported by s390? >>>>>> Indirect descriptors are fine. event_idx will not work IIUC because we >>>>>> always need to do a sync before we see changes, and this needs an >>>>>> interrupt to trigger. >>>>> Sounds like the old s390 isn't cache coherent? Because you mean >>>>> write in one side can only be observed from another side with an >>>>> explicit notification or interrupt. >>>>> >>>>> On arm/arm64, we didn't see any problem with event_idx. >>>> But you probably have the queues in guest memory, as on other >>>> transports (including virtio-ccw)? The old s390-virtio transport keeps >>>> the devices and their virtqueues in a memory area behind the guest >>>> memory - the guest does not see that memory directly, but a sync has to >>>> be performed to see virtqueue movement (see s390_virtio_device_sync()). >>> OK, it looks like a real physical device, :-) >>> >>> I will keep s390-virtio as it is, thanks for your explanation. >> BTW, do you want me to add DEFINE_VIRTIO_COMMON_FEATURES() >> to s390_virtio_net_properties and s390_virtio_scsi_properties since >> I remove them from their default properties? > It might be better to remove them for good :) > > Nobody has probably tried to use them for some time... > I just start a very minimal guest with a virtio-console and a > virtio-blk device. Don't know whether Alex has a more advanced setup > at hand? It's the only target that works with TCG, no? Alex
On Tue, 17 Jun 2014 14:06:25 +0200 Alexander Graf <agraf@suse.de> wrote: > > On 17.06.14 14:04, Cornelia Huck wrote: > > On Tue, 17 Jun 2014 18:45:09 +0800 > > Ming Lei <ming.lei@canonical.com> wrote: > > > >> On Tue, Jun 17, 2014 at 6:40 PM, Ming Lei <ming.lei@canonical.com> wrote: > >>> On Tue, Jun 17, 2014 at 6:25 PM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > >>>> On Tue, 17 Jun 2014 18:15:39 +0800 > >>>> Ming Lei <ming.lei@canonical.com> wrote: > >>>> > >>>>> On Tue, Jun 17, 2014 at 4:46 PM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > >>>>>>> Looks like s390 virtio-blk never enables the two common features, is > >>>>>>> there any reason the two features can't be supported by s390? > >>>>>> Indirect descriptors are fine. event_idx will not work IIUC because we > >>>>>> always need to do a sync before we see changes, and this needs an > >>>>>> interrupt to trigger. > >>>>> Sounds like the old s390 isn't cache coherent? Because you mean > >>>>> write in one side can only be observed from another side with an > >>>>> explicit notification or interrupt. > >>>>> > >>>>> On arm/arm64, we didn't see any problem with event_idx. > >>>> But you probably have the queues in guest memory, as on other > >>>> transports (including virtio-ccw)? The old s390-virtio transport keeps > >>>> the devices and their virtqueues in a memory area behind the guest > >>>> memory - the guest does not see that memory directly, but a sync has to > >>>> be performed to see virtqueue movement (see s390_virtio_device_sync()). > >>> OK, it looks like a real physical device, :-) > >>> > >>> I will keep s390-virtio as it is, thanks for your explanation. > >> BTW, do you want me to add DEFINE_VIRTIO_COMMON_FEATURES() > >> to s390_virtio_net_properties and s390_virtio_scsi_properties since > >> I remove them from their default properties? > > It might be better to remove them for good :) > > > > Nobody has probably tried to use them for some time... > > I just start a very minimal guest with a virtio-console and a > > virtio-blk device. Don't know whether Alex has a more advanced setup > > at hand? > > It's the only target that works with TCG, no? Yup. Do you have a test setup with net, scsi, ...? I usually test networking (for example) only via a libvirt setup, which will only work with virtio-ccw.
On 17.06.14 14:56, Cornelia Huck wrote: > On Tue, 17 Jun 2014 14:06:25 +0200 > Alexander Graf <agraf@suse.de> wrote: > >> On 17.06.14 14:04, Cornelia Huck wrote: >>> On Tue, 17 Jun 2014 18:45:09 +0800 >>> Ming Lei <ming.lei@canonical.com> wrote: >>> >>>> On Tue, Jun 17, 2014 at 6:40 PM, Ming Lei <ming.lei@canonical.com> wrote: >>>>> On Tue, Jun 17, 2014 at 6:25 PM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote: >>>>>> On Tue, 17 Jun 2014 18:15:39 +0800 >>>>>> Ming Lei <ming.lei@canonical.com> wrote: >>>>>> >>>>>>> On Tue, Jun 17, 2014 at 4:46 PM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote: >>>>>>>>> Looks like s390 virtio-blk never enables the two common features, is >>>>>>>>> there any reason the two features can't be supported by s390? >>>>>>>> Indirect descriptors are fine. event_idx will not work IIUC because we >>>>>>>> always need to do a sync before we see changes, and this needs an >>>>>>>> interrupt to trigger. >>>>>>> Sounds like the old s390 isn't cache coherent? Because you mean >>>>>>> write in one side can only be observed from another side with an >>>>>>> explicit notification or interrupt. >>>>>>> >>>>>>> On arm/arm64, we didn't see any problem with event_idx. >>>>>> But you probably have the queues in guest memory, as on other >>>>>> transports (including virtio-ccw)? The old s390-virtio transport keeps >>>>>> the devices and their virtqueues in a memory area behind the guest >>>>>> memory - the guest does not see that memory directly, but a sync has to >>>>>> be performed to see virtqueue movement (see s390_virtio_device_sync()). >>>>> OK, it looks like a real physical device, :-) >>>>> >>>>> I will keep s390-virtio as it is, thanks for your explanation. >>>> BTW, do you want me to add DEFINE_VIRTIO_COMMON_FEATURES() >>>> to s390_virtio_net_properties and s390_virtio_scsi_properties since >>>> I remove them from their default properties? >>> It might be better to remove them for good :) >>> >>> Nobody has probably tried to use them for some time... >>> I just start a very minimal guest with a virtio-console and a >>> virtio-blk device. Don't know whether Alex has a more advanced setup >>> at hand? >> It's the only target that works with TCG, no? > Yup. Do you have a test setup with net, scsi, ...? I usually test > networking (for example) only via a libvirt setup, which will only work > with virtio-ccw. I don't regularly test it, but OBS uses that target for example. Alex
From 8b58db209e5703583f9d8133e32c69a03ecca6ec Mon Sep 17 00:00:00 2001 From: Ming Lei <ming.lei@canonical.com> Date: Mon, 16 Jun 2014 23:23:28 +0800 Subject: [PATCH 2/4] s390-virtio-bus: move common virtio properties to virtio s390 device class The two common virtio features can be defined per bus, so move all into virtio-s390 class device to make code more clean. Suggested-by: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Ming Lei <ming.lei@canonical.com> --- hw/s390x/s390-virtio-bus.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c index 9c71afa..34f6de8 100644 --- a/hw/s390x/s390-virtio-bus.c +++ b/hw/s390x/s390-virtio-bus.c @@ -571,7 +571,6 @@ static const TypeInfo s390_virtio_serial = { }; static Property s390_virtio_rng_properties[] = { - DEFINE_VIRTIO_COMMON_FEATURES(VirtIOS390Device, host_features), DEFINE_VIRTIO_RNG_PROPERTIES(VirtIORNGS390, vdev.conf), DEFINE_PROP_END_OF_LIST(), }; @@ -610,10 +609,16 @@ static void s390_virtio_busdev_reset(DeviceState *dev) virtio_reset(_dev->vdev); } +static Property virtio_s390_properties[] = { + DEFINE_VIRTIO_COMMON_FEATURES(VirtIOS390Device, host_features), + DEFINE_PROP_END_OF_LIST(), +}; + static void virtio_s390_device_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); + dc->props = virtio_s390_properties; dc->init = s390_virtio_busdev_init; dc->bus_type = TYPE_S390_VIRTIO_BUS; dc->unplug = qdev_simple_unplug_cb; @@ -654,7 +659,6 @@ static const TypeInfo s390_virtio_scsi = { #ifdef CONFIG_VHOST_SCSI static Property s390_vhost_scsi_properties[] = { - DEFINE_VIRTIO_COMMON_FEATURES(VirtIOS390Device, host_features), DEFINE_VHOST_SCSI_PROPERTIES(VHostSCSIS390, vdev.parent_obj.conf), DEFINE_PROP_END_OF_LIST(), }; -- 1.7.9.5