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

[PATCH 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

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

 drivers/vdpa/ifcvf/ifcvf_base.c | 162 +++++++++++++++++++-------------
 drivers/vdpa/ifcvf/ifcvf_base.h |  16 ++--
 drivers/vdpa/ifcvf/ifcvf_main.c |  97 ++++---------------
 3 files changed, 122 insertions(+), 153 deletions(-)

-- 
2.39.1

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

[PATCH 1/5] 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

[PATCH 2/5] get_driver_features from virito 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

[PATCH 3/5] 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>
---
 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 4/5] 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 | 61 ++++++++++++++++++++++++++-------
 drivers/vdpa/ifcvf/ifcvf_main.c | 45 +++---------------------
 2 files changed, 54 insertions(+), 52 deletions(-)

diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c
index 79e313c5e10e..49949aec20ef 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.c
+++ b/drivers/vdpa/ifcvf/ifcvf_base.c
@@ -170,12 +170,7 @@ 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);
 }
 
 u64 ifcvf_get_hw_features(struct ifcvf_hw *hw)
@@ -368,20 +363,62 @@ 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 synchronize_per_vq_irq(struct ifcvf_hw *hw)
 {
-	u32 i;
+	u16 qid;
 
-	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);
+	for (qid = 0; qid < hw->nr_vring; qid++) {
+		if (hw->vring[qid].irq != -EINVAL)
+			synchronize_irq(hw->vring[qid].irq);
 	}
 }
 
