DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH v2 0/7] qede: SR-IOV PF driver support
@ 2020-07-13 15:13 Manish Chopra
  2020-07-13 15:13 ` [dpdk-dev] [PATCH v2 1/7] lib/librte_pci: add rte_pci_regs.h Manish Chopra
                   ` (6 more replies)
  0 siblings, 7 replies; 26+ messages in thread
From: Manish Chopra @ 2020-07-13 15:13 UTC (permalink / raw)
  To: jerinjacobk, jerinj, ferruh.yigit, grive
  Cc: dev, irusskikh, rmody, GR-Everest-DPDK-Dev, anatoly.burakov,
	xavier.huwei, humin29, yisen.zhuang, xiao.w.wang, qiming.yang,
	qi.z.zhang, heinrich.kuhn

Hi,

This series adds SR-IOV PF pmd driver support to have VF pmd
driver work over PF pmd driver instances in order to run the
adapter completely under DPDK environment for one of the use
cases like ovs-dpdk.

This is very initial bring-up with following testing covered -

* Enable/Disable SR-IOV VFs through igb_uio sysfs hook.
* Load VFs, run fastpath, teardown VFs in hypervisor and guest VM.
* VF FLR flow (in case of VF PCI passthrough to the guest VM)
* Bulletin mechanism tested to communicate link changes to the VFs.

Note that this series is intended for upcoming DPDK release (20.08)
Please consider applying this series to dpdk-next-net-mrvl.git

V1->V2: (Incorporated comments from Jerin Jacob)
================================================

* Added rte_pci_regs.h file (copy of linux/pci_regs.h) under
  lib/librte_pci to remove the dependency of dpdk on user headers

* Added generic API to find PCI extended capability and use
  that in the drivers, removed individual functions implemented
  by the drivers

Thanks,
Manish

Manish Chopra (7):
  lib/librte_pci: add rte_pci_regs.h
  drivers: add generic API to find PCI extended cap
  net/qede: define PCI config space specific osals
  net/qede: configure VFs on hardware
  net/qede: add infrastructure support for VF load
  net/qede: initialize VF MAC and link
  net/qede: add VF FLR support

 doc/guides/nics/features/qede.ini          |    1 +
 doc/guides/nics/qede.rst                   |    7 +-
 drivers/bus/pci/linux/pci_uio.c            |    2 +-
 drivers/bus/pci/linux/pci_vfio.c           |    2 +-
 drivers/bus/pci/pci_common.c               |   41 +
 drivers/bus/pci/rte_bus_pci.h              |   11 +
 drivers/net/bnx2x/bnx2x.h                  |    2 +-
 drivers/net/hns3/hns3_ethdev_vf.c          |    2 +-
 drivers/net/ice/ice_ethdev.c               |   53 +-
 drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c |   48 +-
 drivers/net/qede/Makefile                  |    1 +
 drivers/net/qede/base/bcm_osal.c           |   31 +
 drivers/net/qede/base/bcm_osal.h           |   33 +-
 drivers/net/qede/base/ecore.h              |    7 +
 drivers/net/qede/base/ecore_iov_api.h      |    3 +
 drivers/net/qede/meson.build               |    1 +
 drivers/net/qede/qede_ethdev.c             |   37 +-
 drivers/net/qede/qede_ethdev.h             |    1 +
 drivers/net/qede/qede_if.h                 |    1 +
 drivers/net/qede/qede_main.c               |   13 +-
 drivers/net/qede/qede_sriov.c              |  219 ++++
 drivers/net/qede/qede_sriov.h              |   22 +
 drivers/vdpa/ifc/base/ifcvf_osdep.h        |    2 +-
 lib/librte_pci/Makefile                    |    1 +
 lib/librte_pci/meson.build                 |    2 +-
 lib/librte_pci/rte_pci_regs.h              | 1075 ++++++++++++++++++++
 26 files changed, 1500 insertions(+), 118 deletions(-)
 create mode 100644 drivers/net/qede/qede_sriov.c
 create mode 100644 drivers/net/qede/qede_sriov.h
 create mode 100644 lib/librte_pci/rte_pci_regs.h

-- 
2.17.1


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [dpdk-dev] [PATCH v2 1/7] lib/librte_pci: add rte_pci_regs.h
  2020-07-13 15:13 [dpdk-dev] [PATCH v2 0/7] qede: SR-IOV PF driver support Manish Chopra
