On Thu, Jan 02, 2014 at 05:16:56PM +0100, Andrew Jones wrote:
> On Sat, Dec 28, 2013 at 10:31:17PM -0800, Christoffer Dall wrote:
> > > +
> > > +/*
> > > + * We have to write all args; nargs, nrets, ... first to
> > > + * avoid executing token's operation until all args are in
> >
> > the token's ?
>
> ok
>
> >
> > > + * place. Then issue the op, and then read the rets. Reading
> >
> > rets? return values?
>
> ok
>
> > > +
> > > +#define to_virtio_mmio_dev(vdev) \
> > > + container_of(vdev, struct virtio_mmio_dev, vdev)
> >
> > argh, macro pain, can you rename the parameter to _vdev or vdev_ptr or
> > something like that?
>
> ok
>
> > > +static struct virtio_dev *virtio_mmio_bind(const struct iomap *m, u32 device)
> > > +{
> > > + struct virtio_mmio_dev *vmdev;
> > > + void *page;
> > > + u32 devid, i;
> > > +
> > > + page = alloc_page();
> > > + vmdev = page;
> > > + vmdev->vdev.config = page + sizeof(struct virtio_mmio_dev);
> > > +
> > > + vmdev->vdev.id.device = device;
> > > + vmdev->vdev.id.vendor = -1;
> > > + vmdev->vdev.config->get = vm_get;
> > > + vmdev->vdev.config->set = vm_set;
> > > +
> > > + device &= 0xffff;
> >
> > what is this mask again? Is this to correspond to the virtio PCI device
> > ID specs, but then shouldn't we also check the range, seems strange to
> > me to just ignore upper-bits garbage?
>
> Right, we're ignoring the upper bits to comply with the spec (and to
> actually find the device if the upper bits weren't zero). Strangely, at
> least to me, struct virtio_device_id in the kernel has device as a u32,
> even though the spec has it as a u16. So I allow u32 devices to be put in
> the device id, but then throw out the upper bits before searching.
>
Can you use a define for this mask instead then?
> >
> > > +
> > > + for (i = 0; i < m->nr; ++i) {
> > > + vmdev->base = compat_ptr(m->addrs[i]);
> > > + devid = readl(vmdev->base + VIRTIO_MMIO_DEVICE_ID);
> > > + if ((devid & 0xffff) == device)
> > > + break;
> > > + }
> > > +
> > > + if (i >= m->nr) {
> >
> > this can just be 'if (i == m->nr)' right?
>
> yeah, I just prefer to choose the broader condition when possible.
>
hmmm.
> >
> > > + printf("%s: Can't find device 0x%x.\n", __func__, device);
> >
> > I would ditch the '.'
>
> ditched
>
Thanks,
-Christoffer
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
