On Thu, Jan 02, 2014 at 09:27:18AM -0800, Christoffer Dall wrote:
> 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?
yup. done.
>
> > >
> > > > +
> > > > + 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
