[dpdk-dev] [PATCH] net/virtio: fix Virtio-PCI ops assignment

[dpdk-dev] [PATCH] net/virtio: fix Virtio-PCI ops assignment

[Maxime Coquelin]

VIRTIO_OPS() macro relies on the port ID stored in the
virtio_hw struct. Issue is that it is used before being
assigned at init time. It results in all devices setting
ops on port ID 0, causing crash later when calling ops
for port IDs other than 0.

This patch ensure port ID assignment is done before being
used.

Bugzilla ID: 631
Fixes: 512e27eeb743 ("net/virtio: move PCI specific dev init to PCI ethdev init")

Reported-by: Wei Ling <weix.ling@intel.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/net/virtio/virtio_pci_ethdev.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/virtio/virtio_pci_ethdev.c b/drivers/net/virtio/virtio_pci_ethdev.c
index 1b818c4565..547cb8ffa0 100644
--- a/drivers/net/virtio/virtio_pci_ethdev.c
+++ b/drivers/net/virtio/virtio_pci_ethdev.c
@@ -76,6 +76,9 @@ eth_virtio_pci_init(struct rte_eth_dev *eth_dev)
 	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(eth_dev);
 	int ret;
 
+	/* Required to assign Virtio ops */
+	hw->port_id = eth_dev->data->port_id;
+
 	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
 		ret = vtpci_init(RTE_ETH_DEV_TO_PCI(eth_dev), dev);
 		if (ret) {
-- 
2.29.2

Re: [dpdk-dev] [PATCH] net/virtio: fix Virtio-PCI ops assignment

[David Marchand]

On Mon, Feb 1, 2021 at 4:16 PM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
>
> VIRTIO_OPS() macro relies on the port ID stored in the
> virtio_hw struct. Issue is that it is used before being
> assigned at init time. It results in all devices setting
> ops on port ID 0, causing crash later when calling ops
> for port IDs other than 0.
>
> This patch ensure port ID assignment is done before being
> used.
>
> Bugzilla ID: 631
> Fixes: 512e27eeb743 ("net/virtio: move PCI specific dev init to PCI ethdev init")
>
> Reported-by: Wei Ling <weix.ling@intel.com>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  drivers/net/virtio/virtio_pci_ethdev.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/net/virtio/virtio_pci_ethdev.c b/drivers/net/virtio/virtio_pci_ethdev.c
> index 1b818c4565..547cb8ffa0 100644
> --- a/drivers/net/virtio/virtio_pci_ethdev.c
> +++ b/drivers/net/virtio/virtio_pci_ethdev.c
> @@ -76,6 +76,9 @@ eth_virtio_pci_init(struct rte_eth_dev *eth_dev)
>         struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(eth_dev);
>         int ret;
>
> +       /* Required to assign Virtio ops */
> +       hw->port_id = eth_dev->data->port_id;
> +
>         if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
>                 ret = vtpci_init(RTE_ETH_DEV_TO_PCI(eth_dev), dev);
>                 if (ret) {

We probably don't need to set hw->port-id in eth_virtio_dev_init anymore.
And I would only set it in the primary process context, letting the
secondary pick what has been chosen by the primary?


-- 
David Marchand

Re: [dpdk-dev] [PATCH] net/virtio: fix Virtio-PCI ops assignment

[Maxime Coquelin]

On 2/1/21 5:32 PM, David Marchand wrote:
> On Mon, Feb 1, 2021 at 4:16 PM Maxime Coquelin
> <maxime.coquelin@redhat.com> wrote:
>>
>> VIRTIO_OPS() macro relies on the port ID stored in the
>> virtio_hw struct. Issue is that it is used before being
>> assigned at init time. It results in all devices setting
>> ops on port ID 0, causing crash later when calling ops
>> for port IDs other than 0.
>>
>> This patch ensure port ID assignment is done before being
>> used.
>>
>> Bugzilla ID: 631
>> Fixes: 512e27eeb743 ("net/virtio: move PCI specific dev init to PCI ethdev init")
>>
>> Reported-by: Wei Ling <weix.ling@intel.com>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>>  drivers/net/virtio/virtio_pci_ethdev.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/net/virtio/virtio_pci_ethdev.c b/drivers/net/virtio/virtio_pci_ethdev.c
>> index 1b818c4565..547cb8ffa0 100644
>> --- a/drivers/net/virtio/virtio_pci_ethdev.c
>> +++ b/drivers/net/virtio/virtio_pci_ethdev.c
>> @@ -76,6 +76,9 @@ eth_virtio_pci_init(struct rte_eth_dev *eth_dev)
>>         struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(eth_dev);
>>         int ret;
>>
>> +       /* Required to assign Virtio ops */
>> +       hw->port_id = eth_dev->data->port_id;
>> +
>>         if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
>>                 ret = vtpci_init(RTE_ETH_DEV_TO_PCI(eth_dev), dev);
>>                 if (ret) {
> 
> We probably don't need to set hw->port-id in eth_virtio_dev_init anymore.
> And I would only set it in the primary process context, letting the
> secondary pick what has been chosen by the primary?
> 
> 

I agree, I will remove the second assignment and also move this one only
for primary process.

Thanks,
Maxime