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
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
>
@@ -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;
}
@@ -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;
-}
@@ -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;
@@ -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) {