[PATCH 00/10] xen/arm: Bug fixes for the vGIC

[PATCH 00/10] xen/arm: Bug fixes for the vGIC

[Julien Grall]

Hello,

The first goal of this series is to fix Linux 3.19 DOM0 booting on GICv3 systems
(see patch #1).

I also took the opportunity to review vGIC drivers and found several issues.
While I believe everything should be ok for vGICv2, there is still some
pending bugs in vGICv3 that will require some rework.

Most of them concern the way we handle redistributor. Unlike the distributor,
the MMIO region of the redistributor are not banked and should be accessible
by anyone.

This should be fixed for Xen 4.6, but I may not have time to work on it. I
would be happy if someone wants to fix it.

Sincerely yours,

Julien Grall (10):
  xen/arm: vgic-v3: Correctly set GICD_TYPER.IDbits
  xen/arm: vgic-v3: Correctly set GICD_TYPER.CPUNumber
  xen/arm: vgic-v3: Correctly handle GICD_CTLR
  xen/arm: vgic-v3: Don't check the size when we ignore the write/read
    as zero
  xen/arm: vgic-v3: Document the current restrictions
  xen/arm: vgic-v3: Print the domain/vcpu in each message
  xen/arm: vgic-v2: Correctly set GICD_TYPER.CPUNumber
  xen/arm: vgic-v2: Don't check the size when we ignore the write/read a
    zero
  xen/arm: vgic-v2: Take the lock when writing into GICD_CTLR
  xen/arm: vgic-v2: Print the domain/vcpu in each message

 xen/arch/arm/vgic-v2.c            |  41 ++++---
 xen/arch/arm/vgic-v3.c            | 227 +++++++++++++++++++++-----------------
 xen/include/asm-arm/gic.h         |   1 +
 xen/include/asm-arm/gic_v3_defs.h |   4 +
 4 files changed, 158 insertions(+), 115 deletions(-)

-- 
2.1.4

[PATCH 01/10] xen/arm: vgic-v3: Correctly set GICD_TYPER.IDbits

[Julien Grall]

>From Linux 3.19, the GICv3 drivers is using GICD_TYPER.IDbits to check
the validity of the hardware interrupt number.

The field IDBits in the register GICD_TYPER is used to know the number of
interrupt identifiers (SPI, PPIs, SGIs, LPIs) supported by GIC Stream Protocol
Interface.

This field contains the number of interrupt identifier bits minus one.

Signed-off-by: Julien Grall <julien.grall@linaro.org>

---
    This patch should be backported to Xen 4.5. Without it any Linux
    kernel > 3.19 won't boot as a Xen domain.

    I'm wondering if we should add a release note for this bug.
---
 xen/arch/arm/vgic-v3.c            | 16 ++++++++++++++++
 xen/include/asm-arm/gic_v3_defs.h |  4 ++++
 2 files changed, 20 insertions(+)

diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index c738ca9..8420c09 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -671,11 +671,27 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
         vgic_unlock(v);
         return 1;
     case GICD_TYPER:
+    {
+        unsigned int irqs = v->domain->arch.vgic.nr_lines + 32;
+        unsigned int order;
+
         if ( dabt.size != DABT_WORD ) goto bad_width;
         /* No secure world support for guests. */
         *r = (((v->domain->max_vcpus << 5) & GICD_TYPE_CPUS ) |
               ((v->domain->arch.vgic.nr_lines / 32) & GICD_TYPE_LINES));
+
+        /*
+         * Calculate number of interrupt identifier bits supported by
+         * the GIC Stream Protocol Interface
+         */
+        irqs--;
+        for ( order = 0; irqs; order++ )
+            irqs >>= 1;
+
+        *r |= ((order - 1) << GICD_TYPE_ID_BITS_SHIFT) & GICD_TYPE_ID_BITS_MASK;
+
         return 1;
+    }
     case GICD_STATUSR:
         /*
          *  Optional, Not implemented for now.
diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h
index 13adb53..6214663 100644
--- a/xen/include/asm-arm/gic_v3_defs.h
+++ b/xen/include/asm-arm/gic_v3_defs.h
@@ -18,6 +18,10 @@
 #ifndef __ASM_ARM_GIC_V3_DEFS_H__
 #define __ASM_ARM_GIC_V3_DEFS_H__
 
+/* Additional bits in GICD_TYPER defined by GICv3 */
+#define GICD_TYPE_ID_BITS_SHIFT 19
+#define GICD_TYPE_ID_BITS_MASK  (0x1f << GICD_TYPE_ID_BITS_SHIFT)
+
 /*
  * Additional registers defined in GIC v3.
  * Common GICD registers are defined in gic.h
-- 
2.1.4

[PATCH 02/10] xen/arm: vgic-v3: Correctly set GICD_TYPER.CPUNumber

[Julien Grall]

On GICv3, the value (CPUNumber + 1) indicates the number of processor that may
be used as interrupts targets when ARE bit is zero. The maximum is 8
processors.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
    The current code of the vGIC doesn't support ARE = 0.
    Nonetheless, the patch is a candidate for backporing to Xen 4.5 to have a
    consistent vGIC driver.
---
 xen/arch/arm/vgic-v3.c    | 7 ++++++-
 xen/include/asm-arm/gic.h | 1 +
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 8420c09..406ea93 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -673,11 +673,16 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
     case GICD_TYPER:
     {
         unsigned int irqs = v->domain->arch.vgic.nr_lines + 32;
+        /*
+         * Number of processors that may be used as interrupt targets when ARE
+         * bit is zero. The maximum is 8.
+         */
+        unsigned int ncpus = min_t(unsigned int, v->domain->max_vcpus, 8);
         unsigned int order;
 
         if ( dabt.size != DABT_WORD ) goto bad_width;
         /* No secure world support for guests. */
-        *r = (((v->domain->max_vcpus << 5) & GICD_TYPE_CPUS ) |
+        *r = ((ncpus - 1) << GICD_TYPE_CPUS_SHIFT |
               ((v->domain->arch.vgic.nr_lines / 32) & GICD_TYPE_LINES));
 
         /*
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 187dc46..0396a8e 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -93,6 +93,7 @@
 #define GICD_CTL_ENABLE 0x1
 
 #define GICD_TYPE_LINES 0x01f
+#define GICD_TYPE_CPUS_SHIFT 5
 #define GICD_TYPE_CPUS  0x0e0
 #define GICD_TYPE_SEC   0x400
 
-- 
2.1.4

[PATCH 03/10] xen/arm: vgic-v3: Correctly handle GICD_CTLR

[Julien Grall]

As backward GICv2 compatibility is not supported in the VGICv3 driver,
the bit ARE_NS should be set at any time.

Futhermore, when ARE_NS is set, the guest can only modify EnableGrp1A.

At same time take the vgic_lock to write into domain.arch.vgic.ctrl. It
was already taken during read.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/vgic-v3.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 406ea93..1aa2f58 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -45,6 +45,12 @@
 #define GICV3_GICR_PIDR2  GICV3_GICD_PIDR2
 #define GICV3_GICR_PIDR4  GICV3_GICD_PIDR4
 
+/*
+ * GICD_CTLR default value:
+ *      - No GICv2 compatibility => ARE = 1
+ */
+#define VGICD_CTLR_DEFAULT  (GICD_CTLR_ARE_NS)
+
 static struct vcpu *vgic_v3_irouter_to_vcpu(struct vcpu *v, uint64_t irouter)
 {
     irouter &= ~(GICD_IROUTER_SPI_MODE_ANY);
@@ -834,8 +840,15 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
     {
     case GICD_CTLR:
         if ( dabt.size != DABT_WORD ) goto bad_width;
-        /* Ignore all but the enable bit */
-        v->domain->arch.vgic.ctlr = (*r) & GICD_CTL_ENABLE;
+
+        vgic_lock(v);
+        /* Only EnableGrp1A can be changed */
+        if ( *r & GICD_CTLR_ENABLE_G1A )
+            v->domain->arch.vgic.ctlr |= GICD_CTLR_ENABLE_G1A;
+        else
+            v->domain->arch.vgic.ctlr &= ~GICD_CTLR_ENABLE_G1A;
+        vgic_unlock(v);
+
         return 1;
     case GICD_TYPER:
         /* RO -- write ignored */
@@ -1092,6 +1105,8 @@ static int vgic_v3_domain_init(struct domain *d)
         register_mmio_handler(d, &vgic_rdistr_mmio_handler,
             d->arch.vgic.rbase[i], d->arch.vgic.rbase_size[i]);
 
+    d->arch.vgic.ctlr = VGICD_CTLR_DEFAULT;
+
     return 0;
 }
 
-- 
2.1.4

[PATCH 04/10] xen/arm: vgic-v3: Don't check the size when we ignore the write/read as zero

[Julien Grall]

In general, it's not necessary/important to check the size. It's better
to log it to let know the guest that its access will have no effect.

Note: On debug build it may happen to see some of these messages during
domain boot.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/vgic-v3.c | 95 +++++++++++++++++++++-----------------------------
 1 file changed, 39 insertions(+), 56 deletions(-)

diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 1aa2f58..1fa1413 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -123,22 +123,22 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info,
         goto read_as_zero;
     case GICR_SETLPIR:
         /* WO. Read as zero */
-        goto read_as_zero_64;
+        goto read_as_zero;
     case GICR_CLRLPIR:
         /* WO. Read as zero */
-        goto read_as_zero_64;
+        goto read_as_zero;
     case GICR_PROPBASER:
         /* LPI's not implemented */
-        goto read_as_zero_64;
+        goto read_as_zero;
     case GICR_PENDBASER:
         /* LPI's not implemented */
-        goto read_as_zero_64;
+        goto read_as_zero;
     case GICR_INVLPIR:
         /* WO. Read as zero */
-        goto read_as_zero_64;
+        goto read_as_zero;
     case GICR_INVALLR:
         /* WO. Read as zero */
-        goto read_as_zero_64;
+        goto read_as_zero;
         return 0;
     case GICR_SYNCR:
         if ( dabt.size != DABT_WORD ) goto bad_width;
@@ -147,10 +147,10 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info,
         return 1;
     case GICR_MOVLPIR:
         /* WO Read as zero */
-        goto read_as_zero_64;
+        goto read_as_zero;
     case GICR_MOVALLR:
         /* WO Read as zero */
-        goto read_as_zero_64;
+        goto read_as_zero;
     case GICR_PIDR0:
         if ( dabt.size != DABT_WORD ) goto bad_width;
         *r = GICV3_GICR_PIDR0;
@@ -184,13 +184,9 @@ bad_width:
     domain_crash_synchronous();
     return 0;
 
-read_as_zero_64:
-    if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width;
-    *r = 0;
-    return 1;
-
 read_as_zero:
-    if ( dabt.size != DABT_WORD ) goto bad_width;
+    gdprintk(XENLOG_DEBUG, "vGICR: read as zero width, %d r%d offset %#08x\n",
+             dabt.size, dabt.reg, gicr_reg);
     *r = 0;
     return 1;
 }
@@ -199,8 +195,6 @@ static int __vgic_v3_rdistr_rd_mmio_write(struct vcpu *v, mmio_info_t *info,
                                           uint32_t gicr_reg)
 {
     struct hsr_dabt dabt = info->dabt;
-    struct cpu_user_regs *regs = guest_cpu_user_regs();
-    register_t *r = select_user_reg(regs, dabt.reg);
 
     switch ( gicr_reg )
     {
@@ -212,7 +206,7 @@ static int __vgic_v3_rdistr_rd_mmio_write(struct vcpu *v, mmio_info_t *info,
         goto write_ignore;
     case GICR_TYPER:
         /* RO */
-        goto write_ignore_64;
+        goto write_ignore;
     case GICR_STATUSR:
         /* Not implemented */
         goto write_ignore;
@@ -221,31 +215,31 @@ static int __vgic_v3_rdistr_rd_mmio_write(struct vcpu *v, mmio_info_t *info,
         goto write_ignore;
     case GICR_SETLPIR:
         /* LPI is not implemented */
-        goto write_ignore_64;
+        goto write_ignore;
     case GICR_CLRLPIR:
         /* LPI is not implemented */
-        goto write_ignore_64;
+        goto write_ignore;
     case GICR_PROPBASER:
         /* LPI is not implemented */
-        goto write_ignore_64;
+        goto write_ignore;
     case GICR_PENDBASER:
         /* LPI is not implemented */
-        goto write_ignore_64;
+        goto write_ignore;
     case GICR_INVLPIR:
         /* LPI is not implemented */
-        goto write_ignore_64;
+        goto write_ignore;
     case GICR_INVALLR:
         /* LPI is not implemented */
-        goto write_ignore_64;
+        goto write_ignore;
     case GICR_SYNCR:
         /* RO */
         goto write_ignore;
     case GICR_MOVLPIR:
         /* LPI is not implemented */
-        goto write_ignore_64;
+        goto write_ignore;
     case GICR_MOVALLR:
         /* LPI is not implemented */
-        goto write_ignore_64;
+        goto write_ignore;
     case GICR_PIDR7... GICR_PIDR0:
         /* RO */
         goto write_ignore;
@@ -253,18 +247,9 @@ static int __vgic_v3_rdistr_rd_mmio_write(struct vcpu *v, mmio_info_t *info,
         printk("vGICR: write r%d offset %#08x\n not found", dabt.reg, gicr_reg);
         return 0;
     }
-bad_width:
-    printk("vGICR: bad write width %d r%d=%"PRIregister" offset %#08x\n",
-           dabt.size, dabt.reg, *r, gicr_reg);
-    domain_crash_synchronous();
-    return 0;
-
-write_ignore_64:
-    if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width;
-    return 1;
-
 write_ignore:
-    if ( dabt.size != DABT_WORD ) goto bad_width;
+    gdprintk(XENLOG_DEBUG, "vGICR: ignore write width %d r%d offset %#08x\n",
+             dabt.size, dabt.reg, gicr_reg);
     return 1;
 }
 
@@ -364,7 +349,9 @@ bad_width:
     return 0;
 
 read_as_zero:
-    if ( dabt.size != DABT_WORD ) goto bad_width;
+    gdprintk(XENLOG_DEBUG,
+             "vGIC{D,R}: read as zero width, %d r%d offset %#08x\n",
+             dabt.size, dabt.reg, reg);
     *r = 0;
     return 1;
 }
@@ -477,7 +464,9 @@ bad_width:
     return 0;
 
 write_ignore:
-    if ( dabt.size != DABT_WORD ) goto bad_width;
+    gdprintk(XENLOG_DEBUG,
+             "vGIC{D,R}: ignore write width %d r%d offset %#08x\n",
+             dabt.size, dabt.reg, reg);
     return 1;
 }
 
@@ -538,7 +527,9 @@ bad_width:
     return 0;
 
 read_as_zero:
-    if ( dabt.size != DABT_WORD ) goto bad_width;
+    gdprintk(XENLOG_DEBUG,
+             "vGICR: SGI: read as zero width, %d r%d offset %#08x\n",
+             dabt.size, dabt.reg, gicr_reg);
     *r = 0;
     return 1;
 }
@@ -603,7 +594,8 @@ bad_width:
     return 0;
 
 write_ignore:
-    if ( dabt.size != DABT_WORD ) goto bad_width;
+    gdprintk(XENLOG_DEBUG, "vGICR: SGI: ignore write width %d r%d offset %#08x\n",
+             dabt.size, dabt.reg, gicr_reg);
     return 1;
 }
 
@@ -732,7 +724,7 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
         return __vgic_v3_distr_common_mmio_read(v, info, gicd_reg);
     case GICD_IROUTER ... GICD_IROUTER31:
         /* SGI/PPI is RES0 */
-        goto read_as_zero_64;
+        goto read_as_zero;
     case GICD_IROUTER32 ... GICD_IROUTERN:
         if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width;
         rank = vgic_rank_offset(v, 64, gicd_reg - GICD_IROUTER,
@@ -797,8 +789,6 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
     case 0xf30 ... 0x5fcc:
     case 0x8000 ... 0xbfcc:
         /* These are reserved register addresses */
-        printk("vGICv3: vGICD: read unknown 0x00c .. 0xfcc r%d offset %#08x\n",
-               dabt.reg, gicd_reg);
         goto read_as_zero;
     default:
         printk("vGICv3: vGICD: unhandled read r%d offset %#08x\n",
@@ -812,13 +802,9 @@ bad_width:
     domain_crash_synchronous();
     return 0;
 
-read_as_zero_64:
-    if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width;
-    *r = 0;
-    return 1;
-
 read_as_zero:
-    if ( dabt.size != DABT_WORD ) goto bad_width;
+    gdprintk(XENLOG_DEBUG, "vGICD: read as zero width, %d r%d offset %#08x\n",
+             dabt.size, dabt.reg, gicd_reg);
     *r = 0;
     return 1;
 }
@@ -891,12 +877,12 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
         return __vgic_v3_distr_common_mmio_write(v, info, gicd_reg);
     case GICD_IROUTER ... GICD_IROUTER31:
         /* SGI/PPI is RES0 */
-        goto write_ignore_64;
+        goto write_ignore;
     case GICD_IROUTER32 ... GICD_IROUTERN:
         if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width;
         rank = vgic_rank_offset(v, 64, gicd_reg - GICD_IROUTER,
                                 DABT_DOUBLE_WORD);
-        if ( rank == NULL ) goto write_ignore_64;
+        if ( rank == NULL ) goto write_ignore;
         BUG_ON(v->domain->max_vcpus > 8);
         new_irouter = *r;
         vgic_lock_rank(v, rank, flags);
@@ -977,11 +963,8 @@ bad_width:
     return 0;
 
 write_ignore:
-    if ( dabt.size != DABT_WORD ) goto bad_width;
-    return 1;
-
-write_ignore_64:
-    if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width;
+    gdprintk(XENLOG_DEBUG, "vGICD: ignore write width %d r%d offset %#08x\n",
+             dabt.size, dabt.reg, gicd_reg);
     return 1;
 }
 
-- 
2.1.4

[PATCH 05/10] xen/arm: vgic-v3: Document the current restrictions

[Julien Grall]

The current vGIC v3 driver doesn't fully implement GICv3 spec:
    - GICv3 backward compatibility is not supported (GICD_CTLR.ARE = 0)
    - A processor can only access his own redistributor. For buggy
    assumption, the current code bank the redistributors MMIO.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/vgic-v3.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 1fa1413..9818a6b 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -16,6 +16,11 @@
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
+ *
+ * Current limitation of the vGIC v3:
+ *      - GICv2 backward compatibility is not supported (GICD_CTRL.ARE = 0)
+ *      - A processor can only access his own redistributor. For buggy
+ *      assumption, the current code bank the redistributors MMIO
  */
 
 #include <xen/bitops.h>
-- 
2.1.4

[PATCH 06/10] xen/arm: vgic-v3: Print the domain/vcpu in each message

[Julien Grall]

Also remove vGICv3 in the message log as gdprintk already print the name
of the file.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/vgic-v3.c | 85 ++++++++++++++++++++++++++------------------------
 1 file changed, 45 insertions(+), 40 deletions(-)

diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 9818a6b..704b774 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -179,13 +179,13 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info,
         /* Reserved0 */
         goto read_as_zero;
     default:
-        printk("vGICv3: vGICR: read r%d offset %#08x\n not found",
-               dabt.reg, gicr_reg);
+        gdprintk(XENLOG_ERR, "vGICR: read r%d offset %#08x\n not found",
+                 dabt.reg, gicr_reg);
         return 0;
     }
 bad_width:
-    printk("vGICv3: vGICR: bad read width %d r%d offset %#08x\n",
-           dabt.size, dabt.reg, gicr_reg);
+    gdprintk(XENLOG_ERR, "vGICR: bad read width %d r%d offset %#08x\n",
+             dabt.size, dabt.reg, gicr_reg);
     domain_crash_synchronous();
     return 0;
 
@@ -249,7 +249,8 @@ static int __vgic_v3_rdistr_rd_mmio_write(struct vcpu *v, mmio_info_t *info,
         /* RO */
         goto write_ignore;
     default:
-        printk("vGICR: write r%d offset %#08x\n not found", dabt.reg, gicr_reg);
+        gdprintk(XENLOG_ERR, "vGICR: write r%d offset %#08x\n not found",
+                 dabt.reg, gicr_reg);
         return 0;
     }
 write_ignore:
@@ -341,15 +342,14 @@ static int __vgic_v3_distr_common_mmio_read(struct vcpu *v, mmio_info_t *info,
         vgic_unlock_rank(v, rank, flags);
         return 1;
     default:
-        printk("vGICv3: vGICD/vGICR: unhandled read r%d offset %#08x\n",
-               dabt.reg, reg);
+        gdprintk(XENLOG_ERR, "vGIC{D,R}: unhandled read r%d offset %#08x\n",
+                 dabt.reg, reg);
         return 0;
     }
 
 bad_width:
-    dprintk(XENLOG_ERR,
-            "vGICv3: vGICD/vGICR: bad read width %d r%d offset %#08x\n",
-            dabt.size, dabt.reg, reg);
+    gdprintk(XENLOG_ERR, "vGIC{D,R}: bad read width %d r%d offset %#08x\n",
+             dabt.size, dabt.reg, reg);
     domain_crash_synchronous();
     return 0;
 
@@ -456,15 +456,16 @@ static int __vgic_v3_distr_common_mmio_write(struct vcpu *v, mmio_info_t *info,
         vgic_unlock_rank(v, rank, flags);
         return 1;
     default:
-        printk("vGICv3: vGICD/vGICR: unhandled write r%d "
-               "=%"PRIregister" offset %#08x\n", dabt.reg, *r, reg);
+        gdprintk(XENLOG_ERR,
+                 "vGIC{D,R}: unhandled write r%d=%"PRIregister" offset %#08x\n",
+                 dabt.reg, *r, reg);
         return 0;
     }
 
 bad_width:
-    dprintk(XENLOG_ERR,
-            "vGICv3: vGICD/vGICR: bad write width %d r%d=%"PRIregister" "
-            "offset %#08x\n", dabt.size, dabt.reg, *r, reg);
+    gdprintk(XENLOG_ERR,
+             "vGIC{D,R}: bad write width %d r%d=%"PRIregister" offset %#08x\n",
+             dabt.size, dabt.reg, *r, reg);
     domain_crash_synchronous();
     return 0;
 
@@ -521,13 +522,13 @@ static int vgic_v3_rdistr_sgi_mmio_read(struct vcpu *v, mmio_info_t *info,
         if ( dabt.size != DABT_WORD ) goto bad_width;
         return 1;
     default:
-        printk("vGICv3: vGICR: read r%d offset %#08x\n not found",
-               dabt.reg, gicr_reg);
+        gdprintk(XENLOG_ERR, "vGICR: SGI: read r%d offset %#08x\n not found",
+                dabt.reg, gicr_reg);
         return 0;
     }
 bad_width:
-    printk("vGICv3: vGICR: bad read width %d r%d offset %#08x\n",
-           dabt.size, dabt.reg, gicr_reg);
+    gdprintk(XENLOG_ERR, "vGICR: SGI: bad read width %d r%d offset %#08x\n",
+            dabt.size, dabt.reg, gicr_reg);
     domain_crash_synchronous();
     return 0;
 
@@ -587,14 +588,15 @@ static int vgic_v3_rdistr_sgi_mmio_write(struct vcpu *v, mmio_info_t *info,
         /* We do not implement security extensions for guests, write ignore */
         goto write_ignore;
     default:
-        printk("vGICv3: vGICR SGI: write r%d offset %#08x\n not found",
-               dabt.reg, gicr_reg);
+        gdprintk(XENLOG_ERR, "vGICR: SGI: write r%d offset %#08x\n not found",
+                 dabt.reg, gicr_reg);
         return 0;
     }
 
 bad_width:
-    printk("vGICR SGI: bad write width %d r%d=%"PRIregister" offset %#08x\n",
-           dabt.size, dabt.reg, *r, gicr_reg);
+    gdprintk(XENLOG_ERR,
+             "vGICR: SGI: bad write width %d r%d=%"PRIregister" offset %#08x\n",
+             dabt.size, dabt.reg, *r, gicr_reg);
     domain_crash_synchronous();
     return 0;
 
@@ -622,7 +624,7 @@ static int vgic_v3_rdistr_mmio_read(struct vcpu *v, mmio_info_t *info)
         return vgic_v3_rdistr_sgi_mmio_read(v, info, (offset - SZ_64K));
     else
         gdprintk(XENLOG_WARNING,
-                 "vGICv3: vGICR: unknown gpa read address %"PRIpaddr"\n",
+                 "vGICR: unknown gpa read address %"PRIpaddr"\n",
                  info->gpa);
 
     return 0;
@@ -646,7 +648,7 @@ static int vgic_v3_rdistr_mmio_write(struct vcpu *v, mmio_info_t *info)
         return vgic_v3_rdistr_sgi_mmio_write(v, info, (offset - SZ_64K));
     else
         gdprintk(XENLOG_WARNING,
-                 "vGICV3: vGICR: unknown gpa write address %"PRIpaddr"\n",
+                 "vGICR: unknown gpa write address %"PRIpaddr"\n",
                  info->gpa);
 
     return 0;
@@ -796,14 +798,14 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
         /* These are reserved register addresses */
         goto read_as_zero;
     default:
-        printk("vGICv3: vGICD: unhandled read r%d offset %#08x\n",
-               dabt.reg, gicd_reg);
+        gdprintk(XENLOG_ERR, "vGICD: unhandled read r%d offset %#08x\n",
+                 dabt.reg, gicd_reg);
         return 0;
     }
 
 bad_width:
-    dprintk(XENLOG_ERR, "vGICv3: vGICD: bad read width %d r%d offset %#08x\n",
-            dabt.size, dabt.reg, gicd_reg);
+    gdprintk(XENLOG_ERR, "vGICD: bad read width %d r%d offset %#08x\n",
+             dabt.size, dabt.reg, gicd_reg);
     domain_crash_synchronous();
     return 0;
 
@@ -865,8 +867,9 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
     case 0x020 ... 0x03c:
     case 0xc000 ... 0xffcc:
         /* Implementation defined -- write ignored */
-        printk("vGICv3: vGICD: write unknown 0x020 - 0x03c r%d offset %#08x\n",
-               dabt.reg, gicd_reg);
+        gdprintk(XENLOG_WARNING,
+                 "vGICD: write unknown 0x020 - 0x03c r%d offset %#08x\n",
+                 dabt.reg, gicd_reg);
         goto write_ignore;
     case GICD_IGROUPR ... GICD_IGROUPRN:
     case GICD_ISENABLER ... GICD_ISENABLERN:
@@ -910,8 +913,9 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
             new_target = new_irouter & MPIDR_AFF0_MASK;
             if ( new_target >= v->domain->max_vcpus )
             {
-                printk("vGICv3: vGICD: wrong irouter at offset %#08x\n val 0x%lx vcpu %x",
-                       gicd_reg, new_target, v->domain->max_vcpus);
+                gdprintk(XENLOG_ERR,
+                         "vGICD: wrong irouter at offset %#08x\n val 0x%lx vcpu %x",
+                         gicd_reg, new_target, v->domain->max_vcpus);
                 vgic_unlock_rank(v, rank, flags);
                 return 0;
             }
@@ -951,19 +955,20 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
     case 0xf30 ... 0x5fcc:
     case 0x8000 ... 0xbfcc:
         /* Reserved register addresses */
-        printk("vGICv3: vGICD: write unknown 0x00c 0xfcc  r%d offset %#08x\n",
-                dabt.reg, gicd_reg);
+        gdprintk(XENLOG_WARNING, "write unknown 0x00c 0xfcc  r%d offset %#08x\n",
+                 dabt.reg, gicd_reg);
         goto write_ignore;
     default:
-        printk("vGICv3: vGICD: unhandled write r%d=%"PRIregister" "
-               "offset %#08x\n", dabt.reg, *r, gicd_reg);
+        gdprintk(XENLOG_ERR,
+                 "vGICD: unhandled write r%d=%"PRIregister" offset %#08x\n",
+                 dabt.reg, *r, gicd_reg);
         return 0;
     }
 
 bad_width:
-    dprintk(XENLOG_ERR,
-            "VGICv3: vGICD: bad write width %d r%d=%"PRIregister" "
-            "offset %#08x\n", dabt.size, dabt.reg, *r, gicd_reg);
+    gdprintk(XENLOG_ERR,
+             "vGICD: bad write width %d r%d=%"PRIregister" offset %#08x\n",
+             dabt.size, dabt.reg, *r, gicd_reg);
     domain_crash_synchronous();
     return 0;
 
-- 
2.1.4

[PATCH 07/10] xen/arm: vgic-v2: Correctly set GICD_TYPER.CPUNumber

[Julien Grall]

The number of implemented CPU interfaces is one more than the value of
this field.

Also avoid to hardcode the shift and remove unuseful mask.

Signed-off-by: Julien Grall <julien.grall@linaro.org>

---
    This patch should be backported to Xen 4.4 and Xen 4.5.

    Although this patch won't apply directly for Xen 4.4 and may require
    to define GICD_TYPE_CPUS_SHIFT if "xen/arm: vgic-v3: Correctly set
    GICD_TYPER.CPUNumber" is not backported.
---
 xen/arch/arm/vgic-v2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index 4dc2267..a34a0c7 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -55,7 +55,7 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
         if ( dabt.size != DABT_WORD ) goto bad_width;
         /* No secure world support for guests. */
         vgic_lock(v);
-        *r = ( (v->domain->max_vcpus << 5) & GICD_TYPE_CPUS )
+        *r = ( ((v->domain->max_vcpus - 1) << GICD_TYPE_CPUS_SHIFT) )
             |( ((v->domain->arch.vgic.nr_lines / 32)) & GICD_TYPE_LINES );
         vgic_unlock(v);
         return 1;
-- 
2.1.4

[PATCH 08/10] xen/arm: vgic-v2: Don't check the size when we ignore the write/read a zero

[Julien Grall]

Some registers, such as GICD_ITARGET0 can be read/write with different
size.

When the write is ignored only word-access is checked. This will lead to
a domain crash if the guest is trying to access via byte-word.

In general, it's not necessary/important to check the size. It's better
to log it to let know the guest that its access will have no effect.

Signed-off-by: Julien Grall <julien.grall@linaro.org>

---
    This patch should be backported to Xen 4.4 and 4.5 as it fixes
    byte-access to GICD_ITARGET0.

    Although, this patch won't apply directly to Xen 4.4.
---
 xen/arch/arm/vgic-v2.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index a34a0c7..8c6ca72 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -227,7 +227,8 @@ bad_width:
     return 0;
 
 read_as_zero:
-    if ( dabt.size != DABT_WORD ) goto bad_width;
+    gdprintk(XENLOG_DEBUG, "vGICD: read as zero width, %d r%d offset %#08x\n",
+             dabt.size, dabt.reg, gicd_reg);
     *r = 0;
     return 1;
 }
@@ -498,7 +499,8 @@ bad_width:
     return 0;
 
 write_ignore:
-    if ( dabt.size != DABT_WORD ) goto bad_width;
+    gdprintk(XENLOG_DEBUG, "vGICD: ignore write width %d r%d offset %#08x\n",
+             dabt.size, dabt.reg, gicd_reg);
     return 1;
 }
 
-- 
2.1.4

[PATCH 09/10] xen/arm: vgic-v2: Take the lock when writing into GICD_CTLR

[Julien Grall]

This register is shared between every vCPUs and the lock was already
taken for read.

Signed-off-by: Julien Grall <julien.grall@linaro.org>

---
    This patch should be backported to Xen 4.4 and Xen 4.5.

    Although, it won't apply directly for Xen 4.4.
---
 xen/arch/arm/vgic-v2.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index 8c6ca72..3e5371f 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -281,7 +281,10 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
     case GICD_CTLR:
         if ( dabt.size != DABT_WORD ) goto bad_width;
         /* Ignore all but the enable bit */
+        vgic_lock(v);
         v->domain->arch.vgic.ctlr = (*r) & GICD_CTL_ENABLE;
+        vgic_unlock(v);
+
         return 1;
 
     /* R/O -- write ignored */
-- 
2.1.4

[PATCH 10/10] xen/arm: vgic-v2: Print the domain/vcpu in each message

[Julien Grall]

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/vgic-v2.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index 3e5371f..86b4340 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -198,7 +198,7 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
 
     case GICD_ICPIDR2:
         if ( dabt.size != DABT_WORD ) goto bad_width;
-        printk("vGICD: unhandled read from ICPIDR2\n");
+        gdprintk(XENLOG_ERR, "vGICD: unhandled read from ICPIDR2\n");
         return 0;
 
     /* Implementation defined -- read as zero */
@@ -215,14 +215,14 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
         goto read_as_zero;
 
     default:
-        printk("vGICD: unhandled read r%d offset %#08x\n",
-               dabt.reg, gicd_reg);
+        gdprintk(XENLOG_ERR, "vGICD: unhandled read r%d offset %#08x\n",
+                 dabt.reg, gicd_reg);
         return 0;
     }
 
 bad_width:
-    printk("vGICD: bad read width %d r%d offset %#08x\n",
-           dabt.size, dabt.reg, gicd_reg);
+    gdprintk(XENLOG_ERR, "vGICD: bad read width %d r%d offset %#08x\n",
+             dabt.size, dabt.reg, gicd_reg);
     domain_crash_synchronous();
     return 0;
 
@@ -458,14 +458,16 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
 
     case GICD_CPENDSGIR ... GICD_CPENDSGIRN:
         if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
-        printk("vGICD: unhandled %s write %#"PRIregister" to ICPENDSGIR%d\n",
-               dabt.size ? "word" : "byte", *r, gicd_reg - GICD_CPENDSGIR);
+        gdprintk(XENLOG_ERR,
+                 "vGICD: unhandled %s write %#"PRIregister" to ICPENDSGIR%d\n",
+                 dabt.size ? "word" : "byte", *r, gicd_reg - GICD_CPENDSGIR);
         return 0;
 
     case GICD_SPENDSGIR ... GICD_SPENDSGIRN:
         if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
-        printk("vGICD: unhandled %s write %#"PRIregister" to ISPENDSGIR%d\n",
-               dabt.size ? "word" : "byte", *r, gicd_reg - GICD_SPENDSGIR);
+        gdprintk(XENLOG_ERR,
+                 "vGICD: unhandled %s write %#"PRIregister" to ISPENDSGIR%d\n",
+                 dabt.size ? "word" : "byte", *r, gicd_reg - GICD_SPENDSGIR);
         return 0;
 
     /* Implementation defined -- write ignored */
@@ -490,14 +492,16 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
         goto write_ignore;
 
     default:
-        printk("vGICD: unhandled write r%d=%"PRIregister" offset %#08x\n",
-               dabt.reg, *r, gicd_reg);
+        gdprintk(XENLOG_ERR,
+                 "vGICD: unhandled write r%d=%"PRIregister" offset %#08x\n",
+                 dabt.reg, *r, gicd_reg);
         return 0;
     }
 
 bad_width:
-    printk("vGICD: bad write width %d r%d=%"PRIregister" offset %#08x\n",
-           dabt.size, dabt.reg, *r, gicd_reg);
+    gdprintk(XENLOG_ERR,
+             "vGICD: bad write width %d r%d=%"PRIregister" offset %#08x\n",
+             dabt.size, dabt.reg, *r, gicd_reg);
     domain_crash_synchronous();
     return 0;
 
-- 
2.1.4

Re: [PATCH 01/10] xen/arm: vgic-v3: Correctly set GICD_TYPER.IDbits

[Ian Campbell]

On Mon, 2015-01-19 at 16:29 +0000, Julien Grall wrote:
> +        unsigned int order;
> +
>          if ( dabt.size != DABT_WORD ) goto bad_width;
>          /* No secure world support for guests. */
>          *r = (((v->domain->max_vcpus << 5) & GICD_TYPE_CPUS ) |
>                ((v->domain->arch.vgic.nr_lines / 32) & GICD_TYPE_LINES));
> +
> +        /*
> +         * Calculate number of interrupt identifier bits supported by
> +         * the GIC Stream Protocol Interface
> +         */
> +        irqs--;
> +        for ( order = 0; irqs; order++ )
> +            irqs >>= 1;

This is some variant on fls(). See get_bitmask_order() or
get_count_order() for an example of use, possibly even one you could
call.

Ian.

Re: [PATCH 01/10] xen/arm: vgic-v3: Correctly set GICD_TYPER.IDbits

[Ian Campbell]

On Mon, 2015-01-19 at 16:29 +0000, Julien Grall wrote:
> +
> +        *r |= ((order - 1) << GICD_TYPE_ID_BITS_SHIFT) & GICD_TYPE_ID_BITS_MASK;

Please can you calculate the value and then construct *r in one go, so
it is easier to see all the parts in one place. Thanks.

I wonder if this field is now complex enough to warrant the union+struct
approach we use for other registers (e.g. the ESR).

Ian.

Re: [PATCH 02/10] xen/arm: vgic-v3: Correctly set GICD_TYPER.CPUNumber

[Ian Campbell]

On Mon, 2015-01-19 at 16:29 +0000, Julien Grall wrote:
> On GICv3, the value (CPUNumber + 1) indicates the number of processor that may
> be used as interrupts targets when ARE bit is zero. The maximum is 8
> processors.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH 03/10] xen/arm: vgic-v3: Correctly handle GICD_CTLR

[Ian Campbell]

On Mon, 2015-01-19 at 16:29 +0000, Julien Grall wrote:
> As backward GICv2 compatibility is not supported in the VGICv3 driver,
> the bit ARE_NS should be set at any time.

Looking at the docs, I think you mean it is RAO/WI if GICv2 compat is
absent (rather than imply the guest should set it).

> 
> Futhermore, when ARE_NS is set, the guest can only modify EnableGrp1A.

"Furthermore"

> At same time take the vgic_lock to write into domain.arch.vgic.ctrl. It
> was already taken during read.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/arch/arm/vgic-v3.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index 406ea93..1aa2f58 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -45,6 +45,12 @@
>  #define GICV3_GICR_PIDR2  GICV3_GICD_PIDR2
>  #define GICV3_GICR_PIDR4  GICV3_GICD_PIDR4
>  
> +/*
> + * GICD_CTLR default value:
> + *      - No GICv2 compatibility => ARE = 1
> + */
> +#define VGICD_CTLR_DEFAULT  (GICD_CTLR_ARE_NS)
> +
>  static struct vcpu *vgic_v3_irouter_to_vcpu(struct vcpu *v, uint64_t irouter)
>  {
>      irouter &= ~(GICD_IROUTER_SPI_MODE_ANY);
> @@ -834,8 +840,15 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
>      {
>      case GICD_CTLR:
>          if ( dabt.size != DABT_WORD ) goto bad_width;
> -        /* Ignore all but the enable bit */
> -        v->domain->arch.vgic.ctlr = (*r) & GICD_CTL_ENABLE;
> +
> +        vgic_lock(v);
> +        /* Only EnableGrp1A can be changed */
> +        if ( *r & GICD_CTLR_ENABLE_G1A )
> +            v->domain->arch.vgic.ctlr |= GICD_CTLR_ENABLE_G1A;
> +        else
> +            v->domain->arch.vgic.ctlr &= ~GICD_CTLR_ENABLE_G1A;
> +        vgic_unlock(v);
> +
>          return 1;
>      case GICD_TYPER:
>          /* RO -- write ignored */
> @@ -1092,6 +1105,8 @@ static int vgic_v3_domain_init(struct domain *d)
>          register_mmio_handler(d, &vgic_rdistr_mmio_handler,
>              d->arch.vgic.rbase[i], d->arch.vgic.rbase_size[i]);
>  
> +    d->arch.vgic.ctlr = VGICD_CTLR_DEFAULT;
> +
>      return 0;
>  }
>  

Re: [PATCH 04/10] xen/arm: vgic-v3: Don't check the size when we ignore the write/read as zero

[Ian Campbell]

On Mon, 2015-01-19 at 16:29 +0000, Julien Grall wrote:
> In general, it's not necessary/important to check the size.

Only if the docs say this register can be accessed by a partial
read/write, or if it is implementation defined what the result would be
(and RAZ/WI is within the set of allowable actions).

Do you have a reference for the behaviour of GICR accesses which aren't
of the register's natural size?

>  It's better
> to log it to let know the guest that its access will have no effect.
> 
> Note: On debug build it may happen to see some of these messages during
> domain boot.

We should only print if the guest has done something wrong, and reading
a RAZ register (or one which we have implemented that way) is not
inherently wrong.

IOW read_as_zero* should be silent, and a different code path used for
"guest did something wrong".

IOW I think the current distinction between bad_width and read_as_zero*
is correct currently, although perhaps the goto's which target them need
adjusting in some cases.

Perhaps you want to add a read_as_zero_32 which has the check, making
read_as_zero accept either 32- or 64-bit accesses, and goto the correct
label for each register?

Ian.

Re: [PATCH 05/10] xen/arm: vgic-v3: Document the current restrictions

[Ian Campbell]

On Mon, 2015-01-19 at 16:29 +0000, Julien Grall wrote:
> The current vGIC v3 driver doesn't fully implement GICv3 spec:
>     - GICv3 backward compatibility is not supported (GICD_CTLR.ARE = 0)

I think you meant GICv2 here as you did in the code.

In which case I believe this is optional in the spec, i.e. we can be
compliant and still not implement this.

That's not to say it isn't desirable, but this is a TODO item, not a
spec non-conformity issue.

>     - A processor can only access his own redistributor. For buggy
>     assumption, the current code bank the redistributors MMIO.

What assumption? It's not clear if you mean that a foreign redistributor
should not be accessible and is, or if it should be accessible and
isn't.

> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/arch/arm/vgic-v3.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index 1fa1413..9818a6b 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -16,6 +16,11 @@
>   * but WITHOUT ANY WARRANTY; without even the implied warranty of
>   * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>   * GNU General Public License for more details.
> + *
> + * Current limitation of the vGIC v3:
> + *      - GICv2 backward compatibility is not supported (GICD_CTRL.ARE = 0)
> + *      - A processor can only access his own redistributor. For buggy
> + *      assumption, the current code bank the redistributors MMIO
>   */
>  
>  #include <xen/bitops.h>

Re: [PATCH 06/10] xen/arm: vgic-v3: Print the domain/vcpu in each message

[Ian Campbell]

On Mon, 2015-01-19 at 16:29 +0000, Julien Grall wrote:
> Also remove vGICv3 in the message log as gdprintk already print the name
> of the file.

Please mention that the switch to gdprintk is how/where the domain/vcpu
is added, e.g. by adding "... by switching to gdprintk" as the first
line of the main body of the commit log (assuming it won't fit in the
subject)

With that:
Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH 07/10] xen/arm: vgic-v2: Correctly set GICD_TYPER.CPUNumber

[Ian Campbell]

On Mon, 2015-01-19 at 16:29 +0000, Julien Grall wrote:
> The number of implemented CPU interfaces is one more than the value of
> this field.
> 
> Also avoid to hardcode the shift and remove unuseful mask.

Also avoid hardcoding...

s/unuseful/useless/

> Signed-off-by: Julien Grall <julien.grall@linaro.org>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH 08/10] xen/arm: vgic-v2: Don't check the size when we ignore the write/read a zero

[Ian Campbell]

On Mon, 2015-01-19 at 16:29 +0000, Julien Grall wrote:
> Some registers, such as GICD_ITARGET0 can be read/write with different
> size.
> 
> When the write is ignored only word-access is checked. This will lead to
> a domain crash if the guest is trying to access via byte-word.
> 
> In general, it's not necessary/important to check the size.

In general it is, unless you have a reference which says otherwise for a
given register.

In the specific case of ITARGET this is true though, but as with the v3
version I disagree with printing in the RAZ/WI cases, we should print
only on bad width accesses.

I think this probably means you want to switch the goto's in the ITARGET
case to something else, probably implementing read_as_zero_* for checked
cases.

>  It's better
> to log it to let know the guest that its access will have no effect.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> ---
>     This patch should be backported to Xen 4.4 and 4.5 as it fixes
>     byte-access to GICD_ITARGET0.
> 
>     Although, this patch won't apply directly to Xen 4.4.
> ---
>  xen/arch/arm/vgic-v2.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index a34a0c7..8c6ca72 100644
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -227,7 +227,8 @@ bad_width:
>      return 0;
>  
>  read_as_zero:
> -    if ( dabt.size != DABT_WORD ) goto bad_width;
> +    gdprintk(XENLOG_DEBUG, "vGICD: read as zero width, %d r%d offset %#08x\n",
> +             dabt.size, dabt.reg, gicd_reg);
>      *r = 0;
>      return 1;
>  }
> @@ -498,7 +499,8 @@ bad_width:
>      return 0;
>  
>  write_ignore:
> -    if ( dabt.size != DABT_WORD ) goto bad_width;
> +    gdprintk(XENLOG_DEBUG, "vGICD: ignore write width %d r%d offset %#08x\n",
> +             dabt.size, dabt.reg, gicd_reg);
>      return 1;
>  }
>  

Re: [PATCH 09/10] xen/arm: vgic-v2: Take the lock when writing into GICD_CTLR

[Ian Campbell]

On Mon, 2015-01-19 at 16:29 +0000, Julien Grall wrote:
> This register is shared between every vCPUs and the lock was already
> taken for read.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH 10/10] xen/arm: vgic-v2: Print the domain/vcpu in each message

[Ian Campbell]

On Mon, 2015-01-19 at 16:29 +0000, Julien Grall wrote:

Please reference the switch to gdprintk being the mechanism for
achieving this, otherwise:
        Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH 01/10] xen/arm: vgic-v3: Correctly set GICD_TYPER.IDbits

[Julien Grall]

Hi Ian,

On 20/01/15 15:34, Ian Campbell wrote:
> On Mon, 2015-01-19 at 16:29 +0000, Julien Grall wrote:
>> +        unsigned int order;
>> +
>>          if ( dabt.size != DABT_WORD ) goto bad_width;
>>          /* No secure world support for guests. */
>>          *r = (((v->domain->max_vcpus << 5) & GICD_TYPE_CPUS ) |
>>                ((v->domain->arch.vgic.nr_lines / 32) & GICD_TYPE_LINES));
>> +
>> +        /*
>> +         * Calculate number of interrupt identifier bits supported by
>> +         * the GIC Stream Protocol Interface
>> +         */
>> +        irqs--;
>> +        for ( order = 0; irqs; order++ )
>> +            irqs >>= 1;
> 
> This is some variant on fls(). See get_bitmask_order() or
> get_count_order() for an example of use, possibly even one you could
> call.

get_count_order sounds the best function to use. I will give a try.

Regards,

-- 
Julien Grall

Re: [PATCH 03/10] xen/arm: vgic-v3: Correctly handle GICD_CTLR

[Julien Grall]

Hi Ian,

On 20/01/15 15:51, Ian Campbell wrote:
> On Mon, 2015-01-19 at 16:29 +0000, Julien Grall wrote:
>> As backward GICv2 compatibility is not supported in the VGICv3 driver,
>> the bit ARE_NS should be set at any time.
> 
> Looking at the docs, I think you mean it is RAO/WI if GICv2 compat is
> absent (rather than imply the guest should set it).

Right, I will update the commit message.

Regards,

-- 
Julien Grall

Re: [PATCH 04/10] xen/arm: vgic-v3: Don't check the size when we ignore the write/read as zero

[Julien Grall]

Hi Ian,

On 20/01/15 15:57, Ian Campbell wrote:
> On Mon, 2015-01-19 at 16:29 +0000, Julien Grall wrote:
>> In general, it's not necessary/important to check the size.
> 
> Only if the docs say this register can be accessed by a partial
> read/write, or if it is implementation defined what the result would be
> (and RAZ/WI is within the set of allowable actions).
> 
> Do you have a reference for the behaviour of GICR accesses which aren't
> of the register's natural size?

It's clearly specify in the spec if the register can be accessed with a
non-natural size.

AFAICT, the spec doesn't give a specific behavior if the register
doesn't support byte/word/double word access.

IHMO, for RAZ register we can safely accept any kind of access as the
final register will in fine always contains 0.

That will also any issue with WI/RAZ register because we may have miss a
line in the spec stating a register is byte and word accessible. (see
the case of vgic-v2).

>>  It's better
>> to log it to let know the guest that its access will have no effect.
>>
>> Note: On debug build it may happen to see some of these messages during
>> domain boot.
> 
> We should only print if the guest has done something wrong, and reading
> a RAZ register (or one which we have implemented that way) is not
> inherently wrong.

That's why it's log in DEBUG and not in ERR. Although, I agree that on
read it's not important. But on write it's help the developer to
understand which his GIC driver is not working.

> IOW read_as_zero* should be silent, and a different code path used for
> "guest did something wrong".
>
> IOW I think the current distinction between bad_width and read_as_zero*
> is correct currently, although perhaps the goto's which target them need
> adjusting in some cases.
> 
> Perhaps you want to add a read_as_zero_32 which has the check, making
> read_as_zero accept either 32- or 64-bit accesses, and goto the correct
> label for each register?

It's register defined which access is allowed for a register.

Regards,

-- 
Julien Grall

Re: [PATCH 05/10] xen/arm: vgic-v3: Document the current restrictions

[Julien Grall]

Hi Ian,

On 20/01/15 16:00, Ian Campbell wrote:
> On Mon, 2015-01-19 at 16:29 +0000, Julien Grall wrote:
>> The current vGIC v3 driver doesn't fully implement GICv3 spec:
>>     - GICv3 backward compatibility is not supported (GICD_CTLR.ARE = 0)
> 
> I think you meant GICv2 here as you did in the code.

Yes.

> In which case I believe this is optional in the spec, i.e. we can be
> compliant and still not implement this.
> 
> That's not to say it isn't desirable, but this is a TODO item, not a
> spec non-conformity issue.

Right, I should rename the section to TODO.

>>     - A processor can only access his own redistributor. For buggy
>>     assumption, the current code bank the redistributors MMIO.
> 
> What assumption? It's not clear if you mean that a foreign redistributor
> should not be accessible and is, or if it should be accessible and
> isn't.

Every redistributor (one per processor) are mapped in distinct MMIO region.

Unlike the distributor, the redistributor is not banked.

Our current implementation (see vgic_v3_rdistr_mmio_write) consider that
the redistributor is banked and replicate n-times in the memory.

If you give a look to the redistributor iniatialization (see Xen and
Linux GICv3 code). The code will go through all the redistributors and
check GICR_TYPER to see if the processor is associated to this
redistributor.

I'm not sure how the redistributor should behave if it's accessed by
another processor. But I'm sure it's wrong to bank it.

Regards,

-- 
Julien Grall

Re: [PATCH 06/10] xen/arm: vgic-v3: Print the domain/vcpu in each message

[Julien Grall]

Hi Ian,

On 20/01/15 16:05, Ian Campbell wrote:
> On Mon, 2015-01-19 at 16:29 +0000, Julien Grall wrote:
>> Also remove vGICv3 in the message log as gdprintk already print the name
>> of the file.
> 
> Please mention that the switch to gdprintk is how/where the domain/vcpu
> is added, e.g. by adding "... by switching to gdprintk" as the first
> line of the main body of the commit log (assuming it won't fit in the
> subject)

Will do.

> With that:
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

Thanks,

-- 
Julien Grall

Re: [PATCH 04/10] xen/arm: vgic-v3: Don't check the size when we ignore the write/read as zero

[Ian Campbell]

On Tue, 2015-01-20 at 17:41 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 20/01/15 15:57, Ian Campbell wrote:
> > On Mon, 2015-01-19 at 16:29 +0000, Julien Grall wrote:
> >> In general, it's not necessary/important to check the size.
> > 
> > Only if the docs say this register can be accessed by a partial
> > read/write, or if it is implementation defined what the result would be
> > (and RAZ/WI is within the set of allowable actions).
> > 
> > Do you have a reference for the behaviour of GICR accesses which aren't
> > of the register's natural size?
> 
> It's clearly specify in the spec if the register can be accessed with a
> non-natural size.
> 
> AFAICT, the spec doesn't give a specific behavior if the register
> doesn't support byte/word/double word access.

5.1.3 of the GICv3 spec defines it as UNPREDICATABLE (and also lists all
the valid options for each register).

> IHMO, for RAZ register we can safely accept any kind of access as the
> final register will in fine always contains 0.
> 
> That will also any issue with WI/RAZ register because we may have miss a
> line in the spec stating a register is byte and word accessible. (see
> the case of vgic-v2).
> 
> >>  It's better
> >> to log it to let know the guest that its access will have no effect.
> >>
> >> Note: On debug build it may happen to see some of these messages during
> >> domain boot.
> > 
> > We should only print if the guest has done something wrong, and reading
> > a RAZ register (or one which we have implemented that way) is not
> > inherently wrong.
> 
> That's why it's log in DEBUG and not in ERR. Although, I agree that on
> read it's not important. But on write it's help the developer to
> understand which his GIC driver is not working.

If the driver is making a legitimate access (i.e. correct size etc) then
we shouldn't be logging, irrespective of whether we are implementing as
RAZ/WI or anything else.

Given the contents of 5.1.3 I could perhaps be convinced accept making
the bad_width: behaviour less catastrophic to the guest, but killing the
guest meets the defintion of UNPREDICTABLE I think and in general not
letting guests take unexpect advantage of odd corner cases is a good
idea IMHO, lest they come to rely on something.

> > IOW read_as_zero* should be silent, and a different code path used for
> > "guest did something wrong".
> >
> > IOW I think the current distinction between bad_width and read_as_zero*
> > is correct currently, although perhaps the goto's which target them need
> > adjusting in some cases.
> > 
> > Perhaps you want to add a read_as_zero_32 which has the check, making
> > read_as_zero accept either 32- or 64-bit accesses, and goto the correct
> > label for each register?
> 
> It's register defined which access is allowed for a register.

Right, so the decoding switch would need to use the right goto for its
case.

Ian.

Re: [PATCH 04/10] xen/arm: vgic-v3: Don't check the size when we ignore the write/read as zero

[Julien Grall]

On 20/01/15 17:41, Julien Grall wrote:
> Hi Ian,
> 
> On 20/01/15 15:57, Ian Campbell wrote:
>> On Mon, 2015-01-19 at 16:29 +0000, Julien Grall wrote:
>>> In general, it's not necessary/important to check the size.
>>
>> Only if the docs say this register can be accessed by a partial
>> read/write, or if it is implementation defined what the result would be
>> (and RAZ/WI is within the set of allowable actions).
>>
>> Do you have a reference for the behaviour of GICR accesses which aren't
>> of the register's natural size?
> 
> It's clearly specify in the spec if the register can be accessed with a
> non-natural size.
> 
> AFAICT, the spec doesn't give a specific behavior if the register
> doesn't support byte/word/double word access.

Hmmm, I read quickly the spec. 5.1.3 says: "Accessing any of these
registers using other accesses is UNPREDICTABLE".

So I think it's fine to go on this behavior. It would help to have a
simpler code.

Regards,

-- 
Julien Grall

Re: [PATCH 04/10] xen/arm: vgic-v3: Don't check the size when we ignore the write/read as zero

[Julien Grall]

On 20/01/15 17:57, Ian Campbell wrote:
> On Tue, 2015-01-20 at 17:41 +0000, Julien Grall wrote:
>> Hi Ian,
>>
>> On 20/01/15 15:57, Ian Campbell wrote:
>>> On Mon, 2015-01-19 at 16:29 +0000, Julien Grall wrote:
>>>> In general, it's not necessary/important to check the size.
>>>
>>> Only if the docs say this register can be accessed by a partial
>>> read/write, or if it is implementation defined what the result would be
>>> (and RAZ/WI is within the set of allowable actions).
>>>
>>> Do you have a reference for the behaviour of GICR accesses which aren't
>>> of the register's natural size?
>>
>> It's clearly specify in the spec if the register can be accessed with a
>> non-natural size.
>>
>> AFAICT, the spec doesn't give a specific behavior if the register
>> doesn't support byte/word/double word access.
> 
> 5.1.3 of the GICv3 spec defines it as UNPREDICATABLE (and also lists all
> the valid options for each register).

I saw it after sending the mail.

>> IHMO, for RAZ register we can safely accept any kind of access as the
>> final register will in fine always contains 0.
>>
>> That will also any issue with WI/RAZ register because we may have miss a
>> line in the spec stating a register is byte and word accessible. (see
>> the case of vgic-v2).
>>
>>>>  It's better
>>>> to log it to let know the guest that its access will have no effect.
>>>>
>>>> Note: On debug build it may happen to see some of these messages during
>>>> domain boot.
>>>
>>> We should only print if the guest has done something wrong, and reading
>>> a RAZ register (or one which we have implemented that way) is not
>>> inherently wrong.
>>
>> That's why it's log in DEBUG and not in ERR. Although, I agree that on
>> read it's not important. But on write it's help the developer to
>> understand which his GIC driver is not working.
> 
> If the driver is making a legitimate access (i.e. correct size etc) then
> we shouldn't be logging, irrespective of whether we are implementing as
> RAZ/WI or anything else.

I agree for RAZ, but WI would mean something will goes wrong. For
instance if the guest is trying to set a bit to 1, while the bit should
be 0.

So I'd like to keep at least a debug message for WI. It's could be very
helpful for the developper.

> Given the contents of 5.1.3 I could perhaps be convinced accept making
> the bad_width: behaviour less catastrophic to the guest, but killing the
> guest meets the defintion of UNPREDICTABLE I think and in general not
> letting guests take unexpect advantage of odd corner cases is a good
> idea IMHO, lest they come to rely on something.
> 
>>> IOW read_as_zero* should be silent, and a different code path used for
>>> "guest did something wrong".
>>>
>>> IOW I think the current distinction between bad_width and read_as_zero*
>>> is correct currently, although perhaps the goto's which target them need
>>> adjusting in some cases.
>>>
>>> Perhaps you want to add a read_as_zero_32 which has the check, making
>>> read_as_zero accept either 32- or 64-bit accesses, and goto the correct
>>> label for each register?
>>
>> It's register defined which access is allowed for a register.
> 
> Right, so the decoding switch would need to use the right goto for its
> case.

As the spec defines the access to be UNPREDICATABLE, I would rather
prefer to not decode the size and directly read as zero/write ignore.

I would end up to a smaller code and more comprehensible one.

Regards,

-- 
Julien Grall

Re: [PATCH 04/10] xen/arm: vgic-v3: Don't check the size when we ignore the write/read as zero

[Ian Campbell]

On Tue, 2015-01-20 at 18:50 +0000, Julien Grall wrote:
[...]
> I agree for RAZ, but WI would mean something will goes wrong. For
> instance if the guest is trying to set a bit to 1, while the bit should
> be 0.

It depends on the reason for the WI. If the reason is that the spec says
the register is WI (in our supported HW configuration) then we should
silently implement that.

If it is WI because we don't implement some functionality which we
should, i.e. something should happen but we don't emulate that, then
debug-level logging might be appropriate (on a case by case basis).

Note that I'm treating things which are WI because we don't support
GICv2 compat and therefore ARE=1 and some register is then WI because of
that as the former case, not the latter. Because not implementing GICv2
compat is a valid h/w configuration.

> As the spec defines the access to be UNPREDICATABLE, I would rather
> prefer to not decode the size and directly read as zero/write ignore.

I thought about this overnight, and I would like to keep UNPREDICATABLE
as the current log + crash please. Apart from the fact that I don't want
guests to be able to rely on unpredictable accesses returning 0 it is
also more consistent with the ARM ARM which says "UNPREDICTABLE
behaviour must not be documented or promoted as having a defined
effect".

So, in summary:

1) Any access which is described as UNPREDICTABLE in GIC spec 5.1.3
should result in the current bad_width behaviour, that is: a log message
and a domain crash.

2) Accesses which are valid and which are correctly emulated according
to the features which our virtual GIC exposes to the guest should
succeed silently, regardless of whether that means WI, read a constant
or actually have some effect.

3) Accesses which are valid but which do not correctly emulate according
to the features of the virtual gic which we are exposing can log if we
think it is useful to do so.

Ian.

Re: [PATCH 05/10] xen/arm: vgic-v3: Document the current restrictions

[Ian Campbell]

On Tue, 2015-01-20 at 17:49 +0000, Julien Grall wrote:

> >>     - A processor can only access his own redistributor. For buggy
> >>     assumption, the current code bank the redistributors MMIO.
> > 
> > What assumption? It's not clear if you mean that a foreign redistributor
> > should not be accessible and is, or if it should be accessible and
> > isn't.
> 
> Every redistributor (one per processor) are mapped in distinct MMIO region.
> 
> Unlike the distributor, the redistributor is not banked.

Understood.

> Our current implementation (see vgic_v3_rdistr_mmio_write) consider that
> the redistributor is banked and replicate n-times in the memory.

IOW instead of having e.g. 8 individual redistributors each vcpu sees
it's own redistributor 8 times. That does seem a bit dubious.

> If you give a look to the redistributor iniatialization (see Xen and
> Linux GICv3 code). The code will go through all the redistributors and
> check GICR_TYPER to see if the processor is associated to this
> redistributor.
> 
> I'm not sure how the redistributor should behave if it's accessed by
> another processor.

Please can you find a spec reference and include it in the clarified
version of this item.

> But I'm sure it's wrong to bank it.

Seems likely, but answering the above Q would probably decide this one
way or another too.

Re: [PATCH 04/10] xen/arm: vgic-v3: Don't check the size when we ignore the write/read as zero

[Julien Grall]

On 21/01/15 12:11, Ian Campbell wrote:
> I thought about this overnight, and I would like to keep UNPREDICATABLE
> as the current log + crash please. Apart from the fact that I don't want
> guests to be able to rely on unpredictable accesses returning 0 it is
> also more consistent with the ARM ARM which says "UNPREDICTABLE
> behaviour must not be documented or promoted as having a defined
> effect".

I was actually planning to suggest this as the access for each registers
are clearly define in the specs.

> So, in summary:
> 
> 1) Any access which is described as UNPREDICTABLE in GIC spec 5.1.3
> should result in the current bad_width behaviour, that is: a log message
> and a domain crash.

Ok.

> 
> 2) Accesses which are valid and which are correctly emulated according
> to the features which our virtual GIC exposes to the guest should
> succeed silently, regardless of whether that means WI, read a constant
> or actually have some effect.

Ok.

> 
> 3) Accesses which are valid but which do not correctly emulate according
> to the features of the virtual gic which we are exposing can log if we
> think it is useful to do so.

I gave a look to the code. We have few registers we don't correctly
emulate. The current behavior seems to be inconsistent, we either inject
a data abort (such as ICPENDR) or ignore the error (such as ICACTIVER).

Shall we take a domain_crash approach (as bad_width) or inject a data abort?

Regards,


-- 
Julien Grall

Re: [PATCH 05/10] xen/arm: vgic-v3: Document the current restrictions

[Julien Grall]

On 21/01/15 12:16, Ian Campbell wrote:
> On Tue, 2015-01-20 at 17:49 +0000, Julien Grall wrote:
> 
>>>>     - A processor can only access his own redistributor. For buggy
>>>>     assumption, the current code bank the redistributors MMIO.
>>>
>>> What assumption? It's not clear if you mean that a foreign redistributor
>>> should not be accessible and is, or if it should be accessible and
>>> isn't.
>>
>> Every redistributor (one per processor) are mapped in distinct MMIO region.
>>
>> Unlike the distributor, the redistributor is not banked.
> 
> Understood.
> 
>> Our current implementation (see vgic_v3_rdistr_mmio_write) consider that
>> the redistributor is banked and replicate n-times in the memory.
> 
> IOW instead of having e.g. 8 individual redistributors each vcpu sees
> it's own redistributor 8 times. That does seem a bit dubious.

It's the current behavior. You can see the difference in linux log. The
address of each redistributor is the same.

>> If you give a look to the redistributor iniatialization (see Xen and
>> Linux GICv3 code). The code will go through all the redistributors and
>> check GICR_TYPER to see if the processor is associated to this
>> redistributor.
>>
>> I'm not sure how the redistributor should behave if it's accessed by
>> another processor.
> 
> Please can you find a spec reference and include it in the clarified
> version of this item.

Rather than the distributor, there is multiple redistributor (one per
processor).

I think the section 5.4.1 in the GICv3 should answer to this question:

"Each re-distributor must be allocated at one page for controlling the
overall behavior of the re-distributor and for controlling physical
LPIs. The base address of this page is referred to as RD_base. In
addition, each re-distributor must be also allocated the following
additional pages".

Regards,

-- 
Julien Grall

Re: [PATCH 04/10] xen/arm: vgic-v3: Don't check the size when we ignore the write/read as zero

[Ian Campbell]

On Wed, 2015-01-21 at 12:28 +0000, Julien Grall wrote:

> > 3) Accesses which are valid but which do not correctly emulate according
> > to the features of the virtual gic which we are exposing can log if we
> > think it is useful to do so.
> 
> I gave a look to the code. We have few registers we don't correctly
> emulate. The current behavior seems to be inconsistent, we either inject
> a data abort (such as ICPENDR) or ignore the error (such as ICACTIVER).
>
> Shall we take a domain_crash approach (as bad_width) or inject a data abort?

I think for these cases since we do update and/or return
rank->iactive/ipend a debug log and continue is appropriate.

Assuming we do need to do something more than track the status of
i{active,pend}, which I expect we do -- e.g. to cancel or inject as
appropriate.

Ian.

Re: [PATCH 04/10] xen/arm: vgic-v3: Don't check the size when we ignore the write/read as zero

[Julien Grall]

