Re: [PATCH, RFC] virtio_blk: add cache control support

From: Anthony Liguori
Date: Wed Mar 23 2011 - 23:06:18 EST

On 03/17/2011 12:06 AM, Rusty Russell wrote:
On Wed, 16 Mar 2011 15:09:58 +0100, Christoph Hellwig<hch@xxxxxx> 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.


Anthony Liguori


To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at