Re: [Qemu-devel] [PATCH v6 1/4] cryptodev: add vhost-user as a new cryptodev backend

[Zhoujian (jay)]

> -----Original Message-----
> From: Michael S. Tsirkin [mailto:address@hidden
> Sent: Wednesday, February 14, 2018 12:46 AM
> To: Zhoujian (jay) <address@hidden>
> Cc: address@hidden; address@hidden; Huangweidong (C)
> <address@hidden>; address@hidden; address@hidden;
> longpeng <address@hidden>; address@hidden; address@hidden;
> Gonglei (Arei) <address@hidden>; wangxin (U)
> <address@hidden>
> Subject: Re: [PATCH v6 1/4] cryptodev: add vhost-user as a new cryptodev
> backend
> 
> On Sun, Jan 21, 2018 at 08:54:47PM +0800, Jay Zhou wrote:
> > diff --git a/backends/cryptodev-vhost-user.c
> > b/backends/cryptodev-vhost-user.c new file mode 100644 index
> > 0000000..4e63ece
> > --- /dev/null
> > +++ b/backends/cryptodev-vhost-user.c
> > @@ -0,0 +1,333 @@
> > +/*
> > + * QEMU Cryptodev backend for QEMU cipher APIs
> > + *
> > + * Copyright (c) 2016 HUAWEI TECHNOLOGIES CO., LTD.
> > + *
> > + * Authors:
> > + *    Gonglei <address@hidden>
> > + *
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2 of the License, or (at your option) any later version.
> > + *
> > + * This library is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with this library; if not, see
> <http://www.gnu.org/licenses/>.
> > + *
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "hw/boards.h"
> > +#include "qapi/error.h"
> > +#include "qapi/qmp/qerror.h"
> > +#include "qemu/error-report.h"
> > +#include "standard-headers/linux/virtio_crypto.h"
> > +#include "sysemu/cryptodev-vhost.h"
> > +#include "chardev/char-fe.h"
> > +
> > +
> > +/**
> > + * @TYPE_CRYPTODEV_BACKEND_VHOST_USER:
> > + * name of backend that uses vhost user server  */ #define
> > +TYPE_CRYPTODEV_BACKEND_VHOST_USER "cryptodev-vhost-user"
> > +
> > +#define CRYPTODEV_BACKEND_VHOST_USER(obj) \
> > +    OBJECT_CHECK(CryptoDevBackendVhostUser, \
> > +                 (obj), TYPE_CRYPTODEV_BACKEND_VHOST_USER)
> > +
> > +
> > +typedef struct CryptoDevBackendVhostUser {
> > +    CryptoDevBackend parent_obj;
> > +
> > +    CharBackend chr;
> > +    char *chr_name;
> > +    bool opened;
> > +    CryptoDevBackendVhost *vhost_crypto[MAX_CRYPTO_QUEUE_NUM];
> > +} CryptoDevBackendVhostUser;
> > +
> > +static int
> > +cryptodev_vhost_user_running(
> > +             CryptoDevBackendVhost *crypto) {
> > +    return crypto ? 1 : 0;
> > +}
> > +
> > +static void cryptodev_vhost_user_stop(int queues,
> > +                          CryptoDevBackendVhostUser *s) {
> > +    size_t i;
> > +
> > +    for (i = 0; i < queues; i++) {
> > +        if (!cryptodev_vhost_user_running(s->vhost_crypto[i])) {
> > +            continue;
> > +        }
> > +
> > +        if (s->vhost_crypto) {
> > +            cryptodev_vhost_cleanup(s->vhost_crypto[i]);
> > +            s->vhost_crypto[i] = NULL;
> > +        }
> > +    }
> > +}
> 
> This test is problematic: clang build triggers an error:
> > /home/petmay01/linaro/qemu-for-merges/backends/cryptodev-vhost-user.c:86:16:
> > error: address of array 's->vhost_crypto' will always evaluate to
> > 'true' [-Werror,-Wpointer-bool-conversion]
> >         if (s->vhost_crypto) {
> >         ~~  ~~~^~~~~~~~~~~~

This line should be

            if (s->vhost_crypto[i]) {
> 
> I really don't see how this could do the right thing, which makes me suspect
> that either you did not test stop, or you always have all queues enabled.
> 
> Pls test a config with some queues disabled.
> 
> In particular this machinery needs some unit tests to catch errors like this.

Okay, will do more tests, sorry about that.

Regards,
Jay

> 
> 
> --
> MST