Re: [RFC PATCH for 8.1 2/6] vdpa: add vhost_vdpa_reset_status_fd

From: Stefano Garzarella <sgarzare@redhat.com>
To: Eugenio Perez Martin <eperezma@redhat.com>
Cc: qemu-devel@nongnu.org, alvaro.karsz@solid-run.com,
	Laurent Vivier <lvivier@redhat.com>,
	Gautam Dawar <gdawar@xilinx.com>,
	Jason Wang <jasowang@redhat.com>,
	Harpreet Singh Anand <hanand@xilinx.com>,
	Zhu Lingshan <lingshan.zhu@intel.com>,
	"Gonglei (Arei)" <arei.gonglei@huawei.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	Eli Cohen <eli@mellanox.com>,
	si-wei.liu@oracle.com, longpeng2@huawei.com,
	Cindy Lu <lulu@redhat.com>, Parav Pandit <parav@mellanox.com>,
	Liuxiangdong <liuxiangdong5@huawei.com>,
	Shannon Nelson <snelson@pensando.io>,
	Lei Yang <leiyang@redhat.com>
Subject: Re: [RFC PATCH for 8.1 2/6] vdpa: add vhost_vdpa_reset_status_fd
Date: Thu, 23 Mar 2023 09:28:10 +0100	[thread overview]
Message-ID: <20230323082810.sn5iaesmz2rqtdew@sgarzare-redhat> (raw)
In-Reply-To: <CAJaqyWdLcCDYfmgGHkSVaBWX5WAX=WEpA5QAec2CnGQr=J4c8Q@mail.gmail.com>

On Wed, Mar 22, 2023 at 06:36:39PM +0100, Eugenio Perez Martin wrote:
>On Wed, Mar 22, 2023 at 3:24 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>>
>> On Fri, Mar 17, 2023 at 03:55:38PM +0100, Eugenio Pérez wrote:
>> >This allows to reset a vhost-vdpa device from external subsystems like
>> >vhost-net.  It is used in subsequent patches to negotiate features and
>> >probe for CVQ ASID isolation.
>> >
>> >Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>> >---
>> > include/hw/virtio/vhost-vdpa.h |  1 +
>> > hw/virtio/vhost-vdpa.c         | 58 +++++++++++++++++++++++-----------
>> > 2 files changed, 41 insertions(+), 18 deletions(-)
>> >
>> >diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h
>> >index c278a2a8de..28de7da91e 100644
>> >--- a/include/hw/virtio/vhost-vdpa.h
>> >+++ b/include/hw/virtio/vhost-vdpa.h
>> >@@ -54,6 +54,7 @@ typedef struct vhost_vdpa {
>> >     VhostVDPAHostNotifier notifier[VIRTIO_QUEUE_MAX];
>> > } VhostVDPA;
>> >
>> >+void vhost_vdpa_reset_status_fd(int fd);
>> > int vhost_vdpa_get_iova_range(int fd, struct vhost_vdpa_iova_range *iova_range);
>> >
>> > int vhost_vdpa_dma_map(struct vhost_vdpa *v, uint32_t asid, hwaddr iova,
>> >diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
>> >index bbabea18f3..7a2053b8d9 100644
>> >--- a/hw/virtio/vhost-vdpa.c
>> >+++ b/hw/virtio/vhost-vdpa.c
>> >@@ -335,38 +335,45 @@ static const MemoryListener vhost_vdpa_memory_listener = {
>> >     .region_del = vhost_vdpa_listener_region_del,
>> > };
>> >
>> >-static int vhost_vdpa_call(struct vhost_dev *dev, unsigned long int request,
>> >-                             void *arg)
>> >+static int vhost_vdpa_dev_fd(const struct vhost_dev *dev)
>>
>> What is the purpose of this refactoring?
>> I guess, since vhost_net does not have `struct vhost_dev *` we want to
>> use fd directly?
>>
>
>Right.
>
>> It might be better to split this patch into two.
>>
>
>Do you mean to create vhost_vdpa_dev_fd first and then users?

Sorry, I meant adding the vhost_vdpa_add_status_fd(), but on second
thought I think it's okay since we use it in
vhost_vdpa_reset_status_fd().

>
>> > {
>> >     struct vhost_vdpa *v = dev->opaque;
>> >-    int fd = v->device_fd;
>> >-    int ret;
>> >
>> >     assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_VDPA);
>> >+    return v->device_fd;
>> >+}
>> >+
>> >+static int vhost_vdpa_call_fd(int fd, unsigned long int request, void *arg)
>> >+{
>> >+    int ret = ioctl(fd, request, arg);
>> >
>> >-    ret = ioctl(fd, request, arg);
>> >     return ret < 0 ? -errno : ret;
>> > }
>> >
>> >-static int vhost_vdpa_add_status(struct vhost_dev *dev, uint8_t status)
>> >+static int vhost_vdpa_call(struct vhost_dev *dev, unsigned long int request,
>> >+                           void *arg)
>> >+{
>> >+    return vhost_vdpa_call_fd(vhost_vdpa_dev_fd(dev), request, arg);
>> >+}
>> >+
>> >+static int vhost_vdpa_add_status_fd(int fd, uint8_t status)
>> > {
>> >     uint8_t s;
>> >     int ret;
>> >
>> >-    trace_vhost_vdpa_add_status(dev, status);
>> >-    ret = vhost_vdpa_call(dev, VHOST_VDPA_GET_STATUS, &s);
>> >+    ret = vhost_vdpa_call_fd(fd, VHOST_VDPA_GET_STATUS, &s);
>> >     if (ret < 0) {
>> >         return ret;
>> >     }
>> >
>> >     s |= status;
>> >
>> >-    ret = vhost_vdpa_call(dev, VHOST_VDPA_SET_STATUS, &s);
>> >+    ret = vhost_vdpa_call_fd(fd, VHOST_VDPA_SET_STATUS, &s);
>> >     if (ret < 0) {
>> >         return ret;
>> >     }
>> >
>> >-    ret = vhost_vdpa_call(dev, VHOST_VDPA_GET_STATUS, &s);
>> >+    ret = vhost_vdpa_call_fd(fd, VHOST_VDPA_GET_STATUS, &s);
>> >     if (ret < 0) {
>> >         return ret;
>> >     }
>> >@@ -378,6 +385,12 @@ static int vhost_vdpa_add_status(struct vhost_dev *dev, uint8_t status)
>> >     return 0;
>> > }
>> >
>> >+static int vhost_vdpa_add_status(struct vhost_dev *dev, uint8_t status)
>> >+{
>> >+    trace_vhost_vdpa_add_status(dev, status);
>> >+    return vhost_vdpa_add_status_fd(vhost_vdpa_dev_fd(dev), status);
>> >+}
>> >+
>> > int vhost_vdpa_get_iova_range(int fd, struct vhost_vdpa_iova_range *iova_range)
>> > {
>> >     int ret = ioctl(fd, VHOST_VDPA_GET_IOVA_RANGE, iova_range);
>> >@@ -709,16 +722,20 @@ static int vhost_vdpa_get_device_id(struct vhost_dev *dev,
>> >     return ret;
>> > }
>> >
>> >+static int vhost_vdpa_reset_device_fd(int fd)
>> >+{
>> >+    uint8_t status = 0;
>> >+
>> >+    return vhost_vdpa_call_fd(fd, VHOST_VDPA_SET_STATUS, &status);
>> >+}
>> >+
>> > static int vhost_vdpa_reset_device(struct vhost_dev *dev)
>> > {
>> >     struct vhost_vdpa *v = dev->opaque;
>> >-    int ret;
>> >-    uint8_t status = 0;
>> >
>> >-    ret = vhost_vdpa_call(dev, VHOST_VDPA_SET_STATUS, &status);
>> >-    trace_vhost_vdpa_reset_device(dev);
>> >     v->suspended = false;
>>
>> I think it is pre-existing, but if VHOST_VDPA_SET_STATUS fails,
>> should we set anyway `v->suspended = false`?
>>
>
>It's a good question. I think the most correct is to keep as the
>previous value, but I'm not sure if reset is actually allowed to fail.

Looking quickly at the parent drivers we have, perhaps the only one that
can fail is VDUSE if it fails to communicate with the daemon.

However, I don't think we can do much to recover the situation if we
can't reset the device.

Thanks,
Stefano