[PATCH v2 0/6] KVM: arm/arm64: vgic: Virtual interrupt grouping support

[PATCH v2 0/6] KVM: arm/arm64: vgic: Virtual interrupt grouping support

[Christoffer Dall]

This small series addresses a peculiarity of the current VGIC
implementation, namely that we don't support interrupt grouping.

KVM either implements a GICv2 without support for the security
extensions, or a GICv3 with DS=1.  For GICv2, on systems without the
security extensions, group 0 interrupts can be configured to be either
signalled as FIQs or as IRQs by the VM, whereas group 1 interrupts are
always IRQs.  For GICv3, with DS=1, group 1 interrupts are always IRQs
and group 0 interrupts are always FIQs, and there is no concept of
secure vs. non-secure group 1 interrupts when DS=1.

We were treating all interrupts on GICv2 as group 0, but yet telling the
guest that they were group 1.  The first patch changes this behavior,
which seems to have no effect on no known guests, but still.

The remaining patches introduce proper interrupt grouping support, along
with MMIO accessors for the VM and userspace to retrieve and set the
which group SGIs, PPIs, and SPIs belong to.  LPIs are always group 1
interrupts as per the architecture, and there is no way to modify this
configuration (no IGROUPR registers for LPIs or equivalent ITS
commands).

Lightly tested on Seattle, TX1, and the foundation model.  I've run a
GICv2 guest on a GICv3 host on the foundation model.  I've tested
migration on Seattle, and as expected, migrating a payload from an older
kernel fails, but migration on same kernel works fine both before and
after this series.  See the last patch for more info.

Applies to v4.18-rc3.

Changes since v1:
 - Bumped implementation revision field in GICD_IIDR along with changes
 - Changed logic in init code to correctly detect vgic emulation type
 - Rebased to v4.18-rc3

Christoffer Dall (6):
  KVM: arm/arm64: vgic: Define GICD_IIDR fields for GICv2 and GIv3
  KVM: arm/arm64: vgic: Keep track of implementation revision
  KVM: arm/arm64: vgic: GICv2 IGROUPR should read as zero
  KVM: arm/arm64: vgic: Add group field to struct irq
  KVM: arm/arm64: vgic: Signal IRQs using their configured group
  KVM: arm/arm64: vgic: Allow configuration of interrupt groups

 include/kvm/arm_vgic.h             |  4 ++++
 include/linux/irqchip/arm-gic-v3.h | 10 ++++++++++
 include/linux/irqchip/arm-gic.h    | 11 +++++++++++
 virt/kvm/arm/vgic/vgic-debug.c     |  8 +++++---
 virt/kvm/arm/vgic/vgic-init.c      | 20 ++++++++++++++++++--
 virt/kvm/arm/vgic/vgic-its.c       |  1 +
 virt/kvm/arm/vgic/vgic-mmio-v2.c   | 19 +++++++++++++++----
 virt/kvm/arm/vgic/vgic-mmio-v3.c   | 20 +++++++++++++++-----
 virt/kvm/arm/vgic/vgic-mmio.c      | 38 ++++++++++++++++++++++++++++++++++++++
 virt/kvm/arm/vgic/vgic-mmio.h      |  6 ++++++
 virt/kvm/arm/vgic/vgic-v2.c        |  3 +++
 virt/kvm/arm/vgic/vgic-v3.c        |  6 +-----
 12 files changed, 127 insertions(+), 19 deletions(-)

--
2.7.4

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

[PATCH v2 1/6] KVM: arm/arm64: vgic: Define GICD_IIDR fields for GICv2 and GIv3

[Christoffer Dall]

Instead of hardcoding the shifts and masks in the GICD_IIDR register
emulation, let's add the definition of these fields to the GIC header
files and use them.

This will make things more obivous when we're going to bump the revision
in the IIDR when we'll make guest-visible changes to the implementation.

Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
---
 include/linux/irqchip/arm-gic-v3.h | 10 ++++++++++
 include/linux/irqchip/arm-gic.h    | 10 ++++++++++
 virt/kvm/arm/vgic/vgic-mmio-v2.c   |  3 ++-
 virt/kvm/arm/vgic/vgic-mmio-v3.c   |  3 ++-
 4 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
index cbb872c..b22f9df 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -61,6 +61,16 @@
 #define GICD_CTLR_ENABLE_G1A           (1U << 1)
 #define GICD_CTLR_ENABLE_G1            (1U << 0)

+#define GICD_IIDR_IMPLEMENTER_SHIFT    0
+#define GICD_IIDR_IMPLEMENTER_MASK     (0xfff << GICD_IIDR_IMPLEMENTER_SHIFT)
+#define GICD_IIDR_REVISION_SHIFT       12
+#define GICD_IIDR_REVISION_MASK                (0xf << GICD_IIDR_REVISION_SHIFT)
+#define GICD_IIDR_VARIANT_SHIFT                16
+#define GICD_IIDR_VARIANT_MASK         (0xf << GICD_IIDR_VARIANT_SHIFT)
+#define GICD_IIDR_PRODUCT_ID_SHIFT     24
+#define GICD_IIDR_PRODUCT_ID_MASK      (0xff << GICD_IIDR_PRODUCT_ID_SHIFT)
+
+
 /*
  * In systems with a single security state (what we emulate in KVM)
  * the meaning of the interrupt group enable bits is slightly different
diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h
index 68d8b1f..484f5bf 100644
--- a/include/linux/irqchip/arm-gic.h
+++ b/include/linux/irqchip/arm-gic.h
@@ -71,6 +71,16 @@
                                        (GICD_INT_DEF_PRI << 8) |\
                                        GICD_INT_DEF_PRI)

+#define GICD_IIDR_IMPLEMENTER_SHIFT    0
+#define GICD_IIDR_IMPLEMENTER_MASK     (0xfff << GICD_IIDR_IMPLEMENTER_SHIFT)
+#define GICD_IIDR_REVISION_SHIFT       12
+#define GICD_IIDR_REVISION_MASK                (0xf << GICD_IIDR_REVISION_SHIFT)
+#define GICD_IIDR_VARIANT_SHIFT                16
+#define GICD_IIDR_VARIANT_MASK         (0xf << GICD_IIDR_VARIANT_SHIFT)
+#define GICD_IIDR_PRODUCT_ID_SHIFT     24
+#define GICD_IIDR_PRODUCT_ID_MASK      (0xff << GICD_IIDR_PRODUCT_ID_SHIFT)
+
+
 #define GICH_HCR                       0x0
 #define GICH_VTR                       0x4
 #define GICH_VMCR                      0x8
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
index ffc587b..af44e569 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
@@ -37,7 +37,8 @@ static unsigned long vgic_mmio_read_v2_misc(struct kvm_vcpu *vcpu,
                value |= (atomic_read(&vcpu->kvm->online_vcpus) - 1) << 5;
                break;
        case GIC_DIST_IIDR:
-               value = (PRODUCT_ID_KVM << 24) | (IMPLEMENTER_ARM << 0);
+               value = (PRODUCT_ID_KVM << GICD_IIDR_PRODUCT_ID_SHIFT) |
+                       (IMPLEMENTER_ARM << GICD_IIDR_IMPLEMENTER_SHIFT);
                break;
        default:
                return 0;
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index 2877840..c03f424 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -81,7 +81,8 @@ static unsigned long vgic_mmio_read_v3_misc(struct kvm_vcpu *vcpu,
                }
                break;
        case GICD_IIDR:
-               value = (PRODUCT_ID_KVM << 24) | (IMPLEMENTER_ARM << 0);
+               value = (PRODUCT_ID_KVM << GICD_IIDR_PRODUCT_ID_SHIFT) |
+                       (IMPLEMENTER_ARM << GICD_IIDR_IMPLEMENTER_SHIFT);
                break;
        default:
                return 0;
--
2.7.4

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

[PATCH v2 2/6] KVM: arm/arm64: vgic: Keep track of implementation revision

[Christoffer Dall]

As we are about to tweak implementation aspects of the VGIC emulation,
while still preserving some level of backwards compatibility support,
add a field to keep track of the implementation revision field which is
reported to the VM and to userspace.

Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
---
 include/kvm/arm_vgic.h           | 3 +++
 virt/kvm/arm/vgic/vgic-init.c    | 1 +
 virt/kvm/arm/vgic/vgic-mmio-v2.c | 6 ++++--
 virt/kvm/arm/vgic/vgic-mmio-v3.c | 6 ++++--
 4 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index cfdd248..7e64c46 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -217,6 +217,9 @@ struct vgic_dist {
        /* vGIC model the kernel emulates for the guest (GICv2 or GICv3) */
        u32                     vgic_model;

+       /* Implementation revision as reported in the GICD_IIDR */
+       u32                     implementation_rev;
+
        /* Do injected MSIs require an additional device ID? */
        bool                    msis_require_devid;

diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
index b714179..8b6fc45 100644
--- a/virt/kvm/arm/vgic/vgic-init.c
+++ b/virt/kvm/arm/vgic/vgic-init.c
@@ -298,6 +298,7 @@ int vgic_init(struct kvm *kvm)

        vgic_debug_init(kvm);

+       dist->implementation_rev = 0;
        dist->initialized = true;

 out:
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
index af44e569..f0c5351 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
@@ -25,19 +25,21 @@
 static unsigned long vgic_mmio_read_v2_misc(struct kvm_vcpu *vcpu,
                                            gpa_t addr, unsigned int len)
 {
+       struct vgic_dist *vgic = &vcpu->kvm->arch.vgic;
        u32 value;

        switch (addr & 0x0c) {
        case GIC_DIST_CTRL:
-               value = vcpu->kvm->arch.vgic.enabled ? GICD_ENABLE : 0;
+               value = vgic->enabled ? GICD_ENABLE : 0;
                break;
        case GIC_DIST_CTR:
-               value = vcpu->kvm->arch.vgic.nr_spis + VGIC_NR_PRIVATE_IRQS;
+               value = vgic->nr_spis + VGIC_NR_PRIVATE_IRQS;
                value = (value >> 5) - 1;
                value |= (atomic_read(&vcpu->kvm->online_vcpus) - 1) << 5;
                break;
        case GIC_DIST_IIDR:
                value = (PRODUCT_ID_KVM << GICD_IIDR_PRODUCT_ID_SHIFT) |
+                       (vgic->implementation_rev << GICD_IIDR_REVISION_SHIFT) |
                        (IMPLEMENTER_ARM << GICD_IIDR_IMPLEMENTER_SHIFT);
                break;
        default:
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index c03f424..ebe10a0 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -62,16 +62,17 @@ bool vgic_supports_direct_msis(struct kvm *kvm)
 static unsigned long vgic_mmio_read_v3_misc(struct kvm_vcpu *vcpu,
                                            gpa_t addr, unsigned int len)
 {
+       struct vgic_dist *vgic = &vcpu->kvm->arch.vgic;
        u32 value = 0;

        switch (addr & 0x0c) {
        case GICD_CTLR:
-               if (vcpu->kvm->arch.vgic.enabled)
+               if (vgic->enabled)
                        value |= GICD_CTLR_ENABLE_SS_G1;
                value |= GICD_CTLR_ARE_NS | GICD_CTLR_DS;
                break;
        case GICD_TYPER:
-               value = vcpu->kvm->arch.vgic.nr_spis + VGIC_NR_PRIVATE_IRQS;
+               value = vgic->nr_spis + VGIC_NR_PRIVATE_IRQS;
                value = (value >> 5) - 1;
                if (vgic_has_its(vcpu->kvm)) {
                        value |= (INTERRUPT_ID_BITS_ITS - 1) << 19;
@@ -82,6 +83,7 @@ static unsigned long vgic_mmio_read_v3_misc(struct kvm_vcpu *vcpu,
                break;
        case GICD_IIDR:
                value = (PRODUCT_ID_KVM << GICD_IIDR_PRODUCT_ID_SHIFT) |
+                       (vgic->implementation_rev << GICD_IIDR_REVISION_SHIFT) |
                        (IMPLEMENTER_ARM << GICD_IIDR_IMPLEMENTER_SHIFT);
                break;
        default:
--
2.7.4

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

[PATCH v2 3/6] KVM: arm/arm64: vgic: GICv2 IGROUPR should read as zero

[Christoffer Dall]

We currently don't support grouping in the emulated VGIC, which is a
known defect on KVM (not hurting any currently used guests as far as
we're aware). This is currently handled by treating all interrupts as
group 0 interrupts for an emulated GICv2 and always signaling interrupts
as group 0 to the virtual CPU interface.

However, when reading which group interrupts belongs to in the guest
from the emulated VGIC, the VGIC currently reports group 1 instead of
group 0, which is misleading.  Fix this temporarily before introducing
full group support by changing the hander to _raz instead of _rao.

Fixes: fb848db39661a "KVM: arm/arm64: vgic-new: Add GICv2 MMIO handling framework"
Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
---
 virt/kvm/arm/vgic/vgic-init.c    | 2 +-
 virt/kvm/arm/vgic/vgic-mmio-v2.c | 8 +++++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
index 8b6fc45..230c922 100644
--- a/virt/kvm/arm/vgic/vgic-init.c
+++ b/virt/kvm/arm/vgic/vgic-init.c
@@ -298,7 +298,7 @@ int vgic_init(struct kvm *kvm)

        vgic_debug_init(kvm);

-       dist->implementation_rev = 0;
+       dist->implementation_rev = 1;
        dist->initialized = true;

 out:
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
index f0c5351..db646f1 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
@@ -22,6 +22,12 @@
 #include "vgic.h"
 #include "vgic-mmio.h"

+/*
+ * The Revision field in the IIDR have the following meanings:
+ *
+ * Revision 1: Report GICv2 interrupts as group 0 instead of group 1
+ */
+
 static unsigned long vgic_mmio_read_v2_misc(struct kvm_vcpu *vcpu,
                                            gpa_t addr, unsigned int len)
 {
@@ -365,7 +371,7 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = {
                vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12,
                VGIC_ACCESS_32bit),
        REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_IGROUP,
-               vgic_mmio_read_rao, vgic_mmio_write_wi, NULL, NULL, 1,
+               vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1,
                VGIC_ACCESS_32bit),
        REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ENABLE_SET,
                vgic_mmio_read_enable, vgic_mmio_write_senable, NULL, NULL, 1,
--
2.7.4

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

[PATCH v2 4/6] KVM: arm/arm64: vgic: Add group field to struct irq

[Christoffer Dall]

In preparation for proper group 0 and group 1 support in the vgic, we
add a field in the struct irq to store the group of all interrupts.

We initialize the group to group 0 when emulating GICv2 and to group 1
when emulating GICv3, just like we treat them today.  LPIs are always
group 1.  We also continue to ignore writes from the guest, preserving
existing functionality, for now.

Finally, we also add this field to the vgic debug logic to show the
group for all interrupts.

Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
---
 include/kvm/arm_vgic.h         |  1 +
 virt/kvm/arm/vgic/vgic-debug.c |  8 +++++---
 virt/kvm/arm/vgic/vgic-init.c  | 19 +++++++++++++++++--
 virt/kvm/arm/vgic/vgic-its.c   |  1 +
 4 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 7e64c46..c661d0e 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -133,6 +133,7 @@ struct vgic_irq {
        u8 source;                      /* GICv2 SGIs only */
        u8 active_source;               /* GICv2 SGIs only */
        u8 priority;
+       u8 group;                       /* 0 == group 0, 1 == group 1 */
        enum vgic_irq_config config;    /* Level or edge */

        /*
diff --git a/virt/kvm/arm/vgic/vgic-debug.c b/virt/kvm/arm/vgic/vgic-debug.c
index c589d4c..d3a403f 100644
--- a/virt/kvm/arm/vgic/vgic-debug.c
+++ b/virt/kvm/arm/vgic/vgic-debug.c
@@ -148,6 +148,7 @@ static void print_dist_state(struct seq_file *s, struct vgic_dist *dist)

        seq_printf(s, "P=pending_latch, L=line_level, A=active\n");
        seq_printf(s, "E=enabled, H=hw, C=config (level=1, edge=0)\n");
+       seq_printf(s, "G=group\n");
 }

 static void print_header(struct seq_file *s, struct vgic_irq *irq,
@@ -162,8 +163,8 @@ static void print_header(struct seq_file *s, struct vgic_irq *irq,
        }

        seq_printf(s, "\n");
-       seq_printf(s, "%s%2d TYP   ID TGT_ID PLAEHC     HWID   TARGET SRC PRI VCPU_ID\n", hdr, id);
-       seq_printf(s, "---------------------------------------------------------------\n");
+       seq_printf(s, "%s%2d TYP   ID TGT_ID PLAEHCG     HWID   TARGET SRC PRI VCPU_ID\n", hdr, id);
+       seq_printf(s, "----------------------------------------------------------------\n");
 }

 static void print_irq_state(struct seq_file *s, struct vgic_irq *irq,
@@ -182,7 +183,7 @@ static void print_irq_state(struct seq_file *s, struct vgic_irq *irq,

        seq_printf(s, "       %s %4d "
                      "    %2d "
-                     "%d%d%d%d%d%d "
+                     "%d%d%d%d%d%d%d "
                      "%8d "
                      "%8x "
                      " %2x "
@@ -197,6 +198,7 @@ static void print_irq_state(struct seq_file *s, struct vgic_irq *irq,
                        irq->enabled,
                        irq->hw,
                        irq->config == VGIC_CONFIG_LEVEL,
+                       irq->group,
                        irq->hwintid,
                        irq->mpidr,
                        irq->source,
diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
index 230c922..a7c19cd 100644
--- a/virt/kvm/arm/vgic/vgic-init.c
+++ b/virt/kvm/arm/vgic/vgic-init.c
@@ -175,10 +175,13 @@ static int kvm_vgic_dist_init(struct kvm *kvm, unsigned int nr_spis)
                irq->vcpu = NULL;
                irq->target_vcpu = vcpu0;
                kref_init(&irq->refcount);
-               if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2)
+               if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2) {
                        irq->targets = 0;
-               else
+                       irq->group = 0;
+               } else {
                        irq->mpidr = 0;
+                       irq->group = 1;
+               }
        }
        return 0;
 }
@@ -227,6 +230,18 @@ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
                        /* PPIs */
                        irq->config = VGIC_CONFIG_LEVEL;
                }
+
+               /*
+                * GICv3 can only be created via the KVM_DEVICE_CREATE API and
+                * so we always know the emulation type at this point as it's
+                * either explicitly configured as GICv3, or explicitly
+                * configured as GICv2, or not configured yet which also
+                * implies GICv2.
+                */
+               if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3)
+                       irq->group = 1;
+               else
+                       irq->group = 0;
        }

        if (!irqchip_in_kernel(vcpu->kvm))
diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 4ed79c9..92840c0 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -71,6 +71,7 @@ static struct vgic_irq *vgic_add_lpi(struct kvm *kvm, u32 intid,
        kref_init(&irq->refcount);
        irq->intid = intid;
        irq->target_vcpu = vcpu;
+       irq->group = 1;

        spin_lock_irqsave(&dist->lpi_list_lock, flags);

--
2.7.4

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

[PATCH v2 5/6] KVM: arm/arm64: vgic: Signal IRQs using their configured group

[Christoffer Dall]

Now when we have a group configuration on the struct IRQ, use this state
when populating the LR and signaling interrupts as either group 0 or
group 1 to the VM.  Depending on the model of the emulated GIC, and the
guest's configuration of the VMCR, interrupts may be signaled as IRQs or
FIQs to the VM.

Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
---
 include/linux/irqchip/arm-gic.h | 1 +
 virt/kvm/arm/vgic/vgic-v2.c     | 3 +++
 virt/kvm/arm/vgic/vgic-v3.c     | 6 +-----
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h
index 484f5bf..6c4aaf0 100644
--- a/include/linux/irqchip/arm-gic.h
+++ b/include/linux/irqchip/arm-gic.h
@@ -104,6 +104,7 @@
 #define GICH_LR_PENDING_BIT            (1 << 28)
 #define GICH_LR_ACTIVE_BIT             (1 << 29)
 #define GICH_LR_EOI                    (1 << 19)
+#define GICH_LR_GROUP1                 (1 << 30)
 #define GICH_LR_HW                     (1 << 31)

 #define GICH_VMCR_ENABLE_GRP0_SHIFT    0
diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
index a5f2e44..df5e6a6 100644
--- a/virt/kvm/arm/vgic/vgic-v2.c
+++ b/virt/kvm/arm/vgic/vgic-v2.c
@@ -159,6 +159,9 @@ void vgic_v2_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr)
                }
        }

+       if (irq->group)
+               val |= GICH_LR_GROUP1;
+
        if (irq->hw) {
                val |= GICH_LR_HW;
                val |= irq->hwintid << GICH_LR_PHYSID_CPUID_SHIFT;
diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
index cdce653..530b849 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -197,11 +197,7 @@ void vgic_v3_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr)
        if (vgic_irq_is_mapped_level(irq) && (val & ICH_LR_PENDING_BIT))
                irq->line_level = false;

-       /*
-        * We currently only support Group1 interrupts, which is a
-        * known defect. This needs to be addressed at some point.
-        */
-       if (model == KVM_DEV_TYPE_ARM_VGIC_V3)
+       if (irq->group)
                val |= ICH_LR_GROUP;

        val |= (u64)irq->priority << ICH_LR_PRIORITY_SHIFT;
--
2.7.4

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

[PATCH v2 6/6] KVM: arm/arm64: vgic: Allow configuration of interrupt groups

[Christoffer Dall]

Implement the required MMIO accessors for GICv2 and GICv3 for the
IGROUPR distributor and redistributor registers.

This can allow guests to change behavior compared to running on previous
versions of KVM, but only to align with the architecture and hardware
implementations.

This also allows userspace to configure the groups for interrupts.  Note
that this potentially results in GICv2 guests not receiving interrupts
after migration if migrating from an older kernel that exposes GICv2
interrupts as group 1.

Cc: Andrew Jones <drjones@redhat.com>
Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
---
I implemented (but stashed) a version of this which predicated the
behavior based on the value of GICD_IIDR revision field, falling back to
ignoring writes and resetting GICv2 groups to 0 if the guest wrote a
revision less than 2.  However, current QEMU implementations simply
don't write the GICD_IIDR, so this doesn't help at all without changing
QEMU anyhow.

The only actual fix I can see here to work around the problem in the
kernel is to require an opt-in to allow restoring groups from userspace,
but that's a lot of logic to support cross-kernel version migration.

Question: Do we expect that cross-kernel version migration is a critical
feature that people really expect to work, and do we actually have
examples of catering to this in the kernel elsewhere?  (Also, how would
then that relate to the whole 'adding a new sysreg breaks migration'
situation?)

 virt/kvm/arm/vgic/vgic-init.c    |  2 +-
 virt/kvm/arm/vgic/vgic-mmio-v2.c |  4 +++-
 virt/kvm/arm/vgic/vgic-mmio-v3.c | 11 +++++++++--
 virt/kvm/arm/vgic/vgic-mmio.c    | 38 ++++++++++++++++++++++++++++++++++++++
 virt/kvm/arm/vgic/vgic-mmio.h    |  6 ++++++
 5 files changed, 57 insertions(+), 4 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
index a7c19cd..c0c0b88 100644
--- a/virt/kvm/arm/vgic/vgic-init.c
+++ b/virt/kvm/arm/vgic/vgic-init.c
@@ -313,7 +313,7 @@ int vgic_init(struct kvm *kvm)

        vgic_debug_init(kvm);

-       dist->implementation_rev = 1;
+       dist->implementation_rev = 2;
        dist->initialized = true;

 out:
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
index db646f1..a7f09b5 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
@@ -26,6 +26,8 @@
  * The Revision field in the IIDR have the following meanings:
  *
  * Revision 1: Report GICv2 interrupts as group 0 instead of group 1
+ * Revision 2: Interrupt groups are guest-configurable and signaled using
+ *            their configured groups.
  */

 static unsigned long vgic_mmio_read_v2_misc(struct kvm_vcpu *vcpu,
@@ -371,7 +373,7 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = {
                vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12,
                VGIC_ACCESS_32bit),
        REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_IGROUP,
-               vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1,
+               vgic_mmio_read_group, vgic_mmio_write_group, NULL, NULL, 1,
                VGIC_ACCESS_32bit),
        REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ENABLE_SET,
                vgic_mmio_read_enable, vgic_mmio_write_senable, NULL, NULL, 1,
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index ebe10a0..49df2a1 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -59,6 +59,13 @@ bool vgic_supports_direct_msis(struct kvm *kvm)
        return kvm_vgic_global_state.has_gicv4 && vgic_has_its(kvm);
 }

+/*
+ * The Revision field in the IIDR have the following meanings:
+ *
+ * Revision 2: Interrupt groups are guest-configurable and signaled using
+ *            their configured groups.
+ */
+
 static unsigned long vgic_mmio_read_v3_misc(struct kvm_vcpu *vcpu,
                                            gpa_t addr, unsigned int len)
 {
@@ -454,7 +461,7 @@ static const struct vgic_register_region vgic_v3_dist_registers[] = {
                vgic_mmio_read_rao, vgic_mmio_write_wi, 4,
                VGIC_ACCESS_32bit),
        REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IGROUPR,
-               vgic_mmio_read_rao, vgic_mmio_write_wi, NULL, NULL, 1,
+               vgic_mmio_read_group, vgic_mmio_write_group, NULL, NULL, 1,
                VGIC_ACCESS_32bit),
        REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISENABLER,
                vgic_mmio_read_enable, vgic_mmio_write_senable, NULL, NULL, 1,
@@ -527,7 +534,7 @@ static const struct vgic_register_region vgic_v3_rdbase_registers[] = {

 static const struct vgic_register_region vgic_v3_sgibase_registers[] = {
        REGISTER_DESC_WITH_LENGTH(GICR_IGROUPR0,
-               vgic_mmio_read_rao, vgic_mmio_write_wi, 4,
+               vgic_mmio_read_group, vgic_mmio_write_group, 4,
                VGIC_ACCESS_32bit),
        REGISTER_DESC_WITH_LENGTH(GICR_ISENABLER0,
                vgic_mmio_read_enable, vgic_mmio_write_senable, 4,
diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
index ff9655c..ae31bd0 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.c
+++ b/virt/kvm/arm/vgic/vgic-mmio.c
@@ -40,6 +40,44 @@ void vgic_mmio_write_wi(struct kvm_vcpu *vcpu, gpa_t addr,
        /* Ignore */
 }

+unsigned long vgic_mmio_read_group(struct kvm_vcpu *vcpu,
+                                  gpa_t addr, unsigned int len)
+{
+       u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
+       u32 value = 0;
+       int i;
+
+       /* Loop over all IRQs affected by this read */
+       for (i = 0; i < len * 8; i++) {
+               struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
+
+               if (irq->group)
+                       value |= BIT(i);
+
+               vgic_put_irq(vcpu->kvm, irq);
+       }
+
+       return value;
+}
+
+void vgic_mmio_write_group(struct kvm_vcpu *vcpu, gpa_t addr,
+                          unsigned int len, unsigned long val)
+{
+       u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
+       int i;
+       unsigned long flags;
+
+       for (i = 0; i < len * 8; i++) {
+               struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
+
+               spin_lock_irqsave(&irq->irq_lock, flags);
+               irq->group = !!(val & BIT(i));
+               vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
+
+               vgic_put_irq(vcpu->kvm, irq);
+       }
+}
+
 /*
  * Read accesses to both GICD_ICENABLER and GICD_ISENABLER return the value
  * of the enabled bit, so there is only one function for both here.
diff --git a/virt/kvm/arm/vgic/vgic-mmio.h b/virt/kvm/arm/vgic/vgic-mmio.h
index 5693f6df..1079862 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.h
+++ b/virt/kvm/arm/vgic/vgic-mmio.h
@@ -134,6 +134,12 @@ unsigned long vgic_mmio_read_rao(struct kvm_vcpu *vcpu,
 void vgic_mmio_write_wi(struct kvm_vcpu *vcpu, gpa_t addr,
                        unsigned int len, unsigned long val);

+unsigned long vgic_mmio_read_group(struct kvm_vcpu *vcpu, gpa_t addr,
+                                  unsigned int len);
+
+void vgic_mmio_write_group(struct kvm_vcpu *vcpu, gpa_t addr,
+                          unsigned int len, unsigned long val);
+
 unsigned long vgic_mmio_read_enable(struct kvm_vcpu *vcpu,
                                    gpa_t addr, unsigned int len);

--
2.7.4

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

[PATCH v2 6/6] KVM: arm/arm64: vgic: Allow configuration of interrupt groups

[Marc Zyngier]

On 04/07/18 10:38, Christoffer Dall wrote:
> Implement the required MMIO accessors for GICv2 and GICv3 for the
> IGROUPR distributor and redistributor registers.
> 
> This can allow guests to change behavior compared to running on previous
> versions of KVM, but only to align with the architecture and hardware
> implementations.
> 
> This also allows userspace to configure the groups for interrupts.  Note
> that this potentially results in GICv2 guests not receiving interrupts
> after migration if migrating from an older kernel that exposes GICv2
> interrupts as group 1.
> 
> Cc: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> ---
> I implemented (but stashed) a version of this which predicated the
> behavior based on the value of GICD_IIDR revision field, falling back to
> ignoring writes and resetting GICv2 groups to 0 if the guest wrote a
> revision less than 2.  However, current QEMU implementations simply
> don't write the GICD_IIDR, so this doesn't help at all without changing
> QEMU anyhow.
> 
> The only actual fix I can see here to work around the problem in the
> kernel is to require an opt-in to allow restoring groups from userspace,
> but that's a lot of logic to support cross-kernel version migration.
> 
> Question: Do we expect that cross-kernel version migration is a critical
> feature that people really expect to work, and do we actually have
> examples of catering to this in the kernel elsewhere?  (Also, how would
> then that relate to the whole 'adding a new sysreg breaks migration'
> situation?)

I don't really get why QEMU doesn't try to restore GICD_IIDR, while it
is definitely trying to restore RO sysregs (and that's how we detect
incompatibilities).

I think we should at least give userspace a chance to do the right
thing. If it doesn't, well, too bad.

How bad is that "writable GICD_IIDR" patch? Because at this stage, and
in the absence of any comment, I'm close to just pick that series and
merge it for 4.19.

Thanks,

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

[PATCH v2 6/6] KVM: arm/arm64: vgic: Allow configuration of interrupt groups

[Peter Maydell]

On 9 July 2018 at 09:42, Marc Zyngier <marc.zyngier@arm.com> wrote:
> I don't really get why QEMU doesn't try to restore GICD_IIDR, while it
> is definitely trying to restore RO sysregs (and that's how we detect
> incompatibilities).

Accident of design, mostly. From QEMU's point of view, GICD_IIDR
is part of the GIC device, which we save and restore as a separate
thing from the CPU. The GIC device was written in what for QEMU
is a more 'traditional' style, where QEMU assumes it knows all the
registers that might have state and saves and restores them all
(and doesn't bother to do anything with constant registers).
The CPU sysregs are done in a completely different style[*], where
we let the kernel be the source of truth for what sysregs exist;
as a side effect of that we end up trying to save and restore
constant sysregs, since QEMU doesn't know they're constant.

[*] there's an argument that in retrospect this was a mistake;
still, it is what we have and trying to upend it now would be
a huge pain.

thanks
-- PMM

[PATCH v2 6/6] KVM: arm/arm64: vgic: Allow configuration of interrupt groups

[Christoffer Dall]

On Mon, Jul 09, 2018 at 09:42:40AM +0100, Marc Zyngier wrote:
> On 04/07/18 10:38, Christoffer Dall wrote:
> > Implement the required MMIO accessors for GICv2 and GICv3 for the
> > IGROUPR distributor and redistributor registers.
> > 
> > This can allow guests to change behavior compared to running on previous
> > versions of KVM, but only to align with the architecture and hardware
> > implementations.
> > 
> > This also allows userspace to configure the groups for interrupts.  Note
> > that this potentially results in GICv2 guests not receiving interrupts
> > after migration if migrating from an older kernel that exposes GICv2
> > interrupts as group 1.
> > 
> > Cc: Andrew Jones <drjones@redhat.com>
> > Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> > ---
> > I implemented (but stashed) a version of this which predicated the
> > behavior based on the value of GICD_IIDR revision field, falling back to
> > ignoring writes and resetting GICv2 groups to 0 if the guest wrote a
> > revision less than 2.  However, current QEMU implementations simply
> > don't write the GICD_IIDR, so this doesn't help at all without changing
> > QEMU anyhow.
> > 
> > The only actual fix I can see here to work around the problem in the
> > kernel is to require an opt-in to allow restoring groups from userspace,
> > but that's a lot of logic to support cross-kernel version migration.
> > 
> > Question: Do we expect that cross-kernel version migration is a critical
> > feature that people really expect to work, and do we actually have
> > examples of catering to this in the kernel elsewhere?  (Also, how would
> > then that relate to the whole 'adding a new sysreg breaks migration'
> > situation?)
> 
> I don't really get why QEMU doesn't try to restore GICD_IIDR, while it
> is definitely trying to restore RO sysregs (and that's how we detect
> incompatibilities).
> 
> I think we should at least give userspace a chance to do the right
> thing. If it doesn't, well, too bad.

This series should give userspace an option to adjust its behavior.

My main concern is that this version of the series results in the worst
kind of migration failures, where the guest simply doesn't run on the
destination side with no warnings or error messages returned to the
user..

We could add logic to return an error code if trying to write a
different revision than what the kernel has (similar to the invariant
sysregs), so that a simple fix to QEMU to save restore the GICD_IIDR
register at least results in an error being returned to userspace.

However, as QEMU doesn't do anything useful here today (not blaming
anyone, I wrote the apparently broken GIC save/restore code for QEMU
myself), we could also argue that QEMU might as well just fix things up
if it detects a different IIDR.

> 
> How bad is that "writable GICD_IIDR" patch? Because at this stage, and
> in the absence of any comment, I'm close to just pick that series and
> merge it for 4.19.
> 

My guess is that a patch to "save/restore GICD_IIDR" is simple enough,
but requires additional logic in the kernel that returns an error if the
GICD_IIDR don't match on write.

A patch which changes the groups and bumps the IIDR in userspace is
probably more complex.

Sounds like I should add the GICD_IIDR checking patch.  Thoughts?

What I would really like to know is whether this is really an issue or
not.  Do people who run products based on KVM, such as RHEV, have any
expectations about cross-kernel version migration?


Thanks,
-Christoffer

[PATCH v2 6/6] KVM: arm/arm64: vgic: Allow configuration of interrupt groups

[Marc Zyngier]

On 09/07/18 23:48, Christoffer Dall wrote:
> On Mon, Jul 09, 2018 at 09:42:40AM +0100, Marc Zyngier wrote:
>> On 04/07/18 10:38, Christoffer Dall wrote:
>>> Implement the required MMIO accessors for GICv2 and GICv3 for the
>>> IGROUPR distributor and redistributor registers.
>>>
>>> This can allow guests to change behavior compared to running on previous
>>> versions of KVM, but only to align with the architecture and hardware
>>> implementations.
>>>
>>> This also allows userspace to configure the groups for interrupts.  Note
>>> that this potentially results in GICv2 guests not receiving interrupts
>>> after migration if migrating from an older kernel that exposes GICv2
>>> interrupts as group 1.
>>>
>>> Cc: Andrew Jones <drjones@redhat.com>
>>> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
>>> ---
>>> I implemented (but stashed) a version of this which predicated the
>>> behavior based on the value of GICD_IIDR revision field, falling back to
>>> ignoring writes and resetting GICv2 groups to 0 if the guest wrote a
>>> revision less than 2.  However, current QEMU implementations simply
>>> don't write the GICD_IIDR, so this doesn't help at all without changing
>>> QEMU anyhow.
>>>
>>> The only actual fix I can see here to work around the problem in the
>>> kernel is to require an opt-in to allow restoring groups from userspace,
>>> but that's a lot of logic to support cross-kernel version migration.
>>>
>>> Question: Do we expect that cross-kernel version migration is a critical
>>> feature that people really expect to work, and do we actually have
>>> examples of catering to this in the kernel elsewhere?  (Also, how would
>>> then that relate to the whole 'adding a new sysreg breaks migration'
>>> situation?)
>>
>> I don't really get why QEMU doesn't try to restore GICD_IIDR, while it
>> is definitely trying to restore RO sysregs (and that's how we detect
>> incompatibilities).
>>
>> I think we should at least give userspace a chance to do the right
>> thing. If it doesn't, well, too bad.
> 
> This series should give userspace an option to adjust its behavior.
> 
> My main concern is that this version of the series results in the worst
> kind of migration failures, where the guest simply doesn't run on the
> destination side with no warnings or error messages returned to the
> user..

I agree, which is why I'm suggesting we make IIDR writeable. And yes, it
still requires userspace to be fixed to actually write IIDR.

> 
> We could add logic to return an error code if trying to write a
> different revision than what the kernel has (similar to the invariant
> sysregs), so that a simple fix to QEMU to save restore the GICD_IIDR
> register at least results in an error being returned to userspace.
> 
> However, as QEMU doesn't do anything useful here today (not blaming
> anyone, I wrote the apparently broken GIC save/restore code for QEMU
> myself), we could also argue that QEMU might as well just fix things up
> if it detects a different IIDR.
> 
>>
>> How bad is that "writable GICD_IIDR" patch? Because at this stage, and
>> in the absence of any comment, I'm close to just pick that series and
>> merge it for 4.19.
>>
> 
> My guess is that a patch to "save/restore GICD_IIDR" is simple enough,
> but requires additional logic in the kernel that returns an error if the
> GICD_IIDR don't match on write.
> 
> A patch which changes the groups and bumps the IIDR in userspace is
> probably more complex.
> 
> Sounds like I should add the GICD_IIDR checking patch.  Thoughts?

I'm quite keen on that. It makes the userspace change trivial, aligns it
somehow on the sysreg and ITS ABI behaviours.

> What I would really like to know is whether this is really an issue or
> not.  Do people who run products based on KVM, such as RHEV, have any
> expectations about cross-kernel version migration?

I'd like to know as well.

Thanks,

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

[PATCH v2 6/6] KVM: arm/arm64: vgic: Allow configuration of interrupt groups

[Auger Eric]

Hi Christoffer, Marc,

On 07/10/2018 10:33 AM, Marc Zyngier wrote:
> On 09/07/18 23:48, Christoffer Dall wrote:
>> On Mon, Jul 09, 2018 at 09:42:40AM +0100, Marc Zyngier wrote:
>>> On 04/07/18 10:38, Christoffer Dall wrote:
>>>> Implement the required MMIO accessors for GICv2 and GICv3 for the
>>>> IGROUPR distributor and redistributor registers.
>>>>
>>>> This can allow guests to change behavior compared to running on previous
>>>> versions of KVM, but only to align with the architecture and hardware
>>>> implementations.
>>>>
>>>> This also allows userspace to configure the groups for interrupts.  Note
>>>> that this potentially results in GICv2 guests not receiving interrupts
>>>> after migration if migrating from an older kernel that exposes GICv2
>>>> interrupts as group 1.
>>>>
>>>> Cc: Andrew Jones <drjones@redhat.com>
>>>> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
>>>> ---
>>>> I implemented (but stashed) a version of this which predicated the
>>>> behavior based on the value of GICD_IIDR revision field, falling back to
>>>> ignoring writes and resetting GICv2 groups to 0 if the guest wrote a
>>>> revision less than 2.  However, current QEMU implementations simply
>>>> don't write the GICD_IIDR, so this doesn't help at all without changing
>>>> QEMU anyhow.
>>>>
>>>> The only actual fix I can see here to work around the problem in the
>>>> kernel is to require an opt-in to allow restoring groups from userspace,
>>>> but that's a lot of logic to support cross-kernel version migration.
>>>>
>>>> Question: Do we expect that cross-kernel version migration is a critical
>>>> feature that people really expect to work, and do we actually have
>>>> examples of catering to this in the kernel elsewhere?  (Also, how would
>>>> then that relate to the whole 'adding a new sysreg breaks migration'
>>>> situation?)
>>>
>>> I don't really get why QEMU doesn't try to restore GICD_IIDR, while it
>>> is definitely trying to restore RO sysregs (and that's how we detect
>>> incompatibilities).
>>>
>>> I think we should at least give userspace a chance to do the right
>>> thing. If it doesn't, well, too bad.
>>
>> This series should give userspace an option to adjust its behavior.
>>
>> My main concern is that this version of the series results in the worst
>> kind of migration failures, where the guest simply doesn't run on the
>> destination side with no warnings or error messages returned to the
>> user..
> 
> I agree, which is why I'm suggesting we make IIDR writeable. And yes, it
> still requires userspace to be fixed to actually write IIDR.
> 
>>
>> We could add logic to return an error code if trying to write a
>> different revision than what the kernel has (similar to the invariant
>> sysregs), so that a simple fix to QEMU to save restore the GICD_IIDR
>> register at least results in an error being returned to userspace.
>>
>> However, as QEMU doesn't do anything useful here today (not blaming
>> anyone, I wrote the apparently broken GIC save/restore code for QEMU
>> myself), we could also argue that QEMU might as well just fix things up
>> if it detects a different IIDR.
>>
>>>
>>> How bad is that "writable GICD_IIDR" patch? Because at this stage, and
>>> in the absence of any comment, I'm close to just pick that series and
>>> merge it for 4.19.
>>>
>>
>> My guess is that a patch to "save/restore GICD_IIDR" is simple enough,
>> but requires additional logic in the kernel that returns an error if the
>> GICD_IIDR don't match on write.
>>
>> A patch which changes the groups and bumps the IIDR in userspace is
>> probably more complex.
>>
>> Sounds like I should add the GICD_IIDR checking patch.  Thoughts?
> 
> I'm quite keen on that. It makes the userspace change trivial, aligns it
> somehow on the sysreg and ITS ABI behaviours.
> 
>> What I would really like to know is whether this is really an issue or
>> not.  Do people who run products based on KVM, such as RHEV, have any
>> expectations about cross-kernel version migration?
> 
> I'd like to know as well.

Sorry I did not have time to review the series properly. My
understanding is we generally care about migration between different
kernels. For the ITS, we made the IIDR writable too to manage the ABI
version properly. See ab01c6bdacc43c41c6b326889f4358f5afc38bf9. But
maybe I missed the crux of the matter.

Thanks

Eric
> 
> Thanks,
> 
> 	M.
> 

[PATCH v2 6/6] KVM: arm/arm64: vgic: Allow configuration of interrupt groups

[Peter Maydell]

On 10 July 2018 at 10:12, Auger Eric <eric.auger@redhat.com> wrote:
> Hi Christoffer, Marc,
>
> On 07/10/2018 10:33 AM, Marc Zyngier wrote:
>> On 09/07/18 23:48, Christoffer Dall wrote:
>>> What I would really like to know is whether this is really an issue or
>>> not.  Do people who run products based on KVM, such as RHEV, have any
>>> expectations about cross-kernel version migration?
>>
>> I'd like to know as well.
>
> Sorry I did not have time to review the series properly. My
> understanding is we generally care about migration between different
> kernels. For the ITS, we made the IIDR writable too to manage the ABI
> version properly. See ab01c6bdacc43c41c6b326889f4358f5afc38bf9. But
> maybe I missed the crux of the matter.

I think the question is whether we're doing this because
we think hypothetical users care, or because we know actual
real in-production users really care...

thanks
-- PMM

[PATCH v2 6/6] KVM: arm/arm64: vgic: Allow configuration of interrupt groups

[Auger Eric]

Hi,

On 07/10/2018 12:48 AM, Christoffer Dall wrote:
> On Mon, Jul 09, 2018 at 09:42:40AM +0100, Marc Zyngier wrote:
>> On 04/07/18 10:38, Christoffer Dall wrote:
>>> Implement the required MMIO accessors for GICv2 and GICv3 for the
>>> IGROUPR distributor and redistributor registers.
>>>
>>> This can allow guests to change behavior compared to running on previous
>>> versions of KVM, but only to align with the architecture and hardware
>>> implementations.
>>>
>>> This also allows userspace to configure the groups for interrupts.  Note
>>> that this potentially results in GICv2 guests not receiving interrupts
>>> after migration if migrating from an older kernel that exposes GICv2
>>> interrupts as group 1.
>>>
>>> Cc: Andrew Jones <drjones@redhat.com>
>>> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
>>> ---
>>> I implemented (but stashed) a version of this which predicated the
>>> behavior based on the value of GICD_IIDR revision field, falling back to
>>> ignoring writes and resetting GICv2 groups to 0 if the guest wrote a
>>> revision less than 2.  However, current QEMU implementations simply
>>> don't write the GICD_IIDR, so this doesn't help at all without changing
>>> QEMU anyhow.
>>>
>>> The only actual fix I can see here to work around the problem in the
>>> kernel is to require an opt-in to allow restoring groups from userspace,
>>> but that's a lot of logic to support cross-kernel version migration.
>>>
>>> Question: Do we expect that cross-kernel version migration is a critical
>>> feature that people really expect to work, and do we actually have
>>> examples of catering to this in the kernel elsewhere?  (Also, how would
>>> then that relate to the whole 'adding a new sysreg breaks migration'
>>> situation?)
>>
>> I don't really get why QEMU doesn't try to restore GICD_IIDR, while it
>> is definitely trying to restore RO sysregs (and that's how we detect
>> incompatibilities).
>>
>> I think we should at least give userspace a chance to do the right
>> thing. If it doesn't, well, too bad.
> 
> This series should give userspace an option to adjust its behavior.
> 
> My main concern is that this version of the series results in the worst
> kind of migration failures, where the guest simply doesn't run on the
> destination side with no warnings or error messages returned to the
> user..

Adding Dave in the loop to comment about general user perspective.
> 
> We could add logic to return an error code if trying to write a
> different revision than what the kernel has (similar to the invariant
> sysregs), so that a simple fix to QEMU to save restore the GICD_IIDR
> register at least results in an error being returned to userspace.
> 
> However, as QEMU doesn't do anything useful here today (not blaming
> anyone, I wrote the apparently broken GIC save/restore code for QEMU
> myself), we could also argue that QEMU might as well just fix things up
> if it detects a different IIDR.
> 
>>
>> How bad is that "writable GICD_IIDR" patch? Because at this stage, and
>> in the absence of any comment, I'm close to just pick that series and
>> merge it for 4.19.
>>
> 
> My guess is that a patch to "save/restore GICD_IIDR" is simple enough,
> but requires additional logic in the kernel that returns an error if the
> GICD_IIDR don't match on write.
> 
> A patch which changes the groups and bumps the IIDR in userspace is
> probably more complex.
> 
> Sounds like I should add the GICD_IIDR checking patch.  Thoughts?
> 
> What I would really like to know is whether this is really an issue or
> not.  Do people who run products based on KVM, such as RHEV, have any
> expectations about cross-kernel version migration?

Thanks

Eric
> 
> 
> Thanks,
> -Christoffer
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[PATCH v2 6/6] KVM: arm/arm64: vgic: Allow configuration of interrupt groups

[Dr. David Alan Gilbert]

* Auger Eric (eric.auger at redhat.com) wrote:
> Hi,
> 
> On 07/10/2018 12:48 AM, Christoffer Dall wrote:
> > On Mon, Jul 09, 2018 at 09:42:40AM +0100, Marc Zyngier wrote:
> >> On 04/07/18 10:38, Christoffer Dall wrote:
> >>> Implement the required MMIO accessors for GICv2 and GICv3 for the
> >>> IGROUPR distributor and redistributor registers.
> >>>
> >>> This can allow guests to change behavior compared to running on previous
> >>> versions of KVM, but only to align with the architecture and hardware
> >>> implementations.
> >>>
> >>> This also allows userspace to configure the groups for interrupts.  Note
> >>> that this potentially results in GICv2 guests not receiving interrupts
> >>> after migration if migrating from an older kernel that exposes GICv2
> >>> interrupts as group 1.
> >>>
> >>> Cc: Andrew Jones <drjones@redhat.com>
> >>> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> >>> ---
> >>> I implemented (but stashed) a version of this which predicated the
> >>> behavior based on the value of GICD_IIDR revision field, falling back to
> >>> ignoring writes and resetting GICv2 groups to 0 if the guest wrote a
> >>> revision less than 2.  However, current QEMU implementations simply
> >>> don't write the GICD_IIDR, so this doesn't help at all without changing
> >>> QEMU anyhow.
> >>>
> >>> The only actual fix I can see here to work around the problem in the
> >>> kernel is to require an opt-in to allow restoring groups from userspace,
> >>> but that's a lot of logic to support cross-kernel version migration.
> >>>
> >>> Question: Do we expect that cross-kernel version migration is a critical
> >>> feature that people really expect to work, and do we actually have
> >>> examples of catering to this in the kernel elsewhere?  (Also, how would
> >>> then that relate to the whole 'adding a new sysreg breaks migration'
> >>> situation?)
> >>
> >> I don't really get why QEMU doesn't try to restore GICD_IIDR, while it
> >> is definitely trying to restore RO sysregs (and that's how we detect
> >> incompatibilities).
> >>
> >> I think we should at least give userspace a chance to do the right
> >> thing. If it doesn't, well, too bad.
> > 
> > This series should give userspace an option to adjust its behavior.
> > 
> > My main concern is that this version of the series results in the worst
> > kind of migration failures, where the guest simply doesn't run on the
> > destination side with no warnings or error messages returned to the
> > user..
> 
> Adding Dave in the loop to comment about general user perspective.

Without understanding the details of the GIC, but I have to agree that
a failure without error where the guest is hung is one of the worst
cases - when a user comes to you saying that their VM hangs after
migration with no diagnostics, then you know you're in for some nasty
debug!

On other architectures we definitely provide this level of compatibility
between kernels, libraries, qemu and everything in between - it's not
easy, and we do screwup from time to time; but it's still what we try
and get right.

So ideally you'd make this switchable and wire it into a versioned
machine type in QEMU, so that only virt-3.0 VMs would use this (and
they'd somehow know they needed the new kernel to do it).

If that's not possible then you could add a subsection to the GIC migration
data if you can detect at migration time that this feature is being
used, and make the destination check for the feature/kernel.
Migrating to an older qemu would fail since it wouldn't know about the
new subsection.  This should at least get a clear failure.

For a user, this still gets messy if they do something like start
upgrading some of the hosts in an openstack cluster, which they often
do incrementally; so you'll suddenly get the situation of a VM that
was started on a host with a newer kernel being migrated to an older one
and stuff breaks.

Dave

> > 
> > We could add logic to return an error code if trying to write a
> > different revision than what the kernel has (similar to the invariant
> > sysregs), so that a simple fix to QEMU to save restore the GICD_IIDR
> > register at least results in an error being returned to userspace.
> > 
> > However, as QEMU doesn't do anything useful here today (not blaming
> > anyone, I wrote the apparently broken GIC save/restore code for QEMU
> > myself), we could also argue that QEMU might as well just fix things up
> > if it detects a different IIDR.
> > 
> >>
> >> How bad is that "writable GICD_IIDR" patch? Because at this stage, and
> >> in the absence of any comment, I'm close to just pick that series and
> >> merge it for 4.19.
> >>
> > 
> > My guess is that a patch to "save/restore GICD_IIDR" is simple enough,
> > but requires additional logic in the kernel that returns an error if the
> > GICD_IIDR don't match on write.
> > 
> > A patch which changes the groups and bumps the IIDR in userspace is
> > probably more complex.
> > 
> > Sounds like I should add the GICD_IIDR checking patch.  Thoughts?
> > 
> > What I would really like to know is whether this is really an issue or
> > not.  Do people who run products based on KVM, such as RHEV, have any
> > expectations about cross-kernel version migration?
> 
> Thanks
> 
> Eric
> > 
> > 
> > Thanks,
> > -Christoffer
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > 
--
Dr. David Alan Gilbert / dgilbert at redhat.com / Manchester, UK

[PATCH v2 6/6] KVM: arm/arm64: vgic: Allow configuration of interrupt groups

[Christoffer Dall]

On Tue, Jul 10, 2018 at 11:32:24AM +0100, Dr. David Alan Gilbert wrote:
> * Auger Eric (eric.auger at redhat.com) wrote:
> > Hi,
> > 
> > On 07/10/2018 12:48 AM, Christoffer Dall wrote:
> > > On Mon, Jul 09, 2018 at 09:42:40AM +0100, Marc Zyngier wrote:
> > >> On 04/07/18 10:38, Christoffer Dall wrote:
> > >>> Implement the required MMIO accessors for GICv2 and GICv3 for the
> > >>> IGROUPR distributor and redistributor registers.
> > >>>
> > >>> This can allow guests to change behavior compared to running on previous
> > >>> versions of KVM, but only to align with the architecture and hardware
> > >>> implementations.
> > >>>
> > >>> This also allows userspace to configure the groups for interrupts.  Note
> > >>> that this potentially results in GICv2 guests not receiving interrupts
> > >>> after migration if migrating from an older kernel that exposes GICv2
> > >>> interrupts as group 1.
> > >>>
> > >>> Cc: Andrew Jones <drjones@redhat.com>
> > >>> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> > >>> ---
> > >>> I implemented (but stashed) a version of this which predicated the
> > >>> behavior based on the value of GICD_IIDR revision field, falling back to
> > >>> ignoring writes and resetting GICv2 groups to 0 if the guest wrote a
> > >>> revision less than 2.  However, current QEMU implementations simply
> > >>> don't write the GICD_IIDR, so this doesn't help at all without changing
> > >>> QEMU anyhow.
> > >>>
> > >>> The only actual fix I can see here to work around the problem in the
> > >>> kernel is to require an opt-in to allow restoring groups from userspace,
> > >>> but that's a lot of logic to support cross-kernel version migration.
> > >>>
> > >>> Question: Do we expect that cross-kernel version migration is a critical
> > >>> feature that people really expect to work, and do we actually have
> > >>> examples of catering to this in the kernel elsewhere?  (Also, how would
> > >>> then that relate to the whole 'adding a new sysreg breaks migration'
> > >>> situation?)
> > >>
> > >> I don't really get why QEMU doesn't try to restore GICD_IIDR, while it
> > >> is definitely trying to restore RO sysregs (and that's how we detect
> > >> incompatibilities).
> > >>
> > >> I think we should at least give userspace a chance to do the right
> > >> thing. If it doesn't, well, too bad.
> > > 
> > > This series should give userspace an option to adjust its behavior.
> > > 
> > > My main concern is that this version of the series results in the worst
> > > kind of migration failures, where the guest simply doesn't run on the
> > > destination side with no warnings or error messages returned to the
> > > user..
> > 
> > Adding Dave in the loop to comment about general user perspective.
> 
> Without understanding the details of the GIC, but I have to agree that
> a failure without error where the guest is hung is one of the worst
> cases - when a user comes to you saying that their VM hangs after
> migration with no diagnostics, then you know you're in for some nasty
> debug!
> 
> On other architectures we definitely provide this level of compatibility
> between kernels, libraries, qemu and everything in between - it's not
> easy, and we do screwup from time to time; but it's still what we try
> and get right.

That's good to know.

> 
> So ideally you'd make this switchable and wire it into a versioned
> machine type in QEMU, so that only virt-3.0 VMs would use this (and
> they'd somehow know they needed the new kernel to do it).
> 
> If that's not possible then you could add a subsection to the GIC migration
> data if you can detect at migration time that this feature is being
> used, and make the destination check for the feature/kernel.
> Migrating to an older qemu would fail since it wouldn't know about the
> new subsection.  This should at least get a clear failure.
> 
> For a user, this still gets messy if they do something like start
> upgrading some of the hosts in an openstack cluster, which they often
> do incrementally; so you'll suddenly get the situation of a VM that
> was started on a host with a newer kernel being migrated to an older one
> and stuff breaks.
> 

I think we should ask userspace to opt-in to the new behavior and fix
userspace to save/restore the IIDR while we're at it.

Unless someone objects, I'll try to come up with a v3 that asks
userspace to confirm it wants writable groups.  Ideally I'd like for
this to happen automatically if userspace writes an IIDR with revision 2
and above, but that may result in either

  (1) imposing ordering on the restore sequence from userspace;
      userspace must write IIDR before IGROUPR if it wants non-default
      groups,
  (2) terrible sequence of locking and resetting everything if IIDR
      hasn't been written before time of first executing a VCPU, or
  (3) an additional bookkeeping flag in the critical path for GICv2
      which ignores the group unless userspace wrote IIDR.

Out of the three, I think (3) is the least desirable because it
precludes the guest from programming its own groups.  I'll have a look
at how (2) looks, because it hides everything, and finally we can fall
back to (1) and document it clearly.

Thanks,
Christoffer

[PATCH v2 6/6] KVM: arm/arm64: vgic: Allow configuration of interrupt groups

[Peter Maydell]

On 10 July 2018 at 16:27, Christoffer Dall <christoffer.dall@arm.com> wrote:
> Unless someone objects, I'll try to come up with a v3 that asks
> userspace to confirm it wants writable groups.  Ideally I'd like for
> this to happen automatically if userspace writes an IIDR with revision 2
> and above, but that may result in either
>
>   (1) imposing ordering on the restore sequence from userspace;
>       userspace must write IIDR before IGROUPR if it wants non-default
>       groups,
>   (2) terrible sequence of locking and resetting everything if IIDR
>       hasn't been written before time of first executing a VCPU, or
>   (3) an additional bookkeeping flag in the critical path for GICv2
>       which ignores the group unless userspace wrote IIDR.
>
> Out of the three, I think (3) is the least desirable because it
> precludes the guest from programming its own groups.  I'll have a look
> at how (2) looks, because it hides everything, and finally we can fall
> back to (1) and document it clearly.

There are already some ordering restrictions on the restore
sequence for the GICv3, I think (eg there's a comment in QEMU
about having to restore GICR_PROPBASER/PENDBASER before GICR_CTLR),
so (1) isn't the end of the world...

thanks
-- PMM

[PATCH v2 6/6] KVM: arm/arm64: vgic: Allow configuration of interrupt groups

[Marc Zyngier]

On 10/07/18 16:27, Christoffer Dall wrote:
> On Tue, Jul 10, 2018 at 11:32:24AM +0100, Dr. David Alan Gilbert wrote:
>> * Auger Eric (eric.auger at redhat.com) wrote:
>>> Hi,
>>>
>>> On 07/10/2018 12:48 AM, Christoffer Dall wrote:
>>>> On Mon, Jul 09, 2018 at 09:42:40AM +0100, Marc Zyngier wrote:
>>>>> On 04/07/18 10:38, Christoffer Dall wrote:
>>>>>> Implement the required MMIO accessors for GICv2 and GICv3 for the
>>>>>> IGROUPR distributor and redistributor registers.
>>>>>>
>>>>>> This can allow guests to change behavior compared to running on previous
>>>>>> versions of KVM, but only to align with the architecture and hardware
>>>>>> implementations.
>>>>>>
>>>>>> This also allows userspace to configure the groups for interrupts.  Note
>>>>>> that this potentially results in GICv2 guests not receiving interrupts
>>>>>> after migration if migrating from an older kernel that exposes GICv2
>>>>>> interrupts as group 1.
>>>>>>
>>>>>> Cc: Andrew Jones <drjones@redhat.com>
>>>>>> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
>>>>>> ---
>>>>>> I implemented (but stashed) a version of this which predicated the
>>>>>> behavior based on the value of GICD_IIDR revision field, falling back to
>>>>>> ignoring writes and resetting GICv2 groups to 0 if the guest wrote a
>>>>>> revision less than 2.  However, current QEMU implementations simply
>>>>>> don't write the GICD_IIDR, so this doesn't help at all without changing
>>>>>> QEMU anyhow.
>>>>>>
>>>>>> The only actual fix I can see here to work around the problem in the
>>>>>> kernel is to require an opt-in to allow restoring groups from userspace,
>>>>>> but that's a lot of logic to support cross-kernel version migration.
>>>>>>
>>>>>> Question: Do we expect that cross-kernel version migration is a critical
>>>>>> feature that people really expect to work, and do we actually have
>>>>>> examples of catering to this in the kernel elsewhere?  (Also, how would
>>>>>> then that relate to the whole 'adding a new sysreg breaks migration'
>>>>>> situation?)
>>>>>
>>>>> I don't really get why QEMU doesn't try to restore GICD_IIDR, while it
>>>>> is definitely trying to restore RO sysregs (and that's how we detect
>>>>> incompatibilities).
>>>>>
>>>>> I think we should at least give userspace a chance to do the right
>>>>> thing. If it doesn't, well, too bad.
>>>>
>>>> This series should give userspace an option to adjust its behavior.
>>>>
>>>> My main concern is that this version of the series results in the worst
>>>> kind of migration failures, where the guest simply doesn't run on the
>>>> destination side with no warnings or error messages returned to the
>>>> user..
>>>
>>> Adding Dave in the loop to comment about general user perspective.
>>
>> Without understanding the details of the GIC, but I have to agree that
>> a failure without error where the guest is hung is one of the worst
>> cases - when a user comes to you saying that their VM hangs after
>> migration with no diagnostics, then you know you're in for some nasty
>> debug!
>>
>> On other architectures we definitely provide this level of compatibility
>> between kernels, libraries, qemu and everything in between - it's not
>> easy, and we do screwup from time to time; but it's still what we try
>> and get right.
> 
> That's good to know.
> 
>>
>> So ideally you'd make this switchable and wire it into a versioned
>> machine type in QEMU, so that only virt-3.0 VMs would use this (and
>> they'd somehow know they needed the new kernel to do it).
>>
>> If that's not possible then you could add a subsection to the GIC migration
>> data if you can detect at migration time that this feature is being
>> used, and make the destination check for the feature/kernel.
>> Migrating to an older qemu would fail since it wouldn't know about the
>> new subsection.  This should at least get a clear failure.
>>
>> For a user, this still gets messy if they do something like start
>> upgrading some of the hosts in an openstack cluster, which they often
>> do incrementally; so you'll suddenly get the situation of a VM that
>> was started on a host with a newer kernel being migrated to an older one
>> and stuff breaks.
>>
> 
> I think we should ask userspace to opt-in to the new behavior and fix
> userspace to save/restore the IIDR while we're at it.
> 
> Unless someone objects, I'll try to come up with a v3 that asks
> userspace to confirm it wants writable groups.  Ideally I'd like for
> this to happen automatically if userspace writes an IIDR with revision 2
> and above, but that may result in either
> 
>   (1) imposing ordering on the restore sequence from userspace;
>       userspace must write IIDR before IGROUPR if it wants non-default
>       groups,
>   (2) terrible sequence of locking and resetting everything if IIDR
>       hasn't been written before time of first executing a VCPU, or
>   (3) an additional bookkeeping flag in the critical path for GICv2
>       which ignores the group unless userspace wrote IIDR.
> 
> Out of the three, I think (3) is the least desirable because it
> precludes the guest from programming its own groups.  I'll have a look
> at how (2) looks, because it hides everything, and finally we can fall
> back to (1) and document it clearly.

I don't see any issue with (1). Userspace just has to make sure that
IIDR is the first thing that gets written, like we have for the ITS
(where the IIDR must be written before restoring the tables).

(2) and (3) are really overkill, IMHO.

Thanks,

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