[PATCH V2 0/5] vDPA/ifcvf: implement immediate initialization mechanism

[PATCH V2 0/5] vDPA/ifcvf: implement immediate initialization mechanism

[Zhu Lingshan]

Formerly, ifcvf driver has implemented a lazy-initialization mechanism
for the virtqueues and other config space contents,
it would store all configurations that passed down from the userspace,
then load them to the device config space upon DRIVER_OK.

This can not serve live migration, so this series implement an
immediate initialization mechanism, which means rather than the
former store-load process, the virtio operations like vq ops
would take immediate actions by access the virtio registers.

This series also implement irq synchronization in the reset
routine

Changes from V1:
1)pull device status in devce_reset (Jason)
2)simplify the procedure which sycn irqs (Jason)
3)fix typos(Michael)

Zhu Lingshan (5):
  vDPA/ifcvf: virt queue ops take immediate actions
  vDPA/ifcvf: get_driver_features from virtio registers
  vDPA/ifcvf: retire ifcvf_start_datapath and ifcvf_add_status
  vDPA/ifcvf: synchronize irqs in the reset routine
  vDPA/ifcvf: a vendor driver should not set _CONFIG_S_FAILED

 drivers/vdpa/ifcvf/ifcvf_base.c | 146 ++++++++++++++++++--------------
 drivers/vdpa/ifcvf/ifcvf_base.h |  17 ++--
 drivers/vdpa/ifcvf/ifcvf_main.c |  98 ++++-----------------
 3 files changed, 108 insertions(+), 153 deletions(-)

-- 
2.39.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH V2 1/5] vDPA/ifcvf: virt queue ops take immediate actions

[Zhu Lingshan]

In this commit, virtqueue operations including:
set_vq_num(), set_vq_address(), set_vq_ready()
and get_vq_ready() access PCI registers directly
to take immediate actions.

Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
---
 drivers/vdpa/ifcvf/ifcvf_base.c | 58 ++++++++++++++++++++-------------
 drivers/vdpa/ifcvf/ifcvf_base.h | 10 +++---
 drivers/vdpa/ifcvf/ifcvf_main.c | 16 +++------
 3 files changed, 45 insertions(+), 39 deletions(-)

diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c
index 5563b3a773c7..6c5650f73007 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.c
+++ b/drivers/vdpa/ifcvf/ifcvf_base.c
@@ -329,31 +329,49 @@ int ifcvf_set_vq_state(struct ifcvf_hw *hw, u16 qid, u16 num)
 	return 0;
 }
 
-static int ifcvf_hw_enable(struct ifcvf_hw *hw)
+void ifcvf_set_vq_num(struct ifcvf_hw *hw, u16 qid, u32 num)
 {
-	struct virtio_pci_common_cfg __iomem *cfg;
-	u32 i;
+	struct virtio_pci_common_cfg __iomem *cfg = hw->common_cfg;
 
-	cfg = hw->common_cfg;
-	for (i = 0; i < hw->nr_vring; i++) {
-		if (!hw->vring[i].ready)
-			break;
+	vp_iowrite16(qid, &cfg->queue_select);
+	vp_iowrite16(num, &cfg->queue_size);
+}
 
-		vp_iowrite16(i, &cfg->queue_select);
-		vp_iowrite64_twopart(hw->vring[i].desc, &cfg->queue_desc_lo,
-				     &cfg->queue_desc_hi);
-		vp_iowrite64_twopart(hw->vring[i].avail, &cfg->queue_avail_lo,
-				      &cfg->queue_avail_hi);
-		vp_iowrite64_twopart(hw->vring[i].used, &cfg->queue_used_lo,
-				     &cfg->queue_used_hi);
-		vp_iowrite16(hw->vring[i].size, &cfg->queue_size);
-		ifcvf_set_vq_state(hw, i, hw->vring[i].last_avail_idx);
-		vp_iowrite16(1, &cfg->queue_enable);
-	}
+int ifcvf_set_vq_address(struct ifcvf_hw *hw, u16 qid, u64 desc_area,
+			 u64 driver_area, u64 device_area)
+{
+	struct virtio_pci_common_cfg __iomem *cfg = hw->common_cfg;
+
+	vp_iowrite16(qid, &cfg->queue_select);
+	vp_iowrite64_twopart(desc_area, &cfg->queue_desc_lo,
+			     &cfg->queue_desc_hi);
+	vp_iowrite64_twopart(driver_area, &cfg->queue_avail_lo,
+			     &cfg->queue_avail_hi);
+	vp_iowrite64_twopart(device_area, &cfg->queue_used_lo,
+			     &cfg->queue_used_hi);
 
 	return 0;
 }
 
