[PATCH 2/2] Revert "virtio_pci: harden MSI-X interrupts"

[Jason Wang]

This reverts commit 9e35276a5344f74d4a3600fc4100b3dd251d5c56. Issue
were reported for the drivers that are using affinity managed IRQ
where manually toggling IRQ status is not expected. And we forget to
enable the interrupts in the restore path as well.

In the future, we will rework on the interrupt hardening.

Fixes: 9e35276a5344 ("virtio_pci: harden MSI-X interrupts")
Reported-by: Marc Zyngier 
Reported-by: Stefano Garzarella 
Signed-off-by: Jason Wang 
---
 drivers/virtio/virtio_pci_common.c | 27 ++-
 drivers/virtio/virtio_pci_common.h |  6 ++
 drivers/virtio/virtio_pci_legacy.c |  5 ++---
 drivers/virtio/virtio_pci_modern.c |  6 ++
 4 files changed, 12 insertions(+), 32 deletions(-)

diff --git a/drivers/virtio/virtio_pci_common.c 
b/drivers/virtio/virtio_pci_common.c
index 3f51fdb7be45..d724f676608b 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -24,8 +24,8 @@ MODULE_PARM_DESC(force_legacy,
 "Force legacy mode for transitional virtio 1 devices");
 #endif
 
-/* disable irq handlers */
-void vp_disable_cbs(struct virtio_device *vdev)
+/* wait for pending irq handlers */
+void vp_synchronize_vectors(struct virtio_device *vdev)
 {
struct virtio_pci_device *vp_dev = to_vp_device(vdev);
int i;
@@ -34,20 +34,7 @@ void vp_disable_cbs(struct virtio_device *vdev)
synchronize_irq(vp_dev->pci_dev->irq);
 
for (i = 0; i < vp_dev->msix_vectors; ++i)
-   disable_irq(pci_irq_vector(vp_dev->pci_dev, i));
-}
-
-/* enable irq handlers */
-void vp_enable_cbs(struct virtio_device *vdev)
-{
-   struct virtio_pci_device *vp_dev = to_vp_device(vdev);
-   int i;
-
-   if (vp_dev->intx_enabled)
-   return;
-
-   for (i = 0; i < vp_dev->msix_vectors; ++i)
-   enable_irq(pci_irq_vector(vp_dev->pci_dev, i));
+   synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i));
 }
 
 /* the notify function used when creating a virt queue */
@@ -154,8 +141,7 @@ static int vp_request_msix_vectors(struct virtio_device 
*vdev, int nvectors,
snprintf(vp_dev->msix_names[v], sizeof *vp_dev->msix_names,
 "%s-config", name);
err = request_irq(pci_irq_vector(vp_dev->pci_dev, v),
- vp_config_changed, IRQF_NO_AUTOEN,
- vp_dev->msix_names[v],
+ vp_config_changed, 0, vp_dev->msix_names[v],
  vp_dev);
if (err)
goto error;
@@ -174,8 +160,7 @@ static int vp_request_msix_vectors(struct virtio_device 
*vdev, int nvectors,
snprintf(vp_dev->msix_names[v], sizeof *vp_dev->msix_names,
 "%s-virtqueues", name);
err = request_irq(pci_irq_vector(vp_dev->pci_dev, v),
- vp_vring_interrupt, IRQF_NO_AUTOEN,
- vp_dev->msix_names[v],
+ vp_vring_interrupt, 0, vp_dev->msix_names[v],
  vp_dev);
if (err)
goto error;
@@ -352,7 +337,7 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, 
unsigned nvqs,
 "%s-%s",
 dev_name(_dev->vdev.dev), names[i]);
err = request_irq(pci_irq_vector(vp_dev->pci_dev, msix_vec),
- vring_interrupt, IRQF_NO_AUTOEN,
+ vring_interrupt, 0,
  vp_dev->msix_names[msix_vec],
  vqs[i]);
if (err)
diff --git a/drivers/virtio/virtio_pci_common.h 
b/drivers/virtio/virtio_pci_common.h
index d3c6f72c7390..eb17a29fc7ef 100644
--- a/drivers/virtio/virtio_pci_common.h
+++ b/drivers/virtio/virtio_pci_common.h
@@ -101,10 +101,8 @@ static struct virtio_pci_device *to_vp_device(struct 
virtio_device *vdev)
return container_of(vdev, struct virtio_pci_device, vdev);
 }
 
-/* disable irq handlers */
-void vp_disable_cbs(struct virtio_device *vdev);
-/* enable irq handlers */
-void vp_enable_cbs(struct virtio_device *vdev);
+/* wait for pending irq handlers */
+void vp_synchronize_vectors(struct virtio_device *vdev);
 /* the notify function used when creating a virt queue */
 bool vp_notify(struct virtqueue *vq);
 /* the config->del_vqs() implementation */
diff --git a/drivers/virtio/virtio_pci_legacy.c 
b/drivers/virtio/virtio_pci_legacy.c
index 34141b9abe27..6f4e34ce96b8 100644
--- a/drivers/virtio/virtio_pci_legacy.c
+++ b/drivers/virtio/virtio_pci_legacy.c
@@ -98,8 +98,8 @@ static void vp_reset(struct virtio_device *vdev)
/* Flush out the status write, and flush in device writes,
 * including MSi-X interrupts, if any. */
vp_legacy_get_status(_dev->ldev);
-   /* Disable VQ/configuration callbacks. */
-   vp_disable_cbs(vdev);
+   /* Flush pending 

[PATCH 1/2] Revert "virtio-pci: harden INTX interrupts"

[Jason Wang]

This reverts commit 080cd7c3ac8701081d143a15ba17dd9475313188. Since
the MSI-X interrupts hardening will be reverted in the next patch. We
will rework the interrupt hardening in the future.

Fixes: 080cd7c3ac87 ("virtio-pci: harden INTX interrupts")
Signed-off-by: Jason Wang 
---
 drivers/virtio/virtio_pci_common.c | 23 ++-
 drivers/virtio/virtio_pci_common.h |  1 -
 2 files changed, 2 insertions(+), 22 deletions(-)

diff --git a/drivers/virtio/virtio_pci_common.c 
b/drivers/virtio/virtio_pci_common.c
index fdbde1db5ec5..3f51fdb7be45 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -30,16 +30,8 @@ void vp_disable_cbs(struct virtio_device *vdev)
struct virtio_pci_device *vp_dev = to_vp_device(vdev);
int i;
 
-   if (vp_dev->intx_enabled) {
-   /*
-* The below synchronize() guarantees that any
-* interrupt for this line arriving after
-* synchronize_irq() has completed is guaranteed to see
-* intx_soft_enabled == false.
-*/
-   WRITE_ONCE(vp_dev->intx_soft_enabled, false);
+   if (vp_dev->intx_enabled)
synchronize_irq(vp_dev->pci_dev->irq);
-   }
 
for (i = 0; i < vp_dev->msix_vectors; ++i)
disable_irq(pci_irq_vector(vp_dev->pci_dev, i));
@@ -51,16 +43,8 @@ void vp_enable_cbs(struct virtio_device *vdev)
struct virtio_pci_device *vp_dev = to_vp_device(vdev);
int i;
 
-   if (vp_dev->intx_enabled) {
-   disable_irq(vp_dev->pci_dev->irq);
-   /*
-* The above disable_irq() provides TSO ordering and
-* as such promotes the below store to store-release.
-*/
-   WRITE_ONCE(vp_dev->intx_soft_enabled, true);
-   enable_irq(vp_dev->pci_dev->irq);
+   if (vp_dev->intx_enabled)
return;
-   }
 
for (i = 0; i < vp_dev->msix_vectors; ++i)
enable_irq(pci_irq_vector(vp_dev->pci_dev, i));
@@ -113,9 +97,6 @@ static irqreturn_t vp_interrupt(int irq, void *opaque)
struct virtio_pci_device *vp_dev = opaque;
u8 isr;
 
-   if (!READ_ONCE(vp_dev->intx_soft_enabled))
-   return IRQ_NONE;
-
/* reading the ISR has the effect of also clearing it so it's very
 * important to save off the value. */
isr = ioread8(vp_dev->isr);
diff --git a/drivers/virtio/virtio_pci_common.h 
b/drivers/virtio/virtio_pci_common.h
index 23f6c5c678d5..d3c6f72c7390 100644
--- a/drivers/virtio/virtio_pci_common.h
+++ b/drivers/virtio/virtio_pci_common.h
@@ -63,7 +63,6 @@ struct virtio_pci_device {
/* MSI-X support */
int msix_enabled;
int intx_enabled;
-   bool intx_soft_enabled;
cpumask_var_t *msix_affinity_masks;
/* Name strings for interrupts. This size should be enough,
 * and I'm too lazy to allocate each name separately. */
-- 
2.25.1

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] virtio: use virtio_device_ready() in virtio_device_restore()

[Jason Wang]

On Tue, Mar 22, 2022 at 10:07 PM Michael S. Tsirkin  wrote:
>
> On Tue, Mar 22, 2022 at 12:43:13PM +0100, Stefano Garzarella wrote:
> > After waking up a suspended VM, the kernel prints the following trace
> > for virtio drivers which do not directly call virtio_device_ready() in
> > the .restore:
> >
> > PM: suspend exit
> > irq 22: nobody cared (try booting with the "irqpoll" option)
> > Call Trace:
> >  
> >  dump_stack_lvl+0x38/0x49
> >  dump_stack+0x10/0x12
> >  __report_bad_irq+0x3a/0xaf
> >  note_interrupt.cold+0xb/0x60
> >  handle_irq_event+0x71/0x80
> >  handle_fasteoi_irq+0x95/0x1e0
> >  __common_interrupt+0x6b/0x110
> >  common_interrupt+0x63/0xe0
> >  asm_common_interrupt+0x1e/0x40
> >  ? __do_softirq+0x75/0x2f3
> >  irq_exit_rcu+0x93/0xe0
> >  sysvec_apic_timer_interrupt+0xac/0xd0
> >  
> >  
> >  asm_sysvec_apic_timer_interrupt+0x12/0x20
> >  arch_cpu_idle+0x12/0x20
> >  default_idle_call+0x39/0xf0
> >  do_idle+0x1b5/0x210
> >  cpu_startup_entry+0x20/0x30
> >  start_secondary+0xf3/0x100
> >  secondary_startup_64_no_verify+0xc3/0xcb
> >  
> > handlers:
> > [<8f9bac49>] vp_interrupt
> > [<8f9bac49>] vp_interrupt
> > Disabling IRQ #22
> >
> > This happens because we don't invoke .enable_cbs callback in
> > virtio_device_restore(). That callback is used by some transports
> > (e.g. virtio-pci) to enable interrupts.
> >
> > Let's fix it, by calling virtio_device_ready() as we do in
> > virtio_dev_probe(). This function calls .enable_cts callback and sets
> > DRIVER_OK status bit.
> >
> > This fix also avoids setting DRIVER_OK twice for those drivers that
> > call virtio_device_ready() in the .restore.
> >
> > Fixes: d50497eb4e55 ("virtio_config: introduce a new .enable_cbs method")
> > Signed-off-by: Stefano Garzarella 
> > ---
> >
> > I'm not sure about the fixes tag. That one is more generic, but the
> > following one I think introduced the issue.
> >
> > Fixes: 9e35276a5344 ("virtio_pci: harden MSI-X interrupts")
>
> Jason what should we do about this one BTW? Just revert? We have other
> issues ...

Let me post a patch to revert it and give it a rework.

Thanks

>
>
> > Thanks,
> > Stefano
> > ---
> >  drivers/virtio/virtio.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > index 22f15f444f75..75c8d560bbd3 100644
> > --- a/drivers/virtio/virtio.c
> > +++ b/drivers/virtio/virtio.c
> > @@ -526,8 +526,9 @@ int virtio_device_restore(struct virtio_device *dev)
> >   goto err;
> >   }
> >
> > - /* Finally, tell the device we're all set */
> > - virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
> > + /* If restore didn't do it, mark device DRIVER_OK ourselves. */
> > + if (!(dev->config->get_status(dev) & VIRTIO_CONFIG_S_DRIVER_OK))
> > + virtio_device_ready(dev);
> >
> >   virtio_config_enable(dev);
> >
> > --
> > 2.35.1
>

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH v3 6/6] virtio-crypto: Introduce RSA algorithm

[zhenwei pi]

There are two parts in this patch:
1, support akcipher service by cryptodev-builtin driver
2, virtio-crypto driver supports akcipher service

Then virtio-crypto gets request from guest side, and forwards the
request to builtin driver to handle it.

Test with a guest linux:
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.

Run script in guest:
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=q...@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

Co-developed-by: lei he 
Signed-off-by: lei he conf.crypto_services =
  1u << VIRTIO_CRYPTO_SERVICE_CIPHER |
  1u << VIRTIO_CRYPTO_SERVICE_HASH |
- 1u << VIRTIO_CRYPTO_SERVICE_MAC;
+ 1u << VIRTIO_CRYPTO_SERVICE_MAC |
+ 1u << VIRTIO_CRYPTO_SERVICE_AKCIPHER;
 backend->conf.cipher_algo_l = 1u << VIRTIO_CRYPTO_CIPHER_AES_CBC;
 backend->conf.hash_algo = 1u << VIRTIO_CRYPTO_HASH_SHA1;
+backend->conf.akcipher_algo = 1u << VIRTIO_CRYPTO_AKCIPHER_RSA;
 /*
  * Set the Maximum length of crypto request.
  * Why this value? Just avoid to overflow when
  * memory allocation for each crypto request.
  */
-backend->conf.max_size = LONG_MAX - sizeof(CryptoDevBackendSymOpInfo);
+backend->conf.max_size = LONG_MAX - sizeof(CryptoDevBackendOpInfo);
 backend->conf.max_cipher_key_len = CRYPTODEV_BUITLIN_MAX_CIPHER_KEY_LEN;
 backend->conf.max_auth_key_len = CRYPTODEV_BUITLIN_MAX_AUTH_KEY_LEN;
 
@@ -148,6 +152,100 @@ err:
return -1;
 }
 