+static void synchronize_vqs_reused_irq(struct ifcvf_hw *hw)
+{
+	if (hw->vqs_reused_irq != -EINVAL)
+		synchronize_irq(hw->vqs_reused_irq);
+}
+
+static void synchronize_vq_irq(struct ifcvf_hw *hw)
+{
+	u8 status = hw->msix_vector_status;
+
+	if (status == MSIX_VECTOR_PER_VQ_AND_CONFIG)
+		synchronize_per_vq_irq(hw);
+	else
+		synchronize_vqs_reused_irq(hw);
+}
+
+static void synchronize_config_irq(struct ifcvf_hw *hw)
+{
+	if (hw->config_irq != -EINVAL)
+		synchronize_irq(hw->config_irq);
+}
+
+static void ifcvf_reset_vring(struct ifcvf_hw *hw)
+{
+	u16 qid;
+
+	for (qid = 0; qid < hw->nr_vring; qid++) {
+		synchronize_vq_irq(hw);
+		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)
+{
+	synchronize_config_irq(hw);
+	hw->config_cb.callback = NULL;
+	hw->config_cb.private = NULL;
+	ifcvf_set_config_vector(hw, VIRTIO_MSI_NO_VECTOR);
+}
+
 void ifcvf_stop_hw(struct ifcvf_hw *hw)
 {
-	ifcvf_hw_disable(hw);
-	ifcvf_reset(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_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
index 968687159e44..15c6157ee841 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -346,33 +346,6 @@ static int ifcvf_request_irq(struct ifcvf_hw *vf)
 	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);
-
-	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 +435,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

[PATCH 5/5] 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>
---
 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 15c6157ee841..f228fba74c61 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -423,9 +423,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 0/5] vDPA/ifcvf: implement immediate initialization mechanism

[Jason Wang]

On Fri, Mar 31, 2023 at 8:49 PM Zhu Lingshan <lingshan.zhu@intel.com> 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.

Is there any chance that ifcvf can use virtio_pci_modern_dev library?

Then we don't need to duplicate the codes.

Note that pds_vdpa will be the second user for virtio_pci_modern_dev
library (and the first vDPA parent to use that library).

Thanks

>
> This series also implement irq synchronization in the reset
> routine
>
> Zhu Lingshan (5):
>   virt queue ops take immediate actions
>   get_driver_features from virito registers
>   retire ifcvf_start_datapath and ifcvf_add_status
>   synchronize irqs in the reset routine
>   a vendor driver should not set _CONFIG_S_FAILED
>
>  drivers/vdpa/ifcvf/ifcvf_base.c | 162 +++++++++++++++++++-------------
>  drivers/vdpa/ifcvf/ifcvf_base.h |  16 ++--
>  drivers/vdpa/ifcvf/ifcvf_main.c |  97 ++++---------------
>  3 files changed, 122 insertions(+), 153 deletions(-)
>
> --
> 2.39.1
>

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

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

[Zhu, Lingshan]

On 4/3/2023 1:28 PM, Jason Wang wrote:
> On Fri, Mar 31, 2023 at 8:49 PM Zhu Lingshan <lingshan.zhu@intel.com> 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.
> Is there any chance that ifcvf can use virtio_pci_modern_dev library?
>
> Then we don't need to duplicate the codes.
>
> Note that pds_vdpa will be the second user for virtio_pci_modern_dev
> library (and the first vDPA parent to use that library).
Yes I agree this library can help a lot for a standard virtio pci device.
But this change would be huge, its like require to change every line of
the driver. For example current driver functions work on the adapter and
ifcvf_hw, if we wants to reuse the lib, we need the driver work on 
struct virtio_pci_modern_device.
Almost need to re-write the driver.

Can we plan this huge change in following series?

Thanks,
Zhu Lingshan
>
> Thanks
>
>> This series also implement irq synchronization in the reset
>> routine
>>
>> Zhu Lingshan (5):
>>    virt queue ops take immediate actions
>>    get_driver_features from virito registers
>>    retire ifcvf_start_datapath and ifcvf_add_status
>>    synchronize irqs in the reset routine
>>    a vendor driver should not set _CONFIG_S_FAILED
>>
>>   drivers/vdpa/ifcvf/ifcvf_base.c | 162 +++++++++++++++++++-------------
>>   drivers/vdpa/ifcvf/ifcvf_base.h |  16 ++--
>>   drivers/vdpa/ifcvf/ifcvf_main.c |  97 ++++---------------
>>   3 files changed, 122 insertions(+), 153 deletions(-)
>>
>> --
>> 2.39.1
>>

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

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

[Zhu, Lingshan]

On 4/3/2023 6:10 PM, Zhu, Lingshan wrote:
>
>
> On 4/3/2023 1:28 PM, Jason Wang wrote:
>> On Fri, Mar 31, 2023 at 8:49 PM Zhu Lingshan <lingshan.zhu@intel.com> 
>> 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.
>> Is there any chance that ifcvf can use virtio_pci_modern_dev library?
>>
>> Then we don't need to duplicate the codes.
>>
>> Note that pds_vdpa will be the second user for virtio_pci_modern_dev
>> library (and the first vDPA parent to use that library).
> Yes I agree this library can help a lot for a standard virtio pci device.
> But this change would be huge, its like require to change every line of
> the driver. For example current driver functions work on the adapter and
> ifcvf_hw, if we wants to reuse the lib, we need the driver work on 
> struct virtio_pci_modern_device.
> Almost need to re-write the driver.
>
> Can we plan this huge change in following series?
ping
>
> Thanks,
> Zhu Lingshan
>>
>> Thanks
>>
>>> This series also implement irq synchronization in the reset
>>> routine
>>>
>>> Zhu Lingshan (5):
>>>    virt queue ops take immediate actions
>>>    get_driver_features from virito registers
>>>    retire ifcvf_start_datapath and ifcvf_add_status
>>>    synchronize irqs in the reset routine
>>>    a vendor driver should not set _CONFIG_S_FAILED
>>>
>>>   drivers/vdpa/ifcvf/ifcvf_base.c | 162 
>>> +++++++++++++++++++-------------
>>>   drivers/vdpa/ifcvf/ifcvf_base.h |  16 ++--
>>>   drivers/vdpa/ifcvf/ifcvf_main.c |  97 ++++---------------
>>>   3 files changed, 122 insertions(+), 153 deletions(-)
>>>
>>> -- 
>>> 2.39.1
>>>
>

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

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

[Jason Wang]

On Thu, Apr 20, 2023 at 5:17 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>
>
>
> On 4/3/2023 6:10 PM, Zhu, Lingshan wrote:
> >
> >
> > On 4/3/2023 1:28 PM, Jason Wang wrote:
> >> On Fri, Mar 31, 2023 at 8:49 PM Zhu Lingshan <lingshan.zhu@intel.com>
> >> 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.
> >> Is there any chance that ifcvf can use virtio_pci_modern_dev library?
> >>
> >> Then we don't need to duplicate the codes.
> >>
> >> Note that pds_vdpa will be the second user for virtio_pci_modern_dev
> >> library (and the first vDPA parent to use that library).
> > Yes I agree this library can help a lot for a standard virtio pci device.
> > But this change would be huge, its like require to change every line of
> > the driver. For example current driver functions work on the adapter and
> > ifcvf_hw, if we wants to reuse the lib, we need the driver work on
> > struct virtio_pci_modern_device.
> > Almost need to re-write the driver.
> >
> > Can we plan this huge change in following series?
> ping

Will go through this this week.

Thanks

> >
> > Thanks,
> > Zhu Lingshan
> >>
> >> Thanks
> >>
> >>> This series also implement irq synchronization in the reset
> >>> routine
> >>>
> >>> Zhu Lingshan (5):
> >>>    virt queue ops take immediate actions
> >>>    get_driver_features from virito registers
> >>>    retire ifcvf_start_datapath and ifcvf_add_status
> >>>    synchronize irqs in the reset routine
> >>>    a vendor driver should not set _CONFIG_S_FAILED
> >>>
> >>>   drivers/vdpa/ifcvf/ifcvf_base.c | 162
> >>> +++++++++++++++++++-------------
> >>>   drivers/vdpa/ifcvf/ifcvf_base.h |  16 ++--
> >>>   drivers/vdpa/ifcvf/ifcvf_main.c |  97 ++++---------------
> >>>   3 files changed, 122 insertions(+), 153 deletions(-)
> >>>
> >>> --
> >>> 2.39.1
> >>>
> >
>

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

Re: [PATCH 2/5] get_driver_features from virito registers

[Michael S. Tsirkin]

subj typo: virtio

On Sat, Apr 01, 2023 at 04:48:51AM +0800, Zhu Lingshan 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>
> ---
>  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 0/5] vDPA/ifcvf: implement immediate initialization mechanism

