On Fri, Apr 15, 2022 at 06:50:19PM +0800, zhenwei pi wrote:
> On 4/15/22 16:41, Michael S. Tsirkin wrote:
> > > diff --git a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> > > index f3ec9420215e..bf7c1aa4be37 100644
> > > --- a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> > > +++ b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> > > @@ -102,107 +102,100 @@ static int virtio_crypto_alg_akcipher_init_session(struct virtio_crypto_akcipher
> > > {
> > > struct scatterlist outhdr_sg, key_sg, inhdr_sg, *sgs[3];
> > > struct virtio_crypto *vcrypto = ctx->vcrypto;
> > > + struct virtio_crypto_ctrl_request *vc_ctrl_req = NULL;
> >
> > this is initialized down the road, I think you can skip = NULL here.
> >
> OK.
> > > uint8_t *pkey;
> > > - unsigned int inlen;
> > > - int err;
> > > + int err = -ENOMEM;
> >
> > I would assign this in the single case where this value is used.
> >
> OK
> > > unsigned int num_out = 0, num_in = 0;
> > > + int node = dev_to_node(&vcrypto->vdev->dev);
> > are you sure it is
> > better to allocate close to device and not to current node
> > which is the default?
> >
> Also with this part:
> /* Internal representation of a data virtqueue */
> @@ -65,11 +66,6 @@ struct virtio_crypto {
> /* Maximum size of per request */
> u64 max_size;
>
> - /* Control VQ buffers: protected by the ctrl_lock */
> - struct virtio_crypto_op_ctrl_req ctrl;
> - struct virtio_crypto_session_input input;
> - struct virtio_crypto_inhdr ctrl_status;
> -
> unsigned long status;
> atomic_t ref_count;
>
> Orignally virtio crypto driver allocates ctrl&input&ctrl_status per-device,
> and protects this with ctrl_lock. This is the reason why the control queue
> reaches the bottleneck of performance. I'll append this in the next version
> in commit message.
>
> Instead of the single request buffer, declare struct
> virtio_crypto_ctrl_request {
> struct virtio_crypto_op_ctrl_req ctrl;
> struct virtio_crypto_session_input input;
> struct virtio_crypto_inhdr ctrl_status;
> ... }
>
> The motivation of this change is to allocate buffer from the same node with
> device during control queue operations.
But are you sure it's a win? quite possibly it's a win to
have it close to driver not close to device.
This kind of change is really best done separately with some
testing showing it's a win. If that is too much to ask,
make it a separate patch and add some analysis explaining
why device accesses the structure more than the driver.
> >
> > > pkey = kmemdup(key, keylen, GFP_ATOMIC);
> > > if (!pkey)
> > > return -ENOMEM;
> > > - spin_lock(&vcrypto->ctrl_lock);
> > > - memcpy(&vcrypto->ctrl.header, header, sizeof(vcrypto->ctrl.header));
> > > - memcpy(&vcrypto->ctrl.u, para, sizeof(vcrypto->ctrl.u));
> > > - vcrypto->input.status = cpu_to_le32(VIRTIO_CRYPTO_ERR);
> > > + vc_ctrl_req = kzalloc_node(sizeof(*vc_ctrl_req), GFP_KERNEL, node);
> > > + if (!vc_ctrl_req)
> > > + goto out;
> >
> > do you need to allocate it with kzalloc?
> > is anything wrong with just keeping it part of device?
> > even if yes this change is better split in a separate patch, would make the patch smaller.
> Because there are padding field in
> virtio_crypto_op_ctrl_req&virtio_crypto_session_input, I suppose the
> original version also needs to clear padding field.
> So I use kzalloc to make sure that the padding field gets cleared.
> If this is reasonable, to separate this patch is OK to me, or I append this
> reason into commit message and comments in code.
Not sure I understand. Maybe add a code comment explaining
what is cleared and why.
> > > +
> > > +void virtcrypto_ctrlq_callback(struct virtqueue *vq)
> > > +{
> > > + struct virtio_crypto *vcrypto = vq->vdev->priv;
> > > + struct virtio_crypto_ctrl_request *vc_ctrl_req;
> > > + unsigned long flags;
> > > + unsigned int len;
> > > +
> > > + spin_lock_irqsave(&vcrypto->ctrl_lock, flags);
> > > + do {
> > > + virtqueue_disable_cb(vq);
> > > + while ((vc_ctrl_req = virtqueue_get_buf(vq, &len)) != NULL) {
> >
> >
> > you really need to break out of this loop if vq is broken,
> > virtqueue_get_buf will keep returning NULL in this case.
> >
> I'm a little confused here, if virtqueue_get_buf return NULL, this loop will
> break. Could you please give me more hints?
Oh right. Sorry was confused.
> >
> > > + spin_unlock_irqrestore(&vcrypto->ctrl_lock, flags);
> > > + if (vc_ctrl_req->ctrl_cb)
> > > + vc_ctrl_req->ctrl_cb(vc_ctrl_req);
> > > + spin_lock_irqsave(&vcrypto->ctrl_lock, flags);
> > > + }
> > > + if (unlikely(virtqueue_is_broken(vq)))
> > > + break;
> > > + } while (!virtqueue_enable_cb(vq));
> > > + spin_unlock_irqrestore(&vcrypto->ctrl_lock, flags);
> >
> > speaking of which existing code does not handle vq broken case
> > all that well but it looks like this patch makes it a bit worse.
> > want to try fixing? basically report an error ...
> >
> > if virtqueue is broken, I can print log.
>
> > > diff --git a/drivers/crypto/virtio/virtio_crypto_core.c b/drivers/crypto/virtio/virtio_crypto_core.c
> > > index c6f482db0bc0..e668d4b1bc6a 100644
> > > --- a/drivers/crypto/virtio/virtio_crypto_core.c
> > > +++ b/drivers/crypto/virtio/virtio_crypto_core.c
> > > @@ -73,7 +73,7 @@ static int virtcrypto_find_vqs(struct virtio_crypto *vi)
> > > goto err_names;
> > > /* Parameters for control virtqueue */
> > > - callbacks[total_vqs - 1] = NULL;
> > > + callbacks[total_vqs - 1] = virtcrypto_ctrlq_callback;
> > > names[total_vqs - 1] = "controlq";
> > > /* Allocate/initialize parameters for data virtqueues */
> > > diff --git a/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c b/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
> > > index a618c46a52b8..b8999dab3e66 100644
> > > --- a/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
> > > +++ b/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
> > > + err = 0;
> > > +out:
> > > + kfree_sensitive(vc_ctrl_req);
> >
> > it is interesting that you use kfree_sensitive here. why is that?
> > is there in fact anything sensitive here? if yes this is a security
> > improvement and might need its own patch, or at least documentation.
> >
>
> OK, kfree is good enough here, I'll fix this.
>
>
> Thanks a lot!
>
>
> --
> zhenwei pi