+static int cryptodev_builtin_get_rsa_hash_algo(
+int virtio_rsa_hash, Error **errp)
+{
+switch (virtio_rsa_hash) {
+case VIRTIO_CRYPTO_RSA_MD2:
+return QCRYPTO_RSA_HASH_ALG_MD2;
+
+case VIRTIO_CRYPTO_RSA_MD3:
+return QCRYPTO_RSA_HASH_ALG_MD3;
+
+case VIRTIO_CRYPTO_RSA_MD4:
+return QCRYPTO_RSA_HASH_ALG_MD4;
+
+case VIRTIO_CRYPTO_RSA_MD5:
+return QCRYPTO_RSA_HASH_ALG_MD5;
+
+case VIRTIO_CRYPTO_RSA_SHA1:
+return QCRYPTO_RSA_HASH_ALG_SHA1;
+
+case VIRTIO_CRYPTO_RSA_SHA256:
+return QCRYPTO_RSA_HASH_ALG_SHA256;
+
+case VIRTIO_CRYPTO_RSA_SHA384:
+return QCRYPTO_RSA_HASH_ALG_SHA384;
+
+case VIRTIO_CRYPTO_RSA_SHA512:
+return QCRYPTO_RSA_HASH_ALG_SHA512;
+
+case VIRTIO_CRYPTO_RSA_SHA224:
+return QCRYPTO_RSA_HASH_ALG_SHA224;
+
+default:
+error_setg(errp, "Unsupported rsa hash algo: %d", virtio_rsa_hash);
+return -1;
+}
+}
+
+static int cryptodev_builtin_set_rsa_options(
+int virtio_padding_algo,
+int virtio_hash_algo,
+QCryptoRsaOptions *opt,
+Error **errp)
+{
+if (virtio_padding_algo == VIRTIO_CRYPTO_RSA_PKCS1_PADDING) {
+opt->padding_algo = QCRYPTO_RSA_PADDING_ALG_PKCS1;
+opt->hash_algo =
+cryptodev_builtin_get_rsa_hash_algo(virtio_hash_algo, errp);
+if (opt->hash_algo < 0) {
+return -1;
+}
+return 0;
+}
+
+if (virtio_padding_algo == VIRTIO_CRYPTO_RSA_RAW_PADDING) {
+opt->padding_algo = QCRYPTO_RSA_PADDING_ALG_RAW;
+return 0;
+}
+
+error_setg(errp, "Unsupported rsa padding algo: %d", virtio_padding_algo);
+return 

[PATCH v3 5/6] tests/crypto: Add test suite for crypto akcipher

[zhenwei pi]

From: Lei He 

Add unit test and benchmark test for crypto akcipher.

Signed-off-by: lei he 
Signed-off-by: zhenwei pi 
---
 tests/bench/benchmark-crypto-akcipher.c | 163 ++
 tests/bench/meson.build |   6 +
 tests/bench/test_akcipher_keys.inc  | 277 +
 tests/unit/meson.build  |   1 +
 tests/unit/test-crypto-akcipher.c   | 715 
 5 files changed, 1162 insertions(+)
 create mode 100644 tests/bench/benchmark-crypto-akcipher.c
 create mode 100644 tests/bench/test_akcipher_keys.inc
 create mode 100644 tests/unit/test-crypto-akcipher.c

diff --git a/tests/bench/benchmark-crypto-akcipher.c 
b/tests/bench/benchmark-crypto-akcipher.c
new file mode 100644
index 00..152fed8d73
--- /dev/null
+++ b/tests/bench/benchmark-crypto-akcipher.c
@@ -0,0 +1,163 @@
+/*
+ * QEMU Crypto cipher speed benchmark
+ *
+ * Copyright (c) 2022 Bytedance
+ *
+ * Authors:
+ *lei he 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * (at your option) any later version.  See the COPYING file in the
+ * top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "crypto/init.h"
+#include "crypto/akcipher.h"
+#include "standard-headers/linux/virtio_crypto.h"
+
+#include "test_akcipher_keys.inc"
+
+static bool keep_running;
+
+static void alarm_handler(int sig)
+{
+keep_running = false;
+}
+
+static QCryptoAkcipher *create_rsa_akcipher(const uint8_t *priv_key,
+size_t keylen,
+QCryptoRsaPaddingAlgorithm padding,
+QCryptoRsaHashAlgorithm hash)
+{
+QCryptoRsaOptions opt;
+QCryptoAkcipher *rsa;
+Error *err = NULL;
+
+opt.padding_algo = padding;
+opt.hash_algo = hash;
+rsa = qcrypto_akcipher_new(QCRYPTO_AKCIPHER_ALG_RSA,
+   QCRYPTO_AKCIPHER_KEY_TYPE_PRIVATE,
+   priv_key, keylen, , );
+
+g_assert(rsa != NULL);
+return rsa;
+}
+
+static void test_rsa_speed(const uint8_t *priv_key, size_t keylen,
+   size_t key_size)
+{
+#define Byte 8
+#define SHA1_DGST_LEN 40
+#define DURATION_SECONDS 10
+#define padding QCRYPTO_RSA_PADDING_ALG_PKCS1
+#define hash QCRYPTO_RSA_HASH_ALG_SHA1
+
+Error *err = NULL;
+QCryptoAkcipher *rsa;
+uint8_t *dgst, *signature;
+size_t count;
+
+rsa = create_rsa_akcipher(priv_key, keylen, padding, hash);
+
+dgst = g_new0(uint8_t, SHA1_DGST_LEN);
+memset(dgst, g_test_rand_int(), SHA1_DGST_LEN);
+signature = g_new0(uint8_t, key_size / Byte);
+
+g_test_message("benchmark rsa%lu (%s-%s) sign in %d seconds", key_size,
+   QCryptoRsaPaddingAlgorithm_str(padding),
+   QCryptoRsaHashAlgorithm_str(hash),
+   DURATION_SECONDS);
+alarm(DURATION_SECONDS);
+g_test_timer_start();
+for (keep_running = true, count = 0; keep_running; ++count) {
+g_assert(qcrypto_akcipher_sign(rsa, dgst, SHA1_DGST_LEN,
+   signature, key_size / Byte, ) > 0);
+}
+g_test_timer_elapsed();
+g_test_message("rsa%lu (%s-%s) sign %lu times in %.2f seconds,"
+   " %.2f times/sec ",
+   key_size,  QCryptoRsaPaddingAlgorithm_str(padding),
+   QCryptoRsaHashAlgorithm_str(hash),
+   count, g_test_timer_last(),
+   (double)count / g_test_timer_last());
+
+g_test_message("benchmark rsa%lu (%s-%s) verify in %d seconds", key_size,
+   QCryptoRsaPaddingAlgorithm_str(padding),
+   QCryptoRsaHashAlgorithm_str(hash),
+   DURATION_SECONDS);
+alarm(DURATION_SECONDS);
+g_test_timer_start();
+for (keep_running = true, count = 0; keep_running; ++count) {
+g_assert(qcrypto_akcipher_verify(rsa, signature, key_size / Byte,
+ dgst, SHA1_DGST_LEN, ) == 0);
+}
+g_test_timer_elapsed();
+g_test_message("rsa%lu (%s-%s) verify %lu times in %.2f seconds,"
+   " %.2f times/sec ",
+   key_size, QCryptoRsaPaddingAlgorithm_str(padding),
+   QCryptoRsaHashAlgorithm_str(hash),
+   count, g_test_timer_last(),
+   (double)count / g_test_timer_last());
+
+g_assert(qcrypto_akcipher_free(rsa, ) == 0);
+g_free(dgst);
+g_free(signature);
+}
+
+static void test_rsa_1024_speed(const void *opaque)
+{
+size_t key_size = (size_t)opaque;
+test_rsa_speed(rsa1024_priv_key, sizeof(rsa1024_priv_key), key_size);
+}
+
+static void test_rsa_2048_speed(const void *opaque)
+{
+size_t key_size = (size_t)opaque;
+test_rsa_speed(rsa2048_priv_key, sizeof(rsa2048_priv_key), key_size);
+}
+
+static void test_rsa_4096_speed(const void *opaque)
+{
+size_t key_size = (size_t)opaque;
+test_rsa_speed(rsa4096_priv_key, 

[PATCH v3 4/6] crypto: Implement RSA algorithm by hogweed

[zhenwei pi]

From: Lei He 

Introduce ASN.1 decoder, and implement RSA algorithm by hogweed
from nettle. Thus QEMU supports a 'real' RSA backend to handle
request from guest side. It's important to test RSA offload case
without OS & hardware requirement.

Signed-off-by: lei he 
Signed-off-by: zhenwei pi 
---
 crypto/akcipher-nettle.c  | 523 ++
 crypto/akcipher.c |   3 +
 crypto/asn1_decoder.c | 185 ++
 crypto/asn1_decoder.h |  42 +++
 crypto/meson.build|   3 +
 include/crypto/akcipher.h |  16 ++
 meson.build   |  11 +
 7 files changed, 783 insertions(+)
 create mode 100644 crypto/akcipher-nettle.c
 create mode 100644 crypto/asn1_decoder.c
 create mode 100644 crypto/asn1_decoder.h

diff --git a/crypto/akcipher-nettle.c b/crypto/akcipher-nettle.c
new file mode 100644
index 00..45b93af772
--- /dev/null
+++ b/crypto/akcipher-nettle.c
@@ -0,0 +1,523 @@
+/*
+ * QEMU Crypto akcipher algorithms
+ *
+ * Copyright (c) 2022 Bytedance
+ * Author: lei he 
+ *
+ * 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.1 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 .
+ *
+ */
+
+#include 
+
+#include 
+
+#include "qemu/osdep.h"
+#include "qemu/host-utils.h"
+#include "asn1_decoder.h"
+#include "crypto/akcipher.h"
+#include "crypto/random.h"
+#include "qapi/error.h"
+#include "sysemu/cryptodev.h"
+
+typedef struct QCryptoNettleRsa {
+QCryptoAkcipher akcipher;
+struct rsa_public_key pub;
+struct rsa_private_key priv;
+QCryptoRsaPaddingAlgorithm padding_algo;
+QCryptoRsaHashAlgorithm hash_algo;
+} QCryptoNettleRsa;
+
+struct asn1_parse_ctx {
+const uint8_t *data;
+size_t dlen;
+};
+
+#define Octet 8
+
+static int extract_value(void *p, const uint8_t *data, size_t dlen)
+{
+struct asn1_parse_ctx *ctx = (struct asn1_parse_ctx *)p;
+ctx->data = (uint8_t *)data;
+ctx->dlen = dlen;
+
+return 0;
+}
+
+static int extract_mpi(void *p, const uint8_t *data, size_t dlen)
+{
+mpz_t *target = (mpz_t *)p;
+nettle_mpz_set_str_256_u(*target, dlen, data);
+
+return 0;
+}
+
+static QCryptoNettleRsa *qcrypto_nettle_rsa_malloc(void);
+
+static void qcrypto_nettle_rsa_destroy(void *ptr)
+{
+QCryptoNettleRsa *rsa = (QCryptoNettleRsa *)ptr;
+if (!rsa) {
+return;
+}
+
+rsa_public_key_clear(>pub);
+rsa_private_key_clear(>priv);
+g_free(rsa);
+}
+
+static QCryptoAkcipher *qcrypto_nettle_new_rsa(
+QCryptoAkcipherKeyType type,
+const uint8_t *key,  size_t keylen,
+QCryptoRsaOptions *opt, Error **errp);
+
+QCryptoAkcipher *qcrypto_akcipher_nettle_new(QCryptoAkcipherAlgorithm alg,
+ QCryptoAkcipherKeyType type,
+ const uint8_t *key,
+ size_t keylen, void *para,
+ Error **errp)
+{
+switch (alg) {
+case QCRYPTO_AKCIPHER_ALG_RSA:
+return qcrypto_nettle_new_rsa(type, key, keylen,
+  (QCryptoRsaOptions *)para, errp);
+default:
+error_setg(errp, "Unsupported algorithm: %u", alg);
+return NULL;
+}
+
+return NULL;
+}
+
+/**
+ * Parse ber encoded rsa private key, asn1 schema:
+ *RsaPrivKey ::= SEQUENCE {
+ * version INTEGER
+ * n   INTEGER
+ * e   INTEGER
+ * d   INTEGER
+ * p   INTEGER
+ * q   INTEGER
+ * e1  INTEGER
+ * e2  INTEGER
+ * u   INTEGER
+ * }
+ */
+static int parse_rsa_private_key(QCryptoNettleRsa *rsa,
+ const uint8_t *key, size_t keylen)
+{
+struct asn1_parse_ctx ctx;
+
+if (ber_decode_seq(, , extract_value, ) != 0 ||
+keylen != 0) {
+return -1;
+}
+
+if (ber_decode_int(, , NULL, NULL) != 0 ||
+ber_decode_int(, , extract_mpi, >pub.n) != 0 ||
+ber_decode_int(, , extract_mpi, >pub.e) != 0 ||
+ber_decode_int(, , extract_mpi, >priv.d) != 0 ||
+ber_decode_int(, , extract_mpi, >priv.p) != 0 ||
+ber_decode_int(, , extract_mpi, >priv.q) != 0 ||
+ber_decode_int(, , extract_mpi, >priv.a) != 0 ||
+ber_decode_int(, , extract_mpi, >priv.b) != 0 ||
+

[PATCH v3 3/6] crypto: Introduce akcipher crypto class

[zhenwei pi]

Support basic asymmetric operations: encrypt, decrypt, sign and
verify.

Co-developed-by: lei he 
Signed-off-by: lei he 
Signed-off-by: zhenwei pi 
---
 crypto/akcipher.c |  78 +
 crypto/meson.build|   1 +
 include/crypto/akcipher.h | 139 ++
 3 files changed, 218 insertions(+)
 create mode 100644 crypto/akcipher.c
 create mode 100644 include/crypto/akcipher.h

diff --git a/crypto/akcipher.c b/crypto/akcipher.c
new file mode 100644
index 00..1e52f2fd76
--- /dev/null
+++ b/crypto/akcipher.c
@@ -0,0 +1,78 @@
+/*
+ * QEMU Crypto akcipher algorithms
+ *
+ * Copyright (c) 2022 Bytedance
+ * Author: zhenwei pi 
+ *
+ * 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.1 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 .
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/host-utils.h"
+#include "qapi/error.h"
+#include "crypto/akcipher.h"
+
+QCryptoAkcipher *qcrypto_akcipher_new(QCryptoAkcipherAlgorithm alg,
+  QCryptoAkcipherKeyType type,
+  const uint8_t *key, size_t keylen,
+  void *para, Error **errp)
+{
+QCryptoAkcipher *akcipher = NULL;
+
+return akcipher;
+}
+
+int qcrypto_akcipher_encrypt(QCryptoAkcipher *akcipher,
+ const void *data, size_t data_len,
+ void *enc, size_t enc_len, Error **errp)
+{
+const QCryptoAkcipherDriver *drv = akcipher->driver;
+
+return drv->encrypt(akcipher, data, data_len, enc, enc_len, errp);
+}
+
+int qcrypto_akcipher_decrypt(struct QCryptoAkcipher *akcipher,
+ const void *enc, size_t enc_len,
+ void *data, size_t data_len, Error **errp)
+{
+const QCryptoAkcipherDriver *drv = akcipher->driver;
+
+return drv->decrypt(akcipher, enc, enc_len, data, data_len, errp);
+}
+
+int qcrypto_akcipher_sign(struct QCryptoAkcipher *akcipher,
+  const void *data, size_t data_len,
+  void *sig, size_t sig_len, Error **errp)
+{
+const QCryptoAkcipherDriver *drv = akcipher->driver;
+
+return drv->sign(akcipher, data, data_len, sig, sig_len, errp);
+}
+
+int qcrypto_akcipher_verify(struct QCryptoAkcipher *akcipher,
+const void *sig, size_t sig_len,
+const void *data, size_t data_len, Error **errp)
+{
+const QCryptoAkcipherDriver *drv = akcipher->driver;
+
+return drv->verify(akcipher, sig, sig_len, data, data_len, errp);
+}
+
+int qcrypto_akcipher_free(struct QCryptoAkcipher *akcipher, Error **errp)
+{
+const QCryptoAkcipherDriver *drv = akcipher->driver;
+
+return drv->free(akcipher, errp);
+}
diff --git a/crypto/meson.build b/crypto/meson.build
index 19c44bea89..c32b57aeda 100644
--- a/crypto/meson.build
+++ b/crypto/meson.build
@@ -19,6 +19,7 @@ crypto_ss.add(files(
   'tlscredspsk.c',
   'tlscredsx509.c',
   'tlssession.c',
+  'akcipher.c',
 ))
 
 if nettle.found()
diff --git a/include/crypto/akcipher.h b/include/crypto/akcipher.h
new file mode 100644
index 00..03cc3bf46b
--- /dev/null
+++ b/include/crypto/akcipher.h
@@ -0,0 +1,139 @@
+/*
+ * QEMU Crypto asymmetric algorithms
+ *
+ * Copyright (c) 2022 Bytedance
+ * Author: zhenwei pi 
+ *
+ * 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.1 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 .
+ *
+ */
+
+#ifndef QCRYPTO_AKCIPHER_H
+#define QCRYPTO_AKCIPHER_H
+
+#include "qemu/typedefs.h"
+#include "qapi/qapi-types-crypto.h"
+
+typedef struct QCryptoAkcipher QCryptoAkcipher;
+typedef struct QCryptoAkcipherDriver QCryptoAkcipherDriver;
+
+struct QCryptoAkcipherDriver {
+int (*encrypt)(struct QCryptoAkcipher *akcipher,
+   const void *data, size_t 

[PATCH v3 2/6] crypto-akcipher: Introduce akcipher types to qapi

[zhenwei pi]

From: Lei He 

Introduce akcipher types, also include RSA & ECDSA related types.

Signed-off-by: Lei He 
Signed-off-by: zhenwei pi 
---
 qapi/crypto.json | 86 
 1 file changed, 86 insertions(+)

diff --git a/qapi/crypto.json b/qapi/crypto.json
index 1ec54c15ca..d44c38e3b1 100644
--- a/qapi/crypto.json
+++ b/qapi/crypto.json
@@ -540,3 +540,89 @@
   'data': { '*loaded': { 'type': 'bool', 'features': ['deprecated'] },
 '*sanity-check': 'bool',
 '*passwordid': 'str' } }
+##
+# @QCryptoAkcipherAlgorithm:
+#
+# The supported algorithms for asymmetric encryption ciphers
+#
+# @rsa: RSA algorithm
+# @ecdsa: ECDSA algorithm
+#
+# Since: 7.0
+##
+{ 'enum': 'QCryptoAkcipherAlgorithm',
+  'prefix': 'QCRYPTO_AKCIPHER_ALG',
+  'data': ['rsa', 'ecdsa']}
+
+##
+# @QCryptoAkcipherKeyType:
+#
+# The type of asymmetric keys.
+#
+# Since: 7.0
+##
+{ 'enum': 'QCryptoAkcipherKeyType',
+  'prefix': 'QCRYPTO_AKCIPHER_KEY_TYPE',
+  'data': ['public', 'private']}
+
+##
+# @QCryptoRsaHashAlgorithm:
+#
+# The hash algorithm for RSA pkcs1 padding algothrim
+#
+# Since: 7.0
+##
+{ 'enum': 'QCryptoRsaHashAlgorithm',
+  'prefix': 'QCRYPTO_RSA_HASH_ALG',
+  'data': [ 'md2', 'md3', 'md4', 'md5', 'sha1', 'sha256', 'sha384', 'sha512', 
'sha224' ]}
+
+##
+# @QCryptoRsaPaddingAlgorithm:
+#
+# The padding algorithm for RSA.
+#
+# @raw: no padding used
+# @pkcs1: pkcs1#v1.5
+#
+# Since: 7.0
+##
+{ 'enum': 'QCryptoRsaPaddingAlgorithm',
+  'prefix': 'QCRYPTO_RSA_PADDING_ALG',
+  'data': ['raw', 'pkcs1']}
+
+##
+# @QCryptoCurveId:
+#
+# The well-known curves, referenced from 
https://csrc.nist.gov/csrc/media/publications/fips/186/3/archive/2009-06-25/documents/fips_186-3.pdf
+#
+# Since: 7.0
+##
+{ 'enum': 'QCryptoCurveId',
+  'prefix': 'QCRYPTO_CURVE_ID',
+  'data': ['nist-p192', 'nist-p224', 'nist-p256', 'nist-p384', 'nist-p521']}
+
+##
+# @QCryptoRsaOptions:
+#
+# Specific parameters for RSA algorithm.
+#
+# @hash-algo: QCryptoRsaHashAlgorithm
+# @padding-algo: QCryptoRsaPaddingAlgorithm
+#
+# Since: 7.0
+##
+{ 'struct': 'QCryptoRsaOptions',
+  'data': { 'hash-algo':'QCryptoRsaHashAlgorithm',
+'padding-algo': 'QCryptoRsaPaddingAlgorithm'}}
+
+##
+# @QCryptoEcdsaOptions:
+#
+# Specific parameter for ECDSA algorithm.
+#
+# @curve-id: QCryptoCurveId
+#
+# Since: 7.0
+##
+{ 'struct': 'QCryptoEcdsaOptions',
+  'data': { 'curve-id': 'QCryptoCurveId' }}
-- 
2.25.1

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH v3 1/6] virtio-crypto: header update

[zhenwei pi]

Update header from linux, support akcipher service.

Reviewed-by: Gonglei 
Signed-off-by: lei he 
Signed-off-by: zhenwei pi 
---
 .../standard-headers/linux/virtio_crypto.h| 82 ++-
 1 file changed, 81 insertions(+), 1 deletion(-)

diff --git a/include/standard-headers/linux/virtio_crypto.h 
b/include/standard-headers/linux/virtio_crypto.h
index 5ff0b4ee59..68066dafb6 100644
--- a/include/standard-headers/linux/virtio_crypto.h
+++ b/include/standard-headers/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)
uint32_t opcode;
uint32_t algo;
uint32_t flag;
@@ -180,6 +185,58 @@ struct virtio_crypto_aead_create_session_req {
uint8_t padding[32];
 };
 
+struct virtio_crypto_rsa_session_para {
+#define VIRTIO_CRYPTO_RSA_RAW_PADDING   0
+#define VIRTIO_CRYPTO_RSA_PKCS1_PADDING 1
+   uint32_t 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
+   uint32_t 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
+   uint32_t curve_id;
+   uint32_t 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
+   uint32_t algo;
+
+#define VIRTIO_CRYPTO_AKCIPHER_KEY_TYPE_PUBLIC  1
+#define VIRTIO_CRYPTO_AKCIPHER_KEY_TYPE_PRIVATE 2
+   uint32_t keytype;
+   uint32_t 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;
+   uint8_t 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;
uint8_t 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)
uint32_t opcode;
/* algo should be service-specific algorithms */
uint32_t algo;
@@ -390,6 +457,16 @@ struct virtio_crypto_aead_data_req {
uint8_t padding[32];
 };
 
+struct virtio_crypto_akcipher_para {
+   uint32_t src_data_len;
+   uint32_t dst_data_len;
+};
+
+struct virtio_crypto_akcipher_data_req {
+   struct virtio_crypto_akcipher_para para;
+   uint8_t padding[40];
+};
+
 /* The request of the data virtqueue's packet */
 struct virtio_crypto_op_data_req {
struct virtio_crypto_op_header header;
@@ -399,6 +476,7 @@ struct virtio_crypto_op_data_req {
struct virtio_crypto_hash_data_req hash_req;

[PATCH v3 0/6] Support akcipher for virtio-crypto

[zhenwei pi]

v2 -> v3:
- Introduce akcipher types to qapi
- Add test/benchmark suite for akcipher class
- Seperate 'virtio_crypto: Support virtio crypto asym operation' into:
  - crypto: Introduce akcipher crypto class
  - virtio-crypto: Introduce RSA algorithm

v1 -> v2:
- Update virtio_crypto.h from v2 version of related kernel patch.

v1:
- Support akcipher for virtio-crypto.
- Introduce akcipher class.
- Introduce ASN1 decoder into QEMU.
- Implement RSA backend by nettle/hogweed.

Lei He (3):
  crypto-akcipher: Introduce akcipher types to qapi
  crypto: Implement RSA algorithm by hogweed
  tests/crypto: Add test suite for crypto akcipher

Zhenwei Pi (3):
  virtio-crypto: header update
  crypto: Introduce akcipher crypto class
  virtio-crypto: Introduce RSA algorithm

 backends/cryptodev-builtin.c  | 319 +++-
 backends/cryptodev-vhost-user.c   |  34 +-
 backends/cryptodev.c  |  32 +-
 crypto/akcipher-nettle.c  | 523 +
 crypto/akcipher.c |  81 ++
 crypto/asn1_decoder.c | 185 +
 crypto/asn1_decoder.h |  42 +
 crypto/meson.build|   4 +
 hw/virtio/virtio-crypto.c | 326 ++--
 include/crypto/akcipher.h | 155 
 include/hw/virtio/virtio-crypto.h |   5 +-
 .../standard-headers/linux/virtio_crypto.h|  82 +-
 include/sysemu/cryptodev.h|  88 ++-
 meson.build   |  11 +
 qapi/crypto.json  |  86 +++
 tests/bench/benchmark-crypto-akcipher.c   | 163 
 tests/bench/meson.build   |   6 +
 tests/bench/test_akcipher_keys.inc| 277 +++
 tests/unit/meson.build|   1 +
 tests/unit/test-crypto-akcipher.c | 715 ++
 20 files changed, 2990 insertions(+), 145 deletions(-)
 create mode 100644 crypto/akcipher-nettle.c
 create mode 100644 crypto/akcipher.c
 create mode 100644 crypto/asn1_decoder.c
 create mode 100644 crypto/asn1_decoder.h
 create mode 100644 include/crypto/akcipher.h
 create mode 100644 tests/bench/benchmark-crypto-akcipher.c
 create mode 100644 tests/bench/test_akcipher_keys.inc
 create mode 100644 tests/unit/test-crypto-akcipher.c

-- 
2.25.1

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 2/5] mm: page_isolation: check specified range for unmovable pages

[David Hildenbrand]

On 21.03.22 19:23, Zi Yan wrote:
> On 21 Mar 2022, at 13:30, David Hildenbrand wrote:
> 
>> On 17.03.22 16:37, Zi Yan wrote:
>>> From: Zi Yan 
>>>
>>> Enable set_migratetype_isolate() to check specified sub-range for
>>> unmovable pages during isolation. Page isolation is done
>>> at max(MAX_ORDER_NR_PAEGS, pageblock_nr_pages) granularity, but not all
>>> pages within that granularity are intended to be isolated. For example,
>>> alloc_contig_range(), which uses page isolation, allows ranges without
>>> alignment. This commit makes unmovable page check only look for
>>> interesting pages, so that page isolation can succeed for any
>>> non-overlapping ranges.
>>>
>>> Signed-off-by: Zi Yan 
>>> ---
>>>  include/linux/page-isolation.h | 10 +
>>>  mm/page_alloc.c| 13 +--
>>>  mm/page_isolation.c| 69 --
>>>  3 files changed, 51 insertions(+), 41 deletions(-)
>>>
>>> diff --git a/include/linux/page-isolation.h b/include/linux/page-isolation.h
>>> index e14eddf6741a..eb4a208fe907 100644
>>> --- a/include/linux/page-isolation.h
>>> +++ b/include/linux/page-isolation.h
>>> @@ -15,6 +15,16 @@ static inline bool is_migrate_isolate(int migratetype)
>>>  {
>>> return migratetype == MIGRATE_ISOLATE;
>>>  }
>>> +static inline unsigned long pfn_max_align_down(unsigned long pfn)
>>> +{
>>> +   return ALIGN_DOWN(pfn, MAX_ORDER_NR_PAGES);
>>> +}
>>> +
>>> +static inline unsigned long pfn_max_align_up(unsigned long pfn)
>>> +{
>>> +   return ALIGN(pfn, MAX_ORDER_NR_PAGES);
>>> +}
>>> +
>>>  #else
>>>  static inline bool has_isolate_pageblock(struct zone *zone)
>>>  {
>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>>> index 6de57d058d3d..680580a40a35 100644
>>> --- a/mm/page_alloc.c
>>> +++ b/mm/page_alloc.c
>>> @@ -8937,16 +8937,6 @@ void *__init alloc_large_system_hash(const char 
>>> *tablename,
>>>  }
>>>
>>>  #ifdef CONFIG_CONTIG_ALLOC
>>> -static unsigned long pfn_max_align_down(unsigned long pfn)
>>> -{
>>> -   return ALIGN_DOWN(pfn, MAX_ORDER_NR_PAGES);
>>> -}
>>> -
>>> -static unsigned long pfn_max_align_up(unsigned long pfn)
>>> -{
>>> -   return ALIGN(pfn, MAX_ORDER_NR_PAGES);
>>> -}
>>> -
>>>  #if defined(CONFIG_DYNAMIC_DEBUG) || \
>>> (defined(CONFIG_DYNAMIC_DEBUG_CORE) && defined(DYNAMIC_DEBUG_MODULE))
>>>  /* Usage: See admin-guide/dynamic-debug-howto.rst */
>>> @@ -9091,8 +9081,7 @@ int alloc_contig_range(unsigned long start, unsigned 
>>> long end,
>>>  * put back to page allocator so that buddy can use them.
>>>  */
>>>
>>> -   ret = start_isolate_page_range(pfn_max_align_down(start),
>>> -  pfn_max_align_up(end), migratetype, 0);
>>> +   ret = start_isolate_page_range(start, end, migratetype, 0);
>>> if (ret)
>>> return ret;
>>
>> Shouldn't we similarly adjust undo_isolate_page_range()? IOW, all users
>> of pfn_max_align_down()/pfn_max_align_up(). would be gone from that file
>> and you can move these defines into mm/page_isolation.c instead of
>> include/linux/page-isolation.h?
> 
> undo_isolate_page_range() faces much simpler situation, just needing
> to unset migratetype. We can just pass pageblock_nr_pages aligned range
> to it. For start_isolate_page_range(), start and end are also used for
> has_unmovable_pages() for precise unmovable page identification, so
> they cannot be pageblock_nr_pages aligned. But for readability and symmetry,
> yes, I can change undo_isolate_page_range() too.
Yeah, we should call both with the same range and any extension of the
range should be handled internally.

I thought about some corner cases, especially once we relax some (CMA)
alignment thingies -- then, the CMA area might be placed at weird
locations. I haven't checked to which degree they apply, but we should
certainly keep them in mind whenever we're extending the isolation range.

We can assume that the contig range we're allocation
a) Belongs to a single zone
b) Does not contain any memory holes / mmap holes

Let's double check


1) Different zones in extended range

...   ZONE A  ][ ZONE B 
[ Pageblock X ][ Pageblock Y ][ Pageblock Z ]
[MAX_ORDER - 1   ]

We can never create a higher-order page between X and Y, because they
are in different zones. Most probably we should *not* extend the range
to cover pageblock X in case the zones don't match.

The same consideration applies to the end of the range, when extending
the isolation range.

But I wonder if we can have such a zone layout. At least
mm/page_alloc.c:find_zone_movable_pfns_for_nodes() makes sure to always
align the start of ZONE_MOVABLE to MAX_ORDER_NR_PAGES. I hope it applies
to all other zones as well? :/

Anyhow, it should be easy to check when isolating/un-isolating. Only
conditionally extend the range if the zones of both pageblocks match.


When eventually growing MAX_ORDER_NR_PAGES further, could we be in
trouble because we could suddenly span multiple zones with a single
MAX_ORDER - 1 page? Then 

Re: [PATCH net] vsock/virtio: enable VQs early on probe

[Stefano Garzarella]

On Tue, Mar 22, 2022 at 10:09:06AM -0400, Michael S. Tsirkin wrote:

On Tue, Mar 22, 2022 at 03:05:00PM +0100, Stefano Garzarella wrote:

On Tue, Mar 22, 2022 at 09:36:14AM -0400, Michael S. Tsirkin wrote:
> On Tue, Mar 22, 2022 at 11:38:23AM +0100, Stefano Garzarella wrote:
> > virtio spec requires drivers to set DRIVER_OK before using VQs.
> > This is set automatically after probe returns, but virtio-vsock
> > driver uses VQs in the probe function to fill rx and event VQs
> > with new buffers.
>
>
> So this is a spec violation. absolutely.
>
> > Let's fix this, calling virtio_device_ready() before using VQs
> > in the probe function.
> >
> > Fixes: 0ea9e1d3a9e3 ("VSOCK: Introduce virtio_transport.ko")
> > Signed-off-by: Stefano Garzarella 
> > ---
> >  net/vmw_vsock/virtio_transport.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/net/vmw_vsock/virtio_transport.c 
b/net/vmw_vsock/virtio_transport.c
> > index 5afc194a58bb..b1962f8cd502 100644
> > --- a/net/vmw_vsock/virtio_transport.c
> > +++ b/net/vmw_vsock/virtio_transport.c
> > @@ -622,6 +622,8 @@ static int virtio_vsock_probe(struct virtio_device 
*vdev)
> >   INIT_WORK(>event_work, virtio_transport_event_work);
> >   INIT_WORK(>send_pkt_work, virtio_transport_send_pkt_work);
> >
> > + virtio_device_ready(vdev);
> > +
> >   mutex_lock(>tx_lock);
> >   vsock->tx_run = true;
> >   mutex_unlock(>tx_lock);
>
> Here's the whole code snippet:
>
>
>mutex_lock(>tx_lock);
>vsock->tx_run = true;
>mutex_unlock(>tx_lock);
>
>mutex_lock(>rx_lock);
>virtio_vsock_rx_fill(vsock);
>vsock->rx_run = true;
>mutex_unlock(>rx_lock);
>
>mutex_lock(>event_lock);
>virtio_vsock_event_fill(vsock);
>vsock->event_run = true;
>mutex_unlock(>event_lock);
>
>if (virtio_has_feature(vdev, VIRTIO_VSOCK_F_SEQPACKET))
>vsock->seqpacket_allow = true;
>
>vdev->priv = vsock;
>rcu_assign_pointer(the_virtio_vsock, vsock);
>
>mutex_unlock(_virtio_vsock_mutex);
>
>
> I worry that this is not the only problem here:
> seqpacket_allow and setting of vdev->priv at least after
> device is active look suspicious.

Right, so if you agree I'll move these before virtio_device_ready().

> E.g.:
>
> static void virtio_vsock_event_done(struct virtqueue *vq)
> {
>struct virtio_vsock *vsock = vq->vdev->priv;
>
>if (!vsock)
>return;
>queue_work(virtio_vsock_workqueue, >event_work);
> }
>
> looks like it will miss events now they will be reported earlier.
> One might say that since vq has been kicked it might send
> interrupts earlier too so not a new problem, but
> there's a chance device actually waits until DRIVER_OK
> to start operating.

Yes I see, should I break into 2 patches (one where I move the code already
present and this one)?

Maybe a single patch is fine since it's the complete solution.

Thank you for the detailed explanation,
Stefano


Two I think since movement can be backported to before the hardening
effort.


Yep, maybe 3 since seqpacket was added later.

Thanks,
Stefano

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net] vsock/virtio: enable VQs early on probe

[Michael S. Tsirkin]

On Tue, Mar 22, 2022 at 03:05:00PM +0100, Stefano Garzarella wrote:
> On Tue, Mar 22, 2022 at 09:36:14AM -0400, Michael S. Tsirkin wrote:
> > On Tue, Mar 22, 2022 at 11:38:23AM +0100, Stefano Garzarella wrote:
> > > virtio spec requires drivers to set DRIVER_OK before using VQs.
> > > This is set automatically after probe returns, but virtio-vsock
> > > driver uses VQs in the probe function to fill rx and event VQs
> > > with new buffers.
> > 
> > 
> > So this is a spec violation. absolutely.
> > 
> > > Let's fix this, calling virtio_device_ready() before using VQs
> > > in the probe function.
> > > 
> > > Fixes: 0ea9e1d3a9e3 ("VSOCK: Introduce virtio_transport.ko")
> > > Signed-off-by: Stefano Garzarella 
> > > ---
> > >  net/vmw_vsock/virtio_transport.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/net/vmw_vsock/virtio_transport.c 
> > > b/net/vmw_vsock/virtio_transport.c
> > > index 5afc194a58bb..b1962f8cd502 100644
> > > --- a/net/vmw_vsock/virtio_transport.c
> > > +++ b/net/vmw_vsock/virtio_transport.c
> > > @@ -622,6 +622,8 @@ static int virtio_vsock_probe(struct virtio_device 
> > > *vdev)
> > >   INIT_WORK(>event_work, virtio_transport_event_work);
> > >   INIT_WORK(>send_pkt_work, virtio_transport_send_pkt_work);
> > > 
> > > + virtio_device_ready(vdev);
> > > +
> > >   mutex_lock(>tx_lock);
> > >   vsock->tx_run = true;
> > >   mutex_unlock(>tx_lock);
> > 
> > Here's the whole code snippet:
> > 
> > 
> >mutex_lock(>tx_lock);
> >vsock->tx_run = true;
> >mutex_unlock(>tx_lock);
> > 
> >mutex_lock(>rx_lock);
> >virtio_vsock_rx_fill(vsock);
> >vsock->rx_run = true;
> >mutex_unlock(>rx_lock);
> > 
> >mutex_lock(>event_lock);
> >virtio_vsock_event_fill(vsock);
> >vsock->event_run = true;
> >mutex_unlock(>event_lock);
> > 
> >if (virtio_has_feature(vdev, VIRTIO_VSOCK_F_SEQPACKET))
> >vsock->seqpacket_allow = true;
> > 
> >vdev->priv = vsock;
> >rcu_assign_pointer(the_virtio_vsock, vsock);
> > 
> >mutex_unlock(_virtio_vsock_mutex);
> > 
> > 
> > I worry that this is not the only problem here:
> > seqpacket_allow and setting of vdev->priv at least after
> > device is active look suspicious.
> 
> Right, so if you agree I'll move these before virtio_device_ready().
> 
> > E.g.:
> > 
> > static void virtio_vsock_event_done(struct virtqueue *vq)
> > {
> >struct virtio_vsock *vsock = vq->vdev->priv;
> > 
> >if (!vsock)
> >return;
> >queue_work(virtio_vsock_workqueue, >event_work);
> > }
> > 
> > looks like it will miss events now they will be reported earlier.
> > One might say that since vq has been kicked it might send
> > interrupts earlier too so not a new problem, but
> > there's a chance device actually waits until DRIVER_OK
> > to start operating.
> 
> Yes I see, should I break into 2 patches (one where I move the code already
> present and this one)?
> 
> Maybe a single patch is fine since it's the complete solution.
> 
> Thank you for the detailed explanation,
> Stefano

Two I think since movement can be backported to before the hardening
effort.

-- 
MST

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] virtio: use virtio_device_ready() in virtio_device_restore()

