Re: [PATCH V2] ifcvf: move IRQ request/free to status change handlers
On 2020/5/19 上午9:51, Cindy Lu wrote: Hi ,Jason It works ok in the latest version of qemu vdpa code , So I think the patch is ok. Thanks Cindy Thanks for the testing, (btw, we'd better not do top posting when discuss in the community). So, Acked-by: Jason Wang On Wed, May 13, 2020 at 3:18 PM Jason Wang wrote: On 2020/5/13 下午12:42, Zhu, Lingshan wrote: On 5/13/2020 12:12 PM, Jason Wang wrote: On 2020/5/12 下午4:00, Zhu Lingshan wrote: This commit move IRQ request and free operations from probe() to VIRTIO status change handler to comply with VIRTIO spec. VIRTIO spec 1.1, section 2.1.2 Device Requirements: Device Status Field The device MUST NOT consume buffers or send any used buffer notifications to the driver before DRIVER_OK. This comment needs to be checked as I said previously. It's only needed if we're sure ifcvf can generate interrupt before DRIVER_OK. Signed-off-by: Zhu Lingshan --- changes from V1: remove ifcvf_stop_datapath() in status == 0 handler, we don't need to do this twice; handle status == 0 after DRIVER_OK -> !DRIVER_OK handler (Jason Wang) Patch looks good to me, but with this patch ping cannot work on my machine. (It works without this patch). Thanks This is strange, it works on my machines, let's have a check offline. Thanks, BR Zhu Lingshan I give it a try with virito-vpda and a tiny userspace. Either works. So it could be an issue of qemu codes. Let's wait for Cindy to test if it really works. Thanks
Re: [PATCH V2] ifcvf: move IRQ request/free to status change handlers
Hi ,Jason It works ok in the latest version of qemu vdpa code , So I think the patch is ok. Thanks Cindy On Wed, May 13, 2020 at 3:18 PM Jason Wang wrote: > > > On 2020/5/13 下午12:42, Zhu, Lingshan wrote: > > > > > > On 5/13/2020 12:12 PM, Jason Wang wrote: > >> > >> On 2020/5/12 下午4:00, Zhu Lingshan wrote: > >>> This commit move IRQ request and free operations from probe() > >>> to VIRTIO status change handler to comply with VIRTIO spec. > >>> > >>> VIRTIO spec 1.1, section 2.1.2 Device Requirements: Device Status Field > >>> The device MUST NOT consume buffers or send any used buffer > >>> notifications to the driver before DRIVER_OK. > >> > >> > >> This comment needs to be checked as I said previously. It's only > >> needed if we're sure ifcvf can generate interrupt before DRIVER_OK. > >> > >> > >>> > >>> Signed-off-by: Zhu Lingshan > >>> --- > >>> changes from V1: > >>> remove ifcvf_stop_datapath() in status == 0 handler, we don't need > >>> to do this > >>> twice; handle status == 0 after DRIVER_OK -> !DRIVER_OK handler > >>> (Jason Wang) > >> > >> > >> Patch looks good to me, but with this patch ping cannot work on my > >> machine. (It works without this patch). > >> > >> Thanks > > This is strange, it works on my machines, let's have a check offline. > > > > Thanks, > > BR > > Zhu Lingshan > > > I give it a try with virito-vpda and a tiny userspace. Either works. > > So it could be an issue of qemu codes. > > Let's wait for Cindy to test if it really works. > > Thanks > >
Re: [PATCH V2] ifcvf: move IRQ request/free to status change handlers
On 2020/5/13 下午12:42, Zhu, Lingshan wrote: On 5/13/2020 12:12 PM, Jason Wang wrote: On 2020/5/12 下午4:00, Zhu Lingshan wrote: This commit move IRQ request and free operations from probe() to VIRTIO status change handler to comply with VIRTIO spec. VIRTIO spec 1.1, section 2.1.2 Device Requirements: Device Status Field The device MUST NOT consume buffers or send any used buffer notifications to the driver before DRIVER_OK. This comment needs to be checked as I said previously. It's only needed if we're sure ifcvf can generate interrupt before DRIVER_OK. Signed-off-by: Zhu Lingshan --- changes from V1: remove ifcvf_stop_datapath() in status == 0 handler, we don't need to do this twice; handle status == 0 after DRIVER_OK -> !DRIVER_OK handler (Jason Wang) Patch looks good to me, but with this patch ping cannot work on my machine. (It works without this patch). Thanks This is strange, it works on my machines, let's have a check offline. Thanks, BR Zhu Lingshan I give it a try with virito-vpda and a tiny userspace. Either works. So it could be an issue of qemu codes. Let's wait for Cindy to test if it really works. Thanks
Re: [PATCH V2] ifcvf: move IRQ request/free to status change handlers
On 2020/5/13 下午12:42, Zhu, Lingshan wrote: On 5/13/2020 12:12 PM, Jason Wang wrote: On 2020/5/12 下午4:00, Zhu Lingshan wrote: This commit move IRQ request and free operations from probe() to VIRTIO status change handler to comply with VIRTIO spec. VIRTIO spec 1.1, section 2.1.2 Device Requirements: Device Status Field The device MUST NOT consume buffers or send any used buffer notifications to the driver before DRIVER_OK. This comment needs to be checked as I said previously. It's only needed if we're sure ifcvf can generate interrupt before DRIVER_OK. Signed-off-by: Zhu Lingshan --- changes from V1: remove ifcvf_stop_datapath() in status == 0 handler, we don't need to do this twice; handle status == 0 after DRIVER_OK -> !DRIVER_OK handler (Jason Wang) Patch looks good to me, but with this patch ping cannot work on my machine. (It works without this patch). Thanks This is strange, it works on my machines, let's have a check offline. Thanks, BR Zhu Lingshan Note that I tested the patch with vhost-vpda. Thanks.
Re: [PATCH V2] ifcvf: move IRQ request/free to status change handlers
On 2020/5/12 下午4:00, Zhu Lingshan wrote:
This commit move IRQ request and free operations from probe()
to VIRTIO status change handler to comply with VIRTIO spec.
VIRTIO spec 1.1, section 2.1.2 Device Requirements: Device Status Field
The device MUST NOT consume buffers or send any used buffer
notifications to the driver before DRIVER_OK.
This comment needs to be checked as I said previously. It's only needed
if we're sure ifcvf can generate interrupt before DRIVER_OK.
Signed-off-by: Zhu Lingshan
---
changes from V1:
remove ifcvf_stop_datapath() in status == 0 handler, we don't need to do this
twice; handle status == 0 after DRIVER_OK -> !DRIVER_OK handler (Jason Wang)
Patch looks good to me, but with this patch ping cannot work on my
machine. (It works without this patch).
Thanks
drivers/vdpa/ifcvf/ifcvf_main.c | 120
1 file changed, 73 insertions(+), 47 deletions(-)
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
index abf6a061..d529ed6 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -28,6 +28,60 @@ static irqreturn_t ifcvf_intr_handler(int irq, void *arg)
return IRQ_HANDLED;
}
+static void ifcvf_free_irq_vectors(void *data)
+{
+ pci_free_irq_vectors(data);
+}
+
+static void ifcvf_free_irq(struct ifcvf_adapter *adapter, int queues)
+{
+ struct pci_dev *pdev = adapter->pdev;
+ struct ifcvf_hw *vf = >vf;
+ int i;
+
+
+ for (i = 0; i < queues; i++)
+ devm_free_irq(>dev, vf->vring[i].irq, >vring[i]);
+
+ ifcvf_free_irq_vectors(pdev);
+}
+
+static int ifcvf_request_irq(struct ifcvf_adapter *adapter)
+{
+ struct pci_dev *pdev = adapter->pdev;
+ struct ifcvf_hw *vf = >vf;
+ int vector, i, ret, irq;
+
+ ret = pci_alloc_irq_vectors(pdev, IFCVF_MAX_INTR,
+ IFCVF_MAX_INTR, PCI_IRQ_MSIX);
+ if (ret < 0) {
+ IFCVF_ERR(pdev, "Failed to alloc IRQ vectors\n");
+ return ret;
+ }
+
+ for (i = 0; i < IFCVF_MAX_QUEUE_PAIRS * 2; i++) {
+ snprintf(vf->vring[i].msix_name, 256, "ifcvf[%s]-%d\n",
+pci_name(pdev), i);
+ vector = i + IFCVF_MSI_QUEUE_OFF;
+ irq = pci_irq_vector(pdev, vector);
+ ret = devm_request_irq(>dev, irq,
+ ifcvf_intr_handler, 0,
+ vf->vring[i].msix_name,
+ >vring[i]);
+ if (ret) {
+ IFCVF_ERR(pdev,
+ "Failed to request irq for vq %d\n", i);
+ ifcvf_free_irq(adapter, i);
+
+ return ret;
+ }
+
+ vf->vring[i].irq = irq;
+ }
+
+ return 0;
+}
+
static int ifcvf_start_datapath(void *private)
{
struct ifcvf_hw *vf = ifcvf_private_to_vf(private);
@@ -118,17 +172,34 @@ static void ifcvf_vdpa_set_status(struct vdpa_device
*vdpa_dev, u8 status)
{
struct ifcvf_adapter *adapter;
struct ifcvf_hw *vf;
+ u8 status_old;
+ int ret;
vf = vdpa_to_vf(vdpa_dev);
adapter = dev_get_drvdata(vdpa_dev->dev.parent);
+ status_old = ifcvf_get_status(vf);
- if (status == 0) {
+ if ((status_old & VIRTIO_CONFIG_S_DRIVER_OK) &&
+ !(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
ifcvf_stop_datapath(adapter);
+ ifcvf_free_irq(adapter, IFCVF_MAX_QUEUE_PAIRS * 2);
+ }
+
+ if (status == 0) {
ifcvf_reset_vring(adapter);
return;
}
- if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
+ if ((status & VIRTIO_CONFIG_S_DRIVER_OK) &&
+ !(status_old & VIRTIO_CONFIG_S_DRIVER_OK)) {
+ ret = ifcvf_request_irq(adapter);
+ if (ret) {
+ status = ifcvf_get_status(vf);
+ status |= VIRTIO_CONFIG_S_FAILED;
+ ifcvf_set_status(vf, status);
+ return;
+ }
+
if (ifcvf_start_datapath(adapter) < 0)
IFCVF_ERR(adapter->pdev,
"Failed to set ifcvf vdpa status %u\n",
@@ -284,38 +355,6 @@ static void ifcvf_vdpa_set_config_cb(struct vdpa_device
*vdpa_dev,
.set_config_cb = ifcvf_vdpa_set_config_cb,
};
-static int ifcvf_request_irq(struct ifcvf_adapter *adapter)
-{
- struct pci_dev *pdev = adapter->pdev;
- struct ifcvf_hw *vf = >vf;
- int vector, i, ret, irq;
-
-
- for (i = 0; i < IFCVF_MAX_QUEUE_PAIRS * 2; i++) {
- snprintf(vf->vring[i].msix_name, 256, "ifcvf[%s]-%d\n",
-pci_name(pdev), i);
- vector = i + IFCVF_MSI_QUEUE_OFF;
- irq = pci_irq_vector(pdev, vector);
-
[PATCH V2] ifcvf: move IRQ request/free to status change handlers
This commit move IRQ request and free operations from probe()
to VIRTIO status change handler to comply with VIRTIO spec.
VIRTIO spec 1.1, section 2.1.2 Device Requirements: Device Status Field
The device MUST NOT consume buffers or send any used buffer
notifications to the driver before DRIVER_OK.
Signed-off-by: Zhu Lingshan
---
changes from V1:
remove ifcvf_stop_datapath() in status == 0 handler, we don't need to do this
twice; handle status == 0 after DRIVER_OK -> !DRIVER_OK handler (Jason Wang)
drivers/vdpa/ifcvf/ifcvf_main.c | 120
1 file changed, 73 insertions(+), 47 deletions(-)
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
index abf6a061..d529ed6 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -28,6 +28,60 @@ static irqreturn_t ifcvf_intr_handler(int irq, void *arg)
return IRQ_HANDLED;
}
+static void ifcvf_free_irq_vectors(void *data)
+{
+ pci_free_irq_vectors(data);
+}
+
+static void ifcvf_free_irq(struct ifcvf_adapter *adapter, int queues)
+{
+ struct pci_dev *pdev = adapter->pdev;
+ struct ifcvf_hw *vf = >vf;
+ int i;
+
+
+ for (i = 0; i < queues; i++)
+ devm_free_irq(>dev, vf->vring[i].irq, >vring[i]);
+
+ ifcvf_free_irq_vectors(pdev);
+}
+
+static int ifcvf_request_irq(struct ifcvf_adapter *adapter)
+{
+ struct pci_dev *pdev = adapter->pdev;
+ struct ifcvf_hw *vf = >vf;
+ int vector, i, ret, irq;
+
+ ret = pci_alloc_irq_vectors(pdev, IFCVF_MAX_INTR,
+ IFCVF_MAX_INTR, PCI_IRQ_MSIX);
+ if (ret < 0) {
+ IFCVF_ERR(pdev, "Failed to alloc IRQ vectors\n");
+ return ret;
+ }
+
+ for (i = 0; i < IFCVF_MAX_QUEUE_PAIRS * 2; i++) {
+ snprintf(vf->vring[i].msix_name, 256, "ifcvf[%s]-%d\n",
+pci_name(pdev), i);
+ vector = i + IFCVF_MSI_QUEUE_OFF;
+ irq = pci_irq_vector(pdev, vector);
+ ret = devm_request_irq(>dev, irq,
+ ifcvf_intr_handler, 0,
+ vf->vring[i].msix_name,
+ >vring[i]);
+ if (ret) {
+ IFCVF_ERR(pdev,
+ "Failed to request irq for vq %d\n", i);
+ ifcvf_free_irq(adapter, i);
+
+ return ret;
+ }
+
+ vf->vring[i].irq = irq;
+ }
+
+ return 0;
+}
+
static int ifcvf_start_datapath(void *private)
{
struct ifcvf_hw *vf = ifcvf_private_to_vf(private);
@@ -118,17 +172,34 @@ static void ifcvf_vdpa_set_status(struct vdpa_device
*vdpa_dev, u8 status)
{
struct ifcvf_adapter *adapter;
struct ifcvf_hw *vf;
+ u8 status_old;
+ int ret;
vf = vdpa_to_vf(vdpa_dev);
adapter = dev_get_drvdata(vdpa_dev->dev.parent);
+ status_old = ifcvf_get_status(vf);
- if (status == 0) {
+ if ((status_old & VIRTIO_CONFIG_S_DRIVER_OK) &&
+ !(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
ifcvf_stop_datapath(adapter);
+ ifcvf_free_irq(adapter, IFCVF_MAX_QUEUE_PAIRS * 2);
+ }
+
+ if (status == 0) {
ifcvf_reset_vring(adapter);
return;
}
- if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
+ if ((status & VIRTIO_CONFIG_S_DRIVER_OK) &&
+ !(status_old & VIRTIO_CONFIG_S_DRIVER_OK)) {
+ ret = ifcvf_request_irq(adapter);
+ if (ret) {
+ status = ifcvf_get_status(vf);
+ status |= VIRTIO_CONFIG_S_FAILED;
+ ifcvf_set_status(vf, status);
+ return;
+ }
+
if (ifcvf_start_datapath(adapter) < 0)
IFCVF_ERR(adapter->pdev,
"Failed to set ifcvf vdpa status %u\n",
@@ -284,38 +355,6 @@ static void ifcvf_vdpa_set_config_cb(struct vdpa_device
*vdpa_dev,
.set_config_cb = ifcvf_vdpa_set_config_cb,
};
-static int ifcvf_request_irq(struct ifcvf_adapter *adapter)
-{
- struct pci_dev *pdev = adapter->pdev;
- struct ifcvf_hw *vf = >vf;
- int vector, i, ret, irq;
-
-
- for (i = 0; i < IFCVF_MAX_QUEUE_PAIRS * 2; i++) {
- snprintf(vf->vring[i].msix_name, 256, "ifcvf[%s]-%d\n",
-pci_name(pdev), i);
- vector = i + IFCVF_MSI_QUEUE_OFF;
- irq = pci_irq_vector(pdev, vector);
- ret = devm_request_irq(>dev, irq,
- ifcvf_intr_handler, 0,
- vf->vring[i].msix_name,
- >vring[i]);
- if (ret) {
- IFCVF_ERR(pdev,
-