@ 2020-07-13 15:13 ` Manish Chopra
  2020-07-16  9:43   ` Gaëtan Rivet
  2020-07-16 10:08   ` Gaëtan Rivet
  2020-07-13 15:13 ` [dpdk-dev] [PATCH v2 2/7] drivers: add generic API to find PCI extended cap Manish Chopra
                   ` (5 subsequent siblings)
  6 siblings, 2 replies; 26+ messages in thread
From: Manish Chopra @ 2020-07-13 15:13 UTC (permalink / raw)
  To: jerinjacobk, jerinj, ferruh.yigit, grive
  Cc: dev, irusskikh, rmody, GR-Everest-DPDK-Dev, anatoly.burakov,
	xavier.huwei, humin29, yisen.zhuang, xiao.w.wang, qiming.yang,
	qi.z.zhang, heinrich.kuhn

This is merely copy of latest linux/pci_regs.h in
order to avoid dependency of dpdk on user headers.

Signed-off-by: Manish Chopra <manishc@marvell.com>
Signed-off-by: Igor Russkikh <irusskikh@marvell.com>
---
 drivers/bus/pci/linux/pci_uio.c     |    2 +-
 drivers/bus/pci/linux/pci_vfio.c    |    2 +-
 drivers/net/bnx2x/bnx2x.h           |    2 +-
 drivers/net/hns3/hns3_ethdev_vf.c   |    2 +-
 drivers/vdpa/ifc/base/ifcvf_osdep.h |    2 +-
 lib/librte_pci/Makefile             |    1 +
 lib/librte_pci/meson.build          |    2 +-
 lib/librte_pci/rte_pci_regs.h       | 1075 +++++++++++++++++++++++++++
 8 files changed, 1082 insertions(+), 6 deletions(-)
 create mode 100644 lib/librte_pci/rte_pci_regs.h

diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
index b62200153..698eace00 100644
--- a/drivers/bus/pci/linux/pci_uio.c
+++ b/drivers/bus/pci/linux/pci_uio.c
@@ -10,12 +10,12 @@
 #include <sys/stat.h>
 #include <sys/mman.h>
 #include <sys/sysmacros.h>
-#include <linux/pci_regs.h>
 
 #if defined(RTE_ARCH_X86)
 #include <sys/io.h>
 #endif
 
+#include <rte_pci_regs.h>
 #include <rte_string_fns.h>
 #include <rte_log.h>
 #include <rte_pci.h>
diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
index fdeb9a8ca..96494fcfd 100644
--- a/drivers/bus/pci/linux/pci_vfio.c
+++ b/drivers/bus/pci/linux/pci_vfio.c
@@ -4,13 +4,13 @@
 
 #include <string.h>
 #include <fcntl.h>
-#include <linux/pci_regs.h>
 #include <sys/eventfd.h>
 #include <sys/socket.h>
 #include <sys/ioctl.h>
 #include <sys/mman.h>
 #include <stdbool.h>
 
+#include <rte_pci_regs.h>
 #include <rte_log.h>
 #include <rte_pci.h>
 #include <rte_bus_pci.h>
diff --git a/drivers/net/bnx2x/bnx2x.h b/drivers/net/bnx2x/bnx2x.h
index 69cc1430a..4ec269dc6 100644
--- a/drivers/net/bnx2x/bnx2x.h
+++ b/drivers/net/bnx2x/bnx2x.h
@@ -18,6 +18,7 @@
 #include <rte_spinlock.h>
 #include <rte_bus_pci.h>
 #include <rte_io.h>
+#include <rte_pci_regs.h>
 
 #include "bnx2x_osal.h"
 #include "bnx2x_ethdev.h"
@@ -31,7 +32,6 @@
 #include "elink.h"
 
 #ifndef RTE_EXEC_ENV_FREEBSD
-#include <linux/pci_regs.h>
 
 #define PCIY_PMG                       PCI_CAP_ID_PM
 #define PCIY_MSI                       PCI_CAP_ID_MSI
diff --git a/drivers/net/hns3/hns3_ethdev_vf.c b/drivers/net/hns3/hns3_ethdev_vf.c
index 54e5dac52..81f9e7edc 100644
--- a/drivers/net/hns3/hns3_ethdev_vf.c
+++ b/drivers/net/hns3/hns3_ethdev_vf.c
@@ -9,8 +9,8 @@
 #include <inttypes.h>
 #include <unistd.h>
 #include <arpa/inet.h>
-#include <linux/pci_regs.h>
 
+#include <rte_pci_regs.h>
 #include <rte_alarm.h>
 #include <rte_atomic.h>
 #include <rte_bus_pci.h>
diff --git a/drivers/vdpa/ifc/base/ifcvf_osdep.h b/drivers/vdpa/ifc/base/ifcvf_osdep.h
index 6aef25ea4..0a759703d 100644
--- a/drivers/vdpa/ifc/base/ifcvf_osdep.h
+++ b/drivers/vdpa/ifc/base/ifcvf_osdep.h
@@ -6,8 +6,8 @@
 #define _IFCVF_OSDEP_H_
 
 #include <stdint.h>
-#include <linux/pci_regs.h>
 
+#include <rte_pci_regs.h>
 #include <rte_cycles.h>
 #include <rte_pci.h>
 #include <rte_bus_pci.h>
diff --git a/lib/librte_pci/Makefile b/lib/librte_pci/Makefile
index 7943f30ca..4d153b9ac 100644
--- a/lib/librte_pci/Makefile
+++ b/lib/librte_pci/Makefile
@@ -15,5 +15,6 @@ EXPORT_MAP := rte_pci_version.map
 SRCS-$(CONFIG_RTE_LIBRTE_PCI) += rte_pci.c
 
 SYMLINK-$(CONFIG_RTE_LIBRTE_PCI)-include += rte_pci.h
+SYMLINK-y-include += rte_pci_regs.h
 
 include $(RTE_SDK)/mk/rte.lib.mk
diff --git a/lib/librte_pci/meson.build b/lib/librte_pci/meson.build
index dd41cd506..5c7afbbfb 100644
--- a/lib/librte_pci/meson.build
+++ b/lib/librte_pci/meson.build
@@ -2,4 +2,4 @@
 # Copyright(c) 2017 Intel Corporation
 
 sources = files('rte_pci.c')
-headers = files('rte_pci.h')
+headers = files('rte_pci.h', 'rte_pci_regs.h')
diff --git a/lib/librte_pci/rte_pci_regs.h b/lib/librte_pci/rte_pci_regs.h
new file mode 100644
index 000000000..1d11f4de5
--- /dev/null
+++ b/lib/librte_pci/rte_pci_regs.h
@@ -0,0 +1,1075 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ *	PCI standard defines
+ *	Copyright 1994, Drew Eckhardt
+ *	Copyright 1997--1999 Martin Mares <mj@ucw.cz>
+ *
+ *	For more information, please consult the following manuals (look at
+ *	http://www.pcisig.com/ for how to get them):
+ *
+ *	PCI BIOS Specification
+ *	PCI Local Bus Specification
+ *	PCI to PCI Bridge Specification
+ *	PCI System Design Guide
+ *
+ *	For HyperTransport information, please consult the following manuals
+ *	from http://www.hypertransport.org :
+ *
+ *	The HyperTransport I/O Link Specification
+ */
+
+#ifndef _RTE_PCI_REGS_H_
+#define _RTE_PCI_REGS_H_
+
+/*
+ * Conventional PCI and PCI-X Mode 1 devices have 256 bytes of
+ * configuration space.  PCI-X Mode 2 and PCIe devices have 4096 bytes of
+ * configuration space.
+ */
+#define PCI_CFG_SPACE_SIZE	256
+#define PCI_CFG_SPACE_EXP_SIZE	4096
+
+/*
+ * Under PCI, each device has 256 bytes of configuration address space,
+ * of which the first 64 bytes are standardized as follows:
+ */
+#define PCI_STD_HEADER_SIZEOF	64
+#define PCI_STD_NUM_BARS	6	/* Number of standard BARs */
+#define PCI_VENDOR_ID		0x00	/* 16 bits */
+#define PCI_DEVICE_ID		0x02	/* 16 bits */
+#define PCI_COMMAND		0x04	/* 16 bits */
+#define  PCI_COMMAND_IO		0x1	/* Enable response in I/O space */
+#define  PCI_COMMAND_MEMORY	0x2	/* Enable response in Memory space */
+#define  PCI_COMMAND_MASTER	0x4	/* Enable bus mastering */
+#define  PCI_COMMAND_SPECIAL	0x8	/* Enable response to special cycles */
+#define  PCI_COMMAND_INVALIDATE	0x10	/* Use memory write and invalidate */
+#define  PCI_COMMAND_VGA_PALETTE 0x20	/* Enable palette snooping */
+#define  PCI_COMMAND_PARITY	0x40	/* Enable parity checking */
+#define  PCI_COMMAND_WAIT	0x80	/* Enable address/data stepping */
+#define  PCI_COMMAND_SERR	0x100	/* Enable SERR */
+#define  PCI_COMMAND_FAST_BACK	0x200	/* Enable back-to-back writes */
+#define  PCI_COMMAND_INTX_DISABLE 0x400 /* INTx Emulation Disable */
+
+#define PCI_STATUS		0x06	/* 16 bits */
+#define  PCI_STATUS_IMM_READY	0x01	/* Immediate Readiness */
+#define  PCI_STATUS_INTERRUPT	0x08	/* Interrupt status */
+#define  PCI_STATUS_CAP_LIST	0x10	/* Support Capability List */
+#define  PCI_STATUS_66MHZ	0x20	/* Support 66 MHz PCI 2.1 bus */
+#define  PCI_STATUS_UDF		0x40	/* Support User Definable Features [obsolete] */
+#define  PCI_STATUS_FAST_BACK	0x80	/* Accept fast-back to back */
+#define  PCI_STATUS_PARITY	0x100	/* Detected parity error */
+#define  PCI_STATUS_DEVSEL_MASK	0x600	/* DEVSEL timing */
+#define  PCI_STATUS_DEVSEL_FAST		0x000
+#define  PCI_STATUS_DEVSEL_MEDIUM	0x200
+#define  PCI_STATUS_DEVSEL_SLOW		0x400
+#define  PCI_STATUS_SIG_TARGET_ABORT	0x800 /* Set on target abort */
+#define  PCI_STATUS_REC_TARGET_ABORT	0x1000 /* Master ack of " */
+#define  PCI_STATUS_REC_MASTER_ABORT	0x2000 /* Set on master abort */
+#define  PCI_STATUS_SIG_SYSTEM_ERROR	0x4000 /* Set when we drive SERR */
+#define  PCI_STATUS_DETECTED_PARITY	0x8000 /* Set on parity error */
+
+#define PCI_CLASS_REVISION	0x08	/* High 24 bits are class, low 8 revision */
+#define PCI_REVISION_ID		0x08	/* Revision ID */
+#define PCI_CLASS_PROG		0x09	/* Reg. Level Programming Interface */
+#define PCI_CLASS_DEVICE	0x0a	/* Device class */
+
+#define PCI_CACHE_LINE_SIZE	0x0c	/* 8 bits */
+#define PCI_LATENCY_TIMER	0x0d	/* 8 bits */
+#define PCI_HEADER_TYPE		0x0e	/* 8 bits */
+#define  PCI_HEADER_TYPE_NORMAL		0
+#define  PCI_HEADER_TYPE_BRIDGE		1
+#define  PCI_HEADER_TYPE_CARDBUS	2
+
+#define PCI_BIST		0x0f	/* 8 bits */
+#define  PCI_BIST_CODE_MASK	0x0f	/* Return result */
+#define  PCI_BIST_START		0x40	/* 1 to start BIST, 2 secs or less */
+#define  PCI_BIST_CAPABLE	0x80	/* 1 if BIST capable */
+
+/*
+ * Base addresses specify locations in memory or I/O space.
+ * Decoded size can be determined by writing a value of
+ * 0xffffffff to the register, and reading it back.  Only
+ * 1 bits are decoded.
+ */
+#define PCI_BASE_ADDRESS_0	0x10	/* 32 bits */
+#define PCI_BASE_ADDRESS_1	0x14	/* 32 bits [htype 0,1 only] */
+#define PCI_BASE_ADDRESS_2	0x18	/* 32 bits [htype 0 only] */
+#define PCI_BASE_ADDRESS_3	0x1c	/* 32 bits */
+#define PCI_BASE_ADDRESS_4	0x20	/* 32 bits */
+#define PCI_BASE_ADDRESS_5	0x24	/* 32 bits */
+#define  PCI_BASE_ADDRESS_SPACE		0x01	/* 0 = memory, 1 = I/O */
+#define  PCI_BASE_ADDRESS_SPACE_IO	0x01
+#define  PCI_BASE_ADDRESS_SPACE_MEMORY	0x00
+#define  PCI_BASE_ADDRESS_MEM_TYPE_MASK	0x06
+#define  PCI_BASE_ADDRESS_MEM_TYPE_32	0x00	/* 32 bit address */
+#define  PCI_BASE_ADDRESS_MEM_TYPE_1M	0x02	/* Below 1M [obsolete] */
+#define  PCI_BASE_ADDRESS_MEM_TYPE_64	0x04	/* 64 bit address */
+#define  PCI_BASE_ADDRESS_MEM_PREFETCH	0x08	/* prefetchable? */
+#define  PCI_BASE_ADDRESS_MEM_MASK	(~0x0fUL)
+#define  PCI_BASE_ADDRESS_IO_MASK	(~0x03UL)
+/* bit 1 is reserved if address_space = 1 */
+
+/* Header type 0 (normal devices) */
+#define PCI_CARDBUS_CIS		0x28
+#define PCI_SUBSYSTEM_VENDOR_ID	0x2c
+#define PCI_SUBSYSTEM_ID	0x2e
+#define PCI_ROM_ADDRESS		0x30	/* Bits 31..11 are address, 10..1 reserved */
+#define  PCI_ROM_ADDRESS_ENABLE	0x01
+#define PCI_ROM_ADDRESS_MASK	(~0x7ffU)
+
+#define PCI_CAPABILITY_LIST	0x34	/* Offset of first capability list entry */
+
+/* 0x35-0x3b are reserved */
+#define PCI_INTERRUPT_LINE	0x3c	/* 8 bits */
+#define PCI_INTERRUPT_PIN	0x3d	/* 8 bits */
+#define PCI_MIN_GNT		0x3e	/* 8 bits */
+#define PCI_MAX_LAT		0x3f	/* 8 bits */
+
+/* Header type 1 (PCI-to-PCI bridges) */
+#define PCI_PRIMARY_BUS		0x18	/* Primary bus number */
+#define PCI_SECONDARY_BUS	0x19	/* Secondary bus number */
+#define PCI_SUBORDINATE_BUS	0x1a	/* Highest bus number behind the bridge */
+#define PCI_SEC_LATENCY_TIMER	0x1b	/* Latency timer for secondary interface */
+#define PCI_IO_BASE		0x1c	/* I/O range behind the bridge */
+#define PCI_IO_LIMIT		0x1d
+#define  PCI_IO_RANGE_TYPE_MASK	0x0fUL	/* I/O bridging type */
+#define  PCI_IO_RANGE_TYPE_16	0x00
+#define  PCI_IO_RANGE_TYPE_32	0x01
+#define  PCI_IO_RANGE_MASK	(~0x0fUL) /* Standard 4K I/O windows */
+#define  PCI_IO_1K_RANGE_MASK	(~0x03UL) /* Intel 1K I/O windows */
+#define PCI_SEC_STATUS		0x1e	/* Secondary status register, only bit 14 used */
+#define PCI_MEMORY_BASE		0x20	/* Memory range behind */
+#define PCI_MEMORY_LIMIT	0x22
+#define  PCI_MEMORY_RANGE_TYPE_MASK 0x0fUL
+#define  PCI_MEMORY_RANGE_MASK	(~0x0fUL)
+#define PCI_PREF_MEMORY_BASE	0x24	/* Prefetchable memory range behind */
+#define PCI_PREF_MEMORY_LIMIT	0x26
+#define  PCI_PREF_RANGE_TYPE_MASK 0x0fUL
+#define  PCI_PREF_RANGE_TYPE_32	0x00
+#define  PCI_PREF_RANGE_TYPE_64	0x01
+#define  PCI_PREF_RANGE_MASK	(~0x0fUL)
+#define PCI_PREF_BASE_UPPER32	0x28	/* Upper half of prefetchable memory range */
+#define PCI_PREF_LIMIT_UPPER32	0x2c
+#define PCI_IO_BASE_UPPER16	0x30	/* Upper half of I/O addresses */
+#define PCI_IO_LIMIT_UPPER16	0x32
+/* 0x34 same as for htype 0 */
+/* 0x35-0x3b is reserved */
+#define PCI_ROM_ADDRESS1	0x38	/* Same as PCI_ROM_ADDRESS, but for htype 1 */
+/* 0x3c-0x3d are same as for htype 0 */
+#define PCI_BRIDGE_CONTROL	0x3e
+#define  PCI_BRIDGE_CTL_PARITY	0x01	/* Enable parity detection on secondary interface */
+#define  PCI_BRIDGE_CTL_SERR	0x02	/* The same for SERR forwarding */
+#define  PCI_BRIDGE_CTL_ISA	0x04	/* Enable ISA mode */
+#define  PCI_BRIDGE_CTL_VGA	0x08	/* Forward VGA addresses */
+#define  PCI_BRIDGE_CTL_MASTER_ABORT	0x20  /* Report master aborts */
+#define  PCI_BRIDGE_CTL_BUS_RESET	0x40	/* Secondary bus reset */
+#define  PCI_BRIDGE_CTL_FAST_BACK	0x80	/* Fast Back2Back enabled on secondary interface */
+
+/* Header type 2 (CardBus bridges) */
+#define PCI_CB_CAPABILITY_LIST	0x14
+/* 0x15 reserved */
+#define PCI_CB_SEC_STATUS	0x16	/* Secondary status */
+#define PCI_CB_PRIMARY_BUS	0x18	/* PCI bus number */
+#define PCI_CB_CARD_BUS		0x19	/* CardBus bus number */
+#define PCI_CB_SUBORDINATE_BUS	0x1a	/* Subordinate bus number */
+#define PCI_CB_LATENCY_TIMER	0x1b	/* CardBus latency timer */
+#define PCI_CB_MEMORY_BASE_0	0x1c
+#define PCI_CB_MEMORY_LIMIT_0	0x20
+#define PCI_CB_MEMORY_BASE_1	0x24
+#define PCI_CB_MEMORY_LIMIT_1	0x28
+#define PCI_CB_IO_BASE_0	0x2c
+#define PCI_CB_IO_BASE_0_HI	0x2e
+#define PCI_CB_IO_LIMIT_0	0x30
+#define PCI_CB_IO_LIMIT_0_HI	0x32
+#define PCI_CB_IO_BASE_1	0x34
+#define PCI_CB_IO_BASE_1_HI	0x36
+#define PCI_CB_IO_LIMIT_1	0x38
+#define PCI_CB_IO_LIMIT_1_HI	0x3a
+#define  PCI_CB_IO_RANGE_MASK	(~0x03UL)
+/* 0x3c-0x3d are same as for htype 0 */
+#define PCI_CB_BRIDGE_CONTROL	0x3e
+#define  PCI_CB_BRIDGE_CTL_PARITY	0x01	/* Similar to standard bridge control register */
+#define  PCI_CB_BRIDGE_CTL_SERR		0x02
+#define  PCI_CB_BRIDGE_CTL_ISA		0x04
+#define  PCI_CB_BRIDGE_CTL_VGA		0x08
+#define  PCI_CB_BRIDGE_CTL_MASTER_ABORT	0x20
+#define  PCI_CB_BRIDGE_CTL_CB_RESET	0x40	/* CardBus reset */
+#define  PCI_CB_BRIDGE_CTL_16BIT_INT	0x80	/* Enable interrupt for 16-bit cards */
+#define  PCI_CB_BRIDGE_CTL_PREFETCH_MEM0 0x100	/* Prefetch enable for both memory regions */
+#define  PCI_CB_BRIDGE_CTL_PREFETCH_MEM1 0x200
+#define  PCI_CB_BRIDGE_CTL_POST_WRITES	0x400
+#define PCI_CB_SUBSYSTEM_VENDOR_ID	0x40
+#define PCI_CB_SUBSYSTEM_ID		0x42
+#define PCI_CB_LEGACY_MODE_BASE		0x44	/* 16-bit PC Card legacy mode base address (ExCa) */
+/* 0x48-0x7f reserved */
+
+/* Capability lists */
+
+#define PCI_CAP_LIST_ID		0	/* Capability ID */
+#define  PCI_CAP_ID_PM		0x01	/* Power Management */
+#define  PCI_CAP_ID_AGP		0x02	/* Accelerated Graphics Port */
+#define  PCI_CAP_ID_VPD		0x03	/* Vital Product Data */
+#define  PCI_CAP_ID_SLOTID	0x04	/* Slot Identification */
+#define  PCI_CAP_ID_MSI		0x05	/* Message Signalled Interrupts */
+#define  PCI_CAP_ID_CHSWP	0x06	/* CompactPCI HotSwap */
+#define  PCI_CAP_ID_PCIX	0x07	/* PCI-X */
+#define  PCI_CAP_ID_HT		0x08	/* HyperTransport */
+#define  PCI_CAP_ID_VNDR	0x09	/* Vendor-Specific */
+#define  PCI_CAP_ID_DBG		0x0A	/* Debug port */
+#define  PCI_CAP_ID_CCRC	0x0B	/* CompactPCI Central Resource Control */
+#define  PCI_CAP_ID_SHPC	0x0C	/* PCI Standard Hot-Plug Controller */
+#define  PCI_CAP_ID_SSVID	0x0D	/* Bridge subsystem vendor/device ID */
+#define  PCI_CAP_ID_AGP3	0x0E	/* AGP Target PCI-PCI bridge */
+#define  PCI_CAP_ID_SECDEV	0x0F	/* Secure Device */
+#define  PCI_CAP_ID_EXP		0x10	/* PCI Express */
+#define  PCI_CAP_ID_MSIX	0x11	/* MSI-X */
+#define  PCI_CAP_ID_SATA	0x12	/* SATA Data/Index Conf. */
+#define  PCI_CAP_ID_AF		0x13	/* PCI Advanced Features */
+#define  PCI_CAP_ID_EA		0x14	/* PCI Enhanced Allocation */
+#define  PCI_CAP_ID_MAX		PCI_CAP_ID_EA
+#define PCI_CAP_LIST_NEXT	1	/* Next capability in the list */
+#define PCI_CAP_FLAGS		2	/* Capability defined flags (16 bits) */
+#define PCI_CAP_SIZEOF		4
+
+/* Power Management Registers */
+
+#define PCI_PM_PMC		2	/* PM Capabilities Register */
+#define  PCI_PM_CAP_VER_MASK	0x0007	/* Version */
+#define  PCI_PM_CAP_PME_CLOCK	0x0008	/* PME clock required */
+#define  PCI_PM_CAP_RESERVED    0x0010  /* Reserved field */
+#define  PCI_PM_CAP_DSI		0x0020	/* Device specific initialization */
+#define  PCI_PM_CAP_AUX_POWER	0x01C0	/* Auxiliary power support mask */
+#define  PCI_PM_CAP_D1		0x0200	/* D1 power state support */
+#define  PCI_PM_CAP_D2		0x0400	/* D2 power state support */
+#define  PCI_PM_CAP_PME		0x0800	/* PME pin supported */
+#define  PCI_PM_CAP_PME_MASK	0xF800	/* PME Mask of all supported states */
+#define  PCI_PM_CAP_PME_D0	0x0800	/* PME# from D0 */
+#define  PCI_PM_CAP_PME_D1	0x1000	/* PME# from D1 */
+#define  PCI_PM_CAP_PME_D2	0x2000	/* PME# from D2 */
+#define  PCI_PM_CAP_PME_D3	0x4000	/* PME# from D3 (hot) */
+#define  PCI_PM_CAP_PME_D3cold	0x8000	/* PME# from D3 (cold) */
+#define  PCI_PM_CAP_PME_SHIFT	11	/* Start of the PME Mask in PMC */
+#define PCI_PM_CTRL		4	/* PM control and status register */
+#define  PCI_PM_CTRL_STATE_MASK	0x0003	/* Current power state (D0 to D3) */
+#define  PCI_PM_CTRL_NO_SOFT_RESET	0x0008	/* No reset for D3hot->D0 */
+#define  PCI_PM_CTRL_PME_ENABLE	0x0100	/* PME pin enable */
+#define  PCI_PM_CTRL_DATA_SEL_MASK	0x1e00	/* Data select (??) */
+#define  PCI_PM_CTRL_DATA_SCALE_MASK	0x6000	/* Data scale (??) */
+#define  PCI_PM_CTRL_PME_STATUS	0x8000	/* PME pin status */
+#define PCI_PM_PPB_EXTENSIONS	6	/* PPB support extensions (??) */
+#define  PCI_PM_PPB_B2_B3	0x40	/* Stop clock when in D3hot (??) */
+#define  PCI_PM_BPCC_ENABLE	0x80	/* Bus power/clock control enable (??) */
+#define PCI_PM_DATA_REGISTER	7	/* (??) */
+#define PCI_PM_SIZEOF		8
+
+/* AGP registers */
+
+#define PCI_AGP_VERSION		2	/* BCD version number */
+#define PCI_AGP_RFU		3	/* Rest of capability flags */
+#define PCI_AGP_STATUS		4	/* Status register */
+#define  PCI_AGP_STATUS_RQ_MASK	0xff000000	/* Maximum number of requests - 1 */
+#define  PCI_AGP_STATUS_SBA	0x0200	/* Sideband addressing supported */
+#define  PCI_AGP_STATUS_64BIT	0x0020	/* 64-bit addressing supported */
+#define  PCI_AGP_STATUS_FW	0x0010	/* FW transfers supported */
+#define  PCI_AGP_STATUS_RATE4	0x0004	/* 4x transfer rate supported */
+#define  PCI_AGP_STATUS_RATE2	0x0002	/* 2x transfer rate supported */
+#define  PCI_AGP_STATUS_RATE1	0x0001	/* 1x transfer rate supported */
+#define PCI_AGP_COMMAND		8	/* Control register */
+#define  PCI_AGP_COMMAND_RQ_MASK 0xff000000  /* Master: Maximum number of requests */
+#define  PCI_AGP_COMMAND_SBA	0x0200	/* Sideband addressing enabled */
+#define  PCI_AGP_COMMAND_AGP	0x0100	/* Allow processing of AGP transactions */
+#define  PCI_AGP_COMMAND_64BIT	0x0020	/* Allow processing of 64-bit addresses */
+#define  PCI_AGP_COMMAND_FW	0x0010	/* Force FW transfers */
+#define  PCI_AGP_COMMAND_RATE4	0x0004	/* Use 4x rate */
+#define  PCI_AGP_COMMAND_RATE2	0x0002	/* Use 2x rate */
+#define  PCI_AGP_COMMAND_RATE1	0x0001	/* Use 1x rate */
+#define PCI_AGP_SIZEOF		12
+
+/* Vital Product Data */
+
+#define PCI_VPD_ADDR		2	/* Address to access (15 bits!) */
+#define  PCI_VPD_ADDR_MASK	0x7fff	/* Address mask */
+#define  PCI_VPD_ADDR_F		0x8000	/* Write 0, 1 indicates completion */
+#define PCI_VPD_DATA		4	/* 32-bits of data returned here */
+#define PCI_CAP_VPD_SIZEOF	8
+
+/* Slot Identification */
+
+#define PCI_SID_ESR		2	/* Expansion Slot Register */
+#define  PCI_SID_ESR_NSLOTS	0x1f	/* Number of expansion slots available */
+#define  PCI_SID_ESR_FIC	0x20	/* First In Chassis Flag */
+#define PCI_SID_CHASSIS_NR	3	/* Chassis Number */
+
+/* Message Signalled Interrupt registers */
+
+#define PCI_MSI_FLAGS		2	/* Message Control */
+#define  PCI_MSI_FLAGS_ENABLE	0x0001	/* MSI feature enabled */
+#define  PCI_MSI_FLAGS_QMASK	0x000e	/* Maximum queue size available */
+#define  PCI_MSI_FLAGS_QSIZE	0x0070	/* Message queue size configured */
+#define  PCI_MSI_FLAGS_64BIT	0x0080	/* 64-bit addresses allowed */
+#define  PCI_MSI_FLAGS_MASKBIT	0x0100	/* Per-vector masking capable */
+#define PCI_MSI_RFU		3	/* Rest of capability flags */
+#define PCI_MSI_ADDRESS_LO	4	/* Lower 32 bits */
+#define PCI_MSI_ADDRESS_HI	8	/* Upper 32 bits (if PCI_MSI_FLAGS_64BIT set) */
+#define PCI_MSI_DATA_32		8	/* 16 bits of data for 32-bit devices */
+#define PCI_MSI_MASK_32		12	/* Mask bits register for 32-bit devices */
+#define PCI_MSI_PENDING_32	16	/* Pending intrs for 32-bit devices */
+#define PCI_MSI_DATA_64		12	/* 16 bits of data for 64-bit devices */
+#define PCI_MSI_MASK_64		16	/* Mask bits register for 64-bit devices */
+#define PCI_MSI_PENDING_64	20	/* Pending intrs for 64-bit devices */
+
+/* MSI-X registers (in MSI-X capability) */
+#define PCI_MSIX_FLAGS		2	/* Message Control */
+#define  PCI_MSIX_FLAGS_QSIZE	0x07FF	/* Table size */
+#define  PCI_MSIX_FLAGS_MASKALL	0x4000	/* Mask all vectors for this function */
+#define  PCI_MSIX_FLAGS_ENABLE	0x8000	/* MSI-X enable */
+#define PCI_MSIX_TABLE		4	/* Table offset */
+#define  PCI_MSIX_TABLE_BIR	0x00000007 /* BAR index */
+#define  PCI_MSIX_TABLE_OFFSET	0xfffffff8 /* Offset into specified BAR */
+#define PCI_MSIX_PBA		8	/* Pending Bit Array offset */
+#define  PCI_MSIX_PBA_BIR	0x00000007 /* BAR index */
+#define  PCI_MSIX_PBA_OFFSET	0xfffffff8 /* Offset into specified BAR */
+#define PCI_MSIX_FLAGS_BIRMASK	PCI_MSIX_PBA_BIR /* deprecated */
+#define PCI_CAP_MSIX_SIZEOF	12	/* size of MSIX registers */
+
+/* MSI-X Table entry format (in memory mapped by a BAR) */
+#define PCI_MSIX_ENTRY_SIZE		16
+#define PCI_MSIX_ENTRY_LOWER_ADDR	0  /* Message Address */
+#define PCI_MSIX_ENTRY_UPPER_ADDR	4  /* Message Upper Address */
+#define PCI_MSIX_ENTRY_DATA		8  /* Message Data */
+#define PCI_MSIX_ENTRY_VECTOR_CTRL	12 /* Vector Control */
+#define  PCI_MSIX_ENTRY_CTRL_MASKBIT	0x00000001
+
+/* CompactPCI Hotswap Register */
+
+#define PCI_CHSWP_CSR		2	/* Control and Status Register */
+#define  PCI_CHSWP_DHA		0x01	/* Device Hiding Arm */
+#define  PCI_CHSWP_EIM		0x02	/* ENUM# Signal Mask */
+#define  PCI_CHSWP_PIE		0x04	/* Pending Insert or Extract */
+#define  PCI_CHSWP_LOO		0x08	/* LED On / Off */
+#define  PCI_CHSWP_PI		0x30	/* Programming Interface */
+#define  PCI_CHSWP_EXT		0x40	/* ENUM# status - extraction */
+#define  PCI_CHSWP_INS		0x80	/* ENUM# status - insertion */
+
+/* PCI Advanced Feature registers */
+
+#define PCI_AF_LENGTH		2
+#define PCI_AF_CAP		3
+#define  PCI_AF_CAP_TP		0x01
+#define  PCI_AF_CAP_FLR		0x02
+#define PCI_AF_CTRL		4
+#define  PCI_AF_CTRL_FLR	0x01
+#define PCI_AF_STATUS		5
+#define  PCI_AF_STATUS_TP	0x01
+#define PCI_CAP_AF_SIZEOF	6	/* size of AF registers */
+
+/* PCI Enhanced Allocation registers */
+
+#define PCI_EA_NUM_ENT		2	/* Number of Capability Entries */
+#define  PCI_EA_NUM_ENT_MASK	0x3f	/* Num Entries Mask */
+#define PCI_EA_FIRST_ENT	4	/* First EA Entry in List */
+#define PCI_EA_FIRST_ENT_BRIDGE	8	/* First EA Entry for Bridges */
+#define  PCI_EA_ES		0x00000007 /* Entry Size */
+#define  PCI_EA_BEI		0x000000f0 /* BAR Equivalent Indicator */
+
+/* EA fixed Secondary and Subordinate bus numbers for Bridge */
+#define PCI_EA_SEC_BUS_MASK	0xff
+#define PCI_EA_SUB_BUS_MASK	0xff00
+#define PCI_EA_SUB_BUS_SHIFT	8
+
+/* 0-5 map to BARs 0-5 respectively */
+#define   PCI_EA_BEI_BAR0		0
+#define   PCI_EA_BEI_BAR5		5
+#define   PCI_EA_BEI_BRIDGE		6	/* Resource behind bridge */
+#define   PCI_EA_BEI_ENI		7	/* Equivalent Not Indicated */
+#define   PCI_EA_BEI_ROM		8	/* Expansion ROM */
+/* 9-14 map to VF BARs 0-5 respectively */
+#define   PCI_EA_BEI_VF_BAR0		9
+#define   PCI_EA_BEI_VF_BAR5		14
+#define   PCI_EA_BEI_RESERVED		15	/* Reserved - Treat like ENI */
+#define  PCI_EA_PP		0x0000ff00	/* Primary Properties */
+#define  PCI_EA_SP		0x00ff0000	/* Secondary Properties */
+#define   PCI_EA_P_MEM			0x00	/* Non-Prefetch Memory */
+#define   PCI_EA_P_MEM_PREFETCH		0x01	/* Prefetchable Memory */
+#define   PCI_EA_P_IO			0x02	/* I/O Space */
+#define   PCI_EA_P_VF_MEM_PREFETCH	0x03	/* VF Prefetchable Memory */
+#define   PCI_EA_P_VF_MEM		0x04	/* VF Non-Prefetch Memory */
+#define   PCI_EA_P_BRIDGE_MEM		0x05	/* Bridge Non-Prefetch Memory */
+#define   PCI_EA_P_BRIDGE_MEM_PREFETCH	0x06	/* Bridge Prefetchable Memory */
+#define   PCI_EA_P_BRIDGE_IO		0x07	/* Bridge I/O Space */
+/* 0x08-0xfc reserved */
+#define   PCI_EA_P_MEM_RESERVED		0xfd	/* Reserved Memory */
+#define   PCI_EA_P_IO_RESERVED		0xfe	/* Reserved I/O Space */
+#define   PCI_EA_P_UNAVAILABLE		0xff	/* Entry Unavailable */
+#define  PCI_EA_WRITABLE	0x40000000	/* Writable: 1 = RW, 0 = HwInit */
+#define  PCI_EA_ENABLE		0x80000000	/* Enable for this entry */
+#define PCI_EA_BASE		4		/* Base Address Offset */
+#define PCI_EA_MAX_OFFSET	8		/* MaxOffset (resource length) */
+/* bit 0 is reserved */
+#define  PCI_EA_IS_64		0x00000002	/* 64-bit field flag */
+#define  PCI_EA_FIELD_MASK	0xfffffffc	/* For Base & Max Offset */
+
+/* PCI-X registers (Type 0 (non-bridge) devices) */
+
+#define PCI_X_CMD		2	/* Modes & Features */
+#define  PCI_X_CMD_DPERR_E	0x0001	/* Data Parity Error Recovery Enable */
+#define  PCI_X_CMD_ERO		0x0002	/* Enable Relaxed Ordering */
+#define  PCI_X_CMD_READ_512	0x0000	/* 512 byte maximum read byte count */
+#define  PCI_X_CMD_READ_1K	0x0004	/* 1Kbyte maximum read byte count */
+#define  PCI_X_CMD_READ_2K	0x0008	/* 2Kbyte maximum read byte count */
+#define  PCI_X_CMD_READ_4K	0x000c	/* 4Kbyte maximum read byte count */
+#define  PCI_X_CMD_MAX_READ	0x000c	/* Max Memory Read Byte Count */
+				/* Max # of outstanding split transactions */
+#define  PCI_X_CMD_SPLIT_1	0x0000	/* Max 1 */
+#define  PCI_X_CMD_SPLIT_2	0x0010	/* Max 2 */
+#define  PCI_X_CMD_SPLIT_3	0x0020	/* Max 3 */
+#define  PCI_X_CMD_SPLIT_4	0x0030	/* Max 4 */
+#define  PCI_X_CMD_SPLIT_8	0x0040	/* Max 8 */
+#define  PCI_X_CMD_SPLIT_12	0x0050	/* Max 12 */
+#define  PCI_X_CMD_SPLIT_16	0x0060	/* Max 16 */
+#define  PCI_X_CMD_SPLIT_32	0x0070	/* Max 32 */
+#define  PCI_X_CMD_MAX_SPLIT	0x0070	/* Max Outstanding Split Transactions */
+#define  PCI_X_CMD_VERSION(x)	(((x) >> 12) & 3) /* Version */
+#define PCI_X_STATUS		4	/* PCI-X capabilities */
+#define  PCI_X_STATUS_DEVFN	0x000000ff	/* A copy of devfn */
+#define  PCI_X_STATUS_BUS	0x0000ff00	/* A copy of bus nr */
+#define  PCI_X_STATUS_64BIT	0x00010000	/* 64-bit device */
+#define  PCI_X_STATUS_133MHZ	0x00020000	/* 133 MHz capable */
+#define  PCI_X_STATUS_SPL_DISC	0x00040000	/* Split Completion Discarded */
+#define  PCI_X_STATUS_UNX_SPL	0x00080000	/* Unexpected Split Completion */
+#define  PCI_X_STATUS_COMPLEX	0x00100000	/* Device Complexity */
+#define  PCI_X_STATUS_MAX_READ	0x00600000	/* Designed Max Memory Read Count */
+#define  PCI_X_STATUS_MAX_SPLIT	0x03800000	/* Designed Max Outstanding Split Transactions */
+#define  PCI_X_STATUS_MAX_CUM	0x1c000000	/* Designed Max Cumulative Read Size */
+#define  PCI_X_STATUS_SPL_ERR	0x20000000	/* Rcvd Split Completion Error Msg */
+#define  PCI_X_STATUS_266MHZ	0x40000000	/* 266 MHz capable */
+#define  PCI_X_STATUS_533MHZ	0x80000000	/* 533 MHz capable */
+#define PCI_X_ECC_CSR		8	/* ECC control and status */
+#define PCI_CAP_PCIX_SIZEOF_V0	8	/* size of registers for Version 0 */
+#define PCI_CAP_PCIX_SIZEOF_V1	24	/* size for Version 1 */
+#define PCI_CAP_PCIX_SIZEOF_V2	PCI_CAP_PCIX_SIZEOF_V1	/* Same for v2 */
+
+/* PCI-X registers (Type 1 (bridge) devices) */
+
+#define PCI_X_BRIDGE_SSTATUS	2	/* Secondary Status */
+#define  PCI_X_SSTATUS_64BIT	0x0001	/* Secondary AD interface is 64 bits */
+#define  PCI_X_SSTATUS_133MHZ	0x0002	/* 133 MHz capable */
+#define  PCI_X_SSTATUS_FREQ	0x03c0	/* Secondary Bus Mode and Frequency */
+#define  PCI_X_SSTATUS_VERS	0x3000	/* PCI-X Capability Version */
+#define  PCI_X_SSTATUS_V1	0x1000	/* Mode 2, not Mode 1 */
+#define  PCI_X_SSTATUS_V2	0x2000	/* Mode 1 or Modes 1 and 2 */
+#define  PCI_X_SSTATUS_266MHZ	0x4000	/* 266 MHz capable */
+#define  PCI_X_SSTATUS_533MHZ	0x8000	/* 533 MHz capable */
+#define PCI_X_BRIDGE_STATUS	4	/* Bridge Status */
+
+/* PCI Bridge Subsystem ID registers */
+
+#define PCI_SSVID_VENDOR_ID     4	/* PCI Bridge subsystem vendor ID */
+#define PCI_SSVID_DEVICE_ID     6	/* PCI Bridge subsystem device ID */
+
+/* PCI Express capability registers */
+
+#define PCI_EXP_FLAGS		2	/* Capabilities register */
+#define  PCI_EXP_FLAGS_VERS	0x000f	/* Capability version */
+#define  PCI_EXP_FLAGS_TYPE	0x00f0	/* Device/Port type */
+#define   PCI_EXP_TYPE_ENDPOINT	   0x0	/* Express Endpoint */
+#define   PCI_EXP_TYPE_LEG_END	   0x1	/* Legacy Endpoint */
+#define   PCI_EXP_TYPE_ROOT_PORT   0x4	/* Root Port */
+#define   PCI_EXP_TYPE_UPSTREAM	   0x5	/* Upstream Port */
+#define   PCI_EXP_TYPE_DOWNSTREAM  0x6	/* Downstream Port */
+#define   PCI_EXP_TYPE_PCI_BRIDGE  0x7	/* PCIe to PCI/PCI-X Bridge */
+#define   PCI_EXP_TYPE_PCIE_BRIDGE 0x8	/* PCI/PCI-X to PCIe Bridge */
+#define   PCI_EXP_TYPE_RC_END	   0x9	/* Root Complex Integrated Endpoint */
+#define   PCI_EXP_TYPE_RC_EC	   0xa	/* Root Complex Event Collector */
+#define  PCI_EXP_FLAGS_SLOT	0x0100	/* Slot implemented */
+#define  PCI_EXP_FLAGS_IRQ	0x3e00	/* Interrupt message number */
+#define PCI_EXP_DEVCAP		4	/* Device capabilities */
+#define  PCI_EXP_DEVCAP_PAYLOAD	0x00000007 /* Max_Payload_Size */
+#define  PCI_EXP_DEVCAP_PHANTOM	0x00000018 /* Phantom functions */
+#define  PCI_EXP_DEVCAP_EXT_TAG	0x00000020 /* Extended tags */
+#define  PCI_EXP_DEVCAP_L0S	0x000001c0 /* L0s Acceptable Latency */
+#define  PCI_EXP_DEVCAP_L1	0x00000e00 /* L1 Acceptable Latency */
+#define  PCI_EXP_DEVCAP_ATN_BUT	0x00001000 /* Attention Button Present */
+#define  PCI_EXP_DEVCAP_ATN_IND	0x00002000 /* Attention Indicator Present */
+#define  PCI_EXP_DEVCAP_PWR_IND	0x00004000 /* Power Indicator Present */
+#define  PCI_EXP_DEVCAP_RBER	0x00008000 /* Role-Based Error Reporting */
+#define  PCI_EXP_DEVCAP_PWR_VAL	0x03fc0000 /* Slot Power Limit Value */
+#define  PCI_EXP_DEVCAP_PWR_SCL	0x0c000000 /* Slot Power Limit Scale */
+#define  PCI_EXP_DEVCAP_FLR     0x10000000 /* Function Level Reset */
+#define PCI_EXP_DEVCTL		8	/* Device Control */
+#define  PCI_EXP_DEVCTL_CERE	0x0001	/* Correctable Error Reporting En. */
+#define  PCI_EXP_DEVCTL_NFERE	0x0002	/* Non-Fatal Error Reporting Enable */
+#define  PCI_EXP_DEVCTL_FERE	0x0004	/* Fatal Error Reporting Enable */
+#define  PCI_EXP_DEVCTL_URRE	0x0008	/* Unsupported Request Reporting En. */
+#define  PCI_EXP_DEVCTL_RELAX_EN 0x0010 /* Enable relaxed ordering */
+#define  PCI_EXP_DEVCTL_PAYLOAD	0x00e0	/* Max_Payload_Size */
+#define  PCI_EXP_DEVCTL_EXT_TAG	0x0100	/* Extended Tag Field Enable */
+#define  PCI_EXP_DEVCTL_PHANTOM	0x0200	/* Phantom Functions Enable */
+#define  PCI_EXP_DEVCTL_AUX_PME	0x0400	/* Auxiliary Power PM Enable */
+#define  PCI_EXP_DEVCTL_NOSNOOP_EN 0x0800  /* Enable No Snoop */
+#define  PCI_EXP_DEVCTL_READRQ	0x7000	/* Max_Read_Request_Size */
+#define  PCI_EXP_DEVCTL_READRQ_128B  0x0000 /* 128 Bytes */
+#define  PCI_EXP_DEVCTL_READRQ_256B  0x1000 /* 256 Bytes */
+#define  PCI_EXP_DEVCTL_READRQ_512B  0x2000 /* 512 Bytes */
+#define  PCI_EXP_DEVCTL_READRQ_1024B 0x3000 /* 1024 Bytes */
+#define  PCI_EXP_DEVCTL_READRQ_2048B 0x4000 /* 2048 Bytes */
+#define  PCI_EXP_DEVCTL_READRQ_4096B 0x5000 /* 4096 Bytes */
+#define  PCI_EXP_DEVCTL_BCR_FLR 0x8000  /* Bridge Configuration Retry / FLR */
+#define PCI_EXP_DEVSTA		10	/* Device Status */
+#define  PCI_EXP_DEVSTA_CED	0x0001	/* Correctable Error Detected */
+#define  PCI_EXP_DEVSTA_NFED	0x0002	/* Non-Fatal Error Detected */
+#define  PCI_EXP_DEVSTA_FED	0x0004	/* Fatal Error Detected */
+#define  PCI_EXP_DEVSTA_URD	0x0008	/* Unsupported Request Detected */
+#define  PCI_EXP_DEVSTA_AUXPD	0x0010	/* AUX Power Detected */
+#define  PCI_EXP_DEVSTA_TRPND	0x0020	/* Transactions Pending */
+#define PCI_CAP_EXP_RC_ENDPOINT_SIZEOF_V1	12	/* v1 endpoints without link end here */
+#define PCI_EXP_LNKCAP		12	/* Link Capabilities */
+#define  PCI_EXP_LNKCAP_SLS	0x0000000f /* Supported Link Speeds */
+#define  PCI_EXP_LNKCAP_SLS_2_5GB 0x00000001 /* LNKCAP2 SLS Vector bit 0 */
+#define  PCI_EXP_LNKCAP_SLS_5_0GB 0x00000002 /* LNKCAP2 SLS Vector bit 1 */
+#define  PCI_EXP_LNKCAP_SLS_8_0GB 0x00000003 /* LNKCAP2 SLS Vector bit 2 */
+#define  PCI_EXP_LNKCAP_SLS_16_0GB 0x00000004 /* LNKCAP2 SLS Vector bit 3 */
+#define  PCI_EXP_LNKCAP_SLS_32_0GB 0x00000005 /* LNKCAP2 SLS Vector bit 4 */
+#define  PCI_EXP_LNKCAP_MLW	0x000003f0 /* Maximum Link Width */
+#define  PCI_EXP_LNKCAP_ASPMS	0x00000c00 /* ASPM Support */
+#define  PCI_EXP_LNKCAP_L0SEL	0x00007000 /* L0s Exit Latency */
+#define  PCI_EXP_LNKCAP_L1EL	0x00038000 /* L1 Exit Latency */
+#define  PCI_EXP_LNKCAP_CLKPM	0x00040000 /* Clock Power Management */
+#define  PCI_EXP_LNKCAP_SDERC	0x00080000 /* Surprise Down Error Reporting Capable */
+#define  PCI_EXP_LNKCAP_DLLLARC	0x00100000 /* Data Link Layer Link Active Reporting Capable */
+#define  PCI_EXP_LNKCAP_LBNC	0x00200000 /* Link Bandwidth Notification Capability */
+#define  PCI_EXP_LNKCAP_PN	0xff000000 /* Port Number */
+#define PCI_EXP_LNKCTL		16	/* Link Control */
+#define  PCI_EXP_LNKCTL_ASPMC	0x0003	/* ASPM Control */
+#define  PCI_EXP_LNKCTL_ASPM_L0S 0x0001	/* L0s Enable */
+#define  PCI_EXP_LNKCTL_ASPM_L1  0x0002	/* L1 Enable */
+#define  PCI_EXP_LNKCTL_RCB	0x0008	/* Read Completion Boundary */
+#define  PCI_EXP_LNKCTL_LD	0x0010	/* Link Disable */
+#define  PCI_EXP_LNKCTL_RL	0x0020	/* Retrain Link */
+#define  PCI_EXP_LNKCTL_CCC	0x0040	/* Common Clock Configuration */
+#define  PCI_EXP_LNKCTL_ES	0x0080	/* Extended Synch */
+#define  PCI_EXP_LNKCTL_CLKREQ_EN 0x0100 /* Enable clkreq */
+#define  PCI_EXP_LNKCTL_HAWD	0x0200	/* Hardware Autonomous Width Disable */
+#define  PCI_EXP_LNKCTL_LBMIE	0x0400	/* Link Bandwidth Management Interrupt Enable */
+#define  PCI_EXP_LNKCTL_LABIE	0x0800	/* Link Autonomous Bandwidth Interrupt Enable */
+#define PCI_EXP_LNKSTA		18	/* Link Status */
+#define  PCI_EXP_LNKSTA_CLS	0x000f	/* Current Link Speed */
+#define  PCI_EXP_LNKSTA_CLS_2_5GB 0x0001 /* Current Link Speed 2.5GT/s */
+#define  PCI_EXP_LNKSTA_CLS_5_0GB 0x0002 /* Current Link Speed 5.0GT/s */
+#define  PCI_EXP_LNKSTA_CLS_8_0GB 0x0003 /* Current Link Speed 8.0GT/s */
+#define  PCI_EXP_LNKSTA_CLS_16_0GB 0x0004 /* Current Link Speed 16.0GT/s */
+#define  PCI_EXP_LNKSTA_CLS_32_0GB 0x0005 /* Current Link Speed 32.0GT/s */
+#define  PCI_EXP_LNKSTA_NLW	0x03f0	/* Negotiated Link Width */
+#define  PCI_EXP_LNKSTA_NLW_X1	0x0010	/* Current Link Width x1 */
+#define  PCI_EXP_LNKSTA_NLW_X2	0x0020	/* Current Link Width x2 */
+#define  PCI_EXP_LNKSTA_NLW_X4	0x0040	/* Current Link Width x4 */
+#define  PCI_EXP_LNKSTA_NLW_X8	0x0080	/* Current Link Width x8 */
+#define  PCI_EXP_LNKSTA_NLW_SHIFT 4	/* start of NLW mask in link status */
+#define  PCI_EXP_LNKSTA_LT	0x0800	/* Link Training */
+#define  PCI_EXP_LNKSTA_SLC	0x1000	/* Slot Clock Configuration */
+#define  PCI_EXP_LNKSTA_DLLLA	0x2000	/* Data Link Layer Link Active */
+#define  PCI_EXP_LNKSTA_LBMS	0x4000	/* Link Bandwidth Management Status */
+#define  PCI_EXP_LNKSTA_LABS	0x8000	/* Link Autonomous Bandwidth Status */
+#define PCI_CAP_EXP_ENDPOINT_SIZEOF_V1	20	/* v1 endpoints with link end here */
+#define PCI_EXP_SLTCAP		20	/* Slot Capabilities */
+#define  PCI_EXP_SLTCAP_ABP	0x00000001 /* Attention Button Present */
+#define  PCI_EXP_SLTCAP_PCP	0x00000002 /* Power Controller Present */
+#define  PCI_EXP_SLTCAP_MRLSP	0x00000004 /* MRL Sensor Present */
+#define  PCI_EXP_SLTCAP_AIP	0x00000008 /* Attention Indicator Present */
+#define  PCI_EXP_SLTCAP_PIP	0x00000010 /* Power Indicator Present */
+#define  PCI_EXP_SLTCAP_HPS	0x00000020 /* Hot-Plug Surprise */
+#define  PCI_EXP_SLTCAP_HPC	0x00000040 /* Hot-Plug Capable */
+#define  PCI_EXP_SLTCAP_SPLV	0x00007f80 /* Slot Power Limit Value */
+#define  PCI_EXP_SLTCAP_SPLS	0x00018000 /* Slot Power Limit Scale */
+#define  PCI_EXP_SLTCAP_EIP	0x00020000 /* Electromechanical Interlock Present */
+#define  PCI_EXP_SLTCAP_NCCS	0x00040000 /* No Command Completed Support */
+#define  PCI_EXP_SLTCAP_PSN	0xfff80000 /* Physical Slot Number */
+#define PCI_EXP_SLTCTL		24	/* Slot Control */
+#define  PCI_EXP_SLTCTL_ABPE	0x0001	/* Attention Button Pressed Enable */
+#define  PCI_EXP_SLTCTL_PFDE	0x0002	/* Power Fault Detected Enable */
+#define  PCI_EXP_SLTCTL_MRLSCE	0x0004	/* MRL Sensor Changed Enable */
+#define  PCI_EXP_SLTCTL_PDCE	0x0008	/* Presence Detect Changed Enable */
+#define  PCI_EXP_SLTCTL_CCIE	0x0010	/* Command Completed Interrupt Enable */
+#define  PCI_EXP_SLTCTL_HPIE	0x0020	/* Hot-Plug Interrupt Enable */
+#define  PCI_EXP_SLTCTL_AIC	0x00c0	/* Attention Indicator Control */
+#define  PCI_EXP_SLTCTL_ATTN_IND_SHIFT 6      /* Attention Indicator shift */
+#define  PCI_EXP_SLTCTL_ATTN_IND_ON    0x0040 /* Attention Indicator on */
+#define  PCI_EXP_SLTCTL_ATTN_IND_BLINK 0x0080 /* Attention Indicator blinking */
+#define  PCI_EXP_SLTCTL_ATTN_IND_OFF   0x00c0 /* Attention Indicator off */
+#define  PCI_EXP_SLTCTL_PIC	0x0300	/* Power Indicator Control */
+#define  PCI_EXP_SLTCTL_PWR_IND_ON     0x0100 /* Power Indicator on */
+#define  PCI_EXP_SLTCTL_PWR_IND_BLINK  0x0200 /* Power Indicator blinking */
+#define  PCI_EXP_SLTCTL_PWR_IND_OFF    0x0300 /* Power Indicator off */
+#define  PCI_EXP_SLTCTL_PCC	0x0400	/* Power Controller Control */
+#define  PCI_EXP_SLTCTL_PWR_ON         0x0000 /* Power On */
+#define  PCI_EXP_SLTCTL_PWR_OFF        0x0400 /* Power Off */
+#define  PCI_EXP_SLTCTL_EIC	0x0800	/* Electromechanical Interlock Control */
+#define  PCI_EXP_SLTCTL_DLLSCE	0x1000	/* Data Link Layer State Changed Enable */
+#define  PCI_EXP_SLTCTL_IBPD_DISABLE	0x4000 /* In-band PD disable */
+#define PCI_EXP_SLTSTA		26	/* Slot Status */
+#define  PCI_EXP_SLTSTA_ABP	0x0001	/* Attention Button Pressed */
+#define  PCI_EXP_SLTSTA_PFD	0x0002	/* Power Fault Detected */
+#define  PCI_EXP_SLTSTA_MRLSC	0x0004	/* MRL Sensor Changed */
+#define  PCI_EXP_SLTSTA_PDC	0x0008	/* Presence Detect Changed */
+#define  PCI_EXP_SLTSTA_CC	0x0010	/* Command Completed */
+#define  PCI_EXP_SLTSTA_MRLSS	0x0020	/* MRL Sensor State */
+#define  PCI_EXP_SLTSTA_PDS	0x0040	/* Presence Detect State */
+#define  PCI_EXP_SLTSTA_EIS	0x0080	/* Electromechanical Interlock Status */
+#define  PCI_EXP_SLTSTA_DLLSC	0x0100	/* Data Link Layer State Changed */
+#define PCI_EXP_RTCTL		28	/* Root Control */
+#define  PCI_EXP_RTCTL_SECEE	0x0001	/* System Error on Correctable Error */
+#define  PCI_EXP_RTCTL_SENFEE	0x0002	/* System Error on Non-Fatal Error */
+#define  PCI_EXP_RTCTL_SEFEE	0x0004	/* System Error on Fatal Error */
+#define  PCI_EXP_RTCTL_PMEIE	0x0008	/* PME Interrupt Enable */
+#define  PCI_EXP_RTCTL_CRSSVE	0x0010	/* CRS Software Visibility Enable */
+#define PCI_EXP_RTCAP		30	/* Root Capabilities */
+#define  PCI_EXP_RTCAP_CRSVIS	0x0001	/* CRS Software Visibility capability */
+#define PCI_EXP_RTSTA		32	/* Root Status */
+#define  PCI_EXP_RTSTA_PME	0x00010000 /* PME status */
+#define  PCI_EXP_RTSTA_PENDING	0x00020000 /* PME pending */
+/*
+ * The Device Capabilities 2, Device Status 2, Device Control 2,
+ * Link Capabilities 2, Link Status 2, Link Control 2,
+ * Slot Capabilities 2, Slot Status 2, and Slot Control 2 registers
+ * are only present on devices with PCIe Capability version 2.
+ * Use pcie_capability_read_word() and similar interfaces to use them
+ * safely.
+ */
+#define PCI_EXP_DEVCAP2		36	/* Device Capabilities 2 */
+#define  PCI_EXP_DEVCAP2_COMP_TMOUT_DIS	0x00000010 /* Completion Timeout Disable supported */
+#define  PCI_EXP_DEVCAP2_ARI		0x00000020 /* Alternative Routing-ID */
+#define  PCI_EXP_DEVCAP2_ATOMIC_ROUTE	0x00000040 /* Atomic Op routing */
+#define  PCI_EXP_DEVCAP2_ATOMIC_COMP32	0x00000080 /* 32b AtomicOp completion */
+#define  PCI_EXP_DEVCAP2_ATOMIC_COMP64	0x00000100 /* 64b AtomicOp completion */
+#define  PCI_EXP_DEVCAP2_ATOMIC_COMP128	0x00000200 /* 128b AtomicOp completion */
+#define  PCI_EXP_DEVCAP2_LTR		0x00000800 /* Latency tolerance reporting */
+#define  PCI_EXP_DEVCAP2_OBFF_MASK	0x000c0000 /* OBFF support mechanism */
+#define  PCI_EXP_DEVCAP2_OBFF_MSG	0x00040000 /* New message signaling */
+#define  PCI_EXP_DEVCAP2_OBFF_WAKE	0x00080000 /* Re-use WAKE# for OBFF */
+#define  PCI_EXP_DEVCAP2_EE_PREFIX	0x00200000 /* End-End TLP Prefix */
+#define PCI_EXP_DEVCTL2		40	/* Device Control 2 */
+#define  PCI_EXP_DEVCTL2_COMP_TIMEOUT	0x000f	/* Completion Timeout Value */
+#define  PCI_EXP_DEVCTL2_COMP_TMOUT_DIS	0x0010	/* Completion Timeout Disable */
+#define  PCI_EXP_DEVCTL2_ARI		0x0020	/* Alternative Routing-ID */
+#define  PCI_EXP_DEVCTL2_ATOMIC_REQ	0x0040	/* Set Atomic requests */
+#define  PCI_EXP_DEVCTL2_ATOMIC_EGRESS_BLOCK 0x0080 /* Block atomic egress */
+#define  PCI_EXP_DEVCTL2_IDO_REQ_EN	0x0100	/* Allow IDO for requests */
+#define  PCI_EXP_DEVCTL2_IDO_CMP_EN	0x0200	/* Allow IDO for completions */
+#define  PCI_EXP_DEVCTL2_LTR_EN		0x0400	/* Enable LTR mechanism */
+#define  PCI_EXP_DEVCTL2_OBFF_MSGA_EN	0x2000	/* Enable OBFF Message type A */
+#define  PCI_EXP_DEVCTL2_OBFF_MSGB_EN	0x4000	/* Enable OBFF Message type B */
+#define  PCI_EXP_DEVCTL2_OBFF_WAKE_EN	0x6000	/* OBFF using WAKE# signaling */
+#define PCI_EXP_DEVSTA2		42	/* Device Status 2 */
+#define PCI_CAP_EXP_RC_ENDPOINT_SIZEOF_V2	44	/* v2 endpoints without link end here */
+#define PCI_EXP_LNKCAP2		44	/* Link Capabilities 2 */
+#define  PCI_EXP_LNKCAP2_SLS_2_5GB	0x00000002 /* Supported Speed 2.5GT/s */
+#define  PCI_EXP_LNKCAP2_SLS_5_0GB	0x00000004 /* Supported Speed 5GT/s */
+#define  PCI_EXP_LNKCAP2_SLS_8_0GB	0x00000008 /* Supported Speed 8GT/s */
+#define  PCI_EXP_LNKCAP2_SLS_16_0GB	0x00000010 /* Supported Speed 16GT/s */
+#define  PCI_EXP_LNKCAP2_SLS_32_0GB	0x00000020 /* Supported Speed 32GT/s */
+#define  PCI_EXP_LNKCAP2_CROSSLINK	0x00000100 /* Crosslink supported */
+#define PCI_EXP_LNKCTL2		48	/* Link Control 2 */
+#define  PCI_EXP_LNKCTL2_TLS		0x000f
+#define  PCI_EXP_LNKCTL2_TLS_2_5GT	0x0001 /* Supported Speed 2.5GT/s */
+#define  PCI_EXP_LNKCTL2_TLS_5_0GT	0x0002 /* Supported Speed 5GT/s */
+#define  PCI_EXP_LNKCTL2_TLS_8_0GT	0x0003 /* Supported Speed 8GT/s */
+#define  PCI_EXP_LNKCTL2_TLS_16_0GT	0x0004 /* Supported Speed 16GT/s */
+#define  PCI_EXP_LNKCTL2_TLS_32_0GT	0x0005 /* Supported Speed 32GT/s */
+#define  PCI_EXP_LNKCTL2_ENTER_COMP	0x0010 /* Enter Compliance */
+#define  PCI_EXP_LNKCTL2_TX_MARGIN	0x0380 /* Transmit Margin */
+#define  PCI_EXP_LNKCTL2_HASD		0x0020 /* HW Autonomous Speed Disable */
+#define PCI_EXP_LNKSTA2		50	/* Link Status 2 */
+#define PCI_CAP_EXP_ENDPOINT_SIZEOF_V2	52	/* v2 endpoints with link end here */
+#define PCI_EXP_SLTCAP2		52	/* Slot Capabilities 2 */
+#define  PCI_EXP_SLTCAP2_IBPD	0x00000001 /* In-band PD Disable Supported */
+#define PCI_EXP_SLTCTL2		56	/* Slot Control 2 */
+#define PCI_EXP_SLTSTA2		58	/* Slot Status 2 */
+
+/* Extended Capabilities (PCI-X 2.0 and Express) */
+#define PCI_EXT_CAP_ID(header)		(header & 0x0000ffff)
+#define PCI_EXT_CAP_VER(header)		((header >> 16) & 0xf)
+#define PCI_EXT_CAP_NEXT(header)	((header >> 20) & 0xffc)
+
+#define PCI_EXT_CAP_ID_ERR	0x01	/* Advanced Error Reporting */
+#define PCI_EXT_CAP_ID_VC	0x02	/* Virtual Channel Capability */
+#define PCI_EXT_CAP_ID_DSN	0x03	/* Device Serial Number */
+#define PCI_EXT_CAP_ID_PWR	0x04	/* Power Budgeting */
+#define PCI_EXT_CAP_ID_RCLD	0x05	/* Root Complex Link Declaration */
+#define PCI_EXT_CAP_ID_RCILC	0x06	/* Root Complex Internal Link Control */
+#define PCI_EXT_CAP_ID_RCEC	0x07	/* Root Complex Event Collector */
+#define PCI_EXT_CAP_ID_MFVC	0x08	/* Multi-Function VC Capability */
+#define PCI_EXT_CAP_ID_VC9	0x09	/* same as _VC */
+#define PCI_EXT_CAP_ID_RCRB	0x0A	/* Root Complex RB? */
+#define PCI_EXT_CAP_ID_VNDR	0x0B	/* Vendor-Specific */
+#define PCI_EXT_CAP_ID_CAC	0x0C	/* Config Access - obsolete */
+#define PCI_EXT_CAP_ID_ACS	0x0D	/* Access Control Services */
+#define PCI_EXT_CAP_ID_ARI	0x0E	/* Alternate Routing ID */
+#define PCI_EXT_CAP_ID_ATS	0x0F	/* Address Translation Services */
+#define PCI_EXT_CAP_ID_SRIOV	0x10	/* Single Root I/O Virtualization */
+#define PCI_EXT_CAP_ID_MRIOV	0x11	/* Multi Root I/O Virtualization */
+#define PCI_EXT_CAP_ID_MCAST	0x12	/* Multicast */
+#define PCI_EXT_CAP_ID_PRI	0x13	/* Page Request Interface */
+#define PCI_EXT_CAP_ID_AMD_XXX	0x14	/* Reserved for AMD */
+#define PCI_EXT_CAP_ID_REBAR	0x15	/* Resizable BAR */
+#define PCI_EXT_CAP_ID_DPA	0x16	/* Dynamic Power Allocation */
+#define PCI_EXT_CAP_ID_TPH	0x17	/* TPH Requester */
+#define PCI_EXT_CAP_ID_LTR	0x18	/* Latency Tolerance Reporting */
+#define PCI_EXT_CAP_ID_SECPCI	0x19	/* Secondary PCIe Capability */
+#define PCI_EXT_CAP_ID_PMUX	0x1A	/* Protocol Multiplexing */
+#define PCI_EXT_CAP_ID_PASID	0x1B	/* Process Address Space ID */
+#define PCI_EXT_CAP_ID_DPC	0x1D	/* Downstream Port Containment */
+#define PCI_EXT_CAP_ID_L1SS	0x1E	/* L1 PM Substates */
+#define PCI_EXT_CAP_ID_PTM	0x1F	/* Precision Time Measurement */
+#define PCI_EXT_CAP_ID_DLF	0x25	/* Data Link Feature */
+#define PCI_EXT_CAP_ID_PL_16GT	0x26	/* Physical Layer 16.0 GT/s */
+#define PCI_EXT_CAP_ID_MAX	PCI_EXT_CAP_ID_PL_16GT
+
+#define PCI_EXT_CAP_DSN_SIZEOF	12
+#define PCI_EXT_CAP_MCAST_ENDPOINT_SIZEOF 40
+
+/* Advanced Error Reporting */
+#define PCI_ERR_UNCOR_STATUS	4	/* Uncorrectable Error Status */
+#define  PCI_ERR_UNC_UND	0x00000001	/* Undefined */
+#define  PCI_ERR_UNC_DLP	0x00000010	/* Data Link Protocol */
+#define  PCI_ERR_UNC_SURPDN	0x00000020	/* Surprise Down */
+#define  PCI_ERR_UNC_POISON_TLP	0x00001000	/* Poisoned TLP */
+#define  PCI_ERR_UNC_FCP	0x00002000	/* Flow Control Protocol */
+#define  PCI_ERR_UNC_COMP_TIME	0x00004000	/* Completion Timeout */
+#define  PCI_ERR_UNC_COMP_ABORT	0x00008000	/* Completer Abort */
+#define  PCI_ERR_UNC_UNX_COMP	0x00010000	/* Unexpected Completion */
+#define  PCI_ERR_UNC_RX_OVER	0x00020000	/* Receiver Overflow */
+#define  PCI_ERR_UNC_MALF_TLP	0x00040000	/* Malformed TLP */
+#define  PCI_ERR_UNC_ECRC	0x00080000	/* ECRC Error Status */
+#define  PCI_ERR_UNC_UNSUP	0x00100000	/* Unsupported Request */
+#define  PCI_ERR_UNC_ACSV	0x00200000	/* ACS Violation */
+#define  PCI_ERR_UNC_INTN	0x00400000	/* internal error */
+#define  PCI_ERR_UNC_MCBTLP	0x00800000	/* MC blocked TLP */
+#define  PCI_ERR_UNC_ATOMEG	0x01000000	/* Atomic egress blocked */
+#define  PCI_ERR_UNC_TLPPRE	0x02000000	/* TLP prefix blocked */
+#define PCI_ERR_UNCOR_MASK	8	/* Uncorrectable Error Mask */
+	/* Same bits as above */
+#define PCI_ERR_UNCOR_SEVER	12	/* Uncorrectable Error Severity */
+	/* Same bits as above */
+#define PCI_ERR_COR_STATUS	16	/* Correctable Error Status */
+#define  PCI_ERR_COR_RCVR	0x00000001	/* Receiver Error Status */
+#define  PCI_ERR_COR_BAD_TLP	0x00000040	/* Bad TLP Status */
+#define  PCI_ERR_COR_BAD_DLLP	0x00000080	/* Bad DLLP Status */
+#define  PCI_ERR_COR_REP_ROLL	0x00000100	/* REPLAY_NUM Rollover */
+#define  PCI_ERR_COR_REP_TIMER	0x00001000	/* Replay Timer Timeout */
+#define  PCI_ERR_COR_ADV_NFAT	0x00002000	/* Advisory Non-Fatal */
+#define  PCI_ERR_COR_INTERNAL	0x00004000	/* Corrected Internal */
+#define  PCI_ERR_COR_LOG_OVER	0x00008000	/* Header Log Overflow */
+#define PCI_ERR_COR_MASK	20	/* Correctable Error Mask */
+	/* Same bits as above */
+#define PCI_ERR_CAP		24	/* Advanced Error Capabilities */
+#define  PCI_ERR_CAP_FEP(x)	((x) & 31)	/* First Error Pointer */
+#define  PCI_ERR_CAP_ECRC_GENC	0x00000020	/* ECRC Generation Capable */
+#define  PCI_ERR_CAP_ECRC_GENE	0x00000040	/* ECRC Generation Enable */
+#define  PCI_ERR_CAP_ECRC_CHKC	0x00000080	/* ECRC Check Capable */
+#define  PCI_ERR_CAP_ECRC_CHKE	0x00000100	/* ECRC Check Enable */
+#define PCI_ERR_HEADER_LOG	28	/* Header Log Register (16 bytes) */
+#define PCI_ERR_ROOT_COMMAND	44	/* Root Error Command */
+#define  PCI_ERR_ROOT_CMD_COR_EN	0x00000001 /* Correctable Err Reporting Enable */
+#define  PCI_ERR_ROOT_CMD_NONFATAL_EN	0x00000002 /* Non-Fatal Err Reporting Enable */
+#define  PCI_ERR_ROOT_CMD_FATAL_EN	0x00000004 /* Fatal Err Reporting Enable */
+#define PCI_ERR_ROOT_STATUS	48
+#define  PCI_ERR_ROOT_COR_RCV		0x00000001 /* ERR_COR Received */
+#define  PCI_ERR_ROOT_MULTI_COR_RCV	0x00000002 /* Multiple ERR_COR */
+#define  PCI_ERR_ROOT_UNCOR_RCV		0x00000004 /* ERR_FATAL/NONFATAL */
+#define  PCI_ERR_ROOT_MULTI_UNCOR_RCV	0x00000008 /* Multiple FATAL/NONFATAL */
+#define  PCI_ERR_ROOT_FIRST_FATAL	0x00000010 /* First UNC is Fatal */
+#define  PCI_ERR_ROOT_NONFATAL_RCV	0x00000020 /* Non-Fatal Received */
+#define  PCI_ERR_ROOT_FATAL_RCV		0x00000040 /* Fatal Received */
+#define  PCI_ERR_ROOT_AER_IRQ		0xf8000000 /* Advanced Error Interrupt Message Number */
+#define PCI_ERR_ROOT_ERR_SRC	52	/* Error Source Identification */
+
+/* Virtual Channel */
+#define PCI_VC_PORT_CAP1	4
+#define  PCI_VC_CAP1_EVCC	0x00000007	/* extended VC count */
+#define  PCI_VC_CAP1_LPEVCC	0x00000070	/* low prio extended VC count */
+#define  PCI_VC_CAP1_ARB_SIZE	0x00000c00
+#define PCI_VC_PORT_CAP2	8
+#define  PCI_VC_CAP2_32_PHASE		0x00000002
+#define  PCI_VC_CAP2_64_PHASE		0x00000004
+#define  PCI_VC_CAP2_128_PHASE		0x00000008
+#define  PCI_VC_CAP2_ARB_OFF		0xff000000
+#define PCI_VC_PORT_CTRL	12
+#define  PCI_VC_PORT_CTRL_LOAD_TABLE	0x00000001
+#define PCI_VC_PORT_STATUS	14
+#define  PCI_VC_PORT_STATUS_TABLE	0x00000001
+#define PCI_VC_RES_CAP		16
+#define  PCI_VC_RES_CAP_32_PHASE	0x00000002
+#define  PCI_VC_RES_CAP_64_PHASE	0x00000004
+#define  PCI_VC_RES_CAP_128_PHASE	0x00000008
+#define  PCI_VC_RES_CAP_128_PHASE_TB	0x00000010
+#define  PCI_VC_RES_CAP_256_PHASE	0x00000020
+#define  PCI_VC_RES_CAP_ARB_OFF		0xff000000
+#define PCI_VC_RES_CTRL		20
+#define  PCI_VC_RES_CTRL_LOAD_TABLE	0x00010000
+#define  PCI_VC_RES_CTRL_ARB_SELECT	0x000e0000
+#define  PCI_VC_RES_CTRL_ID		0x07000000
+#define  PCI_VC_RES_CTRL_ENABLE		0x80000000
+#define PCI_VC_RES_STATUS	26
+#define  PCI_VC_RES_STATUS_TABLE	0x00000001
+#define  PCI_VC_RES_STATUS_NEGO		0x00000002
+#define PCI_CAP_VC_BASE_SIZEOF		0x10
+#define PCI_CAP_VC_PER_VC_SIZEOF	0x0C
+
+/* Power Budgeting */
+#define PCI_PWR_DSR		4	/* Data Select Register */
+#define PCI_PWR_DATA		8	/* Data Register */
+#define  PCI_PWR_DATA_BASE(x)	((x) & 0xff)	    /* Base Power */
+#define  PCI_PWR_DATA_SCALE(x)	(((x) >> 8) & 3)    /* Data Scale */
+#define  PCI_PWR_DATA_PM_SUB(x)	(((x) >> 10) & 7)   /* PM Sub State */
+#define  PCI_PWR_DATA_PM_STATE(x) (((x) >> 13) & 3) /* PM State */
+#define  PCI_PWR_DATA_TYPE(x)	(((x) >> 15) & 7)   /* Type */
+#define  PCI_PWR_DATA_RAIL(x)	(((x) >> 18) & 7)   /* Power Rail */
+#define PCI_PWR_CAP		12	/* Capability */
+#define  PCI_PWR_CAP_BUDGET(x)	((x) & 1)	/* Included in system budget */
+#define PCI_EXT_CAP_PWR_SIZEOF	16
+
+/* Vendor-Specific (VSEC, PCI_EXT_CAP_ID_VNDR) */
+#define PCI_VNDR_HEADER		4	/* Vendor-Specific Header */
+#define  PCI_VNDR_HEADER_ID(x)	((x) & 0xffff)
+#define  PCI_VNDR_HEADER_REV(x)	(((x) >> 16) & 0xf)
+#define  PCI_VNDR_HEADER_LEN(x)	(((x) >> 20) & 0xfff)
+
+/*
+ * HyperTransport sub capability types
+ *
+ * Unfortunately there are both 3 bit and 5 bit capability types defined
+ * in the HT spec, catering for that is a little messy. You probably don't
+ * want to use these directly, just use pci_find_ht_capability() and it
+ * will do the right thing for you.
+ */
+#define HT_3BIT_CAP_MASK	0xE0
+#define HT_CAPTYPE_SLAVE	0x00	/* Slave/Primary link configuration */
+#define HT_CAPTYPE_HOST		0x20	/* Host/Secondary link configuration */
+
+#define HT_5BIT_CAP_MASK	0xF8
+#define HT_CAPTYPE_IRQ		0x80	/* IRQ Configuration */
+#define HT_CAPTYPE_REMAPPING_40	0xA0	/* 40 bit address remapping */
+#define HT_CAPTYPE_REMAPPING_64 0xA2	/* 64 bit address remapping */
+#define HT_CAPTYPE_UNITID_CLUMP	0x90	/* Unit ID clumping */
+#define HT_CAPTYPE_EXTCONF	0x98	/* Extended Configuration Space Access */
+#define HT_CAPTYPE_MSI_MAPPING	0xA8	/* MSI Mapping Capability */
+#define  HT_MSI_FLAGS		0x02		/* Offset to flags */
+#define  HT_MSI_FLAGS_ENABLE	0x1		/* Mapping enable */
+#define  HT_MSI_FLAGS_FIXED	0x2		/* Fixed mapping only */
+#define  HT_MSI_FIXED_ADDR	0x00000000FEE00000ULL	/* Fixed addr */
+#define  HT_MSI_ADDR_LO		0x04		/* Offset to low addr bits */
+#define  HT_MSI_ADDR_LO_MASK	0xFFF00000	/* Low address bit mask */
+#define  HT_MSI_ADDR_HI		0x08		/* Offset to high addr bits */
+#define HT_CAPTYPE_DIRECT_ROUTE	0xB0	/* Direct routing configuration */
+#define HT_CAPTYPE_VCSET	0xB8	/* Virtual Channel configuration */
+#define HT_CAPTYPE_ERROR_RETRY	0xC0	/* Retry on error configuration */
+#define HT_CAPTYPE_GEN3		0xD0	/* Generation 3 HyperTransport configuration */
+#define HT_CAPTYPE_PM		0xE0	/* HyperTransport power management configuration */
+#define HT_CAP_SIZEOF_LONG	28	/* slave & primary */
+#define HT_CAP_SIZEOF_SHORT	24	/* host & secondary */
+
+/* Alternative Routing-ID Interpretation */
+#define PCI_ARI_CAP		0x04	/* ARI Capability Register */
+#define  PCI_ARI_CAP_MFVC	0x0001	/* MFVC Function Groups Capability */
+#define  PCI_ARI_CAP_ACS	0x0002	/* ACS Function Groups Capability */
+#define  PCI_ARI_CAP_NFN(x)	(((x) >> 8) & 0xff) /* Next Function Number */
+#define PCI_ARI_CTRL		0x06	/* ARI Control Register */
+#define  PCI_ARI_CTRL_MFVC	0x0001	/* MFVC Function Groups Enable */
+#define  PCI_ARI_CTRL_ACS	0x0002	/* ACS Function Groups Enable */
+#define  PCI_ARI_CTRL_FG(x)	(((x) >> 4) & 7) /* Function Group */
+#define PCI_EXT_CAP_ARI_SIZEOF	8
+
+/* Address Translation Service */
+#define PCI_ATS_CAP		0x04	/* ATS Capability Register */
+#define  PCI_ATS_CAP_QDEP(x)	((x) & 0x1f)	/* Invalidate Queue Depth */
+#define  PCI_ATS_MAX_QDEP	32	/* Max Invalidate Queue Depth */
+#define  PCI_ATS_CAP_PAGE_ALIGNED	0x0020 /* Page Aligned Request */
+#define PCI_ATS_CTRL		0x06	/* ATS Control Register */
+#define  PCI_ATS_CTRL_ENABLE	0x8000	/* ATS Enable */
+#define  PCI_ATS_CTRL_STU(x)	((x) & 0x1f)	/* Smallest Translation Unit */
+#define  PCI_ATS_MIN_STU	12	/* shift of minimum STU block */
+#define PCI_EXT_CAP_ATS_SIZEOF	8
+
+/* Page Request Interface */
+#define PCI_PRI_CTRL		0x04	/* PRI control register */
+#define  PCI_PRI_CTRL_ENABLE	0x0001	/* Enable */
+#define  PCI_PRI_CTRL_RESET	0x0002	/* Reset */
+#define PCI_PRI_STATUS		0x06	/* PRI status register */
+#define  PCI_PRI_STATUS_RF	0x0001	/* Response Failure */
+#define  PCI_PRI_STATUS_UPRGI	0x0002	/* Unexpected PRG index */
+#define  PCI_PRI_STATUS_STOPPED	0x0100	/* PRI Stopped */
+#define  PCI_PRI_STATUS_PASID	0x8000	/* PRG Response PASID Required */
+#define PCI_PRI_MAX_REQ		0x08	/* PRI max reqs supported */
+#define PCI_PRI_ALLOC_REQ	0x0c	/* PRI max reqs allowed */
+#define PCI_EXT_CAP_PRI_SIZEOF	16
+
+/* Process Address Space ID */
+#define PCI_PASID_CAP		0x04    /* PASID feature register */
+#define  PCI_PASID_CAP_EXEC	0x02	/* Exec permissions Supported */
+#define  PCI_PASID_CAP_PRIV	0x04	/* Privilege Mode Supported */
+#define PCI_PASID_CTRL		0x06    /* PASID control register */
+#define  PCI_PASID_CTRL_ENABLE	0x01	/* Enable bit */
+#define  PCI_PASID_CTRL_EXEC	0x02	/* Exec permissions Enable */
+#define  PCI_PASID_CTRL_PRIV	0x04	/* Privilege Mode Enable */
+#define PCI_EXT_CAP_PASID_SIZEOF	8
+
+/* Single Root I/O Virtualization */
+#define PCI_SRIOV_CAP		0x04	/* SR-IOV Capabilities */
+#define  PCI_SRIOV_CAP_VFM	0x00000001  /* VF Migration Capable */
+#define  PCI_SRIOV_CAP_INTR(x)	((x) >> 21) /* Interrupt Message Number */
+#define PCI_SRIOV_CTRL		0x08	/* SR-IOV Control */
+#define  PCI_SRIOV_CTRL_VFE	0x0001	/* VF Enable */
+#define  PCI_SRIOV_CTRL_VFM	0x0002	/* VF Migration Enable */
+#define  PCI_SRIOV_CTRL_INTR	0x0004	/* VF Migration Interrupt Enable */
+#define  PCI_SRIOV_CTRL_MSE	0x0008	/* VF Memory Space Enable */
+#define  PCI_SRIOV_CTRL_ARI	0x0010	/* ARI Capable Hierarchy */
+#define PCI_SRIOV_STATUS	0x0a	/* SR-IOV Status */
+#define  PCI_SRIOV_STATUS_VFM	0x0001	/* VF Migration Status */
+#define PCI_SRIOV_INITIAL_VF	0x0c	/* Initial VFs */
+#define PCI_SRIOV_TOTAL_VF	0x0e	/* Total VFs */
+#define PCI_SRIOV_NUM_VF	0x10	/* Number of VFs */
+#define PCI_SRIOV_FUNC_LINK	0x12	/* Function Dependency Link */
+#define PCI_SRIOV_VF_OFFSET	0x14	/* First VF Offset */
+#define PCI_SRIOV_VF_STRIDE	0x16	/* Following VF Stride */
+#define PCI_SRIOV_VF_DID	0x1a	/* VF Device ID */
+#define PCI_SRIOV_SUP_PGSIZE	0x1c	/* Supported Page Sizes */
+#define PCI_SRIOV_SYS_PGSIZE	0x20	/* System Page Size */
+#define PCI_SRIOV_BAR		0x24	/* VF BAR0 */
+#define  PCI_SRIOV_NUM_BARS	6	/* Number of VF BARs */
+#define PCI_SRIOV_VFM		0x3c	/* VF Migration State Array Offset*/
+#define  PCI_SRIOV_VFM_BIR(x)	((x) & 7)	/* State BIR */
+#define  PCI_SRIOV_VFM_OFFSET(x) ((x) & ~7)	/* State Offset */
+#define  PCI_SRIOV_VFM_UA	0x0	/* Inactive.Unavailable */
+#define  PCI_SRIOV_VFM_MI	0x1	/* Dormant.MigrateIn */
+#define  PCI_SRIOV_VFM_MO	0x2	/* Active.MigrateOut */
+#define  PCI_SRIOV_VFM_AV	0x3	/* Active.Available */
+#define PCI_EXT_CAP_SRIOV_SIZEOF 64
+
+#define PCI_LTR_MAX_SNOOP_LAT	0x4
+#define PCI_LTR_MAX_NOSNOOP_LAT	0x6
+#define  PCI_LTR_VALUE_MASK	0x000003ff
+#define  PCI_LTR_SCALE_MASK	0x00001c00
+#define  PCI_LTR_SCALE_SHIFT	10
+#define PCI_EXT_CAP_LTR_SIZEOF	8
+
+/* Access Control Service */
+#define PCI_ACS_CAP		0x04	/* ACS Capability Register */
+#define  PCI_ACS_SV		0x0001	/* Source Validation */
+#define  PCI_ACS_TB		0x0002	/* Translation Blocking */
+#define  PCI_ACS_RR		0x0004	/* P2P Request Redirect */
+#define  PCI_ACS_CR		0x0008	/* P2P Completion Redirect */
+#define  PCI_ACS_UF		0x0010	/* Upstream Forwarding */
+#define  PCI_ACS_EC		0x0020	/* P2P Egress Control */
+#define  PCI_ACS_DT		0x0040	/* Direct Translated P2P */
+#define PCI_ACS_EGRESS_BITS	0x05	/* ACS Egress Control Vector Size */
+#define PCI_ACS_CTRL		0x06	/* ACS Control Register */
+#define PCI_ACS_EGRESS_CTL_V	0x08	/* ACS Egress Control Vector */
+
+#define PCI_VSEC_HDR		4	/* extended cap - vendor-specific */
+#define  PCI_VSEC_HDR_LEN_SHIFT	20	/* shift for length field */
+
+/* SATA capability */
+#define PCI_SATA_REGS		4	/* SATA REGs specifier */
+#define  PCI_SATA_REGS_MASK	0xF	/* location - BAR#/inline */
+#define  PCI_SATA_REGS_INLINE	0xF	/* REGS in config space */
+#define PCI_SATA_SIZEOF_SHORT	8
+#define PCI_SATA_SIZEOF_LONG	16
+
+/* Resizable BARs */
+#define PCI_REBAR_CAP		4	/* capability register */
+#define  PCI_REBAR_CAP_SIZES		0x00FFFFF0  /* supported BAR sizes */
+#define PCI_REBAR_CTRL		8	/* control register */
+#define  PCI_REBAR_CTRL_BAR_IDX		0x00000007  /* BAR index */
+#define  PCI_REBAR_CTRL_NBAR_MASK	0x000000E0  /* # of resizable BARs */
+#define  PCI_REBAR_CTRL_NBAR_SHIFT	5	    /* shift for # of BARs */
+#define  PCI_REBAR_CTRL_BAR_SIZE	0x00001F00  /* BAR size */
+#define  PCI_REBAR_CTRL_BAR_SHIFT	8	    /* shift for BAR size */
+
+/* Dynamic Power Allocation */
+#define PCI_DPA_CAP		4	/* capability register */
+#define  PCI_DPA_CAP_SUBSTATE_MASK	0x1F	/* # substates - 1 */
+#define PCI_DPA_BASE_SIZEOF	16	/* size with 0 substates */
+
+/* TPH Requester */
+#define PCI_TPH_CAP		4	/* capability register */
+#define  PCI_TPH_CAP_LOC_MASK	0x600	/* location mask */
+#define   PCI_TPH_LOC_NONE	0x000	/* no location */
+#define   PCI_TPH_LOC_CAP	0x200	/* in capability */
+#define   PCI_TPH_LOC_MSIX	0x400	/* in MSI-X */
+#define PCI_TPH_CAP_ST_MASK	0x07FF0000	/* st table mask */
+#define PCI_TPH_CAP_ST_SHIFT	16	/* st table shift */
+#define PCI_TPH_BASE_SIZEOF	12	/* size with no st table */
+
+/* Downstream Port Containment */
+#define PCI_EXP_DPC_CAP			4	/* DPC Capability */
+#define PCI_EXP_DPC_IRQ			0x001F	/* Interrupt Message Number */
+#define  PCI_EXP_DPC_CAP_RP_EXT		0x0020	/* Root Port Extensions */
+#define  PCI_EXP_DPC_CAP_POISONED_TLP	0x0040	/* Poisoned TLP Egress Blocking Supported */
+#define  PCI_EXP_DPC_CAP_SW_TRIGGER	0x0080	/* Software Triggering Supported */
+#define  PCI_EXP_DPC_RP_PIO_LOG_SIZE	0x0F00	/* RP PIO Log Size */
+#define  PCI_EXP_DPC_CAP_DL_ACTIVE	0x1000	/* ERR_COR signal on DL_Active supported */
+
+#define PCI_EXP_DPC_CTL			6	/* DPC control */
+#define  PCI_EXP_DPC_CTL_EN_FATAL	0x0001	/* Enable trigger on ERR_FATAL message */
+#define  PCI_EXP_DPC_CTL_EN_NONFATAL	0x0002	/* Enable trigger on ERR_NONFATAL message */
+#define  PCI_EXP_DPC_CTL_INT_EN		0x0008	/* DPC Interrupt Enable */
+
+#define PCI_EXP_DPC_STATUS		8	/* DPC Status */
+#define  PCI_EXP_DPC_STATUS_TRIGGER	    0x0001 /* Trigger Status */
+#define  PCI_EXP_DPC_STATUS_TRIGGER_RSN	    0x0006 /* Trigger Reason */
+#define  PCI_EXP_DPC_STATUS_INTERRUPT	    0x0008 /* Interrupt Status */
+#define  PCI_EXP_DPC_RP_BUSY		    0x0010 /* Root Port Busy */
+#define  PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT 0x0060 /* Trig Reason Extension */
+
+#define PCI_EXP_DPC_SOURCE_ID		10	/* DPC Source Identifier */
+
+#define PCI_EXP_DPC_RP_PIO_STATUS	 0x0C	/* RP PIO Status */
+#define PCI_EXP_DPC_RP_PIO_MASK		 0x10	/* RP PIO Mask */
+#define PCI_EXP_DPC_RP_PIO_SEVERITY	 0x14	/* RP PIO Severity */
+#define PCI_EXP_DPC_RP_PIO_SYSERROR	 0x18	/* RP PIO SysError */
+#define PCI_EXP_DPC_RP_PIO_EXCEPTION	 0x1C	/* RP PIO Exception */
+#define PCI_EXP_DPC_RP_PIO_HEADER_LOG	 0x20	/* RP PIO Header Log */
+#define PCI_EXP_DPC_RP_PIO_IMPSPEC_LOG	 0x30	/* RP PIO ImpSpec Log */
+#define PCI_EXP_DPC_RP_PIO_TLPPREFIX_LOG 0x34	/* RP PIO TLP Prefix Log */
+
+/* Precision Time Measurement */
+#define PCI_PTM_CAP			0x04	    /* PTM Capability */
+#define  PCI_PTM_CAP_REQ		0x00000001  /* Requester capable */
+#define  PCI_PTM_CAP_ROOT		0x00000004  /* Root capable */
+#define  PCI_PTM_GRANULARITY_MASK	0x0000FF00  /* Clock granularity */
+#define PCI_PTM_CTRL			0x08	    /* PTM Control */
+#define  PCI_PTM_CTRL_ENABLE		0x00000001  /* PTM enable */
+#define  PCI_PTM_CTRL_ROOT		0x00000002  /* Root select */
+
+/* ASPM L1 PM Substates */
+#define PCI_L1SS_CAP		0x04	/* Capabilities Register */
+#define  PCI_L1SS_CAP_PCIPM_L1_2	0x00000001  /* PCI-PM L1.2 Supported */
+#define  PCI_L1SS_CAP_PCIPM_L1_1	0x00000002  /* PCI-PM L1.1 Supported */
+#define  PCI_L1SS_CAP_ASPM_L1_2		0x00000004  /* ASPM L1.2 Supported */
+#define  PCI_L1SS_CAP_ASPM_L1_1		0x00000008  /* ASPM L1.1 Supported */
+#define  PCI_L1SS_CAP_L1_PM_SS		0x00000010  /* L1 PM Substates Supported */
+#define  PCI_L1SS_CAP_CM_RESTORE_TIME	0x0000ff00  /* Port Common_Mode_Restore_Time */
+#define  PCI_L1SS_CAP_P_PWR_ON_SCALE	0x00030000  /* Port T_POWER_ON scale */
+#define  PCI_L1SS_CAP_P_PWR_ON_VALUE	0x00f80000  /* Port T_POWER_ON value */
+#define PCI_L1SS_CTL1		0x08	/* Control 1 Register */
+#define  PCI_L1SS_CTL1_PCIPM_L1_2	0x00000001  /* PCI-PM L1.2 Enable */
+#define  PCI_L1SS_CTL1_PCIPM_L1_1	0x00000002  /* PCI-PM L1.1 Enable */
+#define  PCI_L1SS_CTL1_ASPM_L1_2	0x00000004  /* ASPM L1.2 Enable */
+#define  PCI_L1SS_CTL1_ASPM_L1_1	0x00000008  /* ASPM L1.1 Enable */
+#define  PCI_L1SS_CTL1_L1SS_MASK	0x0000000f
+#define  PCI_L1SS_CTL1_CM_RESTORE_TIME	0x0000ff00  /* Common_Mode_Restore_Time */
+#define  PCI_L1SS_CTL1_LTR_L12_TH_VALUE	0x03ff0000  /* LTR_L1.2_THRESHOLD_Value */
+#define  PCI_L1SS_CTL1_LTR_L12_TH_SCALE	0xe0000000  /* LTR_L1.2_THRESHOLD_Scale */
+#define PCI_L1SS_CTL2		0x0c	/* Control 2 Register */
+
+/* Data Link Feature */
+#define PCI_DLF_CAP		0x04	/* Capabilities Register */
+#define  PCI_DLF_EXCHANGE_ENABLE	0x80000000  /* Data Link Feature Exchange Enable */
+
+/* Physical Layer 16.0 GT/s */
+#define PCI_PL_16GT_LE_CTRL	0x20	/* Lane Equalization Control Register */
+#define  PCI_PL_16GT_LE_CTRL_DSP_TX_PRESET_MASK		0x0000000F
+#define  PCI_PL_16GT_LE_CTRL_USP_TX_PRESET_MASK		0x000000F0
+#define  PCI_PL_16GT_LE_CTRL_USP_TX_PRESET_SHIFT	4
+
+#endif /* _RTE_PCI_REGS_H_ */
-- 
2.17.1


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [dpdk-dev] [PATCH v2 2/7] drivers: add generic API to find PCI extended cap
  2020-07-13 15:13 [dpdk-dev] [PATCH v2 0/7] qede: SR-IOV PF driver support Manish Chopra
  2020-07-13 15:13 ` [dpdk-dev] [PATCH v2 1/7] lib/librte_pci: add rte_pci_regs.h Manish Chopra