[Michael S. Tsirkin]

On Tue, Mar 22, 2022 at 12:43:13PM +0100, Stefano Garzarella wrote:
> After waking up a suspended VM, the kernel prints the following trace
> for virtio drivers which do not directly call virtio_device_ready() in
> the .restore:
> 
> PM: suspend exit
> irq 22: nobody cared (try booting with the "irqpoll" option)
> Call Trace:
>  
>  dump_stack_lvl+0x38/0x49
>  dump_stack+0x10/0x12
>  __report_bad_irq+0x3a/0xaf
>  note_interrupt.cold+0xb/0x60
>  handle_irq_event+0x71/0x80
>  handle_fasteoi_irq+0x95/0x1e0
>  __common_interrupt+0x6b/0x110
>  common_interrupt+0x63/0xe0
>  asm_common_interrupt+0x1e/0x40
>  ? __do_softirq+0x75/0x2f3
>  irq_exit_rcu+0x93/0xe0
>  sysvec_apic_timer_interrupt+0xac/0xd0
>  
>  
>  asm_sysvec_apic_timer_interrupt+0x12/0x20
>  arch_cpu_idle+0x12/0x20
>  default_idle_call+0x39/0xf0
>  do_idle+0x1b5/0x210
>  cpu_startup_entry+0x20/0x30
>  start_secondary+0xf3/0x100
>  secondary_startup_64_no_verify+0xc3/0xcb
>  
> handlers:
> [<8f9bac49>] vp_interrupt
> [<8f9bac49>] vp_interrupt
> Disabling IRQ #22
> 
> This happens because we don't invoke .enable_cbs callback in
> virtio_device_restore(). That callback is used by some transports
> (e.g. virtio-pci) to enable interrupts.
> 
> Let's fix it, by calling virtio_device_ready() as we do in
> virtio_dev_probe(). This function calls .enable_cts callback and sets
> DRIVER_OK status bit.
> 
> This fix also avoids setting DRIVER_OK twice for those drivers that
> call virtio_device_ready() in the .restore.
> 
> Fixes: d50497eb4e55 ("virtio_config: introduce a new .enable_cbs method")
> Signed-off-by: Stefano Garzarella 
> ---
> 
> I'm not sure about the fixes tag. That one is more generic, but the
> following one I think introduced the issue.
> 
> Fixes: 9e35276a5344 ("virtio_pci: harden MSI-X interrupts")

