Re: [PATCH 8/9] Introduce virtio-testdev

[Andrew Jones <drjones@redhat.com>]

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.

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

> 
> > +		printf("%s: Can't find device 0x%x.\n", __func__, device);
> 
> I would ditch the '.'

ditched

drew