@ 2020-07-13 15:13 ` Manish Chopra
  2020-07-20  9:36   ` Gaëtan Rivet
  2020-07-13 15:13 ` [dpdk-dev] [PATCH v2 3/7] net/qede: define PCI config space specific osals Manish Chopra
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 26+ messages in thread
From: Manish Chopra @ 2020-07-13 15:13 UTC (permalink / raw)
  To: jerinjacobk, jerinj, ferruh.yigit, grive
  Cc: dev, irusskikh, rmody, GR-Everest-DPDK-Dev, anatoly.burakov,
	xavier.huwei, humin29, yisen.zhuang, xiao.w.wang, qiming.yang,
	qi.z.zhang, heinrich.kuhn

By adding generic API, this patch removes individual
functions/defines implemented by drivers to find PCI
extended capability.

Signed-off-by: Manish Chopra <manishc@marvell.com>
Signed-off-by: Igor Russkikh <irusskikh@marvell.com>
---
 drivers/bus/pci/pci_common.c               | 41 +++++++++++++++++
 drivers/bus/pci/rte_bus_pci.h              | 11 +++++
 drivers/net/ice/ice_ethdev.c               | 53 ++--------------------
 drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c | 48 ++------------------
 lib/librte_pci/rte_pci_regs.h              |  2 +-
 5 files changed, 60 insertions(+), 95 deletions(-)

diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
index a8e5fd52c..5117c8e7b 100644
--- a/drivers/bus/pci/pci_common.c
+++ b/drivers/bus/pci/pci_common.c
@@ -23,6 +23,7 @@
 #include <rte_common.h>
 #include <rte_devargs.h>
 #include <rte_vfio.h>
+#include <rte_pci_regs.h>
 
 #include "private.h"
 
@@ -665,6 +666,46 @@ rte_pci_get_iommu_class(void)
 	return iova_mode;
 }
 
+int rte_pci_find_next_ext_capability(struct rte_pci_device *dev, int cap)
+{
+	int pos = PCI_CFG_SPACE_SIZE;
+	uint32_t header;
+	int ttl;
+
+	/* minimum 8 bytes per capability */
+	ttl = (PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE) / 8;
+
+	if (rte_pci_read_config(dev, &header, 4, pos) < 0) {
+		RTE_LOG(ERR, EAL, "error in reading extended capabilities\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * If we have no capabilities, this is indicated by cap ID,
+	 * cap version and next pointer all being 0.
+	 */
+	if (header == 0)
+		return 0;
+
+	while (ttl-- > 0) {
+		if (PCI_EXT_CAP_ID(header) == cap)
+			return pos;
+
+		pos = PCI_EXT_CAP_NEXT(header);
+
+		if (pos < PCI_CFG_SPACE_SIZE)
+			break;
+
+		if (rte_pci_read_config(dev, &header, 4, pos) < 0) {
+			RTE_LOG(ERR, EAL,
+				"error in reading extended capabilities\n");
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
 struct rte_pci_bus rte_pci_bus = {
 	.bus = {
 		.scan = rte_pci_scan,
diff --git a/drivers/bus/pci/rte_bus_pci.h b/drivers/bus/pci/rte_bus_pci.h
index 29bea6d70..3cc66220a 100644
--- a/drivers/bus/pci/rte_bus_pci.h
+++ b/drivers/bus/pci/rte_bus_pci.h
@@ -224,6 +224,17 @@ void rte_pci_unmap_device(struct rte_pci_device *dev);
  */
 void rte_pci_dump(FILE *f);
 
+/**
+ * Find device's extended capability
+ *
+ *  @param dev
+ *    A pointer to rte_pci_device structure
+ *
+ *  @param cap
+ *    Extended capability to find
+ */
+int rte_pci_find_next_ext_capability(struct rte_pci_device *dev, int cap);
+
 /**
  * Register a PCI driver.
  *
diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
index 3534d18ca..0724324d2 100644
--- a/drivers/net/ice/ice_ethdev.c
+++ b/drivers/net/ice/ice_ethdev.c
@@ -4,6 +4,8 @@
 
 #include <rte_string_fns.h>
 #include <rte_ethdev_pci.h>
+#include <rte_bus_pci.h>
+#include <rte_pci_regs.h>
 
 #include <stdio.h>
 #include <sys/types.h>
@@ -1730,53 +1732,6 @@ ice_pf_setup(struct ice_pf *pf)
 	return 0;
 }
 
-/* PCIe configuration space setting */
-#define PCI_CFG_SPACE_SIZE          256
-#define PCI_CFG_SPACE_EXP_SIZE      4096
-#define PCI_EXT_CAP_ID(header)      (int)((header) & 0x0000ffff)
-#define PCI_EXT_CAP_NEXT(header)    (((header) >> 20) & 0xffc)
-#define PCI_EXT_CAP_ID_DSN          0x03
-
-static int
-ice_pci_find_next_ext_capability(struct rte_pci_device *dev, int cap)
-{
-	uint32_t header;
-	int ttl;
-	int pos = PCI_CFG_SPACE_SIZE;
-
-	/* minimum 8 bytes per capability */
-	ttl = (PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE) / 8;
-
-	if (rte_pci_read_config(dev, &header, 4, pos) < 0) {
-		PMD_INIT_LOG(ERR, "ice error reading extended capabilities\n");
-		return -1;
-	}
-
-	/*
-	 * If we have no capabilities, this is indicated by cap ID,
-	 * cap version and next pointer all being 0.
-	 */
-	if (header == 0)
-		return 0;
-
-	while (ttl-- > 0) {
-		if (PCI_EXT_CAP_ID(header) == cap)
-			return pos;
-
-		pos = PCI_EXT_CAP_NEXT(header);
-
-		if (pos < PCI_CFG_SPACE_SIZE)
-			break;
-
-		if (rte_pci_read_config(dev, &header, 4, pos) < 0) {
-			PMD_INIT_LOG(ERR, "ice error reading extended capabilities\n");
-			return -1;
-		}
-	}
-
-	return 0;
-}
-
 /*
  * Extract device serial number from PCIe Configuration Space and
  * determine the pkg file path according to the DSN.
@@ -1789,9 +1744,9 @@ ice_pkg_file_search_path(struct rte_pci_device *pci_dev, char *pkg_file)
 	uint32_t dsn_low, dsn_high;
 	memset(opt_ddp_filename, 0, ICE_MAX_PKG_FILENAME_SIZE);
 
-	pos = ice_pci_find_next_ext_capability(pci_dev, PCI_EXT_CAP_ID_DSN);
+	pos = rte_pci_find_next_ext_capability(pci_dev, PCI_EXT_CAP_ID_DSN);
 
-	if (pos) {
+	if (pos > 0) {
 		rte_pci_read_config(pci_dev, &dsn_low, 4, pos + 4);
 		rte_pci_read_config(pci_dev, &dsn_high, 4, pos + 8);
 		snprintf(opt_ddp_filename, ICE_MAX_PKG_FILENAME_SIZE,
diff --git a/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c b/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
index 0b9db974e..b11d8148a 100644
--- a/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
+++ b/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
@@ -35,6 +35,8 @@
 
 #include <rte_ethdev_pci.h>
 #include <rte_string_fns.h>
+#include <rte_bus_pci.h>
+#include <rte_pci_regs.h>
 
 #include "nfp_cpp.h"
 #include "nfp_target.h"
@@ -746,50 +748,6 @@ nfp6000_set_interface(struct rte_pci_device *dev, struct nfp_cpp *cpp)
 	return 0;
 }
 
-#define PCI_CFG_SPACE_SIZE	256
-#define PCI_CFG_SPACE_EXP_SIZE	4096
-#define PCI_EXT_CAP_ID(header)		(int)(header & 0x0000ffff)
-#define PCI_EXT_CAP_NEXT(header)	((header >> 20) & 0xffc)
-#define PCI_EXT_CAP_ID_DSN	0x03
-static int
-nfp_pci_find_next_ext_capability(struct rte_pci_device *dev, int cap)
-{
-	uint32_t header;
-	int ttl;
-	int pos = PCI_CFG_SPACE_SIZE;
-
-	/* minimum 8 bytes per capability */
-	ttl = (PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE) / 8;
-
-	if (rte_pci_read_config(dev, &header, 4, pos) < 0) {
-		printf("nfp error reading extended capabilities\n");
-		return -1;
-	}
-
-	/*
-	 * If we have no capabilities, this is indicated by cap ID,
-	 * cap version and next pointer all being 0.
-	 */
-	if (header == 0)
-		return 0;
-
-	while (ttl-- > 0) {
-		if (PCI_EXT_CAP_ID(header) == cap)
-			return pos;
-
-		pos = PCI_EXT_CAP_NEXT(header);
-		if (pos < PCI_CFG_SPACE_SIZE)
-			break;
-
-		if (rte_pci_read_config(dev, &header, 4, pos) < 0) {
-			printf("nfp error reading extended capabilities\n");
-			return -1;
-		}
-	}
-
-	return 0;
-}
-
 static int
 nfp6000_set_serial(struct rte_pci_device *dev, struct nfp_cpp *cpp)
 {
@@ -798,7 +756,7 @@ nfp6000_set_serial(struct rte_pci_device *dev, struct nfp_cpp *cpp)
 	int serial_len = 6;
 	int pos;
 
-	pos = nfp_pci_find_next_ext_capability(dev, PCI_EXT_CAP_ID_DSN);
+	pos = rte_pci_find_next_ext_capability(dev, PCI_EXT_CAP_ID_DSN);
 	if (pos <= 0) {
 		printf("PCI_EXT_CAP_ID_DSN not found. nfp set serial failed\n");
 		return -1;
diff --git a/lib/librte_pci/rte_pci_regs.h b/lib/librte_pci/rte_pci_regs.h
index 1d11f4de5..108193049 100644
--- a/lib/librte_pci/rte_pci_regs.h
+++ b/lib/librte_pci/rte_pci_regs.h
@@ -686,7 +686,7 @@
 #define PCI_EXP_SLTSTA2		58	/* Slot Status 2 */
 
 /* Extended Capabilities (PCI-X 2.0 and Express) */
-#define PCI_EXT_CAP_ID(header)		(header & 0x0000ffff)
+#define PCI_EXT_CAP_ID(header)		(int)(header & 0x0000ffff)
 #define PCI_EXT_CAP_VER(header)		((header >> 16) & 0xf)
 #define PCI_EXT_CAP_NEXT(header)	((header >> 20) & 0xffc)
 
-- 
2.17.1


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [dpdk-dev] [PATCH v2 3/7] net/qede: define PCI config space specific osals
  2020-07-13 15:13 [dpdk-dev] [PATCH v2 0/7] qede: SR-IOV PF driver support Manish Chopra
  2020-07-13 15:13 ` [dpdk-dev] [PATCH v2 1/7] lib/librte_pci: add rte_pci_regs.h Manish Chopra
  2020-07-13 15:13 ` [dpdk-dev] [PATCH v2 2/7] drivers: add generic API to find PCI extended cap Manish Chopra
@ 2020-07-13 15:13 ` Manish Chopra
  2020-07-13 15:13 ` [dpdk-dev] [PATCH v2 4/7] net/qede: configure VFs on hardware Manish Chopra
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 26+ messages in thread
From: Manish Chopra @ 2020-07-13 15:13 UTC (permalink / raw)
  To: jerinjacobk, jerinj, ferruh.yigit, grive
  Cc: dev, irusskikh, rmody, GR-Everest-DPDK-Dev, anatoly.burakov,
	xavier.huwei, humin29, yisen.zhuang, xiao.w.wang, qiming.yang,
	qi.z.zhang, heinrich.kuhn

This patch defines various PCI config space access APIs
in order to read and find IOV specific PCI capabilities.

With these definitions implemented, it enables the base
driver to do SR-IOV specific initialization and HW specific
configuration required from PF-PMD driver instance.

Signed-off-by: Manish Chopra <manishc@marvell.com>
Signed-off-by: Igor Russkikh <irusskikh@marvell.com>
Signed-off-by: Rasesh Mody <rmody@marvell.com>
---
 drivers/net/qede/base/bcm_osal.h | 18 +++++++++++++-----
 drivers/net/qede/base/ecore.h    |  3 +++
 drivers/net/qede/qede_main.c     |  1 +
 3 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/net/qede/base/bcm_osal.h b/drivers/net/qede/base/bcm_osal.h
index 5d4df5907..317e088e1 100644
--- a/drivers/net/qede/base/bcm_osal.h
+++ b/drivers/net/qede/base/bcm_osal.h
@@ -21,6 +21,11 @@
 #include <rte_ether.h>
 #include <rte_io.h>
 #include <rte_version.h>
+#include <rte_bus_pci.h>
+#include <rte_pci_regs.h>
+
+#define PCICFG_VENDOR_ID_OFFSET PCI_VENDOR_ID
+#define PCICFG_DEVICE_ID_OFFSET PCI_DEVICE_ID
 
 /* Forward declaration */
 struct ecore_dev;
@@ -286,11 +291,14 @@ typedef struct osal_list_t {
 	OSAL_LIST_PUSH_HEAD(new_entry, list)
 
 /* PCI config space */
-
-#define OSAL_PCI_READ_CONFIG_BYTE(dev, address, dst) nothing
-#define OSAL_PCI_READ_CONFIG_WORD(dev, address, dst) nothing
-#define OSAL_PCI_READ_CONFIG_DWORD(dev, address, dst) nothing
-#define OSAL_PCI_FIND_EXT_CAPABILITY(dev, pcie_id) 0
+#define OSAL_PCI_READ_CONFIG_BYTE(dev, address, dst) \
+	rte_pci_read_config((dev)->pci_dev, dst, 1, address)
+#define OSAL_PCI_READ_CONFIG_WORD(dev, address, dst) \
+	rte_pci_read_config((dev)->pci_dev, dst, 2, address)
+#define OSAL_PCI_READ_CONFIG_DWORD(dev, address, dst) \
+	rte_pci_read_config((dev)->pci_dev, dst, 4, address)
+#define OSAL_PCI_FIND_EXT_CAPABILITY(dev, cap) \
+	rte_pci_find_next_ext_capability((dev)->pci_dev, cap)
 #define OSAL_PCI_FIND_CAPABILITY(dev, pcie_id) 0
 #define OSAL_PCI_WRITE_CONFIG_WORD(dev, address, val) nothing
 #define OSAL_BAR_SIZE(dev, bar_id) 0
diff --git a/drivers/net/qede/base/ecore.h b/drivers/net/qede/base/ecore.h
index 63bd7466a..750e99a8f 100644
--- a/drivers/net/qede/base/ecore.h
+++ b/drivers/net/qede/base/ecore.h
@@ -937,6 +937,9 @@ struct ecore_dev {
 	struct ecore_dbg_feature	dbg_features[DBG_FEATURE_NUM];
 	struct ecore_dbg_params		dbg_params;
 	osal_mutex_t			dbg_lock;
+
+	/* DPDK specific ecore field */
+	struct rte_pci_device		*pci_dev;
 };
 
 enum ecore_hsi_def_type {
diff --git a/drivers/net/qede/qede_main.c b/drivers/net/qede/qede_main.c
index 987a6f1e1..d919f9f11 100644
--- a/drivers/net/qede/qede_main.c
+++ b/drivers/net/qede/qede_main.c
@@ -37,6 +37,7 @@ static void qed_init_pci(struct ecore_dev *edev, struct rte_pci_device *pci_dev)
 	edev->regview = pci_dev->mem_resource[0].addr;
 	edev->doorbells = pci_dev->mem_resource[2].addr;
 	edev->db_size = pci_dev->mem_resource[2].len;
+	edev->pci_dev = pci_dev;
 }
 
 static int
-- 
2.17.1


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [dpdk-dev] [PATCH v2 4/7] net/qede: configure VFs on hardware
  2020-07-13 15:13 [dpdk-dev] [PATCH v2 0/7] qede: SR-IOV PF driver support Manish Chopra
                   ` (2 preceding siblings ...)
  2020-07-13 15:13 ` [dpdk-dev] [PATCH v2 3/7] net/qede: define PCI config space specific osals Manish Chopra
@ 2020-07-13 15:13 ` Manish Chopra
  2020-07-13 15:13 ` [dpdk-dev] [PATCH v2 5/7] net/qede: add infrastructure support for VF load Manish Chopra
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 26+ messages in thread
From: Manish Chopra @ 2020-07-13 15:13 UTC (permalink / raw)
  To: jerinjacobk, jerinj, ferruh.yigit, grive
  Cc: dev, irusskikh, rmody, GR-Everest-DPDK-Dev, anatoly.burakov,
	xavier.huwei, humin29, yisen.zhuang, xiao.w.wang, qiming.yang,
	qi.z.zhang, heinrich.kuhn

Based on number of VFs enabled at PCI, PF-PMD driver instance
enables/configures those VFs from hardware perspective, such
that in later patches they could get required HW access to
communicate with PFs for slowpath configuration and run the
fastpath themsleves.

This patch also add two new qede IOV files [qede_sriov(.c|.h)]
under qede directory to add non-base driver IOV APIs/contents there.

Signed-off-by: Manish Chopra <manishc@marvell.com>
Signed-off-by: Igor Russkikh <irusskikh@marvell.com>
Signed-off-by: Rasesh Mody <rmody@marvell.com>
---
 drivers/net/qede/Makefile      |  1 +
 drivers/net/qede/meson.build   |  1 +
 drivers/net/qede/qede_ethdev.c |  1 +
 drivers/net/qede/qede_ethdev.h |  1 +
 drivers/net/qede/qede_if.h     |  1 +
 drivers/net/qede/qede_main.c   |  1 +
 drivers/net/qede/qede_sriov.c  | 85 ++++++++++++++++++++++++++++++++++
 drivers/net/qede/qede_sriov.h  |  9 ++++
 8 files changed, 100 insertions(+)
 create mode 100644 drivers/net/qede/qede_sriov.c
 create mode 100644 drivers/net/qede/qede_sriov.h

diff --git a/drivers/net/qede/Makefile b/drivers/net/qede/Makefile
index 0e8a67b0d..c57bef0e3 100644
--- a/drivers/net/qede/Makefile
+++ b/drivers/net/qede/Makefile
@@ -105,5 +105,6 @@ SRCS-$(CONFIG_RTE_LIBRTE_QEDE_PMD) += qede_rxtx.c
 SRCS-$(CONFIG_RTE_LIBRTE_QEDE_PMD) += qede_filter.c
 SRCS-$(CONFIG_RTE_LIBRTE_QEDE_PMD) += qede_debug.c
 SRCS-$(CONFIG_RTE_LIBRTE_QEDE_PMD) += qede_regs.c
+SRCS-$(CONFIG_RTE_LIBRTE_QEDE_PMD) += qede_sriov.c
 
 include $(RTE_SDK)/mk/rte.lib.mk
diff --git a/drivers/net/qede/meson.build b/drivers/net/qede/meson.build
index 05c9bff73..ff0ac0b03 100644
--- a/drivers/net/qede/meson.build
+++ b/drivers/net/qede/meson.build
@@ -11,6 +11,7 @@ sources = files(
 	'qede_rxtx.c',
 	'qede_debug.c',
 	'qede_regs.c',
+	'qede_sriov.c',
 )
 
 if cc.has_argument('-Wno-format-nonliteral')
diff --git a/drivers/net/qede/qede_ethdev.c b/drivers/net/qede/qede_ethdev.c
index e5a2581dd..6008d88e7 100644
--- a/drivers/net/qede/qede_ethdev.c
+++ b/drivers/net/qede/qede_ethdev.c
@@ -2703,6 +2703,7 @@ static int qede_common_dev_init(struct rte_eth_dev *eth_dev, bool is_vf)
 		adapter->vxlan.enable = false;
 		adapter->geneve.enable = false;
 		adapter->ipgre.enable = false;
+		qed_ops->sriov_configure(edev, pci_dev->max_vfs);
 	}
 
 	DP_INFO(edev, "MAC address : %02x:%02x:%02x:%02x:%02x:%02x\n",
diff --git a/drivers/net/qede/qede_ethdev.h b/drivers/net/qede/qede_ethdev.h
index 76c5dae3b..4fb77b05c 100644
--- a/drivers/net/qede/qede_ethdev.h
+++ b/drivers/net/qede/qede_ethdev.h
@@ -34,6 +34,7 @@
 #include "base/ecore_l2.h"
 #include "base/ecore_vf.h"
 
+#include "qede_sriov.h"
 #include "qede_logs.h"
 #include "qede_if.h"
 #include "qede_rxtx.h"
diff --git a/drivers/net/qede/qede_if.h b/drivers/net/qede/qede_if.h
index c5ae3fb2e..1693a243f 100644
--- a/drivers/net/qede/qede_if.h
+++ b/drivers/net/qede/qede_if.h
@@ -82,6 +82,7 @@ struct qed_eth_ops {
 	const struct qed_common_ops *common;
 	int (*fill_dev_info)(struct ecore_dev *edev,
 			     struct qed_dev_eth_info *info);
+	void (*sriov_configure)(struct ecore_dev *edev, int num_vfs);
 };
 
 struct qed_link_params {
diff --git a/drivers/net/qede/qede_main.c b/drivers/net/qede/qede_main.c
index d919f9f11..c37e8ebe0 100644
--- a/drivers/net/qede/qede_main.c
+++ b/drivers/net/qede/qede_main.c
@@ -822,6 +822,7 @@ const struct qed_common_ops qed_common_ops_pass = {
 const struct qed_eth_ops qed_eth_ops_pass = {
 	INIT_STRUCT_FIELD(common, &qed_common_ops_pass),
 	INIT_STRUCT_FIELD(fill_dev_info, &qed_fill_eth_dev_info),
+	INIT_STRUCT_FIELD(sriov_configure, &qed_sriov_configure),
 };
 
 const struct qed_eth_ops *qed_get_eth_ops(void)
diff --git a/drivers/net/qede/qede_sriov.c b/drivers/net/qede/qede_sriov.c
new file mode 100644
index 000000000..ba4384e90
--- /dev/null
+++ b/drivers/net/qede/qede_sriov.c
@@ -0,0 +1,85 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright (c) 2020 Marvell.
+ * All rights reserved.
+ * www.marvell.com
+ */
+
+#include "qede_sriov.h"
+
+static void qed_sriov_enable_qid_config(struct ecore_hwfn *hwfn,
+					u16 vfid,
+					struct ecore_iov_vf_init_params *params)
+{
+	u16 num_pf_l2_queues, base, i;
+
+	/* Since we have an equal resource distribution per-VF, and we assume
+	 * PF has acquired its first queues, we start setting sequentially from
+	 * there.
+	 */
+	num_pf_l2_queues = (u16)FEAT_NUM(hwfn, ECORE_PF_L2_QUE);
+
+	base = num_pf_l2_queues + vfid * params->num_queues;
+	params->rel_vf_id = vfid;
+
+	for (i = 0; i < params->num_queues; i++) {
+		params->req_rx_queue[i] = base + i;
+		params->req_tx_queue[i] = base + i;
+	}
+
+	/* PF uses indices 0 for itself; Set vport/RSS afterwards */
+	params->vport_id = vfid + 1;
+	params->rss_eng_id = vfid + 1;
+}
+
+static void qed_sriov_enable(struct ecore_dev *edev, int num)
+{
+	struct ecore_iov_vf_init_params params;
+	struct ecore_hwfn *p_hwfn;
+	struct ecore_ptt *p_ptt;
+	int i, j, rc;
+
+	if ((u32)num >= RESC_NUM(&edev->hwfns[0], ECORE_VPORT)) {
+		DP_NOTICE(edev, false, "Can start at most %d VFs\n",
+			  RESC_NUM(&edev->hwfns[0], ECORE_VPORT) - 1);
+		return;
+	}
+
+	OSAL_MEMSET(&params, 0, sizeof(struct ecore_iov_vf_init_params));
+
+	for_each_hwfn(edev, j) {
+		int feat_num;
+
+		p_hwfn = &edev->hwfns[j];
+		p_ptt = ecore_ptt_acquire(p_hwfn);
+		feat_num = FEAT_NUM(p_hwfn, ECORE_VF_L2_QUE) / num;
+
+		params.num_queues = OSAL_MIN_T(int, feat_num, 16);
+
+		for (i = 0; i < num; i++) {
+			if (!ecore_iov_is_valid_vfid(p_hwfn, i, false, true))
+				continue;
+
+			qed_sriov_enable_qid_config(p_hwfn, i, &params);
+
+			rc = ecore_iov_init_hw_for_vf(p_hwfn, p_ptt, &params);
+			if (rc) {
+				DP_ERR(edev, "Failed to enable VF[%d]\n", i);
+				ecore_ptt_release(p_hwfn, p_ptt);
+				return;
+			}
+		}
+
+		ecore_ptt_release(p_hwfn, p_ptt);
+	}
+}
+
+void qed_sriov_configure(struct ecore_dev *edev, int num_vfs_param)
+{
+	if (!IS_ECORE_SRIOV(edev)) {
+		DP_VERBOSE(edev, ECORE_MSG_IOV, "SR-IOV is not supported\n");
+		return;
+	}
+
+	if (num_vfs_param)
+		qed_sriov_enable(edev, num_vfs_param);
+}
diff --git a/drivers/net/qede/qede_sriov.h b/drivers/net/qede/qede_sriov.h
new file mode 100644
index 000000000..6c85b1dd5
--- /dev/null
+++ b/drivers/net/qede/qede_sriov.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright (c) 2020 Marvell.
+ * All rights reserved.
+ * www.marvell.com
+ */
+
+#include "qede_ethdev.h"
+
+void qed_sriov_configure(struct ecore_dev *edev, int num_vfs_param);
-- 
2.17.1


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [dpdk-dev] [PATCH v2 5/7] net/qede: add infrastructure support for VF load
  2020-07-13 15:13 [dpdk-dev] [PATCH v2 0/7] qede: SR-IOV PF driver support Manish Chopra
                   ` (3 preceding siblings ...)
  2020-07-13 15:13 ` [dpdk-dev] [PATCH v2 4/7] net/qede: configure VFs on hardware Manish Chopra
@ 2020-07-13 15:13 ` Manish Chopra
  2020-07-13 15:13 ` [dpdk-dev] [PATCH v2 6/7] net/qede: initialize VF MAC and link Manish Chopra
  2020-07-13 15:13 ` [dpdk-dev] [PATCH v2 7/7] net/qede: add VF FLR support Manish Chopra
  6 siblings, 0 replies; 26+ messages in thread