Jason what should we do about this one BTW? Just revert? We have other
issues ...


> Thanks,
> Stefano
> ---
>  drivers/virtio/virtio.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index 22f15f444f75..75c8d560bbd3 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -526,8 +526,9 @@ int virtio_device_restore(struct virtio_device *dev)
>   goto err;
>   }
>  
> - /* Finally, tell the device we're all set */
> - virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
> + /* If restore didn't do it, mark device DRIVER_OK ourselves. */
> + if (!(dev->config->get_status(dev) & VIRTIO_CONFIG_S_DRIVER_OK))
> + virtio_device_ready(dev);
>  
>   virtio_config_enable(dev);
>  
> -- 
> 2.35.1

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net] vsock/virtio: enable VQs early on probe

[Stefano Garzarella]

On Tue, Mar 22, 2022 at 09:36:14AM -0400, Michael S. Tsirkin wrote:

On Tue, Mar 22, 2022 at 11:38:23AM +0100, Stefano Garzarella wrote:

virtio spec requires drivers to set DRIVER_OK before using VQs.
This is set automatically after probe returns, but virtio-vsock
driver uses VQs in the probe function to fill rx and event VQs
with new buffers.



So this is a spec violation. absolutely.


Let's fix this, calling virtio_device_ready() before using VQs
in the probe function.

Fixes: 0ea9e1d3a9e3 ("VSOCK: Introduce virtio_transport.ko")
Signed-off-by: Stefano Garzarella 
---
 net/vmw_vsock/virtio_transport.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
index 5afc194a58bb..b1962f8cd502 100644
--- a/net/vmw_vsock/virtio_transport.c
+++ b/net/vmw_vsock/virtio_transport.c
@@ -622,6 +622,8 @@ static int virtio_vsock_probe(struct virtio_device *vdev)
INIT_WORK(>event_work, virtio_transport_event_work);
INIT_WORK(>send_pkt_work, virtio_transport_send_pkt_work);

+   virtio_device_ready(vdev);
+
mutex_lock(>tx_lock);
vsock->tx_run = true;
mutex_unlock(>tx_lock);


Here's the whole code snippet:


   mutex_lock(>tx_lock);
   vsock->tx_run = true;
   mutex_unlock(>tx_lock);

   mutex_lock(>rx_lock);
   virtio_vsock_rx_fill(vsock);
   vsock->rx_run = true;
   mutex_unlock(>rx_lock);

   mutex_lock(>event_lock);
   virtio_vsock_event_fill(vsock);
   vsock->event_run = true;
   mutex_unlock(>event_lock);

   if (virtio_has_feature(vdev, VIRTIO_VSOCK_F_SEQPACKET))
   vsock->seqpacket_allow = true;

   vdev->priv = vsock;
   rcu_assign_pointer(the_virtio_vsock, vsock);

   mutex_unlock(_virtio_vsock_mutex);


I worry that this is not the only problem here:
seqpacket_allow and setting of vdev->priv at least after
device is active look suspicious.


Right, so if you agree I'll move these before virtio_device_ready().


E.g.:

static void virtio_vsock_event_done(struct virtqueue *vq)
{
   struct virtio_vsock *vsock = vq->vdev->priv;

   if (!vsock)
   return;
   queue_work(virtio_vsock_workqueue, >event_work);
}

looks like it will miss events now they will be reported earlier.
One might say that since vq has been kicked it might send
interrupts earlier too so not a new problem, but
there's a chance device actually waits until DRIVER_OK
to start operating.


Yes I see, should I break into 2 patches (one where I move the code 
already present and this one)?


Maybe a single patch is fine since it's the complete solution.

Thank you for the detailed explanation,
Stefano

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net] vsock/virtio: enable VQs early on probe

[Michael S. Tsirkin]

On Tue, Mar 22, 2022 at 11:38:23AM +0100, Stefano Garzarella wrote:
> virtio spec requires drivers to set DRIVER_OK before using VQs.
> This is set automatically after probe returns, but virtio-vsock
> driver uses VQs in the probe function to fill rx and event VQs
> with new buffers.


So this is a spec violation. absolutely.

> Let's fix this, calling virtio_device_ready() before using VQs
> in the probe function.
> 
> Fixes: 0ea9e1d3a9e3 ("VSOCK: Introduce virtio_transport.ko")
> Signed-off-by: Stefano Garzarella 
> ---
>  net/vmw_vsock/virtio_transport.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/vmw_vsock/virtio_transport.c 
> b/net/vmw_vsock/virtio_transport.c
> index 5afc194a58bb..b1962f8cd502 100644
> --- a/net/vmw_vsock/virtio_transport.c
> +++ b/net/vmw_vsock/virtio_transport.c
> @@ -622,6 +622,8 @@ static int virtio_vsock_probe(struct virtio_device *vdev)
>   INIT_WORK(>event_work, virtio_transport_event_work);
>   INIT_WORK(>send_pkt_work, virtio_transport_send_pkt_work);
>  
> + virtio_device_ready(vdev);
> +
>   mutex_lock(>tx_lock);
>   vsock->tx_run = true;
>   mutex_unlock(>tx_lock);

