diff mbox series

[v2,09/44] net/virtio: move MSIX detection to PCI ethdev

Message ID 20210119212507.1043636-10-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 Jan. 19, 2021, 9:24 p.m. UTC
This patch introduces a new callback to notify the bus
driver some interrupt related operation was done.

This is used by Virtio PCI driver to check msix status.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/net/virtio/virtio_ethdev.c     |  12 +--
 drivers/net/virtio/virtio_pci.c        | 120 ++++++++++++++-----------
 drivers/net/virtio/virtio_pci.h        |   6 +-
 drivers/net/virtio/virtio_pci_ethdev.c |   2 +
 4 files changed, 82 insertions(+), 58 deletions(-)

Comments

Xia, Chenbo Jan. 21, 2021, 7:12 a.m. UTC | #1
Hi Maxime,

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Wednesday, January 20, 2021 5:25 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 v2 09/44] net/virtio: move MSIX detection to PCI ethdev
> 
> This patch introduces a new callback to notify the bus
> driver some interrupt related operation was done.
> 
> This is used by Virtio PCI driver to check msix status.
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  drivers/net/virtio/virtio_ethdev.c     |  12 +--
>  drivers/net/virtio/virtio_pci.c        | 120 ++++++++++++++-----------
>  drivers/net/virtio/virtio_pci.h        |   6 +-
>  drivers/net/virtio/virtio_pci_ethdev.c |   2 +
>  4 files changed, 82 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_ethdev.c
> b/drivers/net/virtio/virtio_ethdev.c
> index a3e81f336d..13d5a76376 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -1287,8 +1287,8 @@ virtio_intr_unmask(struct rte_eth_dev *dev)
>  	if (rte_intr_ack(dev->intr_handle) < 0)
>  		return -1;
> 
> -	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));
> +	if (VTPCI_OPS(hw)->intr_event)

Emmm.. why is callback called intr_event? The callback detects/checks msix and lsc.
IMHO, maybe intr_detect? Or another similar verb you like 😊

> +		VTPCI_OPS(hw)->intr_event(hw);
> 
>  	return 0;

<snip>

> 
> +msix_detect:
> +	hw->use_msix = vtpci_msix_detect(dev);
> +

Just use use the callback introduced? Should also work later when intr_lsc is added.

Thanks,
Chenbo