+bool ifcvf_get_vq_ready(struct ifcvf_hw *hw, u16 qid)
+{
+	struct virtio_pci_common_cfg __iomem *cfg = hw->common_cfg;
+	u16 queue_enable;
+
+	vp_iowrite16(qid, &cfg->queue_select);
+	queue_enable = vp_ioread16(&cfg->queue_enable);
+
+	return (bool)queue_enable;
+}
+
+void ifcvf_set_vq_ready(struct ifcvf_hw *hw, u16 qid, bool ready)
+{
+	struct virtio_pci_common_cfg __iomem *cfg = hw->common_cfg;
+
+	vp_iowrite16(qid, &cfg->queue_select);
+	vp_iowrite16(ready, &cfg->queue_enable);
+}
+
 static void ifcvf_hw_disable(struct ifcvf_hw *hw)
 {
 	u32 i;
@@ -366,16 +384,12 @@ static void ifcvf_hw_disable(struct ifcvf_hw *hw)
 
 int ifcvf_start_hw(struct ifcvf_hw *hw)
 {
-	ifcvf_reset(hw);
 	ifcvf_add_status(hw, VIRTIO_CONFIG_S_ACKNOWLEDGE);
 	ifcvf_add_status(hw, VIRTIO_CONFIG_S_DRIVER);
 
 	if (ifcvf_config_features(hw) < 0)
 		return -EINVAL;
 
-	if (ifcvf_hw_enable(hw) < 0)
-		return -EINVAL;
-
 	ifcvf_add_status(hw, VIRTIO_CONFIG_S_DRIVER_OK);
 
 	return 0;
diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
index c20d1c40214e..d545a9411143 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.h
+++ b/drivers/vdpa/ifcvf/ifcvf_base.h
@@ -47,12 +47,7 @@
 #define MSIX_VECTOR_DEV_SHARED			3
 
 struct vring_info {
-	u64 desc;
-	u64 avail;
-	u64 used;
-	u16 size;
 	u16 last_avail_idx;
-	bool ready;
 	void __iomem *notify_addr;
 	phys_addr_t notify_pa;
 	u32 irq;
@@ -137,4 +132,9 @@ int ifcvf_probed_virtio_net(struct ifcvf_hw *hw);
 u32 ifcvf_get_config_size(struct ifcvf_hw *hw);
 u16 ifcvf_set_vq_vector(struct ifcvf_hw *hw, u16 qid, int vector);
 u16 ifcvf_set_config_vector(struct ifcvf_hw *hw, int vector);
+void ifcvf_set_vq_num(struct ifcvf_hw *hw, u16 qid, u32 num);
+int ifcvf_set_vq_address(struct ifcvf_hw *hw, u16 qid, u64 desc_area,
+			 u64 driver_area, u64 device_area);
+bool ifcvf_get_vq_ready(struct ifcvf_hw *hw, u16 qid);
+void ifcvf_set_vq_ready(struct ifcvf_hw *hw, u16 qid, bool ready);
 #endif /* _IFCVF_H_ */
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
index 7f78c47e40d6..1357c67014ab 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -382,10 +382,6 @@ static void ifcvf_reset_vring(struct ifcvf_adapter *adapter)
 
 	for (i = 0; i < vf->nr_vring; i++) {
 		vf->vring[i].last_avail_idx = 0;
-		vf->vring[i].desc = 0;
-		vf->vring[i].avail = 0;
-		vf->vring[i].used = 0;
-		vf->vring[i].ready = 0;
 		vf->vring[i].cb.callback = NULL;
 		vf->vring[i].cb.private = NULL;
 	}
@@ -542,14 +538,14 @@ static void ifcvf_vdpa_set_vq_ready(struct vdpa_device *vdpa_dev,
 {
 	struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
 
-	vf->vring[qid].ready = ready;
+	ifcvf_set_vq_ready(vf, qid, ready);
 }
 
 static bool ifcvf_vdpa_get_vq_ready(struct vdpa_device *vdpa_dev, u16 qid)
 {
 	struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
 
-	return vf->vring[qid].ready;
+	return ifcvf_get_vq_ready(vf, qid);
 }
 
 static void ifcvf_vdpa_set_vq_num(struct vdpa_device *vdpa_dev, u16 qid,
@@ -557,7 +553,7 @@ static void ifcvf_vdpa_set_vq_num(struct vdpa_device *vdpa_dev, u16 qid,
 {
 	struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
 
-	vf->vring[qid].size = num;
+	ifcvf_set_vq_num(vf, qid, num);
 }
 
 static int ifcvf_vdpa_set_vq_address(struct vdpa_device *vdpa_dev, u16 qid,
@@ -566,11 +562,7 @@ static int ifcvf_vdpa_set_vq_address(struct vdpa_device *vdpa_dev, u16 qid,
 {
 	struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
 
-	vf->vring[qid].desc = desc_area;
-	vf->vring[qid].avail = driver_area;
-	vf->vring[qid].used = device_area;
-
-	return 0;
+	return ifcvf_set_vq_address(vf, qid, desc_area, driver_area, device_area);
 }
 
 static void ifcvf_vdpa_kick_vq(struct vdpa_device *vdpa_dev, u16 qid)
-- 
2.39.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V2 1/5] vDPA/ifcvf: virt queue ops take immediate actions

[Jason Wang]

On Mon, May 8, 2023 at 6:05 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>
> In this commit, virtqueue operations including:
> set_vq_num(), set_vq_address(), set_vq_ready()
> and get_vq_ready() access PCI registers directly
> to take immediate actions.
>
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>

Acked-by: Jason Wang <jasowang@redhat.com>

Thanks

> ---
>  drivers/vdpa/ifcvf/ifcvf_base.c | 58 ++++++++++++++++++++-------------
>  drivers/vdpa/ifcvf/ifcvf_base.h | 10 +++---
>  drivers/vdpa/ifcvf/ifcvf_main.c | 16 +++------
>  3 files changed, 45 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c
> index 5563b3a773c7..6c5650f73007 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_base.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
> @@ -329,31 +329,49 @@ int ifcvf_set_vq_state(struct ifcvf_hw *hw, u16 qid, u16 num)
>         return 0;
>  }
>
> -static int ifcvf_hw_enable(struct ifcvf_hw *hw)
> +void ifcvf_set_vq_num(struct ifcvf_hw *hw, u16 qid, u32 num)
>  {
> -       struct virtio_pci_common_cfg __iomem *cfg;
> -       u32 i;
> +       struct virtio_pci_common_cfg __iomem *cfg = hw->common_cfg;
>
> -       cfg = hw->common_cfg;
> -       for (i = 0; i < hw->nr_vring; i++) {
> -               if (!hw->vring[i].ready)
> -                       break;
> +       vp_iowrite16(qid, &cfg->queue_select);
> +       vp_iowrite16(num, &cfg->queue_size);
> +}
>
> -               vp_iowrite16(i, &cfg->queue_select);
> -               vp_iowrite64_twopart(hw->vring[i].desc, &cfg->queue_desc_lo,
> -                                    &cfg->queue_desc_hi);
> -               vp_iowrite64_twopart(hw->vring[i].avail, &cfg->queue_avail_lo,
> -                                     &cfg->queue_avail_hi);
> -               vp_iowrite64_twopart(hw->vring[i].used, &cfg->queue_used_lo,
> -                                    &cfg->queue_used_hi);
> -               vp_iowrite16(hw->vring[i].size, &cfg->queue_size);
> -               ifcvf_set_vq_state(hw, i, hw->vring[i].last_avail_idx);
> -               vp_iowrite16(1, &cfg->queue_enable);
> -       }
> +int ifcvf_set_vq_address(struct ifcvf_hw *hw, u16 qid, u64 desc_area,
> +                        u64 driver_area, u64 device_area)
> +{
> +       struct virtio_pci_common_cfg __iomem *cfg = hw->common_cfg;
> +
> +       vp_iowrite16(qid, &cfg->queue_select);
> +       vp_iowrite64_twopart(desc_area, &cfg->queue_desc_lo,
> +                            &cfg->queue_desc_hi);
> +       vp_iowrite64_twopart(driver_area, &cfg->queue_avail_lo,
> +                            &cfg->queue_avail_hi);
> +       vp_iowrite64_twopart(device_area, &cfg->queue_used_lo,
> +                            &cfg->queue_used_hi);
>
>         return 0;
>  }
>
> +bool ifcvf_get_vq_ready(struct ifcvf_hw *hw, u16 qid)
> +{
> +       struct virtio_pci_common_cfg __iomem *cfg = hw->common_cfg;
> +       u16 queue_enable;
> +
> +       vp_iowrite16(qid, &cfg->queue_select);
> +       queue_enable = vp_ioread16(&cfg->queue_enable);
> +
> +       return (bool)queue_enable;
> +}
> +
> +void ifcvf_set_vq_ready(struct ifcvf_hw *hw, u16 qid, bool ready)
> +{
> +       struct virtio_pci_common_cfg __iomem *cfg = hw->common_cfg;
> +
> +       vp_iowrite16(qid, &cfg->queue_select);
> +       vp_iowrite16(ready, &cfg->queue_enable);
> +}
> +
>  static void ifcvf_hw_disable(struct ifcvf_hw *hw)
>  {
>         u32 i;
> @@ -366,16 +384,12 @@ static void ifcvf_hw_disable(struct ifcvf_hw *hw)
>
>  int ifcvf_start_hw(struct ifcvf_hw *hw)
>  {
> -       ifcvf_reset(hw);
>         ifcvf_add_status(hw, VIRTIO_CONFIG_S_ACKNOWLEDGE);
>         ifcvf_add_status(hw, VIRTIO_CONFIG_S_DRIVER);
>
>         if (ifcvf_config_features(hw) < 0)
>                 return -EINVAL;
>
> -       if (ifcvf_hw_enable(hw) < 0)
> -               return -EINVAL;
> -
>         ifcvf_add_status(hw, VIRTIO_CONFIG_S_DRIVER_OK);
>
>         return 0;
> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
> index c20d1c40214e..d545a9411143 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
> @@ -47,12 +47,7 @@
>  #define MSIX_VECTOR_DEV_SHARED                 3
>
>  struct vring_info {
> -       u64 desc;
> -       u64 avail;
> -       u64 used;
> -       u16 size;
>         u16 last_avail_idx;
> -       bool ready;
>         void __iomem *notify_addr;
>         phys_addr_t notify_pa;
>         u32 irq;
> @@ -137,4 +132,9 @@ int ifcvf_probed_virtio_net(struct ifcvf_hw *hw);
>  u32 ifcvf_get_config_size(struct ifcvf_hw *hw);
>  u16 ifcvf_set_vq_vector(struct ifcvf_hw *hw, u16 qid, int vector);
>  u16 ifcvf_set_config_vector(struct ifcvf_hw *hw, int vector);
> +void ifcvf_set_vq_num(struct ifcvf_hw *hw, u16 qid, u32 num);
> +int ifcvf_set_vq_address(struct ifcvf_hw *hw, u16 qid, u64 desc_area,
> +                        u64 driver_area, u64 device_area);
> +bool ifcvf_get_vq_ready(struct ifcvf_hw *hw, u16 qid);
> +void ifcvf_set_vq_ready(struct ifcvf_hw *hw, u16 qid, bool ready);
>  #endif /* _IFCVF_H_ */
> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
> index 7f78c47e40d6..1357c67014ab 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
> @@ -382,10 +382,6 @@ static void ifcvf_reset_vring(struct ifcvf_adapter *adapter)
>
>         for (i = 0; i < vf->nr_vring; i++) {
>                 vf->vring[i].last_avail_idx = 0;
> -               vf->vring[i].desc = 0;
> -               vf->vring[i].avail = 0;
> -               vf->vring[i].used = 0;
> -               vf->vring[i].ready = 0;
>                 vf->vring[i].cb.callback = NULL;
>                 vf->vring[i].cb.private = NULL;
>         }
> @@ -542,14 +538,14 @@ static void ifcvf_vdpa_set_vq_ready(struct vdpa_device *vdpa_dev,
>  {
>         struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>
> -       vf->vring[qid].ready = ready;
> +       ifcvf_set_vq_ready(vf, qid, ready);
>  }
>
>  static bool ifcvf_vdpa_get_vq_ready(struct vdpa_device *vdpa_dev, u16 qid)
>  {
>         struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>
> -       return vf->vring[qid].ready;
> +       return ifcvf_get_vq_ready(vf, qid);
>  }
>
>  static void ifcvf_vdpa_set_vq_num(struct vdpa_device *vdpa_dev, u16 qid,
> @@ -557,7 +553,7 @@ static void ifcvf_vdpa_set_vq_num(struct vdpa_device *vdpa_dev, u16 qid,
>  {
>         struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>
> -       vf->vring[qid].size = num;
> +       ifcvf_set_vq_num(vf, qid, num);
>  }
>
>  static int ifcvf_vdpa_set_vq_address(struct vdpa_device *vdpa_dev, u16 qid,
> @@ -566,11 +562,7 @@ static int ifcvf_vdpa_set_vq_address(struct vdpa_device *vdpa_dev, u16 qid,
>  {
>         struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>
> -       vf->vring[qid].desc = desc_area;
> -       vf->vring[qid].avail = driver_area;
> -       vf->vring[qid].used = device_area;
> -
> -       return 0;
> +       return ifcvf_set_vq_address(vf, qid, desc_area, driver_area, device_area);
>  }
>
>  static void ifcvf_vdpa_kick_vq(struct vdpa_device *vdpa_dev, u16 qid)
> --
> 2.39.1
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH V2 2/5] vDPA/ifcvf: get_driver_features from virtio registers

[Zhu Lingshan]

This commit implements a new function ifcvf_get_driver_feature()
which read driver_features from virtio registers.

To be less ambiguous, ifcvf_set_features() is renamed to
ifcvf_set_driver_features(), and ifcvf_get_features()
is renamed to ifcvf_get_dev_features() which returns
the provisioned vDPA device features.

Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
---
 drivers/vdpa/ifcvf/ifcvf_base.c | 38 +++++++++++++++++----------------
 drivers/vdpa/ifcvf/ifcvf_base.h |  5 +++--
 drivers/vdpa/ifcvf/ifcvf_main.c |  9 +++++---
 3 files changed, 29 insertions(+), 23 deletions(-)

diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c
index 6c5650f73007..546e923bcd16 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.c
+++ b/drivers/vdpa/ifcvf/ifcvf_base.c
@@ -204,11 +204,29 @@ u64 ifcvf_get_hw_features(struct ifcvf_hw *hw)
 	return features;
 }
 
-u64 ifcvf_get_features(struct ifcvf_hw *hw)
+/* return provisioned vDPA dev features */
+u64 ifcvf_get_dev_features(struct ifcvf_hw *hw)
 {
 	return hw->dev_features;
 }
 
+u64 ifcvf_get_driver_features(struct ifcvf_hw *hw)
+{
+	struct virtio_pci_common_cfg __iomem *cfg = hw->common_cfg;
+	u32 features_lo, features_hi;
+	u64 features;
+
+	vp_iowrite32(0, &cfg->device_feature_select);
+	features_lo = vp_ioread32(&cfg->guest_feature);
+
+	vp_iowrite32(1, &cfg->device_feature_select);
+	features_hi = vp_ioread32(&cfg->guest_feature);
+
+	features = ((u64)features_hi << 32) | features_lo;
+
+	return features;
+}
+
 int ifcvf_verify_min_features(struct ifcvf_hw *hw, u64 features)
 {
 	if (!(features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM)) && features) {
@@ -275,7 +293,7 @@ void ifcvf_write_dev_config(struct ifcvf_hw *hw, u64 offset,
 		vp_iowrite8(*p++, hw->dev_cfg + offset + i);
 }
 
-static void ifcvf_set_features(struct ifcvf_hw *hw, u64 features)
+void ifcvf_set_driver_features(struct ifcvf_hw *hw, u64 features)
 {
 	struct virtio_pci_common_cfg __iomem *cfg = hw->common_cfg;
 
@@ -286,19 +304,6 @@ static void ifcvf_set_features(struct ifcvf_hw *hw, u64 features)
 	vp_iowrite32(features >> 32, &cfg->guest_feature);
 }
 
-static int ifcvf_config_features(struct ifcvf_hw *hw)
-{
-	ifcvf_set_features(hw, hw->req_features);
-	ifcvf_add_status(hw, VIRTIO_CONFIG_S_FEATURES_OK);
-
-	if (!(ifcvf_get_status(hw) & VIRTIO_CONFIG_S_FEATURES_OK)) {
-		IFCVF_ERR(hw->pdev, "Failed to set FEATURES_OK status\n");
-		return -EIO;
-	}
-
-	return 0;
-}
-
 u16 ifcvf_get_vq_state(struct ifcvf_hw *hw, u16 qid)
 {
 	struct ifcvf_lm_cfg __iomem *ifcvf_lm;
@@ -387,9 +392,6 @@ int ifcvf_start_hw(struct ifcvf_hw *hw)
 	ifcvf_add_status(hw, VIRTIO_CONFIG_S_ACKNOWLEDGE);
 	ifcvf_add_status(hw, VIRTIO_CONFIG_S_DRIVER);
 
-	if (ifcvf_config_features(hw) < 0)
-		return -EINVAL;
-
 	ifcvf_add_status(hw, VIRTIO_CONFIG_S_DRIVER_OK);
 
 	return 0;
diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
index d545a9411143..cb19196c3ece 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.h
+++ b/drivers/vdpa/ifcvf/ifcvf_base.h
@@ -69,7 +69,6 @@ struct ifcvf_hw {
 	phys_addr_t notify_base_pa;
 	u32 notify_off_multiplier;
 	u32 dev_type;
-	u64 req_features;
 	u64 hw_features;
 	/* provisioned device features */
 	u64 dev_features;
@@ -122,7 +121,7 @@ u8 ifcvf_get_status(struct ifcvf_hw *hw);
 void ifcvf_set_status(struct ifcvf_hw *hw, u8 status);
 void io_write64_twopart(u64 val, u32 *lo, u32 *hi);
 void ifcvf_reset(struct ifcvf_hw *hw);
-u64 ifcvf_get_features(struct ifcvf_hw *hw);
+u64 ifcvf_get_dev_features(struct ifcvf_hw *hw);
 u64 ifcvf_get_hw_features(struct ifcvf_hw *hw);
 int ifcvf_verify_min_features(struct ifcvf_hw *hw, u64 features);
 u16 ifcvf_get_vq_state(struct ifcvf_hw *hw, u16 qid);
@@ -137,4 +136,6 @@ int ifcvf_set_vq_address(struct ifcvf_hw *hw, u16 qid, u64 desc_area,
 			 u64 driver_area, u64 device_area);
 bool ifcvf_get_vq_ready(struct ifcvf_hw *hw, u16 qid);
 void ifcvf_set_vq_ready(struct ifcvf_hw *hw, u16 qid, bool ready);
+void ifcvf_set_driver_features(struct ifcvf_hw *hw, u64 features);
+u64 ifcvf_get_driver_features(struct ifcvf_hw *hw);
 #endif /* _IFCVF_H_ */
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
index 1357c67014ab..4588484bd53d 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -410,7 +410,7 @@ static u64 ifcvf_vdpa_get_device_features(struct vdpa_device *vdpa_dev)
 	u64 features;
 
 	if (type == VIRTIO_ID_NET || type == VIRTIO_ID_BLOCK)
-		features = ifcvf_get_features(vf);
+		features = ifcvf_get_dev_features(vf);
 	else {
 		features = 0;
 		IFCVF_ERR(pdev, "VIRTIO ID %u not supported\n", vf->dev_type);
@@ -428,7 +428,7 @@ static int ifcvf_vdpa_set_driver_features(struct vdpa_device *vdpa_dev, u64 feat
 	if (ret)
 		return ret;
 
-	vf->req_features = features;
+	ifcvf_set_driver_features(vf, features);
 
 	return 0;
 }
@@ -436,8 +436,11 @@ static int ifcvf_vdpa_set_driver_features(struct vdpa_device *vdpa_dev, u64 feat
 static u64 ifcvf_vdpa_get_driver_features(struct vdpa_device *vdpa_dev)
 {
 	struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
+	u64 features;
+
+	features = ifcvf_get_driver_features(vf);
 
-	return vf->req_features;
+	return features;
 }
 
 static u8 ifcvf_vdpa_get_status(struct vdpa_device *vdpa_dev)
-- 
2.39.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V2 2/5] vDPA/ifcvf: get_driver_features from virtio registers

[Jason Wang]

On Mon, May 8, 2023 at 6:05 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>
> This commit implements a new function ifcvf_get_driver_feature()
> which read driver_features from virtio registers.
>
> To be less ambiguous, ifcvf_set_features() is renamed to
> ifcvf_set_driver_features(), and ifcvf_get_features()
> is renamed to ifcvf_get_dev_features() which returns
> the provisioned vDPA device features.
>
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>

Acked-by: Jason Wang <jasowang@redhat.com>

Thanks

> ---
>  drivers/vdpa/ifcvf/ifcvf_base.c | 38 +++++++++++++++++----------------
>  drivers/vdpa/ifcvf/ifcvf_base.h |  5 +++--
>  drivers/vdpa/ifcvf/ifcvf_main.c |  9 +++++---
>  3 files changed, 29 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c
> index 6c5650f73007..546e923bcd16 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_base.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
> @@ -204,11 +204,29 @@ u64 ifcvf_get_hw_features(struct ifcvf_hw *hw)
>         return features;
>  }
>
> -u64 ifcvf_get_features(struct ifcvf_hw *hw)
> +/* return provisioned vDPA dev features */
> +u64 ifcvf_get_dev_features(struct ifcvf_hw *hw)
>  {
>         return hw->dev_features;
>  }
>
> +u64 ifcvf_get_driver_features(struct ifcvf_hw *hw)
> +{
> +       struct virtio_pci_common_cfg __iomem *cfg = hw->common_cfg;
> +       u32 features_lo, features_hi;
> +       u64 features;
> +
> +       vp_iowrite32(0, &cfg->device_feature_select);
> +       features_lo = vp_ioread32(&cfg->guest_feature);
> +
> +       vp_iowrite32(1, &cfg->device_feature_select);
> +       features_hi = vp_ioread32(&cfg->guest_feature);
> +
> +       features = ((u64)features_hi << 32) | features_lo;
> +
> +       return features;
> +}
> +
>  int ifcvf_verify_min_features(struct ifcvf_hw *hw, u64 features)
>  {
>         if (!(features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM)) && features) {
> @@ -275,7 +293,7 @@ void ifcvf_write_dev_config(struct ifcvf_hw *hw, u64 offset,
>                 vp_iowrite8(*p++, hw->dev_cfg + offset + i);
>  }
>
> -static void ifcvf_set_features(struct ifcvf_hw *hw, u64 features)
> +void ifcvf_set_driver_features(struct ifcvf_hw *hw, u64 features)
>  {
>         struct virtio_pci_common_cfg __iomem *cfg = hw->common_cfg;
>
> @@ -286,19 +304,6 @@ static void ifcvf_set_features(struct ifcvf_hw *hw, u64 features)
>         vp_iowrite32(features >> 32, &cfg->guest_feature);
>  }
>
> -static int ifcvf_config_features(struct ifcvf_hw *hw)
> -{
> -       ifcvf_set_features(hw, hw->req_features);
> -       ifcvf_add_status(hw, VIRTIO_CONFIG_S_FEATURES_OK);
> -
> -       if (!(ifcvf_get_status(hw) & VIRTIO_CONFIG_S_FEATURES_OK)) {
> -               IFCVF_ERR(hw->pdev, "Failed to set FEATURES_OK status\n");
> -               return -EIO;
> -       }
> -
> -       return 0;
> -}
> -
>  u16 ifcvf_get_vq_state(struct ifcvf_hw *hw, u16 qid)
>  {
>         struct ifcvf_lm_cfg __iomem *ifcvf_lm;
> @@ -387,9 +392,6 @@ int ifcvf_start_hw(struct ifcvf_hw *hw)
>         ifcvf_add_status(hw, VIRTIO_CONFIG_S_ACKNOWLEDGE);
>         ifcvf_add_status(hw, VIRTIO_CONFIG_S_DRIVER);
>
> -       if (ifcvf_config_features(hw) < 0)
> -               return -EINVAL;
> -
>         ifcvf_add_status(hw, VIRTIO_CONFIG_S_DRIVER_OK);
>
>         return 0;
> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
> index d545a9411143..cb19196c3ece 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
> @@ -69,7 +69,6 @@ struct ifcvf_hw {
>         phys_addr_t notify_base_pa;
>         u32 notify_off_multiplier;
>         u32 dev_type;
> -       u64 req_features;
>         u64 hw_features;
>         /* provisioned device features */
>         u64 dev_features;
> @@ -122,7 +121,7 @@ u8 ifcvf_get_status(struct ifcvf_hw *hw);
>  void ifcvf_set_status(struct ifcvf_hw *hw, u8 status);
>  void io_write64_twopart(u64 val, u32 *lo, u32 *hi);
>  void ifcvf_reset(struct ifcvf_hw *hw);
> -u64 ifcvf_get_features(struct ifcvf_hw *hw);
> +u64 ifcvf_get_dev_features(struct ifcvf_hw *hw);
>  u64 ifcvf_get_hw_features(struct ifcvf_hw *hw);
>  int ifcvf_verify_min_features(struct ifcvf_hw *hw, u64 features);
>  u16 ifcvf_get_vq_state(struct ifcvf_hw *hw, u16 qid);
> @@ -137,4 +136,6 @@ int ifcvf_set_vq_address(struct ifcvf_hw *hw, u16 qid, u64 desc_area,
>                          u64 driver_area, u64 device_area);
>  bool ifcvf_get_vq_ready(struct ifcvf_hw *hw, u16 qid);
>  void ifcvf_set_vq_ready(struct ifcvf_hw *hw, u16 qid, bool ready);
> +void ifcvf_set_driver_features(struct ifcvf_hw *hw, u64 features);
> +u64 ifcvf_get_driver_features(struct ifcvf_hw *hw);
>  #endif /* _IFCVF_H_ */
> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
> index 1357c67014ab..4588484bd53d 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
> @@ -410,7 +410,7 @@ static u64 ifcvf_vdpa_get_device_features(struct vdpa_device *vdpa_dev)
>         u64 features;
>
>         if (type == VIRTIO_ID_NET || type == VIRTIO_ID_BLOCK)
> -               features = ifcvf_get_features(vf);
> +               features = ifcvf_get_dev_features(vf);
>         else {
>                 features = 0;
>                 IFCVF_ERR(pdev, "VIRTIO ID %u not supported\n", vf->dev_type);
> @@ -428,7 +428,7 @@ static int ifcvf_vdpa_set_driver_features(struct vdpa_device *vdpa_dev, u64 feat
>         if (ret)
>                 return ret;
>
> -       vf->req_features = features;
> +       ifcvf_set_driver_features(vf, features);
>
>         return 0;
>  }
> @@ -436,8 +436,11 @@ static int ifcvf_vdpa_set_driver_features(struct vdpa_device *vdpa_dev, u64 feat
>  static u64 ifcvf_vdpa_get_driver_features(struct vdpa_device *vdpa_dev)
>  {
>         struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
> +       u64 features;
> +
> +       features = ifcvf_get_driver_features(vf);
>
> -       return vf->req_features;
> +       return features;
>  }
>
>  static u8 ifcvf_vdpa_get_status(struct vdpa_device *vdpa_dev)
> --
> 2.39.1
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH V2 3/5] vDPA/ifcvf: retire ifcvf_start_datapath and ifcvf_add_status

[Zhu Lingshan]

Rather than former lazy-initialization mechanism,
now the virtqueue operations and driver_features related
ops access the virtio registers directly to take
immediate actions. So ifcvf_start_datapath() should
retire.

ifcvf_add_status() is retierd because we should not change
device status by a vendor driver's decision, this driver should
only set device status which is from virito drivers
upon vdpa_ops.set_status()

Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
Acked-by: Jason Wang <jasowang@redhat.com>
---
 drivers/vdpa/ifcvf/ifcvf_base.c | 19 -------------------
 drivers/vdpa/ifcvf/ifcvf_base.h |  1 -
 drivers/vdpa/ifcvf/ifcvf_main.c | 23 -----------------------
 3 files changed, 43 deletions(-)

diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c
index 546e923bcd16..79e313c5e10e 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.c
+++ b/drivers/vdpa/ifcvf/ifcvf_base.c
@@ -178,15 +178,6 @@ void ifcvf_reset(struct ifcvf_hw *hw)
 	ifcvf_get_status(hw);
 }
 
-static void ifcvf_add_status(struct ifcvf_hw *hw, u8 status)
-{
-	if (status != 0)
-		status |= ifcvf_get_status(hw);
-
-	ifcvf_set_status(hw, status);
-	ifcvf_get_status(hw);
-}
-
 u64 ifcvf_get_hw_features(struct ifcvf_hw *hw)
 {
 	struct virtio_pci_common_cfg __iomem *cfg = hw->common_cfg;
@@ -387,16 +378,6 @@ static void ifcvf_hw_disable(struct ifcvf_hw *hw)
 	}
 }
 
-int ifcvf_start_hw(struct ifcvf_hw *hw)
-{
-	ifcvf_add_status(hw, VIRTIO_CONFIG_S_ACKNOWLEDGE);
-	ifcvf_add_status(hw, VIRTIO_CONFIG_S_DRIVER);
-
-	ifcvf_add_status(hw, VIRTIO_CONFIG_S_DRIVER_OK);
-
-	return 0;
-}
-
 void ifcvf_stop_hw(struct ifcvf_hw *hw)
 {
 	ifcvf_hw_disable(hw);
diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
index cb19196c3ece..d34d3bc0dbf4 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.h
+++ b/drivers/vdpa/ifcvf/ifcvf_base.h
@@ -110,7 +110,6 @@ struct ifcvf_vdpa_mgmt_dev {
 };
 
 int ifcvf_init_hw(struct ifcvf_hw *hw, struct pci_dev *dev);
-int ifcvf_start_hw(struct ifcvf_hw *hw);
 void ifcvf_stop_hw(struct ifcvf_hw *hw);
 void ifcvf_notify_queue(struct ifcvf_hw *hw, u16 qid);
 void ifcvf_read_dev_config(struct ifcvf_hw *hw, u64 offset,
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
index 4588484bd53d..968687159e44 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -346,22 +346,6 @@ static int ifcvf_request_irq(struct ifcvf_hw *vf)
 	return 0;
 }
 
-static int ifcvf_start_datapath(struct ifcvf_adapter *adapter)
-{
-	struct ifcvf_hw *vf = adapter->vf;
-	u8 status;
-	int ret;
-
-	ret = ifcvf_start_hw(vf);
-	if (ret < 0) {
-		status = ifcvf_get_status(vf);
-		status |= VIRTIO_CONFIG_S_FAILED;
-		ifcvf_set_status(vf, status);
-	}
-
-	return ret;
-}
-
 static int ifcvf_stop_datapath(struct ifcvf_adapter *adapter)
 {
 	struct ifcvf_hw *vf = adapter->vf;
@@ -452,13 +436,11 @@ static u8 ifcvf_vdpa_get_status(struct vdpa_device *vdpa_dev)
 
 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 = vdpa_to_adapter(vdpa_dev);
 	status_old = ifcvf_get_status(vf);
 
 	if (status_old == status)
@@ -473,11 +455,6 @@ static void ifcvf_vdpa_set_status(struct vdpa_device *vdpa_dev, u8 status)
 			ifcvf_set_status(vf, status);
 			return;
 		}
-
-		if (ifcvf_start_datapath(adapter) < 0)
-			IFCVF_ERR(adapter->pdev,
-				  "Failed to set ifcvf vdpa  status %u\n",
-				  status);
 	}
 
 	ifcvf_set_status(vf, status);
-- 
2.39.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH V2 4/5] vDPA/ifcvf: synchronize irqs in the reset routine

[Zhu Lingshan]

This commit synchronize irqs of the virtqueues
and config space in the reset routine.
Thus ifcvf_stop_hw() and reset() are refactored as well.

Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
---
 drivers/vdpa/ifcvf/ifcvf_base.c | 41 +++++++++++++++++++++--------
 drivers/vdpa/ifcvf/ifcvf_base.h |  1 +
 drivers/vdpa/ifcvf/ifcvf_main.c | 46 +++++----------------------------
 3 files changed, 38 insertions(+), 50 deletions(-)

diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c
index 79e313c5e10e..1f39290baa38 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.c
+++ b/drivers/vdpa/ifcvf/ifcvf_base.c
@@ -170,12 +170,9 @@ void ifcvf_set_status(struct ifcvf_hw *hw, u8 status)
 
 void ifcvf_reset(struct ifcvf_hw *hw)
 {
-	hw->config_cb.callback = NULL;
-	hw->config_cb.private = NULL;
-
 	ifcvf_set_status(hw, 0);
-	/* flush set_status, make sure VF is stopped, reset */
-	ifcvf_get_status(hw);
+	while (ifcvf_get_status(hw))
+		msleep(1);
 }
 
 u64 ifcvf_get_hw_features(struct ifcvf_hw *hw)
@@ -368,20 +365,42 @@ void ifcvf_set_vq_ready(struct ifcvf_hw *hw, u16 qid, bool ready)
 	vp_iowrite16(ready, &cfg->queue_enable);
 }
 
-static void ifcvf_hw_disable(struct ifcvf_hw *hw)
+static void ifcvf_reset_vring(struct ifcvf_hw *hw)
 {
-	u32 i;
+	u16 qid;
+
+	for (qid = 0; qid < hw->nr_vring; qid++) {
+		hw->vring[qid].cb.callback = NULL;
+		hw->vring[qid].cb.private = NULL;
+		ifcvf_set_vq_vector(hw, qid, VIRTIO_MSI_NO_VECTOR);
+	}
+}
 
+static void ifcvf_reset_config_handler(struct ifcvf_hw *hw)
+{
+	hw->config_cb.callback = NULL;
+	hw->config_cb.private = NULL;
 	ifcvf_set_config_vector(hw, VIRTIO_MSI_NO_VECTOR);
-	for (i = 0; i < hw->nr_vring; i++) {
-		ifcvf_set_vq_vector(hw, i, VIRTIO_MSI_NO_VECTOR);
+}
+
+static void ifcvf_synchronize_irq(struct ifcvf_hw *hw)
+{
+	u32 nvectors = hw->num_msix_vectors;
+	struct pci_dev *pdev = hw->pdev;
+	int i, irq;
+
+	for (i = 0; i < nvectors; i++) {
+		irq = pci_irq_vector(pdev, i);
+		if (irq >= 0)
+			synchronize_irq(irq);
 	}
 }
 
 void ifcvf_stop_hw(struct ifcvf_hw *hw)
 {
-	ifcvf_hw_disable(hw);
-	ifcvf_reset(hw);
+	ifcvf_synchronize_irq(hw);
+	ifcvf_reset_vring(hw);
+	ifcvf_reset_config_handler(hw);
 }
 
 void ifcvf_notify_queue(struct ifcvf_hw *hw, u16 qid)
diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
index d34d3bc0dbf4..7430f80779be 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.h
+++ b/drivers/vdpa/ifcvf/ifcvf_base.h
@@ -82,6 +82,7 @@ struct ifcvf_hw {
 	int vqs_reused_irq;
 	u16 nr_vring;
 	/* VIRTIO_PCI_CAP_DEVICE_CFG size */
+	u32 num_msix_vectors;
 	u32 cap_dev_config_size;
 	struct pci_dev *pdev;
 };
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
index 968687159e44..3401b9901dd2 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -125,6 +125,7 @@ static void ifcvf_free_irq(struct ifcvf_hw *vf)
 	ifcvf_free_vq_irq(vf);
 	ifcvf_free_config_irq(vf);
 	ifcvf_free_irq_vectors(pdev);
+	vf->num_msix_vectors = 0;
 }
 
 /* ifcvf MSIX vectors allocator, this helper tries to allocate
@@ -343,36 +344,11 @@ static int ifcvf_request_irq(struct ifcvf_hw *vf)
 	if (ret)
 		return ret;
 
-	return 0;
-}
-
-static int ifcvf_stop_datapath(struct ifcvf_adapter *adapter)
-{
-	struct ifcvf_hw *vf = adapter->vf;
-	int i;
-
-	for (i = 0; i < vf->nr_vring; i++)
-		vf->vring[i].cb.callback = NULL;
-
-	ifcvf_stop_hw(vf);
+	vf->num_msix_vectors = nvectors;
 
 	return 0;
 }
 
-static void ifcvf_reset_vring(struct ifcvf_adapter *adapter)
-{
-	struct ifcvf_hw *vf = adapter->vf;
-	int i;
-
-	for (i = 0; i < vf->nr_vring; i++) {
-		vf->vring[i].last_avail_idx = 0;
-		vf->vring[i].cb.callback = NULL;
-		vf->vring[i].cb.private = NULL;
-	}
-
-	ifcvf_reset(vf);
-}
-
 static struct ifcvf_adapter *vdpa_to_adapter(struct vdpa_device *vdpa_dev)
 {
 	return container_of(vdpa_dev, struct ifcvf_adapter, vdpa);
@@ -462,23 +438,15 @@ static void ifcvf_vdpa_set_status(struct vdpa_device *vdpa_dev, u8 status)
 
 static int ifcvf_vdpa_reset(struct vdpa_device *vdpa_dev)
 {
-	struct ifcvf_adapter *adapter;
-	struct ifcvf_hw *vf;
-	u8 status_old;
-
-	vf  = vdpa_to_vf(vdpa_dev);
-	adapter = vdpa_to_adapter(vdpa_dev);
-	status_old = ifcvf_get_status(vf);
+	struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
+	u8 status = ifcvf_get_status(vf);
 
-	if (status_old == 0)
-		return 0;
+	ifcvf_stop_hw(vf);
 
-	if (status_old & VIRTIO_CONFIG_S_DRIVER_OK) {
-		ifcvf_stop_datapath(adapter);
+	if (status & VIRTIO_CONFIG_S_DRIVER_OK)
 		ifcvf_free_irq(vf);
-	}
 
-	ifcvf_reset_vring(adapter);
+	ifcvf_reset(vf);
 
 	return 0;
 }
-- 
2.39.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V2 4/5] vDPA/ifcvf: synchronize irqs in the reset routine

[Jason Wang]

On Mon, May 8, 2023 at 6:05 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>
> This commit synchronize irqs of the virtqueues
> and config space in the reset routine.
> Thus ifcvf_stop_hw() and reset() are refactored as well.
>
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
>  drivers/vdpa/ifcvf/ifcvf_base.c | 41 +++++++++++++++++++++--------
>  drivers/vdpa/ifcvf/ifcvf_base.h |  1 +
>  drivers/vdpa/ifcvf/ifcvf_main.c | 46 +++++----------------------------
>  3 files changed, 38 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c
> index 79e313c5e10e..1f39290baa38 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_base.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
> @@ -170,12 +170,9 @@ void ifcvf_set_status(struct ifcvf_hw *hw, u8 status)
>
>  void ifcvf_reset(struct ifcvf_hw *hw)
>  {
> -       hw->config_cb.callback = NULL;
> -       hw->config_cb.private = NULL;
> -
>         ifcvf_set_status(hw, 0);
> -       /* flush set_status, make sure VF is stopped, reset */
> -       ifcvf_get_status(hw);
> +       while (ifcvf_get_status(hw))
> +               msleep(1);
>  }
>
>  u64 ifcvf_get_hw_features(struct ifcvf_hw *hw)
> @@ -368,20 +365,42 @@ void ifcvf_set_vq_ready(struct ifcvf_hw *hw, u16 qid, bool ready)
>         vp_iowrite16(ready, &cfg->queue_enable);
>  }
>
> -static void ifcvf_hw_disable(struct ifcvf_hw *hw)
> +static void ifcvf_reset_vring(struct ifcvf_hw *hw)
>  {
> -       u32 i;
> +       u16 qid;
> +
> +       for (qid = 0; qid < hw->nr_vring; qid++) {
> +               hw->vring[qid].cb.callback = NULL;
> +               hw->vring[qid].cb.private = NULL;
> +               ifcvf_set_vq_vector(hw, qid, VIRTIO_MSI_NO_VECTOR);
> +       }
> +}
>
> +static void ifcvf_reset_config_handler(struct ifcvf_hw *hw)
> +{
> +       hw->config_cb.callback = NULL;
> +       hw->config_cb.private = NULL;
>         ifcvf_set_config_vector(hw, VIRTIO_MSI_NO_VECTOR);
> -       for (i = 0; i < hw->nr_vring; i++) {
> -               ifcvf_set_vq_vector(hw, i, VIRTIO_MSI_NO_VECTOR);
> +}
> +
> +static void ifcvf_synchronize_irq(struct ifcvf_hw *hw)
> +{
> +       u32 nvectors = hw->num_msix_vectors;
> +       struct pci_dev *pdev = hw->pdev;
> +       int i, irq;
> +
> +       for (i = 0; i < nvectors; i++) {
> +               irq = pci_irq_vector(pdev, i);
> +               if (irq >= 0)
> +                       synchronize_irq(irq);
>         }
>  }
>
>  void ifcvf_stop_hw(struct ifcvf_hw *hw)
>  {
> -       ifcvf_hw_disable(hw);
> -       ifcvf_reset(hw);
> +       ifcvf_synchronize_irq(hw);
> +       ifcvf_reset_vring(hw);
> +       ifcvf_reset_config_handler(hw);

Nit:

So the name of this function is kind of misleading since irq
synchronization and virtqueue/config handler are not belong to
hardware?

Maybe it would be better to call it ifcvf_stop().

Thanks

>  }
>
>  void ifcvf_notify_queue(struct ifcvf_hw *hw, u16 qid)
> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
> index d34d3bc0dbf4..7430f80779be 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
> @@ -82,6 +82,7 @@ struct ifcvf_hw {
>         int vqs_reused_irq;
>         u16 nr_vring;
>         /* VIRTIO_PCI_CAP_DEVICE_CFG size */
> +       u32 num_msix_vectors;
>         u32 cap_dev_config_size;
>         struct pci_dev *pdev;
>  };
> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
> index 968687159e44..3401b9901dd2 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
> @@ -125,6 +125,7 @@ static void ifcvf_free_irq(struct ifcvf_hw *vf)
>         ifcvf_free_vq_irq(vf);
>         ifcvf_free_config_irq(vf);
>         ifcvf_free_irq_vectors(pdev);
> +       vf->num_msix_vectors = 0;
>  }
>
>  /* ifcvf MSIX vectors allocator, this helper tries to allocate
> @@ -343,36 +344,11 @@ static int ifcvf_request_irq(struct ifcvf_hw *vf)
>         if (ret)
>                 return ret;
>
> -       return 0;
> -}
> -
> -static int ifcvf_stop_datapath(struct ifcvf_adapter *adapter)
> -{
> -       struct ifcvf_hw *vf = adapter->vf;
> -       int i;
> -
> -       for (i = 0; i < vf->nr_vring; i++)
> -               vf->vring[i].cb.callback = NULL;
> -
> -       ifcvf_stop_hw(vf);
> +       vf->num_msix_vectors = nvectors;
>
>         return 0;
>  }
>
> -static void ifcvf_reset_vring(struct ifcvf_adapter *adapter)
> -{
> -       struct ifcvf_hw *vf = adapter->vf;
> -       int i;
> -
> -       for (i = 0; i < vf->nr_vring; i++) {
> -               vf->vring[i].last_avail_idx = 0;
> -               vf->vring[i].cb.callback = NULL;
> -               vf->vring[i].cb.private = NULL;
> -       }
> -
> -       ifcvf_reset(vf);
> -}
> -
>  static struct ifcvf_adapter *vdpa_to_adapter(struct vdpa_device *vdpa_dev)
>  {
>         return container_of(vdpa_dev, struct ifcvf_adapter, vdpa);
> @@ -462,23 +438,15 @@ static void ifcvf_vdpa_set_status(struct vdpa_device *vdpa_dev, u8 status)
>
>  static int ifcvf_vdpa_reset(struct vdpa_device *vdpa_dev)
>  {
> -       struct ifcvf_adapter *adapter;
> -       struct ifcvf_hw *vf;
> -       u8 status_old;
> -
> -       vf  = vdpa_to_vf(vdpa_dev);
> -       adapter = vdpa_to_adapter(vdpa_dev);
> -       status_old = ifcvf_get_status(vf);
> +       struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
> +       u8 status = ifcvf_get_status(vf);
>
> -       if (status_old == 0)
> -               return 0;
> +       ifcvf_stop_hw(vf);
>
> -       if (status_old & VIRTIO_CONFIG_S_DRIVER_OK) {
> -               ifcvf_stop_datapath(adapter);
> +       if (status & VIRTIO_CONFIG_S_DRIVER_OK)
>                 ifcvf_free_irq(vf);
> -       }
>
> -       ifcvf_reset_vring(adapter);
> +       ifcvf_reset(vf);
>
>         return 0;
>  }
> --
> 2.39.1
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V2 4/5] vDPA/ifcvf: synchronize irqs in the reset routine

[Jason Wang]

On Wed, May 24, 2023 at 4:03 PM Jason Wang <jasowang@redhat.com> wrote:
>
> On Mon, May 8, 2023 at 6:05 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
> >
> > This commit synchronize irqs of the virtqueues
> > and config space in the reset routine.
> > Thus ifcvf_stop_hw() and reset() are refactored as well.
> >
> > Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> > ---
> >  drivers/vdpa/ifcvf/ifcvf_base.c | 41 +++++++++++++++++++++--------
> >  drivers/vdpa/ifcvf/ifcvf_base.h |  1 +
> >  drivers/vdpa/ifcvf/ifcvf_main.c | 46 +++++----------------------------
> >  3 files changed, 38 insertions(+), 50 deletions(-)
> >
> > diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c
> > index 79e313c5e10e..1f39290baa38 100644
> > --- a/drivers/vdpa/ifcvf/ifcvf_base.c
> > +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
> > @@ -170,12 +170,9 @@ void ifcvf_set_status(struct ifcvf_hw *hw, u8 status)
> >
> >  void ifcvf_reset(struct ifcvf_hw *hw)
> >  {
> > -       hw->config_cb.callback = NULL;
> > -       hw->config_cb.private = NULL;
> > -
> >         ifcvf_set_status(hw, 0);
> > -       /* flush set_status, make sure VF is stopped, reset */
> > -       ifcvf_get_status(hw);
> > +       while (ifcvf_get_status(hw))
> > +               msleep(1);
> >  }
> >
> >  u64 ifcvf_get_hw_features(struct ifcvf_hw *hw)
> > @@ -368,20 +365,42 @@ void ifcvf_set_vq_ready(struct ifcvf_hw *hw, u16 qid, bool ready)
> >         vp_iowrite16(ready, &cfg->queue_enable);
> >  }
> >
> > -static void ifcvf_hw_disable(struct ifcvf_hw *hw)
> > +static void ifcvf_reset_vring(struct ifcvf_hw *hw)
> >  {
> > -       u32 i;
> > +       u16 qid;
> > +
> > +       for (qid = 0; qid < hw->nr_vring; qid++) {
> > +               hw->vring[qid].cb.callback = NULL;
> > +               hw->vring[qid].cb.private = NULL;
> > +               ifcvf_set_vq_vector(hw, qid, VIRTIO_MSI_NO_VECTOR);
> > +       }
> > +}
> >
> > +static void ifcvf_reset_config_handler(struct ifcvf_hw *hw)
> > +{
> > +       hw->config_cb.callback = NULL;
> > +       hw->config_cb.private = NULL;
> >         ifcvf_set_config_vector(hw, VIRTIO_MSI_NO_VECTOR);
> > -       for (i = 0; i < hw->nr_vring; i++) {
> > -               ifcvf_set_vq_vector(hw, i, VIRTIO_MSI_NO_VECTOR);
> > +}
> > +
> > +static void ifcvf_synchronize_irq(struct ifcvf_hw *hw)
> > +{
> > +       u32 nvectors = hw->num_msix_vectors;
> > +       struct pci_dev *pdev = hw->pdev;
> > +       int i, irq;
> > +
> > +       for (i = 0; i < nvectors; i++) {
> > +               irq = pci_irq_vector(pdev, i);
> > +               if (irq >= 0)
> > +                       synchronize_irq(irq);
> >         }
> >  }
> >
> >  void ifcvf_stop_hw(struct ifcvf_hw *hw)
> >  {
> > -       ifcvf_hw_disable(hw);
> > -       ifcvf_reset(hw);
> > +       ifcvf_synchronize_irq(hw);
> > +       ifcvf_reset_vring(hw);
> > +       ifcvf_reset_config_handler(hw);
>
> Nit:
>
> So the name of this function is kind of misleading since irq
> synchronization and virtqueue/config handler are not belong to
> hardware?
>
> Maybe it would be better to call it ifcvf_stop().

I think we can tweak this on top. So

Acked-by: Jason Wang <jasowang@redhat.com>

Thanks

>
> Thanks
>
> >  }
> >
> >  void ifcvf_notify_queue(struct ifcvf_hw *hw, u16 qid)
> > diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
> > index d34d3bc0dbf4..7430f80779be 100644
> > --- a/drivers/vdpa/ifcvf/ifcvf_base.h
> > +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
> > @@ -82,6 +82,7 @@ struct ifcvf_hw {
> >         int vqs_reused_irq;
> >         u16 nr_vring;
> >         /* VIRTIO_PCI_CAP_DEVICE_CFG size */
> > +       u32 num_msix_vectors;
> >         u32 cap_dev_config_size;
> >         struct pci_dev *pdev;
> >  };
> > diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
> > index 968687159e44..3401b9901dd2 100644
> > --- a/drivers/vdpa/ifcvf/ifcvf_main.c
> > +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
> > @@ -125,6 +125,7 @@ static void ifcvf_free_irq(struct ifcvf_hw *vf)
> >         ifcvf_free_vq_irq(vf);
> >         ifcvf_free_config_irq(vf);
> >         ifcvf_free_irq_vectors(pdev);
> > +       vf->num_msix_vectors = 0;
> >  }
> >
> >  /* ifcvf MSIX vectors allocator, this helper tries to allocate
> > @@ -343,36 +344,11 @@ static int ifcvf_request_irq(struct ifcvf_hw *vf)
> >         if (ret)
> >                 return ret;
> >
> > -       return 0;
> > -}
> > -
> > -static int ifcvf_stop_datapath(struct ifcvf_adapter *adapter)
> > -{
> > -       struct ifcvf_hw *vf = adapter->vf;
> > -       int i;
> > -
> > -       for (i = 0; i < vf->nr_vring; i++)
> > -               vf->vring[i].cb.callback = NULL;
> > -
> > -       ifcvf_stop_hw(vf);
> > +       vf->num_msix_vectors = nvectors;
> >
> >         return 0;
> >  }
> >
> > -static void ifcvf_reset_vring(struct ifcvf_adapter *adapter)
> > -{
> > -       struct ifcvf_hw *vf = adapter->vf;
> > -       int i;
> > -
> > -       for (i = 0; i < vf->nr_vring; i++) {
> > -               vf->vring[i].last_avail_idx = 0;
> > -               vf->vring[i].cb.callback = NULL;
> > -               vf->vring[i].cb.private = NULL;
> > -       }
> > -
> > -       ifcvf_reset(vf);
> > -}
> > -
> >  static struct ifcvf_adapter *vdpa_to_adapter(struct vdpa_device *vdpa_dev)
> >  {
> >         return container_of(vdpa_dev, struct ifcvf_adapter, vdpa);
> > @@ -462,23 +438,15 @@ static void ifcvf_vdpa_set_status(struct vdpa_device *vdpa_dev, u8 status)
> >
> >  static int ifcvf_vdpa_reset(struct vdpa_device *vdpa_dev)
> >  {
> > -       struct ifcvf_adapter *adapter;
> > -       struct ifcvf_hw *vf;
> > -       u8 status_old;
> > -
> > -       vf  = vdpa_to_vf(vdpa_dev);
> > -       adapter = vdpa_to_adapter(vdpa_dev);
> > -       status_old = ifcvf_get_status(vf);
> > +       struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
> > +       u8 status = ifcvf_get_status(vf);
> >
> > -       if (status_old == 0)
> > -               return 0;
> > +       ifcvf_stop_hw(vf);
> >
> > -       if (status_old & VIRTIO_CONFIG_S_DRIVER_OK) {
> > -               ifcvf_stop_datapath(adapter);
> > +       if (status & VIRTIO_CONFIG_S_DRIVER_OK)
> >                 ifcvf_free_irq(vf);
> > -       }
> >
> > -       ifcvf_reset_vring(adapter);
> > +       ifcvf_reset(vf);
> >
> >         return 0;
> >  }
> > --
> > 2.39.1
> >

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V2 4/5] vDPA/ifcvf: synchronize irqs in the reset routine

[Zhu, Lingshan]

On 5/24/2023 4:03 PM, Jason Wang wrote:
> On Mon, May 8, 2023 at 6:05 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>> This commit synchronize irqs of the virtqueues
>> and config space in the reset routine.
>> Thus ifcvf_stop_hw() and reset() are refactored as well.
>>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>> ---
>>   drivers/vdpa/ifcvf/ifcvf_base.c | 41 +++++++++++++++++++++--------
>>   drivers/vdpa/ifcvf/ifcvf_base.h |  1 +
>>   drivers/vdpa/ifcvf/ifcvf_main.c | 46 +++++----------------------------
>>   3 files changed, 38 insertions(+), 50 deletions(-)
>>
>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c
>> index 79e313c5e10e..1f39290baa38 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_base.c
>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
>> @@ -170,12 +170,9 @@ void ifcvf_set_status(struct ifcvf_hw *hw, u8 status)
>>
>>   void ifcvf_reset(struct ifcvf_hw *hw)
>>   {
>> -       hw->config_cb.callback = NULL;
>> -       hw->config_cb.private = NULL;
>> -
>>          ifcvf_set_status(hw, 0);
>> -       /* flush set_status, make sure VF is stopped, reset */
>> -       ifcvf_get_status(hw);
>> +       while (ifcvf_get_status(hw))
>> +               msleep(1);
>>   }
>>
>>   u64 ifcvf_get_hw_features(struct ifcvf_hw *hw)
>> @@ -368,20 +365,42 @@ void ifcvf_set_vq_ready(struct ifcvf_hw *hw, u16 qid, bool ready)
>>          vp_iowrite16(ready, &cfg->queue_enable);
>>   }
>>
>> -static void ifcvf_hw_disable(struct ifcvf_hw *hw)
>> +static void ifcvf_reset_vring(struct ifcvf_hw *hw)
>>   {
>> -       u32 i;
>> +       u16 qid;
>> +
>> +       for (qid = 0; qid < hw->nr_vring; qid++) {
>> +               hw->vring[qid].cb.callback = NULL;
>> +               hw->vring[qid].cb.private = NULL;
>> +               ifcvf_set_vq_vector(hw, qid, VIRTIO_MSI_NO_VECTOR);
>> +       }
>> +}
>>
>> +static void ifcvf_reset_config_handler(struct ifcvf_hw *hw)
>> +{
>> +       hw->config_cb.callback = NULL;
>> +       hw->config_cb.private = NULL;
>>          ifcvf_set_config_vector(hw, VIRTIO_MSI_NO_VECTOR);
>> -       for (i = 0; i < hw->nr_vring; i++) {
>> -               ifcvf_set_vq_vector(hw, i, VIRTIO_MSI_NO_VECTOR);
>> +}
>> +
>> +static void ifcvf_synchronize_irq(struct ifcvf_hw *hw)
>> +{
>> +       u32 nvectors = hw->num_msix_vectors;
>> +       struct pci_dev *pdev = hw->pdev;
>> +       int i, irq;
>> +
>> +       for (i = 0; i < nvectors; i++) {
>> +               irq = pci_irq_vector(pdev, i);
>> +               if (irq >= 0)
>> +                       synchronize_irq(irq);
>>          }
>>   }
>>
>>   void ifcvf_stop_hw(struct ifcvf_hw *hw)
>>   {
>> -       ifcvf_hw_disable(hw);
>> -       ifcvf_reset(hw);
>> +       ifcvf_synchronize_irq(hw);
>> +       ifcvf_reset_vring(hw);
>> +       ifcvf_reset_config_handler(hw);
> Nit:
>
> So the name of this function is kind of misleading since irq
> synchronization and virtqueue/config handler are not belong to
> hardware?
>
> Maybe it would be better to call it ifcvf_stop().
Sure, I will send a V3 with this renaming,
do you ack patch 1/5?

Thanks
Zhu Lingshan
>
> Thanks
>
>>   }
>>
>>   void ifcvf_notify_queue(struct ifcvf_hw *hw, u16 qid)
>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
>> index d34d3bc0dbf4..7430f80779be 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
>> @@ -82,6 +82,7 @@ struct ifcvf_hw {
>>          int vqs_reused_irq;
>>          u16 nr_vring;
>>          /* VIRTIO_PCI_CAP_DEVICE_CFG size */
>> +       u32 num_msix_vectors;
>>          u32 cap_dev_config_size;
>>          struct pci_dev *pdev;
>>   };
>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
>> index 968687159e44..3401b9901dd2 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>> @@ -125,6 +125,7 @@ static void ifcvf_free_irq(struct ifcvf_hw *vf)
>>          ifcvf_free_vq_irq(vf);
>>          ifcvf_free_config_irq(vf);
>>          ifcvf_free_irq_vectors(pdev);
>> +       vf->num_msix_vectors = 0;
>>   }
>>
>>   /* ifcvf MSIX vectors allocator, this helper tries to allocate
>> @@ -343,36 +344,11 @@ static int ifcvf_request_irq(struct ifcvf_hw *vf)
>>          if (ret)
>>                  return ret;
>>
>> -       return 0;
>> -}
>> -
>> -static int ifcvf_stop_datapath(struct ifcvf_adapter *adapter)
>> -{
>> -       struct ifcvf_hw *vf = adapter->vf;
>> -       int i;
>> -
>> -       for (i = 0; i < vf->nr_vring; i++)
>> -               vf->vring[i].cb.callback = NULL;
>> -
>> -       ifcvf_stop_hw(vf);
>> +       vf->num_msix_vectors = nvectors;
>>
>>          return 0;
>>   }
>>
>> -static void ifcvf_reset_vring(struct ifcvf_adapter *adapter)
>> -{
>> -       struct ifcvf_hw *vf = adapter->vf;
>> -       int i;
>> -
>> -       for (i = 0; i < vf->nr_vring; i++) {
>> -               vf->vring[i].last_avail_idx = 0;
>> -               vf->vring[i].cb.callback = NULL;
>> -               vf->vring[i].cb.private = NULL;
>> -       }
>> -
>> -       ifcvf_reset(vf);
>> -}
>> -
>>   static struct ifcvf_adapter *vdpa_to_adapter(struct vdpa_device *vdpa_dev)
>>   {
>>          return container_of(vdpa_dev, struct ifcvf_adapter, vdpa);
>> @@ -462,23 +438,15 @@ static void ifcvf_vdpa_set_status(struct vdpa_device *vdpa_dev, u8 status)
>>
>>   static int ifcvf_vdpa_reset(struct vdpa_device *vdpa_dev)
>>   {
>> -       struct ifcvf_adapter *adapter;
>> -       struct ifcvf_hw *vf;
>> -       u8 status_old;
>> -
>> -       vf  = vdpa_to_vf(vdpa_dev);
>> -       adapter = vdpa_to_adapter(vdpa_dev);
>> -       status_old = ifcvf_get_status(vf);
>> +       struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>> +       u8 status = ifcvf_get_status(vf);
>>
>> -       if (status_old == 0)
>> -               return 0;
>> +       ifcvf_stop_hw(vf);
>>
>> -       if (status_old & VIRTIO_CONFIG_S_DRIVER_OK) {
>> -               ifcvf_stop_datapath(adapter);
>> +       if (status & VIRTIO_CONFIG_S_DRIVER_OK)
>>                  ifcvf_free_irq(vf);
>> -       }
>>
>> -       ifcvf_reset_vring(adapter);
>> +       ifcvf_reset(vf);
>>
>>          return 0;
>>   }
>> --
>> 2.39.1
>>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V2 4/5] vDPA/ifcvf: synchronize irqs in the reset routine

[Jason Wang]

On Thu, May 25, 2023 at 5:38 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>
>
>
> On 5/24/2023 4:03 PM, Jason Wang wrote:
> > On Mon, May 8, 2023 at 6:05 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
> >> This commit synchronize irqs of the virtqueues
> >> and config space in the reset routine.
> >> Thus ifcvf_stop_hw() and reset() are refactored as well.
> >>
> >> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> >> ---
> >>   drivers/vdpa/ifcvf/ifcvf_base.c | 41 +++++++++++++++++++++--------
> >>   drivers/vdpa/ifcvf/ifcvf_base.h |  1 +
> >>   drivers/vdpa/ifcvf/ifcvf_main.c | 46 +++++----------------------------
> >>   3 files changed, 38 insertions(+), 50 deletions(-)
> >>
> >> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c
> >> index 79e313c5e10e..1f39290baa38 100644
> >> --- a/drivers/vdpa/ifcvf/ifcvf_base.c
> >> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
> >> @@ -170,12 +170,9 @@ void ifcvf_set_status(struct ifcvf_hw *hw, u8 status)
> >>
> >>   void ifcvf_reset(struct ifcvf_hw *hw)
> >>   {
> >> -       hw->config_cb.callback = NULL;
> >> -       hw->config_cb.private = NULL;
> >> -
> >>          ifcvf_set_status(hw, 0);
> >> -       /* flush set_status, make sure VF is stopped, reset */
> >> -       ifcvf_get_status(hw);
> >> +       while (ifcvf_get_status(hw))
> >> +               msleep(1);
> >>   }
> >>
> >>   u64 ifcvf_get_hw_features(struct ifcvf_hw *hw)
> >> @@ -368,20 +365,42 @@ void ifcvf_set_vq_ready(struct ifcvf_hw *hw, u16 qid, bool ready)
> >>          vp_iowrite16(ready, &cfg->queue_enable);
> >>   }
> >>
> >> -static void ifcvf_hw_disable(struct ifcvf_hw *hw)
> >> +static void ifcvf_reset_vring(struct ifcvf_hw *hw)
> >>   {
> >> -       u32 i;
> >> +       u16 qid;
> >> +
> >> +       for (qid = 0; qid < hw->nr_vring; qid++) {
> >> +               hw->vring[qid].cb.callback = NULL;
> >> +               hw->vring[qid].cb.private = NULL;
> >> +               ifcvf_set_vq_vector(hw, qid, VIRTIO_MSI_NO_VECTOR);
> >> +       }
> >> +}
> >>
> >> +static void ifcvf_reset_config_handler(struct ifcvf_hw *hw)
> >> +{
> >> +       hw->config_cb.callback = NULL;
> >> +       hw->config_cb.private = NULL;
> >>          ifcvf_set_config_vector(hw, VIRTIO_MSI_NO_VECTOR);
> >> -       for (i = 0; i < hw->nr_vring; i++) {
> >> -               ifcvf_set_vq_vector(hw, i, VIRTIO_MSI_NO_VECTOR);
> >> +}
> >> +
> >> +static void ifcvf_synchronize_irq(struct ifcvf_hw *hw)
> >> +{
> >> +       u32 nvectors = hw->num_msix_vectors;
> >> +       struct pci_dev *pdev = hw->pdev;
> >> +       int i, irq;
> >> +
> >> +       for (i = 0; i < nvectors; i++) {
> >> +               irq = pci_irq_vector(pdev, i);
> >> +               if (irq >= 0)
> >> +                       synchronize_irq(irq);
> >>          }
> >>   }
> >>
> >>   void ifcvf_stop_hw(struct ifcvf_hw *hw)
> >>   {
> >> -       ifcvf_hw_disable(hw);
> >> -       ifcvf_reset(hw);
> >> +       ifcvf_synchronize_irq(hw);
> >> +       ifcvf_reset_vring(hw);
> >> +       ifcvf_reset_config_handler(hw);
> > Nit:
> >
> > So the name of this function is kind of misleading since irq
> > synchronization and virtqueue/config handler are not belong to
> > hardware?
> >
> > Maybe it would be better to call it ifcvf_stop().
> Sure, I will send a V3 with this renaming,
> do you ack patch 1/5?

Yes, I think I've acked to that patch.

Thanks

>
> Thanks
> Zhu Lingshan
> >
> > Thanks
> >
> >>   }
> >>
> >>   void ifcvf_notify_queue(struct ifcvf_hw *hw, u16 qid)
> >> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
> >> index d34d3bc0dbf4..7430f80779be 100644
> >> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
> >> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
> >> @@ -82,6 +82,7 @@ struct ifcvf_hw {
> >>          int vqs_reused_irq;
> >>          u16 nr_vring;
> >>          /* VIRTIO_PCI_CAP_DEVICE_CFG size */
> >> +       u32 num_msix_vectors;
> >>          u32 cap_dev_config_size;
> >>          struct pci_dev *pdev;
> >>   };
> >> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
> >> index 968687159e44..3401b9901dd2 100644
> >> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
> >> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
> >> @@ -125,6 +125,7 @@ static void ifcvf_free_irq(struct ifcvf_hw *vf)
> >>          ifcvf_free_vq_irq(vf);
> >>          ifcvf_free_config_irq(vf);
> >>          ifcvf_free_irq_vectors(pdev);
> >> +       vf->num_msix_vectors = 0;
> >>   }
> >>
> >>   /* ifcvf MSIX vectors allocator, this helper tries to allocate
> >> @@ -343,36 +344,11 @@ static int ifcvf_request_irq(struct ifcvf_hw *vf)
> >>          if (ret)
> >>                  return ret;
> >>
> >> -       return 0;
> >> -}
> >> -
> >> -static int ifcvf_stop_datapath(struct ifcvf_adapter *adapter)
> >> -{
> >> -       struct ifcvf_hw *vf = adapter->vf;
> >> -       int i;
> >> -
> >> -       for (i = 0; i < vf->nr_vring; i++)
> >> -               vf->vring[i].cb.callback = NULL;
> >> -
> >> -       ifcvf_stop_hw(vf);
> >> +       vf->num_msix_vectors = nvectors;
> >>
> >>          return 0;
> >>   }
> >>
> >> -static void ifcvf_reset_vring(struct ifcvf_adapter *adapter)
> >> -{
> >> -       struct ifcvf_hw *vf = adapter->vf;
> >> -       int i;
> >> -
> >> -       for (i = 0; i < vf->nr_vring; i++) {
> >> -               vf->vring[i].last_avail_idx = 0;
> >> -               vf->vring[i].cb.callback = NULL;
> >> -               vf->vring[i].cb.private = NULL;
> >> -       }
> >> -
> >> -       ifcvf_reset(vf);
> >> -}
> >> -
> >>   static struct ifcvf_adapter *vdpa_to_adapter(struct vdpa_device *vdpa_dev)
> >>   {
> >>          return container_of(vdpa_dev, struct ifcvf_adapter, vdpa);
> >> @@ -462,23 +438,15 @@ static void ifcvf_vdpa_set_status(struct vdpa_device *vdpa_dev, u8 status)
> >>
> >>   static int ifcvf_vdpa_reset(struct vdpa_device *vdpa_dev)
> >>   {
> >> -       struct ifcvf_adapter *adapter;
> >> -       struct ifcvf_hw *vf;
> >> -       u8 status_old;
> >> -
> >> -       vf  = vdpa_to_vf(vdpa_dev);
> >> -       adapter = vdpa_to_adapter(vdpa_dev);
> >> -       status_old = ifcvf_get_status(vf);
> >> +       struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
> >> +       u8 status = ifcvf_get_status(vf);
> >>
> >> -       if (status_old == 0)
> >> -               return 0;
> >> +       ifcvf_stop_hw(vf);
> >>
> >> -       if (status_old & VIRTIO_CONFIG_S_DRIVER_OK) {
> >> -               ifcvf_stop_datapath(adapter);
> >> +       if (status & VIRTIO_CONFIG_S_DRIVER_OK)
> >>                  ifcvf_free_irq(vf);
> >> -       }
> >>
> >> -       ifcvf_reset_vring(adapter);
> >> +       ifcvf_reset(vf);
> >>
> >>          return 0;
> >>   }
> >> --
> >> 2.39.1
> >>
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V2 4/5] vDPA/ifcvf: synchronize irqs in the reset routine

[Zhu, Lingshan]

On 5/26/2023 9:34 AM, Jason Wang wrote:
> On Thu, May 25, 2023 at 5:38 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>>
>>
>> On 5/24/2023 4:03 PM, Jason Wang wrote:
>>> On Mon, May 8, 2023 at 6:05 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>>>> This commit synchronize irqs of the virtqueues
>>>> and config space in the reset routine.
>>>> Thus ifcvf_stop_hw() and reset() are refactored as well.
>>>>
>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>> ---
>>>>    drivers/vdpa/ifcvf/ifcvf_base.c | 41 +++++++++++++++++++++--------
>>>>    drivers/vdpa/ifcvf/ifcvf_base.h |  1 +
>>>>    drivers/vdpa/ifcvf/ifcvf_main.c | 46 +++++----------------------------
>>>>    3 files changed, 38 insertions(+), 50 deletions(-)
>>>>
>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c
>>>> index 79e313c5e10e..1f39290baa38 100644
>>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.c
>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
>>>> @@ -170,12 +170,9 @@ void ifcvf_set_status(struct ifcvf_hw *hw, u8 status)
>>>>
>>>>    void ifcvf_reset(struct ifcvf_hw *hw)
>>>>    {
>>>> -       hw->config_cb.callback = NULL;
>>>> -       hw->config_cb.private = NULL;
>>>> -
>>>>           ifcvf_set_status(hw, 0);
>>>> -       /* flush set_status, make sure VF is stopped, reset */
>>>> -       ifcvf_get_status(hw);
>>>> +       while (ifcvf_get_status(hw))
>>>> +               msleep(1);
>>>>    }
>>>>
>>>>    u64 ifcvf_get_hw_features(struct ifcvf_hw *hw)
>>>> @@ -368,20 +365,42 @@ void ifcvf_set_vq_ready(struct ifcvf_hw *hw, u16 qid, bool ready)
>>>>           vp_iowrite16(ready, &cfg->queue_enable);
>>>>    }
>>>>
>>>> -static void ifcvf_hw_disable(struct ifcvf_hw *hw)
>>>> +static void ifcvf_reset_vring(struct ifcvf_hw *hw)
>>>>    {
>>>> -       u32 i;
>>>> +       u16 qid;
>>>> +
>>>> +       for (qid = 0; qid < hw->nr_vring; qid++) {
>>>> +               hw->vring[qid].cb.callback = NULL;
>>>> +               hw->vring[qid].cb.private = NULL;
>>>> +               ifcvf_set_vq_vector(hw, qid, VIRTIO_MSI_NO_VECTOR);
>>>> +       }
>>>> +}
>>>>
>>>> +static void ifcvf_reset_config_handler(struct ifcvf_hw *hw)
>>>> +{
>>>> +       hw->config_cb.callback = NULL;
>>>> +       hw->config_cb.private = NULL;
>>>>           ifcvf_set_config_vector(hw, VIRTIO_MSI_NO_VECTOR);
>>>> -       for (i = 0; i < hw->nr_vring; i++) {
>>>> -               ifcvf_set_vq_vector(hw, i, VIRTIO_MSI_NO_VECTOR);
>>>> +}
>>>> +
>>>> +static void ifcvf_synchronize_irq(struct ifcvf_hw *hw)
>>>> +{
>>>> +       u32 nvectors = hw->num_msix_vectors;
>>>> +       struct pci_dev *pdev = hw->pdev;
>>>> +       int i, irq;
>>>> +
>>>> +       for (i = 0; i < nvectors; i++) {
>>>> +               irq = pci_irq_vector(pdev, i);
>>>> +               if (irq >= 0)
>>>> +                       synchronize_irq(irq);
>>>>           }
>>>>    }
>>>>
>>>>    void ifcvf_stop_hw(struct ifcvf_hw *hw)
>>>>    {
>>>> -       ifcvf_hw_disable(hw);
>>>> -       ifcvf_reset(hw);
>>>> +       ifcvf_synchronize_irq(hw);
>>>> +       ifcvf_reset_vring(hw);
>>>> +       ifcvf_reset_config_handler(hw);
>>> Nit:
>>>
>>> So the name of this function is kind of misleading since irq
>>> synchronization and virtqueue/config handler are not belong to
>>> hardware?
>>>
>>> Maybe it would be better to call it ifcvf_stop().
>> Sure, I will send a V3 with this renaming,
>> do you ack patch 1/5?
> Yes, I think I've acked to that patch.
I will send a V3 with this renaming and your ack for patch 1/5
>
> Thanks
>
>> Thanks
>> Zhu Lingshan
>>> Thanks
>>>
>>>>    }
>>>>
>>>>    void ifcvf_notify_queue(struct ifcvf_hw *hw, u16 qid)
>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
>>>> index d34d3bc0dbf4..7430f80779be 100644
>>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
>>>> @@ -82,6 +82,7 @@ struct ifcvf_hw {
>>>>           int vqs_reused_irq;
>>>>           u16 nr_vring;
>>>>           /* VIRTIO_PCI_CAP_DEVICE_CFG size */
>>>> +       u32 num_msix_vectors;
>>>>           u32 cap_dev_config_size;
>>>>           struct pci_dev *pdev;
>>>>    };
>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>> index 968687159e44..3401b9901dd2 100644
>>>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>> @@ -125,6 +125,7 @@ static void ifcvf_free_irq(struct ifcvf_hw *vf)
>>>>           ifcvf_free_vq_irq(vf);
>>>>           ifcvf_free_config_irq(vf);
>>>>           ifcvf_free_irq_vectors(pdev);
>>>> +       vf->num_msix_vectors = 0;
>>>>    }
>>>>
>>>>    /* ifcvf MSIX vectors allocator, this helper tries to allocate
>>>> @@ -343,36 +344,11 @@ static int ifcvf_request_irq(struct ifcvf_hw *vf)
>>>>           if (ret)
>>>>                   return ret;
>>>>
>>>> -       return 0;
>>>> -}
>>>> -
>>>> -static int ifcvf_stop_datapath(struct ifcvf_adapter *adapter)
>>>> -{
>>>> -       struct ifcvf_hw *vf = adapter->vf;
>>>> -       int i;
>>>> -
>>>> -       for (i = 0; i < vf->nr_vring; i++)
>>>> -               vf->vring[i].cb.callback = NULL;
>>>> -
>>>> -       ifcvf_stop_hw(vf);
>>>> +       vf->num_msix_vectors = nvectors;
>>>>
>>>>           return 0;
>>>>    }
>>>>
>>>> -static void ifcvf_reset_vring(struct ifcvf_adapter *adapter)
>>>> -{
>>>> -       struct ifcvf_hw *vf = adapter->vf;
>>>> -       int i;
>>>> -
>>>> -       for (i = 0; i < vf->nr_vring; i++) {
>>>> -               vf->vring[i].last_avail_idx = 0;
>>>> -               vf->vring[i].cb.callback = NULL;
>>>> -               vf->vring[i].cb.private = NULL;
>>>> -       }
>>>> -
>>>> -       ifcvf_reset(vf);
>>>> -}
>>>> -
>>>>    static struct ifcvf_adapter *vdpa_to_adapter(struct vdpa_device *vdpa_dev)
>>>>    {
>>>>           return container_of(vdpa_dev, struct ifcvf_adapter, vdpa);
>>>> @@ -462,23 +438,15 @@ static void ifcvf_vdpa_set_status(struct vdpa_device *vdpa_dev, u8 status)
>>>>
>>>>    static int ifcvf_vdpa_reset(struct vdpa_device *vdpa_dev)
>>>>    {
>>>> -       struct ifcvf_adapter *adapter;
>>>> -       struct ifcvf_hw *vf;
>>>> -       u8 status_old;
>>>> -
>>>> -       vf  = vdpa_to_vf(vdpa_dev);
>>>> -       adapter = vdpa_to_adapter(vdpa_dev);
>>>> -       status_old = ifcvf_get_status(vf);
>>>> +       struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>>>> +       u8 status = ifcvf_get_status(vf);
>>>>
>>>> -       if (status_old == 0)
>>>> -               return 0;
>>>> +       ifcvf_stop_hw(vf);
>>>>
>>>> -       if (status_old & VIRTIO_CONFIG_S_DRIVER_OK) {
>>>> -               ifcvf_stop_datapath(adapter);
>>>> +       if (status & VIRTIO_CONFIG_S_DRIVER_OK)
>>>>                   ifcvf_free_irq(vf);
>>>> -       }
>>>>
>>>> -       ifcvf_reset_vring(adapter);
>>>> +       ifcvf_reset(vf);
>>>>
>>>>           return 0;
>>>>    }
>>>> --
>>>> 2.39.1
>>>>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V2 4/5] vDPA/ifcvf: synchronize irqs in the reset routine

[Zhu, Lingshan]

On 5/26/2023 11:36 AM, Zhu, Lingshan wrote:
>
>
> On 5/26/2023 9:34 AM, Jason Wang wrote:
>> On Thu, May 25, 2023 at 5:38 PM Zhu, Lingshan 
>> <lingshan.zhu@intel.com> wrote:
>>>
>>>
>>> On 5/24/2023 4:03 PM, Jason Wang wrote:
>>>> On Mon, May 8, 2023 at 6:05 PM Zhu Lingshan 
>>>> <lingshan.zhu@intel.com> wrote:
>>>>> This commit synchronize irqs of the virtqueues
>>>>> and config space in the reset routine.
>>>>> Thus ifcvf_stop_hw() and reset() are refactored as well.
>>>>>
>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>>> ---
>>>>>    drivers/vdpa/ifcvf/ifcvf_base.c | 41 +++++++++++++++++++++--------
>>>>>    drivers/vdpa/ifcvf/ifcvf_base.h |  1 +
>>>>>    drivers/vdpa/ifcvf/ifcvf_main.c | 46 
>>>>> +++++----------------------------
>>>>>    3 files changed, 38 insertions(+), 50 deletions(-)
>>>>>
>>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c 
>>>>> b/drivers/vdpa/ifcvf/ifcvf_base.c
>>>>> index 79e313c5e10e..1f39290baa38 100644
>>>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.c
>>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
>>>>> @@ -170,12 +170,9 @@ void ifcvf_set_status(struct ifcvf_hw *hw, u8 
>>>>> status)
>>>>>
>>>>>    void ifcvf_reset(struct ifcvf_hw *hw)
>>>>>    {
>>>>> -       hw->config_cb.callback = NULL;
>>>>> -       hw->config_cb.private = NULL;
>>>>> -
>>>>>           ifcvf_set_status(hw, 0);
>>>>> -       /* flush set_status, make sure VF is stopped, reset */
>>>>> -       ifcvf_get_status(hw);
>>>>> +       while (ifcvf_get_status(hw))
>>>>> +               msleep(1);
>>>>>    }
>>>>>
>>>>>    u64 ifcvf_get_hw_features(struct ifcvf_hw *hw)
>>>>> @@ -368,20 +365,42 @@ void ifcvf_set_vq_ready(struct ifcvf_hw *hw, 
>>>>> u16 qid, bool ready)
>>>>>           vp_iowrite16(ready, &cfg->queue_enable);
>>>>>    }
>>>>>
>>>>> -static void ifcvf_hw_disable(struct ifcvf_hw *hw)
>>>>> +static void ifcvf_reset_vring(struct ifcvf_hw *hw)
>>>>>    {
>>>>> -       u32 i;
>>>>> +       u16 qid;
>>>>> +
>>>>> +       for (qid = 0; qid < hw->nr_vring; qid++) {
>>>>> +               hw->vring[qid].cb.callback = NULL;
>>>>> +               hw->vring[qid].cb.private = NULL;
>>>>> +               ifcvf_set_vq_vector(hw, qid, VIRTIO_MSI_NO_VECTOR);
>>>>> +       }
>>>>> +}
>>>>>
>>>>> +static void ifcvf_reset_config_handler(struct ifcvf_hw *hw)
>>>>> +{
>>>>> +       hw->config_cb.callback = NULL;
>>>>> +       hw->config_cb.private = NULL;
>>>>>           ifcvf_set_config_vector(hw, VIRTIO_MSI_NO_VECTOR);
>>>>> -       for (i = 0; i < hw->nr_vring; i++) {
>>>>> -               ifcvf_set_vq_vector(hw, i, VIRTIO_MSI_NO_VECTOR);
>>>>> +}
>>>>> +
>>>>> +static void ifcvf_synchronize_irq(struct ifcvf_hw *hw)
>>>>> +{
>>>>> +       u32 nvectors = hw->num_msix_vectors;
>>>>> +       struct pci_dev *pdev = hw->pdev;
>>>>> +       int i, irq;
>>>>> +
>>>>> +       for (i = 0; i < nvectors; i++) {
>>>>> +               irq = pci_irq_vector(pdev, i);
>>>>> +               if (irq >= 0)
>>>>> +                       synchronize_irq(irq);
>>>>>           }
>>>>>    }
>>>>>
>>>>>    void ifcvf_stop_hw(struct ifcvf_hw *hw)
>>>>>    {
>>>>> -       ifcvf_hw_disable(hw);
>>>>> -       ifcvf_reset(hw);
>>>>> +       ifcvf_synchronize_irq(hw);
>>>>> +       ifcvf_reset_vring(hw);
>>>>> +       ifcvf_reset_config_handler(hw);
>>>> Nit:
>>>>
>>>> So the name of this function is kind of misleading since irq
>>>> synchronization and virtqueue/config handler are not belong to
>>>> hardware?
>>>>
>>>> Maybe it would be better to call it ifcvf_stop().
>>> Sure, I will send a V3 with this renaming,
>>> do you ack patch 1/5?
>> Yes, I think I've acked to that patch.
> I will send a V3 with this renaming and your ack for patch 1/5
By the way, do you ack this one after this function renaming?
If so, I will also include your ack in V3, so we don't need another
review process, I will ping Michael for a merge.
>>
>> Thanks
>>
>>> Thanks
>>> Zhu Lingshan
>>>> Thanks
>>>>
>>>>>    }
>>>>>
>>>>>    void ifcvf_notify_queue(struct ifcvf_hw *hw, u16 qid)
>>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h 
>>>>> b/drivers/vdpa/ifcvf/ifcvf_base.h
>>>>> index d34d3bc0dbf4..7430f80779be 100644
>>>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
>>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
>>>>> @@ -82,6 +82,7 @@ struct ifcvf_hw {
>>>>>           int vqs_reused_irq;
>>>>>           u16 nr_vring;
>>>>>           /* VIRTIO_PCI_CAP_DEVICE_CFG size */
>>>>> +       u32 num_msix_vectors;
>>>>>           u32 cap_dev_config_size;
>>>>>           struct pci_dev *pdev;
>>>>>    };
>>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c 
>>>>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>> index 968687159e44..3401b9901dd2 100644
>>>>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>> @@ -125,6 +125,7 @@ static void ifcvf_free_irq(struct ifcvf_hw *vf)
>>>>>           ifcvf_free_vq_irq(vf);
>>>>>           ifcvf_free_config_irq(vf);
>>>>>           ifcvf_free_irq_vectors(pdev);
>>>>> +       vf->num_msix_vectors = 0;
>>>>>    }
>>>>>
>>>>>    /* ifcvf MSIX vectors allocator, this helper tries to allocate
>>>>> @@ -343,36 +344,11 @@ static int ifcvf_request_irq(struct ifcvf_hw 
>>>>> *vf)
>>>>>           if (ret)
>>>>>                   return ret;
>>>>>
>>>>> -       return 0;
>>>>> -}
>>>>> -
>>>>> -static int ifcvf_stop_datapath(struct ifcvf_adapter *adapter)
>>>>> -{
>>>>> -       struct ifcvf_hw *vf = adapter->vf;
>>>>> -       int i;
>>>>> -
>>>>> -       for (i = 0; i < vf->nr_vring; i++)
>>>>> -               vf->vring[i].cb.callback = NULL;
>>>>> -
>>>>> -       ifcvf_stop_hw(vf);
>>>>> +       vf->num_msix_vectors = nvectors;
>>>>>
>>>>>           return 0;
>>>>>    }
>>>>>
>>>>> -static void ifcvf_reset_vring(struct ifcvf_adapter *adapter)
>>>>> -{
>>>>> -       struct ifcvf_hw *vf = adapter->vf;
>>>>> -       int i;
>>>>> -
>>>>> -       for (i = 0; i < vf->nr_vring; i++) {
>>>>> -               vf->vring[i].last_avail_idx = 0;
>>>>> -               vf->vring[i].cb.callback = NULL;
>>>>> -               vf->vring[i].cb.private = NULL;
>>>>> -       }
>>>>> -
>>>>> -       ifcvf_reset(vf);
>>>>> -}
>>>>> -
>>>>>    static struct ifcvf_adapter *vdpa_to_adapter(struct vdpa_device 
>>>>> *vdpa_dev)
>>>>>    {
>>>>>           return container_of(vdpa_dev, struct ifcvf_adapter, vdpa);
>>>>> @@ -462,23 +438,15 @@ static void ifcvf_vdpa_set_status(struct 
>>>>> vdpa_device *vdpa_dev, u8 status)
>>>>>
>>>>>    static int ifcvf_vdpa_reset(struct vdpa_device *vdpa_dev)
>>>>>    {
>>>>> -       struct ifcvf_adapter *adapter;
>>>>> -       struct ifcvf_hw *vf;
>>>>> -       u8 status_old;
>>>>> -
>>>>> -       vf  = vdpa_to_vf(vdpa_dev);
>>>>> -       adapter = vdpa_to_adapter(vdpa_dev);
>>>>> -       status_old = ifcvf_get_status(vf);
>>>>> +       struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>>>>> +       u8 status = ifcvf_get_status(vf);
>>>>>
>>>>> -       if (status_old == 0)
>>>>> -               return 0;
>>>>> +       ifcvf_stop_hw(vf);
>>>>>
>>>>> -       if (status_old & VIRTIO_CONFIG_S_DRIVER_OK) {
>>>>> -               ifcvf_stop_datapath(adapter);
>>>>> +       if (status & VIRTIO_CONFIG_S_DRIVER_OK)
>>>>>                   ifcvf_free_irq(vf);
>>>>> -       }
>>>>>
>>>>> -       ifcvf_reset_vring(adapter);
>>>>> +       ifcvf_reset(vf);
>>>>>
>>>>>           return 0;
>>>>>    }
>>>>> -- 
>>>>> 2.39.1
>>>>>
>
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V2 4/5] vDPA/ifcvf: synchronize irqs in the reset routine

[Jason Wang]

On Fri, May 26, 2023 at 1:30 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>
>
>
> On 5/26/2023 11:36 AM, Zhu, Lingshan wrote:
> >
> >
> > On 5/26/2023 9:34 AM, Jason Wang wrote:
> >> On Thu, May 25, 2023 at 5:38 PM Zhu, Lingshan
> >> <lingshan.zhu@intel.com> wrote:
> >>>
> >>>
> >>> On 5/24/2023 4:03 PM, Jason Wang wrote:
> >>>> On Mon, May 8, 2023 at 6:05 PM Zhu Lingshan
> >>>> <lingshan.zhu@intel.com> wrote:
> >>>>> This commit synchronize irqs of the virtqueues
> >>>>> and config space in the reset routine.
> >>>>> Thus ifcvf_stop_hw() and reset() are refactored as well.
> >>>>>
> >>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> >>>>> ---
> >>>>>    drivers/vdpa/ifcvf/ifcvf_base.c | 41 +++++++++++++++++++++--------
> >>>>>    drivers/vdpa/ifcvf/ifcvf_base.h |  1 +
> >>>>>    drivers/vdpa/ifcvf/ifcvf_main.c | 46
> >>>>> +++++----------------------------
> >>>>>    3 files changed, 38 insertions(+), 50 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c
> >>>>> b/drivers/vdpa/ifcvf/ifcvf_base.c
> >>>>> index 79e313c5e10e..1f39290baa38 100644
> >>>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.c
> >>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
> >>>>> @@ -170,12 +170,9 @@ void ifcvf_set_status(struct ifcvf_hw *hw, u8
> >>>>> status)
> >>>>>
> >>>>>    void ifcvf_reset(struct ifcvf_hw *hw)
> >>>>>    {
> >>>>> -       hw->config_cb.callback = NULL;
> >>>>> -       hw->config_cb.private = NULL;
> >>>>> -
> >>>>>           ifcvf_set_status(hw, 0);
> >>>>> -       /* flush set_status, make sure VF is stopped, reset */
> >>>>> -       ifcvf_get_status(hw);
> >>>>> +       while (ifcvf_get_status(hw))
> >>>>> +               msleep(1);
> >>>>>    }
> >>>>>
> >>>>>    u64 ifcvf_get_hw_features(struct ifcvf_hw *hw)
> >>>>> @@ -368,20 +365,42 @@ void ifcvf_set_vq_ready(struct ifcvf_hw *hw,
> >>>>> u16 qid, bool ready)
> >>>>>           vp_iowrite16(ready, &cfg->queue_enable);
> >>>>>    }
> >>>>>
> >>>>> -static void ifcvf_hw_disable(struct ifcvf_hw *hw)
> >>>>> +static void ifcvf_reset_vring(struct ifcvf_hw *hw)
> >>>>>    {
> >>>>> -       u32 i;
> >>>>> +       u16 qid;
> >>>>> +
> >>>>> +       for (qid = 0; qid < hw->nr_vring; qid++) {
> >>>>> +               hw->vring[qid].cb.callback = NULL;
> >>>>> +               hw->vring[qid].cb.private = NULL;
> >>>>> +               ifcvf_set_vq_vector(hw, qid, VIRTIO_MSI_NO_VECTOR);
> >>>>> +       }
> >>>>> +}
> >>>>>
> >>>>> +static void ifcvf_reset_config_handler(struct ifcvf_hw *hw)
> >>>>> +{
> >>>>> +       hw->config_cb.callback = NULL;
> >>>>> +       hw->config_cb.private = NULL;
> >>>>>           ifcvf_set_config_vector(hw, VIRTIO_MSI_NO_VECTOR);
> >>>>> -       for (i = 0; i < hw->nr_vring; i++) {
> >>>>> -               ifcvf_set_vq_vector(hw, i, VIRTIO_MSI_NO_VECTOR);
> >>>>> +}
> >>>>> +
> >>>>> +static void ifcvf_synchronize_irq(struct ifcvf_hw *hw)
> >>>>> +{
> >>>>> +       u32 nvectors = hw->num_msix_vectors;
> >>>>> +       struct pci_dev *pdev = hw->pdev;
> >>>>> +       int i, irq;
> >>>>> +
> >>>>> +       for (i = 0; i < nvectors; i++) {
> >>>>> +               irq = pci_irq_vector(pdev, i);
> >>>>> +               if (irq >= 0)
> >>>>> +                       synchronize_irq(irq);
> >>>>>           }
> >>>>>    }
> >>>>>
> >>>>>    void ifcvf_stop_hw(struct ifcvf_hw *hw)
> >>>>>    {
> >>>>> -       ifcvf_hw_disable(hw);
> >>>>> -       ifcvf_reset(hw);
> >>>>> +       ifcvf_synchronize_irq(hw);
> >>>>> +       ifcvf_reset_vring(hw);
> >>>>> +       ifcvf_reset_config_handler(hw);
> >>>> Nit:
> >>>>
> >>>> So the name of this function is kind of misleading since irq
> >>>> synchronization and virtqueue/config handler are not belong to
> >>>> hardware?
> >>>>
> >>>> Maybe it would be better to call it ifcvf_stop().
> >>> Sure, I will send a V3 with this renaming,
> >>> do you ack patch 1/5?
> >> Yes, I think I've acked to that patch.
> > I will send a V3 with this renaming and your ack for patch 1/5
> By the way, do you ack this one after this function renaming?
> If so, I will also include your ack in V3, so we don't need another
> review process, I will ping Michael for a merge.

Have you seen the ack here?

https://lists.linuxfoundation.org/pipermail/virtualization/2023-May/066890.html

Thanks

> >>
> >> Thanks
> >>
> >>> Thanks
> >>> Zhu Lingshan
> >>>> Thanks
> >>>>
> >>>>>    }
> >>>>>
> >>>>>    void ifcvf_notify_queue(struct ifcvf_hw *hw, u16 qid)
> >>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h
> >>>>> b/drivers/vdpa/ifcvf/ifcvf_base.h
> >>>>> index d34d3bc0dbf4..7430f80779be 100644
> >>>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
> >>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
> >>>>> @@ -82,6 +82,7 @@ struct ifcvf_hw {
> >>>>>           int vqs_reused_irq;
> >>>>>           u16 nr_vring;
> >>>>>           /* VIRTIO_PCI_CAP_DEVICE_CFG size */
> >>>>> +       u32 num_msix_vectors;
> >>>>>           u32 cap_dev_config_size;
> >>>>>           struct pci_dev *pdev;
> >>>>>    };
> >>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c
> >>>>> b/drivers/vdpa/ifcvf/ifcvf_main.c
> >>>>> index 968687159e44..3401b9901dd2 100644
> >>>>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
> >>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
> >>>>> @@ -125,6 +125,7 @@ static void ifcvf_free_irq(struct ifcvf_hw *vf)
> >>>>>           ifcvf_free_vq_irq(vf);
> >>>>>           ifcvf_free_config_irq(vf);
> >>>>>           ifcvf_free_irq_vectors(pdev);
> >>>>> +       vf->num_msix_vectors = 0;
> >>>>>    }
> >>>>>
> >>>>>    /* ifcvf MSIX vectors allocator, this helper tries to allocate
> >>>>> @@ -343,36 +344,11 @@ static int ifcvf_request_irq(struct ifcvf_hw
> >>>>> *vf)
> >>>>>           if (ret)
> >>>>>                   return ret;
> >>>>>
> >>>>> -       return 0;
> >>>>> -}
> >>>>> -
> >>>>> -static int ifcvf_stop_datapath(struct ifcvf_adapter *adapter)
> >>>>> -{
> >>>>> -       struct ifcvf_hw *vf = adapter->vf;
> >>>>> -       int i;
> >>>>> -
> >>>>> -       for (i = 0; i < vf->nr_vring; i++)
> >>>>> -               vf->vring[i].cb.callback = NULL;
> >>>>> -
> >>>>> -       ifcvf_stop_hw(vf);
> >>>>> +       vf->num_msix_vectors = nvectors;
> >>>>>
> >>>>>           return 0;
> >>>>>    }
> >>>>>
> >>>>> -static void ifcvf_reset_vring(struct ifcvf_adapter *adapter)
> >>>>> -{
> >>>>> -       struct ifcvf_hw *vf = adapter->vf;
> >>>>> -       int i;
> >>>>> -
> >>>>> -       for (i = 0; i < vf->nr_vring; i++) {
> >>>>> -               vf->vring[i].last_avail_idx = 0;
> >>>>> -               vf->vring[i].cb.callback = NULL;
> >>>>> -               vf->vring[i].cb.private = NULL;
> >>>>> -       }
> >>>>> -
> >>>>> -       ifcvf_reset(vf);
> >>>>> -}
> >>>>> -
> >>>>>    static struct ifcvf_adapter *vdpa_to_adapter(struct vdpa_device
> >>>>> *vdpa_dev)
> >>>>>    {
> >>>>>           return container_of(vdpa_dev, struct ifcvf_adapter, vdpa);
> >>>>> @@ -462,23 +438,15 @@ static void ifcvf_vdpa_set_status(struct
> >>>>> vdpa_device *vdpa_dev, u8 status)
> >>>>>
> >>>>>    static int ifcvf_vdpa_reset(struct vdpa_device *vdpa_dev)
> >>>>>    {
> >>>>> -       struct ifcvf_adapter *adapter;
> >>>>> -       struct ifcvf_hw *vf;
> >>>>> -       u8 status_old;
> >>>>> -
> >>>>> -       vf  = vdpa_to_vf(vdpa_dev);
> >>>>> -       adapter = vdpa_to_adapter(vdpa_dev);
> >>>>> -       status_old = ifcvf_get_status(vf);
> >>>>> +       struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
> >>>>> +       u8 status = ifcvf_get_status(vf);
> >>>>>
> >>>>> -       if (status_old == 0)
> >>>>> -               return 0;
> >>>>> +       ifcvf_stop_hw(vf);
> >>>>>
> >>>>> -       if (status_old & VIRTIO_CONFIG_S_DRIVER_OK) {
> >>>>> -               ifcvf_stop_datapath(adapter);
> >>>>> +       if (status & VIRTIO_CONFIG_S_DRIVER_OK)
> >>>>>                   ifcvf_free_irq(vf);
> >>>>> -       }
> >>>>>
> >>>>> -       ifcvf_reset_vring(adapter);
> >>>>> +       ifcvf_reset(vf);
> >>>>>
> >>>>>           return 0;
> >>>>>    }
> >>>>> --
> >>>>> 2.39.1
> >>>>>
> >
> > _______________________________________________
> > Virtualization mailing list
> > Virtualization@lists.linux-foundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V2 4/5] vDPA/ifcvf: synchronize irqs in the reset routine

[Zhu, Lingshan]

On 5/26/2023 2:09 PM, Jason Wang wrote:
> On Fri, May 26, 2023 at 1:30 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>>
>>
>> On 5/26/2023 11:36 AM, Zhu, Lingshan wrote:
>>>
>>> On 5/26/2023 9:34 AM, Jason Wang wrote:
>>>> On Thu, May 25, 2023 at 5:38 PM Zhu, Lingshan
>>>> <lingshan.zhu@intel.com> wrote:
>>>>>
>>>>> On 5/24/2023 4:03 PM, Jason Wang wrote:
>>>>>> On Mon, May 8, 2023 at 6:05 PM Zhu Lingshan
>>>>>> <lingshan.zhu@intel.com> wrote:
>>>>>>> This commit synchronize irqs of the virtqueues
>>>>>>> and config space in the reset routine.
>>>>>>> Thus ifcvf_stop_hw() and reset() are refactored as well.
>>>>>>>
>>>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>>>>> ---
>>>>>>>     drivers/vdpa/ifcvf/ifcvf_base.c | 41 +++++++++++++++++++++--------
>>>>>>>     drivers/vdpa/ifcvf/ifcvf_base.h |  1 +
>>>>>>>     drivers/vdpa/ifcvf/ifcvf_main.c | 46
>>>>>>> +++++----------------------------
>>>>>>>     3 files changed, 38 insertions(+), 50 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c
>>>>>>> b/drivers/vdpa/ifcvf/ifcvf_base.c
>>>>>>> index 79e313c5e10e..1f39290baa38 100644
>>>>>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.c
>>>>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
>>>>>>> @@ -170,12 +170,9 @@ void ifcvf_set_status(struct ifcvf_hw *hw, u8
>>>>>>> status)
>>>>>>>
>>>>>>>     void ifcvf_reset(struct ifcvf_hw *hw)
>>>>>>>     {
>>>>>>> -       hw->config_cb.callback = NULL;
>>>>>>> -       hw->config_cb.private = NULL;
>>>>>>> -
>>>>>>>            ifcvf_set_status(hw, 0);
>>>>>>> -       /* flush set_status, make sure VF is stopped, reset */
>>>>>>> -       ifcvf_get_status(hw);
>>>>>>> +       while (ifcvf_get_status(hw))
>>>>>>> +               msleep(1);
>>>>>>>     }
>>>>>>>
>>>>>>>     u64 ifcvf_get_hw_features(struct ifcvf_hw *hw)
>>>>>>> @@ -368,20 +365,42 @@ void ifcvf_set_vq_ready(struct ifcvf_hw *hw,
>>>>>>> u16 qid, bool ready)
>>>>>>>            vp_iowrite16(ready, &cfg->queue_enable);
>>>>>>>     }
>>>>>>>
>>>>>>> -static void ifcvf_hw_disable(struct ifcvf_hw *hw)
>>>>>>> +static void ifcvf_reset_vring(struct ifcvf_hw *hw)
>>>>>>>     {
>>>>>>> -       u32 i;
>>>>>>> +       u16 qid;
>>>>>>> +
>>>>>>> +       for (qid = 0; qid < hw->nr_vring; qid++) {
>>>>>>> +               hw->vring[qid].cb.callback = NULL;
>>>>>>> +               hw->vring[qid].cb.private = NULL;
>>>>>>> +               ifcvf_set_vq_vector(hw, qid, VIRTIO_MSI_NO_VECTOR);
>>>>>>> +       }
>>>>>>> +}
>>>>>>>
>>>>>>> +static void ifcvf_reset_config_handler(struct ifcvf_hw *hw)
>>>>>>> +{
>>>>>>> +       hw->config_cb.callback = NULL;
>>>>>>> +       hw->config_cb.private = NULL;
>>>>>>>            ifcvf_set_config_vector(hw, VIRTIO_MSI_NO_VECTOR);
>>>>>>> -       for (i = 0; i < hw->nr_vring; i++) {
>>>>>>> -               ifcvf_set_vq_vector(hw, i, VIRTIO_MSI_NO_VECTOR);
>>>>>>> +}
>>>>>>> +
>>>>>>> +static void ifcvf_synchronize_irq(struct ifcvf_hw *hw)
>>>>>>> +{
>>>>>>> +       u32 nvectors = hw->num_msix_vectors;
>>>>>>> +       struct pci_dev *pdev = hw->pdev;
>>>>>>> +       int i, irq;
>>>>>>> +
>>>>>>> +       for (i = 0; i < nvectors; i++) {
>>>>>>> +               irq = pci_irq_vector(pdev, i);
>>>>>>> +               if (irq >= 0)
>>>>>>> +                       synchronize_irq(irq);
>>>>>>>            }
>>>>>>>     }
>>>>>>>
>>>>>>>     void ifcvf_stop_hw(struct ifcvf_hw *hw)
>>>>>>>     {
>>>>>>> -       ifcvf_hw_disable(hw);
>>>>>>> -       ifcvf_reset(hw);
>>>>>>> +       ifcvf_synchronize_irq(hw);
>>>>>>> +       ifcvf_reset_vring(hw);
>>>>>>> +       ifcvf_reset_config_handler(hw);
>>>>>> Nit:
>>>>>>
>>>>>> So the name of this function is kind of misleading since irq
>>>>>> synchronization and virtqueue/config handler are not belong to
>>>>>> hardware?
>>>>>>
>>>>>> Maybe it would be better to call it ifcvf_stop().
>>>>> Sure, I will send a V3 with this renaming,
>>>>> do you ack patch 1/5?
>>>> Yes, I think I've acked to that patch.
>>> I will send a V3 with this renaming and your ack for patch 1/5
>> By the way, do you ack this one after this function renaming?
>> If so, I will also include your ack in V3, so we don't need another
>> review process, I will ping Michael for a merge.
> Have you seen the ack here?
>
> https://lists.linuxfoundation.org/pipermail/virtualization/2023-May/066890.html
Sorry I missed this, a patch only renames a function may be trivial, so 
I will
rebase this 4/5 with your ack. So all patches are ack-ed, thanks!
>
> Thanks
>
>>>> Thanks
>>>>
>>>>> Thanks
>>>>> Zhu Lingshan
>>>>>> Thanks
>>>>>>
>>>>>>>     }
>>>>>>>
>>>>>>>     void ifcvf_notify_queue(struct ifcvf_hw *hw, u16 qid)
>>>>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h
>>>>>>> b/drivers/vdpa/ifcvf/ifcvf_base.h
>>>>>>> index d34d3bc0dbf4..7430f80779be 100644
>>>>>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
>>>>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
>>>>>>> @@ -82,6 +82,7 @@ struct ifcvf_hw {
>>>>>>>            int vqs_reused_irq;
>>>>>>>            u16 nr_vring;
>>>>>>>            /* VIRTIO_PCI_CAP_DEVICE_CFG size */
>>>>>>> +       u32 num_msix_vectors;
>>>>>>>            u32 cap_dev_config_size;
>>>>>>>            struct pci_dev *pdev;
>>>>>>>     };
>>>>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>>>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>>>> index 968687159e44..3401b9901dd2 100644
>>>>>>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>>>> @@ -125,6 +125,7 @@ static void ifcvf_free_irq(struct ifcvf_hw *vf)
>>>>>>>            ifcvf_free_vq_irq(vf);
>>>>>>>            ifcvf_free_config_irq(vf);
>>>>>>>            ifcvf_free_irq_vectors(pdev);
>>>>>>> +       vf->num_msix_vectors = 0;
>>>>>>>     }
>>>>>>>
>>>>>>>     /* ifcvf MSIX vectors allocator, this helper tries to allocate
>>>>>>> @@ -343,36 +344,11 @@ static int ifcvf_request_irq(struct ifcvf_hw
>>>>>>> *vf)
>>>>>>>            if (ret)
>>>>>>>                    return ret;
>>>>>>>
>>>>>>> -       return 0;
>>>>>>> -}
>>>>>>> -
>>>>>>> -static int ifcvf_stop_datapath(struct ifcvf_adapter *adapter)
>>>>>>> -{
>>>>>>> -       struct ifcvf_hw *vf = adapter->vf;
>>>>>>> -       int i;
>>>>>>> -
>>>>>>> -       for (i = 0; i < vf->nr_vring; i++)
>>>>>>> -               vf->vring[i].cb.callback = NULL;
>>>>>>> -
>>>>>>> -       ifcvf_stop_hw(vf);
>>>>>>> +       vf->num_msix_vectors = nvectors;
>>>>>>>
>>>>>>>            return 0;
>>>>>>>     }
>>>>>>>
>>>>>>> -static void ifcvf_reset_vring(struct ifcvf_adapter *adapter)
>>>>>>> -{
>>>>>>> -       struct ifcvf_hw *vf = adapter->vf;
>>>>>>> -       int i;
>>>>>>> -
>>>>>>> -       for (i = 0; i < vf->nr_vring; i++) {
>>>>>>> -               vf->vring[i].last_avail_idx = 0;
>>>>>>> -               vf->vring[i].cb.callback = NULL;
>>>>>>> -               vf->vring[i].cb.private = NULL;
>>>>>>> -       }
>>>>>>> -
>>>>>>> -       ifcvf_reset(vf);
>>>>>>> -}
>>>>>>> -
>>>>>>>     static struct ifcvf_adapter *vdpa_to_adapter(struct vdpa_device
>>>>>>> *vdpa_dev)
>>>>>>>     {
>>>>>>>            return container_of(vdpa_dev, struct ifcvf_adapter, vdpa);
>>>>>>> @@ -462,23 +438,15 @@ static void ifcvf_vdpa_set_status(struct
>>>>>>> vdpa_device *vdpa_dev, u8 status)
>>>>>>>
>>>>>>>     static int ifcvf_vdpa_reset(struct vdpa_device *vdpa_dev)
>>>>>>>     {
>>>>>>> -       struct ifcvf_adapter *adapter;
>>>>>>> -       struct ifcvf_hw *vf;
>>>>>>> -       u8 status_old;
>>>>>>> -
>>>>>>> -       vf  = vdpa_to_vf(vdpa_dev);
>>>>>>> -       adapter = vdpa_to_adapter(vdpa_dev);
>>>>>>> -       status_old = ifcvf_get_status(vf);
>>>>>>> +       struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>>>>>>> +       u8 status = ifcvf_get_status(vf);
>>>>>>>
>>>>>>> -       if (status_old == 0)
>>>>>>> -               return 0;
>>>>>>> +       ifcvf_stop_hw(vf);
>>>>>>>
>>>>>>> -       if (status_old & VIRTIO_CONFIG_S_DRIVER_OK) {
>>>>>>> -               ifcvf_stop_datapath(adapter);
>>>>>>> +       if (status & VIRTIO_CONFIG_S_DRIVER_OK)
>>>>>>>                    ifcvf_free_irq(vf);
>>>>>>> -       }
>>>>>>>
>>>>>>> -       ifcvf_reset_vring(adapter);
>>>>>>> +       ifcvf_reset(vf);
>>>>>>>
>>>>>>>            return 0;
>>>>>>>     }
>>>>>>> --
>>>>>>> 2.39.1
>>>>>>>
>>> _______________________________________________
>>> Virtualization mailing list
>>> Virtualization@lists.linux-foundation.org
>>> https://lists.linuxfoundation.org/mailman/listinfo/virtualization

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH V2 5/5] vDPA/ifcvf: a vendor driver should not set _CONFIG_S_FAILED

[Zhu Lingshan]

VIRTIO_CONFIG_S_FAILED indicates the guest driver has given up
the device due to fatal errors. So it is the guest decision,
the vendor driver should not set this status to the device.

Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
Acked-by: Jason Wang <jasowang@redhat.com>
---
 drivers/vdpa/ifcvf/ifcvf_main.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
index 3401b9901dd2..b413688e13c4 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -426,9 +426,7 @@ static void ifcvf_vdpa_set_status(struct vdpa_device *vdpa_dev, u8 status)
 	    !(status_old & VIRTIO_CONFIG_S_DRIVER_OK)) {
 		ret = ifcvf_request_irq(vf);
 		if (ret) {
-			status = ifcvf_get_status(vf);
-			status |= VIRTIO_CONFIG_S_FAILED;
-			ifcvf_set_status(vf, status);
+			IFCVF_ERR(vf->pdev, "failed to request irq with error %d\n", ret);
 			return;
 		}
 	}
-- 
2.39.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V2 0/5] vDPA/ifcvf: implement immediate initialization mechanism

[Zhu, Lingshan]

ping

On 5/9/2023 2:05 AM, Zhu Lingshan wrote:
> Formerly, ifcvf driver has implemented a lazy-initialization mechanism
> for the virtqueues and other config space contents,
> it would store all configurations that passed down from the userspace,
> then load them to the device config space upon DRIVER_OK.
>
> This can not serve live migration, so this series implement an
> immediate initialization mechanism, which means rather than the
> former store-load process, the virtio operations like vq ops
> would take immediate actions by access the virtio registers.
>
> This series also implement irq synchronization in the reset
> routine
>
> Changes from V1:
> 1)pull device status in devce_reset (Jason)
> 2)simplify the procedure which sycn irqs (Jason)
> 3)fix typos(Michael)
>
> Zhu Lingshan (5):
>    vDPA/ifcvf: virt queue ops take immediate actions
>    vDPA/ifcvf: get_driver_features from virtio registers
>    vDPA/ifcvf: retire ifcvf_start_datapath and ifcvf_add_status
>    vDPA/ifcvf: synchronize irqs in the reset routine
>    vDPA/ifcvf: a vendor driver should not set _CONFIG_S_FAILED
>
>   drivers/vdpa/ifcvf/ifcvf_base.c | 146 ++++++++++++++++++--------------
>   drivers/vdpa/ifcvf/ifcvf_base.h |  17 ++--
>   drivers/vdpa/ifcvf/ifcvf_main.c |  98 ++++-----------------
>   3 files changed, 108 insertions(+), 153 deletions(-)
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization