From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932189AbbFRJ7J (ORCPT ); Thu, 18 Jun 2015 05:59:09 -0400 Received: from mx1.redhat.com ([209.132.183.28]:36997 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754916AbbFRJ66 (ORCPT ); Thu, 18 Jun 2015 05:58:58 -0400 Date: Thu, 18 Jun 2015 11:58:55 +0200 From: "Michael S. Tsirkin" To: Gerd Hoffmann Cc: "open list:VIRTIO CORE, NET..." , open list Subject: Re: [PATCH v2] virtio-pci: alloc only resources actually used. Message-ID: <20150618115029-mutt-send-email-mst@redhat.com> References: <1434619654-10142-1-git-send-email-kraxel@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1434619654-10142-1-git-send-email-kraxel@redhat.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Jun 18, 2015 at 11:27:34AM +0200, Gerd Hoffmann wrote: > Move resource allocation from common code to legacy and modern code. > Only request resources actually used, i.e. bar0 in legacy mode and > the regions specified by capabilities in modern mode. > > Signed-off-by: Gerd Hoffmann > --- > drivers/virtio/virtio_pci_common.c | 7 ---- > drivers/virtio/virtio_pci_common.h | 5 +++ > drivers/virtio/virtio_pci_legacy.c | 13 +++++++- > drivers/virtio/virtio_pci_modern.c | 66 +++++++++++++++++++++++++++++++++++++- > 4 files changed, 82 insertions(+), 9 deletions(-) > > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c > index eba1b7a..327a121 100644 > --- a/drivers/virtio/virtio_pci_common.c > +++ b/drivers/virtio/virtio_pci_common.c > @@ -510,10 +510,6 @@ static int virtio_pci_probe(struct pci_dev *pci_dev, > if (rc) > goto err_enable_device; > > - rc = pci_request_regions(pci_dev, "virtio-pci"); > - if (rc) > - goto err_request_regions; > - > if (force_legacy) { > rc = virtio_pci_legacy_probe(vp_dev); > /* Also try modern mode if we can't map BAR0 (no IO space). */ > @@ -543,8 +539,6 @@ err_register: > else > virtio_pci_modern_remove(vp_dev); > err_probe: > - pci_release_regions(pci_dev); > -err_request_regions: > pci_disable_device(pci_dev); > err_enable_device: > kfree(vp_dev); > @@ -562,7 +556,6 @@ static void virtio_pci_remove(struct pci_dev *pci_dev) > else > virtio_pci_modern_remove(vp_dev); > > - pci_release_regions(pci_dev); > pci_disable_device(pci_dev); > } > > diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h > index 28ee4e5..9e744b8 100644 > --- a/drivers/virtio/virtio_pci_common.h > +++ b/drivers/virtio/virtio_pci_common.h > @@ -75,6 +75,11 @@ struct virtio_pci_device { > /* Multiply queue_notify_off by this value. (non-legacy mode). */ > u32 notify_offset_multiplier; > > + struct resource *res_common; > + struct resource *res_isr; > + struct resource *res_device; > + struct resource *res_notify; > + > /* Legacy only field */ > /* the IO mapping for the PCI config space */ > void __iomem *ioaddr; > diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c > index 256a527..48bc979 100644 > --- a/drivers/virtio/virtio_pci_legacy.c > +++ b/drivers/virtio/virtio_pci_legacy.c > @@ -215,6 +215,7 @@ static const struct virtio_config_ops virtio_pci_config_ops = { > int virtio_pci_legacy_probe(struct virtio_pci_device *vp_dev) > { > struct pci_dev *pci_dev = vp_dev->pci_dev; > + int rc; > > /* We only own devices >= 0x1000 and <= 0x103f: leave the rest. */ > if (pci_dev->device < 0x1000 || pci_dev->device > 0x103f) > @@ -226,9 +227,14 @@ int virtio_pci_legacy_probe(struct virtio_pci_device *vp_dev) > return -ENODEV; > } > > + rc = pci_request_region(pci_dev, 0, "virtio-pci-legacy"); > + if (rc) > + return rc; > + > + rc = -ENOMEM; > vp_dev->ioaddr = pci_iomap(pci_dev, 0, 0); > if (!vp_dev->ioaddr) > - return -ENOMEM; > + goto err_iomap; > > vp_dev->isr = vp_dev->ioaddr + VIRTIO_PCI_ISR; > > @@ -246,6 +252,10 @@ int virtio_pci_legacy_probe(struct virtio_pci_device *vp_dev) > vp_dev->del_vq = del_vq; > > return 0; > + > +err_iomap: > + pci_release_region(pci_dev, 0); > + return rc; > } > > void virtio_pci_legacy_remove(struct virtio_pci_device *vp_dev) > @@ -253,4 +263,5 @@ void virtio_pci_legacy_remove(struct virtio_pci_device *vp_dev) > struct pci_dev *pci_dev = vp_dev->pci_dev; > > pci_iounmap(pci_dev, vp_dev->ioaddr); > + pci_release_region(pci_dev, 0); > } > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c > index e88e099..d9976ad 100644 > --- a/drivers/virtio/virtio_pci_modern.c > +++ b/drivers/virtio/virtio_pci_modern.c > @@ -64,6 +64,37 @@ static void vp_iowrite64_twopart(u64 val, > vp_iowrite32(val >> 32, hi); > } > > +static struct resource *request_capability(struct pci_dev *dev, int off, > + int limit, const char *name) > +{ > + u8 bar; > + u32 offset, length; > + > + 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); > + > + if (offset + length < offset || > + offset + length > pci_resource_len(dev, bar)) { > + dev_err(&dev->dev, > + "virtio_pci: virtio capability %u@%u " > + "out of range on bar %i length %lu\n", > + length, offset, bar, > + (unsigned long)pci_resource_len(dev, bar)); > + return NULL; > + } > + > + if (limit && length > limit) > + length = limit; > + I'll have to review the above carefully. Hopefully next week. Any reason you didn't just move code out map_capability to a helper, without changes? Would have made review easier. I don't see reasons to request regions that we aren't going to claim ... > + return request_mem_region(pci_resource_start(dev, bar) + offset, > + length, name); Hmm this seems wrong, resource can be IO, not just memory. > +} > + > static void __iomem *map_capability(struct pci_dev *dev, int off, > size_t minlen, > u32 align, > @@ -131,10 +162,12 @@ static void __iomem *map_capability(struct pci_dev *dev, int off, > } > > p = pci_iomap_range(dev, bar, offset, length); > - if (!p) > + if (!p) { > dev_err(&dev->dev, > "virtio_pci: unable to map virtio %u@%u on bar %i\n", > length, offset, bar); > + return NULL; > + } > return p; > } > > @@ -642,6 +675,22 @@ int virtio_pci_modern_probe(struct virtio_pci_device *vp_dev) > device = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_DEVICE_CFG, > IORESOURCE_IO | IORESOURCE_MEM); > > + vp_dev->res_common = request_capability(pci_dev, common, 0, Please make limit = sizeof(struct virtio_pci_common_cfg) and not 0. > + "virtio-pci-common"); > + vp_dev->res_isr = request_capability(pci_dev, isr, 0, > + "virtio-pci-isr"); And here, 1. > + vp_dev->res_notify = request_capability(pci_dev, notify, 0, > + "virtio-pci-notify"); > + if (device) { > + vp_dev->res_device = request_capability(pci_dev, device, > + PAGE_SIZE, > + "virtio-pci-device"); > + } Please drop {} around a single statement. > + err = -EINVAL; > + if (!vp_dev->res_common || !vp_dev->res_isr || !vp_dev->res_notify || > + (device && !vp_dev->res_device)) > + goto err_reserve; > + > err = -EINVAL; > vp_dev->common = map_capability(pci_dev, common, > sizeof(struct virtio_pci_common_cfg), 4, > @@ -714,6 +763,15 @@ err_map_notify: > err_map_isr: > pci_iounmap(pci_dev, vp_dev->common); > err_map_common: > +err_reserve: > + if (vp_dev->res_common) > + release_resource(vp_dev->res_common); > + if (vp_dev->res_isr) > + release_resource(vp_dev->res_isr); > + if (vp_dev->res_notify) > + release_resource(vp_dev->res_notify); > + if (vp_dev->res_device) > + release_resource(vp_dev->res_device); > return err; > } > > @@ -727,4 +785,10 @@ void virtio_pci_modern_remove(struct virtio_pci_device *vp_dev) > pci_iounmap(pci_dev, vp_dev->notify_base); > pci_iounmap(pci_dev, vp_dev->isr); > pci_iounmap(pci_dev, vp_dev->common); > + > + release_resource(vp_dev->res_common); > + release_resource(vp_dev->res_isr); > + release_resource(vp_dev->res_notify); > + if (vp_dev->res_device) > + release_resource(vp_dev->res_device); > } > -- > 1.8.3.1