[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 09/12] iov: add iov_get_ptr() to reference ve
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH v3 09/12] iov: add iov_get_ptr() to reference vector data |
Date: |
Thu, 22 Nov 2012 16:18:58 +0100 |
On Thu, Nov 22, 2012 at 1:35 PM, Michael S. Tsirkin <address@hidden> wrote:
> On Thu, Nov 22, 2012 at 12:58:23PM +0100, Stefan Hajnoczi wrote:
>> On Thu, Nov 22, 2012 at 10:34:13AM +0100, Paolo Bonzini wrote:
>> > Il 21/11/2012 19:32, Stefan Hajnoczi ha scritto:
>> > > The iov_get_ptr() data returns a pointer to contiguous data within a
>> > > vector. This allows the caller to manipulate data inside the vector
>> > > without copying in/out using iov_from_buf()/iov_to_buf() when we know
>> > > that data is contiguous within an iovec element.
>> >
>> > This works for you because you have a single byte to write. It would
>> > not work for the SG_IO inhdr, which would need iov_to_buf().
>>
>> Guilty as charged, your honor. :)
>>
>> Let me give a few more details about the motivation for this function:
>>
>> In virtio-blk-data-plane we have an iovec[] array. In the read/write
>> code path we discard the inhdr/outhdr so just the data buffers are left
>> in the iovec[] array. Then we can pass the iovec[] array straight to
>> the Linux AIO functions.
>>
>> Because we're using the iovec[] array for data buffers and we're not
>> allowed to make assumptions about iovec layout, we cannot use
>> iov_to_buf()/iov_from_buf() at the end to fill in the status field - the
>> inhdr has already been discarded from the iovec[] array.
>
> How about using iov_copy?
>
> We have exactly this problem in virtio net if we run
> on host that does not support mergeable buffer header,
> and we solve it by copying out the iovec.
>
>> Since I knew the inhdr is only 1 byte I decided against doing something
>> like dynamically allocating/freeing a QEMUIOVector which could handle
>> spanning iovecs.
>>
>> That said, I think this function is okay as-is because it works fine for
>> non-virtio cases where the caller *knows* the iovec[] layout. As a
>> utility function it stands on its own.
>>
>
> My concern is these APIs are unsafe to use: you get back a pointer and
> you must verify length is not too big before access. Since the iov can
> be manipulated by guest this looks like a good place to put extra
> safeguards.
>
>> > What about the following alternative API:
>> >
>> > void *iov_get_ptr(struct iovec *iov, unsigned int iov_cnt,
>> > ssize_t offset, size_t *bytes);
>> >
>> > which would place the number of valid bytes (i.e. the length of the
>> > remainder of the iovec entry) in *bytes?
>> >
>> > Also, I think that offset == iov_size(iov, iov_cnt) should be
>> > acceptable, and it would be the only case in which *bytes == 0.
>>
>> Hmm...this may be more useful than the version I proposed since the
>> caller can also use it to find out how many bytes are contiguous.
>>
>> Michael: Any concerns if I update the code to reflect Paolo's
>> suggestion?
>>
>> Stefan
>
> I'd prefer something that actually works for all cases
> rather than making callers check and handle failure,
> or reason why it can't fail.
I just sent out a new version of the patch which goes whole hog and
uses a QEMUIOVector to safely access virtio_blk_inhdr regardless of
its size or iovec spanning.
Stefan
- [Qemu-devel] [PATCH v3 09/12] iov: add iov_get_ptr() to reference vector data, (continued)
- [Qemu-devel] [PATCH v3 09/12] iov: add iov_get_ptr() to reference vector data, Stefan Hajnoczi, 2012/11/21
- Re: [Qemu-devel] [PATCH v3 09/12] iov: add iov_get_ptr() to reference vector data, Paolo Bonzini, 2012/11/22
- Re: [Qemu-devel] [PATCH v3 09/12] iov: add iov_get_ptr() to reference vector data, Michael S. Tsirkin, 2012/11/22
- Re: [Qemu-devel] [PATCH v3 09/12] iov: add iov_get_ptr() to reference vector data, Paolo Bonzini, 2012/11/22
- Re: [Qemu-devel] [PATCH v3 09/12] iov: add iov_get_ptr() to reference vector data, Michael S. Tsirkin, 2012/11/22
- Re: [Qemu-devel] [PATCH v3 09/12] iov: add iov_get_ptr() to reference vector data, Paolo Bonzini, 2012/11/22
- Re: [Qemu-devel] [PATCH v3 09/12] iov: add iov_get_ptr() to reference vector data, Michael S. Tsirkin, 2012/11/22
- Re: [Qemu-devel] [PATCH v3 09/12] iov: add iov_get_ptr() to reference vector data, Stefan Hajnoczi, 2012/11/22
- Re: [Qemu-devel] [PATCH v3 09/12] iov: add iov_get_ptr() to reference vector data, Paolo Bonzini, 2012/11/22
- Re: [Qemu-devel] [PATCH v3 09/12] iov: add iov_get_ptr() to reference vector data, Michael S. Tsirkin, 2012/11/22
- Re: [Qemu-devel] [PATCH v3 09/12] iov: add iov_get_ptr() to reference vector data,
Stefan Hajnoczi <=
[Qemu-devel] [PATCH v3 12/12] virtio-blk: add x-data-plane=on|off performance feature, Stefan Hajnoczi, 2012/11/21
[Qemu-devel] [PATCH v3 11/12] dataplane: add virtio-blk data plane code, Stefan Hajnoczi, 2012/11/21
[Qemu-devel] [PATCH v3 08/12] test-iov: add iov_discard() testcase, Stefan Hajnoczi, 2012/11/21
[Qemu-devel] [PATCH v3 03/12] dataplane: add host memory mapping code, Stefan Hajnoczi, 2012/11/21
[Qemu-devel] [PATCH v3 04/12] dataplane: add virtqueue vring code, Stefan Hajnoczi, 2012/11/21