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
>
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