[Michael S. Tsirkin]

On Sat, Apr 01, 2023 at 04:48:49AM +0800, 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


Please, prefix each patch subject with vDPA/ifcvf:


> Zhu Lingshan (5):
>   virt queue ops take immediate actions
>   get_driver_features from virito registers
>   retire ifcvf_start_datapath and ifcvf_add_status
>   synchronize irqs in the reset routine
>   a vendor driver should not set _CONFIG_S_FAILED
> 
>  drivers/vdpa/ifcvf/ifcvf_base.c | 162 +++++++++++++++++++-------------
>  drivers/vdpa/ifcvf/ifcvf_base.h |  16 ++--
>  drivers/vdpa/ifcvf/ifcvf_main.c |  97 ++++---------------
>  3 files changed, 122 insertions(+), 153 deletions(-)
> 
> -- 
> 2.39.1

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

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

[Michael S. Tsirkin]

On Mon, Apr 24, 2023 at 11:50:20AM +0800, Jason Wang wrote:
> On Thu, Apr 20, 2023 at 5:17 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
> >
> >
> >
> > On 4/3/2023 6:10 PM, Zhu, Lingshan wrote:
> > >
> > >
> > > On 4/3/2023 1:28 PM, Jason Wang wrote:
> > >> On Fri, Mar 31, 2023 at 8:49 PM Zhu Lingshan <lingshan.zhu@intel.com>
> > >> 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.
> > >> Is there any chance that ifcvf can use virtio_pci_modern_dev library?
> > >>
> > >> Then we don't need to duplicate the codes.
> > >>
> > >> Note that pds_vdpa will be the second user for virtio_pci_modern_dev
> > >> library (and the first vDPA parent to use that library).
> > > Yes I agree this library can help a lot for a standard virtio pci device.
> > > But this change would be huge, its like require to change every line of
> > > the driver. For example current driver functions work on the adapter and
> > > ifcvf_hw, if we wants to reuse the lib, we need the driver work on
> > > struct virtio_pci_modern_device.
> > > Almost need to re-write the driver.
> > >
> > > Can we plan this huge change in following series?
> > ping
> 
> Will go through this this week.
> 
> Thanks