>  	return 0;
>  }
> 
> -enum virtio_msix_status
> -vtpci_msix_detect(struct rte_pci_device *dev)
> -{
> -	uint8_t pos;
> -	int ret;
> -
> -	ret = rte_pci_read_config(dev, &pos, 1, PCI_CAPABILITY_LIST);
> -	if (ret != 1) {
> -		PMD_INIT_LOG(DEBUG,
> -			     "failed to read pci capability list, ret %d", ret);
> -		return VIRTIO_MSIX_NONE;
> -	}
> -
> -	while (pos) {
> -		uint8_t cap[2];
> -
> -		ret = rte_pci_read_config(dev, cap, sizeof(cap), pos);
> -		if (ret != sizeof(cap)) {
> -			PMD_INIT_LOG(DEBUG,
> -				     "failed to read pci cap at pos: %x ret %d",
> -				     pos, ret);
> -			break;
> -		}
> -
> -		if (cap[0] == PCI_CAP_ID_MSIX) {
> -			uint16_t flags;
> -
> -			ret = rte_pci_read_config(dev, &flags, sizeof(flags),
> -					pos + sizeof(cap));
> -			if (ret != sizeof(flags)) {
> -				PMD_INIT_LOG(DEBUG,
> -					     "failed to read pci cap at pos:"
> -					     " %x ret %d", pos + 2, ret);
> -				break;
> -			}
> -
> -			if (flags & PCI_MSIX_ENABLE)
> -				return VIRTIO_MSIX_ENABLED;
> -			else
> -				return VIRTIO_MSIX_DISABLED;
> -		}
> -
> -		pos = cap[1];
> -	}
> -
> -	return VIRTIO_MSIX_NONE;
> -}
> diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
> index 59f6688218..6f5c53c4b7 100644
> --- a/drivers/net/virtio/virtio_pci.h
> +++ b/drivers/net/virtio/virtio_pci.h
> @@ -239,6 +239,7 @@ struct virtio_pci_ops {
>  	int (*setup_queue)(struct virtio_hw *hw, struct virtqueue *vq);
>  	void (*del_queue)(struct virtio_hw *hw, struct virtqueue *vq);
>  	void (*notify_queue)(struct virtio_hw *hw, struct virtqueue *vq);
> +	void (*intr_event)(struct virtio_hw *hw);
>  };
> 
>  struct virtio_net_config;
> @@ -303,10 +304,13 @@ struct virtio_pci_dev {
>  struct virtio_hw_internal {
>  	const struct virtio_pci_ops *vtpci_ops;
>  	struct rte_pci_ioport io;
> +	struct rte_pci_device *dev;
>  };
> 
>  #define VTPCI_OPS(hw)	(virtio_hw_internal[(hw)->port_id].vtpci_ops)
>  #define VTPCI_IO(hw)	(&virtio_hw_internal[(hw)->port_id].io)
> +#define VTPCI_DEV(hw)	(virtio_hw_internal[(hw)->port_id].dev)
> +
> 
>  extern struct virtio_hw_internal virtio_hw_internal[RTE_MAX_ETHPORTS];
> 
> @@ -383,8 +387,6 @@ void vtpci_read_dev_config(struct virtio_hw *, size_t,
> void *, int);
> 
>  uint8_t vtpci_isr(struct virtio_hw *);
> 
> -enum virtio_msix_status vtpci_msix_detect(struct rte_pci_device *dev);
> -
>  extern const struct virtio_pci_ops legacy_ops;
>  extern const struct virtio_pci_ops modern_ops;
>  extern const struct virtio_pci_ops virtio_user_ops;
> diff --git a/drivers/net/virtio/virtio_pci_ethdev.c
> b/drivers/net/virtio/virtio_pci_ethdev.c
> index 6a0ef6edc3..045b134ef2 100644
> --- a/drivers/net/virtio/virtio_pci_ethdev.c
> +++ b/drivers/net/virtio/virtio_pci_ethdev.c
> @@ -73,6 +73,8 @@ 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;
> 
> +	VTPCI_DEV(hw) = pci_dev;
> +
>  	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
>  		ret = vtpci_init(RTE_ETH_DEV_TO_PCI(eth_dev), hw);
>  		if (ret) {
> --
> 2.29.2
Maxime Coquelin Jan. 25, 2021, 12:41 p.m. UTC | #2
On 1/21/21 8:12 AM, Xia, Chenbo wrote:
> Hi Maxime,
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Wednesday, January 20, 2021 5:25 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 v2 09/44] net/virtio: move MSIX detection to PCI ethdev
>>
>> This patch introduces a new callback to notify the bus
>> driver some interrupt related operation was done.
>>
>> This is used by Virtio PCI driver to check msix status.
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>>  drivers/net/virtio/virtio_ethdev.c     |  12 +--
>>  drivers/net/virtio/virtio_pci.c        | 120 ++++++++++++++-----------
>>  drivers/net/virtio/virtio_pci.h        |   6 +-
>>  drivers/net/virtio/virtio_pci_ethdev.c |   2 +
>>  4 files changed, 82 insertions(+), 58 deletions(-)
>>
>> diff --git a/drivers/net/virtio/virtio_ethdev.c
>> b/drivers/net/virtio/virtio_ethdev.c
>> index a3e81f336d..13d5a76376 100644
>> --- a/drivers/net/virtio/virtio_ethdev.c
>> +++ b/drivers/net/virtio/virtio_ethdev.c
>> @@ -1287,8 +1287,8 @@ virtio_intr_unmask(struct rte_eth_dev *dev)
>>  	if (rte_intr_ack(dev->intr_handle) < 0)
>>  		return -1;
>>
>> -	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));
>> +	if (VTPCI_OPS(hw)->intr_event)
> 
> Emmm.. why is callback called intr_event? The callback detects/checks msix and lsc.
> IMHO, maybe intr_detect? Or another similar verb you like 😊

I called it intr_event because it could be used for other purpose than
intr detection. But this is the only use for now so let's immplement
your suggestion and we can revisit later if needed.

>> +		VTPCI_OPS(hw)->intr_event(hw);
>>
>>  	return 0;
> 
> <snip>
> 
>>
>> +msix_detect:
>> +	hw->use_msix = vtpci_msix_detect(dev);
>> +
> 
> Just use use the callback introduced? Should also work later when intr_lsc is added.

I didn't do it given that it was in the same file and that the callback
was to report an event happened. With renaming the callback as you
suggest, it makes sense to use it.

I will fix in v3.

Thanks,
Maxime

> Thanks,
> Chenbo
>
diff mbox series

Patch

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index a3e81f336d..13d5a76376 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1287,8 +1287,8 @@  virtio_intr_unmask(struct rte_eth_dev *dev)
 	if (rte_intr_ack(dev->intr_handle) < 0)
 		return -1;
 
-	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));
+	if (VTPCI_OPS(hw)->intr_event)
+		VTPCI_OPS(hw)->intr_event(hw);
 
 	return 0;
 }
@@ -1301,8 +1301,8 @@  virtio_intr_enable(struct rte_eth_dev *dev)
 	if (rte_intr_enable(dev->intr_handle) < 0)
 		return -1;
 
