* [PATCH 1/5] virt queue ops take immediate actions
2023-03-31 20:48 [PATCH 0/5] vDPA/ifcvf: implement immediate initialization mechanism Zhu Lingshan
@ 2023-03-31 20:48 ` Zhu Lingshan
2023-04-26 3:39 ` Jason Wang
2023-03-31 20:48 ` [PATCH 2/5] get_driver_features from virito registers Zhu Lingshan
` (5 subsequent siblings)
6 siblings, 1 reply; 25+ messages in thread
From: Zhu Lingshan @ 2023-03-31 20:48 UTC (permalink / raw)
To: mst, jasowang; +Cc: virtualization
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
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH 1/5] virt queue ops take immediate actions
2023-03-31 20:48 ` [PATCH 1/5] virt queue ops take immediate actions Zhu Lingshan
@ 2023-04-26 3:39 ` Jason Wang
2023-04-27 8:02 ` Zhu, Lingshan
0 siblings, 1 reply; 25+ messages in thread
From: Jason Wang @ 2023-04-26 3:39 UTC (permalink / raw)
To: Zhu Lingshan, mst; +Cc: virtualization
在 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
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 1/5] virt queue ops take immediate actions
2023-04-26 3:39 ` Jason Wang
@ 2023-04-27 8:02 ` Zhu, Lingshan
0 siblings, 0 replies; 25+ messages in thread
From: Zhu, Lingshan @ 2023-04-27 8:02 UTC (permalink / raw)
To: Jason Wang, mst; +Cc: virtualization
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 2/5] get_driver_features from virito registers
2023-03-31 20:48 [PATCH 0/5] vDPA/ifcvf: implement immediate initialization mechanism Zhu Lingshan
2023-03-31 20:48 ` [PATCH 1/5] virt queue ops take immediate actions Zhu Lingshan
@ 2023-03-31 20:48 ` Zhu Lingshan
2023-04-24 4:50 ` Michael S. Tsirkin
2023-04-26 4:02 ` Jason Wang
2023-03-31 20:48 ` [PATCH 3/5] retire ifcvf_start_datapath and ifcvf_add_status Zhu Lingshan
` (4 subsequent siblings)
6 siblings, 2 replies; 25+ messages in thread
From: Zhu Lingshan @ 2023-03-31 20:48 UTC (permalink / raw)
To: mst, jasowang; +Cc: virtualization
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
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH 2/5] get_driver_features from virito registers
2023-03-31 20:48 ` [PATCH 2/5] get_driver_features from virito registers Zhu Lingshan
@ 2023-04-24 4:50 ` Michael S. Tsirkin
2023-04-24 7:24 ` Zhu, Lingshan
2023-04-26 4:02 ` Jason Wang
1 sibling, 1 reply; 25+ messages in thread
From: Michael S. Tsirkin @ 2023-04-24 4:50 UTC (permalink / raw)
To: Zhu Lingshan; +Cc: virtualization
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
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 2/5] get_driver_features from virito registers
2023-04-24 4:50 ` Michael S. Tsirkin
@ 2023-04-24 7:24 ` Zhu, Lingshan
0 siblings, 0 replies; 25+ messages in thread
From: Zhu, Lingshan @ 2023-04-24 7:24 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: virtualization
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/5] get_driver_features from virito registers
2023-03-31 20:48 ` [PATCH 2/5] get_driver_features from virito registers Zhu Lingshan
2023-04-24 4:50 ` Michael S. Tsirkin
@ 2023-04-26 4:02 ` Jason Wang
2023-04-27 8:28 ` Zhu, Lingshan
1 sibling, 1 reply; 25+ messages in thread
From: Jason Wang @ 2023-04-26 4:02 UTC (permalink / raw)
To: Zhu Lingshan, mst; +Cc: virtualization
在 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
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 2/5] get_driver_features from virito registers
2023-04-26 4:02 ` Jason Wang
@ 2023-04-27 8:28 ` Zhu, Lingshan
0 siblings, 0 replies; 25+ messages in thread
From: Zhu, Lingshan @ 2023-04-27 8:28 UTC (permalink / raw)
To: Jason Wang, mst; +Cc: virtualization
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 3/5] retire ifcvf_start_datapath and ifcvf_add_status
2023-03-31 20:48 [PATCH 0/5] vDPA/ifcvf: implement immediate initialization mechanism Zhu Lingshan
2023-03-31 20:48 ` [PATCH 1/5] virt queue ops take immediate actions Zhu Lingshan
2023-03-31 20:48 ` [PATCH 2/5] get_driver_features from virito registers Zhu Lingshan
@ 2023-03-31 20:48 ` Zhu Lingshan
2023-04-26 4:04 ` Jason Wang
2023-03-31 20:48 ` [PATCH 4/5] synchronize irqs in the reset routine Zhu Lingshan
` (3 subsequent siblings)
6 siblings, 1 reply; 25+ messages in thread
From: Zhu Lingshan @ 2023-03-31 20:48 UTC (permalink / raw)
To: mst, jasowang; +Cc: virtualization
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
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH 3/5] retire ifcvf_start_datapath and ifcvf_add_status
2023-03-31 20:48 ` [PATCH 3/5] retire ifcvf_start_datapath and ifcvf_add_status Zhu Lingshan
@ 2023-04-26 4:04 ` Jason Wang
0 siblings, 0 replies; 25+ messages in thread
From: Jason Wang @ 2023-04-26 4:04 UTC (permalink / raw)
To: Zhu Lingshan, mst; +Cc: virtualization
在 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
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 4/5] synchronize irqs in the reset routine
2023-03-31 20:48 [PATCH 0/5] vDPA/ifcvf: implement immediate initialization mechanism Zhu Lingshan
` (2 preceding siblings ...)
2023-03-31 20:48 ` [PATCH 3/5] retire ifcvf_start_datapath and ifcvf_add_status Zhu Lingshan
@ 2023-03-31 20:48 ` Zhu Lingshan
2023-04-26 5:06 ` Jason Wang
2023-03-31 20:48 ` [PATCH 5/5] a vendor driver should not set _CONFIG_S_FAILED Zhu Lingshan
` (2 subsequent siblings)
6 siblings, 1 reply; 25+ messages in thread
From: Zhu Lingshan @ 2023-03-31 20:48 UTC (permalink / raw)
To: mst, jasowang; +Cc: virtualization
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
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH 4/5] synchronize irqs in the reset routine
2023-03-31 20:48 ` [PATCH 4/5] synchronize irqs in the reset routine Zhu Lingshan
@ 2023-04-26 5:06 ` Jason Wang
2023-04-27 9:07 ` Zhu, Lingshan
0 siblings, 1 reply; 25+ messages in thread
From: Jason Wang @ 2023-04-26 5:06 UTC (permalink / raw)
To: Zhu Lingshan, mst; +Cc: virtualization
在 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
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 4/5] synchronize irqs in the reset routine
2023-04-26 5:06 ` Jason Wang
@ 2023-04-27 9:07 ` Zhu, Lingshan
0 siblings, 0 replies; 25+ messages in thread
From: Zhu, Lingshan @ 2023-04-27 9:07 UTC (permalink / raw)
To: Jason Wang, mst; +Cc: virtualization
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 5/5] a vendor driver should not set _CONFIG_S_FAILED
2023-03-31 20:48 [PATCH 0/5] vDPA/ifcvf: implement immediate initialization mechanism Zhu Lingshan
` (3 preceding siblings ...)
2023-03-31 20:48 ` [PATCH 4/5] synchronize irqs in the reset routine Zhu Lingshan
@ 2023-03-31 20:48 ` Zhu Lingshan
2023-04-26 5:10 ` Jason Wang
2023-04-03 5:28 ` [PATCH 0/5] vDPA/ifcvf: implement immediate initialization mechanism Jason Wang
2023-04-24 4:51 ` Michael S. Tsirkin
6 siblings, 1 reply; 25+ messages in thread
From: Zhu Lingshan @ 2023-03-31 20:48 UTC (permalink / raw)
To: mst, jasowang; +Cc: virtualization
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
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH 5/5] a vendor driver should not set _CONFIG_S_FAILED
2023-03-31 20:48 ` [PATCH 5/5] a vendor driver should not set _CONFIG_S_FAILED Zhu Lingshan
@ 2023-04-26 5:10 ` Jason Wang
0 siblings, 0 replies; 25+ messages in thread
From: Jason Wang @ 2023-04-26 5:10 UTC (permalink / raw)
To: Zhu Lingshan, mst; +Cc: virtualization
在 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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/5] vDPA/ifcvf: implement immediate initialization mechanism
2023-03-31 20:48 [PATCH 0/5] vDPA/ifcvf: implement immediate initialization mechanism Zhu Lingshan
` (4 preceding siblings ...)
2023-03-31 20:48 ` [PATCH 5/5] a vendor driver should not set _CONFIG_S_FAILED Zhu Lingshan
@ 2023-04-03 5:28 ` Jason Wang
2023-04-03 10:10 ` Zhu, Lingshan
2023-04-24 4:51 ` Michael S. Tsirkin
6 siblings, 1 reply; 25+ messages in thread
From: Jason Wang @ 2023-04-03 5:28 UTC (permalink / raw)
To: Zhu Lingshan; +Cc: virtualization, mst
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
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 0/5] vDPA/ifcvf: implement immediate initialization mechanism
2023-04-03 5:28 ` [PATCH 0/5] vDPA/ifcvf: implement immediate initialization mechanism Jason Wang
@ 2023-04-03 10:10 ` Zhu, Lingshan
2023-04-20 9:17 ` Zhu, Lingshan
0 siblings, 1 reply; 25+ messages in thread
From: Zhu, Lingshan @ 2023-04-03 10:10 UTC (permalink / raw)
To: Jason Wang; +Cc: virtualization, mst
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/5] vDPA/ifcvf: implement immediate initialization mechanism
2023-04-03 10:10 ` Zhu, Lingshan
@ 2023-04-20 9:17 ` Zhu, Lingshan
2023-04-24 3:50 ` Jason Wang
0 siblings, 1 reply; 25+ messages in thread
From: Zhu, Lingshan @ 2023-04-20 9:17 UTC (permalink / raw)
To: Jason Wang, Michael S. Tsirkin; +Cc: virtualization
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/5] vDPA/ifcvf: implement immediate initialization mechanism
2023-04-20 9:17 ` Zhu, Lingshan
@ 2023-04-24 3:50 ` Jason Wang
2023-04-24 4:52 ` Michael S. Tsirkin
0 siblings, 1 reply; 25+ messages in thread
From: Jason Wang @ 2023-04-24 3:50 UTC (permalink / raw)
To: Zhu, Lingshan; +Cc: virtualization, Michael S. Tsirkin
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/5] vDPA/ifcvf: implement immediate initialization mechanism
2023-04-24 3:50 ` Jason Wang
@ 2023-04-24 4:52 ` Michael S. Tsirkin
2023-04-24 5:20 ` Jason Wang
0 siblings, 1 reply; 25+ messages in thread
From: Michael S. Tsirkin @ 2023-04-24 4:52 UTC (permalink / raw)
To: Jason Wang; +Cc: virtualization
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/5] vDPA/ifcvf: implement immediate initialization mechanism
2023-04-24 4:52 ` Michael S. Tsirkin
@ 2023-04-24 5:20 ` Jason Wang
2023-04-24 9:53 ` Michael S. Tsirkin
0 siblings, 1 reply; 25+ messages in thread
From: Jason Wang @ 2023-04-24 5:20 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: virtualization
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/5] vDPA/ifcvf: implement immediate initialization mechanism
2023-04-24 5:20 ` Jason Wang
@ 2023-04-24 9:53 ` Michael S. Tsirkin
0 siblings, 0 replies; 25+ messages in thread
From: Michael S. Tsirkin @ 2023-04-24 9:53 UTC (permalink / raw)
To: Jason Wang; +Cc: virtualization
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/5] vDPA/ifcvf: implement immediate initialization mechanism
2023-03-31 20:48 [PATCH 0/5] vDPA/ifcvf: implement immediate initialization mechanism Zhu Lingshan
` (5 preceding siblings ...)
2023-04-03 5:28 ` [PATCH 0/5] vDPA/ifcvf: implement immediate initialization mechanism Jason Wang
@ 2023-04-24 4:51 ` Michael S. Tsirkin
2023-04-24 7:25 ` Zhu, Lingshan
6 siblings, 1 reply; 25+ messages in thread
From: Michael S. Tsirkin @ 2023-04-24 4:51 UTC (permalink / raw)
To: Zhu Lingshan; +Cc: virtualization
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
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 0/5] vDPA/ifcvf: implement immediate initialization mechanism
2023-04-24 4:51 ` Michael S. Tsirkin
@ 2023-04-24 7:25 ` Zhu, Lingshan
0 siblings, 0 replies; 25+ messages in thread
From: Zhu, Lingshan @ 2023-04-24 7:25 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: virtualization
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
^ permalink raw reply [flat|nested] 25+ messages in thread