* [PATCH] ifcvf: move IRQ request/free to status change handlers
@ 2020-05-11 7:19 Zhu Lingshan
2020-05-11 9:26 ` Jason Wang
0 siblings, 1 reply; 7+ messages in thread
From: Zhu Lingshan @ 2020-05-11 7:19 UTC (permalink / raw)
To: mst, kvm, virtualization, linux-kernel, netdev, jasowang
Cc: lulu, dan.daly, cunming.liang, Zhu Lingshan
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 <lingshan.zhu@intel.com>
---
drivers/vdpa/ifcvf/ifcvf_main.c | 119 ++++++++++++++++++++++++----------------
1 file changed, 73 insertions(+), 46 deletions(-)
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
index abf6a061..4d58bf2 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 = &adapter->vf;
+ int i;
+
+
+ for (i = 0; i < queues; i++)
+ devm_free_irq(&pdev->dev, vf->vring[i].irq, &vf->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 = &adapter->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(&pdev->dev, irq,
+ ifcvf_intr_handler, 0,
+ vf->vring[i].msix_name,
+ &vf->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,9 +172,12 @@ static void ifcvf_vdpa_set_status(struct vdpa_device *vdpa_dev, u8 status)
{
struct ifcvf_adapter *adapter;
struct ifcvf_hw *vf;
+ u8 status_old;
+ int ret;
vf = vdpa_to_vf(vdpa_dev);
adapter = dev_get_drvdata(vdpa_dev->dev.parent);
+ status_old = ifcvf_get_status(vf);
if (status == 0) {
ifcvf_stop_datapath(adapter);
@@ -128,7 +185,22 @@ static void ifcvf_vdpa_set_status(struct vdpa_device *vdpa_dev, u8 status)
return;
}
- if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
+ if ((status_old & VIRTIO_CONFIG_S_DRIVER_OK) &&
+ !(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
+ ifcvf_stop_datapath(adapter);
+ ifcvf_free_irq(adapter, IFCVF_MAX_QUEUE_PAIRS * 2);
+ }
+
+ if ((status & VIRTIO_CONFIG_S_DRIVER_OK) &&
+ !(status_old & VIRTIO_CONFIG_S_DRIVER_OK)) {
+ ret = ifcvf_request_irq(adapter);
+ if (ret) {
+ status = ifcvf_get_status(vf);
+ status |= VIRTIO_CONFIG_S_FAILED;
+ ifcvf_set_status(vf, status);
+ return;
+ }
+
if (ifcvf_start_datapath(adapter) < 0)
IFCVF_ERR(adapter->pdev,
"Failed to set ifcvf vdpa status %u\n",
@@ -284,38 +356,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 = &adapter->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(&pdev->dev, irq,
- ifcvf_intr_handler, 0,
- vf->vring[i].msix_name,
- &vf->vring[i]);
- if (ret) {
- IFCVF_ERR(pdev,
- "Failed to request irq for vq %d\n", i);
- return ret;
- }
- vf->vring[i].irq = irq;
- }
-
- return 0;
-}
-
-static void ifcvf_free_irq_vectors(void *data)
-{
- pci_free_irq_vectors(data);
-}
-
static int ifcvf_probe(struct pci_dev *pdev, const struct pci_device_id *id)
{
struct device *dev = &pdev->dev;
@@ -349,13 +389,6 @@ static int ifcvf_probe(struct pci_dev *pdev, const struct pci_device_id *id)
return ret;
}
- 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;
- }
-
ret = devm_add_action_or_reset(dev, ifcvf_free_irq_vectors, pdev);
if (ret) {
IFCVF_ERR(pdev,
@@ -379,12 +412,6 @@ static int ifcvf_probe(struct pci_dev *pdev, const struct pci_device_id *id)
adapter->pdev = pdev;
adapter->vdpa.dma_dev = &pdev->dev;
- ret = ifcvf_request_irq(adapter);
- if (ret) {
- IFCVF_ERR(pdev, "Failed to request MSI-X irq\n");
- goto err;
- }
-
ret = ifcvf_init_hw(vf, pdev);
if (ret) {
IFCVF_ERR(pdev, "Failed to init IFCVF hw\n");
--
1.8.3.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] ifcvf: move IRQ request/free to status change handlers
2020-05-11 7:19 [PATCH] ifcvf: move IRQ request/free to status change handlers Zhu Lingshan
@ 2020-05-11 9:26 ` Jason Wang
2020-05-11 10:18 ` Francesco Lavra
[not found] ` <0f822630-14ad-e0cd-4171-6213c30f0799@intel.com>
0 siblings, 2 replies; 7+ messages in thread
From: Jason Wang @ 2020-05-11 9:26 UTC (permalink / raw)
To: Zhu Lingshan, mst, kvm, virtualization, linux-kernel, netdev
Cc: lulu, dan.daly, cunming.liang
On 2020/5/11 下午3:19, 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.
My previous explanation might be wrong here. It depends on how you
implement your hardware, if you hardware guarantee that no interrupt
will be triggered before DRIVER_OK, then it's fine.
And the main goal for this patch is to allocate the interrupt on demand.
>
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
> drivers/vdpa/ifcvf/ifcvf_main.c | 119 ++++++++++++++++++++++++----------------
> 1 file changed, 73 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
> index abf6a061..4d58bf2 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 = &adapter->vf;
> + int i;
> +
> +
> + for (i = 0; i < queues; i++)
> + devm_free_irq(&pdev->dev, vf->vring[i].irq, &vf->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 = &adapter->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(&pdev->dev, irq,
> + ifcvf_intr_handler, 0,
> + vf->vring[i].msix_name,
> + &vf->vring[i]);
> + if (ret) {
> + IFCVF_ERR(pdev,
> + "Failed to request irq for vq %d\n", i);
> + ifcvf_free_irq(adapter, i);
I'm not sure this unwind is correct. It looks like we should loop and
call devm_free_irq() for virtqueue [0, 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,9 +172,12 @@ static void ifcvf_vdpa_set_status(struct vdpa_device *vdpa_dev, u8 status)
> {
> struct ifcvf_adapter *adapter;
> struct ifcvf_hw *vf;
> + u8 status_old;
> + int ret;
>
> vf = vdpa_to_vf(vdpa_dev);
> adapter = dev_get_drvdata(vdpa_dev->dev.parent);
> + status_old = ifcvf_get_status(vf);
>
> if (status == 0) {
> ifcvf_stop_datapath(adapter);
> @@ -128,7 +185,22 @@ static void ifcvf_vdpa_set_status(struct vdpa_device *vdpa_dev, u8 status)
> return;
> }
>
> - if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
> + if ((status_old & VIRTIO_CONFIG_S_DRIVER_OK) &&
> + !(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
> + ifcvf_stop_datapath(adapter);
> + ifcvf_free_irq(adapter, IFCVF_MAX_QUEUE_PAIRS * 2);
> + }
> +
> + if ((status & VIRTIO_CONFIG_S_DRIVER_OK) &&
> + !(status_old & VIRTIO_CONFIG_S_DRIVER_OK)) {
> + ret = ifcvf_request_irq(adapter);
> + if (ret) {
> + status = ifcvf_get_status(vf);
> + status |= VIRTIO_CONFIG_S_FAILED;
> + ifcvf_set_status(vf, status);
> + return;
> + }
> +
Have a hard though on the logic here.
This depends on the status setting from guest or userspace. Which means
it can not deal with e.g when qemu or userspace is crashed? Do we need
to care this or it's a over engineering?
Thanks
> if (ifcvf_start_datapath(adapter) < 0)
> IFCVF_ERR(adapter->pdev,
> "Failed to set ifcvf vdpa status %u\n",
> @@ -284,38 +356,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 = &adapter->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(&pdev->dev, irq,
> - ifcvf_intr_handler, 0,
> - vf->vring[i].msix_name,
> - &vf->vring[i]);
> - if (ret) {
> - IFCVF_ERR(pdev,
> - "Failed to request irq for vq %d\n", i);
> - return ret;
> - }
> - vf->vring[i].irq = irq;
> - }
> -
> - return 0;
> -}
> -
> -static void ifcvf_free_irq_vectors(void *data)
> -{
> - pci_free_irq_vectors(data);
> -}
> -
> static int ifcvf_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> {
> struct device *dev = &pdev->dev;
> @@ -349,13 +389,6 @@ static int ifcvf_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> return ret;
> }
>
> - 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;
> - }
> -
> ret = devm_add_action_or_reset(dev, ifcvf_free_irq_vectors, pdev);
> if (ret) {
> IFCVF_ERR(pdev,
> @@ -379,12 +412,6 @@ static int ifcvf_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> adapter->pdev = pdev;
> adapter->vdpa.dma_dev = &pdev->dev;
>
> - ret = ifcvf_request_irq(adapter);
> - if (ret) {
> - IFCVF_ERR(pdev, "Failed to request MSI-X irq\n");
> - goto err;
> - }
> -
> ret = ifcvf_init_hw(vf, pdev);
> if (ret) {
> IFCVF_ERR(pdev, "Failed to init IFCVF hw\n");
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ifcvf: move IRQ request/free to status change handlers
2020-05-11 9:26 ` Jason Wang
@ 2020-05-11 10:18 ` Francesco Lavra
2020-05-12 3:41 ` Jason Wang
[not found] ` <0f822630-14ad-e0cd-4171-6213c30f0799@intel.com>
1 sibling, 1 reply; 7+ messages in thread
From: Francesco Lavra @ 2020-05-11 10:18 UTC (permalink / raw)
To: Jason Wang, Zhu Lingshan, mst, kvm, virtualization, linux-kernel, netdev
Cc: lulu, dan.daly, cunming.liang
On 5/11/20 11:26 AM, Jason Wang wrote:
>
> On 2020/5/11 下午3:19, 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.
>
>
> My previous explanation might be wrong here. It depends on how you
> implement your hardware, if you hardware guarantee that no interrupt
> will be triggered before DRIVER_OK, then it's fine.
>
> And the main goal for this patch is to allocate the interrupt on demand.
>
>
>>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>> ---
>> drivers/vdpa/ifcvf/ifcvf_main.c | 119
>> ++++++++++++++++++++++++----------------
>> 1 file changed, 73 insertions(+), 46 deletions(-)
>>
>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c
>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>> index abf6a061..4d58bf2 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 = &adapter->vf;
>> + int i;
>> +
>> +
>> + for (i = 0; i < queues; i++)
>> + devm_free_irq(&pdev->dev, vf->vring[i].irq, &vf->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 = &adapter->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(&pdev->dev, irq,
>> + ifcvf_intr_handler, 0,
>> + vf->vring[i].msix_name,
>> + &vf->vring[i]);
>> + if (ret) {
>> + IFCVF_ERR(pdev,
>> + "Failed to request irq for vq %d\n", i);
>> + ifcvf_free_irq(adapter, i);
>
>
> I'm not sure this unwind is correct. It looks like we should loop and
> call devm_free_irq() for virtqueue [0, i);
That's exactly what the code does: ifcvf_free_irq() contains a (i = 0; i
< queues; i++) loop, and here the function is called with the `queues`
argument set to `i`.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ifcvf: move IRQ request/free to status change handlers
[not found] ` <0f822630-14ad-e0cd-4171-6213c30f0799@intel.com>
@ 2020-05-12 3:38 ` Jason Wang
0 siblings, 0 replies; 7+ messages in thread
From: Jason Wang @ 2020-05-12 3:38 UTC (permalink / raw)
To: Zhu, Lingshan, mst, kvm, virtualization, linux-kernel, netdev
Cc: lulu, dan.daly, cunming.liang
On 2020/5/11 下午6:11, Zhu, Lingshan wrote:
>
>
> On 5/11/2020 5:26 PM, Jason Wang wrote:
>>
>> On 2020/5/11 下午3:19, 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.
>>
>>
>> My previous explanation might be wrong here. It depends on how you
>> implement your hardware, if you hardware guarantee that no interrupt
>> will be triggered before DRIVER_OK, then it's fine.
>>
>> And the main goal for this patch is to allocate the interrupt on demand.
> Hi Jason,
>
> So these code can a double assurance.
>>
>>
>>>
>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>> ---
>>> drivers/vdpa/ifcvf/ifcvf_main.c | 119
>>> ++++++++++++++++++++++++----------------
>>> 1 file changed, 73 insertions(+), 46 deletions(-)
>>>
>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c
>>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>>> index abf6a061..4d58bf2 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 = &adapter->vf;
>>> + int i;
>>> +
>>> +
>>> + for (i = 0; i < queues; i++)
>>> + devm_free_irq(&pdev->dev, vf->vring[i].irq, &vf->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 = &adapter->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(&pdev->dev, irq,
>>> + ifcvf_intr_handler, 0,
>>> + vf->vring[i].msix_name,
>>> + &vf->vring[i]);
>>> + if (ret) {
>>> + IFCVF_ERR(pdev,
>>> + "Failed to request irq for vq %d\n", i);
>>> + ifcvf_free_irq(adapter, i);
>>
>>
>> I'm not sure this unwind is correct. It looks like we should loop and
>> call devm_free_irq() for virtqueue [0, i);
> we have the loop in ifcvf_free_irq(struct ifcvf_adapter *adapter, int queues),
> it takes a parameter queues, and a loop
>
> + for (i = 0; i < queues; i++)+ devm_free_irq(&pdev->dev,
> vf->vring[i].irq, &vf->vring[i]); will free irq for vq[0,queues)
Aha, I get this.
>>
>>
>>
>>> +
>>> + 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,9 +172,12 @@ static void ifcvf_vdpa_set_status(struct
>>> vdpa_device *vdpa_dev, u8 status)
>>> {
>>> struct ifcvf_adapter *adapter;
>>> struct ifcvf_hw *vf;
>>> + u8 status_old;
>>> + int ret;
>>> vf = vdpa_to_vf(vdpa_dev);
>>> adapter = dev_get_drvdata(vdpa_dev->dev.parent);
>>> + status_old = ifcvf_get_status(vf);
>>> if (status == 0) {
>>> ifcvf_stop_datapath(adapter);
>>> @@ -128,7 +185,22 @@ static void ifcvf_vdpa_set_status(struct
>>> vdpa_device *vdpa_dev, u8 status)
>>> return;
>>> }
>>> - if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
>>> + if ((status_old & VIRTIO_CONFIG_S_DRIVER_OK) &&
>>> + !(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
>>> + ifcvf_stop_datapath(adapter);
>>> + ifcvf_free_irq(adapter, IFCVF_MAX_QUEUE_PAIRS * 2);
>>> + }
>>> +
>>> + if ((status & VIRTIO_CONFIG_S_DRIVER_OK) &&
>>> + !(status_old & VIRTIO_CONFIG_S_DRIVER_OK)) {
>>> + ret = ifcvf_request_irq(adapter);
>>> + if (ret) {
>>> + status = ifcvf_get_status(vf);
>>> + status |= VIRTIO_CONFIG_S_FAILED;
>>> + ifcvf_set_status(vf, status);
>>> + return;
>>> + }
>>> +
>>
>>
>> Have a hard though on the logic here.
>>
>> This depends on the status setting from guest or userspace. Which
>> means it can not deal with e.g when qemu or userspace is crashed? Do
>> we need to care this or it's a over engineering?
>>
>> Thanks
> If qemu crash, I guess users may re-run qmeu / re-initialize the device, according to the spec, there should be a reset routine.
> This code piece handles status change on DRIVER_OK flipping. I am not sure I get your point, mind to give more hints?
The problem is if we don't launch new qemu instance, the interrupt will
be still there?
Thanks
>
> Thanks,
> BR
> Zhu Lingshan
>
>>
>>
>>> if (ifcvf_start_datapath(adapter) < 0)
>>> IFCVF_ERR(adapter->pdev,
>>> "Failed to set ifcvf vdpa status %u\n",
>>> @@ -284,38 +356,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 = &adapter->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(&pdev->dev, irq,
>>> - ifcvf_intr_handler, 0,
>>> - vf->vring[i].msix_name,
>>> - &vf->vring[i]);
>>> - if (ret) {
>>> - IFCVF_ERR(pdev,
>>> - "Failed to request irq for vq %d\n", i);
>>> - return ret;
>>> - }
>>> - vf->vring[i].irq = irq;
>>> - }
>>> -
>>> - return 0;
>>> -}
>>> -
>>> -static void ifcvf_free_irq_vectors(void *data)
>>> -{
>>> - pci_free_irq_vectors(data);
>>> -}
>>> -
>>> static int ifcvf_probe(struct pci_dev *pdev, const struct
>>> pci_device_id *id)
>>> {
>>> struct device *dev = &pdev->dev;
>>> @@ -349,13 +389,6 @@ static int ifcvf_probe(struct pci_dev *pdev,
>>> const struct pci_device_id *id)
>>> return ret;
>>> }
>>> - 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;
>>> - }
>>> -
>>> ret = devm_add_action_or_reset(dev, ifcvf_free_irq_vectors,
>>> pdev);
>>> if (ret) {
>>> IFCVF_ERR(pdev,
>>> @@ -379,12 +412,6 @@ static int ifcvf_probe(struct pci_dev *pdev,
>>> const struct pci_device_id *id)
>>> adapter->pdev = pdev;
>>> adapter->vdpa.dma_dev = &pdev->dev;
>>> - ret = ifcvf_request_irq(adapter);
>>> - if (ret) {
>>> - IFCVF_ERR(pdev, "Failed to request MSI-X irq\n");
>>> - goto err;
>>> - }
>>> -
>>> ret = ifcvf_init_hw(vf, pdev);
>>> if (ret) {
>>> IFCVF_ERR(pdev, "Failed to init IFCVF hw\n");
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ifcvf: move IRQ request/free to status change handlers
@ 2020-05-12 3:38 ` Jason Wang
0 siblings, 0 replies; 7+ messages in thread
From: Jason Wang @ 2020-05-12 3:38 UTC (permalink / raw)
To: Zhu, Lingshan, mst, kvm, virtualization, linux-kernel, netdev; +Cc: lulu
On 2020/5/11 下午6:11, Zhu, Lingshan wrote:
>
>
> On 5/11/2020 5:26 PM, Jason Wang wrote:
>>
>> On 2020/5/11 下午3:19, 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.
>>
>>
>> My previous explanation might be wrong here. It depends on how you
>> implement your hardware, if you hardware guarantee that no interrupt
>> will be triggered before DRIVER_OK, then it's fine.
>>
>> And the main goal for this patch is to allocate the interrupt on demand.
> Hi Jason,
>
> So these code can a double assurance.
>>
>>
>>>
>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>> ---
>>> drivers/vdpa/ifcvf/ifcvf_main.c | 119
>>> ++++++++++++++++++++++++----------------
>>> 1 file changed, 73 insertions(+), 46 deletions(-)
>>>
>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c
>>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>>> index abf6a061..4d58bf2 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 = &adapter->vf;
>>> + int i;
>>> +
>>> +
>>> + for (i = 0; i < queues; i++)
>>> + devm_free_irq(&pdev->dev, vf->vring[i].irq, &vf->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 = &adapter->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(&pdev->dev, irq,
>>> + ifcvf_intr_handler, 0,
>>> + vf->vring[i].msix_name,
>>> + &vf->vring[i]);
>>> + if (ret) {
>>> + IFCVF_ERR(pdev,
>>> + "Failed to request irq for vq %d\n", i);
>>> + ifcvf_free_irq(adapter, i);
>>
>>
>> I'm not sure this unwind is correct. It looks like we should loop and
>> call devm_free_irq() for virtqueue [0, i);
> we have the loop in ifcvf_free_irq(struct ifcvf_adapter *adapter, int queues),
> it takes a parameter queues, and a loop
>
> + for (i = 0; i < queues; i++)+ devm_free_irq(&pdev->dev,
> vf->vring[i].irq, &vf->vring[i]); will free irq for vq[0,queues)
Aha, I get this.
>>
>>
>>
>>> +
>>> + 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,9 +172,12 @@ static void ifcvf_vdpa_set_status(struct
>>> vdpa_device *vdpa_dev, u8 status)
>>> {
>>> struct ifcvf_adapter *adapter;
>>> struct ifcvf_hw *vf;
>>> + u8 status_old;
>>> + int ret;
>>> vf = vdpa_to_vf(vdpa_dev);
>>> adapter = dev_get_drvdata(vdpa_dev->dev.parent);
>>> + status_old = ifcvf_get_status(vf);
>>> if (status == 0) {
>>> ifcvf_stop_datapath(adapter);
>>> @@ -128,7 +185,22 @@ static void ifcvf_vdpa_set_status(struct
>>> vdpa_device *vdpa_dev, u8 status)
>>> return;
>>> }
>>> - if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
>>> + if ((status_old & VIRTIO_CONFIG_S_DRIVER_OK) &&
>>> + !(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
>>> + ifcvf_stop_datapath(adapter);
>>> + ifcvf_free_irq(adapter, IFCVF_MAX_QUEUE_PAIRS * 2);
>>> + }
>>> +
>>> + if ((status & VIRTIO_CONFIG_S_DRIVER_OK) &&
>>> + !(status_old & VIRTIO_CONFIG_S_DRIVER_OK)) {
>>> + ret = ifcvf_request_irq(adapter);
>>> + if (ret) {
>>> + status = ifcvf_get_status(vf);
>>> + status |= VIRTIO_CONFIG_S_FAILED;
>>> + ifcvf_set_status(vf, status);
>>> + return;
>>> + }
>>> +
>>
>>
>> Have a hard though on the logic here.
>>
>> This depends on the status setting from guest or userspace. Which
>> means it can not deal with e.g when qemu or userspace is crashed? Do
>> we need to care this or it's a over engineering?
>>
>> Thanks
> If qemu crash, I guess users may re-run qmeu / re-initialize the device, according to the spec, there should be a reset routine.
> This code piece handles status change on DRIVER_OK flipping. I am not sure I get your point, mind to give more hints?
The problem is if we don't launch new qemu instance, the interrupt will
be still there?
Thanks
>
> Thanks,
> BR
> Zhu Lingshan
>
>>
>>
>>> if (ifcvf_start_datapath(adapter) < 0)
>>> IFCVF_ERR(adapter->pdev,
>>> "Failed to set ifcvf vdpa status %u\n",
>>> @@ -284,38 +356,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 = &adapter->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(&pdev->dev, irq,
>>> - ifcvf_intr_handler, 0,
>>> - vf->vring[i].msix_name,
>>> - &vf->vring[i]);
>>> - if (ret) {
>>> - IFCVF_ERR(pdev,
>>> - "Failed to request irq for vq %d\n", i);
>>> - return ret;
>>> - }
>>> - vf->vring[i].irq = irq;
>>> - }
>>> -
>>> - return 0;
>>> -}
>>> -
>>> -static void ifcvf_free_irq_vectors(void *data)
>>> -{
>>> - pci_free_irq_vectors(data);
>>> -}
>>> -
>>> static int ifcvf_probe(struct pci_dev *pdev, const struct
>>> pci_device_id *id)
>>> {
>>> struct device *dev = &pdev->dev;
>>> @@ -349,13 +389,6 @@ static int ifcvf_probe(struct pci_dev *pdev,
>>> const struct pci_device_id *id)
>>> return ret;
>>> }
>>> - 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;
>>> - }
>>> -
>>> ret = devm_add_action_or_reset(dev, ifcvf_free_irq_vectors,
>>> pdev);
>>> if (ret) {
>>> IFCVF_ERR(pdev,
>>> @@ -379,12 +412,6 @@ static int ifcvf_probe(struct pci_dev *pdev,
>>> const struct pci_device_id *id)
>>> adapter->pdev = pdev;
>>> adapter->vdpa.dma_dev = &pdev->dev;
>>> - ret = ifcvf_request_irq(adapter);
>>> - if (ret) {
>>> - IFCVF_ERR(pdev, "Failed to request MSI-X irq\n");
>>> - goto err;
>>> - }
>>> -
>>> ret = ifcvf_init_hw(vf, pdev);
>>> if (ret) {
>>> IFCVF_ERR(pdev, "Failed to init IFCVF hw\n");
>>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ifcvf: move IRQ request/free to status change handlers
2020-05-11 10:18 ` Francesco Lavra
@ 2020-05-12 3:41 ` Jason Wang
0 siblings, 0 replies; 7+ messages in thread
From: Jason Wang @ 2020-05-12 3:41 UTC (permalink / raw)
To: Francesco Lavra, Zhu Lingshan, mst, kvm, virtualization,
linux-kernel, netdev
Cc: lulu, dan.daly, cunming.liang
On 2020/5/11 下午6:18, Francesco Lavra wrote:
> On 5/11/20 11:26 AM, Jason Wang wrote:
>>
>> On 2020/5/11 下午3:19, 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.
>>
>>
>> My previous explanation might be wrong here. It depends on how you
>> implement your hardware, if you hardware guarantee that no interrupt
>> will be triggered before DRIVER_OK, then it's fine.
>>
>> And the main goal for this patch is to allocate the interrupt on demand.
>>
>>
>>>
>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>> ---
>>> drivers/vdpa/ifcvf/ifcvf_main.c | 119
>>> ++++++++++++++++++++++++----------------
>>> 1 file changed, 73 insertions(+), 46 deletions(-)
>>>
>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c
>>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>>> index abf6a061..4d58bf2 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 = &adapter->vf;
>>> + int i;
>>> +
>>> +
>>> + for (i = 0; i < queues; i++)
>>> + devm_free_irq(&pdev->dev, vf->vring[i].irq, &vf->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 = &adapter->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(&pdev->dev, irq,
>>> + ifcvf_intr_handler, 0,
>>> + vf->vring[i].msix_name,
>>> + &vf->vring[i]);
>>> + if (ret) {
>>> + IFCVF_ERR(pdev,
>>> + "Failed to request irq for vq %d\n", i);
>>> + ifcvf_free_irq(adapter, i);
>>
>>
>> I'm not sure this unwind is correct. It looks like we should loop and
>> call devm_free_irq() for virtqueue [0, i);
>
> That's exactly what the code does: ifcvf_free_irq() contains a (i = 0;
> i < queues; i++) loop, and here the function is called with the
> `queues` argument set to `i`.
>
Oh right.
Thanks
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ifcvf: move IRQ request/free to status change handlers
2020-05-12 3:38 ` Jason Wang
(?)
@ 2020-05-12 5:51 ` Jason Wang
-1 siblings, 0 replies; 7+ messages in thread
From: Jason Wang @ 2020-05-12 5:51 UTC (permalink / raw)
To: Zhu, Lingshan, mst, kvm, virtualization, linux-kernel, netdev
Cc: lulu, dan.daly, cunming.liang
On 2020/5/12 上午11:38, Jason Wang wrote:
>>>>
>>>> static int ifcvf_start_datapath(void *private)
>>>> {
>>>> struct ifcvf_hw *vf = ifcvf_private_to_vf(private);
>>>> @@ -118,9 +172,12 @@ static void ifcvf_vdpa_set_status(struct
>>>> vdpa_device *vdpa_dev, u8 status)
>>>> {
>>>> struct ifcvf_adapter *adapter;
>>>> struct ifcvf_hw *vf;
>>>> + u8 status_old;
>>>> + int ret;
>>>> vf = vdpa_to_vf(vdpa_dev);
>>>> adapter = dev_get_drvdata(vdpa_dev->dev.parent);
>>>> + status_old = ifcvf_get_status(vf);
>>>> if (status == 0) {
>>>> ifcvf_stop_datapath(adapter);
>>>> @@ -128,7 +185,22 @@ static void ifcvf_vdpa_set_status(struct
>>>> vdpa_device *vdpa_dev, u8 status)
>>>> return;
>>>> }
>>>> - if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
>>>> + if ((status_old & VIRTIO_CONFIG_S_DRIVER_OK) &&
>>>> + !(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
>>>> + ifcvf_stop_datapath(adapter);
>>>> + ifcvf_free_irq(adapter, IFCVF_MAX_QUEUE_PAIRS * 2);
>>>> + }
>>>> +
>>>> + if ((status & VIRTIO_CONFIG_S_DRIVER_OK) &&
>>>> + !(status_old & VIRTIO_CONFIG_S_DRIVER_OK)) {
>>>> + ret = ifcvf_request_irq(adapter);
>>>> + if (ret) {
>>>> + status = ifcvf_get_status(vf);
>>>> + status |= VIRTIO_CONFIG_S_FAILED;
>>>> + ifcvf_set_status(vf, status);
>>>> + return;
>>>> + }
>>>> +
>>>
>>>
>>> Have a hard though on the logic here.
>>>
>>> This depends on the status setting from guest or userspace. Which
>>> means it can not deal with e.g when qemu or userspace is crashed? Do
>>> we need to care this or it's a over engineering?
>>>
>>> Thanks
>> If qemu crash, I guess users may re-run qmeu / re-initialize the
>> device, according to the spec, there should be a reset routine.
>> This code piece handles status change on DRIVER_OK flipping. I am not
>> sure I get your point, mind to give more hints?
>
>
> The problem is if we don't launch new qemu instance, the interrupt
> will be still there?
Ok, we reset on vhost_vdpa_release() so the following is suspicious:
With the patch, we do:
static void ifcvf_vdpa_set_status(struct vdpa_device *vdpa_dev, u8 status)
{
struct ifcvf_adapter *adapter;
struct ifcvf_hw *vf;
u8 status_old;
int ret;
vf = vdpa_to_vf(vdpa_dev);
adapter = dev_get_drvdata(vdpa_dev->dev.parent);
status_old = ifcvf_get_status(vf);
if (status == 0) {
ifcvf_stop_datapath(adapter);
ifcvf_reset_vring(adapter);
return;
}
if ((status_old & VIRTIO_CONFIG_S_DRIVER_OK) &&
!(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
ifcvf_stop_datapath(adapter);
ifcvf_free_irq(adapter, IFCVF_MAX_QUEUE_PAIRS * 2);
}
...
So the handling of status == 0 looks wrong.
The OK -> !OK check should already cover the datapath stopping and irq
stuffs.
We only need to deal with vring reset and only need to do it after we
stop the datapath/irq stuffs.
Thanks
>
> Thanks
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-05-12 5:51 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-11 7:19 [PATCH] ifcvf: move IRQ request/free to status change handlers Zhu Lingshan
2020-05-11 9:26 ` Jason Wang
2020-05-11 10:18 ` Francesco Lavra
2020-05-12 3:41 ` Jason Wang
[not found] ` <0f822630-14ad-e0cd-4171-6213c30f0799@intel.com>
2020-05-12 3:38 ` Jason Wang
2020-05-12 3:38 ` Jason Wang
2020-05-12 5:51 ` Jason Wang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.