why do you expect it to go through, you didn't ack?

> > >
> > > Thanks,
> > > Zhu Lingshan
> > >>
> > >> Thanks
> > >>
> > >>> This series also implement irq synchronization in the reset
> > >>> routine
> > >>>
> > >>> Zhu Lingshan (5):
> > >>>    virt queue ops take immediate actions
> > >>>    get_driver_features from virito registers
> > >>>    retire ifcvf_start_datapath and ifcvf_add_status
> > >>>    synchronize irqs in the reset routine
> > >>>    a vendor driver should not set _CONFIG_S_FAILED
> > >>>
> > >>>   drivers/vdpa/ifcvf/ifcvf_base.c | 162
> > >>> +++++++++++++++++++-------------
> > >>>   drivers/vdpa/ifcvf/ifcvf_base.h |  16 ++--
> > >>>   drivers/vdpa/ifcvf/ifcvf_main.c |  97 ++++---------------
> > >>>   3 files changed, 122 insertions(+), 153 deletions(-)
> > >>>
> > >>> --
> > >>> 2.39.1
> > >>>
> > >
> >

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

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

[Jason Wang]

On Mon, Apr 24, 2023 at 12:53 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Apr 24, 2023 at 11:50:20AM +0800, Jason Wang wrote:
> > On Thu, Apr 20, 2023 at 5:17 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
> > >
> > >
> > >
> > > On 4/3/2023 6:10 PM, Zhu, Lingshan wrote:
> > > >
> > > >
> > > > On 4/3/2023 1:28 PM, Jason Wang wrote:
> > > >> On Fri, Mar 31, 2023 at 8:49 PM Zhu Lingshan <lingshan.zhu@intel.com>
> > > >> 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.
> > > >> Is there any chance that ifcvf can use virtio_pci_modern_dev library?
> > > >>
> > > >> Then we don't need to duplicate the codes.
> > > >>
> > > >> Note that pds_vdpa will be the second user for virtio_pci_modern_dev
> > > >> library (and the first vDPA parent to use that library).
> > > > Yes I agree this library can help a lot for a standard virtio pci device.
> > > > But this change would be huge, its like require to change every line of
> > > > the driver. For example current driver functions work on the adapter and
> > > > ifcvf_hw, if we wants to reuse the lib, we need the driver work on
> > > > struct virtio_pci_modern_device.
> > > > Almost need to re-write the driver.
> > > >
> > > > Can we plan this huge change in following series?
> > > ping
> >
> > Will go through this this week.
> >
> > Thanks
>
> why do you expect it to go through, you didn't ack?

I meant I will have a look at it this week,

(Google told me "go through" meant "to look at or examine something carefully")

Thanks

