| Message ID | 20150506120737.8607.30158.stgit@bahia.huguette.org |
|---|---|
| State | New |
| Headers | show |
On Wed, 06 May 2015 14:07:37 +0200 Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > Unlike with add and clear, there is no valid reason to abort when checking > for a feature. It makes more sense to return false (i.e. the feature bit > isn't set). This is exactly what __virtio_has_feature() does if fbit >= 32. > > This allows to introduce code that is aware about new 64-bit features like > VIRTIO_F_VERSION_1, even if they are still not implemented. > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > --- > include/hw/virtio/virtio.h | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h > index d95f8b6..6ef70f1 100644 > --- a/include/hw/virtio/virtio.h > +++ b/include/hw/virtio/virtio.h > @@ -233,7 +233,6 @@ static inline void virtio_clear_feature(uint32_t *features, unsigned int fbit) > > static inline bool __virtio_has_feature(uint32_t features, unsigned int fbit) > { > - assert(fbit < 32); > return !!(features & (1 << fbit)); > } > > > I must say I'm not very comfortable with knowingly passing out-of-rage values to this function. Can we perhaps apply at least the feature-bit-size extending patches prior to your patchset, if the remainder of the virtio-1 patchset still takes some time?
On Tue, May 12, 2015 at 03:14:53PM +0200, Cornelia Huck wrote: > On Wed, 06 May 2015 14:07:37 +0200 > Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > > > Unlike with add and clear, there is no valid reason to abort when checking > > for a feature. It makes more sense to return false (i.e. the feature bit > > isn't set). This is exactly what __virtio_has_feature() does if fbit >= 32. > > > > This allows to introduce code that is aware about new 64-bit features like > > VIRTIO_F_VERSION_1, even if they are still not implemented. > > > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > > --- > > include/hw/virtio/virtio.h | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h > > index d95f8b6..6ef70f1 100644 > > --- a/include/hw/virtio/virtio.h > > +++ b/include/hw/virtio/virtio.h > > @@ -233,7 +233,6 @@ static inline void virtio_clear_feature(uint32_t *features, unsigned int fbit) > > > > static inline bool __virtio_has_feature(uint32_t features, unsigned int fbit) > > { > > - assert(fbit < 32); > > return !!(features & (1 << fbit)); > > } > > > > > > > > I must say I'm not very comfortable with knowingly passing out-of-rage > values to this function. > > Can we perhaps apply at least the feature-bit-size extending patches > prior to your patchset, if the remainder of the virtio-1 patchset still > takes some time? So the feature-bit-size extending patches currently don't support migration correctly, that's why they are not merged. What I think we need to do for this is move host_features out from transports into core virtio device. Then we can simply check host features >31 and skip migrating low guest features is none set. Thoughts? Any takers?
On Tue, 12 May 2015 15:34:47 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Tue, May 12, 2015 at 03:14:53PM +0200, Cornelia Huck wrote: > > On Wed, 06 May 2015 14:07:37 +0200 > > Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > > > > > Unlike with add and clear, there is no valid reason to abort when checking > > > for a feature. It makes more sense to return false (i.e. the feature bit > > > isn't set). This is exactly what __virtio_has_feature() does if fbit >= 32. > > > > > > This allows to introduce code that is aware about new 64-bit features like > > > VIRTIO_F_VERSION_1, even if they are still not implemented. > > > > > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > > > --- > > > include/hw/virtio/virtio.h | 1 - > > > 1 file changed, 1 deletion(-) > > > > > > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h > > > index d95f8b6..6ef70f1 100644 > > > --- a/include/hw/virtio/virtio.h > > > +++ b/include/hw/virtio/virtio.h > > > @@ -233,7 +233,6 @@ static inline void virtio_clear_feature(uint32_t *features, unsigned int fbit) > > > > > > static inline bool __virtio_has_feature(uint32_t features, unsigned int fbit) > > > { > > > - assert(fbit < 32); > > > return !!(features & (1 << fbit)); > > > } > > > > > > > > > > > > > I must say I'm not very comfortable with knowingly passing out-of-rage > > values to this function. > > > > Can we perhaps apply at least the feature-bit-size extending patches > > prior to your patchset, if the remainder of the virtio-1 patchset still > > takes some time? > > So the feature-bit-size extending patches currently don't support > migration correctly, that's why they are not merged. > > What I think we need to do for this is move host_features out > from transports into core virtio device. > > Then we can simply check host features >31 and skip > migrating low guest features is none set. > > Thoughts? Any takers? > After we move host_features, put them into an optional vmstate subsection? I think with the recent patchsets, most of the interesting stuff is already not handled by the transport anymore. There's only VIRTIO_F_NOTIFY_ON_EMPTY and VIRTIO_F_BAD_FEATURE left (set by pci and ccw).
On 12 May 2015 at 14:14, Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > On Wed, 06 May 2015 14:07:37 +0200 > Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: >> @@ -233,7 +233,6 @@ static inline void virtio_clear_feature(uint32_t *features, unsigned int fbit) >> >> static inline bool __virtio_has_feature(uint32_t features, unsigned int fbit) >> { >> - assert(fbit < 32); >> return !!(features & (1 << fbit)); >> } >> >> >> > > I must say I'm not very comfortable with knowingly passing out-of-rage > values to this function. It would invoke C undefined behaviour, so clearly a bug if we did pass an out-of-range value here. You'd need to at least do if (fbit >= 32) { return false; } if you want to make it valid. -- PMM
On Tue, 12 May 2015 15:14:53 +0200 Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > On Wed, 06 May 2015 14:07:37 +0200 > Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > > > Unlike with add and clear, there is no valid reason to abort when checking > > for a feature. It makes more sense to return false (i.e. the feature bit > > isn't set). This is exactly what __virtio_has_feature() does if fbit >= 32. > > > > This allows to introduce code that is aware about new 64-bit features like > > VIRTIO_F_VERSION_1, even if they are still not implemented. > > > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > > --- > > include/hw/virtio/virtio.h | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h > > index d95f8b6..6ef70f1 100644 > > --- a/include/hw/virtio/virtio.h > > +++ b/include/hw/virtio/virtio.h > > @@ -233,7 +233,6 @@ static inline void virtio_clear_feature(uint32_t *features, unsigned int fbit) > > > > static inline bool __virtio_has_feature(uint32_t features, unsigned int fbit) > > { > > - assert(fbit < 32); > > return !!(features & (1 << fbit)); > > } > > > > > > > > I must say I'm not very comfortable with knowingly passing out-of-rage > values to this function. > I take that as a valid reason then :) > Can we perhaps apply at least the feature-bit-size extending patches > prior to your patchset, if the remainder of the virtio-1 patchset still > takes some time? Hmm... if I remember well, it still lacks migration support. -- Greg
On Tue, 12 May 2015 15:44:46 +0200 Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > On Tue, 12 May 2015 15:34:47 +0200 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Tue, May 12, 2015 at 03:14:53PM +0200, Cornelia Huck wrote: > > > On Wed, 06 May 2015 14:07:37 +0200 > > > Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > > > > > > > Unlike with add and clear, there is no valid reason to abort when checking > > > > for a feature. It makes more sense to return false (i.e. the feature bit > > > > isn't set). This is exactly what __virtio_has_feature() does if fbit >= 32. > > > > > > > > This allows to introduce code that is aware about new 64-bit features like > > > > VIRTIO_F_VERSION_1, even if they are still not implemented. > > > > > > > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > > > > --- > > > > include/hw/virtio/virtio.h | 1 - > > > > 1 file changed, 1 deletion(-) > > > > > > > > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h > > > > index d95f8b6..6ef70f1 100644 > > > > --- a/include/hw/virtio/virtio.h > > > > +++ b/include/hw/virtio/virtio.h > > > > @@ -233,7 +233,6 @@ static inline void virtio_clear_feature(uint32_t *features, unsigned int fbit) > > > > > > > > static inline bool __virtio_has_feature(uint32_t features, unsigned int fbit) > > > > { > > > > - assert(fbit < 32); > > > > return !!(features & (1 << fbit)); > > > > } > > > > > > > > > > > > > > > > > > I must say I'm not very comfortable with knowingly passing out-of-rage > > > values to this function. > > > > > > Can we perhaps apply at least the feature-bit-size extending patches > > > prior to your patchset, if the remainder of the virtio-1 patchset still > > > takes some time? > > > > So the feature-bit-size extending patches currently don't support > > migration correctly, that's why they are not merged. > > > > What I think we need to do for this is move host_features out > > from transports into core virtio device. > > > > Then we can simply check host features >31 and skip > > migrating low guest features is none set. > > > > Thoughts? Any takers? > > > > After we move host_features, put them into an optional vmstate > subsection? > > I think with the recent patchsets, most of the interesting stuff is > already not handled by the transport anymore. There's only > VIRTIO_F_NOTIFY_ON_EMPTY and VIRTIO_F_BAD_FEATURE left (set by pci and > ccw). Thinking a bit more, we probably don't need this move of host_features to get migration right (although it might be a nice cleanup later). Could we - keep migration of bits 0..31 as-is - add a vmstate subsection for bits 32..63 only included if one of those bits is set - have a post handler that performs a validation of the full set of bits 0..63 ? We could do a similar exercise with a subsection containing the addresses for avail and used with a post handler overwriting any addresses set by the old style migration code. Does that make sense?
On Tue, May 12, 2015 at 04:46:11PM +0200, Cornelia Huck wrote: > On Tue, 12 May 2015 15:44:46 +0200 > Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > > > On Tue, 12 May 2015 15:34:47 +0200 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > On Tue, May 12, 2015 at 03:14:53PM +0200, Cornelia Huck wrote: > > > > On Wed, 06 May 2015 14:07:37 +0200 > > > > Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > > > > > > > > > Unlike with add and clear, there is no valid reason to abort when checking > > > > > for a feature. It makes more sense to return false (i.e. the feature bit > > > > > isn't set). This is exactly what __virtio_has_feature() does if fbit >= 32. > > > > > > > > > > This allows to introduce code that is aware about new 64-bit features like > > > > > VIRTIO_F_VERSION_1, even if they are still not implemented. > > > > > > > > > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > > > > > --- > > > > > include/hw/virtio/virtio.h | 1 - > > > > > 1 file changed, 1 deletion(-) > > > > > > > > > > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h > > > > > index d95f8b6..6ef70f1 100644 > > > > > --- a/include/hw/virtio/virtio.h > > > > > +++ b/include/hw/virtio/virtio.h > > > > > @@ -233,7 +233,6 @@ static inline void virtio_clear_feature(uint32_t *features, unsigned int fbit) > > > > > > > > > > static inline bool __virtio_has_feature(uint32_t features, unsigned int fbit) > > > > > { > > > > > - assert(fbit < 32); > > > > > return !!(features & (1 << fbit)); > > > > > } > > > > > > > > > > > > > > > > > > > > > > > I must say I'm not very comfortable with knowingly passing out-of-rage > > > > values to this function. > > > > > > > > Can we perhaps apply at least the feature-bit-size extending patches > > > > prior to your patchset, if the remainder of the virtio-1 patchset still > > > > takes some time? > > > > > > So the feature-bit-size extending patches currently don't support > > > migration correctly, that's why they are not merged. > > > > > > What I think we need to do for this is move host_features out > > > from transports into core virtio device. > > > > > > Then we can simply check host features >31 and skip > > > migrating low guest features is none set. > > > > > > Thoughts? Any takers? > > > > > > > After we move host_features, put them into an optional vmstate > > subsection? > > > > I think with the recent patchsets, most of the interesting stuff is > > already not handled by the transport anymore. There's only > > VIRTIO_F_NOTIFY_ON_EMPTY and VIRTIO_F_BAD_FEATURE left (set by pci and > > ccw). notify on empty is likely safe to set for everyone. bad feature should be pci specific, it's a mistake that we have it in ccw. it's there to detect very old buggy guests. in fact ccw ignores this bit completely. For PCI, I think VIRTIO_F_BAD_FEATURE is never actually set in guest features. If guest attempts to set it, it is immediately cleared. So it can be handled in pci specific code, and won't affect migration. > Thinking a bit more, we probably don't need this move of host_features > to get migration right (although it might be a nice cleanup later). > > Could we > - keep migration of bits 0..31 as-is > - add a vmstate subsection for bits 32..63 only included if one of > those bits is set > - have a post handler that performs a validation of the full set of > bits 0..63 > ? > > We could do a similar exercise with a subsection containing the > addresses for avail and used with a post handler overwriting any > addresses set by the old style migration code. > > Does that make sense? I don't see how it does: on the receive side you don't know whether guest acked bits 32..63 so you can't decide whether to parse bits 32..63. The right thing to do IMHO is to migrate the high guest bits if and only if the *host* bits 32..63 are set. And that needs the host features in core, or at least is easier if they are there.
On Tue, 12 May 2015 17:30:21 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Tue, May 12, 2015 at 04:46:11PM +0200, Cornelia Huck wrote: > > On Tue, 12 May 2015 15:44:46 +0200 > > Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > > > > > On Tue, 12 May 2015 15:34:47 +0200 > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > On Tue, May 12, 2015 at 03:14:53PM +0200, Cornelia Huck wrote: > > > > > On Wed, 06 May 2015 14:07:37 +0200 > > > > > Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > > > > > > > > > > > Unlike with add and clear, there is no valid reason to abort when checking > > > > > > for a feature. It makes more sense to return false (i.e. the feature bit > > > > > > isn't set). This is exactly what __virtio_has_feature() does if fbit >= 32. > > > > > > > > > > > > This allows to introduce code that is aware about new 64-bit features like > > > > > > VIRTIO_F_VERSION_1, even if they are still not implemented. > > > > > > > > > > > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > > > > > > --- > > > > > > include/hw/virtio/virtio.h | 1 - > > > > > > 1 file changed, 1 deletion(-) > > > > > > > > > > > > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h > > > > > > index d95f8b6..6ef70f1 100644 > > > > > > --- a/include/hw/virtio/virtio.h > > > > > > +++ b/include/hw/virtio/virtio.h > > > > > > @@ -233,7 +233,6 @@ static inline void virtio_clear_feature(uint32_t *features, unsigned int fbit) > > > > > > > > > > > > static inline bool __virtio_has_feature(uint32_t features, unsigned int fbit) > > > > > > { > > > > > > - assert(fbit < 32); > > > > > > return !!(features & (1 << fbit)); > > > > > > } > > > > > > > > > > > > > > > > > > > > > > > > > > > > I must say I'm not very comfortable with knowingly passing out-of-rage > > > > > values to this function. > > > > > > > > > > Can we perhaps apply at least the feature-bit-size extending patches > > > > > prior to your patchset, if the remainder of the virtio-1 patchset still > > > > > takes some time? > > > > > > > > So the feature-bit-size extending patches currently don't support > > > > migration correctly, that's why they are not merged. > > > > > > > > What I think we need to do for this is move host_features out > > > > from transports into core virtio device. > > > > > > > > Then we can simply check host features >31 and skip > > > > migrating low guest features is none set. > > > > > > > > Thoughts? Any takers? > > > > > > > > > > After we move host_features, put them into an optional vmstate > > > subsection? > > > > > > I think with the recent patchsets, most of the interesting stuff is > > > already not handled by the transport anymore. There's only > > > VIRTIO_F_NOTIFY_ON_EMPTY and VIRTIO_F_BAD_FEATURE left (set by pci and > > > ccw). > > notify on empty is likely safe to set for everyone. > > bad feature should be pci specific, it's a mistake that > we have it in ccw. it's there to detect very old buggy guests. > in fact ccw ignores this bit completely. > > For PCI, I think VIRTIO_F_BAD_FEATURE is never > actually set in guest features. If guest attempts to set it, > it is immediately cleared. > > So it can be handled in pci specific code, and won't > affect migration. > > > > Thinking a bit more, we probably don't need this move of host_features > > to get migration right (although it might be a nice cleanup later). > > > > Could we > > - keep migration of bits 0..31 as-is > > - add a vmstate subsection for bits 32..63 only included if one of > > those bits is set > > - have a post handler that performs a validation of the full set of > > bits 0..63 > > ? > > > > We could do a similar exercise with a subsection containing the > > addresses for avail and used with a post handler overwriting any > > addresses set by the old style migration code. > > > > Does that make sense? > > I don't see how it does: on the receive side you don't know > whether guest acked bits 32..63 so you can't decide whether > to parse bits 32..63. But if it wasn't set, it obviously wasn't acked, I'd think? > > The right thing to do IMHO is to migrate the high guest bits if and only > if the *host* bits 32..63 are set. And that needs the host features in > core, or at least is easier if they are there. Aren't the host bits a prereq? Confused. I'll think about that tomorrow when it's hopefully a bit cooler around here :)
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h index d95f8b6..6ef70f1 100644 --- a/include/hw/virtio/virtio.h +++ b/include/hw/virtio/virtio.h @@ -233,7 +233,6 @@ static inline void virtio_clear_feature(uint32_t *features, unsigned int fbit) static inline bool __virtio_has_feature(uint32_t features, unsigned int fbit) { - assert(fbit < 32); return !!(features & (1 << fbit)); }
Unlike with add and clear, there is no valid reason to abort when checking for a feature. It makes more sense to return false (i.e. the feature bit isn't set). This is exactly what __virtio_has_feature() does if fbit >= 32. This allows to introduce code that is aware about new 64-bit features like VIRTIO_F_VERSION_1, even if they are still not implemented. Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> --- include/hw/virtio/virtio.h | 1 - 1 file changed, 1 deletion(-)