From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60491) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1brP54-0001Cp-LY for qemu-devel@nongnu.org; Tue, 04 Oct 2016 08:44:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1brP50-0007ah-82 for qemu-devel@nongnu.org; Tue, 04 Oct 2016 08:44:21 -0400 Received: from snt004-omc4s35.hotmail.com ([65.55.90.238]:57401) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1brP4z-0007Wi-QS for qemu-devel@nongnu.org; Tue, 04 Oct 2016 08:44:18 -0400 From: gong lei Date: Tue, 4 Oct 2016 12:24:02 +0000 Message-ID: References: <37e75a50-1a76-727e-d25a-aea359783a72@hotmail.com> <20161004090513.GB4587@stefanha-x1.localdomain> In-Reply-To: <20161004090513.GB4587@stefanha-x1.localdomain> Content-Language: en-US Content-Type: text/plain; charset="Windows-1252" Content-ID: <4B7060C39FF1304FB811A2475FF9CE3C@apcprd06.prod.outlook.com> 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 Cc: "mst@redhat.com" , "cornelia.huck@de.ibm.com" , "denglingli@chinamobile.com" , "Jani.Kokkonen@huawei.com" , "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@huawei.com" , "mike.caraman@nxp.com" , "weidong.huang@huawei.com" , "luonengjun@huawei.com" , "peter.huangpeng@huawei.com" , "are.gonglei@huawei.com" , "wu.wubin@huawei.com" , "agraf@suse.de" , "Claudio.Fontana@huawei.com" , "Shiqing.Fan@huawei.com" , "jianjay.zhou@huawei.com" , "arei.gonglei@huawei.com" , "qemu-devel@nongnu.org" , "virtio-dev@lists.oasis-open.org" 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. Addresses >>> can be translated by the virtio transport (PCI, MMIO, CCW). For exampl= e >>> 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 dataq, >>>> 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, each >> crypto request include >> many buffers. Take algorithm chain' request as an examle, the driver >> need to transmit source >> data, dstination data, initializaion vector, additional authentication >> data, digest result >> data etc. to the device. In those data, the source data and destionation >> 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 contents 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 that > 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 before > 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 SHOULD=20 NOT read a device-writable buffer (it MAY do so for debugging or diagnostic purposes). > I think you'll be able to use indirect vring with the same performance > 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 will > 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 has > to process without giving special meaning to scatter-gather buffer size. > > Stefan Yes, I knew that and I did that last year, but I didn't get the good=20 performance unfortunately. Because we need to handle multiple buffers respectively on one request,=20 which add the overhead of software layer. It's obviously for vhost-user cryptodev backend in DPDK. Maybe I didn't express my meaning clearly. Let's assume that a have 6=20 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=20 table 2.add_outbuf() for BufB, which will occupy one entry in the descryptor=20 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=20 table 4. add_inbuf() for BufD, which will occupy one entry in the descryptor=20 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 tabl= e 6. add_outbuf() for BufF, which will occupy one entry in the descryptor=20 table It means that one request will occupy 6 entries at least. Am I right? --=20 Regards, -Gonglei