* Re: [RFC PATCH net-next 15/19] pds_vdpa: virtio bar setup for vdpa
[not found] ` <20221118225656.48309-16-snelson@pensando.io>
@ 2022-11-22 3:32 ` Jason Wang
2022-11-22 6:36 ` Jason Wang
0 siblings, 1 reply; 13+ messages in thread
From: Jason Wang @ 2022-11-22 3:32 UTC (permalink / raw)
To: Shannon Nelson, netdev, davem, kuba, mst, virtualization; +Cc: drivers
在 2022/11/19 06:56, Shannon Nelson 写道:
> The PDS vDPA device has a virtio BAR for describing itself, and
> the pds_vdpa driver needs to access it. Here we copy liberally
> from the existing drivers/virtio/virtio_pci_modern_dev.c as it
> has what we need, but we need to modify it so that it can work
> with our device id and so we can use our own DMA mask.
>
> We suspect there is room for discussion here about making the
> existing code a little more flexible, but we thought we'd at
> least start the discussion here.
Exactly, since the virtio_pci_modern_dev.c is a library, we could tweak
it to allow the caller to pass the device_id with the DMA mask. Then we
can avoid code/bug duplication here.
Thanks
>
> Signed-off-by: Shannon Nelson <snelson@pensando.io>
> ---
> drivers/vdpa/pds/Makefile | 3 +-
> drivers/vdpa/pds/pci_drv.c | 10 ++
> drivers/vdpa/pds/pci_drv.h | 2 +
> drivers/vdpa/pds/virtio_pci.c | 283 ++++++++++++++++++++++++++++++++++
> 4 files changed, 297 insertions(+), 1 deletion(-)
> create mode 100644 drivers/vdpa/pds/virtio_pci.c
>
> diff --git a/drivers/vdpa/pds/Makefile b/drivers/vdpa/pds/Makefile
> index 3ba28a875574..b8376ab165bc 100644
> --- a/drivers/vdpa/pds/Makefile
> +++ b/drivers/vdpa/pds/Makefile
> @@ -4,4 +4,5 @@
> obj-$(CONFIG_PDS_VDPA) := pds_vdpa.o
>
> pds_vdpa-y := pci_drv.o \
> - debugfs.o
> + debugfs.o \
> + virtio_pci.o
> diff --git a/drivers/vdpa/pds/pci_drv.c b/drivers/vdpa/pds/pci_drv.c
> index 369e11153f21..10491e22778c 100644
> --- a/drivers/vdpa/pds/pci_drv.c
> +++ b/drivers/vdpa/pds/pci_drv.c
> @@ -44,6 +44,14 @@ pds_vdpa_pci_probe(struct pci_dev *pdev,
> goto err_out_free_mem;
> }
>
> + vdpa_pdev->vd_mdev.pci_dev = pdev;
> + err = pds_vdpa_probe_virtio(&vdpa_pdev->vd_mdev);
> + if (err) {
> + dev_err(dev, "Unable to probe for virtio configuration: %pe\n",
> + ERR_PTR(err));
> + goto err_out_free_mem;
> + }
> +
> pci_enable_pcie_error_reporting(pdev);
>
> /* Use devres management */
> @@ -74,6 +82,7 @@ pds_vdpa_pci_probe(struct pci_dev *pdev,
> err_out_pci_release_device:
> pci_disable_device(pdev);
> err_out_free_mem:
> + pds_vdpa_remove_virtio(&vdpa_pdev->vd_mdev);
> pci_disable_pcie_error_reporting(pdev);
> kfree(vdpa_pdev);
> return err;
> @@ -88,6 +97,7 @@ pds_vdpa_pci_remove(struct pci_dev *pdev)
> pci_clear_master(pdev);
> pci_disable_pcie_error_reporting(pdev);
> pci_disable_device(pdev);
> + pds_vdpa_remove_virtio(&vdpa_pdev->vd_mdev);
> kfree(vdpa_pdev);
>
> dev_info(&pdev->dev, "Removed\n");
> diff --git a/drivers/vdpa/pds/pci_drv.h b/drivers/vdpa/pds/pci_drv.h
> index 747809e0df9e..15f3b34fafa9 100644
> --- a/drivers/vdpa/pds/pci_drv.h
> +++ b/drivers/vdpa/pds/pci_drv.h
> @@ -43,4 +43,6 @@ struct pds_vdpa_pci_device {
> struct virtio_pci_modern_device vd_mdev;
> };
>
> +int pds_vdpa_probe_virtio(struct virtio_pci_modern_device *mdev);
> +void pds_vdpa_remove_virtio(struct virtio_pci_modern_device *mdev);
> #endif /* _PCI_DRV_H */
> diff --git a/drivers/vdpa/pds/virtio_pci.c b/drivers/vdpa/pds/virtio_pci.c
> new file mode 100644
> index 000000000000..0f4ac9467199
> --- /dev/null
> +++ b/drivers/vdpa/pds/virtio_pci.c
> @@ -0,0 +1,283 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +/*
> + * adapted from drivers/virtio/virtio_pci_modern_dev.c, v6.0-rc1
> + */
> +
> +#include <linux/virtio_pci_modern.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/delay.h>
> +
> +#include "pci_drv.h"
> +
> +/*
> + * pds_vdpa_map_capability - map a part of virtio pci capability
> + * @mdev: the modern virtio-pci device
> + * @off: offset of the capability
> + * @minlen: minimal length of the capability
> + * @align: align requirement
> + * @start: start from the capability
> + * @size: map size
> + * @len: the length that is actually mapped
> + * @pa: physical address of the capability
> + *
> + * Returns the io address of for the part of the capability
> + */
> +static void __iomem *
> +pds_vdpa_map_capability(struct virtio_pci_modern_device *mdev, int off,
> + size_t minlen, u32 align, u32 start, u32 size,
> + size_t *len, resource_size_t *pa)
> +{
> + struct pci_dev *dev = mdev->pci_dev;
> + u8 bar;
> + u32 offset, length;
> + void __iomem *p;
> +
> + pci_read_config_byte(dev, off + offsetof(struct virtio_pci_cap,
> + bar),
> + &bar);
> + pci_read_config_dword(dev, off + offsetof(struct virtio_pci_cap, offset),
> + &offset);
> + pci_read_config_dword(dev, off + offsetof(struct virtio_pci_cap, length),
> + &length);
> +
> + /* Check if the BAR may have changed since we requested the region. */
> + if (bar >= PCI_STD_NUM_BARS || !(mdev->modern_bars & (1 << bar))) {
> + dev_err(&dev->dev,
> + "virtio_pci: bar unexpectedly changed to %u\n", bar);
> + return NULL;
> + }
> +
> + if (length <= start) {
> + dev_err(&dev->dev,
> + "virtio_pci: bad capability len %u (>%u expected)\n",
> + length, start);
> + return NULL;
> + }
> +
> + if (length - start < minlen) {
> + dev_err(&dev->dev,
> + "virtio_pci: bad capability len %u (>=%zu expected)\n",
> + length, minlen);
> + return NULL;
> + }
> +
> + length -= start;
> +
> + if (start + offset < offset) {
> + dev_err(&dev->dev,
> + "virtio_pci: map wrap-around %u+%u\n",
> + start, offset);
> + return NULL;
> + }
> +
> + offset += start;
> +
> + if (offset & (align - 1)) {
> + dev_err(&dev->dev,
> + "virtio_pci: offset %u not aligned to %u\n",
> + offset, align);
> + return NULL;
> + }
> +
> + if (length > size)
> + length = size;
> +
> + if (len)
> + *len = length;
> +
> + if (minlen + offset < minlen ||
> + minlen + offset > pci_resource_len(dev, bar)) {
> + dev_err(&dev->dev,
> + "virtio_pci: map virtio %zu@%u out of range on bar %i length %lu\n",
> + minlen, offset,
> + bar, (unsigned long)pci_resource_len(dev, bar));
> + return NULL;
> + }
> +
> + p = pci_iomap_range(dev, bar, offset, length);
> + if (!p)
> + dev_err(&dev->dev,
> + "virtio_pci: unable to map virtio %u@%u on bar %i\n",
> + length, offset, bar);
> + else if (pa)
> + *pa = pci_resource_start(dev, bar) + offset;
> +
> + return p;
> +}
> +
> +/**
> + * virtio_pci_find_capability - walk capabilities to find device info.
> + * @dev: the pci device
> + * @cfg_type: the VIRTIO_PCI_CAP_* value we seek
> + * @ioresource_types: IORESOURCE_MEM and/or IORESOURCE_IO.
> + * @bars: the bitmask of BARs
> + *
> + * Returns offset of the capability, or 0.
> + */
> +static inline int virtio_pci_find_capability(struct pci_dev *dev, u8 cfg_type,
> + u32 ioresource_types, int *bars)
> +{
> + int pos;
> +
> + for (pos = pci_find_capability(dev, PCI_CAP_ID_VNDR);
> + pos > 0;
> + pos = pci_find_next_capability(dev, pos, PCI_CAP_ID_VNDR)) {
> + u8 type, bar;
> +
> + pci_read_config_byte(dev, pos + offsetof(struct virtio_pci_cap,
> + cfg_type),
> + &type);
> + pci_read_config_byte(dev, pos + offsetof(struct virtio_pci_cap,
> + bar),
> + &bar);
> +
> + /* Ignore structures with reserved BAR values */
> + if (bar >= PCI_STD_NUM_BARS)
> + continue;
> +
> + if (type == cfg_type) {
> + if (pci_resource_len(dev, bar) &&
> + pci_resource_flags(dev, bar) & ioresource_types) {
> + *bars |= (1 << bar);
> + return pos;
> + }
> + }
> + }
> + return 0;
> +}
> +
> +/*
> + * pds_vdpa_probe_virtio: probe the modern virtio pci device, note that the
> + * caller is required to enable PCI device before calling this function.
> + * @mdev: the modern virtio-pci device
> + *
> + * Return 0 on succeed otherwise fail
> + */
> +int pds_vdpa_probe_virtio(struct virtio_pci_modern_device *mdev)
> +{
> + struct pci_dev *pci_dev = mdev->pci_dev;
> + int err, common, isr, notify, device;
> + u32 notify_length;
> + u32 notify_offset;
> +
> + /* check for a common config: if not, use legacy mode (bar 0). */
> + common = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_COMMON_CFG,
> + IORESOURCE_IO | IORESOURCE_MEM,
> + &mdev->modern_bars);
> + if (!common) {
> + dev_info(&pci_dev->dev,
> + "virtio_pci: missing common config\n");
> + return -ENODEV;
> + }
> +
> + /* If common is there, these should be too... */
> + isr = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_ISR_CFG,
> + IORESOURCE_IO | IORESOURCE_MEM,
> + &mdev->modern_bars);
> + notify = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_NOTIFY_CFG,
> + IORESOURCE_IO | IORESOURCE_MEM,
> + &mdev->modern_bars);
> + if (!isr || !notify) {
> + dev_err(&pci_dev->dev,
> + "virtio_pci: missing capabilities %i/%i/%i\n",
> + common, isr, notify);
> + return -EINVAL;
> + }
> +
> + /* Device capability is only mandatory for devices that have
> + * device-specific configuration.
> + */
> + device = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_DEVICE_CFG,
> + IORESOURCE_IO | IORESOURCE_MEM,
> + &mdev->modern_bars);
> +
> + err = pci_request_selected_regions(pci_dev, mdev->modern_bars,
> + "virtio-pci-modern");
> + if (err)
> + return err;
> +
> + err = -EINVAL;
> + mdev->common = pds_vdpa_map_capability(mdev, common,
> + sizeof(struct virtio_pci_common_cfg), 4,
> + 0, sizeof(struct virtio_pci_common_cfg),
> + NULL, NULL);
> + if (!mdev->common)
> + goto err_map_common;
> + mdev->isr = pds_vdpa_map_capability(mdev, isr, sizeof(u8), 1,
> + 0, 1,
> + NULL, NULL);
> + if (!mdev->isr)
> + goto err_map_isr;
> +
> + /* Read notify_off_multiplier from config space. */
> + pci_read_config_dword(pci_dev,
> + notify + offsetof(struct virtio_pci_notify_cap,
> + notify_off_multiplier),
> + &mdev->notify_offset_multiplier);
> + /* Read notify length and offset from config space. */
> + pci_read_config_dword(pci_dev,
> + notify + offsetof(struct virtio_pci_notify_cap,
> + cap.length),
> + ¬ify_length);
> +
> + pci_read_config_dword(pci_dev,
> + notify + offsetof(struct virtio_pci_notify_cap,
> + cap.offset),
> + ¬ify_offset);
> +
> + /* We don't know how many VQs we'll map, ahead of the time.
> + * If notify length is small, map it all now.
> + * Otherwise, map each VQ individually later.
> + */
> + if ((u64)notify_length + (notify_offset % PAGE_SIZE) <= PAGE_SIZE) {
> + mdev->notify_base = pds_vdpa_map_capability(mdev, notify,
> + 2, 2,
> + 0, notify_length,
> + &mdev->notify_len,
> + &mdev->notify_pa);
> + if (!mdev->notify_base)
> + goto err_map_notify;
> + } else {
> + mdev->notify_map_cap = notify;
> + }
> +
> + /* Again, we don't know how much we should map, but PAGE_SIZE
> + * is more than enough for all existing devices.
> + */
> + if (device) {
> + mdev->device = pds_vdpa_map_capability(mdev, device, 0, 4,
> + 0, PAGE_SIZE,
> + &mdev->device_len,
> + NULL);
> + if (!mdev->device)
> + goto err_map_device;
> + }
> +
> + return 0;
> +
> +err_map_device:
> + if (mdev->notify_base)
> + pci_iounmap(pci_dev, mdev->notify_base);
> +err_map_notify:
> + pci_iounmap(pci_dev, mdev->isr);
> +err_map_isr:
> + pci_iounmap(pci_dev, mdev->common);
> +err_map_common:
> + pci_release_selected_regions(pci_dev, mdev->modern_bars);
> + return err;
> +}
> +
> +void pds_vdpa_remove_virtio(struct virtio_pci_modern_device *mdev)
> +{
> + struct pci_dev *pci_dev = mdev->pci_dev;
> +
> + if (mdev->device)
> + pci_iounmap(pci_dev, mdev->device);
> + if (mdev->notify_base)
> + pci_iounmap(pci_dev, mdev->notify_base);
> + pci_iounmap(pci_dev, mdev->isr);
> + pci_iounmap(pci_dev, mdev->common);
> + pci_release_selected_regions(pci_dev, mdev->modern_bars);
> +}
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH net-next 14/19] pds_vdpa: Add new PCI VF device for PDS vDPA services
[not found] ` <20221118225656.48309-15-snelson@pensando.io>
@ 2022-11-22 3:53 ` Jason Wang
0 siblings, 0 replies; 13+ messages in thread
From: Jason Wang @ 2022-11-22 3:53 UTC (permalink / raw)
To: Shannon Nelson, netdev, davem, kuba, mst, virtualization; +Cc: drivers
在 2022/11/19 06:56, Shannon Nelson 写道:
> This is the initial PCI driver framework for the new pds_vdpa VF
> device driver, an auxiliary_bus client of the pds_core driver.
> This does the very basics of registering for the new PCI
> device 1dd8:100b, setting up debugfs entries, and registering
> with devlink.
>
> The new PCI device id has not made it to the official PCI ID Repository
> yet, but will soon be registered there.
>
> Signed-off-by: Shannon Nelson <snelson@pensando.io>
> ---
> drivers/vdpa/pds/Makefile | 7 +
> drivers/vdpa/pds/debugfs.c | 44 +++++++
> drivers/vdpa/pds/debugfs.h | 22 ++++
> drivers/vdpa/pds/pci_drv.c | 143 +++++++++++++++++++++
> drivers/vdpa/pds/pci_drv.h | 46 +++++++
> include/linux/pds/pds_core_if.h | 1 +
> include/linux/pds/pds_vdpa.h | 219 ++++++++++++++++++++++++++++++++
> 7 files changed, 482 insertions(+)
> create mode 100644 drivers/vdpa/pds/Makefile
> create mode 100644 drivers/vdpa/pds/debugfs.c
> create mode 100644 drivers/vdpa/pds/debugfs.h
> create mode 100644 drivers/vdpa/pds/pci_drv.c
> create mode 100644 drivers/vdpa/pds/pci_drv.h
> create mode 100644 include/linux/pds/pds_vdpa.h
>
> diff --git a/drivers/vdpa/pds/Makefile b/drivers/vdpa/pds/Makefile
> new file mode 100644
> index 000000000000..3ba28a875574
> --- /dev/null
> +++ b/drivers/vdpa/pds/Makefile
> @@ -0,0 +1,7 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +# Copyright(c) 2022 Pensando Systems, Inc
> +
> +obj-$(CONFIG_PDS_VDPA) := pds_vdpa.o
> +
> +pds_vdpa-y := pci_drv.o \
> + debugfs.o
> diff --git a/drivers/vdpa/pds/debugfs.c b/drivers/vdpa/pds/debugfs.c
> new file mode 100644
> index 000000000000..f5b6654ae89b
> --- /dev/null
> +++ b/drivers/vdpa/pds/debugfs.c
> @@ -0,0 +1,44 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright(c) 2022 Pensando Systems, Inc */
> +
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/types.h>
> +
> +#include <linux/pds/pds_core_if.h>
> +#include <linux/pds/pds_vdpa.h>
> +
> +#include "pci_drv.h"
> +#include "debugfs.h"
> +
> +#ifdef CONFIG_DEBUG_FS
> +
> +static struct dentry *dbfs_dir;
> +
> +void
> +pds_vdpa_debugfs_create(void)
> +{
> + dbfs_dir = debugfs_create_dir(PDS_VDPA_DRV_NAME, NULL);
> +}
> +
> +void
> +pds_vdpa_debugfs_destroy(void)
> +{
> + debugfs_remove_recursive(dbfs_dir);
> + dbfs_dir = NULL;
> +}
> +
> +void
> +pds_vdpa_debugfs_add_pcidev(struct pds_vdpa_pci_device *vdpa_pdev)
> +{
> + vdpa_pdev->dentry = debugfs_create_dir(pci_name(vdpa_pdev->pdev), dbfs_dir);
> +}
> +
> +void
> +pds_vdpa_debugfs_del_pcidev(struct pds_vdpa_pci_device *vdpa_pdev)
> +{
> + debugfs_remove_recursive(vdpa_pdev->dentry);
> + vdpa_pdev->dentry = NULL;
> +}
> +
> +#endif /* CONFIG_DEBUG_FS */
> diff --git a/drivers/vdpa/pds/debugfs.h b/drivers/vdpa/pds/debugfs.h
> new file mode 100644
> index 000000000000..ac31ab47746b
> --- /dev/null
> +++ b/drivers/vdpa/pds/debugfs.h
> @@ -0,0 +1,22 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright(c) 2022 Pensando Systems, Inc */
> +
> +#ifndef _PDS_VDPA_DEBUGFS_H_
> +#define _PDS_VDPA_DEBUGFS_H_
> +
> +#include <linux/debugfs.h>
> +
> +#ifdef CONFIG_DEBUG_FS
> +
> +void pds_vdpa_debugfs_create(void);
> +void pds_vdpa_debugfs_destroy(void);
> +void pds_vdpa_debugfs_add_pcidev(struct pds_vdpa_pci_device *vdpa_pdev);
> +void pds_vdpa_debugfs_del_pcidev(struct pds_vdpa_pci_device *vdpa_pdev);
> +#else
> +static inline void pds_vdpa_debugfs_create(void) { }
> +static inline void pds_vdpa_debugfs_destroy(void) { }
> +static inline void pds_vdpa_debugfs_add_pcidev(struct pds_vdpa_pci_device *vdpa_pdev) { }
> +static inline void pds_vdpa_debugfs_del_pcidev(struct pds_vdpa_pci_device *vdpa_pdev) { }
> +#endif
> +
> +#endif /* _PDS_VDPA_DEBUGFS_H_ */
> diff --git a/drivers/vdpa/pds/pci_drv.c b/drivers/vdpa/pds/pci_drv.c
> new file mode 100644
> index 000000000000..369e11153f21
> --- /dev/null
> +++ b/drivers/vdpa/pds/pci_drv.c
> @@ -0,0 +1,143 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright(c) 2022 Pensando Systems, Inc */
> +
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/aer.h>
> +#include <linux/types.h>
> +#include <linux/vdpa.h>
> +
> +#include <linux/pds/pds_core_if.h>
> +#include <linux/pds/pds_vdpa.h>
> +
> +#include "pci_drv.h"
> +#include "debugfs.h"
> +
> +static void
> +pds_vdpa_dma_action(void *data)
> +{
> + pci_free_irq_vectors((struct pci_dev *)data);
> +}
Nit: since we're release irq vectors, it might be better to use
"pds_vdpa_irq_action"
> +
> +static int
> +pds_vdpa_pci_probe(struct pci_dev *pdev,
> + const struct pci_device_id *id)
> +{
> + struct pds_vdpa_pci_device *vdpa_pdev;
> + struct device *dev = &pdev->dev;
> + int err;
> +
> + vdpa_pdev = kzalloc(sizeof(*vdpa_pdev), GFP_KERNEL);
> + if (!vdpa_pdev)
> + return -ENOMEM;
> + pci_set_drvdata(pdev, vdpa_pdev);
> +
> + vdpa_pdev->pdev = pdev;
> + vdpa_pdev->vf_id = pci_iov_vf_id(pdev);
> + vdpa_pdev->pci_id = PCI_DEVID(pdev->bus->number, pdev->devfn);
> +
> + /* Query system for DMA addressing limitation for the device. */
> + err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(PDS_CORE_ADDR_LEN));
> + if (err) {
> + dev_err(dev, "Unable to obtain 64-bit DMA for consistent allocations, aborting. %pe\n",
> + ERR_PTR(err));
> + goto err_out_free_mem;
> + }
> +
> + pci_enable_pcie_error_reporting(pdev);
> +
> + /* Use devres management */
> + err = pcim_enable_device(pdev);
> + if (err) {
> + dev_err(dev, "Cannot enable PCI device: %pe\n", ERR_PTR(err));
> + goto err_out_free_mem;
> + }
> +
> + err = devm_add_action_or_reset(dev, pds_vdpa_dma_action, pdev);
> + if (err) {
> + dev_err(dev, "Failed adding devres for freeing irq vectors: %pe\n",
> + ERR_PTR(err));
> + goto err_out_pci_release_device;
> + }
> +
> + pci_set_master(pdev);
> +
> + pds_vdpa_debugfs_add_pcidev(vdpa_pdev);
> +
> + dev_info(dev, "%s: PF %#04x VF %#04x (%d) vf_id %d domain %d vdpa_aux %p vdpa_pdev %p\n",
> + __func__, pci_dev_id(vdpa_pdev->pdev->physfn),
> + vdpa_pdev->pci_id, vdpa_pdev->pci_id, vdpa_pdev->vf_id,
> + pci_domain_nr(pdev->bus), vdpa_pdev->vdpa_aux, vdpa_pdev);
> +
> + return 0;
> +
> +err_out_pci_release_device:
> + pci_disable_device(pdev);
Do we still need to care about this consider we use
devres/pcim_enable_device()?
> +err_out_free_mem:
> + pci_disable_pcie_error_reporting(pdev);
> + kfree(vdpa_pdev);
> + return err;
> +}
> +
> +static void
> +pds_vdpa_pci_remove(struct pci_dev *pdev)
> +{
> + struct pds_vdpa_pci_device *vdpa_pdev = pci_get_drvdata(pdev);
> +
> + pds_vdpa_debugfs_del_pcidev(vdpa_pdev);
> + pci_clear_master(pdev);
> + pci_disable_pcie_error_reporting(pdev);
> + pci_disable_device(pdev);
> + kfree(vdpa_pdev);
> +
> + dev_info(&pdev->dev, "Removed\n");
> +}
> +
> +static const struct pci_device_id
> +pds_vdpa_pci_table[] = {
> + { PCI_VDEVICE(PENSANDO, PCI_DEVICE_ID_PENSANDO_VDPA_VF) },
> + { 0, }
> +};
> +MODULE_DEVICE_TABLE(pci, pds_vdpa_pci_table);
> +
> +static struct pci_driver
> +pds_vdpa_pci_driver = {
> + .name = PDS_VDPA_DRV_NAME,
> + .id_table = pds_vdpa_pci_table,
> + .probe = pds_vdpa_pci_probe,
> + .remove = pds_vdpa_pci_remove
> +};
> +
> +static void __exit
> +pds_vdpa_pci_cleanup(void)
> +{
> + pci_unregister_driver(&pds_vdpa_pci_driver);
> +
> + pds_vdpa_debugfs_destroy();
> +}
> +module_exit(pds_vdpa_pci_cleanup);
> +
> +static int __init
> +pds_vdpa_pci_init(void)
> +{
> + int err;
> +
> + pds_vdpa_debugfs_create();
> +
> + err = pci_register_driver(&pds_vdpa_pci_driver);
> + if (err) {
> + pr_err("%s: pci driver register failed: %pe\n", __func__, ERR_PTR(err));
> + goto err_pci;
> + }
> +
> + return 0;
> +
> +err_pci:
> + pds_vdpa_debugfs_destroy();
> + return err;
> +}
> +module_init(pds_vdpa_pci_init);
> +
> +MODULE_DESCRIPTION(PDS_VDPA_DRV_DESCRIPTION);
> +MODULE_AUTHOR("Pensando Systems, Inc");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/vdpa/pds/pci_drv.h b/drivers/vdpa/pds/pci_drv.h
> new file mode 100644
> index 000000000000..747809e0df9e
> --- /dev/null
> +++ b/drivers/vdpa/pds/pci_drv.h
> @@ -0,0 +1,46 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/* Copyright(c) 2022 Pensando Systems, Inc */
> +
> +#ifndef _PCI_DRV_H
> +#define _PCI_DRV_H
> +
> +#include <linux/pci.h>
> +#include <linux/virtio_pci_modern.h>
> +
> +#define PDS_VDPA_DRV_NAME "pds_vdpa"
> +#define PDS_VDPA_DRV_DESCRIPTION "Pensando vDPA VF Device Driver"
> +
> +#define PDS_VDPA_BAR_BASE 0
> +#define PDS_VDPA_BAR_INTR 2
> +#define PDS_VDPA_BAR_DBELL 4
> +
> +struct pds_dev_bar {
> + int index;
> + void __iomem *vaddr;
> + phys_addr_t pa;
> + unsigned long len;
> +};
> +
> +struct pds_vdpa_intr_info {
> + int index;
> + int irq;
> + int qid;
> + char name[32];
> +};
> +
> +struct pds_vdpa_pci_device {
> + struct pci_dev *pdev;
> + struct pds_vdpa_aux *vdpa_aux;
> +
> + int vf_id;
> + int pci_id;
> +
> + int nintrs;
> + struct pds_vdpa_intr_info *intrs;
> +
> + struct dentry *dentry;
> +
> + struct virtio_pci_modern_device vd_mdev;
> +};
> +
> +#endif /* _PCI_DRV_H */
> diff --git a/include/linux/pds/pds_core_if.h b/include/linux/pds/pds_core_if.h
> index 6333ec351e14..6e92697657e4 100644
> --- a/include/linux/pds/pds_core_if.h
> +++ b/include/linux/pds/pds_core_if.h
> @@ -8,6 +8,7 @@
>
> #define PCI_VENDOR_ID_PENSANDO 0x1dd8
> #define PCI_DEVICE_ID_PENSANDO_CORE_PF 0x100c
> +#define PCI_DEVICE_ID_PENSANDO_VDPA_VF 0x100b
>
> #define PDS_CORE_BARS_MAX 4
> #define PDS_CORE_PCI_BAR_DBELL 1
> diff --git a/include/linux/pds/pds_vdpa.h b/include/linux/pds/pds_vdpa.h
> new file mode 100644
> index 000000000000..7ecef890f175
> --- /dev/null
> +++ b/include/linux/pds/pds_vdpa.h
> @@ -0,0 +1,219 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/* Copyright(c) 2022 Pensando Systems, Inc */
> +
> +#ifndef _PDS_VDPA_IF_H_
> +#define _PDS_VDPA_IF_H_
> +
> +#include <linux/pds/pds_common.h>
> +
> +#define PDS_DEV_TYPE_VDPA_STR "vDPA"
> +#define PDS_VDPA_DEV_NAME PDS_CORE_DRV_NAME "." PDS_DEV_TYPE_VDPA_STR
> +
> +/*
> + * enum pds_vdpa_cmd_opcode - vDPA Device commands
> + */
> +enum pds_vdpa_cmd_opcode {
> + PDS_VDPA_CMD_INIT = 48,
> + PDS_VDPA_CMD_IDENT = 49,
> + PDS_VDPA_CMD_RESET = 51,
> + PDS_VDPA_CMD_VQ_RESET = 52,
> + PDS_VDPA_CMD_VQ_INIT = 53,
> + PDS_VDPA_CMD_STATUS_UPDATE = 54,
> + PDS_VDPA_CMD_SET_FEATURES = 55,
> + PDS_VDPA_CMD_SET_ATTR = 56,
> +};
> +
> +/**
> + * struct pds_vdpa_cmd - generic command
> + * @opcode: Opcode
> + * @vdpa_index: Index for vdpa subdevice
> + * @vf_id: VF id
> + */
> +struct pds_vdpa_cmd {
> + u8 opcode;
> + u8 vdpa_index;
> + __le16 vf_id;
> +};
> +
> +/**
> + * struct pds_vdpa_comp - generic command completion
> + * @status: Status of the command (enum pds_core_status_code)
> + * @rsvd: Word boundary padding
> + * @color: Color bit
> + */
> +struct pds_vdpa_comp {
> + u8 status;
> + u8 rsvd[14];
> + u8 color;
> +};
> +
> +/**
> + * struct pds_vdpa_init_cmd - INIT command
> + * @opcode: Opcode PDS_VDPA_CMD_INIT
> + * @vdpa_index: Index for vdpa subdevice
> + * @vf_id: VF id
> + * @len: length of config info DMA space
> + * @config_pa: address for DMA of virtio_net_config struct
Looks like the structure is not specific to net, if yes, we may tweak
the above comment to say it's the address of the device configuration space.
> + */
> +struct pds_vdpa_init_cmd {
> + u8 opcode;
> + u8 vdpa_index;
> + __le16 vf_id;
> + __le32 len;
> + __le64 config_pa;
> +};
> +
> +/**
> + * struct pds_vdpa_ident - vDPA identification data
> + * @hw_features: vDPA features supported by device
> + * @max_vqs: max queues available (2 queues for a single queuepair)
> + * @max_qlen: log(2) of maximum number of descriptors
> + * @min_qlen: log(2) of minimum number of descriptors
Note that is you have the plan to support packed virtqueue, the qlen is
not necessarily the power of 2.
> + *
> + * This struct is used in a DMA block that is set up for the PDS_VDPA_CMD_IDENT
> + * transaction. Set up the DMA block and send the address in the IDENT cmd
> + * data, the DSC will write the ident information, then we can remove the DMA
> + * block after reading the answer. If the completion status is 0, then there
> + * is valid information, else there was an error and the data should be invalid.
> + */
> +struct pds_vdpa_ident {
> + __le64 hw_features;
> + __le16 max_vqs;
> + __le16 max_qlen;
> + __le16 min_qlen;
> +};
> +
> +/**
> + * struct pds_vdpa_ident_cmd - IDENT command
> + * @opcode: Opcode PDS_VDPA_CMD_IDENT
> + * @rsvd: Word boundary padding
> + * @vf_id: VF id
> + * @len: length of ident info DMA space
> + * @ident_pa: address for DMA of ident info (struct pds_vdpa_ident)
> + * only used for this transaction, then forgotten by DSC
> + */
> +struct pds_vdpa_ident_cmd {
> + u8 opcode;
> + u8 rsvd;
> + __le16 vf_id;
> + __le32 len;
> + __le64 ident_pa;
> +};
> +
> +/**
> + * struct pds_vdpa_status_cmd - STATUS_UPDATE command
> + * @opcode: Opcode PDS_VDPA_CMD_STATUS_UPDATE
> + * @vdpa_index: Index for vdpa subdevice
> + * @vf_id: VF id
> + * @status: new status bits
> + */
> +struct pds_vdpa_status_cmd {
> + u8 opcode;
> + u8 vdpa_index;
> + __le16 vf_id;
> + u8 status;
> +};
> +
> +/**
> + * enum pds_vdpa_attr - List of VDPA device attributes
> + * @PDS_VDPA_ATTR_MAC: MAC address
> + * @PDS_VDPA_ATTR_MAX_VQ_PAIRS: Max virtqueue pairs
> + */
> +enum pds_vdpa_attr {
> + PDS_VDPA_ATTR_MAC = 1,
> + PDS_VDPA_ATTR_MAX_VQ_PAIRS = 2,
> +};
> +
> +/**
> + * struct pds_vdpa_setattr_cmd - SET_ATTR command
> + * @opcode: Opcode PDS_VDPA_CMD_SET_ATTR
> + * @vdpa_index: Index for vdpa subdevice
> + * @vf_id: VF id
> + * @attr: attribute to be changed (enum pds_vdpa_attr)
> + * @pad: Word boundary padding
> + * @mac: new mac address to be assigned as vdpa device address
> + * @max_vq_pairs: new limit of virtqueue pairs
> + */
> +struct pds_vdpa_setattr_cmd {
> + u8 opcode;
> + u8 vdpa_index;
> + __le16 vf_id;
> + u8 attr;
> + u8 pad[3];
> + union {
> + u8 mac[6];
> + __le16 max_vq_pairs;
So does this mean if we want to set both mac and max_vq_paris, we need
two commands? The seems to be less efficient, since the mgmt layer could
provision more attributes here. Can we pack all attributes into a single
command?
> + } __packed;
> +};
> +
> +/**
> + * struct pds_vdpa_vq_init_cmd - queue init command
> + * @opcode: Opcode PDS_VDPA_CMD_VQ_INIT
> + * @vdpa_index: Index for vdpa subdevice
> + * @vf_id: VF id
> + * @qid: Queue id (bit0 clear = rx, bit0 set = tx, qid=N is ctrlq)
I wonder any reason we need to design it like this, it would be better
to make it general to be used by other type of virtio devices.
> + * @len: log(2) of max descriptor count
> + * @desc_addr: DMA address of descriptor area
> + * @avail_addr: DMA address of available descriptors (aka driver area)
> + * @used_addr: DMA address of used descriptors (aka device area)
> + * @intr_index: interrupt index
Is this something like MSI-X vector?
> + */
> +struct pds_vdpa_vq_init_cmd {
> + u8 opcode;
> + u8 vdpa_index;
> + __le16 vf_id;
> + __le16 qid;
> + __le16 len;
> + __le64 desc_addr;
> + __le64 avail_addr;
> + __le64 used_addr;
> + __le16 intr_index;
> +};
> +
> +/**
> + * struct pds_vdpa_vq_init_comp - queue init completion
> + * @status: Status of the command (enum pds_core_status_code)
> + * @hw_qtype: HW queue type, used in doorbell selection
> + * @hw_qindex: HW queue index, used in doorbell selection
> + * @rsvd: Word boundary padding
> + * @color: Color bit
More comment is needed to know the how to use this color bit.
> + */
> +struct pds_vdpa_vq_init_comp {
> + u8 status;
> + u8 hw_qtype;
> + __le16 hw_qindex;
> + u8 rsvd[11];
> + u8 color;
> +};
> +
> +/**
> + * struct pds_vdpa_vq_reset_cmd - queue reset command
> + * @opcode: Opcode PDS_VDPA_CMD_VQ_RESET
Is there a chance that we could have more type of opcode here?
Thanks
> + * @vdpa_index: Index for vdpa subdevice
> + * @vf_id: VF id
> + * @qid: Queue id
> + */
> +struct pds_vdpa_vq_reset_cmd {
> + u8 opcode;
> + u8 vdpa_index;
> + __le16 vf_id;
> + __le16 qid;
> +};
> +
> +/**
> + * struct pds_vdpa_set_features_cmd - set hw features
> + * @opcode: Opcode PDS_VDPA_CMD_SET_FEATURES
> + * @vdpa_index: Index for vdpa subdevice
> + * @vf_id: VF id
> + * @rsvd: Word boundary padding
> + * @features: Feature bit mask
> + */
> +struct pds_vdpa_set_features_cmd {
> + u8 opcode;
> + u8 vdpa_index;
> + __le16 vf_id;
> + __le32 rsvd;
> + __le64 features;
> +};
> +
> +#endif /* _PDS_VDPA_IF_H_ */
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH net-next 17/19] pds_vdpa: add vdpa config client commands
[not found] ` <20221118225656.48309-18-snelson@pensando.io>
@ 2022-11-22 6:32 ` Jason Wang
0 siblings, 0 replies; 13+ messages in thread
From: Jason Wang @ 2022-11-22 6:32 UTC (permalink / raw)
To: Shannon Nelson; +Cc: mst, netdev, virtualization, kuba, drivers, davem
On Sat, Nov 19, 2022 at 6:57 AM Shannon Nelson <snelson@pensando.io> wrote:
>
> These are the adminq commands that will be needed for
> setting up and using the vDPA device.
>
> Signed-off-by: Shannon Nelson <snelson@pensando.io>
> ---
> drivers/vdpa/pds/Makefile | 1 +
> drivers/vdpa/pds/cmds.c | 266 ++++++++++++++++++++++++++++++++++++
> drivers/vdpa/pds/cmds.h | 17 +++
> drivers/vdpa/pds/vdpa_dev.h | 60 ++++++++
> 4 files changed, 344 insertions(+)
> create mode 100644 drivers/vdpa/pds/cmds.c
> create mode 100644 drivers/vdpa/pds/cmds.h
> create mode 100644 drivers/vdpa/pds/vdpa_dev.h
>
[...]
> +struct pds_vdpa_device {
> + struct vdpa_device vdpa_dev;
> + struct pds_vdpa_aux *vdpa_aux;
> + struct pds_vdpa_hw hw;
> +
> + struct virtio_net_config vn_config;
> + dma_addr_t vn_config_pa;
So this is the dma address not necessarily pa, we'd better drop the "pa" suffix.
Thanks
> + struct dentry *dentry;
> +};
> +
> +int pds_vdpa_get_mgmt_info(struct pds_vdpa_aux *vdpa_aux);
> +
> +#endif /* _VDPA_DEV_H_ */
> --
> 2.17.1
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH net-next 18/19] pds_vdpa: add support for vdpa and vdpamgmt interfaces
[not found] ` <20221118225656.48309-19-snelson@pensando.io>
@ 2022-11-22 6:32 ` Jason Wang
[not found] ` <4b4e7474-5b36-6b2c-a0a5-2e198e1bab0c@amd.com>
0 siblings, 1 reply; 13+ messages in thread
From: Jason Wang @ 2022-11-22 6:32 UTC (permalink / raw)
To: Shannon Nelson; +Cc: mst, netdev, virtualization, kuba, drivers, davem
On Sat, Nov 19, 2022 at 6:57 AM Shannon Nelson <snelson@pensando.io> wrote:
>
> This is the vDPA device support, where we advertise that we can
> support the virtio queues and deal with the configuration work
> through the pds_core's adminq.
>
> Signed-off-by: Shannon Nelson <snelson@pensando.io>
> ---
> drivers/vdpa/pds/Makefile | 3 +-
> drivers/vdpa/pds/aux_drv.c | 33 ++
> drivers/vdpa/pds/debugfs.c | 167 ++++++++
> drivers/vdpa/pds/debugfs.h | 4 +
> drivers/vdpa/pds/vdpa_dev.c | 796 ++++++++++++++++++++++++++++++++++++
> 5 files changed, 1002 insertions(+), 1 deletion(-)
> create mode 100644 drivers/vdpa/pds/vdpa_dev.c
>
> diff --git a/drivers/vdpa/pds/Makefile b/drivers/vdpa/pds/Makefile
> index fafd356ddf86..7fde4a4a1620 100644
> --- a/drivers/vdpa/pds/Makefile
> +++ b/drivers/vdpa/pds/Makefile
> @@ -7,4 +7,5 @@ pds_vdpa-y := aux_drv.o \
> cmds.o \
> pci_drv.o \
> debugfs.o \
> - virtio_pci.o
> + virtio_pci.o \
> + vdpa_dev.o
> diff --git a/drivers/vdpa/pds/aux_drv.c b/drivers/vdpa/pds/aux_drv.c
> index aef3c984dc90..83b9a5a79325 100644
> --- a/drivers/vdpa/pds/aux_drv.c
> +++ b/drivers/vdpa/pds/aux_drv.c
> @@ -12,6 +12,7 @@
> #include <linux/pds/pds_vdpa.h>
>
> #include "aux_drv.h"
> +#include "vdpa_dev.h"
> #include "pci_drv.h"
> #include "debugfs.h"
>
> @@ -25,10 +26,25 @@ static void
> pds_vdpa_aux_notify_handler(struct pds_auxiliary_dev *padev,
> union pds_core_notifyq_comp *event)
> {
> + struct pds_vdpa_device *pdsv = padev->priv;
> struct device *dev = &padev->aux_dev.dev;
> u16 ecode = le16_to_cpu(event->ecode);
>
> dev_info(dev, "%s: event code %d\n", __func__, ecode);
> +
> + /* Give the upper layers a hint that something interesting
> + * may have happened. It seems that the only thing this
> + * triggers in the virtio-net drivers above us is a check
> + * of link status.
> + *
> + * We don't set the NEEDS_RESET flag for EVENT_RESET
> + * because we're likely going through a recovery or
> + * fw_update and will be back up and running soon.
> + */
> + if (ecode == PDS_EVENT_RESET || ecode == PDS_EVENT_LINK_CHANGE) {
> + if (pdsv->hw.config_cb.callback)
> + pdsv->hw.config_cb.callback(pdsv->hw.config_cb.private);
> + }
> }
>
> static int
> @@ -80,10 +96,25 @@ pds_vdpa_aux_probe(struct auxiliary_device *aux_dev,
> goto err_register_client;
> }
>
> + /* Get device ident info and set up the vdpa_mgmt_dev */
> + err = pds_vdpa_get_mgmt_info(vdpa_aux);
> + if (err)
> + goto err_register_client;
> +
> + /* Let vdpa know that we can provide devices */
> + err = vdpa_mgmtdev_register(&vdpa_aux->vdpa_mdev);
> + if (err) {
> + dev_err(dev, "%s: Failed to initialize vdpa_mgmt interface: %pe\n",
> + __func__, ERR_PTR(err));
> + goto err_mgmt_reg;
> + }
> +
> pds_vdpa_debugfs_add_ident(vdpa_aux);
>
> return 0;
>
> +err_mgmt_reg:
> + padev->ops->unregister_client(padev);
> err_register_client:
> auxiliary_set_drvdata(aux_dev, NULL);
> err_invalid_driver:
> @@ -98,6 +129,8 @@ pds_vdpa_aux_remove(struct auxiliary_device *aux_dev)
> struct pds_vdpa_aux *vdpa_aux = auxiliary_get_drvdata(aux_dev);
> struct device *dev = &aux_dev->dev;
>
> + vdpa_mgmtdev_unregister(&vdpa_aux->vdpa_mdev);
> +
> vdpa_aux->padev->ops->unregister_client(vdpa_aux->padev);
> if (vdpa_aux->vdpa_vf)
> pci_dev_put(vdpa_aux->vdpa_vf->pdev);
> diff --git a/drivers/vdpa/pds/debugfs.c b/drivers/vdpa/pds/debugfs.c
> index f766412209df..aa3143126a7e 100644
> --- a/drivers/vdpa/pds/debugfs.c
> +++ b/drivers/vdpa/pds/debugfs.c
> @@ -11,6 +11,7 @@
> #include <linux/pds/pds_auxbus.h>
> #include <linux/pds/pds_vdpa.h>
>
> +#include "vdpa_dev.h"
> #include "aux_drv.h"
> #include "pci_drv.h"
> #include "debugfs.h"
> @@ -19,6 +20,72 @@
>
> static struct dentry *dbfs_dir;
>
> +#define PRINT_SBIT_NAME(__seq, __f, __name) \
> + do { \
> + if (__f & __name) \
> + seq_printf(__seq, " %s", &#__name[16]); \
> + } while (0)
> +
> +static void
> +print_status_bits(struct seq_file *seq, u16 status)
> +{
> + seq_puts(seq, "status:");
> + PRINT_SBIT_NAME(seq, status, VIRTIO_CONFIG_S_ACKNOWLEDGE);
> + PRINT_SBIT_NAME(seq, status, VIRTIO_CONFIG_S_DRIVER);
> + PRINT_SBIT_NAME(seq, status, VIRTIO_CONFIG_S_DRIVER_OK);
> + PRINT_SBIT_NAME(seq, status, VIRTIO_CONFIG_S_FEATURES_OK);
> + PRINT_SBIT_NAME(seq, status, VIRTIO_CONFIG_S_NEEDS_RESET);
> + PRINT_SBIT_NAME(seq, status, VIRTIO_CONFIG_S_FAILED);
> + seq_puts(seq, "\n");
> +}
> +
> +#define PRINT_FBIT_NAME(__seq, __f, __name) \
> + do { \
> + if (__f & BIT_ULL(__name)) \
> + seq_printf(__seq, " %s", #__name); \
> + } while (0)
> +
> +static void
> +print_feature_bits(struct seq_file *seq, u64 features)
> +{
> + seq_puts(seq, "features:");
> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_CSUM);
> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_GUEST_CSUM);
> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS);
> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_MTU);
> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_MAC);
> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_GUEST_TSO4);
> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_GUEST_TSO6);
> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_GUEST_ECN);
> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_GUEST_UFO);
> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_HOST_TSO4);
> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_HOST_TSO6);
> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_HOST_ECN);
> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_HOST_UFO);
> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_MRG_RXBUF);
> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_STATUS);
> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_CTRL_VQ);
> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_CTRL_RX);
> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_CTRL_VLAN);
> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_CTRL_RX_EXTRA);
> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_GUEST_ANNOUNCE);
> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_MQ);
> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_CTRL_MAC_ADDR);
> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_HASH_REPORT);
> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_RSS);
> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_RSC_EXT);
> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_STANDBY);
> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_SPEED_DUPLEX);
> + PRINT_FBIT_NAME(seq, features, VIRTIO_F_NOTIFY_ON_EMPTY);
> + PRINT_FBIT_NAME(seq, features, VIRTIO_F_ANY_LAYOUT);
> + PRINT_FBIT_NAME(seq, features, VIRTIO_F_VERSION_1);
> + PRINT_FBIT_NAME(seq, features, VIRTIO_F_ACCESS_PLATFORM);
> + PRINT_FBIT_NAME(seq, features, VIRTIO_F_RING_PACKED);
> + PRINT_FBIT_NAME(seq, features, VIRTIO_F_ORDER_PLATFORM);
> + PRINT_FBIT_NAME(seq, features, VIRTIO_F_SR_IOV);
> + seq_puts(seq, "\n");
> +}
> +
> void
> pds_vdpa_debugfs_create(void)
> {
> @@ -49,10 +116,18 @@ static int
> identity_show(struct seq_file *seq, void *v)
> {
> struct pds_vdpa_aux *vdpa_aux = seq->private;
> + struct vdpa_mgmt_dev *mgmt;
>
> seq_printf(seq, "aux_dev: %s\n",
> dev_name(&vdpa_aux->padev->aux_dev.dev));
>
> + mgmt = &vdpa_aux->vdpa_mdev;
> + seq_printf(seq, "max_vqs: %d\n", mgmt->max_supported_vqs);
> + seq_printf(seq, "config_attr_mask: %#llx\n", mgmt->config_attr_mask);
> + seq_printf(seq, "supported_features: %#llx\n", mgmt->supported_features);
> + print_feature_bits(seq, mgmt->supported_features);
> + seq_printf(seq, "local_mac_bit: %d\n", vdpa_aux->local_mac_bit);
> +
> return 0;
> }
> DEFINE_SHOW_ATTRIBUTE(identity);
> @@ -64,4 +139,96 @@ pds_vdpa_debugfs_add_ident(struct pds_vdpa_aux *vdpa_aux)
> vdpa_aux, &identity_fops);
> }
>
> +static int
> +config_show(struct seq_file *seq, void *v)
> +{
> + struct pds_vdpa_device *pdsv = seq->private;
> + struct virtio_net_config *vc = &pdsv->vn_config;
> +
> + seq_printf(seq, "mac: %pM\n", vc->mac);
> + seq_printf(seq, "max_virtqueue_pairs: %d\n",
> + __virtio16_to_cpu(true, vc->max_virtqueue_pairs));
> + seq_printf(seq, "mtu: %d\n", __virtio16_to_cpu(true, vc->mtu));
> + seq_printf(seq, "speed: %d\n", le32_to_cpu(vc->speed));
> + seq_printf(seq, "duplex: %d\n", vc->duplex);
> + seq_printf(seq, "rss_max_key_size: %d\n", vc->rss_max_key_size);
> + seq_printf(seq, "rss_max_indirection_table_length: %d\n",
> + le16_to_cpu(vc->rss_max_indirection_table_length));
> + seq_printf(seq, "supported_hash_types: %#x\n",
> + le32_to_cpu(vc->supported_hash_types));
> + seq_printf(seq, "vn_status: %#x\n",
> + __virtio16_to_cpu(true, vc->status));
> + print_status_bits(seq, __virtio16_to_cpu(true, vc->status));
> +
> + seq_printf(seq, "hw_status: %#x\n", pdsv->hw.status);
> + print_status_bits(seq, pdsv->hw.status);
> + seq_printf(seq, "req_features: %#llx\n", pdsv->hw.req_features);
> + print_feature_bits(seq, pdsv->hw.req_features);
> + seq_printf(seq, "actual_features: %#llx\n", pdsv->hw.actual_features);
> + print_feature_bits(seq, pdsv->hw.actual_features);
> + seq_printf(seq, "vdpa_index: %d\n", pdsv->hw.vdpa_index);
> + seq_printf(seq, "num_vqs: %d\n", pdsv->hw.num_vqs);
> +
> + return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(config);
> +
> +static int
> +vq_show(struct seq_file *seq, void *v)
> +{
> + struct pds_vdpa_vq_info *vq = seq->private;
> + struct pds_vdpa_intr_info *intrs;
> +
> + seq_printf(seq, "ready: %d\n", vq->ready);
> + seq_printf(seq, "desc_addr: %#llx\n", vq->desc_addr);
> + seq_printf(seq, "avail_addr: %#llx\n", vq->avail_addr);
> + seq_printf(seq, "used_addr: %#llx\n", vq->used_addr);
> + seq_printf(seq, "q_len: %d\n", vq->q_len);
> + seq_printf(seq, "qid: %d\n", vq->qid);
> +
> + seq_printf(seq, "doorbell: %#llx\n", vq->doorbell);
> + seq_printf(seq, "avail_idx: %d\n", vq->avail_idx);
> + seq_printf(seq, "used_idx: %d\n", vq->used_idx);
> + seq_printf(seq, "intr_index: %d\n", vq->intr_index);
> +
> + intrs = vq->pdsv->vdpa_aux->vdpa_vf->intrs;
> + seq_printf(seq, "irq: %d\n", intrs[vq->intr_index].irq);
> + seq_printf(seq, "irq-name: %s\n", intrs[vq->intr_index].name);
> +
> + seq_printf(seq, "hw_qtype: %d\n", vq->hw_qtype);
> + seq_printf(seq, "hw_qindex: %d\n", vq->hw_qindex);
> +
> + return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(vq);
> +
> +void
> +pds_vdpa_debugfs_add_vdpadev(struct pds_vdpa_device *pdsv)
> +{
> + struct dentry *dentry;
> + const char *name;
> + int i;
> +
> + dentry = pdsv->vdpa_aux->vdpa_vf->dentry;
> + name = dev_name(&pdsv->vdpa_dev.dev);
> +
> + pdsv->dentry = debugfs_create_dir(name, dentry);
> +
> + debugfs_create_file("config", 0400, pdsv->dentry, pdsv, &config_fops);
> +
> + for (i = 0; i < pdsv->hw.num_vqs; i++) {
> + char name[8];
> +
> + snprintf(name, sizeof(name), "vq%02d", i);
> + debugfs_create_file(name, 0400, pdsv->dentry, &pdsv->hw.vqs[i], &vq_fops);
> + }
> +}
> +
> +void
> +pds_vdpa_debugfs_del_vdpadev(struct pds_vdpa_device *pdsv)
> +{
> + debugfs_remove_recursive(pdsv->dentry);
> + pdsv->dentry = NULL;
> +}
> +
> #endif /* CONFIG_DEBUG_FS */
> diff --git a/drivers/vdpa/pds/debugfs.h b/drivers/vdpa/pds/debugfs.h
> index 939a4c248aac..f0567e4ee4e4 100644
> --- a/drivers/vdpa/pds/debugfs.h
> +++ b/drivers/vdpa/pds/debugfs.h
> @@ -13,12 +13,16 @@ void pds_vdpa_debugfs_destroy(void);
> void pds_vdpa_debugfs_add_pcidev(struct pds_vdpa_pci_device *vdpa_pdev);
> void pds_vdpa_debugfs_del_pcidev(struct pds_vdpa_pci_device *vdpa_pdev);
> void pds_vdpa_debugfs_add_ident(struct pds_vdpa_aux *vdpa_aux);
> +void pds_vdpa_debugfs_add_vdpadev(struct pds_vdpa_device *pdsv);
> +void pds_vdpa_debugfs_del_vdpadev(struct pds_vdpa_device *pdsv);
> #else
> static inline void pds_vdpa_debugfs_create(void) { }
> static inline void pds_vdpa_debugfs_destroy(void) { }
> static inline void pds_vdpa_debugfs_add_pcidev(struct pds_vdpa_pci_device *vdpa_pdev) { }
> static inline void pds_vdpa_debugfs_del_pcidev(struct pds_vdpa_pci_device *vdpa_pdev) { }
> static inline void pds_vdpa_debugfs_add_ident(struct pds_vdpa_aux *vdpa_aux) { }
> +static inline void pds_vdpa_debugfs_add_vdpadev(struct pds_vdpa_device *pdsv) { }
> +static inline void pds_vdpa_debugfs_del_vdpadev(struct pds_vdpa_device *pdsv) { }
> #endif
>
> #endif /* _PDS_VDPA_DEBUGFS_H_ */
> diff --git a/drivers/vdpa/pds/vdpa_dev.c b/drivers/vdpa/pds/vdpa_dev.c
> new file mode 100644
> index 000000000000..824be42aff0d
> --- /dev/null
> +++ b/drivers/vdpa/pds/vdpa_dev.c
> @@ -0,0 +1,796 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright(c) 2022 Pensando Systems, Inc */
> +
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/sysfs.h>
> +#include <linux/types.h>
> +#include <linux/vdpa.h>
> +#include <uapi/linux/virtio_pci.h>
> +#include <uapi/linux/vdpa.h>
> +
> +#include <linux/pds/pds_intr.h>
> +#include <linux/pds/pds_core_if.h>
> +#include <linux/pds/pds_adminq.h>
> +#include <linux/pds/pds_auxbus.h>
> +#include <linux/pds/pds_vdpa.h>
> +
> +#include "vdpa_dev.h"
> +#include "pci_drv.h"
> +#include "aux_drv.h"
> +#include "pci_drv.h"
> +#include "cmds.h"
> +#include "debugfs.h"
> +
> +static int
> +pds_vdpa_setup_driver(struct pds_vdpa_device *pdsv)
> +{
> + struct device *dev = &pdsv->vdpa_dev.dev;
> + int err = 0;
> + int i;
> +
> + /* Verify all vqs[] are in ready state */
> + for (i = 0; i < pdsv->hw.num_vqs; i++) {
> + if (!pdsv->hw.vqs[i].ready) {
> + dev_warn(dev, "%s: qid %d not ready\n", __func__, i);
> + err = -ENOENT;
> + }
> + }
> +
> + return err;
> +}
> +
> +static struct pds_vdpa_device *
> +vdpa_to_pdsv(struct vdpa_device *vdpa_dev)
> +{
> + return container_of(vdpa_dev, struct pds_vdpa_device, vdpa_dev);
> +}
> +
> +static struct pds_vdpa_hw *
> +vdpa_to_hw(struct vdpa_device *vdpa_dev)
> +{
> + struct pds_vdpa_device *pdsv = vdpa_to_pdsv(vdpa_dev);
> +
> + return &pdsv->hw;
> +}
> +
> +static int
> +pds_vdpa_set_vq_address(struct vdpa_device *vdpa_dev, u16 qid,
> + u64 desc_addr, u64 driver_addr, u64 device_addr)
> +{
> + struct pds_vdpa_hw *hw = vdpa_to_hw(vdpa_dev);
> +
> + hw->vqs[qid].desc_addr = desc_addr;
> + hw->vqs[qid].avail_addr = driver_addr;
> + hw->vqs[qid].used_addr = device_addr;
> +
> + return 0;
> +}
> +
> +static void
> +pds_vdpa_set_vq_num(struct vdpa_device *vdpa_dev, u16 qid, u32 num)
> +{
> + struct pds_vdpa_hw *hw = vdpa_to_hw(vdpa_dev);
> +
> + hw->vqs[qid].q_len = num;
> +}
> +
> +static void
> +pds_vdpa_kick_vq(struct vdpa_device *vdpa_dev, u16 qid)
> +{
> + struct pds_vdpa_device *pdsv = vdpa_to_pdsv(vdpa_dev);
> +
> + iowrite16(qid, pdsv->hw.vqs[qid].notify);
> +}
> +
> +static void
> +pds_vdpa_set_vq_cb(struct vdpa_device *vdpa_dev, u16 qid,
> + struct vdpa_callback *cb)
> +{
> + struct pds_vdpa_hw *hw = vdpa_to_hw(vdpa_dev);
> +
> + hw->vqs[qid].event_cb = *cb;
> +}
> +
> +static irqreturn_t
> +pds_vdpa_isr(int irq, void *data)
> +{
> + struct pds_core_intr __iomem *intr_ctrl;
> + struct pds_vdpa_device *pdsv;
> + struct pds_vdpa_vq_info *vq;
> +
> + vq = data;
> + pdsv = vq->pdsv;
> +
> + if (vq->event_cb.callback)
> + vq->event_cb.callback(vq->event_cb.private);
> +
> + /* Since we don't actually know how many vq descriptors are
> + * covered in this interrupt cycle, we simply clean all the
> + * credits and re-enable the interrupt.
> + */
> + intr_ctrl = (struct pds_core_intr __iomem *)pdsv->vdpa_aux->vdpa_vf->vd_mdev.isr;
> + pds_core_intr_clean_flags(&intr_ctrl[vq->intr_index],
> + PDS_CORE_INTR_CRED_REARM);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static void
> +pds_vdpa_release_irq(struct pds_vdpa_device *pdsv, int qid)
> +{
> + struct pds_vdpa_intr_info *intrs = pdsv->vdpa_aux->vdpa_vf->intrs;
> + struct pci_dev *pdev = pdsv->vdpa_aux->vdpa_vf->pdev;
> + struct pds_core_intr __iomem *intr_ctrl;
> + int index;
> +
> + intr_ctrl = (struct pds_core_intr __iomem *)pdsv->vdpa_aux->vdpa_vf->vd_mdev.isr;
> + index = pdsv->hw.vqs[qid].intr_index;
> + if (index == VIRTIO_MSI_NO_VECTOR)
> + return;
> +
> + if (intrs[index].irq == VIRTIO_MSI_NO_VECTOR)
> + return;
> +
> + if (qid & 0x1) {
> + pdsv->hw.vqs[qid].intr_index = VIRTIO_MSI_NO_VECTOR;
> + } else {
> + pds_core_intr_mask(&intr_ctrl[index], PDS_CORE_INTR_MASK_SET);
> + devm_free_irq(&pdev->dev, intrs[index].irq, &pdsv->hw.vqs[qid]);
> + pdsv->hw.vqs[qid].intr_index = VIRTIO_MSI_NO_VECTOR;
> + intrs[index].irq = VIRTIO_MSI_NO_VECTOR;
> + }
> +}
> +
> +static void
> +pds_vdpa_set_vq_ready(struct vdpa_device *vdpa_dev, u16 qid, bool ready)
> +{
> + struct pds_vdpa_device *pdsv = vdpa_to_pdsv(vdpa_dev);
> + struct pci_dev *pdev = pdsv->vdpa_aux->vdpa_vf->pdev;
> + struct pds_vdpa_hw *hw = vdpa_to_hw(vdpa_dev);
> + struct device *dev = &pdsv->vdpa_dev.dev;
> + struct pds_core_intr __iomem *intr_ctrl;
> + int err;
> +
> + dev_dbg(dev, "%s: qid %d ready %d => %d\n",
> + __func__, qid, hw->vqs[qid].ready, ready);
> + if (ready == hw->vqs[qid].ready)
> + return;
> +
> + intr_ctrl = (struct pds_core_intr __iomem *)pdsv->vdpa_aux->vdpa_vf->vd_mdev.isr;
It looks to me pds has a different layout/semantic for isr than virtio
spec. I'd suggest to not reuse spec isr here to avoid confusion.
> + if (ready) {
Spec said no interrupt before DRIVER_OK, it looks more simple if we
move the interrupt setup to set_status():
E.g we can know if we have sufficient vectors and use different
mapping policies in advance.
> + struct pds_vdpa_intr_info *intrs = pdsv->vdpa_aux->vdpa_vf->intrs;
> + int index = VIRTIO_MSI_NO_VECTOR;
> + int i;
> +
> + /* Tx and Rx queues share interrupts, and they start with
> + * even numbers, so only find an interrupt for the even numbered
> + * qid, and let the odd number use what the previous queue got.
> + */
> + if (qid & 0x1) {
> + int even = qid & ~0x1;
> +
> + index = hw->vqs[even].intr_index;
> + } else {
> + for (i = 0; i < pdsv->vdpa_aux->vdpa_vf->nintrs; i++) {
> + if (intrs[i].irq == VIRTIO_MSI_NO_VECTOR) {
> + index = i;
> + break;
> + }
> + }
> + }
> +
> + if (qid & 0x1) {
> + hw->vqs[qid].intr_index = index;
> + } else if (index != VIRTIO_MSI_NO_VECTOR) {
> + int irq;
> +
> + irq = pci_irq_vector(pdev, index);
> + snprintf(intrs[index].name, sizeof(intrs[index].name),
> + "vdpa-%s-%d", dev_name(dev), index);
> +
> + err = devm_request_irq(&pdev->dev, irq, pds_vdpa_isr, 0,
> + intrs[index].name, &hw->vqs[qid]);
> + if (err) {
> + dev_info(dev, "%s: no irq for qid %d: %pe\n",
> + __func__, qid, ERR_PTR(err));
Should we fail?
> + } else {
> + intrs[index].irq = irq;
> + hw->vqs[qid].intr_index = index;
> + pds_core_intr_mask(&intr_ctrl[index],
> + PDS_CORE_INTR_MASK_CLEAR);
I guess the reason that you don't simply use VF MSI-X is the DPU can
support vDPA subdevice in the future?
> + }
> + } else {
> + dev_info(dev, "%s: no intr slot for qid %d\n",
> + __func__, qid);
Do we need to fail here?
> + }
> +
> + /* Pass vq setup info to DSC */
> + err = pds_vdpa_cmd_init_vq(pdsv, qid, &hw->vqs[qid]);
> + if (err) {
> + pds_vdpa_release_irq(pdsv, qid);
> + ready = false;
> + }
> + } else {
> + pds_vdpa_release_irq(pdsv, qid);
> + (void) pds_vdpa_cmd_reset_vq(pdsv, qid);
> + }
> +
> + hw->vqs[qid].ready = ready;
> +}
> +
> +static bool
> +pds_vdpa_get_vq_ready(struct vdpa_device *vdpa_dev, u16 qid)
> +{
> + struct pds_vdpa_hw *hw = vdpa_to_hw(vdpa_dev);
> +
> + return hw->vqs[qid].ready;
> +}
> +
> +static int
> +pds_vdpa_set_vq_state(struct vdpa_device *vdpa_dev, u16 qid,
> + const struct vdpa_vq_state *state)
> +{
> + struct pds_vdpa_hw *hw = vdpa_to_hw(vdpa_dev);
> +
> + hw->vqs[qid].used_idx = state->split.avail_index;
> + hw->vqs[qid].avail_idx = state->split.avail_index;
> +
> + return 0;
> +}
> +
> +static int
> +pds_vdpa_get_vq_state(struct vdpa_device *vdpa_dev, u16 qid,
> + struct vdpa_vq_state *state)
> +{
> + struct pds_vdpa_hw *hw = vdpa_to_hw(vdpa_dev);
> +
> + state->split.avail_index = hw->vqs[qid].avail_idx;
Who is in charge of reading avail_idx from the hardware?
> +
> + return 0;
> +}
> +
> +static struct vdpa_notification_area
> +pds_vdpa_get_vq_notification(struct vdpa_device *vdpa_dev, u16 qid)
> +{
> + struct pds_vdpa_device *pdsv = vdpa_to_pdsv(vdpa_dev);
> + struct pds_vdpa_hw *hw = vdpa_to_hw(vdpa_dev);
> + struct virtio_pci_modern_device *vd_mdev;
> + struct vdpa_notification_area area;
> +
> + area.addr = hw->vqs[qid].notify_pa;
> +
> + vd_mdev = &pdsv->vdpa_aux->vdpa_vf->vd_mdev;
> + if (!vd_mdev->notify_offset_multiplier)
> + area.size = PAGE_SIZE;
> + else
> + area.size = vd_mdev->notify_offset_multiplier;
> +
> + return area;
> +}
> +
> +static int
> +pds_vdpa_get_vq_irq(struct vdpa_device *vdpa_dev, u16 qid)
> +{
> + struct pds_vdpa_device *pdsv = vdpa_to_pdsv(vdpa_dev);
> + struct pds_vdpa_hw *hw = vdpa_to_hw(vdpa_dev);
> + int irq = VIRTIO_MSI_NO_VECTOR;
> + int index;
> +
> + if (pdsv->vdpa_aux->vdpa_vf->intrs) {
> + index = hw->vqs[qid].intr_index;
> + irq = pdsv->vdpa_aux->vdpa_vf->intrs[index].irq;
The notification area mapping might only work well when each vq has
it's own irq. Otherwise guest may see spurious interrupt which may
degrade the performance.
> + }
> +
> + return irq;
> +}
> +
> +static u32
> +pds_vdpa_get_vq_align(struct vdpa_device *vdpa_dev)
> +{
> +
> + return PAGE_SIZE;
> +}
> +
> +static u32
> +pds_vdpa_get_vq_group(struct vdpa_device *vdpa_dev, u16 idx)
> +{
> + return 0;
> +}
> +
> +static u64
> +pds_vdpa_get_device_features(struct vdpa_device *vdpa_dev)
> +{
> + struct pds_vdpa_device *pdsv = vdpa_to_pdsv(vdpa_dev);
> +
> + return le64_to_cpu(pdsv->vdpa_aux->ident.hw_features);
> +}
> +
> +static int
> +pds_vdpa_set_driver_features(struct vdpa_device *vdpa_dev, u64 features)
> +{
> + struct pds_vdpa_device *pdsv = vdpa_to_pdsv(vdpa_dev);
> + struct pds_vdpa_hw *hw = vdpa_to_hw(vdpa_dev);
> + struct device *dev = &pdsv->vdpa_dev.dev;
> + u64 nego_features;
> + u64 set_features;
> + u64 missing;
> + int err;
> +
> + if (!(features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM)) && features) {
> + dev_err(dev, "VIRTIO_F_ACCESS_PLATFORM is not negotiated\n");
> + return -EOPNOTSUPP;
Should we fail the FEATURE_OK in this case and all the other below
error conditions?
> + }
> +
> + hw->req_features = features;
> +
> + /* Check for valid feature bits */
> + nego_features = features & le64_to_cpu(pdsv->vdpa_aux->ident.hw_features);
> + missing = hw->req_features & ~nego_features;
> + if (missing) {
> + dev_err(dev, "Can't support all requested features in %#llx, missing %#llx features\n",
> + hw->req_features, missing);
> + return -EOPNOTSUPP;
> + }
> +
> + dev_dbg(dev, "%s: %#llx => %#llx\n",
> + __func__, hw->actual_features, nego_features);
> +
> + if (hw->actual_features == nego_features)
> + return 0;
> +
> + /* Update hw feature configuration, strip MAC bit if locally set */
> + if (pdsv->vdpa_aux->local_mac_bit)
> + set_features = nego_features & ~BIT_ULL(VIRTIO_NET_F_MAC);
Need some document to explain how local_mac_bit work.
> + else
> + set_features = nego_features;
> + err = pds_vdpa_cmd_set_features(pdsv, set_features);
> + if (!err)
> + hw->actual_features = nego_features;
> +
> + return err;
> +}
> +
> +static u64
> +pds_vdpa_get_driver_features(struct vdpa_device *vdpa_dev)
> +{
> + struct pds_vdpa_hw *hw = vdpa_to_hw(vdpa_dev);
> +
> + return hw->actual_features;
> +}
> +
> +static void
> +pds_vdpa_set_config_cb(struct vdpa_device *vdpa_dev, struct vdpa_callback *cb)
> +{
> + struct pds_vdpa_hw *hw = vdpa_to_hw(vdpa_dev);
> +
> + hw->config_cb.callback = cb->callback;
> + hw->config_cb.private = cb->private;
> +}
> +
> +static u16
> +pds_vdpa_get_vq_num_max(struct vdpa_device *vdpa_dev)
> +{
> + struct pds_vdpa_device *pdsv = vdpa_to_pdsv(vdpa_dev);
> + u32 max_qlen;
> +
> + max_qlen = min_t(u32, PDS_VDPA_MAX_QLEN,
> + 1 << le16_to_cpu(pdsv->vdpa_aux->ident.max_qlen));
Assuming we can fetch the max_qlen from the device, any reason have
another layer like PDS_VDPA_MAX_QLEN?
> +
> + return (u16)max_qlen;
> +}
> +
> +static u32
> +pds_vdpa_get_device_id(struct vdpa_device *vdpa_dev)
> +{
> + return VIRTIO_ID_NET;
> +}
> +
> +static u32
> +pds_vdpa_get_vendor_id(struct vdpa_device *vdpa_dev)
> +{
> + return PCI_VENDOR_ID_PENSANDO;
> +}
> +
> +static u8
> +pds_vdpa_get_status(struct vdpa_device *vdpa_dev)
> +{
> + struct pds_vdpa_hw *hw = vdpa_to_hw(vdpa_dev);
> +
> + return hw->status;
How is this synchronized with the device or it is fully emulated by this driver?
> +}
> +
> +static void
> +pds_vdpa_set_status(struct vdpa_device *vdpa_dev, u8 status)
> +{
> + struct pds_vdpa_device *pdsv = vdpa_to_pdsv(vdpa_dev);
> + struct pds_vdpa_hw *hw = vdpa_to_hw(vdpa_dev);
> + struct device *dev = &pdsv->vdpa_dev.dev;
> + int err;
> +
> + if (hw->status == status)
> + return;
> +
> + /* If the DRIVER_OK bit turns on, time to start the queues */
> + if ((status ^ hw->status) & VIRTIO_CONFIG_S_DRIVER_OK) {
> + if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
> + err = pds_vdpa_setup_driver(pdsv);
> + if (err) {
> + dev_err(dev, "failed to setup driver: %pe\n", ERR_PTR(err));
> + status = hw->status | VIRTIO_CONFIG_S_FAILED;
> + }
> + } else {
> + dev_warn(dev, "did not expect DRIVER_OK to be cleared\n");
> + }
> + }
> +
> + err = pds_vdpa_cmd_set_status(pdsv, status);
> + if (!err)
> + hw->status = status;
> +}
> +
> +static int
> +pds_vdpa_reset(struct vdpa_device *vdpa_dev)
> +{
> + struct pds_vdpa_device *pdsv = vdpa_to_pdsv(vdpa_dev);
> + struct pds_vdpa_hw *hw = vdpa_to_hw(vdpa_dev);
> + int i;
> +
> + if (hw->status == 0)
> + return 0;
> +
> + if (hw->status & VIRTIO_CONFIG_S_DRIVER_OK) {
> +
> + /* Reset the vqs */
> + for (i = 0; i < hw->num_vqs; i++) {
> + pds_vdpa_release_irq(pdsv, i);
> + (void) pds_vdpa_cmd_reset_vq(pdsv, i);
(void) is unnecessary.
> +
> + memset(&pdsv->hw.vqs[i], 0, sizeof(pdsv->hw.vqs[0]));
> + pdsv->hw.vqs[i].ready = false;
> + }
> + }
> +
> + hw->status = 0;
> + (void) pds_vdpa_cmd_set_status(pdsv, 0);
> +
> + return 0;
> +}
> +
> +static size_t
> +pds_vdpa_get_config_size(struct vdpa_device *vdpa_dev)
> +{
> + return sizeof(struct virtio_net_config);
> +}
> +
> +static void
> +pds_vdpa_get_config(struct vdpa_device *vdpa_dev,
> + unsigned int offset,
> + void *buf, unsigned int len)
> +{
> + struct pds_vdpa_device *pdsv = vdpa_to_pdsv(vdpa_dev);
> +
> + if (offset + len <= sizeof(struct virtio_net_config))
> + memcpy(buf, (u8 *)&pdsv->vn_config + offset, len);
> +}
> +
> +static void
> +pds_vdpa_set_config(struct vdpa_device *vdpa_dev,
> + unsigned int offset, const void *buf,
> + unsigned int len)
> +{
> + /* In the virtio_net context, this callback seems to only be
> + * called in drivers supporting the older non-VERSION_1 API,
> + * so we can leave this an empty function, but we need to
> + * define the function in case it does get called, as there
> + * are currently no checks for existence before calling in
> + * that path.
> + *
> + * The implementation would be something like:
> + * if (offset + len <= sizeof(struct virtio_net_config))
> + * memcpy((u8 *)&pdsv->vn_config + offset, buf, len);
> + */
And we need to notify the hardware that config has been changed.
> +}
> +
> +static const struct vdpa_config_ops pds_vdpa_ops = {
> + .set_vq_address = pds_vdpa_set_vq_address,
> + .set_vq_num = pds_vdpa_set_vq_num,
> + .kick_vq = pds_vdpa_kick_vq,
> + .set_vq_cb = pds_vdpa_set_vq_cb,
> + .set_vq_ready = pds_vdpa_set_vq_ready,
> + .get_vq_ready = pds_vdpa_get_vq_ready,
> + .set_vq_state = pds_vdpa_set_vq_state,
> + .get_vq_state = pds_vdpa_get_vq_state,
> + .get_vq_notification = pds_vdpa_get_vq_notification,
> + .get_vq_irq = pds_vdpa_get_vq_irq,
> + .get_vq_align = pds_vdpa_get_vq_align,
> + .get_vq_group = pds_vdpa_get_vq_group,
> +
> + .get_device_features = pds_vdpa_get_device_features,
> + .set_driver_features = pds_vdpa_set_driver_features,
> + .get_driver_features = pds_vdpa_get_driver_features,
> + .set_config_cb = pds_vdpa_set_config_cb,
> + .get_vq_num_max = pds_vdpa_get_vq_num_max,
> +/* .get_vq_num_min (optional) */
> + .get_device_id = pds_vdpa_get_device_id,
> + .get_vendor_id = pds_vdpa_get_vendor_id,
> + .get_status = pds_vdpa_get_status,
> + .set_status = pds_vdpa_set_status,
> + .reset = pds_vdpa_reset,
> + .get_config_size = pds_vdpa_get_config_size,
> + .get_config = pds_vdpa_get_config,
> + .set_config = pds_vdpa_set_config,
> +
> +/* .get_generation (optional) */
> +/* .get_iova_range (optional) */
> +/* .set_group_asid */
> +/* .set_map (optional) */
> +/* .dma_map (optional) */
> +/* .dma_unmap (optional) */
> +/* .free (optional) */
> +};
> +static struct virtio_device_id pds_vdpa_id_table[] = {
> + {VIRTIO_ID_NET, VIRTIO_DEV_ANY_ID},
> + {0},
> +};
> +
> +static int
> +pds_vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name,
> + const struct vdpa_dev_set_config *add_config)
> +{
> + struct pds_vdpa_aux *vdpa_aux;
> + struct pds_vdpa_device *pdsv;
> + struct vdpa_mgmt_dev *mgmt;
> + u16 fw_max_vqs, vq_pairs;
> + struct device *dma_dev;
> + struct pds_vdpa_hw *hw;
> + struct pci_dev *pdev;
> + struct device *dev;
> + u8 mac[ETH_ALEN];
> + int err;
> + int i;
> +
> + vdpa_aux = container_of(mdev, struct pds_vdpa_aux, vdpa_mdev);
> + dev = &vdpa_aux->padev->aux_dev.dev;
> + mgmt = &vdpa_aux->vdpa_mdev;
> +
> + if (vdpa_aux->pdsv) {
> + dev_warn(dev, "Multiple vDPA devices on a VF is not supported.\n");
> + return -EOPNOTSUPP;
> + }
> +
> + pdsv = vdpa_alloc_device(struct pds_vdpa_device, vdpa_dev,
> + dev, &pds_vdpa_ops, 1, 1, name, false);
> + if (IS_ERR(pdsv)) {
> + dev_err(dev, "Failed to allocate vDPA structure: %pe\n", pdsv);
> + return PTR_ERR(pdsv);
> + }
> +
> + vdpa_aux->pdsv = pdsv;
> + pdsv->vdpa_aux = vdpa_aux;
> + pdsv->vdpa_aux->padev->priv = pdsv;
> +
> + pdev = vdpa_aux->vdpa_vf->pdev;
> + pdsv->vdpa_dev.dma_dev = &pdev->dev;
> + dma_dev = pdsv->vdpa_dev.dma_dev;
> + hw = &pdsv->hw;
> +
> + pdsv->vn_config_pa = dma_map_single(dma_dev, &pdsv->vn_config,
> + sizeof(pdsv->vn_config), DMA_FROM_DEVICE);
I think we should use coherent mapping instead of streaming mapping
otherwise we may end up with coherency issues when accessing the
device configuration space.
> + if (dma_mapping_error(dma_dev, pdsv->vn_config_pa)) {
> + dev_err(dma_dev, "Failed to map vn_config space\n");
> + pdsv->vn_config_pa = 0;
> + err = -ENOMEM;
> + goto err_out;
> + }
> +
> + err = pds_vdpa_init_hw(pdsv);
> + if (err) {
> + dev_err(dev, "Failed to init hw: %pe\n", ERR_PTR(err));
> + goto err_unmap;
> + }
> +
> + fw_max_vqs = le16_to_cpu(pdsv->vdpa_aux->ident.max_vqs);
> + vq_pairs = fw_max_vqs / 2;
> +
> + /* Make sure we have the queues being requested */
> + if (add_config->mask & (1 << VDPA_ATTR_DEV_NET_CFG_MAX_VQP))
> + vq_pairs = add_config->net.max_vq_pairs;
> +
> + hw->num_vqs = 2 * vq_pairs;
> + if (mgmt->supported_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ))
> + hw->num_vqs++;
> +
> + if (hw->num_vqs > fw_max_vqs) {
> + dev_err(dev, "%s: queue count requested %u greater than max %u\n",
> + __func__, hw->num_vqs, fw_max_vqs);
> + err = -ENOSPC;
> + goto err_unmap;
> + }
> +
> + if (hw->num_vqs != fw_max_vqs) {
> + err = pds_vdpa_cmd_set_max_vq_pairs(pdsv, vq_pairs);
> + if (err == -ERANGE) {
> + hw->num_vqs = fw_max_vqs;
> + dev_warn(dev, "Known FW issue - overriding to use max_vq_pairs %d\n",
> + hw->num_vqs / 2);
Should we fail here? Since the device has a different max_vqp that expected.
> + } else if (err) {
> + dev_err(dev, "Failed to update max_vq_pairs: %pe\n",
> + ERR_PTR(err));
> + goto err_unmap;
> + }
> + }
> +
> + /* Set a mac, either from the user config if provided
> + * or set a random mac if default is 00:..:00
> + */
> + if (add_config->mask & (1 << VDPA_ATTR_DEV_NET_CFG_MACADDR)) {
> + ether_addr_copy(mac, add_config->net.mac);
> + pds_vdpa_cmd_set_mac(pdsv, mac);
> + } else if (is_zero_ether_addr(pdsv->vn_config.mac)) {
> + eth_random_addr(mac);
> + pds_vdpa_cmd_set_mac(pdsv, mac);
> + }
> +
> + for (i = 0; i < hw->num_vqs; i++) {
> + hw->vqs[i].qid = i;
> + hw->vqs[i].pdsv = pdsv;
> + hw->vqs[i].intr_index = VIRTIO_MSI_NO_VECTOR;
Let's rename this as msix_vector to be aligned with the virtio spec.
> + hw->vqs[i].notify = vp_modern_map_vq_notify(&pdsv->vdpa_aux->vdpa_vf->vd_mdev,
> + i, &hw->vqs[i].notify_pa);
> + }
> +
> + pdsv->vdpa_dev.mdev = &vdpa_aux->vdpa_mdev;
> +
> + /* We use the _vdpa_register_device() call rather than the
> + * vdpa_register_device() to avoid a deadlock because this
> + * dev_add() is called with the vdpa_dev_lock already set
> + * by vdpa_nl_cmd_dev_add_set_doit()
> + */
> + err = _vdpa_register_device(&pdsv->vdpa_dev, hw->num_vqs);
> + if (err) {
> + dev_err(dev, "Failed to register to vDPA bus: %pe\n", ERR_PTR(err));
> + goto err_unmap;
> + }
> +
> + pds_vdpa_debugfs_add_vdpadev(pdsv);
> + dev_info(&pdsv->vdpa_dev.dev, "Added with mac %pM\n", pdsv->vn_config.mac);
dev_dbg?
> +
> + return 0;
> +
> +err_unmap:
> + dma_unmap_single(dma_dev, pdsv->vn_config_pa,
> + sizeof(pdsv->vn_config), DMA_FROM_DEVICE);
> +err_out:
> + put_device(&pdsv->vdpa_dev.dev);
> + vdpa_aux->pdsv = NULL;
> + return err;
> +}
> +
> +static void
> +pds_vdpa_dev_del(struct vdpa_mgmt_dev *mdev, struct vdpa_device *vdpa_dev)
> +{
> + struct pds_vdpa_device *pdsv = vdpa_to_pdsv(vdpa_dev);
> + struct pds_vdpa_aux *vdpa_aux;
> +
> + dev_info(&vdpa_dev->dev, "Removed\n");
> +
> + vdpa_aux = container_of(mdev, struct pds_vdpa_aux, vdpa_mdev);
> + _vdpa_unregister_device(vdpa_dev);
> + pds_vdpa_debugfs_del_vdpadev(pdsv);
> +
> + if (vdpa_aux->pdsv->vn_config_pa)
> + dma_unmap_single(vdpa_dev->dma_dev, vdpa_aux->pdsv->vn_config_pa,
> + sizeof(vdpa_aux->pdsv->vn_config), DMA_FROM_DEVICE);
> +
> + vdpa_aux->pdsv = NULL;
> +}
> +
> +static const struct vdpa_mgmtdev_ops pds_vdpa_mgmt_dev_ops = {
> + .dev_add = pds_vdpa_dev_add,
> + .dev_del = pds_vdpa_dev_del
> +};
> +
> +int
> +pds_vdpa_get_mgmt_info(struct pds_vdpa_aux *vdpa_aux)
> +{
> + struct pds_vdpa_pci_device *vdpa_pdev;
> + struct pds_vdpa_ident_cmd ident_cmd = {
> + .opcode = PDS_VDPA_CMD_IDENT,
> + .vf_id = cpu_to_le16(vdpa_aux->vdpa_vf->vf_id),
> + };
> + struct pds_vdpa_comp ident_comp = {0};
> + struct vdpa_mgmt_dev *mgmt;
> + struct device *dma_dev;
> + dma_addr_t ident_pa;
> + struct pci_dev *pdev;
> + struct device *dev;
> + __le64 mac_bit;
> + u16 max_vqs;
> + int err;
> + int i;
> +
> + vdpa_pdev = vdpa_aux->vdpa_vf;
> + pdev = vdpa_pdev->pdev;
> + dev = &vdpa_aux->padev->aux_dev.dev;
> + mgmt = &vdpa_aux->vdpa_mdev;
> +
> + /* Get resource info from the device */
> + dma_dev = &pdev->dev;
> + ident_pa = dma_map_single(dma_dev, &vdpa_aux->ident,
> + sizeof(vdpa_aux->ident), DMA_FROM_DEVICE);
I wonder how this work. The ident_pa is mapped through VF, but the
command is sent to PF adminq if I understand correctly. If yes, this
might work but looks tricky. We'd better explain this is safe since
vDPA is not yet created so no userspace can use that. Or I wonder if
we can just piggyback the ident via the adminq response so we don't
need to worry the security implications.
Thanks
> + if (dma_mapping_error(dma_dev, ident_pa)) {
> + dev_err(dma_dev, "Failed to map ident space\n");
> + return -ENOMEM;
> + }
> +
> + ident_cmd.ident_pa = cpu_to_le64(ident_pa);
> + ident_cmd.len = cpu_to_le32(sizeof(vdpa_aux->ident));
> + err = vdpa_aux->padev->ops->adminq_cmd(vdpa_aux->padev,
> + (union pds_core_adminq_cmd *)&ident_cmd,
> + sizeof(ident_cmd),
> + (union pds_core_adminq_comp *)&ident_comp,
> + 0);
> + dma_unmap_single(dma_dev, ident_pa,
> + sizeof(vdpa_aux->ident), DMA_FROM_DEVICE);
> + if (err) {
> + dev_err(dev, "Failed to ident hw, status %d: %pe\n",
> + ident_comp.status, ERR_PTR(err));
> + return err;
> + }
> +
> + /* The driver adds a default mac address if the device doesn't,
> + * so we need to sure we advertise VIRTIO_NET_F_MAC
> + */
> + mac_bit = cpu_to_le64(BIT_ULL(VIRTIO_NET_F_MAC));
> + if (!(vdpa_aux->ident.hw_features & mac_bit)) {
> + vdpa_aux->ident.hw_features |= mac_bit;
> + vdpa_aux->local_mac_bit = true;
> + }
> +
> + max_vqs = le16_to_cpu(vdpa_aux->ident.max_vqs);
> + mgmt->max_supported_vqs = min_t(u16, PDS_VDPA_MAX_QUEUES, max_vqs);
> + if (max_vqs > PDS_VDPA_MAX_QUEUES)
> + dev_info(dev, "FYI - Device supports more vqs (%d) than driver (%d)\n",
> + max_vqs, PDS_VDPA_MAX_QUEUES);
> +
> + mgmt->ops = &pds_vdpa_mgmt_dev_ops;
> + mgmt->id_table = pds_vdpa_id_table;
> + mgmt->device = dev;
> + mgmt->supported_features = le64_to_cpu(vdpa_aux->ident.hw_features);
> + mgmt->config_attr_mask = BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR);
> + mgmt->config_attr_mask |= BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP);
> +
> + /* Set up interrupts now that we know how many we might want
> + * TX and RX pairs will share interrupts, so halve the vq count
> + * Add another for a control queue if supported
> + */
> + vdpa_pdev->nintrs = mgmt->max_supported_vqs / 2;
> + if (mgmt->supported_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ))
> + vdpa_pdev->nintrs++;
> +
> + err = pci_alloc_irq_vectors(pdev, vdpa_pdev->nintrs, vdpa_pdev->nintrs,
> + PCI_IRQ_MSIX);
> + if (err < 0) {
> + dev_err(dma_dev, "Couldn't get %d msix vectors: %pe\n",
> + vdpa_pdev->nintrs, ERR_PTR(err));
> + return err;
> + }
> + vdpa_pdev->nintrs = err;
> + err = 0;
> +
> + vdpa_pdev->intrs = devm_kcalloc(&pdev->dev, vdpa_pdev->nintrs,
> + sizeof(*vdpa_pdev->intrs),
> + GFP_KERNEL);
> + if (!vdpa_pdev->intrs) {
> + vdpa_pdev->nintrs = 0;
> + pci_free_irq_vectors(pdev);
> + return -ENOMEM;
> + }
> +
> + for (i = 0; i < vdpa_pdev->nintrs; i++)
> + vdpa_pdev->intrs[i].irq = VIRTIO_MSI_NO_VECTOR;
> +
> + return 0;
> +}
> --
> 2.17.1
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH net-next 19/19] pds_vdpa: add Kconfig entry and pds_vdpa.rst
[not found] ` <20221118225656.48309-20-snelson@pensando.io>
@ 2022-11-22 6:35 ` Jason Wang
0 siblings, 0 replies; 13+ messages in thread
From: Jason Wang @ 2022-11-22 6:35 UTC (permalink / raw)
To: Shannon Nelson; +Cc: mst, netdev, virtualization, kuba, drivers, davem
On Sat, Nov 19, 2022 at 6:57 AM Shannon Nelson <snelson@pensando.io> wrote:
>
> Signed-off-by: Shannon Nelson <snelson@pensando.io>
> ---
> .../ethernet/pensando/pds_vdpa.rst | 85 +++++++++++++++++++
> MAINTAINERS | 1 +
> drivers/vdpa/Kconfig | 7 ++
> 3 files changed, 93 insertions(+)
> create mode 100644 Documentation/networking/device_drivers/ethernet/pensando/pds_vdpa.rst
>
> diff --git a/Documentation/networking/device_drivers/ethernet/pensando/pds_vdpa.rst b/Documentation/networking/device_drivers/ethernet/pensando/pds_vdpa.rst
> new file mode 100644
> index 000000000000..c517f337d212
> --- /dev/null
> +++ b/Documentation/networking/device_drivers/ethernet/pensando/pds_vdpa.rst
> @@ -0,0 +1,85 @@
> +.. SPDX-License-Identifier: GPL-2.0+
> +.. note: can be edited and viewed with /usr/bin/formiko-vim
> +
> +==========================================================
> +PCI vDPA driver for the Pensando(R) DSC adapter family
> +==========================================================
> +
> +Pensando vDPA VF Device Driver
> +Copyright(c) 2022 Pensando Systems, Inc
> +
> +Overview
> +========
> +
> +The ``pds_vdpa`` driver is a PCI and auxiliary bus driver and supplies
> +a vDPA device for use by the virtio network stack. It is used with
> +the Pensando Virtual Function devices that offer vDPA and virtio queue
> +services. It depends on the ``pds_core`` driver and hardware for the PF
> +and for device configuration services.
> +
> +Using the device
> +================
> +
> +The ``pds_vdpa`` device is enabled via multiple configuration steps and
> +depends on the ``pds_core`` driver to create and enable SR-IOV Virtual
> +Function devices.
> +
> +Shown below are the steps to bind the driver to a VF and also to the
> +associated auxiliary device created by the ``pds_core`` driver. This
> +example assumes the pds_core and pds_vdpa modules are already
> +loaded.
> +
> +.. code-block:: bash
> +
> + #!/bin/bash
> +
> + modprobe pds_core
> + modprobe pds_vdpa
> +
> + PF_BDF=`grep "vDPA.*1" /sys/kernel/debug/pds_core/*/viftypes | head -1 | awk -F / '{print $6}'`
> +
> + # Enable vDPA VF auxiliary device(s) in the PF
> + devlink dev param set pci/$PF_BDF name enable_vnet value true cmode runtime
> +
> + # Create a VF for vDPA use
> + echo 1 > /sys/bus/pci/drivers/pds_core/$PF_BDF/sriov_numvfs
> +
> + # Find the vDPA services/devices available
> + PDS_VDPA_MGMT=`vdpa mgmtdev show | grep vDPA | head -1 | cut -d: -f1`
> +
> + # Create a vDPA device for use in virtio network configurations
> + vdpa dev add name vdpa1 mgmtdev $PDS_VDPA_MGMT mac 00:11:22:33:44:55
> +
> + # Set up an ethernet interface on the vdpa device
> + modprobe virtio_vdpa
> +
> +
> +
> +Enabling the driver
> +===================
> +
> +The driver is enabled via the standard kernel configuration system,
> +using the make command::
> +
> + make oldconfig/menuconfig/etc.
> +
> +The driver is located in the menu structure at:
> +
> + -> Device Drivers
> + -> Network device support (NETDEVICES [=y])
> + -> Ethernet driver support
> + -> Pensando devices
> + -> Pensando Ethernet PDS_VDPA Support
> +
> +Support
> +=======
> +
> +For general Linux networking support, please use the netdev mailing
> +list, which is monitored by Pensando personnel::
> +
> + netdev@vger.kernel.org
> +
> +For more specific support needs, please use the Pensando driver support
> +email::
> +
> + drivers@pensando.io
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a4f989fa8192..a4d96e854757 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -16152,6 +16152,7 @@ L: netdev@vger.kernel.org
> S: Supported
> F: Documentation/networking/device_drivers/ethernet/pensando/
> F: drivers/net/ethernet/pensando/
> +F: drivers/vdpa/pds/
> F: include/linux/pds/
>
> PER-CPU MEMORY ALLOCATOR
> diff --git a/drivers/vdpa/Kconfig b/drivers/vdpa/Kconfig
> index 50f45d037611..1c44df18f3da 100644
> --- a/drivers/vdpa/Kconfig
> +++ b/drivers/vdpa/Kconfig
> @@ -86,4 +86,11 @@ config ALIBABA_ENI_VDPA
> VDPA driver for Alibaba ENI (Elastic Network Interface) which is built upon
> virtio 0.9.5 specification.
>
> +config PDS_VDPA
> + tristate "vDPA driver for Pensando DSC devices"
> + select VHOST_RING
Any reason it needs to select on vringh?
Thanks
> + depends on PDS_CORE
> + help
> + VDPA network driver for Pensando's PDS Core devices.
> +
> endif # VDPA
> --
> 2.17.1
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH net-next 15/19] pds_vdpa: virtio bar setup for vdpa
2022-11-22 3:32 ` [RFC PATCH net-next 15/19] pds_vdpa: virtio bar setup for vdpa Jason Wang
@ 2022-11-22 6:36 ` Jason Wang
0 siblings, 0 replies; 13+ messages in thread
From: Jason Wang @ 2022-11-22 6:36 UTC (permalink / raw)
To: Shannon Nelson, netdev, davem, kuba, mst, virtualization; +Cc: drivers
On Tue, Nov 22, 2022 at 11:32 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 在 2022/11/19 06:56, Shannon Nelson 写道:
> > The PDS vDPA device has a virtio BAR for describing itself, and
> > the pds_vdpa driver needs to access it. Here we copy liberally
> > from the existing drivers/virtio/virtio_pci_modern_dev.c as it
> > has what we need, but we need to modify it so that it can work
> > with our device id and so we can use our own DMA mask.
> >
> > We suspect there is room for discussion here about making the
> > existing code a little more flexible, but we thought we'd at
> > least start the discussion here.
>
>
> Exactly, since the virtio_pci_modern_dev.c is a library, we could tweak
> it to allow the caller to pass the device_id with the DMA mask. Then we
> can avoid code/bug duplication here.
Btw, I found only isr/notification were used but not the others? If
this is true, we can avoid mapping those capabilities.
Thanks
>
> Thanks
>
>
> >
> > Signed-off-by: Shannon Nelson <snelson@pensando.io>
> > ---
> > drivers/vdpa/pds/Makefile | 3 +-
> > drivers/vdpa/pds/pci_drv.c | 10 ++
> > drivers/vdpa/pds/pci_drv.h | 2 +
> > drivers/vdpa/pds/virtio_pci.c | 283 ++++++++++++++++++++++++++++++++++
> > 4 files changed, 297 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/vdpa/pds/virtio_pci.c
> >
> > diff --git a/drivers/vdpa/pds/Makefile b/drivers/vdpa/pds/Makefile
> > index 3ba28a875574..b8376ab165bc 100644
> > --- a/drivers/vdpa/pds/Makefile
> > +++ b/drivers/vdpa/pds/Makefile
> > @@ -4,4 +4,5 @@
> > obj-$(CONFIG_PDS_VDPA) := pds_vdpa.o
> >
> > pds_vdpa-y := pci_drv.o \
> > - debugfs.o
> > + debugfs.o \
> > + virtio_pci.o
> > diff --git a/drivers/vdpa/pds/pci_drv.c b/drivers/vdpa/pds/pci_drv.c
> > index 369e11153f21..10491e22778c 100644
> > --- a/drivers/vdpa/pds/pci_drv.c
> > +++ b/drivers/vdpa/pds/pci_drv.c
> > @@ -44,6 +44,14 @@ pds_vdpa_pci_probe(struct pci_dev *pdev,
> > goto err_out_free_mem;
> > }
> >
> > + vdpa_pdev->vd_mdev.pci_dev = pdev;
> > + err = pds_vdpa_probe_virtio(&vdpa_pdev->vd_mdev);
> > + if (err) {
> > + dev_err(dev, "Unable to probe for virtio configuration: %pe\n",
> > + ERR_PTR(err));
> > + goto err_out_free_mem;
> > + }
> > +
> > pci_enable_pcie_error_reporting(pdev);
> >
> > /* Use devres management */
> > @@ -74,6 +82,7 @@ pds_vdpa_pci_probe(struct pci_dev *pdev,
> > err_out_pci_release_device:
> > pci_disable_device(pdev);
> > err_out_free_mem:
> > + pds_vdpa_remove_virtio(&vdpa_pdev->vd_mdev);
> > pci_disable_pcie_error_reporting(pdev);
> > kfree(vdpa_pdev);
> > return err;
> > @@ -88,6 +97,7 @@ pds_vdpa_pci_remove(struct pci_dev *pdev)
> > pci_clear_master(pdev);
> > pci_disable_pcie_error_reporting(pdev);
> > pci_disable_device(pdev);
> > + pds_vdpa_remove_virtio(&vdpa_pdev->vd_mdev);
> > kfree(vdpa_pdev);
> >
> > dev_info(&pdev->dev, "Removed\n");
> > diff --git a/drivers/vdpa/pds/pci_drv.h b/drivers/vdpa/pds/pci_drv.h
> > index 747809e0df9e..15f3b34fafa9 100644
> > --- a/drivers/vdpa/pds/pci_drv.h
> > +++ b/drivers/vdpa/pds/pci_drv.h
> > @@ -43,4 +43,6 @@ struct pds_vdpa_pci_device {
> > struct virtio_pci_modern_device vd_mdev;
> > };
> >
> > +int pds_vdpa_probe_virtio(struct virtio_pci_modern_device *mdev);
> > +void pds_vdpa_remove_virtio(struct virtio_pci_modern_device *mdev);
> > #endif /* _PCI_DRV_H */
> > diff --git a/drivers/vdpa/pds/virtio_pci.c b/drivers/vdpa/pds/virtio_pci.c
> > new file mode 100644
> > index 000000000000..0f4ac9467199
> > --- /dev/null
> > +++ b/drivers/vdpa/pds/virtio_pci.c
> > @@ -0,0 +1,283 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +
> > +/*
> > + * adapted from drivers/virtio/virtio_pci_modern_dev.c, v6.0-rc1
> > + */
> > +
> > +#include <linux/virtio_pci_modern.h>
> > +#include <linux/module.h>
> > +#include <linux/pci.h>
> > +#include <linux/delay.h>
> > +
> > +#include "pci_drv.h"
> > +
> > +/*
> > + * pds_vdpa_map_capability - map a part of virtio pci capability
> > + * @mdev: the modern virtio-pci device
> > + * @off: offset of the capability
> > + * @minlen: minimal length of the capability
> > + * @align: align requirement
> > + * @start: start from the capability
> > + * @size: map size
> > + * @len: the length that is actually mapped
> > + * @pa: physical address of the capability
> > + *
> > + * Returns the io address of for the part of the capability
> > + */
> > +static void __iomem *
> > +pds_vdpa_map_capability(struct virtio_pci_modern_device *mdev, int off,
> > + size_t minlen, u32 align, u32 start, u32 size,
> > + size_t *len, resource_size_t *pa)
> > +{
> > + struct pci_dev *dev = mdev->pci_dev;
> > + u8 bar;
> > + u32 offset, length;
> > + void __iomem *p;
> > +
> > + pci_read_config_byte(dev, off + offsetof(struct virtio_pci_cap,
> > + bar),
> > + &bar);
> > + pci_read_config_dword(dev, off + offsetof(struct virtio_pci_cap, offset),
> > + &offset);
> > + pci_read_config_dword(dev, off + offsetof(struct virtio_pci_cap, length),
> > + &length);
> > +
> > + /* Check if the BAR may have changed since we requested the region. */
> > + if (bar >= PCI_STD_NUM_BARS || !(mdev->modern_bars & (1 << bar))) {
> > + dev_err(&dev->dev,
> > + "virtio_pci: bar unexpectedly changed to %u\n", bar);
> > + return NULL;
> > + }
> > +
> > + if (length <= start) {
> > + dev_err(&dev->dev,
> > + "virtio_pci: bad capability len %u (>%u expected)\n",
> > + length, start);
> > + return NULL;
> > + }
> > +
> > + if (length - start < minlen) {
> > + dev_err(&dev->dev,
> > + "virtio_pci: bad capability len %u (>=%zu expected)\n",
> > + length, minlen);
> > + return NULL;
> > + }
> > +
> > + length -= start;
> > +
> > + if (start + offset < offset) {
> > + dev_err(&dev->dev,
> > + "virtio_pci: map wrap-around %u+%u\n",
> > + start, offset);
> > + return NULL;
> > + }
> > +
> > + offset += start;
> > +
> > + if (offset & (align - 1)) {
> > + dev_err(&dev->dev,
> > + "virtio_pci: offset %u not aligned to %u\n",
> > + offset, align);
> > + return NULL;
> > + }
> > +
> > + if (length > size)
> > + length = size;
> > +
> > + if (len)
> > + *len = length;
> > +
> > + if (minlen + offset < minlen ||
> > + minlen + offset > pci_resource_len(dev, bar)) {
> > + dev_err(&dev->dev,
> > + "virtio_pci: map virtio %zu@%u out of range on bar %i length %lu\n",
> > + minlen, offset,
> > + bar, (unsigned long)pci_resource_len(dev, bar));
> > + return NULL;
> > + }
> > +
> > + p = pci_iomap_range(dev, bar, offset, length);
> > + if (!p)
> > + dev_err(&dev->dev,
> > + "virtio_pci: unable to map virtio %u@%u on bar %i\n",
> > + length, offset, bar);
> > + else if (pa)
> > + *pa = pci_resource_start(dev, bar) + offset;
> > +
> > + return p;
> > +}
> > +
> > +/**
> > + * virtio_pci_find_capability - walk capabilities to find device info.
> > + * @dev: the pci device
> > + * @cfg_type: the VIRTIO_PCI_CAP_* value we seek
> > + * @ioresource_types: IORESOURCE_MEM and/or IORESOURCE_IO.
> > + * @bars: the bitmask of BARs
> > + *
> > + * Returns offset of the capability, or 0.
> > + */
> > +static inline int virtio_pci_find_capability(struct pci_dev *dev, u8 cfg_type,
> > + u32 ioresource_types, int *bars)
> > +{
> > + int pos;
> > +
> > + for (pos = pci_find_capability(dev, PCI_CAP_ID_VNDR);
> > + pos > 0;
> > + pos = pci_find_next_capability(dev, pos, PCI_CAP_ID_VNDR)) {
> > + u8 type, bar;
> > +
> > + pci_read_config_byte(dev, pos + offsetof(struct virtio_pci_cap,
> > + cfg_type),
> > + &type);
> > + pci_read_config_byte(dev, pos + offsetof(struct virtio_pci_cap,
> > + bar),
> > + &bar);
> > +
> > + /* Ignore structures with reserved BAR values */
> > + if (bar >= PCI_STD_NUM_BARS)
> > + continue;
> > +
> > + if (type == cfg_type) {
> > + if (pci_resource_len(dev, bar) &&
> > + pci_resource_flags(dev, bar) & ioresource_types) {
> > + *bars |= (1 << bar);
> > + return pos;
> > + }
> > + }
> > + }
> > + return 0;
> > +}
> > +
> > +/*
> > + * pds_vdpa_probe_virtio: probe the modern virtio pci device, note that the
> > + * caller is required to enable PCI device before calling this function.
> > + * @mdev: the modern virtio-pci device
> > + *
> > + * Return 0 on succeed otherwise fail
> > + */
> > +int pds_vdpa_probe_virtio(struct virtio_pci_modern_device *mdev)
> > +{
> > + struct pci_dev *pci_dev = mdev->pci_dev;
> > + int err, common, isr, notify, device;
> > + u32 notify_length;
> > + u32 notify_offset;
> > +
> > + /* check for a common config: if not, use legacy mode (bar 0). */
> > + common = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_COMMON_CFG,
> > + IORESOURCE_IO | IORESOURCE_MEM,
> > + &mdev->modern_bars);
> > + if (!common) {
> > + dev_info(&pci_dev->dev,
> > + "virtio_pci: missing common config\n");
> > + return -ENODEV;
> > + }
> > +
> > + /* If common is there, these should be too... */
> > + isr = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_ISR_CFG,
> > + IORESOURCE_IO | IORESOURCE_MEM,
> > + &mdev->modern_bars);
> > + notify = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_NOTIFY_CFG,
> > + IORESOURCE_IO | IORESOURCE_MEM,
> > + &mdev->modern_bars);
> > + if (!isr || !notify) {
> > + dev_err(&pci_dev->dev,
> > + "virtio_pci: missing capabilities %i/%i/%i\n",
> > + common, isr, notify);
> > + return -EINVAL;
> > + }
> > +
> > + /* Device capability is only mandatory for devices that have
> > + * device-specific configuration.
> > + */
> > + device = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_DEVICE_CFG,
> > + IORESOURCE_IO | IORESOURCE_MEM,
> > + &mdev->modern_bars);
> > +
> > + err = pci_request_selected_regions(pci_dev, mdev->modern_bars,
> > + "virtio-pci-modern");
> > + if (err)
> > + return err;
> > +
> > + err = -EINVAL;
> > + mdev->common = pds_vdpa_map_capability(mdev, common,
> > + sizeof(struct virtio_pci_common_cfg), 4,
> > + 0, sizeof(struct virtio_pci_common_cfg),
> > + NULL, NULL);
> > + if (!mdev->common)
> > + goto err_map_common;
> > + mdev->isr = pds_vdpa_map_capability(mdev, isr, sizeof(u8), 1,
> > + 0, 1,
> > + NULL, NULL);
> > + if (!mdev->isr)
> > + goto err_map_isr;
> > +
> > + /* Read notify_off_multiplier from config space. */
> > + pci_read_config_dword(pci_dev,
> > + notify + offsetof(struct virtio_pci_notify_cap,
> > + notify_off_multiplier),
> > + &mdev->notify_offset_multiplier);
> > + /* Read notify length and offset from config space. */
> > + pci_read_config_dword(pci_dev,
> > + notify + offsetof(struct virtio_pci_notify_cap,
> > + cap.length),
> > + ¬ify_length);
> > +
> > + pci_read_config_dword(pci_dev,
> > + notify + offsetof(struct virtio_pci_notify_cap,
> > + cap.offset),
> > + ¬ify_offset);
> > +
> > + /* We don't know how many VQs we'll map, ahead of the time.
> > + * If notify length is small, map it all now.
> > + * Otherwise, map each VQ individually later.
> > + */
> > + if ((u64)notify_length + (notify_offset % PAGE_SIZE) <= PAGE_SIZE) {
> > + mdev->notify_base = pds_vdpa_map_capability(mdev, notify,
> > + 2, 2,
> > + 0, notify_length,
> > + &mdev->notify_len,
> > + &mdev->notify_pa);
> > + if (!mdev->notify_base)
> > + goto err_map_notify;
> > + } else {
> > + mdev->notify_map_cap = notify;
> > + }
> > +
> > + /* Again, we don't know how much we should map, but PAGE_SIZE
> > + * is more than enough for all existing devices.
> > + */
> > + if (device) {
> > + mdev->device = pds_vdpa_map_capability(mdev, device, 0, 4,
> > + 0, PAGE_SIZE,
> > + &mdev->device_len,
> > + NULL);
> > + if (!mdev->device)
> > + goto err_map_device;
> > + }
> > +
> > + return 0;
> > +
> > +err_map_device:
> > + if (mdev->notify_base)
> > + pci_iounmap(pci_dev, mdev->notify_base);
> > +err_map_notify:
> > + pci_iounmap(pci_dev, mdev->isr);
> > +err_map_isr:
> > + pci_iounmap(pci_dev, mdev->common);
> > +err_map_common:
> > + pci_release_selected_regions(pci_dev, mdev->modern_bars);
> > + return err;
> > +}
> > +
> > +void pds_vdpa_remove_virtio(struct virtio_pci_modern_device *mdev)
> > +{
> > + struct pci_dev *pci_dev = mdev->pci_dev;
> > +
> > + if (mdev->device)
> > + pci_iounmap(pci_dev, mdev->device);
> > + if (mdev->notify_base)
> > + pci_iounmap(pci_dev, mdev->notify_base);
> > + pci_iounmap(pci_dev, mdev->isr);
> > + pci_iounmap(pci_dev, mdev->common);
> > + pci_release_selected_regions(pci_dev, mdev->modern_bars);
> > +}
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH net-next 10/19] pds_core: devlink params for enabling VIF support
[not found] ` <f7457718-cff6-e5e1-242e-89b0e118ec3f@amd.com>
@ 2022-11-28 22:57 ` Andrew Lunn
[not found] ` <43eebffe-7ac1-6311-6973-c7a53935e42d@amd.com>
0 siblings, 1 reply; 13+ messages in thread
From: Andrew Lunn @ 2022-11-28 22:57 UTC (permalink / raw)
To: Shannon Nelson
Cc: mst, netdev, virtualization, Jakub Kicinski, drivers,
Shannon Nelson, davem
On Mon, Nov 28, 2022 at 02:26:26PM -0800, Shannon Nelson wrote:
> On 11/28/22 10:29 AM, Jakub Kicinski wrote:
> > On Fri, 18 Nov 2022 14:56:47 -0800 Shannon Nelson wrote:
> > > + DEVLINK_PARAM_DRIVER(PDSC_DEVLINK_PARAM_ID_LM,
> > > + "enable_lm",
> > > + DEVLINK_PARAM_TYPE_BOOL,
> > > + BIT(DEVLINK_PARAM_CMODE_RUNTIME),
> > > + pdsc_dl_enable_get,
> > > + pdsc_dl_enable_set,
> > > + pdsc_dl_enable_validate),
> >
> > Terrible name, not vendor specific.
>
> ... but useful for starting a conversation.
>
> How about we add
> DEVLINK_PARAM_GENERIC_ID_ENABLE_LM,
I know we are running short of short acronyms and we have to recycle
them, rfc5513 and all, so could you actually use
DEVLINK_PARAM_GENERIC_ID_ENABLE_LIST_MANAGER making it clear your
Smart NIC is running majordomo and will soon replace vger.
Andrew
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH net-next 10/19] pds_core: devlink params for enabling VIF support
[not found] ` <43eebffe-7ac1-6311-6973-c7a53935e42d@amd.com>
@ 2022-11-28 23:29 ` Andrew Lunn
[not found] ` <20221128153922.2e94958a@kernel.org>
0 siblings, 1 reply; 13+ messages in thread
From: Andrew Lunn @ 2022-11-28 23:29 UTC (permalink / raw)
To: Shannon Nelson
Cc: mst, netdev, virtualization, Jakub Kicinski, drivers,
Shannon Nelson, davem
> > I know we are running short of short acronyms and we have to recycle
> > them, rfc5513 and all, so could you actually use
> > DEVLINK_PARAM_GENERIC_ID_ENABLE_LIST_MANAGER making it clear your
> > Smart NIC is running majordomo and will soon replace vger.
> >
> > Andrew
>
> Oh, hush, someone might hear you speak of our plan to take over the email
> world!
It seems like something a Smart NIC would be ideal to do. Here is an
email body and 10,000 email addresses i recently acquired, go send
spam to them at line rate.
> How about:
> DEVLINK_PARAM_GENERIC_ID_ENABLE_LIVE_MIGRATION
Much better.
Andrew
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [RFC PATCH net-next 06/19] pds_core: add FW update feature to devlink
[not found] ` <20221128153315.11535ddd@kernel.org>
@ 2022-11-29 0:13 ` Keller, Jacob E
[not found] ` <2acf60f9-1ce0-4853-7b99-58c890627259@amd.com>
1 sibling, 0 replies; 13+ messages in thread
From: Keller, Jacob E @ 2022-11-29 0:13 UTC (permalink / raw)
To: Jakub Kicinski, Shannon Nelson
Cc: mst@redhat.com, netdev@vger.kernel.org,
virtualization@lists.linux-foundation.org, drivers@pensando.io,
Shannon Nelson, davem@davemloft.net
> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Monday, November 28, 2022 3:33 PM
> To: Shannon Nelson <shnelson@amd.com>
> Cc: Shannon Nelson <snelson@pensando.io>; netdev@vger.kernel.org;
> davem@davemloft.net; mst@redhat.com; jasowang@redhat.com;
> virtualization@lists.linux-foundation.org; drivers@pensando.io; Keller, Jacob E
> <jacob.e.keller@intel.com>
> Subject: Re: [RFC PATCH net-next 06/19] pds_core: add FW update feature to
> devlink
>
> On Mon, 28 Nov 2022 14:25:46 -0800 Shannon Nelson wrote:
> > I don't think Intel selects which FW image to boot, but it looks like
> > mlxsw and nfp use the PARAM_GENERIC_FW_LOAD_POLICY to select between
> > DRIVER, FLASH, or DISK. Shall I add a couple of generic SLOT_x items to
> > the enum devlink_param_fw_load_policy_value and use this API? For example:
> >
> > DEVLINK_PARAM_FW_LOAD_POLICY_VALUE_SLOT_0,
> > DEVLINK_PARAM_FW_LOAD_POLICY_VALUE_SLOT_1,
> > DEVLINK_PARAM_FW_LOAD_POLICY_VALUE_SLOT_2,
> > DEVLINK_PARAM_FW_LOAD_POLICY_VALUE_SLOT_3,
>
> Not the worst idea, although I presume normal FW flashing should switch
> between slots to activate the new image by default? Which means the
> action of fw flashing would alter the policy set by the user. A little
> awkward from an API purist standpoint.
>
> I'd just expose the active "bank" via netlink directly.
>
> > I could then modify the devlink dev info printed to refer to fw_slot_0,
> > fw.slot_1, and fw.slot_2 instead of our vendor specific names.
>
> Jake, didn't you have a similar capability in ice?
>
We have two banks of flash, the active bank, and an inactive bank used for updates. We can determine the active bank from the Shadow RAM contents which are generated as the EMP firmware boots up.
> Knowing my memory I may have acquiesced to something in another driver
> already. That said - I think it's cleaner if we just list the stored
> versions per bank, no?
I think it would make sense to store them per bank and make the bank number some index instead of something separate as like this DEVLINK_PARAM_FW_LOAD_POLICY_VALUE_SLOT_<X> where each <X> makes a separate parameter.
Currently devlink info reports "stored" and "active", which aligns with our current use of the active vs inactive flash bank. We could be explicit and indicate which bank it is, though its a bit tricky since most of the firmware interface deals with it in terms of "active" and "inactive" rather than the absolute position of "bank 0 or bank 1".
Especially if another device has more than 2 banks I think its a good extension to devlink info, and we could probably get away with something like a new info attribute that specifies which bank index it is, and an attribute which indicates whether its active or not.
Thanks,
Jake
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [RFC PATCH net-next 06/19] pds_core: add FW update feature to devlink
[not found] ` <2acf60f9-1ce0-4853-7b99-58c890627259@amd.com>
@ 2022-11-29 0:18 ` Keller, Jacob E
0 siblings, 0 replies; 13+ messages in thread
From: Keller, Jacob E @ 2022-11-29 0:18 UTC (permalink / raw)
To: Shannon Nelson, Jakub Kicinski
Cc: mst@redhat.com, netdev@vger.kernel.org,
virtualization@lists.linux-foundation.org, drivers@pensando.io,
Shannon Nelson, davem@davemloft.net
> -----Original Message-----
> From: Shannon Nelson <shnelson@amd.com>
> Sent: Monday, November 28, 2022 3:46 PM
> To: Jakub Kicinski <kuba@kernel.org>
> Cc: Shannon Nelson <snelson@pensando.io>; netdev@vger.kernel.org;
> davem@davemloft.net; mst@redhat.com; jasowang@redhat.com;
> virtualization@lists.linux-foundation.org; drivers@pensando.io; Keller, Jacob E
> <jacob.e.keller@intel.com>
> Subject: Re: [RFC PATCH net-next 06/19] pds_core: add FW update feature to
> devlink
>
> On 11/28/22 3:33 PM, Jakub Kicinski wrote:
> > On Mon, 28 Nov 2022 14:25:46 -0800 Shannon Nelson wrote:
> >> I don't think Intel selects which FW image to boot, but it looks like
> >> mlxsw and nfp use the PARAM_GENERIC_FW_LOAD_POLICY to select between
> >> DRIVER, FLASH, or DISK. Shall I add a couple of generic SLOT_x items to
> >> the enum devlink_param_fw_load_policy_value and use this API? For
> example:
> >>
> >> DEVLINK_PARAM_FW_LOAD_POLICY_VALUE_SLOT_0,
> >> DEVLINK_PARAM_FW_LOAD_POLICY_VALUE_SLOT_1,
> >> DEVLINK_PARAM_FW_LOAD_POLICY_VALUE_SLOT_2,
> >> DEVLINK_PARAM_FW_LOAD_POLICY_VALUE_SLOT_3,
> >
> > Not the worst idea, although I presume normal FW flashing should switch
> > between slots to activate the new image by default? Which means the
> > action of fw flashing would alter the policy set by the user. A little
> > awkward from an API purist standpoint.
This could potentially be handled by having DELVINK_PARAM_FW_LOAD_POLICY_FLASH be the automatic "select best version", and if a user has set a manual value then don't allow flashing until a reboot or the value is set back to POLICY_FLASH?
>
> Yes, the action of flashing will set the new bank/slot to use on the
> next boot. However, we have the ability to select from multiple valid
> images and we want to pass this flexibility to the user rather than
> force them to go through a whole flash sequence just to get to the other
> bank.
>
> >
> > I'd just expose the active "bank" via netlink directly.
> >
> >> I could then modify the devlink dev info printed to refer to fw_slot_0,
> >> fw.slot_1, and fw.slot_2 instead of our vendor specific names.
> >
> > Jake, didn't you have a similar capability in ice?
> >
> > Knowing my memory I may have acquiesced to something in another driver
> > already. That said - I think it's cleaner if we just list the stored
> > versions per bank, no?
>
> We are listing the stored images in the devlink dev info output, just
> want to let the user choose which of those valid images to use next.
>
> Cheers,
> sln
Technically I think we could do something similar in ice to switch between the banks, at least as long as there is a valid image in the bank. The big trick is that I am not sure we can verify ahead of time whether we have a valid image and if you happen to boot into an invalid or blank image. There is some recovery firmware that should activate in that case, but I think our current driver doesn't implement enough of a recovery mode to actually handle this case to allow user to switch back.
Still, I think the ability to select the bank is valuable, and finding the right way to expose it is good.
Thanks,
Jake
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH net-next 10/19] pds_core: devlink params for enabling VIF support
[not found] ` <20221128153922.2e94958a@kernel.org>
@ 2022-11-29 9:00 ` Leon Romanovsky
2022-11-29 9:13 ` Jiri Pirko
1 sibling, 0 replies; 13+ messages in thread
From: Leon Romanovsky @ 2022-11-29 9:00 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Andrew Lunn, Shannon Nelson, mst, netdev, virtualization, drivers,
Shannon Nelson, davem
On Mon, Nov 28, 2022 at 03:39:22PM -0800, Jakub Kicinski wrote:
> On Tue, 29 Nov 2022 00:29:42 +0100 Andrew Lunn wrote:
> > > How about:
> > > DEVLINK_PARAM_GENERIC_ID_ENABLE_LIVE_MIGRATION
> >
> > Much better.
>
> +1, although I care much less about the define name which is stupidly
> long anyway and more about the actual value that the user will see
We have enable/disable devlink live migration knob in our queue. Saeed
thought to send it next week.
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH net-next 10/19] pds_core: devlink params for enabling VIF support
[not found] ` <20221128153922.2e94958a@kernel.org>
2022-11-29 9:00 ` Leon Romanovsky
@ 2022-11-29 9:13 ` Jiri Pirko
1 sibling, 0 replies; 13+ messages in thread
From: Jiri Pirko @ 2022-11-29 9:13 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Andrew Lunn, Shannon Nelson, mst, netdev, virtualization, drivers,
Shannon Nelson, davem
Tue, Nov 29, 2022 at 12:39:22AM CET, kuba@kernel.org wrote:
>On Tue, 29 Nov 2022 00:29:42 +0100 Andrew Lunn wrote:
>> > How about:
>> > DEVLINK_PARAM_GENERIC_ID_ENABLE_LIVE_MIGRATION
>>
>> Much better.
>
>+1, although I care much less about the define name which is stupidly
>long anyway and more about the actual value that the user will see
We have patches that introduce live migration as a generic port function
capability bit. It is an attribute of the function.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH net-next 18/19] pds_vdpa: add support for vdpa and vdpamgmt interfaces
[not found] ` <4b4e7474-5b36-6b2c-a0a5-2e198e1bab0c@amd.com>
@ 2022-12-05 7:40 ` Jason Wang
0 siblings, 0 replies; 13+ messages in thread
From: Jason Wang @ 2022-12-05 7:40 UTC (permalink / raw)
To: Shannon Nelson
Cc: mst, netdev, virtualization, kuba, drivers, Shannon Nelson, davem
On Wed, Nov 30, 2022 at 8:11 AM Shannon Nelson <shnelson@amd.com> wrote:
>
> On 11/21/22 10:32 PM, Jason Wang wrote:
> > On Sat, Nov 19, 2022 at 6:57 AM Shannon Nelson <snelson@pensando.io> wrote:
> >>
> >> This is the vDPA device support, where we advertise that we can
> >> support the virtio queues and deal with the configuration work
> >> through the pds_core's adminq.
> >>
> >> Signed-off-by: Shannon Nelson <snelson@pensando.io>
> >> ---
> >> drivers/vdpa/pds/Makefile | 3 +-
> >> drivers/vdpa/pds/aux_drv.c | 33 ++
> >> drivers/vdpa/pds/debugfs.c | 167 ++++++++
> >> drivers/vdpa/pds/debugfs.h | 4 +
> >> drivers/vdpa/pds/vdpa_dev.c | 796 ++++++++++++++++++++++++++++++++++++
> >> 5 files changed, 1002 insertions(+), 1 deletion(-)
> >> create mode 100644 drivers/vdpa/pds/vdpa_dev.c
> >>
> >> diff --git a/drivers/vdpa/pds/Makefile b/drivers/vdpa/pds/Makefile
> >> index fafd356ddf86..7fde4a4a1620 100644
> >> --- a/drivers/vdpa/pds/Makefile
> >> +++ b/drivers/vdpa/pds/Makefile
> >> @@ -7,4 +7,5 @@ pds_vdpa-y := aux_drv.o \
> >> cmds.o \
> >> pci_drv.o \
> >> debugfs.o \
> >> - virtio_pci.o
> >> + virtio_pci.o \
> >> + vdpa_dev.o
> >> diff --git a/drivers/vdpa/pds/aux_drv.c b/drivers/vdpa/pds/aux_drv.c
> >> index aef3c984dc90..83b9a5a79325 100644
> >> --- a/drivers/vdpa/pds/aux_drv.c
> >> +++ b/drivers/vdpa/pds/aux_drv.c
> >> @@ -12,6 +12,7 @@
> >> #include <linux/pds/pds_vdpa.h>
> >>
> >> #include "aux_drv.h"
> >> +#include "vdpa_dev.h"
> >> #include "pci_drv.h"
> >> #include "debugfs.h"
> >>
> >> @@ -25,10 +26,25 @@ static void
> >> pds_vdpa_aux_notify_handler(struct pds_auxiliary_dev *padev,
> >> union pds_core_notifyq_comp *event)
> >> {
> >> + struct pds_vdpa_device *pdsv = padev->priv;
> >> struct device *dev = &padev->aux_dev.dev;
> >> u16 ecode = le16_to_cpu(event->ecode);
> >>
> >> dev_info(dev, "%s: event code %d\n", __func__, ecode);
> >> +
> >> + /* Give the upper layers a hint that something interesting
> >> + * may have happened. It seems that the only thing this
> >> + * triggers in the virtio-net drivers above us is a check
> >> + * of link status.
> >> + *
> >> + * We don't set the NEEDS_RESET flag for EVENT_RESET
> >> + * because we're likely going through a recovery or
> >> + * fw_update and will be back up and running soon.
> >> + */
> >> + if (ecode == PDS_EVENT_RESET || ecode == PDS_EVENT_LINK_CHANGE) {
> >> + if (pdsv->hw.config_cb.callback)
> >> + pdsv->hw.config_cb.callback(pdsv->hw.config_cb.private);
> >> + }
> >> }
> >>
> >> static int
> >> @@ -80,10 +96,25 @@ pds_vdpa_aux_probe(struct auxiliary_device *aux_dev,
> >> goto err_register_client;
> >> }
> >>
> >> + /* Get device ident info and set up the vdpa_mgmt_dev */
> >> + err = pds_vdpa_get_mgmt_info(vdpa_aux);
> >> + if (err)
> >> + goto err_register_client;
> >> +
> >> + /* Let vdpa know that we can provide devices */
> >> + err = vdpa_mgmtdev_register(&vdpa_aux->vdpa_mdev);
> >> + if (err) {
> >> + dev_err(dev, "%s: Failed to initialize vdpa_mgmt interface: %pe\n",
> >> + __func__, ERR_PTR(err));
> >> + goto err_mgmt_reg;
> >> + }
> >> +
> >> pds_vdpa_debugfs_add_ident(vdpa_aux);
> >>
> >> return 0;
> >>
> >> +err_mgmt_reg:
> >> + padev->ops->unregister_client(padev);
> >> err_register_client:
> >> auxiliary_set_drvdata(aux_dev, NULL);
> >> err_invalid_driver:
> >> @@ -98,6 +129,8 @@ pds_vdpa_aux_remove(struct auxiliary_device *aux_dev)
> >> struct pds_vdpa_aux *vdpa_aux = auxiliary_get_drvdata(aux_dev);
> >> struct device *dev = &aux_dev->dev;
> >>
> >> + vdpa_mgmtdev_unregister(&vdpa_aux->vdpa_mdev);
> >> +
> >> vdpa_aux->padev->ops->unregister_client(vdpa_aux->padev);
> >> if (vdpa_aux->vdpa_vf)
> >> pci_dev_put(vdpa_aux->vdpa_vf->pdev);
> >> diff --git a/drivers/vdpa/pds/debugfs.c b/drivers/vdpa/pds/debugfs.c
> >> index f766412209df..aa3143126a7e 100644
> >> --- a/drivers/vdpa/pds/debugfs.c
> >> +++ b/drivers/vdpa/pds/debugfs.c
> >> @@ -11,6 +11,7 @@
> >> #include <linux/pds/pds_auxbus.h>
> >> #include <linux/pds/pds_vdpa.h>
> >>
> >> +#include "vdpa_dev.h"
> >> #include "aux_drv.h"
> >> #include "pci_drv.h"
> >> #include "debugfs.h"
> >> @@ -19,6 +20,72 @@
> >>
> >> static struct dentry *dbfs_dir;
> >>
> >> +#define PRINT_SBIT_NAME(__seq, __f, __name) \
> >> + do { \
> >> + if (__f & __name) \
> >> + seq_printf(__seq, " %s", &#__name[16]); \
> >> + } while (0)
> >> +
> >> +static void
> >> +print_status_bits(struct seq_file *seq, u16 status)
> >> +{
> >> + seq_puts(seq, "status:");
> >> + PRINT_SBIT_NAME(seq, status, VIRTIO_CONFIG_S_ACKNOWLEDGE);
> >> + PRINT_SBIT_NAME(seq, status, VIRTIO_CONFIG_S_DRIVER);
> >> + PRINT_SBIT_NAME(seq, status, VIRTIO_CONFIG_S_DRIVER_OK);
> >> + PRINT_SBIT_NAME(seq, status, VIRTIO_CONFIG_S_FEATURES_OK);
> >> + PRINT_SBIT_NAME(seq, status, VIRTIO_CONFIG_S_NEEDS_RESET);
> >> + PRINT_SBIT_NAME(seq, status, VIRTIO_CONFIG_S_FAILED);
> >> + seq_puts(seq, "\n");
> >> +}
> >> +
> >> +#define PRINT_FBIT_NAME(__seq, __f, __name) \
> >> + do { \
> >> + if (__f & BIT_ULL(__name)) \
> >> + seq_printf(__seq, " %s", #__name); \
> >> + } while (0)
> >> +
> >> +static void
> >> +print_feature_bits(struct seq_file *seq, u64 features)
> >> +{
> >> + seq_puts(seq, "features:");
> >> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_CSUM);
> >> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_GUEST_CSUM);
> >> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS);
> >> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_MTU);
> >> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_MAC);
> >> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_GUEST_TSO4);
> >> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_GUEST_TSO6);
> >> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_GUEST_ECN);
> >> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_GUEST_UFO);
> >> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_HOST_TSO4);
> >> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_HOST_TSO6);
> >> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_HOST_ECN);
> >> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_HOST_UFO);
> >> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_MRG_RXBUF);
> >> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_STATUS);
> >> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_CTRL_VQ);
> >> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_CTRL_RX);
> >> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_CTRL_VLAN);
> >> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_CTRL_RX_EXTRA);
> >> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_GUEST_ANNOUNCE);
> >> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_MQ);
> >> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_CTRL_MAC_ADDR);
> >> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_HASH_REPORT);
> >> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_RSS);
> >> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_RSC_EXT);
> >> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_STANDBY);
> >> + PRINT_FBIT_NAME(seq, features, VIRTIO_NET_F_SPEED_DUPLEX);
> >> + PRINT_FBIT_NAME(seq, features, VIRTIO_F_NOTIFY_ON_EMPTY);
> >> + PRINT_FBIT_NAME(seq, features, VIRTIO_F_ANY_LAYOUT);
> >> + PRINT_FBIT_NAME(seq, features, VIRTIO_F_VERSION_1);
> >> + PRINT_FBIT_NAME(seq, features, VIRTIO_F_ACCESS_PLATFORM);
> >> + PRINT_FBIT_NAME(seq, features, VIRTIO_F_RING_PACKED);
> >> + PRINT_FBIT_NAME(seq, features, VIRTIO_F_ORDER_PLATFORM);
> >> + PRINT_FBIT_NAME(seq, features, VIRTIO_F_SR_IOV);
> >> + seq_puts(seq, "\n");
> >> +}
> >> +
> >> void
> >> pds_vdpa_debugfs_create(void)
> >> {
> >> @@ -49,10 +116,18 @@ static int
> >> identity_show(struct seq_file *seq, void *v)
> >> {
> >> struct pds_vdpa_aux *vdpa_aux = seq->private;
> >> + struct vdpa_mgmt_dev *mgmt;
> >>
> >> seq_printf(seq, "aux_dev: %s\n",
> >> dev_name(&vdpa_aux->padev->aux_dev.dev));
> >>
> >> + mgmt = &vdpa_aux->vdpa_mdev;
> >> + seq_printf(seq, "max_vqs: %d\n", mgmt->max_supported_vqs);
> >> + seq_printf(seq, "config_attr_mask: %#llx\n", mgmt->config_attr_mask);
> >> + seq_printf(seq, "supported_features: %#llx\n", mgmt->supported_features);
> >> + print_feature_bits(seq, mgmt->supported_features);
> >> + seq_printf(seq, "local_mac_bit: %d\n", vdpa_aux->local_mac_bit);
> >> +
> >> return 0;
> >> }
> >> DEFINE_SHOW_ATTRIBUTE(identity);
> >> @@ -64,4 +139,96 @@ pds_vdpa_debugfs_add_ident(struct pds_vdpa_aux *vdpa_aux)
> >> vdpa_aux, &identity_fops);
> >> }
> >>
> >> +static int
> >> +config_show(struct seq_file *seq, void *v)
> >> +{
> >> + struct pds_vdpa_device *pdsv = seq->private;
> >> + struct virtio_net_config *vc = &pdsv->vn_config;
> >> +
> >> + seq_printf(seq, "mac: %pM\n", vc->mac);
> >> + seq_printf(seq, "max_virtqueue_pairs: %d\n",
> >> + __virtio16_to_cpu(true, vc->max_virtqueue_pairs));
> >> + seq_printf(seq, "mtu: %d\n", __virtio16_to_cpu(true, vc->mtu));
> >> + seq_printf(seq, "speed: %d\n", le32_to_cpu(vc->speed));
> >> + seq_printf(seq, "duplex: %d\n", vc->duplex);
> >> + seq_printf(seq, "rss_max_key_size: %d\n", vc->rss_max_key_size);
> >> + seq_printf(seq, "rss_max_indirection_table_length: %d\n",
> >> + le16_to_cpu(vc->rss_max_indirection_table_length));
> >> + seq_printf(seq, "supported_hash_types: %#x\n",
> >> + le32_to_cpu(vc->supported_hash_types));
> >> + seq_printf(seq, "vn_status: %#x\n",
> >> + __virtio16_to_cpu(true, vc->status));
> >> + print_status_bits(seq, __virtio16_to_cpu(true, vc->status));
> >> +
> >> + seq_printf(seq, "hw_status: %#x\n", pdsv->hw.status);
> >> + print_status_bits(seq, pdsv->hw.status);
> >> + seq_printf(seq, "req_features: %#llx\n", pdsv->hw.req_features);
> >> + print_feature_bits(seq, pdsv->hw.req_features);
> >> + seq_printf(seq, "actual_features: %#llx\n", pdsv->hw.actual_features);
> >> + print_feature_bits(seq, pdsv->hw.actual_features);
> >> + seq_printf(seq, "vdpa_index: %d\n", pdsv->hw.vdpa_index);
> >> + seq_printf(seq, "num_vqs: %d\n", pdsv->hw.num_vqs);
> >> +
> >> + return 0;
> >> +}
> >> +DEFINE_SHOW_ATTRIBUTE(config);
> >> +
> >> +static int
> >> +vq_show(struct seq_file *seq, void *v)
> >> +{
> >> + struct pds_vdpa_vq_info *vq = seq->private;
> >> + struct pds_vdpa_intr_info *intrs;
> >> +
> >> + seq_printf(seq, "ready: %d\n", vq->ready);
> >> + seq_printf(seq, "desc_addr: %#llx\n", vq->desc_addr);
> >> + seq_printf(seq, "avail_addr: %#llx\n", vq->avail_addr);
> >> + seq_printf(seq, "used_addr: %#llx\n", vq->used_addr);
> >> + seq_printf(seq, "q_len: %d\n", vq->q_len);
> >> + seq_printf(seq, "qid: %d\n", vq->qid);
> >> +
> >> + seq_printf(seq, "doorbell: %#llx\n", vq->doorbell);
> >> + seq_printf(seq, "avail_idx: %d\n", vq->avail_idx);
> >> + seq_printf(seq, "used_idx: %d\n", vq->used_idx);
> >> + seq_printf(seq, "intr_index: %d\n", vq->intr_index);
> >> +
> >> + intrs = vq->pdsv->vdpa_aux->vdpa_vf->intrs;
> >> + seq_printf(seq, "irq: %d\n", intrs[vq->intr_index].irq);
> >> + seq_printf(seq, "irq-name: %s\n", intrs[vq->intr_index].name);
> >> +
> >> + seq_printf(seq, "hw_qtype: %d\n", vq->hw_qtype);
> >> + seq_printf(seq, "hw_qindex: %d\n", vq->hw_qindex);
> >> +
> >> + return 0;
> >> +}
> >> +DEFINE_SHOW_ATTRIBUTE(vq);
> >> +
> >> +void
> >> +pds_vdpa_debugfs_add_vdpadev(struct pds_vdpa_device *pdsv)
> >> +{
> >> + struct dentry *dentry;
> >> + const char *name;
> >> + int i;
> >> +
> >> + dentry = pdsv->vdpa_aux->vdpa_vf->dentry;
> >> + name = dev_name(&pdsv->vdpa_dev.dev);
> >> +
> >> + pdsv->dentry = debugfs_create_dir(name, dentry);
> >> +
> >> + debugfs_create_file("config", 0400, pdsv->dentry, pdsv, &config_fops);
> >> +
> >> + for (i = 0; i < pdsv->hw.num_vqs; i++) {
> >> + char name[8];
> >> +
> >> + snprintf(name, sizeof(name), "vq%02d", i);
> >> + debugfs_create_file(name, 0400, pdsv->dentry, &pdsv->hw.vqs[i], &vq_fops);
> >> + }
> >> +}
> >> +
> >> +void
> >> +pds_vdpa_debugfs_del_vdpadev(struct pds_vdpa_device *pdsv)
> >> +{
> >> + debugfs_remove_recursive(pdsv->dentry);
> >> + pdsv->dentry = NULL;
> >> +}
> >> +
> >> #endif /* CONFIG_DEBUG_FS */
> >> diff --git a/drivers/vdpa/pds/debugfs.h b/drivers/vdpa/pds/debugfs.h
> >> index 939a4c248aac..f0567e4ee4e4 100644
> >> --- a/drivers/vdpa/pds/debugfs.h
> >> +++ b/drivers/vdpa/pds/debugfs.h
> >> @@ -13,12 +13,16 @@ void pds_vdpa_debugfs_destroy(void);
> >> void pds_vdpa_debugfs_add_pcidev(struct pds_vdpa_pci_device *vdpa_pdev);
> >> void pds_vdpa_debugfs_del_pcidev(struct pds_vdpa_pci_device *vdpa_pdev);
> >> void pds_vdpa_debugfs_add_ident(struct pds_vdpa_aux *vdpa_aux);
> >> +void pds_vdpa_debugfs_add_vdpadev(struct pds_vdpa_device *pdsv);
> >> +void pds_vdpa_debugfs_del_vdpadev(struct pds_vdpa_device *pdsv);
> >> #else
> >> static inline void pds_vdpa_debugfs_create(void) { }
> >> static inline void pds_vdpa_debugfs_destroy(void) { }
> >> static inline void pds_vdpa_debugfs_add_pcidev(struct pds_vdpa_pci_device *vdpa_pdev) { }
> >> static inline void pds_vdpa_debugfs_del_pcidev(struct pds_vdpa_pci_device *vdpa_pdev) { }
> >> static inline void pds_vdpa_debugfs_add_ident(struct pds_vdpa_aux *vdpa_aux) { }
> >> +static inline void pds_vdpa_debugfs_add_vdpadev(struct pds_vdpa_device *pdsv) { }
> >> +static inline void pds_vdpa_debugfs_del_vdpadev(struct pds_vdpa_device *pdsv) { }
> >> #endif
> >>
> >> #endif /* _PDS_VDPA_DEBUGFS_H_ */
> >> diff --git a/drivers/vdpa/pds/vdpa_dev.c b/drivers/vdpa/pds/vdpa_dev.c
> >> new file mode 100644
> >> index 000000000000..824be42aff0d
> >> --- /dev/null
> >> +++ b/drivers/vdpa/pds/vdpa_dev.c
> >> @@ -0,0 +1,796 @@
> >> +// SPDX-License-Identifier: GPL-2.0-only
> >> +/* Copyright(c) 2022 Pensando Systems, Inc */
> >> +
> >> +#include <linux/interrupt.h>
> >> +#include <linux/module.h>
> >> +#include <linux/pci.h>
> >> +#include <linux/sysfs.h>
> >> +#include <linux/types.h>
> >> +#include <linux/vdpa.h>
> >> +#include <uapi/linux/virtio_pci.h>
> >> +#include <uapi/linux/vdpa.h>
> >> +
> >> +#include <linux/pds/pds_intr.h>
> >> +#include <linux/pds/pds_core_if.h>
> >> +#include <linux/pds/pds_adminq.h>
> >> +#include <linux/pds/pds_auxbus.h>
> >> +#include <linux/pds/pds_vdpa.h>
> >> +
> >> +#include "vdpa_dev.h"
> >> +#include "pci_drv.h"
> >> +#include "aux_drv.h"
> >> +#include "pci_drv.h"
> >> +#include "cmds.h"
> >> +#include "debugfs.h"
> >> +
> >> +static int
> >> +pds_vdpa_setup_driver(struct pds_vdpa_device *pdsv)
> >> +{
> >> + struct device *dev = &pdsv->vdpa_dev.dev;
> >> + int err = 0;
> >> + int i;
> >> +
> >> + /* Verify all vqs[] are in ready state */
> >> + for (i = 0; i < pdsv->hw.num_vqs; i++) {
> >> + if (!pdsv->hw.vqs[i].ready) {
> >> + dev_warn(dev, "%s: qid %d not ready\n", __func__, i);
> >> + err = -ENOENT;
> >> + }
> >> + }
> >> +
> >> + return err;
> >> +}
> >> +
> >> +static struct pds_vdpa_device *
> >> +vdpa_to_pdsv(struct vdpa_device *vdpa_dev)
> >> +{
> >> + return container_of(vdpa_dev, struct pds_vdpa_device, vdpa_dev);
> >> +}
> >> +
> >> +static struct pds_vdpa_hw *
> >> +vdpa_to_hw(struct vdpa_device *vdpa_dev)
> >> +{
> >> + struct pds_vdpa_device *pdsv = vdpa_to_pdsv(vdpa_dev);
> >> +
> >> + return &pdsv->hw;
> >> +}
> >> +
> >> +static int
> >> +pds_vdpa_set_vq_address(struct vdpa_device *vdpa_dev, u16 qid,
> >> + u64 desc_addr, u64 driver_addr, u64 device_addr)
> >> +{
> >> + struct pds_vdpa_hw *hw = vdpa_to_hw(vdpa_dev);
> >> +
> >> + hw->vqs[qid].desc_addr = desc_addr;
> >> + hw->vqs[qid].avail_addr = driver_addr;
> >> + hw->vqs[qid].used_addr = device_addr;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static void
> >> +pds_vdpa_set_vq_num(struct vdpa_device *vdpa_dev, u16 qid, u32 num)
> >> +{
> >> + struct pds_vdpa_hw *hw = vdpa_to_hw(vdpa_dev);
> >> +
> >> + hw->vqs[qid].q_len = num;
> >> +}
> >> +
> >> +static void
> >> +pds_vdpa_kick_vq(struct vdpa_device *vdpa_dev, u16 qid)
> >> +{
> >> + struct pds_vdpa_device *pdsv = vdpa_to_pdsv(vdpa_dev);
> >> +
> >> + iowrite16(qid, pdsv->hw.vqs[qid].notify);
> >> +}
> >> +
> >> +static void
> >> +pds_vdpa_set_vq_cb(struct vdpa_device *vdpa_dev, u16 qid,
> >> + struct vdpa_callback *cb)
> >> +{
> >> + struct pds_vdpa_hw *hw = vdpa_to_hw(vdpa_dev);
> >> +
> >> + hw->vqs[qid].event_cb = *cb;
> >> +}
> >> +
> >> +static irqreturn_t
> >> +pds_vdpa_isr(int irq, void *data)
> >> +{
> >> + struct pds_core_intr __iomem *intr_ctrl;
> >> + struct pds_vdpa_device *pdsv;
> >> + struct pds_vdpa_vq_info *vq;
> >> +
> >> + vq = data;
> >> + pdsv = vq->pdsv;
> >> +
> >> + if (vq->event_cb.callback)
> >> + vq->event_cb.callback(vq->event_cb.private);
> >> +
> >> + /* Since we don't actually know how many vq descriptors are
> >> + * covered in this interrupt cycle, we simply clean all the
> >> + * credits and re-enable the interrupt.
> >> + */
> >> + intr_ctrl = (struct pds_core_intr __iomem *)pdsv->vdpa_aux->vdpa_vf->vd_mdev.isr;
> >> + pds_core_intr_clean_flags(&intr_ctrl[vq->intr_index],
> >> + PDS_CORE_INTR_CRED_REARM);
> >> +
> >> + return IRQ_HANDLED;
> >> +}
> >> +
> >> +static void
> >> +pds_vdpa_release_irq(struct pds_vdpa_device *pdsv, int qid)
> >> +{
> >> + struct pds_vdpa_intr_info *intrs = pdsv->vdpa_aux->vdpa_vf->intrs;
> >> + struct pci_dev *pdev = pdsv->vdpa_aux->vdpa_vf->pdev;
> >> + struct pds_core_intr __iomem *intr_ctrl;
> >> + int index;
> >> +
> >> + intr_ctrl = (struct pds_core_intr __iomem *)pdsv->vdpa_aux->vdpa_vf->vd_mdev.isr;
> >> + index = pdsv->hw.vqs[qid].intr_index;
> >> + if (index == VIRTIO_MSI_NO_VECTOR)
> >> + return;
> >> +
> >> + if (intrs[index].irq == VIRTIO_MSI_NO_VECTOR)
> >> + return;
> >> +
> >> + if (qid & 0x1) {
> >> + pdsv->hw.vqs[qid].intr_index = VIRTIO_MSI_NO_VECTOR;
> >> + } else {
> >> + pds_core_intr_mask(&intr_ctrl[index], PDS_CORE_INTR_MASK_SET);
> >> + devm_free_irq(&pdev->dev, intrs[index].irq, &pdsv->hw.vqs[qid]);
> >> + pdsv->hw.vqs[qid].intr_index = VIRTIO_MSI_NO_VECTOR;
> >> + intrs[index].irq = VIRTIO_MSI_NO_VECTOR;
> >> + }
> >> +}
> >> +
> >> +static void
> >> +pds_vdpa_set_vq_ready(struct vdpa_device *vdpa_dev, u16 qid, bool ready)
> >> +{
> >> + struct pds_vdpa_device *pdsv = vdpa_to_pdsv(vdpa_dev);
> >> + struct pci_dev *pdev = pdsv->vdpa_aux->vdpa_vf->pdev;
> >> + struct pds_vdpa_hw *hw = vdpa_to_hw(vdpa_dev);
> >> + struct device *dev = &pdsv->vdpa_dev.dev;
> >> + struct pds_core_intr __iomem *intr_ctrl;
> >> + int err;
> >> +
> >> + dev_dbg(dev, "%s: qid %d ready %d => %d\n",
> >> + __func__, qid, hw->vqs[qid].ready, ready);
> >> + if (ready == hw->vqs[qid].ready)
> >> + return;
> >> +
> >> + intr_ctrl = (struct pds_core_intr __iomem *)pdsv->vdpa_aux->vdpa_vf->vd_mdev.isr;
> >
> > It looks to me pds has a different layout/semantic for isr than virtio
> > spec. I'd suggest to not reuse spec isr here to avoid confusion.
>
> Hmm, yes, that needs some straightening out.
>
> >
> >> + if (ready) {
> >
> > Spec said no interrupt before DRIVER_OK, it looks more simple if we
> > move the interrupt setup to set_status():
> >
> > E.g we can know if we have sufficient vectors and use different
> > mapping policies in advance.
>
> I'll look at that.
>
> >
> >> + struct pds_vdpa_intr_info *intrs = pdsv->vdpa_aux->vdpa_vf->intrs;
> >> + int index = VIRTIO_MSI_NO_VECTOR;
> >> + int i;
> >> +
> >> + /* Tx and Rx queues share interrupts, and they start with
> >> + * even numbers, so only find an interrupt for the even numbered
> >> + * qid, and let the odd number use what the previous queue got.
> >> + */
> >> + if (qid & 0x1) {
> >> + int even = qid & ~0x1;
> >> +
> >> + index = hw->vqs[even].intr_index;
> >> + } else {
> >> + for (i = 0; i < pdsv->vdpa_aux->vdpa_vf->nintrs; i++) {
> >> + if (intrs[i].irq == VIRTIO_MSI_NO_VECTOR) {
> >> + index = i;
> >> + break;
> >> + }
> >> + }
> >> + }
> >> +
> >> + if (qid & 0x1) {
> >> + hw->vqs[qid].intr_index = index;
> >> + } else if (index != VIRTIO_MSI_NO_VECTOR) {
> >> + int irq;
> >> +
> >> + irq = pci_irq_vector(pdev, index);
> >> + snprintf(intrs[index].name, sizeof(intrs[index].name),
> >> + "vdpa-%s-%d", dev_name(dev), index);
> >> +
> >> + err = devm_request_irq(&pdev->dev, irq, pds_vdpa_isr, 0,
> >> + intrs[index].name, &hw->vqs[qid]);
> >> + if (err) {
> >> + dev_info(dev, "%s: no irq for qid %d: %pe\n",
> >> + __func__, qid, ERR_PTR(err));
> >
> > Should we fail?
> >
> >> + } else {
> >> + intrs[index].irq = irq;
> >> + hw->vqs[qid].intr_index = index;
> >> + pds_core_intr_mask(&intr_ctrl[index],
> >> + PDS_CORE_INTR_MASK_CLEAR);
> >
> > I guess the reason that you don't simply use VF MSI-X is the DPU can
> > support vDPA subdevice in the future?
> >
> >> + }
> >> + } else {
> >> + dev_info(dev, "%s: no intr slot for qid %d\n",
> >> + __func__, qid);
> >
> > Do we need to fail here?
> >
> >> + }
> >> +
> >> + /* Pass vq setup info to DSC */
> >> + err = pds_vdpa_cmd_init_vq(pdsv, qid, &hw->vqs[qid]);
> >> + if (err) {
> >> + pds_vdpa_release_irq(pdsv, qid);
> >> + ready = false;
> >> + }
> >> + } else {
> >> + pds_vdpa_release_irq(pdsv, qid);
> >> + (void) pds_vdpa_cmd_reset_vq(pdsv, qid);
> >> + }
> >> +
> >> + hw->vqs[qid].ready = ready;
> >> +}
> >> +
> >> +static bool
> >> +pds_vdpa_get_vq_ready(struct vdpa_device *vdpa_dev, u16 qid)
> >> +{
> >> + struct pds_vdpa_hw *hw = vdpa_to_hw(vdpa_dev);
> >> +
> >> + return hw->vqs[qid].ready;
> >> +}
> >> +
> >> +static int
> >> +pds_vdpa_set_vq_state(struct vdpa_device *vdpa_dev, u16 qid,
> >> + const struct vdpa_vq_state *state)
> >> +{
> >> + struct pds_vdpa_hw *hw = vdpa_to_hw(vdpa_dev);
> >> +
> >> + hw->vqs[qid].used_idx = state->split.avail_index;
> >> + hw->vqs[qid].avail_idx = state->split.avail_index;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int
> >> +pds_vdpa_get_vq_state(struct vdpa_device *vdpa_dev, u16 qid,
> >> + struct vdpa_vq_state *state)
> >> +{
> >> + struct pds_vdpa_hw *hw = vdpa_to_hw(vdpa_dev);
> >> +
> >> + state->split.avail_index = hw->vqs[qid].avail_idx;
> >
> > Who is in charge of reading avail_idx from the hardware?
>
> We didn't have that available in the early FW, so it isn't here yet.
> Work in progerss.
>
> >
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static struct vdpa_notification_area
> >> +pds_vdpa_get_vq_notification(struct vdpa_device *vdpa_dev, u16 qid)
> >> +{
> >> + struct pds_vdpa_device *pdsv = vdpa_to_pdsv(vdpa_dev);
> >> + struct pds_vdpa_hw *hw = vdpa_to_hw(vdpa_dev);
> >> + struct virtio_pci_modern_device *vd_mdev;
> >> + struct vdpa_notification_area area;
> >> +
> >> + area.addr = hw->vqs[qid].notify_pa;
> >> +
> >> + vd_mdev = &pdsv->vdpa_aux->vdpa_vf->vd_mdev;
> >> + if (!vd_mdev->notify_offset_multiplier)
> >> + area.size = PAGE_SIZE;
> >> + else
> >> + area.size = vd_mdev->notify_offset_multiplier;
> >> +
> >> + return area;
> >> +}
> >> +
> >> +static int
> >> +pds_vdpa_get_vq_irq(struct vdpa_device *vdpa_dev, u16 qid)
> >> +{
> >> + struct pds_vdpa_device *pdsv = vdpa_to_pdsv(vdpa_dev);
> >> + struct pds_vdpa_hw *hw = vdpa_to_hw(vdpa_dev);
> >> + int irq = VIRTIO_MSI_NO_VECTOR;
> >> + int index;
> >> +
> >> + if (pdsv->vdpa_aux->vdpa_vf->intrs) {
> >> + index = hw->vqs[qid].intr_index;
> >> + irq = pdsv->vdpa_aux->vdpa_vf->intrs[index].irq;
> >
> > The notification area mapping might only work well when each vq has
> > it's own irq. Otherwise guest may see spurious interrupt which may
> > degrade the performance.
>
> We haven't been expecting to use shared interrupts - are we being overly
> optimistic?
So at least from the codes above, I think we may end up with e.g two
queues that are using the same irq? And the comment said:
/* Tx and Rx queues share interrupts, and they start with
* even numbers, so only find an interrupt for the
even numbered
* qid, and let the odd number use what the previous queue got.
*/
if (qid & 0x1) {
int even = qid & ~0x1;
index = hw->vqs[even].intr_index;
It said TX and RX share interrupts.
>
>
> >
> >> + }
> >> +
> >> + return irq;
> >> +}
> >> +
> >> +static u32
> >> +pds_vdpa_get_vq_align(struct vdpa_device *vdpa_dev)
> >> +{
> >> +
> >> + return PAGE_SIZE;
> >> +}
> >> +
> >> +static u32
> >> +pds_vdpa_get_vq_group(struct vdpa_device *vdpa_dev, u16 idx)
> >> +{
> >> + return 0;
> >> +}
> >> +
> >> +static u64
> >> +pds_vdpa_get_device_features(struct vdpa_device *vdpa_dev)
> >> +{
> >> + struct pds_vdpa_device *pdsv = vdpa_to_pdsv(vdpa_dev);
> >> +
> >> + return le64_to_cpu(pdsv->vdpa_aux->ident.hw_features);
> >> +}
> >> +
> >> +static int
> >> +pds_vdpa_set_driver_features(struct vdpa_device *vdpa_dev, u64 features)
> >> +{
> >> + struct pds_vdpa_device *pdsv = vdpa_to_pdsv(vdpa_dev);
> >> + struct pds_vdpa_hw *hw = vdpa_to_hw(vdpa_dev);
> >> + struct device *dev = &pdsv->vdpa_dev.dev;
> >> + u64 nego_features;
> >> + u64 set_features;
> >> + u64 missing;
> >> + int err;
> >> +
> >> + if (!(features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM)) && features) {
> >> + dev_err(dev, "VIRTIO_F_ACCESS_PLATFORM is not negotiated\n");
> >> + return -EOPNOTSUPP;
> >
> > Should we fail the FEATURE_OK in this case and all the other below
> > error conditions?
>
> Perhaps I'm missing a nuance in the inteface... isn't that what we're
> doing by returning a non-zero status?
Kind of, but to be compliant with the spec, the subsequent get_feature
should return status without FEATURE_OK, I'm not sure this can be
guaranteed:
static u8
pds_vdpa_get_status(struct vdpa_device *vdpa_dev)
{
struct pds_vdpa_hw *hw = vdpa_to_hw(vdpa_dev);
return hw->status;
}
> >> + dev_warn(dev, "Known FW issue - overriding to use max_vq_pairs %d\n",
> >> + hw->num_vqs / 2);
> >
> > Should we fail here? Since the device has a different max_vqp that expected.
>
> Wasn't sure if we should annoy users with a fail here, or try to adjust
> and continue on with something that should work.
I think it's better to fail since it's the behaviour of other vDPA
devices and software virtio devices.
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2022-12-05 7:41 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20221118225656.48309-1-snelson@pensando.io>
[not found] ` <20221118225656.48309-16-snelson@pensando.io>
2022-11-22 3:32 ` [RFC PATCH net-next 15/19] pds_vdpa: virtio bar setup for vdpa Jason Wang
2022-11-22 6:36 ` Jason Wang
[not found] ` <20221118225656.48309-15-snelson@pensando.io>
2022-11-22 3:53 ` [RFC PATCH net-next 14/19] pds_vdpa: Add new PCI VF device for PDS vDPA services Jason Wang
[not found] ` <20221118225656.48309-18-snelson@pensando.io>
2022-11-22 6:32 ` [RFC PATCH net-next 17/19] pds_vdpa: add vdpa config client commands Jason Wang
[not found] ` <20221118225656.48309-19-snelson@pensando.io>
2022-11-22 6:32 ` [RFC PATCH net-next 18/19] pds_vdpa: add support for vdpa and vdpamgmt interfaces Jason Wang
[not found] ` <4b4e7474-5b36-6b2c-a0a5-2e198e1bab0c@amd.com>
2022-12-05 7:40 ` Jason Wang
[not found] ` <20221118225656.48309-20-snelson@pensando.io>
2022-11-22 6:35 ` [RFC PATCH net-next 19/19] pds_vdpa: add Kconfig entry and pds_vdpa.rst Jason Wang
[not found] ` <20221118225656.48309-11-snelson@pensando.io>
[not found] ` <20221128102953.2a61e246@kernel.org>
[not found] ` <f7457718-cff6-e5e1-242e-89b0e118ec3f@amd.com>
2022-11-28 22:57 ` [RFC PATCH net-next 10/19] pds_core: devlink params for enabling VIF support Andrew Lunn
[not found] ` <43eebffe-7ac1-6311-6973-c7a53935e42d@amd.com>
2022-11-28 23:29 ` Andrew Lunn
[not found] ` <20221128153922.2e94958a@kernel.org>
2022-11-29 9:00 ` Leon Romanovsky
2022-11-29 9:13 ` Jiri Pirko
[not found] ` <20221118225656.48309-7-snelson@pensando.io>
[not found] ` <20221128102709.444e3724@kernel.org>
[not found] ` <11905a1a-4559-1e44-59ea-3a02f924419b@amd.com>
[not found] ` <20221128153315.11535ddd@kernel.org>
2022-11-29 0:13 ` [RFC PATCH net-next 06/19] pds_core: add FW update feature to devlink Keller, Jacob E
[not found] ` <2acf60f9-1ce0-4853-7b99-58c890627259@amd.com>
2022-11-29 0:18 ` Keller, Jacob E
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).