RE: [PATCH v3 4/4] virtio-crypto: rename skcipher algs
> -Original Message-
> From: zhenwei pi [mailto:pizhen...@bytedance.com]
> Sent: Wednesday, March 2, 2022 11:39 AM
> To: Gonglei (Arei) ; m...@redhat.com
> Cc: jasow...@redhat.com; virtualization@lists.linux-foundation.org;
> linux-cry...@vger.kernel.org; linux-ker...@vger.kernel.org;
> herb...@gondor.apana.org.au; helei.si...@bytedance.com; zhenwei pi
>
> Subject: [PATCH v3 4/4] virtio-crypto: rename skcipher algs
>
> Suggested by Gonglei, rename virtio_crypto_algs.c to
> virtio_crypto_skcipher_algs.c. Also minor changes for function name.
> Thus the function of source files get clear: skcipher services in
> virtio_crypto_skcipher_algs.c and akcipher services in
> virtio_crypto_akcipher_algs.c.
>
> Signed-off-by: zhenwei pi
> ---
> drivers/crypto/virtio/Makefile| 2 +-
> drivers/crypto/virtio/virtio_crypto_common.h | 4 ++--
> drivers/crypto/virtio/virtio_crypto_mgr.c | 8
> ...virtio_crypto_algs.c => virtio_crypto_skcipher_algs.c} | 4 ++--
> 4 files changed, 9 insertions(+), 9 deletions(-) rename
> drivers/crypto/virtio/{virtio_crypto_algs.c => virtio_crypto_skcipher_algs.c}
> (99%)
>
Reviewed-by: Gonglei
Regards,
-Gonglei
> diff --git a/drivers/crypto/virtio/Makefile b/drivers/crypto/virtio/Makefile
> index
> f2b839473d61..bfa6cbae342e 100644
> --- a/drivers/crypto/virtio/Makefile
> +++ b/drivers/crypto/virtio/Makefile
> @@ -1,7 +1,7 @@
> # SPDX-License-Identifier: GPL-2.0
> obj-$(CONFIG_CRYPTO_DEV_VIRTIO) += virtio_crypto.o virtio_crypto-objs :=
> \
> - virtio_crypto_algs.o \
> + virtio_crypto_skcipher_algs.o \
> virtio_crypto_akcipher_algs.o \
> virtio_crypto_mgr.o \
> virtio_crypto_core.o
> diff --git a/drivers/crypto/virtio/virtio_crypto_common.h
> b/drivers/crypto/virtio/virtio_crypto_common.h
> index 214f9a6fcf84..e693d4ee83a6 100644
> --- a/drivers/crypto/virtio/virtio_crypto_common.h
> +++ b/drivers/crypto/virtio/virtio_crypto_common.h
> @@ -130,8 +130,8 @@ static inline int virtio_crypto_get_current_node(void)
> return node;
> }
>
> -int virtio_crypto_algs_register(struct virtio_crypto *vcrypto); -void
> virtio_crypto_algs_unregister(struct virtio_crypto *vcrypto);
> +int virtio_crypto_skcipher_algs_register(struct virtio_crypto
> +*vcrypto); void virtio_crypto_skcipher_algs_unregister(struct
> +virtio_crypto *vcrypto);
> int virtio_crypto_akcipher_algs_register(struct virtio_crypto *vcrypto);
> void
> virtio_crypto_akcipher_algs_unregister(struct virtio_crypto *vcrypto);
>
> diff --git a/drivers/crypto/virtio/virtio_crypto_mgr.c
> b/drivers/crypto/virtio/virtio_crypto_mgr.c
> index 1cb92418b321..70e778aac0f2 100644
> --- a/drivers/crypto/virtio/virtio_crypto_mgr.c
> +++ b/drivers/crypto/virtio/virtio_crypto_mgr.c
> @@ -237,14 +237,14 @@ struct virtio_crypto *virtcrypto_get_dev_node(int
> node, uint32_t service,
> */
> int virtcrypto_dev_start(struct virtio_crypto *vcrypto) {
> - if (virtio_crypto_algs_register(vcrypto)) {
> - pr_err("virtio_crypto: Failed to register crypto algs\n");
> + if (virtio_crypto_skcipher_algs_register(vcrypto)) {
> + pr_err("virtio_crypto: Failed to register crypto skcipher
> algs\n");
> return -EFAULT;
> }
>
> if (virtio_crypto_akcipher_algs_register(vcrypto)) {
> pr_err("virtio_crypto: Failed to register crypto akcipher
> algs\n");
> - virtio_crypto_algs_unregister(vcrypto);
> + virtio_crypto_skcipher_algs_unregister(vcrypto);
> return -EFAULT;
> }
>
> @@ -263,7 +263,7 @@ int virtcrypto_dev_start(struct virtio_crypto *vcrypto)
> */
> void virtcrypto_dev_stop(struct virtio_crypto *vcrypto) {
> - virtio_crypto_algs_unregister(vcrypto);
> + virtio_crypto_skcipher_algs_unregister(vcrypto);
> virtio_crypto_akcipher_algs_unregister(vcrypto);
> }
>
> diff --git a/drivers/crypto/virtio/virtio_crypto_algs.c
> b/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
> similarity index 99%
> rename from drivers/crypto/virtio/virtio_crypto_algs.c
> rename to drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
> index 583c0b535d13..a618c46a52b8 100644
> --- a/drivers/crypto/virtio/virtio_crypto_algs.c
> +++ b/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
> @@ -613,7 +613,7 @@ static struct virtio_crypto_algo virtio_crypto_algs[] =
> { {
> },
> } };
>
> -int virtio_crypto_algs_register(struct virtio_crypto *vcrypto)
> +int virtio_crypto_skcipher_algs_register(struct virtio_crypto *vcrypto)
> {
> int ret = 0;
> int i = 0;
> @@ -644,7 +644,7 @@ int virtio_crypto_algs_register(struct virtio_crypto
> *vcrypto)
> return ret;
> }
>
> -void virtio_crypto_algs_unregister(struct virtio_crypto *vcrypto)
> +void virtio_crypto_skcipher_algs_unregister(struct virtio_crypto
> +*vcrypto)
> {
> int i = 0;
>
> --
> 2.20.1
___
Virtualization
RE: [PATCH v3 3/4] virtio-crypto: implement RSA algorithm
> -Original Message-
> From: zhenwei pi [mailto:pizhen...@bytedance.com]
> Sent: Wednesday, March 2, 2022 11:39 AM
> To: Gonglei (Arei) ; m...@redhat.com
> Cc: jasow...@redhat.com; virtualization@lists.linux-foundation.org;
> linux-cry...@vger.kernel.org; linux-ker...@vger.kernel.org;
> herb...@gondor.apana.org.au; helei.si...@bytedance.com; zhenwei pi
> ; kernel test robot
> Subject: [PATCH v3 3/4] virtio-crypto: implement RSA algorithm
>
> Support rsa & pkcs1pad(rsa,sha1) with priority 150.
>
> Test with QEMU built-in backend, it works fine.
> 1, The self-test framework of crypto layer works fine in guest kernel 2, Test
> with Linux guest(with asym support), the following script test(note that
> pkey_XXX is supported only in a newer version of keyutils):
> - both public key & private key
> - create/close session
> - encrypt/decrypt/sign/verify basic driver operation
> - also test with kernel crypto layer(pkey add/query)
>
> All the cases work fine.
>
> rm -rf *.der *.pem *.pfx
> modprobe pkcs8_key_parser # if CONFIG_PKCS8_PRIVATE_KEY_PARSER=m rm
> -rf /tmp/data dd if=/dev/random of=/tmp/data count=1 bs=226
>
> openssl req -nodes -x509 -newkey rsa:2048 -keyout key.pem -out cert.pem -subj
> "/C=CN/ST=BJ/L=HD/O=qemu/OU=dev/CN=qemu/emailAddress=qemu@qemu
> .org"
> openssl pkcs8 -in key.pem -topk8 -nocrypt -outform DER -out key.der openssl
> x509 -in cert.pem -inform PEM -outform DER -out cert.der
>
> PRIV_KEY_ID=`cat key.der | keyctl padd asymmetric test_priv_key @s` echo
> "priv key id = "$PRIV_KEY_ID PUB_KEY_ID=`cat cert.der | keyctl padd
> asymmetric test_pub_key @s` echo "pub key id = "$PUB_KEY_ID
>
> keyctl pkey_query $PRIV_KEY_ID 0
> keyctl pkey_query $PUB_KEY_ID 0
>
> echo "Enc with priv key..."
> keyctl pkey_encrypt $PRIV_KEY_ID 0 /tmp/data enc=pkcs1 >/tmp/enc.priv echo
> "Dec with pub key..."
> keyctl pkey_decrypt $PRIV_KEY_ID 0 /tmp/enc.priv enc=pkcs1 >/tmp/dec cmp
> /tmp/data /tmp/dec
>
> echo "Sign with priv key..."
> keyctl pkey_sign $PRIV_KEY_ID 0 /tmp/data enc=pkcs1 hash=sha1 > /tmp/sig
> echo "Verify with pub key..."
> keyctl pkey_verify $PRIV_KEY_ID 0 /tmp/data /tmp/sig enc=pkcs1 hash=sha1
>
> echo "Enc with pub key..."
> keyctl pkey_encrypt $PUB_KEY_ID 0 /tmp/data enc=pkcs1 >/tmp/enc.pub echo
> "Dec with priv key..."
> keyctl pkey_decrypt $PRIV_KEY_ID 0 /tmp/enc.pub enc=pkcs1 >/tmp/dec cmp
> /tmp/data /tmp/dec
>
> echo "Verify with pub key..."
> keyctl pkey_verify $PUB_KEY_ID 0 /tmp/data /tmp/sig enc=pkcs1 hash=sha1
>
> [1 compiling warning during development]
> Reported-by: kernel test robot
>
> Co-developed-by: lei he
> Signed-off-by: lei he
> Signed-off-by: zhenwei pi
> ---
> drivers/crypto/virtio/Makefile| 1 +
> .../virtio/virtio_crypto_akcipher_algs.c | 585 ++
> drivers/crypto/virtio/virtio_crypto_common.h | 3 +
> drivers/crypto/virtio/virtio_crypto_core.c| 6 +-
> drivers/crypto/virtio/virtio_crypto_mgr.c | 11 +
> 5 files changed, 605 insertions(+), 1 deletion(-) create mode 100644
> drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
>
Reviewed-by: Gonglei
Regards,
-Gonglei
> diff --git a/drivers/crypto/virtio/Makefile b/drivers/crypto/virtio/Makefile
> index
> cbffa135..f2b839473d61 100644
> --- a/drivers/crypto/virtio/Makefile
> +++ b/drivers/crypto/virtio/Makefile
> @@ -2,5 +2,6 @@
> obj-$(CONFIG_CRYPTO_DEV_VIRTIO) += virtio_crypto.o virtio_crypto-objs :=
> \
> virtio_crypto_algs.o \
> + virtio_crypto_akcipher_algs.o \
> virtio_crypto_mgr.o \
> virtio_crypto_core.o
> diff --git a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> new file mode 100644
> index ..f3ec9420215e
> --- /dev/null
> +++ b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> @@ -0,0 +1,585 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> + /* Asymmetric algorithms supported by virtio crypto device
> + *
> + * Authors: zhenwei pi
> + * lei he
> + *
> + * Copyright 2022 Bytedance CO., LTD.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include "virtio_crypto_common.h"
> +
> +struct virtio_crypto_rsa_ctx {
> + MPI n;
> +};
> +
> +struct virtio_crypto_akcipher_ctx {
> + struct crypto_engine_ctx enginectx;
> + struct virtio_crypto *vcrypto;
> + struct crypto_akcipher *tfm;
> + bool session_valid;
> + __u64 session_id;
> + union {
> + struct virtio_crypto_rsa_ctx rsa_ctx;
> + };
> +};
> +
> +struct virtio_crypto_akcipher_request {
> + struct virtio_crypto_request base;
> + struct virtio_crypto_akcipher_ctx *akcipher_ctx;
> + struct akcipher_request *akcipher_req;
> + void *src_buf;
> + void *dst_buf;
> + uint32_t opcode;
> +};
> +
> +struct virtio_crypto_akcipher_algo {
> + uint32_t algonum;
> + uint32_t service;
> + unsigned
RE: [PATCH v3 2/4] virtio-crypto: introduce akcipher service
> -Original Message-
> From: zhenwei pi [mailto:pizhen...@bytedance.com]
> Sent: Wednesday, March 2, 2022 11:39 AM
> To: Gonglei (Arei) ; m...@redhat.com
> Cc: jasow...@redhat.com; virtualization@lists.linux-foundation.org;
> linux-cry...@vger.kernel.org; linux-ker...@vger.kernel.org;
> herb...@gondor.apana.org.au; helei.si...@bytedance.com; zhenwei pi
>
> Subject: [PATCH v3 2/4] virtio-crypto: introduce akcipher service
>
> Introduce asymmetric service definition, asymmetric operations and several
> well known algorithms.
>
> Co-developed-by: lei he
> Signed-off-by: lei he
> Signed-off-by: zhenwei pi
> ---
> include/uapi/linux/virtio_crypto.h | 81 +-
> 1 file changed, 80 insertions(+), 1 deletion(-)
>
Reviewed-by: Gonglei
Regards,
-Gonglei
> diff --git a/include/uapi/linux/virtio_crypto.h
> b/include/uapi/linux/virtio_crypto.h
> index 1166a49084b0..71a54a6849ca 100644
> --- a/include/uapi/linux/virtio_crypto.h
> +++ b/include/uapi/linux/virtio_crypto.h
> @@ -37,6 +37,7 @@
> #define VIRTIO_CRYPTO_SERVICE_HASH 1
> #define VIRTIO_CRYPTO_SERVICE_MAC2
> #define VIRTIO_CRYPTO_SERVICE_AEAD 3
> +#define VIRTIO_CRYPTO_SERVICE_AKCIPHER 4
>
> #define VIRTIO_CRYPTO_OPCODE(service, op) (((service) << 8) | (op))
>
> @@ -57,6 +58,10 @@ struct virtio_crypto_ctrl_header {
> VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x02)
> #define VIRTIO_CRYPTO_AEAD_DESTROY_SESSION \
> VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x03)
> +#define VIRTIO_CRYPTO_AKCIPHER_CREATE_SESSION \
> +VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AKCIPHER, 0x04)
> #define
> +VIRTIO_CRYPTO_AKCIPHER_DESTROY_SESSION \
> +VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AKCIPHER,
> 0x05)
> __le32 opcode;
> __le32 algo;
> __le32 flag;
> @@ -180,6 +185,58 @@ struct virtio_crypto_aead_create_session_req {
> __u8 padding[32];
> };
>
> +struct virtio_crypto_rsa_session_para {
> +#define VIRTIO_CRYPTO_RSA_RAW_PADDING 0
> +#define VIRTIO_CRYPTO_RSA_PKCS1_PADDING 1
> + __le32 padding_algo;
> +
> +#define VIRTIO_CRYPTO_RSA_NO_HASH 0
> +#define VIRTIO_CRYPTO_RSA_MD2 1
> +#define VIRTIO_CRYPTO_RSA_MD3 2
> +#define VIRTIO_CRYPTO_RSA_MD4 3
> +#define VIRTIO_CRYPTO_RSA_MD5 4
> +#define VIRTIO_CRYPTO_RSA_SHA1 5
> +#define VIRTIO_CRYPTO_RSA_SHA2566
> +#define VIRTIO_CRYPTO_RSA_SHA3847
> +#define VIRTIO_CRYPTO_RSA_SHA5128
> +#define VIRTIO_CRYPTO_RSA_SHA2249
> + __le32 hash_algo;
> +};
> +
> +struct virtio_crypto_ecdsa_session_para {
> +#define VIRTIO_CRYPTO_CURVE_UNKNOWN 0
> +#define VIRTIO_CRYPTO_CURVE_NIST_P192 1 #define
> +VIRTIO_CRYPTO_CURVE_NIST_P224 2 #define
> VIRTIO_CRYPTO_CURVE_NIST_P256 3
> +#define VIRTIO_CRYPTO_CURVE_NIST_P384 4 #define
> +VIRTIO_CRYPTO_CURVE_NIST_P521 5
> + __le32 curve_id;
> + __le32 padding;
> +};
> +
> +struct virtio_crypto_akcipher_session_para {
> +#define VIRTIO_CRYPTO_NO_AKCIPHER0
> +#define VIRTIO_CRYPTO_AKCIPHER_RSA 1
> +#define VIRTIO_CRYPTO_AKCIPHER_DSA 2
> +#define VIRTIO_CRYPTO_AKCIPHER_ECDSA 3
> + __le32 algo;
> +
> +#define VIRTIO_CRYPTO_AKCIPHER_KEY_TYPE_PUBLIC 1 #define
> +VIRTIO_CRYPTO_AKCIPHER_KEY_TYPE_PRIVATE 2
> + __le32 keytype;
> + __le32 keylen;
> +
> + union {
> + struct virtio_crypto_rsa_session_para rsa;
> + struct virtio_crypto_ecdsa_session_para ecdsa;
> + } u;
> +};
> +
> +struct virtio_crypto_akcipher_create_session_req {
> + struct virtio_crypto_akcipher_session_para para;
> + __u8 padding[36];
> +};
> +
> struct virtio_crypto_alg_chain_session_para { #define
> VIRTIO_CRYPTO_SYM_ALG_CHAIN_ORDER_HASH_THEN_CIPHER 1
> #define VIRTIO_CRYPTO_SYM_ALG_CHAIN_ORDER_CIPHER_THEN_HASH 2
> @@ -247,6 +304,8 @@ struct virtio_crypto_op_ctrl_req {
> mac_create_session;
> struct virtio_crypto_aead_create_session_req
> aead_create_session;
> + struct virtio_crypto_akcipher_create_session_req
> + akcipher_create_session;
> struct virtio_crypto_destroy_session_req
> destroy_session;
> __u8 padding[56];
> @@ -266,6 +325,14 @@ struct virtio_crypto_op_header {
> VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x00)
> #define VIRTIO_CRYPTO_AEAD_DECRYPT \
> VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x01)
> +#define VIRTIO_CRYPTO_AKCIPHER_ENCRYPT \
> + VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AKCIPHER, 0x00)
> #define
> +VIRTIO_CRYPTO_AKCIPHER_DECRYPT \
> + VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AKCIPHER, 0x01)
> #define
> +VIRTIO_CRYPTO_AKCIPHER_SIGN \
> + VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AKCIPHER, 0x02)
> #define
> +VIRTIO_CRYPTO_AKCIPHER_VERIFY \
> + VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AKCIPHER, 0x03)
> __le32 opcode;
> /* algo should be service-specific algorithms */
>
Re: [PATCH v1 1/2] vdpa: Add support for querying vendor statistics
Sorry, I somehow missed this after my break. Please see comments in line.
On 2/16/2022 10:46 PM, Eli Cohen wrote:
On Wed, Feb 16, 2022 at 10:49:26AM -0800, Si-Wei Liu wrote:
On 2/16/2022 12:00 AM, Eli Cohen wrote:
Allows to read vendor statistics of a vdpa device. The specific statistics
data is received by the upstream driver in the form of an (attribute
name, attribute value) pairs.
An example of statistics for mlx5_vdpa device are:
received_desc - number of descriptors received by the virtqueue
completed_desc - number of descriptors completed by the virtqueue
A descriptor using indirect buffers is still counted as 1. In addition,
N chained descriptors are counted correctly N times as one would expect.
A new callback was added to vdpa_config_ops which provides the means for
the vdpa driver to return statistics results.
The interface allows for reading all the supported virtqueues, including
the control virtqueue if it exists.
Below are some examples taken from mlx5_vdpa which are introduced in the
following patch:
1. Read statistics for the virtqueue at index 1
$ vdpa dev vstats show vdpa-a qidx 1
vdpa-a:
queue_type tx queue_index 1 received_desc 3844836 completed_desc 3844836
2. Read statistics for the virtqueue at index 32
$ vdpa dev vstats show vdpa-a qidx 32
vdpa-a:
queue_type control_vq queue_index 32 received_desc 62 completed_desc 62
3. Read statisitics for the virtqueue at index 0 with json output
$ vdpa -j dev vstats show vdpa-a qidx 0
{"vstats":{"vdpa-a":{
"queue_type":"rx","queue_index":0,"name":"received_desc","value":417776,\
"name":"completed_desc","value":417548}}}
4. Read statistics for the virtqueue at index 0 with preety json output
$ vdpa -jp dev vstats show vdpa-a qidx 0
{
"vstats": {
"vdpa-a": {
"queue_type": "rx",
I wonder where this info can be inferred? I don't see relevant change in the
patch series that helps gather the VDPA_ATTR_DEV_QUEUE_TYPE? Is this an
arbitrary string defined by the vendor as well? If so, how does the user
expect to consume it?
The queue tupe is deduced from the index and whether we have a
virtqueue. Even numbers are rx, odd numbers are tx and if there is CVQ,
the last one is CVQ.
OK, then VDPA_ATTR_DEV_QUEUE_TYPE attribute introduced in this patch
might not be useful at all? And how do you determine in the vdpa tool if
CVQ is negotiated or not? Looks to me there are still some loose end I
don't quite yet understand.
"queue_index": 0,
"name": "received_desc",
"value": 417776,
"name": "completed_desc",
"value": 417548
Not for this kernel patch, but IMHO it's the best to put the name & value
pairs in an array instead of flat entries in json's hash/dictionary. The
hash entries can be re-ordered deliberately by external json parsing tool,
ending up with inconsistent stat values.
This comment is missed for some reason. Please change the example in the
log if you agree to address it in vdpa tool. Or justify why keeping the
order for json hash/dictionary is fine.
Thanks,
-Siwei
Thanks,
-Siwei
}
}
}
Signed-off-by: Eli Cohen
---
drivers/vdpa/vdpa.c | 129 ++
include/linux/vdpa.h | 5 ++
include/uapi/linux/vdpa.h | 7 +++
3 files changed, 141 insertions(+)
diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index 9846c9de4bfa..d0ff671baf88 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -909,6 +909,74 @@ vdpa_dev_config_fill(struct vdpa_device *vdev, struct
sk_buff *msg, u32 portid,
return err;
}
+static int vdpa_fill_stats_rec(struct vdpa_device *vdev, struct sk_buff *msg,
+ struct genl_info *info, u32 index)
+{
+ int err;
+
+ if (nla_put_u32(msg, VDPA_ATTR_DEV_QUEUE_INDEX, index))
+ return -EMSGSIZE;
+
+ err = vdev->config->get_vendor_vq_stats(vdev, index, msg, info->extack);
+ if (err)
+ return err;
+
+ return 0;
+}
+
+static int vendor_stats_fill(struct vdpa_device *vdev, struct sk_buff *msg,
+struct genl_info *info, u32 index)
+{
+ int err;
+
+ if (!vdev->config->get_vendor_vq_stats)
+ return -EOPNOTSUPP;
+
+ err = vdpa_fill_stats_rec(vdev, msg, info, index);
+ if (err)
+ return err;
+
+ return 0;
+}
+
+static int vdpa_dev_vendor_stats_fill(struct vdpa_device *vdev,
+ struct sk_buff *msg,
+ struct genl_info *info, u32 index)
+{
+ u32 device_id;
+ void *hdr;
+ int err;
+ u32 portid = info->snd_portid;
+ u32 seq = info->snd_seq;
+ u32 flags = 0;
+
+ hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags,
+ VDPA_CMD_DEV_VSTATS_GET);
+ if (!hdr)
+ return -EMSGSIZE;
+
+ if (nla_put_string
Re: [PATCH 1/1] vhost: Protect the virtqueue from being cleared whilst still in use
On Fri, 04 Mar 2022, Michael S. Tsirkin wrote: > On Wed, Mar 02, 2022 at 07:54:21AM +, Lee Jones wrote: > > vhost_vsock_handle_tx_kick() already holds the mutex during its call > > to vhost_get_vq_desc(). All we have to do is take the same lock > > during virtqueue clean-up and we mitigate the reported issues. > > > > Link: https://syzkaller.appspot.com/bug?extid=279432d30d825e63ba00 > > > > Cc: > > Reported-by: syzbot+adc3cb32385586bec...@syzkaller.appspotmail.com > > Signed-off-by: Lee Jones > > OK so please post series with this and the warning > cleaned up comments and commit logs explaining that > this is just to make debugging easier in case > we have issues in the future, it's not a bugfix. No problem. Just to clarify, drop Cc: Stable also? -- Lee Jones [李琼斯] Principal Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/1] vhost: Protect the virtqueue from being cleared whilst still in use
On Wed, Mar 02, 2022 at 07:54:21AM +, Lee Jones wrote:
> vhost_vsock_handle_tx_kick() already holds the mutex during its call
> to vhost_get_vq_desc(). All we have to do is take the same lock
> during virtqueue clean-up and we mitigate the reported issues.
>
> Link: https://syzkaller.appspot.com/bug?extid=279432d30d825e63ba00
>
> Cc:
> Reported-by: syzbot+adc3cb32385586bec...@syzkaller.appspotmail.com
> Signed-off-by: Lee Jones
OK so please post series with this and the warning
cleaned up comments and commit logs explaining that
this is just to make debugging easier in case
we have issues in the future, it's not a bugfix.
> ---
> drivers/vhost/vhost.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 59edb5a1ffe28..bbaff6a5e21b8 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -693,6 +693,7 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
> int i;
>
> for (i = 0; i < dev->nvqs; ++i) {
> + mutex_lock(&dev->vqs[i]->mutex);
> if (dev->vqs[i]->error_ctx)
> eventfd_ctx_put(dev->vqs[i]->error_ctx);
> if (dev->vqs[i]->kick)
> @@ -700,6 +701,7 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
> if (dev->vqs[i]->call_ctx.ctx)
> eventfd_ctx_put(dev->vqs[i]->call_ctx.ctx);
> vhost_vq_reset(dev, dev->vqs[i]);
> + mutex_unlock(&dev->vqs[i]->mutex);
> }
> vhost_dev_free_iovecs(dev);
> if (dev->log_ctx)
> --
> 2.35.1.574.g5d30c73bfb-goog
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/1] vhost: Protect the virtqueue from being cleared whilst still in use
On Wed, Mar 02, 2022 at 04:36:43PM +0100, Stefano Garzarella wrote:
> On Wed, Mar 02, 2022 at 09:50:38AM -0500, Michael S. Tsirkin wrote:
> > On Wed, Mar 02, 2022 at 03:11:21PM +0100, Stefano Garzarella wrote:
> > > On Wed, Mar 02, 2022 at 08:35:08AM -0500, Michael S. Tsirkin wrote:
> > > > On Wed, Mar 02, 2022 at 10:34:46AM +0100, Stefano Garzarella wrote:
> > > > > On Wed, Mar 02, 2022 at 07:54:21AM +, Lee Jones wrote:
> > > > > > vhost_vsock_handle_tx_kick() already holds the mutex during its call
> > > > > > to vhost_get_vq_desc(). All we have to do is take the same lock
> > > > > > during virtqueue clean-up and we mitigate the reported issues.
> > > > > >
> > > > > > Link: https://syzkaller.appspot.com/bug?extid=279432d30d825e63ba00
> > > > >
> > > > > This issue is similar to [1] that should be already fixed upstream by
> > > > > [2].
> > > > >
> > > > > However I think this patch would have prevented some issues, because
> > > > > vhost_vq_reset() sets vq->private to NULL, preventing the worker from
> > > > > running.
> > > > >
> > > > > Anyway I think that when we enter in vhost_dev_cleanup() the worker
> > > > > should
> > > > > be already stopped, so it shouldn't be necessary to take the mutex.
> > > > > But in
> > > > > order to prevent future issues maybe it's better to take them, so:
> > > > >
> > > > > Reviewed-by: Stefano Garzarella
> > > > >
> > > > > [1]
> > > > > https://syzkaller.appspot.com/bug?id=993d8b5e64393ed9e6a70f9ae4de0119c605a822
> > > > > [2]
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a58da53ffd70294ebea8ecd0eb45fd0d74add9f9
> > > >
> > > >
> > > > Right. I want to queue this but I would like to get a warning
> > > > so we can detect issues like [2] before they cause more issues.
> > >
> > > I agree, what about moving the warning that we already have higher up,
> > > right
> > > at the beginning of the function?
> > >
> > > I mean something like this:
> > >
> > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > > index 59edb5a1ffe2..1721ff3f18c0 100644
> > > --- a/drivers/vhost/vhost.c
> > > +++ b/drivers/vhost/vhost.c
> > > @@ -692,6 +692,8 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
> > > {
> > > int i;
> > > + WARN_ON(!llist_empty(&dev->work_list));
> > > +
> > > for (i = 0; i < dev->nvqs; ++i) {
> > > if (dev->vqs[i]->error_ctx)
> > > eventfd_ctx_put(dev->vqs[i]->error_ctx);
> > > @@ -712,7 +714,6 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
> > > dev->iotlb = NULL;
> > > vhost_clear_msg(dev);
> > > wake_up_interruptible_poll(&dev->wait, EPOLLIN | EPOLLRDNORM);
> > > - WARN_ON(!llist_empty(&dev->work_list));
> > > if (dev->worker) {
> > > kthread_stop(dev->worker);
> > > dev->worker = NULL;
> > >
> >
> > Hmm I'm not sure why it matters.
>
> Because after this new patch, putting locks in the while loop, when we
> finish the loop the workers should be stopped, because vhost_vq_reset() sets
> vq->private to NULL.
>
> But the best thing IMHO is to check that there is no backend set for each
> vq, so the workers have been stopped correctly at this point.
>
> Thanks,
> Stefano
It's the list of workers waiting to run though. That is not affected by
vq lock at all.
--
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 9/9] virtio_net: xdp xmit use virtio dma api
On Thu, Feb 24, 2022 at 07:04:02PM +0800, Xuan Zhuo wrote:
> XDP xmit uses virtio dma api for DMA operations. No longer let virtio
> core manage DMA address.
>
> To record the DMA address, allocate a space in the xdp_frame headroom to
> store the DMA address.
>
> Introduce virtnet_return_xdp_frame() to release the xdp frame and
> complete the dma unmap operation.
This commit suffers from the same issue as most other commits
in this series: log just repeats what patch is doing without
adding motivation.
So with this patch applied, what happened exactly? Did something
previously broken start working now?
This is what we want in the commit log, first of all.
Thanks!
> Signed-off-by: Xuan Zhuo
> ---
> drivers/net/virtio_net.c | 42 +---
> 1 file changed, 35 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index a801ea40908f..0efbf7992a95 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -321,6 +321,20 @@ static struct page *get_a_page(struct receive_queue *rq,
> gfp_t gfp_mask)
> return p;
> }
>
> +static void virtnet_return_xdp_frame(struct send_queue *sq,
> + struct xdp_frame *frame)
> +{
> + struct virtnet_info *vi = sq->vq->vdev->priv;
> + dma_addr_t *p_addr, addr;
> +
> + p_addr = frame->data - sizeof(*p_addr);
> + addr = *p_addr;
> +
> + virtio_dma_unmap(&vi->vdev->dev, addr, frame->len, DMA_TO_DEVICE);
> +
> + xdp_return_frame(frame);
> +}
> +
> static void virtqueue_napi_schedule(struct napi_struct *napi,
> struct virtqueue *vq)
> {
> @@ -504,9 +518,11 @@ static int __virtnet_xdp_xmit_one(struct virtnet_info
> *vi,
> struct xdp_frame *xdpf)
> {
> struct virtio_net_hdr_mrg_rxbuf *hdr;
> + struct device *dev = &vi->vdev->dev;
> + dma_addr_t addr, *p_addr;
> int err;
>
> - if (unlikely(xdpf->headroom < vi->hdr_len))
> + if (unlikely(xdpf->headroom < vi->hdr_len + sizeof(addr)))
> return -EOVERFLOW;
>
> /* Make room for virtqueue hdr (also change xdpf->headroom?) */
> @@ -516,10 +532,21 @@ static int __virtnet_xdp_xmit_one(struct virtnet_info
> *vi,
> memset(hdr, 0, vi->hdr_len);
> xdpf->len += vi->hdr_len;
>
> - sg_init_one(sq->sg, xdpf->data, xdpf->len);
> + p_addr = xdpf->data - sizeof(addr);
> +
> + addr = virtio_dma_map(dev, xdpf->data, xdpf->len, DMA_TO_DEVICE);
> +
> + if (virtio_dma_mapping_error(dev, addr))
> + return -ENOMEM;
> +
> + *p_addr = addr;
> +
> + sg_init_table(sq->sg, 1);
> + sq->sg->dma_address = addr;
> + sq->sg->length = xdpf->len;
>
> - err = virtqueue_add_outbuf(sq->vq, sq->sg, 1, xdp_to_ptr(xdpf),
> -GFP_ATOMIC);
> + err = virtqueue_add_outbuf_premapped(sq->vq, sq->sg, 1,
> + xdp_to_ptr(xdpf), GFP_ATOMIC);
> if (unlikely(err))
> return -ENOSPC; /* Caller handle free/refcnt */
>
> @@ -600,7 +627,7 @@ static int virtnet_xdp_xmit(struct net_device *dev,
> struct xdp_frame *frame = ptr_to_xdp(ptr);
>
> bytes += frame->len;
> - xdp_return_frame(frame);
> + virtnet_return_xdp_frame(sq, frame);
> } else {
> struct sk_buff *skb = ptr;
>
> @@ -1486,7 +1513,7 @@ static void free_old_xmit_skbs(struct send_queue *sq,
> bool in_napi)
> struct xdp_frame *frame = ptr_to_xdp(ptr);
>
> bytes += frame->len;
> - xdp_return_frame(frame);
> + virtnet_return_xdp_frame(sq, frame);
> }
> packets++;
> }
> @@ -2815,7 +2842,8 @@ static void free_unused_bufs(struct virtnet_info *vi)
> if (!is_xdp_frame(buf))
> dev_kfree_skb(buf);
> else
> - xdp_return_frame(ptr_to_xdp(buf));
> + virtnet_return_xdp_frame(vi->sq + i,
> + ptr_to_xdp(buf));
> }
> }
>
> --
> 2.31.0
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v1 0/2] Show statistics for a vdpa device
On Wed, Feb 16, 2022 at 10:00:20AM +0200, Eli Cohen wrote: > The following two patch series adds support to read vendor statistics > for a vdpa device. > > The first patch lays the ground to allow an upstream driver to provide > statistics in the form of an attribute name/attribute value pairs. > > The second patch implements this for mlx5_vdpa which gives received > descriptors and completed descriptors information for all the > virtqueues. > > V0 -> V1: > 1. Function name changes to emphasize the fact that this is for vendor > statistics. > 2. Increase the size of VDPA_ATTR_DEV_QUEUE_INDEX to U32 so it can > handle the entire range of virtqueue indices. > 3. Change output string names to avoid abbreviations. Jason had a minor comment. Were you goint to address it? > Eli Cohen (2): > vdpa: Add support for querying vendor statistics > vdpa/mlx5: Add support for reading descriptor statistics > > drivers/vdpa/mlx5/core/mlx5_vdpa.h | 2 + > drivers/vdpa/mlx5/net/mlx5_vnet.c | 156 + > drivers/vdpa/vdpa.c| 129 > include/linux/mlx5/mlx5_ifc.h | 1 + > include/linux/mlx5/mlx5_ifc_vdpa.h | 39 > include/linux/vdpa.h | 5 + > include/uapi/linux/vdpa.h | 7 ++ > 7 files changed, 339 insertions(+) > > -- > 2.34.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/1] vhost: Protect the virtqueue from being cleared whilst still in use
On Fri, 04 Mar 2022, Michael S. Tsirkin wrote: > On Wed, Mar 02, 2022 at 07:54:21AM +, Lee Jones wrote: > > vhost_vsock_handle_tx_kick() already holds the mutex during its call > > to vhost_get_vq_desc(). All we have to do is take the same lock > > during virtqueue clean-up and we mitigate the reported issues. > > > > Link: https://syzkaller.appspot.com/bug?extid=279432d30d825e63ba00 > > > > Cc: > > Reported-by: syzbot+adc3cb32385586bec...@syzkaller.appspotmail.com > > Signed-off-by: Lee Jones > > So combine with the warning patch and update description with > the comment I posted, explaining it's more a just in case thing. Will do. Plan is to submit this on Monday. -- Lee Jones [李琼斯] Principal Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v4 3/4] drivers/net/virtio_net: Added RSS hash report.
Hi all,
Yes, I'll prepare a new commit later.
On Fri, Mar 4, 2022 at 10:08 AM Michael S. Tsirkin wrote:
>
> On Wed, Feb 23, 2022 at 03:15:28AM +0800, kernel test robot wrote:
> > Hi Andrew,
> >
> > Thank you for the patch! Perhaps something to improve:
> >
> > [auto build test WARNING on mst-vhost/linux-next]
> > [also build test WARNING on net/master horms-ipvs/master net-next/master
> > linus/master v5.17-rc5 next-20220217]
> > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > And when submitting patch, we suggest to use '--base' as documented in
> > https://git-scm.com/docs/git-format-patch]
>
>
> Andrew,
> do you plan to fix this?
>
> > url:
> > https://github.com/0day-ci/linux/commits/Andrew-Melnychenko/RSS-support-for-VirtioNet/20220222-200334
> > base: https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git
> > linux-next
> > config: i386-randconfig-s002-20220221
> > (https://download.01.org/0day-ci/archive/20220223/202202230342.hpye6dha-...@intel.com/config)
> > compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
> > reproduce:
> > # apt-get install sparse
> > # sparse version: v0.6.4-dirty
> > #
> > https://github.com/0day-ci/linux/commit/4fda71c17afd24d8afb675baa0bb14dbbc6cd23c
> > git remote add linux-review https://github.com/0day-ci/linux
> > git fetch --no-tags linux-review
> > Andrew-Melnychenko/RSS-support-for-VirtioNet/20220222-200334
> > git checkout 4fda71c17afd24d8afb675baa0bb14dbbc6cd23c
> > # save the config file to linux build tree
> > mkdir build_dir
> > make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'
> > O=build_dir ARCH=i386 SHELL=/bin/bash
> >
> > If you fix the issue, kindly add following tag as appropriate
> > Reported-by: kernel test robot
> >
> >
> > sparse warnings: (new ones prefixed by >>)
> >drivers/net/virtio_net.c:1160:25: sparse: sparse: restricted __le16
> > degrades to integer
> >drivers/net/virtio_net.c:1160:25: sparse: sparse: restricted __le16
> > degrades to integer
> >drivers/net/virtio_net.c:1160:25: sparse: sparse: restricted __le16
> > degrades to integer
> >drivers/net/virtio_net.c:1160:25: sparse: sparse: restricted __le16
> > degrades to integer
> >drivers/net/virtio_net.c:1160:25: sparse: sparse: restricted __le16
> > degrades to integer
> >drivers/net/virtio_net.c:1160:25: sparse: sparse: restricted __le16
> > degrades to integer
> >drivers/net/virtio_net.c:1160:25: sparse: sparse: restricted __le16
> > degrades to integer
> >drivers/net/virtio_net.c:1160:25: sparse: sparse: restricted __le16
> > degrades to integer
> >drivers/net/virtio_net.c:1160:25: sparse: sparse: restricted __le16
> > degrades to integer
> > >> drivers/net/virtio_net.c:1178:35: sparse: sparse: incorrect type in
> > >> argument 2 (different base types) @@ expected unsigned int
> > >> [usertype] hash @@ got restricted __le32 const [usertype] hash_value
> > >> @@
> >drivers/net/virtio_net.c:1178:35: sparse: expected unsigned int
> > [usertype] hash
> >drivers/net/virtio_net.c:1178:35: sparse: got restricted __le32
> > const [usertype] hash_value
> >
> > vim +1178 drivers/net/virtio_net.c
> >
> > 1151
> > 1152static void virtio_skb_set_hash(const struct
> > virtio_net_hdr_v1_hash *hdr_hash,
> > 1153struct sk_buff *skb)
> > 1154{
> > 1155enum pkt_hash_types rss_hash_type;
> > 1156
> > 1157if (!hdr_hash || !skb)
> > 1158return;
> > 1159
> > 1160switch (hdr_hash->hash_report) {
> > 1161case VIRTIO_NET_HASH_REPORT_TCPv4:
> > 1162case VIRTIO_NET_HASH_REPORT_UDPv4:
> > 1163case VIRTIO_NET_HASH_REPORT_TCPv6:
> > 1164case VIRTIO_NET_HASH_REPORT_UDPv6:
> > 1165case VIRTIO_NET_HASH_REPORT_TCPv6_EX:
> > 1166case VIRTIO_NET_HASH_REPORT_UDPv6_EX:
> > 1167rss_hash_type = PKT_HASH_TYPE_L4;
> > 1168break;
> > 1169case VIRTIO_NET_HASH_REPORT_IPv4:
> > 1170case VIRTIO_NET_HASH_REPORT_IPv6:
> > 1171case VIRTIO_NET_HASH_REPORT_IPv6_EX:
> > 1172rss_hash_type = PKT_HASH_TYPE_L3;
> > 1173break;
> > 1174case VIRTIO_NET_HASH_REPORT_NONE:
> > 1175default:
> > 1176rss_hash_type = PKT_HASH_TYPE_NONE;
> > 1177}
> > > 1178skb_set_hash(skb, hdr_hash->hash_value,
> > > rss_hash_type);
> > 1179}
> > 1180
> >
> > ---
> > 0-DAY CI Kernel Test Service, Intel Corporation
> > https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
>
___
Virtualization mailing list
Virtualizat
Re: [PATCH 1/1] vhost: Provide a kernel warning if mutex is held whilst clean-up in progress
On Fri, 04 Mar 2022, Stefano Garzarella wrote:
> On Thu, Mar 03, 2022 at 04:01:06PM -0500, Michael S. Tsirkin wrote:
> > On Thu, Mar 03, 2022 at 09:14:36PM +0200, Leon Romanovsky wrote:
> > > On Thu, Mar 03, 2022 at 03:19:29PM +, Lee Jones wrote:
> > > > All workers/users should be halted before any clean-up should take
> > > > place.
> > > >
> > > > Suggested-by: Michael S. Tsirkin
> > > > Signed-off-by: Lee Jones
> > > > ---
> > > > drivers/vhost/vhost.c | 3 +++
> > > > 1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > > > index bbaff6a5e21b8..d935d2506963f 100644
> > > > --- a/drivers/vhost/vhost.c
> > > > +++ b/drivers/vhost/vhost.c
> > > > @@ -693,6 +693,9 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
> > > > int i;
> > > >
> > > > for (i = 0; i < dev->nvqs; ++i) {
> > > > + /* Ideally all workers should be stopped prior to
> > > > clean-up */
> > > > + WARN_ON(mutex_is_locked(&dev->vqs[i]->mutex));
> > > > +
> > > > mutex_lock(&dev->vqs[i]->mutex);
> > >
> > > I know nothing about vhost, but this construction and patch looks
> > > strange to me.
> > >
> > > If all workers were stopped, you won't need mutex_lock(). The mutex_lock
> > > here suggests to me that workers can still run here.
> > >
> > > Thanks
> >
> >
> > "Ideally" here is misleading, we need a bigger detailed comment
> > along the lines of:
> >
> > /*
> > * By design, no workers can run here. But if there's a bug and the
> > * driver did not flush all work properly then they might, and we
> > * encountered such bugs in the past. With no proper flush guest won't
> > * work correctly but avoiding host memory corruption in this case
> > * sounds like a good idea.
> > */
>
> Can we use vhost_vq_get_backend() to check this situation?
>
> IIUC all the vhost devices clear the backend to stop the workers.
> This is not racy (if we do after the mutex_lock) and should cover all cases.
I can look into this too if you like.
--
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/1] vhost: Provide a kernel warning if mutex is held whilst clean-up in progress
On Fri, 04 Mar 2022, Leon Romanovsky wrote:
> On Thu, Mar 03, 2022 at 04:01:06PM -0500, Michael S. Tsirkin wrote:
> > On Thu, Mar 03, 2022 at 09:14:36PM +0200, Leon Romanovsky wrote:
> > > On Thu, Mar 03, 2022 at 03:19:29PM +, Lee Jones wrote:
> > > > All workers/users should be halted before any clean-up should take
> > > > place.
> > > >
> > > > Suggested-by: Michael S. Tsirkin
> > > > Signed-off-by: Lee Jones
> > > > ---
> > > > drivers/vhost/vhost.c | 3 +++
> > > > 1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > > > index bbaff6a5e21b8..d935d2506963f 100644
> > > > --- a/drivers/vhost/vhost.c
> > > > +++ b/drivers/vhost/vhost.c
> > > > @@ -693,6 +693,9 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
> > > > int i;
> > > >
> > > > for (i = 0; i < dev->nvqs; ++i) {
> > > > + /* Ideally all workers should be stopped prior to
> > > > clean-up */
> > > > + WARN_ON(mutex_is_locked(&dev->vqs[i]->mutex));
> > > > +
> > > > mutex_lock(&dev->vqs[i]->mutex);
HERE ---^
> > > I know nothing about vhost, but this construction and patch looks
> > > strange to me.
> > >
> > > If all workers were stopped, you won't need mutex_lock(). The mutex_lock
> > > here suggests to me that workers can still run here.
> > >
> > > Thanks
> >
> >
> > "Ideally" here is misleading, we need a bigger detailed comment
> > along the lines of:
> >
> > /*
> > * By design, no workers can run here. But if there's a bug and the
> > * driver did not flush all work properly then they might, and we
> > * encountered such bugs in the past. With no proper flush guest won't
> > * work correctly but avoiding host memory corruption in this case
> > * sounds like a good idea.
> > */
>
> This description looks better, but the check is inherently racy.
> Why don't you add a comment and mutex_lock()?
We do, look up. ^
> The WARN_ON here is more distraction than actual help.
The WARN() is just an indication that something else has gone wrong.
Stefano patched one problem in:
vhost: Protect the virtqueue from being cleared whilst still in use
... but others may crop up and the WARN() is how we'll be informed.
--
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v4 3/4] drivers/net/virtio_net: Added RSS hash report.
On Wed, Feb 23, 2022 at 03:15:28AM +0800, kernel test robot wrote:
> Hi Andrew,
>
> Thank you for the patch! Perhaps something to improve:
>
> [auto build test WARNING on mst-vhost/linux-next]
> [also build test WARNING on net/master horms-ipvs/master net-next/master
> linus/master v5.17-rc5 next-20220217]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
Andrew,
do you plan to fix this?
> url:
> https://github.com/0day-ci/linux/commits/Andrew-Melnychenko/RSS-support-for-VirtioNet/20220222-200334
> base: https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git
> linux-next
> config: i386-randconfig-s002-20220221
> (https://download.01.org/0day-ci/archive/20220223/202202230342.hpye6dha-...@intel.com/config)
> compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
> reproduce:
> # apt-get install sparse
> # sparse version: v0.6.4-dirty
> #
> https://github.com/0day-ci/linux/commit/4fda71c17afd24d8afb675baa0bb14dbbc6cd23c
> git remote add linux-review https://github.com/0day-ci/linux
> git fetch --no-tags linux-review
> Andrew-Melnychenko/RSS-support-for-VirtioNet/20220222-200334
> git checkout 4fda71c17afd24d8afb675baa0bb14dbbc6cd23c
> # save the config file to linux build tree
> mkdir build_dir
> make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir
> ARCH=i386 SHELL=/bin/bash
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot
>
>
> sparse warnings: (new ones prefixed by >>)
>drivers/net/virtio_net.c:1160:25: sparse: sparse: restricted __le16
> degrades to integer
>drivers/net/virtio_net.c:1160:25: sparse: sparse: restricted __le16
> degrades to integer
>drivers/net/virtio_net.c:1160:25: sparse: sparse: restricted __le16
> degrades to integer
>drivers/net/virtio_net.c:1160:25: sparse: sparse: restricted __le16
> degrades to integer
>drivers/net/virtio_net.c:1160:25: sparse: sparse: restricted __le16
> degrades to integer
>drivers/net/virtio_net.c:1160:25: sparse: sparse: restricted __le16
> degrades to integer
>drivers/net/virtio_net.c:1160:25: sparse: sparse: restricted __le16
> degrades to integer
>drivers/net/virtio_net.c:1160:25: sparse: sparse: restricted __le16
> degrades to integer
>drivers/net/virtio_net.c:1160:25: sparse: sparse: restricted __le16
> degrades to integer
> >> drivers/net/virtio_net.c:1178:35: sparse: sparse: incorrect type in
> >> argument 2 (different base types) @@ expected unsigned int [usertype]
> >> hash @@ got restricted __le32 const [usertype] hash_value @@
>drivers/net/virtio_net.c:1178:35: sparse: expected unsigned int
> [usertype] hash
>drivers/net/virtio_net.c:1178:35: sparse: got restricted __le32 const
> [usertype] hash_value
>
> vim +1178 drivers/net/virtio_net.c
>
> 1151
> 1152static void virtio_skb_set_hash(const struct
> virtio_net_hdr_v1_hash *hdr_hash,
> 1153struct sk_buff *skb)
> 1154{
> 1155enum pkt_hash_types rss_hash_type;
> 1156
> 1157if (!hdr_hash || !skb)
> 1158return;
> 1159
> 1160switch (hdr_hash->hash_report) {
> 1161case VIRTIO_NET_HASH_REPORT_TCPv4:
> 1162case VIRTIO_NET_HASH_REPORT_UDPv4:
> 1163case VIRTIO_NET_HASH_REPORT_TCPv6:
> 1164case VIRTIO_NET_HASH_REPORT_UDPv6:
> 1165case VIRTIO_NET_HASH_REPORT_TCPv6_EX:
> 1166case VIRTIO_NET_HASH_REPORT_UDPv6_EX:
> 1167rss_hash_type = PKT_HASH_TYPE_L4;
> 1168break;
> 1169case VIRTIO_NET_HASH_REPORT_IPv4:
> 1170case VIRTIO_NET_HASH_REPORT_IPv6:
> 1171case VIRTIO_NET_HASH_REPORT_IPv6_EX:
> 1172rss_hash_type = PKT_HASH_TYPE_L3;
> 1173break;
> 1174case VIRTIO_NET_HASH_REPORT_NONE:
> 1175default:
> 1176rss_hash_type = PKT_HASH_TYPE_NONE;
> 1177}
> > 1178skb_set_hash(skb, hdr_hash->hash_value, rss_hash_type);
> 1179}
> 1180
>
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