-	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));
+	if (VTPCI_OPS(hw)->intr_event)
+		VTPCI_OPS(hw)->intr_event(hw);
 
 	return 0;
 }
@@ -1315,8 +1315,8 @@  virtio_intr_disable(struct rte_eth_dev *dev)
 	if (rte_intr_disable(dev->intr_handle) < 0)
 		return -1;
 
-	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));
+	if (VTPCI_OPS(hw)->intr_event)
+		VTPCI_OPS(hw)->intr_event(hw);
 
 	return 0;
 }
diff --git a/drivers/net/virtio/virtio_pci.c b/drivers/net/virtio/virtio_pci.c
index 345d73f868..63bc31f732 100644
--- a/drivers/net/virtio/virtio_pci.c
+++ b/drivers/net/virtio/virtio_pci.c
@@ -47,6 +47,56 @@  check_vq_phys_addr_ok(struct virtqueue *vq)
 	return 1;
 }
 
+#define PCI_MSIX_ENABLE 0x8000
+
+static enum virtio_msix_status
+vtpci_msix_detect(struct rte_pci_device *dev)
+{
+	uint8_t pos;
+	int ret;
+
+	ret = rte_pci_read_config(dev, &pos, 1, PCI_CAPABILITY_LIST);
+	if (ret != 1) {
+		PMD_INIT_LOG(DEBUG,
+			     "failed to read pci capability list, ret %d", ret);
+		return VIRTIO_MSIX_NONE;
+	}
+
+	while (pos) {
+		uint8_t cap[2];
+
+		ret = rte_pci_read_config(dev, cap, sizeof(cap), pos);
+		if (ret != sizeof(cap)) {
+			PMD_INIT_LOG(DEBUG,
+				     "failed to read pci cap at pos: %x ret %d",
+				     pos, ret);
+			break;
+		}
+
+		if (cap[0] == PCI_CAP_ID_MSIX) {
+			uint16_t flags;
+
+			ret = rte_pci_read_config(dev, &flags, sizeof(flags),
+					pos + sizeof(cap));
+			if (ret != sizeof(flags)) {
+				PMD_INIT_LOG(DEBUG,
+					     "failed to read pci cap at pos:"
+					     " %x ret %d", pos + 2, ret);
+				break;
+			}
+
+			if (flags & PCI_MSIX_ENABLE)
+				return VIRTIO_MSIX_ENABLED;
+			else
+				return VIRTIO_MSIX_DISABLED;
+		}
+
+		pos = cap[1];
+	}
+
+	return VIRTIO_MSIX_NONE;
+}
+
 /*
  * Since we are in legacy mode:
  * http://ozlabs.org/~rusty/virtio-spec/virtio-0.9.5.pdf
@@ -241,6 +291,12 @@  legacy_notify_queue(struct virtio_hw *hw, struct virtqueue *vq)
 		VIRTIO_PCI_QUEUE_NOTIFY);
 }
 
+static void
+legacy_intr_event(struct virtio_hw *hw)
+{
+	hw->use_msix = vtpci_msix_detect(VTPCI_DEV(hw));
+}
+
 const struct virtio_pci_ops legacy_ops = {
 	.read_dev_cfg	= legacy_read_dev_config,
 	.write_dev_cfg	= legacy_write_dev_config,
@@ -255,6 +311,7 @@  const struct virtio_pci_ops legacy_ops = {
 	.setup_queue	= legacy_setup_queue,
 	.del_queue	= legacy_del_queue,
 	.notify_queue	= legacy_notify_queue,
+	.intr_event	= legacy_intr_event,
 };
 
 static inline void
@@ -446,6 +503,14 @@  modern_notify_queue(struct virtio_hw *hw, struct virtqueue *vq)
 	rte_write32(notify_data, vq->notify_addr);
 }
 
+
+
+static void
+modern_intr_event(struct virtio_hw *hw)
+{
+	hw->use_msix = vtpci_msix_detect(VTPCI_DEV(hw));
+}
+
 const struct virtio_pci_ops modern_ops = {
 	.read_dev_cfg	= modern_read_dev_config,
 	.write_dev_cfg	= modern_write_dev_config,
@@ -460,6 +525,7 @@  const struct virtio_pci_ops modern_ops = {
 	.setup_queue	= modern_setup_queue,
 	.del_queue	= modern_del_queue,
 	.notify_queue	= modern_notify_queue,
+	.intr_event	= modern_intr_event,
 };
 
 
@@ -562,8 +628,6 @@  get_cfg_addr(struct rte_pci_device *dev, struct virtio_pci_cap *cap)
 	return base + offset;
 }
 
-#define PCI_MSIX_ENABLE 0x8000
-
 static int
 virtio_read_caps(struct rte_pci_device *dev, struct virtio_hw *hw)
 {
@@ -700,7 +764,7 @@  vtpci_init(struct rte_pci_device *dev, struct virtio_hw *hw)
 		PMD_INIT_LOG(INFO, "modern virtio pci detected.");
 		virtio_hw_internal[hw->port_id].vtpci_ops = &modern_ops;
 		hw->bus_type = VIRTIO_BUS_PCI_MODERN;
-		return 0;
+		goto msix_detect;
 	}
 
 	PMD_INIT_LOG(INFO, "trying with legacy virtio pci.");
@@ -720,53 +784,9 @@  vtpci_init(struct rte_pci_device *dev, struct virtio_hw *hw)
 	virtio_hw_internal[hw->port_id].vtpci_ops = &legacy_ops;
 	hw->bus_type = VIRTIO_BUS_PCI_LEGACY;
 
+msix_detect:
+	hw->use_msix = vtpci_msix_detect(dev);
+
 	return 0;
 }
 
-enum virtio_msix_status
-vtpci_msix_detect(struct rte_pci_device *dev)
-{
-	uint8_t pos;
-	int ret;
-
-	ret = rte_pci_read_config(dev, &pos, 1, PCI_CAPABILITY_LIST);
-	if (ret != 1) {
-		PMD_INIT_LOG(DEBUG,
-			     "failed to read pci capability list, ret %d", ret);
-		return VIRTIO_MSIX_NONE;
-	}
-
-	while (pos) {
-		uint8_t cap[2];
-
-		ret = rte_pci_read_config(dev, cap, sizeof(cap), pos);
-		if (ret != sizeof(cap)) {
-			PMD_INIT_LOG(DEBUG,
-				     "failed to read pci cap at pos: %x ret %d",
-				     pos, ret);
-			break;
-		}
-
-		if (cap[0] == PCI_CAP_ID_MSIX) {
-			uint16_t flags;
-
-			ret = rte_pci_read_config(dev, &flags, sizeof(flags),
-					pos + sizeof(cap));
-			if (ret != sizeof(flags)) {
-				PMD_INIT_LOG(DEBUG,
-					     "failed to read pci cap at pos:"
-					     " %x ret %d", pos + 2, ret);
-				break;
-			}
-
-			if (flags & PCI_MSIX_ENABLE)
-				return VIRTIO_MSIX_ENABLED;
-			else
-				return VIRTIO_MSIX_DISABLED;
-		}
-
-		pos = cap[1];
-	}
-
-	return VIRTIO_MSIX_NONE;
-}
diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
index 59f6688218..6f5c53c4b7 100644
--- a/drivers/net/virtio/virtio_pci.h
+++ b/drivers/net/virtio/virtio_pci.h
@@ -239,6 +239,7 @@  struct virtio_pci_ops {
 	int (*setup_queue)(struct virtio_hw *hw, struct virtqueue *vq);
 	void (*del_queue)(struct virtio_hw *hw, struct virtqueue *vq);
 	void (*notify_queue)(struct virtio_hw *hw, struct virtqueue *vq);
+	void (*intr_event)(struct virtio_hw *hw);
 };
 
 struct virtio_net_config;
@@ -303,10 +304,13 @@  struct virtio_pci_dev {
 struct virtio_hw_internal {
 	const struct virtio_pci_ops *vtpci_ops;
 	struct rte_pci_ioport io;
+	struct rte_pci_device *dev;
 };
 
 #define VTPCI_OPS(hw)	(virtio_hw_internal[(hw)->port_id].vtpci_ops)
 #define VTPCI_IO(hw)	(&virtio_hw_internal[(hw)->port_id].io)
+#define VTPCI_DEV(hw)	(virtio_hw_internal[(hw)->port_id].dev)
+
 
 extern struct virtio_hw_internal virtio_hw_internal[RTE_MAX_ETHPORTS];
 
@@ -383,8 +387,6 @@  void vtpci_read_dev_config(struct virtio_hw *, size_t, void *, int);
 
 uint8_t vtpci_isr(struct virtio_hw *);
 
-enum virtio_msix_status vtpci_msix_detect(struct rte_pci_device *dev);
-
 extern const struct virtio_pci_ops legacy_ops;
 extern const struct virtio_pci_ops modern_ops;
 extern const struct virtio_pci_ops virtio_user_ops;
diff --git a/drivers/net/virtio/virtio_pci_ethdev.c b/drivers/net/virtio/virtio_pci_ethdev.c
index 6a0ef6edc3..045b134ef2 100644
--- a/drivers/net/virtio/virtio_pci_ethdev.c
+++ b/drivers/net/virtio/virtio_pci_ethdev.c
@@ -73,6 +73,8 @@  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;
 
+	VTPCI_DEV(hw) = pci_dev;
+
 	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
 		ret = vtpci_init(RTE_ETH_DEV_TO_PCI(eth_dev), hw);
 		if (ret) {