* [RFC PATCH v1 0/3] virtio: early detect 'modern' virtio @ 2021-10-28 22:00 Halil Pasic 2021-10-28 22:00 ` [RFC PATCH v1 1/3] virtio: introduce virtio_force_modern() Halil Pasic ` (3 more replies) 0 siblings, 4 replies; 10+ messages in thread From: Halil Pasic @ 2021-10-28 22:00 UTC (permalink / raw) To: Michael S. Tsirkin, Cornelia Huck, Halil Pasic, qemu-s390x, qemu-devel Cc: Christian Borntraeger, Thomas Huth, Richard Henderson, David Hildenbrand This is an early RFC for a transport specific early detecton of modern virtio, which is most relevant for transitional devices on big endian platforms, when drivers access the config space before FEATURES_OK is set. The most important part that is missing here is fixing the problems that arrise in the situation described in the previous paragraph, when the config is managed by a vhost device (and thus outside QEMU). This series was only lightly tested. Halil Pasic (3): virtio: introduce virtio_force_modern() virtio-ccw: use virtio_force_modern virtio-pci: use virtio_force_modern() hw/s390x/virtio-ccw.c | 3 +++ hw/virtio/virtio-pci.c | 1 + hw/virtio/virtio.c | 12 ++++++++++++ include/hw/virtio/virtio.h | 1 + 4 files changed, 17 insertions(+) base-commit: 2c3e83f92d93fbab071b8a96b8ab769b01902475 -- 2.25.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [RFC PATCH v1 1/3] virtio: introduce virtio_force_modern() 2021-10-28 22:00 [RFC PATCH v1 0/3] virtio: early detect 'modern' virtio Halil Pasic @ 2021-10-28 22:00 ` Halil Pasic 2021-10-29 14:53 ` Cornelia Huck 2021-10-28 22:00 ` [RFC PATCH v1 2/3] virtio-ccw: use virtio_force_modern Halil Pasic ` (2 subsequent siblings) 3 siblings, 1 reply; 10+ messages in thread From: Halil Pasic @ 2021-10-28 22:00 UTC (permalink / raw) To: Michael S. Tsirkin, Cornelia Huck, Halil Pasic, qemu-s390x, qemu-devel Cc: Christian Borntraeger, Thomas Huth, Richard Henderson, David Hildenbrand Legacy vs modern should be detected via transport specific means. We can't wait till feature negotiation is done. Let us introduce virtio_force_modern() as a means for the transport code to signal that the device should operate in modern mode (because a modern driver was detected). Signed-off-by: Halil Pasic <pasic@linux.ibm.com> --- I'm still struggling with how to deal with vhost-user and co. The problem is that I'm not very familiar with the life-cycle of, let us say, a vhost_user device. Looks to me like the vhost part might be just an implementation detail, and could even become a hot swappable thing. Another thing is, that vhost processes set_features differently. It might or might not be a good idea to change this. Does anybody know why don't we propagate the features on features_set, but under a set of different conditions, one of which is the vhost device is started? --- hw/virtio/virtio.c | 12 ++++++++++++ include/hw/virtio/virtio.h | 1 + 2 files changed, 13 insertions(+) diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 3a1f6c520c..75aee0e098 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -3281,6 +3281,18 @@ void virtio_init(VirtIODevice *vdev, const char *name, vdev->use_guest_notifier_mask = true; } +void virtio_force_modern(VirtIODevice *vdev) +{ + /* + * This takes care of the devices that implement config space access + * in QEMU. For vhost-user and similar we need to make sure the features + * are actually propagated to the device implementing the config space. + * + * A VirtioDeviceClass callback may be a good idea. + */ + virtio_set_features(vdev, (1ULL << VIRTIO_F_VERSION_1)); +} + /* * Only devices that have already been around prior to defining the virtio * standard support legacy mode; this includes devices not specified in the diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h index 8bab9cfb75..2ea92de7a5 100644 --- a/include/hw/virtio/virtio.h +++ b/include/hw/virtio/virtio.h @@ -394,6 +394,7 @@ static inline bool virtio_device_disabled(VirtIODevice *vdev) return unlikely(vdev->disabled || vdev->broken); } +void virtio_force_modern(VirtIODevice *vdev); bool virtio_legacy_allowed(VirtIODevice *vdev); bool virtio_legacy_check_disabled(VirtIODevice *vdev); -- 2.25.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [RFC PATCH v1 1/3] virtio: introduce virtio_force_modern() 2021-10-28 22:00 ` [RFC PATCH v1 1/3] virtio: introduce virtio_force_modern() Halil Pasic @ 2021-10-29 14:53 ` Cornelia Huck 2021-11-12 15:42 ` Halil Pasic 0 siblings, 1 reply; 10+ messages in thread From: Cornelia Huck @ 2021-10-29 14:53 UTC (permalink / raw) To: Halil Pasic, Michael S. Tsirkin, Halil Pasic, qemu-s390x, qemu-devel Cc: Christian Borntraeger, Thomas Huth, Richard Henderson, David Hildenbrand On Fri, Oct 29 2021, Halil Pasic <pasic@linux.ibm.com> wrote: > Legacy vs modern should be detected via transport specific means. We > can't wait till feature negotiation is done. Let us introduce > virtio_force_modern() as a means for the transport code to signal > that the device should operate in modern mode (because a modern driver > was detected). > > Signed-off-by: Halil Pasic <pasic@linux.ibm.com> > --- > > I'm still struggling with how to deal with vhost-user and co. The > problem is that I'm not very familiar with the life-cycle of, let us > say, a vhost_user device. > > Looks to me like the vhost part might be just an implementation detail, > and could even become a hot swappable thing. > > Another thing is, that vhost processes set_features differently. It > might or might not be a good idea to change this. > > Does anybody know why don't we propagate the features on features_set, > but under a set of different conditions, one of which is the vhost > device is started? > --- > hw/virtio/virtio.c | 12 ++++++++++++ > include/hw/virtio/virtio.h | 1 + > 2 files changed, 13 insertions(+) > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index 3a1f6c520c..75aee0e098 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -3281,6 +3281,18 @@ void virtio_init(VirtIODevice *vdev, const char *name, > vdev->use_guest_notifier_mask = true; > } > > +void virtio_force_modern(VirtIODevice *vdev) <bikeshed> I'm not sure I like that name. We're not actually forcing the device to be modern, we just set an early indication in the device before proper feature negotiation has finished. Maybe virtio_indicate_modern()? </bikeshed> > +{ > + /* > + * This takes care of the devices that implement config space access > + * in QEMU. For vhost-user and similar we need to make sure the features > + * are actually propagated to the device implementing the config space. > + * > + * A VirtioDeviceClass callback may be a good idea. > + */ > + virtio_set_features(vdev, (1ULL << VIRTIO_F_VERSION_1)); Do we really need/want to do the whole song-and-dance for setting features, just for setting VERSION_1? Devices may modify some of their behaviour or features, depending on what features they are called with, and we will be calling this one again later with what is likely a different feature set. Also, the return code is not checked. Maybe introduce a new function that sets guest_features directly and errors out if the features are not set in host_features? If we try to set VERSION_1 here despite the device not offering it, we are in a pickle anyway, as we should not have gotten here if we did not offer it, and we really should moan and fail in that case. > +} > + > /* > * Only devices that have already been around prior to defining the virtio > * standard support legacy mode; this includes devices not specified in the ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH v1 1/3] virtio: introduce virtio_force_modern() 2021-10-29 14:53 ` Cornelia Huck @ 2021-11-12 15:42 ` Halil Pasic 2021-11-12 15:55 ` Cornelia Huck 0 siblings, 1 reply; 10+ messages in thread From: Halil Pasic @ 2021-11-12 15:42 UTC (permalink / raw) To: Cornelia Huck Cc: Thomas Huth, Michael S. Tsirkin, David Hildenbrand, Richard Henderson, qemu-devel, Halil Pasic, Christian Borntraeger, qemu-s390x On Fri, 29 Oct 2021 16:53:25 +0200 Cornelia Huck <cohuck@redhat.com> wrote: > On Fri, Oct 29 2021, Halil Pasic <pasic@linux.ibm.com> wrote: > > > Legacy vs modern should be detected via transport specific means. We > > can't wait till feature negotiation is done. Let us introduce > > virtio_force_modern() as a means for the transport code to signal > > that the device should operate in modern mode (because a modern driver > > was detected). > > > > Signed-off-by: Halil Pasic <pasic@linux.ibm.com> > > --- > > > > I'm still struggling with how to deal with vhost-user and co. The > > problem is that I'm not very familiar with the life-cycle of, let us > > say, a vhost_user device. > > > > Looks to me like the vhost part might be just an implementation detail, > > and could even become a hot swappable thing. > > > > Another thing is, that vhost processes set_features differently. It > > might or might not be a good idea to change this. > > > > Does anybody know why don't we propagate the features on features_set, > > but under a set of different conditions, one of which is the vhost > > device is started? > > --- > > hw/virtio/virtio.c | 12 ++++++++++++ > > include/hw/virtio/virtio.h | 1 + > > 2 files changed, 13 insertions(+) > > > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > > index 3a1f6c520c..75aee0e098 100644 > > --- a/hw/virtio/virtio.c > > +++ b/hw/virtio/virtio.c > > @@ -3281,6 +3281,18 @@ void virtio_init(VirtIODevice *vdev, const char *name, > > vdev->use_guest_notifier_mask = true; > > } > > > > +void virtio_force_modern(VirtIODevice *vdev) > > <bikeshed> I'm not sure I like that name. We're not actually forcing the > device to be modern, we just set an early indication in the device > before proper feature negotiation has finished. Maybe > virtio_indicate_modern()? </bikeshed> I don't like virtio_indicate_modern(dev) form object orientation perspective. In an OO language one would write it like dev.virtio_indicate_modern() which would read like the device should indicate modern to somebody. In my opinion what happens is that we want to disable the legacy interface if it is exposed by the device, or in other words instruct the device that should act (precisely and exclusively) according to the interface specification of the modern interface. Maybe we can find a better name than force_modern, but I don't think indicate_modern is a better name. > > > +{ > > + /* > > + * This takes care of the devices that implement config space access > > + * in QEMU. For vhost-user and similar we need to make sure the features > > + * are actually propagated to the device implementing the config space. > > + * > > + * A VirtioDeviceClass callback may be a good idea. > > + */ > > + virtio_set_features(vdev, (1ULL << VIRTIO_F_VERSION_1)); > > Do we really need/want to do the whole song-and-dance for setting > features, just for setting VERSION_1? When doing the whole song-and-dance the chance is higher that the information will propagate to every place it needs to reach. For example to the acked_features of vhost_dev. I've just posted a v2 RFC. It should not be hard to see what I mean after examining that RFC. > Devices may modify some of their > behaviour or features, depending on what features they are called with, I believe, if this is the case, we want the behavior that corresponds to VERSION_1 set, i.e. 'modern'. So in my understanding this is rather good than bad. > and we will be calling this one again later with what is likely a > different feature set. That is true, but the driver is allowed to set the features multiple times, and since transports only support piecemeal access to the features (32 bits at a time), I guess this is biz as usual. >Also, the return code is not checked. > That is true! It might be a good idea to log an error. Unfortunately I don't think there is anything else we can sanely do. > Maybe introduce a new function that sets guest_features directly and > errors out if the features are not set in host_features? See above. > If we try to > set VERSION_1 here despite the device not offering it, we are in a > pickle anyway, as we should not have gotten here if we did not offer it, > and we really should moan and fail in that case. I agree about the moan part. I'm not sure what is the best way to 'fail'. Maybe we should continue this discussion in the v2 thread. Thanks for your feedback! Sorry I didn't answer before sending out a v2. Regards, Halil > > > +} > > + > > /* > > * Only devices that have already been around prior to defining the virtio > > * standard support legacy mode; this includes devices not specified in the > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH v1 1/3] virtio: introduce virtio_force_modern() 2021-11-12 15:42 ` Halil Pasic @ 2021-11-12 15:55 ` Cornelia Huck 2021-11-12 16:36 ` Halil Pasic 0 siblings, 1 reply; 10+ messages in thread From: Cornelia Huck @ 2021-11-12 15:55 UTC (permalink / raw) To: Halil Pasic Cc: Thomas Huth, Michael S. Tsirkin, David Hildenbrand, Richard Henderson, qemu-devel, Halil Pasic, Christian Borntraeger, qemu-s390x On Fri, Nov 12 2021, Halil Pasic <pasic@linux.ibm.com> wrote: > On Fri, 29 Oct 2021 16:53:25 +0200 > Cornelia Huck <cohuck@redhat.com> wrote: > >> On Fri, Oct 29 2021, Halil Pasic <pasic@linux.ibm.com> wrote: >> >> > Legacy vs modern should be detected via transport specific means. We >> > can't wait till feature negotiation is done. Let us introduce >> > virtio_force_modern() as a means for the transport code to signal >> > that the device should operate in modern mode (because a modern driver >> > was detected). >> > >> > Signed-off-by: Halil Pasic <pasic@linux.ibm.com> >> > --- >> > >> > I'm still struggling with how to deal with vhost-user and co. The >> > problem is that I'm not very familiar with the life-cycle of, let us >> > say, a vhost_user device. >> > >> > Looks to me like the vhost part might be just an implementation detail, >> > and could even become a hot swappable thing. >> > >> > Another thing is, that vhost processes set_features differently. It >> > might or might not be a good idea to change this. >> > >> > Does anybody know why don't we propagate the features on features_set, >> > but under a set of different conditions, one of which is the vhost >> > device is started? >> > --- >> > hw/virtio/virtio.c | 12 ++++++++++++ >> > include/hw/virtio/virtio.h | 1 + >> > 2 files changed, 13 insertions(+) >> > >> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >> > index 3a1f6c520c..75aee0e098 100644 >> > --- a/hw/virtio/virtio.c >> > +++ b/hw/virtio/virtio.c >> > @@ -3281,6 +3281,18 @@ void virtio_init(VirtIODevice *vdev, const char *name, >> > vdev->use_guest_notifier_mask = true; >> > } >> > >> > +void virtio_force_modern(VirtIODevice *vdev) >> >> <bikeshed> I'm not sure I like that name. We're not actually forcing the >> device to be modern, we just set an early indication in the device >> before proper feature negotiation has finished. Maybe >> virtio_indicate_modern()? </bikeshed> > > > I don't like virtio_indicate_modern(dev) form object orientation > perspective. In an OO language one would write it like > dev.virtio_indicate_modern() > which would read like the device should indicate modern to somebody. I think that is actually what happens: we indicate that it is a modern device to the code making the endianness decisions. > > In my opinion what happens is that we want to disable the legacy > interface if it is exposed by the device, or in other words instruct the > device that should act (precisely and exclusively) according to the > interface specification of the modern interface. I don't see us disabling anything; the driver has already chosen what they want, and we simply need to make sure that all code honours that decision. > > Maybe we can find a better name than force_modern, but I don't think > indicate_modern is a better name. > >> >> > +{ >> > + /* >> > + * This takes care of the devices that implement config space access >> > + * in QEMU. For vhost-user and similar we need to make sure the features >> > + * are actually propagated to the device implementing the config space. >> > + * >> > + * A VirtioDeviceClass callback may be a good idea. >> > + */ >> > + virtio_set_features(vdev, (1ULL << VIRTIO_F_VERSION_1)); >> >> Do we really need/want to do the whole song-and-dance for setting >> features, just for setting VERSION_1? > > When doing the whole song-and-dance the chance is higher that the > information will propagate to every place it needs to reach. For > example to the acked_features of vhost_dev. I've just posted a v2 RFC. > It should not be hard to see what I mean after examining that RFC. > >> Devices may modify some of their >> behaviour or features, depending on what features they are called with, > > I believe, if this is the case, we want the behavior that corresponds to > VERSION_1 set, i.e. 'modern'. So in my understanding this is rather good > than bad. > >> and we will be calling this one again later with what is likely a >> different feature set. > > That is true, but the driver is allowed to set the features multiple > times, and since transports only support piecemeal access to the > features (32 bits at a time), I guess this is biz as usual. Also see my comment in the v2: I'm not sure how well tested that actually is. > >>Also, the return code is not checked. >> > > That is true! It might be a good idea to log an error. Unfortunately I > don't think there is anything else we can sanely do. > >> Maybe introduce a new function that sets guest_features directly and >> errors out if the features are not set in host_features? > > See above. > >> If we try to >> set VERSION_1 here despite the device not offering it, we are in a >> pickle anyway, as we should not have gotten here if we did not offer it, >> and we really should moan and fail in that case. > > I agree about the moan part. I'm not sure what is the best way to > 'fail'. Maybe we should continue this discussion in the v2 thread. Yeah, let's continue there, since that code is a bit different. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH v1 1/3] virtio: introduce virtio_force_modern() 2021-11-12 15:55 ` Cornelia Huck @ 2021-11-12 16:36 ` Halil Pasic 0 siblings, 0 replies; 10+ messages in thread From: Halil Pasic @ 2021-11-12 16:36 UTC (permalink / raw) To: Cornelia Huck Cc: Thomas Huth, Michael S. Tsirkin, David Hildenbrand, Richard Henderson, qemu-devel, Halil Pasic, Christian Borntraeger, qemu-s390x On Fri, 12 Nov 2021 16:55:10 +0100 Cornelia Huck <cohuck@redhat.com> wrote: > On Fri, Nov 12 2021, Halil Pasic <pasic@linux.ibm.com> wrote: > > > On Fri, 29 Oct 2021 16:53:25 +0200 > > Cornelia Huck <cohuck@redhat.com> wrote: > > > >> On Fri, Oct 29 2021, Halil Pasic <pasic@linux.ibm.com> wrote: > >> > >> > Legacy vs modern should be detected via transport specific means. We > >> > can't wait till feature negotiation is done. Let us introduce > >> > virtio_force_modern() as a means for the transport code to signal > >> > that the device should operate in modern mode (because a modern driver > >> > was detected). > >> > > >> > Signed-off-by: Halil Pasic <pasic@linux.ibm.com> > >> > --- > >> > > >> > I'm still struggling with how to deal with vhost-user and co. The > >> > problem is that I'm not very familiar with the life-cycle of, let us > >> > say, a vhost_user device. > >> > > >> > Looks to me like the vhost part might be just an implementation detail, > >> > and could even become a hot swappable thing. > >> > > >> > Another thing is, that vhost processes set_features differently. It > >> > might or might not be a good idea to change this. > >> > > >> > Does anybody know why don't we propagate the features on features_set, > >> > but under a set of different conditions, one of which is the vhost > >> > device is started? > >> > --- > >> > hw/virtio/virtio.c | 12 ++++++++++++ > >> > include/hw/virtio/virtio.h | 1 + > >> > 2 files changed, 13 insertions(+) > >> > > >> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > >> > index 3a1f6c520c..75aee0e098 100644 > >> > --- a/hw/virtio/virtio.c > >> > +++ b/hw/virtio/virtio.c > >> > @@ -3281,6 +3281,18 @@ void virtio_init(VirtIODevice *vdev, const char *name, > >> > vdev->use_guest_notifier_mask = true; > >> > } > >> > > >> > +void virtio_force_modern(VirtIODevice *vdev) > >> > >> <bikeshed> I'm not sure I like that name. We're not actually forcing the > >> device to be modern, we just set an early indication in the device > >> before proper feature negotiation has finished. Maybe > >> virtio_indicate_modern()? </bikeshed> > > > > > > I don't like virtio_indicate_modern(dev) form object orientation > > perspective. In an OO language one would write it like > > dev.virtio_indicate_modern() > > which would read like the device should indicate modern to somebody. > > I think that is actually what happens: we indicate that it is a modern > device to the code making the endianness decisions. > But in an OO school of thought that code belongs to the given virtio device object and is one of the building blocks that makes the object what it is. What I'm trying to explain is: that code ain't no external entity we have to indicate something to. On the contrary, if we had to indicate 'modern' to the driver, how would you name that function? Clearly we don't need such functionality, I'm just trying to make an argument here. To take a different example, imagine a ccw channel path. We may break the the channel path, we may indicate to the OS that the channel path is broken (via CRW), and we may do first break than indicate. > > > > In my opinion what happens is that we want to disable the legacy > > interface if it is exposed by the device, or in other words instruct the > > device that should act (precisely and exclusively) according to the > > interface specification of the modern interface. > > I don't see us disabling anything; the driver has already chosen what > they want, and we simply need to make sure that all code honours that > decision. IMHO a buggy driver could make an attempt at using the legacy interface at least in case of pci. My understanding is that the decision of the driver results in an interaction between the driver and the device, and as a result of that interaction, the state of the device changes. This function is supposed to implement that state-change. Do we agree that there is a state change? If yes, how would you describe that state change? > > > > > Maybe we can find a better name than force_modern, but I don't think > > indicate_modern is a better name. > > > >> > >> > +{ > >> > + /* > >> > + * This takes care of the devices that implement config space access > >> > + * in QEMU. For vhost-user and similar we need to make sure the features > >> > + * are actually propagated to the device implementing the config space. > >> > + * > >> > + * A VirtioDeviceClass callback may be a good idea. > >> > + */ > >> > + virtio_set_features(vdev, (1ULL << VIRTIO_F_VERSION_1)); > >> > >> Do we really need/want to do the whole song-and-dance for setting > >> features, just for setting VERSION_1? > > > > When doing the whole song-and-dance the chance is higher that the > > information will propagate to every place it needs to reach. For > > example to the acked_features of vhost_dev. I've just posted a v2 RFC. > > It should not be hard to see what I mean after examining that RFC. > > > >> Devices may modify some of their > >> behaviour or features, depending on what features they are called with, > > > > I believe, if this is the case, we want the behavior that corresponds to > > VERSION_1 set, i.e. 'modern'. So in my understanding this is rather good > > than bad. > > > >> and we will be calling this one again later with what is likely a > >> different feature set. > > > > That is true, but the driver is allowed to set the features multiple > > times, and since transports only support piecemeal access to the > > features (32 bits at a time), I guess this is biz as usual. > > Also see my comment in the v2: I'm not sure how well tested that > actually is. > Will answer there. > > > >>Also, the return code is not checked. > >> > > > > That is true! It might be a good idea to log an error. Unfortunately I > > don't think there is anything else we can sanely do. > > > >> Maybe introduce a new function that sets guest_features directly and > >> errors out if the features are not set in host_features? > > > > See above. > > > >> If we try to > >> set VERSION_1 here despite the device not offering it, we are in a > >> pickle anyway, as we should not have gotten here if we did not offer it, > >> and we really should moan and fail in that case. > > > > I agree about the moan part. I'm not sure what is the best way to > > 'fail'. Maybe we should continue this discussion in the v2 thread. > > Yeah, let's continue there, since that code is a bit different. > Nod! Thanks! Halil ^ permalink raw reply [flat|nested] 10+ messages in thread
* [RFC PATCH v1 2/3] virtio-ccw: use virtio_force_modern 2021-10-28 22:00 [RFC PATCH v1 0/3] virtio: early detect 'modern' virtio Halil Pasic 2021-10-28 22:00 ` [RFC PATCH v1 1/3] virtio: introduce virtio_force_modern() Halil Pasic @ 2021-10-28 22:00 ` Halil Pasic 2021-10-28 22:00 ` [RFC PATCH v1 3/3] virtio-pci: use virtio_force_modern() Halil Pasic 2021-11-05 7:42 ` [RFC PATCH v1 0/3] virtio: early detect 'modern' virtio Michael S. Tsirkin 3 siblings, 0 replies; 10+ messages in thread From: Halil Pasic @ 2021-10-28 22:00 UTC (permalink / raw) To: Michael S. Tsirkin, Cornelia Huck, Halil Pasic, qemu-s390x, qemu-devel Cc: Christian Borntraeger, Thomas Huth, Richard Henderson, David Hildenbrand The fact that revision > 0 was negotiated implies that VIRTIO_VERSION_1 aka modern must be used. This negotiation is done before the obligatory reset. Let us call virtio_force_modern() after the reset if revision > 0 was negotiated, so that the VIRTIO_VERSION_1 feature can be set, and endianness starts working as it should for devices that comply to the virtio spec. Signed-off-by: Halil Pasic <pasic@linux.ibm.com> --- hw/s390x/virtio-ccw.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c index 6a2df1c1e9..88fbe87942 100644 --- a/hw/s390x/virtio-ccw.c +++ b/hw/s390x/virtio-ccw.c @@ -266,6 +266,9 @@ static void virtio_ccw_reset_virtio(VirtioCcwDevice *dev, VirtIODevice *vdev) dev->summary_indicator = NULL; } ccw_dev->sch->thinint_active = false; + if (dev->revision > 0) { + virtio_force_modern(vdev); + } } static int virtio_ccw_handle_set_vq(SubchDev *sch, CCW1 ccw, bool check_len, -- 2.25.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [RFC PATCH v1 3/3] virtio-pci: use virtio_force_modern() 2021-10-28 22:00 [RFC PATCH v1 0/3] virtio: early detect 'modern' virtio Halil Pasic 2021-10-28 22:00 ` [RFC PATCH v1 1/3] virtio: introduce virtio_force_modern() Halil Pasic 2021-10-28 22:00 ` [RFC PATCH v1 2/3] virtio-ccw: use virtio_force_modern Halil Pasic @ 2021-10-28 22:00 ` Halil Pasic 2021-11-05 7:42 ` [RFC PATCH v1 0/3] virtio: early detect 'modern' virtio Michael S. Tsirkin 3 siblings, 0 replies; 10+ messages in thread From: Halil Pasic @ 2021-10-28 22:00 UTC (permalink / raw) To: Michael S. Tsirkin, Cornelia Huck, Halil Pasic, qemu-s390x, qemu-devel Cc: Christian Borntraeger, Thomas Huth, Richard Henderson, David Hildenbrand Let us detect usage via the modern interface by tapping into the place that implements the 'modern' reset. Signed-off-by: Halil Pasic <pasic@linux.ibm.com> --- hw/virtio/virtio-pci.c | 1 + 1 file changed, 1 insertion(+) diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index 6e16e2705c..8dd862da21 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -1297,6 +1297,7 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr, if (vdev->status == 0) { virtio_pci_reset(DEVICE(proxy)); + virtio_force_modern(virtio_bus_get_device(&proxy->bus)); } break; -- 2.25.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [RFC PATCH v1 0/3] virtio: early detect 'modern' virtio 2021-10-28 22:00 [RFC PATCH v1 0/3] virtio: early detect 'modern' virtio Halil Pasic ` (2 preceding siblings ...) 2021-10-28 22:00 ` [RFC PATCH v1 3/3] virtio-pci: use virtio_force_modern() Halil Pasic @ 2021-11-05 7:42 ` Michael S. Tsirkin 2021-11-09 10:12 ` Halil Pasic 3 siblings, 1 reply; 10+ messages in thread From: Michael S. Tsirkin @ 2021-11-05 7:42 UTC (permalink / raw) To: Halil Pasic Cc: Thomas Huth, David Hildenbrand, Cornelia Huck, Richard Henderson, qemu-devel, Christian Borntraeger, qemu-s390x On Fri, Oct 29, 2021 at 12:00:14AM +0200, Halil Pasic wrote: > This is an early RFC for a transport specific early detecton of > modern virtio, which is most relevant for transitional devices on big > endian platforms, when drivers access the config space before > FEATURES_OK is set. > > The most important part that is missing here is fixing the problems that > arrise in the situation described in the previous paragraph, when the > config is managed by a vhost device (and thus outside QEMU). > > This series was only lightly tested. I think it's a good enough approach, and we are getting close to release. For vhost - a new callback just for that? Would be ok. Or just invoke set features - if this works. > Halil Pasic (3): > virtio: introduce virtio_force_modern() > virtio-ccw: use virtio_force_modern > virtio-pci: use virtio_force_modern() > > hw/s390x/virtio-ccw.c | 3 +++ > hw/virtio/virtio-pci.c | 1 + > hw/virtio/virtio.c | 12 ++++++++++++ > include/hw/virtio/virtio.h | 1 + > 4 files changed, 17 insertions(+) > > > base-commit: 2c3e83f92d93fbab071b8a96b8ab769b01902475 > -- > 2.25.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH v1 0/3] virtio: early detect 'modern' virtio 2021-11-05 7:42 ` [RFC PATCH v1 0/3] virtio: early detect 'modern' virtio Michael S. Tsirkin @ 2021-11-09 10:12 ` Halil Pasic 0 siblings, 0 replies; 10+ messages in thread From: Halil Pasic @ 2021-11-09 10:12 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Thomas Huth, David Hildenbrand, Cornelia Huck, Richard Henderson, qemu-devel, Halil Pasic, Christian Borntraeger, qemu-s390x On Fri, 5 Nov 2021 03:42:33 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote: > > This series was only lightly tested. > > I think it's a good enough approach, and we are getting close > to release. For vhost - a new callback just for that? Would be ok. > Or just invoke set features - if this works. As of today I'm back from vacation. I intend to handle this problem/series with priority. Regards, Halil ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-11-12 16:36 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-28 22:00 [RFC PATCH v1 0/3] virtio: early detect 'modern' virtio Halil Pasic 2021-10-28 22:00 ` [RFC PATCH v1 1/3] virtio: introduce virtio_force_modern() Halil Pasic 2021-10-29 14:53 ` Cornelia Huck 2021-11-12 15:42 ` Halil Pasic 2021-11-12 15:55 ` Cornelia Huck 2021-11-12 16:36 ` Halil Pasic 2021-10-28 22:00 ` [RFC PATCH v1 2/3] virtio-ccw: use virtio_force_modern Halil Pasic 2021-10-28 22:00 ` [RFC PATCH v1 3/3] virtio-pci: use virtio_force_modern() Halil Pasic 2021-11-05 7:42 ` [RFC PATCH v1 0/3] virtio: early detect 'modern' virtio Michael S. Tsirkin 2021-11-09 10:12 ` Halil Pasic
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).