On 21/01/15 12:36, Ian Campbell wrote:
> On Wed, 2015-01-21 at 12:28 +0000, Julien Grall wrote:
> 
>>> 3) Accesses which are valid but which do not correctly emulate according
>>> to the features of the virtual gic which we are exposing can log if we
>>> think it is useful to do so.
>>
>> I gave a look to the code. We have few registers we don't correctly
>> emulate. The current behavior seems to be inconsistent, we either inject
>> a data abort (such as ICPENDR) or ignore the error (such as ICACTIVER).
>>
>> Shall we take a domain_crash approach (as bad_width) or inject a data abort?
> 
> I think for these cases since we do update and/or return
> rank->iactive/ipend a debug log and continue is appropriate.
>
> Assuming we do need to do something more than track the status of
> i{active,pend}, which I expect we do -- e.g. to cancel or inject as
> appropriate.

This code is actually buggy as we never use those fields. I have a patch
to drop iactive/ipend fields as we will unlikely handle ACTIVE/PENDING
status via those bit. We already track in different way as we already
have tracking per vIRQ.

My plan was to replace them with an error log and maybe a data abort.
If you think a debug message is enough, I could go this way. Though, it
may be more difficult for a developer to find the actual error if there
is may logs.

Regards,

-- 
Julien Grall

Re: [PATCH 05/10] xen/arm: vgic-v3: Document the current restrictions

[Ian Campbell]

On Wed, 2015-01-21 at 12:33 +0000, Julien Grall wrote:
> On 21/01/15 12:16, Ian Campbell wrote:
> > On Tue, 2015-01-20 at 17:49 +0000, Julien Grall wrote:
> > 
> >>>>     - A processor can only access his own redistributor. For buggy
> >>>>     assumption, the current code bank the redistributors MMIO.
> >>>
> >>> What assumption? It's not clear if you mean that a foreign redistributor
> >>> should not be accessible and is, or if it should be accessible and
> >>> isn't.
> >>
> >> Every redistributor (one per processor) are mapped in distinct MMIO region.
> >>
> >> Unlike the distributor, the redistributor is not banked.
> > 
> > Understood.
> > 
> >> Our current implementation (see vgic_v3_rdistr_mmio_write) consider that
> >> the redistributor is banked and replicate n-times in the memory.
> > 
> > IOW instead of having e.g. 8 individual redistributors each vcpu sees
> > it's own redistributor 8 times. That does seem a bit dubious.
> 
> It's the current behavior. You can see the difference in linux log. The
> address of each redistributor is the same.

Are you sure that isn't the "redistributor region"? One of those can
contain multiple redistributors, i.e. gic_v3_init sets up a single
region and that propagates to the DTB given to the guest.

So the error is just in the mmio decode stage I think, not in the setup
we are trying to achieve.

> >> If you give a look to the redistributor iniatialization (see Xen and
> >> Linux GICv3 code). The code will go through all the redistributors and
> >> check GICR_TYPER to see if the processor is associated to this
> >> redistributor.
> >>
> >> I'm not sure how the redistributor should behave if it's accessed by
> >> another processor.
> > 
> > Please can you find a spec reference and include it in the clarified
> > version of this item.
> 
> Rather than the distributor, there is multiple redistributor (one per
> processor).
> 
> I think the section 5.4.1 in the GICv3 should answer to this question:
> 
> "Each re-distributor must be allocated at one page for controlling the
> overall behavior of the re-distributor and for controlling physical
> LPIs. The base address of this page is referred to as RD_base. In
> addition, each re-distributor must be also allocated the following
> additional pages".

That doesn't say anything about one CPU touching another's
redistributor.

Ian.

Re: [PATCH 04/10] xen/arm: vgic-v3: Don't check the size when we ignore the write/read as zero

[Ian Campbell]

On Wed, 2015-01-21 at 12:45 +0000, Julien Grall wrote:
> On 21/01/15 12:36, Ian Campbell wrote:
> > On Wed, 2015-01-21 at 12:28 +0000, Julien Grall wrote:
> > 
> >>> 3) Accesses which are valid but which do not correctly emulate according
> >>> to the features of the virtual gic which we are exposing can log if we
> >>> think it is useful to do so.
> >>
> >> I gave a look to the code. We have few registers we don't correctly
> >> emulate. The current behavior seems to be inconsistent, we either inject
> >> a data abort (such as ICPENDR) or ignore the error (such as ICACTIVER).
> >>
> >> Shall we take a domain_crash approach (as bad_width) or inject a data abort?
> > 
> > I think for these cases since we do update and/or return
> > rank->iactive/ipend a debug log and continue is appropriate.
> >
> > Assuming we do need to do something more than track the status of
> > i{active,pend}, which I expect we do -- e.g. to cancel or inject as
> > appropriate.
> 
> This code is actually buggy as we never use those fields. I have a patch
> to drop iactive/ipend fields as we will unlikely handle ACTIVE/PENDING
> status via those bit. We already track in different way as we already
> have tracking per vIRQ.
> 
> My plan was to replace them with an error log and maybe a data abort.
> If you think a debug message is enough, I could go this way. Though, it
> may be more difficult for a developer to find the actual error if there
> is may logs.

The best thing to do would be to implement them properly. I'm not sure
that switching to an abort of any sort would be a good idea now that
we've pretended to implement them a bit, logging then RAZ/WI might be an
ok change.

Ian.

Re: [PATCH 05/10] xen/arm: vgic-v3: Document the current restrictions

[Julien Grall]

On 21/01/15 12:48, Ian Campbell wrote:
> On Wed, 2015-01-21 at 12:33 +0000, Julien Grall wrote:
>> On 21/01/15 12:16, Ian Campbell wrote:
>>> On Tue, 2015-01-20 at 17:49 +0000, Julien Grall wrote:
>>>
>>>>>>     - A processor can only access his own redistributor. For buggy
>>>>>>     assumption, the current code bank the redistributors MMIO.
>>>>>
>>>>> What assumption? It's not clear if you mean that a foreign redistributor
>>>>> should not be accessible and is, or if it should be accessible and
>>>>> isn't.
>>>>
>>>> Every redistributor (one per processor) are mapped in distinct MMIO region.
>>>>
>>>> Unlike the distributor, the redistributor is not banked.
>>>
>>> Understood.
>>>
>>>> Our current implementation (see vgic_v3_rdistr_mmio_write) consider that
>>>> the redistributor is banked and replicate n-times in the memory.
>>>
>>> IOW instead of having e.g. 8 individual redistributors each vcpu sees
>>> it's own redistributor 8 times. That does seem a bit dubious.
>>
>> It's the current behavior. You can see the difference in linux log. The
>> address of each redistributor is the same.
> 
> Are you sure that isn't the "redistributor region"? One of those can
> contain multiple redistributors, i.e. gic_v3_init sets up a single
> region and that propagates to the DTB given to the guest.
> 
> So the error is just in the mmio decode stage I think, not in the setup
> we are trying to achieve.

Right, the device tree is valid, we expose a region containing enough
space for up to 8 distributor.
The problem is in our MMIO emulation.

>>>> If you give a look to the redistributor iniatialization (see Xen and
>>>> Linux GICv3 code). The code will go through all the redistributors and
>>>> check GICR_TYPER to see if the processor is associated to this
>>>> redistributor.
>>>>
>>>> I'm not sure how the redistributor should behave if it's accessed by
>>>> another processor.
>>>
>>> Please can you find a spec reference and include it in the clarified
>>> version of this item.
>>
>> Rather than the distributor, there is multiple redistributor (one per
>> processor).
>>
>> I think the section 5.4.1 in the GICv3 should answer to this question:
>>
>> "Each re-distributor must be allocated at one page for controlling the
>> overall behavior of the re-distributor and for controlling physical
>> LPIs. The base address of this page is referred to as RD_base. In
>> addition, each re-distributor must be also allocated the following
>> additional pages".
> 
> That doesn't say anything about one CPU touching another's
> redistributor.

Linux is at least using GICR_TYPER, to retrieve the right distributor.
For the other the register I've no idea. I've asked ARM. Let see what
they will answer.

-- 
Julien Grall

Re: [PATCH 05/10] xen/arm: vgic-v3: Document the current restrictions

[Julien Grall]

Hi,

On 21/01/15 13:19, Julien Grall wrote:
>>>>> If you give a look to the redistributor iniatialization (see Xen and
>>>>> Linux GICv3 code). The code will go through all the redistributors and
>>>>> check GICR_TYPER to see if the processor is associated to this
>>>>> redistributor.
>>>>>
>>>>> I'm not sure how the redistributor should behave if it's accessed by
>>>>> another processor.
>>>>
>>>> Please can you find a spec reference and include it in the clarified
>>>> version of this item.
>>>
>>> Rather than the distributor, there is multiple redistributor (one per
>>> processor).
>>>
>>> I think the section 5.4.1 in the GICv3 should answer to this question:
>>>
>>> "Each re-distributor must be allocated at one page for controlling the
>>> overall behavior of the re-distributor and for controlling physical
>>> LPIs. The base address of this page is referred to as RD_base. In
>>> addition, each re-distributor must be also allocated the following
>>> additional pages".
>>
>> That doesn't say anything about one CPU touching another's
>> redistributor.
> 
> Linux is at least using GICR_TYPER, to retrieve the right distributor.
> For the other the register I've no idea. I've asked ARM. Let see what
> they will answer.

For documentation purpose, the re-distributor should be accessible by
any processor. It has been confirmed by ARM and I've found the paragraph
in spec stating:

"GICR_* registers. Multiple pages of registers per re-distributor
accessible to all processors." Section 4.3.5 of the r24 document.

I have a working implementation of this behavior. I will add it in the
v2 of this series and drop this patch.

Regards,

-- 
Julien Grall