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

[Christoffer Dall <christoffer.dall@linaro.org>]

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