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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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

  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

_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization




[Index of Archives]     [KVM Development]     [Libvirt Development]     [Libvirt Users]     [CentOS Virtualization]     [Netdev]     [Ethernet Bridging]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]

  Powered by Linux