From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47366) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1brdJ7-0006Cw-4m for qemu-devel@nongnu.org; Tue, 04 Oct 2016 23:55:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1brdJ2-0002Xl-Ry for qemu-devel@nongnu.org; Tue, 04 Oct 2016 23:55:48 -0400 Received: from szxga01-in.huawei.com ([58.251.152.64]:55062) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1brdJ1-0002We-Pf for qemu-devel@nongnu.org; Tue, 04 Oct 2016 23:55:44 -0400 From: "Gonglei (Arei)" Date: Wed, 5 Oct 2016 03:51:08 +0000 Message-ID: <33183CC9F5247A488A2544077AF19020B03E65D0@SZXEMA503-MBS.china.huawei.com> References: <37e75a50-1a76-727e-d25a-aea359783a72@hotmail.com> <20161004090513.GB4587@stefanha-x1.localdomain> <20161004161625.GF28028@stefanha-x1.localdomain> In-Reply-To: <20161004161625.GF28028@stefanha-x1.localdomain> Content-Language: zh-CN Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH v11 1/2] virtio-crypto: Add virtio crypto device specification List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi , gong lei Cc: "mst@redhat.com" , "cornelia.huck@de.ibm.com" , "denglingli@chinamobile.com" , Jani Kokkonen , "Ola.Liljedahl@arm.com" , "Varun.Sethi@freescale.com" , "xin.zeng@intel.com" , "brian.a.keating@intel.com" , "liang.j.ma@intel.com" , "john.griffin@intel.com" , "Hanweidong (Randy)" , "mike.caraman@nxp.com" , "Huangweidong (C)" , Luonengjun , "Huangpeng (Peter)" , "are.gonglei@huawei.com" , "Wubin (H)" , "agraf@suse.de" , Claudio Fontana , Shiqing Fan , "Zhoujian (jay, Euler)" , "qemu-devel@nongnu.org" , "virtio-dev@lists.oasis-open.org" > -----Original Message----- > From: Stefan Hajnoczi [mailto:stefanha@redhat.com] > Sent: Wednesday, October 05, 2016 12:16 AM > Subject: Re: [Qemu-devel] [PATCH v11 1/2] virtio-crypto: Add virtio crypt= o > device specification >=20 > On Tue, Oct 04, 2016 at 12:24:02PM +0000, gong lei wrote: > > On 2016/10/4 17:05, Stefan Hajnoczi wrote: > > > On Tue, Oct 04, 2016 at 02:53:25AM +0000, gong lei wrote: > > >> Hi Stefan, > > >> > > >> Thanks for your comments. > > >> > > >>> On Wed, Sep 28, 2016 at 05:08:24PM +0800, Gonglei wrote: > > >>>> /+For scatter/gather list support, a buffer can be represented by = / > > >>>> /virtio_crypto_iovec structure./ > > >>>> /+/ > > >>>> /+The structure is defined as follows:/ > > >>>> /+/ > > >>>> /+\begin{lstlisting}/ > > >>>> /+struct virtio_crypto_iovec {/ > > >>>> /+ /* Guest physical address *// > > >>>> /+ le64 addr;/ > > >>>> /+ /* Length of guest physical address *// > > >>>> /+ le32 len;/ > > >>>> /+/* This marks a buffer as continuing via the next field *// > > >>>> /+#define VIRTIO_CRYPTO_IOVEC_F_NEXT 1/ > > >>>> /+ /* The flags as indicated above VIRTIO_CRYPTO_IOVEC_F_*. *// > > >>>> /+ le32 flags;/ > > >>>> /+ /* Pointer to next struct virtio_crypto_iovec if flags & NEXT *= // > > >>>> /+ le64 next_iovec;/ > > >>>> /+};/ > > >>>> /+\end{lstlisting}/ > > >>> The vring already provides scatter-gather I/O. It is usually not > > >>> necessary to define scatter-gather I/O at the device level. Address= es > > >>> can be translated by the virtio transport (PCI, MMIO, CCW). For > example > > >>> PCI bus addresses with IO-MMU. Therefore it's messy to use guest > > >>> physical addresses in the device specification. > > >>> > > >>>> /+Each data request uses virtio_crypto_hash_data_req structure to > > >>>> store / > > >>>> /information/ > > >>>> /+used to run the HASH operations. The request only occupies one > entry/ > > >>>> /+in the Vring Descriptor Table in the virtio crypto device's data= q, > > >>>> which / > > >>>> /improves/ > > >>>> /+the throughput of data transmitted for the HASH service, so that= the > > >>> virtio / > > >>>> /crypto/ > > >>>> /+device can be better accelerated./ > > >>> Indirect vrings also only require one entry in the descriptor table= . I > > >>> don't understand why you are reinventing scatter-gather I/O. > > >>> > > >>> Am I missing something? > > >> Yes, I knew indirect vring. But for virtio-crypto device' request, e= ach > > >> crypto request include > > >> many buffers. Take algorithm chain' request as an examle, the driver > > >> need to transmit source > > >> data, dstination data, initializaion vector, additional authenticati= on > > >> data, digest result > > >> data etc. to the device. In those data, the source data and destiona= tion > > >> data etc. may be composed > > >> by scatter-gather I/O. That's the background. > > >> > > >> In virtio-crypto spec, we use a structure to store all those content= s so > > >> that we can put all those data > > >> into one entry in the Descriptor Table, otherwise the effect of > > >> acceleration is not good. We > > >> thought other methods to inprove the performance as well, such as > > >> increasing the virng size > > >> of dataq, but the maxinum is 1024 at present, and it maybe influence= the > > >> latency if the vring > > >> size is too big. > > >> > > >> For the indirect ving in existing Virtio spec, is only used for one > > >> buffer, which can't cover > > >> our situation. > > > Indirect vring uses 1 descriptor in the vring descriptor table, but t= hat > > > descriptor points to a separate table that can have many descriptors. > > > You are not limited to just 1 scatter-gather element. > > > > > > Also, I've noticed that the structs in the spec are mixing > > > device-readable and device-writable fields in structs. This is not > > > allowed since virtio requires that all device-readable data comes bef= ore > > > all all device-writable data. > > The spec 2.4.5 says something related, but not forced. > > > > A device MUST NOT write to a device-readable buffer, and a device SHOUL= D > > NOT read a device-writable > > buffer (it MAY do so for debugging or diagnostic purposes). >=20 > Please see: >=20 > 3.2.1.1 Placing Buffers Into The Descriptor Table >=20 > A buffer consists of zero or more read-only physically-contiguous > elements followed by zero or more physically-contiguous write-only > elements (it must have at least one element). >=20 > You must lay out the request (read-only) and response (write-only) parts > in order. It's not possible to have read-only elements after a > write-only element. >=20 > > > I think you'll be able to use indirect vring with the same performanc= e > > > as customer scatter-gather I/O once you think through the layout of > > > device-readable and device-writable data. > > > > > > In order to lay out requests correctly the device-readable headers > > > struct must contain the length of all variable-sized fields. > > > > > > For example, the header would have an iv_len field and the device wil= l > > > expect that number of bytes after the header struct: > > > > > > Device-readable: > > > [header] > > > [iv] > > > [input data] > > > > > > Device-writeable: > > > [output data] > > > [return/error values] > > > > > > One more thing to keep in mind is that according to the VIRTIO > > > specification the device must not rely on the framing (or boundaries = of > > > scatter-gather elements). In the example above it means that the [iv= ] > > > could either be a single vring_desc or it could be multiple descs, or= it > > > could share vring_descs with [header] and/or [input data]. > > > > > > In other words, the device emulation code must not assume elem->in[0]= is > > > [header], elem->in[1] is [iv], and elem->in[2] is [input data]. It h= as > > > to process without giving special meaning to scatter-gather buffer si= ze. > > > > > > Stefan > > > > Yes, I knew that and I did that last year, but I didn't get the good > > performance unfortunately. > > Because we need to handle multiple buffers respectively on one request, > > which add the overhead > > of software layer. It's obviously for vhost-user cryptodev backend in D= PDK. > > > > Maybe I didn't express my meaning clearly. Let's assume that a have 6 > > buffers > > in one crypto request: > > > > BufA: output data, a single buffer > > BufB: output data, a scatter-gather I/O > > BufC: output data, a single buffer > > BufD: in data, a scatter-gather I/O > > BufE: in data, a single buffer > > BufF: in data, a single buffer > > > > We need following steps to translate the request to the device: > > > > 1. add_outbuf() for BufA, which will occupy one entry in the descryptor > > table > > 2.add_outbuf() for BufB, which will occupy one entry in the descryptor > > table if using > > indirect vring, or multiple entries if not using indirecting vring > > 3. add_outbuf() for BufC, which will occupy one entry in the descryptor > > table > > 4. add_inbuf() for BufD, which will occupy one entry in the descryptor > > table if using > > indirect vring, or multiple entries if not using indirecting vring > > 5. add_inbuf() for BufE, which will occupy one entry in the descryptor = table > > 6. add_outbuf() for BufF, which will occupy one entry in the descryptor > > table > > > > It means that one request will occupy 6 entries at least. Am I right? >=20 > No. If these 6 buffers belong to 1 request then you only need a single > indirect descriptor in the vring. >=20 > The indirect descriptor table will have: >=20 > [header] > [BufA] > [BufB1] > [BufB2] > [BufB...] > [BufBN] > [BufC] > [BufD1] > [BufD2] > [BufD...] > [BufDN] > [BufE] > [BufF] > [footer] >=20 I check the code, and you are right. :) I'll fix the spec based on your comments and Virtio requirements though the indirect table will add one allocation and free of struct virng_desc for each request. Regards, -Gonglei