| Message ID | 20210729191910.317114-2-lvivier@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtio: failover: allow to keep the VFIO device rather than the virtio-net one | expand |
在 2021/7/30 上午3:19, Laurent Vivier 写道: > Add virtio_queue_disable()/virtio_queue_enable() to disable/enable a queue > by setting vring.num to 0 (or num_default). > This is needed to be able to disable a guest driver from the host side I suspect this won't work correclty for vhost. And I believe we should only do this after the per queue enabling/disabling is supported by the spec. (only MMIO support that AFAIK) Thanks > > Signed-off-by: Laurent Vivier <lvivier@redhat.com> > --- > include/hw/virtio/virtio.h | 2 ++ > hw/virtio/virtio.c | 10 ++++++++++ > 2 files changed, 12 insertions(+) > > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h > index 8bab9cfb7507..6a3f71b4cd88 100644 > --- a/include/hw/virtio/virtio.h > +++ b/include/hw/virtio/virtio.h > @@ -251,6 +251,8 @@ void virtio_config_modern_writel(VirtIODevice *vdev, > uint32_t addr, uint32_t data); > void virtio_queue_set_addr(VirtIODevice *vdev, int n, hwaddr addr); > hwaddr virtio_queue_get_addr(VirtIODevice *vdev, int n); > +void virtio_queue_enable(VirtIODevice *vdev, int n); > +void virtio_queue_disable(VirtIODevice *vdev, int n); > void virtio_queue_set_num(VirtIODevice *vdev, int n, int num); > int virtio_queue_get_num(VirtIODevice *vdev, int n); > int virtio_queue_get_max_num(VirtIODevice *vdev, int n); > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index 874377f37a70..fa5228c1a2d6 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -2244,6 +2244,16 @@ void virtio_queue_set_rings(VirtIODevice *vdev, int n, hwaddr desc, > virtio_init_region_cache(vdev, n); > } > > +void virtio_queue_disable(VirtIODevice *vdev, int n) > +{ > + vdev->vq[n].vring.num = 0; > +} > + > +void virtio_queue_enable(VirtIODevice *vdev, int n) > +{ > + vdev->vq[n].vring.num = vdev->vq[n].vring.num_default; > +} > + > void virtio_queue_set_num(VirtIODevice *vdev, int n, int num) > { > /* Don't allow guest to flip queue between existent and
On 02/08/2021 06:50, Jason Wang wrote: > > 在 2021/7/30 上午3:19, Laurent Vivier 写道: >> Add virtio_queue_disable()/virtio_queue_enable() to disable/enable a queue >> by setting vring.num to 0 (or num_default). >> This is needed to be able to disable a guest driver from the host side > > > I suspect this won't work correclty for vhost. With my test it seems to work with vhost too. > > And I believe we should only do this after the per queue enabling/disabling is supported > by the spec. > > (only MMIO support that AFAIK) I don't want to modify the spec. I need something that works without modifying existing (old) drivers. The idea is to be able to disable the virtio-net kernel driver from QEMU if the driver is too old (i.e. it doesn't support STANDBY feature). Setting vring.num to 0 forces the kernel driver to exit on error in the probe function. It's what I want: the device is present but disabled (the driver is not loaded). Any other suggestion? Thanks, Laurent > Thanks > > >> >> Signed-off-by: Laurent Vivier <lvivier@redhat.com> >> --- >> include/hw/virtio/virtio.h | 2 ++ >> hw/virtio/virtio.c | 10 ++++++++++ >> 2 files changed, 12 insertions(+) >> >> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h >> index 8bab9cfb7507..6a3f71b4cd88 100644 >> --- a/include/hw/virtio/virtio.h >> +++ b/include/hw/virtio/virtio.h >> @@ -251,6 +251,8 @@ void virtio_config_modern_writel(VirtIODevice *vdev, >> uint32_t addr, uint32_t data); >> void virtio_queue_set_addr(VirtIODevice *vdev, int n, hwaddr addr); >> hwaddr virtio_queue_get_addr(VirtIODevice *vdev, int n); >> +void virtio_queue_enable(VirtIODevice *vdev, int n); >> +void virtio_queue_disable(VirtIODevice *vdev, int n); >> void virtio_queue_set_num(VirtIODevice *vdev, int n, int num); >> int virtio_queue_get_num(VirtIODevice *vdev, int n); >> int virtio_queue_get_max_num(VirtIODevice *vdev, int n); >> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >> index 874377f37a70..fa5228c1a2d6 100644 >> --- a/hw/virtio/virtio.c >> +++ b/hw/virtio/virtio.c >> @@ -2244,6 +2244,16 @@ void virtio_queue_set_rings(VirtIODevice *vdev, int n, hwaddr desc, >> virtio_init_region_cache(vdev, n); >> } >> +void virtio_queue_disable(VirtIODevice *vdev, int n) >> +{ >> + vdev->vq[n].vring.num = 0; >> +} >> + >> +void virtio_queue_enable(VirtIODevice *vdev, int n) >> +{ >> + vdev->vq[n].vring.num = vdev->vq[n].vring.num_default; >> +} >> + >> void virtio_queue_set_num(VirtIODevice *vdev, int n, int num) >> { >> /* Don't allow guest to flip queue between existent and >
在 2021/8/2 下午4:42, Laurent Vivier 写道: > On 02/08/2021 06:50, Jason Wang wrote: >> 在 2021/7/30 上午3:19, Laurent Vivier 写道: >>> Add virtio_queue_disable()/virtio_queue_enable() to disable/enable a queue >>> by setting vring.num to 0 (or num_default). >>> This is needed to be able to disable a guest driver from the host side >> >> I suspect this won't work correclty for vhost. > With my test it seems to work with vhost too. So setting 0 will lead -EINVAL to be returned during VHOST_SET_VRING_NUM. I think qemu will warn the failure in this case. What's more important, it's not guaranteed to work for the case of vhost-user or vhost-vDPA. > >> And I believe we should only do this after the per queue enabling/disabling is supported >> by the spec. >> >> (only MMIO support that AFAIK) > I don't want to modify the spec. > > I need something that works without modifying existing (old) drivers. > > The idea is to be able to disable the virtio-net kernel driver from QEMU if the driver is > too old (i.e. it doesn't support STANDBY feature). > > Setting vring.num to 0 forces the kernel driver to exit on error in the probe function. > It's what I want: the device is present but disabled (the driver is not loaded). > > Any other suggestion? I think we should probably disable the device instead of doing it per virtqueue. Thanks > > Thanks, > Laurent > >> Thanks >> >> >>> Signed-off-by: Laurent Vivier <lvivier@redhat.com> >>> --- >>> include/hw/virtio/virtio.h | 2 ++ >>> hw/virtio/virtio.c | 10 ++++++++++ >>> 2 files changed, 12 insertions(+) >>> >>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h >>> index 8bab9cfb7507..6a3f71b4cd88 100644 >>> --- a/include/hw/virtio/virtio.h >>> +++ b/include/hw/virtio/virtio.h >>> @@ -251,6 +251,8 @@ void virtio_config_modern_writel(VirtIODevice *vdev, >>> uint32_t addr, uint32_t data); >>> void virtio_queue_set_addr(VirtIODevice *vdev, int n, hwaddr addr); >>> hwaddr virtio_queue_get_addr(VirtIODevice *vdev, int n); >>> +void virtio_queue_enable(VirtIODevice *vdev, int n); >>> +void virtio_queue_disable(VirtIODevice *vdev, int n); >>> void virtio_queue_set_num(VirtIODevice *vdev, int n, int num); >>> int virtio_queue_get_num(VirtIODevice *vdev, int n); >>> int virtio_queue_get_max_num(VirtIODevice *vdev, int n); >>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >>> index 874377f37a70..fa5228c1a2d6 100644 >>> --- a/hw/virtio/virtio.c >>> +++ b/hw/virtio/virtio.c >>> @@ -2244,6 +2244,16 @@ void virtio_queue_set_rings(VirtIODevice *vdev, int n, hwaddr desc, >>> virtio_init_region_cache(vdev, n); >>> } >>> +void virtio_queue_disable(VirtIODevice *vdev, int n) >>> +{ >>> + vdev->vq[n].vring.num = 0; >>> +} >>> + >>> +void virtio_queue_enable(VirtIODevice *vdev, int n) >>> +{ >>> + vdev->vq[n].vring.num = vdev->vq[n].vring.num_default; >>> +} >>> + >>> void virtio_queue_set_num(VirtIODevice *vdev, int n, int num) >>> { >>> /* Don't allow guest to flip queue between existent and
On 06/08/2021 08:25, Jason Wang wrote: > > 在 2021/8/2 下午4:42, Laurent Vivier 写道: >> On 02/08/2021 06:50, Jason Wang wrote: >>> 在 2021/7/30 上午3:19, Laurent Vivier 写道: >>>> Add virtio_queue_disable()/virtio_queue_enable() to disable/enable a queue >>>> by setting vring.num to 0 (or num_default). >>>> This is needed to be able to disable a guest driver from the host side >>> >>> I suspect this won't work correclty for vhost. >> With my test it seems to work with vhost too. > > > So setting 0 will lead -EINVAL to be returned during VHOST_SET_VRING_NUM. I think qemu > will warn the failure in this case. I didn't see any error when I tried. I will check the code. > What's more important, it's not guaranteed to work for the case of vhost-user or vhost-vDPA. Perhaps we can target only the vhost host case, as this is used for failover and usually the virtio-net device is backed by a bridge on same network as the VFIO device? > > >> >>> And I believe we should only do this after the per queue enabling/disabling is supported >>> by the spec. >>> >>> (only MMIO support that AFAIK) >> I don't want to modify the spec. >> >> I need something that works without modifying existing (old) drivers. >> >> The idea is to be able to disable the virtio-net kernel driver from QEMU if the driver is >> too old (i.e. it doesn't support STANDBY feature). >> >> Setting vring.num to 0 forces the kernel driver to exit on error in the probe function. >> It's what I want: the device is present but disabled (the driver is not loaded). >> >> Any other suggestion? > > > I think we should probably disable the device instead of doing it per virtqueue. > I tried to use virtio_set_disabled() but it doesn't work. Perhaps it's too late when I call the function (I need to do that in virtio_net_set_features()). What I want is to prevent the load of the driver in the guest kernel to hide the virtio-net device. Setting vring.num to 0 triggers an error in the driver probe function and prevents the load of the driver. Thanks, Laurent
在 2021/8/6 下午3:27, Laurent Vivier 写道: > On 06/08/2021 08:25, Jason Wang wrote: >> 在 2021/8/2 下午4:42, Laurent Vivier 写道: >>> On 02/08/2021 06:50, Jason Wang wrote: >>>> 在 2021/7/30 上午3:19, Laurent Vivier 写道: >>>>> Add virtio_queue_disable()/virtio_queue_enable() to disable/enable a queue >>>>> by setting vring.num to 0 (or num_default). >>>>> This is needed to be able to disable a guest driver from the host side >>>> I suspect this won't work correclty for vhost. >>> With my test it seems to work with vhost too. >> >> So setting 0 will lead -EINVAL to be returned during VHOST_SET_VRING_NUM. I think qemu >> will warn the failure in this case. > I didn't see any error when I tried. I will check the code. > >> What's more important, it's not guaranteed to work for the case of vhost-user or vhost-vDPA. > Perhaps we can target only the vhost host case, as this is used for failover and usually > the virtio-net device is backed by a bridge on same network as the VFIO device? Probably not, it should be a general feature that can work for all types of virtio/vhost backends. > >> >>>> And I believe we should only do this after the per queue enabling/disabling is supported >>>> by the spec. >>>> >>>> (only MMIO support that AFAIK) >>> I don't want to modify the spec. >>> >>> I need something that works without modifying existing (old) drivers. >>> >>> The idea is to be able to disable the virtio-net kernel driver from QEMU if the driver is >>> too old (i.e. it doesn't support STANDBY feature). >>> >>> Setting vring.num to 0 forces the kernel driver to exit on error in the probe function. >>> It's what I want: the device is present but disabled (the driver is not loaded). >>> >>> Any other suggestion? >> >> I think we should probably disable the device instead of doing it per virtqueue. >> > I tried to use virtio_set_disabled() but it doesn't work. > Perhaps it's too late when I call the function (I need to do that in > virtio_net_set_features()). What I want is to prevent the load of the driver in the guest > kernel to hide the virtio-net device. Setting vring.num to 0 triggers an error in the > driver probe function and prevents the load of the driver. How about fail the validate_features() in this case? Thanks > > Thanks, > Laurent >
On 09/08/2021 05:01, Jason Wang wrote: > > 在 2021/8/6 下午3:27, Laurent Vivier 写道: >> On 06/08/2021 08:25, Jason Wang wrote: >>> 在 2021/8/2 下午4:42, Laurent Vivier 写道: >>>> On 02/08/2021 06:50, Jason Wang wrote: >>>>> 在 2021/7/30 上午3:19, Laurent Vivier 写道: >>>>>> Add virtio_queue_disable()/virtio_queue_enable() to disable/enable a queue >>>>>> by setting vring.num to 0 (or num_default). >>>>>> This is needed to be able to disable a guest driver from the host side >>>>> I suspect this won't work correclty for vhost. >>>> With my test it seems to work with vhost too. >>> >>> So setting 0 will lead -EINVAL to be returned during VHOST_SET_VRING_NUM. I think qemu >>> will warn the failure in this case. >> I didn't see any error when I tried. I will check the code. >> >>> What's more important, it's not guaranteed to work for the case of vhost-user or >>> vhost-vDPA. >> Perhaps we can target only the vhost host case, as this is used for failover and usually >> the virtio-net device is backed by a bridge on same network as the VFIO device? > > > Probably not, it should be a general feature that can work for all types of virtio/vhost > backends. > > >> >>> >>>>> And I believe we should only do this after the per queue enabling/disabling is supported >>>>> by the spec. >>>>> >>>>> (only MMIO support that AFAIK) >>>> I don't want to modify the spec. >>>> >>>> I need something that works without modifying existing (old) drivers. >>>> >>>> The idea is to be able to disable the virtio-net kernel driver from QEMU if the driver is >>>> too old (i.e. it doesn't support STANDBY feature). >>>> >>>> Setting vring.num to 0 forces the kernel driver to exit on error in the probe function. >>>> It's what I want: the device is present but disabled (the driver is not loaded). >>>> >>>> Any other suggestion? >>> >>> I think we should probably disable the device instead of doing it per virtqueue. >>> >> I tried to use virtio_set_disabled() but it doesn't work. >> Perhaps it's too late when I call the function (I need to do that in >> virtio_net_set_features()). What I want is to prevent the load of the driver in the guest >> kernel to hide the virtio-net device. Setting vring.num to 0 triggers an error in the >> driver probe function and prevents the load of the driver. > > > How about fail the validate_features() in this case? It's a good suggestion and it seems to work. I'm going to send an updated patch. Thanks, Laurent
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h index 8bab9cfb7507..6a3f71b4cd88 100644 --- a/include/hw/virtio/virtio.h +++ b/include/hw/virtio/virtio.h @@ -251,6 +251,8 @@ void virtio_config_modern_writel(VirtIODevice *vdev, uint32_t addr, uint32_t data); void virtio_queue_set_addr(VirtIODevice *vdev, int n, hwaddr addr); hwaddr virtio_queue_get_addr(VirtIODevice *vdev, int n); +void virtio_queue_enable(VirtIODevice *vdev, int n); +void virtio_queue_disable(VirtIODevice *vdev, int n); void virtio_queue_set_num(VirtIODevice *vdev, int n, int num); int virtio_queue_get_num(VirtIODevice *vdev, int n); int virtio_queue_get_max_num(VirtIODevice *vdev, int n); diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 874377f37a70..fa5228c1a2d6 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -2244,6 +2244,16 @@ void virtio_queue_set_rings(VirtIODevice *vdev, int n, hwaddr desc, virtio_init_region_cache(vdev, n); } +void virtio_queue_disable(VirtIODevice *vdev, int n) +{ + vdev->vq[n].vring.num = 0; +} + +void virtio_queue_enable(VirtIODevice *vdev, int n) +{ + vdev->vq[n].vring.num = vdev->vq[n].vring.num_default; +} + void virtio_queue_set_num(VirtIODevice *vdev, int n, int num) { /* Don't allow guest to flip queue between existent and
Add virtio_queue_disable()/virtio_queue_enable() to disable/enable a queue by setting vring.num to 0 (or num_default). This is needed to be able to disable a guest driver from the host side Signed-off-by: Laurent Vivier <lvivier@redhat.com> --- include/hw/virtio/virtio.h | 2 ++ hw/virtio/virtio.c | 10 ++++++++++ 2 files changed, 12 insertions(+)