Here's the whole code snippet:


mutex_lock(>tx_lock);
vsock->tx_run = true;
mutex_unlock(>tx_lock);

mutex_lock(>rx_lock);
virtio_vsock_rx_fill(vsock);
vsock->rx_run = true;
mutex_unlock(>rx_lock);

mutex_lock(>event_lock);
virtio_vsock_event_fill(vsock);
vsock->event_run = true;
mutex_unlock(>event_lock);

if (virtio_has_feature(vdev, VIRTIO_VSOCK_F_SEQPACKET))
vsock->seqpacket_allow = true;

vdev->priv = vsock;
rcu_assign_pointer(the_virtio_vsock, vsock);

mutex_unlock(_virtio_vsock_mutex);


I worry that this is not the only problem here:
seqpacket_allow and setting of vdev->priv at least after
device is active look suspicious.
E.g.:

static void virtio_vsock_event_done(struct virtqueue *vq)
{
struct virtio_vsock *vsock = vq->vdev->priv;

if (!vsock)
return;
queue_work(virtio_vsock_workqueue, >event_work);
}

looks like it will miss events now they will be reported earlier.
One might say that since vq has been kicked it might send
interrupts earlier too so not a new problem, but
there's a chance device actually waits until DRIVER_OK
to start operating.


> -- 
> 2.35.1

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] virtio: pci: Sanity check the bar index in find_shm_cap

[Michael S. Tsirkin]

On Tue, Mar 22, 2022 at 09:20:27AM +, Keir Fraser wrote:
> On Tue, Mar 22, 2022 at 11:36:23AM +0800, Jason Wang wrote:
> > On Mon, Mar 21, 2022 at 11:49 PM Keir Fraser  wrote:
> > >
> > > The bar index is used as an index into the device's resource list
> > > and should be checked as within range for a standard bar.
> > >
> > > Signed-off-by: Keir Fraser 
> > 
> > Patch looks good, do we need a similar check in vp_modern_map_capability()?
> 
> Yes it looks like we do. Would you like me to fix that also? If so: separate 
> patch, or
> revised version of this patch?

Yes pls. A fixed version is better, less work for everyone.

> > Thanks
> > 
> > > ---
> > >  drivers/virtio/virtio_pci_modern.c | 10 --
> > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/virtio/virtio_pci_modern.c 
> > > b/drivers/virtio/virtio_pci_modern.c
> > > index 5455bc041fb6..84bace98dff5 100644
> > > --- a/drivers/virtio/virtio_pci_modern.c
> > > +++ b/drivers/virtio/virtio_pci_modern.c
> > > @@ -293,7 +293,7 @@ static int virtio_pci_find_shm_cap(struct pci_dev 
> > > *dev, u8 required_id,
> > >
> > > for (pos = pci_find_capability(dev, PCI_CAP_ID_VNDR); pos > 0;
> > >  pos = pci_find_next_capability(dev, pos, PCI_CAP_ID_VNDR)) {
> > > -   u8 type, cap_len, id;
> > > +   u8 type, cap_len, id, res_bar;
> > > u32 tmp32;
> > > u64 res_offset, res_length;
> > >
> > > @@ -317,7 +317,12 @@ static int virtio_pci_find_shm_cap(struct pci_dev 
> > > *dev, u8 required_id,
> > >
> > > /* Type, and ID match, looks good */
> > > pci_read_config_byte(dev, pos + offsetof(struct 
> > > virtio_pci_cap,
> > > -bar), bar);
> > > +bar), _bar);
> > > +   if (res_bar >= PCI_STD_NUM_BARS) {
> > > +   dev_err(>dev, "%s: shm cap with bad bar: 
> > > %d\n",
> > > +   __func__, res_bar);
> > > +   continue;
> > > +   }
> > >
> > > /* Read the lower 32bit of length and offset */
> > > pci_read_config_dword(dev, pos + offsetof(struct 
> > > virtio_pci_cap,
> > > @@ -337,6 +342,7 @@ static int virtio_pci_find_shm_cap(struct pci_dev 
> > > *dev, u8 required_id,
> > >  length_hi), );
> > > res_length |= ((u64)tmp32) << 32;
> > >
> > > +   *bar = res_bar;
> > > *offset = res_offset;
> > > *len = res_length;
> > >
> > > --
> > > 2.35.1.894.gb6a874cedc-goog
> > >
> > 

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH] virtio: use virtio_device_ready() in virtio_device_restore()

[Stefano Garzarella]

After waking up a suspended VM, the kernel prints the following trace
for virtio drivers which do not directly call virtio_device_ready() in
the .restore:

PM: suspend exit
irq 22: nobody cared (try booting with the "irqpoll" option)
Call Trace:
 
 dump_stack_lvl+0x38/0x49
 dump_stack+0x10/0x12
 __report_bad_irq+0x3a/0xaf
 note_interrupt.cold+0xb/0x60
 handle_irq_event+0x71/0x80
 handle_fasteoi_irq+0x95/0x1e0
 __common_interrupt+0x6b/0x110
 common_interrupt+0x63/0xe0
 asm_common_interrupt+0x1e/0x40
 ? __do_softirq+0x75/0x2f3
 irq_exit_rcu+0x93/0xe0
 sysvec_apic_timer_interrupt+0xac/0xd0
 
 
 asm_sysvec_apic_timer_interrupt+0x12/0x20
 arch_cpu_idle+0x12/0x20
 default_idle_call+0x39/0xf0
 do_idle+0x1b5/0x210
 cpu_startup_entry+0x20/0x30
 start_secondary+0xf3/0x100
 secondary_startup_64_no_verify+0xc3/0xcb
 
handlers:
[<8f9bac49>] vp_interrupt
[<8f9bac49>] vp_interrupt
Disabling IRQ #22

This happens because we don't invoke .enable_cbs callback in
virtio_device_restore(). That callback is used by some transports
(e.g. virtio-pci) to enable interrupts.

Let's fix it, by calling virtio_device_ready() as we do in
virtio_dev_probe(). This function calls .enable_cts callback and sets
DRIVER_OK status bit.

This fix also avoids setting DRIVER_OK twice for those drivers that
call virtio_device_ready() in the .restore.

Fixes: d50497eb4e55 ("virtio_config: introduce a new .enable_cbs method")
Signed-off-by: Stefano Garzarella 
---

I'm not sure about the fixes tag. That one is more generic, but the
following one I think introduced the issue.

Fixes: 9e35276a5344 ("virtio_pci: harden MSI-X interrupts")

Thanks,
Stefano
---
 drivers/virtio/virtio.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 22f15f444f75..75c8d560bbd3 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -526,8 +526,9 @@ int virtio_device_restore(struct virtio_device *dev)
goto err;
}
 
-   /* Finally, tell the device we're all set */
-   virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
+   /* If restore didn't do it, mark device DRIVER_OK ourselves. */
+   if (!(dev->config->get_status(dev) & VIRTIO_CONFIG_S_DRIVER_OK))
+   virtio_device_ready(dev);
 
virtio_config_enable(dev);
 
-- 
2.35.1

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH net] vsock/virtio: enable VQs early on probe

[Stefano Garzarella]

virtio spec requires drivers to set DRIVER_OK before using VQs.
This is set automatically after probe returns, but virtio-vsock
driver uses VQs in the probe function to fill rx and event VQs
with new buffers.

Let's fix this, calling virtio_device_ready() before using VQs
in the probe function.

Fixes: 0ea9e1d3a9e3 ("VSOCK: Introduce virtio_transport.ko")
Signed-off-by: Stefano Garzarella 
---
 net/vmw_vsock/virtio_transport.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
index 5afc194a58bb..b1962f8cd502 100644
--- a/net/vmw_vsock/virtio_transport.c
+++ b/net/vmw_vsock/virtio_transport.c
@@ -622,6 +622,8 @@ static int virtio_vsock_probe(struct virtio_device *vdev)
INIT_WORK(>event_work, virtio_transport_event_work);
INIT_WORK(>send_pkt_work, virtio_transport_send_pkt_work);
 
+   virtio_device_ready(vdev);
+
mutex_lock(>tx_lock);
vsock->tx_run = true;
mutex_unlock(>tx_lock);
-- 
2.35.1

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH] drm/virtio: fix NULL pointer dereference in virtio_gpu_conn_get_modes

[Liu Zixian via Virtualization]

drm_cvt_mode may return NULL and we should check it.

This bug is found by syzkaller:

FAULT_INJECTION stacktrace:
[  168.567394] FAULT_INJECTION: forcing a failure.
name failslab, interval 1, probability 0, space 0, times 1
[  168.567403] CPU: 1 PID: 6425 Comm: syz Kdump: loaded Not tainted 
4.19.90-vhulk2201.1.0.h1035.kasan.eulerosv2r10.aarch64 #1
[  168.567406] Hardware name: QEMU KVM Virtual Machine, BIOS 0.0.0 02/06/2015
[  168.567408] Call trace:
[  168.567414]  dump_backtrace+0x0/0x310
[  168.567418]  show_stack+0x28/0x38
[  168.567423]  dump_stack+0xec/0x15c
[  168.567427]  should_fail+0x3ac/0x3d0
[  168.567437]  __should_failslab+0xb8/0x120
[  168.567441]  should_failslab+0x28/0xc0
[  168.567445]  kmem_cache_alloc_trace+0x50/0x640
[  168.567454]  drm_mode_create+0x40/0x90
[  168.567458]  drm_cvt_mode+0x48/0xc78
[  168.567477]  virtio_gpu_conn_get_modes+0xa8/0x140 [virtio_gpu]
[  168.567485]  drm_helper_probe_single_connector_modes+0x3a4/0xd80
[  168.567492]  drm_mode_getconnector+0x2e0/0xa70
[  168.567496]  drm_ioctl_kernel+0x11c/0x1d8
[  168.567514]  drm_ioctl+0x558/0x6d0
[  168.567522]  do_vfs_ioctl+0x160/0xf30
[  168.567525]  ksys_ioctl+0x98/0xd8
[  168.567530]  __arm64_sys_ioctl+0x50/0xc8
[  168.567536]  el0_svc_common+0xc8/0x320
[  168.567540]  el0_svc_handler+0xf8/0x160
[  168.567544]  el0_svc+0x10/0x218

KASAN stacktrace:
[  168.567561] BUG: KASAN: null-ptr-deref in 
virtio_gpu_conn_get_modes+0xb4/0x140 [virtio_gpu]
[  168.567565] Read of size 4 at addr 0054 by task syz/6425
[  168.567566]
[  168.567571] CPU: 1 PID: 6425 Comm: syz Kdump: loaded Not tainted 
4.19.90-vhulk2201.1.0.h1035.kasan.eulerosv2r10.aarch64 #1
[  168.567573] Hardware name: QEMU KVM Virtual Machine, BIOS 0.0.0 02/06/2015
[  168.567575] Call trace:
[  168.567578]  dump_backtrace+0x0/0x310
[  168.567582]  show_stack+0x28/0x38
[  168.567586]  dump_stack+0xec/0x15c
[  168.567591]  kasan_report+0x244/0x2f0
[  168.567594]  __asan_load4+0x58/0xb0
[  168.567607]  virtio_gpu_conn_get_modes+0xb4/0x140 [virtio_gpu]
[  168.567612]  drm_helper_probe_single_connector_modes+0x3a4/0xd80
[  168.567617]  drm_mode_getconnector+0x2e0/0xa70
[  168.567621]  drm_ioctl_kernel+0x11c/0x1d8
[  168.567624]  drm_ioctl+0x558/0x6d0
[  168.567628]  do_vfs_ioctl+0x160/0xf30
[  168.567632]  ksys_ioctl+0x98/0xd8
[  168.567636]  __arm64_sys_ioctl+0x50/0xc8
[  168.567641]  el0_svc_common+0xc8/0x320
[  168.567645]  el0_svc_handler+0xf8/0x160
[  168.567649]  el0_svc+0x10/0x218