>
> > > >
> > > > Thanks,
> > > > Zhu Lingshan
> > > >>
> > > >> Thanks
> > > >>
> > > >>> This series also implement irq synchronization in the reset
> > > >>> routine
> > > >>>
> > > >>> Zhu Lingshan (5):
> > > >>>    virt queue ops take immediate actions
> > > >>>    get_driver_features from virito registers
> > > >>>    retire ifcvf_start_datapath and ifcvf_add_status
> > > >>>    synchronize irqs in the reset routine
> > > >>>    a vendor driver should not set _CONFIG_S_FAILED
> > > >>>
> > > >>>   drivers/vdpa/ifcvf/ifcvf_base.c | 162
> > > >>> +++++++++++++++++++-------------
> > > >>>   drivers/vdpa/ifcvf/ifcvf_base.h |  16 ++--
> > > >>>   drivers/vdpa/ifcvf/ifcvf_main.c |  97 ++++---------------
> > > >>>   3 files changed, 122 insertions(+), 153 deletions(-)
> > > >>>
> > > >>> --
> > > >>> 2.39.1
> > > >>>
> > > >
> > >
>

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

Re: [PATCH 2/5] get_driver_features from virito registers

[Zhu, Lingshan]

On 4/24/2023 12:50 PM, Michael S. Tsirkin wrote:
> subj typo: virtio
will fix in V2, thanks!
>
> On Sat, Apr 01, 2023 at 04:48:51AM +0800, Zhu Lingshan 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>
>> ---
>>   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 0/5] vDPA/ifcvf: implement immediate initialization mechanism

[Zhu, Lingshan]

On 4/24/2023 12:51 PM, Michael S. Tsirkin wrote:
> On Sat, Apr 01, 2023 at 04:48:49AM +0800, 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
>
> Please, prefix each patch subject with vDPA/ifcvf:
I will fix this in V2. Thanks
>
>
>> Zhu Lingshan (5):
>>    virt queue ops take immediate actions
>>    get_driver_features from virito registers
>>    retire ifcvf_start_datapath and ifcvf_add_status
>>    synchronize irqs in the reset routine
>>    a vendor driver should not set _CONFIG_S_FAILED
>>
>>   drivers/vdpa/ifcvf/ifcvf_base.c | 162 +++++++++++++++++++-------------
>>   drivers/vdpa/ifcvf/ifcvf_base.h |  16 ++--
>>   drivers/vdpa/ifcvf/ifcvf_main.c |  97 ++++---------------
>>   3 files changed, 122 insertions(+), 153 deletions(-)
>>
>> -- 
>> 2.39.1

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

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

[Michael S. Tsirkin]

On Mon, Apr 24, 2023 at 01:20:12PM +0800, Jason Wang wrote:
> On Mon, Apr 24, 2023 at 12:53 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Mon, Apr 24, 2023 at 11:50:20AM +0800, Jason Wang wrote:
> > > On Thu, Apr 20, 2023 at 5:17 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
> > > >
> > > >
> > > >
> > > > On 4/3/2023 6:10 PM, Zhu, Lingshan wrote:
> > > > >
> > > > >
> > > > > On 4/3/2023 1:28 PM, Jason Wang wrote:
> > > > >> On Fri, Mar 31, 2023 at 8:49 PM Zhu Lingshan <lingshan.zhu@intel.com>
> > > > >> 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.
> > > > >> Is there any chance that ifcvf can use virtio_pci_modern_dev library?
> > > > >>
> > > > >> Then we don't need to duplicate the codes.
> > > > >>
> > > > >> Note that pds_vdpa will be the second user for virtio_pci_modern_dev
> > > > >> library (and the first vDPA parent to use that library).
> > > > > Yes I agree this library can help a lot for a standard virtio pci device.
> > > > > But this change would be huge, its like require to change every line of
> > > > > the driver. For example current driver functions work on the adapter and
> > > > > ifcvf_hw, if we wants to reuse the lib, we need the driver work on
> > > > > struct virtio_pci_modern_device.
> > > > > Almost need to re-write the driver.
> > > > >
> > > > > Can we plan this huge change in following series?
> > > > ping
> > >
> > > Will go through this this week.
> > >
> > > Thanks
> >
> > why do you expect it to go through, you didn't ack?
> 
> I meant I will have a look at it this week,
> 
> (Google told me "go through" meant "to look at or examine something carefully")
> 
> Thanks

