Re: [PATCH] ifcvf: move IRQ request/free to status change handlers

On 2020/5/12 上午11:38, Jason Wang wrote:

quoted

quoted

quoted

  static int ifcvf_start_datapath(void *private)
  {
      struct ifcvf_hw *vf = ifcvf_private_to_vf(private);

@@ -118,9 +172,12 @@ static void ifcvf_vdpa_set_status(struct 

vdpa_device *vdpa_dev, u8 status)
  {
      struct ifcvf_adapter *adapter;
      struct ifcvf_hw *vf;
+    u8 status_old;
+    int ret;
        vf  = vdpa_to_vf(vdpa_dev);
      adapter = dev_get_drvdata(vdpa_dev->dev.parent);
+    status_old = ifcvf_get_status(vf);
        if (status == 0) {
          ifcvf_stop_datapath(adapter);

@@ -128,7 +185,22 @@ static void ifcvf_vdpa_set_status(struct 

vdpa_device *vdpa_dev, u8 status)
          return;
      }
  -    if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
+    if ((status_old & VIRTIO_CONFIG_S_DRIVER_OK) &&
+        !(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
+        ifcvf_stop_datapath(adapter);
+        ifcvf_free_irq(adapter, IFCVF_MAX_QUEUE_PAIRS * 2);
+    }
+
+    if ((status & VIRTIO_CONFIG_S_DRIVER_OK) &&
+        !(status_old & VIRTIO_CONFIG_S_DRIVER_OK)) {
+        ret = ifcvf_request_irq(adapter);
+        if (ret) {
+            status = ifcvf_get_status(vf);
+            status |= VIRTIO_CONFIG_S_FAILED;
+            ifcvf_set_status(vf, status);
+            return;
+        }
+

Have a hard though on the logic here.

This depends on the status setting from guest or userspace. Which 
means it can not deal with e.g when qemu or userspace is crashed? Do 
we need to care this or it's a over engineering?

Thanks

If qemu crash, I guess users may re-run qmeu / re-initialize the 
device, according to the spec, there should be a reset routine.
This code piece handles status change on DRIVER_OK flipping. I am not 
sure I get your point, mind to give more hints?

The problem is if we don't launch new qemu instance, the interrupt 
will be still there?

Ok, we reset on vhost_vdpa_release() so the following is suspicious:

With the patch, we do:

static void ifcvf_vdpa_set_status(struct vdpa_device *vdpa_dev, u8 status)
{
         struct ifcvf_adapter *adapter;
         struct ifcvf_hw *vf;
         u8 status_old;
         int ret;

         vf  = vdpa_to_vf(vdpa_dev);
         adapter = dev_get_drvdata(vdpa_dev->dev.parent);
         status_old = ifcvf_get_status(vf);

         if (status == 0) {
                 ifcvf_stop_datapath(adapter);
                 ifcvf_reset_vring(adapter);
                 return;
         }

         if ((status_old & VIRTIO_CONFIG_S_DRIVER_OK) &&
             !(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
                 ifcvf_stop_datapath(adapter);
                 ifcvf_free_irq(adapter, IFCVF_MAX_QUEUE_PAIRS * 2);
         }

...

So the handling of status == 0 looks wrong.

The OK -> !OK check should already cover the datapath stopping and irq 
stuffs.

We only need to deal with vring reset and only need to do it after we 
stop the datapath/irq stuffs.

Thanks

Thanks