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

[Gerd Hoffmann]

  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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/