Re: Re: [PATCH 0/3] recover hardware corrupted page by virtio balloon
On 5/27/22 02:37, Peter Xu wrote: On Wed, May 25, 2022 at 01:16:34PM -0700, Jue Wang wrote: The hypervisor _must_ emulate poisons identified in guest physical address space (could be transported from the source VM), this is to prevent silent data corruption in the guest. With a paravirtual approach like this patch series, the hypervisor can clear some of the poisoned HVAs knowing for certain that the guest OS has isolated the poisoned page. I wonder how much value it provides to the guest if the guest and workload are _not_ in a pressing need for the extra KB/MB worth of memory. I'm curious the same on how unpoisoning could help here. The reasoning behind would be great material to be mentioned in the next cover letter. Shouldn't we consider migrating serious workloads off the host already where there's a sign of more severe hardware issues, instead? Thanks, I'm maintaining 1000,000+ virtual machines, from my experience: UE is quite unusual and occurs randomly, and I did not hit UE storm case in the past years. The memory also has no obvious performance drop after hitting UE. I hit several CE storm case, the performance memory drops a lot. But I can't find obvious relationship between UE and CE. So from the point of my view, to fix the corrupted page for VM seems good enough. And yes, unpoisoning several pages does not help significantly, but it is still a chance to make the virtualization better. -- zhenwei pi ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH V6 9/9] virtio: use WARN_ON() to warning illegal status value
We used to use BUG_ON() in virtio_device_ready() to detect illegal
status value, this seems sub-optimal since the value is under the
control of the device. Switch to use WARN_ON() instead.
Cc: Thomas Gleixner
Cc: Peter Zijlstra
Cc: "Paul E. McKenney"
Cc: Marc Zyngier
Cc: Halil Pasic
Cc: Cornelia Huck
Cc: Vineeth Vijayan
Cc: Peter Oberparleiter
Cc: linux-s...@vger.kernel.org
Signed-off-by: Jason Wang
---
include/linux/virtio_config.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index d4edfd7d91bb..9a36051ceb76 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -255,7 +255,7 @@ void virtio_device_ready(struct virtio_device *dev)
{
unsigned status = dev->config->get_status(dev);
- BUG_ON(status & VIRTIO_CONFIG_S_DRIVER_OK);
+ WARN_ON(status & VIRTIO_CONFIG_S_DRIVER_OK);
/*
* The virtio_synchronize_cbs() makes sure vring_interrupt()
--
2.25.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH V6 8/9] virtio: harden vring IRQ
This is a rework on the previous IRQ hardening that is done for
virtio-pci where several drawbacks were found and were reverted:
1) try to use IRQF_NO_AUTOEN which is not friendly to affinity managed IRQ
that is used by some device such as virtio-blk
2) done only for PCI transport
The vq->broken is re-used in this patch for implementing the IRQ
hardening. The vq->broken is set to true during both initialization
and reset. And the vq->broken is set to false in
virtio_device_ready(). Then vring_interrupt() can check and return
when vq->broken is true. And in this case, switch to return IRQ_NONE
to let the interrupt core aware of such invalid interrupt to prevent
IRQ storm.
The reason of using a per queue variable instead of a per device one
is that we may need it for per queue reset hardening in the future.
Note that the hardening is only done for vring interrupt since the
config interrupt hardening is already done in commit 22b7050a024d7
("virtio: defer config changed notifications"). But the method that is
used by config interrupt can't be reused by the vring interrupt
handler because it uses spinlock to do the synchronization which is
expensive.
Cc: Thomas Gleixner
Cc: Peter Zijlstra
Cc: "Paul E. McKenney"
Cc: Marc Zyngier
Cc: Halil Pasic
Cc: Cornelia Huck
Cc: Vineeth Vijayan
Cc: Peter Oberparleiter
Cc: linux-s...@vger.kernel.org
Signed-off-by: Jason Wang
---
drivers/s390/virtio/virtio_ccw.c | 4
drivers/virtio/virtio.c| 15 ---
drivers/virtio/virtio_mmio.c | 5 +
drivers/virtio/virtio_pci_modern_dev.c | 5 +
drivers/virtio/virtio_ring.c | 11 +++
include/linux/virtio_config.h | 20
6 files changed, 53 insertions(+), 7 deletions(-)
diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
index c188e4f20ca3..97e51c34e6cf 100644
--- a/drivers/s390/virtio/virtio_ccw.c
+++ b/drivers/s390/virtio/virtio_ccw.c
@@ -971,6 +971,10 @@ static void virtio_ccw_set_status(struct virtio_device
*vdev, u8 status)
ccw->flags = 0;
ccw->count = sizeof(status);
ccw->cda = (__u32)(unsigned long)&vcdev->dma_area->status;
+ /* We use ssch for setting the status which is a serializing
+* instruction that guarantees the memory writes have
+* completed before ssch.
+*/
ret = ccw_io_helper(vcdev, ccw, VIRTIO_CCW_DOING_WRITE_STATUS);
/* Write failed? We assume status is unchanged. */
if (ret)
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index aa1eb5132767..95fac4c97c8b 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -220,6 +220,15 @@ static int virtio_features_ok(struct virtio_device *dev)
* */
void virtio_reset_device(struct virtio_device *dev)
{
+ /*
+* The below virtio_synchronize_cbs() guarantees that any
+* interrupt for this line arriving after
+* virtio_synchronize_vqs() has completed is guaranteed to see
+* vq->broken as true.
+*/
+ virtio_break_device(dev);
+ virtio_synchronize_cbs(dev);
+
dev->config->reset(dev);
}
EXPORT_SYMBOL_GPL(virtio_reset_device);
@@ -428,6 +437,9 @@ int register_virtio_device(struct virtio_device *dev)
dev->config_enabled = false;
dev->config_change_pending = false;
+ INIT_LIST_HEAD(&dev->vqs);
+ spin_lock_init(&dev->vqs_list_lock);
+
/* We always start by resetting the device, in case a previous
* driver messed it up. This also tests that code path a little. */
virtio_reset_device(dev);
@@ -435,9 +447,6 @@ int register_virtio_device(struct virtio_device *dev)
/* Acknowledge that we've seen the device. */
virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
- INIT_LIST_HEAD(&dev->vqs);
- spin_lock_init(&dev->vqs_list_lock);
-
/*
* device_add() causes the bus infrastructure to look for a matching
* driver.
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index c9699a59f93c..f9a36bc7ac27 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -253,6 +253,11 @@ static void vm_set_status(struct virtio_device *vdev, u8
status)
/* We should never be setting status to 0. */
BUG_ON(status == 0);
+ /*
+* Per memory-barriers.txt, wmb() is not needed to guarantee
+* that the the cache coherent memory writes have completed
+* before writing to the MMIO region.
+*/
writel(status, vm_dev->base + VIRTIO_MMIO_STATUS);
}
diff --git a/drivers/virtio/virtio_pci_modern_dev.c
b/drivers/virtio/virtio_pci_modern_dev.c
index 4093f9cca7a6..a0fa14f28a7f 100644
--- a/drivers/virtio/virtio_pci_modern_dev.c
+++ b/drivers/virtio/virtio_pci_modern_dev.c
@@ -467,6 +467,11 @@ void vp_modern_set_status(struct virtio_pci_modern_device
*mdev,
{
struct virtio_pci_com
[PATCH V6 7/9] virtio: allow to unbreak virtqueue
This patch allows the new introduced __virtio_break_device() to
unbreak the virtqueue.
Cc: Thomas Gleixner
Cc: Peter Zijlstra
Cc: "Paul E. McKenney"
Cc: Marc Zyngier
Cc: Halil Pasic
Cc: Cornelia Huck
Cc: Vineeth Vijayan
Cc: Peter Oberparleiter
Cc: linux-s...@vger.kernel.org
Signed-off-by: Jason Wang
---
drivers/virtio/virtio_ring.c | 22 ++
include/linux/virtio.h | 1 +
2 files changed, 23 insertions(+)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 9d0bae4293be..9c231e1fded7 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -2395,6 +2395,28 @@ void virtio_break_device(struct virtio_device *dev)
}
EXPORT_SYMBOL_GPL(virtio_break_device);
+/*
+ * This should allow the device to be used by the driver. You may
+ * need to grab appropriate locks to flush the write to
+ * vq->broken. This should only be used in some specific case e.g
+ * (probing and restoring). This function should only be called by the
+ * core, not directly by the driver.
+ */
+void __virtio_unbreak_device(struct virtio_device *dev)
+{
+ struct virtqueue *_vq;
+
+ spin_lock(&dev->vqs_list_lock);
+ list_for_each_entry(_vq, &dev->vqs, list) {
+ struct vring_virtqueue *vq = to_vvq(_vq);
+
+ /* Pairs with READ_ONCE() in virtqueue_is_broken(). */
+ WRITE_ONCE(vq->broken, false);
+ }
+ spin_unlock(&dev->vqs_list_lock);
+}
+EXPORT_SYMBOL_GPL(__virtio_unbreak_device);
+
dma_addr_t virtqueue_get_desc_addr(struct virtqueue *_vq)
{
struct vring_virtqueue *vq = to_vvq(_vq);
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 5464f398912a..d8fdf170637c 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -131,6 +131,7 @@ void unregister_virtio_device(struct virtio_device *dev);
bool is_virtio_device(struct device *dev);
void virtio_break_device(struct virtio_device *dev);
+void __virtio_unbreak_device(struct virtio_device *dev);
void virtio_config_changed(struct virtio_device *dev);
#ifdef CONFIG_PM_SLEEP
--
2.25.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH V6 6/9] virtio-ccw: implement synchronize_cbs()
This patch tries to implement the synchronize_cbs() for ccw. For the
vring_interrupt() that is called via virtio_airq_handler(), the
synchronization is simply done via the airq_info's lock. For the
vring_interrupt() that is called via virtio_ccw_int_handler(), a per
device rwlock is introduced and used in the synchronization method.
Cc: Thomas Gleixner
Cc: Peter Zijlstra
Cc: "Paul E. McKenney"
Cc: Marc Zyngier
Cc: Halil Pasic
Cc: Cornelia Huck
Cc: Vineeth Vijayan
Cc: Peter Oberparleiter
Cc: linux-s...@vger.kernel.org
Reviewed-by: Halil Pasic
Signed-off-by: Jason Wang
---
drivers/s390/virtio/virtio_ccw.c | 30 ++
1 file changed, 30 insertions(+)
diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
index d35e7a3f7067..c188e4f20ca3 100644
--- a/drivers/s390/virtio/virtio_ccw.c
+++ b/drivers/s390/virtio/virtio_ccw.c
@@ -62,6 +62,7 @@ struct virtio_ccw_device {
unsigned int revision; /* Transport revision */
wait_queue_head_t wait_q;
spinlock_t lock;
+ rwlock_t irq_lock;
struct mutex io_lock; /* Serializes I/O requests */
struct list_head virtqueues;
bool is_thinint;
@@ -984,6 +985,30 @@ static const char *virtio_ccw_bus_name(struct
virtio_device *vdev)
return dev_name(&vcdev->cdev->dev);
}
+static void virtio_ccw_synchronize_cbs(struct virtio_device *vdev)
+{
+ struct virtio_ccw_device *vcdev = to_vc_device(vdev);
+ struct airq_info *info = vcdev->airq_info;
+
+ if (info) {
+ /*
+* This device uses adapter interrupts: synchronize with
+* vring_interrupt() called by virtio_airq_handler()
+* via the indicator area lock.
+*/
+ write_lock_irq(&info->lock);
+ write_unlock_irq(&info->lock);
+ } else {
+ /* This device uses classic interrupts: synchronize
+* with vring_interrupt() called by
+* virtio_ccw_int_handler() via the per-device
+* irq_lock
+*/
+ write_lock_irq(&vcdev->irq_lock);
+ write_unlock_irq(&vcdev->irq_lock);
+ }
+}
+
static const struct virtio_config_ops virtio_ccw_config_ops = {
.get_features = virtio_ccw_get_features,
.finalize_features = virtio_ccw_finalize_features,
@@ -995,6 +1020,7 @@ static const struct virtio_config_ops
virtio_ccw_config_ops = {
.find_vqs = virtio_ccw_find_vqs,
.del_vqs = virtio_ccw_del_vqs,
.bus_name = virtio_ccw_bus_name,
+ .synchronize_cbs = virtio_ccw_synchronize_cbs,
};
@@ -1106,6 +1132,8 @@ static void virtio_ccw_int_handler(struct ccw_device
*cdev,
vcdev->err = -EIO;
}
virtio_ccw_check_activity(vcdev, activity);
+ /* Interrupts are disabled here */
+ read_lock(&vcdev->irq_lock);
for_each_set_bit(i, indicators(vcdev),
sizeof(*indicators(vcdev)) * BITS_PER_BYTE) {
/* The bit clear must happen before the vring kick. */
@@ -1114,6 +1142,7 @@ static void virtio_ccw_int_handler(struct ccw_device
*cdev,
vq = virtio_ccw_vq_by_ind(vcdev, i);
vring_interrupt(0, vq);
}
+ read_unlock(&vcdev->irq_lock);
if (test_bit(0, indicators2(vcdev))) {
virtio_config_changed(&vcdev->vdev);
clear_bit(0, indicators2(vcdev));
@@ -1284,6 +1313,7 @@ static int virtio_ccw_online(struct ccw_device *cdev)
init_waitqueue_head(&vcdev->wait_q);
INIT_LIST_HEAD(&vcdev->virtqueues);
spin_lock_init(&vcdev->lock);
+ rwlock_init(&vcdev->irq_lock);
mutex_init(&vcdev->io_lock);
spin_lock_irqsave(get_ccwdev_lock(cdev), flags);
--
2.25.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH V6 5/9] virtio-mmio: implement synchronize_cbs()
Simply synchronize the platform irq that is used by us.
Cc: Thomas Gleixner
Cc: Peter Zijlstra
Cc: "Paul E. McKenney"
Cc: Marc Zyngier
Cc: Halil Pasic
Cc: Cornelia Huck
Cc: Vineeth Vijayan
Cc: Peter Oberparleiter
Cc: linux-s...@vger.kernel.org
Reviewed-by: Cornelia Huck
Signed-off-by: Jason Wang
---
drivers/virtio/virtio_mmio.c | 8
1 file changed, 8 insertions(+)
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 839684d672af..c9699a59f93c 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -345,6 +345,13 @@ static void vm_del_vqs(struct virtio_device *vdev)
free_irq(platform_get_irq(vm_dev->pdev, 0), vm_dev);
}
+static void vm_synchronize_cbs(struct virtio_device *vdev)
+{
+ struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
+
+ synchronize_irq(platform_get_irq(vm_dev->pdev, 0));
+}
+
static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned int
index,
void (*callback)(struct virtqueue *vq),
const char *name, bool ctx)
@@ -541,6 +548,7 @@ static const struct virtio_config_ops
virtio_mmio_config_ops = {
.finalize_features = vm_finalize_features,
.bus_name = vm_bus_name,
.get_shm_region = vm_get_shm_region,
+ .synchronize_cbs = vm_synchronize_cbs,
};
--
2.25.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH V6 4/9] virtio-pci: implement synchronize_cbs()
We can simply reuse vp_synchronize_vectors() for .synchronize_cbs().
Cc: Thomas Gleixner
Cc: Peter Zijlstra
Cc: "Paul E. McKenney"
Cc: Marc Zyngier
Cc: Halil Pasic
Cc: Cornelia Huck
Cc: Vineeth Vijayan
Cc: Peter Oberparleiter
Cc: linux-s...@vger.kernel.org
Reviewed-by: Cornelia Huck
Signed-off-by: Jason Wang
---
drivers/virtio/virtio_pci_legacy.c | 1 +
drivers/virtio/virtio_pci_modern.c | 2 ++
2 files changed, 3 insertions(+)
diff --git a/drivers/virtio/virtio_pci_legacy.c
b/drivers/virtio/virtio_pci_legacy.c
index 7fe4caa4b519..a5e5721145c7 100644
--- a/drivers/virtio/virtio_pci_legacy.c
+++ b/drivers/virtio/virtio_pci_legacy.c
@@ -192,6 +192,7 @@ static const struct virtio_config_ops virtio_pci_config_ops
= {
.reset = vp_reset,
.find_vqs = vp_find_vqs,
.del_vqs= vp_del_vqs,
+ .synchronize_cbs = vp_synchronize_vectors,
.get_features = vp_get_features,
.finalize_features = vp_finalize_features,
.bus_name = vp_bus_name,
diff --git a/drivers/virtio/virtio_pci_modern.c
b/drivers/virtio/virtio_pci_modern.c
index 4acb34409f0b..623906b4996c 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -394,6 +394,7 @@ static const struct virtio_config_ops
virtio_pci_config_nodev_ops = {
.reset = vp_reset,
.find_vqs = vp_modern_find_vqs,
.del_vqs= vp_del_vqs,
+ .synchronize_cbs = vp_synchronize_vectors,
.get_features = vp_get_features,
.finalize_features = vp_finalize_features,
.bus_name = vp_bus_name,
@@ -411,6 +412,7 @@ static const struct virtio_config_ops virtio_pci_config_ops
= {
.reset = vp_reset,
.find_vqs = vp_modern_find_vqs,
.del_vqs= vp_del_vqs,
+ .synchronize_cbs = vp_synchronize_vectors,
.get_features = vp_get_features,
.finalize_features = vp_finalize_features,
.bus_name = vp_bus_name,
--
2.25.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH V6 3/9] virtio: introduce config op to synchronize vring callbacks
This patch introduces new virtio config op to vring
callbacks. Transport specific method is required to make sure the
write before this function is visible to the vring_interrupt() that is
called after the return of this function. For the transport that
doesn't provide synchronize_vqs(), use synchornize_rcu() which
synchronize with IRQ implicitly as a fallback.
Cc: Thomas Gleixner
Cc: Peter Zijlstra
Cc: "Paul E. McKenney"
Cc: Marc Zyngier
Cc: Halil Pasic
Cc: Cornelia Huck
Cc: Vineeth Vijayan
Cc: Peter Oberparleiter
Cc: linux-s...@vger.kernel.org
Reviewed-by: Cornelia Huck
Signed-off-by: Jason Wang
---
include/linux/virtio_config.h | 25 +
1 file changed, 25 insertions(+)
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index b341dd62aa4d..25be018810a7 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -57,6 +57,11 @@ struct virtio_shm_region {
* include a NULL entry for vqs unused by driver
* Returns 0 on success or error status
* @del_vqs: free virtqueues found by find_vqs().
+ * @synchronize_cbs: synchronize with the virtqueue callbacks (optional)
+ * The function guarantees that all memory operations on the
+ * queue before it are visible to the vring_interrupt() that is
+ * called after it.
+ * vdev: the virtio_device
* @get_features: get the array of feature bits for this device.
* vdev: the virtio_device
* Returns the first 64 feature bits (all we currently need).
@@ -89,6 +94,7 @@ struct virtio_config_ops {
const char * const names[], const bool *ctx,
struct irq_affinity *desc);
void (*del_vqs)(struct virtio_device *);
+ void (*synchronize_cbs)(struct virtio_device *);
u64 (*get_features)(struct virtio_device *vdev);
int (*finalize_features)(struct virtio_device *vdev);
const char *(*bus_name)(struct virtio_device *vdev);
@@ -217,6 +223,25 @@ int virtio_find_vqs_ctx(struct virtio_device *vdev,
unsigned nvqs,
desc);
}
+/**
+ * virtio_synchronize_cbs - synchronize with virtqueue callbacks
+ * @vdev: the device
+ */
+static inline
+void virtio_synchronize_cbs(struct virtio_device *dev)
+{
+ if (dev->config->synchronize_cbs) {
+ dev->config->synchronize_cbs(dev);
+ } else {
+ /*
+* A best effort fallback to synchronize with
+* interrupts, preemption and softirq disabled
+* regions. See comment above synchronize_rcu().
+*/
+ synchronize_rcu();
+ }
+}
+
/**
* virtio_device_ready - enable vq use in probe function
* @vdev: the device
--
2.25.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH V6 2/9] virtio: use virtio_reset_device() when possible
This allows us to do common extension without duplicating code. Cc: Thomas Gleixner Cc: Peter Zijlstra Cc: "Paul E. McKenney" Cc: Marc Zyngier Cc: Halil Pasic Cc: Cornelia Huck Cc: Vineeth Vijayan Cc: Peter Oberparleiter Cc: linux-s...@vger.kernel.org Reviewed-by: Cornelia Huck Signed-off-by: Jason Wang --- drivers/virtio/virtio.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c index 938e975029d4..aa1eb5132767 100644 --- a/drivers/virtio/virtio.c +++ b/drivers/virtio/virtio.c @@ -430,7 +430,7 @@ int register_virtio_device(struct virtio_device *dev) /* We always start by resetting the device, in case a previous * driver messed it up. This also tests that code path a little. */ - dev->config->reset(dev); + virtio_reset_device(dev); /* Acknowledge that we've seen the device. */ virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE); @@ -496,7 +496,7 @@ int virtio_device_restore(struct virtio_device *dev) /* We always start by resetting the device, in case a previous * driver messed it up. */ - dev->config->reset(dev); + virtio_reset_device(dev); /* Acknowledge that we've seen the device. */ virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE); -- 2.25.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH V6 1/9] virtio: use virtio_device_ready() in virtio_device_restore()
From: Stefano Garzarella It will allow us to do extension on virtio_device_ready() without duplicating code. Cc: Thomas Gleixner Cc: Peter Zijlstra Cc: "Paul E. McKenney" Cc: Marc Zyngier Cc: Halil Pasic Cc: Cornelia Huck Cc: Vineeth Vijayan Cc: Peter Oberparleiter Cc: linux-s...@vger.kernel.org Reviewed-by: Cornelia Huck Signed-off-by: Stefano Garzarella Signed-off-by: Jason Wang --- 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 ce424c16997d..938e975029d4 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.25.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH V6 0/9] rework on the IRQ hardening of virtio
Hi All:
This is a rework on the IRQ hardening for virtio which is done
previously by the following commits are reverted:
9e35276a5344 ("virtio_pci: harden MSI-X interrupts")
080cd7c3ac87 ("virtio-pci: harden INTX interrupts")
The reason is that it depends on the IRQF_NO_AUTOEN which may conflict
with the assumption of the affinity managed IRQ that is used by some
virtio drivers. And what's more, it is only done for virtio-pci but
not other transports.
In this rework, I try to implement a general virtio solution which
borrows the idea of the INTX hardening by re-using per virtqueue
boolean vq->broken and toggle it in virtio_device_ready() and
virtio_reset_device(). Then we can simply reuse the existing checks in
the vring_interrupt() and return early if the driver is not ready.
Note that, I only did compile test on ccw and MMIO transport.
Please review.
Changes since V5:
- Various tweaks on the comments
Changes since V4:
- use spin_lock_irq()/spin_unlock_irq() to synchronize with
vring_interrupt() for ccw
- use spin_lock()/spin_unlock() to protect vring_interrupt() for non
airq
- add comment to explain the ordering implications of set_status() for
PCI, ccw and MMIO
- various tweaks on the comments and changelogs
Changes since V3:
- Rename synchornize_vqs() to synchronize_cbs()
- tweak the comment for synchronize_cbs()
- switch to use a dedicated helper __virtio_unbreak_device() and
document it should be only used for probing
- switch to use rwlock to synchornize the non airq for ccw
Changes since V2:
- add ccw and MMIO support
- rename synchronize_vqs() to synchronize_cbs()
- switch to re-use vq->broken instead of introducing new device
attributes for the future virtqueue reset support
- remove unnecssary READ_ONCE()/WRITE_ONCE()
- a new patch to remove device triggerable BUG_ON()
- more tweaks on the comments
Changes since v1:
- Use transport specific irq synchronization method when possible
- Drop the module parameter and enable the hardening unconditonally
- Tweak the barrier/ordering facilities used in the code
- Reanme irq_soft_enabled to driver_ready
- Avoid unnecssary IRQ synchornization (e.g during boot)
Jason Wang (8):
virtio: use virtio_reset_device() when possible
virtio: introduce config op to synchronize vring callbacks
virtio-pci: implement synchronize_cbs()
virtio-mmio: implement synchronize_cbs()
virtio-ccw: implement synchronize_cbs()
virtio: allow to unbreak virtqueue
virtio: harden vring IRQ
virtio: use WARN_ON() to warning illegal status value
Stefano Garzarella (1):
virtio: use virtio_device_ready() in virtio_device_restore()
drivers/s390/virtio/virtio_ccw.c | 34 +++
drivers/virtio/virtio.c| 24 +
drivers/virtio/virtio_mmio.c | 13 +++
drivers/virtio/virtio_pci_legacy.c | 1 +
drivers/virtio/virtio_pci_modern.c | 2 ++
drivers/virtio/virtio_pci_modern_dev.c | 5 +++
drivers/virtio/virtio_ring.c | 33 +++---
include/linux/virtio.h | 1 +
include/linux/virtio_config.h | 47 +-
9 files changed, 148 insertions(+), 12 deletions(-)
--
2.25.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 0/3] recover hardware corrupted page by virtio balloon
Hi, Andrew & Naoya I would appreciate it if you could give me any hint about the changes of memory/memory-failure! On 5/20/22 15:06, zhenwei pi wrote: Hi, I'm trying to recover hardware corrupted page by virtio balloon, the workflow of this feature like this: Guest 5.MF -> 6.RVQ FE10.Unpoison page / \/ ---+-+--+--- | | | 4.MCE7.RVQ BE 9.RVQ Event QEMU / \ / 3.SIGBUS 8.Remap / + | +--2.MF Host / 1.HW error 1, HardWare page error occurs randomly. 2, host side handles corrupted page by Memory Failure mechanism, sends SIGBUS to the user process if early-kill is enabled. 3, QEMU handles SIGBUS, if the address belongs to guest RAM, then: 4, QEMU tries to inject MCE into guest. 5, guest handles memory failure again. 1-5 is already supported for a long time, the next steps are supported in this patch(also related driver patch): 6, guest balloon driver gets noticed of the corrupted PFN, and sends request to host side by Recover VQ FrontEnd. 7, QEMU handles request from Recover VQ BackEnd, then: 8, QEMU remaps the corrupted HVA fo fix the memory failure, then: 9, QEMU acks the guest side the result by Recover VQ. 10, guest unpoisons the page if the corrupted page gets recoverd successfully. Test: This patch set can be tested with QEMU(also in developing): https://github.com/pizhenwei/qemu/tree/balloon-recover Emulate MCE by QEMU(guest RAM normal page only, hugepage is not supported): virsh qemu-monitor-command vm --hmp mce 0 9 0xbdc0 0xd 0x61646678 0x8c The guest works fine(on Intel Platinum 8260): mce: [Hardware Error]: Machine check events logged Memory failure: 0x61646: recovery action for dirty LRU page: Recovered virtio_balloon virtio5: recovered pfn 0x61646 Unpoison: Unpoisoned page 0x61646 by virtio-balloon MCE: Killing stress:24502 due to hardware memory corruption fault at 7f5be2e5a010 And the 'HardwareCorrupted' in /proc/meminfo also shows 0 kB. About the protocol of virtio balloon recover VQ, it's undefined and in developing currently: - 'struct virtio_balloon_recover' defines the structure which is used to exchange message between guest and host. - '__le32 corrupted_pages' in struct virtio_balloon_config is used in the next step: 1, a VM uses RAM of 2M huge page, once a MCE occurs, the 2M becomes unaccessible. Reporting 512 * 4K 'corrupted_pages' to the guest, the guest has a chance to isolate the 512 pages ahead of time. 2, after migrating to another host, the corrupted pages are actually recovered, once the guest gets the 'corrupted_pages' with 0, then the guest could unpoison all the poisoned pages which are recorded in the balloon driver. zhenwei pi (3): memory-failure: Introduce memory failure notifier mm/memory-failure.c: support reset PTE during unpoison virtio_balloon: Introduce memory recover drivers/virtio/virtio_balloon.c | 243 include/linux/mm.h | 4 +- include/uapi/linux/virtio_balloon.h | 16 ++ mm/hwpoison-inject.c| 2 +- mm/memory-failure.c | 59 ++- 5 files changed, 315 insertions(+), 9 deletions(-) -- zhenwei pi ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v4 0/4] Implement vdpasim stop operation
On Thu, May 26, 2022 at 8:54 PM Parav Pandit wrote: > > > > > From: Eugenio Pérez > > Sent: Thursday, May 26, 2022 8:44 AM > > > Implement stop operation for vdpa_sim devices, so vhost-vdpa will offer > > > > that backend feature and userspace can effectively stop the device. > > > > > > > > This is a must before get virtqueue indexes (base) for live migration, > > > > since the device could modify them after userland gets them. There are > > > > individual ways to perform that action for some devices > > > > (VHOST_NET_SET_BACKEND, VHOST_VSOCK_SET_RUNNING, ...) but there > > was no > > > > way to perform it for any vhost device (and, in particular, vhost-vdpa). > > > > > > > > After the return of ioctl with stop != 0, the device MUST finish any > > > > pending operations like in flight requests. It must also preserve all > > > > the necessary state (the virtqueue vring base plus the possible device > > > > specific states) that is required for restoring in the future. The > > > > device must not change its configuration after that point. > > > > > > > > After the return of ioctl with stop == 0, the device can continue > > > > processing buffers as long as typical conditions are met (vq is enabled, > > > > DRIVER_OK status bit is enabled, etc). > > Just to be clear, we are adding vdpa level new ioctl() that doesn’t map to > any mechanism in the virtio spec. We try to provide forward compatibility to VIRTIO_CONFIG_S_STOP. That means it is expected to implement at least a subset of VIRTIO_CONFIG_S_STOP. > > Why can't we use this ioctl() to indicate driver to start/stop the device > instead of driving it through the driver_ok? So the idea is to add capability that does not exist in the spec. Then came the stop/resume which can't be done via DRIVER_OK. I think we should only allow the stop/resume to succeed after DRIVER_OK is set. > This is in the context of other discussion we had in the LM series. Do you see any issue that blocks the live migration? Thanks ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: Re: [PATCH 3/3] virtio_balloon: Introduce memory recover
On 5/27/22 03:18, Michael S. Tsirkin wrote:
On Fri, May 20, 2022 at 03:06:48PM +0800, zhenwei pi wrote:
Introduce a new queue 'recover VQ', this allows guest to recover
hardware corrupted page:
Guest 5.MF -> 6.RVQ FE10.Unpoison page
/ \/
---+-+--+---
| | |
4.MCE7.RVQ BE 9.RVQ Event
QEMU / \ /
3.SIGBUS 8.Remap
/
+
|
+--2.MF
Host /
1.HW error
The workflow:
1, HardWare page error occurs randomly.
2, host side handles corrupted page by Memory Failure mechanism, sends
SIGBUS to the user process if early-kill is enabled.
3, QEMU handles SIGBUS, if the address belongs to guest RAM, then:
4, QEMU tries to inject MCE into guest.
5, guest handles memory failure again.
1-5 is already supported for a long time, the next steps are supported
in this patch(also related driver patch):
6, guest balloon driver gets noticed of the corrupted PFN, and sends
request to host side by Recover VQ FrontEnd.
7, QEMU handles request from Recover VQ BackEnd, then:
8, QEMU remaps the corrupted HVA fo fix the memory failure, then:
9, QEMU acks the guest side the result by Recover VQ.
10, guest unpoisons the page if the corrupted page gets recoverd
successfully.
Then the guest fixes the HW page error dynamiclly without rebooting.
Emulate MCE by QEMU, the guest works fine:
mce: [Hardware Error]: Machine check events logged
Memory failure: 0x61646: recovery action for dirty LRU page: Recovered
virtio_balloon virtio5: recovered pfn 0x61646
Unpoison: Unpoisoned page 0x61646 by virtio-balloon
MCE: Killing stress:24502 due to hardware memory corruption fault at
7f5be2e5a010
The 'HardwareCorrupted' in /proc/meminfo also shows 0 kB.
Signed-off-by: zhenwei pi
---
drivers/virtio/virtio_balloon.c | 243
include/uapi/linux/virtio_balloon.h | 16 ++
2 files changed, 259 insertions(+)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index f4c34a2a6b8e..f9d95d1d8a4d 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -52,6 +52,7 @@ enum virtio_balloon_vq {
VIRTIO_BALLOON_VQ_STATS,
VIRTIO_BALLOON_VQ_FREE_PAGE,
VIRTIO_BALLOON_VQ_REPORTING,
+ VIRTIO_BALLOON_VQ_RECOVER,
VIRTIO_BALLOON_VQ_MAX
};
@@ -59,6 +60,12 @@ enum virtio_balloon_config_read {
VIRTIO_BALLOON_CONFIG_READ_CMD_ID = 0,
};
+/* the request body to commucate with host side */
+struct __virtio_balloon_recover {
+ struct virtio_balloon_recover vbr;
+ __virtio32 pfns[VIRTIO_BALLOON_PAGES_PER_PAGE];
+};
+
I don't think this idea of passing 32 bit pfns is going to fly.
What is wrong with just passing the pages normally as a s/g list?
this is what is done for the hints at the moment.
neither should you use __virtio types for new functionality
(should all be __le), nor use __virtio for the struct name.
Guest side sends GPA/PFN to host side by passing the pages normally as a
s/g list, this is OK.
But in this scenario, guest also needs to get
status(recovered?corrupted?failed to recover?) of page from the host side.
For a normal page(Ex, 4K), the host could return the status quite
immediately. But for the 2M hugetlb of guest RAM, the host should be
pending until the guest requests 512*4K to recover. Once the 2M hugepage
gets recovered(or failed to recover), the host returns 512 PFNs with
status to guest. There are at most 512 recover requests of a single 2M
huge page.
For example, the guest tries to recover a corrupted page:
struct scatterlist status_sg, page_sg, *sgs[2];
sg_init_one(&status_sg, status, sizeof(*status));
sgs[0] = &status_sg;
p = page_address(page);
sg_init_one(&page_sg, p, PAGE_SIZE);
sgs[1] = &page_sg;
virtqueue_add_sgs(recover_vq, sgs, 1, 1, NULL, GFP_ATOMIC);
The host handles 4K recover request on 2M hugepage, this request is
pending until the full 2M huge page gets recovered(or failed).
To avoid too many pending request in virt queue, I designed as this
patch(should use __le), passing PFN in request body, using a single IN
request only.
...
--- a/include/uapi/linux/virtio_balloon.h
+++ b/include/uapi/linux/virtio_balloon.h
@@ -37,6 +37,7 @@
#define VIRTIO_BALLOON_F_FREE_PAGE_HINT 3 /* VQ to report free pages */
#define VIRTIO_BALLOON_F_PAGE_POISON 4 /* Guest is using page poisoning */
#define VIRTIO_BALLOON_F_REPORTING5 /* Page reporting virtqueue */
+#define VIRTIO_BALLOON_F_RECOVER 6 /* Memory recover virtqueue */
/* Size of a PFN in the balloon interface. */
#define VIRTIO_BALLOON_PFN_SHIFT 12
Please get this feature recorded in the spec with the virtio TC.
They will a
Re: [PATCH] vhost-vdpa: Fix some error handling path in vhost_vdpa_process_iotlb_msg()
On Sun, May 22, 2022 at 03:59:01PM +0200, Christophe JAILLET wrote:
> In the error paths introduced by the commit in the Fixes tag, a mutex may
> be left locked.
> Add the correct goto instead of a direct return.
>
> Fixes: a1468175bb17 ("vhost-vdpa: support ASID based IOTLB API")
> Signed-off-by: Christophe JAILLET
Acked-by: Michael S. Tsirkin
> ---
> WARNING: This patch only fixes the goto vs return mix-up in this function.
> However, the 2nd hunk looks really spurious to me. I think that the:
> - return -EINVAL;
> + r = -EINVAL;
> + goto unlock;
> should be done only in the 'if (!iotlb)' block.
>
> As I don't know this code, I just leave it as-is but draw your attention
> in case this is another bug lurking.
> ---
> drivers/vhost/vdpa.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index 1f1d1c425573..3e86080041fc 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -1000,7 +1000,8 @@ static int vhost_vdpa_process_iotlb_msg(struct
> vhost_dev *dev, u32 asid,
> if (!as) {
> dev_err(&v->dev, "can't find and alloc asid %d\n",
> asid);
> - return -EINVAL;
> + r = -EINVAL;
> + goto unlock;
> }
> iotlb = &as->iotlb;
> } else
> @@ -1013,7 +1014,8 @@ static int vhost_vdpa_process_iotlb_msg(struct
> vhost_dev *dev, u32 asid,
> }
> if (!iotlb)
> dev_err(&v->dev, "no iotlb for asid %d\n", asid);
> - return -EINVAL;
> + r = -EINVAL;
> + goto unlock;
> }
>
> switch (msg->type) {
> --
> 2.34.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 3/3] virtio_balloon: Introduce memory recover
On Fri, May 20, 2022 at 03:06:48PM +0800, zhenwei pi wrote:
> Introduce a new queue 'recover VQ', this allows guest to recover
> hardware corrupted page:
>
> Guest 5.MF -> 6.RVQ FE10.Unpoison page
> / \/
> ---+-+--+---
>| | |
> 4.MCE7.RVQ BE 9.RVQ Event
> QEMU / \ /
> 3.SIGBUS 8.Remap
> /
> +
> |
> +--2.MF
> Host /
>1.HW error
>
> The workflow:
> 1, HardWare page error occurs randomly.
> 2, host side handles corrupted page by Memory Failure mechanism, sends
>SIGBUS to the user process if early-kill is enabled.
> 3, QEMU handles SIGBUS, if the address belongs to guest RAM, then:
> 4, QEMU tries to inject MCE into guest.
> 5, guest handles memory failure again.
>
> 1-5 is already supported for a long time, the next steps are supported
> in this patch(also related driver patch):
> 6, guest balloon driver gets noticed of the corrupted PFN, and sends
>request to host side by Recover VQ FrontEnd.
> 7, QEMU handles request from Recover VQ BackEnd, then:
> 8, QEMU remaps the corrupted HVA fo fix the memory failure, then:
> 9, QEMU acks the guest side the result by Recover VQ.
> 10, guest unpoisons the page if the corrupted page gets recoverd
> successfully.
>
> Then the guest fixes the HW page error dynamiclly without rebooting.
>
> Emulate MCE by QEMU, the guest works fine:
> mce: [Hardware Error]: Machine check events logged
> Memory failure: 0x61646: recovery action for dirty LRU page: Recovered
> virtio_balloon virtio5: recovered pfn 0x61646
> Unpoison: Unpoisoned page 0x61646 by virtio-balloon
> MCE: Killing stress:24502 due to hardware memory corruption fault at
> 7f5be2e5a010
>
> The 'HardwareCorrupted' in /proc/meminfo also shows 0 kB.
>
> Signed-off-by: zhenwei pi
> ---
> drivers/virtio/virtio_balloon.c | 243
> include/uapi/linux/virtio_balloon.h | 16 ++
> 2 files changed, 259 insertions(+)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index f4c34a2a6b8e..f9d95d1d8a4d 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -52,6 +52,7 @@ enum virtio_balloon_vq {
> VIRTIO_BALLOON_VQ_STATS,
> VIRTIO_BALLOON_VQ_FREE_PAGE,
> VIRTIO_BALLOON_VQ_REPORTING,
> + VIRTIO_BALLOON_VQ_RECOVER,
> VIRTIO_BALLOON_VQ_MAX
> };
>
> @@ -59,6 +60,12 @@ enum virtio_balloon_config_read {
> VIRTIO_BALLOON_CONFIG_READ_CMD_ID = 0,
> };
>
> +/* the request body to commucate with host side */
> +struct __virtio_balloon_recover {
> + struct virtio_balloon_recover vbr;
> + __virtio32 pfns[VIRTIO_BALLOON_PAGES_PER_PAGE];
> +};
> +
I don't think this idea of passing 32 bit pfns is going to fly.
What is wrong with just passing the pages normally as a s/g list?
this is what is done for the hints at the moment.
neither should you use __virtio types for new functionality
(should all be __le), nor use __virtio for the struct name.
> struct virtio_balloon {
> struct virtio_device *vdev;
> struct virtqueue *inflate_vq, *deflate_vq, *stats_vq, *free_page_vq;
> @@ -126,6 +133,16 @@ struct virtio_balloon {
> /* Free page reporting device */
> struct virtqueue *reporting_vq;
> struct page_reporting_dev_info pr_dev_info;
> +
> + /* Memory recover VQ - VIRTIO_BALLOON_F_RECOVER */
> + struct virtqueue *recover_vq;
> + spinlock_t recover_vq_lock;
> + struct notifier_block memory_failure_nb;
> + struct list_head corrupted_page_list;
> + struct list_head recovered_page_list;
> + spinlock_t recover_page_list_lock;
> + struct __virtio_balloon_recover in_vbr;
> + struct work_struct unpoison_memory_work;
> };
>
> static const struct virtio_device_id id_table[] = {
> @@ -494,6 +511,198 @@ static void update_balloon_size_func(struct work_struct
> *work)
> queue_work(system_freezable_wq, work);
> }
>
> +/*
> + * virtballoon_memory_failure - notified by memory failure, try to fix the
> + * corrupted page.
> + * The memory failure notifier is designed to call back when the kernel
> handled
> + * successfully only, WARN_ON_ONCE on the unlikely condition to find out any
> + * error(memory error handling is a best effort, not 100% coverd).
covered
> + */
> +static int virtballoon_memory_failure(struct notifier_block *notifier,
> + unsigned long pfn, void *parm)
> +{
> + struct virtio_balloon *vb = container_of(notifier, struct
> virtio_balloon,
> + memory_failure_nb);
> + struct page *page;
> + struct __virtio_balloon_recover *out_vbr
Re: [PATCH v3 2/4] vhost-vdpa: introduce STOP backend feature bit
On Thu, May 26, 2022 at 07:00:06PM +0200, Eugenio Perez Martin wrote: > > It feels like returning any literal that isn't 1 or 0 should trigger a > > warning... I've written that and will check it out tonight. > > > > I'm not sure this should be so strict, or "literal" does not include pointers? > What I mean in exact terms, is that if you're returning a known value and the function returns bool then the known value should be 0 or 1. Don't "return 3;". This new warning will complain if you return a known pointer as in "return &a;". It won't complain if you return an unknown pointer "return p;". > As an experiment, can Smatch be used to count how many times a > returned pointer is converted to int / bool before returning vs not > converted? I'm not super excited to write that code... :/ > > I find Smatch interesting, especially when switching between projects > frequently. Does it support changing the code like clang-format? To > offload cognitive load to tools is usually good :). No. Coccinelle does that really well though. regards, dan carpenter ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 0/3] recover hardware corrupted page by virtio balloon
On Wed, May 25, 2022 at 01:16:34PM -0700, Jue Wang wrote: > The hypervisor _must_ emulate poisons identified in guest physical > address space (could be transported from the source VM), this is to > prevent silent data corruption in the guest. With a paravirtual > approach like this patch series, the hypervisor can clear some of the > poisoned HVAs knowing for certain that the guest OS has isolated the > poisoned page. I wonder how much value it provides to the guest if the > guest and workload are _not_ in a pressing need for the extra KB/MB > worth of memory. I'm curious the same on how unpoisoning could help here. The reasoning behind would be great material to be mentioned in the next cover letter. Shouldn't we consider migrating serious workloads off the host already where there's a sign of more severe hardware issues, instead? Thanks, -- Peter Xu ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [linux-next:master] BUILD REGRESSION 8cb8311e95e3bb58bd84d6350365f14a718faa6d
On Thu, May 26, 2022 at 03:28:25PM +0100, Matthew Wilcox wrote: > On Thu, May 26, 2022 at 11:48:32AM +0300, Dan Carpenter wrote: > > On Thu, May 26, 2022 at 02:16:34AM +0100, Matthew Wilcox wrote: > > > Bizarre this started showing up now. The recent patch was: > > > > > > - info->alloced += compound_nr(page); > > > - inode->i_blocks += BLOCKS_PER_PAGE << compound_order(page); > > > + info->alloced += folio_nr_pages(folio); > > > + inode->i_blocks += BLOCKS_PER_PAGE << folio_order(folio); > > > > > > so it could tell that compound_order() was small, but folio_order() > > > might be large? > > > > The old code also generates a warning on my test system. Smatch thinks > > both compound_order() and folio_order() are 0-255. I guess because of > > the "unsigned char compound_order;" in the struct page. > > It'd be nice if we could annotate that as "contains a value between > 1 and BITS_PER_LONG - PAGE_SHIFT". Then be able to optionally enable > a checker that ensures that's true on loads/stores. Maybe we need a > language that isn't C :-P Ada can do this ... I don't think Rust can. Machine Parsable Comments. It's a matter of figuring out the best format and writing the code. In Smatch, I have table of hard coded return values in the format: https://github.com/error27/smatch/blob/master/smatch_data/db/kernel.return_fixes I don't have code to handle something like BITS_PER_LONG or PAGE_SHIFT. To be honest, Smatch code always assumes that PAGE_SIZE is 4096 but I should actually look it up... It's not impossible to do. The GFP_KERNEL values changed enough so that I eventually made that look up the actual defines. I also have a table in the database where I could edit the values of (struct page)->compound_order. regards, dan carpenter ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [linux-next:master] BUILD REGRESSION 8cb8311e95e3bb58bd84d6350365f14a718faa6d
On Thu, May 26, 2022 at 11:48:32AM +0300, Dan Carpenter wrote: > On Thu, May 26, 2022 at 02:16:34AM +0100, Matthew Wilcox wrote: > > Bizarre this started showing up now. The recent patch was: > > > > - info->alloced += compound_nr(page); > > - inode->i_blocks += BLOCKS_PER_PAGE << compound_order(page); > > + info->alloced += folio_nr_pages(folio); > > + inode->i_blocks += BLOCKS_PER_PAGE << folio_order(folio); > > > > so it could tell that compound_order() was small, but folio_order() > > might be large? > > The old code also generates a warning on my test system. Smatch thinks > both compound_order() and folio_order() are 0-255. I guess because of > the "unsigned char compound_order;" in the struct page. It'd be nice if we could annotate that as "contains a value between 1 and BITS_PER_LONG - PAGE_SHIFT". Then be able to optionally enable a checker that ensures that's true on loads/stores. Maybe we need a language that isn't C :-P Ada can do this ... I don't think Rust can. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v4 4/4] vdpa_sim: Implement stop vdpa op
On Thu, May 26, 2022 at 02:43:38PM +0200, Eugenio Pérez wrote:
Implement stop operation for vdpa_sim devices, so vhost-vdpa will offer
that backend feature and userspace can effectively stop the device.
This is a must before get virtqueue indexes (base) for live migration,
since the device could modify them after userland gets them. There are
individual ways to perform that action for some devices
(VHOST_NET_SET_BACKEND, VHOST_VSOCK_SET_RUNNING, ...) but there was no
way to perform it for any vhost device (and, in particular, vhost-vdpa).
Signed-off-by: Eugenio Pérez
---
drivers/vdpa/vdpa_sim/vdpa_sim.c | 21 +
drivers/vdpa/vdpa_sim/vdpa_sim.h | 1 +
drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 3 +++
drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 3 +++
4 files changed, 28 insertions(+)
Reviewed-by: Stefano Garzarella
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
index 50d721072beb..0515cf314bed 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
@@ -107,6 +107,7 @@ static void vdpasim_do_reset(struct vdpasim *vdpasim)
for (i = 0; i < vdpasim->dev_attr.nas; i++)
vhost_iotlb_reset(&vdpasim->iommu[i]);
+ vdpasim->running = true;
spin_unlock(&vdpasim->iommu_lock);
vdpasim->features = 0;
@@ -505,6 +506,24 @@ static int vdpasim_reset(struct vdpa_device *vdpa)
return 0;
}
+static int vdpasim_stop(struct vdpa_device *vdpa, bool stop)
+{
+ struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
+ int i;
+
+ spin_lock(&vdpasim->lock);
+ vdpasim->running = !stop;
+ if (vdpasim->running) {
+ /* Check for missed buffers */
+ for (i = 0; i < vdpasim->dev_attr.nvqs; ++i)
+ vdpasim_kick_vq(vdpa, i);
+
+ }
+ spin_unlock(&vdpasim->lock);
+
+ return 0;
+}
+
static size_t vdpasim_get_config_size(struct vdpa_device *vdpa)
{
struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
@@ -694,6 +713,7 @@ static const struct vdpa_config_ops vdpasim_config_ops = {
.get_status = vdpasim_get_status,
.set_status = vdpasim_set_status,
.reset = vdpasim_reset,
+ .stop = vdpasim_stop,
.get_config_size= vdpasim_get_config_size,
.get_config = vdpasim_get_config,
.set_config = vdpasim_set_config,
@@ -726,6 +746,7 @@ static const struct vdpa_config_ops
vdpasim_batch_config_ops = {
.get_status = vdpasim_get_status,
.set_status = vdpasim_set_status,
.reset = vdpasim_reset,
+ .stop = vdpasim_stop,
.get_config_size= vdpasim_get_config_size,
.get_config = vdpasim_get_config,
.set_config = vdpasim_set_config,
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.h b/drivers/vdpa/vdpa_sim/vdpa_sim.h
index 622782e92239..061986f30911 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim.h
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.h
@@ -66,6 +66,7 @@ struct vdpasim {
u32 generation;
u64 features;
u32 groups;
+ bool running;
/* spinlock to synchronize iommu table */
spinlock_t iommu_lock;
};
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
index 42d401d43911..bcdb1982c378 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
@@ -204,6 +204,9 @@ static void vdpasim_blk_work(struct work_struct *work)
if (!(vdpasim->status & VIRTIO_CONFIG_S_DRIVER_OK))
goto out;
+ if (!vdpasim->running)
+ goto out;
+
for (i = 0; i < VDPASIM_BLK_VQ_NUM; i++) {
struct vdpasim_virtqueue *vq = &vdpasim->vqs[i];
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
index 5125976a4df8..886449e88502 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
@@ -154,6 +154,9 @@ static void vdpasim_net_work(struct work_struct *work)
spin_lock(&vdpasim->lock);
+ if (!vdpasim->running)
+ goto out;
+
if (!(vdpasim->status & VIRTIO_CONFIG_S_DRIVER_OK))
goto out;
--
2.31.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v4 1/4] vdpa: Add stop operation
On Thu, May 26, 2022 at 02:43:35PM +0200, Eugenio Pérez wrote:
This operation is optional: It it's not implemented, backend feature bit
will not be exposed.
Signed-off-by: Eugenio Pérez
---
include/linux/vdpa.h | 6 ++
1 file changed, 6 insertions(+)
diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
index 15af802d41c4..ddfebc4e1e01 100644
--- a/include/linux/vdpa.h
+++ b/include/linux/vdpa.h
@@ -215,6 +215,11 @@ struct vdpa_map_file {
* @reset: Reset device
* @vdev: vdpa device
* Returns integer: success (0) or error (< 0)
+ * @stop: Stop or resume the device (optional, but it must
+ * be implemented if require device stop)
+ * @vdev: vdpa device
+ * @stop: stop (true), not stop (false)
Sorry for just seeing this now, but if you have to send a v5, maybe we
could use "resume" here instead of "not stop".
Thanks,
Stefano
+ * Returns integer: success (0) or error (< 0)
* @get_config_size:Get the size of the configuration space includes
* fields that are conditional on feature bits.
* @vdev: vdpa device
@@ -316,6 +321,7 @@ struct vdpa_config_ops {
u8 (*get_status)(struct vdpa_device *vdev);
void (*set_status)(struct vdpa_device *vdev, u8 status);
int (*reset)(struct vdpa_device *vdev);
+ int (*stop)(struct vdpa_device *vdev, bool stop);
size_t (*get_config_size)(struct vdpa_device *vdev);
void (*get_config)(struct vdpa_device *vdev, unsigned int offset,
void *buf, unsigned int len);
--
2.31.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
RE: [PATCH 9/9] crypto: Introduce RSA algorithm
> -Original Message-
> From: Lei He [mailto:helei.si...@bytedance.com]
> Sent: Wednesday, May 25, 2022 5:01 PM
> To: m...@redhat.com; Gonglei (Arei) ;
> berra...@redhat.com
> Cc: qemu-de...@nongnu.org; virtualization@lists.linux-foundation.org;
> linux-cry...@vger.kernel.org; jasow...@redhat.com; coh...@redhat.com;
> pizhen...@bytedance.com; helei.si...@bytedance.com
> Subject: [PATCH 9/9] crypto: Introduce RSA algorithm
>
> From: zhenwei pi
>
> There are two parts in this patch:
> 1, support akcipher service by cryptodev-builtin driver 2, virtio-crypto
> driver
> supports akcipher service
>
> In principle, we should separate this into two patches, to avoid compiling
> error,
> merge them into one.
>
> 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=20
>
> 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
>
> Signed-off-by: zhenwei pi
> Signed-off-by: lei he ---
> backends/cryptodev-builtin.c | 272
> +++-
> backends/cryptodev-vhost-user.c | 34 +++-
> backends/cryptodev.c | 32 ++--
> hw/virtio/virtio-crypto.c | 323
> ++
> include/hw/virtio/virtio-crypto.h | 5 +-
> include/sysemu/cryptodev.h| 83 --
> 6 files changed, 604 insertions(+), 145 deletions(-)
>
> diff --git a/backends/cryptodev-builtin.c b/backends/cryptodev-builtin.c index
> 0671bf9f3e..388aedd8df 100644
> --- a/backends/cryptodev-builtin.c
> +++ b/backends/cryptodev-builtin.c
> @@ -26,6 +26,7 @@
> #include "qapi/error.h"
> #include "standard-headers/linux/virtio_crypto.h"
> #include "crypto/cipher.h"
> +#include "crypto/akcipher.h"
> #include "qom/object.h"
>
>
> @@ -41,11 +42,12 @@
> OBJECT_DECLARE_SIMPLE_TYPE(CryptoDevBackendBuiltin,
> CRYPTODEV_BACKEND_BUILTIN) typedef struct
> CryptoDevBackendBuiltinSession {
> QCryptoCipher *cipher;
> uint8_t direction; /* encryption or decryption */
> -uint8_t type; /* cipher? hash? aead? */
> +uint8_t type; /* cipher? hash? aead? akcipher? */
Do you actually use the type for akcipher?
> +QCryptoAkCipher *akcipher;
> QTAILQ_ENTRY(CryptoDevBackendBuiltinSession) next; }
> CryptoDevBackendBuiltinSession;
>
> -/* Max number of symmetric sessions */
> +/* Max number of symmetric/asymmetric sessions */
> #define MAX_NUM_SESSIONS 256
>
> #define CRYPTODEV_BUITLIN_MAX_AUTH_KEY_LEN512
> @@ -80,15 +82,17 @@ static void cryptodev_builtin_init(
> backend->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 << VIRTI
Re: [PATCH v3 2/4] vhost-vdpa: introduce STOP backend feature bit
On Thu, May 26, 2022 at 02:44:02PM +0200, Eugenio Perez Martin wrote:
> > >> +static bool vhost_vdpa_can_stop(const struct vhost_vdpa *v) {
> > >> + struct vdpa_device *vdpa = v->vdpa;
> > >> + const struct vdpa_config_ops *ops = vdpa->config;
> > >> +
> > >> + return ops->stop;
> > >> [GD>>] Would it be better to explicitly return a bool to match the
> > >> return type?
> > >
> > >I'm not sure about the kernel code style regarding that casting. Maybe
> > >it's better to return !!ops->stop here. The macros likely and unlikely
> > >do that.
> >
> > IIUC `ops->stop` is a function pointer, so what about
> >
> > return ops->stop != NULL;
> >
>
> I'm ok with any method proposed. Both three ways can be found in the
> kernel so I think they are all valid (although the double negation is
> from bool to integer in (0,1) set actually).
>
> Maybe Jason or Michael (as maintainers) can state the preferred method here.
Always just do whatever the person who responded feels like because
they're likely the person who cares the most. ;)
I don't think there are any static analysis tools which will complain
about this. Smatch will complain if you return a negative literal.
It feels like returning any literal that isn't 1 or 0 should trigger a
warning... I've written that and will check it out tonight.
Really anything negative should trigger a warning. See new Smatch stuff
below.
regards,
dan carpenter
TEST CASE =
int x;
_Bool one(int *p)
{
if (p)
x = -2;
return x;
}
_Bool two(int *p)
{
return -4; // returning 2 triggers a warning now
}
=== OUTPUT =
test.c:10 one() warn: potential negative cast to bool 'x'
test.c:14 two() warn: signedness bug returning '(-4)'
test.c:14 two() warn: '(-4)' is not bool
=== CODE ===
#include "smatch.h"
#include "smatch_extra.h"
#include "smatch_slist.h"
static int my_id;
static void match_literals(struct expression *ret_value)
{
struct symbol *type;
sval_t sval;
type = cur_func_return_type();
if (!type || sval_type_max(type).value != 1)
return;
if (!get_implied_value(ret_value, &sval))
return;
if (sval.value == 0 || sval.value == 1)
return;
sm_warning("'%s' is not bool", sval_to_str(sval));
}
static void match_any_negative(struct expression *ret_value)
{
struct symbol *type;
struct sm_state *extra, *sm;
sval_t sval;
char *name;
type = cur_func_return_type();
if (!type || sval_type_max(type).value != 1)
return;
extra = get_extra_sm_state(ret_value);
if (!extra)
return;
FOR_EACH_PTR(extra->possible, sm) {
if (estate_get_single_value(sm->state, &sval) &&
sval_is_negative(sval)) {
name = expr_to_str(ret_value);
sm_warning("potential negative cast to bool '%s'",
name);
free_string(name);
return;
}
} END_FOR_EACH_PTR(sm);
}
void check_bool_return(int id)
{
my_id = id;
add_hook(&match_literals, RETURN_HOOK);
add_hook(&match_any_negative, RETURN_HOOK);
}
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
RE: [PATCH v4 0/4] Implement vdpasim stop operation
> From: Eugenio Pérez > Sent: Thursday, May 26, 2022 8:44 AM > Implement stop operation for vdpa_sim devices, so vhost-vdpa will offer > > that backend feature and userspace can effectively stop the device. > > > > This is a must before get virtqueue indexes (base) for live migration, > > since the device could modify them after userland gets them. There are > > individual ways to perform that action for some devices > > (VHOST_NET_SET_BACKEND, VHOST_VSOCK_SET_RUNNING, ...) but there > was no > > way to perform it for any vhost device (and, in particular, vhost-vdpa). > > > > After the return of ioctl with stop != 0, the device MUST finish any > > pending operations like in flight requests. It must also preserve all > > the necessary state (the virtqueue vring base plus the possible device > > specific states) that is required for restoring in the future. The > > device must not change its configuration after that point. > > > > After the return of ioctl with stop == 0, the device can continue > > processing buffers as long as typical conditions are met (vq is enabled, > > DRIVER_OK status bit is enabled, etc). Just to be clear, we are adding vdpa level new ioctl() that doesn’t map to any mechanism in the virtio spec. Why can't we use this ioctl() to indicate driver to start/stop the device instead of driving it through the driver_ok? This is in the context of other discussion we had in the LM series. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH v2 0/5] TUN/VirtioNet USO features support.
I'll check it, thank you! On Thu, May 26, 2022 at 9:56 AM Jason Wang wrote: > > On Tue, May 24, 2022 at 7:07 PM Andrew Melnichenko wrote: > > > > Hi all, > > > > The issue is that host segments packets between guests on the same host. > > Tests show that it happens because SKB_GSO_DODGY skb offload in > > virtio_net_hdr_from_skb(). > > To do segmentation you need to remove SKB_GSO_DODGY or add SKB_GSO_PARTIAL > > The solution with DODGY/PARTIAL offload looks like a dirty hack, so > > for now, I've lived it as it is for further investigation. > > Ok, I managed to find the previous discussion. It looks to me the > reason is that __udp_gso_segment will segment dodgy packets > unconditionally. > > I wonder if the attached patch works? (compile test only). > > Thanks > > > > > > > On Tue, May 17, 2022 at 9:32 AM Jason Wang wrote: > > > > > > On Thu, May 12, 2022 at 7:33 PM Andrew Melnychenko > > > wrote: > > > > > > > > Added new offloads for TUN devices TUN_F_USO4 and TUN_F_USO6. > > > > Technically they enable NETIF_F_GSO_UDP_L4 > > > > (and only if USO4 & USO6 are set simultaneously). > > > > It allows to transmission of large UDP packets. > > > > > > > > Different features USO4 and USO6 are required for qemu where Windows > > > > guests can > > > > enable disable USO receives for IPv4 and IPv6 separately. > > > > On the other side, Linux can't really differentiate USO4 and USO6, for > > > > now. > > > > For now, to enable USO for TUN it requires enabling USO4 and USO6 > > > > together. > > > > In the future, there would be a mechanism to control UDP_L4 GSO > > > > separately. > > > > > > > > Test it WIP Qemu https://github.com/daynix/qemu/tree/Dev_USOv2 > > > > > > > > New types for VirtioNet already on mailing: > > > > https://lists.oasis-open.org/archives/virtio-comment/202110/msg00010.html > > > > > > > > Also, there is a known issue with transmitting packages between two > > > > guests. > > > > > > Could you explain this more? It looks like a bug. (Or any pointer to > > > the discussion) > > > > > > Thanks > > > > > > > Without hacks with skb's GSO - packages are still segmented on the > > > > host's postrouting. > > > > > > > > Andrew (5): > > > > uapi/linux/if_tun.h: Added new offload types for USO4/6. > > > > driver/net/tun: Added features for USO. > > > > uapi/linux/virtio_net.h: Added USO types. > > > > linux/virtio_net.h: Support USO offload in vnet header. > > > > drivers/net/virtio_net.c: Added USO support. > > > > > > > > drivers/net/tap.c | 10 -- > > > > drivers/net/tun.c | 8 +++- > > > > drivers/net/virtio_net.c| 19 +++ > > > > include/linux/virtio_net.h | 9 + > > > > include/uapi/linux/if_tun.h | 2 ++ > > > > include/uapi/linux/virtio_net.h | 4 > > > > 6 files changed, 45 insertions(+), 7 deletions(-) > > > > > > > > -- > > > > 2.35.1 > > > > > > > > > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: Re: [PATCH v7 0/9] Introduce akcipher service for virtio-crypto
Hi, Daniel
Thanks a lot for your review!
On 5/26/22 18:48, Daniel P. Berrangé wrote:
I've sent a pull request containing all the crypto/ changes,
as that covers stuff I maintain. ie patches 2-8
Patches 1 and 9, I'll leave for MST to review & queue since the
virtual hardware is not my area of knowledge.
On Wed, May 25, 2022 at 05:01:09PM +0800, Lei He wrote:
v6 -> v7:
- Fix serval build errors for some specific platforms/configurations.
- Use '%zu' instead of '%lu' for size_t parameters.
- AkCipher-gcrypt: avoid setting wrong error messages when parsing RSA
keys.
- AkCipher-benchmark: process constant amount of sign/verify instead
of running sign/verify for a constant duration.
v5 -> v6:
- Fix build errors and codestyles.
- Add parameter 'Error **errp' for qcrypto_akcipher_rsakey_parse.
- Report more detailed errors.
- Fix buffer length check and return values of akcipher-nettle, allows caller to
pass a buffer with larger size than actual needed.
A million thanks to Daniel!
v4 -> v5:
- Move QCryptoAkCipher into akcipherpriv.h, and modify the related comments.
- Rename asn1_decoder.c to der.c.
- Code style fix: use 'cleanup' & 'error' lables.
- Allow autoptr type to auto-free.
- Add test cases for rsakey to handle DER error.
- Other minor fixes.
v3 -> v4:
- Coding style fix: Akcipher -> AkCipher, struct XXX -> XXX, Rsa -> RSA,
XXX-alg -> XXX-algo.
- Change version info in qapi/crypto.json, from 7.0 -> 7.1.
- Remove ecdsa from qapi/crypto.json, it would be introduced with the
implemetion later.
- Use QCryptoHashAlgothrim instead of QCryptoRSAHashAlgorithm(removed) in
qapi/crypto.json.
- Rename arguments of qcrypto_akcipher_XXX to keep aligned with
qcrypto_cipher_XXX(dec/enc/sign/vefiry -> in/out/in2), and add
qcrypto_akcipher_max_XXX APIs.
- Add new API: qcrypto_akcipher_supports.
- Change the return value of qcrypto_akcipher_enc/dec/sign, these functions
return the actual length of result.
- Separate ASN.1 source code and test case clean.
- Disable RSA raw encoding for akcipher-nettle.
- Separate RSA key parser into rsakey.{hc}, and implememts it with
builtin-asn1-decoder and nettle respectivly.
- Implement RSA(pkcs1 and raw encoding) algorithm by gcrypt. This has higher
priority than nettle.
- For some akcipher operations(eg, decryption of pkcs1pad(rsa)), the length of
returned result maybe less than the dst buffer size, return the actual length
of result instead of the buffer length to the guest side. (in function
virtio_crypto_akcipher_input_data_helper)
- Other minor changes.
Thanks to Daniel!
Eric pointed out this missing part of use case, send it here again.
In our plan, the feature is designed for HTTPS offloading case and other
applications which use kernel RSA/ecdsa by keyctl syscall. The full picture
shows bellow:
Nginx/openssl[1] ... Apps
Guest -
virtio-crypto driver[2]
-
virtio-crypto backend[3]
Host-
/ | \
builtin[4] vhost keyctl[5] ...
[1] User applications can offload RSA calculation to kernel by keyctl syscall.
There is no keyctl engine in openssl currently, we developed a engine and tried
to contribute it to openssl upstream, but openssl 1.x does not accept new
feature. Link:
https://github.com/openssl/openssl/pull/16689
This branch is available and maintained by Lei
https://github.com/TousakaRin/openssl/tree/OpenSSL_1_1_1-kctl_engine
We tested nginx(change config file only) with openssl keyctl engine, it works
fine.
[2] virtio-crypto driver is used to communicate with host side, send requests
to host side to do asymmetric calculation.
https://lkml.org/lkml/2022/3/1/1425
[3] virtio-crypto backend handles requests from guest side, and forwards
request to crypto backend driver of QEMU.
[4] Currently RSA is supported only in builtin driver. This driver is supposed
to test the full feature without other software(Ex vhost process) and hardware
dependence. ecdsa is introduced into qapi type without implementation, this may
be implemented in Q3-2022 or later. If ecdsa type definition should be added
with the implementation together, I'll remove this in next version.
[5] keyctl backend is in development, we will post this feature in Q2-2022.
keyctl backend can use hardware acceleration(Ex, Intel QAT).
Setup the full environment, tested with Intel QAT on host side, the QPS of
HTTPS increase to ~200% in a guest.
VS PCI passthrough: the most important benefit of this solution makes the VM
migratable.
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 ve
RE: [PATCH v7 0/9] Introduce akcipher service for virtio-crypto
> -Original Message-
> From: Daniel P. Berrangé [mailto:berra...@redhat.com]
> Sent: Thursday, May 26, 2022 6:48 PM
> To: Lei He
> Cc: m...@redhat.com; Gonglei (Arei) ;
> qemu-de...@nongnu.org; virtualization@lists.linux-foundation.org;
> linux-cry...@vger.kernel.org; jasow...@redhat.com; coh...@redhat.com;
> pizhen...@bytedance.com
> Subject: Re: [PATCH v7 0/9] Introduce akcipher service for virtio-crypto
>
> I've sent a pull request containing all the crypto/ changes, as that covers
> stuff I
> maintain. ie patches 2-8
>
> Patches 1 and 9, I'll leave for MST to review & queue since the virtual
> hardware
> is not my area of knowledge.
>
Thanks for your work, Daniel.
Regards,
-Gonglei
> On Wed, May 25, 2022 at 05:01:09PM +0800, Lei He wrote:
> > v6 -> v7:
> > - Fix serval build errors for some specific platforms/configurations.
> > - Use '%zu' instead of '%lu' for size_t parameters.
> > - AkCipher-gcrypt: avoid setting wrong error messages when parsing RSA
> > keys.
> > - AkCipher-benchmark: process constant amount of sign/verify instead
> > of running sign/verify for a constant duration.
> >
> > v5 -> v6:
> > - Fix build errors and codestyles.
> > - Add parameter 'Error **errp' for qcrypto_akcipher_rsakey_parse.
> > - Report more detailed errors.
> > - Fix buffer length check and return values of akcipher-nettle, allows
> > caller to pass a buffer with larger size than actual needed.
> >
> > A million thanks to Daniel!
> >
> > v4 -> v5:
> > - Move QCryptoAkCipher into akcipherpriv.h, and modify the related
> comments.
> > - Rename asn1_decoder.c to der.c.
> > - Code style fix: use 'cleanup' & 'error' lables.
> > - Allow autoptr type to auto-free.
> > - Add test cases for rsakey to handle DER error.
> > - Other minor fixes.
> >
> > v3 -> v4:
> > - Coding style fix: Akcipher -> AkCipher, struct XXX -> XXX, Rsa ->
> > RSA, XXX-alg -> XXX-algo.
> > - Change version info in qapi/crypto.json, from 7.0 -> 7.1.
> > - Remove ecdsa from qapi/crypto.json, it would be introduced with the
> implemetion later.
> > - Use QCryptoHashAlgothrim instead of QCryptoRSAHashAlgorithm(removed)
> in qapi/crypto.json.
> > - Rename arguments of qcrypto_akcipher_XXX to keep aligned with
> qcrypto_cipher_XXX(dec/enc/sign/vefiry -> in/out/in2), and add
> qcrypto_akcipher_max_XXX APIs.
> > - Add new API: qcrypto_akcipher_supports.
> > - Change the return value of qcrypto_akcipher_enc/dec/sign, these functions
> return the actual length of result.
> > - Separate ASN.1 source code and test case clean.
> > - Disable RSA raw encoding for akcipher-nettle.
> > - Separate RSA key parser into rsakey.{hc}, and implememts it with
> builtin-asn1-decoder and nettle respectivly.
> > - Implement RSA(pkcs1 and raw encoding) algorithm by gcrypt. This has
> higher priority than nettle.
> > - For some akcipher operations(eg, decryption of pkcs1pad(rsa)), the
> > length of returned result maybe less than the dst buffer size, return
> > the actual length of result instead of the buffer length to the guest
> > side. (in function virtio_crypto_akcipher_input_data_helper)
> > - Other minor changes.
> >
> > Thanks to Daniel!
> >
> > Eric pointed out this missing part of use case, send it here again.
> >
> > In our plan, the feature is designed for HTTPS offloading case and other
> applications which use kernel RSA/ecdsa by keyctl syscall. The full picture
> shows bellow:
> >
> >
> > Nginx/openssl[1] ... Apps
> > Guest -
> > virtio-crypto driver[2]
> > -
> > virtio-crypto backend[3]
> > Host-
> > / | \
> > builtin[4] vhost keyctl[5] ...
> >
> >
> > [1] User applications can offload RSA calculation to kernel by keyctl
> > syscall.
> There is no keyctl engine in openssl currently, we developed a engine and
> tried
> to contribute it to openssl upstream, but openssl 1.x does not accept new
> feature. Link:
> >https://github.com/openssl/openssl/pull/16689
> >
> > This branch is available and maintained by Lei
> >
> > https://github.com/TousakaRin/openssl/tree/OpenSSL_1_1_1-kctl_engine
> >
> > We tested nginx(change config file only) with openssl keyctl engine, it
> > works
> fine.
> >
> > [2] virtio-crypto driver is used to communicate with host side, send
> > requests
> to host side to do asymmetric calculation.
> >https://lkml.org/lkml/2022/3/1/1425
> >
> > [3] virtio-crypto backend handles requests from guest side, and forwards
> request to crypto backend driver of QEMU.
> >
> > [4] Currently RSA is supported only in builtin driver. This driver is
> > supposed to
> test the full feature without other software(Ex vhost process) and hardware
> dependence. ecdsa is introduced into qapi type without implementation, this
> may be implemented in Q3-2022 or later. If ecdsa type definition should
Re: [PATCH v7 0/9] Introduce akcipher service for virtio-crypto
I've sent a pull request containing all the crypto/ changes,
as that covers stuff I maintain. ie patches 2-8
Patches 1 and 9, I'll leave for MST to review & queue since the
virtual hardware is not my area of knowledge.
On Wed, May 25, 2022 at 05:01:09PM +0800, Lei He wrote:
> v6 -> v7:
> - Fix serval build errors for some specific platforms/configurations.
> - Use '%zu' instead of '%lu' for size_t parameters.
> - AkCipher-gcrypt: avoid setting wrong error messages when parsing RSA
> keys.
> - AkCipher-benchmark: process constant amount of sign/verify instead
> of running sign/verify for a constant duration.
>
> v5 -> v6:
> - Fix build errors and codestyles.
> - Add parameter 'Error **errp' for qcrypto_akcipher_rsakey_parse.
> - Report more detailed errors.
> - Fix buffer length check and return values of akcipher-nettle, allows caller
> to
> pass a buffer with larger size than actual needed.
>
> A million thanks to Daniel!
>
> v4 -> v5:
> - Move QCryptoAkCipher into akcipherpriv.h, and modify the related comments.
> - Rename asn1_decoder.c to der.c.
> - Code style fix: use 'cleanup' & 'error' lables.
> - Allow autoptr type to auto-free.
> - Add test cases for rsakey to handle DER error.
> - Other minor fixes.
>
> v3 -> v4:
> - Coding style fix: Akcipher -> AkCipher, struct XXX -> XXX, Rsa -> RSA,
> XXX-alg -> XXX-algo.
> - Change version info in qapi/crypto.json, from 7.0 -> 7.1.
> - Remove ecdsa from qapi/crypto.json, it would be introduced with the
> implemetion later.
> - Use QCryptoHashAlgothrim instead of QCryptoRSAHashAlgorithm(removed) in
> qapi/crypto.json.
> - Rename arguments of qcrypto_akcipher_XXX to keep aligned with
> qcrypto_cipher_XXX(dec/enc/sign/vefiry -> in/out/in2), and add
> qcrypto_akcipher_max_XXX APIs.
> - Add new API: qcrypto_akcipher_supports.
> - Change the return value of qcrypto_akcipher_enc/dec/sign, these functions
> return the actual length of result.
> - Separate ASN.1 source code and test case clean.
> - Disable RSA raw encoding for akcipher-nettle.
> - Separate RSA key parser into rsakey.{hc}, and implememts it with
> builtin-asn1-decoder and nettle respectivly.
> - Implement RSA(pkcs1 and raw encoding) algorithm by gcrypt. This has higher
> priority than nettle.
> - For some akcipher operations(eg, decryption of pkcs1pad(rsa)), the length
> of returned result maybe less than the dst buffer size, return the actual
> length of result instead of the buffer length to the guest side. (in function
> virtio_crypto_akcipher_input_data_helper)
> - Other minor changes.
>
> Thanks to Daniel!
>
> Eric pointed out this missing part of use case, send it here again.
>
> In our plan, the feature is designed for HTTPS offloading case and other
> applications which use kernel RSA/ecdsa by keyctl syscall. The full picture
> shows bellow:
>
>
> Nginx/openssl[1] ... Apps
> Guest -
> virtio-crypto driver[2]
> -
> virtio-crypto backend[3]
> Host-
> / | \
> builtin[4] vhost keyctl[5] ...
>
>
> [1] User applications can offload RSA calculation to kernel by keyctl
> syscall. There is no keyctl engine in openssl currently, we developed a
> engine and tried to contribute it to openssl upstream, but openssl 1.x does
> not accept new feature. Link:
>https://github.com/openssl/openssl/pull/16689
>
> This branch is available and maintained by Lei
>https://github.com/TousakaRin/openssl/tree/OpenSSL_1_1_1-kctl_engine
>
> We tested nginx(change config file only) with openssl keyctl engine, it works
> fine.
>
> [2] virtio-crypto driver is used to communicate with host side, send requests
> to host side to do asymmetric calculation.
>https://lkml.org/lkml/2022/3/1/1425
>
> [3] virtio-crypto backend handles requests from guest side, and forwards
> request to crypto backend driver of QEMU.
>
> [4] Currently RSA is supported only in builtin driver. This driver is
> supposed to test the full feature without other software(Ex vhost process)
> and hardware dependence. ecdsa is introduced into qapi type without
> implementation, this may be implemented in Q3-2022 or later. If ecdsa type
> definition should be added with the implementation together, I'll remove this
> in next version.
>
> [5] keyctl backend is in development, we will post this feature in Q2-2022.
> keyctl backend can use hardware acceleration(Ex, Intel QAT).
>
> Setup the full environment, tested with Intel QAT on host side, the QPS of
> HTTPS increase to ~200% in a guest.
>
> VS PCI passthrough: the most important benefit of this solution makes the VM
> migratable.
>
> v2 -> v3:
> - Introduce akcipher types to qapi
> - Add test/benchmark suite for akcipher class
> - Seperate 'virtio_crypto: Support virtio crypto asym operation' into:
> - crypto:
Re: [PATCH v3 2/4] vhost-vdpa: introduce STOP backend feature bit
On Thu, May 26, 2022 at 10:57:03AM +0200, Eugenio Perez Martin wrote:
On Wed, May 25, 2022 at 1:23 PM Dawar, Gautam wrote:
[AMD Official Use Only - General]
-Original Message-
From: Eugenio Pérez
Sent: Wednesday, May 25, 2022 4:29 PM
To: Michael S. Tsirkin ; net...@vger.kernel.org;
linux-ker...@vger.kernel.org; k...@vger.kernel.org;
virtualization@lists.linux-foundation.org; Jason Wang
Cc: Zhu Lingshan ; mart...@xilinx.com; Stefano Garzarella ; ecree.xil...@gmail.com; Eli Cohen
; Dan Carpenter ; Parav Pandit ; Wu Zongyong
; din...@xilinx.com; Christophe JAILLET ; Xie Yongji
; Dawar, Gautam ; l...@redhat.com; marti...@xilinx.com; pab...@xilinx.com; Longpeng
; piotr.umin...@intel.com; Kamde, Tanuj ; Si-Wei Liu ;
habetsm.xil...@gmail.com; lviv...@redhat.com; Zhang Min ; han...@xilinx.com
Subject: [PATCH v3 2/4] vhost-vdpa: introduce STOP backend feature bit
[CAUTION: External Email]
Userland knows if it can stop the device or not by checking this feature bit.
It's only offered if the vdpa driver backend implements the stop() operation
callback, and try to set it if the backend does not offer that callback is an
error.
Signed-off-by: Eugenio Pérez
---
drivers/vhost/vdpa.c | 16 +++-
include/uapi/linux/vhost_types.h | 2 ++
2 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c index
1f1d1c425573..32713db5831d 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -347,6 +347,14 @@ static long vhost_vdpa_set_config(struct vhost_vdpa *v,
return 0;
}
+static bool vhost_vdpa_can_stop(const struct vhost_vdpa *v) {
+ struct vdpa_device *vdpa = v->vdpa;
+ const struct vdpa_config_ops *ops = vdpa->config;
+
+ return ops->stop;
[GD>>] Would it be better to explicitly return a bool to match the return type?
I'm not sure about the kernel code style regarding that casting. Maybe
it's better to return !!ops->stop here. The macros likely and unlikely
do that.
IIUC `ops->stop` is a function pointer, so what about
return ops->stop != NULL;
Thanks,
Stefano
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [linux-next:master] BUILD REGRESSION 8cb8311e95e3bb58bd84d6350365f14a718faa6d
On Thu, May 26, 2022 at 02:16:34AM +0100, Matthew Wilcox wrote: > Bizarre this started showing up now. The recent patch was: > > - info->alloced += compound_nr(page); > - inode->i_blocks += BLOCKS_PER_PAGE << compound_order(page); > + info->alloced += folio_nr_pages(folio); > + inode->i_blocks += BLOCKS_PER_PAGE << folio_order(folio); > > so it could tell that compound_order() was small, but folio_order() > might be large? The old code also generates a warning on my test system. Smatch thinks both compound_order() and folio_order() are 0-255. I guess because of the "unsigned char compound_order;" in the struct page. regards, dan carpenter ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [linux-next:master] BUILD REGRESSION 8cb8311e95e3bb58bd84d6350365f14a718faa6d
On Wed, May 25, 2022 at 11:35 PM kernel test robot wrote: > .__mulsi3.o.cmd: No such file or directory > Makefile:686: arch/h8300/Makefile: No such file or directory > Makefile:765: arch/h8300/Makefile: No such file or directory > arch/Kconfig:10: can't open file "arch/h8300/Kconfig" Please stop building h8300 after the asm-generic tree is merged, the architecture is getting removed. Arnd ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [linux-next:master] BUILD REGRESSION 8cb8311e95e3bb58bd84d6350365f14a718faa6d
On Wed, May 25, 2022 at 02:50:56PM -0700, Andrew Morton wrote: > On Thu, 26 May 2022 05:35:20 +0800 kernel test robot wrote: > > > tree/branch: > > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master > > branch HEAD: 8cb8311e95e3bb58bd84d6350365f14a718faa6d Add linux-next > > specific files for 20220525 > > > > Error/Warning reports: > > > > ... > > > > Unverified Error/Warning (likely false positive, please contact us if > > interested): > > Could be so. > > > mm/shmem.c:1948 shmem_getpage_gfp() warn: should '(((1) << 12) / 512) << > > folio_order(folio)' be a 64 bit type? > > I've been seeing this one for a while. And from this report I can't > figure out what tool emitted it. Clang? This is a Smatch warning. I normally look over Smatch warnings before forwarding kbuild-bot emails but this email is a grab bag of static checker warnings from different tools. This warning has a high rate of false positives so I'm going to disable it by default. > > > > > ... > > > > |-- i386-randconfig-m021 > > | `-- > > mm-shmem.c-shmem_getpage_gfp()-warn:should-((()-)-)-folio_order(folio)-be-a-bit-type > > If you're going to use randconfig then shouldn't you make the config > available? Or maybe quote the KCONFIG_SEED - presumably there's a way > for others to regenerate. > > Anyway, the warning seems wrong to me. > > > #define PAGE_SIZE (_AC(1,UL) << PAGE_SHIFT) > > #define BLOCKS_PER_PAGE (PAGE_SIZE/512) > > inode->i_blocks += BLOCKS_PER_PAGE << folio_order(folio); > > so the RHS here should have unsigned long type. Being able to generate > the cpp output would be helpful. That requires the .config. The heuristic is that "inode->i_blocks" is a u64 but this .config must be for a 32bit CPU. I'm just going to turn off all these warnings until I can figure out a better heuristic. regards, dan carpenter ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
