Messages in this thread | | | Date | Wed, 23 Mar 2011 22:05:56 -0500 | From | Anthony Liguori <> | Subject | Re: [PATCH, RFC] virtio_blk: add cache control support |
| |
On 03/17/2011 12:06 AM, Rusty Russell wrote: > On Wed, 16 Mar 2011 15:09:58 +0100, Christoph Hellwig<hch@lst.de> 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. > > >
| |