[Qemu-devel] Question about add AF_ALG backend for virtio-crypto

[Qemu-devel] Question about add AF_ALG backend for virtio-crypto

[Longpeng (Mike)]

Hi guys,

I'm one of Gonglei's virtio-crypto project members, and we plan to add a AF_ALG
backend for virtio-crypto(there's only builtin-backend currently).

I found that Catalin, Paolo and Stefan had discussed about this in 2015
(http://www.spinics.net/lists/kvm/msg115457.html), but it seems that Catalin
didn't do it, so I'm confuse about wether it is need to add a AF_ALG backend.

Do you have any suggestion? Thanks :)

-- 
Regards,
Longpeng(Mike)

Re: [Qemu-devel] Question about add AF_ALG backend for virtio-crypto

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 568 bytes --]

On Mon, Jan 09, 2017 at 03:04:55PM +0800, Longpeng (Mike) wrote:
> I'm one of Gonglei's virtio-crypto project members, and we plan to add a AF_ALG
> backend for virtio-crypto(there's only builtin-backend currently).
> 
> I found that Catalin, Paolo and Stefan had discussed about this in 2015
> (http://www.spinics.net/lists/kvm/msg115457.html), but it seems that Catalin
> didn't do it, so I'm confuse about wether it is need to add a AF_ALG backend.
> 
> Do you have any suggestion? Thanks :)

I have no objections to an AF_ALG backend in QEMU.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] Question about add AF_ALG backend for virtio-crypto

[Daniel P. Berrange]

On Mon, Jan 09, 2017 at 01:43:10PM +0000, Stefan Hajnoczi wrote:
> On Mon, Jan 09, 2017 at 03:04:55PM +0800, Longpeng (Mike) wrote:
> > I'm one of Gonglei's virtio-crypto project members, and we plan to add a AF_ALG
> > backend for virtio-crypto(there's only builtin-backend currently).
> > 
> > I found that Catalin, Paolo and Stefan had discussed about this in 2015
> > (http://www.spinics.net/lists/kvm/msg115457.html), but it seems that Catalin
> > didn't do it, so I'm confuse about wether it is need to add a AF_ALG backend.
> > 
> > Do you have any suggestion? Thanks :)
> 
> I have no objections to an AF_ALG backend in QEMU.

Rather than do another backend for virtio-crypto, IMHO, we should have
an AF_ALG impl of the crypto/ APIs. That way any potential performance
benefits will enhance our LUKS encryption code too.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

Re: [Qemu-devel] Question about add AF_ALG backend for virtio-crypto

[Gonglei (Arei)]

Hi,

> 
> On Mon, Jan 09, 2017 at 01:43:10PM +0000, Stefan Hajnoczi wrote:
> > On Mon, Jan 09, 2017 at 03:04:55PM +0800, Longpeng (Mike) wrote:
> > > I'm one of Gonglei's virtio-crypto project members, and we plan to add a
> AF_ALG
> > > backend for virtio-crypto(there's only builtin-backend currently).
> > >
> > > I found that Catalin, Paolo and Stefan had discussed about this in 2015
> > > (http://www.spinics.net/lists/kvm/msg115457.html), but it seems that
> Catalin
> > > didn't do it, so I'm confuse about wether it is need to add a AF_ALG
> backend.
> > >
> > > Do you have any suggestion? Thanks :)
> >
> > I have no objections to an AF_ALG backend in QEMU.
> 
> Rather than do another backend for virtio-crypto, IMHO, we should have
> an AF_ALG impl of the crypto/ APIs. That way any potential performance
> benefits will enhance our LUKS encryption code too.
> 
According to the currently schemas of crypto/ APIs, we can't choose the
specific backend dynamically. This is a limitation for virtio-crypto device I think.

If an AF_ALG backend is supported, we can directly use the hardware
accelerators based on LKCF, which makes many sense.

Regards,
-Gonglei

Re: [Qemu-devel] Question about add AF_ALG backend for virtio-crypto

[Daniel P. Berrange]

On Tue, Jan 10, 2017 at 09:03:45AM +0000, Gonglei (Arei) wrote:
> Hi,
> 
> > 
> > On Mon, Jan 09, 2017 at 01:43:10PM +0000, Stefan Hajnoczi wrote:
> > > On Mon, Jan 09, 2017 at 03:04:55PM +0800, Longpeng (Mike) wrote:
> > > > I'm one of Gonglei's virtio-crypto project members, and we plan to add a
> > AF_ALG
> > > > backend for virtio-crypto(there's only builtin-backend currently).
> > > >
> > > > I found that Catalin, Paolo and Stefan had discussed about this in 2015
> > > > (http://www.spinics.net/lists/kvm/msg115457.html), but it seems that
> > Catalin
> > > > didn't do it, so I'm confuse about wether it is need to add a AF_ALG
> > backend.
> > > >
> > > > Do you have any suggestion? Thanks :)
> > >
> > > I have no objections to an AF_ALG backend in QEMU.
> > 
> > Rather than do another backend for virtio-crypto, IMHO, we should have
> > an AF_ALG impl of the crypto/ APIs. That way any potential performance
> > benefits will enhance our LUKS encryption code too.
> > 
> According to the currently schemas of crypto/ APIs, we can't choose the
> specific backend dynamically. This is a limitation for virtio-crypto
> device I think.

Do we really need to be able to choose the backend explicitly. If the AF_ALG
backend is faster, why would you simply not use that automatically if it is
available. I'm not seeing an obvious need for dynamically choosing between
backends.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

Re: [Qemu-devel] Question about add AF_ALG backend for virtio-crypto

[Gonglei (Arei)]

Hi Daniel,

>
> >
> > >
> > > On Mon, Jan 09, 2017 at 01:43:10PM +0000, Stefan Hajnoczi wrote:
> > > > On Mon, Jan 09, 2017 at 03:04:55PM +0800, Longpeng (Mike) wrote:
> > > > > I'm one of Gonglei's virtio-crypto project members, and we plan to add a
> > > AF_ALG
> > > > > backend for virtio-crypto(there's only builtin-backend currently).
> > > > >
> > > > > I found that Catalin, Paolo and Stefan had discussed about this in 2015
> > > > > (http://www.spinics.net/lists/kvm/msg115457.html), but it seems that
> > > Catalin
> > > > > didn't do it, so I'm confuse about wether it is need to add a AF_ALG
> > > backend.
> > > > >
> > > > > Do you have any suggestion? Thanks :)
> > > >
> > > > I have no objections to an AF_ALG backend in QEMU.
> > >
> > > Rather than do another backend for virtio-crypto, IMHO, we should have
> > > an AF_ALG impl of the crypto/ APIs. That way any potential performance
> > > benefits will enhance our LUKS encryption code too.
> > >
> > According to the currently schemas of crypto/ APIs, we can't choose the
> > specific backend dynamically. This is a limitation for virtio-crypto
> > device I think.
> 
> Do we really need to be able to choose the backend explicitly. If the AF_ALG
> backend is faster, why would you simply not use that automatically if it is
> available.

Can we realize the purpose based on the crypto/ APIs? IIUC the crypto
subsystem chooses a backend during the building according to the specific priority,
nettle > gcrypt > cipher-builtin.

If we add an AF_ALG implementation for crypto subsystem, shall we set it
as the highest priority? If so, other backends won't be used since AF_ALG
is always available. If not, how can we use AF_ALG backend for crypto/ API?

Please correct me if I'm missing something. 

> I'm not seeing an obvious need for dynamically choosing between
> backends.
> 

Thanks,
-Gonglei

Re: [Qemu-devel] Question about add AF_ALG backend for virtio-crypto

[Daniel P. Berrange]

On Tue, Jan 10, 2017 at 11:36:19AM +0000, Gonglei (Arei) wrote:
> Hi Daniel,
> 
> >
> > >
> > > >
> > > > On Mon, Jan 09, 2017 at 01:43:10PM +0000, Stefan Hajnoczi wrote:
> > > > > On Mon, Jan 09, 2017 at 03:04:55PM +0800, Longpeng (Mike) wrote:
> > > > > > I'm one of Gonglei's virtio-crypto project members, and we plan to add a
> > > > AF_ALG
> > > > > > backend for virtio-crypto(there's only builtin-backend currently).
> > > > > >
> > > > > > I found that Catalin, Paolo and Stefan had discussed about this in 2015
> > > > > > (http://www.spinics.net/lists/kvm/msg115457.html), but it seems that
> > > > Catalin
> > > > > > didn't do it, so I'm confuse about wether it is need to add a AF_ALG
> > > > backend.
> > > > > >
> > > > > > Do you have any suggestion? Thanks :)
> > > > >
> > > > > I have no objections to an AF_ALG backend in QEMU.
> > > >
> > > > Rather than do another backend for virtio-crypto, IMHO, we should have
> > > > an AF_ALG impl of the crypto/ APIs. That way any potential performance
> > > > benefits will enhance our LUKS encryption code too.
> > > >
> > > According to the currently schemas of crypto/ APIs, we can't choose the
> > > specific backend dynamically. This is a limitation for virtio-crypto
> > > device I think.
> > 
> > Do we really need to be able to choose the backend explicitly. If the AF_ALG
> > backend is faster, why would you simply not use that automatically if it is
> > available.
> 
> Can we realize the purpose based on the crypto/ APIs? IIUC the crypto
> subsystem chooses a backend during the building according to the specific priority,
> nettle > gcrypt > cipher-builtin.
> 
> If we add an AF_ALG implementation for crypto subsystem, shall we set it
> as the highest priority? If so, other backends won't be used since AF_ALG
> is always available. If not, how can we use AF_ALG backend for crypto/ API?
> 
> Please correct me if I'm missing something.

While AF_ALG has been available for a while, not all features have. For
example AEAD support was only added in kernel 4.1

So if we had AF_ALG in QEMU, we would have to have a stacked impl, where
we try AF_ALG and then fallback to the current code when QEMU runs on a
kernel lacking the feature needed.

We could potentially also have a global arg to switch backends e.g.

 -crypto-backend [afalg|builtin]


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

Re: [Qemu-devel] Question about add AF_ALG backend for virtio-crypto

[Gonglei (Arei)]

>
> > >
> > > >
> > > > >
> > > > > On Mon, Jan 09, 2017 at 01:43:10PM +0000, Stefan Hajnoczi wrote:
> > > > > > On Mon, Jan 09, 2017 at 03:04:55PM +0800, Longpeng (Mike) wrote:
> > > > > > > I'm one of Gonglei's virtio-crypto project members, and we plan to
> add a
> > > > > AF_ALG
> > > > > > > backend for virtio-crypto(there's only builtin-backend currently).
> > > > > > >
> > > > > > > I found that Catalin, Paolo and Stefan had discussed about this in
> 2015
> > > > > > > (http://www.spinics.net/lists/kvm/msg115457.html), but it seems
> that
> > > > > Catalin
> > > > > > > didn't do it, so I'm confuse about wether it is need to add a AF_ALG
> > > > > backend.
> > > > > > >
> > > > > > > Do you have any suggestion? Thanks :)
> > > > > >
> > > > > > I have no objections to an AF_ALG backend in QEMU.
> > > > >
> > > > > Rather than do another backend for virtio-crypto, IMHO, we should have
> > > > > an AF_ALG impl of the crypto/ APIs. That way any potential performance
> > > > > benefits will enhance our LUKS encryption code too.
> > > > >
> > > > According to the currently schemas of crypto/ APIs, we can't choose the
> > > > specific backend dynamically. This is a limitation for virtio-crypto
> > > > device I think.
> > >
> > > Do we really need to be able to choose the backend explicitly. If the AF_ALG
> > > backend is faster, why would you simply not use that automatically if it is
> > > available.
> >
> > Can we realize the purpose based on the crypto/ APIs? IIUC the crypto
> > subsystem chooses a backend during the building according to the specific
> priority,
> > nettle > gcrypt > cipher-builtin.
> >
> > If we add an AF_ALG implementation for crypto subsystem, shall we set it
> > as the highest priority? If so, other backends won't be used since AF_ALG
> > is always available. If not, how can we use AF_ALG backend for crypto/ API?
> >
> > Please correct me if I'm missing something.
> 
> While AF_ALG has been available for a while, not all features have. For
> example AEAD support was only added in kernel 4.1
> 
OK.

> So if we had AF_ALG in QEMU, we would have to have a stacked impl, where
> we try AF_ALG and then fallback to the current code when QEMU runs on a
> kernel lacking the feature needed.
> 
It makes sense though the implementation will be more complicate
since the code should identify the different error codes are returned
by AF_ALG APIs.

> We could potentially also have a global arg to switch backends e.g.
> 
>  -crypto-backend [afalg|builtin]
> 
So the backend here is not the cryptodev backend?

Thanks,
-Gonglei

Re: [Qemu-devel] Question about add AF_ALG backend for virtio-crypto

[Daniel P. Berrange]

On Tue, Jan 10, 2017 at 12:17:48PM +0000, Gonglei (Arei) wrote:
> >
> > > >
> > > > >
> > > > > >
> > > > > > On Mon, Jan 09, 2017 at 01:43:10PM +0000, Stefan Hajnoczi wrote:
> > > > > > > On Mon, Jan 09, 2017 at 03:04:55PM +0800, Longpeng (Mike) wrote:
> > > > > > > > I'm one of Gonglei's virtio-crypto project members, and we plan to
> > add a
> > > > > > AF_ALG
> > > > > > > > backend for virtio-crypto(there's only builtin-backend currently).
> > > > > > > >
> > > > > > > > I found that Catalin, Paolo and Stefan had discussed about this in
> > 2015
> > > > > > > > (http://www.spinics.net/lists/kvm/msg115457.html), but it seems
> > that
> > > > > > Catalin
> > > > > > > > didn't do it, so I'm confuse about wether it is need to add a AF_ALG
> > > > > > backend.
> > > > > > > >
> > > > > > > > Do you have any suggestion? Thanks :)
> > > > > > >
> > > > > > > I have no objections to an AF_ALG backend in QEMU.
> > > > > >
> > > > > > Rather than do another backend for virtio-crypto, IMHO, we should have
> > > > > > an AF_ALG impl of the crypto/ APIs. That way any potential performance
> > > > > > benefits will enhance our LUKS encryption code too.
> > > > > >
> > > > > According to the currently schemas of crypto/ APIs, we can't choose the
> > > > > specific backend dynamically. This is a limitation for virtio-crypto
> > > > > device I think.
> > > >
> > > > Do we really need to be able to choose the backend explicitly. If the AF_ALG
> > > > backend is faster, why would you simply not use that automatically if it is
> > > > available.
> > >
> > > Can we realize the purpose based on the crypto/ APIs? IIUC the crypto
> > > subsystem chooses a backend during the building according to the specific
> > priority,
> > > nettle > gcrypt > cipher-builtin.
> > >
> > > If we add an AF_ALG implementation for crypto subsystem, shall we set it
> > > as the highest priority? If so, other backends won't be used since AF_ALG
> > > is always available. If not, how can we use AF_ALG backend for crypto/ API?
> > >
> > > Please correct me if I'm missing something.
> > 
> > While AF_ALG has been available for a while, not all features have. For
> > example AEAD support was only added in kernel 4.1
> > 
> OK.
> 
> > So if we had AF_ALG in QEMU, we would have to have a stacked impl, where
> > we try AF_ALG and then fallback to the current code when QEMU runs on a
> > kernel lacking the feature needed.
> > 
> It makes sense though the implementation will be more complicate
> since the code should identify the different error codes are returned
> by AF_ALG APIs.
> 
> > We could potentially also have a global arg to switch backends e.g.
> > 
> >  -crypto-backend [afalg|builtin]
> > 
> So the backend here is not the cryptodev backend?

No, it would apply to the crypto/ APIs, so it affects all users of those
APIs not just cryptodev


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

Re: [Qemu-devel] Question about add AF_ALG backend for virtio-crypto

[Longpeng (Mike)]

Hi Daniel,

I was writing AF_ALG-backed for QEMU crypto these days, I think there're more
than two ways to implements it.

The first one look likes below:
[ cipher.c ]
qcrypto_cipher_new(...)
{
	if (...) { /* use AF_ALG */
		cipher = afalg_cipher_new(...)
		if (cipher) {
			return cipher;
		}
	}
	
	/* disabled AF_ALG or AF_ALG failed, then back to
	 * using 'builtin'(gcrypt/nettle/...)
	 */
	cipher = __qcrypto_cipher_new(...)
}

[ cipher-afalg.c ]
afalg_cipher_new(...) {....}
afalg_cipher_encrypt(...) {...}
......

[ cipher-gcrypt.c ]
__qcrypto_cipher_new(...) {...}
__qcrypto_cipher_encrypt(...) {...}
......

[ cipher-nettle.c ]
__qcrypto_cipher_new(...) {...}
__qcrypto_cipher_encrypt(...) {...}
......

In this way, I think I need to rename most functions in
cipher-gcrypt.c/cipher-nettle.c with a prefixion(such as '__')


Alternative way is:
[ cipher-afalg.c ]
afalg_cipher_new(...) {....}
afalg_cipher_encrypt(...) {...}
......

[ cipher-gcrypt.c ]
qcrypto_cipher_new(...)
{
	if (...) { /* use AF_ALG */
		cipher = afalg_cipher_new(...)
		if (cipher) {
			return cipher;
		}
	}
	
	/* disabled AF_ALG or AF_ALG failed, then back to
	 * using 'builtin'
	 */
	.......( the existing code )
}
......

[ cipher-nettle.c ]
qcrypto_cipher_new(...)
{
	if (...) { /* use AF_ALG */
		cipher = afalg_cipher_new(...)
		if (cipher) {
			return cipher;
		}
	}
	
	/* disabled AF_ALG or AF_ALG failed, then back to
	 * using 'builtin'
	 */
	.......( the existing code )
}
......

In this way, we should add AF_ALG-backed code in most functions in
cipher-gcrypt.c/cipher-nettle.c, I'm afraid this would introduce lots of
duplicate code because the same AF_ALG-backed code must in both gcrypt-backed
impls and nettle-backed impls as above.


I'm confusing about which way you'd prefer, or do you have any better suggestion?

Thanks!

-- 
Regards,
Longpeng(Mike)


On 2017/1/10 21:30, Daniel P. Berrange wrote:

> On Tue, Jan 10, 2017 at 12:17:48PM +0000, Gonglei (Arei) wrote:
>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>> On Mon, Jan 09, 2017 at 01:43:10PM +0000, Stefan Hajnoczi wrote:
>>>>>>>> On Mon, Jan 09, 2017 at 03:04:55PM +0800, Longpeng (Mike) wrote:
>>>>>>>>> I'm one of Gonglei's virtio-crypto project members, and we plan to
>>> add a
>>>>>>> AF_ALG
>>>>>>>>> backend for virtio-crypto(there's only builtin-backend currently).
>>>>>>>>>
>>>>>>>>> I found that Catalin, Paolo and Stefan had discussed about this in
>>> 2015
>>>>>>>>> (http://www.spinics.net/lists/kvm/msg115457.html), but it seems
>>> that
>>>>>>> Catalin
>>>>>>>>> didn't do it, so I'm confuse about wether it is need to add a AF_ALG
>>>>>>> backend.
>>>>>>>>>
>>>>>>>>> Do you have any suggestion? Thanks :)
>>>>>>>>
>>>>>>>> I have no objections to an AF_ALG backend in QEMU.
>>>>>>>
>>>>>>> Rather than do another backend for virtio-crypto, IMHO, we should have
>>>>>>> an AF_ALG impl of the crypto/ APIs. That way any potential performance
>>>>>>> benefits will enhance our LUKS encryption code too.
>>>>>>>
>>>>>> According to the currently schemas of crypto/ APIs, we can't choose the
>>>>>> specific backend dynamically. This is a limitation for virtio-crypto
>>>>>> device I think.
>>>>>
>>>>> Do we really need to be able to choose the backend explicitly. If the AF_ALG
>>>>> backend is faster, why would you simply not use that automatically if it is
>>>>> available.
>>>>
>>>> Can we realize the purpose based on the crypto/ APIs? IIUC the crypto
>>>> subsystem chooses a backend during the building according to the specific
>>> priority,
>>>> nettle > gcrypt > cipher-builtin.
>>>>
>>>> If we add an AF_ALG implementation for crypto subsystem, shall we set it
>>>> as the highest priority? If so, other backends won't be used since AF_ALG
>>>> is always available. If not, how can we use AF_ALG backend for crypto/ API?
>>>>
>>>> Please correct me if I'm missing something.
>>>
>>> While AF_ALG has been available for a while, not all features have. For
>>> example AEAD support was only added in kernel 4.1
>>>
>> OK.
>>
>>> So if we had AF_ALG in QEMU, we would have to have a stacked impl, where
>>> we try AF_ALG and then fallback to the current code when QEMU runs on a
>>> kernel lacking the feature needed.
>>>
>> It makes sense though the implementation will be more complicate
>> since the code should identify the different error codes are returned
>> by AF_ALG APIs.
>>
>>> We could potentially also have a global arg to switch backends e.g.
>>>
>>>  -crypto-backend [afalg|builtin]
>>>
>> So the backend here is not the cryptodev backend?
> 
> No, it would apply to the crypto/ APIs, so it affects all users of those
> APIs not just cryptodev
> 
> 
> Regards,
> Daniel


-- 
Regards,
Longpeng(Mike)

Re: [Qemu-devel] Question about add AF_ALG backend for virtio-crypto

[Daniel P. Berrange]

On Wed, Feb 08, 2017 at 06:46:04PM +0800, Longpeng (Mike) wrote:
> Hi Daniel,
> 
> I was writing AF_ALG-backed for QEMU crypto these days, I think there're more
> than two ways to implements it.
> 
> The first one look likes below:
> [ cipher.c ]
> qcrypto_cipher_new(...)
> {
> 	if (...) { /* use AF_ALG */
> 		cipher = afalg_cipher_new(...)
> 		if (cipher) {
> 			return cipher;
> 		}
> 	}
> 	
> 	/* disabled AF_ALG or AF_ALG failed, then back to
> 	 * using 'builtin'(gcrypt/nettle/...)
> 	 */
> 	cipher = __qcrypto_cipher_new(...)
> }
> 
> [ cipher-afalg.c ]
> afalg_cipher_new(...) {....}
> afalg_cipher_encrypt(...) {...}
> ......
> 
> [ cipher-gcrypt.c ]
> __qcrypto_cipher_new(...) {...}
> __qcrypto_cipher_encrypt(...) {...}
> ......
> 
> [ cipher-nettle.c ]
> __qcrypto_cipher_new(...) {...}
> __qcrypto_cipher_encrypt(...) {...}
> ......
> 
> In this way, I think I need to rename most functions in
> cipher-gcrypt.c/cipher-nettle.c with a prefixion(such as '__')
> 
> 
> Alternative way is:
> [ cipher-afalg.c ]
> afalg_cipher_new(...) {....}
> afalg_cipher_encrypt(...) {...}
> ......
> 
> [ cipher-gcrypt.c ]
> qcrypto_cipher_new(...)
> {
> 	if (...) { /* use AF_ALG */
> 		cipher = afalg_cipher_new(...)
> 		if (cipher) {
> 			return cipher;
> 		}
> 	}
> 	
> 	/* disabled AF_ALG or AF_ALG failed, then back to
> 	 * using 'builtin'
> 	 */
> 	.......( the existing code )
> }
> ......
> 
> [ cipher-nettle.c ]
> qcrypto_cipher_new(...)
> {
> 	if (...) { /* use AF_ALG */
> 		cipher = afalg_cipher_new(...)
> 		if (cipher) {
> 			return cipher;
> 		}
> 	}
> 	
> 	/* disabled AF_ALG or AF_ALG failed, then back to
> 	 * using 'builtin'
> 	 */
> 	.......( the existing code )
> }
> ......
> 
> In this way, we should add AF_ALG-backed code in most functions in
> cipher-gcrypt.c/cipher-nettle.c, I'm afraid this would introduce lots of
> duplicate code because the same AF_ALG-backed code must in both gcrypt-backed
> impls and nettle-backed impls as above.
> 
> I'm confusing about which way you'd prefer, or do you have any better
> suggestion?

Yeah, both approaches have some reasonably significant downsides. Approach
1 is sort of like providing a virtual driver table, except it is hardcoded
to switch between 2 impls only.

A variant on approach 1 is to actually setup a proper driver-table dispatch
layer. eg define a struct that contains callbacks for each public api
operation. The qcrypto_cipher_new() method will then either setup callbacks
for AF_ALG, or for the library impl.

This is the design we took in crypto/{ivgen.c,ivgenpriv.h}


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

Re: [Qemu-devel] Question about add AF_ALG backend for virtio-crypto

[Longpeng (Mike)]

Hi Daniel,

On 2017/2/8 18:53, Daniel P. Berrange wrote:

> On Wed, Feb 08, 2017 at 06:46:04PM +0800, Longpeng (Mike) wrote:
>> Hi Daniel,
>>
>> I was writing AF_ALG-backed for QEMU crypto these days, I think there're more
>> than two ways to implements it.
>>
>> The first one look likes below:
>> [ cipher.c ]
>> qcrypto_cipher_new(...)
>> {
>> 	if (...) { /* use AF_ALG */
>> 		cipher = afalg_cipher_new(...)
>> 		if (cipher) {
>> 			return cipher;
>> 		}
>> 	}
>> 	
>> 	/* disabled AF_ALG or AF_ALG failed, then back to
>> 	 * using 'builtin'(gcrypt/nettle/...)
>> 	 */
>> 	cipher = __qcrypto_cipher_new(...)
>> }
>>
>> [ cipher-afalg.c ]
>> afalg_cipher_new(...) {....}
>> afalg_cipher_encrypt(...) {...}
>> ......
>>
>> [ cipher-gcrypt.c ]
>> __qcrypto_cipher_new(...) {...}
>> __qcrypto_cipher_encrypt(...) {...}
>> ......
>>
>> [ cipher-nettle.c ]
>> __qcrypto_cipher_new(...) {...}
>> __qcrypto_cipher_encrypt(...) {...}
>> ......
>>
>> In this way, I think I need to rename most functions in
>> cipher-gcrypt.c/cipher-nettle.c with a prefixion(such as '__')
>>


>> I'm confusing about which way you'd prefer, or do you have any better
>> suggestion?
> 
> Yeah, both approaches have some reasonably significant downsides. Approach
> 1 is sort of like providing a virtual driver table, except it is hardcoded
> to switch between 2 impls only.
> 
> A variant on approach 1 is to actually setup a proper driver-table dispatch
> layer. eg define a struct that contains callbacks for each public api
> operation. The qcrypto_cipher_new() method will then either setup callbacks
> for AF_ALG, or for the library impl.
> 
> This is the design we took in crypto/{ivgen.c,ivgenpriv.h}
> 


So...you prefer approach 1 with a driver-table dispatch layer, right?
And this implies that we must either rename some public methods in
cipher-gcrypt.c/cipher-nettle.c, or change them to 'static'.


I also have some other ideas:

1) *using bitmap to improve performance*

As you suggested before:
"if we had AF_ALG in QEMU, we would have to have a stacked impl, where
we try AF_ALG and then fallback to the current code when QEMU runs on a
kernel lacking the feature needed."

I think it would impact the performance if we "try AF_ALG and then fallback to
library" each time, so we can use a bitmap to indicate whether the @alg is
supported by AF_ALG.


2) *maybe we need a heuristic policy*

I added some speed test in test-crypto-cipher/hash and found that for big
packets AF_ALG is much faster than library-impl while library-impl is better
when the packets is small:

packet(bytes)	AF_ALG(MB/sec, intel QAT)	Library-impl(MB/sec)
512		53.68				127.82
1024		98.39				133.21
2048		167.56				134.62
4096		276.21				135.10
8192		410.80				135.82
16384		545.08				136.01
32768		654.49				136.30
65536		723.00				136.29

If a @alg is both supported by AF_ALG and library-impl, I think we should decide
to use which one dynamically.

> 
> Regards,
> Daniel


-- 
Regards,
Longpeng(Mike)

Re: [Qemu-devel] Question about add AF_ALG backend for virtio-crypto

[Daniel P. Berrange]

On Thu, Feb 09, 2017 at 10:58:55AM +0800, Longpeng (Mike) wrote:
> Hi Daniel,
> 
> On 2017/2/8 18:53, Daniel P. Berrange wrote:
> 
> > On Wed, Feb 08, 2017 at 06:46:04PM +0800, Longpeng (Mike) wrote:
> >> Hi Daniel,
> >>
> >> I was writing AF_ALG-backed for QEMU crypto these days, I think there're more
> >> than two ways to implements it.
> >>
> >> The first one look likes below:
> >> [ cipher.c ]
> >> qcrypto_cipher_new(...)
> >> {
> >> 	if (...) { /* use AF_ALG */
> >> 		cipher = afalg_cipher_new(...)
> >> 		if (cipher) {
> >> 			return cipher;
> >> 		}
> >> 	}
> >> 	
> >> 	/* disabled AF_ALG or AF_ALG failed, then back to
> >> 	 * using 'builtin'(gcrypt/nettle/...)
> >> 	 */
> >> 	cipher = __qcrypto_cipher_new(...)
> >> }
> >>
> >> [ cipher-afalg.c ]
> >> afalg_cipher_new(...) {....}
> >> afalg_cipher_encrypt(...) {...}
> >> ......
> >>
> >> [ cipher-gcrypt.c ]
> >> __qcrypto_cipher_new(...) {...}
> >> __qcrypto_cipher_encrypt(...) {...}
> >> ......
> >>
> >> [ cipher-nettle.c ]
> >> __qcrypto_cipher_new(...) {...}
> >> __qcrypto_cipher_encrypt(...) {...}
> >> ......
> >>
> >> In this way, I think I need to rename most functions in
> >> cipher-gcrypt.c/cipher-nettle.c with a prefixion(such as '__')
> >>
> 
> 
> >> I'm confusing about which way you'd prefer, or do you have any better
> >> suggestion?
> > 
> > Yeah, both approaches have some reasonably significant downsides. Approach
> > 1 is sort of like providing a virtual driver table, except it is hardcoded
> > to switch between 2 impls only.
> > 
> > A variant on approach 1 is to actually setup a proper driver-table dispatch
> > layer. eg define a struct that contains callbacks for each public api
> > operation. The qcrypto_cipher_new() method will then either setup callbacks
> > for AF_ALG, or for the library impl.
> > 
> > This is the design we took in crypto/{ivgen.c,ivgenpriv.h}
> > 
> 
> 
> So...you prefer approach 1 with a driver-table dispatch layer, right?
> And this implies that we must either rename some public methods in
> cipher-gcrypt.c/cipher-nettle.c, or change them to 'static'.

I'd suggest both - renaming them to have 'gcrypt' or 'nettle' in their
name, and also make them static.

> I also have some other ideas:
> 
> 1) *using bitmap to improve performance*
> 
> As you suggested before:
> "if we had AF_ALG in QEMU, we would have to have a stacked impl, where
> we try AF_ALG and then fallback to the current code when QEMU runs on a
> kernel lacking the feature needed."
> 
> I think it would impact the performance if we "try AF_ALG and then fallback to
> library" each time, so we can use a bitmap to indicate whether the @alg is
> supported by AF_ALG.

Yep, remembering the decision makes total sense if that is a high overhead
decision.

> 2) *maybe we need a heuristic policy*
> 
> I added some speed test in test-crypto-cipher/hash and found that for big
> packets AF_ALG is much faster than library-impl while library-impl is better
> when the packets is small:
>
> packet(bytes)	AF_ALG(MB/sec, intel QAT)	Library-impl(MB/sec)
> 512		53.68				127.82
> 1024		98.39				133.21
> 2048		167.56				134.62
> 4096		276.21				135.10
> 8192		410.80				135.82
> 16384		545.08				136.01
> 32768		654.49				136.30
> 65536		723.00				136.29
> 
> If a @alg is both supported by AF_ALG and library-impl, I think we should decide
> to use which one dynamically.

What exactly are you measuring here?

Is this comparing encryption of a fixed total size of data, and
varying the packet size. ie sending 1024 * 512 byte packets against
256  * 2048 byte packages.

Or is it sending a constant number of packets eg 1024 * 512 byte
packets against 1024 * 2048 byte packets ?

The problem is that when constructing the cipher initially, we have no
about the intended usage pattern, so can't decide which impl to use as
is


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

Re: [Qemu-devel] Question about add AF_ALG backend for virtio-crypto

[Longpeng (Mike)]

Hi Daniel,

On 2017/2/9 18:11, Daniel P. Berrange wrote:

> On Thu, Feb 09, 2017 at 10:58:55AM +0800, Longpeng (Mike) wrote:
>> Hi Daniel,


>>
>> So...you prefer approach 1 with a driver-table dispatch layer, right?
>> And this implies that we must either rename some public methods in
>> cipher-gcrypt.c/cipher-nettle.c, or change them to 'static'.
> 
> I'd suggest both - renaming them to have 'gcrypt' or 'nettle' in their
> name, and also make them static.
> 


OK.

>> I also have some other ideas:
>>

>> 2) *maybe we need a heuristic policy*
>>
>> I added some speed test in test-crypto-cipher/hash and found that for big
>> packets AF_ALG is much faster than library-impl while library-impl is better
>> when the packets is small:
>>
>> packet(bytes)	AF_ALG(MB/sec, intel QAT)	Library-impl(MB/sec)
>> 512		53.68				127.82
>> 1024		98.39				133.21
>> 2048		167.56				134.62
>> 4096		276.21				135.10
>> 8192		410.80				135.82
>> 16384		545.08				136.01
>> 32768		654.49				136.30
>> 65536		723.00				136.29
>>
>> If a @alg is both supported by AF_ALG and library-impl, I think we should decide
>> to use which one dynamically.
> 
> What exactly are you measuring here?
> 
> Is this comparing encryption of a fixed total size of data, and
> varying the packet size. ie sending 1024 * 512 byte packets against
> 256  * 2048 byte packages.
> 
> Or is it sending a constant number of packets eg 1024 * 512 byte
> packets against 1024 * 2048 byte packets ?
> 


The testcase encrypts data for 5 seconds and then calculates how many MBs it can
encrypt per second, as below:

    g_test_timer_start();
    do {
        g_assert(qcrypto_cipher_setiv(cipher,
                                      iv, niv,
                                      &err) == 0);

        g_assert(qcrypto_cipher_encrypt(cipher,
                                        plaintext,
                                        ciphertext,
                                        chunk_size,
                                        &err) == 0);
        total += chunk_size;
    } while (g_test_timer_elapsed() < 5.0);

    total /= 1024 * 1024; /* to MB */
    g_print("Testing cbc(aes128): ");
    g_print("Encrypting in chunks of %ld bytes: ", chunk_size);
    g_print("done. %.2f MB in %.2f secs: ", total, g_test_timer_last());
    g_print("%.2f MB/sec\t", total / g_test_timer_last());

chunk_size = 512/1024/2048/.../65536 bytes.

Some other projects (ie. cryptodev-linux, libkcapi) also use this way to test speed.

> The problem is that when constructing the cipher initially, we have no
> about the intended usage pattern, so can't decide which impl to use as
> is
> 


Yep, I wanted to allocate both af_alg and library-impl objects and then we can
choose dynamically before, but now it seems too stupid. :)

Thanks.

> 
> Regards,
> Daniel


-- 
Regards,
Longpeng(Mike)

Re: [Qemu-devel] Question about add AF_ALG backend for virtio-crypto

[Daniel P. Berrange]

On Thu, Feb 09, 2017 at 07:03:57PM +0800, Longpeng (Mike) wrote:
> Hi Daniel,
> 
> On 2017/2/9 18:11, Daniel P. Berrange wrote:
> 
> > On Thu, Feb 09, 2017 at 10:58:55AM +0800, Longpeng (Mike) wrote:
> >> Hi Daniel,
> 
> 
> >>
> >> So...you prefer approach 1 with a driver-table dispatch layer, right?
> >> And this implies that we must either rename some public methods in
> >> cipher-gcrypt.c/cipher-nettle.c, or change them to 'static'.
> > 
> > I'd suggest both - renaming them to have 'gcrypt' or 'nettle' in their
> > name, and also make them static.
> > 
> 
> 
> OK.
> 
> >> I also have some other ideas:
> >>
> 
> >> 2) *maybe we need a heuristic policy*
> >>
> >> I added some speed test in test-crypto-cipher/hash and found that for big
> >> packets AF_ALG is much faster than library-impl while library-impl is better
> >> when the packets is small:
> >>
> >> packet(bytes)	AF_ALG(MB/sec, intel QAT)	Library-impl(MB/sec)
> >> 512		53.68				127.82
> >> 1024		98.39				133.21
> >> 2048		167.56				134.62
> >> 4096		276.21				135.10
> >> 8192		410.80				135.82
> >> 16384		545.08				136.01
> >> 32768		654.49				136.30
> >> 65536		723.00				136.29
> >>
> >> If a @alg is both supported by AF_ALG and library-impl, I think we should decide
> >> to use which one dynamically.
> > 
> > What exactly are you measuring here?
> > 
> > Is this comparing encryption of a fixed total size of data, and
> > varying the packet size. ie sending 1024 * 512 byte packets against
> > 256  * 2048 byte packages.
> > 
> > Or is it sending a constant number of packets eg 1024 * 512 byte
> > packets against 1024 * 2048 byte packets ?
> > 
> 
> 
> The testcase encrypts data for 5 seconds and then calculates how many MBs it can
> encrypt per second, as below:
> 
>     g_test_timer_start();
>     do {
>         g_assert(qcrypto_cipher_setiv(cipher,
>                                       iv, niv,
>                                       &err) == 0);
> 
>         g_assert(qcrypto_cipher_encrypt(cipher,
>                                         plaintext,
>                                         ciphertext,
>                                         chunk_size,
>                                         &err) == 0);
>         total += chunk_size;
>     } while (g_test_timer_elapsed() < 5.0);
> 
>     total /= 1024 * 1024; /* to MB */
>     g_print("Testing cbc(aes128): ");
>     g_print("Encrypting in chunks of %ld bytes: ", chunk_size);
>     g_print("done. %.2f MB in %.2f secs: ", total, g_test_timer_last());
>     g_print("%.2f MB/sec\t", total / g_test_timer_last());
> 
> chunk_size = 512/1024/2048/.../65536 bytes.
> 
> Some other projects (ie. cryptodev-linux, libkcapi) also use this way to test speed.

I'd be interested to know if there's any different in the results if
you put the qcrypto_cipher_setiv() call outside of the loop.

Depending on the way the API is used, you might set an IV, and then
write many chunks of data before setting the next IV.

Of course for LUKS, we set a new IV every 512 bytes of data, since IVs
are tied to disk sectors, so we'd be hitting the small chunk size
code path.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|