From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=46841 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Q2asG-00058B-9l for qemu-devel@nongnu.org; Wed, 23 Mar 2011 23:06:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Q2asE-0004mt-PU for qemu-devel@nongnu.org; Wed, 23 Mar 2011 23:06:11 -0400 Received: from e38.co.us.ibm.com ([32.97.110.159]:39264) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Q2asE-0004mT-K6 for qemu-devel@nongnu.org; Wed, 23 Mar 2011 23:06:10 -0400 Received: from d03relay05.boulder.ibm.com (d03relay05.boulder.ibm.com [9.17.195.107]) by e38.co.us.ibm.com (8.14.4/8.13.1) with ESMTP id p2O33Lnm005429 for ; Wed, 23 Mar 2011 21:03:21 -0600 Received: from d03av01.boulder.ibm.com (d03av01.boulder.ibm.com [9.17.195.167]) by d03relay05.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p2O367bi106560 for ; Wed, 23 Mar 2011 21:06:07 -0600 Received: from d03av01.boulder.ibm.com (loopback [127.0.0.1]) by d03av01.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p2O366pW002033 for ; Wed, 23 Mar 2011 21:06:07 -0600 Message-ID: <4D8AB514.5020306@us.ibm.com> Date: Wed, 23 Mar 2011 22:05:56 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <20110315141049.GA30627@lst.de> <20110315141644.GA30803@lst.de> <87y64fhfjw.fsf@rustcorp.com.au> <20110316140958.GB21877@lst.de> <877hbygwu7.fsf@rustcorp.com.au> In-Reply-To: <877hbygwu7.fsf@rustcorp.com.au> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH, RFC] virtio_blk: add cache control support List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Rusty Russell Cc: kwolf@redhat.com, stefanha@gmail.com, linux-kernel@vger.kernel.org, qemu-devel@nongnu.org, Christian Borntraeger , prerna@linux.vnet.ibm.com, Christoph Hellwig On 03/17/2011 12:06 AM, Rusty Russell wrote: > On Wed, 16 Mar 2011 15:09:58 +0100, Christoph Hellwig wrote: >> On Wed, Mar 16, 2011 at 02:39:39PM +1030, Rusty Russell wrote: >>>> + if (strncmp(buf, "write through", sizeof("write through") - 1) == 0) { >>>> + ; >>>> + } else if (strncmp(buf, "write back", sizeof("write back") - 1) == 0) { >>> Is there a reason we're not letting gcc and/or strcmp do the >>> optimization work here? >> I'm happ to switch strcmp. > Of course, that's assuming buf is nul terminated. > >>>> + vdev->config->set(vdev, offsetof(struct virtio_blk_config, features), >>>> + &features, sizeof(features)); >>>> + >>>> + vdev->config->get(vdev, offsetof(struct virtio_blk_config, features), >>>> + &features2, sizeof(features2)); >>>> + >>>> + if ((features& VIRTIO_BLK_RT_WCE) != >>>> + (features2& VIRTIO_BLK_RT_WCE)) >>>> + return -EIO; >>> This seems like a debugging check you left in. Or do you suspect >>> some issues? >> No, it's intentional. config space writes can't return errors, so we need >> to check that the value has really changed. I'll add a comment explaining it. > OK, under what circumstances could it fail? > > If you're using this mechanism to indicate that the host doesn't support > the feature, that's making an assumption about the nature of config > space writes which isn't true for non-PCI virtio. > > ie. lguest and S/390 don't trap writes to config space. > > Or perhaps they should? But we should be explicit about needing it... I don't think we ever operated on the assumption that config space writes would trap. I don't think adding it is the right thing either because you can do byte access to the config space which makes atomicity difficult. Any reason not to use a control queue to negotiate dynamic features? The authorative source of what the currently enabled features are can still be config space but the guest's enabling or disabling of a feature ought to be a control queue message. Regards, Anthony Liguori > Thanks, > Rusty. > > >