On Thu, Jun 18, 2015 at 12:33:20PM +0200, Gerd Hoffmann wrote:

  Hi,

quoted

quoted

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

quoted

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.

quoted

quoted

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

quoted

quoted

+	vp_dev->res_common = request_capability(pci_dev, common, 0,

Please make limit = sizeof(struct virtio_pci_common_cfg) and not 0.

quoted

+						"virtio-pci-common");
+	vp_dev->res_isr = request_capability(pci_dev, isr, 0,
+						"virtio-pci-isr");

And here, 1.

quoted

+	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