From: Manish Chopra @ 2020-07-13 15:13 UTC (permalink / raw)
  To: jerinjacobk, jerinj, ferruh.yigit, grive
  Cc: dev, irusskikh, rmody, GR-Everest-DPDK-Dev, anatoly.burakov,
	xavier.huwei, humin29, yisen.zhuang, xiao.w.wang, qiming.yang,
	qi.z.zhang, heinrich.kuhn

This patch adds necessary infrastructure support (required to handle
messages from VF and sending ramrod on behalf of VF's configuration
request from alarm handler context) to start/load the VF-PMD driver
instance on top of PF-PMD driver instance.

Signed-off-by: Manish Chopra <manishc@marvell.com>
Signed-off-by: Igor Russkikh <irusskikh@marvell.com>
Signed-off-by: Rasesh Mody <rmody@marvell.com>
---
 drivers/net/qede/base/bcm_osal.c      | 26 ++++++++++++
 drivers/net/qede/base/bcm_osal.h      | 11 +++--
 drivers/net/qede/base/ecore.h         |  4 ++
 drivers/net/qede/base/ecore_iov_api.h |  3 ++
 drivers/net/qede/qede_ethdev.c        |  2 +
 drivers/net/qede/qede_main.c          |  4 +-
 drivers/net/qede/qede_sriov.c         | 61 +++++++++++++++++++++++++++
 drivers/net/qede/qede_sriov.h         | 16 ++++++-
 8 files changed, 121 insertions(+), 6 deletions(-)

diff --git a/drivers/net/qede/base/bcm_osal.c b/drivers/net/qede/base/bcm_osal.c
index 65837b53d..ef47339df 100644
--- a/drivers/net/qede/base/bcm_osal.c
+++ b/drivers/net/qede/base/bcm_osal.c
@@ -14,6 +14,32 @@
 #include "ecore_iov_api.h"
 #include "ecore_mcp_api.h"
 #include "ecore_l2_api.h"
+#include "../qede_sriov.h"
+
+int osal_pf_vf_msg(struct ecore_hwfn *p_hwfn)
+{
+	int rc;
+
+	rc = qed_schedule_iov(p_hwfn, QED_IOV_WQ_MSG_FLAG);
+	if (rc) {
+		DP_VERBOSE(p_hwfn, ECORE_MSG_IOV,
+			   "Failed to schedule alarm handler rc=%d\n", rc);
+	}
+
+	return rc;
+}
+
+void osal_poll_mode_dpc(osal_int_ptr_t hwfn_cookie)
+{
+	struct ecore_hwfn *p_hwfn = (struct ecore_hwfn *)hwfn_cookie;
+
+	if (!p_hwfn)
+		return;
+
+	OSAL_SPIN_LOCK(&p_hwfn->spq_lock);
+	ecore_int_sp_dpc((osal_int_ptr_t)(p_hwfn));
+	OSAL_SPIN_UNLOCK(&p_hwfn->spq_lock);
+}
 
 /* Array of memzone pointers */
 static const struct rte_memzone *ecore_mz_mapping[RTE_MAX_MEMZONE];
diff --git a/drivers/net/qede/base/bcm_osal.h b/drivers/net/qede/base/bcm_osal.h
index 317e088e1..fad441752 100644
--- a/drivers/net/qede/base/bcm_osal.h
+++ b/drivers/net/qede/base/bcm_osal.h
@@ -182,9 +182,12 @@ typedef pthread_mutex_t osal_mutex_t;
 
 /* DPC */
 
+void osal_poll_mode_dpc(osal_int_ptr_t hwfn_cookie);
 #define OSAL_DPC_ALLOC(hwfn) OSAL_ALLOC(hwfn, GFP, sizeof(osal_dpc_t))
-#define OSAL_DPC_INIT(dpc, hwfn) nothing
-#define OSAL_POLL_MODE_DPC(hwfn) nothing
+#define OSAL_DPC_INIT(dpc, hwfn) \
+	OSAL_SPIN_LOCK_INIT(&(hwfn)->spq_lock)
+#define OSAL_POLL_MODE_DPC(hwfn) \
+	osal_poll_mode_dpc((osal_int_ptr_t)(p_hwfn))
 #define OSAL_DPC_SYNC(hwfn) nothing
 
 /* Lists */
@@ -349,10 +352,12 @@ u32 qede_find_first_zero_bit(u32 *bitmap, u32 length);
 
 /* SR-IOV channel */
 
+int osal_pf_vf_msg(struct ecore_hwfn *p_hwfn);
 #define OSAL_VF_FLR_UPDATE(hwfn) nothing
 #define OSAL_VF_SEND_MSG2PF(dev, done, msg, reply_addr, msg_size, reply_size) 0
 #define OSAL_VF_CQE_COMPLETION(_dev_p, _cqe, _protocol)	(0)
-#define OSAL_PF_VF_MSG(hwfn, vfid) 0
+#define OSAL_PF_VF_MSG(hwfn, vfid) \
+	osal_pf_vf_msg(hwfn)
 #define OSAL_PF_VF_MALICIOUS(hwfn, vfid) nothing
 #define OSAL_IOV_CHK_UCAST(hwfn, vfid, params) 0
 #define OSAL_IOV_POST_START_VPORT(hwfn, vf, vport_id, opaque_fid) nothing
diff --git a/drivers/net/qede/base/ecore.h b/drivers/net/qede/base/ecore.h
index 750e99a8f..6c8e6d407 100644
--- a/drivers/net/qede/base/ecore.h
+++ b/drivers/net/qede/base/ecore.h
@@ -714,6 +714,10 @@ struct ecore_hwfn {
 
 	/* @DPDK */
 	struct ecore_ptt		*p_arfs_ptt;
+
+	/* DPDK specific, not the part of vanilla ecore */
+	osal_spinlock_t spq_lock;
+	u32 iov_task_flags;
 };
 
 enum ecore_mf_mode {
diff --git a/drivers/net/qede/base/ecore_iov_api.h b/drivers/net/qede/base/ecore_iov_api.h
index 545001812..bd7c5703f 100644
--- a/drivers/net/qede/base/ecore_iov_api.h
+++ b/drivers/net/qede/base/ecore_iov_api.h
@@ -14,6 +14,9 @@
 #define ECORE_ETH_VF_NUM_VLAN_FILTERS 2
 #define ECORE_VF_ARRAY_LENGTH (3)
 
+#define ECORE_VF_ARRAY_GET_VFID(arr, vfid)	\
+	(((arr)[(vfid) / 64]) & (1ULL << ((vfid) % 64)))
+
 #define IS_VF(p_dev)		((p_dev)->b_is_vf)
 #define IS_PF(p_dev)		(!((p_dev)->b_is_vf))
 #ifdef CONFIG_ECORE_SRIOV
diff --git a/drivers/net/qede/qede_ethdev.c b/drivers/net/qede/qede_ethdev.c
index 6008d88e7..a30294f2e 100644
--- a/drivers/net/qede/qede_ethdev.c
+++ b/drivers/net/qede/qede_ethdev.c
@@ -281,7 +281,9 @@ qede_fw_version_get(struct rte_eth_dev *dev, char *fw_ver, size_t fw_size)
 
 static void qede_interrupt_action(struct ecore_hwfn *p_hwfn)
 {
+	OSAL_SPIN_LOCK(&p_hwfn->spq_lock);
 	ecore_int_sp_dpc((osal_int_ptr_t)(p_hwfn));
+	OSAL_SPIN_UNLOCK(&p_hwfn->spq_lock);
 }
 
 static void
diff --git a/drivers/net/qede/qede_main.c b/drivers/net/qede/qede_main.c
index c37e8ebe0..0afacc064 100644
--- a/drivers/net/qede/qede_main.c
+++ b/drivers/net/qede/qede_main.c
@@ -221,7 +221,9 @@ static void qed_stop_iov_task(struct ecore_dev *edev)
 
 	for_each_hwfn(edev, i) {
 		p_hwfn = &edev->hwfns[i];
-		if (!IS_PF(edev))
+		if (IS_PF(edev))
+			rte_eal_alarm_cancel(qed_iov_pf_task, p_hwfn);
+		else
 			rte_eal_alarm_cancel(qede_vf_task, p_hwfn);
 	}
 }
diff --git a/drivers/net/qede/qede_sriov.c b/drivers/net/qede/qede_sriov.c
index ba4384e90..6d620dde8 100644
--- a/drivers/net/qede/qede_sriov.c
+++ b/drivers/net/qede/qede_sriov.c
@@ -4,6 +4,14 @@
  * www.marvell.com
  */
 
+#include <rte_alarm.h>
+
+#include "base/bcm_osal.h"
+#include "base/ecore.h"
+#include "base/ecore_sriov.h"
+#include "base/ecore_mcp.h"
+#include "base/ecore_vf.h"
+
 #include "qede_sriov.h"
 
 static void qed_sriov_enable_qid_config(struct ecore_hwfn *hwfn,
@@ -83,3 +91,56 @@ void qed_sriov_configure(struct ecore_dev *edev, int num_vfs_param)
 	if (num_vfs_param)
 		qed_sriov_enable(edev, num_vfs_param);
 }
+
+static void qed_handle_vf_msg(struct ecore_hwfn *hwfn)
+{
+	u64 events[ECORE_VF_ARRAY_LENGTH];
+	struct ecore_ptt *ptt;
+	int i;
+
+	ptt = ecore_ptt_acquire(hwfn);
+	if (!ptt) {
+		DP_NOTICE(hwfn, true, "PTT acquire failed\n");
+		qed_schedule_iov(hwfn, QED_IOV_WQ_MSG_FLAG);
+		return;
+	}
+
+	ecore_iov_pf_get_pending_events(hwfn, events);
+
+	ecore_for_each_vf(hwfn, i) {
+		/* Skip VFs with no pending messages */
+		if (!ECORE_VF_ARRAY_GET_VFID(events, i))
+			continue;
+
+		DP_VERBOSE(hwfn, ECORE_MSG_IOV,
+			   "Handling VF message from VF 0x%02x [Abs 0x%02x]\n",
+			   i, hwfn->p_dev->p_iov_info->first_vf_in_pf + i);
+
+		/* Copy VF's message to PF's request buffer for that VF */
+		if (ecore_iov_copy_vf_msg(hwfn, ptt, i))
+			continue;
+
+		ecore_iov_process_mbx_req(hwfn, ptt, i);
+	}
+
+	ecore_ptt_release(hwfn, ptt);
+}
+
+void qed_iov_pf_task(void *arg)
+{
+	struct ecore_hwfn *p_hwfn = arg;
+
+	if (OSAL_GET_BIT(QED_IOV_WQ_MSG_FLAG, &p_hwfn->iov_task_flags)) {
+		OSAL_CLEAR_BIT(QED_IOV_WQ_MSG_FLAG, &p_hwfn->iov_task_flags);
+		qed_handle_vf_msg(p_hwfn);
+	}
+}
+
+int qed_schedule_iov(struct ecore_hwfn *p_hwfn, enum qed_iov_wq_flag flag)
+{
+	DP_VERBOSE(p_hwfn, ECORE_MSG_IOV, "Scheduling iov task [Flag: %d]\n",
+		   flag);
+
+	OSAL_SET_BIT(flag, &p_hwfn->iov_task_flags);
+	return rte_eal_alarm_set(1, qed_iov_pf_task, p_hwfn);
+}
diff --git a/drivers/net/qede/qede_sriov.h b/drivers/net/qede/qede_sriov.h
index 6c85b1dd5..8b7fa7daa 100644
--- a/drivers/net/qede/qede_sriov.h
+++ b/drivers/net/qede/qede_sriov.h
@@ -4,6 +4,18 @@
  * www.marvell.com
  */
 
-#include "qede_ethdev.h"
-
 void qed_sriov_configure(struct ecore_dev *edev, int num_vfs_param);
+
+enum qed_iov_wq_flag {
+	QED_IOV_WQ_MSG_FLAG,
+	QED_IOV_WQ_SET_UNICAST_FILTER_FLAG,
+	QED_IOV_WQ_BULLETIN_UPDATE_FLAG,
+	QED_IOV_WQ_STOP_WQ_FLAG,
+	QED_IOV_WQ_FLR_FLAG,
+	QED_IOV_WQ_TRUST_FLAG,
+	QED_IOV_WQ_VF_FORCE_LINK_QUERY_FLAG,
+	QED_IOV_WQ_DB_REC_HANDLER,
+};
+
+int qed_schedule_iov(struct ecore_hwfn *p_hwfn, enum qed_iov_wq_flag flag);
+void qed_iov_pf_task(void *arg);
-- 
2.17.1


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [dpdk-dev] [PATCH v2 6/7] net/qede: initialize VF MAC and link
  2020-07-13 15:13 [dpdk-dev] [PATCH v2 0/7] qede: SR-IOV PF driver support Manish Chopra
                   ` (4 preceding siblings ...)
  2020-07-13 15:13 ` [dpdk-dev] [PATCH v2 5/7] net/qede: add infrastructure support for VF load Manish Chopra
@ 2020-07-13 15:13 ` Manish Chopra
  2020-07-13 15:13 ` [dpdk-dev] [PATCH v2 7/7] net/qede: add VF FLR support Manish Chopra
  6 siblings, 0 replies; 26+ messages in thread
From: Manish Chopra @ 2020-07-13 15:13 UTC (permalink / raw)
  To: jerinjacobk, jerinj, ferruh.yigit, grive
  Cc: dev, irusskikh, rmody, GR-Everest-DPDK-Dev, anatoly.burakov,
	xavier.huwei, humin29, yisen.zhuang, xiao.w.wang, qiming.yang,
	qi.z.zhang, heinrich.kuhn

This patch configures VFs with random mac if no MAC is
provided by the PF/bulletin. This also adds required bulletin
APIs by PF-PMD driver to communicate LINK properties/changes to
the VFs through bulletin update mechanism.

With these changes, VF-PMD instance is able to run
fastpath over PF-PMD driver instance.

Signed-off-by: Manish Chopra <manishc@marvell.com>
Signed-off-by: Igor Russkikh <irusskikh@marvell.com>
Signed-off-by: Rasesh Mody <rmody@marvell.com>
---
 drivers/net/qede/qede_ethdev.c | 34 ++++++++++++++++++++-
 drivers/net/qede/qede_main.c   |  7 ++++-
 drivers/net/qede/qede_sriov.c  | 55 ++++++++++++++++++++++++++++++++++
 drivers/net/qede/qede_sriov.h  |  1 +
 4 files changed, 95 insertions(+), 2 deletions(-)

diff --git a/drivers/net/qede/qede_ethdev.c b/drivers/net/qede/qede_ethdev.c
index a30294f2e..e39629481 100644
--- a/drivers/net/qede/qede_ethdev.c
+++ b/drivers/net/qede/qede_ethdev.c
@@ -2482,6 +2482,24 @@ static void qede_update_pf_params(struct ecore_dev *edev)
 	qed_ops->common->update_pf_params(edev, &pf_params);
 }
 
+static void qede_generate_random_mac_addr(struct rte_ether_addr *mac_addr)
+{
+	uint64_t random;
+
+	/* Set Organizationally Unique Identifier (OUI) prefix. */
+	mac_addr->addr_bytes[0] = 0x00;
+	mac_addr->addr_bytes[1] = 0x09;
+	mac_addr->addr_bytes[2] = 0xC0;
+
+	/* Force indication of locally assigned MAC address. */
+	mac_addr->addr_bytes[0] |= RTE_ETHER_LOCAL_ADMIN_ADDR;
+
+	/* Generate the last 3 bytes of the MAC address with a random number. */
+	random = rte_rand();
+
+	memcpy(&mac_addr->addr_bytes[3], &random, 3);
+}
+
 static int qede_common_dev_init(struct rte_eth_dev *eth_dev, bool is_vf)
 {
 	struct rte_pci_device *pci_dev;
@@ -2494,7 +2512,7 @@ static int qede_common_dev_init(struct rte_eth_dev *eth_dev, bool is_vf)
 	uint8_t bulletin_change;
 	uint8_t vf_mac[RTE_ETHER_ADDR_LEN];
 	uint8_t is_mac_forced;
-	bool is_mac_exist;
+	bool is_mac_exist = false;
 	/* Fix up ecore debug level */
 	uint32_t dp_module = ~0 & ~ECORE_MSG_HW;
 	uint8_t dp_level = ECORE_LEVEL_VERBOSE;
@@ -2672,6 +2690,20 @@ static int qede_common_dev_init(struct rte_eth_dev *eth_dev, bool is_vf)
 				DP_ERR(edev, "No VF macaddr assigned\n");
 			}
 		}
