From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Jones Subject: Re: [PATCH 6/9] Introduce virtio-testdev Date: Thu, 17 Oct 2013 11:51:26 +0200 Message-ID: <20131017095126.GB2172@hawk.usersys.redhat.com> References: <1381767815-12510-1-git-send-email-drjones@redhat.com> <1381767815-12510-7-git-send-email-drjones@redhat.com> <20131017010614.GH24837@cbox> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu, gleb@redhat.com To: Christoffer Dall Return-path: Received: from mx1.redhat.com ([209.132.183.28]:25946 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753426Ab3JQJvk (ORCPT ); Thu, 17 Oct 2013 05:51:40 -0400 Content-Disposition: inline In-Reply-To: <20131017010614.GH24837@cbox> Sender: kvm-owner@vger.kernel.org List-ID: On Wed, Oct 16, 2013 at 06:06:14PM -0700, Christoffer Dall wrote: > There's an interesting mix of space and tab indentations in this file... > > I assume using the kernel coding style would be approprate here... > Actually, that could be added to the readme too. (Or whichever format we > choose). > > Could you fix? I was attempting to adopt the style from the common code lib/*, see lib/string.c, lib/printf.c, lib/argv,c. The style used there is 1. 'four spaces' 2. 'tab' 3. 'tab + four spaces' 4. 'tab + tab' ... However, I'd actually prefer to use the kernel style as well, and I see that not even those three common files are consistent. argv.c only uses spaces. Other x86 files also have a variety of styles (kernel style being one of them). So probably the best choice is to go with kernel style, and to add another cleanup patch that changes lib/* files as well. > > + for (i = 0; i < m->nr; ++i) { > > + volatile u32 *base = (u32 *)compat_ptr(m->addrs[i]); > > + u32 devid32 = base[VIRTIO_MMIO_DEVICEID / 4]; > > do we have readl/writel in this framework? If not, maybe we should to > indicate IO read/writes and ensure the compiler doesn't reorder things > and that we have the necessary memory barriers etc...? > > That would apply to virtio_testdev above as well. OK, I agree it makes sense to add readl and writel in. I'll do that, and then rework my mmio reads to use them. > + printf("Refusing to run with virtio-testdev major = %d, minor = %d\n", > + major, minor); > "Refusing to run"? > How about "incompatible version of virtio-testdev"? Sure. OK. drew