From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754435AbbFRKde (ORCPT ); Thu, 18 Jun 2015 06:33:34 -0400 Received: from mx1.redhat.com ([209.132.183.28]:45996 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754364AbbFRKdX (ORCPT ); Thu, 18 Jun 2015 06:33:23 -0400 Message-ID: <1434623600.4968.29.camel@redhat.com> Subject: Re: [PATCH v2] virtio-pci: alloc only resources actually used. From: Gerd Hoffmann To: "Michael S. Tsirkin" Cc: "open list:VIRTIO CORE, NET..." , open list Date: Thu, 18 Jun 2015 12:33:20 +0200 In-Reply-To: <20150618115029-mutt-send-email-mst@redhat.com> References: <1434619654-10142-1-git-send-email-kraxel@redhat.com> <20150618115029-mutt-send-email-mst@redhat.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, > > + 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. Doesn't work that easily as there are different things to check. request_capability verifies the capability itself only, map_capability has a bunch of additional range checks because it supports partial maps. > I don't see reasons to request regions that we aren't going to > claim ... I had it that way first, simply calling request_mem_region from map_capability. But then you get tons of notify entries in /proc/iomem, and keeping track of the requested regions (for cleanup in virtio_pci_modern_remove) also is more complicated than with a simple set of 4 fixed regions. > > + return request_mem_region(pci_resource_start(dev, bar) + offset, > > + length, name); > > Hmm this seems wrong, resource can be IO, not just memory. Right (not the case in the current qemu implementation, but still ...). > > + 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. I'll fix them for the next version. cheers, Gerd From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gerd Hoffmann Subject: Re: [PATCH v2] virtio-pci: alloc only resources actually used. Date: Thu, 18 Jun 2015 12:33:20 +0200 Message-ID: <1434623600.4968.29.camel@redhat.com> References: <1434619654-10142-1-git-send-email-kraxel@redhat.com> <20150618115029-mutt-send-email-mst@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20150618115029-mutt-send-email-mst@redhat.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: virtualization-bounces@lists.linux-foundation.org Errors-To: virtualization-bounces@lists.linux-foundation.org To: "Michael S. Tsirkin" Cc: open list , "open list:VIRTIO CORE, NET..." List-Id: virtualization@lists.linuxfoundation.org Hi, > > + 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. Doesn't work that easily as there are different things to check. request_capability verifies the capability itself only, map_capability has a bunch of additional range checks because it supports partial maps. > I don't see reasons to request regions that we aren't going to > claim ... I had it that way first, simply calling request_mem_region from map_capability. But then you get tons of notify entries in /proc/iomem, and keeping track of the requested regions (for cleanup in virtio_pci_modern_remove) also is more complicated than with a simple set of 4 fixed regions. > > + return request_mem_region(pci_resource_start(dev, bar) + offset, > > + length, name); > > Hmm this seems wrong, resource can be IO, not just memory. Right (not the case in the current qemu implementation, but still ...). > > + 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. I'll fix them for the next version. cheers, Gerd