From mboxrd@z Thu Jan  1 00:00:00 1970
From: Christoffer Dall <christoffer.dall@linaro.org>
Subject: Re: [PATCH 8/9] Introduce virtio-testdev
Date: Thu, 2 Jan 2014 09:27:18 -0800
Message-ID: <20140102172718.GG27806@cbox>
References: <1386175377-23086-1-git-send-email-drjones@redhat.com>
 <1386175377-23086-9-git-send-email-drjones@redhat.com>
 <20131229063117.GG13601@cbox>
 <20140102161655.GF9725@hawk.usersys.redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org
To: Andrew Jones <drjones@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mail-pd0-f178.google.com ([209.85.192.178]:39551 "EHLO
	mail-pd0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1752400AbaABR1S (ORCPT <rfc822;kvm@vger.kernel.org>);
	Thu, 2 Jan 2014 12:27:18 -0500
Received: by mail-pd0-f178.google.com with SMTP id y10so14404243pdj.37
        for <kvm@vger.kernel.org>; Thu, 02 Jan 2014 09:27:17 -0800 (PST)
Content-Disposition: inline
In-Reply-To: <20140102161655.GF9725@hawk.usersys.redhat.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.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