Oh, I misread what you wrote. Indeed. For some reason I read "this will
go through this week". That would have been different.


> >
> > > > >
> > > > > Thanks,
> > > > > Zhu Lingshan
> > > > >>
> > > > >> Thanks
> > > > >>
> > > > >>> This series also implement irq synchronization in the reset
> > > > >>> routine
> > > > >>>
> > > > >>> Zhu Lingshan (5):
> > > > >>>    virt queue ops take immediate actions
> > > > >>>    get_driver_features from virito registers
> > > > >>>    retire ifcvf_start_datapath and ifcvf_add_status
> > > > >>>    synchronize irqs in the reset routine
> > > > >>>    a vendor driver should not set _CONFIG_S_FAILED
> > > > >>>
> > > > >>>   drivers/vdpa/ifcvf/ifcvf_base.c | 162
> > > > >>> +++++++++++++++++++-------------
> > > > >>>   drivers/vdpa/ifcvf/ifcvf_base.h |  16 ++--
> > > > >>>   drivers/vdpa/ifcvf/ifcvf_main.c |  97 ++++---------------
> > > > >>>   3 files changed, 122 insertions(+), 153 deletions(-)
> > > > >>>
> > > > >>> --
> > > > >>> 2.39.1
> > > > >>>
> > > > >
> > > >
> >

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

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

[Jason Wang]

在 2023/4/1 04:48, 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);


This seems unrelated to the immediate actions?

The rest looks good.

Thanks


>   	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)

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

Re: [PATCH 2/5] get_driver_features from virito registers

[Jason Wang]

在 2023/4/1 04:48, 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;
> +}


This duplicates with the logic ifcvf_get_hw_features(), it would be 
simpler if we just do a rename.

Thanks


> +
>   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)

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

Re: [PATCH 3/5] retire ifcvf_start_datapath and ifcvf_add_status

[Jason Wang]

在 2023/4/1 04:48, 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>

Thanks


> ---
>   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);

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

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

[Jason Wang]

在 2023/4/1 04:48, 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 | 61 ++++++++++++++++++++++++++-------
>   drivers/vdpa/ifcvf/ifcvf_main.c | 45 +++---------------------
>   2 files changed, 54 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c
> index 79e313c5e10e..49949aec20ef 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_base.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
> @@ -170,12 +170,7 @@ 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);


If we don't flush or poll how can we know the reset is done?

E.g modern virtio-pci did:

         /* 0 status means a reset. */
         vp_modern_set_status(mdev, 0);
         /* After writing 0 to device_status, the driver MUST wait for a 
read of
          * device_status to return 0 before reinitializing the device.
          * This will flush out the status write, and flush in device 
writes,
          * including MSI-X interrupts, if any.
          */
         while (vp_modern_get_status(mdev))
                 msleep(1);
         /* Flush pending VQ/configuration callbacks. */
        vp_synchronize_vectors(vdev);

>   }
>   
>   u64 ifcvf_get_hw_features(struct ifcvf_hw *hw)
> @@ -368,20 +363,62 @@ 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 synchronize_per_vq_irq(struct ifcvf_hw *hw)
>   {
> -	u32 i;
> +	u16 qid;
>   
> -	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);
> +	for (qid = 0; qid < hw->nr_vring; qid++) {
> +		if (hw->vring[qid].irq != -EINVAL)
> +			synchronize_irq(hw->vring[qid].irq);
>   	}
>   }
>   
> +static void synchronize_vqs_reused_irq(struct ifcvf_hw *hw)
> +{
> +	if (hw->vqs_reused_irq != -EINVAL)
> +		synchronize_irq(hw->vqs_reused_irq);
> +}
> +
> +static void synchronize_vq_irq(struct ifcvf_hw *hw)
> +{
> +	u8 status = hw->msix_vector_status;
> +
> +	if (status == MSIX_VECTOR_PER_VQ_AND_CONFIG)
> +		synchronize_per_vq_irq(hw);
> +	else
> +		synchronize_vqs_reused_irq(hw);
> +}


I wonder if we need to go with such complicated ways,can we synchronize 
through the vectors like virtio-pci did?

         for (i = 0; i < vp_dev->msix_vectors; ++i)
                 synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i));
?


> +
> +static void synchronize_config_irq(struct ifcvf_hw *hw)
> +{
> +	if (hw->config_irq != -EINVAL)
> +		synchronize_irq(hw->config_irq);
> +}
> +
> +static void ifcvf_reset_vring(struct ifcvf_hw *hw)
> +{
> +	u16 qid;
> +
> +	for (qid = 0; qid < hw->nr_vring; qid++) {
> +		synchronize_vq_irq(hw);

Since IRQ could be shared, this will result extra complexity, like a irq 
could be flushed multiple times?

Thanks


> +		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)
> +{
> +	synchronize_config_irq(hw);
> +	hw->config_cb.callback = NULL;
> +	hw->config_cb.private = NULL;
> +	ifcvf_set_config_vector(hw, VIRTIO_MSI_NO_VECTOR);
> +}
> +
>   void ifcvf_stop_hw(struct ifcvf_hw *hw)
>   {
> -	ifcvf_hw_disable(hw);
> -	ifcvf_reset(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_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
> index 968687159e44..15c6157ee841 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
> @@ -346,33 +346,6 @@ static int ifcvf_request_irq(struct ifcvf_hw *vf)
>   	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);
> -
> -	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 +435,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;
>   }

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

Re: [PATCH 5/5] a vendor driver should not set _CONFIG_S_FAILED

[Jason Wang]

在 2023/4/1 04:48, 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>

Thanks


> ---
>   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 15c6157ee841..f228fba74c61 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
> @@ -423,9 +423,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;
>   		}
>   	}

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

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

[Zhu, Lingshan]

On 4/26/2023 11:39 AM, Jason Wang wrote:
>
> 在 2023/4/1 04:48, 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);
>
>
> This seems unrelated to the immediate actions?
This is because we must not reset the device after vq settings already
take affections, e.g., we must not reset the hw after set_vq_address or
set_vq_ready.

Thanks
>
> The rest looks good.
>
> Thanks
>
>
>> 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)
>

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

Re: [PATCH 2/5] get_driver_features from virito registers

[Zhu, Lingshan]

On 4/26/2023 12:02 PM, Jason Wang wrote:
>
> 在 2023/4/1 04:48, 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;
>> +}
>
>
> This duplicates with the logic ifcvf_get_hw_features(), it would be 
> simpler if we just do a rename.
Yes, they look very similar. ifcvf_get_hw_features() reads 
virtio_pci_common_cfg.device_feature
and ifcvf_get_driver_features reads virtio_pci_common_cfg.driver_feature.

Do you suggest we merge these two functions? something like this may 
look chaotic:

u64 ifcvf_get_features(struct ifcvf_hw *hw, bool device_feature)
{
     struct virtio_pci_common_cfg __iomem *cfg = hw->common_cfg;
     u32 features_lo, features_hi;
     u64 features;

     if (device_feature) {
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);
     } else {
         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;
}

Maybe separate functions looks better.
>
> Thanks
>
>
>> +
>>   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)
>

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

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

[Zhu, Lingshan]

On 4/26/2023 1:06 PM, Jason Wang wrote:
>
> 在 2023/4/1 04:48, 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 | 61 ++++++++++++++++++++++++++-------
>>   drivers/vdpa/ifcvf/ifcvf_main.c | 45 +++---------------------
>>   2 files changed, 54 insertions(+), 52 deletions(-)
>>
>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c 
>> b/drivers/vdpa/ifcvf/ifcvf_base.c
>> index 79e313c5e10e..49949aec20ef 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_base.c
>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
>> @@ -170,12 +170,7 @@ 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);
>
>
> If we don't flush or poll how can we know the reset is done?
>
> E.g modern virtio-pci did:
>
>         /* 0 status means a reset. */
>         vp_modern_set_status(mdev, 0);
>         /* After writing 0 to device_status, the driver MUST wait for 
> a read of
>          * device_status to return 0 before reinitializing the device.
>          * This will flush out the status write, and flush in device 
> writes,
>          * including MSI-X interrupts, if any.
>          */
>         while (vp_modern_get_status(mdev))
>                 msleep(1);
>         /* Flush pending VQ/configuration callbacks. */
>        vp_synchronize_vectors(vdev);
Thanks, I can implement a similar get_status() here.
>
>>   }
>>     u64 ifcvf_get_hw_features(struct ifcvf_hw *hw)
>> @@ -368,20 +363,62 @@ 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 synchronize_per_vq_irq(struct ifcvf_hw *hw)
>>   {
>> -    u32 i;
>> +    u16 qid;
>>   -    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);
>> +    for (qid = 0; qid < hw->nr_vring; qid++) {
>> +        if (hw->vring[qid].irq != -EINVAL)
>> +            synchronize_irq(hw->vring[qid].irq);
>>       }
>>   }
>>   +static void synchronize_vqs_reused_irq(struct ifcvf_hw *hw)
>> +{
>> +    if (hw->vqs_reused_irq != -EINVAL)
>> +        synchronize_irq(hw->vqs_reused_irq);
>> +}
>> +
>> +static void synchronize_vq_irq(struct ifcvf_hw *hw)
>> +{
>> +    u8 status = hw->msix_vector_status;
>> +
>> +    if (status == MSIX_VECTOR_PER_VQ_AND_CONFIG)
>> +        synchronize_per_vq_irq(hw);
>> +    else
>> +        synchronize_vqs_reused_irq(hw);
>> +}
>
>
> I wonder if we need to go with such complicated ways,can we 
> synchronize through the vectors like virtio-pci did?
>
>         for (i = 0; i < vp_dev->msix_vectors; ++i)
> synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i));
> ?
I can record the number of msix_vectors and sycn irq based on it in V2.
>
>
>> +
>> +static void synchronize_config_irq(struct ifcvf_hw *hw)
>> +{
>> +    if (hw->config_irq != -EINVAL)
>> +        synchronize_irq(hw->config_irq);
>> +}
>> +
>> +static void ifcvf_reset_vring(struct ifcvf_hw *hw)
>> +{
>> +    u16 qid;
>> +
>> +    for (qid = 0; qid < hw->nr_vring; qid++) {
>> +        synchronize_vq_irq(hw);
>
> Since IRQ could be shared, this will result extra complexity, like a 
> irq could be flushed multiple times?
No for this code path, E.g., if the all vqs share one irq, it will only 
be flushed once in synchronize_vqs_reused_irq()

Thanks
>
> Thanks
>
>
>> + 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)
>> +{
>> +    synchronize_config_irq(hw);
>> +    hw->config_cb.callback = NULL;
>> +    hw->config_cb.private = NULL;
>> +    ifcvf_set_config_vector(hw, VIRTIO_MSI_NO_VECTOR);
>> +}
>> +
>>   void ifcvf_stop_hw(struct ifcvf_hw *hw)
>>   {
>> -    ifcvf_hw_disable(hw);
>> -    ifcvf_reset(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_main.c 
>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>> index 968687159e44..15c6157ee841 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>> @@ -346,33 +346,6 @@ static int ifcvf_request_irq(struct ifcvf_hw *vf)
>>       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);
>> -
>> -    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 +435,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;
>>   }
>

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