Date:   Tue, 22 Nov 2022 11:32:24 +0800
From:   Jason Wang <jasowang@...hat.com>
To:     Shannon Nelson <snelson@...sando.io>, netdev@...r.kernel.org,
        davem@...emloft.net, kuba@...nel.org, mst@...hat.com,
        virtualization@...ts.linux-foundation.org
Cc:     drivers@...sando.io
Subject: Re: [RFC PATCH net-next 15/19] pds_vdpa: virtio bar setup for vdpa


在 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@...sando.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),
> +			      &notify_length);
> +
> +	pci_read_config_dword(pci_dev,
> +			      notify + offsetof(struct virtio_pci_notify_cap,
> +						cap.offset),
> +			      &notify_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);
> +}

Powered by blists - more mailing lists