From: "Michael S. Tsirkin" <mst@redhat.com>
To: Jason Wang <jasowang@redhat.com>
Cc: virtualization@lists.linux-foundation.org,
linux-kernel@vger.kernel.org, shahafs@mellanox.com
Subject: Re: [PATCH V2 02/14] virtio-pci: switch to use devres for modern devices
Date: Thu, 26 Nov 2020 08:57:34 -0500 [thread overview]
Message-ID: <20201126084708-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <20201126092604.208033-3-jasowang@redhat.com>
On Thu, Nov 26, 2020 at 05:25:52PM +0800, Jason Wang wrote:
> This patch tries to convert the modern device to use devres to manage
> its resources (iomaps). Before this patch the IO address is mapped
> individually according to the capability. After this patch, we simply
> map the whole BAR.
I think the point of mapping capability was e.g. for devices with
huge BARs. We don't want to waste virtual memory for e.g. 32 bit guests.
And in particular the spec says:
The drivers SHOULD only map part of configuration structure large enough for device operation. The drivers
MUST handle an unexpectedly large length, but MAY check that length is large enough for device operation.
I also wonder how would this interact with cases where device memory is
mapped for different reasons, such as for MSI table access, into userspace
as it has resources such as virtio mem, etc.
E.g. don't e.g. intel CPUs disallow mapping the same address twice
with different attributes?
> This simplify the work of splitting modern device logic into an
> separate module.
>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> drivers/virtio/virtio_pci_common.c | 10 --
> drivers/virtio/virtio_pci_common.h | 2 +
> drivers/virtio/virtio_pci_legacy.c | 13 ++-
> drivers/virtio/virtio_pci_modern.c | 141 +++++++++--------------------
> 4 files changed, 54 insertions(+), 112 deletions(-)
>
> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> index 222d630c41fc..e786701fa1b4 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -527,11 +527,6 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
> INIT_LIST_HEAD(&vp_dev->virtqueues);
> spin_lock_init(&vp_dev->lock);
>
> - /* enable the device */
> - rc = pci_enable_device(pci_dev);
> - if (rc)
> - goto err_enable_device;
> -
> if (force_legacy) {
> rc = virtio_pci_legacy_probe(vp_dev);
> /* Also try modern mode if we can't map BAR0 (no IO space). */
> @@ -559,11 +554,8 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
> err_register:
> if (vp_dev->ioaddr)
> virtio_pci_legacy_remove(vp_dev);
> - else
> - virtio_pci_modern_remove(vp_dev);
> err_probe:
> pci_disable_device(pci_dev);
> -err_enable_device:
> if (reg_dev)
> put_device(&vp_dev->vdev.dev);
> else
> @@ -582,8 +574,6 @@ static void virtio_pci_remove(struct pci_dev *pci_dev)
>
> if (vp_dev->ioaddr)
> virtio_pci_legacy_remove(vp_dev);
> - else
> - virtio_pci_modern_remove(vp_dev);
>
> pci_disable_device(pci_dev);
> put_device(dev);
> diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
> index b2f0eb4067cb..1d23420f7ed6 100644
> --- a/drivers/virtio/virtio_pci_common.h
> +++ b/drivers/virtio/virtio_pci_common.h
> @@ -49,6 +49,8 @@ struct virtio_pci_device {
> u8 __iomem *isr;
>
> /* Modern only fields */
> + /* The IO mapping for the BARs */
> + void __iomem * const *base;
> /* The IO mapping for the PCI config space (non-legacy mode) */
> struct virtio_pci_common_cfg __iomem *common;
> /* Device-specific data (non-legacy mode) */
> diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c
> index d62e9835aeec..890f155ff48c 100644
> --- a/drivers/virtio/virtio_pci_legacy.c
> +++ b/drivers/virtio/virtio_pci_legacy.c
> @@ -214,14 +214,19 @@ int virtio_pci_legacy_probe(struct virtio_pci_device *vp_dev)
> struct pci_dev *pci_dev = vp_dev->pci_dev;
> int rc;
>
> + rc = pci_enable_device(pci_dev);
> + if (rc)
> + return rc;
> +
> + rc = -ENODEV;
> /* We only own devices >= 0x1000 and <= 0x103f: leave the rest. */
> if (pci_dev->device < 0x1000 || pci_dev->device > 0x103f)
> - return -ENODEV;
> + goto err_id;
>
> if (pci_dev->revision != VIRTIO_PCI_ABI_VERSION) {
> printk(KERN_ERR "virtio_pci: expected ABI version %d, got %d\n",
> VIRTIO_PCI_ABI_VERSION, pci_dev->revision);
> - return -ENODEV;
> + goto err_id;
> }
>
> rc = dma_set_mask(&pci_dev->dev, DMA_BIT_MASK(64));
> @@ -241,7 +246,7 @@ int virtio_pci_legacy_probe(struct virtio_pci_device *vp_dev)
>
> rc = pci_request_region(pci_dev, 0, "virtio-pci-legacy");
> if (rc)
> - return rc;
> + goto err_id;
>
> rc = -ENOMEM;
> vp_dev->ioaddr = pci_iomap(pci_dev, 0, 0);
> @@ -267,6 +272,8 @@ int virtio_pci_legacy_probe(struct virtio_pci_device *vp_dev)
>
> err_iomap:
> pci_release_region(pci_dev, 0);
> +err_id:
> + pci_disable_device(pci_dev);
> return rc;
> }
>
> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> index df1481fd400c..33cc21b818de 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
> @@ -63,15 +63,15 @@ static void vp_iowrite64_twopart(u64 val,
> vp_iowrite32(val >> 32, hi);
> }
>
> -static void __iomem *map_capability(struct pci_dev *dev, int off,
> +static void __iomem *map_capability(struct virtio_pci_device *vp_dev, int off,
> size_t minlen,
> u32 align,
> - u32 start, u32 size,
> + u32 size,
> size_t *len)
> {
> + struct pci_dev *dev = vp_dev->pci_dev;
> u8 bar;
> u32 offset, length;
> - void __iomem *p;
>
> pci_read_config_byte(dev, off + offsetof(struct virtio_pci_cap,
> bar),
> @@ -81,31 +81,13 @@ static void __iomem *map_capability(struct pci_dev *dev, int off,
> pci_read_config_dword(dev, off + offsetof(struct virtio_pci_cap, length),
> &length);
>
> - if (length <= start) {
> - dev_err(&dev->dev,
> - "virtio_pci: bad capability len %u (>%u expected)\n",
> - length, start);
> - return NULL;
> - }
> -
> - if (length - start < minlen) {
> + if (length < minlen) {
> dev_err(&dev->dev,
> "virtio_pci: bad capability len %u (>=%zu expected)\n",
> length, minlen);
> return NULL;
> }
>
> - length -= start;
> -
> - if (start + offset < offset) {
why kill wrap around?
> - 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",
> @@ -129,12 +111,7 @@ static void __iomem *map_capability(struct pci_dev *dev, int off,
> 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);
> - return p;
> + return vp_dev->base[bar] + offset;
> }
>
> /* virtio config->get_features() implementation */
> @@ -369,27 +346,21 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
> vp_iowrite64_twopart(virtqueue_get_used_addr(vq),
> &cfg->queue_used_lo, &cfg->queue_used_hi);
>
> - if (vp_dev->notify_base) {
> - /* offset should not wrap */
> - if ((u64)off * vp_dev->notify_offset_multiplier + 2
> - > vp_dev->notify_len) {
> - dev_warn(&vp_dev->pci_dev->dev,
> - "bad notification offset %u (x %u) "
> - "for queue %u > %zd",
> - off, vp_dev->notify_offset_multiplier,
> - index, vp_dev->notify_len);
> - err = -EINVAL;
> - goto err_map_notify;
> - }
> - vq->priv = (void __force *)vp_dev->notify_base +
> - off * vp_dev->notify_offset_multiplier;
> - } else {
> - vq->priv = (void __force *)map_capability(vp_dev->pci_dev,
> - vp_dev->notify_map_cap, 2, 2,
> - off * vp_dev->notify_offset_multiplier, 2,
> - NULL);
> + /* offset should not wrap */
> + if ((u64)off * vp_dev->notify_offset_multiplier + 2
> + > vp_dev->notify_len) {
> + dev_warn(&vp_dev->pci_dev->dev,
> + "bad notification offset %u (x %u) "
> + "for queue %u > %zd",
> + off, vp_dev->notify_offset_multiplier,
> + index, vp_dev->notify_len);
> + err = -EINVAL;
> + goto err_map_notify;
> }
>
> + vq->priv = (void __force *)vp_dev->notify_base +
> + off * vp_dev->notify_offset_multiplier;
> +
> if (!vq->priv) {
> err = -ENOMEM;
> goto err_map_notify;
> @@ -400,15 +371,12 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
> msix_vec = vp_ioread16(&cfg->queue_msix_vector);
> if (msix_vec == VIRTIO_MSI_NO_VECTOR) {
> err = -EBUSY;
> - goto err_assign_vector;
> + goto err_map_notify;
> }
> }
>
> return vq;
>
> -err_assign_vector:
> - if (!vp_dev->notify_base)
> - pci_iounmap(vp_dev->pci_dev, (void __iomem __force *)vq->priv);
> err_map_notify:
> vring_del_virtqueue(vq);
> return ERR_PTR(err);
> @@ -454,9 +422,6 @@ static void del_vq(struct virtio_pci_vq_info *info)
> vp_ioread16(&cfg->queue_msix_vector);
> }
>
> - if (!vp_dev->notify_base)
> - pci_iounmap(vp_dev->pci_dev, (void __force __iomem *)vq->priv);
> -
> vring_del_virtqueue(vq);
> }
>
> @@ -700,6 +665,10 @@ int virtio_pci_modern_probe(struct virtio_pci_device *vp_dev)
>
> check_offsets();
>
> + err = pcim_enable_device(pci_dev);
> + if (err)
> + return err;
> +
> /* We only own devices >= 0x1000 and <= 0x107f: leave the rest. */
> if (pci_dev->device < 0x1000 || pci_dev->device > 0x107f)
> return -ENODEV;
> @@ -753,23 +722,24 @@ int virtio_pci_modern_probe(struct virtio_pci_device *vp_dev)
> IORESOURCE_IO | IORESOURCE_MEM,
> &vp_dev->modern_bars);
>
> - err = pci_request_selected_regions(pci_dev, vp_dev->modern_bars,
> - "virtio-pci-modern");
> + err = pcim_iomap_regions(pci_dev, vp_dev->modern_bars,
> + "virtio-pci-modern");
> if (err)
> return err;
>
> + vp_dev->base = pcim_iomap_table(pci_dev);
> +
> err = -EINVAL;
> - vp_dev->common = map_capability(pci_dev, common,
> + vp_dev->common = map_capability(vp_dev, common,
> sizeof(struct virtio_pci_common_cfg), 4,
> - 0, sizeof(struct virtio_pci_common_cfg),
> + sizeof(struct virtio_pci_common_cfg),
> NULL);
> if (!vp_dev->common)
> - goto err_map_common;
> - vp_dev->isr = map_capability(pci_dev, isr, sizeof(u8), 1,
> - 0, 1,
> - NULL);
> + goto err;
> + vp_dev->isr = map_capability(vp_dev, isr, sizeof(u8), 1,
> + 1, NULL);
> if (!vp_dev->isr)
> - goto err_map_isr;
> + goto err;
>
> /* Read notify_off_multiplier from config space. */
> pci_read_config_dword(pci_dev,
> @@ -787,29 +757,21 @@ int virtio_pci_modern_probe(struct virtio_pci_device *vp_dev)
> 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) {
> - vp_dev->notify_base = map_capability(pci_dev, notify, 2, 2,
> - 0, notify_length,
> - &vp_dev->notify_len);
> - if (!vp_dev->notify_base)
> - goto err_map_notify;
> - } else {
> - vp_dev->notify_map_cap = notify;
> - }
> + vp_dev->notify_base = map_capability(vp_dev, notify, 2, 2,
> + notify_length,
> + &vp_dev->notify_len);
> + if (!vp_dev->notify_base)
> + goto err;
>
> /* Again, we don't know how much we should map, but PAGE_SIZE
> * is more than enough for all existing devices.
> */
> if (device) {
> - vp_dev->device = map_capability(pci_dev, device, 0, 4,
> - 0, PAGE_SIZE,
> + vp_dev->device = map_capability(vp_dev, device, 0, 4,
> + PAGE_SIZE,
> &vp_dev->device_len);
> if (!vp_dev->device)
> - goto err_map_device;
> + goto err;
>
> vp_dev->vdev.config = &virtio_pci_config_ops;
> } else {
> @@ -822,26 +784,7 @@ int virtio_pci_modern_probe(struct virtio_pci_device *vp_dev)
>
> return 0;
>
> -err_map_device:
> - if (vp_dev->notify_base)
> - pci_iounmap(pci_dev, vp_dev->notify_base);
> -err_map_notify:
> - pci_iounmap(pci_dev, vp_dev->isr);
> -err_map_isr:
> - pci_iounmap(pci_dev, vp_dev->common);
> -err_map_common:
> +err:
> return err;
> }
>
> -void virtio_pci_modern_remove(struct virtio_pci_device *vp_dev)
> -{
> - struct pci_dev *pci_dev = vp_dev->pci_dev;
> -
> - if (vp_dev->device)
> - pci_iounmap(pci_dev, vp_dev->device);
> - if (vp_dev->notify_base)
> - pci_iounmap(pci_dev, vp_dev->notify_base);
> - pci_iounmap(pci_dev, vp_dev->isr);
> - pci_iounmap(pci_dev, vp_dev->common);
> - pci_release_selected_regions(pci_dev, vp_dev->modern_bars);
> -}
> --
> 2.25.1
next prev parent reply other threads:[~2020-11-26 13:57 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-26 9:25 [PATCH V2 00/14] vDPA driver for virtio-pci device Jason Wang
2020-11-26 9:25 ` [PATCH V2 01/14] virtio-pci: do not access iomem via virtio_pci_device directly Jason Wang
2020-11-26 13:46 ` Michael S. Tsirkin
2020-11-27 2:50 ` Jason Wang
2020-11-26 9:25 ` [PATCH V2 02/14] virtio-pci: switch to use devres for modern devices Jason Wang
2020-11-26 13:57 ` Michael S. Tsirkin [this message]
2020-11-27 2:54 ` Jason Wang
2020-11-26 9:25 ` [PATCH V2 03/14] virtio-pci: split out modern device Jason Wang
2020-11-26 9:25 ` [PATCH V2 04/14] virtio-pci: move the notification sanity check to vp_modern_probe() Jason Wang
2020-11-26 9:25 ` [PATCH V2 05/14] virtio-pci-modern: introduce vp_modern_set_queue_vector() Jason Wang
2020-11-26 9:25 ` [PATCH V2 06/14] virtio-pci-modern: introduce vp_modern_queue_address() Jason Wang
2020-11-26 9:25 ` [PATCH V2 07/14] virtio-pci-modern: introduce helper to set/get queue_enable Jason Wang
2020-11-26 9:25 ` [PATCH V2 08/14] virtio-pci-modern: introduce helper for setting/geting queue size Jason Wang
2020-11-26 9:25 ` [PATCH V2 09/14] virtio-pci-modern: introduce helper for getting queue nums Jason Wang
2020-11-26 9:26 ` [PATCH V2 10/14] virtio-pci-modern: introduce helper to get notification offset Jason Wang
2020-11-26 9:26 ` [PATCH V2 11/14] virtio-pci: introduce modern device module Jason Wang
2020-11-26 9:26 ` [PATCH V2 12/14] vdpa: set the virtqueue num during register Jason Wang
2020-11-26 9:26 ` [PATCH V2 13/14] virtio_vdpa: don't warn when fail to disable vq Jason Wang
2020-11-26 9:26 ` [PATCH V2 14/14] vdpa: introduce virtio pci driver Jason Wang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201126084708-mutt-send-email-mst@kernel.org \
--to=mst@redhat.com \
--cc=jasowang@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=shahafs@mellanox.com \
--cc=virtualization@lists.linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).