In GICv4.1, migration has been supported except for (directly-injected)
VLPI. And GICv4.1 spec explicitly gives a way to get the VLPI's pending
state (which was crucially missing in GICv4.0). So we make VLPI migration
capable on GICv4.1 in this patch set.
In order to support VLPI migration, we need to save and restore all
required configuration information and pending states of VLPIs. But
in fact, the configuration information of VLPIs has already been saved
(or will be reallocated on the dst host...) in vgic(kvm) migration.
So we only have to migrate the pending states of VLPIs specially.
Below is the related workflow in migration.
On the save path:
In migration completion:
pause all vCPUs
|
call each VM state change handler:
pause other devices (just keep from sending interrupts, and
such as VFIO migration protocol has already realized it [1])
|
flush ITS tables into guest RAM
|
flush RDIST pending tables (also flush VLPI state here)
|
...
On the resume path:
load each device's state:
restore ITS tables (include pending tables) from guest RAM
|
for other (PCI) devices (paused), if configured to have VLPIs,
establish the forwarding paths of their VLPIs (and transfer
the pending states from kvm's vgic to VPT here)
Yet TODO:
- For some reason, such as for VFIO PCI devices, there may be repeated
resettings of HW VLPI configuration in load_state, resulting in the
loss of pending state. A very intuitive solution is to retrieve the
pending state in unset_forwarding (and this should be so regardless
of migration). But at normal run time, this function may be called
when all devices are running, in which case the unmapping of VPE is
not allowed. It seems to be an almost insoluble bug...
There are other possible solutions as follows:
1) avoid unset_forwarding being called from QEMU in resuming (simply
allocate all needed vectors first), which is more reasonable and
efficient.
2) add a new dedicated interface to transfer these pending states to
HW in GIC VM state change handler corresponding to save_pending_tables.
...
Any comments and suggestions are very welcome.
Besides, we have tested this series in VFIO migration, and nothing else
goes wrong (with two issues committed [2][3]).
Links:
[1] vfio: UAPI for migration interface for device state:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/
commit/?id=a8a24f3f6e38103b77cf399c38eb54e1219d00d6
[2] vfio: Move the saving of the config space to the right place in VFIO migration:
https://patchwork.ozlabs.org/patch/1400246/
[3] vfio: Set the priority of VFIO VM state change handler explicitly:
https://patchwork.ozlabs.org/patch/1401280/
Shenming Lu (2):
KVM: arm64: GICv4.1: Try to save hw pending state in
save_pending_tables
KVM: arm64: GICv4.1: Give a chance to save VLPI's pending state
Zenghui Yu (2):
irqchip/gic-v4.1: Plumb get_irqchip_state VLPI callback
KVM: arm64: GICv4.1: Restore VLPI's pending state to physical side
.../virt/kvm/devices/arm-vgic-its.rst | 2 +-
arch/arm64/kvm/vgic/vgic-its.c | 6 +-
arch/arm64/kvm/vgic/vgic-v3.c | 62 +++++++++++++++++--
arch/arm64/kvm/vgic/vgic-v4.c | 12 ++++
drivers/irqchip/irq-gic-v3-its.c | 38 ++++++++++++
5 files changed, 110 insertions(+), 10 deletions(-)
--
2.23.0
After pausing all vCPUs and devices capable of interrupting, in order
to save the information of all interrupts, besides flushing the pending
states in kvm’s vgic, we also try to flush the states of VLPIs in the
virtual pending tables into guest RAM, but we need to have GICv4.1 and
safely unmap the vPEs first.
Signed-off-by: Shenming Lu <[email protected]>
---
arch/arm64/kvm/vgic/vgic-v3.c | 62 +++++++++++++++++++++++++++++++----
1 file changed, 56 insertions(+), 6 deletions(-)
diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
index 9cdf39a94a63..e1b3aa4b2b12 100644
--- a/arch/arm64/kvm/vgic/vgic-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-v3.c
@@ -1,6 +1,8 @@
// SPDX-License-Identifier: GPL-2.0-only
#include <linux/irqchip/arm-gic-v3.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
#include <linux/kvm.h>
#include <linux/kvm_host.h>
#include <kvm/arm_vgic.h>
@@ -356,6 +358,39 @@ int vgic_v3_lpi_sync_pending_status(struct kvm *kvm, struct vgic_irq *irq)
return 0;
}
+/*
+ * With GICv4.1, we can get the VLPI's pending state after unmapping
+ * the vPE. The deactivation of the doorbell interrupt will trigger
+ * the unmapping of the associated vPE.
+ */
+static void get_vlpi_state_pre(struct vgic_dist *dist)
+{
+ struct irq_desc *desc;
+ int i;
+
+ if (!kvm_vgic_global_state.has_gicv4_1)
+ return;
+
+ for (i = 0; i < dist->its_vm.nr_vpes; i++) {
+ desc = irq_to_desc(dist->its_vm.vpes[i]->irq);
+ irq_domain_deactivate_irq(irq_desc_get_irq_data(desc));
+ }
+}
+
+static void get_vlpi_state_post(struct vgic_dist *dist)
+{
+ struct irq_desc *desc;
+ int i;
+
+ if (!kvm_vgic_global_state.has_gicv4_1)
+ return;
+
+ for (i = 0; i < dist->its_vm.nr_vpes; i++) {
+ desc = irq_to_desc(dist->its_vm.vpes[i]->irq);
+ irq_domain_activate_irq(irq_desc_get_irq_data(desc), false);
+ }
+}
+
/**
* vgic_v3_save_pending_tables - Save the pending tables into guest RAM
* kvm lock and all vcpu lock must be held
@@ -365,14 +400,17 @@ int vgic_v3_save_pending_tables(struct kvm *kvm)
struct vgic_dist *dist = &kvm->arch.vgic;
struct vgic_irq *irq;
gpa_t last_ptr = ~(gpa_t)0;
- int ret;
+ int ret = 0;
u8 val;
+ get_vlpi_state_pre(dist);
+
list_for_each_entry(irq, &dist->lpi_list_head, lpi_list) {
int byte_offset, bit_nr;
struct kvm_vcpu *vcpu;
gpa_t pendbase, ptr;
bool stored;
+ bool is_pending = irq->pending_latch;
vcpu = irq->target_vcpu;
if (!vcpu)
@@ -387,24 +425,36 @@ int vgic_v3_save_pending_tables(struct kvm *kvm)
if (ptr != last_ptr) {
ret = kvm_read_guest_lock(kvm, ptr, &val, 1);
if (ret)
- return ret;
+ goto out;
last_ptr = ptr;
}
stored = val & (1U << bit_nr);
- if (stored == irq->pending_latch)
+
+ /* also flush hw pending state */
+ if (irq->hw) {
+ WARN_RATELIMIT(irq_get_irqchip_state(irq->host_irq,
+ IRQCHIP_STATE_PENDING, &is_pending),
+ "IRQ %d", irq->host_irq);
+ }
+
+ if (stored == is_pending)
continue;
- if (irq->pending_latch)
+ if (is_pending)
val |= 1 << bit_nr;
else
val &= ~(1 << bit_nr);
ret = kvm_write_guest_lock(kvm, ptr, &val, 1);
if (ret)
- return ret;
+ goto out;
}
- return 0;
+
+out:
+ get_vlpi_state_post(dist);
+
+ return ret;
}
/**
--
2.23.0
Before GICv4.1, we do not have direct access to the VLPI's pending
state. So we simply let it fail early when encountering any VLPI.
But now we don't have to return -EACCES directly if on GICv4.1. So
let’s change the hard code and give a chance to save the VLPI's pending
state (and preserve the interfaces).
Signed-off-by: Shenming Lu <[email protected]>
---
Documentation/virt/kvm/devices/arm-vgic-its.rst | 2 +-
arch/arm64/kvm/vgic/vgic-its.c | 6 +++---
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/Documentation/virt/kvm/devices/arm-vgic-its.rst b/Documentation/virt/kvm/devices/arm-vgic-its.rst
index 6c304fd2b1b4..d257eddbae29 100644
--- a/Documentation/virt/kvm/devices/arm-vgic-its.rst
+++ b/Documentation/virt/kvm/devices/arm-vgic-its.rst
@@ -80,7 +80,7 @@ KVM_DEV_ARM_VGIC_GRP_CTRL
-EFAULT Invalid guest ram access
-EBUSY One or more VCPUS are running
-EACCES The virtual ITS is backed by a physical GICv4 ITS, and the
- state is not available
+ state is not available without GICv4.1
======= ==========================================================
KVM_DEV_ARM_VGIC_GRP_ITS_REGS
diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
index 40cbaca81333..ec7543a9617c 100644
--- a/arch/arm64/kvm/vgic/vgic-its.c
+++ b/arch/arm64/kvm/vgic/vgic-its.c
@@ -2218,10 +2218,10 @@ static int vgic_its_save_itt(struct vgic_its *its, struct its_device *device)
/*
* If an LPI carries the HW bit, this means that this
* interrupt is controlled by GICv4, and we do not
- * have direct access to that state. Let's simply fail
- * the save operation...
+ * have direct access to that state without GICv4.1.
+ * Let's simply fail the save operation...
*/
- if (ite->irq->hw)
+ if (ite->irq->hw && !kvm_vgic_global_state.has_gicv4_1)
return -EACCES;
ret = vgic_its_save_ite(its, device, ite, gpa, ite_esz);
--
2.23.0
From: Zenghui Yu <[email protected]>
When setting the forwarding path of a VLPI, it is more consistent to
also transfer the pending state from irq->pending_latch to VPT (especially
in migration, the pending states of VLPIs are restored into kvm’s vgic
first). And we currently send "INT+VSYNC" to trigger a VLPI to pending.
Signed-off-by: Zenghui Yu <[email protected]>
Signed-off-by: Shenming Lu <[email protected]>
---
arch/arm64/kvm/vgic/vgic-v4.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/arch/arm64/kvm/vgic/vgic-v4.c b/arch/arm64/kvm/vgic/vgic-v4.c
index b5fa73c9fd35..cc3ab9cea182 100644
--- a/arch/arm64/kvm/vgic/vgic-v4.c
+++ b/arch/arm64/kvm/vgic/vgic-v4.c
@@ -418,6 +418,18 @@ int kvm_vgic_v4_set_forwarding(struct kvm *kvm, int virq,
irq->host_irq = virq;
atomic_inc(&map.vpe->vlpi_count);
+ /* Transfer pending state */
+ ret = irq_set_irqchip_state(irq->host_irq,
+ IRQCHIP_STATE_PENDING,
+ irq->pending_latch);
+ WARN_RATELIMIT(ret, "IRQ %d", irq->host_irq);
+
+ /*
+ * Let it be pruned from ap_list later and don't bother
+ * the List Register.
+ */
+ irq->pending_latch = false;
+
out:
mutex_unlock(&its->its_lock);
return ret;
--
2.23.0
On 2020-11-23 06:54, Shenming Lu wrote:
> After pausing all vCPUs and devices capable of interrupting, in order
^^^^^^^^^^^^^^^^^
See my comment below about this.
> to save the information of all interrupts, besides flushing the pending
> states in kvm’s vgic, we also try to flush the states of VLPIs in the
> virtual pending tables into guest RAM, but we need to have GICv4.1 and
> safely unmap the vPEs first.
>
> Signed-off-by: Shenming Lu <[email protected]>
> ---
> arch/arm64/kvm/vgic/vgic-v3.c | 62 +++++++++++++++++++++++++++++++----
> 1 file changed, 56 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c
> b/arch/arm64/kvm/vgic/vgic-v3.c
> index 9cdf39a94a63..e1b3aa4b2b12 100644
> --- a/arch/arm64/kvm/vgic/vgic-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-v3.c
> @@ -1,6 +1,8 @@
> // SPDX-License-Identifier: GPL-2.0-only
>
> #include <linux/irqchip/arm-gic-v3.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> #include <linux/kvm.h>
> #include <linux/kvm_host.h>
> #include <kvm/arm_vgic.h>
> @@ -356,6 +358,39 @@ int vgic_v3_lpi_sync_pending_status(struct kvm
> *kvm, struct vgic_irq *irq)
> return 0;
> }
>
> +/*
> + * With GICv4.1, we can get the VLPI's pending state after unmapping
> + * the vPE. The deactivation of the doorbell interrupt will trigger
> + * the unmapping of the associated vPE.
> + */
> +static void get_vlpi_state_pre(struct vgic_dist *dist)
> +{
> + struct irq_desc *desc;
> + int i;
> +
> + if (!kvm_vgic_global_state.has_gicv4_1)
> + return;
> +
> + for (i = 0; i < dist->its_vm.nr_vpes; i++) {
> + desc = irq_to_desc(dist->its_vm.vpes[i]->irq);
> + irq_domain_deactivate_irq(irq_desc_get_irq_data(desc));
> + }
> +}
> +
> +static void get_vlpi_state_post(struct vgic_dist *dist)
nit: the naming feels a bit... odd. Pre/post what?
> +{
> + struct irq_desc *desc;
> + int i;
> +
> + if (!kvm_vgic_global_state.has_gicv4_1)
> + return;
> +
> + for (i = 0; i < dist->its_vm.nr_vpes; i++) {
> + desc = irq_to_desc(dist->its_vm.vpes[i]->irq);
> + irq_domain_activate_irq(irq_desc_get_irq_data(desc), false);
> + }
> +}
> +
> /**
> * vgic_v3_save_pending_tables - Save the pending tables into guest
> RAM
> * kvm lock and all vcpu lock must be held
> @@ -365,14 +400,17 @@ int vgic_v3_save_pending_tables(struct kvm *kvm)
> struct vgic_dist *dist = &kvm->arch.vgic;
> struct vgic_irq *irq;
> gpa_t last_ptr = ~(gpa_t)0;
> - int ret;
> + int ret = 0;
> u8 val;
>
> + get_vlpi_state_pre(dist);
> +
> list_for_each_entry(irq, &dist->lpi_list_head, lpi_list) {
> int byte_offset, bit_nr;
> struct kvm_vcpu *vcpu;
> gpa_t pendbase, ptr;
> bool stored;
> + bool is_pending = irq->pending_latch;
>
> vcpu = irq->target_vcpu;
> if (!vcpu)
> @@ -387,24 +425,36 @@ int vgic_v3_save_pending_tables(struct kvm *kvm)
> if (ptr != last_ptr) {
> ret = kvm_read_guest_lock(kvm, ptr, &val, 1);
> if (ret)
> - return ret;
> + goto out;
> last_ptr = ptr;
> }
>
> stored = val & (1U << bit_nr);
> - if (stored == irq->pending_latch)
> +
> + /* also flush hw pending state */
This comment looks out of place, as we aren't flushing anything.
> + if (irq->hw) {
> + WARN_RATELIMIT(irq_get_irqchip_state(irq->host_irq,
> + IRQCHIP_STATE_PENDING, &is_pending),
> + "IRQ %d", irq->host_irq);
Isn't this going to warn like mad on a GICv4.0 system where this, by
definition,
will generate an error?
> + }
> +
> + if (stored == is_pending)
> continue;
>
> - if (irq->pending_latch)
> + if (is_pending)
> val |= 1 << bit_nr;
> else
> val &= ~(1 << bit_nr);
>
> ret = kvm_write_guest_lock(kvm, ptr, &val, 1);
> if (ret)
> - return ret;
> + goto out;
> }
> - return 0;
> +
> +out:
> + get_vlpi_state_post(dist);
This bit worries me: you have unmapped the VPEs, so any interrupt that
has been
generated during that phase is now forever lost (the GIC doesn't have
ownership
of the pending tables).
Do you really expect the VM to be restartable from that point? I don't
see how
this is possible.
> +
> + return ret;
> }
>
> /**
Thanks,
M.
--
Jazz is not dead. It just smells funny...
On 2020-11-23 06:54, Shenming Lu wrote:
> From: Zenghui Yu <[email protected]>
>
> When setting the forwarding path of a VLPI, it is more consistent to
I'm not sure it is more consistent. It is a *new* behaviour, because it
only
matters for migration, which has been so far unsupported.
> also transfer the pending state from irq->pending_latch to VPT
> (especially
> in migration, the pending states of VLPIs are restored into kvm’s vgic
> first). And we currently send "INT+VSYNC" to trigger a VLPI to pending.
>
> Signed-off-by: Zenghui Yu <[email protected]>
> Signed-off-by: Shenming Lu <[email protected]>
> ---
> arch/arm64/kvm/vgic/vgic-v4.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/arch/arm64/kvm/vgic/vgic-v4.c
> b/arch/arm64/kvm/vgic/vgic-v4.c
> index b5fa73c9fd35..cc3ab9cea182 100644
> --- a/arch/arm64/kvm/vgic/vgic-v4.c
> +++ b/arch/arm64/kvm/vgic/vgic-v4.c
> @@ -418,6 +418,18 @@ int kvm_vgic_v4_set_forwarding(struct kvm *kvm,
> int virq,
> irq->host_irq = virq;
> atomic_inc(&map.vpe->vlpi_count);
>
> + /* Transfer pending state */
> + ret = irq_set_irqchip_state(irq->host_irq,
> + IRQCHIP_STATE_PENDING,
> + irq->pending_latch);
> + WARN_RATELIMIT(ret, "IRQ %d", irq->host_irq);
> +
> + /*
> + * Let it be pruned from ap_list later and don't bother
> + * the List Register.
> + */
> + irq->pending_latch = false;
It occurs to me that calling into irq_set_irqchip_state() for a large
number of interrupts can take a significant amount of time. It is also
odd that you dump the VPT with the VPE unmapped, but rely on the VPE
being mapped for the opposite operation.
Shouldn't these be symmetric, all performed while the VPE is unmapped?
It would also save a lot of ITS traffic.
> +
> out:
> mutex_unlock(&its->its_lock);
> return ret;
Thanks,
M.
--
Jazz is not dead. It just smells funny...
On 2020/11/23 17:18, Marc Zyngier wrote:
> On 2020-11-23 06:54, Shenming Lu wrote:
>> After pausing all vCPUs and devices capable of interrupting, in order
> ^^^^^^^^^^^^^^^^^
> See my comment below about this.
>
>> to save the information of all interrupts, besides flushing the pending
>> states in kvm’s vgic, we also try to flush the states of VLPIs in the
>> virtual pending tables into guest RAM, but we need to have GICv4.1 and
>> safely unmap the vPEs first.
>>
>> Signed-off-by: Shenming Lu <[email protected]>
>> ---
>> arch/arm64/kvm/vgic/vgic-v3.c | 62 +++++++++++++++++++++++++++++++----
>> 1 file changed, 56 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
>> index 9cdf39a94a63..e1b3aa4b2b12 100644
>> --- a/arch/arm64/kvm/vgic/vgic-v3.c
>> +++ b/arch/arm64/kvm/vgic/vgic-v3.c
>> @@ -1,6 +1,8 @@
>> // SPDX-License-Identifier: GPL-2.0-only
>>
>> #include <linux/irqchip/arm-gic-v3.h>
>> +#include <linux/irq.h>
>> +#include <linux/irqdomain.h>
>> #include <linux/kvm.h>
>> #include <linux/kvm_host.h>
>> #include <kvm/arm_vgic.h>
>> @@ -356,6 +358,39 @@ int vgic_v3_lpi_sync_pending_status(struct kvm
>> *kvm, struct vgic_irq *irq)
>> return 0;
>> }
>>
>> +/*
>> + * With GICv4.1, we can get the VLPI's pending state after unmapping
>> + * the vPE. The deactivation of the doorbell interrupt will trigger
>> + * the unmapping of the associated vPE.
>> + */
>> +static void get_vlpi_state_pre(struct vgic_dist *dist)
>> +{
>> + struct irq_desc *desc;
>> + int i;
>> +
>> + if (!kvm_vgic_global_state.has_gicv4_1)
>> + return;
>> +
>> + for (i = 0; i < dist->its_vm.nr_vpes; i++) {
>> + desc = irq_to_desc(dist->its_vm.vpes[i]->irq);
>> + irq_domain_deactivate_irq(irq_desc_get_irq_data(desc));
>> + }
>> +}
>> +
>> +static void get_vlpi_state_post(struct vgic_dist *dist)
>
> nit: the naming feels a bit... odd. Pre/post what?
My understanding is that the unmapping is a preparation for get_vlpi_state...
Maybe just call it unmap/map_all_vpes?
>
>> +{
>> + struct irq_desc *desc;
>> + int i;
>> +
>> + if (!kvm_vgic_global_state.has_gicv4_1)
>> + return;
>> +
>> + for (i = 0; i < dist->its_vm.nr_vpes; i++) {
>> + desc = irq_to_desc(dist->its_vm.vpes[i]->irq);
>> + irq_domain_activate_irq(irq_desc_get_irq_data(desc), false);
>> + }
>> +}
>> +
>> /**
>> * vgic_v3_save_pending_tables - Save the pending tables into guest RAM
>> * kvm lock and all vcpu lock must be held
>> @@ -365,14 +400,17 @@ int vgic_v3_save_pending_tables(struct kvm *kvm)
>> struct vgic_dist *dist = &kvm->arch.vgic;
>> struct vgic_irq *irq;
>> gpa_t last_ptr = ~(gpa_t)0;
>> - int ret;
>> + int ret = 0;
>> u8 val;
>>
>> + get_vlpi_state_pre(dist);
>> +
>> list_for_each_entry(irq, &dist->lpi_list_head, lpi_list) {
>> int byte_offset, bit_nr;
>> struct kvm_vcpu *vcpu;
>> gpa_t pendbase, ptr;
>> bool stored;
>> + bool is_pending = irq->pending_latch;
>>
>> vcpu = irq->target_vcpu;
>> if (!vcpu)
>> @@ -387,24 +425,36 @@ int vgic_v3_save_pending_tables(struct kvm *kvm)
>> if (ptr != last_ptr) {
>> ret = kvm_read_guest_lock(kvm, ptr, &val, 1);
>> if (ret)
>> - return ret;
>> + goto out;
>> last_ptr = ptr;
>> }
>>
>> stored = val & (1U << bit_nr);
>> - if (stored == irq->pending_latch)
>> +
>> + /* also flush hw pending state */
>
> This comment looks out of place, as we aren't flushing anything.
Ok, I will correct it.
>
>> + if (irq->hw) {
>> + WARN_RATELIMIT(irq_get_irqchip_state(irq->host_irq,
>> + IRQCHIP_STATE_PENDING, &is_pending),
>> + "IRQ %d", irq->host_irq);
>
> Isn't this going to warn like mad on a GICv4.0 system where this, by definition,
> will generate an error?
As we have returned an error in save_its_tables if hw && !has_gicv4_1, we don't
have to warn this here?
>
>> + }
>> +
>> + if (stored == is_pending)
>> continue;
>>
>> - if (irq->pending_latch)
>> + if (is_pending)
>> val |= 1 << bit_nr;
>> else
>> val &= ~(1 << bit_nr);
>>
>> ret = kvm_write_guest_lock(kvm, ptr, &val, 1);
>> if (ret)
>> - return ret;
>> + goto out;
>> }
>> - return 0;
>> +
>> +out:
>> + get_vlpi_state_post(dist);
>
> This bit worries me: you have unmapped the VPEs, so any interrupt that has been
> generated during that phase is now forever lost (the GIC doesn't have ownership
> of the pending tables).
In my opinion, during this phase, the devices capable of interrupting should have
already been paused (prevent from sending interrupts), such as VFIO migration protocol
has already realized it.
>
> Do you really expect the VM to be restartable from that point? I don't see how
> this is possible.
>
If the migration has encountered an error, the src VM might be restarted, so we have to
map the vPEs back.
>> +
>> + return ret;
>> }
>>
>> /**
>
> Thanks,
>
> M.
Thanks,
Shenming
On 2020/11/23 17:27, Marc Zyngier wrote:
> On 2020-11-23 06:54, Shenming Lu wrote:
>> From: Zenghui Yu <[email protected]>
>>
>> When setting the forwarding path of a VLPI, it is more consistent to
>
> I'm not sure it is more consistent. It is a *new* behaviour, because it only
> matters for migration, which has been so far unsupported.
Alright, consistent may not be accurate...
But I have doubt that whether there is really no need to transfer the pending states
from kvm'vgic to VPT in set_forwarding regardless of migration, and the similar
for unset_forwarding.
>
>> also transfer the pending state from irq->pending_latch to VPT (especially
>> in migration, the pending states of VLPIs are restored into kvm’s vgic
>> first). And we currently send "INT+VSYNC" to trigger a VLPI to pending.
>>
>> Signed-off-by: Zenghui Yu <[email protected]>
>> Signed-off-by: Shenming Lu <[email protected]>
>> ---
>> arch/arm64/kvm/vgic/vgic-v4.c | 12 ++++++++++++
>> 1 file changed, 12 insertions(+)
>>
>> diff --git a/arch/arm64/kvm/vgic/vgic-v4.c b/arch/arm64/kvm/vgic/vgic-v4.c
>> index b5fa73c9fd35..cc3ab9cea182 100644
>> --- a/arch/arm64/kvm/vgic/vgic-v4.c
>> +++ b/arch/arm64/kvm/vgic/vgic-v4.c
>> @@ -418,6 +418,18 @@ int kvm_vgic_v4_set_forwarding(struct kvm *kvm, int virq,
>> irq->host_irq = virq;
>> atomic_inc(&map.vpe->vlpi_count);
>>
>> + /* Transfer pending state */
>> + ret = irq_set_irqchip_state(irq->host_irq,
>> + IRQCHIP_STATE_PENDING,
>> + irq->pending_latch);
>> + WARN_RATELIMIT(ret, "IRQ %d", irq->host_irq);
>> +
>> + /*
>> + * Let it be pruned from ap_list later and don't bother
>> + * the List Register.
>> + */
>> + irq->pending_latch = false;
>
> It occurs to me that calling into irq_set_irqchip_state() for a large
> number of interrupts can take a significant amount of time. It is also
> odd that you dump the VPT with the VPE unmapped, but rely on the VPE
> being mapped for the opposite operation.
>
> Shouldn't these be symmetric, all performed while the VPE is unmapped?
> It would also save a lot of ITS traffic.
>
My thought was to use the existing interface directly without unmapping...
If you want to unmap the vPE and poke the VPT here, as I said in the cover
letter, set/unset_forwarding might also be called when all devices are running
at normal run time, in which case the unmapping of the vPE is not allowed...
Another possible solution is to add a new dedicated interface to QEMU to transfer
these pending states to HW in GIC VM state change handler corresponding to
save_pending_tables?
>> +
>> out:
>> mutex_unlock(&its->its_lock);
>> return ret;
>
> Thanks,
>
> M.
On 2020-11-24 07:40, Shenming Lu wrote:
> On 2020/11/23 17:18, Marc Zyngier wrote:
>> On 2020-11-23 06:54, Shenming Lu wrote:
>>> After pausing all vCPUs and devices capable of interrupting, in order
>> ^^^^^^^^^^^^^^^^^
>> See my comment below about this.
>>
>>> to save the information of all interrupts, besides flushing the
>>> pending
>>> states in kvm’s vgic, we also try to flush the states of VLPIs in the
>>> virtual pending tables into guest RAM, but we need to have GICv4.1
>>> and
>>> safely unmap the vPEs first.
>>>
>>> Signed-off-by: Shenming Lu <[email protected]>
>>> ---
>>> arch/arm64/kvm/vgic/vgic-v3.c | 62
>>> +++++++++++++++++++++++++++++++----
>>> 1 file changed, 56 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c
>>> b/arch/arm64/kvm/vgic/vgic-v3.c
>>> index 9cdf39a94a63..e1b3aa4b2b12 100644
>>> --- a/arch/arm64/kvm/vgic/vgic-v3.c
>>> +++ b/arch/arm64/kvm/vgic/vgic-v3.c
>>> @@ -1,6 +1,8 @@
>>> // SPDX-License-Identifier: GPL-2.0-only
>>>
>>> #include <linux/irqchip/arm-gic-v3.h>
>>> +#include <linux/irq.h>
>>> +#include <linux/irqdomain.h>
>>> #include <linux/kvm.h>
>>> #include <linux/kvm_host.h>
>>> #include <kvm/arm_vgic.h>
>>> @@ -356,6 +358,39 @@ int vgic_v3_lpi_sync_pending_status(struct kvm
>>> *kvm, struct vgic_irq *irq)
>>> return 0;
>>> }
>>>
>>> +/*
>>> + * With GICv4.1, we can get the VLPI's pending state after unmapping
>>> + * the vPE. The deactivation of the doorbell interrupt will trigger
>>> + * the unmapping of the associated vPE.
>>> + */
>>> +static void get_vlpi_state_pre(struct vgic_dist *dist)
>>> +{
>>> + struct irq_desc *desc;
>>> + int i;
>>> +
>>> + if (!kvm_vgic_global_state.has_gicv4_1)
>>> + return;
>>> +
>>> + for (i = 0; i < dist->its_vm.nr_vpes; i++) {
>>> + desc = irq_to_desc(dist->its_vm.vpes[i]->irq);
>>> + irq_domain_deactivate_irq(irq_desc_get_irq_data(desc));
>>> + }
>>> +}
>>> +
>>> +static void get_vlpi_state_post(struct vgic_dist *dist)
>>
>> nit: the naming feels a bit... odd. Pre/post what?
>
> My understanding is that the unmapping is a preparation for
> get_vlpi_state...
> Maybe just call it unmap/map_all_vpes?
Yes, much better.
[...]
>>> + if (irq->hw) {
>>> + WARN_RATELIMIT(irq_get_irqchip_state(irq->host_irq,
>>> + IRQCHIP_STATE_PENDING, &is_pending),
>>> + "IRQ %d", irq->host_irq);
>>
>> Isn't this going to warn like mad on a GICv4.0 system where this, by
>> definition,
>> will generate an error?
>
> As we have returned an error in save_its_tables if hw && !has_gicv4_1,
> we don't
> have to warn this here?
Are you referring to the check in vgic_its_save_itt() that occurs in
patch 4?
Fair enough, though I think the use of irq_get_irqchip_state() isn't
quite
what we want, as per my comments on patch #1.
>>
>>> + }
>>> +
>>> + if (stored == is_pending)
>>> continue;
>>>
>>> - if (irq->pending_latch)
>>> + if (is_pending)
>>> val |= 1 << bit_nr;
>>> else
>>> val &= ~(1 << bit_nr);
>>>
>>> ret = kvm_write_guest_lock(kvm, ptr, &val, 1);
>>> if (ret)
>>> - return ret;
>>> + goto out;
>>> }
>>> - return 0;
>>> +
>>> +out:
>>> + get_vlpi_state_post(dist);
>>
>> This bit worries me: you have unmapped the VPEs, so any interrupt that
>> has been
>> generated during that phase is now forever lost (the GIC doesn't have
>> ownership
>> of the pending tables).
>
> In my opinion, during this phase, the devices capable of interrupting
> should have already been paused (prevent from sending interrupts),
> such as VFIO migration protocol has already realized it.
Is that a hard guarantee? Pausing devices *may* be possible for a
limited
set of endpoints, but I'm not sure that is universally possible to
restart
them and expect a consistent state (you have just dropped a bunch of
network
packets on the floor...).
>> Do you really expect the VM to be restartable from that point? I don't
>> see how
>> this is possible.
>>
>
> If the migration has encountered an error, the src VM might be
> restarted, so we have to map the vPEs back.
As I said above, I doubt it is universally possible to do so, but
after all, this probably isn't worse that restarting on the target...
M.
--
Jazz is not dead. It just smells funny...
On 2020/11/24 16:44, Marc Zyngier wrote:
> On 2020-11-24 08:10, Shenming Lu wrote:
>> On 2020/11/23 17:27, Marc Zyngier wrote:
>>> On 2020-11-23 06:54, Shenming Lu wrote:
>>>> From: Zenghui Yu <[email protected]>
>>>>
>>>> When setting the forwarding path of a VLPI, it is more consistent to
>>>
>>> I'm not sure it is more consistent. It is a *new* behaviour, because it only
>>> matters for migration, which has been so far unsupported.
>>
>> Alright, consistent may not be accurate...
>> But I have doubt that whether there is really no need to transfer the
>> pending states
>> from kvm'vgic to VPT in set_forwarding regardless of migration, and the similar
>> for unset_forwarding.
>
> If you have to transfer that state outside of the a save/restore, it means that
> you have missed the programming of the PCI endpoint. This is an established
> restriction that the MSI programming must occur *after* the translation has
> been established using MAPI/MAPTI (see the large comment at the beginning of
> vgic-v4.c).
>
> If you want to revisit this, fair enough. But you will need a lot more than
> just opportunistically transfer the pending state.
Thanks, I will look at what you mentioned.
>
>>
>>>
>>>> also transfer the pending state from irq->pending_latch to VPT (especially
>>>> in migration, the pending states of VLPIs are restored into kvm’s vgic
>>>> first). And we currently send "INT+VSYNC" to trigger a VLPI to pending.
>>>>
>>>> Signed-off-by: Zenghui Yu <[email protected]>
>>>> Signed-off-by: Shenming Lu <[email protected]>
>>>> ---
>>>> arch/arm64/kvm/vgic/vgic-v4.c | 12 ++++++++++++
>>>> 1 file changed, 12 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/kvm/vgic/vgic-v4.c b/arch/arm64/kvm/vgic/vgic-v4.c
>>>> index b5fa73c9fd35..cc3ab9cea182 100644
>>>> --- a/arch/arm64/kvm/vgic/vgic-v4.c
>>>> +++ b/arch/arm64/kvm/vgic/vgic-v4.c
>>>> @@ -418,6 +418,18 @@ int kvm_vgic_v4_set_forwarding(struct kvm *kvm, int virq,
>>>> irq->host_irq = virq;
>>>> atomic_inc(&map.vpe->vlpi_count);
>>>>
>>>> + /* Transfer pending state */
>>>> + ret = irq_set_irqchip_state(irq->host_irq,
>>>> + IRQCHIP_STATE_PENDING,
>>>> + irq->pending_latch);
>>>> + WARN_RATELIMIT(ret, "IRQ %d", irq->host_irq);
>>>> +
>>>> + /*
>>>> + * Let it be pruned from ap_list later and don't bother
>>>> + * the List Register.
>>>> + */
>>>> + irq->pending_latch = false;
>>>
>>> It occurs to me that calling into irq_set_irqchip_state() for a large
>>> number of interrupts can take a significant amount of time. It is also
>>> odd that you dump the VPT with the VPE unmapped, but rely on the VPE
>>> being mapped for the opposite operation.
>>>
>>> Shouldn't these be symmetric, all performed while the VPE is unmapped?
>>> It would also save a lot of ITS traffic.
>>>
>>
>> My thought was to use the existing interface directly without unmapping...
>>
>> If you want to unmap the vPE and poke the VPT here, as I said in the cover
>> letter, set/unset_forwarding might also be called when all devices are running
>> at normal run time, in which case the unmapping of the vPE is not allowed...
>
> No, I'm suggesting that you don't do anything here, but instead as a by-product
> of restoring the ITS tables. What goes wrong if you use the
> KVM_DEV_ARM_ITS_RESTORE_TABLE backend instead?
There is an issue if we do it in the restoring of the ITS tables: the transferring
of the pending state needs the irq to be marked as hw before, which is done by the
pass-through device, but the configuring of the forwarding path of the VLPI depends
on the restoring of the vgic first... It is a circular dependency.
>
>> Another possible solution is to add a new dedicated interface to QEMU
>> to transfer
>> these pending states to HW in GIC VM state change handler corresponding to
>> save_pending_tables?
>
> Userspace has no way to know we use GICv4, and I intend to keep it
> completely out of the loop. The API is already pretty tortuous, and
> I really don't want to add any extra complexity to it.
>
> Thanks,
>
> M.
On 2020/11/24 16:26, Marc Zyngier wrote:
> On 2020-11-24 07:40, Shenming Lu wrote:
>> On 2020/11/23 17:18, Marc Zyngier wrote:
>>> On 2020-11-23 06:54, Shenming Lu wrote:
>>>> After pausing all vCPUs and devices capable of interrupting, in order
>>> ^^^^^^^^^^^^^^^^^
>>> See my comment below about this.
>>>
>>>> to save the information of all interrupts, besides flushing the pending
>>>> states in kvm’s vgic, we also try to flush the states of VLPIs in the
>>>> virtual pending tables into guest RAM, but we need to have GICv4.1 and
>>>> safely unmap the vPEs first.
>>>>
>>>> Signed-off-by: Shenming Lu <[email protected]>
>>>> ---
>>>> arch/arm64/kvm/vgic/vgic-v3.c | 62 +++++++++++++++++++++++++++++++----
>>>> 1 file changed, 56 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
>>>> index 9cdf39a94a63..e1b3aa4b2b12 100644
>>>> --- a/arch/arm64/kvm/vgic/vgic-v3.c
>>>> +++ b/arch/arm64/kvm/vgic/vgic-v3.c
>>>> @@ -1,6 +1,8 @@
>>>> // SPDX-License-Identifier: GPL-2.0-only
>>>>
>>>> #include <linux/irqchip/arm-gic-v3.h>
>>>> +#include <linux/irq.h>
>>>> +#include <linux/irqdomain.h>
>>>> #include <linux/kvm.h>
>>>> #include <linux/kvm_host.h>
>>>> #include <kvm/arm_vgic.h>
>>>> @@ -356,6 +358,39 @@ int vgic_v3_lpi_sync_pending_status(struct kvm
>>>> *kvm, struct vgic_irq *irq)
>>>> return 0;
>>>> }
>>>>
>>>> +/*
>>>> + * With GICv4.1, we can get the VLPI's pending state after unmapping
>>>> + * the vPE. The deactivation of the doorbell interrupt will trigger
>>>> + * the unmapping of the associated vPE.
>>>> + */
>>>> +static void get_vlpi_state_pre(struct vgic_dist *dist)
>>>> +{
>>>> + struct irq_desc *desc;
>>>> + int i;
>>>> +
>>>> + if (!kvm_vgic_global_state.has_gicv4_1)
>>>> + return;
>>>> +
>>>> + for (i = 0; i < dist->its_vm.nr_vpes; i++) {
>>>> + desc = irq_to_desc(dist->its_vm.vpes[i]->irq);
>>>> + irq_domain_deactivate_irq(irq_desc_get_irq_data(desc));
>>>> + }
>>>> +}
>>>> +
>>>> +static void get_vlpi_state_post(struct vgic_dist *dist)
>>>
>>> nit: the naming feels a bit... odd. Pre/post what?
>>
>> My understanding is that the unmapping is a preparation for get_vlpi_state...
>> Maybe just call it unmap/map_all_vpes?
>
> Yes, much better.
>
> [...]
>
>>>> + if (irq->hw) {
>>>> + WARN_RATELIMIT(irq_get_irqchip_state(irq->host_irq,
>>>> + IRQCHIP_STATE_PENDING, &is_pending),
>>>> + "IRQ %d", irq->host_irq);
>>>
>>> Isn't this going to warn like mad on a GICv4.0 system where this, by definition,
>>> will generate an error?
>>
>> As we have returned an error in save_its_tables if hw && !has_gicv4_1, we don't
>> have to warn this here?
>
> Are you referring to the check in vgic_its_save_itt() that occurs in patch 4?
> Fair enough, though I think the use of irq_get_irqchip_state() isn't quite
> what we want, as per my comments on patch #1.
>
>>>
>>>> + }
>>>> +
>>>> + if (stored == is_pending)
>>>> continue;
>>>>
>>>> - if (irq->pending_latch)
>>>> + if (is_pending)
>>>> val |= 1 << bit_nr;
>>>> else
>>>> val &= ~(1 << bit_nr);
>>>>
>>>> ret = kvm_write_guest_lock(kvm, ptr, &val, 1);
>>>> if (ret)
>>>> - return ret;
>>>> + goto out;
>>>> }
>>>> - return 0;
>>>> +
>>>> +out:
>>>> + get_vlpi_state_post(dist);
>>>
>>> This bit worries me: you have unmapped the VPEs, so any interrupt that has been
>>> generated during that phase is now forever lost (the GIC doesn't have ownership
>>> of the pending tables).
>>
>> In my opinion, during this phase, the devices capable of interrupting
>> should have already been paused (prevent from sending interrupts),
>> such as VFIO migration protocol has already realized it.
>
> Is that a hard guarantee? Pausing devices *may* be possible for a limited
> set of endpoints, but I'm not sure that is universally possible to restart
> them and expect a consistent state (you have just dropped a bunch of network
> packets on the floor...).
No, as far as I know, if the VFIO device does not support pause, the migration would
fail early... And the specific action is decided by the vendor driver.
In fact, the VFIO migration is still in an experimental phase... I will pay attention
to the follow-up development.
>
>>> Do you really expect the VM to be restartable from that point? I don't see how
>>> this is possible.
>>>
>>
>> If the migration has encountered an error, the src VM might be
>> restarted, so we have to map the vPEs back.
>
> As I said above, I doubt it is universally possible to do so, but
> after all, this probably isn't worse that restarting on the target...
>
> M.
On 2020-11-24 08:10, Shenming Lu wrote:
> On 2020/11/23 17:27, Marc Zyngier wrote:
>> On 2020-11-23 06:54, Shenming Lu wrote:
>>> From: Zenghui Yu <[email protected]>
>>>
>>> When setting the forwarding path of a VLPI, it is more consistent to
>>
>> I'm not sure it is more consistent. It is a *new* behaviour, because
>> it only
>> matters for migration, which has been so far unsupported.
>
> Alright, consistent may not be accurate...
> But I have doubt that whether there is really no need to transfer the
> pending states
> from kvm'vgic to VPT in set_forwarding regardless of migration, and the
> similar
> for unset_forwarding.
If you have to transfer that state outside of the a save/restore, it
means that
you have missed the programming of the PCI endpoint. This is an
established
restriction that the MSI programming must occur *after* the translation
has
been established using MAPI/MAPTI (see the large comment at the
beginning of
vgic-v4.c).
If you want to revisit this, fair enough. But you will need a lot more
than
just opportunistically transfer the pending state.
>
>>
>>> also transfer the pending state from irq->pending_latch to VPT
>>> (especially
>>> in migration, the pending states of VLPIs are restored into kvm’s
>>> vgic
>>> first). And we currently send "INT+VSYNC" to trigger a VLPI to
>>> pending.
>>>
>>> Signed-off-by: Zenghui Yu <[email protected]>
>>> Signed-off-by: Shenming Lu <[email protected]>
>>> ---
>>> arch/arm64/kvm/vgic/vgic-v4.c | 12 ++++++++++++
>>> 1 file changed, 12 insertions(+)
>>>
>>> diff --git a/arch/arm64/kvm/vgic/vgic-v4.c
>>> b/arch/arm64/kvm/vgic/vgic-v4.c
>>> index b5fa73c9fd35..cc3ab9cea182 100644
>>> --- a/arch/arm64/kvm/vgic/vgic-v4.c
>>> +++ b/arch/arm64/kvm/vgic/vgic-v4.c
>>> @@ -418,6 +418,18 @@ int kvm_vgic_v4_set_forwarding(struct kvm *kvm,
>>> int virq,
>>> irq->host_irq = virq;
>>> atomic_inc(&map.vpe->vlpi_count);
>>>
>>> + /* Transfer pending state */
>>> + ret = irq_set_irqchip_state(irq->host_irq,
>>> + IRQCHIP_STATE_PENDING,
>>> + irq->pending_latch);
>>> + WARN_RATELIMIT(ret, "IRQ %d", irq->host_irq);
>>> +
>>> + /*
>>> + * Let it be pruned from ap_list later and don't bother
>>> + * the List Register.
>>> + */
>>> + irq->pending_latch = false;
>>
>> It occurs to me that calling into irq_set_irqchip_state() for a large
>> number of interrupts can take a significant amount of time. It is also
>> odd that you dump the VPT with the VPE unmapped, but rely on the VPE
>> being mapped for the opposite operation.
>>
>> Shouldn't these be symmetric, all performed while the VPE is unmapped?
>> It would also save a lot of ITS traffic.
>>
>
> My thought was to use the existing interface directly without
> unmapping...
>
> If you want to unmap the vPE and poke the VPT here, as I said in the
> cover
> letter, set/unset_forwarding might also be called when all devices are
> running
> at normal run time, in which case the unmapping of the vPE is not
> allowed...
No, I'm suggesting that you don't do anything here, but instead as a
by-product
of restoring the ITS tables. What goes wrong if you use the
KVM_DEV_ARM_ITS_RESTORE_TABLE backend instead?
> Another possible solution is to add a new dedicated interface to QEMU
> to transfer
> these pending states to HW in GIC VM state change handler corresponding
> to
> save_pending_tables?
Userspace has no way to know we use GICv4, and I intend to keep it
completely out of the loop. The API is already pretty tortuous, and
I really don't want to add any extra complexity to it.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
On 2020/11/24 21:12, Shenming Lu wrote:
> On 2020/11/24 16:44, Marc Zyngier wrote:
>> On 2020-11-24 08:10, Shenming Lu wrote:
>>> On 2020/11/23 17:27, Marc Zyngier wrote:
>>>> On 2020-11-23 06:54, Shenming Lu wrote:
>>>>> From: Zenghui Yu <[email protected]>
>>>>>
>>>>> When setting the forwarding path of a VLPI, it is more consistent to
>>>>
>>>> I'm not sure it is more consistent. It is a *new* behaviour, because it only
>>>> matters for migration, which has been so far unsupported.
>>>
>>> Alright, consistent may not be accurate...
>>> But I have doubt that whether there is really no need to transfer the
>>> pending states
>>> from kvm'vgic to VPT in set_forwarding regardless of migration, and the similar
>>> for unset_forwarding.
>>
>> If you have to transfer that state outside of the a save/restore, it means that
>> you have missed the programming of the PCI endpoint. This is an established
>> restriction that the MSI programming must occur *after* the translation has
>> been established using MAPI/MAPTI (see the large comment at the beginning of
>> vgic-v4.c).
>>
>> If you want to revisit this, fair enough. But you will need a lot more than
>> just opportunistically transfer the pending state.
>
> Thanks, I will look at what you mentioned.
>
>>
>>>
>>>>
>>>>> also transfer the pending state from irq->pending_latch to VPT (especially
>>>>> in migration, the pending states of VLPIs are restored into kvm’s vgic
>>>>> first). And we currently send "INT+VSYNC" to trigger a VLPI to pending.
>>>>>
>>>>> Signed-off-by: Zenghui Yu <[email protected]>
>>>>> Signed-off-by: Shenming Lu <[email protected]>
>>>>> ---
>>>>> arch/arm64/kvm/vgic/vgic-v4.c | 12 ++++++++++++
>>>>> 1 file changed, 12 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm64/kvm/vgic/vgic-v4.c b/arch/arm64/kvm/vgic/vgic-v4.c
>>>>> index b5fa73c9fd35..cc3ab9cea182 100644
>>>>> --- a/arch/arm64/kvm/vgic/vgic-v4.c
>>>>> +++ b/arch/arm64/kvm/vgic/vgic-v4.c
>>>>> @@ -418,6 +418,18 @@ int kvm_vgic_v4_set_forwarding(struct kvm *kvm, int virq,
>>>>> irq->host_irq = virq;
>>>>> atomic_inc(&map.vpe->vlpi_count);
>>>>>
>>>>> + /* Transfer pending state */
>>>>> + ret = irq_set_irqchip_state(irq->host_irq,
>>>>> + IRQCHIP_STATE_PENDING,
>>>>> + irq->pending_latch);
>>>>> + WARN_RATELIMIT(ret, "IRQ %d", irq->host_irq);
>>>>> +
>>>>> + /*
>>>>> + * Let it be pruned from ap_list later and don't bother
>>>>> + * the List Register.
>>>>> + */
>>>>> + irq->pending_latch = false;
>>>>
>>>> It occurs to me that calling into irq_set_irqchip_state() for a large
>>>> number of interrupts can take a significant amount of time. It is also
>>>> odd that you dump the VPT with the VPE unmapped, but rely on the VPE
>>>> being mapped for the opposite operation.
>>>>
>>>> Shouldn't these be symmetric, all performed while the VPE is unmapped?
>>>> It would also save a lot of ITS traffic.
>>>>
>>>
>>> My thought was to use the existing interface directly without unmapping...
>>>
>>> If you want to unmap the vPE and poke the VPT here, as I said in the cover
>>> letter, set/unset_forwarding might also be called when all devices are running
>>> at normal run time, in which case the unmapping of the vPE is not allowed...
>>
>> No, I'm suggesting that you don't do anything here, but instead as a by-product
>> of restoring the ITS tables. What goes wrong if you use the
>> KVM_DEV_ARM_ITS_RESTORE_TABLE backend instead?
>
> There is an issue if we do it in the restoring of the ITS tables: the transferring
> of the pending state needs the irq to be marked as hw before, which is done by the
> pass-through device, but the configuring of the forwarding path of the VLPI depends
> on the restoring of the vgic first... It is a circular dependency.
>
Hi Marc,
We are pondering over this problem these days, but still don't get a good solution...
Could you give us some advice on this?
Or could we move the restoring of the pending states (include the sync from guest
RAM and the transfer to HW) to the GIC VM state change handler, which is completely
corresponding to save_pending_tables (more symmetric?) and don't expose GICv4...
Thanks,
Shenming
>>
>>> Another possible solution is to add a new dedicated interface to QEMU
>>> to transfer
>>> these pending states to HW in GIC VM state change handler corresponding to
>>> save_pending_tables?
>>
>> Userspace has no way to know we use GICv4, and I intend to keep it
>> completely out of the loop. The API is already pretty tortuous, and
>> I really don't want to add any extra complexity to it.
>>
>> Thanks,
>>
>> M.
On 2020-11-30 07:23, Shenming Lu wrote:
Hi Shenming,
> We are pondering over this problem these days, but still don't get a
> good solution...
> Could you give us some advice on this?
>
> Or could we move the restoring of the pending states (include the sync
> from guest RAM and the transfer to HW) to the GIC VM state change
> handler,
> which is completely corresponding to save_pending_tables (more
> symmetric?)
> and don't expose GICv4...
What is "the GIC VM state change handler"? Is that a QEMU thing?
We don't really have that concept in KVM, so I'd appreciate if you could
be a bit more explicit on this.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
On 2020/12/1 18:55, Marc Zyngier wrote:
> On 2020-11-30 07:23, Shenming Lu wrote:
>
> Hi Shenming,
>
>> We are pondering over this problem these days, but still don't get a
>> good solution...
>> Could you give us some advice on this?
>>
>> Or could we move the restoring of the pending states (include the sync
>> from guest RAM and the transfer to HW) to the GIC VM state change handler,
>> which is completely corresponding to save_pending_tables (more symmetric?)
>> and don't expose GICv4...
>
> What is "the GIC VM state change handler"? Is that a QEMU thing?
Yeah, it is a a QEMU thing...
> We don't really have that concept in KVM, so I'd appreciate if you could
> be a bit more explicit on this.
My thought is to add a new interface (to QEMU) for the restoring of the pending
states, which is completely corresponding to KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES...
And it is called from the GIC VM state change handler in QEMU, which is happening
after the restoring (call kvm_vgic_v4_set_forwarding()) but before the starting
(running) of the VFIO device.
Thanks,
Shenming
>
> Thanks,
>
> M.
On 2020-12-01 11:40, Shenming Lu wrote:
> On 2020/12/1 18:55, Marc Zyngier wrote:
>> On 2020-11-30 07:23, Shenming Lu wrote:
>>
>> Hi Shenming,
>>
>>> We are pondering over this problem these days, but still don't get a
>>> good solution...
>>> Could you give us some advice on this?
>>>
>>> Or could we move the restoring of the pending states (include the
>>> sync
>>> from guest RAM and the transfer to HW) to the GIC VM state change
>>> handler,
>>> which is completely corresponding to save_pending_tables (more
>>> symmetric?)
>>> and don't expose GICv4...
>>
>> What is "the GIC VM state change handler"? Is that a QEMU thing?
>
> Yeah, it is a a QEMU thing...
>
>> We don't really have that concept in KVM, so I'd appreciate if you
>> could
>> be a bit more explicit on this.
>
> My thought is to add a new interface (to QEMU) for the restoring of
> the pending states, which is completely corresponding to
> KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES...
> And it is called from the GIC VM state change handler in QEMU, which
> is happening after the restoring (call kvm_vgic_v4_set_forwarding())
> but before the starting (running) of the VFIO device.
Right, that makes sense. I still wonder how much the GIC save/restore
stuff differs from other architectures that implement similar features,
such as x86 with VT-D.
It is obviously too late to change the userspace interface, but I wonder
whether we missed something at the time.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
On 2020/12/1 19:50, Marc Zyngier wrote:
> On 2020-12-01 11:40, Shenming Lu wrote:
>> On 2020/12/1 18:55, Marc Zyngier wrote:
>>> On 2020-11-30 07:23, Shenming Lu wrote:
>>>
>>> Hi Shenming,
>>>
>>>> We are pondering over this problem these days, but still don't get a
>>>> good solution...
>>>> Could you give us some advice on this?
>>>>
>>>> Or could we move the restoring of the pending states (include the sync
>>>> from guest RAM and the transfer to HW) to the GIC VM state change handler,
>>>> which is completely corresponding to save_pending_tables (more symmetric?)
>>>> and don't expose GICv4...
>>>
>>> What is "the GIC VM state change handler"? Is that a QEMU thing?
>>
>> Yeah, it is a a QEMU thing...
>>
>>> We don't really have that concept in KVM, so I'd appreciate if you could
>>> be a bit more explicit on this.
>>
>> My thought is to add a new interface (to QEMU) for the restoring of
>> the pending states, which is completely corresponding to
>> KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES...
>> And it is called from the GIC VM state change handler in QEMU, which
>> is happening after the restoring (call kvm_vgic_v4_set_forwarding())
>> but before the starting (running) of the VFIO device.
>
> Right, that makes sense. I still wonder how much the GIC save/restore
> stuff differs from other architectures that implement similar features,
> such as x86 with VT-D.
I am not familiar with it...
>
> It is obviously too late to change the userspace interface, but I wonder
> whether we missed something at the time.
The interface seems to be really asymmetrical?...
Or is there a possibility that we could know which irq is hw before the VFIO
device calls kvm_vgic_v4_set_forwarding()?
Thanks,
Shenming
>
> Thanks,
>
> M.
On 2020/12/1 20:15, Shenming Lu wrote:
> On 2020/12/1 19:50, Marc Zyngier wrote:
>> On 2020-12-01 11:40, Shenming Lu wrote:
>>> On 2020/12/1 18:55, Marc Zyngier wrote:
>>>> On 2020-11-30 07:23, Shenming Lu wrote:
>>>>
>>>> Hi Shenming,
>>>>
>>>>> We are pondering over this problem these days, but still don't get a
>>>>> good solution...
>>>>> Could you give us some advice on this?
>>>>>
>>>>> Or could we move the restoring of the pending states (include the sync
>>>>> from guest RAM and the transfer to HW) to the GIC VM state change handler,
>>>>> which is completely corresponding to save_pending_tables (more symmetric?)
>>>>> and don't expose GICv4...
>>>>
>>>> What is "the GIC VM state change handler"? Is that a QEMU thing?
>>>
>>> Yeah, it is a a QEMU thing...
>>>
>>>> We don't really have that concept in KVM, so I'd appreciate if you could
>>>> be a bit more explicit on this.
>>>
>>> My thought is to add a new interface (to QEMU) for the restoring of
>>> the pending states, which is completely corresponding to
>>> KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES...
>>> And it is called from the GIC VM state change handler in QEMU, which
>>> is happening after the restoring (call kvm_vgic_v4_set_forwarding())
>>> but before the starting (running) of the VFIO device.
>>
>> Right, that makes sense. I still wonder how much the GIC save/restore
>> stuff differs from other architectures that implement similar features,
>> such as x86 with VT-D.
>
> I am not familiar with it...
>
>>
>> It is obviously too late to change the userspace interface, but I wonder
>> whether we missed something at the time.
>
> The interface seems to be really asymmetrical?...
>
> Or is there a possibility that we could know which irq is hw before the VFIO
> device calls kvm_vgic_v4_set_forwarding()?
>
> Thanks,
> Shenming
>
>>
>> Thanks,
>>
>> M.
> .
>
Hi Marc,
I am learning VT-d Posted Interrupt (PI) these days.
As far as I can tell, the posted interrupts are firstly recorded in the Posted
Interrupt Request (*PIR*) field of the Posted Interrupt Descriptor (a temporary
storage area (data structure in memory) which is specific to PI), and when the
vCPU is running, a notification event (host vector) will be generated and sent
to the CPU (the target vCPU is currently scheduled on it), which will cause the
CPU to transfer the posted interrupt in the PIR field to the *Virtual-APIC page*
(a data structure in kvm, the virtual interrupts delivered through kvm are put
here, and it is also accessed by the VMX microcode (the layout matches the register
layout seen by the guest)) of the vCPU and directly deliver it to the vCPU.
So they only have to sync the PIR field to the Virtual-APIC page for the migration
saving, and do nothing for the resuming...
Besides, on x86 the setting of the IRQ bypass is independent of the VM interrupt
setup...
Not sure if I have missed something.
In addition, I found that the enabling of the vAPIC is at the end of the migration
(just before the VM start) on x86. So I am wondering if we could move the calling
of *vgic_enable_lpis()* back, and transfer the pending state to the VPT there if the
irq is hw (and I think the semantics of this function should include the transfer).
In fact, this function is dependent on the restoring of the vgic(lpi_list)...
After exploration, there seems to be no perfect place to transfer the pending states
to HW in order to be compatible with the existing interface and under the current
architecture, but we have to choose one solution?
Thanks,
Shenming
On 2020/12/16 18:35, Auger Eric wrote:
> Hi Shenming,
>
> On 12/1/20 1:15 PM, Shenming Lu wrote:
>> On 2020/12/1 19:50, Marc Zyngier wrote:
>>> On 2020-12-01 11:40, Shenming Lu wrote:
>>>> On 2020/12/1 18:55, Marc Zyngier wrote:
>>>>> On 2020-11-30 07:23, Shenming Lu wrote:
>>>>>
>>>>> Hi Shenming,
>>>>>
>>>>>> We are pondering over this problem these days, but still don't get a
>>>>>> good solution...
>>>>>> Could you give us some advice on this?
>>>>>>
>>>>>> Or could we move the restoring of the pending states (include the sync
>>>>>> from guest RAM and the transfer to HW) to the GIC VM state change handler,
>>>>>> which is completely corresponding to save_pending_tables (more symmetric?)
>>>>>> and don't expose GICv4...
>>>>>
>>>>> What is "the GIC VM state change handler"? Is that a QEMU thing?
>>>>
>>>> Yeah, it is a a QEMU thing...
>>>>
>>>>> We don't really have that concept in KVM, so I'd appreciate if you could
>>>>> be a bit more explicit on this.
>>>>
>>>> My thought is to add a new interface (to QEMU) for the restoring of
>>>> the pending states, which is completely corresponding to
>>>> KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES...
>>>> And it is called from the GIC VM state change handler in QEMU, which
>>>> is happening after the restoring (call kvm_vgic_v4_set_forwarding())
>>>> but before the starting (running) of the VFIO device.
>>>
>>> Right, that makes sense. I still wonder how much the GIC save/restore
>>> stuff differs from other architectures that implement similar features,
>>> such as x86 with VT-D.
>>
>> I am not familiar with it...
>>
>>>
>>> It is obviously too late to change the userspace interface, but I wonder
>>> whether we missed something at the time.
>>
>> The interface seems to be really asymmetrical?...
>
> in qemu d5aa0c229a ("hw/intc/arm_gicv3_kvm: Implement pending table
> save") commit message, it is traced:
>
> "There is no explicit restore as the tables are implicitly sync'ed
> on ITS table restore and on LPI enable at redistributor level."
>
> At that time there was no real justification behind adding the RESTORE
> fellow attr.
>
> Maybe a stupid question but isn't it possible to unset the forwarding
> when saving and rely on VFIO to automatically restore it when resuming
> on destination?
It seems that the unset_forwarding would not be called when saving, it would
be called after migration completion...
As for the resuming/set_forwarding, I still wonder: is it really improper to
transfer the pending states from vgic to VPT in set_forwarding (not only in
migration)?... -_-
Thanks,
Shenming
>
> Thanks
>
> Eric
>
>
>>
>> Or is there a possibility that we could know which irq is hw before the VFIO
>> device calls kvm_vgic_v4_set_forwarding()?
>>
>> Thanks,
>> Shenming
>>
>>>
>>> Thanks,
>>>
>>> M.
>>
>
> .
>