Jason Wang
2014-Nov-13 09:11 UTC
[PATCH 1/2] virito: introduce methods of fixing device features
On 11/13/2014 04:46 PM, Cornelia Huck wrote:> On Thu, 13 Nov 2014 13:52:53 +0800 > Jason Wang <jasowang at redhat.com> wrote: > > typo in subject-prefix: s/virito/virtio/ >Will correct this.>> Buggy host may advertised buggy host features (a usual case is that host >> advertise a feature whose dependencies were missed). In this case, driver >> should detect and disable the buggy features by itself. >> >> This patch introduces driver specific fix_features() method which is called >> just before features finalizing to detect and disable buggy features >> advertised by host. > So the basic problem this patch fixes is that an individual driver may > only specify a static set of features but cannot specify any > dependencies, right?Right, and what even worse is qemu could not handle dependencies as well. So we need fix both sides.> Adding a sanitizer step makes sense, I guess. > >> Virtio-net will be the first user. >> >> Cc: Rusty Russell <rusty at rustcorp.com.au> >> Cc: Michael S. Tsirkin <mst at redhat.com> >> Signed-off-by: Jason Wang <jasowang at redhat.com> >> --- >> drivers/virtio/virtio.c | 4 ++++ >> include/linux/virtio.h | 1 + >> include/linux/virtio_config.h | 12 ++++++++++++ >> 3 files changed, 17 insertions(+) >> >> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c >> index df598dd..7001d6e 100644 >> --- a/drivers/virtio/virtio.c >> +++ b/drivers/virtio/virtio.c >> @@ -181,6 +181,10 @@ static int virtio_dev_probe(struct device *_d) >> if (device_features & (1 << i)) >> set_bit(i, dev->features); >> >> + /* Fix buggy features advertised by host */ >> + if (drv->fix_features) >> + drv->fix_features(dev); > I'd probably call this "sanitize_features" instead.Ok.>> + >> dev->config->finalize_features(dev); >> >> err = drv->probe(dev); >> diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h >> index 7f4ef66..7bd89ea 100644 >> --- a/include/linux/virtio_config.h >> +++ b/include/linux/virtio_config.h >> @@ -96,6 +96,18 @@ static inline bool virtio_has_feature(const struct virtio_device *vdev, >> return test_bit(fbit, vdev->features); >> } >> >> +static inline void virtio_disable_feature(struct virtio_device *vdev, >> + unsigned int fbit) >> +{ >> + BUG_ON(fbit >= VIRTIO_TRANSPORT_F_START); >> + BUG_ON(vdev->config->get_status(vdev) & >> + ~(VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER)); > When we add virtio-1 support, we can add a check for FEATURES_OK here, > so we're really on the safe side. >If I read the spec correctly, FEATURES_OK was set only after writing the features bits to device. But we want to sanitize the them before.>> + >> + virtio_check_driver_offered_feature(vdev, fbit); >> + >> + clear_bit(fbit, vdev->features); >> +} >> + >> static inline >> struct virtqueue *virtio_find_single_vq(struct virtio_device *vdev, >> vq_callback_t *c, const char *n) > The approach looks good to me. >Thanks for the review.
Cornelia Huck
2014-Nov-13 09:14 UTC
[PATCH 1/2] virito: introduce methods of fixing device features
On Thu, 13 Nov 2014 17:11:30 +0800 Jason Wang <jasowang at redhat.com> wrote:> On 11/13/2014 04:46 PM, Cornelia Huck wrote: > > On Thu, 13 Nov 2014 13:52:53 +0800 > > Jason Wang <jasowang at redhat.com> wrote:> >> +static inline void virtio_disable_feature(struct virtio_device *vdev, > >> + unsigned int fbit) > >> +{ > >> + BUG_ON(fbit >= VIRTIO_TRANSPORT_F_START); > >> + BUG_ON(vdev->config->get_status(vdev) & > >> + ~(VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER)); > > When we add virtio-1 support, we can add a check for FEATURES_OK here, > > so we're really on the safe side. > > > > If I read the spec correctly, FEATURES_OK was set only after writing the > features bits to device. But we want to sanitize the them before.I meant doing a BUG when FEATURES_OK is set - sorry for not being clear.
Jason Wang
2014-Nov-13 09:49 UTC
[PATCH 1/2] virito: introduce methods of fixing device features
On 11/13/2014 05:14 PM, Cornelia Huck wrote:> On Thu, 13 Nov 2014 17:11:30 +0800 > Jason Wang <jasowang at redhat.com> wrote: > >> On 11/13/2014 04:46 PM, Cornelia Huck wrote: >>> On Thu, 13 Nov 2014 13:52:53 +0800 >>> Jason Wang <jasowang at redhat.com> wrote: >>>> +static inline void virtio_disable_feature(struct virtio_device *vdev, >>>> + unsigned int fbit) >>>> +{ >>>> + BUG_ON(fbit >= VIRTIO_TRANSPORT_F_START); >>>> + BUG_ON(vdev->config->get_status(vdev) & >>>> + ~(VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER)); >>> When we add virtio-1 support, we can add a check for FEATURES_OK here, >>> so we're really on the safe side. >>> >> If I read the spec correctly, FEATURES_OK was set only after writing the >> features bits to device. But we want to sanitize the them before. > I meant doing a BUG when FEATURES_OK is set - sorry for not being clear. >I get it, thanks for the clarification.
Seemingly Similar Threads
- [PATCH 1/2] virito: introduce methods of fixing device features
- [PATCH 1/2] virito: introduce methods of fixing device features
- [PATCH 1/2] virito: introduce methods of fixing device features
- [PATCH 1/2] virito: introduce methods of fixing device features
- [PATCH 1/2] virito: introduce methods of fixing device features