Re: [PATCH v2] virtio-pci: alloc only resources actually used.

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Jun 18, 2015 at 12:33:20PM +0200, Gerd Hoffmann wrote:
>   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.

So use a helper, call it from both places with slightly
different parameters.

> > 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.

In case of notify I guess we can limit the resource to 64K VQs,
will help keep it simple.

> > > +	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
>