Signed-off-by: Liu Zixian 
---
 drivers/gpu/drm/virtio/virtgpu_display.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c 
b/drivers/gpu/drm/virtio/virtgpu_display.c
index 5b00310ac..f73352e7b 100644
--- a/drivers/gpu/drm/virtio/virtgpu_display.c
+++ b/drivers/gpu/drm/virtio/virtgpu_display.c
@@ -179,6 +179,8 @@ static int virtio_gpu_conn_get_modes(struct drm_connector 
*connector)
DRM_DEBUG("add mode: %dx%d\n", width, height);
mode = drm_cvt_mode(connector->dev, width, height, 60,
false, false, false);
+   if (!mode)
+   return count;
mode->type |= DRM_MODE_TYPE_PREFERRED;
drm_mode_probed_add(connector, mode);
count++;
-- 
2.33.0

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] dt-bindings: virtio: mmio: add optional virtio,wakeup property

[Krzysztof Kozlowski]

On 22/03/2022 07:19, Minghao Xue wrote:
> Hi Krzysztof,
> 
> Thanks for your comment. First of all, which "generic wakeup-source
> property" do you mean? 

There is only one generic - wakeup-source.

> Could you give an example? I find "wakeup-source"
> property in several binding files. Are you pointing to this? 

Yes, use wakeup-source.

Please avoid top-posting.



Best regards,
Krzysztof
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 00/16] virtio pci support VIRTIO_F_RING_RESET (refactor vring)

[Jason Wang]

在 2022/3/14 下午5:34, Xuan Zhuo 写道:

The virtio spec already supports the virtio queue reset function. This patch set
is to add this function to the kernel. The relevant virtio spec information is
here:

 https://github.com/oasis-tcs/virtio-spec/issues/124

Also regarding MMIO support for queue reset, I plan to support it after this
patch is passed.

This patch set implements the refactoring of vring. Finally, the
virtuque_resize() interface is provided based on the reset function of the
transport layer.

Test environment:
 Host: 4.19.91
 Qemu: QEMU emulator version 6.2.50 (with vq reset support)



It would be better if you can post Qemu code for review as well.



 Test Cmd:  ethtool -G eth1 rx $1 tx $2; ethtool -g eth1

 The default is split mode, modify Qemu virtio-net to add PACKED feature to 
test
 packed mode.

Please review. Thanks.



Looks good overall, see comments in individual patch.

I think the code is structured in a way that is not friendly to the 
reviewers. For next version, we can bring back the ethtool -G for 
virtio-net.


Thanks




v8:
   1. Provide a virtqueue_reset() interface directly
   2. Split the two patch sets, this is the first part
   3. Add independent allocation helper for allocating state, extra

v7:
   1. fix #6 subject typo
   2. fix #6 ring_size_in_bytes is uninitialized
   3. check by: make W=12

v6:
   1. virtio_pci: use synchronize_irq(irq) to sync the irq callbacks
   2. Introduce virtqueue_reset_vring() to implement the reset of vring during
  the reset process. May use the old vring if num of the vq not change.
   3. find_vqs() support sizes to special the max size of each vq

v5:
   1. add virtio-net support set_ringparam

v4:
   1. just the code of virtio, without virtio-net
   2. Performing reset on a queue is divided into these steps:
 1. reset_vq: reset one vq
 2. recycle the buffer from vq by virtqueue_detach_unused_buf()
 3. release the ring of the vq by vring_release_virtqueue()
 4. enable_reset_vq: re-enable the reset queue
   3. Simplify the parameters of enable_reset_vq()
   4. add container structures for virtio_pci_common_cfg

v3:
   1. keep vq, irq unreleased

Xuan Zhuo (16):
   virtio: add helper virtqueue_get_vring_max_size()
   virtio: struct virtio_config_ops add callbacks for queue_reset
   virtio_ring: update the document of the virtqueue_detach_unused_buf
 for queue reset
   virtio_ring: remove the arg vq of vring_alloc_desc_extra()
   virtio_ring: extract the logic of freeing vring
   virtio_ring: split: extract the logic of alloc queue
   virtio_ring: split: extract the logic of alloc state and extra
   virtio_ring: split: extract the logic of attach vring
   virtio_ring: split: extract the logic of vq init
   virtio_ring: split: implement virtqueue_resize_split()
   virtio_ring: packed: extract the logic of alloc queue
   virtio_ring: packed: extract the logic of alloc state and extra
   virtio_ring: packed: extract the logic of attach vring
   virtio_ring: packed: extract the logic of vq init
   virtio_ring: packed: implement virtqueue_resize_packed()
   virtio_ring: introduce virtqueue_resize()

  drivers/virtio/virtio_mmio.c   |   2 +
  drivers/virtio/virtio_pci_legacy.c |   2 +
  drivers/virtio/virtio_pci_modern.c |   2 +
  drivers/virtio/virtio_ring.c   | 604 ++---
  include/linux/virtio.h |   5 +
  include/linux/virtio_config.h  |  12 +
  6 files changed, 494 insertions(+), 133 deletions(-)

--
2.31.0



___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 11/16] virtio_ring: packed: extract the logic of alloc queue

[Jason Wang]

在 2022/3/14 下午5:34, Xuan Zhuo 写道:

Separate the logic of packed to create vring queue.

For the convenience of passing parameters, add a structure
vring_packed.

This feature is required for subsequent virtuqueue reset vring.

Signed-off-by: Xuan Zhuo 
---
  drivers/virtio/virtio_ring.c | 121 ++-
  1 file changed, 92 insertions(+), 29 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index a15869514146..96ff2dabda14 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -85,6 +85,18 @@ struct vring_desc_extra {
u16 next;   /* The next desc state in a list. */
  };
  
+struct vring_packed {

+   u32 num;
+   struct vring_packed_desc *ring;
+   struct vring_packed_desc_event *driver;
+   struct vring_packed_desc_event *device;
+   dma_addr_t ring_dma_addr;
+   dma_addr_t driver_event_dma_addr;
+   dma_addr_t device_event_dma_addr;
+   size_t ring_size_in_bytes;
+   size_t event_size_in_bytes;
+};
+



Interesting,  a nitpick here is that if it is only used by one helper, 
it's probably not worth to bother.


And if we want to do that, we need

1) use a separated patch

2) do it for split virtqueue as well

Thanks



  struct vring_virtqueue {
struct virtqueue vq;
  
@@ -1735,45 +1747,101 @@ static struct vring_desc_extra *vring_alloc_desc_extra(unsigned int num)

return desc_extra;
  }
  
-static struct virtqueue *vring_create_virtqueue_packed(

-   unsigned int index,
-   unsigned int num,
-   unsigned int vring_align,
-   struct virtio_device *vdev,
-   bool weak_barriers,
-   bool may_reduce_num,
-   bool context,
-   bool (*notify)(struct virtqueue *),
-   void (*callback)(struct virtqueue *),
-   const char *name)
+static void vring_free_vring_packed(struct vring_packed *vring,
+   struct virtio_device *vdev)
+{
+   dma_addr_t ring_dma_addr, driver_event_dma_addr, device_event_dma_addr;
+   struct vring_packed_desc_event *driver, *device;
+   size_t ring_size_in_bytes, event_size_in_bytes;
+   struct vring_packed_desc *ring;
+
+   ring  = vring->ring;
+   driver= vring->driver;
+   device= vring->device;
+   ring_size_in_bytes= vring->ring_size_in_bytes;
+   event_size_in_bytes   = vring->event_size_in_bytes;
+   ring_dma_addr = vring->ring_dma_addr;
+   driver_event_dma_addr = vring->driver_event_dma_addr;
+   device_event_dma_addr = vring->device_event_dma_addr;
+
+   if (device)
+   vring_free_queue(vdev, event_size_in_bytes, device, 
device_event_dma_addr);
+
+   if (driver)
+   vring_free_queue(vdev, event_size_in_bytes, driver, 
driver_event_dma_addr);
+
+   if (ring)
+   vring_free_queue(vdev, ring_size_in_bytes, ring, ring_dma_addr);
+}
+
+static int vring_alloc_queue_packed(struct vring_packed *vring,
+   struct virtio_device *vdev,
+   u32 num)
  {
-   struct vring_virtqueue *vq;
struct vring_packed_desc *ring;
struct vring_packed_desc_event *driver, *device;
dma_addr_t ring_dma_addr, driver_event_dma_addr, device_event_dma_addr;
size_t ring_size_in_bytes, event_size_in_bytes;
  
+	memset(vring, 0, sizeof(*vring));

+
ring_size_in_bytes = num * sizeof(struct vring_packed_desc);
  
  	ring = vring_alloc_queue(vdev, ring_size_in_bytes,

 _dma_addr,
 GFP_KERNEL|__GFP_NOWARN|__GFP_ZERO);
if (!ring)
-   goto err_ring;
+   goto err;
+
+   vring->num = num;
+   vring->ring = ring;
+   vring->ring_size_in_bytes = ring_size_in_bytes;
+   vring->ring_dma_addr = ring_dma_addr;
  
  	event_size_in_bytes = sizeof(struct vring_packed_desc_event);

+   vring->event_size_in_bytes = event_size_in_bytes;
  
  	driver = vring_alloc_queue(vdev, event_size_in_bytes,

   _event_dma_addr,
   GFP_KERNEL|__GFP_NOWARN|__GFP_ZERO);
if (!driver)
-   goto err_driver;
+   goto err;
+
+   vring->driver = driver;
+   vring->driver_event_dma_addr = driver_event_dma_addr;
  
  	device = vring_alloc_queue(vdev, event_size_in_bytes,

   _event_dma_addr,
   GFP_KERNEL|__GFP_NOWARN|__GFP_ZERO);
if (!device)
-   goto err_device;
+   goto err;
+
+   vring->device = device;
+   vring->device_event_dma_addr = device_event_dma_addr;
+   return 0;
+
+err:
+   vring_free_vring_packed(vring, vdev);
+   return -ENOMEM;
+}
+
+static struct virtqueue *vring_create_virtqueue_packed(
+   unsigned int index,

Re: [PATCH v8 07/16] virtio_ring: split: extract the logic of alloc state and extra

[Jason Wang]

在 2022/3/14 下午5:34, Xuan Zhuo 写道:

Separate the logic of creating desc_state, desc_extra, and subsequent
patches will call it independently.

Signed-off-by: Xuan Zhuo 
---
  drivers/virtio/virtio_ring.c | 52 +---
  1 file changed, 37 insertions(+), 15 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index d7667f74c42b..9b850188c38e 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -2186,6 +2186,33 @@ irqreturn_t vring_interrupt(int irq, void *_vq)
  }
  EXPORT_SYMBOL_GPL(vring_interrupt);
  
+static int __vring_alloc_state_extra_split(u32 num,

+  struct vring_desc_state_split 
**desc_state,
+  struct vring_desc_extra **desc_extra)



A nit here: I think we can simply remove the "__" prefix since:

1) We haven't had a version of helper without "__"

2) There're other internal helpers that doesn't use "__"

Thanks



