| Message ID | 20230720181459.607008-8-eperezma@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Prefer to use SVQ to stall dataplane at NIC state restore through CVQ | expand |
On 7/20/2023 11:14 AM, Eugenio Pérez wrote: > Split out vq reset operation in its own function, as it may be called > with ring reset. > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > --- > hw/virtio/vhost-vdpa.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c > index 6ae276ccde..df2515a247 100644 > --- a/hw/virtio/vhost-vdpa.c > +++ b/hw/virtio/vhost-vdpa.c > @@ -547,6 +547,21 @@ int vhost_vdpa_set_vring_ready(struct vhost_vdpa *v, unsigned idx) > return vhost_vdpa_set_vring_ready_internal(v, idx, true); > } > > +/* TODO: Properly reorder static functions */ > +static void vhost_vdpa_svq_stop(struct vhost_dev *dev, unsigned idx); > +static void vhost_vdpa_reset_queue(struct vhost_dev *dev, int idx) > +{ > + struct vhost_vdpa *v = dev->opaque; > + > + if (dev->features & VIRTIO_F_RING_RESET) { > + vhost_vdpa_set_vring_ready_internal(v, idx, false); I'm not sure I understand this patch - this is NOT the spec defined way to initiate RING_RESET? Quoting the spec diff from the original RING_RESET tex doc: +The device MUST reset the queue when 1 is written to \field{queue_reset}, and +present a 1 in \field{queue_reset} after the queue has been reset, until the +driver re-enables the queue via \field{queue_enable} or the device is reset. +The device MUST present consistent default values after queue reset. +(see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}). Or you intend to rewrite it to be spec conforming later on? -Siwei > + } > + > + if (v->shadow_vqs_enabled) { > + vhost_vdpa_svq_stop(dev, idx - dev->vq_index); > + } > +} > + > /* > * The use of this function is for requests that only need to be > * applied once. Typically such request occurs at the beginning > @@ -1543,4 +1558,5 @@ const VhostOps vdpa_ops = { > .vhost_force_iommu = vhost_vdpa_force_iommu, > .vhost_set_config_call = vhost_vdpa_set_config_call, > .vhost_reset_status = vhost_vdpa_reset_status, > + .vhost_reset_queue = vhost_vdpa_reset_queue, > };
On Fri, Jul 21, 2023 at 11:57 PM Si-Wei Liu <si-wei.liu@oracle.com> wrote: > > > > On 7/20/2023 11:14 AM, Eugenio Pérez wrote: > > Split out vq reset operation in its own function, as it may be called > > with ring reset. > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > > --- > > hw/virtio/vhost-vdpa.c | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > > > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c > > index 6ae276ccde..df2515a247 100644 > > --- a/hw/virtio/vhost-vdpa.c > > +++ b/hw/virtio/vhost-vdpa.c > > @@ -547,6 +547,21 @@ int vhost_vdpa_set_vring_ready(struct vhost_vdpa *v, unsigned idx) > > return vhost_vdpa_set_vring_ready_internal(v, idx, true); > > } > > > > +/* TODO: Properly reorder static functions */ > > +static void vhost_vdpa_svq_stop(struct vhost_dev *dev, unsigned idx); > > +static void vhost_vdpa_reset_queue(struct vhost_dev *dev, int idx) > > +{ > > + struct vhost_vdpa *v = dev->opaque; > > + > > + if (dev->features & VIRTIO_F_RING_RESET) { > > + vhost_vdpa_set_vring_ready_internal(v, idx, false); > I'm not sure I understand this patch - this is NOT the spec defined way > to initiate RING_RESET? Quoting the spec diff from the original > RING_RESET tex doc: > > +The device MUST reset the queue when 1 is written to \field{queue_reset}, and > +present a 1 in \field{queue_reset} after the queue has been reset, until the > +driver re-enables the queue via \field{queue_enable} or the device is reset. > +The device MUST present consistent default values after queue reset. > +(see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}). > > Or you intend to rewrite it to be spec conforming later on? > Sorry for the late notice, but yes, the plan would be either to rewrite this piece of code or to make vDPA uAPI work that way (unlikely). I didn't create a new ioctl for that. Please see https://lists.nongnu.org/archive/html/qemu-devel/2023-07/msg04144.html Thanks! > -Siwei > > + } > > + > > + if (v->shadow_vqs_enabled) { > > + vhost_vdpa_svq_stop(dev, idx - dev->vq_index); > > + } > > +} > > + > > /* > > * The use of this function is for requests that only need to be > > * applied once. Typically such request occurs at the beginning > > @@ -1543,4 +1558,5 @@ const VhostOps vdpa_ops = { > > .vhost_force_iommu = vhost_vdpa_force_iommu, > > .vhost_set_config_call = vhost_vdpa_set_config_call, > > .vhost_reset_status = vhost_vdpa_reset_status, > > + .vhost_reset_queue = vhost_vdpa_reset_queue, > > }; >
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c index 6ae276ccde..df2515a247 100644 --- a/hw/virtio/vhost-vdpa.c +++ b/hw/virtio/vhost-vdpa.c @@ -547,6 +547,21 @@ int vhost_vdpa_set_vring_ready(struct vhost_vdpa *v, unsigned idx) return vhost_vdpa_set_vring_ready_internal(v, idx, true); } +/* TODO: Properly reorder static functions */ +static void vhost_vdpa_svq_stop(struct vhost_dev *dev, unsigned idx); +static void vhost_vdpa_reset_queue(struct vhost_dev *dev, int idx) +{ + struct vhost_vdpa *v = dev->opaque; + + if (dev->features & VIRTIO_F_RING_RESET) { + vhost_vdpa_set_vring_ready_internal(v, idx, false); + } + + if (v->shadow_vqs_enabled) { + vhost_vdpa_svq_stop(dev, idx - dev->vq_index); + } +} + /* * The use of this function is for requests that only need to be * applied once. Typically such request occurs at the beginning @@ -1543,4 +1558,5 @@ const VhostOps vdpa_ops = { .vhost_force_iommu = vhost_vdpa_force_iommu, .vhost_set_config_call = vhost_vdpa_set_config_call, .vhost_reset_status = vhost_vdpa_reset_status, + .vhost_reset_queue = vhost_vdpa_reset_queue, };
Split out vq reset operation in its own function, as it may be called with ring reset. Signed-off-by: Eugenio Pérez <eperezma@redhat.com> --- hw/virtio/vhost-vdpa.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)