+
+		/* If MAC doesn't exist from PF, generate random one */
+		if (!is_mac_exist) {
+			struct rte_ether_addr *mac_addr;
+
+			mac_addr = (struct rte_ether_addr *)&vf_mac;
+			qede_generate_random_mac_addr(mac_addr);
+
+			rte_ether_addr_copy(mac_addr,
+					    &eth_dev->data->mac_addrs[0]);
+
+			rte_ether_addr_copy(&eth_dev->data->mac_addrs[0],
+					    &adapter->primary_mac);
+		}
 	}
 
 	eth_dev->dev_ops = (is_vf) ? &qede_eth_vf_dev_ops : &qede_eth_dev_ops;
diff --git a/drivers/net/qede/qede_main.c b/drivers/net/qede/qede_main.c
index 0afacc064..805a95e3c 100644
--- a/drivers/net/qede/qede_main.c
+++ b/drivers/net/qede/qede_main.c
@@ -651,10 +651,15 @@ void qed_link_update(struct ecore_hwfn *hwfn)
 	struct ecore_dev *edev = hwfn->p_dev;
 	struct qede_dev *qdev = (struct qede_dev *)edev;
 	struct rte_eth_dev *dev = (struct rte_eth_dev *)qdev->ethdev;
+	int rc;
+
+	rc = qede_link_update(dev, 0);
+	qed_inform_vf_link_state(hwfn);
 
-	if (!qede_link_update(dev, 0))
+	if (!rc) {
 		_rte_eth_dev_callback_process(dev,
 					      RTE_ETH_EVENT_INTR_LSC, NULL);
+	}
 }
 
 static int qed_drain(struct ecore_dev *edev)
diff --git a/drivers/net/qede/qede_sriov.c b/drivers/net/qede/qede_sriov.c
index 6d620dde8..93f7a2a55 100644
--- a/drivers/net/qede/qede_sriov.c
+++ b/drivers/net/qede/qede_sriov.c
@@ -126,6 +126,28 @@ static void qed_handle_vf_msg(struct ecore_hwfn *hwfn)
 	ecore_ptt_release(hwfn, ptt);
 }
 
+static void qed_handle_bulletin_post(struct ecore_hwfn *hwfn)
+{
+	struct ecore_ptt *ptt;
+	int i;
+
+	ptt = ecore_ptt_acquire(hwfn);
+	if (!ptt) {
+		DP_NOTICE(hwfn, true, "PTT acquire failed\n");
+		qed_schedule_iov(hwfn, QED_IOV_WQ_BULLETIN_UPDATE_FLAG);
+		return;
+	}
+
+	/* TODO - at the moment update bulletin board of all VFs.
+	 * if this proves to costly, we can mark VFs that need their
+	 * bulletins updated.
+	 */
+	ecore_for_each_vf(hwfn, i)
+		ecore_iov_post_vf_bulletin(hwfn, i, ptt);
+
+	ecore_ptt_release(hwfn, ptt);
+}
+
 void qed_iov_pf_task(void *arg)
 {
 	struct ecore_hwfn *p_hwfn = arg;
@@ -134,6 +156,13 @@ void qed_iov_pf_task(void *arg)
 		OSAL_CLEAR_BIT(QED_IOV_WQ_MSG_FLAG, &p_hwfn->iov_task_flags);
 		qed_handle_vf_msg(p_hwfn);
 	}
+
+	if (OSAL_GET_BIT(QED_IOV_WQ_BULLETIN_UPDATE_FLAG,
+			 &p_hwfn->iov_task_flags)) {
+		OSAL_CLEAR_BIT(QED_IOV_WQ_BULLETIN_UPDATE_FLAG,
+			       &p_hwfn->iov_task_flags);
+		qed_handle_bulletin_post(p_hwfn);
+	}
 }
 
 int qed_schedule_iov(struct ecore_hwfn *p_hwfn, enum qed_iov_wq_flag flag)
@@ -144,3 +173,29 @@ int qed_schedule_iov(struct ecore_hwfn *p_hwfn, enum qed_iov_wq_flag flag)
 	OSAL_SET_BIT(flag, &p_hwfn->iov_task_flags);
 	return rte_eal_alarm_set(1, qed_iov_pf_task, p_hwfn);
 }
+
+void qed_inform_vf_link_state(struct ecore_hwfn *hwfn)
+{
+	struct ecore_hwfn *lead_hwfn = ECORE_LEADING_HWFN(hwfn->p_dev);
+	struct ecore_mcp_link_capabilities caps;
+	struct ecore_mcp_link_params params;
+	struct ecore_mcp_link_state link;
+	int i;
+
+	if (!hwfn->pf_iov_info)
+		return;
+
+	rte_memcpy(&params, ecore_mcp_get_link_params(lead_hwfn),
+		   sizeof(params));
+	rte_memcpy(&link, ecore_mcp_get_link_state(lead_hwfn), sizeof(link));
+	rte_memcpy(&caps, ecore_mcp_get_link_capabilities(lead_hwfn),
+		   sizeof(caps));
+
+	/* Update bulletin of all future possible VFs with link configuration */
+	for (i = 0; i < hwfn->p_dev->p_iov_info->total_vfs; i++) {
+		ecore_iov_set_link(hwfn, i,
+				   &params, &link, &caps);
+	}
+
+	qed_schedule_iov(hwfn, QED_IOV_WQ_BULLETIN_UPDATE_FLAG);
+}
diff --git a/drivers/net/qede/qede_sriov.h b/drivers/net/qede/qede_sriov.h
index 8b7fa7daa..e58ecc2a5 100644
--- a/drivers/net/qede/qede_sriov.h
+++ b/drivers/net/qede/qede_sriov.h
@@ -17,5 +17,6 @@ enum qed_iov_wq_flag {
 	QED_IOV_WQ_DB_REC_HANDLER,
 };
 
+void qed_inform_vf_link_state(struct ecore_hwfn *hwfn);
 int qed_schedule_iov(struct ecore_hwfn *p_hwfn, enum qed_iov_wq_flag flag);
 void qed_iov_pf_task(void *arg);
-- 
2.17.1


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [dpdk-dev] [PATCH v2 7/7] net/qede: add VF FLR support
  2020-07-13 15:13 [dpdk-dev] [PATCH v2 0/7] qede: SR-IOV PF driver support Manish Chopra
                   ` (5 preceding siblings ...)
  2020-07-13 15:13 ` [dpdk-dev] [PATCH v2 6/7] net/qede: initialize VF MAC and link Manish Chopra
@ 2020-07-13 15:13 ` Manish Chopra
  6 siblings, 0 replies; 26+ messages in thread
From: Manish Chopra @ 2020-07-13 15:13 UTC (permalink / raw)
  To: jerinjacobk, jerinj, ferruh.yigit, grive
  Cc: dev, irusskikh, rmody, GR-Everest-DPDK-Dev, anatoly.burakov,
	xavier.huwei, humin29, yisen.zhuang, xiao.w.wang, qiming.yang,
	qi.z.zhang, heinrich.kuhn

This patch adds required bit to handle VF FLR
indication from Management FW (MFW) of the device

With that VFs were able to load in VM (VF attached as PCI
passthrough to the guest VM) followed by FLR successfully

Updated the docs/guides with the feature support

Signed-off-by: Manish Chopra <manishc@marvell.com>
Signed-off-by: Igor Russkikh <irusskikh@marvell.com>
Signed-off-by: Rasesh Mody <rmody@marvell.com>
---
 doc/guides/nics/features/qede.ini |  1 +
 doc/guides/nics/qede.rst          |  7 +------
 drivers/net/qede/base/bcm_osal.c  |  5 +++++
 drivers/net/qede/base/bcm_osal.h  |  4 +++-
 drivers/net/qede/qede_sriov.c     | 18 ++++++++++++++++++
 5 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/doc/guides/nics/features/qede.ini b/doc/guides/nics/features/qede.ini
index f8716523e..46fba8e6c 100644
--- a/doc/guides/nics/features/qede.ini
+++ b/doc/guides/nics/features/qede.ini
@@ -32,6 +32,7 @@ Basic stats          = Y
 Extended stats       = Y
 Stats per queue      = Y
 Registers dump       = Y
+SR-IOV               = Y
 Multiprocess aware   = Y
 Linux UIO            = Y
 Linux VFIO           = Y
diff --git a/doc/guides/nics/qede.rst b/doc/guides/nics/qede.rst
index 5b2f86895..e874915c2 100644
--- a/doc/guides/nics/qede.rst
+++ b/doc/guides/nics/qede.rst
@@ -34,18 +34,13 @@ Supported Features
 - VLAN offload - Filtering and stripping
 - N-tuple filter and flow director (limited support)
 - NPAR (NIC Partitioning)
-- SR-IOV VF
+- SR-IOV PF and VF
 - GRE Tunneling offload
 - GENEVE Tunneling offload
 - VXLAN Tunneling offload
 - MPLSoUDP Tx Tunneling offload
 - Generic flow API
 
-Non-supported Features
-----------------------
-
-- SR-IOV PF
-
 Co-existence considerations
 ---------------------------
 
diff --git a/drivers/net/qede/base/bcm_osal.c b/drivers/net/qede/base/bcm_osal.c
index ef47339df..44a8692f5 100644
--- a/drivers/net/qede/base/bcm_osal.c
+++ b/drivers/net/qede/base/bcm_osal.c
@@ -29,6 +29,11 @@ int osal_pf_vf_msg(struct ecore_hwfn *p_hwfn)
 	return rc;
 }
 
+void osal_vf_flr_update(struct ecore_hwfn *p_hwfn)
+{
+	qed_schedule_iov(p_hwfn, QED_IOV_WQ_FLR_FLAG);
+}
+
 void osal_poll_mode_dpc(osal_int_ptr_t hwfn_cookie)
 {
 	struct ecore_hwfn *p_hwfn = (struct ecore_hwfn *)hwfn_cookie;
diff --git a/drivers/net/qede/base/bcm_osal.h b/drivers/net/qede/base/bcm_osal.h
index fad441752..75084ce23 100644
--- a/drivers/net/qede/base/bcm_osal.h
+++ b/drivers/net/qede/base/bcm_osal.h
@@ -353,7 +353,9 @@ u32 qede_find_first_zero_bit(u32 *bitmap, u32 length);
 /* SR-IOV channel */
 
 int osal_pf_vf_msg(struct ecore_hwfn *p_hwfn);
-#define OSAL_VF_FLR_UPDATE(hwfn) nothing
+void osal_vf_flr_update(struct ecore_hwfn *p_hwfn);
+#define OSAL_VF_FLR_UPDATE(hwfn) \
+	osal_vf_flr_update(hwfn)
 #define OSAL_VF_SEND_MSG2PF(dev, done, msg, reply_addr, msg_size, reply_size) 0
 #define OSAL_VF_CQE_COMPLETION(_dev_p, _cqe, _protocol)	(0)
 #define OSAL_PF_VF_MSG(hwfn, vfid) \
diff --git a/drivers/net/qede/qede_sriov.c b/drivers/net/qede/qede_sriov.c
index 93f7a2a55..0b99a8d6f 100644
--- a/drivers/net/qede/qede_sriov.c
+++ b/drivers/net/qede/qede_sriov.c
@@ -151,6 +151,7 @@ static void qed_handle_bulletin_post(struct ecore_hwfn *hwfn)
 void qed_iov_pf_task(void *arg)
 {
 	struct ecore_hwfn *p_hwfn = arg;
+	int rc;
 
 	if (OSAL_GET_BIT(QED_IOV_WQ_MSG_FLAG, &p_hwfn->iov_task_flags)) {
 		OSAL_CLEAR_BIT(QED_IOV_WQ_MSG_FLAG, &p_hwfn->iov_task_flags);
@@ -163,6 +164,23 @@ void qed_iov_pf_task(void *arg)
 			       &p_hwfn->iov_task_flags);
 		qed_handle_bulletin_post(p_hwfn);
 	}
+
+	if (OSAL_GET_BIT(QED_IOV_WQ_FLR_FLAG, &p_hwfn->iov_task_flags)) {
+		struct ecore_ptt *p_ptt = ecore_ptt_acquire(p_hwfn);
+
+		OSAL_CLEAR_BIT(QED_IOV_WQ_FLR_FLAG, &p_hwfn->iov_task_flags);
+
+		if (!p_ptt) {
+			qed_schedule_iov(p_hwfn, QED_IOV_WQ_FLR_FLAG);
+			return;
+		}
+
+		rc = ecore_iov_vf_flr_cleanup(p_hwfn, p_ptt);
+		if (rc)
+			qed_schedule_iov(p_hwfn, QED_IOV_WQ_FLR_FLAG);
+
+		ecore_ptt_release(p_hwfn, p_ptt);
+	}
 }
 
 int qed_schedule_iov(struct ecore_hwfn *p_hwfn, enum qed_iov_wq_flag flag)
-- 
2.17.1


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [dpdk-dev] [PATCH v2 1/7] lib/librte_pci: add rte_pci_regs.h
  2020-07-13 15:13 ` [dpdk-dev] [PATCH v2 1/7] lib/librte_pci: add rte_pci_regs.h Manish Chopra
@ 2020-07-16  9:43   ` Gaëtan Rivet
  2020-07-16 10:08   ` Gaëtan Rivet
  1 sibling, 0 replies; 26+ messages in thread
From: Gaëtan Rivet @ 2020-07-16  9:43 UTC (permalink / raw)
  To: Manish Chopra, ferruh.yigit, Thomas Monjalon
  Cc: jerinjacobk, irusskikh, rmody, GR-Everest-DPDK-Dev,
	anatoly.burakov, xavier.huwei, humin29, yisen.zhuang,
	xiao.w.wang, qiming.yang, qi.z.zhang, heinrich.kuhn

On 13/07/20 08:13 -0700, Manish Chopra wrote:
> This is merely copy of latest linux/pci_regs.h in
> order to avoid dependency of dpdk on user headers.
> 

I guess this dependency is an issue on non-linux systems, when you must
use those defines in a generic implementation. Can you confirm this is
the motivation here?

If so, I think it would be clearer to state "in order to avoid
dependency of DPDK on linux headers".

> Signed-off-by: Manish Chopra <manishc@marvell.com>
> Signed-off-by: Igor Russkikh <irusskikh@marvell.com>
> ---
>  drivers/bus/pci/linux/pci_uio.c     |    2 +-
>  drivers/bus/pci/linux/pci_vfio.c    |    2 +-
>  drivers/net/bnx2x/bnx2x.h           |    2 +-
>  drivers/net/hns3/hns3_ethdev_vf.c   |    2 +-
>  drivers/vdpa/ifc/base/ifcvf_osdep.h |    2 +-
>  lib/librte_pci/Makefile             |    1 +
>  lib/librte_pci/meson.build          |    2 +-
>  lib/librte_pci/rte_pci_regs.h       | 1075 +++++++++++++++++++++++++++
>  8 files changed, 1082 insertions(+), 6 deletions(-)
>  create mode 100644 lib/librte_pci/rte_pci_regs.h
> 

[...]

> diff --git a/lib/librte_pci/rte_pci_regs.h b/lib/librte_pci/rte_pci_regs.h
> new file mode 100644
> index 000000000..1d11f4de5
> --- /dev/null
> +++ b/lib/librte_pci/rte_pci_regs.h
> @@ -0,0 +1,1075 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*

This file is delivered alongside the PCI lib, targeting userspace.
This seems to be an exception to the license policy described in
license/README. Code shared between kernel and userspace is expected
to be dual-licensed BSD-3 and GPL-2.0.

As it is a copy of Linux user includes, re-licensing it as BSD-3 as well
is not possible.

So I think it might require a techboard + governing board exception
approval. Ferruh or Thomas, what do you think?

-- 
Gaëtan

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [dpdk-dev] [PATCH v2 1/7] lib/librte_pci: add rte_pci_regs.h
  2020-07-13 15:13 ` [dpdk-dev] [PATCH v2 1/7] lib/librte_pci: add rte_pci_regs.h Manish Chopra
  2020-07-16  9:43   ` Gaëtan Rivet
@ 2020-07-16 10:08   ` Gaëtan Rivet
  2020-07-16 10:17     ` Gaëtan Rivet
  1 sibling, 1 reply; 26+ messages in thread
From: Gaëtan Rivet @ 2020-07-16 10:08 UTC (permalink / raw)
  To: Manish Chopra, ferruh.yigit, Thomas Monjalon; +Cc: jerinjacobk, irusskikh, dev

Re-CCing dev@dpdk.org as it was removed from the reply.

On 13/07/20 08:13 -0700, Manish Chopra wrote:
> This is merely copy of latest linux/pci_regs.h in
> order to avoid dependency of dpdk on user headers.
> 

I guess this dependency is an issue on non-linux systems, when you must
use those defines in a generic implementation. Can you confirm this is
the motivation here?

If so, I think it would be clearer to state "in order to avoid
dependency of DPDK on linux headers".

> Signed-off-by: Manish Chopra <manishc@marvell.com>
> Signed-off-by: Igor Russkikh <irusskikh@marvell.com>
> ---
>  drivers/bus/pci/linux/pci_uio.c     |    2 +-
>  drivers/bus/pci/linux/pci_vfio.c    |    2 +-
>  drivers/net/bnx2x/bnx2x.h           |    2 +-
>  drivers/net/hns3/hns3_ethdev_vf.c   |    2 +-
>  drivers/vdpa/ifc/base/ifcvf_osdep.h |    2 +-
>  lib/librte_pci/Makefile             |    1 +
>  lib/librte_pci/meson.build          |    2 +-
>  lib/librte_pci/rte_pci_regs.h       | 1075 +++++++++++++++++++++++++++
>  8 files changed, 1082 insertions(+), 6 deletions(-)
>  create mode 100644 lib/librte_pci/rte_pci_regs.h
> 

[...]

> diff --git a/lib/librte_pci/rte_pci_regs.h b/lib/librte_pci/rte_pci_regs.h
> new file mode 100644
> index 000000000..1d11f4de5
> --- /dev/null
> +++ b/lib/librte_pci/rte_pci_regs.h
> @@ -0,0 +1,1075 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*

This file is delivered alongside the PCI lib, targeting userspace.
This seems to be an exception to the license policy described in
license/README. Code shared between kernel and userspace is expected
to be dual-licensed BSD-3 and GPL-2.0.

As it is a copy of Linux user includes, re-licensing it as BSD-3 as well
is not possible.

So I think it might require a techboard + governing board exception
approval. Ferruh or Thomas, what do you think?

-- 
Gaëtan

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [dpdk-dev] [PATCH v2 1/7] lib/librte_pci: add rte_pci_regs.h
  2020-07-16 10:08   ` Gaëtan Rivet
@ 2020-07-16 10:17     ` Gaëtan Rivet
  2020-07-16 10:27       ` Jerin Jacob
  0 siblings, 1 reply; 26+ messages in thread
From: Gaëtan Rivet @ 2020-07-16 10:17 UTC (permalink / raw)
  To: Manish Chopra, ferruh.yigit, Thomas Monjalon; +Cc: jerinjacobk, irusskikh, dev

On 16/07/20 12:08 +0200, Gaëtan Rivet wrote:
> Re-CCing dev@dpdk.org as it was removed from the reply.
> 
> On 13/07/20 08:13 -0700, Manish Chopra wrote:
> > This is merely copy of latest linux/pci_regs.h in
> > order to avoid dependency of dpdk on user headers.
> > 
> 
> I guess this dependency is an issue on non-linux systems, when you must
> use those defines in a generic implementation. Can you confirm this is
> the motivation here?
> 
> If so, I think it would be clearer to state "in order to avoid
> dependency of DPDK on linux headers".
> 

To add to it, if this is actually the motivation to add this header, I
don't think it is sufficient.

You can restrict the function definition to the linux part of the
PCI bus driver instead, using stubs for other systems.

> > Signed-off-by: Manish Chopra <manishc@marvell.com>
> > Signed-off-by: Igor Russkikh <irusskikh@marvell.com>
> > ---
> >  drivers/bus/pci/linux/pci_uio.c     |    2 +-
> >  drivers/bus/pci/linux/pci_vfio.c    |    2 +-
> >  drivers/net/bnx2x/bnx2x.h           |    2 +-
> >  drivers/net/hns3/hns3_ethdev_vf.c   |    2 +-
> >  drivers/vdpa/ifc/base/ifcvf_osdep.h |    2 +-
> >  lib/librte_pci/Makefile             |    1 +
> >  lib/librte_pci/meson.build          |    2 +-
> >  lib/librte_pci/rte_pci_regs.h       | 1075 +++++++++++++++++++++++++++
> >  8 files changed, 1082 insertions(+), 6 deletions(-)
> >  create mode 100644 lib/librte_pci/rte_pci_regs.h
> > 
> 
> [...]
> 
> > diff --git a/lib/librte_pci/rte_pci_regs.h b/lib/librte_pci/rte_pci_regs.h
> > new file mode 100644
> > index 000000000..1d11f4de5
> > --- /dev/null
> > +++ b/lib/librte_pci/rte_pci_regs.h
> > @@ -0,0 +1,1075 @@
> > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > +/*
> 
> This file is delivered alongside the PCI lib, targeting userspace.
> This seems to be an exception to the license policy described in
> license/README. Code shared between kernel and userspace is expected
> to be dual-licensed BSD-3 and GPL-2.0.
> 
> As it is a copy of Linux user includes, re-licensing it as BSD-3 as well
> is not possible.
> 
> So I think it might require a techboard + governing board exception
> approval. Ferruh or Thomas, what do you think?
> 
> -- 
> Gaëtan

-- 
Gaëtan

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [dpdk-dev] [PATCH v2 1/7] lib/librte_pci: add rte_pci_regs.h
  2020-07-16 10:17     ` Gaëtan Rivet
@ 2020-07-16 10:27       ` Jerin Jacob
  2020-07-16 11:26         ` Thomas Monjalon
  0 siblings, 1 reply; 26+ messages in thread
From: Jerin Jacob @ 2020-07-16 10:27 UTC (permalink / raw)
  To: Gaëtan Rivet
  Cc: Manish Chopra, Ferruh Yigit, Thomas Monjalon, Igor Russkikh, dpdk-dev