+{
+   struct vring_desc_state_split *state;
+   struct vring_desc_extra *extra;
+
+   state = kmalloc_array(num, sizeof(struct vring_desc_state_split), 
GFP_KERNEL);
+   if (!state)
+   goto err_state;
+
+   extra = vring_alloc_desc_extra(num);
+   if (!extra)
+   goto err_extra;
+
+   memset(state, 0, num * sizeof(struct vring_desc_state_split));
+
+   *desc_state = state;
+   *desc_extra = extra;
+   return 0;
+
+err_extra:
+   kfree(state);
+err_state:
+   return -ENOMEM;
+}
+
  /* Only available for split ring */
  struct virtqueue *__vring_new_virtqueue(unsigned int index,
struct vring vring,
@@ -2196,7 +2223,10 @@ struct virtqueue *__vring_new_virtqueue(unsigned int 
index,
void (*callback)(struct virtqueue *),
const char *name)
  {
+   struct vring_desc_state_split *state;
+   struct vring_desc_extra *extra;
struct vring_virtqueue *vq;
+   int err;
  
  	if (virtio_has_feature(vdev, VIRTIO_F_RING_PACKED))

return NULL;
@@ -2246,30 +2276,22 @@ struct virtqueue *__vring_new_virtqueue(unsigned int 
index,
vq->split.avail_flags_shadow);
}
  
-	vq->split.desc_state = kmalloc_array(vring.num,

-   sizeof(struct vring_desc_state_split), GFP_KERNEL);
-   if (!vq->split.desc_state)
-   goto err_state;
+   err = __vring_alloc_state_extra_split(vring.num, , );
+   if (err) {
+   kfree(vq);
+   return NULL;
+   }
  
-	vq->split.desc_extra = vring_alloc_desc_extra(vring.num);

-   if (!vq->split.desc_extra)
-   goto err_extra;
+   vq->split.desc_state = state;
+   vq->split.desc_extra = extra;
  
  	/* Put everything in free lists. */

vq->free_head = 0;
-   memset(vq->split.desc_state, 0, vring.num *
-   sizeof(struct vring_desc_state_split));
  
  	spin_lock(>vqs_list_lock);

list_add_tail(>vq.list, >vqs);
spin_unlock(>vqs_list_lock);
return >vq;
-
-err_extra:
-   kfree(vq->split.desc_state);
-err_state:
-   kfree(vq);
-   return NULL;
  }
  EXPORT_SYMBOL_GPL(__vring_new_virtqueue);
  


___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 10/16] virtio_ring: split: implement virtqueue_resize_split()

[Jason Wang]

在 2022/3/14 下午5:34, Xuan Zhuo 写道:

virtio ring split supports resize.

Only after the new vring is successfully allocated based on the new num,
we will release the old vring. In any case, an error is returned,
indicating that the vring still points to the old vring. In the case of
an error, we will re-initialize the state of the vring to ensure that
the vring can be used.

In addition, vring_align, may_reduce_num are necessary for reallocating
vring, so they are retained for creating vq.

Signed-off-by: Xuan Zhuo 
---
  drivers/virtio/virtio_ring.c | 62 
  1 file changed, 62 insertions(+)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 81bbfd65411e..a15869514146 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -139,6 +139,12 @@ struct vring_virtqueue {
/* DMA address and size information */
dma_addr_t queue_dma_addr;
size_t queue_size_in_bytes;
+
+   /* The parameters for creating vrings are reserved for
+* creating new vrings when enabling reset queue.
+*/
+   u32 vring_align;
+   bool may_reduce_num;
} split;
  
  		/* Available for packed ring */

@@ -198,6 +204,16 @@ struct vring_virtqueue {
  #endif
  };
  
+static void __vring_free(struct virtqueue *_vq);

+static void __vring_virtqueue_init_split(struct vring_virtqueue *vq,
+struct virtio_device *vdev);
+static void __vring_virtqueue_attach_split(struct vring_virtqueue *vq,
+  struct vring vring,
+  struct vring_desc_state_split 
*desc_state,
+  struct vring_desc_extra *desc_extra);
+static int __vring_alloc_state_extra_split(u32 num,
+  struct vring_desc_state_split 
**desc_state,
+  struct vring_desc_extra 
**desc_extra);
  
  /*

   * Helpers.
@@ -991,6 +1007,8 @@ static struct virtqueue *vring_create_virtqueue_split(
return NULL;
}
  
+	to_vvq(vq)->split.vring_align = vring_align;

+   to_vvq(vq)->split.may_reduce_num = may_reduce_num;
to_vvq(vq)->split.queue_dma_addr = dma_addr;
to_vvq(vq)->split.queue_size_in_bytes = queue_size_in_bytes;
to_vvq(vq)->we_own_ring = true;
@@ -998,6 +1016,50 @@ static struct virtqueue *vring_create_virtqueue_split(
return vq;
  }
  
+static int virtqueue_resize_split(struct virtqueue *_vq, u32 num)

+{
+   struct vring_virtqueue *vq = to_vvq(_vq);
+   struct virtio_device *vdev = _vq->vdev;
+   struct vring_desc_state_split *state;
+   struct vring_desc_extra *extra;
+   size_t queue_size_in_bytes;
+   dma_addr_t dma_addr;
+   struct vring vring;
+   int err = -ENOMEM;
+   void *queue;
+
+   BUG_ON(!vq->we_own_ring);



I don't see any checks in virtqueue_resize(). So I think it's better to 
either


1) return -EINVAL here

or

2) add a check in virtqueue_resize and fail there



+
+   queue = vring_alloc_queue_split(vdev, _addr, ,
+   vq->split.vring_align,
+   vq->weak_barriers,
+   vq->split.may_reduce_num);
+   if (!queue)
+   goto init;
+
+   queue_size_in_bytes = vring_size(num, vq->split.vring_align);
+
+   err = __vring_alloc_state_extra_split(num, , );
+   if (err) {
+   vring_free_queue(vdev, queue_size_in_bytes, queue, dma_addr);
+   goto init;
+   }
+
+   __vring_free(>vq);
+
+   vring_init(, num, queue, vq->split.vring_align);
+   __vring_virtqueue_attach_split(vq, vring, state, extra);



I wonder if we need a symmetric virtqueue_resize_detach() internal helper.



+   vq->split.queue_dma_addr = dma_addr;
+   vq->split.queue_size_in_bytes = queue_size_in_bytes;
+
+   err = 0;
+
+init:
+   __vring_virtqueue_init_split(vq, vdev);
+   vq->we_own_ring = true;



Then we can leave this unchanged.

Thanks



+   return err;
+}
+
  
  /*

   * Packed ring specific functions - *_packed().


___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 01/16] virtio: add helper virtqueue_get_vring_max_size()

[Michael S. Tsirkin]

On Mon, Mar 14, 2022 at 07:21:18PM +0800, Xuan Zhuo wrote:
> On Mon, 14 Mar 2022 07:18:27 -0400, "Michael S. Tsirkin"  
> wrote:
> > On Mon, Mar 14, 2022 at 10:50:08AM +0100, Cornelia Huck wrote:
> > > On Mon, Mar 14 2022, Xuan Zhuo  wrote:
> > >
> > > > Record the maximum queue num supported by the device.
> > > >
> > > > virtio-net can display the maximum (supported by hardware) ring size in
> > > > ethtool -g eth0.
> > > >
> > > > When the subsequent patch implements vring reset, it can judge whether
> > > > the ring size passed by the driver is legal based on this.
> > > >
> > > > Signed-off-by: Xuan Zhuo 
> > > > Acked-by: Jason Wang 
> > > > ---
> > > >  drivers/virtio/virtio_mmio.c   |  2 ++
> > > >  drivers/virtio/virtio_pci_legacy.c |  2 ++
> > > >  drivers/virtio/virtio_pci_modern.c |  2 ++
> > > >  drivers/virtio/virtio_ring.c   | 14 ++
> > > >  include/linux/virtio.h |  2 ++
> > > >  5 files changed, 22 insertions(+)
> > >
> > > Don't you also need to init this for ccw (even though we won't do ring
> > > reset there), just for completeness? (Any other transports?)
> >
> > rpmsg?
> 
> There should be these.
> 
>  arch/um/drivers/virtio_uml.c |  1 +
>  drivers/platform/mellanox/mlxbf-tmfifo.c |  2 ++
>  drivers/remoteproc/remoteproc_virtio.c   |  2 ++
>  drivers/s390/virtio/virtio_ccw.c |  3 +++
>  drivers/virtio/virtio_mmio.c |  2 ++
>  drivers/virtio/virtio_pci_legacy.c   |  2 ++
>  drivers/virtio/virtio_pci_modern.c   |  2 ++
>  drivers/virtio/virtio_vdpa.c |  2 ++
> 
> It will be included in the next version.

Hmm so merge window is open but we don't have a final version yet.
Not sure it can make it in this merge window ..


> Thanks.

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 16/16] virtio_ring: introduce virtqueue_resize()

[Jason Wang]

在 2022/3/14 下午5:34, Xuan Zhuo 写道:

Introduce virtqueue_resize() to implement the resize of vring.
Based on these, the driver can dynamically adjust the size of the vring.
For example: ethtool -G.

virtqueue_resize() implements resize based on the vq reset function. In
case of failure to allocate a new vring, it will give up resize and use
the original vring.

During this process, if the re-enable reset vq fails, the vq can no
longer be used. Although the probability of this situation is not high.

The parameter recycle is used to recycle the buffer that is no longer
used.

Signed-off-by: Xuan Zhuo 
---
  drivers/virtio/virtio_ring.c | 67 
  include/linux/virtio.h   |  3 ++
  2 files changed, 70 insertions(+)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index fb0abf9a2f57..b1dde086a8a4 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -2528,6 +2528,73 @@ struct virtqueue *vring_create_virtqueue(
  }
  EXPORT_SYMBOL_GPL(vring_create_virtqueue);
  
+/**

+ * virtqueue_resize - resize the vring of vq
+ * @vq: the struct virtqueue we're talking about.
+ * @num: new ring num
+ * @recycle: callback for recycle the useless buffer
+ *
+ * When it is really necessary to create a new vring, it will set the current 
vq
+ * into the reset state. Then call the passed cb to recycle the buffer that is
+ * no longer used. Only after the new vring is successfully created, the old
+ * vring will be released.
+ *
+ * Caller must ensure we don't call this with other virtqueue operations
+ * at the same time (except where noted).
+ *
+ * Returns zero or a negative error.
+ * -ENOMEM: create new vring fail. But vq can still work
+ * -EBUSY:  reset/re-enable vq fail. vq may cannot work
+ * -ENOENT: not support resize
+ * -E2BIG/-EINVAL: param num error
+ */
+int virtqueue_resize(struct virtqueue *vq, u32 num,
+void (*recycle)(struct virtqueue *vq, void *buf))
+{
+   struct virtio_device *vdev = vq->vdev;
+   void *buf;
+   int err;
+
+   if (num > vq->num_max)
+   return -E2BIG;
+
+   if (!num)
+   return -EINVAL;
+
+   if (to_vvq(vq)->packed.vring.num == num)
+   return 0;



Any reason we need to check a packed specific attribute here?



+
+   if (!vq->vdev->config->reset_vq)
+   return -ENOENT;
+
+   if (!vq->vdev->config->enable_reset_vq)
+   return -ENOENT;
+
+   err = vq->vdev->config->reset_vq(vq);
+   if (err) {
+   if (err != -ENOENT)
+   err = -EBUSY;
+   return err;
+   }
+
+   while ((buf = virtqueue_detach_unused_buf(vq)) != NULL)
+   recycle(vq, buf);
+
+   if (virtio_has_feature(vdev, VIRTIO_F_RING_PACKED))
+   err = virtqueue_resize_packed(vq, num);
+   else
+   err = virtqueue_resize_split(vq, num);
+
+   if (err)
+   err = -ENOMEM;



So this assumes that the -ENOMEM is the only possible error value for 
virtqueue_resize_xxx(). Is this true? (E.g wrong size)




+
+   if (vq->vdev->config->enable_reset_vq(vq))
+   return -EBUSY;
+
+   return err;
+}
+EXPORT_SYMBOL_GPL(virtqueue_resize);
+
  /* Only available for split ring */
  struct virtqueue *vring_new_virtqueue(unsigned int index,
  unsigned int num,
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index d59adc4be068..c86ff02e0ca0 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -91,6 +91,9 @@ dma_addr_t virtqueue_get_desc_addr(struct virtqueue *vq);
  dma_addr_t virtqueue_get_avail_addr(struct virtqueue *vq);
  dma_addr_t virtqueue_get_used_addr(struct virtqueue *vq);
  
+int virtqueue_resize(struct virtqueue *vq, u32 num,

+void (*recycle)(struct virtqueue *vq, void *buf));



I wonder what's the advantages of coupling virtqueue_reset in 
virtqueue_resize().


It looks to me it wold be more flexible to let the driver do:

rest()

detach()

resize()

enable_reset()

One reason is that in the future we may want to add more functionality 
e.g switching PASID during virtqueue reset.


Thanks



+
  /**
   * virtio_device - representation of a device using virtio
   * @index: unique position on the virtio bus


___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization