[PATCH v3 1/4] vdpa/octeon_ep: enable support for multiple interrupts per device

[PATCH v3 1/4] vdpa/octeon_ep: enable support for multiple interrupts per device

[Shijith Thotton]

Updated the driver to utilize all the MSI-X interrupt vectors supported
by each OCTEON endpoint VF, instead of relying on a single vector.
Enabling more interrupts allows packets from multiple rings to be
distributed across multiple cores, improving parallelism and
performance.

Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>
Signed-off-by: Shijith Thotton <sthotton@marvell.com>
---
v1:
- https://lore.kernel.org/virtualization/20241120070508.789508-1-sthotton@marvell.com

Changes in v2:
- Handle reset getting called twice.
- Use devm_kcalloc to allocate irq array.
- IRQ is never zero. Adjusted code accordingly.

Changes in v3:
- Made the interrupt handling independent of device type.
- Dropped the patch to add crypto device specific interrupt handler.
- Moved spec specific macro and define to spec header file.

 drivers/vdpa/octeon_ep/octep_vdpa.h      | 12 ++--
 drivers/vdpa/octeon_ep/octep_vdpa_hw.c   |  2 -
 drivers/vdpa/octeon_ep/octep_vdpa_main.c | 87 +++++++++++++++---------
 3 files changed, 62 insertions(+), 39 deletions(-)

diff --git a/drivers/vdpa/octeon_ep/octep_vdpa.h b/drivers/vdpa/octeon_ep/octep_vdpa.h
index 046710ec4d42..2cadb878e679 100644
--- a/drivers/vdpa/octeon_ep/octep_vdpa.h
+++ b/drivers/vdpa/octeon_ep/octep_vdpa.h
@@ -29,12 +29,12 @@
 #define OCTEP_EPF_RINFO(x) (0x000209f0 | ((x) << 25))
 #define OCTEP_VF_MBOX_DATA(x) (0x00010210 | ((x) << 17))
 #define OCTEP_PF_MBOX_DATA(x) (0x00022000 | ((x) << 4))
-
-#define OCTEP_EPF_RINFO_RPVF(val) (((val) >> 32) & 0xF)
-#define OCTEP_EPF_RINFO_NVFS(val) (((val) >> 48) & 0x7F)
+#define OCTEP_VF_IN_CTRL(x)        (0x00010000 | ((x) << 17))
+#define OCTEP_VF_IN_CTRL_RPVF(val) (((val) >> 48) & 0xF)
 
 #define OCTEP_FW_READY_SIGNATURE0  0xFEEDFEED
 #define OCTEP_FW_READY_SIGNATURE1  0x3355ffaa
+#define OCTEP_MAX_CB_INTR          8
 
 enum octep_vdpa_dev_status {
 	OCTEP_VDPA_DEV_STATUS_INVALID,
@@ -48,9 +48,8 @@ enum octep_vdpa_dev_status {
 struct octep_vring_info {
 	struct vdpa_callback cb;
 	void __iomem *notify_addr;
-	u32 __iomem *cb_notify_addr;
+	void __iomem *cb_notify_addr;
 	phys_addr_t notify_pa;
-	char msix_name[256];
 };
 
 struct octep_hw {
@@ -68,7 +67,8 @@ struct octep_hw {
 	u64 features;
 	u16 nr_vring;
 	u32 config_size;
-	int irq;
+	int nb_irqs;
+	int *irqs;
 };
 
 u8 octep_hw_get_status(struct octep_hw *oct_hw);
diff --git a/drivers/vdpa/octeon_ep/octep_vdpa_hw.c b/drivers/vdpa/octeon_ep/octep_vdpa_hw.c
index 1d4767b33315..d5a599f87e18 100644
--- a/drivers/vdpa/octeon_ep/octep_vdpa_hw.c
+++ b/drivers/vdpa/octeon_ep/octep_vdpa_hw.c
@@ -495,8 +495,6 @@ int octep_hw_caps_read(struct octep_hw *oct_hw, struct pci_dev *pdev)
 	if (!oct_hw->vqs)
 		return -ENOMEM;
 
-	oct_hw->irq = -1;
-
 	dev_info(&pdev->dev, "Device features : %llx\n", oct_hw->features);
 	dev_info(&pdev->dev, "Maximum queues : %u\n", oct_hw->nr_vring);
 
diff --git a/drivers/vdpa/octeon_ep/octep_vdpa_main.c b/drivers/vdpa/octeon_ep/octep_vdpa_main.c
index cd55b1aac151..e9c3e57b321f 100644
--- a/drivers/vdpa/octeon_ep/octep_vdpa_main.c
+++ b/drivers/vdpa/octeon_ep/octep_vdpa_main.c
@@ -49,11 +49,25 @@ static irqreturn_t octep_vdpa_intr_handler(int irq, void *data)
 	struct octep_hw *oct_hw = data;
 	int i;
 
-	for (i = 0; i < oct_hw->nr_vring; i++) {
-		if (oct_hw->vqs[i].cb.callback && ioread32(oct_hw->vqs[i].cb_notify_addr)) {
-			/* Acknowledge the per queue notification to the device */
-			iowrite32(0, oct_hw->vqs[i].cb_notify_addr);
-			oct_hw->vqs[i].cb.callback(oct_hw->vqs[i].cb.private);
+	/* Each device has multiple interrupts (nb_irqs) shared among rings
+	 * (nr_vring). Device interrupts are mapped to the rings in a
+	 * round-robin fashion.
+	 *
+	 * For example, if nb_irqs = 8 and nr_vring = 64:
+	 * 0 -> 0, 8, 16, 24, 32, 40, 48, 56;
+	 * 1 -> 1, 9, 17, 25, 33, 41, 49, 57;
+	 * ...
+	 * 7 -> 7, 15, 23, 31, 39, 47, 55, 63;
+	 */
+
+	for (i = irq - oct_hw->irqs[0]; i < oct_hw->nr_vring; i += oct_hw->nb_irqs) {
+		if (ioread8(oct_hw->vqs[i].cb_notify_addr)) {
+			/* Acknowledge the per ring notification to the device */
+			iowrite8(0, oct_hw->vqs[i].cb_notify_addr);
+
+			if (likely(oct_hw->vqs[i].cb.callback))
+				oct_hw->vqs[i].cb.callback(oct_hw->vqs[i].cb.private);
+			break;
 		}
 	}
 
@@ -63,44 +77,53 @@ static irqreturn_t octep_vdpa_intr_handler(int irq, void *data)
 static void octep_free_irqs(struct octep_hw *oct_hw)
 {
 	struct pci_dev *pdev = oct_hw->pdev;
+	int irq;
+
+	if (!oct_hw->irqs)
+		return;
 
-	if (oct_hw->irq != -1) {
-		devm_free_irq(&pdev->dev, oct_hw->irq, oct_hw);
-		oct_hw->irq = -1;
+	for (irq = 0; irq < oct_hw->nb_irqs; irq++) {
+		if (!oct_hw->irqs[irq])
+			break;
+
+		devm_free_irq(&pdev->dev, oct_hw->irqs[irq], oct_hw);
 	}
+
 	pci_free_irq_vectors(pdev);
+	devm_kfree(&pdev->dev, oct_hw->irqs);
+	oct_hw->irqs = NULL;
 }
 
 static int octep_request_irqs(struct octep_hw *oct_hw)
 {
 	struct pci_dev *pdev = oct_hw->pdev;
-	int ret, irq;
+	int ret, irq, idx;
 
-	/* Currently HW device provisions one IRQ per VF, hence
-	 * allocate one IRQ for all virtqueues call interface.
-	 */
-	ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSIX);
+	oct_hw->irqs = devm_kcalloc(&pdev->dev, oct_hw->nb_irqs, sizeof(int), GFP_KERNEL);
+	if (!oct_hw->irqs)
+		return -ENOMEM;
+
+	ret = pci_alloc_irq_vectors(pdev, 1, oct_hw->nb_irqs, PCI_IRQ_MSIX);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "Failed to alloc msix vector");
 		return ret;
 	}
 
-	snprintf(oct_hw->vqs->msix_name, sizeof(oct_hw->vqs->msix_name),
-		 OCTEP_VDPA_DRIVER_NAME "-vf-%d", pci_iov_vf_id(pdev));
-
-	irq = pci_irq_vector(pdev, 0);
-	ret = devm_request_irq(&pdev->dev, irq, octep_vdpa_intr_handler, 0,
-			       oct_hw->vqs->msix_name, oct_hw);
-	if (ret) {
-		dev_err(&pdev->dev, "Failed to register interrupt handler\n");
-		goto free_irq_vec;
+	for (idx = 0; idx < oct_hw->nb_irqs; idx++) {
+		irq = pci_irq_vector(pdev, idx);
+		ret = devm_request_irq(&pdev->dev, irq, octep_vdpa_intr_handler, 0,
+				       dev_name(&pdev->dev), oct_hw);
+		if (ret) {
+			dev_err(&pdev->dev, "Failed to register interrupt handler\n");
+			goto free_irqs;
+		}
+		oct_hw->irqs[idx] = irq;
 	}
-	oct_hw->irq = irq;
 
 	return 0;
 
-free_irq_vec:
-	pci_free_irq_vectors(pdev);
+free_irqs:
+	octep_free_irqs(oct_hw);
 	return ret;
 }
 
@@ -559,6 +582,7 @@ static void octep_vdpa_setup_task(struct work_struct *work)
 	struct device *dev = &pdev->dev;
 	struct octep_hw *oct_hw;
 	unsigned long timeout;
+	u64 val;
 	int ret;
 
 	oct_hw = &mgmt_dev->oct_hw;
@@ -590,6 +614,13 @@ static void octep_vdpa_setup_task(struct work_struct *work)
 	if (ret)
 		return;
 
+	val = readq(oct_hw->base[OCTEP_HW_MBOX_BAR] + OCTEP_VF_IN_CTRL(0));
+	oct_hw->nb_irqs = OCTEP_VF_IN_CTRL_RPVF(val);
+	if (!oct_hw->nb_irqs || oct_hw->nb_irqs > OCTEP_MAX_CB_INTR) {
+		dev_err(dev, "Invalid number of interrupts %d\n", oct_hw->nb_irqs);
+		goto unmap_region;
+	}
+
 	ret = octep_hw_caps_read(oct_hw, pdev);
 	if (ret < 0)
 		goto unmap_region;
@@ -768,12 +799,6 @@ static int octep_vdpa_pf_setup(struct octep_pf *octpf)
 		return -EINVAL;
 	}
 
-	if (OCTEP_EPF_RINFO_RPVF(val) != BIT_ULL(0)) {
-		val &= ~GENMASK_ULL(35, 32);
-		val |= BIT_ULL(32);
-		writeq(val, addr + OCTEP_EPF_RINFO(0));
-	}
-
 	len = pci_resource_len(pdev, OCTEP_HW_CAPS_BAR);
 
 	octpf->vf_stride = len / totalvfs;
-- 
2.25.1

[PATCH v3 2/4] vdpa/octeon_ep: handle device config change events

[Shijith Thotton]

From: Satha Rao <skoteshwar@marvell.com>

The first interrupt of the device is used to notify the host about
device configuration changes, such as link status updates. The ISR
configuration area is updated to indicate a config change event when
triggered.

Signed-off-by: Satha Rao <skoteshwar@marvell.com>
Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>
Acked-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Shijith Thotton <sthotton@marvell.com>
---
 drivers/vdpa/octeon_ep/octep_vdpa_main.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/vdpa/octeon_ep/octep_vdpa_main.c b/drivers/vdpa/octeon_ep/octep_vdpa_main.c
index e9c3e57b321f..4d56be64ae56 100644
--- a/drivers/vdpa/octeon_ep/octep_vdpa_main.c
+++ b/drivers/vdpa/octeon_ep/octep_vdpa_main.c
@@ -71,6 +71,14 @@ static irqreturn_t octep_vdpa_intr_handler(int irq, void *data)
 		}
 	}
 
+	/* Check for config interrupt. Config uses the first interrupt */
+	if (unlikely(irq == oct_hw->irqs[0] && ioread8(oct_hw->isr))) {
+		iowrite8(0, oct_hw->isr);
+
+		if (oct_hw->config_cb.callback)
+			oct_hw->config_cb.callback(oct_hw->config_cb.private);
+	}
+
 	return IRQ_HANDLED;
 }
 
-- 
2.25.1

[PATCH v3 3/4] virtio-pci: define type and header for PCI vendor data

[Shijith Thotton]

Added macro definition for VIRTIO_PCI_CAP_VENDOR_CFG to identify the PCI
vendor data type in the virtio_pci_cap structure. Defined a new struct
virtio_pci_vndr_data for the vendor data capability header as per the
specification.

Signed-off-by: Shijith Thotton <sthotton@marvell.com>
---
 include/uapi/linux/virtio_pci.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/uapi/linux/virtio_pci.h b/include/uapi/linux/virtio_pci.h
index a8208492e822..1f3ea5d2a6af 100644
--- a/include/uapi/linux/virtio_pci.h
+++ b/include/uapi/linux/virtio_pci.h
@@ -115,6 +115,8 @@
 #define VIRTIO_PCI_CAP_PCI_CFG		5
 /* Additional shared memory capability */
 #define VIRTIO_PCI_CAP_SHARED_MEMORY_CFG 8
+/* PCI vendor data configuration */
+#define VIRTIO_PCI_CAP_VENDOR_CFG	9
 
 /* This is the PCI capability header: */
 struct virtio_pci_cap {
@@ -129,6 +131,15 @@ struct virtio_pci_cap {
 	__le32 length;		/* Length of the structure, in bytes. */
 };
 
+/* This is the PCI vendor data capability header: */
+struct virtio_pci_vndr_data {
+	__u8 cap_vndr;		/* Generic PCI field: PCI_CAP_ID_VNDR */
+	__u8 cap_next;		/* Generic PCI field: next ptr. */
+	__u8 cap_len;		/* Generic PCI field: capability length */
+	__u8 cfg_type;		/* Identifies the structure. */
+	__u16 vendor_id;	/* Identifies the vendor-specific format. */
+};
+
 struct virtio_pci_cap64 {
 	struct virtio_pci_cap cap;
 	__le32 offset_hi;             /* Most sig 32 bits of offset */
-- 
2.25.1

Re: [PATCH v3 3/4] virtio-pci: define type and header for PCI vendor data

[Jason Wang]

On Mon, Dec 9, 2024 at 11:16 PM Shijith Thotton <sthotton@marvell.com> wrote:
>
> Added macro definition for VIRTIO_PCI_CAP_VENDOR_CFG to identify the PCI
> vendor data type in the virtio_pci_cap structure. Defined a new struct
> virtio_pci_vndr_data for the vendor data capability header as per the
> specification.
>
> Signed-off-by: Shijith Thotton <sthotton@marvell.com>
> ---
>  include/uapi/linux/virtio_pci.h | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/include/uapi/linux/virtio_pci.h b/include/uapi/linux/virtio_pci.h
> index a8208492e822..1f3ea5d2a6af 100644
> --- a/include/uapi/linux/virtio_pci.h
> +++ b/include/uapi/linux/virtio_pci.h
> @@ -115,6 +115,8 @@
>  #define VIRTIO_PCI_CAP_PCI_CFG         5
>  /* Additional shared memory capability */
>  #define VIRTIO_PCI_CAP_SHARED_MEMORY_CFG 8
> +/* PCI vendor data configuration */
> +#define VIRTIO_PCI_CAP_VENDOR_CFG      9
>
>  /* This is the PCI capability header: */
>  struct virtio_pci_cap {
> @@ -129,6 +131,15 @@ struct virtio_pci_cap {
>         __le32 length;          /* Length of the structure, in bytes. */
>  };
>
> +/* This is the PCI vendor data capability header: */
> +struct virtio_pci_vndr_data {
> +       __u8 cap_vndr;          /* Generic PCI field: PCI_CAP_ID_VNDR */
> +       __u8 cap_next;          /* Generic PCI field: next ptr. */
> +       __u8 cap_len;           /* Generic PCI field: capability length */
> +       __u8 cfg_type;          /* Identifies the structure. */
> +       __u16 vendor_id;        /* Identifies the vendor-specific format. */
> +};

Nit: I would have the following comments from the virtio spec:

"""
 /* For Vendor Definition */
  /* Pads structure to a multiple of 4 bytes */
  /* Reads must not have side effects */
"""

Thanks

> +
>  struct virtio_pci_cap64 {
>         struct virtio_pci_cap cap;
>         __le32 offset_hi;             /* Most sig 32 bits of offset */
> --
> 2.25.1
>

Re: [PATCH v3 3/4] virtio-pci: define type and header for PCI vendor data

[Shijith Thotton]

>> Added macro definition for VIRTIO_PCI_CAP_VENDOR_CFG to identify the
>PCI
>> vendor data type in the virtio_pci_cap structure. Defined a new struct
>> virtio_pci_vndr_data for the vendor data capability header as per the
>> specification.
>>
>> Signed-off-by: Shijith Thotton <sthotton@marvell.com>
>> ---
>>  include/uapi/linux/virtio_pci.h | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/include/uapi/linux/virtio_pci.h b/include/uapi/linux/virtio_pci.h
>> index a8208492e822..1f3ea5d2a6af 100644
>> --- a/include/uapi/linux/virtio_pci.h
>> +++ b/include/uapi/linux/virtio_pci.h
>> @@ -115,6 +115,8 @@
>>  #define VIRTIO_PCI_CAP_PCI_CFG         5
>>  /* Additional shared memory capability */
>>  #define VIRTIO_PCI_CAP_SHARED_MEMORY_CFG 8
>> +/* PCI vendor data configuration */
>> +#define VIRTIO_PCI_CAP_VENDOR_CFG      9
>>
>>  /* This is the PCI capability header: */
>>  struct virtio_pci_cap {
>> @@ -129,6 +131,15 @@ struct virtio_pci_cap {
>>         __le32 length;          /* Length of the structure, in bytes. */
>>  };
>>
>> +/* This is the PCI vendor data capability header: */
>> +struct virtio_pci_vndr_data {
>> +       __u8 cap_vndr;          /* Generic PCI field: PCI_CAP_ID_VNDR */
>> +       __u8 cap_next;          /* Generic PCI field: next ptr. */
>> +       __u8 cap_len;           /* Generic PCI field: capability length */
>> +       __u8 cfg_type;          /* Identifies the structure. */
>> +       __u16 vendor_id;        /* Identifies the vendor-specific format. */
>> +};
>
>Nit: I would have the following comments from the virtio spec:
>
>"""
> /* For Vendor Definition */
>  /* Pads structure to a multiple of 4 bytes */
>  /* Reads must not have side effects */
>"""

I will add the comments.

Thanks,
Shijith

[PATCH v3 4/4] vdpa/octeon_ep: read vendor-specific PCI capability

[Shijith Thotton]

Added support to read the vendor-specific PCI capability to identify the
type of device being emulated.

Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>
Signed-off-by: Shijith Thotton <sthotton@marvell.com>
---
 drivers/vdpa/octeon_ep/octep_vdpa.h      | 20 ++++++++++++++
 drivers/vdpa/octeon_ep/octep_vdpa_hw.c   | 33 +++++++++++++++++++++++-
 drivers/vdpa/octeon_ep/octep_vdpa_main.c |  4 ++-
 3 files changed, 55 insertions(+), 2 deletions(-)

diff --git a/drivers/vdpa/octeon_ep/octep_vdpa.h b/drivers/vdpa/octeon_ep/octep_vdpa.h
index 2cadb878e679..53b020b019f7 100644
--- a/drivers/vdpa/octeon_ep/octep_vdpa.h
+++ b/drivers/vdpa/octeon_ep/octep_vdpa.h
@@ -8,6 +8,7 @@
 #include <linux/pci_regs.h>
 #include <linux/vdpa.h>
 #include <linux/virtio_pci_modern.h>
+#include <uapi/linux/virtio_crypto.h>
 #include <uapi/linux/virtio_net.h>
 #include <uapi/linux/virtio_blk.h>
 #include <uapi/linux/virtio_config.h>
@@ -52,6 +53,24 @@ struct octep_vring_info {
 	phys_addr_t notify_pa;
 };
 
+enum octep_pci_vndr_cfg_type {
+	OCTEP_PCI_VNDR_CFG_TYPE_VIRTIO_ID,
+	OCTEP_PCI_VNDR_CFG_TYPE_MAX,
+};
+
+struct octep_pci_vndr_data {
+	struct virtio_pci_vndr_data hdr;
+	u8 id;
+	u8 bar;
+	union {
+		u64 data;
+		struct {
+			u32 offset;
+			u32 length;
+		};
+	};
+};
+
 struct octep_hw {
 	struct pci_dev *pdev;
 	u8 __iomem *base[PCI_STD_NUM_BARS];
@@ -69,6 +88,7 @@ struct octep_hw {
 	u32 config_size;
 	int nb_irqs;
 	int *irqs;
+	u8 dev_id;
 };
 
 u8 octep_hw_get_status(struct octep_hw *oct_hw);
diff --git a/drivers/vdpa/octeon_ep/octep_vdpa_hw.c b/drivers/vdpa/octeon_ep/octep_vdpa_hw.c
index d5a599f87e18..b932f5da511e 100644
--- a/drivers/vdpa/octeon_ep/octep_vdpa_hw.c
+++ b/drivers/vdpa/octeon_ep/octep_vdpa_hw.c
@@ -358,7 +358,14 @@ u16 octep_get_vq_size(struct octep_hw *oct_hw)
 
 static u32 octep_get_config_size(struct octep_hw *oct_hw)
 {
-	return sizeof(struct virtio_net_config);
+	switch (oct_hw->dev_id) {
+	case VIRTIO_ID_NET:
+		return sizeof(struct virtio_net_config);
+	case VIRTIO_ID_CRYPTO:
+		return sizeof(struct virtio_crypto_config);
+	default:
+		return 0;
+	}
 }
 
 static void __iomem *octep_get_cap_addr(struct octep_hw *oct_hw, struct virtio_pci_cap *cap)
@@ -416,8 +423,23 @@ static int octep_pci_signature_verify(struct octep_hw *oct_hw)
 	return 0;
 }
 
+static void octep_vndr_data_process(struct octep_hw *oct_hw,
+				    struct octep_pci_vndr_data *vndr_data)
+{
+	switch (vndr_data->id) {
+	case OCTEP_PCI_VNDR_CFG_TYPE_VIRTIO_ID:
+		oct_hw->dev_id = (u8)vndr_data->data;
+		break;
+	default:
+		dev_err(&oct_hw->pdev->dev, "Invalid vendor data id %u\n",
+			vndr_data->id);
+		break;
+	}
+}
+
 int octep_hw_caps_read(struct octep_hw *oct_hw, struct pci_dev *pdev)
 {
+	struct octep_pci_vndr_data vndr_data;
 	struct octep_mbox __iomem *mbox;
 	struct device *dev = &pdev->dev;
 	struct virtio_pci_cap cap;
@@ -466,6 +488,15 @@ int octep_hw_caps_read(struct octep_hw *oct_hw, struct pci_dev *pdev)
 		case VIRTIO_PCI_CAP_ISR_CFG:
 			oct_hw->isr = octep_get_cap_addr(oct_hw, &cap);
 			break;
+		case VIRTIO_PCI_CAP_VENDOR_CFG:
+			octep_pci_caps_read(oct_hw, &vndr_data, sizeof(vndr_data), pos);
+			if (vndr_data.hdr.vendor_id != PCI_VENDOR_ID_CAVIUM) {
+				dev_err(dev, "Invalid vendor data\n");
+				return -EINVAL;
+			}
+
+			octep_vndr_data_process(oct_hw, &vndr_data);
+			break;
 		}
 
 		pos = cap.cap_next;
diff --git a/drivers/vdpa/octeon_ep/octep_vdpa_main.c b/drivers/vdpa/octeon_ep/octep_vdpa_main.c
index 4d56be64ae56..f3d4dda4e04c 100644
--- a/drivers/vdpa/octeon_ep/octep_vdpa_main.c
+++ b/drivers/vdpa/octeon_ep/octep_vdpa_main.c
@@ -302,7 +302,9 @@ static u32 octep_vdpa_get_generation(struct vdpa_device *vdpa_dev)
 
 static u32 octep_vdpa_get_device_id(struct vdpa_device *vdpa_dev)
 {
-	return VIRTIO_ID_NET;
+	struct octep_hw *oct_hw = vdpa_to_octep_hw(vdpa_dev);
+
+	return oct_hw->dev_id;
 }
 
 static u32 octep_vdpa_get_vendor_id(struct vdpa_device *vdpa_dev)
-- 
2.25.1

Re: [PATCH v3 4/4] vdpa/octeon_ep: read vendor-specific PCI capability

[Jason Wang]

On Mon, Dec 9, 2024 at 11:16 PM Shijith Thotton <sthotton@marvell.com> wrote:
>
> Added support to read the vendor-specific PCI capability to identify the
> type of device being emulated.
>
> Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>
> Signed-off-by: Shijith Thotton <sthotton@marvell.com>
> ---
>  drivers/vdpa/octeon_ep/octep_vdpa.h      | 20 ++++++++++++++
>  drivers/vdpa/octeon_ep/octep_vdpa_hw.c   | 33 +++++++++++++++++++++++-
>  drivers/vdpa/octeon_ep/octep_vdpa_main.c |  4 ++-
>  3 files changed, 55 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vdpa/octeon_ep/octep_vdpa.h b/drivers/vdpa/octeon_ep/octep_vdpa.h
> index 2cadb878e679..53b020b019f7 100644
> --- a/drivers/vdpa/octeon_ep/octep_vdpa.h
> +++ b/drivers/vdpa/octeon_ep/octep_vdpa.h
> @@ -8,6 +8,7 @@
>  #include <linux/pci_regs.h>
>  #include <linux/vdpa.h>
>  #include <linux/virtio_pci_modern.h>
> +#include <uapi/linux/virtio_crypto.h>
>  #include <uapi/linux/virtio_net.h>
>  #include <uapi/linux/virtio_blk.h>
>  #include <uapi/linux/virtio_config.h>
> @@ -52,6 +53,24 @@ struct octep_vring_info {
>         phys_addr_t notify_pa;
>  };
>
> +enum octep_pci_vndr_cfg_type {
> +       OCTEP_PCI_VNDR_CFG_TYPE_VIRTIO_ID,
> +       OCTEP_PCI_VNDR_CFG_TYPE_MAX,
> +};
> +
> +struct octep_pci_vndr_data {
> +       struct virtio_pci_vndr_data hdr;
> +       u8 id;
> +       u8 bar;
> +       union {
> +               u64 data;
> +               struct {
> +                       u32 offset;
> +                       u32 length;
> +               };
> +       };
> +};

This seems not to be padded to a multiple of 4 bytes?

Others look good.

Thanks

Re: [PATCH v3 4/4] vdpa/octeon_ep: read vendor-specific PCI capability

[Shijith Thotton]

>>
>> Added support to read the vendor-specific PCI capability to identify the
>> type of device being emulated.
>>
>> Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>
>> Signed-off-by: Shijith Thotton <sthotton@marvell.com>
>> ---
>>  drivers/vdpa/octeon_ep/octep_vdpa.h      | 20 ++++++++++++++
>>  drivers/vdpa/octeon_ep/octep_vdpa_hw.c   | 33
>+++++++++++++++++++++++-
>>  drivers/vdpa/octeon_ep/octep_vdpa_main.c |  4 ++-
>>  3 files changed, 55 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/vdpa/octeon_ep/octep_vdpa.h
>b/drivers/vdpa/octeon_ep/octep_vdpa.h
>> index 2cadb878e679..53b020b019f7 100644
>> --- a/drivers/vdpa/octeon_ep/octep_vdpa.h
>> +++ b/drivers/vdpa/octeon_ep/octep_vdpa.h
>> @@ -8,6 +8,7 @@
>>  #include <linux/pci_regs.h>
>>  #include <linux/vdpa.h>
>>  #include <linux/virtio_pci_modern.h>
>> +#include <uapi/linux/virtio_crypto.h>
>>  #include <uapi/linux/virtio_net.h>
>>  #include <uapi/linux/virtio_blk.h>
>>  #include <uapi/linux/virtio_config.h>
>> @@ -52,6 +53,24 @@ struct octep_vring_info {
>>         phys_addr_t notify_pa;
>>  };
>>
>> +enum octep_pci_vndr_cfg_type {
>> +       OCTEP_PCI_VNDR_CFG_TYPE_VIRTIO_ID,
>> +       OCTEP_PCI_VNDR_CFG_TYPE_MAX,
>> +};
>> +
>> +struct octep_pci_vndr_data {
>> +       struct virtio_pci_vndr_data hdr;
>> +       u8 id;
>> +       u8 bar;
>> +       union {
>> +               u64 data;
>> +               struct {
>> +                       u32 offset;
>> +                       u32 length;
>> +               };
>> +       };
>> +};
>
>This seems not to be padded to a multiple of 4 bytes?
>

Looks like, the structure is naturally aligned.

struct virtio_pci_vndr_data {
        uint8_t                    cap_vndr;             /*     0     1 */
        uint8_t                    cap_next;             /*     1     1 */
        uint8_t                    cap_len;              /*     2     1 */
        uint8_t                    cfg_type;             /*     3     1 */
        uint16_t                   vendor_id;            /*     4     2 */
        /* size: 6, cachelines: 1, members: 5 */                                                                                                                                                                                                                      
        /* last cacheline: 6 bytes */
};
struct octep_pci_vndr_data {                                                                                                                                                                                                                                          
        struct virtio_pci_vndr_data hdr;                 /*     0     6 */
        uint8_t                    id;                   /*     6     1 */                                                                                                                                                                                            
        uint8_t                    bar;                  /*     7     1 */                                                                                                                                                                                            
        union {
                uint64_t           data;                 /*     8     8 */
                struct {
                        uint32_t   offset;               /*     8     4 */
                        uint32_t   length;               /*    12     4 */
                };                                       /*     8     8 */
        };                                               /*     8     8 */

        /* size: 16, cachelines: 1, members: 4 */
        /* last cacheline: 16 bytes */
};

I will add __attribute__((aligned(4))) to be safe.

Thanks,
Shijith

RE: [PATCH v3 4/4] vdpa/octeon_ep: read vendor-specific PCI capability

[Shijith Thotton]

>>>
>>> Added support to read the vendor-specific PCI capability to identify the
>>> type of device being emulated.
>>>
>>> Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>
>>> Signed-off-by: Shijith Thotton <sthotton@marvell.com>
>>> ---
>>>  drivers/vdpa/octeon_ep/octep_vdpa.h      | 20 ++++++++++++++
>>>  drivers/vdpa/octeon_ep/octep_vdpa_hw.c   | 33
>>+++++++++++++++++++++++-
>>>  drivers/vdpa/octeon_ep/octep_vdpa_main.c |  4 ++-
>>>  3 files changed, 55 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/vdpa/octeon_ep/octep_vdpa.h
>>b/drivers/vdpa/octeon_ep/octep_vdpa.h
>>> index 2cadb878e679..53b020b019f7 100644
>>> --- a/drivers/vdpa/octeon_ep/octep_vdpa.h
>>> +++ b/drivers/vdpa/octeon_ep/octep_vdpa.h
>>> @@ -8,6 +8,7 @@
>>>  #include <linux/pci_regs.h>
>>>  #include <linux/vdpa.h>
>>>  #include <linux/virtio_pci_modern.h>
>>> +#include <uapi/linux/virtio_crypto.h>
>>>  #include <uapi/linux/virtio_net.h>
>>>  #include <uapi/linux/virtio_blk.h>
>>>  #include <uapi/linux/virtio_config.h>
>>> @@ -52,6 +53,24 @@ struct octep_vring_info {
>>>         phys_addr_t notify_pa;
>>>  };
>>>
>>> +enum octep_pci_vndr_cfg_type {
>>> +       OCTEP_PCI_VNDR_CFG_TYPE_VIRTIO_ID,
>>> +       OCTEP_PCI_VNDR_CFG_TYPE_MAX,
>>> +};
>>> +
>>> +struct octep_pci_vndr_data {
>>> +       struct virtio_pci_vndr_data hdr;
>>> +       u8 id;
>>> +       u8 bar;
>>> +       union {
>>> +               u64 data;
>>> +               struct {
>>> +                       u32 offset;
>>> +                       u32 length;
>>> +               };
>>> +       };
>>> +};
>>
>>This seems not to be padded to a multiple of 4 bytes?
>>
>
>Looks like, the structure is naturally aligned.
>
>struct virtio_pci_vndr_data {
>        uint8_t                    cap_vndr;             /*     0     1 */
>        uint8_t                    cap_next;             /*     1     1 */
>        uint8_t                    cap_len;              /*     2     1 */
>        uint8_t                    cfg_type;             /*     3     1 */
>        uint16_t                   vendor_id;            /*     4     2 */
>        /* size: 6, cachelines: 1, members: 5 */
>        /* last cacheline: 6 bytes */
>};
>struct octep_pci_vndr_data {
>        struct virtio_pci_vndr_data hdr;                 /*     0     6 */
>        uint8_t                    id;                   /*     6     1 */
>        uint8_t                    bar;                  /*     7     1 */
>        union {
>                uint64_t           data;                 /*     8     8 */
>                struct {
>                        uint32_t   offset;               /*     8     4 */
>                        uint32_t   length;               /*    12     4 */
>                };                                       /*     8     8 */
>        };                                               /*     8     8 */
>
>        /* size: 16, cachelines: 1, members: 4 */
>        /* last cacheline: 16 bytes */
>};
>
>I will add __attribute__((aligned(4))) to be safe.
>

Small correction, I meant   __attribute__((packed, aligned(4))).

Thanks,
Shijith

Re: [PATCH v3 4/4] vdpa/octeon_ep: read vendor-specific PCI capability

[Michael S. Tsirkin]

On Fri, Dec 13, 2024 at 02:20:24PM +0000, Shijith Thotton wrote:
> >>>
> >>> Added support to read the vendor-specific PCI capability to identify the
> >>> type of device being emulated.
> >>>
> >>> Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>
> >>> Signed-off-by: Shijith Thotton <sthotton@marvell.com>
> >>> ---
> >>>  drivers/vdpa/octeon_ep/octep_vdpa.h      | 20 ++++++++++++++
> >>>  drivers/vdpa/octeon_ep/octep_vdpa_hw.c   | 33
> >>+++++++++++++++++++++++-
> >>>  drivers/vdpa/octeon_ep/octep_vdpa_main.c |  4 ++-
> >>>  3 files changed, 55 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/vdpa/octeon_ep/octep_vdpa.h
> >>b/drivers/vdpa/octeon_ep/octep_vdpa.h
> >>> index 2cadb878e679..53b020b019f7 100644
> >>> --- a/drivers/vdpa/octeon_ep/octep_vdpa.h
> >>> +++ b/drivers/vdpa/octeon_ep/octep_vdpa.h
> >>> @@ -8,6 +8,7 @@
> >>>  #include <linux/pci_regs.h>
> >>>  #include <linux/vdpa.h>
> >>>  #include <linux/virtio_pci_modern.h>
> >>> +#include <uapi/linux/virtio_crypto.h>
> >>>  #include <uapi/linux/virtio_net.h>
> >>>  #include <uapi/linux/virtio_blk.h>
> >>>  #include <uapi/linux/virtio_config.h>
> >>> @@ -52,6 +53,24 @@ struct octep_vring_info {
> >>>         phys_addr_t notify_pa;
> >>>  };
> >>>
> >>> +enum octep_pci_vndr_cfg_type {
> >>> +       OCTEP_PCI_VNDR_CFG_TYPE_VIRTIO_ID,
> >>> +       OCTEP_PCI_VNDR_CFG_TYPE_MAX,
> >>> +};
> >>> +
> >>> +struct octep_pci_vndr_data {
> >>> +       struct virtio_pci_vndr_data hdr;
> >>> +       u8 id;
> >>> +       u8 bar;
> >>> +       union {
> >>> +               u64 data;
> >>> +               struct {
> >>> +                       u32 offset;
> >>> +                       u32 length;
> >>> +               };
> >>> +       };
> >>> +};
> >>
> >>This seems not to be padded to a multiple of 4 bytes?
> >>
> >
> >Looks like, the structure is naturally aligned.
> >
> >struct virtio_pci_vndr_data {
> >        uint8_t                    cap_vndr;             /*     0     1 */
> >        uint8_t                    cap_next;             /*     1     1 */
> >        uint8_t                    cap_len;              /*     2     1 */
> >        uint8_t                    cfg_type;             /*     3     1 */
> >        uint16_t                   vendor_id;            /*     4     2 */
> >        /* size: 6, cachelines: 1, members: 5 */
> >        /* last cacheline: 6 bytes */
> >};
> >struct octep_pci_vndr_data {
> >        struct virtio_pci_vndr_data hdr;                 /*     0     6 */
> >        uint8_t                    id;                   /*     6     1 */
> >        uint8_t                    bar;                  /*     7     1 */
> >        union {
> >                uint64_t           data;                 /*     8     8 */
> >                struct {
> >                        uint32_t   offset;               /*     8     4 */
> >                        uint32_t   length;               /*    12     4 */
> >                };                                       /*     8     8 */
> >        };                                               /*     8     8 */
> >
> >        /* size: 16, cachelines: 1, members: 4 */
> >        /* last cacheline: 16 bytes */
> >};
> >
> >I will add __attribute__((aligned(4))) to be safe.
> >
> 
> Small correction, I meant   __attribute__((packed, aligned(4))).
> 
> Thanks,
> Shijith

Don't add packed pls, for me it means "something is not packed naturally
here". when in fact it is.

aligned is also bad, since it can add padding which creates all
kind of security issues.

If you want to be sure, add BUILD_BUG_ON and check that it is aligned
as expected.

Re: [PATCH v3 4/4] vdpa/octeon_ep: read vendor-specific PCI capability

[Shijith Thotton]

>> >>> Added support to read the vendor-specific PCI capability to identify the
>> >>> type of device being emulated.
>> >>>
>> >>> Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>
>> >>> Signed-off-by: Shijith Thotton <sthotton@marvell.com>
>> >>> ---
>> >>>  drivers/vdpa/octeon_ep/octep_vdpa.h      | 20 ++++++++++++++
>> >>>  drivers/vdpa/octeon_ep/octep_vdpa_hw.c   | 33
>> >>+++++++++++++++++++++++-
>> >>>  drivers/vdpa/octeon_ep/octep_vdpa_main.c |  4 ++-
>> >>>  3 files changed, 55 insertions(+), 2 deletions(-)
>> >>>
>> >>> diff --git a/drivers/vdpa/octeon_ep/octep_vdpa.h
>> >>b/drivers/vdpa/octeon_ep/octep_vdpa.h
>> >>> index 2cadb878e679..53b020b019f7 100644
>> >>> --- a/drivers/vdpa/octeon_ep/octep_vdpa.h
>> >>> +++ b/drivers/vdpa/octeon_ep/octep_vdpa.h
>> >>> @@ -8,6 +8,7 @@
>> >>>  #include <linux/pci_regs.h>
>> >>>  #include <linux/vdpa.h>
>> >>>  #include <linux/virtio_pci_modern.h>
>> >>> +#include <uapi/linux/virtio_crypto.h>
>> >>>  #include <uapi/linux/virtio_net.h>
>> >>>  #include <uapi/linux/virtio_blk.h>
>> >>>  #include <uapi/linux/virtio_config.h>
>> >>> @@ -52,6 +53,24 @@ struct octep_vring_info {
>> >>>         phys_addr_t notify_pa;
>> >>>  };
>> >>>
>> >>> +enum octep_pci_vndr_cfg_type {
>> >>> +       OCTEP_PCI_VNDR_CFG_TYPE_VIRTIO_ID,
>> >>> +       OCTEP_PCI_VNDR_CFG_TYPE_MAX,
>> >>> +};
>> >>> +
>> >>> +struct octep_pci_vndr_data {
>> >>> +       struct virtio_pci_vndr_data hdr;
>> >>> +       u8 id;
>> >>> +       u8 bar;
>> >>> +       union {
>> >>> +               u64 data;
>> >>> +               struct {
>> >>> +                       u32 offset;
>> >>> +                       u32 length;
>> >>> +               };
>> >>> +       };
>> >>> +};
>> >>
>> >>This seems not to be padded to a multiple of 4 bytes?
>> >>
>> >
>> >Looks like, the structure is naturally aligned.
>> >
>> >struct virtio_pci_vndr_data {
>> >        uint8_t                    cap_vndr;             /*     0     1 */
>> >        uint8_t                    cap_next;             /*     1     1 */
>> >        uint8_t                    cap_len;              /*     2     1 */
>> >        uint8_t                    cfg_type;             /*     3     1 */
>> >        uint16_t                   vendor_id;            /*     4     2 */
>> >        /* size: 6, cachelines: 1, members: 5 */
>> >        /* last cacheline: 6 bytes */
>> >};
>> >struct octep_pci_vndr_data {
>> >        struct virtio_pci_vndr_data hdr;                 /*     0     6 */
>> >        uint8_t                    id;                   /*     6     1 */
>> >        uint8_t                    bar;                  /*     7     1 */
>> >        union {
>> >                uint64_t           data;                 /*     8     8 */
>> >                struct {
>> >                        uint32_t   offset;               /*     8     4 */
>> >                        uint32_t   length;               /*    12     4 */
>> >                };                                       /*     8     8 */
>> >        };                                               /*     8     8 */
>> >
>> >        /* size: 16, cachelines: 1, members: 4 */
>> >        /* last cacheline: 16 bytes */
>> >};
>> >
>> >I will add __attribute__((aligned(4))) to be safe.
>> >
>>
>> Small correction, I meant   __attribute__((packed, aligned(4))).
>>
>> Thanks,
>> Shijith
>
>Don't add packed pls, for me it means "something is not packed naturally
>here". when in fact it is.
>
>aligned is also bad, since it can add padding which creates all
>kind of security issues.
>
>If you want to be sure, add BUILD_BUG_ON and check that it is aligned
>as expected.

Sure, I will use BUILD_BUG_ON to check the structure size.
BUILD_BUG_ON(sizeof(struct octep_pci_vndr_data) % 4 != 0);

Thanks,
Shijith

Re: [PATCH v3 1/4] vdpa/octeon_ep: enable support for multiple interrupts per device

[Jason Wang]

On Mon, Dec 9, 2024 at 11:16 PM Shijith Thotton <sthotton@marvell.com> wrote:
>
> Updated the driver to utilize all the MSI-X interrupt vectors supported
> by each OCTEON endpoint VF, instead of relying on a single vector.
> Enabling more interrupts allows packets from multiple rings to be
> distributed across multiple cores, improving parallelism and
> performance.
>
> Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>
> Signed-off-by: Shijith Thotton <sthotton@marvell.com>

Acked-by: Jason Wang <jasowang@redhat.com>

Thanks