On Thu, Jul 16, 2020 at 3:48 PM Gaëtan Rivet <grive@u256.net> wrote:
>
> On 16/07/20 12:08 +0200, Gaëtan Rivet wrote:
> > Re-CCing dev@dpdk.org as it was removed from the reply.
> >
> > On 13/07/20 08:13 -0700, Manish Chopra wrote:
> > > This is merely copy of latest linux/pci_regs.h in
> > > order to avoid dependency of dpdk on user headers.
> > >
> >
> > I guess this dependency is an issue on non-linux systems, when you must
> > use those defines in a generic implementation. Can you confirm this is
> > the motivation here?
> >
> > If so, I think it would be clearer to state "in order to avoid
> > dependency of DPDK on linux headers".
> >
>
> To add to it, if this is actually the motivation to add this header, I
> don't think it is sufficient.
>
> You can restrict the function definition to the linux part of the
> PCI bus driver instead, using stubs for other systems.
>
> > > Signed-off-by: Manish Chopra <manishc@marvell.com>
> > > Signed-off-by: Igor Russkikh <irusskikh@marvell.com>
> > > ---
> > >  drivers/bus/pci/linux/pci_uio.c     |    2 +-
> > >  drivers/bus/pci/linux/pci_vfio.c    |    2 +-
> > >  drivers/net/bnx2x/bnx2x.h           |    2 +-
> > >  drivers/net/hns3/hns3_ethdev_vf.c   |    2 +-
> > >  drivers/vdpa/ifc/base/ifcvf_osdep.h |    2 +-
> > >  lib/librte_pci/Makefile             |    1 +
> > >  lib/librte_pci/meson.build          |    2 +-
> > >  lib/librte_pci/rte_pci_regs.h       | 1075 +++++++++++++++++++++++++++
> > >  8 files changed, 1082 insertions(+), 6 deletions(-)
> > >  create mode 100644 lib/librte_pci/rte_pci_regs.h
> > >
> >
> > [...]
> >
> > > diff --git a/lib/librte_pci/rte_pci_regs.h b/lib/librte_pci/rte_pci_regs.h
> > > new file mode 100644
> > > index 000000000..1d11f4de5
> > > --- /dev/null
> > > +++ b/lib/librte_pci/rte_pci_regs.h
> > > @@ -0,0 +1,1075 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > > +/*
> >
> > This file is delivered alongside the PCI lib, targeting userspace.
> > This seems to be an exception to the license policy described in
> > license/README. Code shared between kernel and userspace is expected
> > to be dual-licensed BSD-3 and GPL-2.0.
> >
> > As it is a copy of Linux user includes, re-licensing it as BSD-3 as well
> > is not possible.
> >
> > So I think it might require a techboard + governing board exception
> > approval. Ferruh or Thomas, what do you think?

I think, instead of importing GPL-2.0 file, We can add the constants
as need by the DPDK
as symbols start from RTE_PCI_*(It will fix up the namespace as well).





> >
> > --
> > Gaëtan
>
> --
> Gaëtan

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [dpdk-dev] [PATCH v2 1/7] lib/librte_pci: add rte_pci_regs.h
  2020-07-16 10:27       ` Jerin Jacob
@ 2020-07-16 11:26         ` Thomas Monjalon
  2020-07-16 11:55           ` Jerin Jacob
  0 siblings, 1 reply; 26+ messages in thread
From: Thomas Monjalon @ 2020-07-16 11:26 UTC (permalink / raw)
  To: Gaëtan Rivet, Manish Chopra, Jerin Jacob
  Cc: dev, Ferruh Yigit, Igor Russkikh, dpdk-dev

16/07/2020 12:27, Jerin Jacob:
> On Thu, Jul 16, 2020 at 3:48 PM Gaëtan Rivet <grive@u256.net> wrote:
> >
> > On 16/07/20 12:08 +0200, Gaëtan Rivet wrote:
> > > Re-CCing dev@dpdk.org as it was removed from the reply.
> > >
> > > On 13/07/20 08:13 -0700, Manish Chopra wrote:
> > > > This is merely copy of latest linux/pci_regs.h in
> > > > order to avoid dependency of dpdk on user headers.
> > > >
> > >
> > > I guess this dependency is an issue on non-linux systems, when you must
> > > use those defines in a generic implementation. Can you confirm this is
> > > the motivation here?
> > >
> > > If so, I think it would be clearer to state "in order to avoid
> > > dependency of DPDK on linux headers".
> > >
> >
> > To add to it, if this is actually the motivation to add this header, I
> > don't think it is sufficient.
> >
> > You can restrict the function definition to the linux part of the
> > PCI bus driver instead, using stubs for other systems.
> >
> > > > Signed-off-by: Manish Chopra <manishc@marvell.com>
> > > > Signed-off-by: Igor Russkikh <irusskikh@marvell.com>
> > > > ---
> > > >  drivers/bus/pci/linux/pci_uio.c     |    2 +-
> > > >  drivers/bus/pci/linux/pci_vfio.c    |    2 +-
> > > >  drivers/net/bnx2x/bnx2x.h           |    2 +-
> > > >  drivers/net/hns3/hns3_ethdev_vf.c   |    2 +-
> > > >  drivers/vdpa/ifc/base/ifcvf_osdep.h |    2 +-
> > > >  lib/librte_pci/Makefile             |    1 +
> > > >  lib/librte_pci/meson.build          |    2 +-
> > > >  lib/librte_pci/rte_pci_regs.h       | 1075 +++++++++++++++++++++++++++
> > > >  8 files changed, 1082 insertions(+), 6 deletions(-)
> > > >  create mode 100644 lib/librte_pci/rte_pci_regs.h
> > > >
> > >
> > > [...]
> > >
> > > > diff --git a/lib/librte_pci/rte_pci_regs.h b/lib/librte_pci/rte_pci_regs.h
> > > > new file mode 100644
> > > > index 000000000..1d11f4de5
> > > > --- /dev/null
> > > > +++ b/lib/librte_pci/rte_pci_regs.h
> > > > @@ -0,0 +1,1075 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > > > +/*
> > >
> > > This file is delivered alongside the PCI lib, targeting userspace.
> > > This seems to be an exception to the license policy described in
> > > license/README. Code shared between kernel and userspace is expected
> > > to be dual-licensed BSD-3 and GPL-2.0.
> > >
> > > As it is a copy of Linux user includes, re-licensing it as BSD-3 as well
> > > is not possible.
> > >
> > > So I think it might require a techboard + governing board exception
> > > approval. Ferruh or Thomas, what do you think?
> 
> I think, instead of importing GPL-2.0 file, We can add the constants
> as need by the DPDK
> as symbols start from RTE_PCI_*(It will fix up the namespace as well).

If symbols can be found in /usr/include/, don't add anything.



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [dpdk-dev] [PATCH v2 1/7] lib/librte_pci: add rte_pci_regs.h
  2020-07-16 11:26         ` Thomas Monjalon
@ 2020-07-16 11:55           ` Jerin Jacob
  2020-07-16 12:49             ` Thomas Monjalon
  0 siblings, 1 reply; 26+ messages in thread
From: Jerin Jacob @ 2020-07-16 11:55 UTC (permalink / raw)
  To: Thomas Monjalon
  Cc: Gaëtan Rivet, Manish Chopra, Ferruh Yigit, Igor Russkikh, dpdk-dev

On Thu, Jul 16, 2020 at 4:57 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 16/07/2020 12:27, Jerin Jacob:
> > On Thu, Jul 16, 2020 at 3:48 PM Gaëtan Rivet <grive@u256.net> wrote:
> > >
> > > On 16/07/20 12:08 +0200, Gaëtan Rivet wrote:
> > > > Re-CCing dev@dpdk.org as it was removed from the reply.
> > > >
> > > > On 13/07/20 08:13 -0700, Manish Chopra wrote:
> > > > > This is merely copy of latest linux/pci_regs.h in
> > > > > order to avoid dependency of dpdk on user headers.
> > > > >
> > > >
> > > > I guess this dependency is an issue on non-linux systems, when you must
> > > > use those defines in a generic implementation. Can you confirm this is
> > > > the motivation here?
> > > >
> > > > If so, I think it would be clearer to state "in order to avoid
> > > > dependency of DPDK on linux headers".
> > > >
> > >
> > > To add to it, if this is actually the motivation to add this header, I
> > > don't think it is sufficient.
> > >
> > > You can restrict the function definition to the linux part of the
> > > PCI bus driver instead, using stubs for other systems.
> > >
> > > > > Signed-off-by: Manish Chopra <manishc@marvell.com>
> > > > > Signed-off-by: Igor Russkikh <irusskikh@marvell.com>
> > > > > ---
> > > > >  drivers/bus/pci/linux/pci_uio.c     |    2 +-
> > > > >  drivers/bus/pci/linux/pci_vfio.c    |    2 +-
> > > > >  drivers/net/bnx2x/bnx2x.h           |    2 +-
> > > > >  drivers/net/hns3/hns3_ethdev_vf.c   |    2 +-
> > > > >  drivers/vdpa/ifc/base/ifcvf_osdep.h |    2 +-
> > > > >  lib/librte_pci/Makefile             |    1 +
> > > > >  lib/librte_pci/meson.build          |    2 +-
> > > > >  lib/librte_pci/rte_pci_regs.h       | 1075 +++++++++++++++++++++++++++
> > > > >  8 files changed, 1082 insertions(+), 6 deletions(-)
> > > > >  create mode 100644 lib/librte_pci/rte_pci_regs.h
> > > > >
> > > >
> > > > [...]
> > > >
> > > > > diff --git a/lib/librte_pci/rte_pci_regs.h b/lib/librte_pci/rte_pci_regs.h
> > > > > new file mode 100644
> > > > > index 000000000..1d11f4de5
> > > > > --- /dev/null
> > > > > +++ b/lib/librte_pci/rte_pci_regs.h
> > > > > @@ -0,0 +1,1075 @@
> > > > > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > > > > +/*
> > > >
> > > > This file is delivered alongside the PCI lib, targeting userspace.
> > > > This seems to be an exception to the license policy described in
> > > > license/README. Code shared between kernel and userspace is expected
> > > > to be dual-licensed BSD-3 and GPL-2.0.
> > > >
> > > > As it is a copy of Linux user includes, re-licensing it as BSD-3 as well
> > > > is not possible.
> > > >
> > > > So I think it might require a techboard + governing board exception
> > > > approval. Ferruh or Thomas, what do you think?
> >
> > I think, instead of importing GPL-2.0 file, We can add the constants
> > as need by the DPDK
> > as symbols start from RTE_PCI_*(It will fix up the namespace as well).
>
> If symbols can be found in /usr/include/, don't add anything.

Not by default on all the distros. It is part of pciutils library.
Moreover, we need these symbols for Windows OS as well.
IMO, We should add absolute minimum constants that needed for DPDK as RTE_PCI_*


>
>

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [dpdk-dev] [PATCH v2 1/7] lib/librte_pci: add rte_pci_regs.h
  2020-07-16 11:55           ` Jerin Jacob
@ 2020-07-16 12:49             ` Thomas Monjalon
  2020-07-16 13:02               ` Jerin Jacob
  0 siblings, 1 reply; 26+ messages in thread
From: Thomas Monjalon @ 2020-07-16 12:49 UTC (permalink / raw)
  To: Jerin Jacob
  Cc: Gaëtan Rivet, Manish Chopra, Ferruh Yigit, Igor Russkikh, dpdk-dev

16/07/2020 13:55, Jerin Jacob:
> On Thu, Jul 16, 2020 at 4:57 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> >
> > 16/07/2020 12:27, Jerin Jacob:
> > > On Thu, Jul 16, 2020 at 3:48 PM Gaëtan Rivet <grive@u256.net> wrote:
> > > >
> > > > On 16/07/20 12:08 +0200, Gaëtan Rivet wrote:
> > > > > Re-CCing dev@dpdk.org as it was removed from the reply.
> > > > >
> > > > > On 13/07/20 08:13 -0700, Manish Chopra wrote:
> > > > > > This is merely copy of latest linux/pci_regs.h in
> > > > > > order to avoid dependency of dpdk on user headers.
> > > > > >
> > > > >
> > > > > I guess this dependency is an issue on non-linux systems, when you must
> > > > > use those defines in a generic implementation. Can you confirm this is
> > > > > the motivation here?
> > > > >
> > > > > If so, I think it would be clearer to state "in order to avoid
> > > > > dependency of DPDK on linux headers".
> > > > >
> > > >
> > > > To add to it, if this is actually the motivation to add this header, I
> > > > don't think it is sufficient.
> > > >
> > > > You can restrict the function definition to the linux part of the
> > > > PCI bus driver instead, using stubs for other systems.
> > > >
> > > > > > Signed-off-by: Manish Chopra <manishc@marvell.com>
> > > > > > Signed-off-by: Igor Russkikh <irusskikh@marvell.com>
> > > > > > ---
> > > > > >  drivers/bus/pci/linux/pci_uio.c     |    2 +-
> > > > > >  drivers/bus/pci/linux/pci_vfio.c    |    2 +-
> > > > > >  drivers/net/bnx2x/bnx2x.h           |    2 +-
> > > > > >  drivers/net/hns3/hns3_ethdev_vf.c   |    2 +-
> > > > > >  drivers/vdpa/ifc/base/ifcvf_osdep.h |    2 +-
> > > > > >  lib/librte_pci/Makefile             |    1 +
> > > > > >  lib/librte_pci/meson.build          |    2 +-
> > > > > >  lib/librte_pci/rte_pci_regs.h       | 1075 +++++++++++++++++++++++++++
> > > > > >  8 files changed, 1082 insertions(+), 6 deletions(-)
> > > > > >  create mode 100644 lib/librte_pci/rte_pci_regs.h
> > > > > >
> > > > >
> > > > > [...]
> > > > >
> > > > > > diff --git a/lib/librte_pci/rte_pci_regs.h b/lib/librte_pci/rte_pci_regs.h
> > > > > > new file mode 100644
> > > > > > index 000000000..1d11f4de5
> > > > > > --- /dev/null
> > > > > > +++ b/lib/librte_pci/rte_pci_regs.h
> > > > > > @@ -0,0 +1,1075 @@
> > > > > > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > > > > > +/*
> > > > >
> > > > > This file is delivered alongside the PCI lib, targeting userspace.
> > > > > This seems to be an exception to the license policy described in
> > > > > license/README. Code shared between kernel and userspace is expected
> > > > > to be dual-licensed BSD-3 and GPL-2.0.
> > > > >
> > > > > As it is a copy of Linux user includes, re-licensing it as BSD-3 as well
> > > > > is not possible.
> > > > >
> > > > > So I think it might require a techboard + governing board exception
> > > > > approval. Ferruh or Thomas, what do you think?
> > >
> > > I think, instead of importing GPL-2.0 file, We can add the constants
> > > as need by the DPDK
> > > as symbols start from RTE_PCI_*(It will fix up the namespace as well).
> >
> > If symbols can be found in /usr/include/, don't add anything.
> 
> Not by default on all the distros. It is part of pciutils library.
> Moreover, we need these symbols for Windows OS as well.
> IMO, We should add absolute minimum constants that needed for DPDK as RTE_PCI_*

I am for mandating the dependency instead of copying it.

pciutils cannot be installed on Windows?
Why do you care about Windows?
I don't see any contribution for qede on Windows.



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [dpdk-dev] [PATCH v2 1/7] lib/librte_pci: add rte_pci_regs.h
  2020-07-16 12:49             ` Thomas Monjalon
@ 2020-07-16 13:02               ` Jerin Jacob
  2020-07-16 15:55                 ` Thomas Monjalon
  0 siblings, 1 reply; 26+ messages in thread
From: Jerin Jacob @ 2020-07-16 13:02 UTC (permalink / raw)
  To: Thomas Monjalon
  Cc: Gaëtan Rivet, Manish Chopra, Ferruh Yigit, Igor Russkikh, dpdk-dev

On Thu, Jul 16, 2020 at 6:20 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 16/07/2020 13:55, Jerin Jacob:
> > On Thu, Jul 16, 2020 at 4:57 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > >
> > > 16/07/2020 12:27, Jerin Jacob:
> > > > On Thu, Jul 16, 2020 at 3:48 PM Gaëtan Rivet <grive@u256.net> wrote:
> > > > >
> > > > > On 16/07/20 12:08 +0200, Gaëtan Rivet wrote:
> > > > > > Re-CCing dev@dpdk.org as it was removed from the reply.
> > > > > >
> > > > > > On 13/07/20 08:13 -0700, Manish Chopra wrote:
> > > > > > > This is merely copy of latest linux/pci_regs.h in
> > > > > > > order to avoid dependency of dpdk on user headers.
> > > > > > >
> > > > > >
> > > > > > I guess this dependency is an issue on non-linux systems, when you must
> > > > > > use those defines in a generic implementation. Can you confirm this is
> > > > > > the motivation here?
> > > > > >
> > > > > > If so, I think it would be clearer to state "in order to avoid
> > > > > > dependency of DPDK on linux headers".
> > > > > >
> > > > >
> > > > > To add to it, if this is actually the motivation to add this header, I
> > > > > don't think it is sufficient.
> > > > >
> > > > > You can restrict the function definition to the linux part of the
> > > > > PCI bus driver instead, using stubs for other systems.
> > > > >
> > > > > > > Signed-off-by: Manish Chopra <manishc@marvell.com>
> > > > > > > Signed-off-by: Igor Russkikh <irusskikh@marvell.com>
> > > > > > > ---
> > > > > > >  drivers/bus/pci/linux/pci_uio.c     |    2 +-
> > > > > > >  drivers/bus/pci/linux/pci_vfio.c    |    2 +-
> > > > > > >  drivers/net/bnx2x/bnx2x.h           |    2 +-
> > > > > > >  drivers/net/hns3/hns3_ethdev_vf.c   |    2 +-
> > > > > > >  drivers/vdpa/ifc/base/ifcvf_osdep.h |    2 +-
> > > > > > >  lib/librte_pci/Makefile             |    1 +
> > > > > > >  lib/librte_pci/meson.build          |    2 +-
> > > > > > >  lib/librte_pci/rte_pci_regs.h       | 1075 +++++++++++++++++++++++++++
> > > > > > >  8 files changed, 1082 insertions(+), 6 deletions(-)
> > > > > > >  create mode 100644 lib/librte_pci/rte_pci_regs.h
> > > > > > >
> > > > > >
> > > > > > [...]
> > > > > >
> > > > > > > diff --git a/lib/librte_pci/rte_pci_regs.h b/lib/librte_pci/rte_pci_regs.h
> > > > > > > new file mode 100644
> > > > > > > index 000000000..1d11f4de5
> > > > > > > --- /dev/null
> > > > > > > +++ b/lib/librte_pci/rte_pci_regs.h
> > > > > > > @@ -0,0 +1,1075 @@
> > > > > > > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > > > > > > +/*
> > > > > >
> > > > > > This file is delivered alongside the PCI lib, targeting userspace.
> > > > > > This seems to be an exception to the license policy described in
> > > > > > license/README. Code shared between kernel and userspace is expected
> > > > > > to be dual-licensed BSD-3 and GPL-2.0.
> > > > > >
> > > > > > As it is a copy of Linux user includes, re-licensing it as BSD-3 as well
> > > > > > is not possible.
> > > > > >
> > > > > > So I think it might require a techboard + governing board exception
> > > > > > approval. Ferruh or Thomas, what do you think?
> > > >
> > > > I think, instead of importing GPL-2.0 file, We can add the constants
> > > > as need by the DPDK
> > > > as symbols start from RTE_PCI_*(It will fix up the namespace as well).
> > >
> > > If symbols can be found in /usr/include/, don't add anything.
> >
> > Not by default on all the distros. It is part of pciutils library.
> > Moreover, we need these symbols for Windows OS as well.
> > IMO, We should add absolute minimum constants that needed for DPDK as RTE_PCI_*
>
> I am for mandating the dependency instead of copying it.

You mean _pciutils_ package as a mandatory dependency to  DPDK.

>
> pciutils cannot be installed on Windows?
> Why do you care about Windows?
> I don't see any contribution for qede on Windows.

You closely review the patch, it not about qede. The proposed file
comes at lib/librte_pci/rte_pci_regs.h which is common to Windows.



>
>

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [dpdk-dev] [PATCH v2 1/7] lib/librte_pci: add rte_pci_regs.h
  2020-07-16 13:02               ` Jerin Jacob
@ 2020-07-16 15:55                 ` Thomas Monjalon
  2020-07-16 16:43                   ` Jerin Jacob
  0 siblings, 1 reply; 26+ messages in thread
From: Thomas Monjalon @ 2020-07-16 15:55 UTC (permalink / raw)
  To: Jerin Jacob
  Cc: Gaëtan Rivet, Manish Chopra, Ferruh Yigit, Igor Russkikh, dpdk-dev

16/07/2020 15:02, Jerin Jacob:
> On Thu, Jul 16, 2020 at 6:20 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> >
> > 16/07/2020 13:55, Jerin Jacob:
> > > On Thu, Jul 16, 2020 at 4:57 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > > >
> > > > 16/07/2020 12:27, Jerin Jacob:
> > > > > On Thu, Jul 16, 2020 at 3:48 PM Gaëtan Rivet <grive@u256.net> wrote:
> > > > > >
> > > > > > On 16/07/20 12:08 +0200, Gaëtan Rivet wrote:
> > > > > > > Re-CCing dev@dpdk.org as it was removed from the reply.
> > > > > > >
> > > > > > > On 13/07/20 08:13 -0700, Manish Chopra wrote:
> > > > > > > > This is merely copy of latest linux/pci_regs.h in
> > > > > > > > order to avoid dependency of dpdk on user headers.
> > > > > > > >
> > > > > > >
> > > > > > > I guess this dependency is an issue on non-linux systems, when you must
> > > > > > > use those defines in a generic implementation. Can you confirm this is
> > > > > > > the motivation here?
> > > > > > >
> > > > > > > If so, I think it would be clearer to state "in order to avoid
> > > > > > > dependency of DPDK on linux headers".
> > > > > > >
> > > > > >
> > > > > > To add to it, if this is actually the motivation to add this header, I
> > > > > > don't think it is sufficient.
> > > > > >
> > > > > > You can restrict the function definition to the linux part of the
> > > > > > PCI bus driver instead, using stubs for other systems.
> > > > > >
> > > > > > > > Signed-off-by: Manish Chopra <manishc@marvell.com>
> > > > > > > > Signed-off-by: Igor Russkikh <irusskikh@marvell.com>
> > > > > > > > ---
> > > > > > > >  drivers/bus/pci/linux/pci_uio.c     |    2 +-
> > > > > > > >  drivers/bus/pci/linux/pci_vfio.c    |    2 +-
> > > > > > > >  drivers/net/bnx2x/bnx2x.h           |    2 +-
> > > > > > > >  drivers/net/hns3/hns3_ethdev_vf.c   |    2 +-
> > > > > > > >  drivers/vdpa/ifc/base/ifcvf_osdep.h |    2 +-
> > > > > > > >  lib/librte_pci/Makefile             |    1 +
> > > > > > > >  lib/librte_pci/meson.build          |    2 +-
> > > > > > > >  lib/librte_pci/rte_pci_regs.h       | 1075 +++++++++++++++++++++++++++
> > > > > > > >  8 files changed, 1082 insertions(+), 6 deletions(-)
> > > > > > > >  create mode 100644 lib/librte_pci/rte_pci_regs.h
> > > > > > > >
> > > > > > >
> > > > > > > [...]
> > > > > > >
> > > > > > > > diff --git a/lib/librte_pci/rte_pci_regs.h b/lib/librte_pci/rte_pci_regs.h
> > > > > > > > new file mode 100644
> > > > > > > > index 000000000..1d11f4de5
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/lib/librte_pci/rte_pci_regs.h
> > > > > > > > @@ -0,0 +1,1075 @@
> > > > > > > > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > > > > > > > +/*
> > > > > > >
> > > > > > > This file is delivered alongside the PCI lib, targeting userspace.
> > > > > > > This seems to be an exception to the license policy described in
> > > > > > > license/README. Code shared between kernel and userspace is expected
> > > > > > > to be dual-licensed BSD-3 and GPL-2.0.
> > > > > > >
> > > > > > > As it is a copy of Linux user includes, re-licensing it as BSD-3 as well
> > > > > > > is not possible.
> > > > > > >
> > > > > > > So I think it might require a techboard + governing board exception
> > > > > > > approval. Ferruh or Thomas, what do you think?
> > > > >
> > > > > I think, instead of importing GPL-2.0 file, We can add the constants
> > > > > as need by the DPDK
> > > > > as symbols start from RTE_PCI_*(It will fix up the namespace as well).
> > > >
> > > > If symbols can be found in /usr/include/, don't add anything.
> > >
> > > Not by default on all the distros. It is part of pciutils library.
> > > Moreover, we need these symbols for Windows OS as well.
> > > IMO, We should add absolute minimum constants that needed for DPDK as RTE_PCI_*
> >
> > I am for mandating the dependency instead of copying it.
> 
> You mean _pciutils_ package as a mandatory dependency to  DPDK.

There is already this dependency:
	#include <linux/pci_regs.h>
I'm missing the real justification for this patch.
Is there some missing definitions?
Is there some environments where this file is missing?

> > pciutils cannot be installed on Windows?
> > Why do you care about Windows?
> > I don't see any contribution for qede on Windows.
> 
> You closely review the patch, it not about qede. The proposed file
> comes at lib/librte_pci/rte_pci_regs.h which is common to Windows.

The series is for qede. I'm trying to understand the motivation.



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [dpdk-dev] [PATCH v2 1/7] lib/librte_pci: add rte_pci_regs.h
  2020-07-16 15:55                 ` Thomas Monjalon
@ 2020-07-16 16:43                   ` Jerin Jacob
  2020-07-16 16:57                     ` Thomas Monjalon
  0 siblings, 1 reply; 26+ messages in thread
From: Jerin Jacob @ 2020-07-16 16:43 UTC (permalink / raw)
  To: Thomas Monjalon
  Cc: Gaëtan Rivet, Manish Chopra, Ferruh Yigit, Igor Russkikh, dpdk-dev

On Thu, Jul 16, 2020 at 9:25 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 16/07/2020 15:02, Jerin Jacob:
> > On Thu, Jul 16, 2020 at 6:20 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > >
> > > 16/07/2020 13:55, Jerin Jacob:
> > > > On Thu, Jul 16, 2020 at 4:57 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > > > >
> > > > > 16/07/2020 12:27, Jerin Jacob:
> > > > > > On Thu, Jul 16, 2020 at 3:48 PM Gaëtan Rivet <grive@u256.net> wrote:
> > > > > > >
> > > > > > > On 16/07/20 12:08 +0200, Gaëtan Rivet wrote:
> > > > > > > > Re-CCing dev@dpdk.org as it was removed from the reply.
> > > > > > > >
> > > > > > > > On 13/07/20 08:13 -0700, Manish Chopra wrote:
> > > > > > > > > This is merely copy of latest linux/pci_regs.h in
> > > > > > > > > order to avoid dependency of dpdk on user headers.
> > > > > > > > >
> > > > > > > >
> > > > > > > > I guess this dependency is an issue on non-linux systems, when you must
> > > > > > > > use those defines in a generic implementation. Can you confirm this is
> > > > > > > > the motivation here?
> > > > > > > >
> > > > > > > > If so, I think it would be clearer to state "in order to avoid
> > > > > > > > dependency of DPDK on linux headers".
> > > > > > > >
> > > > > > >
> > > > > > > To add to it, if this is actually the motivation to add this header, I
> > > > > > > don't think it is sufficient.
> > > > > > >
> > > > > > > You can restrict the function definition to the linux part of the
> > > > > > > PCI bus driver instead, using stubs for other systems.
> > > > > > >
> > > > > > > > > Signed-off-by: Manish Chopra <manishc@marvell.com>
> > > > > > > > > Signed-off-by: Igor Russkikh <irusskikh@marvell.com>
> > > > > > > > > ---
> > > > > > > > >  drivers/bus/pci/linux/pci_uio.c     |    2 +-
> > > > > > > > >  drivers/bus/pci/linux/pci_vfio.c    |    2 +-
> > > > > > > > >  drivers/net/bnx2x/bnx2x.h           |    2 +-
> > > > > > > > >  drivers/net/hns3/hns3_ethdev_vf.c   |    2 +-
> > > > > > > > >  drivers/vdpa/ifc/base/ifcvf_osdep.h |    2 +-
> > > > > > > > >  lib/librte_pci/Makefile             |    1 +
> > > > > > > > >  lib/librte_pci/meson.build          |    2 +-
> > > > > > > > >  lib/librte_pci/rte_pci_regs.h       | 1075 +++++++++++++++++++++++++++
> > > > > > > > >  8 files changed, 1082 insertions(+), 6 deletions(-)
> > > > > > > > >  create mode 100644 lib/librte_pci/rte_pci_regs.h
> > > > > > > > >
> > > > > > > >
> > > > > > > > [...]
> > > > > > > >
> > > > > > > > > diff --git a/lib/librte_pci/rte_pci_regs.h b/lib/librte_pci/rte_pci_regs.h
> > > > > > > > > new file mode 100644
> > > > > > > > > index 000000000..1d11f4de5
> > > > > > > > > --- /dev/null
> > > > > > > > > +++ b/lib/librte_pci/rte_pci_regs.h
> > > > > > > > > @@ -0,0 +1,1075 @@
> > > > > > > > > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > > > > > > > > +/*
> > > > > > > >
> > > > > > > > This file is delivered alongside the PCI lib, targeting userspace.
> > > > > > > > This seems to be an exception to the license policy described in
> > > > > > > > license/README. Code shared between kernel and userspace is expected
> > > > > > > > to be dual-licensed BSD-3 and GPL-2.0.
> > > > > > > >
> > > > > > > > As it is a copy of Linux user includes, re-licensing it as BSD-3 as well
> > > > > > > > is not possible.
> > > > > > > >
> > > > > > > > So I think it might require a techboard + governing board exception
> > > > > > > > approval. Ferruh or Thomas, what do you think?
> > > > > >
> > > > > > I think, instead of importing GPL-2.0 file, We can add the constants
> > > > > > as need by the DPDK
> > > > > > as symbols start from RTE_PCI_*(It will fix up the namespace as well).
> > > > >
> > > > > If symbols can be found in /usr/include/, don't add anything.
> > > >
> > > > Not by default on all the distros. It is part of pciutils library.
> > > > Moreover, we need these symbols for Windows OS as well.
> > > > IMO, We should add absolute minimum constants that needed for DPDK as RTE_PCI_*
> > >
> > > I am for mandating the dependency instead of copying it.
> >
> > You mean _pciutils_ package as a mandatory dependency to  DPDK.
>
> There is already this dependency:
>         #include <linux/pci_regs.h>

I just checked in archlinux, PCI headers can be provided by

# pacman -F /usr/include/pci/header.h
usr/include/pci/header.h is owned by core/pciutils 3.7.0-

# pacman -F /usr/include/linux/pci.h
usr/include/linux/pci.h is owned by core/linux-api-headers 5.4.17-1


> I'm missing the real justification for this patch.

See below.

> Is there some missing definitions?
> Is there some environments where this file is missing?
>
> > > pciutils cannot be installed on Windows?
> > > Why do you care about Windows?
> > > I don't see any contribution for qede on Windows.
> >
> > You closely review the patch, it not about qede. The proposed file
> > comes at lib/librte_pci/rte_pci_regs.h which is common to Windows.
>
> The series is for qede. I'm trying to understand the motivation.

First version of qede driver sent with defined generic PCI symbols and
generic PCI function like pci_find_next_ext_capability() in qede driver.

In the review, I suggested using generic rte_ function as
a) It is not specific to qede.
b) Other drivers also doing the same thing in their own driver space
as there is no dpdk API for the same.
This patches create generic API for pci_find_next_ext_capability() and
remove duplicate implementation
from the drivers.
http://patches.dpdk.org/patch/73959/




>
>

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [dpdk-dev] [PATCH v2 1/7] lib/librte_pci: add rte_pci_regs.h
  2020-07-16 16:43                   ` Jerin Jacob
@ 2020-07-16 16:57                     ` Thomas Monjalon
  2020-07-16 17:33                       ` Jerin Jacob
  2020-07-16 17:56                       ` Gaëtan Rivet
  0 siblings, 2 replies; 26+ messages in thread
From: Thomas Monjalon @ 2020-07-16 16:57 UTC (permalink / raw)
  To: Jerin Jacob
  Cc: Gaëtan Rivet, Manish Chopra, Ferruh Yigit, Igor Russkikh, dpdk-dev

16/07/2020 18:43, Jerin Jacob:
> On Thu, Jul 16, 2020 at 9:25 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> >
> > 16/07/2020 15:02, Jerin Jacob:
> > > On Thu, Jul 16, 2020 at 6:20 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > > >
> > > > 16/07/2020 13:55, Jerin Jacob:
> > > > > On Thu, Jul 16, 2020 at 4:57 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > > > > >
> > > > > > 16/07/2020 12:27, Jerin Jacob:
> > > > > > > On Thu, Jul 16, 2020 at 3:48 PM Gaëtan Rivet <grive@u256.net> wrote:
> > > > > > > >
> > > > > > > > On 16/07/20 12:08 +0200, Gaëtan Rivet wrote:
> > > > > > > > > Re-CCing dev@dpdk.org as it was removed from the reply.
> > > > > > > > >
> > > > > > > > > On 13/07/20 08:13 -0700, Manish Chopra wrote:
> > > > > > > > > > This is merely copy of latest linux/pci_regs.h in
> > > > > > > > > > order to avoid dependency of dpdk on user headers.
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > I guess this dependency is an issue on non-linux systems, when you must
> > > > > > > > > use those defines in a generic implementation. Can you confirm this is
> > > > > > > > > the motivation here?
> > > > > > > > >
> > > > > > > > > If so, I think it would be clearer to state "in order to avoid
> > > > > > > > > dependency of DPDK on linux headers".
> > > > > > > > >
> > > > > > > >
> > > > > > > > To add to it, if this is actually the motivation to add this header, I
> > > > > > > > don't think it is sufficient.
> > > > > > > >
> > > > > > > > You can restrict the function definition to the linux part of the
> > > > > > > > PCI bus driver instead, using stubs for other systems.
> > > > > > > >
> > > > > > > > > > Signed-off-by: Manish Chopra <manishc@marvell.com>
> > > > > > > > > > Signed-off-by: Igor Russkikh <irusskikh@marvell.com>
> > > > > > > > > > ---
> > > > > > > > > >  drivers/bus/pci/linux/pci_uio.c     |    2 +-
> > > > > > > > > >  drivers/bus/pci/linux/pci_vfio.c    |    2 +-
> > > > > > > > > >  drivers/net/bnx2x/bnx2x.h           |    2 +-
> > > > > > > > > >  drivers/net/hns3/hns3_ethdev_vf.c   |    2 +-
> > > > > > > > > >  drivers/vdpa/ifc/base/ifcvf_osdep.h |    2 +-
> > > > > > > > > >  lib/librte_pci/Makefile             |    1 +
> > > > > > > > > >  lib/librte_pci/meson.build          |    2 +-
> > > > > > > > > >  lib/librte_pci/rte_pci_regs.h       | 1075 +++++++++++++++++++++++++++
> > > > > > > > > >  8 files changed, 1082 insertions(+), 6 deletions(-)
> > > > > > > > > >  create mode 100644 lib/librte_pci/rte_pci_regs.h
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > [...]
> > > > > > > > >
> > > > > > > > > > diff --git a/lib/librte_pci/rte_pci_regs.h b/lib/librte_pci/rte_pci_regs.h
> > > > > > > > > > new file mode 100644
> > > > > > > > > > index 000000000..1d11f4de5
> > > > > > > > > > --- /dev/null
> > > > > > > > > > +++ b/lib/librte_pci/rte_pci_regs.h
> > > > > > > > > > @@ -0,0 +1,1075 @@
> > > > > > > > > > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > > > > > > > > > +/*
> > > > > > > > >
> > > > > > > > > This file is delivered alongside the PCI lib, targeting userspace.
> > > > > > > > > This seems to be an exception to the license policy described in
> > > > > > > > > license/README. Code shared between kernel and userspace is expected
> > > > > > > > > to be dual-licensed BSD-3 and GPL-2.0.
> > > > > > > > >
> > > > > > > > > As it is a copy of Linux user includes, re-licensing it as BSD-3 as well
> > > > > > > > > is not possible.
> > > > > > > > >
> > > > > > > > > So I think it might require a techboard + governing board exception
> > > > > > > > > approval. Ferruh or Thomas, what do you think?
> > > > > > >
> > > > > > > I think, instead of importing GPL-2.0 file, We can add the constants
> > > > > > > as need by the DPDK
> > > > > > > as symbols start from RTE_PCI_*(It will fix up the namespace as well).
> > > > > >
> > > > > > If symbols can be found in /usr/include/, don't add anything.
> > > > >
> > > > > Not by default on all the distros. It is part of pciutils library.
> > > > > Moreover, we need these symbols for Windows OS as well.
> > > > > IMO, We should add absolute minimum constants that needed for DPDK as RTE_PCI_*
> > > >
> > > > I am for mandating the dependency instead of copying it.
> > >
> > > You mean _pciutils_ package as a mandatory dependency to  DPDK.
> >
> > There is already this dependency:
> >         #include <linux/pci_regs.h>
> 
> I just checked in archlinux, PCI headers can be provided by
> 
> # pacman -F /usr/include/pci/header.h
> usr/include/pci/header.h is owned by core/pciutils 3.7.0-
> 
> # pacman -F /usr/include/linux/pci.h
> usr/include/linux/pci.h is owned by core/linux-api-headers 5.4.17-1
> 
> 
> > I'm missing the real justification for this patch.
> 
> See below.
> 
> > Is there some missing definitions?
> > Is there some environments where this file is missing?
> >
> > > > pciutils cannot be installed on Windows?
> > > > Why do you care about Windows?
> > > > I don't see any contribution for qede on Windows.
> > >
> > > You closely review the patch, it not about qede. The proposed file
> > > comes at lib/librte_pci/rte_pci_regs.h which is common to Windows.
> >
> > The series is for qede. I'm trying to understand the motivation.
> 
> First version of qede driver sent with defined generic PCI symbols and
> generic PCI function like pci_find_next_ext_capability() in qede driver.

That's a pity the v2 is not threaded with v1,
I would have found these explanations easily myself.

> In the review, I suggested using generic rte_ function as
> a) It is not specific to qede.
> b) Other drivers also doing the same thing in their own driver space
> as there is no dpdk API for the same.
> This patches create generic API for pci_find_next_ext_capability() and
> remove duplicate implementation
> from the drivers.
> http://patches.dpdk.org/patch/73959/

I agree it's good to have an API for such thing.

So far such feature is supported in drivers on Linux,
requiring only Linux headers to be installed.
Do we need more?



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [dpdk-dev] [PATCH v2 1/7] lib/librte_pci: add rte_pci_regs.h
  2020-07-16 16:57                     ` Thomas Monjalon
@ 2020-07-16 17:33                       ` Jerin Jacob
  2020-07-16 17:56                       ` Gaëtan Rivet
  1 sibling, 0 replies; 26+ messages in thread
From: Jerin Jacob @ 2020-07-16 17:33 UTC (permalink / raw)
  To: Thomas Monjalon
  Cc: Gaëtan Rivet, Manish Chopra, Ferruh Yigit, Igor Russkikh, dpdk-dev

On Thu, Jul 16, 2020 at 10:27 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 16/07/2020 18:43, Jerin Jacob:
> > On Thu, Jul 16, 2020 at 9:25 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > >
> > > 16/07/2020 15:02, Jerin Jacob:
> > > > On Thu, Jul 16, 2020 at 6:20 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > > > >
> > > > > 16/07/2020 13:55, Jerin Jacob:
> > > > > > On Thu, Jul 16, 2020 at 4:57 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > > > > > >
> > > > > > > 16/07/2020 12:27, Jerin Jacob:
> > > > > > > > On Thu, Jul 16, 2020 at 3:48 PM Gaëtan Rivet <grive@u256.net> wrote:
> > > > > > > > >
> > > > > > > > > On 16/07/20 12:08 +0200, Gaëtan Rivet wrote:
> > > > > > > > > > Re-CCing dev@dpdk.org as it was removed from the reply.
> > > > > > > > > >
> > > > > > > > > > On 13/07/20 08:13 -0700, Manish Chopra wrote:
> > > > > > > > > > > This is merely copy of latest linux/pci_regs.h in
> > > > > > > > > > > order to avoid dependency of dpdk on user headers.
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > I guess this dependency is an issue on non-linux systems, when you must
> > > > > > > > > > use those defines in a generic implementation. Can you confirm this is
> > > > > > > > > > the motivation here?
> > > > > > > > > >
> > > > > > > > > > If so, I think it would be clearer to state "in order to avoid
> > > > > > > > > > dependency of DPDK on linux headers".
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > To add to it, if this is actually the motivation to add this header, I
> > > > > > > > > don't think it is sufficient.
> > > > > > > > >
> > > > > > > > > You can restrict the function definition to the linux part of the
> > > > > > > > > PCI bus driver instead, using stubs for other systems.
> > > > > > > > >
> > > > > > > > > > > Signed-off-by: Manish Chopra <manishc@marvell.com>
> > > > > > > > > > > Signed-off-by: Igor Russkikh <irusskikh@marvell.com>
> > > > > > > > > > > ---
> > > > > > > > > > >  drivers/bus/pci/linux/pci_uio.c     |    2 +-
> > > > > > > > > > >  drivers/bus/pci/linux/pci_vfio.c    |    2 +-
> > > > > > > > > > >  drivers/net/bnx2x/bnx2x.h           |    2 +-
> > > > > > > > > > >  drivers/net/hns3/hns3_ethdev_vf.c   |    2 +-
> > > > > > > > > > >  drivers/vdpa/ifc/base/ifcvf_osdep.h |    2 +-
> > > > > > > > > > >  lib/librte_pci/Makefile             |    1 +
> > > > > > > > > > >  lib/librte_pci/meson.build          |    2 +-
> > > > > > > > > > >  lib/librte_pci/rte_pci_regs.h       | 1075 +++++++++++++++++++++++++++
> > > > > > > > > > >  8 files changed, 1082 insertions(+), 6 deletions(-)
> > > > > > > > > > >  create mode 100644 lib/librte_pci/rte_pci_regs.h
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > [...]
> > > > > > > > > >
> > > > > > > > > > > diff --git a/lib/librte_pci/rte_pci_regs.h b/lib/librte_pci/rte_pci_regs.h
> > > > > > > > > > > new file mode 100644
> > > > > > > > > > > index 000000000..1d11f4de5
> > > > > > > > > > > --- /dev/null
> > > > > > > > > > > +++ b/lib/librte_pci/rte_pci_regs.h
> > > > > > > > > > > @@ -0,0 +1,1075 @@
> > > > > > > > > > > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > > > > > > > > > > +/*
> > > > > > > > > >
> > > > > > > > > > This file is delivered alongside the PCI lib, targeting userspace.
> > > > > > > > > > This seems to be an exception to the license policy described in
> > > > > > > > > > license/README. Code shared between kernel and userspace is expected
> > > > > > > > > > to be dual-licensed BSD-3 and GPL-2.0.
> > > > > > > > > >
> > > > > > > > > > As it is a copy of Linux user includes, re-licensing it as BSD-3 as well
> > > > > > > > > > is not possible.
> > > > > > > > > >
> > > > > > > > > > So I think it might require a techboard + governing board exception
> > > > > > > > > > approval. Ferruh or Thomas, what do you think?
> > > > > > > >
> > > > > > > > I think, instead of importing GPL-2.0 file, We can add the constants
> > > > > > > > as need by the DPDK
> > > > > > > > as symbols start from RTE_PCI_*(It will fix up the namespace as well).
> > > > > > >
> > > > > > > If symbols can be found in /usr/include/, don't add anything.
> > > > > >
> > > > > > Not by default on all the distros. It is part of pciutils library.
> > > > > > Moreover, we need these symbols for Windows OS as well.
> > > > > > IMO, We should add absolute minimum constants that needed for DPDK as RTE_PCI_*
> > > > >
> > > > > I am for mandating the dependency instead of copying it.
> > > >
> > > > You mean _pciutils_ package as a mandatory dependency to  DPDK.
> > >
> > > There is already this dependency:
> > >         #include <linux/pci_regs.h>
> >
> > I just checked in archlinux, PCI headers can be provided by
> >
> > # pacman -F /usr/include/pci/header.h
> > usr/include/pci/header.h is owned by core/pciutils 3.7.0-
> >
> > # pacman -F /usr/include/linux/pci.h
> > usr/include/linux/pci.h is owned by core/linux-api-headers 5.4.17-1
> >
> >
> > > I'm missing the real justification for this patch.
> >
> > See below.
> >
> > > Is there some missing definitions?
> > > Is there some environments where this file is missing?
> > >
> > > > > pciutils cannot be installed on Windows?
> > > > > Why do you care about Windows?
> > > > > I don't see any contribution for qede on Windows.
> > > >
> > > > You closely review the patch, it not about qede. The proposed file
> > > > comes at lib/librte_pci/rte_pci_regs.h which is common to Windows.
> > >
> > > The series is for qede. I'm trying to understand the motivation.
> >
> > First version of qede driver sent with defined generic PCI symbols and
> > generic PCI function like pci_find_next_ext_capability() in qede driver.
>
> That's a pity the v2 is not threaded with v1,
> I would have found these explanations easily myself.
>
> > In the review, I suggested using generic rte_ function as
> > a) It is not specific to qede.
> > b) Other drivers also doing the same thing in their own driver space
> > as there is no dpdk API for the same.
> > This patches create generic API for pci_find_next_ext_capability() and
> > remove duplicate implementation
> > from the drivers.
> > http://patches.dpdk.org/patch/73959/
>
> I agree it's good to have an API for such thing.
>
> So far such feature is supported in drivers on Linux,
> requiring only Linux headers to be installed.
> Do we need more?

We would need only Linux headers for Implementing
rte_pci_find_next_ext_capability().
I leave, @Manish Chopra to comment on other PCI symbols requirements.



>
>

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [dpdk-dev] [PATCH v2 1/7] lib/librte_pci: add rte_pci_regs.h
  2020-07-16 16:57                     ` Thomas Monjalon
  2020-07-16 17:33                       ` Jerin Jacob
@ 2020-07-16 17:56                       ` Gaëtan Rivet
  2020-07-16 20:49                         ` [dpdk-dev] [EXT] " Manish Chopra
  1 sibling, 1 reply; 26+ messages in thread
From: Gaëtan Rivet @ 2020-07-16 17:56 UTC (permalink / raw)
  To: Thomas Monjalon
  Cc: Jerin Jacob, Manish Chopra, Ferruh Yigit, Igor Russkikh, dpdk-dev

On 16/07/20 18:57 +0200, Thomas Monjalon wrote:
> 16/07/2020 18:43, Jerin Jacob:
> > On Thu, Jul 16, 2020 at 9:25 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > >
> > > 16/07/2020 15:02, Jerin Jacob:
> > > > On Thu, Jul 16, 2020 at 6:20 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > > > >
> > > > > 16/07/2020 13:55, Jerin Jacob:
> > > > > > On Thu, Jul 16, 2020 at 4:57 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > > > > > >
> > > > > > > 16/07/2020 12:27, Jerin Jacob:
> > > > > > > > On Thu, Jul 16, 2020 at 3:48 PM Gaëtan Rivet <grive@u256.net> wrote:
> > > > > > > > >
> > > > > > > > > On 16/07/20 12:08 +0200, Gaëtan Rivet wrote:
> > > > > > > > > > Re-CCing dev@dpdk.org as it was removed from the reply.
> > > > > > > > > >
> > > > > > > > > > On 13/07/20 08:13 -0700, Manish Chopra wrote:
> > > > > > > > > > > This is merely copy of latest linux/pci_regs.h in
> > > > > > > > > > > order to avoid dependency of dpdk on user headers.
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > I guess this dependency is an issue on non-linux systems, when you must
> > > > > > > > > > use those defines in a generic implementation. Can you confirm this is
> > > > > > > > > > the motivation here?
> > > > > > > > > >
> > > > > > > > > > If so, I think it would be clearer to state "in order to avoid
> > > > > > > > > > dependency of DPDK on linux headers".
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > To add to it, if this is actually the motivation to add this header, I
> > > > > > > > > don't think it is sufficient.
> > > > > > > > >
> > > > > > > > > You can restrict the function definition to the linux part of the
> > > > > > > > > PCI bus driver instead, using stubs for other systems.
> > > > > > > > >
> > > > > > > > > > > Signed-off-by: Manish Chopra <manishc@marvell.com>
> > > > > > > > > > > Signed-off-by: Igor Russkikh <irusskikh@marvell.com>
> > > > > > > > > > > ---
> > > > > > > > > > >  drivers/bus/pci/linux/pci_uio.c     |    2 +-
> > > > > > > > > > >  drivers/bus/pci/linux/pci_vfio.c    |    2 +-
> > > > > > > > > > >  drivers/net/bnx2x/bnx2x.h           |    2 +-
> > > > > > > > > > >  drivers/net/hns3/hns3_ethdev_vf.c   |    2 +-
> > > > > > > > > > >  drivers/vdpa/ifc/base/ifcvf_osdep.h |    2 +-
> > > > > > > > > > >  lib/librte_pci/Makefile             |    1 +
> > > > > > > > > > >  lib/librte_pci/meson.build          |    2 +-
> > > > > > > > > > >  lib/librte_pci/rte_pci_regs.h       | 1075 +++++++++++++++++++++++++++
> > > > > > > > > > >  8 files changed, 1082 insertions(+), 6 deletions(-)
> > > > > > > > > > >  create mode 100644 lib/librte_pci/rte_pci_regs.h
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > [...]
> > > > > > > > > >
> > > > > > > > > > > diff --git a/lib/librte_pci/rte_pci_regs.h b/lib/librte_pci/rte_pci_regs.h
> > > > > > > > > > > new file mode 100644
> > > > > > > > > > > index 000000000..1d11f4de5
> > > > > > > > > > > --- /dev/null
> > > > > > > > > > > +++ b/lib/librte_pci/rte_pci_regs.h
> > > > > > > > > > > @@ -0,0 +1,1075 @@
> > > > > > > > > > > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > > > > > > > > > > +/*
> > > > > > > > > >
> > > > > > > > > > This file is delivered alongside the PCI lib, targeting userspace.
> > > > > > > > > > This seems to be an exception to the license policy described in
> > > > > > > > > > license/README. Code shared between kernel and userspace is expected
> > > > > > > > > > to be dual-licensed BSD-3 and GPL-2.0.
> > > > > > > > > >
> > > > > > > > > > As it is a copy of Linux user includes, re-licensing it as BSD-3 as well
> > > > > > > > > > is not possible.
> > > > > > > > > >
> > > > > > > > > > So I think it might require a techboard + governing board exception
> > > > > > > > > > approval. Ferruh or Thomas, what do you think?
> > > > > > > >
> > > > > > > > I think, instead of importing GPL-2.0 file, We can add the constants
> > > > > > > > as need by the DPDK
> > > > > > > > as symbols start from RTE_PCI_*(It will fix up the namespace as well).
> > > > > > >
> > > > > > > If symbols can be found in /usr/include/, don't add anything.
> > > > > >
> > > > > > Not by default on all the distros. It is part of pciutils library.
> > > > > > Moreover, we need these symbols for Windows OS as well.
> > > > > > IMO, We should add absolute minimum constants that needed for DPDK as RTE_PCI_*
> > > > >
> > > > > I am for mandating the dependency instead of copying it.
> > > >
> > > > You mean _pciutils_ package as a mandatory dependency to  DPDK.
> > >
> > > There is already this dependency:
> > >         #include <linux/pci_regs.h>
> > 
> > I just checked in archlinux, PCI headers can be provided by
> > 
> > # pacman -F /usr/include/pci/header.h
> > usr/include/pci/header.h is owned by core/pciutils 3.7.0-
> > 
> > # pacman -F /usr/include/linux/pci.h
> > usr/include/linux/pci.h is owned by core/linux-api-headers 5.4.17-1
> > 
> > 
> > > I'm missing the real justification for this patch.
> > 
> > See below.
> > 
> > > Is there some missing definitions?
> > > Is there some environments where this file is missing?
> > >
> > > > > pciutils cannot be installed on Windows?
> > > > > Why do you care about Windows?
> > > > > I don't see any contribution for qede on Windows.
> > > >
> > > > You closely review the patch, it not about qede. The proposed file
> > > > comes at lib/librte_pci/rte_pci_regs.h which is common to Windows.
> > >
> > > The series is for qede. I'm trying to understand the motivation.
> > 
> > First version of qede driver sent with defined generic PCI symbols and
> > generic PCI function like pci_find_next_ext_capability() in qede driver.
> 
> That's a pity the v2 is not threaded with v1,
> I would have found these explanations easily myself.
> 
> > In the review, I suggested using generic rte_ function as
> > a) It is not specific to qede.
> > b) Other drivers also doing the same thing in their own driver space
> > as there is no dpdk API for the same.
> > This patches create generic API for pci_find_next_ext_capability() and
> > remove duplicate implementation
> > from the drivers.
> > http://patches.dpdk.org/patch/73959/
> 
> I agree it's good to have an API for such thing.
> 
> So far such feature is supported in drivers on Linux,
> requiring only Linux headers to be installed.
> Do we need more?
> 
> 

+1 to make it generic, no question here.

On linux, the dependency is already there (either from linux headers or
pciutils) to have the original. So including this header in DPDK is only
useful for other OSes.

I think right now we should only add pci_find_next_ext_capability() full
implementation within linux part of PCI bus, other systems being stubs.

We can go with your suggestion Jerin about adding only the specific
symbols needed, prefixed with RTE_, once we decide to have windows
support. Question is whether we need it right now. Is there a driver
that would make use of it support more than linux currently?

-- 
Gaëtan

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [dpdk-dev] [EXT] Re: [PATCH v2 1/7] lib/librte_pci: add rte_pci_regs.h
  2020-07-16 17:56                       ` Gaëtan Rivet
@ 2020-07-16 20:49                         ` Manish Chopra
  2020-07-18 19:42                           ` Manish Chopra
  0 siblings, 1 reply; 26+ messages in thread
From: Manish Chopra @ 2020-07-16 20:49 UTC (permalink / raw)
  To: Gaëtan Rivet, Thomas Monjalon
  Cc: Jerin Jacob, Ferruh Yigit, Igor Russkikh, dpdk-dev

> -----Original Message-----
> From: Gaëtan Rivet <grive@u256.net>
> Sent: Thursday, July 16, 2020 11:26 PM
> To: Thomas Monjalon <thomas@monjalon.net>
> Cc: Jerin Jacob <jerinjacobk@gmail.com>; Manish Chopra
> <manishc@marvell.com>; Ferruh Yigit <ferruh.yigit@intel.com>; Igor
> Russkikh <irusskikh@marvell.com>; dpdk-dev <dev@dpdk.org>
> Subject: [EXT] Re: [dpdk-dev] [PATCH v2 1/7] lib/librte_pci: add
> rte_pci_regs.h
> 
> External Email
> 
> ----------------------------------------------------------------------
> On 16/07/20 18:57 +0200, Thomas Monjalon wrote:
> > 16/07/2020 18:43, Jerin Jacob:
> > > On Thu, Jul 16, 2020 at 9:25 PM Thomas Monjalon
> <thomas@monjalon.net> wrote:
> > > >
> > > > 16/07/2020 15:02, Jerin Jacob:
> > > > > On Thu, Jul 16, 2020 at 6:20 PM Thomas Monjalon
> <thomas@monjalon.net> wrote:
> > > > > >
> > > > > > 16/07/2020 13:55, Jerin Jacob:
> > > > > > > On Thu, Jul 16, 2020 at 4:57 PM Thomas Monjalon
> <thomas@monjalon.net> wrote:
> > > > > > > >
> > > > > > > > 16/07/2020 12:27, Jerin Jacob:
> > > > > > > > > On Thu, Jul 16, 2020 at 3:48 PM Gaëtan Rivet
> <grive@u256.net> wrote:
> > > > > > > > > >
> > > > > > > > > > On 16/07/20 12:08 +0200, Gaëtan Rivet wrote:
> > > > > > > > > > > Re-CCing dev@dpdk.org as it was removed from the reply.
> > > > > > > > > > >
> > > > > > > > > > > On 13/07/20 08:13 -0700, Manish Chopra wrote:
> > > > > > > > > > > > This is merely copy of latest linux/pci_regs.h in
> > > > > > > > > > > > order to avoid dependency of dpdk on user headers.
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > I guess this dependency is an issue on non-linux
> > > > > > > > > > > systems, when you must use those defines in a
> > > > > > > > > > > generic implementation. Can you confirm this is the
> motivation here?
> > > > > > > > > > >
> > > > > > > > > > > If so, I think it would be clearer to state "in
> > > > > > > > > > > order to avoid dependency of DPDK on linux headers".
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > To add to it, if this is actually the motivation to
> > > > > > > > > > add this header, I don't think it is sufficient.
> > > > > > > > > >
> > > > > > > > > > You can restrict the function definition to the linux
> > > > > > > > > > part of the PCI bus driver instead, using stubs for other
> systems.
> > > > > > > > > >
> > > > > > > > > > > > Signed-off-by: Manish Chopra <manishc@marvell.com>
> > > > > > > > > > > > Signed-off-by: Igor Russkikh
> > > > > > > > > > > > <irusskikh@marvell.com>
> > > > > > > > > > > > ---
> > > > > > > > > > > >  drivers/bus/pci/linux/pci_uio.c     |    2 +-
> > > > > > > > > > > >  drivers/bus/pci/linux/pci_vfio.c    |    2 +-
> > > > > > > > > > > >  drivers/net/bnx2x/bnx2x.h           |    2 +-
> > > > > > > > > > > >  drivers/net/hns3/hns3_ethdev_vf.c   |    2 +-
> > > > > > > > > > > >  drivers/vdpa/ifc/base/ifcvf_osdep.h |    2 +-
> > > > > > > > > > > >  lib/librte_pci/Makefile             |    1 +
> > > > > > > > > > > >  lib/librte_pci/meson.build          |    2 +-
> > > > > > > > > > > >  lib/librte_pci/rte_pci_regs.h       | 1075
> +++++++++++++++++++++++++++
> > > > > > > > > > > >  8 files changed, 1082 insertions(+), 6
> > > > > > > > > > > > deletions(-)  create mode 100644
> > > > > > > > > > > > lib/librte_pci/rte_pci_regs.h
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > [...]
> > > > > > > > > > >
> > > > > > > > > > > > diff --git a/lib/librte_pci/rte_pci_regs.h
> > > > > > > > > > > > b/lib/librte_pci/rte_pci_regs.h new file mode
> > > > > > > > > > > > 100644 index 000000000..1d11f4de5
> > > > > > > > > > > > --- /dev/null
> > > > > > > > > > > > +++ b/lib/librte_pci/rte_pci_regs.h
> > > > > > > > > > > > @@ -0,0 +1,1075 @@
> > > > > > > > > > > > +/* SPDX-License-Identifier: GPL-2.0 WITH
> > > > > > > > > > > > +Linux-syscall-note */
> > > > > > > > > > > > +/*
> > > > > > > > > > >
> > > > > > > > > > > This file is delivered alongside the PCI lib, targeting
> userspace.
> > > > > > > > > > > This seems to be an exception to the license policy
> > > > > > > > > > > described in license/README. Code shared between
> > > > > > > > > > > kernel and userspace is expected to be dual-licensed BSD-3
> and GPL-2.0.
> > > > > > > > > > >
> > > > > > > > > > > As it is a copy of Linux user includes, re-licensing
> > > > > > > > > > > it as BSD-3 as well is not possible.
> > > > > > > > > > >
> > > > > > > > > > > So I think it might require a techboard + governing
> > > > > > > > > > > board exception approval. Ferruh or Thomas, what do you
> think?
> > > > > > > > >
> > > > > > > > > I think, instead of importing GPL-2.0 file, We can add
> > > > > > > > > the constants as need by the DPDK as symbols start from
> > > > > > > > > RTE_PCI_*(It will fix up the namespace as well).
> > > > > > > >
> > > > > > > > If symbols can be found in /usr/include/, don't add anything.
> > > > > > >
> > > > > > > Not by default on all the distros. It is part of pciutils library.
> > > > > > > Moreover, we need these symbols for Windows OS as well.
> > > > > > > IMO, We should add absolute minimum constants that needed
> > > > > > > for DPDK as RTE_PCI_*
> > > > > >
> > > > > > I am for mandating the dependency instead of copying it.
> > > > >
> > > > > You mean _pciutils_ package as a mandatory dependency to  DPDK.
> > > >
> > > > There is already this dependency:
> > > >         #include <linux/pci_regs.h>
> > >
> > > I just checked in archlinux, PCI headers can be provided by
> > >
> > > # pacman -F /usr/include/pci/header.h usr/include/pci/header.h is
> > > owned by core/pciutils 3.7.0-
> > >
> > > # pacman -F /usr/include/linux/pci.h usr/include/linux/pci.h is
> > > owned by core/linux-api-headers 5.4.17-1
> > >
> > >
> > > > I'm missing the real justification for this patch.
> > >
> > > See below.
> > >
> > > > Is there some missing definitions?
> > > > Is there some environments where this file is missing?
> > > >
> > > > > > pciutils cannot be installed on Windows?
> > > > > > Why do you care about Windows?
> > > > > > I don't see any contribution for qede on Windows.
> > > > >
> > > > > You closely review the patch, it not about qede. The proposed
> > > > > file comes at lib/librte_pci/rte_pci_regs.h which is common to
> Windows.
> > > >
> > > > The series is for qede. I'm trying to understand the motivation.
> > >
> > > First version of qede driver sent with defined generic PCI symbols
> > > and generic PCI function like pci_find_next_ext_capability() in qede
> driver.
> >
> > That's a pity the v2 is not threaded with v1, I would have found these
> > explanations easily myself.
> >
> > > In the review, I suggested using generic rte_ function as
> > > a) It is not specific to qede.
> > > b) Other drivers also doing the same thing in their own driver space
> > > as there is no dpdk API for the same.
> > > This patches create generic API for pci_find_next_ext_capability()
> > > and remove duplicate implementation from the drivers.
> > > https://urldefense.proofpoint.com/v2/url?u=http-3A__patches.dpdk.org
> > >
> _patch_73959_&d=DwIDaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=bMTgx2X48QVX
> yXOEL8
> > > ALyI4dsWoR-m74c5n3d-
> ruJI8&m=eNuzGYhB7u2Wzru3VeBTY7QDZSSb9VQ9eQXW56D4
> > > 64Y&s=eatY5xyw-474yS0cBJXyG7gLyPXFo243P2LmBDDsXd8&e=
> >
> > I agree it's good to have an API for such thing.
> >
> > So far such feature is supported in drivers on Linux, requiring only
> > Linux headers to be installed.
> > Do we need more?
> >
> >
> 
> +1 to make it generic, no question here.
> 
> On linux, the dependency is already there (either from linux headers or
> pciutils) to have the original. So including this header in DPDK is only useful
> for other OSes.
> 
> I think right now we should only add pci_find_next_ext_capability() full
> implementation within linux part of PCI bus, other systems being stubs.
> 
> We can go with your suggestion Jerin about adding only the specific symbols
> needed, prefixed with RTE_, once we decide to have windows support.
> Question is whether we need it right now. Is there a driver that would make
> use of it support more than linux currently?
> 
> --

I don't know if there are any drivers which will require this other than linux as of today - 

My only motivation of adding these symbols in dpdk via rte_pci_regs.h (new file in lib/librte_pci/) was to avoid any dependency of dpdk on /usr/include/../pci_regs.h,
since I was little unsure whether in all distributions (linux/windows) supported will have the required PCI defines available in /usr/include/../pci_regs.h file or not in order
to implement rte_pci_find_next_ext_capability().  (unless user to bound for updating headers by mean of installing any latest _pciutils_/packages). Moreover, for not just
this API, but if going forward if we have to add any new APIs which could rely/depend on PCI defines availability under /usr/include.

From the discussion so far - 

1. Define the function under drivers/bus/pci/linux/pci.c only and add empty/stub implementation for windows/pci.c and bsd/pci.c ?
2.  Just relying on /usr/include/ is perfectly okay without adding any defines anywhere for now ?, it will just require <linux/pci_regs.h> inclusion in
     drivers/bus/pci/linux/pci.c. OR Shall I add (may be in lib/librte_pci/rte_pci.h ?) only required PCI defines with RTE_ prefixed and use them instead ?

Thanks,
Manish

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [dpdk-dev] [EXT] Re: [PATCH v2 1/7] lib/librte_pci: add rte_pci_regs.h
  2020-07-16 20:49                         ` [dpdk-dev] [EXT] " Manish Chopra
@ 2020-07-18 19:42                           ` Manish Chopra
  2020-07-19  8:47                             ` Jerin Jacob
  0 siblings, 1 reply; 26+ messages in thread
From: Manish Chopra @ 2020-07-18 19:42 UTC (permalink / raw)
  To: Manish Chopra, Gaëtan Rivet, Jerin Jacob
  Cc: Ferruh Yigit, Igor Russkikh, dpdk-dev, Thomas Monjalon

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Manish Chopra
> Sent: Friday, July 17, 2020 2:19 AM
> To: Gaëtan Rivet <grive@u256.net>; Thomas Monjalon
> <thomas@monjalon.net>
> Cc: Jerin Jacob <jerinjacobk@gmail.com>; Ferruh Yigit
> <ferruh.yigit@intel.com>; Igor Russkikh <irusskikh@marvell.com>; dpdk-dev
> <dev@dpdk.org>
> Subject: Re: [dpdk-dev] [EXT] Re: [PATCH v2 1/7] lib/librte_pci: add
> rte_pci_regs.h
> 
> > -----Original Message-----
> > From: Gaëtan Rivet <grive@u256.net>
> > Sent: Thursday, July 16, 2020 11:26 PM
> > To: Thomas Monjalon <thomas@monjalon.net>
> > Cc: Jerin Jacob <jerinjacobk@gmail.com>; Manish Chopra
> > <manishc@marvell.com>; Ferruh Yigit <ferruh.yigit@intel.com>; Igor
> > Russkikh <irusskikh@marvell.com>; dpdk-dev <dev@dpdk.org>
> > Subject: [EXT] Re: [dpdk-dev] [PATCH v2 1/7] lib/librte_pci: add
> > rte_pci_regs.h
> >
> > External Email
> >
> > ----------------------------------------------------------------------
> > On 16/07/20 18:57 +0200, Thomas Monjalon wrote:
> > > 16/07/2020 18:43, Jerin Jacob:
> > > > On Thu, Jul 16, 2020 at 9:25 PM Thomas Monjalon
> > <thomas@monjalon.net> wrote:
> > > > >
> > > > > 16/07/2020 15:02, Jerin Jacob:
> > > > > > On Thu, Jul 16, 2020 at 6:20 PM Thomas Monjalon
> > <thomas@monjalon.net> wrote:
> > > > > > >
> > > > > > > 16/07/2020 13:55, Jerin Jacob:
> > > > > > > > On Thu, Jul 16, 2020 at 4:57 PM Thomas Monjalon
> > <thomas@monjalon.net> wrote:
> > > > > > > > >
> > > > > > > > > 16/07/2020 12:27, Jerin Jacob:
> > > > > > > > > > On Thu, Jul 16, 2020 at 3:48 PM Gaëtan Rivet
> > <grive@u256.net> wrote:
> > > > > > > > > > >
> > > > > > > > > > > On 16/07/20 12:08 +0200, Gaëtan Rivet wrote:
> > > > > > > > > > > > Re-CCing dev@dpdk.org as it was removed from the
> reply.
> > > > > > > > > > > >
> > > > > > > > > > > > On 13/07/20 08:13 -0700, Manish Chopra wrote:
> > > > > > > > > > > > > This is merely copy of latest linux/pci_regs.h
> > > > > > > > > > > > > in order to avoid dependency of dpdk on user headers.
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > I guess this dependency is an issue on non-linux
> > > > > > > > > > > > systems, when you must use those defines in a
> > > > > > > > > > > > generic implementation. Can you confirm this is
> > > > > > > > > > > > the
> > motivation here?
> > > > > > > > > > > >
> > > > > > > > > > > > If so, I think it would be clearer to state "in
> > > > > > > > > > > > order to avoid dependency of DPDK on linux headers".
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > To add to it, if this is actually the motivation to
> > > > > > > > > > > add this header, I don't think it is sufficient.
> > > > > > > > > > >
> > > > > > > > > > > You can restrict the function definition to the
> > > > > > > > > > > linux part of the PCI bus driver instead, using
> > > > > > > > > > > stubs for other
> > systems.
> > > > > > > > > > >
> > > > > > > > > > > > > Signed-off-by: Manish Chopra
> > > > > > > > > > > > > <manishc@marvell.com>
> > > > > > > > > > > > > Signed-off-by: Igor Russkikh
> > > > > > > > > > > > > <irusskikh@marvell.com>
> > > > > > > > > > > > > ---
> > > > > > > > > > > > >  drivers/bus/pci/linux/pci_uio.c     |    2 +-
> > > > > > > > > > > > >  drivers/bus/pci/linux/pci_vfio.c    |    2 +-
> > > > > > > > > > > > >  drivers/net/bnx2x/bnx2x.h           |    2 +-
> > > > > > > > > > > > >  drivers/net/hns3/hns3_ethdev_vf.c   |    2 +-
> > > > > > > > > > > > >  drivers/vdpa/ifc/base/ifcvf_osdep.h |    2 +-
> > > > > > > > > > > > >  lib/librte_pci/Makefile             |    1 +
> > > > > > > > > > > > >  lib/librte_pci/meson.build          |    2 +-
> > > > > > > > > > > > >  lib/librte_pci/rte_pci_regs.h       | 1075
> > +++++++++++++++++++++++++++
> > > > > > > > > > > > >  8 files changed, 1082 insertions(+), 6
> > > > > > > > > > > > > deletions(-)  create mode 100644
> > > > > > > > > > > > > lib/librte_pci/rte_pci_regs.h
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > [...]
> > > > > > > > > > > >
> > > > > > > > > > > > > diff --git a/lib/librte_pci/rte_pci_regs.h
> > > > > > > > > > > > > b/lib/librte_pci/rte_pci_regs.h new file mode
> > > > > > > > > > > > > 100644 index 000000000..1d11f4de5
> > > > > > > > > > > > > --- /dev/null
> > > > > > > > > > > > > +++ b/lib/librte_pci/rte_pci_regs.h
> > > > > > > > > > > > > @@ -0,0 +1,1075 @@
> > > > > > > > > > > > > +/* SPDX-License-Identifier: GPL-2.0 WITH
> > > > > > > > > > > > > +Linux-syscall-note */
> > > > > > > > > > > > > +/*
> > > > > > > > > > > >
> > > > > > > > > > > > This file is delivered alongside the PCI lib,
> > > > > > > > > > > > targeting
> > userspace.
> > > > > > > > > > > > This seems to be an exception to the license
> > > > > > > > > > > > policy described in license/README. Code shared
> > > > > > > > > > > > between kernel and userspace is expected to be
> > > > > > > > > > > > dual-licensed BSD-3
> > and GPL-2.0.
> > > > > > > > > > > >
> > > > > > > > > > > > As it is a copy of Linux user includes,
> > > > > > > > > > > > re-licensing it as BSD-3 as well is not possible.
> > > > > > > > > > > >
> > > > > > > > > > > > So I think it might require a techboard +
> > > > > > > > > > > > governing board exception approval. Ferruh or
> > > > > > > > > > > > Thomas, what do you
> > think?
> > > > > > > > > >
> > > > > > > > > > I think, instead of importing GPL-2.0 file, We can add
> > > > > > > > > > the constants as need by the DPDK as symbols start
> > > > > > > > > > from RTE_PCI_*(It will fix up the namespace as well).
> > > > > > > > >
> > > > > > > > > If symbols can be found in /usr/include/, don't add anything.
> > > > > > > >
> > > > > > > > Not by default on all the distros. It is part of pciutils library.
> > > > > > > > Moreover, we need these symbols for Windows OS as well.
> > > > > > > > IMO, We should add absolute minimum constants that needed
> > > > > > > > for DPDK as RTE_PCI_*
> > > > > > >
> > > > > > > I am for mandating the dependency instead of copying it.
> > > > > >
> > > > > > You mean _pciutils_ package as a mandatory dependency to  DPDK.
> > > > >
> > > > > There is already this dependency:
> > > > >         #include <linux/pci_regs.h>
> > > >
> > > > I just checked in archlinux, PCI headers can be provided by
> > > >
> > > > # pacman -F /usr/include/pci/header.h usr/include/pci/header.h is
> > > > owned by core/pciutils 3.7.0-
> > > >
> > > > # pacman -F /usr/include/linux/pci.h usr/include/linux/pci.h is
> > > > owned by core/linux-api-headers 5.4.17-1
> > > >
> > > >
> > > > > I'm missing the real justification for this patch.
> > > >
> > > > See below.
> > > >
> > > > > Is there some missing definitions?
> > > > > Is there some environments where this file is missing?
> > > > >
> > > > > > > pciutils cannot be installed on Windows?
> > > > > > > Why do you care about Windows?
> > > > > > > I don't see any contribution for qede on Windows.
> > > > > >
> > > > > > You closely review the patch, it not about qede. The proposed
> > > > > > file comes at lib/librte_pci/rte_pci_regs.h which is common to
> > Windows.
> > > > >
> > > > > The series is for qede. I'm trying to understand the motivation.
> > > >
> > > > First version of qede driver sent with defined generic PCI symbols
> > > > and generic PCI function like pci_find_next_ext_capability() in
> > > > qede
> > driver.
> > >
> > > That's a pity the v2 is not threaded with v1, I would have found
> > > these explanations easily myself.
> > >
> > > > In the review, I suggested using generic rte_ function as
> > > > a) It is not specific to qede.
> > > > b) Other drivers also doing the same thing in their own driver
> > > > space as there is no dpdk API for the same.
> > > > This patches create generic API for pci_find_next_ext_capability()
> > > > and remove duplicate implementation from the drivers.
> > > > https://urldefense.proofpoint.com/v2/url?u=http-3A__patches.dpdk.o
> > > > rg
> > > >
> >
> _patch_73959_&d=DwIDaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=bMTgx2X48QVX
> > yXOEL8
> > > > ALyI4dsWoR-m74c5n3d-
> > ruJI8&m=eNuzGYhB7u2Wzru3VeBTY7QDZSSb9VQ9eQXW56D4
> > > > 64Y&s=eatY5xyw-474yS0cBJXyG7gLyPXFo243P2LmBDDsXd8&e=
> > >
> > > I agree it's good to have an API for such thing.
> > >
> > > So far such feature is supported in drivers on Linux, requiring only
> > > Linux headers to be installed.
> > > Do we need more?
> > >
> > >
> >
> > +1 to make it generic, no question here.
> >
> > On linux, the dependency is already there (either from linux headers
> > or
> > pciutils) to have the original. So including this header in DPDK is
> > only useful for other OSes.
> >
> > I think right now we should only add pci_find_next_ext_capability()
> > full implementation within linux part of PCI bus, other systems being
> stubs.
> >
> > We can go with your suggestion Jerin about adding only the specific
> > symbols needed, prefixed with RTE_, once we decide to have windows
> support.
> > Question is whether we need it right now. Is there a driver that would
> > make use of it support more than linux currently?
> >
> > --
> 
> I don't know if there are any drivers which will require this other than linux
> as of today -
> 
> My only motivation of adding these symbols in dpdk via rte_pci_regs.h (new
> file in lib/librte_pci/) was to avoid any dependency of dpdk on
> /usr/include/../pci_regs.h, since I was little unsure whether in all
> distributions (linux/windows) supported will have the required PCI defines
> available in /usr/include/../pci_regs.h file or not in order to implement
> rte_pci_find_next_ext_capability().  (unless user to bound for updating
> headers by mean of installing any latest _pciutils_/packages). Moreover, for
> not just this API, but if going forward if we have to add any new APIs which
> could rely/depend on PCI defines availability under /usr/include.
> 
> From the discussion so far -
> 
> 1. Define the function under drivers/bus/pci/linux/pci.c only and add
> empty/stub implementation for windows/pci.c and bsd/pci.c ?
> 2.  Just relying on /usr/include/ is perfectly okay without adding any defines
> anywhere for now ?, it will just require <linux/pci_regs.h> inclusion in
>      drivers/bus/pci/linux/pci.c. OR Shall I add (may be in
> lib/librte_pci/rte_pci.h ?) only required PCI defines with RTE_ prefixed and
> use them instead ?
> 

Hello Gaetan/Jerin,

Could you please comment on above - what's sufficient to be incorporated in v3 for now ?
I will work on the changes accordingly.

Thanks,
Manish

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [dpdk-dev] [EXT] Re: [PATCH v2 1/7] lib/librte_pci: add rte_pci_regs.h
  2020-07-18 19:42                           ` Manish Chopra
@ 2020-07-19  8:47                             ` Jerin Jacob
  2020-07-20  8:57                               ` Gaëtan Rivet
  0 siblings, 1 reply; 26+ messages in thread
From: Jerin Jacob @ 2020-07-19  8:47 UTC (permalink / raw)
  To: Manish Chopra
  Cc: Gaëtan Rivet, Ferruh Yigit, Igor Russkikh, dpdk-dev,
	Thomas Monjalon

On Sun, Jul 19, 2020 at 1:12 AM Manish Chopra <manishc@marvell.com> wrote:
>
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Manish Chopra
> > Sent: Friday, July 17, 2020 2:19 AM
> > To: Gaëtan Rivet <grive@u256.net>; Thomas Monjalon
> > <thomas@monjalon.net>
> > Cc: Jerin Jacob <jerinjacobk@gmail.com>; Ferruh Yigit
> > <ferruh.yigit@intel.com>; Igor Russkikh <irusskikh@marvell.com>; dpdk-dev
> > <dev@dpdk.org>
> > Subject: Re: [dpdk-dev] [EXT] Re: [PATCH v2 1/7] lib/librte_pci: add
> > rte_pci_regs.h
> >
> > > -----Original Message-----
> > > From: Gaëtan Rivet <grive@u256.net>
> > > Sent: Thursday, July 16, 2020 11:26 PM
> > > To: Thomas Monjalon <thomas@monjalon.net>
> > > Cc: Jerin Jacob <jerinjacobk@gmail.com>; Manish Chopra
> > > <manishc@marvell.com>; Ferruh Yigit <ferruh.yigit@intel.com>; Igor
> > > Russkikh <irusskikh@marvell.com>; dpdk-dev <dev@dpdk.org>
> > > Subject: [EXT] Re: [dpdk-dev] [PATCH v2 1/7] lib/librte_pci: add
> > > rte_pci_regs.h
> > >
> > > External Email
> > >
> > > ----------------------------------------------------------------------
> > > On 16/07/20 18:57 +0200, Thomas Monjalon wrote:
> > > > 16/07/2020 18:43, Jerin Jacob:
> > > > > On Thu, Jul 16, 2020 at 9:25 PM Thomas Monjalon
> > > <thomas@monjalon.net> wrote:
> > > > > >
> > > > > > 16/07/2020 15:02, Jerin Jacob:
> > > > > > > On Thu, Jul 16, 2020 at 6:20 PM Thomas Monjalon
> > > <thomas@monjalon.net> wrote:
> > > > > > > >
> > > > > > > > 16/07/2020 13:55, Jerin Jacob:
> > > > > > > > > On Thu, Jul 16, 2020 at 4:57 PM Thomas Monjalon
> > > <thomas@monjalon.net> wrote:
> > > > > > > > > >
> > > > > > > > > > 16/07/2020 12:27, Jerin Jacob:
> > > > > > > > > > > On Thu, Jul 16, 2020 at 3:48 PM Gaëtan Rivet
> > > <grive@u256.net> wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > On 16/07/20 12:08 +0200, Gaëtan Rivet wrote:
> > > > > > > > > > > > > Re-CCing dev@dpdk.org as it was removed from the
> > reply.
> > > > > > > > > > > > >
> > > > > > > > > > > > > On 13/07/20 08:13 -0700, Manish Chopra wrote:
> > > > > > > > > > > > > > This is merely copy of latest linux/pci_regs.h
> > > > > > > > > > > > > > in order to avoid dependency of dpdk on user headers.
> > > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > > I guess this dependency is an issue on non-linux
> > > > > > > > > > > > > systems, when you must use those defines in a
> > > > > > > > > > > > > generic implementation. Can you confirm this is
> > > > > > > > > > > > > the
> > > motivation here?
> > > > > > > > > > > > >
> > > > > > > > > > > > > If so, I think it would be clearer to state "in
> > > > > > > > > > > > > order to avoid dependency of DPDK on linux headers".
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > To add to it, if this is actually the motivation to
> > > > > > > > > > > > add this header, I don't think it is sufficient.
> > > > > > > > > > > >
> > > > > > > > > > > > You can restrict the function definition to the
> > > > > > > > > > > > linux part of the PCI bus driver instead, using
> > > > > > > > > > > > stubs for other
> > > systems.
> > > > > > > > > > > >
> > > > > > > > > > > > > > Signed-off-by: Manish Chopra
> > > > > > > > > > > > > > <manishc@marvell.com>
> > > > > > > > > > > > > > Signed-off-by: Igor Russkikh
> > > > > > > > > > > > > > <irusskikh@marvell.com>
> > > > > > > > > > > > > > ---
> > > > > > > > > > > > > >  drivers/bus/pci/linux/pci_uio.c     |    2 +-
> > > > > > > > > > > > > >  drivers/bus/pci/linux/pci_vfio.c    |    2 +-
> > > > > > > > > > > > > >  drivers/net/bnx2x/bnx2x.h           |    2 +-
> > > > > > > > > > > > > >  drivers/net/hns3/hns3_ethdev_vf.c   |    2 +-
> > > > > > > > > > > > > >  drivers/vdpa/ifc/base/ifcvf_osdep.h |    2 +-
> > > > > > > > > > > > > >  lib/librte_pci/Makefile             |    1 +
> > > > > > > > > > > > > >  lib/librte_pci/meson.build          |    2 +-
> > > > > > > > > > > > > >  lib/librte_pci/rte_pci_regs.h       | 1075
> > > +++++++++++++++++++++++++++
> > > > > > > > > > > > > >  8 files changed, 1082 insertions(+), 6
> > > > > > > > > > > > > > deletions(-)  create mode 100644
> > > > > > > > > > > > > > lib/librte_pci/rte_pci_regs.h
> > > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > > [...]
> > > > > > > > > > > > >
> > > > > > > > > > > > > > diff --git a/lib/librte_pci/rte_pci_regs.h
> > > > > > > > > > > > > > b/lib/librte_pci/rte_pci_regs.h new file mode
> > > > > > > > > > > > > > 100644 index 000000000..1d11f4de5
> > > > > > > > > > > > > > --- /dev/null
> > > > > > > > > > > > > > +++ b/lib/librte_pci/rte_pci_regs.h
> > > > > > > > > > > > > > @@ -0,0 +1,1075 @@
> > > > > > > > > > > > > > +/* SPDX-License-Identifier: GPL-2.0 WITH
> > > > > > > > > > > > > > +Linux-syscall-note */
> > > > > > > > > > > > > > +/*
> > > > > > > > > > > > >
> > > > > > > > > > > > > This file is delivered alongside the PCI lib,
> > > > > > > > > > > > > targeting
> > > userspace.
> > > > > > > > > > > > > This seems to be an exception to the license
> > > > > > > > > > > > > policy described in license/README. Code shared
> > > > > > > > > > > > > between kernel and userspace is expected to be
> > > > > > > > > > > > > dual-licensed BSD-3
> > > and GPL-2.0.
> > > > > > > > > > > > >
> > > > > > > > > > > > > As it is a copy of Linux user includes,
> > > > > > > > > > > > > re-licensing it as BSD-3 as well is not possible.
> > > > > > > > > > > > >
> > > > > > > > > > > > > So I think it might require a techboard +
> > > > > > > > > > > > > governing board exception approval. Ferruh or
> > > > > > > > > > > > > Thomas, what do you
> > > think?
> > > > > > > > > > >
> > > > > > > > > > > I think, instead of importing GPL-2.0 file, We can add
> > > > > > > > > > > the constants as need by the DPDK as symbols start
> > > > > > > > > > > from RTE_PCI_*(It will fix up the namespace as well).
> > > > > > > > > >
> > > > > > > > > > If symbols can be found in /usr/include/, don't add anything.
> > > > > > > > >
> > > > > > > > > Not by default on all the distros. It is part of pciutils library.
> > > > > > > > > Moreover, we need these symbols for Windows OS as well.
> > > > > > > > > IMO, We should add absolute minimum constants that needed
> > > > > > > > > for DPDK as RTE_PCI_*
> > > > > > > >
> > > > > > > > I am for mandating the dependency instead of copying it.
> > > > > > >
> > > > > > > You mean _pciutils_ package as a mandatory dependency to  DPDK.
> > > > > >
> > > > > > There is already this dependency:
> > > > > >         #include <linux/pci_regs.h>
> > > > >
> > > > > I just checked in archlinux, PCI headers can be provided by
> > > > >
> > > > > # pacman -F /usr/include/pci/header.h usr/include/pci/header.h is
> > > > > owned by core/pciutils 3.7.0-
> > > > >
> > > > > # pacman -F /usr/include/linux/pci.h usr/include/linux/pci.h is
> > > > > owned by core/linux-api-headers 5.4.17-1
> > > > >
> > > > >
> > > > > > I'm missing the real justification for this patch.
> > > > >
> > > > > See below.
> > > > >
> > > > > > Is there some missing definitions?
> > > > > > Is there some environments where this file is missing?
> > > > > >
> > > > > > > > pciutils cannot be installed on Windows?
> > > > > > > > Why do you care about Windows?
> > > > > > > > I don't see any contribution for qede on Windows.
> > > > > > >
> > > > > > > You closely review the patch, it not about qede. The proposed
> > > > > > > file comes at lib/librte_pci/rte_pci_regs.h which is common to
> > > Windows.
> > > > > >
> > > > > > The series is for qede. I'm trying to understand the motivation.
> > > > >
> > > > > First version of qede driver sent with defined generic PCI symbols
> > > > > and generic PCI function like pci_find_next_ext_capability() in
> > > > > qede
> > > driver.
> > > >
> > > > That's a pity the v2 is not threaded with v1, I would have found
> > > > these explanations easily myself.
> > > >
> > > > > In the review, I suggested using generic rte_ function as
> > > > > a) It is not specific to qede.
> > > > > b) Other drivers also doing the same thing in their own driver
> > > > > space as there is no dpdk API for the same.
> > > > > This patches create generic API for pci_find_next_ext_capability()
> > > > > and remove duplicate implementation from the drivers.
> > > > > https://urldefense.proofpoint.com/v2/url?u=http-3A__patches.dpdk.o
> > > > > rg
> > > > >
> > >
> > _patch_73959_&d=DwIDaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=bMTgx2X48QVX
> > > yXOEL8
> > > > > ALyI4dsWoR-m74c5n3d-
> > > ruJI8&m=eNuzGYhB7u2Wzru3VeBTY7QDZSSb9VQ9eQXW56D4
> > > > > 64Y&s=eatY5xyw-474yS0cBJXyG7gLyPXFo243P2LmBDDsXd8&e=
> > > >
> > > > I agree it's good to have an API for such thing.
> > > >
> > > > So far such feature is supported in drivers on Linux, requiring only
> > > > Linux headers to be installed.
> > > > Do we need more?
> > > >
> > > >
> > >
> > > +1 to make it generic, no question here.
> > >
> > > On linux, the dependency is already there (either from linux headers
> > > or
> > > pciutils) to have the original. So including this header in DPDK is
> > > only useful for other OSes.
> > >
> > > I think right now we should only add pci_find_next_ext_capability()
> > > full implementation within linux part of PCI bus, other systems being
> > stubs.
> > >
> > > We can go with your suggestion Jerin about adding only the specific
> > > symbols needed, prefixed with RTE_, once we decide to have windows
> > support.
> > > Question is whether we need it right now. Is there a driver that would
> > > make use of it support more than linux currently?
> > >
> > > --
> >
> > I don't know if there are any drivers which will require this other than linux
> > as of today -
> >
> > My only motivation of adding these symbols in dpdk via rte_pci_regs.h (new
> > file in lib/librte_pci/) was to avoid any dependency of dpdk on
> > /usr/include/../pci_regs.h, since I was little unsure whether in all
> > distributions (linux/windows) supported will have the required PCI defines
> > available in /usr/include/../pci_regs.h file or not in order to implement
> > rte_pci_find_next_ext_capability().  (unless user to bound for updating
> > headers by mean of installing any latest _pciutils_/packages). Moreover, for
> > not just this API, but if going forward if we have to add any new APIs which
> > could rely/depend on PCI defines availability under /usr/include.
> >
> > From the discussion so far -
> >
> > 1. Define the function under drivers/bus/pci/linux/pci.c only and add
> > empty/stub implementation for windows/pci.c and bsd/pci.c ?
> > 2.  Just relying on /usr/include/ is perfectly okay without adding any defines
> > anywhere for now ?, it will just require <linux/pci_regs.h> inclusion in
> >      drivers/bus/pci/linux/pci.c. OR Shall I add (may be in
> > lib/librte_pci/rte_pci.h ?) only required PCI defines with RTE_ prefixed and
> > use them instead ?
> >
>
> Hello Gaetan/Jerin,
>
> Could you please comment on above - what's sufficient to be incorporated in v3 for now ?
> I will work on the changes accordingly.

I am for introducing a handful(Not the completed list, Only add what
is needed for DPDK) of PCI constants as RTE_PCI_ in our public API.
And implement rte_pci_find_next_ext_capability() as common routine as
it is not specific to Linux.(I.e reading the config registers should
be only Linux/Windows-specific)

I leave the final decision to PCI and other maintainers.




>
> Thanks,
> Manish

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [dpdk-dev] [EXT] Re: [PATCH v2 1/7] lib/librte_pci: add rte_pci_regs.h
  2020-07-19  8:47                             ` Jerin Jacob
@ 2020-07-20  8:57                               ` Gaëtan Rivet
  0 siblings, 0 replies; 26+ messages in thread
From: Gaëtan Rivet @ 2020-07-20  8:57 UTC (permalink / raw)
  To: Jerin Jacob
  Cc: Manish Chopra, Ferruh Yigit, Igor Russkikh, dpdk-dev, Thomas Monjalon

On 19/07/20 14:17 +0530, Jerin Jacob wrote:
> On Sun, Jul 19, 2020 at 1:12 AM Manish Chopra <manishc@marvell.com> wrote:
> >
> > > -----Original Message-----
> > > From: dev <dev-bounces@dpdk.org> On Behalf Of Manish Chopra
> > > Sent: Friday, July 17, 2020 2:19 AM
> > > To: Gaëtan Rivet <grive@u256.net>; Thomas Monjalon
> > > <thomas@monjalon.net>
> > > Cc: Jerin Jacob <jerinjacobk@gmail.com>; Ferruh Yigit
> > > <ferruh.yigit@intel.com>; Igor Russkikh <irusskikh@marvell.com>; dpdk-dev
> > > <dev@dpdk.org>
> > > Subject: Re: [dpdk-dev] [EXT] Re: [PATCH v2 1/7] lib/librte_pci: add
> > > rte_pci_regs.h
> > >
> > > > -----Original Message-----
> > > > From: Gaëtan Rivet <grive@u256.net>
> > > > Sent: Thursday, July 16, 2020 11:26 PM
> > > > To: Thomas Monjalon <thomas@monjalon.net>
> > > > Cc: Jerin Jacob <jerinjacobk@gmail.com>; Manish Chopra
> > > > <manishc@marvell.com>; Ferruh Yigit <ferruh.yigit@intel.com>; Igor
> > > > Russkikh <irusskikh@marvell.com>; dpdk-dev <dev@dpdk.org>
> > > > Subject: [EXT] Re: [dpdk-dev] [PATCH v2 1/7] lib/librte_pci: add
> > > > rte_pci_regs.h
> > > >
> > > > External Email
> > > >
> > > > ----------------------------------------------------------------------
> > > > On 16/07/20 18:57 +0200, Thomas Monjalon wrote:
> > > > > 16/07/2020 18:43, Jerin Jacob:
> > > > > > On Thu, Jul 16, 2020 at 9:25 PM Thomas Monjalon
> > > > <thomas@monjalon.net> wrote:
> > > > > > >
> > > > > > > 16/07/2020 15:02, Jerin Jacob:
> > > > > > > > On Thu, Jul 16, 2020 at 6:20 PM Thomas Monjalon
> > > > <thomas@monjalon.net> wrote:
> > > > > > > > >
> > > > > > > > > 16/07/2020 13:55, Jerin Jacob:
> > > > > > > > > > On Thu, Jul 16, 2020 at 4:57 PM Thomas Monjalon
> > > > <thomas@monjalon.net> wrote:
> > > > > > > > > > >
> > > > > > > > > > > 16/07/2020 12:27, Jerin Jacob:
> > > > > > > > > > > > On Thu, Jul 16, 2020 at 3:48 PM Gaëtan Rivet
> > > > <grive@u256.net> wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > On 16/07/20 12:08 +0200, Gaëtan Rivet wrote:
> > > > > > > > > > > > > > Re-CCing dev@dpdk.org as it was removed from the
> > > reply.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > On 13/07/20 08:13 -0700, Manish Chopra wrote:
> > > > > > > > > > > > > > > This is merely copy of latest linux/pci_regs.h
> > > > > > > > > > > > > > > in order to avoid dependency of dpdk on user headers.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > I guess this dependency is an issue on non-linux
> > > > > > > > > > > > > > systems, when you must use those defines in a
> > > > > > > > > > > > > > generic implementation. Can you confirm this is
> > > > > > > > > > > > > > the
> > > > motivation here?
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > If so, I think it would be clearer to state "in
> > > > > > > > > > > > > > order to avoid dependency of DPDK on linux headers".
> > > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > > To add to it, if this is actually the motivation to
> > > > > > > > > > > > > add this header, I don't think it is sufficient.
> > > > > > > > > > > > >
> > > > > > > > > > > > > You can restrict the function definition to the
> > > > > > > > > > > > > linux part of the PCI bus driver instead, using
> > > > > > > > > > > > > stubs for other
> > > > systems.
> > > > > > > > > > > > >
> > > > > > > > > > > > > > > Signed-off-by: Manish Chopra
> > > > > > > > > > > > > > > <manishc@marvell.com>
> > > > > > > > > > > > > > > Signed-off-by: Igor Russkikh
> > > > > > > > > > > > > > > <irusskikh@marvell.com>
> > > > > > > > > > > > > > > ---
> > > > > > > > > > > > > > >  drivers/bus/pci/linux/pci_uio.c     |    2 +-
> > > > > > > > > > > > > > >  drivers/bus/pci/linux/pci_vfio.c    |    2 +-
> > > > > > > > > > > > > > >  drivers/net/bnx2x/bnx2x.h           |    2 +-
> > > > > > > > > > > > > > >  drivers/net/hns3/hns3_ethdev_vf.c   |    2 +-
> > > > > > > > > > > > > > >  drivers/vdpa/ifc/base/ifcvf_osdep.h |    2 +-
> > > > > > > > > > > > > > >  lib/librte_pci/Makefile             |    1 +
> > > > > > > > > > > > > > >  lib/librte_pci/meson.build          |    2 +-
> > > > > > > > > > > > > > >  lib/librte_pci/rte_pci_regs.h       | 1075
> > > > +++++++++++++++++++++++++++
> > > > > > > > > > > > > > >  8 files changed, 1082 insertions(+), 6
> > > > > > > > > > > > > > > deletions(-)  create mode 100644
> > > > > > > > > > > > > > > lib/librte_pci/rte_pci_regs.h
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > [...]
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > > diff --git a/lib/librte_pci/rte_pci_regs.h
> > > > > > > > > > > > > > > b/lib/librte_pci/rte_pci_regs.h new file mode
> > > > > > > > > > > > > > > 100644 index 000000000..1d11f4de5
> > > > > > > > > > > > > > > --- /dev/null
> > > > > > > > > > > > > > > +++ b/lib/librte_pci/rte_pci_regs.h
> > > > > > > > > > > > > > > @@ -0,0 +1,1075 @@
> > > > > > > > > > > > > > > +/* SPDX-License-Identifier: GPL-2.0 WITH
> > > > > > > > > > > > > > > +Linux-syscall-note */
> > > > > > > > > > > > > > > +/*
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > This file is delivered alongside the PCI lib,
> > > > > > > > > > > > > > targeting
> > > > userspace.
> > > > > > > > > > > > > > This seems to be an exception to the license
> > > > > > > > > > > > > > policy described in license/README. Code shared
> > > > > > > > > > > > > > between kernel and userspace is expected to be
> > > > > > > > > > > > > > dual-licensed BSD-3
> > > > and GPL-2.0.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > As it is a copy of Linux user includes,
> > > > > > > > > > > > > > re-licensing it as BSD-3 as well is not possible.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > So I think it might require a techboard +
> > > > > > > > > > > > > > governing board exception approval. Ferruh or
> > > > > > > > > > > > > > Thomas, what do you
> > > > think?
> > > > > > > > > > > >
> > > > > > > > > > > > I think, instead of importing GPL-2.0 file, We can add
> > > > > > > > > > > > the constants as need by the DPDK as symbols start
> > > > > > > > > > > > from RTE_PCI_*(It will fix up the namespace as well).
> > > > > > > > > > >
> > > > > > > > > > > If symbols can be found in /usr/include/, don't add anything.
> > > > > > > > > >
> > > > > > > > > > Not by default on all the distros. It is part of pciutils library.
> > > > > > > > > > Moreover, we need these symbols for Windows OS as well.
> > > > > > > > > > IMO, We should add absolute minimum constants that needed
> > > > > > > > > > for DPDK as RTE_PCI_*
> > > > > > > > >
> > > > > > > > > I am for mandating the dependency instead of copying it.
> > > > > > > >
> > > > > > > > You mean _pciutils_ package as a mandatory dependency to  DPDK.
> > > > > > >
> > > > > > > There is already this dependency:
> > > > > > >         #include <linux/pci_regs.h>
> > > > > >
> > > > > > I just checked in archlinux, PCI headers can be provided by
> > > > > >
> > > > > > # pacman -F /usr/include/pci/header.h usr/include/pci/header.h is
> > > > > > owned by core/pciutils 3.7.0-
> > > > > >
> > > > > > # pacman -F /usr/include/linux/pci.h usr/include/linux/pci.h is
> > > > > > owned by core/linux-api-headers 5.4.17-1
> > > > > >
> > > > > >
> > > > > > > I'm missing the real justification for this patch.
> > > > > >
> > > > > > See below.
> > > > > >
> > > > > > > Is there some missing definitions?
> > > > > > > Is there some environments where this file is missing?
> > > > > > >
> > > > > > > > > pciutils cannot be installed on Windows?
> > > > > > > > > Why do you care about Windows?
> > > > > > > > > I don't see any contribution for qede on Windows.
> > > > > > > >
> > > > > > > > You closely review the patch, it not about qede. The proposed
> > > > > > > > file comes at lib/librte_pci/rte_pci_regs.h which is common to
> > > > Windows.
> > > > > > >
> > > > > > > The series is for qede. I'm trying to understand the motivation.
> > > > > >
> > > > > > First version of qede driver sent with defined generic PCI symbols
> > > > > > and generic PCI function like pci_find_next_ext_capability() in
> > > > > > qede
> > > > driver.
> > > > >
> > > > > That's a pity the v2 is not threaded with v1, I would have found
> > > > > these explanations easily myself.
> > > > >
> > > > > > In the review, I suggested using generic rte_ function as
> > > > > > a) It is not specific to qede.
> > > > > > b) Other drivers also doing the same thing in their own driver
> > > > > > space as there is no dpdk API for the same.
> > > > > > This patches create generic API for pci_find_next_ext_capability()
> > > > > > and remove duplicate implementation from the drivers.
> > > > > > https://urldefense.proofpoint.com/v2/url?u=http-3A__patches.dpdk.o
> > > > > > rg
> > > > > >
> > > >
> > > _patch_73959_&d=DwIDaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=bMTgx2X48QVX
> > > > yXOEL8
> > > > > > ALyI4dsWoR-m74c5n3d-
> > > > ruJI8&m=eNuzGYhB7u2Wzru3VeBTY7QDZSSb9VQ9eQXW56D4
> > > > > > 64Y&s=eatY5xyw-474yS0cBJXyG7gLyPXFo243P2LmBDDsXd8&e=
> > > > >
> > > > > I agree it's good to have an API for such thing.
> > > > >
> > > > > So far such feature is supported in drivers on Linux, requiring only
> > > > > Linux headers to be installed.
> > > > > Do we need more?
> > > > >
> > > > >
> > > >
> > > > +1 to make it generic, no question here.
> > > >
> > > > On linux, the dependency is already there (either from linux headers
> > > > or
> > > > pciutils) to have the original. So including this header in DPDK is
> > > > only useful for other OSes.
> > > >
> > > > I think right now we should only add pci_find_next_ext_capability()
> > > > full implementation within linux part of PCI bus, other systems being
> > > stubs.
> > > >
> > > > We can go with your suggestion Jerin about adding only the specific
> > > > symbols needed, prefixed with RTE_, once we decide to have windows
> > > support.
> > > > Question is whether we need it right now. Is there a driver that would
> > > > make use of it support more than linux currently?
> > > >
> > > > --
> > >
> > > I don't know if there are any drivers which will require this other than linux
> > > as of today -
> > >
> > > My only motivation of adding these symbols in dpdk via rte_pci_regs.h (new
> > > file in lib/librte_pci/) was to avoid any dependency of dpdk on
> > > /usr/include/../pci_regs.h, since I was little unsure whether in all
> > > distributions (linux/windows) supported will have the required PCI defines
> > > available in /usr/include/../pci_regs.h file or not in order to implement
> > > rte_pci_find_next_ext_capability().  (unless user to bound for updating
> > > headers by mean of installing any latest _pciutils_/packages). Moreover, for
> > > not just this API, but if going forward if we have to add any new APIs which
> > > could rely/depend on PCI defines availability under /usr/include.
> > >
> > > From the discussion so far -
> > >
> > > 1. Define the function under drivers/bus/pci/linux/pci.c only and add
> > > empty/stub implementation for windows/pci.c and bsd/pci.c ?
> > > 2.  Just relying on /usr/include/ is perfectly okay without adding any defines
> > > anywhere for now ?, it will just require <linux/pci_regs.h> inclusion in
> > >      drivers/bus/pci/linux/pci.c. OR Shall I add (may be in
> > > lib/librte_pci/rte_pci.h ?) only required PCI defines with RTE_ prefixed and
> > > use them instead ?
> > >
> >
> > Hello Gaetan/Jerin,
> >
> > Could you please comment on above - what's sufficient to be incorporated in v3 for now ?
> > I will work on the changes accordingly.
> 
> I am for introducing a handful(Not the completed list, Only add what
> is needed for DPDK) of PCI constants as RTE_PCI_ in our public API.
> And implement rte_pci_find_next_ext_capability() as common routine as
> it is not specific to Linux.(I.e reading the config registers should
> be only Linux/Windows-specific)
> 
> I leave the final decision to PCI and other maintainers.

Hi Jerin, Manish,

There is currently no PCI bus maintainer, but most people in the project
are interested in it working properly :).

I think it's a good principle to try to limit as much as possible the
amount of system-specific code. rte_pci_read_config() seems like a fair
cut-off point.

Given than the current PMD implementations were doing exactly what you
describe, I think it's better to go for it currently, even if no one is
using those symbols outside linux. So +1 from me regarding what you
propose.

-- 
Gaëtan

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [dpdk-dev] [PATCH v2 2/7] drivers: add generic API to find PCI extended cap
  2020-07-13 15:13 ` [dpdk-dev] [PATCH v2 2/7] drivers: add generic API to find PCI extended cap Manish Chopra
@ 2020-07-20  9:36   ` Gaëtan Rivet
  0 siblings, 0 replies; 26+ messages in thread
From: Gaëtan Rivet @ 2020-07-20  9:36 UTC (permalink / raw)
  To: Manish Chopra; +Cc: jerinj, ferruh.yigit, dev

On 13/07/20 08:13 -0700, Manish Chopra wrote:
> By adding generic API, this patch removes individual
> functions/defines implemented by drivers to find PCI
> extended capability.
> 
> Signed-off-by: Manish Chopra <manishc@marvell.com>
> Signed-off-by: Igor Russkikh <irusskikh@marvell.com>
> ---
>  drivers/bus/pci/pci_common.c               | 41 +++++++++++++++++
>  drivers/bus/pci/rte_bus_pci.h              | 11 +++++
>  drivers/net/ice/ice_ethdev.c               | 53 ++--------------------
>  drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c | 48 ++------------------
>  lib/librte_pci/rte_pci_regs.h              |  2 +-
>  5 files changed, 60 insertions(+), 95 deletions(-)
> 
> diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
> index a8e5fd52c..5117c8e7b 100644
> --- a/drivers/bus/pci/pci_common.c
> +++ b/drivers/bus/pci/pci_common.c
> @@ -23,6 +23,7 @@
>  #include <rte_common.h>
>  #include <rte_devargs.h>
>  #include <rte_vfio.h>
> +#include <rte_pci_regs.h>
>  
>  #include "private.h"
>  
> @@ -665,6 +666,46 @@ rte_pci_get_iommu_class(void)
>  	return iova_mode;
>  }
>  
> +int rte_pci_find_next_ext_capability(struct rte_pci_device *dev, int cap)
> +{
> +	int pos = PCI_CFG_SPACE_SIZE;

pos is somewhat nondescript. position is long however.
offset seems more appropriate, short but describing properly the value.

Remember to prefix all those defines with 'RTE_'.

> +	uint32_t header;
> +	int ttl;
> +
> +	/* minimum 8 bytes per capability */
> +	ttl = (PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE) / 8;
> +
> +	if (rte_pci_read_config(dev, &header, 4, pos) < 0) {
> +		RTE_LOG(ERR, EAL, "error in reading extended capabilities\n");
> +		return -EINVAL;

The -EINVAL here is not useful. It is not used by callers anyway. It
can be simplified to -1.

> +	}
> +
> +	/*
> +	 * If we have no capabilities, this is indicated by cap ID,
> +	 * cap version and next pointer all being 0.
> +	 */
> +	if (header == 0)
> +		return 0;
> +
> +	while (ttl-- > 0) {

Not a fan of this, the checkpatch-compliant form of (ttl --> 0).
Decrementing the value within the loop conditional is generally frowned
upon and should be avoided.

    while (ttl != 0) {
            [...]
            ttl -= 1;
    }

> +		if (PCI_EXT_CAP_ID(header) == cap)

This define would originally resolve to uint32_t, which is why you had to cast it to
int in the macro itself.

This is not ok, the cap type should already be in sync (uint32_t).
Casting should not be needed. It should not be used just to shut up a
warning about signed to unsigned comparison.

> +			return pos;

Similarly for the return type, it is used as off_t afterward when calling
rte_pci_read_config(). All users of this API will be encouraged to use
an int where they should be using off_t instead (even if POSIX defines
it as signed int anyway).

It's really messy that the return type is an int that will be used
either as <0 on read error, 0 on missing cap, >0 for proper offset.
Separating the error channel from the payload won't make the API cleaner
though, so let's keep it that way. However, given that POSIX defines
off_t as a signed type anyway, let's use off_t directly, -1 for error, 0
for no error but no header, and >0 for a proper offset.

This means this function will not be a transparent drop-in for the
previous implem, but so be it.

> +
> +		pos = PCI_EXT_CAP_NEXT(header);
> +
> +		if (pos < PCI_CFG_SPACE_SIZE)
> +			break;
> +
> +		if (rte_pci_read_config(dev, &header, 4, pos) < 0) {
> +			RTE_LOG(ERR, EAL,
> +				"error in reading extended capabilities\n");
> +			return -EINVAL;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  struct rte_pci_bus rte_pci_bus = {
>  	.bus = {
>  		.scan = rte_pci_scan,
> diff --git a/drivers/bus/pci/rte_bus_pci.h b/drivers/bus/pci/rte_bus_pci.h
> index 29bea6d70..3cc66220a 100644
> --- a/drivers/bus/pci/rte_bus_pci.h
> +++ b/drivers/bus/pci/rte_bus_pci.h
> @@ -224,6 +224,17 @@ void rte_pci_unmap_device(struct rte_pci_device *dev);
>   */
>  void rte_pci_dump(FILE *f);
>  
> +/**
> + * Find device's extended capability
> + *
> + *  @param dev
> + *    A pointer to rte_pci_device structure

Missing a period here.

> + *
> + *  @param cap
> + *    Extended capability to find

ditto.

> + */

The doc is missing the return values and their signification. It
returns an offset where the ext. cap can be read in the PCI config?
Users should be able to understand this reading only this comment.

Also, 0 meaning the cap is not available is critical to know.

> +int rte_pci_find_next_ext_capability(struct rte_pci_device *dev, int cap);
> +
>  /**
>   * Register a PCI driver.
>   *
> diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
> index 3534d18ca..0724324d2 100644
> --- a/drivers/net/ice/ice_ethdev.c
> +++ b/drivers/net/ice/ice_ethdev.c
> @@ -4,6 +4,8 @@
>  
>  #include <rte_string_fns.h>
>  #include <rte_ethdev_pci.h>
> +#include <rte_bus_pci.h>
> +#include <rte_pci_regs.h>
>  
>  #include <stdio.h>
>  #include <sys/types.h>
> @@ -1730,53 +1732,6 @@ ice_pf_setup(struct ice_pf *pf)
>  	return 0;
>  }
>  
> -/* PCIe configuration space setting */
> -#define PCI_CFG_SPACE_SIZE          256
> -#define PCI_CFG_SPACE_EXP_SIZE      4096
> -#define PCI_EXT_CAP_ID(header)      (int)((header) & 0x0000ffff)
> -#define PCI_EXT_CAP_NEXT(header)    (((header) >> 20) & 0xffc)
> -#define PCI_EXT_CAP_ID_DSN          0x03
> -
> -static int
> -ice_pci_find_next_ext_capability(struct rte_pci_device *dev, int cap)
> -{
> -	uint32_t header;
> -	int ttl;
> -	int pos = PCI_CFG_SPACE_SIZE;
> -
> -	/* minimum 8 bytes per capability */
> -	ttl = (PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE) / 8;
> -
> -	if (rte_pci_read_config(dev, &header, 4, pos) < 0) {
> -		PMD_INIT_LOG(ERR, "ice error reading extended capabilities\n");
> -		return -1;
> -	}
> -
> -	/*
> -	 * If we have no capabilities, this is indicated by cap ID,
> -	 * cap version and next pointer all being 0.
> -	 */
> -	if (header == 0)
> -		return 0;
> -
> -	while (ttl-- > 0) {
> -		if (PCI_EXT_CAP_ID(header) == cap)
> -			return pos;
> -
> -		pos = PCI_EXT_CAP_NEXT(header);
> -
> -		if (pos < PCI_CFG_SPACE_SIZE)
> -			break;
> -
> -		if (rte_pci_read_config(dev, &header, 4, pos) < 0) {
> -			PMD_INIT_LOG(ERR, "ice error reading extended capabilities\n");
> -			return -1;
> -		}
> -	}
> -
> -	return 0;
> -}
> -
>  /*
>   * Extract device serial number from PCIe Configuration Space and
>   * determine the pkg file path according to the DSN.
> @@ -1789,9 +1744,9 @@ ice_pkg_file_search_path(struct rte_pci_device *pci_dev, char *pkg_file)
>  	uint32_t dsn_low, dsn_high;
>  	memset(opt_ddp_filename, 0, ICE_MAX_PKG_FILENAME_SIZE);
>  
> -	pos = ice_pci_find_next_ext_capability(pci_dev, PCI_EXT_CAP_ID_DSN);
> +	pos = rte_pci_find_next_ext_capability(pci_dev, PCI_EXT_CAP_ID_DSN);
>  
> -	if (pos) {
> +	if (pos > 0) {

You are fixing a bug in the ICE driver here.
On PCI config read error, -1 is returned, meaning the offset (-1 + 4) is used
to read 4 bytes, which does not seem ok.

This should be fixed in a separate commit to allow a backport.

>  		rte_pci_read_config(pci_dev, &dsn_low, 4, pos + 4);
>  		rte_pci_read_config(pci_dev, &dsn_high, 4, pos + 8);
>  		snprintf(opt_ddp_filename, ICE_MAX_PKG_FILENAME_SIZE,
> diff --git a/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c b/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
> index 0b9db974e..b11d8148a 100644
> --- a/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
> +++ b/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
> @@ -35,6 +35,8 @@
>  
>  #include <rte_ethdev_pci.h>
>  #include <rte_string_fns.h>
> +#include <rte_bus_pci.h>
> +#include <rte_pci_regs.h>
>  
>  #include "nfp_cpp.h"
>  #include "nfp_target.h"
> @@ -746,50 +748,6 @@ nfp6000_set_interface(struct rte_pci_device *dev, struct nfp_cpp *cpp)
>  	return 0;
>  }
>  
> -#define PCI_CFG_SPACE_SIZE	256
> -#define PCI_CFG_SPACE_EXP_SIZE	4096
> -#define PCI_EXT_CAP_ID(header)		(int)(header & 0x0000ffff)
> -#define PCI_EXT_CAP_NEXT(header)	((header >> 20) & 0xffc)
> -#define PCI_EXT_CAP_ID_DSN	0x03
> -static int
> -nfp_pci_find_next_ext_capability(struct rte_pci_device *dev, int cap)
> -{
> -	uint32_t header;
> -	int ttl;
> -	int pos = PCI_CFG_SPACE_SIZE;
> -
> -	/* minimum 8 bytes per capability */
> -	ttl = (PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE) / 8;
> -
> -	if (rte_pci_read_config(dev, &header, 4, pos) < 0) {
> -		printf("nfp error reading extended capabilities\n");
> -		return -1;
> -	}
> -
> -	/*
> -	 * If we have no capabilities, this is indicated by cap ID,
> -	 * cap version and next pointer all being 0.
> -	 */
> -	if (header == 0)
> -		return 0;
> -
> -	while (ttl-- > 0) {
> -		if (PCI_EXT_CAP_ID(header) == cap)
> -			return pos;
> -
> -		pos = PCI_EXT_CAP_NEXT(header);
> -		if (pos < PCI_CFG_SPACE_SIZE)
> -			break;
> -
> -		if (rte_pci_read_config(dev, &header, 4, pos) < 0) {
> -			printf("nfp error reading extended capabilities\n");
> -			return -1;
> -		}
> -	}
> -
> -	return 0;
> -}
> -
>  static int
>  nfp6000_set_serial(struct rte_pci_device *dev, struct nfp_cpp *cpp)
>  {
> @@ -798,7 +756,7 @@ nfp6000_set_serial(struct rte_pci_device *dev, struct nfp_cpp *cpp)
>  	int serial_len = 6;
>  	int pos;
>  
> -	pos = nfp_pci_find_next_ext_capability(dev, PCI_EXT_CAP_ID_DSN);
> +	pos = rte_pci_find_next_ext_capability(dev, PCI_EXT_CAP_ID_DSN);
>  	if (pos <= 0) {
>  		printf("PCI_EXT_CAP_ID_DSN not found. nfp set serial failed\n");
>  		return -1;
> diff --git a/lib/librte_pci/rte_pci_regs.h b/lib/librte_pci/rte_pci_regs.h
> index 1d11f4de5..108193049 100644
> --- a/lib/librte_pci/rte_pci_regs.h
> +++ b/lib/librte_pci/rte_pci_regs.h
> @@ -686,7 +686,7 @@
>  #define PCI_EXP_SLTSTA2		58	/* Slot Status 2 */
>  
>  /* Extended Capabilities (PCI-X 2.0 and Express) */
> -#define PCI_EXT_CAP_ID(header)		(header & 0x0000ffff)
> +#define PCI_EXT_CAP_ID(header)		(int)(header & 0x0000ffff)
>  #define PCI_EXT_CAP_VER(header)		((header >> 16) & 0xf)
>  #define PCI_EXT_CAP_NEXT(header)	((header >> 20) & 0xffc)
>  
> -- 
> 2.17.1
> 

-- 
Gaëtan

^ permalink raw reply	[flat|nested] 26+ messages in thread

end of thread, other threads:[~2020-07-20  9:36 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-13 15:13 [dpdk-dev] [PATCH v2 0/7] qede: SR-IOV PF driver support Manish Chopra
2020-07-13 15:13 ` [dpdk-dev] [PATCH v2 1/7] lib/librte_pci: add rte_pci_regs.h Manish Chopra
2020-07-16  9:43   ` Gaëtan Rivet
2020-07-16 10:08   ` Gaëtan Rivet
2020-07-16 10:17     ` Gaëtan Rivet
2020-07-16 10:27       ` Jerin Jacob
2020-07-16 11:26         ` Thomas Monjalon
2020-07-16 11:55           ` Jerin Jacob
2020-07-16 12:49             ` Thomas Monjalon
2020-07-16 13:02               ` Jerin Jacob
2020-07-16 15:55                 ` Thomas Monjalon
2020-07-16 16:43                   ` Jerin Jacob
2020-07-16 16:57                     ` Thomas Monjalon
2020-07-16 17:33                       ` Jerin Jacob
2020-07-16 17:56                       ` Gaëtan Rivet
2020-07-16 20:49                         ` [dpdk-dev] [EXT] " Manish Chopra
2020-07-18 19:42                           ` Manish Chopra
2020-07-19  8:47                             ` Jerin Jacob
2020-07-20  8:57                               ` Gaëtan Rivet
2020-07-13 15:13 ` [dpdk-dev] [PATCH v2 2/7] drivers: add generic API to find PCI extended cap Manish Chopra
2020-07-20  9:36   ` Gaëtan Rivet
2020-07-13 15:13 ` [dpdk-dev] [PATCH v2 3/7] net/qede: define PCI config space specific osals Manish Chopra
2020-07-13 15:13 ` [dpdk-dev] [PATCH v2 4/7] net/qede: configure VFs on hardware Manish Chopra
2020-07-13 15:13 ` [dpdk-dev] [PATCH v2 5/7] net/qede: add infrastructure support for VF load Manish Chopra
2020-07-13 15:13 ` [dpdk-dev] [PATCH v2 6/7] net/qede: initialize VF MAC and link Manish Chopra
2020-07-13 15:13 ` [dpdk-dev] [PATCH v2 7/7] net/qede: add VF FLR support Manish Chopra

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).