diff mbox series

[02/40] net/virtio: Introduce Virtio bus type

Message ID 20201220211405.313012-3-maxime.coquelin@redhat.com (mailing list archive)
State Superseded, archived
Delegated to: Maxime Coquelin
Headers show
Series net/virtio: Virtio PMD rework | expand

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Maxime Coquelin Dec. 20, 2020, 9:13 p.m. UTC
This patch is preliminary work for introducing a bus layer
in Virtio PMD, in order to improve Virtio-user integration.

A new bus type is added to provide a unified way to distinguish
which bus type is used (PCI modern, PCI legacy or Virtio-user).

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/net/virtio/virtio_ethdev.c      | 43 ++++++++++++-------------
 drivers/net/virtio/virtio_pci.c         |  4 +--
 drivers/net/virtio/virtio_pci.h         |  9 +++++-
 drivers/net/virtio/virtio_user_ethdev.c |  2 +-
 4 files changed, 32 insertions(+), 26 deletions(-)

Comments

Chenbo Xia Dec. 30, 2020, 3:02 a.m. UTC | #1
> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Monday, December 21, 2020 5:13 AM
> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; olivier.matz@6wind.com;
> amorenoz@redhat.com; david.marchand@redhat.com
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
> Subject: [PATCH 02/40] net/virtio: Introduce Virtio bus type
> 
> This patch is preliminary work for introducing a bus layer
> in Virtio PMD, in order to improve Virtio-user integration.
> 
> A new bus type is added to provide a unified way to distinguish
> which bus type is used (PCI modern, PCI legacy or Virtio-user).
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  drivers/net/virtio/virtio_ethdev.c      | 43 ++++++++++++-------------
>  drivers/net/virtio/virtio_pci.c         |  4 +--
>  drivers/net/virtio/virtio_pci.h         |  9 +++++-
>  drivers/net/virtio/virtio_user_ethdev.c |  2 +-
>  4 files changed, 32 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_ethdev.c
> b/drivers/net/virtio/virtio_ethdev.c
> index 6c233b75ba..b3086297c0 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -592,9 +592,9 @@ virtio_init_queue(struct rte_eth_dev *dev, uint16_t
> vtpci_queue_idx)
>  	 * we use virtual address. And we need properly set _offset_, please see
>  	 * VIRTIO_MBUF_DATA_DMA_ADDR in virtqueue.h for more information.
>  	 */
> -	if (!hw->virtio_user_dev)
> +	if (hw->bus_type == VIRTIO_BUS_PCI_LEGACY || hw->bus_type ==
> VIRTIO_BUS_PCI_MODERN) {
>  		vq->offset = offsetof(struct rte_mbuf, buf_iova);
> -	else {
> +	} else if (hw->bus_type == VIRTIO_BUS_USER) {
>  		vq->vq_ring_mem = (uintptr_t)mz->addr;
>  		vq->offset = offsetof(struct rte_mbuf, buf_addr);
>  		if (queue_type == VTNET_TQ)
> @@ -746,13 +746,13 @@ virtio_dev_close(struct rte_eth_dev *dev)
>  	virtio_free_queues(hw);
> 
>  #ifdef RTE_VIRTIO_USER
> -	if (hw->virtio_user_dev)
> +	if (hw->bus_type == VIRTIO_BUS_USER)
>  		virtio_user_dev_uninit(hw->virtio_user_dev);
>  	else
>  #endif
>  	if (dev->device) {
>  		rte_pci_unmap_device(RTE_ETH_DEV_TO_PCI(dev));
> -		if (!hw->modern)
> +		if (hw->bus_type == VIRTIO_BUS_PCI_LEGACY)
>  			rte_pci_ioport_unmap(VTPCI_IO(hw));
>  	}
> 
> @@ -1299,7 +1299,7 @@ virtio_intr_unmask(struct rte_eth_dev *dev)
>  	if (rte_intr_ack(dev->intr_handle) < 0)
>  		return -1;
> 
> -	if (!hw->virtio_user_dev)
> +	if (hw->bus_type == VIRTIO_BUS_PCI_LEGACY || hw->bus_type ==
> VIRTIO_BUS_PCI_MODERN)
>  		hw->use_msix = vtpci_msix_detect(RTE_ETH_DEV_TO_PCI(dev));
> 
>  	return 0;
> @@ -1313,7 +1313,7 @@ virtio_intr_enable(struct rte_eth_dev *dev)
>  	if (rte_intr_enable(dev->intr_handle) < 0)
>  		return -1;
> 
> -	if (!hw->virtio_user_dev)
> +	if (hw->bus_type == VIRTIO_BUS_PCI_LEGACY || hw->bus_type ==
> VIRTIO_BUS_PCI_MODERN)
>  		hw->use_msix = vtpci_msix_detect(RTE_ETH_DEV_TO_PCI(dev));
> 
>  	return 0;
> @@ -1327,7 +1327,7 @@ virtio_intr_disable(struct rte_eth_dev *dev)
>  	if (rte_intr_disable(dev->intr_handle) < 0)
>  		return -1;
> 
> -	if (!hw->virtio_user_dev)
> +	if (hw->bus_type == VIRTIO_BUS_PCI_LEGACY || hw->bus_type ==
> VIRTIO_BUS_PCI_MODERN)
>  		hw->use_msix = vtpci_msix_detect(RTE_ETH_DEV_TO_PCI(dev));
> 
>  	return 0;
> @@ -1368,13 +1368,13 @@ virtio_negotiate_features(struct virtio_hw *hw,
> uint64_t req_features)
>  	PMD_INIT_LOG(DEBUG, "features after negotiate = %" PRIx64,
>  		hw->guest_features);
> 
> -	if (hw->modern && !vtpci_with_feature(hw, VIRTIO_F_VERSION_1)) {
> +	if (hw->bus_type == VIRTIO_BUS_PCI_MODERN && !vtpci_with_feature(hw,
> VIRTIO_F_VERSION_1)) {
>  		PMD_INIT_LOG(ERR,
>  			"VIRTIO_F_VERSION_1 features is not enabled.");
>  		return -1;
>  	}
> 
> -	if (hw->modern || hw->virtio_user_dev) {
> +	if (hw->bus_type == VIRTIO_BUS_PCI_MODERN || hw->bus_type ==
> VIRTIO_BUS_USER) {
>  		vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_FEATURES_OK);
>  		if (!(vtpci_get_status(hw) & VIRTIO_CONFIG_STATUS_FEATURES_OK)) {
>  			PMD_INIT_LOG(ERR,
> @@ -1709,7 +1709,7 @@ virtio_init_device(struct rte_eth_dev *eth_dev, uint64_t
> req_features)
> 
>  	hw->weak_barriers = !vtpci_with_feature(hw, VIRTIO_F_ORDER_PLATFORM);
> 
> -	if (!hw->virtio_user_dev)
> +	if (hw->bus_type == VIRTIO_BUS_PCI_LEGACY || hw->bus_type ==
> VIRTIO_BUS_PCI_MODERN)
>  		pci_dev = RTE_ETH_DEV_TO_PCI(eth_dev);
> 
>  	/* If host does not support both status and MSI-X then disable LSC */
> @@ -1856,7 +1856,7 @@ virtio_init_device(struct rte_eth_dev *eth_dev, uint64_t
> req_features)
>  static int
>  virtio_remap_pci(struct rte_pci_device *pci_dev, struct virtio_hw *hw)
>  {
> -	if (hw->modern) {
> +	if (hw->bus_type == VIRTIO_BUS_PCI_MODERN) {
>  		/*
>  		 * We don't have to re-parse the PCI config space, since
>  		 * rte_pci_map_device() makes sure the mapped address
> @@ -1872,7 +1872,7 @@ virtio_remap_pci(struct rte_pci_device *pci_dev, struct
> virtio_hw *hw)
>  			PMD_INIT_LOG(DEBUG, "failed to map pci device!");
>  			return -1;
>  		}
> -	} else {
> +	} else if (hw->bus_type == VIRTIO_BUS_PCI_LEGACY) {
>  		if (rte_pci_ioport_map(pci_dev, 0, VTPCI_IO(hw)) < 0)
>  			return -1;
>  	}
> @@ -1883,15 +1883,14 @@ virtio_remap_pci(struct rte_pci_device *pci_dev,
> struct virtio_hw *hw)
>  static void
>  virtio_set_vtpci_ops(struct virtio_hw *hw)
>  {
> -#ifdef RTE_VIRTIO_USER
> -	if (hw->virtio_user_dev)
> +	if (hw->bus_type == VIRTIO_BUS_USER)
>  		VTPCI_OPS(hw) = &virtio_user_ops;
> -	else
> -#endif
> -	if (hw->modern)
> +	else if (hw->bus_type == VIRTIO_BUS_PCI_MODERN)
>  		VTPCI_OPS(hw) = &modern_ops;
> -	else
> +	else if (hw->bus_type == VIRTIO_BUS_PCI_LEGACY)
>  		VTPCI_OPS(hw) = &legacy_ops;
> +
> +	return;
>  }
> 
>  /*
> @@ -1919,7 +1918,7 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
>  	eth_dev->rx_descriptor_done = virtio_dev_rx_queue_done;
> 
>  	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
> -		if (!hw->virtio_user_dev) {
> +		if (hw->bus_type != VIRTIO_BUS_USER) {
>  			ret = virtio_remap_pci(RTE_ETH_DEV_TO_PCI(eth_dev), hw);
>  			if (ret)
>  				return ret;
> @@ -1950,7 +1949,7 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
>  	/* For virtio_user case the hw->virtio_user_dev is populated by
>  	 * virtio_user_eth_dev_alloc() before eth_virtio_dev_init() is called.
>  	 */
> -	if (!hw->virtio_user_dev) {
> +	if (hw->bus_type != VIRTIO_BUS_USER) {
>  		ret = vtpci_init(RTE_ETH_DEV_TO_PCI(eth_dev), hw);
>  		if (ret)
>  			goto err_vtpci_init;
> @@ -1982,9 +1981,9 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
>  	return 0;
> 
>  err_virtio_init:
> -	if (!hw->virtio_user_dev) {
> +	if (hw->bus_type == VIRTIO_BUS_PCI_MODERN || hw->bus_type ==
> VIRTIO_BUS_PCI_LEGACY) {
>  		rte_pci_unmap_device(RTE_ETH_DEV_TO_PCI(eth_dev));
> -		if (!hw->modern)
> +		if (hw->bus_type == VIRTIO_BUS_PCI_LEGACY)
>  			rte_pci_ioport_unmap(VTPCI_IO(hw));
>  	}
>  err_vtpci_init:
> diff --git a/drivers/net/virtio/virtio_pci.c b/drivers/net/virtio/virtio_pci.c
> index d6b950ee69..1692268f30 100644
> --- a/drivers/net/virtio/virtio_pci.c
> +++ b/drivers/net/virtio/virtio_pci.c
> @@ -697,7 +697,7 @@ vtpci_init(struct rte_pci_device *dev, struct virtio_hw
> *hw)
>  	if (virtio_read_caps(dev, hw) == 0) {
>  		PMD_INIT_LOG(INFO, "modern virtio pci detected.");
>  		virtio_hw_internal[hw->port_id].vtpci_ops = &modern_ops;
> -		hw->modern = 1;
> +		hw->bus_type = VIRTIO_BUS_PCI_MODERN;
>  		return 0;
>  	}
> 
> @@ -716,7 +716,7 @@ vtpci_init(struct rte_pci_device *dev, struct virtio_hw
> *hw)
>  	}
> 
>  	virtio_hw_internal[hw->port_id].vtpci_ops = &legacy_ops;
> -	hw->modern   = 0;
> +	hw->bus_type = VIRTIO_BUS_PCI_LEGACY;
> 
>  	return 0;
>  }
> diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
> index ab61e911b8..6388f0a74d 100644
> --- a/drivers/net/virtio/virtio_pci.h
> +++ b/drivers/net/virtio/virtio_pci.h
> @@ -243,7 +243,15 @@ struct virtio_pci_ops {
> 
>  struct virtio_net_config;
> 
> +enum virtio_bus_type {
> +	VIRTIO_BUS_UNKNOWN,
> +	VIRTIO_BUS_PCI_LEGACY,
> +	VIRTIO_BUS_PCI_MODERN,
> +	VIRTIO_BUS_USER,
> +};
> +
>  struct virtio_hw {
> +	enum virtio_bus_type bus_type;
>  	struct virtnet_ctl *cvq;
>  	uint64_t    req_guest_features;
>  	uint64_t    guest_features;
> @@ -253,7 +261,6 @@ struct virtio_hw {
>  	uint16_t    vtnet_hdr_size;
>  	uint8_t	    vlan_strip;
>  	uint8_t	    use_msix;
> -	uint8_t     modern;
>  	uint8_t     use_vec_rx;
>  	uint8_t     use_vec_tx;
>  	uint8_t     use_inorder_rx;
> diff --git a/drivers/net/virtio/virtio_user_ethdev.c
> b/drivers/net/virtio/virtio_user_ethdev.c
> index 40345193e6..516d0ee577 100644
> --- a/drivers/net/virtio/virtio_user_ethdev.c
> +++ b/drivers/net/virtio/virtio_user_ethdev.c
> @@ -629,7 +629,7 @@ virtio_user_eth_dev_alloc(struct rte_vdev_device *vdev)
>  	 * Here just pretend that we support msix.
>  	 */
>  	hw->use_msix = 1;
> -	hw->modern   = 0;
> +	hw->bus_type = VIRTIO_BUS_USER;
>  	hw->use_vec_rx = 0;
>  	hw->use_vec_tx = 0;
>  	hw->use_inorder_rx = 0;
> --
> 2.29.2

Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
David Marchand Jan. 5, 2021, 9:15 p.m. UTC | #2
On Sun, Dec 20, 2020 at 10:14 PM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
>
> This patch is preliminary work for introducing a bus layer
> in Virtio PMD, in order to improve Virtio-user integration.
>
> A new bus type is added to provide a unified way to distinguish
> which bus type is used (PCI modern, PCI legacy or Virtio-user).

In dpdk, we don't use a capital letter for starting a title.


> @@ -1883,15 +1883,14 @@ virtio_remap_pci(struct rte_pci_device *pci_dev, struct virtio_hw *hw)
>  static void
>  virtio_set_vtpci_ops(struct virtio_hw *hw)
>  {
> -#ifdef RTE_VIRTIO_USER

Too soon to remove this check, since virtio_user_ops comes from
virtio_user_ethdev.c.
This will break compilation on FreeBSD.


> -       if (hw->virtio_user_dev)
> +       if (hw->bus_type == VIRTIO_BUS_USER)
>                 VTPCI_OPS(hw) = &virtio_user_ops;
> -       else
> -#endif
> -       if (hw->modern)
> +       else if (hw->bus_type == VIRTIO_BUS_PCI_MODERN)
>                 VTPCI_OPS(hw) = &modern_ops;
> -       else
> +       else if (hw->bus_type == VIRTIO_BUS_PCI_LEGACY)
>                 VTPCI_OPS(hw) = &legacy_ops;
> +
> +       return;
>  }
>
>  /*
> @@ -1919,7 +1918,7 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
>         eth_dev->rx_descriptor_done = virtio_dev_rx_queue_done;
>
>         if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
> -               if (!hw->virtio_user_dev) {
> +               if (hw->bus_type != VIRTIO_BUS_USER) {

In the rest of the patch, we check for PCI types when dealing with PCI
code, so I'd rather be consistent and check for modern and legacy pci
types here too.


>                         ret = virtio_remap_pci(RTE_ETH_DEV_TO_PCI(eth_dev), hw);
>                         if (ret)
>                                 return ret;
> @@ -1950,7 +1949,7 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
>         /* For virtio_user case the hw->virtio_user_dev is populated by
>          * virtio_user_eth_dev_alloc() before eth_virtio_dev_init() is called.
>          */
> -       if (!hw->virtio_user_dev) {
> +       if (hw->bus_type != VIRTIO_BUS_USER) {

Idem.


>                 ret = vtpci_init(RTE_ETH_DEV_TO_PCI(eth_dev), hw);
>                 if (ret)
>                         goto err_vtpci_init;
> @@ -1982,9 +1981,9 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
>         return 0;
>
>  err_virtio_init:
> -       if (!hw->virtio_user_dev) {
> +       if (hw->bus_type == VIRTIO_BUS_PCI_MODERN || hw->bus_type == VIRTIO_BUS_PCI_LEGACY) {
>                 rte_pci_unmap_device(RTE_ETH_DEV_TO_PCI(eth_dev));
> -               if (!hw->modern)
> +               if (hw->bus_type == VIRTIO_BUS_PCI_LEGACY)
>                         rte_pci_ioport_unmap(VTPCI_IO(hw));
>         }
>  err_vtpci_init:
Maxime Coquelin Jan. 14, 2021, 9:24 a.m. UTC | #3
On 1/5/21 10:15 PM, David Marchand wrote:
> On Sun, Dec 20, 2020 at 10:14 PM Maxime Coquelin
> <maxime.coquelin@redhat.com> wrote:
>>
>> This patch is preliminary work for introducing a bus layer
>> in Virtio PMD, in order to improve Virtio-user integration.
>>
>> A new bus type is added to provide a unified way to distinguish
>> which bus type is used (PCI modern, PCI legacy or Virtio-user).
> 
> In dpdk, we don't use a capital letter for starting a title.
> 
> 
>> @@ -1883,15 +1883,14 @@ virtio_remap_pci(struct rte_pci_device *pci_dev, struct virtio_hw *hw)
>>  static void
>>  virtio_set_vtpci_ops(struct virtio_hw *hw)
>>  {
>> -#ifdef RTE_VIRTIO_USER
> 
> Too soon to remove this check, since virtio_user_ops comes from
> virtio_user_ethdev.c.
> This will break compilation on FreeBSD.

Agree, I restored it back in v2.

> 
>> -       if (hw->virtio_user_dev)
>> +       if (hw->bus_type == VIRTIO_BUS_USER)
>>                 VTPCI_OPS(hw) = &virtio_user_ops;
>> -       else
>> -#endif
>> -       if (hw->modern)
>> +       else if (hw->bus_type == VIRTIO_BUS_PCI_MODERN)
>>                 VTPCI_OPS(hw) = &modern_ops;
>> -       else
>> +       else if (hw->bus_type == VIRTIO_BUS_PCI_LEGACY)
>>                 VTPCI_OPS(hw) = &legacy_ops;
>> +
>> +       return;
>>  }
>>
>>  /*
>> @@ -1919,7 +1918,7 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
>>         eth_dev->rx_descriptor_done = virtio_dev_rx_queue_done;
>>
>>         if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
>> -               if (!hw->virtio_user_dev) {
>> +               if (hw->bus_type != VIRTIO_BUS_USER) {
> 
> In the rest of the patch, we check for PCI types when dealing with PCI
> code, so I'd rather be consistent and check for modern and legacy pci
> types here too.

Agree, it is not consistent. I fixed it here and below for v2.

> 
>>                         ret = virtio_remap_pci(RTE_ETH_DEV_TO_PCI(eth_dev), hw);
>>                         if (ret)
>>                                 return ret;
>> @@ -1950,7 +1949,7 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
>>         /* For virtio_user case the hw->virtio_user_dev is populated by
>>          * virtio_user_eth_dev_alloc() before eth_virtio_dev_init() is called.
>>          */
>> -       if (!hw->virtio_user_dev) {
>> +       if (hw->bus_type != VIRTIO_BUS_USER) {
> 
> Idem.
> 
> 
>>                 ret = vtpci_init(RTE_ETH_DEV_TO_PCI(eth_dev), hw);
>>                 if (ret)
>>                         goto err_vtpci_init;
>> @@ -1982,9 +1981,9 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
>>         return 0;
>>
>>  err_virtio_init:
>> -       if (!hw->virtio_user_dev) {
>> +       if (hw->bus_type == VIRTIO_BUS_PCI_MODERN || hw->bus_type == VIRTIO_BUS_PCI_LEGACY) {
>>                 rte_pci_unmap_device(RTE_ETH_DEV_TO_PCI(eth_dev));
>> -               if (!hw->modern)
>> +               if (hw->bus_type == VIRTIO_BUS_PCI_LEGACY)
>>                         rte_pci_ioport_unmap(VTPCI_IO(hw));
>>         }
>>  err_vtpci_init:
> 
>
Maxime Coquelin Jan. 14, 2021, 10:54 a.m. UTC | #4
On 1/14/21 10:24 AM, Maxime Coquelin wrote:
> 
> 
> On 1/5/21 10:15 PM, David Marchand wrote:
>> On Sun, Dec 20, 2020 at 10:14 PM Maxime Coquelin
>> <maxime.coquelin@redhat.com> wrote:
>>>
>>> This patch is preliminary work for introducing a bus layer
>>> in Virtio PMD, in order to improve Virtio-user integration.
>>>
>>> A new bus type is added to provide a unified way to distinguish
>>> which bus type is used (PCI modern, PCI legacy or Virtio-user).
>>
>> In dpdk, we don't use a capital letter for starting a title.
>>
>>
>>> @@ -1883,15 +1883,14 @@ virtio_remap_pci(struct rte_pci_device *pci_dev, struct virtio_hw *hw)
>>>  static void
>>>  virtio_set_vtpci_ops(struct virtio_hw *hw)
>>>  {
>>> -#ifdef RTE_VIRTIO_USER
>>
>> Too soon to remove this check, since virtio_user_ops comes from
>> virtio_user_ethdev.c.
>> This will break compilation on FreeBSD.
> 
> Agree, I restored it back in v2.
> 
>>
>>> -       if (hw->virtio_user_dev)
>>> +       if (hw->bus_type == VIRTIO_BUS_USER)
>>>                 VTPCI_OPS(hw) = &virtio_user_ops;
>>> -       else
>>> -#endif
>>> -       if (hw->modern)
>>> +       else if (hw->bus_type == VIRTIO_BUS_PCI_MODERN)
>>>                 VTPCI_OPS(hw) = &modern_ops;
>>> -       else
>>> +       else if (hw->bus_type == VIRTIO_BUS_PCI_LEGACY)
>>>                 VTPCI_OPS(hw) = &legacy_ops;
>>> +
>>> +       return;
>>>  }
>>>
>>>  /*
>>> @@ -1919,7 +1918,7 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
>>>         eth_dev->rx_descriptor_done = virtio_dev_rx_queue_done;
>>>
>>>         if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
>>> -               if (!hw->virtio_user_dev) {
>>> +               if (hw->bus_type != VIRTIO_BUS_USER) {
>>
>> In the rest of the patch, we check for PCI types when dealing with PCI
>> code, so I'd rather be consistent and check for modern and legacy pci
>> types here too.
> 
> Agree, it is not consistent. I fixed it here and below for v2.

Wait, as we discussed off-list, we need hw->bus_type != VIRTIO_BUS_USER,
as the bus_type is still unknown for PCI bus at that stage.

>>
>>>                         ret = virtio_remap_pci(RTE_ETH_DEV_TO_PCI(eth_dev), hw);
>>>                         if (ret)
>>>                                 return ret;
>>> @@ -1950,7 +1949,7 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
>>>         /* For virtio_user case the hw->virtio_user_dev is populated by
>>>          * virtio_user_eth_dev_alloc() before eth_virtio_dev_init() is called.
>>>          */
>>> -       if (!hw->virtio_user_dev) {
>>> +       if (hw->bus_type != VIRTIO_BUS_USER) {
>>
>> Idem.

Same here.

>>
>>
>>>                 ret = vtpci_init(RTE_ETH_DEV_TO_PCI(eth_dev), hw);

bus_type is set to VIRTIO_BUS_PCI_MODERN or VIRTIO_BUS_PCI_LEGACY in
vtcpi_init().

>>>                 if (ret)
>>>                         goto err_vtpci_init;
>>> @@ -1982,9 +1981,9 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
>>>         return 0;
>>>
>>>  err_virtio_init:
>>> -       if (!hw->virtio_user_dev) {
>>> +       if (hw->bus_type == VIRTIO_BUS_PCI_MODERN || hw->bus_type == VIRTIO_BUS_PCI_LEGACY) {
>>>                 rte_pci_unmap_device(RTE_ETH_DEV_TO_PCI(eth_dev));
>>> -               if (!hw->modern)
>>> +               if (hw->bus_type == VIRTIO_BUS_PCI_LEGACY)
>>>                         rte_pci_ioport_unmap(VTPCI_IO(hw));
>>>         }
>>>  err_vtpci_init:
>>
>>
>
David Marchand Jan. 14, 2021, 11:55 a.m. UTC | #5
On Thu, Jan 14, 2021 at 11:55 AM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
> >>> @@ -1919,7 +1918,7 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
> >>>         eth_dev->rx_descriptor_done = virtio_dev_rx_queue_done;
> >>>
> >>>         if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
> >>> -               if (!hw->virtio_user_dev) {
> >>> +               if (hw->bus_type != VIRTIO_BUS_USER) {
> >>
> >> In the rest of the patch, we check for PCI types when dealing with PCI
> >> code, so I'd rather be consistent and check for modern and legacy pci
> >> types here too.
> >
> > Agree, it is not consistent. I fixed it here and below for v2.
>
> Wait, as we discussed off-list, we need hw->bus_type != VIRTIO_BUS_USER,
> as the bus_type is still unknown for PCI bus at that stage.

Yep, thanks for double checking.
diff mbox series

Patch

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 6c233b75ba..b3086297c0 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -592,9 +592,9 @@  virtio_init_queue(struct rte_eth_dev *dev, uint16_t vtpci_queue_idx)
 	 * we use virtual address. And we need properly set _offset_, please see
 	 * VIRTIO_MBUF_DATA_DMA_ADDR in virtqueue.h for more information.
 	 */
-	if (!hw->virtio_user_dev)
+	if (hw->bus_type == VIRTIO_BUS_PCI_LEGACY || hw->bus_type == VIRTIO_BUS_PCI_MODERN) {
 		vq->offset = offsetof(struct rte_mbuf, buf_iova);
-	else {
+	} else if (hw->bus_type == VIRTIO_BUS_USER) {
 		vq->vq_ring_mem = (uintptr_t)mz->addr;
 		vq->offset = offsetof(struct rte_mbuf, buf_addr);
 		if (queue_type == VTNET_TQ)
@@ -746,13 +746,13 @@  virtio_dev_close(struct rte_eth_dev *dev)
 	virtio_free_queues(hw);
 
 #ifdef RTE_VIRTIO_USER
-	if (hw->virtio_user_dev)
+	if (hw->bus_type == VIRTIO_BUS_USER)
 		virtio_user_dev_uninit(hw->virtio_user_dev);
 	else
 #endif
 	if (dev->device) {
 		rte_pci_unmap_device(RTE_ETH_DEV_TO_PCI(dev));
-		if (!hw->modern)
+		if (hw->bus_type == VIRTIO_BUS_PCI_LEGACY)
 			rte_pci_ioport_unmap(VTPCI_IO(hw));
 	}
 
@@ -1299,7 +1299,7 @@  virtio_intr_unmask(struct rte_eth_dev *dev)
 	if (rte_intr_ack(dev->intr_handle) < 0)
 		return -1;
 
-	if (!hw->virtio_user_dev)
+	if (hw->bus_type == VIRTIO_BUS_PCI_LEGACY || hw->bus_type == VIRTIO_BUS_PCI_MODERN)
 		hw->use_msix = vtpci_msix_detect(RTE_ETH_DEV_TO_PCI(dev));
 
 	return 0;
@@ -1313,7 +1313,7 @@  virtio_intr_enable(struct rte_eth_dev *dev)
 	if (rte_intr_enable(dev->intr_handle) < 0)
 		return -1;
 
-	if (!hw->virtio_user_dev)
+	if (hw->bus_type == VIRTIO_BUS_PCI_LEGACY || hw->bus_type == VIRTIO_BUS_PCI_MODERN)
 		hw->use_msix = vtpci_msix_detect(RTE_ETH_DEV_TO_PCI(dev));
 
 	return 0;
@@ -1327,7 +1327,7 @@  virtio_intr_disable(struct rte_eth_dev *dev)
 	if (rte_intr_disable(dev->intr_handle) < 0)
 		return -1;
 
-	if (!hw->virtio_user_dev)
+	if (hw->bus_type == VIRTIO_BUS_PCI_LEGACY || hw->bus_type == VIRTIO_BUS_PCI_MODERN)
 		hw->use_msix = vtpci_msix_detect(RTE_ETH_DEV_TO_PCI(dev));
 
 	return 0;
@@ -1368,13 +1368,13 @@  virtio_negotiate_features(struct virtio_hw *hw, uint64_t req_features)
 	PMD_INIT_LOG(DEBUG, "features after negotiate = %" PRIx64,
 		hw->guest_features);
 
-	if (hw->modern && !vtpci_with_feature(hw, VIRTIO_F_VERSION_1)) {
+	if (hw->bus_type == VIRTIO_BUS_PCI_MODERN && !vtpci_with_feature(hw, VIRTIO_F_VERSION_1)) {
 		PMD_INIT_LOG(ERR,
 			"VIRTIO_F_VERSION_1 features is not enabled.");
 		return -1;
 	}
 
-	if (hw->modern || hw->virtio_user_dev) {
+	if (hw->bus_type == VIRTIO_BUS_PCI_MODERN || hw->bus_type == VIRTIO_BUS_USER) {
 		vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_FEATURES_OK);
 		if (!(vtpci_get_status(hw) & VIRTIO_CONFIG_STATUS_FEATURES_OK)) {
 			PMD_INIT_LOG(ERR,
@@ -1709,7 +1709,7 @@  virtio_init_device(struct rte_eth_dev *eth_dev, uint64_t req_features)
 
 	hw->weak_barriers = !vtpci_with_feature(hw, VIRTIO_F_ORDER_PLATFORM);
 
-	if (!hw->virtio_user_dev)
+	if (hw->bus_type == VIRTIO_BUS_PCI_LEGACY || hw->bus_type == VIRTIO_BUS_PCI_MODERN)
 		pci_dev = RTE_ETH_DEV_TO_PCI(eth_dev);
 
 	/* If host does not support both status and MSI-X then disable LSC */
@@ -1856,7 +1856,7 @@  virtio_init_device(struct rte_eth_dev *eth_dev, uint64_t req_features)
 static int
 virtio_remap_pci(struct rte_pci_device *pci_dev, struct virtio_hw *hw)
 {
-	if (hw->modern) {
+	if (hw->bus_type == VIRTIO_BUS_PCI_MODERN) {
 		/*
 		 * We don't have to re-parse the PCI config space, since
 		 * rte_pci_map_device() makes sure the mapped address
@@ -1872,7 +1872,7 @@  virtio_remap_pci(struct rte_pci_device *pci_dev, struct virtio_hw *hw)
 			PMD_INIT_LOG(DEBUG, "failed to map pci device!");
 			return -1;
 		}
-	} else {
+	} else if (hw->bus_type == VIRTIO_BUS_PCI_LEGACY) {
 		if (rte_pci_ioport_map(pci_dev, 0, VTPCI_IO(hw)) < 0)
 			return -1;
 	}
@@ -1883,15 +1883,14 @@  virtio_remap_pci(struct rte_pci_device *pci_dev, struct virtio_hw *hw)
 static void
 virtio_set_vtpci_ops(struct virtio_hw *hw)
 {
-#ifdef RTE_VIRTIO_USER
-	if (hw->virtio_user_dev)
+	if (hw->bus_type == VIRTIO_BUS_USER)
 		VTPCI_OPS(hw) = &virtio_user_ops;
-	else
-#endif
-	if (hw->modern)
+	else if (hw->bus_type == VIRTIO_BUS_PCI_MODERN)
 		VTPCI_OPS(hw) = &modern_ops;
-	else
+	else if (hw->bus_type == VIRTIO_BUS_PCI_LEGACY)
 		VTPCI_OPS(hw) = &legacy_ops;
+
+	return;
 }
 
 /*
@@ -1919,7 +1918,7 @@  eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
 	eth_dev->rx_descriptor_done = virtio_dev_rx_queue_done;
 
 	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
-		if (!hw->virtio_user_dev) {
+		if (hw->bus_type != VIRTIO_BUS_USER) {
 			ret = virtio_remap_pci(RTE_ETH_DEV_TO_PCI(eth_dev), hw);
 			if (ret)
 				return ret;
@@ -1950,7 +1949,7 @@  eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
 	/* For virtio_user case the hw->virtio_user_dev is populated by
 	 * virtio_user_eth_dev_alloc() before eth_virtio_dev_init() is called.
 	 */
-	if (!hw->virtio_user_dev) {
+	if (hw->bus_type != VIRTIO_BUS_USER) {
 		ret = vtpci_init(RTE_ETH_DEV_TO_PCI(eth_dev), hw);
 		if (ret)
 			goto err_vtpci_init;
@@ -1982,9 +1981,9 @@  eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
 	return 0;
 
 err_virtio_init:
-	if (!hw->virtio_user_dev) {
+	if (hw->bus_type == VIRTIO_BUS_PCI_MODERN || hw->bus_type == VIRTIO_BUS_PCI_LEGACY) {
 		rte_pci_unmap_device(RTE_ETH_DEV_TO_PCI(eth_dev));
-		if (!hw->modern)
+		if (hw->bus_type == VIRTIO_BUS_PCI_LEGACY)
 			rte_pci_ioport_unmap(VTPCI_IO(hw));
 	}
 err_vtpci_init:
diff --git a/drivers/net/virtio/virtio_pci.c b/drivers/net/virtio/virtio_pci.c
index d6b950ee69..1692268f30 100644
--- a/drivers/net/virtio/virtio_pci.c
+++ b/drivers/net/virtio/virtio_pci.c
@@ -697,7 +697,7 @@  vtpci_init(struct rte_pci_device *dev, struct virtio_hw *hw)
 	if (virtio_read_caps(dev, hw) == 0) {
 		PMD_INIT_LOG(INFO, "modern virtio pci detected.");
 		virtio_hw_internal[hw->port_id].vtpci_ops = &modern_ops;
-		hw->modern = 1;
+		hw->bus_type = VIRTIO_BUS_PCI_MODERN;
 		return 0;
 	}
 
@@ -716,7 +716,7 @@  vtpci_init(struct rte_pci_device *dev, struct virtio_hw *hw)
 	}
 
 	virtio_hw_internal[hw->port_id].vtpci_ops = &legacy_ops;
-	hw->modern   = 0;
+	hw->bus_type = VIRTIO_BUS_PCI_LEGACY;
 
 	return 0;
 }
diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
index ab61e911b8..6388f0a74d 100644
--- a/drivers/net/virtio/virtio_pci.h
+++ b/drivers/net/virtio/virtio_pci.h
@@ -243,7 +243,15 @@  struct virtio_pci_ops {
 
 struct virtio_net_config;
 
+enum virtio_bus_type {
+	VIRTIO_BUS_UNKNOWN,
+	VIRTIO_BUS_PCI_LEGACY,
+	VIRTIO_BUS_PCI_MODERN,
+	VIRTIO_BUS_USER,
+};
+
 struct virtio_hw {
+	enum virtio_bus_type bus_type;
 	struct virtnet_ctl *cvq;
 	uint64_t    req_guest_features;
 	uint64_t    guest_features;
@@ -253,7 +261,6 @@  struct virtio_hw {
 	uint16_t    vtnet_hdr_size;
 	uint8_t	    vlan_strip;
 	uint8_t	    use_msix;
-	uint8_t     modern;
 	uint8_t     use_vec_rx;
 	uint8_t     use_vec_tx;
 	uint8_t     use_inorder_rx;
diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index 40345193e6..516d0ee577 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -629,7 +629,7 @@  virtio_user_eth_dev_alloc(struct rte_vdev_device *vdev)
 	 * Here just pretend that we support msix.
 	 */
 	hw->use_msix = 1;
-	hw->modern   = 0;
+	hw->bus_type = VIRTIO_BUS_USER;
 	hw->use_vec_rx = 0;
 	hw->use_vec_tx = 0;
 	hw->use_inorder_rx = 0;