LinuxLists.cc - [PATCH v5 00/23] irqchip/gic-v4: GICv4.1 architecture support

[permalink] [raw]

Subject: [PATCH v5 02/23] irqchip/gic-v4.1: Skip absent CPUs while iterating over redistributors

In a system that is only sparsly populated with CPUs, we can end-up with
redistributors structures that are not initialized. Let's make sure we
don't try and access those when iterating over them (in this case when
checking we have a L2 VPE table).

Fixes: 4e6437f12d6e ("irqchip/gic-v4.1: Ensure L2 vPE table is allocated at RD level")
Signed-off-by: Marc Zyngier <[email protected]>
Reviewed-by: Zenghui Yu <[email protected]>
---
drivers/irqchip/irq-gic-v3-its.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 83b1186ffcad..da883a691028 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -2452,6 +2452,10 @@ static bool allocate_vpe_l2_table(int cpu, u32 id)
if (!gic_rdists->has_rvpeid)
return true;

+ /* Skip non-present CPUs */
+ if (!base)
+ return true;
+
val = gicr_read_vpropbaser(base + SZ_128K + GICR_VPROPBASER);

esz = FIELD_GET(GICR_VPROPBASER_4_1_ENTRY_SIZE, val) + 1;
--
2.20.1

2020-03-04 20:37:09

[permalink] [raw]

Subject: [PATCH v5 11/23] irqchip/gic-v4.1: Plumb get/set_irqchip_state SGI callbacks

To implement the get/set_irqchip_state callbacks (limited to the
PENDING state), we have to use a particular set of hacks:

- Reading the pending state is done by using a pair of new redistributor
registers (GICR_VSGIR, GICR_VSGIPENDR), which allow the 16 interrupts
state to be retrieved.
- Setting the pending state is done by generating it as we'd otherwise do
for a guest (writing to GITS_SGIR).
- Clearing the pending state is done by emiting a VSGI command with the
"clear" bit set.

This requires some interesting locking though:
- When talking to the redistributor, we must make sure that the VPE
affinity doesn't change, hence taking the VPE lock.
- At the same time, we must ensure that nobody accesses the same
redistributor's GICR_VSGI*R registers for a different VPE, which
would corrupt the reading of the pending bits. We thus take the
per-RD spinlock. Much fun.

Signed-off-by: Marc Zyngier <[email protected]>
---
drivers/irqchip/irq-gic-v3-its.c | 73 ++++++++++++++++++++++++++++++
include/linux/irqchip/arm-gic-v3.h | 14 ++++++
2 files changed, 87 insertions(+)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index c93f178914ee..fb2b836c31ff 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -3962,11 +3962,84 @@ static int its_sgi_set_affinity(struct irq_data *d,
return -EINVAL;
}

+static int its_sgi_set_irqchip_state(struct irq_data *d,
+ enum irqchip_irq_state which,
+ bool state)
+{
+ if (which != IRQCHIP_STATE_PENDING)
+ return -EINVAL;
+
+ if (state) {
+ struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
+ struct its_node *its = find_4_1_its();
+ u64 val;
+
+ val = FIELD_PREP(GITS_SGIR_VPEID, vpe->vpe_id);
+ val |= FIELD_PREP(GITS_SGIR_VINTID, d->hwirq);
+ writeq_relaxed(val, its->sgir_base + GITS_SGIR - SZ_128K);
+ } else {
+ its_configure_sgi(d, true);
+ }
+
+ return 0;
+}
+
+static int its_sgi_get_irqchip_state(struct irq_data *d,
+ enum irqchip_irq_state which, bool *val)
+{
+ struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
+ void __iomem *base;
+ unsigned long flags;
+ u32 count = 1000000; /* 1s! */
+ u32 status;
+ int cpu;
+
+ if (which != IRQCHIP_STATE_PENDING)
+ return -EINVAL;
+
+ /*
+ * Locking galore! We can race against two different events:
+ *
+ * - Concurent vPE affinity change: we must make sure it cannot
+ * happen, or we'll talk to the wrong redistributor. This is
+ * identical to what happens with vLPIs.
+ *
+ * - Concurrent VSGIPENDR access: As it involves accessing two
+ * MMIO registers, this must be made atomic one way or another.
+ */
+ cpu = vpe_to_cpuid_lock(vpe, &flags);
+ raw_spin_lock(&gic_data_rdist_cpu(cpu)->rd_lock);
+ base = gic_data_rdist_cpu(cpu)->rd_base + SZ_128K;
+ writel_relaxed(vpe->vpe_id, base + GICR_VSGIR);
+ do {
+ status = readl_relaxed(base + GICR_VSGIPENDR);
+ if (!(status & GICR_VSGIPENDR_BUSY))
+ goto out;
+
+ count--;
+ if (!count) {
+ pr_err_ratelimited("Unable to get SGI status\n");
+ goto out;
+ }
+ cpu_relax();
+ udelay(1);
+ } while(count);
+
+out:
+ raw_spin_unlock(&gic_data_rdist_cpu(cpu)->rd_lock);
+ vpe_to_cpuid_unlock(vpe, flags);
+ *val = !!(status & (1 << d->hwirq));
+
+ return 0;
+}
+
static struct irq_chip its_sgi_irq_chip = {
.name = "GICv4.1-sgi",
.irq_mask = its_sgi_mask_irq,
.irq_unmask = its_sgi_unmask_irq,
.irq_set_affinity = its_sgi_set_affinity,
+ .irq_set_irqchip_state = its_sgi_set_irqchip_state,
+ .irq_get_irqchip_state = its_sgi_get_irqchip_state,
};

static int its_sgi_irq_domain_alloc(struct irq_domain *domain,
diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
index fd3be49ac9a5..830d2abf14b3 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -345,6 +345,15 @@
#define GICR_VPENDBASER_4_1_VGRP1EN (1ULL << 58)
#define GICR_VPENDBASER_4_1_VPEID GENMASK_ULL(15, 0)

+#define GICR_VSGIR 0x0080
+
+#define GICR_VSGIR_VPEID GENMASK(15, 0)
+
+#define GICR_VSGIPENDR 0x0088
+
+#define GICR_VSGIPENDR_BUSY (1U << 31)
+#define GICR_VSGIPENDR_PENDING GENMASK(15, 0)
+
/*
* ITS registers, offsets from ITS_base
*/
@@ -368,6 +377,11 @@

#define GITS_TRANSLATER 0x10040

+#define GITS_SGIR 0x20020
+
+#define GITS_SGIR_VPEID GENMASK_ULL(47, 32)
+#define GITS_SGIR_VINTID GENMASK_ULL(7, 0)
+
#define GITS_CTLR_ENABLE (1U << 0)
#define GITS_CTLR_ImDe (1U << 1)
#define GITS_CTLR_ITS_NUMBER_SHIFT 4
--
2.20.1

2020-03-04 20:37:11

[permalink] [raw]

Subject: [PATCH v5 20/23] KVM: arm64: GICv4.1: Plumb SGI implementation selection in the distributor

The GICv4.1 architecture gives the hypervisor the option to let
the guest choose whether it wants the good old SGIs with an
active state, or the new, HW-based ones that do not have one.

For this, plumb the configuration of SGIs into the GICv3 MMIO
handling, present the GICD_TYPER2.nASSGIcap to the guest,
and handle the GICD_CTLR.nASSGIreq setting.

In order to be able to deal with the restore of a guest, also
apply the GICD_CTLR.nASSGIreq setting at first run so that we
can move the restored SGIs to the HW if that's what the guest
had selected in a previous life.

Signed-off-by: Marc Zyngier <[email protected]>
---
virt/kvm/arm/vgic/vgic-mmio-v3.c | 48 ++++++++++++++++++++++++++++++--
virt/kvm/arm/vgic/vgic-v3.c | 2 ++
2 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index de89da76a379..442f3b8c2559 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -3,6 +3,7 @@
* VGICv3 MMIO handling functions
*/

+#include <linux/bitfield.h>
#include <linux/irqchip/arm-gic-v3.h>
#include <linux/kvm.h>
#include <linux/kvm_host.h>
@@ -70,6 +71,8 @@ static unsigned long vgic_mmio_read_v3_misc(struct kvm_vcpu *vcpu,
if (vgic->enabled)
value |= GICD_CTLR_ENABLE_SS_G1;
value |= GICD_CTLR_ARE_NS | GICD_CTLR_DS;
+ if (kvm_vgic_global_state.has_gicv4_1 && vgic->nassgireq)
+ value |= GICD_CTLR_nASSGIreq;
break;
case GICD_TYPER:
value = vgic->nr_spis + VGIC_NR_PRIVATE_IRQS;
@@ -81,6 +84,10 @@ static unsigned long vgic_mmio_read_v3_misc(struct kvm_vcpu *vcpu,
value |= (INTERRUPT_ID_BITS_SPIS - 1) << 19;
}
break;
+ case GICD_TYPER2:
+ if (kvm_vgic_global_state.has_gicv4_1)
+ value = GICD_TYPER2_nASSGIcap;
+ break;
case GICD_IIDR:
value = (PRODUCT_ID_KVM << GICD_IIDR_PRODUCT_ID_SHIFT) |
(vgic->implementation_rev << GICD_IIDR_REVISION_SHIFT) |
@@ -98,17 +105,43 @@ static void vgic_mmio_write_v3_misc(struct kvm_vcpu *vcpu,
unsigned long val)
{
struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
- bool was_enabled = dist->enabled;

switch (addr & 0x0c) {
- case GICD_CTLR:
+ case GICD_CTLR: {
+ bool was_enabled, is_hwsgi;
+
+ mutex_lock(&vcpu->kvm->lock);
+
+ was_enabled = dist->enabled;
+ is_hwsgi = dist->nassgireq;
+
dist->enabled = val & GICD_CTLR_ENABLE_SS_G1;

+ /* Not a GICv4.1? No HW SGIs */
+ if (!kvm_vgic_global_state.has_gicv4_1)
+ val &= ~GICD_CTLR_nASSGIreq;
+
+ /* Dist stays enabled? nASSGIreq is RO */
+ if (was_enabled && dist->enabled) {
+ val &= ~GICD_CTLR_nASSGIreq;
+ val |= FIELD_PREP(GICD_CTLR_nASSGIreq, is_hwsgi);
+ }
+
+ /* Switching HW SGIs? */
+ dist->nassgireq = val & GICD_CTLR_nASSGIreq;
+ if (is_hwsgi != dist->nassgireq)
+ vgic_v4_configure_vsgis(vcpu->kvm);
+
if (!was_enabled && dist->enabled)
vgic_kick_vcpus(vcpu->kvm);
+
+ mutex_unlock(&vcpu->kvm->lock);
break;
+ }
case GICD_TYPER:
+ case GICD_TYPER2:
case GICD_IIDR:
+ /* This is at best for documentation purposes... */
return;
}
}
@@ -117,10 +150,21 @@ static int vgic_mmio_uaccess_write_v3_misc(struct kvm_vcpu *vcpu,
gpa_t addr, unsigned int len,
unsigned long val)
{
+ struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
+
switch (addr & 0x0c) {
case GICD_IIDR:
if (val != vgic_mmio_read_v3_misc(vcpu, addr, len))
return -EINVAL;
+ return 0;
+ case GICD_CTLR:
+ /* Not a GICv4.1? No HW SGIs */
+ if (!kvm_vgic_global_state.has_gicv4_1)
+ val &= ~GICD_CTLR_nASSGIreq;
+
+ dist->enabled = val & GICD_CTLR_ENABLE_SS_G1;
+ dist->nassgireq = val & GICD_CTLR_nASSGIreq;
+ return 0;
}

vgic_mmio_write_v3_misc(vcpu, addr, len, val);
diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
index 1bc09b523486..2c9fc13e2c59 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -540,6 +540,8 @@ int vgic_v3_map_resources(struct kvm *kvm)
goto out;
}

+ if (kvm_vgic_global_state.has_gicv4_1)
+ vgic_v4_configure_vsgis(kvm);
dist->ready = true;

out:
--
2.20.1

2020-03-04 20:37:19

[permalink] [raw]

Subject: [PATCH v5 19/23] KVM: arm64: GICv4.1: Allow SGIs to switch between HW and SW interrupts

In order to let a guest buy in the new, active-less SGIs, we
need to be able to switch between the two modes.

Handle this by stopping all guest activity, transfer the state
from one mode to the other, and resume the guest. Nothing calls
this code so far, but a later patch will plug it into the MMIO
emulation.

Signed-off-by: Marc Zyngier <[email protected]>
---
include/kvm/arm_vgic.h | 3 ++
virt/kvm/arm/vgic/vgic-v4.c | 94 +++++++++++++++++++++++++++++++++++++
virt/kvm/arm/vgic/vgic.h | 1 +
3 files changed, 98 insertions(+)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 63457908c9c4..69f4164d6477 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -231,6 +231,9 @@ struct vgic_dist {
/* distributor enabled */
bool enabled;

+ /* Wants SGIs without active state */
+ bool nassgireq;
+
struct vgic_irq *spis;

struct vgic_io_device dist_iodev;
diff --git a/virt/kvm/arm/vgic/vgic-v4.c b/virt/kvm/arm/vgic/vgic-v4.c
index c2fcde104ea2..a65dc1c85363 100644
--- a/virt/kvm/arm/vgic/vgic-v4.c
+++ b/virt/kvm/arm/vgic/vgic-v4.c
@@ -97,6 +97,100 @@ static irqreturn_t vgic_v4_doorbell_handler(int irq, void *info)
return IRQ_HANDLED;
}

+static void vgic_v4_sync_sgi_config(struct its_vpe *vpe, struct vgic_irq *irq)
+{
+ vpe->sgi_config[irq->intid].enabled = irq->enabled;
+ vpe->sgi_config[irq->intid].group = irq->group;
+ vpe->sgi_config[irq->intid].priority = irq->priority;
+}
+
+static void vgic_v4_enable_vsgis(struct kvm_vcpu *vcpu)
+{
+ struct its_vpe *vpe = &vcpu->arch.vgic_cpu.vgic_v3.its_vpe;
+ int i;
+
+ /*
+ * With GICv4.1, every virtual SGI can be directly injected. So
+ * let's pretend that they are HW interrupts, tied to a host
+ * IRQ. The SGI code will do its magic.
+ */
+ for (i = 0; i < VGIC_NR_SGIS; i++) {
+ struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, i);
+ struct irq_desc *desc;
+ int ret;
+
+ if (irq->hw) {
+ vgic_put_irq(vcpu->kvm, irq);
+ continue;
+ }
+
+ irq->hw = true;
+ irq->host_irq = irq_find_mapping(vpe->sgi_domain, i);
+
+ /* Transfer the full irq state to the vPE */
+ vgic_v4_sync_sgi_config(vpe, irq);
+ desc = irq_to_desc(irq->host_irq);
+ ret = irq_domain_activate_irq(irq_desc_get_irq_data(desc),
+ false);
+ if (!WARN_ON(ret)) {
+ /* Transfer pending state */
+ ret = irq_set_irqchip_state(irq->host_irq,
+ IRQCHIP_STATE_PENDING,
+ irq->pending_latch);
+ WARN_ON(ret);
+ irq->pending_latch = false;
+ }
+
+ vgic_put_irq(vcpu->kvm, irq);
+ }
+}
+
+static void vgic_v4_disable_vsgis(struct kvm_vcpu *vcpu)
+{
+ int i;
+
+ for (i = 0; i < VGIC_NR_SGIS; i++) {
+ struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, i);
+ struct irq_desc *desc;
+ int ret;
+
+ if (!irq->hw) {
+ vgic_put_irq(vcpu->kvm, irq);
+ continue;
+ }
+
+ irq->hw = false;
+ ret = irq_get_irqchip_state(irq->host_irq,
+ IRQCHIP_STATE_PENDING,
+ &irq->pending_latch);
+ WARN_ON(ret);
+
+ desc = irq_to_desc(irq->host_irq);
+ irq_domain_deactivate_irq(irq_desc_get_irq_data(desc));
+
+ vgic_put_irq(vcpu->kvm, irq);
+ }
+}
+
+/* Must be called with the kvm lock held */
+void vgic_v4_configure_vsgis(struct kvm *kvm)
+{
+ struct vgic_dist *dist = &kvm->arch.vgic;
+ struct kvm_vcpu *vcpu;
+ int i;
+
+ kvm_arm_halt_guest(kvm);
+
+ kvm_for_each_vcpu(i, vcpu, kvm) {
+ if (dist->nassgireq)
+ vgic_v4_enable_vsgis(vcpu);
+ else
+ vgic_v4_disable_vsgis(vcpu);
+ }
+
+ kvm_arm_resume_guest(kvm);
+}
+
/**
* vgic_v4_init - Initialize the GICv4 data structures
* @kvm: Pointer to the VM being initialized
diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
index c7fefd6b1c80..769e4802645e 100644
--- a/virt/kvm/arm/vgic/vgic.h
+++ b/virt/kvm/arm/vgic/vgic.h
@@ -316,5 +316,6 @@ void vgic_its_invalidate_cache(struct kvm *kvm);
bool vgic_supports_direct_msis(struct kvm *kvm);
int vgic_v4_init(struct kvm *kvm);
void vgic_v4_teardown(struct kvm *kvm);
+void vgic_v4_configure_vsgis(struct kvm *kvm);

#endif
--
2.20.1

2020-03-04 20:37:24

[permalink] [raw]

Subject: [PATCH v5 22/23] KVM: arm64: GICv4.1: Allow non-trapping WFI when using HW SGIs

Just like for VLPIs, it is beneficial to avoid trapping on WFI when the
vcpu is using the GICv4.1 SGIs.

Add such a check to vcpu_clear_wfx_traps().

Signed-off-by: Marc Zyngier <[email protected]>
---
arch/arm64/include/asm/kvm_emulate.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index f658dda12364..a30b4eec7cb4 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -89,7 +89,8 @@ static inline unsigned long *vcpu_hcr(struct kvm_vcpu *vcpu)
static inline void vcpu_clear_wfx_traps(struct kvm_vcpu *vcpu)
{
vcpu->arch.hcr_el2 &= ~HCR_TWE;
- if (atomic_read(&vcpu->arch.vgic_cpu.vgic_v3.its_vpe.vlpi_count))
+ if (atomic_read(&vcpu->arch.vgic_cpu.vgic_v3.its_vpe.vlpi_count) ||
+ vcpu->kvm->arch.vgic.nassgireq)
vcpu->arch.hcr_el2 &= ~HCR_TWI;
else
vcpu->arch.hcr_el2 |= HCR_TWI;
--
2.20.1

2020-03-04 20:37:32

[permalink] [raw]

Subject: [PATCH v5 13/23] irqchip/gic-v4.1: Move doorbell management to the GICv4 abstraction layer

In order to hide some of the differences between v4.0 and v4.1, move
the doorbell management out of the KVM code, and into the GICv4-specific
layer. This allows the calling code to ask for the doorbell when blocking,
and otherwise to leave the doorbell permanently disabled.

This matches the v4.1 code perfectly, and only results in a minor
refactoring of the v4.0 code.

Signed-off-by: Marc Zyngier <[email protected]>
---
drivers/irqchip/irq-gic-v4.c | 45 +++++++++++++++++++++++++++---
include/kvm/arm_vgic.h | 1 +
include/linux/irqchip/arm-gic-v4.h | 3 +-
virt/kvm/arm/vgic/vgic-v3.c | 4 ++-
virt/kvm/arm/vgic/vgic-v4.c | 34 ++++++++++------------
5 files changed, 61 insertions(+), 26 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v4.c b/drivers/irqchip/irq-gic-v4.c
index c01910d53f9e..117ba6db023d 100644
--- a/drivers/irqchip/irq-gic-v4.c
+++ b/drivers/irqchip/irq-gic-v4.c
@@ -87,6 +87,11 @@ static struct irq_domain *gic_domain;
static const struct irq_domain_ops *vpe_domain_ops;
static const struct irq_domain_ops *sgi_domain_ops;

+static bool has_v4_1(void)
+{
+ return !!sgi_domain_ops;
+}
+
int its_alloc_vcpu_irqs(struct its_vm *vm)
{
int vpe_base_irq, i;
@@ -139,18 +144,50 @@ static int its_send_vpe_cmd(struct its_vpe *vpe, struct its_cmd_info *info)
return irq_set_vcpu_affinity(vpe->irq, info);
}

-int its_schedule_vpe(struct its_vpe *vpe, bool on)
+int its_make_vpe_non_resident(struct its_vpe *vpe, bool db)
{
- struct its_cmd_info info;
+ struct irq_desc *desc = irq_to_desc(vpe->irq);
+ struct its_cmd_info info = { };
int ret;

WARN_ON(preemptible());

- info.cmd_type = on ? SCHEDULE_VPE : DESCHEDULE_VPE;
+ info.cmd_type = DESCHEDULE_VPE;
+ if (has_v4_1()) {
+ /* GICv4.1 can directly deal with doorbells */
+ info.req_db = db;
+ } else {
+ /* Undo the nested disable_irq() calls... */
+ while (db && irqd_irq_disabled(&desc->irq_data))
+ enable_irq(vpe->irq);
+ }
+
+ ret = its_send_vpe_cmd(vpe, &info);
+ if (!ret)
+ vpe->resident = false;
+
+ return ret;
+}
+
+int its_make_vpe_resident(struct its_vpe *vpe, bool g0en, bool g1en)
+{
+ struct its_cmd_info info = { };
+ int ret;
+
+ WARN_ON(preemptible());
+
+ info.cmd_type = SCHEDULE_VPE;
+ if (has_v4_1()) {
+ info.g0en = g0en;
+ info.g1en = g1en;
+ } else {
+ /* Disabled the doorbell, as we're about to enter the guest */
+ disable_irq_nosync(vpe->irq);
+ }

ret = its_send_vpe_cmd(vpe, &info);
if (!ret)
- vpe->resident = on;
+ vpe->resident = true;

return ret;
}
diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 9d53f545a3d5..63457908c9c4 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -70,6 +70,7 @@ struct vgic_global {

/* Hardware has GICv4? */
bool has_gicv4;
+ bool has_gicv4_1;

/* GIC system register CPU interface */
struct static_key_false gicv3_cpuif;
diff --git a/include/linux/irqchip/arm-gic-v4.h b/include/linux/irqchip/arm-gic-v4.h
index b4dbf899460b..8b42d9d9b17e 100644
--- a/include/linux/irqchip/arm-gic-v4.h
+++ b/include/linux/irqchip/arm-gic-v4.h
@@ -125,7 +125,8 @@ struct its_cmd_info {

int its_alloc_vcpu_irqs(struct its_vm *vm);
void its_free_vcpu_irqs(struct its_vm *vm);
-int its_schedule_vpe(struct its_vpe *vpe, bool on);
+int its_make_vpe_resident(struct its_vpe *vpe, bool g0en, bool g1en);
+int its_make_vpe_non_resident(struct its_vpe *vpe, bool db);
int its_invall_vpe(struct its_vpe *vpe);
int its_map_vlpi(int irq, struct its_vlpi_map *map);
int its_get_vlpi(int irq, struct its_vlpi_map *map);
diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
index f45635a6f0ec..1bc09b523486 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -595,7 +595,9 @@ int vgic_v3_probe(const struct gic_kvm_info *info)
/* GICv4 support? */
if (info->has_v4) {
kvm_vgic_global_state.has_gicv4 = gicv4_enable;
- kvm_info("GICv4 support %sabled\n",
+ kvm_vgic_global_state.has_gicv4_1 = info->has_v4_1 && gicv4_enable;
+ kvm_info("GICv4%s support %sabled\n",
+ kvm_vgic_global_state.has_gicv4_1 ? ".1" : "",
gicv4_enable ? "en" : "dis");
}

diff --git a/virt/kvm/arm/vgic/vgic-v4.c b/virt/kvm/arm/vgic/vgic-v4.c
index 46f875589c47..1eb0f8c76219 100644
--- a/virt/kvm/arm/vgic/vgic-v4.c
+++ b/virt/kvm/arm/vgic/vgic-v4.c
@@ -67,10 +67,10 @@
* it. And if we've migrated our vcpu from one CPU to another, we must
* tell the ITS (so that the messages reach the right redistributor).
* This is done in two steps: first issue a irq_set_affinity() on the
- * irq corresponding to the vcpu, then call its_schedule_vpe(). You
- * must be in a non-preemptible context. On exit, another call to
- * its_schedule_vpe() tells the redistributor that we're done with the
- * vcpu.
+ * irq corresponding to the vcpu, then call its_make_vpe_resident().
+ * You must be in a non-preemptible context. On exit, a call to
+ * its_make_vpe_non_resident() tells the redistributor that we're done
+ * with the vcpu.
*
* Finally, the doorbell handling: Each vcpu is allocated an interrupt
* which will fire each time a VLPI is made pending whilst the vcpu is
@@ -86,7 +86,8 @@ static irqreturn_t vgic_v4_doorbell_handler(int irq, void *info)
struct kvm_vcpu *vcpu = info;

/* We got the message, no need to fire again */
- if (!irqd_irq_disabled(&irq_to_desc(irq)->irq_data))
+ if (!kvm_vgic_global_state.has_gicv4_1 &&
+ !irqd_irq_disabled(&irq_to_desc(irq)->irq_data))
disable_irq_nosync(irq);

vcpu->arch.vgic_cpu.vgic_v3.its_vpe.pending_last = true;
@@ -199,19 +200,11 @@ void vgic_v4_teardown(struct kvm *kvm)
int vgic_v4_put(struct kvm_vcpu *vcpu, bool need_db)
{
struct its_vpe *vpe = &vcpu->arch.vgic_cpu.vgic_v3.its_vpe;
- struct irq_desc *desc = irq_to_desc(vpe->irq);

if (!vgic_supports_direct_msis(vcpu->kvm) || !vpe->resident)
return 0;

- /*
- * If blocking, a doorbell is required. Undo the nested
- * disable_irq() calls...
- */
- while (need_db && irqd_irq_disabled(&desc->irq_data))
- enable_irq(vpe->irq);
-
- return its_schedule_vpe(vpe, false);
+ return its_make_vpe_non_resident(vpe, need_db);
}

int vgic_v4_load(struct kvm_vcpu *vcpu)
@@ -232,18 +225,19 @@ int vgic_v4_load(struct kvm_vcpu *vcpu)
if (err)
return err;

- /* Disabled the doorbell, as we're about to enter the guest */
- disable_irq_nosync(vpe->irq);
-
- err = its_schedule_vpe(vpe, true);
+ err = its_make_vpe_resident(vpe, false, vcpu->kvm->arch.vgic.enabled);
if (err)
return err;

/*
* Now that the VPE is resident, let's get rid of a potential
- * doorbell interrupt that would still be pending.
+ * doorbell interrupt that would still be pending. This is a
+ * GICv4.0 only "feature"...
*/
- return irq_set_irqchip_state(vpe->irq, IRQCHIP_STATE_PENDING, false);
+ if (!kvm_vgic_global_state.has_gicv4_1)
+ err = irq_set_irqchip_state(vpe->irq, IRQCHIP_STATE_PENDING, false);
+
+ return err;
}

static struct vgic_its *vgic_get_its(struct kvm *kvm,
--
2.20.1

2020-03-04 20:37:35

[permalink] [raw]

Subject: [PATCH v5 23/23] KVM: arm64: GICv4.1: Expose HW-based SGIs in debugfs

The vgic-state debugfs file could do with showing the pending state
of the HW-backed SGIs. Plug it into the low-level code.

Signed-off-by: Marc Zyngier <[email protected]>
---
virt/kvm/arm/vgic/vgic-debug.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/virt/kvm/arm/vgic/vgic-debug.c b/virt/kvm/arm/vgic/vgic-debug.c
index cc12fe9b2df3..b13a9e3f99dd 100644
--- a/virt/kvm/arm/vgic/vgic-debug.c
+++ b/virt/kvm/arm/vgic/vgic-debug.c
@@ -178,6 +178,8 @@ static void print_irq_state(struct seq_file *s, struct vgic_irq *irq,
struct kvm_vcpu *vcpu)
{
char *type;
+ bool pending;
+
if (irq->intid < VGIC_NR_SGIS)
type = "SGI";
else if (irq->intid < VGIC_NR_PRIVATE_IRQS)
@@ -190,6 +192,16 @@ static void print_irq_state(struct seq_file *s, struct vgic_irq *irq,
if (irq->intid ==0 || irq->intid == VGIC_NR_PRIVATE_IRQS)
print_header(s, irq, vcpu);

+ pending = irq->pending_latch;
+ if (irq->hw && vgic_irq_is_sgi(irq->intid)) {
+ int err;
+
+ err = irq_get_irqchip_state(irq->host_irq,
+ IRQCHIP_STATE_PENDING,
+ &pending);
+ WARN_ON_ONCE(err);
+ }
+
seq_printf(s, " %s %4d "
" %2d "
"%d%d%d%d%d%d%d "
@@ -201,7 +213,7 @@ static void print_irq_state(struct seq_file *s, struct vgic_irq *irq,
"\n",
type, irq->intid,
(irq->target_vcpu) ? irq->target_vcpu->vcpu_id : -1,
- irq->pending_latch,
+ pending,
irq->line_level,
irq->active,
irq->enabled,
--
2.20.1

2020-03-04 20:37:39

[permalink] [raw]

Subject: [PATCH v5 21/23] KVM: arm64: GICv4.1: Reload VLPI configuration on distributor enable/disable

Each time a Group-enable bit gets flipped, the state of these bits
needs to be forwarded to the hardware. This is a pretty heavy
handed operation, requiring all vcpus to reload their GICv4
configuration. It is thus implemented as a new request type.

Of course, we only support Group-1 for now...

Signed-off-by: Marc Zyngier <[email protected]>
---
arch/arm/include/asm/kvm_host.h | 1 +
arch/arm64/include/asm/kvm_host.h | 1 +
virt/kvm/arm/arm.c | 8 ++++++++
virt/kvm/arm/vgic/vgic-mmio-v3.c | 5 ++++-
4 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index a827b4d60d38..3da57e863df6 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -39,6 +39,7 @@
#define KVM_REQ_IRQ_PENDING KVM_ARCH_REQ(1)
#define KVM_REQ_VCPU_RESET KVM_ARCH_REQ(2)
#define KVM_REQ_RECORD_STEAL KVM_ARCH_REQ(3)
+#define KVM_REQ_RELOAD_GICv4 KVM_ARCH_REQ(4)

DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use);

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 57fd46acd058..32c8a675e5a4 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -44,6 +44,7 @@
#define KVM_REQ_IRQ_PENDING KVM_ARCH_REQ(1)
#define KVM_REQ_VCPU_RESET KVM_ARCH_REQ(2)
#define KVM_REQ_RECORD_STEAL KVM_ARCH_REQ(3)
+#define KVM_REQ_RELOAD_GICv4 KVM_ARCH_REQ(4)

DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use);

diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index eda7b624eab8..4d864f857ac8 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -625,6 +625,14 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)

if (kvm_check_request(KVM_REQ_RECORD_STEAL, vcpu))
kvm_update_stolen_time(vcpu);
+
+ if (kvm_check_request(KVM_REQ_RELOAD_GICv4, vcpu)) {
+ /* The distributor enable bits were changed */
+ preempt_disable();
+ vgic_v4_put(vcpu, false);
+ vgic_v4_load(vcpu);
+ preempt_enable();
+ }
}
}

diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index 442f3b8c2559..48fd9fc229a2 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -132,7 +132,10 @@ static void vgic_mmio_write_v3_misc(struct kvm_vcpu *vcpu,
if (is_hwsgi != dist->nassgireq)
vgic_v4_configure_vsgis(vcpu->kvm);

- if (!was_enabled && dist->enabled)
+ if (kvm_vgic_global_state.has_gicv4_1 &&
+ was_enabled != dist->enabled)
+ kvm_make_all_cpus_request(vcpu->kvm, KVM_REQ_RELOAD_GICv4);
+ else if (!was_enabled && dist->enabled)
vgic_kick_vcpus(vcpu->kvm);

mutex_unlock(&vcpu->kvm->lock);
--
2.20.1

2020-03-04 20:37:46

[permalink] [raw]

Subject: [PATCH v5 15/23] irqchip/gic-v4.1: Add VSGI property setup

Add the SGI configuration entry point for KVM to use.

Signed-off-by: Marc Zyngier <[email protected]>
---
drivers/irqchip/irq-gic-v3-its.c | 2 +-
drivers/irqchip/irq-gic-v4.c | 13 +++++++++++++
include/linux/irqchip/arm-gic-v4.h | 3 ++-
3 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index effb0e0b0c9d..b65fba67bd85 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -4039,7 +4039,7 @@ static int its_sgi_set_vcpu_affinity(struct irq_data *d, void *vcpu_info)
struct its_cmd_info *info = vcpu_info;

switch (info->cmd_type) {
- case PROP_UPDATE_SGI:
+ case PROP_UPDATE_VSGI:
vpe->sgi_config[d->hwirq].priority = info->priority;
vpe->sgi_config[d->hwirq].group = info->group;
its_configure_sgi(d, false);
diff --git a/drivers/irqchip/irq-gic-v4.c b/drivers/irqchip/irq-gic-v4.c
index 99b33f60ac63..0c18714ae13e 100644
--- a/drivers/irqchip/irq-gic-v4.c
+++ b/drivers/irqchip/irq-gic-v4.c
@@ -320,6 +320,19 @@ int its_prop_update_vlpi(int irq, u8 config, bool inv)
return irq_set_vcpu_affinity(irq, &info);
}

+int its_prop_update_vsgi(int irq, u8 priority, bool group)
+{
+ struct its_cmd_info info = {
+ .cmd_type = PROP_UPDATE_VSGI,
+ {
+ .priority = priority,
+ .group = group,
+ },
+ };
+
+ return irq_set_vcpu_affinity(irq, &info);
+}
+
int its_init_v4(struct irq_domain *domain,
const struct irq_domain_ops *vpe_ops,
const struct irq_domain_ops *sgi_ops)
diff --git a/include/linux/irqchip/arm-gic-v4.h b/include/linux/irqchip/arm-gic-v4.h
index 0bb111b4a504..6976b8331b60 100644
--- a/include/linux/irqchip/arm-gic-v4.h
+++ b/include/linux/irqchip/arm-gic-v4.h
@@ -105,7 +105,7 @@ enum its_vcpu_info_cmd_type {
SCHEDULE_VPE,
DESCHEDULE_VPE,
INVALL_VPE,
- PROP_UPDATE_SGI,
+ PROP_UPDATE_VSGI,
};

struct its_cmd_info {
@@ -134,6 +134,7 @@ int its_map_vlpi(int irq, struct its_vlpi_map *map);
int its_get_vlpi(int irq, struct its_vlpi_map *map);
int its_unmap_vlpi(int irq);
int its_prop_update_vlpi(int irq, u8 config, bool inv);
+int its_prop_update_vsgi(int irq, u8 priority, bool group);

struct irq_domain_ops;
int its_init_v4(struct irq_domain *domain,
--
2.20.1

2020-03-04 20:37:51

[permalink] [raw]

Subject: [PATCH v5 18/23] KVM: arm64: GICv4.1: Add direct injection capability to SGI registers

Most of the GICv3 emulation code that deals with SGIs now has to be
aware of the v4.1 capabilities in order to benefit from it.

Add such support, keyed on the interrupt having the hw flag set and
being a SGI.

Signed-off-by: Marc Zyngier <[email protected]>
---
virt/kvm/arm/vgic/vgic-mmio-v3.c | 15 +++++-
virt/kvm/arm/vgic/vgic-mmio.c | 88 ++++++++++++++++++++++++++++++--
2 files changed, 96 insertions(+), 7 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index ebc218840fc2..de89da76a379 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -6,6 +6,7 @@
#include <linux/irqchip/arm-gic-v3.h>
#include <linux/kvm.h>
#include <linux/kvm_host.h>
+#include <linux/interrupt.h>
#include <kvm/iodev.h>
#include <kvm/arm_vgic.h>

@@ -942,8 +943,18 @@ void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg, bool allow_group1)
* generate interrupts of either group.
*/
if (!irq->group || allow_group1) {
- irq->pending_latch = true;
- vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
+ if (!irq->hw) {
+ irq->pending_latch = true;
+ vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
+ } else {
+ /* HW SGI? Ask the GIC to inject it */
+ int err;
+ err = irq_set_irqchip_state(irq->host_irq,
+ IRQCHIP_STATE_PENDING,
+ true);
+ WARN_RATELIMIT(err, "IRQ %d", irq->host_irq);
+ raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
+ }
} else {
raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
}
diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
index 97fb2a40e6ba..2199302597fa 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.c
+++ b/virt/kvm/arm/vgic/vgic-mmio.c
@@ -5,6 +5,8 @@

#include <linux/bitops.h>
#include <linux/bsearch.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
#include <linux/kvm.h>
#include <linux/kvm_host.h>
#include <kvm/iodev.h>
@@ -59,6 +61,11 @@ unsigned long vgic_mmio_read_group(struct kvm_vcpu *vcpu,
return value;
}

+static void vgic_update_vsgi(struct vgic_irq *irq)
+{
+ WARN_ON(its_prop_update_vsgi(irq->host_irq, irq->priority, irq->group));
+}
+
void vgic_mmio_write_group(struct kvm_vcpu *vcpu, gpa_t addr,
unsigned int len, unsigned long val)
{
@@ -71,7 +78,12 @@ void vgic_mmio_write_group(struct kvm_vcpu *vcpu, gpa_t addr,

raw_spin_lock_irqsave(&irq->irq_lock, flags);
irq->group = !!(val & BIT(i));
- vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
+ if (irq->hw && vgic_irq_is_sgi(irq->intid)) {
+ vgic_update_vsgi(irq);
+ raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
+ } else {
+ vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
+ }

vgic_put_irq(vcpu->kvm, irq);
}
@@ -113,7 +125,21 @@ void vgic_mmio_write_senable(struct kvm_vcpu *vcpu,
struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);

raw_spin_lock_irqsave(&irq->irq_lock, flags);
- if (vgic_irq_is_mapped_level(irq)) {
+ if (irq->hw && vgic_irq_is_sgi(irq->intid)) {
+ if (!irq->enabled) {
+ struct irq_data *data;
+
+ irq->enabled = true;
+ data = &irq_to_desc(irq->host_irq)->irq_data;
+ while (irqd_irq_disabled(data))
+ enable_irq(irq->host_irq);
+ }
+
+ raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
+ vgic_put_irq(vcpu->kvm, irq);
+
+ continue;
+ } else if (vgic_irq_is_mapped_level(irq)) {
bool was_high = irq->line_level;

/*
@@ -148,6 +174,8 @@ void vgic_mmio_write_cenable(struct kvm_vcpu *vcpu,
struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);

raw_spin_lock_irqsave(&irq->irq_lock, flags);
+ if (irq->hw && vgic_irq_is_sgi(irq->intid) && irq->enabled)
+ disable_irq_nosync(irq->host_irq);

irq->enabled = false;

@@ -167,10 +195,22 @@ unsigned long vgic_mmio_read_pending(struct kvm_vcpu *vcpu,
for (i = 0; i < len * 8; i++) {
struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
unsigned long flags;
+ bool val;

raw_spin_lock_irqsave(&irq->irq_lock, flags);
- if (irq_is_pending(irq))
- value |= (1U << i);
+ if (irq->hw && vgic_irq_is_sgi(irq->intid)) {
+ int err;
+
+ val = false;
+ err = irq_get_irqchip_state(irq->host_irq,
+ IRQCHIP_STATE_PENDING,
+ &val);
+ WARN_RATELIMIT(err, "IRQ %d", irq->host_irq);
+ } else {
+ val = irq_is_pending(irq);
+ }
+
+ value |= ((u32)val << i);
raw_spin_unlock_irqrestore(&irq->irq_lock, flags);

vgic_put_irq(vcpu->kvm, irq);
@@ -215,6 +255,21 @@ void vgic_mmio_write_spending(struct kvm_vcpu *vcpu,
}

raw_spin_lock_irqsave(&irq->irq_lock, flags);
+
+ if (irq->hw && vgic_irq_is_sgi(irq->intid)) {
+ /* HW SGI? Ask the GIC to inject it */
+ int err;
+ err = irq_set_irqchip_state(irq->host_irq,
+ IRQCHIP_STATE_PENDING,
+ true);
+ WARN_RATELIMIT(err, "IRQ %d", irq->host_irq);
+
+ raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
+ vgic_put_irq(vcpu->kvm, irq);
+
+ continue;
+ }
+
if (irq->hw)
vgic_hw_irq_spending(vcpu, irq, is_uaccess);
else
@@ -269,6 +324,20 @@ void vgic_mmio_write_cpending(struct kvm_vcpu *vcpu,

raw_spin_lock_irqsave(&irq->irq_lock, flags);

+ if (irq->hw && vgic_irq_is_sgi(irq->intid)) {
+ /* HW SGI? Ask the GIC to clear its pending bit */
+ int err;
+ err = irq_set_irqchip_state(irq->host_irq,
+ IRQCHIP_STATE_PENDING,
+ false);
+ WARN_RATELIMIT(err, "IRQ %d", irq->host_irq);
+
+ raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
+ vgic_put_irq(vcpu->kvm, irq);
+
+ continue;
+ }
+
if (irq->hw)
vgic_hw_irq_cpending(vcpu, irq, is_uaccess);
else
@@ -318,8 +387,15 @@ static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,

raw_spin_lock_irqsave(&irq->irq_lock, flags);

- if (irq->hw) {
+ if (irq->hw && !vgic_irq_is_sgi(irq->intid)) {
vgic_hw_irq_change_active(vcpu, irq, active, !requester_vcpu);
+ } else if (irq->hw && vgic_irq_is_sgi(irq->intid)) {
+ /*
+ * GICv4.1 VSGI feature doesn't track an active state,
+ * so let's not kid ourselves, there is nothing we can
+ * do here.
+ */
+ irq->active = false;
} else {
u32 model = vcpu->kvm->arch.vgic.vgic_model;
u8 active_source;
@@ -493,6 +569,8 @@ void vgic_mmio_write_priority(struct kvm_vcpu *vcpu,
raw_spin_lock_irqsave(&irq->irq_lock, flags);
/* Narrow the priority range to what we actually support */
irq->priority = (val >> (i * 8)) & GENMASK(7, 8 - VGIC_PRI_BITS);
+ if (irq->hw && vgic_irq_is_sgi(irq->intid))
+ vgic_update_vsgi(irq);
raw_spin_unlock_irqrestore(&irq->irq_lock, flags);

vgic_put_irq(vcpu->kvm, irq);
--
2.20.1

2020-03-04 20:37:53

[permalink] [raw]

Subject: [PATCH v5 16/23] irqchip/gic-v4.1: Eagerly vmap vPEs

Now that we have HW-accelerated SGIs being delivered to VPEs, it
becomes required to map the VPEs on all ITSs instead of relying
on the lazy approach that we would use when using the ITS-list
mechanism.

Signed-off-by: Marc Zyngier <[email protected]>
---
drivers/irqchip/irq-gic-v3-its.c | 39 +++++++++++++++++++++++++-------
1 file changed, 31 insertions(+), 8 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index b65fba67bd85..6277b3e3731f 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1586,12 +1586,31 @@ static int its_irq_set_irqchip_state(struct irq_data *d,
return 0;
}

+/*
+ * Two favourable cases:
+ *
+ * (a) Either we have a GICv4.1, and all vPEs have to be mapped at all times
+ * for vSGI delivery
+ *
+ * (b) Or the ITSs do not use a list map, meaning that VMOVP is cheap enough
+ * and we're better off mapping all VPEs always
+ *
+ * If neither (a) nor (b) is true, then we map vPEs on demand.
+ *
+ */
+static bool gic_requires_eager_mapping(void)
+{
+ if (!its_list_map || gic_rdists->has_rvpeid)
+ return true;
+
+ return false;
+}
+
static void its_map_vm(struct its_node *its, struct its_vm *vm)
{
unsigned long flags;

- /* Not using the ITS list? Everything is always mapped. */
- if (!its_list_map)
+ if (gic_requires_eager_mapping())
return;

raw_spin_lock_irqsave(&vmovp_lock, flags);
@@ -1625,7 +1644,7 @@ static void its_unmap_vm(struct its_node *its, struct its_vm *vm)
unsigned long flags;

/* Not using the ITS list? Everything is always mapped. */
- if (!its_list_map)
+ if (gic_requires_eager_mapping())
return;

raw_spin_lock_irqsave(&vmovp_lock, flags);
@@ -4260,8 +4279,12 @@ static int its_vpe_irq_domain_activate(struct irq_domain *domain,
struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
struct its_node *its;

- /* If we use the list map, we issue VMAPP on demand... */
- if (its_list_map)
+ /*
+ * If we use the list map, we issue VMAPP on demand... Unless
+ * we're on a GICv4.1 and we eagerly map the VPE on all ITSs
+ * so that VSGIs can work.
+ */
+ if (!gic_requires_eager_mapping())
return 0;

/* Map the VPE to the first possible CPU */
@@ -4287,10 +4310,10 @@ static void its_vpe_irq_domain_deactivate(struct irq_domain *domain,
struct its_node *its;

/*
- * If we use the list map, we unmap the VPE once no VLPIs are
- * associated with the VM.
+ * If we use the list map on GICv4.0, we unmap the VPE once no
+ * VLPIs are associated with the VM.
*/
- if (its_list_map)
+ if (!gic_requires_eager_mapping())
return;

list_for_each_entry(its, &its_nodes, entry) {
--
2.20.1

2020-03-04 20:38:03

[permalink] [raw]

Subject: [PATCH v5 14/23] irqchip/gic-v4.1: Add VSGI allocation/teardown

Allocate per-VPE SGIs when initializing the GIC-specific part of the
VPE data structure.

Signed-off-by: Marc Zyngier <[email protected]>
---
drivers/irqchip/irq-gic-v4.c | 68 +++++++++++++++++++++++++++++-
include/linux/irqchip/arm-gic-v4.h | 2 +
2 files changed, 69 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-gic-v4.c b/drivers/irqchip/irq-gic-v4.c
index 117ba6db023d..99b33f60ac63 100644
--- a/drivers/irqchip/irq-gic-v4.c
+++ b/drivers/irqchip/irq-gic-v4.c
@@ -92,6 +92,47 @@ static bool has_v4_1(void)
return !!sgi_domain_ops;
}

+static int its_alloc_vcpu_sgis(struct its_vpe *vpe, int idx)
+{
+ char *name;
+ int sgi_base;
+
+ if (!has_v4_1())
+ return 0;
+
+ name = kasprintf(GFP_KERNEL, "GICv4-sgi-%d", task_pid_nr(current));
+ if (!name)
+ goto err;
+
+ vpe->fwnode = irq_domain_alloc_named_id_fwnode(name, idx);
+ if (!vpe->fwnode)
+ goto err;
+
+ kfree(name);
+ name = NULL;
+
+ vpe->sgi_domain = irq_domain_create_linear(vpe->fwnode, 16,
+ sgi_domain_ops, vpe);
+ if (!vpe->sgi_domain)
+ goto err;
+
+ sgi_base = __irq_domain_alloc_irqs(vpe->sgi_domain, -1, 16,
+ NUMA_NO_NODE, vpe,
+ false, NULL);
+ if (sgi_base <= 0)
+ goto err;
+
+ return 0;
+
+err:
+ if (vpe->sgi_domain)
+ irq_domain_remove(vpe->sgi_domain);
+ if (vpe->fwnode)
+ irq_domain_free_fwnode(vpe->fwnode);
+ kfree(name);
+ return -ENOMEM;
+}
+
int its_alloc_vcpu_irqs(struct its_vm *vm)
{
int vpe_base_irq, i;
@@ -118,8 +159,13 @@ int its_alloc_vcpu_irqs(struct its_vm *vm)
if (vpe_base_irq <= 0)
goto err;

- for (i = 0; i < vm->nr_vpes; i++)
+ for (i = 0; i < vm->nr_vpes; i++) {
+ int ret;
vm->vpes[i]->irq = vpe_base_irq + i;
+ ret = its_alloc_vcpu_sgis(vm->vpes[i], i);
+ if (ret)
+ goto err;
+ }

return 0;

@@ -132,8 +178,28 @@ int its_alloc_vcpu_irqs(struct its_vm *vm)
return -ENOMEM;
}

+static void its_free_sgi_irqs(struct its_vm *vm)
+{
+ int i;
+
+ if (!has_v4_1())
+ return;
+
+ for (i = 0; i < vm->nr_vpes; i++) {
+ unsigned int irq = irq_find_mapping(vm->vpes[i]->sgi_domain, 0);
+
+ if (WARN_ON(!irq))
+ continue;
+
+ irq_domain_free_irqs(irq, 16);
+ irq_domain_remove(vm->vpes[i]->sgi_domain);
+ irq_domain_free_fwnode(vm->vpes[i]->fwnode);
+ }
+}
+
void its_free_vcpu_irqs(struct its_vm *vm)
{
+ its_free_sgi_irqs(vm);
irq_domain_free_irqs(vm->vpes[0]->irq, vm->nr_vpes);
irq_domain_remove(vm->domain);
irq_domain_free_fwnode(vm->fwnode);
diff --git a/include/linux/irqchip/arm-gic-v4.h b/include/linux/irqchip/arm-gic-v4.h
index 8b42d9d9b17e..0bb111b4a504 100644
--- a/include/linux/irqchip/arm-gic-v4.h
+++ b/include/linux/irqchip/arm-gic-v4.h
@@ -49,6 +49,8 @@ struct its_vpe {
};
/* GICv4.1 implementations */
struct {
+ struct fwnode_handle *fwnode;
+ struct irq_domain *sgi_domain;
struct {
u8 priority;
bool enabled;
--
2.20.1

2020-03-04 20:38:46

[permalink] [raw]

Subject: [PATCH v5 10/23] irqchip/gic-v4.1: Plumb mask/unmask SGI callbacks

Implement mask/unmask for virtual SGIs by calling into the
configuration helper.

Signed-off-by: Marc Zyngier <[email protected]>
Reviewed-by: Zenghui Yu <[email protected]>
---
drivers/irqchip/irq-gic-v3-its.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index e0db3f906f87..c93f178914ee 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -3939,6 +3939,22 @@ static void its_configure_sgi(struct irq_data *d, bool clear)
its_send_single_vcommand(find_4_1_its(), its_build_vsgi_cmd, &desc);
}

+static void its_sgi_mask_irq(struct irq_data *d)
+{
+ struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
+
+ vpe->sgi_config[d->hwirq].enabled = false;
+ its_configure_sgi(d, false);
+}
+
+static void its_sgi_unmask_irq(struct irq_data *d)
+{
+ struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
+
+ vpe->sgi_config[d->hwirq].enabled = true;
+ its_configure_sgi(d, false);
+}
+
static int its_sgi_set_affinity(struct irq_data *d,
const struct cpumask *mask_val,
bool force)
@@ -3948,6 +3964,8 @@ static int its_sgi_set_affinity(struct irq_data *d,

static struct irq_chip its_sgi_irq_chip = {
.name = "GICv4.1-sgi",
+ .irq_mask = its_sgi_mask_irq,
+ .irq_unmask = its_sgi_unmask_irq,
.irq_set_affinity = its_sgi_set_affinity,
};

--
2.20.1

2020-03-05 03:40:43

[permalink] [raw]

Subject: Re: [PATCH v5 00/23] irqchip/gic-v4: GICv4.1 architecture support

Hi Marc,

On 2020/3/5 4:33, Marc Zyngier wrote:
> This (now shorter) series expands the existing GICv4 support to deal
> with the new GICv4.1 architecture, which comes with a set of major
> improvements compared to v4.0:
>
> - One architectural doorbell per vcpu, instead of one doorbell per VLPI
>
> - Doorbell entirely managed by the HW, with an "at most once" delivery
> guarantee per non-residency phase and only when requested by the
> hypervisor
>
> - A shared memory scheme between ITSs and redistributors, allowing for an
> optimised residency sequence (the use of VMOVP becomes less frequent)
>
> - Support for direct virtual SGI delivery (the injection path still involves
> the hypervisor), at the cost of losing the active state on SGIs. It
> shouldn't be a big deal, but some guest operating systems might notice
> (Linux definitely won't care).
>
> On the other hand, public documentation is not available yet, so that's a
> bit annoying...
>
> The series is roughly organised in 3 parts:
>
> (0) Fixes
> (1) v4.1 doorbell management
> (2) Virtual SGI support
> (3) Plumbing of virtual SGIs in KVM
>
> Notes:
>
> - The whole thing is tested on a FVP model, which can be obtained
> free of charge on ARM's developer website. It requires you to
> create an account, unfortunately... You'll need a fix for the
> devicetree that is in the kernel tree (should be merged before
> the 5.6 release).
>
> - This series has uncovered a behaviour that looks like a HW bug on
> the Cavium ThunderX (aka TX1) platform. I'd very much welcome some
> clarification from the Marvell/Cavium folks on Cc, as well as an
> official erratum number if this happens to be an actual bug.
>
> [v3 update]
> People have ignored for two months now, and it is fairly obvious
> that support for this machine is slowly bit-rotting. Maybe I'll
> drop the patch and instead start the process of removing all TX1
> support from the kernel (we'd certainly be better off without it).
>
> [v4 update]
> TX1 is now broken in mainline, and nobody cares. Make of this what
> you want.
>
> - I'm extremely grateful for Zenghui Yu's huge effort in carefully
> reviewing this rather difficult series (if we ever get to meet
> face to face, drinks are definitely on me!).

It's a pleasure to review this work and it's pretty useful for
understanding how Linux works as a GICv4.1-capable hypervisor.
Yay, cheers ;-)!

I'll go through the v4.1 spec one more time before the final
review of this series, as we still have plenty of time to do
some reviews (and even some tests) before the 5.7 MW.

>
> - Unless someone cries wolf, I plan to take this into 5.7.

Good news!

Thanks,
Zenghui

2020-03-09 08:19:20

[permalink] [raw]

Subject: Re: [PATCH v5 00/23] irqchip/gic-v4: GICv4.1 architecture support

On 2020/3/5 4:33, Marc Zyngier wrote:
> On the other hand, public documentation is not available yet, so that's a
> bit annoying...

The IHI0069F is now available. People can have a look at:

https://developer.arm.com/docs/ihi0069/latest

Thanks,
Zenghui

2020-03-09 08:47:16

[permalink] [raw]

Subject: Re: [PATCH v5 00/23] irqchip/gic-v4: GICv4.1 architecture support

On 2020-03-09 08:17, Zenghui Yu wrote:
> On 2020/3/5 4:33, Marc Zyngier wrote:
>> On the other hand, public documentation is not available yet, so
>> that's a
>> bit annoying...
>
> The IHI0069F is now available. People can have a look at:
>
> https://developer.arm.com/docs/ihi0069/latest

Party! ;-)

M.
--
Jazz is not dead. It just smells funny...

2020-03-12 07:43:43

[permalink] [raw]

Subject: Re: [PATCH v5 11/23] irqchip/gic-v4.1: Plumb get/set_irqchip_state SGI callbacks

Hi Marc,

Some checkpatch errors below:

On 2020/3/5 4:33, Marc Zyngier wrote:
> To implement the get/set_irqchip_state callbacks (limited to the
> PENDING state), we have to use a particular set of hacks:
>
> - Reading the pending state is done by using a pair of new redistributor
> registers (GICR_VSGIR, GICR_VSGIPENDR), which allow the 16 interrupts
> state to be retrieved.
> - Setting the pending state is done by generating it as we'd otherwise do
> for a guest (writing to GITS_SGIR).
> - Clearing the pending state is done by emiting a VSGI command with the
> "clear" bit set.
>
> This requires some interesting locking though:
> - When talking to the redistributor, we must make sure that the VPE
> affinity doesn't change, hence taking the VPE lock.
> - At the same time, we must ensure that nobody accesses the same
> redistributor's GICR_VSGI*R registers for a different VPE, which
> would corrupt the reading of the pending bits. We thus take the
> per-RD spinlock. Much fun.
>
> Signed-off-by: Marc Zyngier <[email protected]>
> ---
> drivers/irqchip/irq-gic-v3-its.c | 73 ++++++++++++++++++++++++++++++
> include/linux/irqchip/arm-gic-v3.h | 14 ++++++
> 2 files changed, 87 insertions(+)
>
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index c93f178914ee..fb2b836c31ff 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -3962,11 +3962,84 @@ static int its_sgi_set_affinity(struct irq_data *d,
> return -EINVAL;
> }
>
> +static int its_sgi_set_irqchip_state(struct irq_data *d,
> + enum irqchip_irq_state which,
> + bool state)
> +{
> + if (which != IRQCHIP_STATE_PENDING)
> + return -EINVAL;
> +
> + if (state) {
> + struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
> + struct its_node *its = find_4_1_its();
> + u64 val;
> +
> + val = FIELD_PREP(GITS_SGIR_VPEID, vpe->vpe_id);
> + val |= FIELD_PREP(GITS_SGIR_VINTID, d->hwirq);
> + writeq_relaxed(val, its->sgir_base + GITS_SGIR - SZ_128K);
> + } else {
> + its_configure_sgi(d, true);
> + }
> +
> + return 0;
> +}
> +
> +static int its_sgi_get_irqchip_state(struct irq_data *d,
> + enum irqchip_irq_state which, bool *val)
> +{
> + struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
> + void __iomem *base;
> + unsigned long flags;
> + u32 count = 1000000; /* 1s! */
> + u32 status;
> + int cpu;
> +
> + if (which != IRQCHIP_STATE_PENDING)
> + return -EINVAL;
> +
> + /*
> + * Locking galore! We can race against two different events:
> + *
> + * - Concurent vPE affinity change: we must make sure it cannot
> + * happen, or we'll talk to the wrong redistributor. This is
> + * identical to what happens with vLPIs.

code indent should use tabs where possible

> + *
> + * - Concurrent VSGIPENDR access: As it involves accessing two
> + * MMIO registers, this must be made atomic one way or another.

The same here.

> + */
> + cpu = vpe_to_cpuid_lock(vpe, &flags);
> + raw_spin_lock(&gic_data_rdist_cpu(cpu)->rd_lock);
> + base = gic_data_rdist_cpu(cpu)->rd_base + SZ_128K;
> + writel_relaxed(vpe->vpe_id, base + GICR_VSGIR);
> + do {
> + status = readl_relaxed(base + GICR_VSGIPENDR);
> + if (!(status & GICR_VSGIPENDR_BUSY))
> + goto out;
> +
> + count--;
> + if (!count) {
> + pr_err_ratelimited("Unable to get SGI status\n");
> + goto out;
> + }
> + cpu_relax();
> + udelay(1);
> + } while(count);

space required before the open parenthesis '('

> +
> +out:
> + raw_spin_unlock(&gic_data_rdist_cpu(cpu)->rd_lock);
> + vpe_to_cpuid_unlock(vpe, flags);
> + *val = !!(status & (1 << d->hwirq));
> +
> + return 0;
> +}
> +
> static struct irq_chip its_sgi_irq_chip = {
> .name = "GICv4.1-sgi",
> .irq_mask = its_sgi_mask_irq,
> .irq_unmask = its_sgi_unmask_irq,
> .irq_set_affinity = its_sgi_set_affinity,
> + .irq_set_irqchip_state = its_sgi_set_irqchip_state,
> + .irq_get_irqchip_state = its_sgi_get_irqchip_state,
> };
>
> static int its_sgi_irq_domain_alloc(struct irq_domain *domain,
> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
> index fd3be49ac9a5..830d2abf14b3 100644
> --- a/include/linux/irqchip/arm-gic-v3.h
> +++ b/include/linux/irqchip/arm-gic-v3.h
> @@ -345,6 +345,15 @@
> #define GICR_VPENDBASER_4_1_VGRP1EN (1ULL << 58)
> #define GICR_VPENDBASER_4_1_VPEID GENMASK_ULL(15, 0)
>
> +#define GICR_VSGIR 0x0080
> +
> +#define GICR_VSGIR_VPEID GENMASK(15, 0)
> +
> +#define GICR_VSGIPENDR 0x0088
> +
> +#define GICR_VSGIPENDR_BUSY (1U << 31)
> +#define GICR_VSGIPENDR_PENDING GENMASK(15, 0)
> +
> /*
> * ITS registers, offsets from ITS_base
> */
> @@ -368,6 +377,11 @@
>
> #define GITS_TRANSLATER 0x10040
>
> +#define GITS_SGIR 0x20020
> +
> +#define GITS_SGIR_VPEID GENMASK_ULL(47, 32)
> +#define GITS_SGIR_VINTID GENMASK_ULL(7, 0)

GENMASK_ULL(3, 0), though not a problem. Besides,

Reviewed-by: Zenghui Yu <[email protected]>

Thanks

2020-03-12 08:09:08

[permalink] [raw]

Subject: Re: [PATCH v5 14/23] irqchip/gic-v4.1: Add VSGI allocation/teardown

On 2020/3/5 4:33, Marc Zyngier wrote:
> Allocate per-VPE SGIs when initializing the GIC-specific part of the
> VPE data structure.
>
> Signed-off-by: Marc Zyngier <[email protected]>

Reviewed-by: Zenghui Yu <[email protected]>

Thanks

2020-03-12 08:11:27

[permalink] [raw]

Subject: Re: [PATCH v5 15/23] irqchip/gic-v4.1: Add VSGI property setup

On 2020/3/5 4:33, Marc Zyngier wrote:
> Add the SGI configuration entry point for KVM to use.
>
> Signed-off-by: Marc Zyngier <[email protected]>

Reviewed-by: Zenghui Yu <[email protected]>

Thanks

2020-03-12 08:14:50

[permalink] [raw]

Subject: Re: [PATCH v5 16/23] irqchip/gic-v4.1: Eagerly vmap vPEs

On 2020/3/5 4:33, Marc Zyngier wrote:
> Now that we have HW-accelerated SGIs being delivered to VPEs, it
> becomes required to map the VPEs on all ITSs instead of relying
> on the lazy approach that we would use when using the ITS-list
> mechanism.
>
> Signed-off-by: Marc Zyngier <[email protected]>

Reviewed-by: Zenghui Yu <[email protected]>

Thanks

2020-03-12 08:20:46

[permalink] [raw]

Subject: Re: [PATCH v5 13/23] irqchip/gic-v4.1: Move doorbell management to the GICv4 abstraction layer

On 2020/3/5 4:33, Marc Zyngier wrote:
> In order to hide some of the differences between v4.0 and v4.1, move
> the doorbell management out of the KVM code, and into the GICv4-specific
> layer. This allows the calling code to ask for the doorbell when blocking,
> and otherwise to leave the doorbell permanently disabled.
>
> This matches the v4.1 code perfectly, and only results in a minor
> refactoring of the v4.0 code.
>
> Signed-off-by: Marc Zyngier <[email protected]>

Reviewed-by: Zenghui Yu <[email protected]>

Thanks

2020-03-16 17:31:45

[permalink] [raw]

Subject: Re: [PATCH v5 02/23] irqchip/gic-v4.1: Skip absent CPUs while iterating over redistributors

Hi Marc

On 3/4/20 9:33 PM, Marc Zyngier wrote:
> In a system that is only sparsly populated with CPUs, we can end-up with
sparsely
> redistributors structures that are not initialized. Let's make sure we
redistributor structures
> don't try and access those when iterating over them (in this case when
> checking we have a L2 VPE table).
>
> Fixes: 4e6437f12d6e ("irqchip/gic-v4.1: Ensure L2 vPE table is allocated at RD level")
> Signed-off-by: Marc Zyngier <[email protected]>
> Reviewed-by: Zenghui Yu <[email protected]>
> ---
> drivers/irqchip/irq-gic-v3-its.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 83b1186ffcad..da883a691028 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -2452,6 +2452,10 @@ static bool allocate_vpe_l2_table(int cpu, u32 id)
> if (!gic_rdists->has_rvpeid)
> return true;
>
> + /* Skip non-present CPUs */
> + if (!base)
> + return true;
> +
> val = gicr_read_vpropbaser(base + SZ_128K + GICR_VPROPBASER);
>
> esz = FIELD_GET(GICR_VPROPBASER_4_1_ENTRY_SIZE, val) + 1;
>
Reviewed-by: Eric Auger <[email protected]>

Eric

2020-03-16 18:22:41

[permalink] [raw]

Subject: Re: [PATCH v5 10/23] irqchip/gic-v4.1: Plumb mask/unmask SGI callbacks

On 3/4/20 9:33 PM, Marc Zyngier wrote:
> Implement mask/unmask for virtual SGIs by calling into the
> configuration helper.
>
> Signed-off-by: Marc Zyngier <[email protected]>
> Reviewed-by: Zenghui Yu <[email protected]>
> ---
> drivers/irqchip/irq-gic-v3-its.c | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index e0db3f906f87..c93f178914ee 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -3939,6 +3939,22 @@ static void its_configure_sgi(struct irq_data *d, bool clear)
> its_send_single_vcommand(find_4_1_its(), its_build_vsgi_cmd, &desc);
> }
>
> +static void its_sgi_mask_irq(struct irq_data *d)
> +{
> + struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
> +
> + vpe->sgi_config[d->hwirq].enabled = false;
> + its_configure_sgi(d, false);
> +}
> +
> +static void its_sgi_unmask_irq(struct irq_data *d)
> +{
> + struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
> +
> + vpe->sgi_config[d->hwirq].enabled = true;
> + its_configure_sgi(d, false);
> +}
> +
> static int its_sgi_set_affinity(struct irq_data *d,
> const struct cpumask *mask_val,
> bool force)
> @@ -3948,6 +3964,8 @@ static int its_sgi_set_affinity(struct irq_data *d,
>
> static struct irq_chip its_sgi_irq_chip = {
> .name = "GICv4.1-sgi",
> + .irq_mask = its_sgi_mask_irq,
> + .irq_unmask = its_sgi_unmask_irq,
> .irq_set_affinity = its_sgi_set_affinity,
> };
>
>
Reviewed-by: Eric Auger <[email protected]>

Thanks

Eric

2020-03-16 21:46:18

[permalink] [raw]

Subject: Re: [PATCH v5 11/23] irqchip/gic-v4.1: Plumb get/set_irqchip_state SGI callbacks

Hi Marc,

On 3/4/20 9:33 PM, Marc Zyngier wrote:
> To implement the get/set_irqchip_state callbacks (limited to the
> PENDING state), we have to use a particular set of hacks:
>
> - Reading the pending state is done by using a pair of new redistributor
> registers (GICR_VSGIR, GICR_VSGIPENDR), which allow the 16 interrupts
> state to be retrieved.
> - Setting the pending state is done by generating it as we'd otherwise do
> for a guest (writing to GITS_SGIR).
> - Clearing the pending state is done by emiting a VSGI command with the
emitting
> "clear" bit set.
>
> This requires some interesting locking though:
> - When talking to the redistributor, we must make sure that the VPE
> affinity doesn't change, hence taking the VPE lock.
> - At the same time, we must ensure that nobody accesses the same
> redistributor's GICR_VSGI*R registers for a different VPE, which
GICR_VSGIR
> would corrupt the reading of the pending bits. We thus take the
> per-RD spinlock. Much fun.
>
> Signed-off-by: Marc Zyngier <[email protected]>
> ---
> drivers/irqchip/irq-gic-v3-its.c | 73 ++++++++++++++++++++++++++++++
> include/linux/irqchip/arm-gic-v3.h | 14 ++++++
> 2 files changed, 87 insertions(+)
>
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index c93f178914ee..fb2b836c31ff 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -3962,11 +3962,84 @@ static int its_sgi_set_affinity(struct irq_data *d,
> return -EINVAL;
> }
>
> +static int its_sgi_set_irqchip_state(struct irq_data *d,
> + enum irqchip_irq_state which,
> + bool state)
> +{
> + if (which != IRQCHIP_STATE_PENDING)
> + return -EINVAL;
> +
> + if (state) {
> + struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
> + struct its_node *its = find_4_1_its();
> + u64 val;
> +
> + val = FIELD_PREP(GITS_SGIR_VPEID, vpe->vpe_id);
> + val |= FIELD_PREP(GITS_SGIR_VINTID, d->hwirq);
> + writeq_relaxed(val, its->sgir_base + GITS_SGIR - SZ_128K);
> + } else {
> + its_configure_sgi(d, true);
> + }
> +
> + return 0;
> +}
> +
> +static int its_sgi_get_irqchip_state(struct irq_data *d,
> + enum irqchip_irq_state which, bool *val)
> +{
> + struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
> + void __iomem *base;
> + unsigned long flags;
> + u32 count = 1000000; /* 1s! */
where does it come from? I did not find any info in the spec about this
delay.
> + u32 status;
> + int cpu;
> +
> + if (which != IRQCHIP_STATE_PENDING)
> + return -EINVAL;
> +
> + /*
> + * Locking galore! We can race against two different events:
> + *
> + * - Concurent vPE affinity change: we must make sure it cannot
> + * happen, or we'll talk to the wrong redistributor. This is
> + * identical to what happens with vLPIs.
> + *
> + * - Concurrent VSGIPENDR access: As it involves accessing two
> + * MMIO registers, this must be made atomic one way or another.
> + */
> + cpu = vpe_to_cpuid_lock(vpe, &flags);
> + raw_spin_lock(&gic_data_rdist_cpu(cpu)->rd_lock);
> + base = gic_data_rdist_cpu(cpu)->rd_base + SZ_128K;
> + writel_relaxed(vpe->vpe_id, base + GICR_VSGIR);
> + do {
> + status = readl_relaxed(base + GICR_VSGIPENDR);
> + if (!(status & GICR_VSGIPENDR_BUSY))
> + goto out;
> +
> + count--;
> + if (!count) {
> + pr_err_ratelimited("Unable to get SGI status\n");
> + goto out;
> + }
> + cpu_relax();
> + udelay(1);
> + } while(count);
> +
> +out:
> + raw_spin_unlock(&gic_data_rdist_cpu(cpu)->rd_lock);
> + vpe_to_cpuid_unlock(vpe, flags);
> + *val = !!(status & (1 << d->hwirq));
> +
> + return 0;
cascade an error on timeout?
> +}
> +
> static struct irq_chip its_sgi_irq_chip = {
> .name = "GICv4.1-sgi",
> .irq_mask = its_sgi_mask_irq,
> .irq_unmask = its_sgi_unmask_irq,
> .irq_set_affinity = its_sgi_set_affinity,
> + .irq_set_irqchip_state = its_sgi_set_irqchip_state,
> + .irq_get_irqchip_state = its_sgi_get_irqchip_state,
> };
>
> static int its_sgi_irq_domain_alloc(struct irq_domain *domain,
> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
> index fd3be49ac9a5..830d2abf14b3 100644
> --- a/include/linux/irqchip/arm-gic-v3.h
> +++ b/include/linux/irqchip/arm-gic-v3.h
> @@ -345,6 +345,15 @@
> #define GICR_VPENDBASER_4_1_VGRP1EN (1ULL << 58)
> #define GICR_VPENDBASER_4_1_VPEID GENMASK_ULL(15, 0)
>
> +#define GICR_VSGIR 0x0080
> +
> +#define GICR_VSGIR_VPEID GENMASK(15, 0)
> +
> +#define GICR_VSGIPENDR 0x0088
> +
> +#define GICR_VSGIPENDR_BUSY (1U << 31)
> +#define GICR_VSGIPENDR_PENDING GENMASK(15, 0)
> +
> /*
> * ITS registers, offsets from ITS_base
> */
> @@ -368,6 +377,11 @@
>
> #define GITS_TRANSLATER 0x10040
>
> +#define GITS_SGIR 0x20020
> +
> +#define GITS_SGIR_VPEID GENMASK_ULL(47, 32)
> +#define GITS_SGIR_VINTID GENMASK_ULL(7, 0)
as spotted by Zenghui 3, 0
> +
> #define GITS_CTLR_ENABLE (1U << 0)
> #define GITS_CTLR_ImDe (1U << 1)
> #define GITS_CTLR_ITS_NUMBER_SHIFT 4
>

Thanks

Eric

2020-03-17 03:29:36

[permalink] [raw]

Subject: Re: [PATCH v5 16/23] irqchip/gic-v4.1: Eagerly vmap vPEs

Hi Marc,

On 2020/3/5 4:33, Marc Zyngier wrote:
> Now that we have HW-accelerated SGIs being delivered to VPEs, it
> becomes required to map the VPEs on all ITSs instead of relying
> on the lazy approach that we would use when using the ITS-list
> mechanism.
>
> Signed-off-by: Marc Zyngier <[email protected]>

Before GICv4.1, we use vlpi_count to evaluate whether the vPE has been
mapped on the specified ITS, and use this refcount to only issue VMOVP
to those involved ITSes. It's broken after this patch.

We may need to re-evaluate "whether the vPE is mapped" now that we're at
GICv4.1, otherwise *no* VMOVP will be issued on the v4.1 capable machine
(I mean those without single VMOVP support).

What I'm saying is something like below (only an idea, it even can't
compile), any thoughts?

diff --git a/drivers/irqchip/irq-gic-v3-its.c
b/drivers/irqchip/irq-gic-v3-its.c
index 2e12bc52e3a2..3450b5e847ca 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -198,7 +198,8 @@ static u16 get_its_list(struct its_vm *vm)
if (!is_v4(its))
continue;

- if (vm->vlpi_count[its->list_nr])
+ if (vm->vlpi_count[its->list_nr] ||
+ gic_requires_eager_mapping())
__set_bit(its->list_nr, &its_list);
}

@@ -1295,7 +1296,8 @@ static void its_send_vmovp(struct its_vpe *vpe)
if (!is_v4(its))
continue;

- if (!vpe->its_vm->vlpi_count[its->list_nr])
+ if (!vpe->its_vm->vlpi_count[its->list_nr] &&
+ !gic_requires_eager_mapping())
continue;

desc.its_vmovp_cmd.col = &its->collections[col_id];

Thanks,
Zenghui

> ---
> drivers/irqchip/irq-gic-v3-its.c | 39 +++++++++++++++++++++++++-------
> 1 file changed, 31 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index b65fba67bd85..6277b3e3731f 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -1586,12 +1586,31 @@ static int its_irq_set_irqchip_state(struct irq_data *d,
> return 0;
> }
>
> +/*
> + * Two favourable cases:
> + *
> + * (a) Either we have a GICv4.1, and all vPEs have to be mapped at all times
> + * for vSGI delivery
> + *
> + * (b) Or the ITSs do not use a list map, meaning that VMOVP is cheap enough
> + * and we're better off mapping all VPEs always
> + *
> + * If neither (a) nor (b) is true, then we map vPEs on demand.
> + *
> + */
> +static bool gic_requires_eager_mapping(void)
> +{
> + if (!its_list_map || gic_rdists->has_rvpeid)
> + return true;
> +
> + return false;
> +}
> +
> static void its_map_vm(struct its_node *its, struct its_vm *vm)
> {
> unsigned long flags;
>
> - /* Not using the ITS list? Everything is always mapped. */
> - if (!its_list_map)
> + if (gic_requires_eager_mapping())
> return;
>
> raw_spin_lock_irqsave(&vmovp_lock, flags);
> @@ -1625,7 +1644,7 @@ static void its_unmap_vm(struct its_node *its, struct its_vm *vm)
> unsigned long flags;
>
> /* Not using the ITS list? Everything is always mapped. */
> - if (!its_list_map)
> + if (gic_requires_eager_mapping())
> return;
>
> raw_spin_lock_irqsave(&vmovp_lock, flags);
> @@ -4260,8 +4279,12 @@ static int its_vpe_irq_domain_activate(struct irq_domain *domain,
> struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
> struct its_node *its;
>
> - /* If we use the list map, we issue VMAPP on demand... */
> - if (its_list_map)
> + /*
> + * If we use the list map, we issue VMAPP on demand... Unless
> + * we're on a GICv4.1 and we eagerly map the VPE on all ITSs
> + * so that VSGIs can work.
> + */
> + if (!gic_requires_eager_mapping())
> return 0;
>
> /* Map the VPE to the first possible CPU */
> @@ -4287,10 +4310,10 @@ static void its_vpe_irq_domain_deactivate(struct irq_domain *domain,
> struct its_node *its;
>
> /*
> - * If we use the list map, we unmap the VPE once no VLPIs are
> - * associated with the VM.
> + * If we use the list map on GICv4.0, we unmap the VPE once no
> + * VLPIs are associated with the VM.
> */
> - if (its_list_map)
> + if (!gic_requires_eager_mapping())
> return;
>
> list_for_each_entry(its, &its_nodes, entry) {
>

2020-03-17 10:31:32

[permalink] [raw]

Subject: Re: [PATCH v5 15/23] irqchip/gic-v4.1: Add VSGI property setup

Hi Marc,

On 3/4/20 9:33 PM, Marc Zyngier wrote:
> Add the SGI configuration entry point for KVM to use.
>
> Signed-off-by: Marc Zyngier <[email protected]>
> ---
> drivers/irqchip/irq-gic-v3-its.c | 2 +-
> drivers/irqchip/irq-gic-v4.c | 13 +++++++++++++
> include/linux/irqchip/arm-gic-v4.h | 3 ++-
> 3 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index effb0e0b0c9d..b65fba67bd85 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -4039,7 +4039,7 @@ static int its_sgi_set_vcpu_affinity(struct irq_data *d, void *vcpu_info)
> struct its_cmd_info *info = vcpu_info;
>
> switch (info->cmd_type) {
> - case PROP_UPDATE_SGI:
> + case PROP_UPDATE_VSGI:
This change rather belongs to
[PATCH v5 12/23] irqchip/gic-v4.1: Plumb set_vcpu_affinity SGI callbacks
> vpe->sgi_config[d->hwirq].priority = info->priority;
> vpe->sgi_config[d->hwirq].group = info->group;
> its_configure_sgi(d, false);
> diff --git a/drivers/irqchip/irq-gic-v4.c b/drivers/irqchip/irq-gic-v4.c
> index 99b33f60ac63..0c18714ae13e 100644
> --- a/drivers/irqchip/irq-gic-v4.c
> +++ b/drivers/irqchip/irq-gic-v4.c
> @@ -320,6 +320,19 @@ int its_prop_update_vlpi(int irq, u8 config, bool inv)
> return irq_set_vcpu_affinity(irq, &info);
> }
>
> +int its_prop_update_vsgi(int irq, u8 priority, bool group)
> +{
> + struct its_cmd_info info = {
> + .cmd_type = PROP_UPDATE_VSGI,
> + {
> + .priority = priority,
> + .group = group,
> + },
> + };
> +
> + return irq_set_vcpu_affinity(irq, &info);
> +}
> +
> int its_init_v4(struct irq_domain *domain,
> const struct irq_domain_ops *vpe_ops,
> const struct irq_domain_ops *sgi_ops)
> diff --git a/include/linux/irqchip/arm-gic-v4.h b/include/linux/irqchip/arm-gic-v4.h
> index 0bb111b4a504..6976b8331b60 100644
> --- a/include/linux/irqchip/arm-gic-v4.h
> +++ b/include/linux/irqchip/arm-gic-v4.h
> @@ -105,7 +105,7 @@ enum its_vcpu_info_cmd_type {
> SCHEDULE_VPE,
> DESCHEDULE_VPE,
> INVALL_VPE,
> - PROP_UPDATE_SGI,
> + PROP_UPDATE_VSGI,
same
> };
>
> struct its_cmd_info {
> @@ -134,6 +134,7 @@ int its_map_vlpi(int irq, struct its_vlpi_map *map);
> int its_get_vlpi(int irq, struct its_vlpi_map *map);
> int its_unmap_vlpi(int irq);
> int its_prop_update_vlpi(int irq, u8 config, bool inv);
> +int its_prop_update_vsgi(int irq, u8 priority, bool group);
>
> struct irq_domain_ops;
> int its_init_v4(struct irq_domain *domain,
>
Besides
Reviewed-by: Eric Auger <[email protected]>

Thanks

Eric

2020-03-18 03:19:25

[permalink] [raw]

Subject: Re: [PATCH v5 21/23] KVM: arm64: GICv4.1: Reload VLPI configuration on distributor enable/disable

On 2020/3/5 4:33, Marc Zyngier wrote:
> Each time a Group-enable bit gets flipped, the state of these bits
> needs to be forwarded to the hardware. This is a pretty heavy
> handed operation, requiring all vcpus to reload their GICv4
> configuration. It is thus implemented as a new request type.

[note to myself]
... and the status are forwarded to HW by programming VGrp{0,1}En
fields of GICR_VPENDBASER when vPEs are made resident next time.

>
> Of course, we only support Group-1 for now...
>
> Signed-off-by: Marc Zyngier <[email protected]>

Reviewed-by: Zenghui Yu <[email protected]>

Thanks

2020-03-18 03:21:49

[permalink] [raw]

Subject: Re: [PATCH v5 23/23] KVM: arm64: GICv4.1: Expose HW-based SGIs in debugfs

Hi Marc,

On 2020/3/5 4:33, Marc Zyngier wrote:
> The vgic-state debugfs file could do with showing the pending state
> of the HW-backed SGIs. Plug it into the low-level code.
>
> Signed-off-by: Marc Zyngier <[email protected]>

Reviewed-by: Zenghui Yu <[email protected]>

Thanks

> ---
> virt/kvm/arm/vgic/vgic-debug.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/virt/kvm/arm/vgic/vgic-debug.c b/virt/kvm/arm/vgic/vgic-debug.c
> index cc12fe9b2df3..b13a9e3f99dd 100644
> --- a/virt/kvm/arm/vgic/vgic-debug.c
> +++ b/virt/kvm/arm/vgic/vgic-debug.c
> @@ -178,6 +178,8 @@ static void print_irq_state(struct seq_file *s, struct vgic_irq *irq,
> struct kvm_vcpu *vcpu)
> {
> char *type;
> + bool pending;
> +
> if (irq->intid < VGIC_NR_SGIS)
> type = "SGI";
> else if (irq->intid < VGIC_NR_PRIVATE_IRQS)
> @@ -190,6 +192,16 @@ static void print_irq_state(struct seq_file *s, struct vgic_irq *irq,
> if (irq->intid ==0 || irq->intid == VGIC_NR_PRIVATE_IRQS)
> print_header(s, irq, vcpu);
>
> + pending = irq->pending_latch;
> + if (irq->hw && vgic_irq_is_sgi(irq->intid)) {
> + int err;
> +
> + err = irq_get_irqchip_state(irq->host_irq,
> + IRQCHIP_STATE_PENDING,
> + &pending);
> + WARN_ON_ONCE(err);
> + }
> +
> seq_printf(s, " %s %4d "
> " %2d "
> "%d%d%d%d%d%d%d "
> @@ -201,7 +213,7 @@ static void print_irq_state(struct seq_file *s, struct vgic_irq *irq,
> "\n",
> type, irq->intid,
> (irq->target_vcpu) ? irq->target_vcpu->vcpu_id : -1,
> - irq->pending_latch,
> + pending,
> irq->line_level,
> irq->active,
> irq->enabled,
>

2020-03-18 03:29:55

[permalink] [raw]

Subject: Re: [PATCH v5 18/23] KVM: arm64: GICv4.1: Add direct injection capability to SGI registers

Hi Marc,

On 2020/3/5 4:33, Marc Zyngier wrote:
> Most of the GICv3 emulation code that deals with SGIs now has to be
> aware of the v4.1 capabilities in order to benefit from it.
>
> Add such support, keyed on the interrupt having the hw flag set and
> being a SGI.
>
> Signed-off-by: Marc Zyngier <[email protected]>

Reviewed-by: Zenghui Yu <[email protected]>

Thanks

2020-03-18 06:36:20

[permalink] [raw]

Subject: Re: [PATCH v5 20/23] KVM: arm64: GICv4.1: Plumb SGI implementation selection in the distributor

Hi Marc,

On 2020/3/5 4:33, Marc Zyngier wrote:
> The GICv4.1 architecture gives the hypervisor the option to let
> the guest choose whether it wants the good old SGIs with an
> active state, or the new, HW-based ones that do not have one.
>
> For this, plumb the configuration of SGIs into the GICv3 MMIO
> handling, present the GICD_TYPER2.nASSGIcap to the guest,
> and handle the GICD_CTLR.nASSGIreq setting.
>
> In order to be able to deal with the restore of a guest, also
> apply the GICD_CTLR.nASSGIreq setting at first run so that we
> can move the restored SGIs to the HW if that's what the guest
> had selected in a previous life.

I'm okay with the restore path. But it seems that we still fail to
save the pending state of vSGI - software pending_latch of HW-based
vSGIs will not be updated (and always be false) because we directly
inject them through ITS, so vgic_v3_uaccess_read_pending() can't
tell the correct pending state to user-space (the correct one should
be latched in HW).

It would be good if we can sync the hardware state into pending_latch
at an appropriate time (just before save), but not sure if we can...

>
> Signed-off-by: Marc Zyngier <[email protected]>
> ---
> virt/kvm/arm/vgic/vgic-mmio-v3.c | 48 ++++++++++++++++++++++++++++++--
> virt/kvm/arm/vgic/vgic-v3.c | 2 ++
> 2 files changed, 48 insertions(+), 2 deletions(-)
>
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index de89da76a379..442f3b8c2559 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -3,6 +3,7 @@
> * VGICv3 MMIO handling functions
> */
>
> +#include <linux/bitfield.h>
> #include <linux/irqchip/arm-gic-v3.h>
> #include <linux/kvm.h>
> #include <linux/kvm_host.h>
> @@ -70,6 +71,8 @@ static unsigned long vgic_mmio_read_v3_misc(struct kvm_vcpu *vcpu,
> if (vgic->enabled)
> value |= GICD_CTLR_ENABLE_SS_G1;
> value |= GICD_CTLR_ARE_NS | GICD_CTLR_DS;
> + if (kvm_vgic_global_state.has_gicv4_1 && vgic->nassgireq)

Looking at how we handle the GICD_CTLR.nASSGIreq setting, I think
"nassgireq==true" already indicates "has_gicv4_1==true". So this
can be simplified.

But I wonder that should we use nassgireq to *only* keep track what
the guest had written into the GICD_CTLR.nASSGIreq. If not, we may
lose the guest-request bit after migration among hosts with different
has_gicv4_1 settings.

The remaining patches all look good to me :-). I will wait for you to
confirm these two concerns.

Thanks,
Zenghui

> + value |= GICD_CTLR_nASSGIreq;
> break;
> case GICD_TYPER:
> value = vgic->nr_spis + VGIC_NR_PRIVATE_IRQS;
> @@ -81,6 +84,10 @@ static unsigned long vgic_mmio_read_v3_misc(struct kvm_vcpu *vcpu,
> value |= (INTERRUPT_ID_BITS_SPIS - 1) << 19;
> }
> break;
> + case GICD_TYPER2:
> + if (kvm_vgic_global_state.has_gicv4_1)
> + value = GICD_TYPER2_nASSGIcap;
> + break;
> case GICD_IIDR:
> value = (PRODUCT_ID_KVM << GICD_IIDR_PRODUCT_ID_SHIFT) |
> (vgic->implementation_rev << GICD_IIDR_REVISION_SHIFT) |
> @@ -98,17 +105,43 @@ static void vgic_mmio_write_v3_misc(struct kvm_vcpu *vcpu,
> unsigned long val)
> {
> struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> - bool was_enabled = dist->enabled;
>
> switch (addr & 0x0c) {
> - case GICD_CTLR:
> + case GICD_CTLR: {
> + bool was_enabled, is_hwsgi;
> +
> + mutex_lock(&vcpu->kvm->lock);
> +
> + was_enabled = dist->enabled;
> + is_hwsgi = dist->nassgireq;
> +
> dist->enabled = val & GICD_CTLR_ENABLE_SS_G1;
>
> + /* Not a GICv4.1? No HW SGIs */
> + if (!kvm_vgic_global_state.has_gicv4_1)
> + val &= ~GICD_CTLR_nASSGIreq;
> +
> + /* Dist stays enabled? nASSGIreq is RO */
> + if (was_enabled && dist->enabled) {
> + val &= ~GICD_CTLR_nASSGIreq;
> + val |= FIELD_PREP(GICD_CTLR_nASSGIreq, is_hwsgi);
> + }
> +
> + /* Switching HW SGIs? */
> + dist->nassgireq = val & GICD_CTLR_nASSGIreq;
> + if (is_hwsgi != dist->nassgireq)
> + vgic_v4_configure_vsgis(vcpu->kvm);
> +
> if (!was_enabled && dist->enabled)
> vgic_kick_vcpus(vcpu->kvm);
> +
> + mutex_unlock(&vcpu->kvm->lock);
> break;
> + }
> case GICD_TYPER:
> + case GICD_TYPER2:
> case GICD_IIDR:
> + /* This is at best for documentation purposes... */
> return;
> }
> }
> @@ -117,10 +150,21 @@ static int vgic_mmio_uaccess_write_v3_misc(struct kvm_vcpu *vcpu,
> gpa_t addr, unsigned int len,
> unsigned long val)
> {
> + struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> +
> switch (addr & 0x0c) {
> case GICD_IIDR:
> if (val != vgic_mmio_read_v3_misc(vcpu, addr, len))
> return -EINVAL;
> + return 0;
> + case GICD_CTLR:
> + /* Not a GICv4.1? No HW SGIs */
> + if (!kvm_vgic_global_state.has_gicv4_1)
> + val &= ~GICD_CTLR_nASSGIreq;
> +
> + dist->enabled = val & GICD_CTLR_ENABLE_SS_G1;
> + dist->nassgireq = val & GICD_CTLR_nASSGIreq;
> + return 0;
> }
>
> vgic_mmio_write_v3_misc(vcpu, addr, len, val);
> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
> index 1bc09b523486..2c9fc13e2c59 100644
> --- a/virt/kvm/arm/vgic/vgic-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-v3.c
> @@ -540,6 +540,8 @@ int vgic_v3_map_resources(struct kvm *kvm)
> goto out;
> }
>
> + if (kvm_vgic_global_state.has_gicv4_1)
> + vgic_v4_configure_vsgis(kvm);
> dist->ready = true;
>
> out:
>

2020-03-19 10:29:32

[permalink] [raw]

Subject: Re: [PATCH v5 11/23] irqchip/gic-v4.1: Plumb get/set_irqchip_state SGI callbacks

Hi Eric,

On 2020-03-16 21:43, Auger Eric wrote:
> Hi Marc,
>
> On 3/4/20 9:33 PM, Marc Zyngier wrote:
>> To implement the get/set_irqchip_state callbacks (limited to the
>> PENDING state), we have to use a particular set of hacks:
>>
>> - Reading the pending state is done by using a pair of new
>> redistributor
>> registers (GICR_VSGIR, GICR_VSGIPENDR), which allow the 16
>> interrupts
>> state to be retrieved.
>> - Setting the pending state is done by generating it as we'd otherwise
>> do
>> for a guest (writing to GITS_SGIR).
>> - Clearing the pending state is done by emiting a VSGI command with
>> the
> emitting
>> "clear" bit set.
>>
>> This requires some interesting locking though:
>> - When talking to the redistributor, we must make sure that the VPE
>> affinity doesn't change, hence taking the VPE lock.
>> - At the same time, we must ensure that nobody accesses the same
>> redistributor's GICR_VSGI*R registers for a different VPE, which
> GICR_VSGIR
>> would corrupt the reading of the pending bits. We thus take the
>> per-RD spinlock. Much fun.
>>
>> Signed-off-by: Marc Zyngier <[email protected]>
>> ---
>> drivers/irqchip/irq-gic-v3-its.c | 73
>> ++++++++++++++++++++++++++++++
>> include/linux/irqchip/arm-gic-v3.h | 14 ++++++
>> 2 files changed, 87 insertions(+)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c
>> b/drivers/irqchip/irq-gic-v3-its.c
>> index c93f178914ee..fb2b836c31ff 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -3962,11 +3962,84 @@ static int its_sgi_set_affinity(struct
>> irq_data *d,
>> return -EINVAL;
>> }
>>
>> +static int its_sgi_set_irqchip_state(struct irq_data *d,
>> + enum irqchip_irq_state which,
>> + bool state)
>> +{
>> + if (which != IRQCHIP_STATE_PENDING)
>> + return -EINVAL;
>> +
>> + if (state) {
>> + struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
>> + struct its_node *its = find_4_1_its();
>> + u64 val;
>> +
>> + val = FIELD_PREP(GITS_SGIR_VPEID, vpe->vpe_id);
>> + val |= FIELD_PREP(GITS_SGIR_VINTID, d->hwirq);
>> + writeq_relaxed(val, its->sgir_base + GITS_SGIR - SZ_128K);
>> + } else {
>> + its_configure_sgi(d, true);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int its_sgi_get_irqchip_state(struct irq_data *d,
>> + enum irqchip_irq_state which, bool *val)
>> +{
>> + struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
>> + void __iomem *base;
>> + unsigned long flags;
>> + u32 count = 1000000; /* 1s! */
> where does it come from? I did not find any info in the spec about this
> delay.

There is no such thing in the spec. However, these timeouts have proven
to be
very useful in detecting broken HW (I'm lucky enough to have such
wonders
at hand...), as well as SW bugs. The ! second value is purely empirical
(if a whole second is not enough for things to move, they're as good as
dead).

>> + u32 status;
>> + int cpu;
>> +
>> + if (which != IRQCHIP_STATE_PENDING)
>> + return -EINVAL;
>> +
>> + /*
>> + * Locking galore! We can race against two different events:
>> + *
>> + * - Concurent vPE affinity change: we must make sure it cannot
>> + * happen, or we'll talk to the wrong redistributor. This
>> is
>> + * identical to what happens with vLPIs.
>> + *
>> + * - Concurrent VSGIPENDR access: As it involves accessing two
>> + * MMIO registers, this must be made atomic one way or
>> another.
>> + */
>> + cpu = vpe_to_cpuid_lock(vpe, &flags);
>> + raw_spin_lock(&gic_data_rdist_cpu(cpu)->rd_lock);
>> + base = gic_data_rdist_cpu(cpu)->rd_base + SZ_128K;
>> + writel_relaxed(vpe->vpe_id, base + GICR_VSGIR);
>> + do {
>> + status = readl_relaxed(base + GICR_VSGIPENDR);
>> + if (!(status & GICR_VSGIPENDR_BUSY))
>> + goto out;
>> +
>> + count--;
>> + if (!count) {
>> + pr_err_ratelimited("Unable to get SGI status\n");
>> + goto out;
>> + }
>> + cpu_relax();
>> + udelay(1);
>> + } while(count);
>> +
>> +out:
>> + raw_spin_unlock(&gic_data_rdist_cpu(cpu)->rd_lock);
>> + vpe_to_cpuid_unlock(vpe, flags);
>> + *val = !!(status & (1 << d->hwirq));
>> +
>> + return 0;
> cascade an error on timeout?

Sure, that's a good idea.

Thanks,

M.
--
Jazz is not dead. It just smells funny...

2020-03-19 10:57:52

[permalink] [raw]

Subject: Re: [PATCH v5 16/23] irqchip/gic-v4.1: Eagerly vmap vPEs

On 2020-03-17 02:49, Zenghui Yu wrote:
> Hi Marc,
>
> On 2020/3/5 4:33, Marc Zyngier wrote:
>> Now that we have HW-accelerated SGIs being delivered to VPEs, it
>> becomes required to map the VPEs on all ITSs instead of relying
>> on the lazy approach that we would use when using the ITS-list
>> mechanism.
>>
>> Signed-off-by: Marc Zyngier <[email protected]>
>
> Before GICv4.1, we use vlpi_count to evaluate whether the vPE has been
> mapped on the specified ITS, and use this refcount to only issue VMOVP
> to those involved ITSes. It's broken after this patch.
>
> We may need to re-evaluate "whether the vPE is mapped" now that we're
> at
> GICv4.1, otherwise *no* VMOVP will be issued on the v4.1 capable
> machine
> (I mean those without single VMOVP support).
>
> What I'm saying is something like below (only an idea, it even can't
> compile), any thoughts?
>
>
> diff --git a/drivers/irqchip/irq-gic-v3-its.c
> b/drivers/irqchip/irq-gic-v3-its.c
> index 2e12bc52e3a2..3450b5e847ca 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -198,7 +198,8 @@ static u16 get_its_list(struct its_vm *vm)
> if (!is_v4(its))
> continue;
>
> - if (vm->vlpi_count[its->list_nr])
> + if (vm->vlpi_count[its->list_nr] ||
> + gic_requires_eager_mapping())
> __set_bit(its->list_nr, &its_list);
> }
>
> @@ -1295,7 +1296,8 @@ static void its_send_vmovp(struct its_vpe *vpe)
> if (!is_v4(its))
> continue;
>
> - if (!vpe->its_vm->vlpi_count[its->list_nr])
> + if (!vpe->its_vm->vlpi_count[its->list_nr] &&
> + !gic_requires_eager_mapping())
> continue;
>
> desc.its_vmovp_cmd.col = &its->collections[col_id];

It took me a while to wrap my head around that one, but you're as usual
spot on.

The use of gic_requires_eager_mapping() is a bit confusing here, as it
isn't
so much that the VPE is eagerly mapped, but the predicate on which we
evaluate
the need for a VMOVP. How about this:

diff --git a/drivers/irqchip/irq-gic-v3-its.c
b/drivers/irqchip/irq-gic-v3-its.c
index cd6451e190c9..348f7a909a69 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -189,6 +189,15 @@ static DEFINE_IDA(its_vpeid_ida);
#define gic_data_rdist_rd_base() (gic_data_rdist()->rd_base)
#define gic_data_rdist_vlpi_base() (gic_data_rdist_rd_base() + SZ_128K)

+/*
+ * Skip ITSs that have no vLPIs mapped, unless we're on GICv4.1, as we
+ * always have vSGIs mapped.
+ */
+static bool require_its_list_vmovp(struct its_vm *vm, struct its_node
*its)
+{
+ return (gic_rdists->has_rvpeid || vm->vlpi_count[its->list_nr]);
+}
+
static u16 get_its_list(struct its_vm *vm)
{
struct its_node *its;
@@ -198,7 +207,7 @@ static u16 get_its_list(struct its_vm *vm)
if (!is_v4(its))
continue;

- if (vm->vlpi_count[its->list_nr])
+ if (require_its_list_vmovp(vm, its))
__set_bit(its->list_nr, &its_list);
}

@@ -1295,7 +1304,7 @@ static void its_send_vmovp(struct its_vpe *vpe)
if (!is_v4(its))
continue;

- if (!vpe->its_vm->vlpi_count[its->list_nr])
+ if (!require_its_list_vmovp(vpe->its_vm, its))
continue;

desc.its_vmovp_cmd.col = &its->collections[col_id];

Thanks,

M.
--
Jazz is not dead. It just smells funny...

2020-03-19 10:59:30

[permalink] [raw]

Subject: Re: [PATCH v5 15/23] irqchip/gic-v4.1: Add VSGI property setup

Hi Eric,

On 2020-03-17 10:30, Auger Eric wrote:
> Hi Marc,
>
> On 3/4/20 9:33 PM, Marc Zyngier wrote:
>> Add the SGI configuration entry point for KVM to use.
>>
>> Signed-off-by: Marc Zyngier <[email protected]>
>> ---
>> drivers/irqchip/irq-gic-v3-its.c | 2 +-
>> drivers/irqchip/irq-gic-v4.c | 13 +++++++++++++
>> include/linux/irqchip/arm-gic-v4.h | 3 ++-
>> 3 files changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c
>> b/drivers/irqchip/irq-gic-v3-its.c
>> index effb0e0b0c9d..b65fba67bd85 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -4039,7 +4039,7 @@ static int its_sgi_set_vcpu_affinity(struct
>> irq_data *d, void *vcpu_info)
>> struct its_cmd_info *info = vcpu_info;
>>
>> switch (info->cmd_type) {
>> - case PROP_UPDATE_SGI:
>> + case PROP_UPDATE_VSGI:
> This change rather belongs to
> [PATCH v5 12/23] irqchip/gic-v4.1: Plumb set_vcpu_affinity SGI
> callbacks

Absolutely. I messed up a rebase, obviously.

Thanks,

M.
--
Jazz is not dead. It just smells funny...

2020-03-19 12:12:30

[permalink] [raw]

Subject: Re: [PATCH v5 20/23] KVM: arm64: GICv4.1: Plumb SGI implementation selection in the distributor

Hi Zenghui,

On 2020-03-18 06:34, Zenghui Yu wrote:
> Hi Marc,
>
> On 2020/3/5 4:33, Marc Zyngier wrote:
>> The GICv4.1 architecture gives the hypervisor the option to let
>> the guest choose whether it wants the good old SGIs with an
>> active state, or the new, HW-based ones that do not have one.
>>
>> For this, plumb the configuration of SGIs into the GICv3 MMIO
>> handling, present the GICD_TYPER2.nASSGIcap to the guest,
>> and handle the GICD_CTLR.nASSGIreq setting.
>>
>> In order to be able to deal with the restore of a guest, also
>> apply the GICD_CTLR.nASSGIreq setting at first run so that we
>> can move the restored SGIs to the HW if that's what the guest
>> had selected in a previous life.
>
> I'm okay with the restore path. But it seems that we still fail to
> save the pending state of vSGI - software pending_latch of HW-based
> vSGIs will not be updated (and always be false) because we directly
> inject them through ITS, so vgic_v3_uaccess_read_pending() can't
> tell the correct pending state to user-space (the correct one should
> be latched in HW).
>
> It would be good if we can sync the hardware state into pending_latch
> at an appropriate time (just before save), but not sure if we can...

The problem is to find the "appropriate time". It would require to
define
a point in the save sequence where we transition the state from HW to
SW. I'm not keen on adding more state than we already have.

But what we can do is to just ask the HW to give us the right state
on userspace access, at all times. How about this:

diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c
b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index 48fd9fc229a2..281fe7216c59 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -305,8 +305,18 @@ static unsigned long
vgic_v3_uaccess_read_pending(struct kvm_vcpu *vcpu,
*/
for (i = 0; i < len * 8; i++) {
struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
+ bool state = irq->pending_latch;

- if (irq->pending_latch)
+ if (irq->hw && vgic_irq_is_sgi(irq->intid)) {
+ int err;
+
+ err = irq_get_irqchip_state(irq->host_irq,
+ IRQCHIP_STATE_PENDING,
+ &state);
+ WARN_ON(err);
+ }
+
+ if (state)
value |= (1U << i);

vgic_put_irq(vcpu->kvm, irq);

I can add this to "KVM: arm64: GICv4.1: Add direct injection capability
to SGI registers".

>
>>
>> Signed-off-by: Marc Zyngier <[email protected]>
>> ---
>> virt/kvm/arm/vgic/vgic-mmio-v3.c | 48
>> ++++++++++++++++++++++++++++++--
>> virt/kvm/arm/vgic/vgic-v3.c | 2 ++
>> 2 files changed, 48 insertions(+), 2 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> index de89da76a379..442f3b8c2559 100644
>> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> @@ -3,6 +3,7 @@
>> * VGICv3 MMIO handling functions
>> */
>> +#include <linux/bitfield.h>
>> #include <linux/irqchip/arm-gic-v3.h>
>> #include <linux/kvm.h>
>> #include <linux/kvm_host.h>
>> @@ -70,6 +71,8 @@ static unsigned long vgic_mmio_read_v3_misc(struct
>> kvm_vcpu *vcpu,
>> if (vgic->enabled)
>> value |= GICD_CTLR_ENABLE_SS_G1;
>> value |= GICD_CTLR_ARE_NS | GICD_CTLR_DS;
>> + if (kvm_vgic_global_state.has_gicv4_1 && vgic->nassgireq)
>
> Looking at how we handle the GICD_CTLR.nASSGIreq setting, I think
> "nassgireq==true" already indicates "has_gicv4_1==true". So this
> can be simplified.

Indeed. I've now dropped the has_gicv4.1 check.

> But I wonder that should we use nassgireq to *only* keep track what
> the guest had written into the GICD_CTLR.nASSGIreq. If not, we may
> lose the guest-request bit after migration among hosts with different
> has_gicv4_1 settings.

I'm unsure of what you're suggesting here. If userspace tries to set
GICD_CTLR.nASSGIreq on a non-4.1 host, this bit will not latch.
Userspace can check that at restore time. Or we could fail the
userspace write, which is a bit odd (the bit is otherwise RES0).

Could you clarify your proposal?

> The remaining patches all look good to me :-). I will wait for you to
> confirm these two concerns.

Thanks,

M.
--
Jazz is not dead. It just smells funny...

2020-03-19 12:20:05

[permalink] [raw]

Subject: Re: [PATCH v5 21/23] KVM: arm64: GICv4.1: Reload VLPI configuration on distributor enable/disable

On 2020-03-18 03:17, Zenghui Yu wrote:
> On 2020/3/5 4:33, Marc Zyngier wrote:
>> Each time a Group-enable bit gets flipped, the state of these bits
>> needs to be forwarded to the hardware. This is a pretty heavy
>> handed operation, requiring all vcpus to reload their GICv4
>> configuration. It is thus implemented as a new request type.
>
> [note to myself]
> ... and the status are forwarded to HW by programming VGrp{0,1}En
> fields of GICR_VPENDBASER when vPEs are made resident next time.

I've added something based on this comment to the commit message.

Thanks!

M.
--
Jazz is not dead. It just smells funny...

2020-03-19 15:07:38

[permalink] [raw]

Subject: Re: [PATCH v5 23/23] KVM: arm64: GICv4.1: Expose HW-based SGIs in debugfs

Hi Marc,

On 3/4/20 9:33 PM, Marc Zyngier wrote:
> The vgic-state debugfs file could do with showing the pending state
> of the HW-backed SGIs. Plug it into the low-level code.
>
> Signed-off-by: Marc Zyngier <[email protected]>
> ---
> virt/kvm/arm/vgic/vgic-debug.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/virt/kvm/arm/vgic/vgic-debug.c b/virt/kvm/arm/vgic/vgic-debug.c
> index cc12fe9b2df3..b13a9e3f99dd 100644
> --- a/virt/kvm/arm/vgic/vgic-debug.c
> +++ b/virt/kvm/arm/vgic/vgic-debug.c
> @@ -178,6 +178,8 @@ static void print_irq_state(struct seq_file *s, struct vgic_irq *irq,
> struct kvm_vcpu *vcpu)
> {
> char *type;
> + bool pending;
nit: can be directly initialized to irq->pending_latch
> +
> if (irq->intid < VGIC_NR_SGIS)
> type = "SGI";
> else if (irq->intid < VGIC_NR_PRIVATE_IRQS)
> @@ -190,6 +192,16 @@ static void print_irq_state(struct seq_file *s, struct vgic_irq *irq,
> if (irq->intid ==0 || irq->intid == VGIC_NR_PRIVATE_IRQS)
> print_header(s, irq, vcpu);
>
> + pending = irq->pending_latch;
> + if (irq->hw && vgic_irq_is_sgi(irq->intid)) {
> + int err;
> +
> + err = irq_get_irqchip_state(irq->host_irq,
> + IRQCHIP_STATE_PENDING,
> + &pending);
> + WARN_ON_ONCE(err);
> + }
> +
> seq_printf(s, " %s %4d "
> " %2d "
> "%d%d%d%d%d%d%d "
> @@ -201,7 +213,7 @@ static void print_irq_state(struct seq_file *s, struct vgic_irq *irq,
> "\n",
> type, irq->intid,
> (irq->target_vcpu) ? irq->target_vcpu->vcpu_id : -1,
> - irq->pending_latch,
> + pending,
> irq->line_level,
> irq->active,
> irq->enabled,
>
The patch looks good to me but I am now lost about how we retrieve the
pending stat of other hw mapped interrupts. Looks we use
irq->pending_latch always. Is that correct?

For the patch:
Reviewed-by: Eric Auger <[email protected]>

Thanks

Eric

2020-03-19 15:22:22

[permalink] [raw]

Subject: Re: [PATCH v5 23/23] KVM: arm64: GICv4.1: Expose HW-based SGIs in debugfs

Hi Eric,

On 2020-03-19 15:05, Auger Eric wrote:
> Hi Marc,
>
> On 3/4/20 9:33 PM, Marc Zyngier wrote:
>> The vgic-state debugfs file could do with showing the pending state
>> of the HW-backed SGIs. Plug it into the low-level code.
>>
>> Signed-off-by: Marc Zyngier <[email protected]>
>> ---
>> virt/kvm/arm/vgic/vgic-debug.c | 14 +++++++++++++-
>> 1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-debug.c
>> b/virt/kvm/arm/vgic/vgic-debug.c
>> index cc12fe9b2df3..b13a9e3f99dd 100644
>> --- a/virt/kvm/arm/vgic/vgic-debug.c
>> +++ b/virt/kvm/arm/vgic/vgic-debug.c
>> @@ -178,6 +178,8 @@ static void print_irq_state(struct seq_file *s,
>> struct vgic_irq *irq,
>> struct kvm_vcpu *vcpu)
>> {
>> char *type;
>> + bool pending;
> nit: can be directly initialized to irq->pending_latch
>> +
>> if (irq->intid < VGIC_NR_SGIS)
>> type = "SGI";
>> else if (irq->intid < VGIC_NR_PRIVATE_IRQS)
>> @@ -190,6 +192,16 @@ static void print_irq_state(struct seq_file *s,
>> struct vgic_irq *irq,
>> if (irq->intid ==0 || irq->intid == VGIC_NR_PRIVATE_IRQS)
>> print_header(s, irq, vcpu);
>>
>> + pending = irq->pending_latch;
>> + if (irq->hw && vgic_irq_is_sgi(irq->intid)) {
>> + int err;
>> +
>> + err = irq_get_irqchip_state(irq->host_irq,
>> + IRQCHIP_STATE_PENDING,
>> + &pending);
>> + WARN_ON_ONCE(err);
>> + }
>> +
>> seq_printf(s, " %s %4d "
>> " %2d "
>> "%d%d%d%d%d%d%d "
>> @@ -201,7 +213,7 @@ static void print_irq_state(struct seq_file *s,
>> struct vgic_irq *irq,
>> "\n",
>> type, irq->intid,
>> (irq->target_vcpu) ? irq->target_vcpu->vcpu_id : -1,
>> - irq->pending_latch,
>> + pending,
>> irq->line_level,
>> irq->active,
>> irq->enabled,
>>
> The patch looks good to me but I am now lost about how we retrieve the
> pending stat of other hw mapped interrupts. Looks we use
> irq->pending_latch always. Is that correct?

Correct. GICv4.0 doesn't give us an architectural way to look at the
vLPI pending state (there isn't even a guarantee about when the GIC
will stop writing to memory, if it ever does).

With GICv4.1, you can introspect the HW state for SGIs. You can also
look at the vLPI state by peeking at the virtual pending table, but
you'd need to unmap the VPE first, which I obviously don't want to do
for this debug interface, specially as it can be used whilst the guest
is up and running.

In the future, we'll have to implement that in order to support guest
save/restore from a GICv4.1 system. I haven't given much thought to it
though.

> For the patch:
> Reviewed-by: Eric Auger <[email protected]>

Thanks,

M.
--
Jazz is not dead. It just smells funny...

2020-03-19 15:45:32

[permalink] [raw]

Subject: Re: [PATCH v5 23/23] KVM: arm64: GICv4.1: Expose HW-based SGIs in debugfs

Hi Marc,

On 3/19/20 4:21 PM, Marc Zyngier wrote:
> Hi Eric,
>
> On 2020-03-19 15:05, Auger Eric wrote:
>> Hi Marc,
>>
>> On 3/4/20 9:33 PM, Marc Zyngier wrote:
>>> The vgic-state debugfs file could do with showing the pending state
>>> of the HW-backed SGIs. Plug it into the low-level code.
>>>
>>> Signed-off-by: Marc Zyngier <[email protected]>
>>> ---
>>> virt/kvm/arm/vgic/vgic-debug.c | 14 +++++++++++++-
>>> 1 file changed, 13 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/virt/kvm/arm/vgic/vgic-debug.c
>>> b/virt/kvm/arm/vgic/vgic-debug.c
>>> index cc12fe9b2df3..b13a9e3f99dd 100644
>>> --- a/virt/kvm/arm/vgic/vgic-debug.c
>>> +++ b/virt/kvm/arm/vgic/vgic-debug.c
>>> @@ -178,6 +178,8 @@ static void print_irq_state(struct seq_file *s,
>>> struct vgic_irq *irq,
>>>                  struct kvm_vcpu *vcpu)
>>> {
>>>      char *type;
>>> +    bool pending;
>> nit: can be directly initialized to irq->pending_latch
>>> +
>>>      if (irq->intid < VGIC_NR_SGIS)
>>>          type = "SGI";
>>>      else if (irq->intid < VGIC_NR_PRIVATE_IRQS)
>>> @@ -190,6 +192,16 @@ static void print_irq_state(struct seq_file *s,
>>> struct vgic_irq *irq,
>>>      if (irq->intid ==0 || irq->intid == VGIC_NR_PRIVATE_IRQS)
>>>          print_header(s, irq, vcpu);
>>>
>>> +    pending = irq->pending_latch;
>>> +    if (irq->hw && vgic_irq_is_sgi(irq->intid)) {
>>> +        int err;
>>> +
>>> +        err = irq_get_irqchip_state(irq->host_irq,
>>> +                        IRQCHIP_STATE_PENDING,
>>> +                        &pending);
>>> +        WARN_ON_ONCE(err);
>>> +    }
>>> +
>>>      seq_printf(s, "       %s %4d "
>>>                "    %2d "
>>>                "%d%d%d%d%d%d%d "
>>> @@ -201,7 +213,7 @@ static void print_irq_state(struct seq_file *s,
>>> struct vgic_irq *irq,
>>>                "\n",
>>>              type, irq->intid,
>>>              (irq->target_vcpu) ? irq->target_vcpu->vcpu_id : -1,
>>> -            irq->pending_latch,
>>> +            pending,
>>>              irq->line_level,
>>>              irq->active,
>>>              irq->enabled,
>>>
>> The patch looks good to me but I am now lost about how we retrieve the
>> pending stat of other hw mapped interrupts. Looks we use
>> irq->pending_latch always. Is that correct?
>
> Correct. GICv4.0 doesn't give us an architectural way to look at the
> vLPI pending state (there isn't even a guarantee about when the GIC
> will stop writing to memory, if it ever does).
>
> With GICv4.1, you can introspect the HW state for SGIs. You can also
> look at the vLPI state by peeking at the virtual pending table, but
> you'd need to unmap the VPE first, which I obviously don't want to do
> for this debug interface, specially as it can be used whilst the guest
> is up and running.
OK for vLPIs, what about other HW mapped IRQs (arch timer?)

Eric
>
> In the future, we'll have to implement that in order to support guest
> save/restore from a GICv4.1 system. I haven't given much thought to it
> though.
>
>> For the patch:
>> Reviewed-by: Eric Auger <[email protected]>
>
> Thanks,
>
>         M.

2020-03-19 16:16:50

[permalink] [raw]

Subject: Re: [PATCH v5 23/23] KVM: arm64: GICv4.1: Expose HW-based SGIs in debugfs

Hi Eric,

On 2020-03-19 15:43, Auger Eric wrote:
> Hi Marc,
>
> On 3/19/20 4:21 PM, Marc Zyngier wrote:
>> Hi Eric,

[...]

>>> The patch looks good to me but I am now lost about how we retrieve
>>> the
>>> pending stat of other hw mapped interrupts. Looks we use
>>> irq->pending_latch always. Is that correct?
>>
>> Correct. GICv4.0 doesn't give us an architectural way to look at the
>> vLPI pending state (there isn't even a guarantee about when the GIC
>> will stop writing to memory, if it ever does).
>>
>> With GICv4.1, you can introspect the HW state for SGIs. You can also
>> look at the vLPI state by peeking at the virtual pending table, but
>> you'd need to unmap the VPE first, which I obviously don't want to do
>> for this debug interface, specially as it can be used whilst the guest
>> is up and running.
> OK for vLPIs, what about other HW mapped IRQs (arch timer?)

Different kind of HW. With those, the injection is still virtual, so the
SW pending bit is still very much valid. You can actually try and make
the timer interrupt pending, it should show up.

What the irq->hw bit means is "this virtual interrupt is somehow related
to the host_irq". How this is interpreted is completely
context-dependent.

Thanks,

M.
--
Jazz is not dead. It just smells funny...

2020-03-19 16:17:36

[permalink] [raw]

Subject: Re: [PATCH v5 19/23] KVM: arm64: GICv4.1: Allow SGIs to switch between HW and SW interrupts

Hi Marc,

On 3/4/20 9:33 PM, Marc Zyngier wrote:
> In order to let a guest buy in the new, active-less SGIs, we
> need to be able to switch between the two modes.
>
> Handle this by stopping all guest activity, transfer the state
> from one mode to the other, and resume the guest. Nothing calls
> this code so far, but a later patch will plug it into the MMIO
> emulation.
>
> Signed-off-by: Marc Zyngier <[email protected]>
> ---
> include/kvm/arm_vgic.h | 3 ++
> virt/kvm/arm/vgic/vgic-v4.c | 94 +++++++++++++++++++++++++++++++++++++
> virt/kvm/arm/vgic/vgic.h | 1 +
> 3 files changed, 98 insertions(+)
>
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 63457908c9c4..69f4164d6477 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -231,6 +231,9 @@ struct vgic_dist {
> /* distributor enabled */
> bool enabled;
>
> + /* Wants SGIs without active state */
> + bool nassgireq;
> +
> struct vgic_irq *spis;
>
> struct vgic_io_device dist_iodev;
> diff --git a/virt/kvm/arm/vgic/vgic-v4.c b/virt/kvm/arm/vgic/vgic-v4.c
> index c2fcde104ea2..a65dc1c85363 100644
> --- a/virt/kvm/arm/vgic/vgic-v4.c
> +++ b/virt/kvm/arm/vgic/vgic-v4.c
> @@ -97,6 +97,100 @@ static irqreturn_t vgic_v4_doorbell_handler(int irq, void *info)
> return IRQ_HANDLED;
> }
>
> +static void vgic_v4_sync_sgi_config(struct its_vpe *vpe, struct vgic_irq *irq)
> +{
> + vpe->sgi_config[irq->intid].enabled = irq->enabled;
> + vpe->sgi_config[irq->intid].group = irq->group;
> + vpe->sgi_config[irq->intid].priority = irq->priority;
> +}
> +
> +static void vgic_v4_enable_vsgis(struct kvm_vcpu *vcpu)
> +{
> + struct its_vpe *vpe = &vcpu->arch.vgic_cpu.vgic_v3.its_vpe;
> + int i;
> +
> + /*
> + * With GICv4.1, every virtual SGI can be directly injected. So
> + * let's pretend that they are HW interrupts, tied to a host
> + * IRQ. The SGI code will do its magic.
> + */
> + for (i = 0; i < VGIC_NR_SGIS; i++) {
> + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, i);
> + struct irq_desc *desc;
> + int ret;
Is is safe without holding the irq->irq_lock?
> +
> + if (irq->hw) {
> + vgic_put_irq(vcpu->kvm, irq);
> + continue;
> + }
> +
> + irq->hw = true;
> + irq->host_irq = irq_find_mapping(vpe->sgi_domain, i);
> +
> + /* Transfer the full irq state to the vPE */
> + vgic_v4_sync_sgi_config(vpe, irq);
> + desc = irq_to_desc(irq->host_irq);
> + ret = irq_domain_activate_irq(irq_desc_get_irq_data(desc),
> + false);
> + if (!WARN_ON(ret)) {
> + /* Transfer pending state */
> + ret = irq_set_irqchip_state(irq->host_irq,
> + IRQCHIP_STATE_PENDING,
> + irq->pending_latch);
> + WARN_ON(ret);
> + irq->pending_latch = false;
> + }
> +
> + vgic_put_irq(vcpu->kvm, irq);
> + }
> +}
> +
> +static void vgic_v4_disable_vsgis(struct kvm_vcpu *vcpu)
> +{
> + int i;
> +
> + for (i = 0; i < VGIC_NR_SGIS; i++) {
> + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, i);
> + struct irq_desc *desc;
> + int ret;
> +
> + if (!irq->hw) {
> + vgic_put_irq(vcpu->kvm, irq);
> + continue;
> + }
> +
> + irq->hw = false;
> + ret = irq_get_irqchip_state(irq->host_irq,
> + IRQCHIP_STATE_PENDING,
> + &irq->pending_latch);
> + WARN_ON(ret);
> +
> + desc = irq_to_desc(irq->host_irq);
> + irq_domain_deactivate_irq(irq_desc_get_irq_data(desc));
> +
> + vgic_put_irq(vcpu->kvm, irq);
> + }
> +}
> +
> +/* Must be called with the kvm lock held */
> +void vgic_v4_configure_vsgis(struct kvm *kvm)
> +{
> + struct vgic_dist *dist = &kvm->arch.vgic;
> + struct kvm_vcpu *vcpu;
> + int i;
> +
> + kvm_arm_halt_guest(kvm);
> +
> + kvm_for_each_vcpu(i, vcpu, kvm) {
> + if (dist->nassgireq)
> + vgic_v4_enable_vsgis(vcpu);
> + else
> + vgic_v4_disable_vsgis(vcpu);
> + }
> +
> + kvm_arm_resume_guest(kvm);
> +}
> +
> /**
> * vgic_v4_init - Initialize the GICv4 data structures
> * @kvm: Pointer to the VM being initialized
> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
> index c7fefd6b1c80..769e4802645e 100644
> --- a/virt/kvm/arm/vgic/vgic.h
> +++ b/virt/kvm/arm/vgic/vgic.h
> @@ -316,5 +316,6 @@ void vgic_its_invalidate_cache(struct kvm *kvm);
> bool vgic_supports_direct_msis(struct kvm *kvm);
> int vgic_v4_init(struct kvm *kvm);
> void vgic_v4_teardown(struct kvm *kvm);
> +void vgic_v4_configure_vsgis(struct kvm *kvm);
>
> #endif
>
Thanks

Eric

2020-03-19 16:20:35

[permalink] [raw]

Subject: Re: [PATCH v5 23/23] KVM: arm64: GICv4.1: Expose HW-based SGIs in debugfs

Hi,

On 3/19/20 5:16 PM, Marc Zyngier wrote:
> Hi Eric,
>
> On 2020-03-19 15:43, Auger Eric wrote:
>> Hi Marc,
>>
>> On 3/19/20 4:21 PM, Marc Zyngier wrote:
>>> Hi Eric,
>
> [...]
>
>>>> The patch looks good to me but I am now lost about how we retrieve the
>>>> pending stat of other hw mapped interrupts. Looks we use
>>>> irq->pending_latch always. Is that correct?
>>>
>>> Correct. GICv4.0 doesn't give us an architectural way to look at the
>>> vLPI pending state (there isn't even a guarantee about when the GIC
>>> will stop writing to memory, if it ever does).
>>>
>>> With GICv4.1, you can introspect the HW state for SGIs. You can also
>>> look at the vLPI state by peeking at the virtual pending table, but
>>> you'd need to unmap the VPE first, which I obviously don't want to do
>>> for this debug interface, specially as it can be used whilst the guest
>>> is up and running.
>> OK for vLPIs, what about other HW mapped IRQs (arch timer?)
>
> Different kind of HW. With those, the injection is still virtual, so the
> SW pending bit is still very much valid. You can actually try and make
> the timer interrupt pending, it should show up.
>
> What the irq->hw bit means is "this virtual interrupt is somehow related
> to the host_irq". How this is interpreted is completely context-dependent.
OK thank you for refreshing my memories ;-)

Eric
>
> Thanks,
>
> M.

2020-03-19 19:53:17

[permalink] [raw]

Subject: Re: [PATCH v5 19/23] KVM: arm64: GICv4.1: Allow SGIs to switch between HW and SW interrupts

Hi Eric,

On 2020-03-19 16:16, Auger Eric wrote:
> Hi Marc,
>
> On 3/4/20 9:33 PM, Marc Zyngier wrote:
>> In order to let a guest buy in the new, active-less SGIs, we
>> need to be able to switch between the two modes.
>>
>> Handle this by stopping all guest activity, transfer the state
>> from one mode to the other, and resume the guest. Nothing calls
>> this code so far, but a later patch will plug it into the MMIO
>> emulation.
>>
>> Signed-off-by: Marc Zyngier <[email protected]>
>> ---
>> include/kvm/arm_vgic.h | 3 ++
>> virt/kvm/arm/vgic/vgic-v4.c | 94
>> +++++++++++++++++++++++++++++++++++++
>> virt/kvm/arm/vgic/vgic.h | 1 +
>> 3 files changed, 98 insertions(+)
>>
>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>> index 63457908c9c4..69f4164d6477 100644
>> --- a/include/kvm/arm_vgic.h
>> +++ b/include/kvm/arm_vgic.h
>> @@ -231,6 +231,9 @@ struct vgic_dist {
>> /* distributor enabled */
>> bool enabled;
>>
>> + /* Wants SGIs without active state */
>> + bool nassgireq;
>> +
>> struct vgic_irq *spis;
>>
>> struct vgic_io_device dist_iodev;
>> diff --git a/virt/kvm/arm/vgic/vgic-v4.c b/virt/kvm/arm/vgic/vgic-v4.c
>> index c2fcde104ea2..a65dc1c85363 100644
>> --- a/virt/kvm/arm/vgic/vgic-v4.c
>> +++ b/virt/kvm/arm/vgic/vgic-v4.c
>> @@ -97,6 +97,100 @@ static irqreturn_t vgic_v4_doorbell_handler(int
>> irq, void *info)
>> return IRQ_HANDLED;
>> }
>>
>> +static void vgic_v4_sync_sgi_config(struct its_vpe *vpe, struct
>> vgic_irq *irq)
>> +{
>> + vpe->sgi_config[irq->intid].enabled = irq->enabled;
>> + vpe->sgi_config[irq->intid].group = irq->group;
>> + vpe->sgi_config[irq->intid].priority = irq->priority;
>> +}
>> +
>> +static void vgic_v4_enable_vsgis(struct kvm_vcpu *vcpu)
>> +{
>> + struct its_vpe *vpe = &vcpu->arch.vgic_cpu.vgic_v3.its_vpe;
>> + int i;
>> +
>> + /*
>> + * With GICv4.1, every virtual SGI can be directly injected. So
>> + * let's pretend that they are HW interrupts, tied to a host
>> + * IRQ. The SGI code will do its magic.
>> + */
>> + for (i = 0; i < VGIC_NR_SGIS; i++) {
>> + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, i);
>> + struct irq_desc *desc;
>> + int ret;
> Is is safe without holding the irq->irq_lock?

The assumption here is that we're coming vgic_v4_configure_vsgis(),
which starts
by stopping the whole guest. My guess is that it should be safe enough,
but
maybe you are thinking of something else?

Thanks,

M.
--
Jazz is not dead. It just smells funny...

2020-03-19 20:15:38

[permalink] [raw]

Subject: Re: [PATCH v5 19/23] KVM: arm64: GICv4.1: Allow SGIs to switch between HW and SW interrupts

Hi Marc,

On 3/19/20 8:52 PM, Marc Zyngier wrote:
> The assumption here is that we're coming vgic_v4_configure_vsgis(),
> which starts
> by stopping the whole guest. My guess is that it should be safe enough, but
> maybe you are thinking of something else?
I don't have a specific case in mind. Just preferred asking to make
sure. Usually when touching those fields we take the lock (that's also
the case in vgic_debug_show for instance).

Thanks

Eric

2020-03-19 20:40:25

[permalink] [raw]

Subject: Re: [PATCH v5 20/23] KVM: arm64: GICv4.1: Plumb SGI implementation selection in the distributor

Hi Marc,
On 3/19/20 1:10 PM, Marc Zyngier wrote:
> Hi Zenghui,
>
> On 2020-03-18 06:34, Zenghui Yu wrote:
>> Hi Marc,
>>
>> On 2020/3/5 4:33, Marc Zyngier wrote:
>>> The GICv4.1 architecture gives the hypervisor the option to let
>>> the guest choose whether it wants the good old SGIs with an
>>> active state, or the new, HW-based ones that do not have one.
>>>
>>> For this, plumb the configuration of SGIs into the GICv3 MMIO
>>> handling, present the GICD_TYPER2.nASSGIcap to the guest,
>>> and handle the GICD_CTLR.nASSGIreq setting.
>>>
>>> In order to be able to deal with the restore of a guest, also
>>> apply the GICD_CTLR.nASSGIreq setting at first run so that we
>>> can move the restored SGIs to the HW if that's what the guest
>>> had selected in a previous life.
>>
>> I'm okay with the restore path.? But it seems that we still fail to
>> save the pending state of vSGI - software pending_latch of HW-based
>> vSGIs will not be updated (and always be false) because we directly
>> inject them through ITS, so vgic_v3_uaccess_read_pending() can't
>> tell the correct pending state to user-space (the correct one should
>> be latched in HW).
>>
>> It would be good if we can sync the hardware state into pending_latch
>> at an appropriate time (just before save), but not sure if we can...
>
> The problem is to find the "appropriate time". It would require to define
> a point in the save sequence where we transition the state from HW to
> SW. I'm not keen on adding more state than we already have.

may be we could use a dedicated device group/attr as we have for the ITS
save tables? the user space would choose.

Thanks

Eric
>
> But what we can do is to just ask the HW to give us the right state
> on userspace access, at all times. How about this:
>
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index 48fd9fc229a2..281fe7216c59 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -305,8 +305,18 @@ static unsigned long
> vgic_v3_uaccess_read_pending(struct kvm_vcpu *vcpu,
> ????? */
> ???? for (i = 0; i < len * 8; i++) {
> ???????? struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
> +??????? bool state = irq->pending_latch;
>
> -??????? if (irq->pending_latch)
> +??????? if (irq->hw && vgic_irq_is_sgi(irq->intid)) {
> +??????????? int err;
> +
> +??????????? err = irq_get_irqchip_state(irq->host_irq,
> +??????????????????????????? IRQCHIP_STATE_PENDING,
> +??????????????????????????? &state);
> +??????????? WARN_ON(err);
> +??????? }
> +
> +??????? if (state)
> ???????????? value |= (1U << i);
>
> ???????? vgic_put_irq(vcpu->kvm, irq);
>
> I can add this to "KVM: arm64: GICv4.1: Add direct injection capability
> to SGI registers".
>
>>
>>>
>>> Signed-off-by: Marc Zyngier <[email protected]>
>>> ---
>>> ? virt/kvm/arm/vgic/vgic-mmio-v3.c | 48 ++++++++++++++++++++++++++++++--
>>> ? virt/kvm/arm/vgic/vgic-v3.c????? |? 2 ++
>>> ? 2 files changed, 48 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c
>>> b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>>> index de89da76a379..442f3b8c2559 100644
>>> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
>>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>>> @@ -3,6 +3,7 @@
>>> ?? * VGICv3 MMIO handling functions
>>> ?? */
>>> ? +#include <linux/bitfield.h>
>>> ? #include <linux/irqchip/arm-gic-v3.h>
>>> ? #include <linux/kvm.h>
>>> ? #include <linux/kvm_host.h>
>>> @@ -70,6 +71,8 @@ static unsigned long vgic_mmio_read_v3_misc(struct
>>> kvm_vcpu *vcpu,
>>> ????????? if (vgic->enabled)
>>> ????????????? value |= GICD_CTLR_ENABLE_SS_G1;
>>> ????????? value |= GICD_CTLR_ARE_NS | GICD_CTLR_DS;
>>> +??????? if (kvm_vgic_global_state.has_gicv4_1 && vgic->nassgireq)
>>
>> Looking at how we handle the GICD_CTLR.nASSGIreq setting, I think
>> "nassgireq==true" already indicates "has_gicv4_1==true".? So this
>> can be simplified.
>
> Indeed. I've now dropped the has_gicv4.1 check.
>
>> But I wonder that should we use nassgireq to *only* keep track what
>> the guest had written into the GICD_CTLR.nASSGIreq.? If not, we may
>> lose the guest-request bit after migration among hosts with different
>> has_gicv4_1 settings.
>
> I'm unsure of what you're suggesting here. If userspace tries to set
> GICD_CTLR.nASSGIreq on a non-4.1 host, this bit will not latch.
> Userspace can check that at restore time. Or we could fail the
> userspace write, which is a bit odd (the bit is otherwise RES0).
>
> Could you clarify your proposal?
>
>> The remaining patches all look good to me :-). I will wait for you to
>> confirm these two concerns.
>
> Thanks,
>
> ??????? M.

2020-03-20 02:32:05

[permalink] [raw]

Subject: Re: [PATCH v5 16/23] irqchip/gic-v4.1: Eagerly vmap vPEs

Hi Marc,

On 2020/3/19 18:55, Marc Zyngier wrote:
> On 2020-03-17 02:49, Zenghui Yu wrote:
>> Hi Marc,
>>
>> On 2020/3/5 4:33, Marc Zyngier wrote:
>>> Now that we have HW-accelerated SGIs being delivered to VPEs, it
>>> becomes required to map the VPEs on all ITSs instead of relying
>>> on the lazy approach that we would use when using the ITS-list
>>> mechanism.
>>>
>>> Signed-off-by: Marc Zyngier <[email protected]>
>>
>> Before GICv4.1, we use vlpi_count to evaluate whether the vPE has been
>> mapped on the specified ITS, and use this refcount to only issue VMOVP
>> to those involved ITSes. It's broken after this patch.
>>
>> We may need to re-evaluate "whether the vPE is mapped" now that we're at
>> GICv4.1, otherwise *no* VMOVP will be issued on the v4.1 capable machine
>> (I mean those without single VMOVP support).
>>
>> What I'm saying is something like below (only an idea, it even can't
>> compile), any thoughts?
>>
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c
>> b/drivers/irqchip/irq-gic-v3-its.c
>> index 2e12bc52e3a2..3450b5e847ca 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -198,7 +198,8 @@ static u16 get_its_list(struct its_vm *vm)
>>          if (!is_v4(its))
>>              continue;
>>
>> -        if (vm->vlpi_count[its->list_nr])
>> +        if (vm->vlpi_count[its->list_nr] ||
>> +            gic_requires_eager_mapping())
>>              __set_bit(its->list_nr, &its_list);
>>      }
>>
>> @@ -1295,7 +1296,8 @@ static void its_send_vmovp(struct its_vpe *vpe)
>>          if (!is_v4(its))
>>              continue;
>>
>> -        if (!vpe->its_vm->vlpi_count[its->list_nr])
>> +        if (!vpe->its_vm->vlpi_count[its->list_nr] &&
>> +            !gic_requires_eager_mapping())
>>              continue;
>>
>>          desc.its_vmovp_cmd.col = &its->collections[col_id];
>
> It took me a while to wrap my head around that one, but you're as usual
> spot on.
>
> The use of gic_requires_eager_mapping() is a bit confusing here, as it
> isn't
> so much that the VPE is eagerly mapped, but the predicate on which we
> evaluate
> the need for a VMOVP. How about this:
>
> diff --git a/drivers/irqchip/irq-gic-v3-its.c
> b/drivers/irqchip/irq-gic-v3-its.c
> index cd6451e190c9..348f7a909a69 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -189,6 +189,15 @@ static DEFINE_IDA(its_vpeid_ida);
> #define gic_data_rdist_rd_base()    (gic_data_rdist()->rd_base)
> #define gic_data_rdist_vlpi_base()    (gic_data_rdist_rd_base() +
> SZ_128K)
>
> +/*
> + * Skip ITSs that have no vLPIs mapped, unless we're on GICv4.1, as we
> + * always have vSGIs mapped.
> + */
> +static bool require_its_list_vmovp(struct its_vm *vm, struct its_node
> *its)
> +{
> +    return (gic_rdists->has_rvpeid || vm->vlpi_count[its->list_nr]);
> +}
> +
> static u16 get_its_list(struct its_vm *vm)
> {
>      struct its_node *its;
> @@ -198,7 +207,7 @@ static u16 get_its_list(struct its_vm *vm)
>          if (!is_v4(its))
>              continue;
>
> -        if (vm->vlpi_count[its->list_nr])
> +        if (require_its_list_vmovp(vm, its))
>              __set_bit(its->list_nr, &its_list);
>      }
>
> @@ -1295,7 +1304,7 @@ static void its_send_vmovp(struct its_vpe *vpe)
>          if (!is_v4(its))
>              continue;
>
> -        if (!vpe->its_vm->vlpi_count[its->list_nr])
> +        if (!require_its_list_vmovp(vpe->its_vm, its))
>              continue;
>
>          desc.its_vmovp_cmd.col = &its->collections[col_id];

Indeed this looks much clearer. We're actually evaluating the need
for issuing VMOVP to a specified ITS, on system using its_list_map
feature (though we evaluate it by checking whether the vPE is mapped
on this ITS).

Thanks,
Zenghui

2020-03-20 03:09:27

[permalink] [raw]

Subject: Re: [PATCH v5 20/23] KVM: arm64: GICv4.1: Plumb SGI implementation selection in the distributor

On 2020/3/20 4:38, Auger Eric wrote:
> Hi Marc,
> On 3/19/20 1:10 PM, Marc Zyngier wrote:
>> Hi Zenghui,
>>
>> On 2020-03-18 06:34, Zenghui Yu wrote:
>>> Hi Marc,
>>>
>>> On 2020/3/5 4:33, Marc Zyngier wrote:
>>>> The GICv4.1 architecture gives the hypervisor the option to let
>>>> the guest choose whether it wants the good old SGIs with an
>>>> active state, or the new, HW-based ones that do not have one.
>>>>
>>>> For this, plumb the configuration of SGIs into the GICv3 MMIO
>>>> handling, present the GICD_TYPER2.nASSGIcap to the guest,
>>>> and handle the GICD_CTLR.nASSGIreq setting.
>>>>
>>>> In order to be able to deal with the restore of a guest, also
>>>> apply the GICD_CTLR.nASSGIreq setting at first run so that we
>>>> can move the restored SGIs to the HW if that's what the guest
>>>> had selected in a previous life.
>>>
>>> I'm okay with the restore path.? But it seems that we still fail to
>>> save the pending state of vSGI - software pending_latch of HW-based
>>> vSGIs will not be updated (and always be false) because we directly
>>> inject them through ITS, so vgic_v3_uaccess_read_pending() can't
>>> tell the correct pending state to user-space (the correct one should
>>> be latched in HW).
>>>
>>> It would be good if we can sync the hardware state into pending_latch
>>> at an appropriate time (just before save), but not sure if we can...
>>
>> The problem is to find the "appropriate time". It would require to define
>> a point in the save sequence where we transition the state from HW to
>> SW. I'm not keen on adding more state than we already have.
>
> may be we could use a dedicated device group/attr as we have for the ITS
> save tables? the user space would choose.

It means that userspace will be aware of some form of GICv4.1 details
(e.g., get/set vSGI state at HW level) that KVM has implemented.
Is it something that userspace required to know? I'm open to this ;-)

>
> Thanks
>
> Eric
>>
>> But what we can do is to just ask the HW to give us the right state
>> on userspace access, at all times. How about this:
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> index 48fd9fc229a2..281fe7216c59 100644
>> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> @@ -305,8 +305,18 @@ static unsigned long
>> vgic_v3_uaccess_read_pending(struct kvm_vcpu *vcpu,
>> ????? */
>> ???? for (i = 0; i < len * 8; i++) {
>> ???????? struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>> +??????? bool state = irq->pending_latch;
>>
>> -??????? if (irq->pending_latch)
>> +??????? if (irq->hw && vgic_irq_is_sgi(irq->intid)) {
>> +??????????? int err;
>> +
>> +??????????? err = irq_get_irqchip_state(irq->host_irq,
>> +??????????????????????????? IRQCHIP_STATE_PENDING,
>> +??????????????????????????? &state);
>> +??????????? WARN_ON(err);
>> +??????? }
>> +
>> +??????? if (state)
>> ???????????? value |= (1U << i);
>>
>> ???????? vgic_put_irq(vcpu->kvm, irq);

Anyway this looks good to me and will do the right thing on a userspace
save.

>>
>> I can add this to "KVM: arm64: GICv4.1: Add direct injection capability
>> to SGI registers".

Thanks,
Zenghui

2020-03-20 03:54:04

[permalink] [raw]

Subject: Re: [PATCH v5 20/23] KVM: arm64: GICv4.1: Plumb SGI implementation selection in the distributor

Hi Marc,

On 2020/3/19 20:10, Marc Zyngier wrote:
>> But I wonder that should we use nassgireq to *only* keep track what
>> the guest had written into the GICD_CTLR.nASSGIreq. If not, we may
>> lose the guest-request bit after migration among hosts with different
>> has_gicv4_1 settings.
>
> I'm unsure of what you're suggesting here. If userspace tries to set
> GICD_CTLR.nASSGIreq on a non-4.1 host, this bit will not latch.

This is exactly what I *was* concerning about.

> Userspace can check that at restore time. Or we could fail the
> userspace write, which is a bit odd (the bit is otherwise RES0).
>
> Could you clarify your proposal?

Let's assume two hosts below. 'has_gicv4_1' is true on host-A while
it is false on host-B because of lack of HW support or the kernel
parameter "kvm-arm.vgic_v4_enable=0".

If we migrate guest through A->B->A, we may end-up lose the initial
guest-request "nASSGIreq=1" and don't use direct vSGI delivery for
this guest when it's migrated back to host-A.

This can be "fixed" by keep track of what guest had written into
nASSGIreq. And we need to evaluate the need for using direct vSGI
for a specified guest by 'has_gicv4_1 && nassgireq'.

But if it's expected that "if userspace tries to set nASSGIreq on
a non-4.1 host, this bit will not latch", then this shouldn't be
a problem at all.

Anyway this is not a big deal to me and I won't complain about it
in the future ;-) Either way, for this patch:

Reviewed-by: Zenghui Yu <[email protected]>

Thanks

2020-03-20 04:23:02

[permalink] [raw]

Subject: Re: [PATCH v5 19/23] KVM: arm64: GICv4.1: Allow SGIs to switch between HW and SW interrupts

Hi Marc,

On 2020/3/5 4:33, Marc Zyngier wrote:
> In order to let a guest buy in the new, active-less SGIs, we
> need to be able to switch between the two modes.
>
> Handle this by stopping all guest activity, transfer the state
> from one mode to the other, and resume the guest. Nothing calls
> this code so far, but a later patch will plug it into the MMIO
> emulation.
>
> Signed-off-by: Marc Zyngier <[email protected]>

Reviewed-by: Zenghui Yu <[email protected]>

Thanks

2020-03-20 04:24:57

[permalink] [raw]

Subject: Re: [PATCH v5 22/23] KVM: arm64: GICv4.1: Allow non-trapping WFI when using HW SGIs

Hi Marc,

On 2020/3/5 4:33, Marc Zyngier wrote:
> Just like for VLPIs, it is beneficial to avoid trapping on WFI when the
> vcpu is using the GICv4.1 SGIs.
>
> Add such a check to vcpu_clear_wfx_traps().
>
> Signed-off-by: Marc Zyngier <[email protected]>

Reviewed-by: Zenghui Yu <[email protected]>

Thanks

> ---
> arch/arm64/include/asm/kvm_emulate.h | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> index f658dda12364..a30b4eec7cb4 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -89,7 +89,8 @@ static inline unsigned long *vcpu_hcr(struct kvm_vcpu *vcpu)
> static inline void vcpu_clear_wfx_traps(struct kvm_vcpu *vcpu)
> {
> vcpu->arch.hcr_el2 &= ~HCR_TWE;
> - if (atomic_read(&vcpu->arch.vgic_cpu.vgic_v3.its_vpe.vlpi_count))
> + if (atomic_read(&vcpu->arch.vgic_cpu.vgic_v3.its_vpe.vlpi_count) ||
> + vcpu->kvm->arch.vgic.nassgireq)
> vcpu->arch.hcr_el2 &= ~HCR_TWI;
> else
> vcpu->arch.hcr_el2 |= HCR_TWI;
>

2020-03-20 04:40:49

[permalink] [raw]

Subject: Re: [PATCH v5 23/23] KVM: arm64: GICv4.1: Expose HW-based SGIs in debugfs

Hi Marc,

On 2020/3/19 23:21, Marc Zyngier wrote:
> With GICv4.1, you can introspect the HW state for SGIs. You can also
> look at the vLPI state by peeking at the virtual pending table, but
> you'd need to unmap the VPE first,

Out of curiosity, could you please point me to the "unmap the VPE"
requirement in the v4.1 spec? I'd like to have a look.

Thanks!
Zenghui

2020-03-20 08:03:36

[permalink] [raw]

Subject: Re: [PATCH v5 20/23] KVM: arm64: GICv4.1: Plumb SGI implementation selection in the distributor

Hi Zenghui,

On 3/20/20 4:08 AM, Zenghui Yu wrote:
> On 2020/3/20 4:38, Auger Eric wrote:
>> Hi Marc,
>> On 3/19/20 1:10 PM, Marc Zyngier wrote:
>>> Hi Zenghui,
>>>
>>> On 2020-03-18 06:34, Zenghui Yu wrote:
>>>> Hi Marc,
>>>>
>>>> On 2020/3/5 4:33, Marc Zyngier wrote:
>>>>> The GICv4.1 architecture gives the hypervisor the option to let
>>>>> the guest choose whether it wants the good old SGIs with an
>>>>> active state, or the new, HW-based ones that do not have one.
>>>>>
>>>>> For this, plumb the configuration of SGIs into the GICv3 MMIO
>>>>> handling, present the GICD_TYPER2.nASSGIcap to the guest,
>>>>> and handle the GICD_CTLR.nASSGIreq setting.
>>>>>
>>>>> In order to be able to deal with the restore of a guest, also
>>>>> apply the GICD_CTLR.nASSGIreq setting at first run so that we
>>>>> can move the restored SGIs to the HW if that's what the guest
>>>>> had selected in a previous life.
>>>>
>>>> I'm okay with the restore path.? But it seems that we still fail to
>>>> save the pending state of vSGI - software pending_latch of HW-based
>>>> vSGIs will not be updated (and always be false) because we directly
>>>> inject them through ITS, so vgic_v3_uaccess_read_pending() can't
>>>> tell the correct pending state to user-space (the correct one should
>>>> be latched in HW).
>>>>
>>>> It would be good if we can sync the hardware state into pending_latch
>>>> at an appropriate time (just before save), but not sure if we can...
>>>
>>> The problem is to find the "appropriate time". It would require to
>>> define
>>> a point in the save sequence where we transition the state from HW to
>>> SW. I'm not keen on adding more state than we already have.
>>
>> may be we could use a dedicated device group/attr as we have for the ITS
>> save tables? the user space would choose.
>
> It means that userspace will be aware of some form of GICv4.1 details
> (e.g., get/set vSGI state at HW level) that KVM has implemented.
> Is it something that userspace required to know? I'm open to this ;-)
Not sure we would be obliged to expose fine details. This could be a
generic save/restore device group/attr whose implementation at KVM level
could differ depending on the version being implemented, no?

Thanks

Eric
>
>>
>> Thanks
>>
>> Eric
>>>
>>> But what we can do is to just ask the HW to give us the right state
>>> on userspace access, at all times. How about this:
>>>
>>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c
>>> b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>>> index 48fd9fc229a2..281fe7216c59 100644
>>> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
>>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>>> @@ -305,8 +305,18 @@ static unsigned long
>>> vgic_v3_uaccess_read_pending(struct kvm_vcpu *vcpu,
>>> ?????? */
>>> ????? for (i = 0; i < len * 8; i++) {
>>> ????????? struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid
>>> + i);
>>> +??????? bool state = irq->pending_latch;
>>>
>>> -??????? if (irq->pending_latch)
>>> +??????? if (irq->hw && vgic_irq_is_sgi(irq->intid)) {
>>> +??????????? int err;
>>> +
>>> +??????????? err = irq_get_irqchip_state(irq->host_irq,
>>> +??????????????????????????? IRQCHIP_STATE_PENDING,
>>> +??????????????????????????? &state);
>>> +??????????? WARN_ON(err);
>>> +??????? }
>>> +
>>> +??????? if (state)
>>> ????????????? value |= (1U << i);
>>>
>>> ????????? vgic_put_irq(vcpu->kvm, irq);
>
> Anyway this looks good to me and will do the right thing on a userspace
> save.
>
>>>
>>> I can add this to "KVM: arm64: GICv4.1: Add direct injection capability
>>> to SGI registers".
>
> Thanks,
> Zenghui
>

2020-03-20 08:12:35

[permalink] [raw]

Subject: Re: [PATCH v5 18/23] KVM: arm64: GICv4.1: Add direct injection capability to SGI registers

Hi Marc,
On 3/4/20 9:33 PM, Marc Zyngier wrote:
> Most of the GICv3 emulation code that deals with SGIs now has to be
> aware of the v4.1 capabilities in order to benefit from it.
>
> Add such support, keyed on the interrupt having the hw flag set and
> being a SGI.
>
> Signed-off-by: Marc Zyngier <[email protected]>
> ---
> virt/kvm/arm/vgic/vgic-mmio-v3.c | 15 +++++-
> virt/kvm/arm/vgic/vgic-mmio.c | 88 ++++++++++++++++++++++++++++++--
> 2 files changed, 96 insertions(+), 7 deletions(-)
>
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index ebc218840fc2..de89da76a379 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -6,6 +6,7 @@
> #include <linux/irqchip/arm-gic-v3.h>
> #include <linux/kvm.h>
> #include <linux/kvm_host.h>
> +#include <linux/interrupt.h>
> #include <kvm/iodev.h>
> #include <kvm/arm_vgic.h>
>
> @@ -942,8 +943,18 @@ void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg, bool allow_group1)
> * generate interrupts of either group.
> */
> if (!irq->group || allow_group1) {
> - irq->pending_latch = true;
> - vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
> + if (!irq->hw) {
> + irq->pending_latch = true;
> + vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
> + } else {
> + /* HW SGI? Ask the GIC to inject it */
> + int err;
nit: add line
> + err = irq_set_irqchip_state(irq->host_irq,
> + IRQCHIP_STATE_PENDING,
> + true);
> + WARN_RATELIMIT(err, "IRQ %d", irq->host_irq);
> + raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
> + }
> } else {
> raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
> }
> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
> index 97fb2a40e6ba..2199302597fa 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
> @@ -5,6 +5,8 @@
>
> #include <linux/bitops.h>
> #include <linux/bsearch.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> #include <linux/kvm.h>
> #include <linux/kvm_host.h>
> #include <kvm/iodev.h>
> @@ -59,6 +61,11 @@ unsigned long vgic_mmio_read_group(struct kvm_vcpu *vcpu,
> return value;
> }
>
> +static void vgic_update_vsgi(struct vgic_irq *irq)
> +{
> + WARN_ON(its_prop_update_vsgi(irq->host_irq, irq->priority, irq->group));
> +}
> +
> void vgic_mmio_write_group(struct kvm_vcpu *vcpu, gpa_t addr,
> unsigned int len, unsigned long val)
> {
> @@ -71,7 +78,12 @@ void vgic_mmio_write_group(struct kvm_vcpu *vcpu, gpa_t addr,
>
> raw_spin_lock_irqsave(&irq->irq_lock, flags);
> irq->group = !!(val & BIT(i));
> - vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
> + if (irq->hw && vgic_irq_is_sgi(irq->intid)) {
> + vgic_update_vsgi(irq);
> + raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
> + } else {
> + vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
> + }
>
> vgic_put_irq(vcpu->kvm, irq);
> }
> @@ -113,7 +125,21 @@ void vgic_mmio_write_senable(struct kvm_vcpu *vcpu,
> struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>
> raw_spin_lock_irqsave(&irq->irq_lock, flags);
> - if (vgic_irq_is_mapped_level(irq)) {
> + if (irq->hw && vgic_irq_is_sgi(irq->intid)) {
> + if (!irq->enabled) {
> + struct irq_data *data;
> +
> + irq->enabled = true;
> + data = &irq_to_desc(irq->host_irq)->irq_data;
> + while (irqd_irq_disabled(data))
> + enable_irq(irq->host_irq);
could you explain me why the while() is requested?
> + }
> +
> + raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
> + vgic_put_irq(vcpu->kvm, irq);
> +
> + continue;
> + } else if (vgic_irq_is_mapped_level(irq)) {
> bool was_high = irq->line_level;
>
> /*
> @@ -148,6 +174,8 @@ void vgic_mmio_write_cenable(struct kvm_vcpu *vcpu,
> struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>
> raw_spin_lock_irqsave(&irq->irq_lock, flags);
> + if (irq->hw && vgic_irq_is_sgi(irq->intid) && irq->enabled)
> + disable_irq_nosync(irq->host_irq);
>
> irq->enabled = false;
>
> @@ -167,10 +195,22 @@ unsigned long vgic_mmio_read_pending(struct kvm_vcpu *vcpu,
> for (i = 0; i < len * 8; i++) {
> struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
> unsigned long flags;
> + bool val;
>
> raw_spin_lock_irqsave(&irq->irq_lock, flags);
> - if (irq_is_pending(irq))
> - value |= (1U << i);
> + if (irq->hw && vgic_irq_is_sgi(irq->intid)) {
> + int err;
> +
> + val = false;
> + err = irq_get_irqchip_state(irq->host_irq,
> + IRQCHIP_STATE_PENDING,
> + &val);
> + WARN_RATELIMIT(err, "IRQ %d", irq->host_irq);
> + } else {
> + val = irq_is_pending(irq);
> + }
> +
> + value |= ((u32)val << i);
> raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
>
> vgic_put_irq(vcpu->kvm, irq);
> @@ -215,6 +255,21 @@ void vgic_mmio_write_spending(struct kvm_vcpu *vcpu,
> }
>
> raw_spin_lock_irqsave(&irq->irq_lock, flags);
> +
> + if (irq->hw && vgic_irq_is_sgi(irq->intid)) {
> + /* HW SGI? Ask the GIC to inject it */
> + int err;
nit: extra line
> + err = irq_set_irqchip_state(irq->host_irq,
> + IRQCHIP_STATE_PENDING,
> + true);
> + WARN_RATELIMIT(err, "IRQ %d", irq->host_irq);
> +
> + raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
> + vgic_put_irq(vcpu->kvm, irq);
> +
> + continue;
> + }
> +
> if (irq->hw)
> vgic_hw_irq_spending(vcpu, irq, is_uaccess);
> else
> @@ -269,6 +324,20 @@ void vgic_mmio_write_cpending(struct kvm_vcpu *vcpu,
>
> raw_spin_lock_irqsave(&irq->irq_lock, flags);
>
> + if (irq->hw && vgic_irq_is_sgi(irq->intid)) {
> + /* HW SGI? Ask the GIC to clear its pending bit */
> + int err;
> + err = irq_set_irqchip_state(irq->host_irq,
> + IRQCHIP_STATE_PENDING,
> + false);
> + WARN_RATELIMIT(err, "IRQ %d", irq->host_irq);
> +
> + raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
> + vgic_put_irq(vcpu->kvm, irq);
> +
> + continue;
> + }
> +
> if (irq->hw)
> vgic_hw_irq_cpending(vcpu, irq, is_uaccess);
> else
> @@ -318,8 +387,15 @@ static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
>
> raw_spin_lock_irqsave(&irq->irq_lock, flags);
>
> - if (irq->hw) {
> + if (irq->hw && !vgic_irq_is_sgi(irq->intid)) {
> vgic_hw_irq_change_active(vcpu, irq, active, !requester_vcpu);
> + } else if (irq->hw && vgic_irq_is_sgi(irq->intid)) {
> + /*
> + * GICv4.1 VSGI feature doesn't track an active state,
> + * so let's not kid ourselves, there is nothing we can
> + * do here.
> + */
> + irq->active = false;
> } else {
> u32 model = vcpu->kvm->arch.vgic.vgic_model;
> u8 active_source;
> @@ -493,6 +569,8 @@ void vgic_mmio_write_priority(struct kvm_vcpu *vcpu,
> raw_spin_lock_irqsave(&irq->irq_lock, flags);
> /* Narrow the priority range to what we actually support */
> irq->priority = (val >> (i * 8)) & GENMASK(7, 8 - VGIC_PRI_BITS);
> + if (irq->hw && vgic_irq_is_sgi(irq->intid))
> + vgic_update_vsgi(irq);
> raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
>
> vgic_put_irq(vcpu->kvm, irq);
>
Reviewed-by: Eric Auger <[email protected]>

Thanks

Eric

2020-03-20 08:13:30

[permalink] [raw]

Subject: Re: [PATCH v5 22/23] KVM: arm64: GICv4.1: Allow non-trapping WFI when using HW SGIs

Hi Marc,

On 3/4/20 9:33 PM, Marc Zyngier wrote:
> Just like for VLPIs, it is beneficial to avoid trapping on WFI when the
> vcpu is using the GICv4.1 SGIs.
>
> Add such a check to vcpu_clear_wfx_traps().
>
> Signed-off-by: Marc Zyngier <[email protected]>
Reviewed-by: Eric Auger <[email protected]>

Thanks

Eric
> ---
> arch/arm64/include/asm/kvm_emulate.h | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> index f658dda12364..a30b4eec7cb4 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -89,7 +89,8 @@ static inline unsigned long *vcpu_hcr(struct kvm_vcpu *vcpu)
> static inline void vcpu_clear_wfx_traps(struct kvm_vcpu *vcpu)
> {
> vcpu->arch.hcr_el2 &= ~HCR_TWE;
> - if (atomic_read(&vcpu->arch.vgic_cpu.vgic_v3.its_vpe.vlpi_count))
> + if (atomic_read(&vcpu->arch.vgic_cpu.vgic_v3.its_vpe.vlpi_count) ||
> + vcpu->kvm->arch.vgic.nassgireq)
> vcpu->arch.hcr_el2 &= ~HCR_TWI;
> else
> vcpu->arch.hcr_el2 |= HCR_TWI;
>

2020-03-20 09:02:18

[permalink] [raw]

Subject: Re: [PATCH v5 20/23] KVM: arm64: GICv4.1: Plumb SGI implementation selection in the distributor

Hi Zenghui,

On 2020-03-20 03:53, Zenghui Yu wrote:
> Hi Marc,
>
> On 2020/3/19 20:10, Marc Zyngier wrote:
>>> But I wonder that should we use nassgireq to *only* keep track what
>>> the guest had written into the GICD_CTLR.nASSGIreq. If not, we may
>>> lose the guest-request bit after migration among hosts with different
>>> has_gicv4_1 settings.
>>
>> I'm unsure of what you're suggesting here. If userspace tries to set
>> GICD_CTLR.nASSGIreq on a non-4.1 host, this bit will not latch.
>
> This is exactly what I *was* concerning about.
>
>> Userspace can check that at restore time. Or we could fail the
>> userspace write, which is a bit odd (the bit is otherwise RES0).
>>
>> Could you clarify your proposal?
>
> Let's assume two hosts below. 'has_gicv4_1' is true on host-A while
> it is false on host-B because of lack of HW support or the kernel
> parameter "kvm-arm.vgic_v4_enable=0".
>
> If we migrate guest through A->B->A, we may end-up lose the initial
> guest-request "nASSGIreq=1" and don't use direct vSGI delivery for
> this guest when it's migrated back to host-A.

My point above is that we shouldn't allow the A->B migration the first
place, and fail it as quickly as possible. We don't know what the guest
has observed in terms of GIC capability, and it may not have enabled the
new flavour of SGIs just yet.

> This can be "fixed" by keep track of what guest had written into
> nASSGIreq. And we need to evaluate the need for using direct vSGI
> for a specified guest by 'has_gicv4_1 && nassgireq'.

It feels odd. It means we have more state than the HW normally has.
I have an alternative proposal, see below.

> But if it's expected that "if userspace tries to set nASSGIreq on
> a non-4.1 host, this bit will not latch", then this shouldn't be
> a problem at all.

Well, that is the semantics of the RES0 bit. It applies from both
guest and userspace.

And actually, maybe we can handle that pretty cheaply. If userspace
tries to restore GICD_TYPER2 to a value that isn't what KVM can
offer, we just return an error. Exactly like we do for GICD_IIDR.
Hence the following patch:

diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c
b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index 28b639fd1abc..e72dcc454247 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -156,6 +156,7 @@ static int vgic_mmio_uaccess_write_v3_misc(struct
kvm_vcpu *vcpu,
struct vgic_dist *dist = &vcpu->kvm->arch.vgic;

switch (addr & 0x0c) {
+ case GICD_TYPER2:
case GICD_IIDR:
if (val != vgic_mmio_read_v3_misc(vcpu, addr, len))
return -EINVAL;

Being a RO register, writing something that isn't compatible with the
possible behaviour of the hypervisor will just return an error.

What do you think?

> Anyway this is not a big deal to me and I won't complain about it
> in the future ;-) Either way, for this patch:
>
> Reviewed-by: Zenghui Yu <[email protected]>

Thanks a lot!

M.
--
Jazz is not dead. It just smells funny...

2020-03-20 09:10:20

[permalink] [raw]

Subject: Re: [PATCH v5 23/23] KVM: arm64: GICv4.1: Expose HW-based SGIs in debugfs

Hi Zenghui,

On 2020-03-20 04:38, Zenghui Yu wrote:
> Hi Marc,
>
> On 2020/3/19 23:21, Marc Zyngier wrote:
>> With GICv4.1, you can introspect the HW state for SGIs. You can also
>> look at the vLPI state by peeking at the virtual pending table, but
>> you'd need to unmap the VPE first,
>
> Out of curiosity, could you please point me to the "unmap the VPE"
> requirement in the v4.1 spec? I'd like to have a look.

Sure. See IHI0069F, 5.3.19 (VMAPP GICv4.1), "Caching of virtual LPI data
structures", and the bit that says:

"A VMAPP with {V,Alloc}=={0,1} cleans and invalidates any caching of the
Virtual Pending Table and Virtual Configuration Table associated with
the
vPEID held in the GIC"

which is what was crucially missing from the GICv4.0 spec (it doesn't
say
when the GIC is done writing to memory).

Side note: it'd be good to know what the rules are for your own GICv4
implementations, so that we can at least make sure the current code is
safe.

Thanks,

M.
--
Jazz is not dead. It just smells funny...

2020-03-20 09:18:48

[permalink] [raw]

Subject: Re: [PATCH v5 19/23] KVM: arm64: GICv4.1: Allow SGIs to switch between HW and SW interrupts

Hi Eric,

On 2020-03-19 20:13, Auger Eric wrote:
> Hi Marc,
>
> On 3/19/20 8:52 PM, Marc Zyngier wrote:
>> The assumption here is that we're coming vgic_v4_configure_vsgis(),
>> which starts
>> by stopping the whole guest. My guess is that it should be safe
>> enough, but
>> maybe you are thinking of something else?
> I don't have a specific case in mind. Just preferred asking to make
> sure. Usually when touching those fields we take the lock (that's also
> the case in vgic_debug_show for instance).

Ah, good thing you mention the debug interface. I think it is the only
thing that can run behind our back... Fair enough, I'll add the locking.

Thanks,

M.
--
Jazz is not dead. It just smells funny...

2020-03-20 09:47:13

[permalink] [raw]

Subject: Re: [PATCH v5 20/23] KVM: arm64: GICv4.1: Plumb SGI implementation selection in the distributor

On 2020-03-20 07:59, Auger Eric wrote:
> Hi Zenghui,
>
> On 3/20/20 4:08 AM, Zenghui Yu wrote:
>> On 2020/3/20 4:38, Auger Eric wrote:
>>> Hi Marc,
>>> On 3/19/20 1:10 PM, Marc Zyngier wrote:
>>>> Hi Zenghui,
>>>>
>>>> On 2020-03-18 06:34, Zenghui Yu wrote:
>>>>> Hi Marc,
>>>>>
>>>>> On 2020/3/5 4:33, Marc Zyngier wrote:
>>>>>> The GICv4.1 architecture gives the hypervisor the option to let
>>>>>> the guest choose whether it wants the good old SGIs with an
>>>>>> active state, or the new, HW-based ones that do not have one.
>>>>>>
>>>>>> For this, plumb the configuration of SGIs into the GICv3 MMIO
>>>>>> handling, present the GICD_TYPER2.nASSGIcap to the guest,
>>>>>> and handle the GICD_CTLR.nASSGIreq setting.
>>>>>>
>>>>>> In order to be able to deal with the restore of a guest, also
>>>>>> apply the GICD_CTLR.nASSGIreq setting at first run so that we
>>>>>> can move the restored SGIs to the HW if that's what the guest
>>>>>> had selected in a previous life.
>>>>>
>>>>> I'm okay with the restore path. But it seems that we still fail to
>>>>> save the pending state of vSGI - software pending_latch of HW-based
>>>>> vSGIs will not be updated (and always be false) because we directly
>>>>> inject them through ITS, so vgic_v3_uaccess_read_pending() can't
>>>>> tell the correct pending state to user-space (the correct one
>>>>> should
>>>>> be latched in HW).
>>>>>
>>>>> It would be good if we can sync the hardware state into
>>>>> pending_latch
>>>>> at an appropriate time (just before save), but not sure if we
>>>>> can...
>>>>
>>>> The problem is to find the "appropriate time". It would require to
>>>> define
>>>> a point in the save sequence where we transition the state from HW
>>>> to
>>>> SW. I'm not keen on adding more state than we already have.
>>>
>>> may be we could use a dedicated device group/attr as we have for the
>>> ITS
>>> save tables? the user space would choose.
>>
>> It means that userspace will be aware of some form of GICv4.1 details
>> (e.g., get/set vSGI state at HW level) that KVM has implemented.
>> Is it something that userspace required to know? I'm open to this ;-)
> Not sure we would be obliged to expose fine details. This could be a
> generic save/restore device group/attr whose implementation at KVM
> level
> could differ depending on the version being implemented, no?

What prevents us from hooking this synchronization to the current
behaviour
of KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES? After all, this is already the
point
where we synchronize the KVM view of the pending state with userspace.
Here, it's just a matter of picking the information from some other
place
(i.e. the host's virtual pending table).

The thing we need though is the guarantee that the guest isn't going to
get more vLPIs at that stage, as they would be lost. This effectively
assumes that we can also save/restore the state of the signalling
devices,
and I don't know if we're quite there yet.

Thanks,

M.
--
Jazz is not dead. It just smells funny...

2020-03-20 10:07:21

[permalink] [raw]

Subject: Re: [PATCH v5 18/23] KVM: arm64: GICv4.1: Add direct injection capability to SGI registers

Hi Eric,

On 2020-03-20 08:11, Auger Eric wrote:
> Hi Marc,
> On 3/4/20 9:33 PM, Marc Zyngier wrote:
>> Most of the GICv3 emulation code that deals with SGIs now has to be
>> aware of the v4.1 capabilities in order to benefit from it.
>>
>> Add such support, keyed on the interrupt having the hw flag set and
>> being a SGI.
>>
>> Signed-off-by: Marc Zyngier <[email protected]>
>> ---
>> virt/kvm/arm/vgic/vgic-mmio-v3.c | 15 +++++-
>> virt/kvm/arm/vgic/vgic-mmio.c | 88
>> ++++++++++++++++++++++++++++++--
>> 2 files changed, 96 insertions(+), 7 deletions(-)
>>

[...]

>> @@ -113,7 +125,21 @@ void vgic_mmio_write_senable(struct kvm_vcpu
>> *vcpu,
>> struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>>
>> raw_spin_lock_irqsave(&irq->irq_lock, flags);
>> - if (vgic_irq_is_mapped_level(irq)) {
>> + if (irq->hw && vgic_irq_is_sgi(irq->intid)) {
>> + if (!irq->enabled) {
>> + struct irq_data *data;
>> +
>> + irq->enabled = true;
>> + data = &irq_to_desc(irq->host_irq)->irq_data;
>> + while (irqd_irq_disabled(data))
>> + enable_irq(irq->host_irq);
> could you explain me why the while() is requested?

Ah, interesting question: disable_irq() (and its variants) can nest.
This
means that if you have done two disable_irq(), you must do two
enable_irq()
to get back to the interrupt being enabled.

The locking should ensure that this nesting doesn't happen, but I'm
paranoid
(see the GICv4.0 doorbell handling). It also makes it easier to reason
about
the initial state.

[...]

> Reviewed-by: Eric Auger <[email protected]>

Thanks!

M.
--
Jazz is not dead. It just smells funny...

2020-03-20 10:58:20

[permalink] [raw]

Subject: Re: [PATCH v5 18/23] KVM: arm64: GICv4.1: Add direct injection capability to SGI registers

Hi Marc,

On 3/20/20 11:05 AM, Marc Zyngier wrote:
> Hi Eric,
>
> On 2020-03-20 08:11, Auger Eric wrote:
>> Hi Marc,
>> On 3/4/20 9:33 PM, Marc Zyngier wrote:
>>> Most of the GICv3 emulation code that deals with SGIs now has to be
>>> aware of the v4.1 capabilities in order to benefit from it.
>>>
>>> Add such support, keyed on the interrupt having the hw flag set and
>>> being a SGI.
>>>
>>> Signed-off-by: Marc Zyngier <[email protected]>
>>> ---
>>> ?virt/kvm/arm/vgic/vgic-mmio-v3.c | 15 +++++-
>>> ?virt/kvm/arm/vgic/vgic-mmio.c??? | 88 ++++++++++++++++++++++++++++++--
>>> ?2 files changed, 96 insertions(+), 7 deletions(-)
>>>
>
> [...]
>
>>> @@ -113,7 +125,21 @@ void vgic_mmio_write_senable(struct kvm_vcpu *vcpu,
>>> ???????? struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid +
>>> i);
>>>
>>> ???????? raw_spin_lock_irqsave(&irq->irq_lock, flags);
>>> -??????? if (vgic_irq_is_mapped_level(irq)) {
>>> +??????? if (irq->hw && vgic_irq_is_sgi(irq->intid)) {
>>> +??????????? if (!irq->enabled) {
>>> +??????????????? struct irq_data *data;
>>> +
>>> +??????????????? irq->enabled = true;
>>> +??????????????? data = &irq_to_desc(irq->host_irq)->irq_data;
>>> +??????????????? while (irqd_irq_disabled(data))
>>> +??????????????????? enable_irq(irq->host_irq);
>> could you explain me why the while() is requested?
>
> Ah, interesting question: disable_irq() (and its variants) can nest. This
> means that if you have done two disable_irq(), you must do two enable_irq()
> to get back to the interrupt being enabled.
>
> The locking should ensure that this nesting doesn't happen, but I'm
> paranoid
> (see the GICv4.0 doorbell handling). It also makes it easier to reason
> about
> the initial state.

OK! thank you for this explanation.

Thanks

Eric
>
> [...]
>
>> Reviewed-by: Eric Auger <[email protected]>
>
> Thanks!
>
> ???????? M.

2020-03-20 11:10:32

[permalink] [raw]

Subject: Re: [PATCH v5 20/23] KVM: arm64: GICv4.1: Plumb SGI implementation selection in the distributor

Hi Marc,

On 3/20/20 10:46 AM, Marc Zyngier wrote:
> On 2020-03-20 07:59, Auger Eric wrote:
>> Hi Zenghui,
>>
>> On 3/20/20 4:08 AM, Zenghui Yu wrote:
>>> On 2020/3/20 4:38, Auger Eric wrote:
>>>> Hi Marc,
>>>> On 3/19/20 1:10 PM, Marc Zyngier wrote:
>>>>> Hi Zenghui,
>>>>>
>>>>> On 2020-03-18 06:34, Zenghui Yu wrote:
>>>>>> Hi Marc,
>>>>>>
>>>>>> On 2020/3/5 4:33, Marc Zyngier wrote:
>>>>>>> The GICv4.1 architecture gives the hypervisor the option to let
>>>>>>> the guest choose whether it wants the good old SGIs with an
>>>>>>> active state, or the new, HW-based ones that do not have one.
>>>>>>>
>>>>>>> For this, plumb the configuration of SGIs into the GICv3 MMIO
>>>>>>> handling, present the GICD_TYPER2.nASSGIcap to the guest,
>>>>>>> and handle the GICD_CTLR.nASSGIreq setting.
>>>>>>>
>>>>>>> In order to be able to deal with the restore of a guest, also
>>>>>>> apply the GICD_CTLR.nASSGIreq setting at first run so that we
>>>>>>> can move the restored SGIs to the HW if that's what the guest
>>>>>>> had selected in a previous life.
>>>>>>
>>>>>> I'm okay with the restore path. But it seems that we still fail to
>>>>>> save the pending state of vSGI - software pending_latch of HW-based
>>>>>> vSGIs will not be updated (and always be false) because we directly
>>>>>> inject them through ITS, so vgic_v3_uaccess_read_pending() can't
>>>>>> tell the correct pending state to user-space (the correct one should
>>>>>> be latched in HW).
>>>>>>
>>>>>> It would be good if we can sync the hardware state into pending_latch
>>>>>> at an appropriate time (just before save), but not sure if we can...
>>>>>
>>>>> The problem is to find the "appropriate time". It would require to
>>>>> define
>>>>> a point in the save sequence where we transition the state from HW to
>>>>> SW. I'm not keen on adding more state than we already have.
>>>>
>>>> may be we could use a dedicated device group/attr as we have for the
>>>> ITS
>>>> save tables? the user space would choose.
>>>
>>> It means that userspace will be aware of some form of GICv4.1 details
>>> (e.g., get/set vSGI state at HW level) that KVM has implemented.
>>> Is it something that userspace required to know? I'm open to this ;-)
>> Not sure we would be obliged to expose fine details. This could be a
>> generic save/restore device group/attr whose implementation at KVM level
>> could differ depending on the version being implemented, no?
>
> What prevents us from hooking this synchronization to the current behaviour
> of KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES? After all, this is already the
> point
> where we synchronize the KVM view of the pending state with userspace.
> Here, it's just a matter of picking the information from some other place
> (i.e. the host's virtual pending table).
agreed
>
> The thing we need though is the guarantee that the guest isn't going to
> get more vLPIs at that stage, as they would be lost. This effectively
> assumes that we can also save/restore the state of the signalling devices,
> and I don't know if we're quite there yet.
On QEMU, when KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES is called, the VM is
stopped.
See cddafd8f353d ("hw/intc/arm_gicv3_its: Implement state save/restore")
So I think it should work, no?

Thanks

Eric

>
> Thanks,
>
> M.

2020-03-20 11:20:49

[permalink] [raw]

Subject: Re: [PATCH v5 20/23] KVM: arm64: GICv4.1: Plumb SGI implementation selection in the distributor

Hi Eric,

On 2020-03-20 11:09, Auger Eric wrote:
> Hi Marc,

[...]

>>>> It means that userspace will be aware of some form of GICv4.1
>>>> details
>>>> (e.g., get/set vSGI state at HW level) that KVM has implemented.
>>>> Is it something that userspace required to know? I'm open to this
>>>> ;-)
>>> Not sure we would be obliged to expose fine details. This could be a
>>> generic save/restore device group/attr whose implementation at KVM
>>> level
>>> could differ depending on the version being implemented, no?
>>
>> What prevents us from hooking this synchronization to the current
>> behaviour
>> of KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES? After all, this is already
>> the
>> point
>> where we synchronize the KVM view of the pending state with userspace.
>> Here, it's just a matter of picking the information from some other
>> place
>> (i.e. the host's virtual pending table).
> agreed
>>
>> The thing we need though is the guarantee that the guest isn't going
>> to
>> get more vLPIs at that stage, as they would be lost. This effectively
>> assumes that we can also save/restore the state of the signalling
>> devices,
>> and I don't know if we're quite there yet.
> On QEMU, when KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES is called, the VM is
> stopped.
> See cddafd8f353d ("hw/intc/arm_gicv3_its: Implement state
> save/restore")
> So I think it should work, no?

The guest being stopped is a good start. But my concern is on the device
side.

If the device is still active (generating interrupts), these interrupts
will
be dropped because the vPE will have been unmapped from the ITS in order
to
clean the ITS caches and make sure the virtual pending table is up to
date.

In turn, restoring the guest may lead to a lockup because we would have
lost
these interrupts. What does QEMU on x86 do in this case?

Thanks,

M.
--
Jazz is not dead. It just smells funny...

2020-03-20 11:37:53

[permalink] [raw]

Subject: Re: [PATCH v5 23/23] KVM: arm64: GICv4.1: Expose HW-based SGIs in debugfs

Hi Marc,

On 2020/3/20 17:09, Marc Zyngier wrote:
> Hi Zenghui,
>
> On 2020-03-20 04:38, Zenghui Yu wrote:
>> Hi Marc,
>>
>> On 2020/3/19 23:21, Marc Zyngier wrote:
>>> With GICv4.1, you can introspect the HW state for SGIs. You can also
>>> look at the vLPI state by peeking at the virtual pending table, but
>>> you'd need to unmap the VPE first,
>>
>> Out of curiosity, could you please point me to the "unmap the VPE"
>> requirement in the v4.1 spec? I'd like to have a look.
>
> Sure. See IHI0069F, 5.3.19 (VMAPP GICv4.1), "Caching of virtual LPI data
> structures", and the bit that says:
>
> "A VMAPP with {V,Alloc}=={0,1} cleans and invalidates any caching of the
> Virtual Pending Table and Virtual Configuration Table associated with the
> vPEID held in the GIC"
>
> which is what was crucially missing from the GICv4.0 spec (it doesn't say
> when the GIC is done writing to memory).

OK. Thanks for the pointer!

>
> Side note: it'd be good to know what the rules are for your own GICv4
> implementations, so that we can at least make sure the current code is
> safe.

As far as I know, there will be some clean and invalidate operations
when v4.0 VPENDBASER.Valid gets programmed. But not sure about behaviors
on VMAPP (unmap), it may be a totally v4.1 stuff. I'll have a talk with
our SOC team.

But how can the current code be unsafe? Is anywhere in the current code
will peek/poke the vpt (whilst GIC continues writing things into it)?

Thanks,
Zenghui

2020-03-20 11:47:13

[permalink] [raw]

Subject: Re: [PATCH v5 23/23] KVM: arm64: GICv4.1: Expose HW-based SGIs in debugfs

On 2020-03-20 11:35, Zenghui Yu wrote:
> Hi Marc,
>
> On 2020/3/20 17:09, Marc Zyngier wrote:
>> Hi Zenghui,
>>
>> On 2020-03-20 04:38, Zenghui Yu wrote:
>>> Hi Marc,
>>>
>>> On 2020/3/19 23:21, Marc Zyngier wrote:
>>>> With GICv4.1, you can introspect the HW state for SGIs. You can also
>>>> look at the vLPI state by peeking at the virtual pending table, but
>>>> you'd need to unmap the VPE first,
>>>
>>> Out of curiosity, could you please point me to the "unmap the VPE"
>>> requirement in the v4.1 spec? I'd like to have a look.
>>
>> Sure. See IHI0069F, 5.3.19 (VMAPP GICv4.1), "Caching of virtual LPI
>> data
>> structures", and the bit that says:
>>
>> "A VMAPP with {V,Alloc}=={0,1} cleans and invalidates any caching of
>> the
>> Virtual Pending Table and Virtual Configuration Table associated with
>> the
>> vPEID held in the GIC"
>>
>> which is what was crucially missing from the GICv4.0 spec (it doesn't
>> say
>> when the GIC is done writing to memory).
>
> OK. Thanks for the pointer!
>
>>
>> Side note: it'd be good to know what the rules are for your own GICv4
>> implementations, so that we can at least make sure the current code is
>> safe.
>
> As far as I know, there will be some clean and invalidate operations
> when v4.0 VPENDBASER.Valid gets programmed.

Interesting. The ideal behaviour would be that the VPT is up-to-date and
the caches clean when Valid is cleared (and once Dirty flips to 0).

> But not sure about behaviors
> on VMAPP (unmap), it may be a totally v4.1 stuff. I'll have a talk with
> our SOC team.

The VMAPP stuff is purely v4.1.

> But how can the current code be unsafe? Is anywhere in the current code
> will peek/poke the vpt (whilst GIC continues writing things into it)?

No. But on VM termination, the memory will be freed, and will eventually
be
reallocated. If the GIC can still write to that memory after it has been
freed, you end-up with memory corruption... Which is why I'm curious of
what ensures that on your implementation.

I'd also like to know the same thing about the QC implementation, but
there's nobody left to find out...

Thanks,

M.
--
Jazz is not dead. It just smells funny...

2020-03-20 12:10:16

[permalink] [raw]

Subject: Re: [PATCH v5 23/23] KVM: arm64: GICv4.1: Expose HW-based SGIs in debugfs

Hi Marc,

On 2020/3/20 19:46, Marc Zyngier wrote:
>>> Side note: it'd be good to know what the rules are for your own GICv4
>>> implementations, so that we can at least make sure the current code
>>> is safe.
>>
>> As far as I know, there will be some clean and invalidate operations
>> when v4.0 VPENDBASER.Valid gets programmed.
>
> Interesting. The ideal behaviour would be that the VPT is up-to-date and
> the caches clean when Valid is cleared (and once Dirty flips to 0).
>
>> But not sure about behaviors
>> on VMAPP (unmap), it may be a totally v4.1 stuff. I'll have a talk with
>> our SOC team.
>
> The VMAPP stuff is purely v4.1.
>
>> But how can the current code be unsafe? Is anywhere in the current code
>> will peek/poke the vpt (whilst GIC continues writing things into it)?
>
> No. But on VM termination, the memory will be freed, and will eventually be
> reallocated. If the GIC can still write to that memory after it has been
> freed, you end-up with memory corruption... Which is why I'm curious of
> what ensures that on your implementation.

Ah, I got it. I will check it with HiSilicon people next week and go
back to you if the code becomes unsafe due to the incomplete GICv4.

Thanks,
Zenghui

2020-03-23 08:11:51

[permalink] [raw]

Subject: Re: [PATCH v5 20/23] KVM: arm64: GICv4.1: Plumb SGI implementation selection in the distributor

Hi Marc,

On 2020/3/20 17:01, Marc Zyngier wrote:
> Hi Zenghui,
>
> On 2020-03-20 03:53, Zenghui Yu wrote:
>> Hi Marc,
>>
>> On 2020/3/19 20:10, Marc Zyngier wrote:
>>>> But I wonder that should we use nassgireq to *only* keep track what
>>>> the guest had written into the GICD_CTLR.nASSGIreq. If not, we may
>>>> lose the guest-request bit after migration among hosts with different
>>>> has_gicv4_1 settings.
>>>
>>> I'm unsure of what you're suggesting here. If userspace tries to set
>>> GICD_CTLR.nASSGIreq on a non-4.1 host, this bit will not latch.
>>
>> This is exactly what I *was* concerning about.
>>
>>> Userspace can check that at restore time. Or we could fail the
>>> userspace write, which is a bit odd (the bit is otherwise RES0).
>>>
>>> Could you clarify your proposal?
>>
>> Let's assume two hosts below. 'has_gicv4_1' is true on host-A while
>> it is false on host-B because of lack of HW support or the kernel
>> parameter "kvm-arm.vgic_v4_enable=0".
>>
>> If we migrate guest through A->B->A, we may end-up lose the initial
>> guest-request "nASSGIreq=1" and don't use direct vSGI delivery for
>> this guest when it's migrated back to host-A.
>
> My point above is that we shouldn't allow the A->B migration the first
> place, and fail it as quickly as possible. We don't know what the guest
> has observed in terms of GIC capability, and it may not have enabled the
> new flavour of SGIs just yet.

Indeed. I didn't realize it.

>
>> This can be "fixed" by keep track of what guest had written into
>> nASSGIreq. And we need to evaluate the need for using direct vSGI
>> for a specified guest by 'has_gicv4_1 && nassgireq'.
>
> It feels odd. It means we have more state than the HW normally has.
> I have an alternative proposal, see below.
>
>> But if it's expected that "if userspace tries to set nASSGIreq on
>> a non-4.1 host, this bit will not latch", then this shouldn't be
>> a problem at all.
>
> Well, that is the semantics of the RES0 bit. It applies from both
> guest and userspace.
>
> And actually, maybe we can handle that pretty cheaply. If userspace
> tries to restore GICD_TYPER2 to a value that isn't what KVM can
> offer, we just return an error. Exactly like we do for GICD_IIDR.
> Hence the following patch:
>
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index 28b639fd1abc..e72dcc454247 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -156,6 +156,7 @@ static int vgic_mmio_uaccess_write_v3_misc(struct
> kvm_vcpu *vcpu,
>      struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>
>      switch (addr & 0x0c) {
> +    case GICD_TYPER2:
>      case GICD_IIDR:
>          if (val != vgic_mmio_read_v3_misc(vcpu, addr, len))
>              return -EINVAL;
>
> Being a RO register, writing something that isn't compatible with the
> possible behaviour of the hypervisor will just return an error.

This is really a nice point to address my concern! I'm happy to see
this in v6 now.

>
> What do you think?

I agreed with you, with a bit nervous though. Some old guests (which
have no knowledge about GICv4.1 vSGIs and don't care about nASSGIcap
at all) will also fail to migrate from A to B, just because now we
present two different (unused) GICD_TYPER2 registers to them.

Is it a little unfair to them :-) ?

Thanks,
Zenghui

2020-03-23 08:26:15

[permalink] [raw]

Subject: Re: [PATCH v5 20/23] KVM: arm64: GICv4.1: Plumb SGI implementation selection in the distributor

Hi Zenghui,

[...]

>> And actually, maybe we can handle that pretty cheaply. If userspace
>> tries to restore GICD_TYPER2 to a value that isn't what KVM can
>> offer, we just return an error. Exactly like we do for GICD_IIDR.
>> Hence the following patch:
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> index 28b639fd1abc..e72dcc454247 100644
>> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> @@ -156,6 +156,7 @@ static int vgic_mmio_uaccess_write_v3_misc(struct
>> kvm_vcpu *vcpu,
>>      struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>>
>>      switch (addr & 0x0c) {
>> +    case GICD_TYPER2:
>>      case GICD_IIDR:
>>          if (val != vgic_mmio_read_v3_misc(vcpu, addr, len))
>>              return -EINVAL;
>>
>> Being a RO register, writing something that isn't compatible with the
>> possible behaviour of the hypervisor will just return an error.
>
> This is really a nice point to address my concern! I'm happy to see
> this in v6 now.
>
>>
>> What do you think?
>
> I agreed with you, with a bit nervous though. Some old guests (which
> have no knowledge about GICv4.1 vSGIs and don't care about nASSGIcap
> at all) will also fail to migrate from A to B, just because now we
> present two different (unused) GICD_TYPER2 registers to them.
>
> Is it a little unfair to them :-) ?

I never pretended to be fair! ;-)

I'm happy to prevent migrating from a v4.1 system (A) to a v4.0
system (B). As soon as the guest has run, it isn't safe to do so
(it may have read TYPER2, and now knows about vSGIs). We *could*
track this and find ways to migrate this state as well, but it
feels fragile.

Migrating from B to A is more appealing. It should be possible to
do so without much difficulty (just check that the nASSGIcap bit
is either 0 or equal to KVM's view of the capability).

But overall I seriously doubt we can easily migrate guests across
very different HW. We've been talking about this for years, and
we still don't have a good solution for it given the diversity
of the ecosystem... :-/

Thanks,

M.
--
Jazz is not dead. It just smells funny...

2020-03-23 12:42:46

[permalink] [raw]

Subject: Re: [PATCH v5 20/23] KVM: arm64: GICv4.1: Plumb SGI implementation selection in the distributor

Hi Marc,

On 2020/3/23 16:25, Marc Zyngier wrote:
> Hi Zenghui,
>
> [...]
>
>>> And actually, maybe we can handle that pretty cheaply. If userspace
>>> tries to restore GICD_TYPER2 to a value that isn't what KVM can
>>> offer, we just return an error. Exactly like we do for GICD_IIDR.
>>> Hence the following patch:
>>>
>>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c
>>> b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>>> index 28b639fd1abc..e72dcc454247 100644
>>> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
>>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>>> @@ -156,6 +156,7 @@ static int vgic_mmio_uaccess_write_v3_misc(struct
>>> kvm_vcpu *vcpu,
>>>       struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>>>
>>>       switch (addr & 0x0c) {
>>> +    case GICD_TYPER2:
>>>       case GICD_IIDR:
>>>           if (val != vgic_mmio_read_v3_misc(vcpu, addr, len))
>>>               return -EINVAL;
>>>
>>> Being a RO register, writing something that isn't compatible with the
>>> possible behaviour of the hypervisor will just return an error.
>>
>> This is really a nice point to address my concern! I'm happy to see
>> this in v6 now.
>>
>>>
>>> What do you think?
>>
>> I agreed with you, with a bit nervous though. Some old guests (which
>> have no knowledge about GICv4.1 vSGIs and don't care about nASSGIcap
>> at all) will also fail to migrate from A to B, just because now we
>> present two different (unused) GICD_TYPER2 registers to them.
>>
>> Is it a little unfair to them :-) ?
>
> I never pretended to be fair! ;-)
>
> I'm happy to prevent migrating from a v4.1 system (A) to a v4.0
> system (B). As soon as the guest has run, it isn't safe to do so
> (it may have read TYPER2, and now knows about vSGIs). We *could*
> track this and find ways to migrate this state as well, but it
> feels fragile.
>
> Migrating from B to A is more appealing. It should be possible to
> do so without much difficulty (just check that the nASSGIcap bit
> is either 0 or equal to KVM's view of the capability).
>
> But overall I seriously doubt we can easily migrate guests across
> very different HW. We've been talking about this for years, and
> we still don't have a good solution for it given the diversity
> of the ecosystem... :-/

Fair enough. Thanks for your detailed explanation!

Zenghui

2020-03-29 20:27:11

[permalink] [raw]

Subject: [tip: irq/core] irqchip/gic-v4.1: Skip absent CPUs while iterating over redistributors

The following commit has been merged into the irq/core branch of tip:

Commit-ID: 28d160de5194c68ff534443d2a8b6f1d10d57c58
Gitweb: https://git.kernel.org/tip/28d160de5194c68ff534443d2a8b6f1d10d57c58
Author: Marc Zyngier <[email protected]>
AuthorDate: Wed, 04 Mar 2020 20:33:09
Committer: Marc Zyngier <[email protected]>
CommitterDate: Thu, 19 Mar 2020 11:21:58

irqchip/gic-v4.1: Skip absent CPUs while iterating over redistributors

In a system that is only sparsly populated with CPUs, we can end-up with
redistributors structures that are not initialized. Let's make sure we
don't try and access those when iterating over them (in this case when
checking we have a L2 VPE table).

Fixes: 4e6437f12d6e ("irqchip/gic-v4.1: Ensure L2 vPE table is allocated at RD level")
Signed-off-by: Marc Zyngier <[email protected]>
Reviewed-by: Zenghui Yu <[email protected]>
Reviewed-by: Eric Auger <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
drivers/irqchip/irq-gic-v3-its.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 83b1186..da883a6 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -2452,6 +2452,10 @@ static bool allocate_vpe_l2_table(int cpu, u32 id)
if (!gic_rdists->has_rvpeid)
return true;

+ /* Skip non-present CPUs */
+ if (!base)
+ return true;
+
val = gicr_read_vpropbaser(base + SZ_128K + GICR_VPROPBASER);

esz = FIELD_GET(GICR_VPROPBASER_4_1_ENTRY_SIZE, val) + 1;

2020-03-29 20:27:18

[permalink] [raw]

Subject: [tip: irq/core] irqchip/gic-v4.1: Move doorbell management to the GICv4 abstraction layer

The following commit has been merged into the irq/core branch of tip:

Commit-ID: ae699ad348cdcd416cbf28e8a02fc468780161f7
Gitweb: https://git.kernel.org/tip/ae699ad348cdcd416cbf28e8a02fc468780161f7
Author: Marc Zyngier <[email protected]>
AuthorDate: Wed, 04 Mar 2020 20:33:20
Committer: Marc Zyngier <[email protected]>
CommitterDate: Tue, 24 Mar 2020 12:15:51

irqchip/gic-v4.1: Move doorbell management to the GICv4 abstraction layer

In order to hide some of the differences between v4.0 and v4.1, move
the doorbell management out of the KVM code, and into the GICv4-specific
layer. This allows the calling code to ask for the doorbell when blocking,
and otherwise to leave the doorbell permanently disabled.

This matches the v4.1 code perfectly, and only results in a minor
refactoring of the v4.0 code.

Signed-off-by: Marc Zyngier <[email protected]>
Reviewed-by: Zenghui Yu <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
drivers/irqchip/irq-gic-v4.c | 45 ++++++++++++++++++++++++++---
include/kvm/arm_vgic.h | 1 +-
include/linux/irqchip/arm-gic-v4.h | 3 +-
virt/kvm/arm/vgic/vgic-v3.c | 4 ++-
virt/kvm/arm/vgic/vgic-v4.c | 34 +++++++++-------------
5 files changed, 61 insertions(+), 26 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v4.c b/drivers/irqchip/irq-gic-v4.c
index c01910d..117ba6d 100644
--- a/drivers/irqchip/irq-gic-v4.c
+++ b/drivers/irqchip/irq-gic-v4.c
@@ -87,6 +87,11 @@ static struct irq_domain *gic_domain;
static const struct irq_domain_ops *vpe_domain_ops;
static const struct irq_domain_ops *sgi_domain_ops;

+static bool has_v4_1(void)
+{
+ return !!sgi_domain_ops;
+}
+
int its_alloc_vcpu_irqs(struct its_vm *vm)
{
int vpe_base_irq, i;
@@ -139,18 +144,50 @@ static int its_send_vpe_cmd(struct its_vpe *vpe, struct its_cmd_info *info)
return irq_set_vcpu_affinity(vpe->irq, info);
}

-int its_schedule_vpe(struct its_vpe *vpe, bool on)
+int its_make_vpe_non_resident(struct its_vpe *vpe, bool db)
{
- struct its_cmd_info info;
+ struct irq_desc *desc = irq_to_desc(vpe->irq);
+ struct its_cmd_info info = { };
int ret;

WARN_ON(preemptible());

- info.cmd_type = on ? SCHEDULE_VPE : DESCHEDULE_VPE;
+ info.cmd_type = DESCHEDULE_VPE;
+ if (has_v4_1()) {
+ /* GICv4.1 can directly deal with doorbells */
+ info.req_db = db;
+ } else {
+ /* Undo the nested disable_irq() calls... */
+ while (db && irqd_irq_disabled(&desc->irq_data))
+ enable_irq(vpe->irq);
+ }
+
+ ret = its_send_vpe_cmd(vpe, &info);
+ if (!ret)
+ vpe->resident = false;
+
+ return ret;
+}
+
+int its_make_vpe_resident(struct its_vpe *vpe, bool g0en, bool g1en)
+{
+ struct its_cmd_info info = { };
+ int ret;
+
+ WARN_ON(preemptible());
+
+ info.cmd_type = SCHEDULE_VPE;
+ if (has_v4_1()) {
+ info.g0en = g0en;
+ info.g1en = g1en;
+ } else {
+ /* Disabled the doorbell, as we're about to enter the guest */
+ disable_irq_nosync(vpe->irq);
+ }

ret = its_send_vpe_cmd(vpe, &info);
if (!ret)
- vpe->resident = on;
+ vpe->resident = true;

return ret;
}
diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 9d53f54..6345790 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -70,6 +70,7 @@ struct vgic_global {

/* Hardware has GICv4? */
bool has_gicv4;
+ bool has_gicv4_1;

/* GIC system register CPU interface */
struct static_key_false gicv3_cpuif;
diff --git a/include/linux/irqchip/arm-gic-v4.h b/include/linux/irqchip/arm-gic-v4.h
index 1b34100..34ed4b5 100644
--- a/include/linux/irqchip/arm-gic-v4.h
+++ b/include/linux/irqchip/arm-gic-v4.h
@@ -125,7 +125,8 @@ struct its_cmd_info {

int its_alloc_vcpu_irqs(struct its_vm *vm);
void its_free_vcpu_irqs(struct its_vm *vm);
-int its_schedule_vpe(struct its_vpe *vpe, bool on);
+int its_make_vpe_resident(struct its_vpe *vpe, bool g0en, bool g1en);
+int its_make_vpe_non_resident(struct its_vpe *vpe, bool db);
int its_invall_vpe(struct its_vpe *vpe);
int its_map_vlpi(int irq, struct its_vlpi_map *map);
int its_get_vlpi(int irq, struct its_vlpi_map *map);
diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
index f45635a..1bc09b5 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -595,7 +595,9 @@ int vgic_v3_probe(const struct gic_kvm_info *info)
/* GICv4 support? */
if (info->has_v4) {
kvm_vgic_global_state.has_gicv4 = gicv4_enable;
- kvm_info("GICv4 support %sabled\n",
+ kvm_vgic_global_state.has_gicv4_1 = info->has_v4_1 && gicv4_enable;
+ kvm_info("GICv4%s support %sabled\n",
+ kvm_vgic_global_state.has_gicv4_1 ? ".1" : "",
gicv4_enable ? "en" : "dis");
}

diff --git a/virt/kvm/arm/vgic/vgic-v4.c b/virt/kvm/arm/vgic/vgic-v4.c
index 46f8755..1eb0f8c 100644
--- a/virt/kvm/arm/vgic/vgic-v4.c
+++ b/virt/kvm/arm/vgic/vgic-v4.c
@@ -67,10 +67,10 @@
* it. And if we've migrated our vcpu from one CPU to another, we must
* tell the ITS (so that the messages reach the right redistributor).
* This is done in two steps: first issue a irq_set_affinity() on the
- * irq corresponding to the vcpu, then call its_schedule_vpe(). You
- * must be in a non-preemptible context. On exit, another call to
- * its_schedule_vpe() tells the redistributor that we're done with the
- * vcpu.
+ * irq corresponding to the vcpu, then call its_make_vpe_resident().
+ * You must be in a non-preemptible context. On exit, a call to
+ * its_make_vpe_non_resident() tells the redistributor that we're done
+ * with the vcpu.
*
* Finally, the doorbell handling: Each vcpu is allocated an interrupt
* which will fire each time a VLPI is made pending whilst the vcpu is
@@ -86,7 +86,8 @@ static irqreturn_t vgic_v4_doorbell_handler(int irq, void *info)
struct kvm_vcpu *vcpu = info;

/* We got the message, no need to fire again */
- if (!irqd_irq_disabled(&irq_to_desc(irq)->irq_data))
+ if (!kvm_vgic_global_state.has_gicv4_1 &&
+ !irqd_irq_disabled(&irq_to_desc(irq)->irq_data))
disable_irq_nosync(irq);

vcpu->arch.vgic_cpu.vgic_v3.its_vpe.pending_last = true;
@@ -199,19 +200,11 @@ void vgic_v4_teardown(struct kvm *kvm)
int vgic_v4_put(struct kvm_vcpu *vcpu, bool need_db)
{
struct its_vpe *vpe = &vcpu->arch.vgic_cpu.vgic_v3.its_vpe;
- struct irq_desc *desc = irq_to_desc(vpe->irq);

if (!vgic_supports_direct_msis(vcpu->kvm) || !vpe->resident)
return 0;

- /*
- * If blocking, a doorbell is required. Undo the nested
- * disable_irq() calls...
- */
- while (need_db && irqd_irq_disabled(&desc->irq_data))
- enable_irq(vpe->irq);
-
- return its_schedule_vpe(vpe, false);
+ return its_make_vpe_non_resident(vpe, need_db);
}

int vgic_v4_load(struct kvm_vcpu *vcpu)
@@ -232,18 +225,19 @@ int vgic_v4_load(struct kvm_vcpu *vcpu)
if (err)
return err;

- /* Disabled the doorbell, as we're about to enter the guest */
- disable_irq_nosync(vpe->irq);
-
- err = its_schedule_vpe(vpe, true);
+ err = its_make_vpe_resident(vpe, false, vcpu->kvm->arch.vgic.enabled);
if (err)
return err;

/*
* Now that the VPE is resident, let's get rid of a potential
- * doorbell interrupt that would still be pending.
+ * doorbell interrupt that would still be pending. This is a
+ * GICv4.0 only "feature"...
*/
- return irq_set_irqchip_state(vpe->irq, IRQCHIP_STATE_PENDING, false);
+ if (!kvm_vgic_global_state.has_gicv4_1)
+ err = irq_set_irqchip_state(vpe->irq, IRQCHIP_STATE_PENDING, false);
+
+ return err;
}

static struct vgic_its *vgic_get_its(struct kvm *kvm,

2020-03-29 20:29:36

[permalink] [raw]

Subject: [tip: irq/core] irqchip/gic-v4.1: Plumb mask/unmask SGI callbacks

The following commit has been merged into the irq/core branch of tip:

Commit-ID: b4e8d644ec623cbb66f192a7fefbd0a66e314be8
Gitweb: https://git.kernel.org/tip/b4e8d644ec623cbb66f192a7fefbd0a66e314be8
Author: Marc Zyngier <[email protected]>
AuthorDate: Wed, 04 Mar 2020 20:33:17
Committer: Marc Zyngier <[email protected]>
CommitterDate: Tue, 24 Mar 2020 12:05:09

irqchip/gic-v4.1: Plumb mask/unmask SGI callbacks

Implement mask/unmask for virtual SGIs by calling into the
configuration helper.

Signed-off-by: Marc Zyngier <[email protected]>
Reviewed-by: Zenghui Yu <[email protected]>
Reviewed-by: Eric Auger <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
drivers/irqchip/irq-gic-v3-its.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 28c856a..bc6666a 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -3944,6 +3944,22 @@ static void its_configure_sgi(struct irq_data *d, bool clear)
its_send_single_vcommand(find_4_1_its(), its_build_vsgi_cmd, &desc);
}

+static void its_sgi_mask_irq(struct irq_data *d)
+{
+ struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
+
+ vpe->sgi_config[d->hwirq].enabled = false;
+ its_configure_sgi(d, false);
+}
+
+static void its_sgi_unmask_irq(struct irq_data *d)
+{
+ struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
+
+ vpe->sgi_config[d->hwirq].enabled = true;
+ its_configure_sgi(d, false);
+}
+
static int its_sgi_set_affinity(struct irq_data *d,
const struct cpumask *mask_val,
bool force)
@@ -3958,6 +3974,8 @@ static int its_sgi_set_affinity(struct irq_data *d,

static struct irq_chip its_sgi_irq_chip = {
.name = "GICv4.1-sgi",
+ .irq_mask = its_sgi_mask_irq,
+ .irq_unmask = its_sgi_unmask_irq,
.irq_set_affinity = its_sgi_set_affinity,
};

2020-03-29 20:30:33

[permalink] [raw]

Subject: [tip: irq/core] irqchip/gic-v4.1: Eagerly vmap vPEs

The following commit has been merged into the irq/core branch of tip:

Commit-ID: 009384b38034111bf2c0c7bfb2740f5bd45c176c
Gitweb: https://git.kernel.org/tip/009384b38034111bf2c0c7bfb2740f5bd45c176c
Author: Marc Zyngier <[email protected]>
AuthorDate: Wed, 04 Mar 2020 20:33:23
Committer: Marc Zyngier <[email protected]>
CommitterDate: Tue, 24 Mar 2020 12:15:51

irqchip/gic-v4.1: Eagerly vmap vPEs

Now that we have HW-accelerated SGIs being delivered to VPEs, it
becomes required to map the VPEs on all ITSs instead of relying
on the lazy approach that we would use when using the ITS-list
mechanism.

Signed-off-by: Marc Zyngier <[email protected]>
Reviewed-by: Zenghui Yu <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
drivers/irqchip/irq-gic-v3-its.c | 52 +++++++++++++++++++++++++------
1 file changed, 42 insertions(+), 10 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index aae5332..1259f7f 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -189,6 +189,15 @@ static DEFINE_IDA(its_vpeid_ida);
#define gic_data_rdist_rd_base() (gic_data_rdist()->rd_base)
#define gic_data_rdist_vlpi_base() (gic_data_rdist_rd_base() + SZ_128K)

+/*
+ * Skip ITSs that have no vLPIs mapped, unless we're on GICv4.1, as we
+ * always have vSGIs mapped.
+ */
+static bool require_its_list_vmovp(struct its_vm *vm, struct its_node *its)
+{
+ return (gic_rdists->has_rvpeid || vm->vlpi_count[its->list_nr]);
+}
+
static u16 get_its_list(struct its_vm *vm)
{
struct its_node *its;
@@ -198,7 +207,7 @@ static u16 get_its_list(struct its_vm *vm)
if (!is_v4(its))
continue;

- if (vm->vlpi_count[its->list_nr])
+ if (require_its_list_vmovp(vm, its))
__set_bit(its->list_nr, &its_list);
}

@@ -1295,7 +1304,7 @@ static void its_send_vmovp(struct its_vpe *vpe)
if (!is_v4(its))
continue;

- if (!vpe->its_vm->vlpi_count[its->list_nr])
+ if (!require_its_list_vmovp(vpe->its_vm, its))
continue;

desc.its_vmovp_cmd.col = &its->collections[col_id];
@@ -1586,12 +1595,31 @@ static int its_irq_set_irqchip_state(struct irq_data *d,
return 0;
}

+/*
+ * Two favourable cases:
+ *
+ * (a) Either we have a GICv4.1, and all vPEs have to be mapped at all times
+ * for vSGI delivery
+ *
+ * (b) Or the ITSs do not use a list map, meaning that VMOVP is cheap enough
+ * and we're better off mapping all VPEs always
+ *
+ * If neither (a) nor (b) is true, then we map vPEs on demand.
+ *
+ */
+static bool gic_requires_eager_mapping(void)
+{
+ if (!its_list_map || gic_rdists->has_rvpeid)
+ return true;
+
+ return false;
+}
+
static void its_map_vm(struct its_node *its, struct its_vm *vm)
{
unsigned long flags;

- /* Not using the ITS list? Everything is always mapped. */
- if (!its_list_map)
+ if (gic_requires_eager_mapping())
return;

raw_spin_lock_irqsave(&vmovp_lock, flags);
@@ -1625,7 +1653,7 @@ static void its_unmap_vm(struct its_node *its, struct its_vm *vm)
unsigned long flags;

/* Not using the ITS list? Everything is always mapped. */
- if (!its_list_map)
+ if (gic_requires_eager_mapping())
return;

raw_spin_lock_irqsave(&vmovp_lock, flags);
@@ -4282,8 +4310,12 @@ static int its_vpe_irq_domain_activate(struct irq_domain *domain,
struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
struct its_node *its;

- /* If we use the list map, we issue VMAPP on demand... */
- if (its_list_map)
+ /*
+ * If we use the list map, we issue VMAPP on demand... Unless
+ * we're on a GICv4.1 and we eagerly map the VPE on all ITSs
+ * so that VSGIs can work.
+ */
+ if (!gic_requires_eager_mapping())
return 0;

/* Map the VPE to the first possible CPU */
@@ -4309,10 +4341,10 @@ static void its_vpe_irq_domain_deactivate(struct irq_domain *domain,
struct its_node *its;

/*
- * If we use the list map, we unmap the VPE once no VLPIs are
- * associated with the VM.
+ * If we use the list map on GICv4.0, we unmap the VPE once no
+ * VLPIs are associated with the VM.
*/
- if (its_list_map)
+ if (!gic_requires_eager_mapping())
return;

list_for_each_entry(its, &its_nodes, entry) {

2020-03-29 20:55:12

[permalink] [raw]

Subject: [tip: irq/core] irqchip/gic-v4.1: Add VSGI allocation/teardown

The following commit has been merged into the irq/core branch of tip:

Commit-ID: 6d31b6ff985dbd144b2c4d519cf573b8f81865d9
Gitweb: https://git.kernel.org/tip/6d31b6ff985dbd144b2c4d519cf573b8f81865d9
Author: Marc Zyngier <[email protected]>
AuthorDate: Wed, 04 Mar 2020 20:33:21
Committer: Marc Zyngier <[email protected]>
CommitterDate: Tue, 24 Mar 2020 12:15:51

irqchip/gic-v4.1: Add VSGI allocation/teardown

Allocate per-VPE SGIs when initializing the GIC-specific part of the
VPE data structure.

Signed-off-by: Marc Zyngier <[email protected]>
Reviewed-by: Zenghui Yu <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
drivers/irqchip/irq-gic-v4.c | 68 ++++++++++++++++++++++++++++-
include/linux/irqchip/arm-gic-v4.h | 2 +-
2 files changed, 69 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-gic-v4.c b/drivers/irqchip/irq-gic-v4.c
index 117ba6d..99b33f6 100644
--- a/drivers/irqchip/irq-gic-v4.c
+++ b/drivers/irqchip/irq-gic-v4.c
@@ -92,6 +92,47 @@ static bool has_v4_1(void)
return !!sgi_domain_ops;
}

+static int its_alloc_vcpu_sgis(struct its_vpe *vpe, int idx)
+{
+ char *name;
+ int sgi_base;
+
+ if (!has_v4_1())
+ return 0;
+
+ name = kasprintf(GFP_KERNEL, "GICv4-sgi-%d", task_pid_nr(current));
+ if (!name)
+ goto err;
+
+ vpe->fwnode = irq_domain_alloc_named_id_fwnode(name, idx);
+ if (!vpe->fwnode)
+ goto err;
+
+ kfree(name);
+ name = NULL;
+
+ vpe->sgi_domain = irq_domain_create_linear(vpe->fwnode, 16,
+ sgi_domain_ops, vpe);
+ if (!vpe->sgi_domain)
+ goto err;
+
+ sgi_base = __irq_domain_alloc_irqs(vpe->sgi_domain, -1, 16,
+ NUMA_NO_NODE, vpe,
+ false, NULL);
+ if (sgi_base <= 0)
+ goto err;
+
+ return 0;
+
+err:
+ if (vpe->sgi_domain)
+ irq_domain_remove(vpe->sgi_domain);
+ if (vpe->fwnode)
+ irq_domain_free_fwnode(vpe->fwnode);
+ kfree(name);
+ return -ENOMEM;
+}
+
int its_alloc_vcpu_irqs(struct its_vm *vm)
{
int vpe_base_irq, i;
@@ -118,8 +159,13 @@ int its_alloc_vcpu_irqs(struct its_vm *vm)
if (vpe_base_irq <= 0)
goto err;

- for (i = 0; i < vm->nr_vpes; i++)
+ for (i = 0; i < vm->nr_vpes; i++) {
+ int ret;
vm->vpes[i]->irq = vpe_base_irq + i;
+ ret = its_alloc_vcpu_sgis(vm->vpes[i], i);
+ if (ret)
+ goto err;
+ }

return 0;

@@ -132,8 +178,28 @@ err:
return -ENOMEM;
}

+static void its_free_sgi_irqs(struct its_vm *vm)
+{
+ int i;
+
+ if (!has_v4_1())
+ return;
+
+ for (i = 0; i < vm->nr_vpes; i++) {
+ unsigned int irq = irq_find_mapping(vm->vpes[i]->sgi_domain, 0);
+
+ if (WARN_ON(!irq))
+ continue;
+
+ irq_domain_free_irqs(irq, 16);
+ irq_domain_remove(vm->vpes[i]->sgi_domain);
+ irq_domain_free_fwnode(vm->vpes[i]->fwnode);
+ }
+}
+
void its_free_vcpu_irqs(struct its_vm *vm)
{
+ its_free_sgi_irqs(vm);
irq_domain_free_irqs(vm->vpes[0]->irq, vm->nr_vpes);
irq_domain_remove(vm->domain);
irq_domain_free_fwnode(vm->fwnode);
diff --git a/include/linux/irqchip/arm-gic-v4.h b/include/linux/irqchip/arm-gic-v4.h
index 34ed4b5..b120a01 100644
--- a/include/linux/irqchip/arm-gic-v4.h
+++ b/include/linux/irqchip/arm-gic-v4.h
@@ -49,6 +49,8 @@ struct its_vpe {
};
/* GICv4.1 implementations */
struct {
+ struct fwnode_handle *fwnode;
+ struct irq_domain *sgi_domain;
struct {
u8 priority;
bool enabled;

2020-03-29 21:00:18

[permalink] [raw]

Subject: [tip: irq/core] irqchip/gic-v4.1: Plumb get/set_irqchip_state SGI callbacks

The following commit has been merged into the irq/core branch of tip:

Commit-ID: 7017ff0ee1de9d45fafee88a4e7890cce92f482e
Gitweb: https://git.kernel.org/tip/7017ff0ee1de9d45fafee88a4e7890cce92f482e
Author: Marc Zyngier <[email protected]>
AuthorDate: Wed, 04 Mar 2020 20:33:18
Committer: Marc Zyngier <[email protected]>
CommitterDate: Tue, 24 Mar 2020 12:05:09

irqchip/gic-v4.1: Plumb get/set_irqchip_state SGI callbacks

To implement the get/set_irqchip_state callbacks (limited to the
PENDING state), we have to use a particular set of hacks:

- Reading the pending state is done by using a pair of new redistributor
registers (GICR_VSGIR, GICR_VSGIPENDR), which allow the 16 interrupts
state to be retrieved.
- Setting the pending state is done by generating it as we'd otherwise do
for a guest (writing to GITS_SGIR).
- Clearing the pending state is done by emitting a VSGI command with the
"clear" bit set.

This requires some interesting locking though:
- When talking to the redistributor, we must make sure that the VPE
affinity doesn't change, hence taking the VPE lock.
- At the same time, we must ensure that nobody accesses the same
redistributor's GICR_VSGIR registers for a different VPE, which
would corrupt the reading of the pending bits. We thus take the
per-RD spinlock. Much fun.

Signed-off-by: Marc Zyngier <[email protected]>
Reviewed-by: Zenghui Yu <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
drivers/irqchip/irq-gic-v3-its.c | 77 +++++++++++++++++++++++++++++-
include/linux/irqchip/arm-gic-v3.h | 14 +++++-
2 files changed, 91 insertions(+)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index bc6666a..c614f0c 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -3972,11 +3972,88 @@ static int its_sgi_set_affinity(struct irq_data *d,
return IRQ_SET_MASK_OK;
}

+static int its_sgi_set_irqchip_state(struct irq_data *d,
+ enum irqchip_irq_state which,
+ bool state)
+{
+ if (which != IRQCHIP_STATE_PENDING)
+ return -EINVAL;
+
+ if (state) {
+ struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
+ struct its_node *its = find_4_1_its();
+ u64 val;
+
+ val = FIELD_PREP(GITS_SGIR_VPEID, vpe->vpe_id);
+ val |= FIELD_PREP(GITS_SGIR_VINTID, d->hwirq);
+ writeq_relaxed(val, its->sgir_base + GITS_SGIR - SZ_128K);
+ } else {
+ its_configure_sgi(d, true);
+ }
+
+ return 0;
+}
+
+static int its_sgi_get_irqchip_state(struct irq_data *d,
+ enum irqchip_irq_state which, bool *val)
+{
+ struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
+ void __iomem *base;
+ unsigned long flags;
+ u32 count = 1000000; /* 1s! */
+ u32 status;
+ int cpu;
+
+ if (which != IRQCHIP_STATE_PENDING)
+ return -EINVAL;
+
+ /*
+ * Locking galore! We can race against two different events:
+ *
+ * - Concurent vPE affinity change: we must make sure it cannot
+ * happen, or we'll talk to the wrong redistributor. This is
+ * identical to what happens with vLPIs.
+ *
+ * - Concurrent VSGIPENDR access: As it involves accessing two
+ * MMIO registers, this must be made atomic one way or another.
+ */
+ cpu = vpe_to_cpuid_lock(vpe, &flags);
+ raw_spin_lock(&gic_data_rdist_cpu(cpu)->rd_lock);
+ base = gic_data_rdist_cpu(cpu)->rd_base + SZ_128K;
+ writel_relaxed(vpe->vpe_id, base + GICR_VSGIR);
+ do {
+ status = readl_relaxed(base + GICR_VSGIPENDR);
+ if (!(status & GICR_VSGIPENDR_BUSY))
+ goto out;
+
+ count--;
+ if (!count) {
+ pr_err_ratelimited("Unable to get SGI status\n");
+ goto out;
+ }
+ cpu_relax();
+ udelay(1);
+ } while (count);
+
+out:
+ raw_spin_unlock(&gic_data_rdist_cpu(cpu)->rd_lock);
+ vpe_to_cpuid_unlock(vpe, flags);
+
+ if (!count)
+ return -ENXIO;
+
+ *val = !!(status & (1 << d->hwirq));
+
+ return 0;
+}
+
static struct irq_chip its_sgi_irq_chip = {
.name = "GICv4.1-sgi",
.irq_mask = its_sgi_mask_irq,
.irq_unmask = its_sgi_unmask_irq,
.irq_set_affinity = its_sgi_set_affinity,
+ .irq_set_irqchip_state = its_sgi_set_irqchip_state,
+ .irq_get_irqchip_state = its_sgi_get_irqchip_state,
};

static int its_sgi_irq_domain_alloc(struct irq_domain *domain,
diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
index fd3be49..590cdbe 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -345,6 +345,15 @@
#define GICR_VPENDBASER_4_1_VGRP1EN (1ULL << 58)
#define GICR_VPENDBASER_4_1_VPEID GENMASK_ULL(15, 0)

+#define GICR_VSGIR 0x0080
+
+#define GICR_VSGIR_VPEID GENMASK(15, 0)
+
+#define GICR_VSGIPENDR 0x0088
+
+#define GICR_VSGIPENDR_BUSY (1U << 31)
+#define GICR_VSGIPENDR_PENDING GENMASK(15, 0)
+
/*
* ITS registers, offsets from ITS_base
*/
@@ -368,6 +377,11 @@

#define GITS_TRANSLATER 0x10040

+#define GITS_SGIR 0x20020
+
+#define GITS_SGIR_VPEID GENMASK_ULL(47, 32)
+#define GITS_SGIR_VINTID GENMASK_ULL(3, 0)
+
#define GITS_CTLR_ENABLE (1U << 0)
#define GITS_CTLR_ImDe (1U << 1)
#define GITS_CTLR_ITS_NUMBER_SHIFT 4

2020-03-29 21:09:32