> From: Stefan Hajnoczi [mailto:stefa...@redhat.com]
> Sent: Tuesday, October 04, 2016 5:38 PM
> Subject: Re: [PATCH v4 05/13] virtio-crypto: add virtio crypto device
> emulation
>
> On Wed, Sep 28, 2016 at 04:25:44PM +0800, Gonglei wrote:
> > Introduce the virtio crypto realization, I'll
> > finish the core code in the following patches. The
> > thoughts came from virtio net realization.
> >
> > For more information see:
> > http://qemu-project.org/Features/VirtioCrypto
>
> This patch contains functions and struct fields that are unused. It
> would make code review easier if you add them when they are needed
> throughout the patch series.
>
OK, will remove them.
> > +static void virtio_crypto_device_realize(DeviceState *dev, Error **errp)
> > +{
> > + VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> > + VirtIOCrypto *vcrypto = VIRTIO_CRYPTO(dev);
> > + int i;
> > +
> > + vcrypto->cryptodev = vcrypto->conf.cryptodev;
> > + if (vcrypto->cryptodev == NULL) {
> > + error_setg(errp, "'cryptodev' parameter expects a valid object");
> > + return;
> > + }
> > +
> > + vcrypto->max_queues = MAX(vcrypto->cryptodev->conf.peers.queues,
> 1);
> > + if (vcrypto->max_queues + 1 > VIRTIO_QUEUE_MAX) {
> > + error_setg(errp, "Invalid number of queues (= %" PRIu16 "), "
> > + "must be a postive integer less than %d.",
> > + vcrypto->max_queues, VIRTIO_QUEUE_MAX - 1);
>
> The error message is off by 1:
>
> must be a positive integer less than VIRTIO_QUEUE_MAX
>
Yes.
> > + return;
> > + }
> > +
> > + virtio_init(vdev, "virtio-crypto", VIRTIO_ID_CRYPTO,
> vcrypto->config_size);
> > + vcrypto->curr_queues = 1;
> > +
> > + for (i = 0; i < vcrypto->max_queues; i++) {
> > + virtio_add_queue(vdev, 1024, virtio_crypto_handle_dataq);
> > + }
> > +
> > + vcrypto->ctrl_vq = virtio_add_queue(vdev, 64,
> virtio_crypto_handle_ctrl);
> > + if (!vcrypto->cryptodev->ready) {
> > + vcrypto->status &= ~VIRTIO_CRYPTO_S_HW_READY;
> > + } else {
> > + vcrypto->status |= VIRTIO_CRYPTO_S_HW_READY;
> > + }
> > + register_savevm(dev, "virtio-crypto", -1, 1, virtio_crypto_save,
> > + virtio_crypto_load, vcrypto);
>
> Please use VMSTATE_VIRTIO_DEVICE() instead of calling register_savevm().
>
OK.
> > +#ifdef DEBUG_VIRTIO_CRYPTO
> > +#define DPRINTF(fmt, ...) \
> > +do { printf("virtio_crypto: " fmt , ## __VA_ARGS__); } while (0)
> > +#else
> > +#define DPRINTF(fmt, ...) do { } while (0)
> > +#endif
>
> Please use tracing (see docs/tracing.txt) or if you really want to use
I planned to polish tracing log in my TODO list. So, currently I'd like to use
DPRINTF().
> debug printfs then define DPRINTF() in a way that prevents bitrot:
>
> #define DEBUG_VIRTIO_CRYPTO 0
> #define DPRINTF(fmt, ...) \
> do { \
> if (DEBUG_VIRTIO_CRYPTO) { \
> fprintf(stderr, "virtio_crypto: " fmt, ##__VA_ARGS__); \
> } \
> } while (0)
Make sense.
Regards,
-Gonglei
