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

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

[Julien Grall]

Hello,

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

It turns out to a bigger series because there were some outstanding bugs in the
vGIC emulation. The most important one is the way we emulate the re-distributor.
Each re-distributor should be associated to a single processor and have it's
own range (see patch #8).

The breakdown of this series is:
    * #1 - #8: vGICv3 fixes
    * #9: vGICv3 log improvement
    * #10 - #13: vGICv2 fixes
    * #14: Drop unused fields
    * #15: GICv3 doc improvement

Most of this patches should be backported to Xen 4.5/Xen 4.5 (see each patch).
Although, the one in GICv2 are not critical.

Changes since v2:
    - Sort re-distributor message
    - Typoes
    - Update/re-work commit messages

Changes since v1:
    - 2 patches of the series turn into an XSA 118 [1]
    - Correctly implement the re-distributor
    - Drop the documentation patch as I succedeed to quickly implement the
    re-distributor emulation
    - Replace " Don't check the size when we ignore the write/read as
    zero" patches by a new version to handle correctly RAZ/WI registers
    - Bunch of new patch to fix registers emulation

For each changes see in each patch.

A branch has been pushed for all the patches:
git://xenbits.xen.org/people/julieng/xen-unstable.git branch vgic-fixes-v3

Sincerely yours,

[1] http://xenbits.xen.org/xsa/advisory-118.html

Cc: Chen Baozi <baozich@gmail.com>

Julien Grall (15):
  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: Correctly handle RAZ/WI registers
  xen/arm: vgic-v3: Correctly implement read into GICR_NSACR
  xen/arm: vgic-v3: Set stride during domain initialization
  xen/arm: vgic-v3: Use a struct to describe contiguous rdist regions
  xen/arm: vgic-v3: Emulate correctly the re-distributor
  xen/arm: vgic-v3: Clarify which distributor is used in the common
    emulation
  xen/arm: vgic-v2: Correctly set GICD_TYPER.CPUNumber
  xen/arm: vgic-v2: Correctly handle RAZ/WI registers
  xen/arm: vgic-v2: Take the lock when writing into GICD_CTLR
  xen/arm: vgic-v2: GICD_I{S,C}PENDR* are only word-accessible
  xen/arm: vgic: Drop iactive, ipend, pendsgi field
  xen/arm: gic-v3: Update some comments in the code

 xen/arch/arm/gic-v3.c             |  56 +++--
 xen/arch/arm/vgic-v2.c            | 112 ++++------
 xen/arch/arm/vgic-v3.c            | 416 +++++++++++++++++++++++---------------
 xen/include/asm-arm/domain.h      |  17 +-
 xen/include/asm-arm/gic.h         |   1 +
 xen/include/asm-arm/gic_v3_defs.h |   3 +
 xen/include/asm-arm/vgic.h        |   2 +-
 7 files changed, 349 insertions(+), 258 deletions(-)

-- 
2.1.4

[PATCH v3 01/15] 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>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

---
    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.

    Changes in v3:
        - Move the GICD_TYPE_ID_BITS_SHIFT definition with the other bit
        definitions
        - Add Ian's ack

    Changes in v2:
         - vgic_num_irqs has been introduced
         - Drop GICD_TYPE_ID_BITS_MASK
         - Use get_count_order
---
 xen/arch/arm/vgic-v3.c            | 11 +++++++++++
 xen/include/asm-arm/gic_v3_defs.h |  3 +++
 2 files changed, 14 insertions(+)

diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index bece189..72b22ee 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -679,11 +679,22 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
         vgic_unlock(v);
         return 1;
     case GICD_TYPER:
+    {
+        /*
+         * Number of interrupt identifier bits supported by the GIC
+         * Stream Protocol Interface
+         */
+        unsigned int irq_bits = get_count_order(vgic_num_irqs(v->domain));
+
         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_spis / 32) & GICD_TYPE_LINES));
+
+        *r |= (irq_bits - 1) << GICD_TYPE_ID_BITS_SHIFT;
+
         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..b8a1c2e 100644
--- a/xen/include/asm-arm/gic_v3_defs.h
+++ b/xen/include/asm-arm/gic_v3_defs.h
@@ -45,6 +45,9 @@
 #define GICC_SRE_EL2_DIB             (1UL << 2)
 #define GICC_SRE_EL2_ENEL1           (1UL << 3)
 
+/* Additional bits in GICD_TYPER defined by GICv3 */
+#define GICD_TYPE_ID_BITS_SHIFT 19
+
 #define GICD_CTLR_RWP                (1UL << 31)
 #define GICD_CTLR_ARE_NS             (1U << 4)
 #define GICD_CTLR_ENABLE_G1A         (1U << 1)
-- 
2.1.4

[PATCH v3 02/15] 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>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
    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.

    Changes in v2:
        - Rebase after the change in patch #1 and the introduction of
        nr_spis/vgic_num_irqs.
        - Add Ian's ack. I keep the ack has the changes was only
        rebasing.
---
 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 72b22ee..e0a7d5b 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -685,10 +685,15 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
          * Stream Protocol Interface
          */
         unsigned int irq_bits = get_count_order(vgic_num_irqs(v->domain));
+        /*
+         * 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);
 
         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_spis / 32) & GICD_TYPE_LINES));
 
         *r |= (irq_bits - 1) << GICD_TYPE_ID_BITS_SHIFT;
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 v3 03/15] 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 is RAO/WI.

Furthermore, 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>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

---
    Changes in v3:
        - Add Ian's ack

    Changes in v2:
        - Clearly explain that ARE_NS is RAO/WI rather than imply the
        guest should set it.
        - Typoes
---
 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 e0a7d5b..9115199 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);
@@ -838,8 +844,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 */
@@ -1100,6 +1113,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 v3 04/15] xen/arm: vgic-v3: Correctly handle RAZ/WI registers

[Julien Grall]

Some of the registers are accessible via multiple size (see GICD_IPRIORITYR*).

Those registers are incorrectly implemented when they should be RAZ. Only
word-access size are currently allowed for them.

The paragraph 5.3.1 in the GICv3 spec (PRD03-GENC-010745 24.0) indicates
the different access-sizes supported for each register.

The current vGICv3 driver is not ready for 32 bits guest and will
require some rework. So, for now, only supporting access-size of a system not
supporting aarch32.

To avoid further issues, introduce different label following the access-size
of the registers:
    - read_as_zero_64 and write_ignore_64: Used for registers accessible
    via a double-word.
    - read_as_zero_32 and write_ignore_32: Used for registers accessible
    via a word.
    - read_as_zero: Used when we don't have to check the access size.

The latter is used when the access size has already been checked in the
register emulation and/or when the register offset is
reserved/implementation defined.

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

---
    This patch is candidate for backporting into Xen 4.5. It fixes
    access to GICD_IPRIORITYR* with byte-access when they are not
    implemented.

    Changes in v3:
        - Typoes in commit message
        - Add a reference to the paragraph 5.3.1
        - Explain why we only use access-sizes for system which doesn't
        support aarch32

    Changes in v2:
        - This patch replaces https://patches.linaro.org/43320/. A new
        approach has been taken to explicitly use the size in the goto
        label and have one version which don't check the access size. It's
        useful for reserved registers and register we already check the access
        size.
---
 xen/arch/arm/vgic-v3.c | 98 +++++++++++++++++++++++++++-----------------------
 1 file changed, 54 insertions(+), 44 deletions(-)

diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 9115199..1145972 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -101,7 +101,7 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info,
     {
     case GICR_CTLR:
         /* We have not implemented LPI's, read zero */
-        goto read_as_zero;
+        goto read_as_zero_32;
     case GICR_IIDR:
         if ( dabt.size != DABT_WORD ) goto bad_width;
         *r = GICV3_GICR_IIDR_VAL;
@@ -117,10 +117,10 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info,
         return 1;
     case GICR_STATUSR:
         /* Not implemented */
-        goto read_as_zero;
+        goto read_as_zero_32;
     case GICR_WAKER:
         /* Power management is not implemented */
-        goto read_as_zero;
+        goto read_as_zero_32;
     case GICR_SETLPIR:
         /* WO. Read as zero */
         goto read_as_zero_64;
@@ -165,14 +165,14 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info,
          return 1;
     case GICR_PIDR3:
         /* Manufacture/customer defined */
-        goto read_as_zero;
+        goto read_as_zero_32;
     case GICR_PIDR4:
         if ( dabt.size != DABT_WORD ) goto bad_width;
         *r = GICV3_GICR_PIDR4;
          return 1;
     case GICR_PIDR5 ... GICR_PIDR7:
         /* Reserved0 */
-        goto read_as_zero;
+        goto read_as_zero_32;
     default:
         printk(XENLOG_G_ERR
                "%pv: vGICR: read r%d offset %#08x\n not found",
@@ -190,7 +190,7 @@ read_as_zero_64:
     *r = 0;
     return 1;
 
-read_as_zero:
+read_as_zero_32:
     if ( dabt.size != DABT_WORD ) goto bad_width;
     *r = 0;
     return 1;
@@ -207,19 +207,19 @@ static int __vgic_v3_rdistr_rd_mmio_write(struct vcpu *v, mmio_info_t *info,
     {
     case GICR_CTLR:
         /* LPI's not implemented */
-        goto write_ignore;
+        goto write_ignore_32;
     case GICR_IIDR:
         /* RO */
-        goto write_ignore;
+        goto write_ignore_32;
     case GICR_TYPER:
         /* RO */
         goto write_ignore_64;
     case GICR_STATUSR:
         /* Not implemented */
-        goto write_ignore;
+        goto write_ignore_32;
     case GICR_WAKER:
         /* Power mgmt not implemented */
-        goto write_ignore;
+        goto write_ignore_32;
     case GICR_SETLPIR:
         /* LPI is not implemented */
         goto write_ignore_64;
@@ -240,7 +240,7 @@ static int __vgic_v3_rdistr_rd_mmio_write(struct vcpu *v, mmio_info_t *info,
         goto write_ignore_64;
     case GICR_SYNCR:
         /* RO */
-        goto write_ignore;
+        goto write_ignore_32;
     case GICR_MOVLPIR:
         /* LPI is not implemented */
         goto write_ignore_64;
@@ -249,7 +249,7 @@ static int __vgic_v3_rdistr_rd_mmio_write(struct vcpu *v, mmio_info_t *info,
         goto write_ignore_64;
     case GICR_PIDR7... GICR_PIDR0:
         /* RO */
-        goto write_ignore;
+        goto write_ignore_32;
     default:
         printk(XENLOG_G_ERR "%pv: vGICR: write r%d offset %#08x\n not found",
                v, dabt.reg, gicr_reg);
@@ -266,7 +266,7 @@ write_ignore_64:
     if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width;
     return 1;
 
-write_ignore:
+write_ignore_32:
     if ( dabt.size != DABT_WORD ) goto bad_width;
     return 1;
 }
@@ -284,6 +284,7 @@ static int __vgic_v3_distr_common_mmio_read(struct vcpu *v, mmio_info_t *info,
     {
     case GICD_IGROUPR ... GICD_IGROUPRN:
         /* We do not implement security extensions for guests, read zero */
+        if ( dabt.size != DABT_WORD ) goto bad_width;
         goto read_as_zero;
     case GICD_ISENABLER ... GICD_ISENABLERN:
         if ( dabt.size != DABT_WORD ) goto bad_width;
@@ -368,7 +369,6 @@ bad_width:
     return 0;
 
 read_as_zero:
-    if ( dabt.size != DABT_WORD ) goto bad_width;
     *r = 0;
     return 1;
 }
@@ -387,7 +387,7 @@ static int __vgic_v3_distr_common_mmio_write(struct vcpu *v, mmio_info_t *info,
     {
     case GICD_IGROUPR ... GICD_IGROUPRN:
         /* We do not implement security extensions for guests, write ignore */
-        goto write_ignore;
+        goto write_ignore_32;
     case GICD_ISENABLER ... GICD_ISENABLERN:
         if ( dabt.size != DABT_WORD ) goto bad_width;
         rank = vgic_rank_offset(v, 1, reg - GICD_ISENABLER, DABT_WORD);
@@ -456,7 +456,7 @@ static int __vgic_v3_distr_common_mmio_write(struct vcpu *v, mmio_info_t *info,
         vgic_unlock_rank(v, rank, flags);
         return 1;
     case GICD_ICFGR: /* Restricted to configure SGIs */
-        goto write_ignore;
+        goto write_ignore_32;
     case GICD_ICFGR + 4 ... GICD_ICFGRN: /* PPI + SPIs */
         /* ICFGR1 for PPI's, which is implementation defined
            if ICFGR1 is programmable or not. We chose to program */
@@ -481,8 +481,9 @@ bad_width:
     domain_crash_synchronous();
     return 0;
 
-write_ignore:
+write_ignore_32:
     if ( dabt.size != DABT_WORD ) goto bad_width;
+write_ignore:
     return 1;
 }
 
@@ -499,7 +500,7 @@ static int vgic_v3_rdistr_sgi_mmio_read(struct vcpu *v, mmio_info_t *info,
     {
     case GICR_IGRPMODR0:
         /* We do not implement security extensions for guests, read zero */
-        goto read_as_zero;
+        goto read_as_zero_32;
     case GICR_IGROUPR0:
     case GICR_ISENABLER0:
     case GICR_ICENABLER0:
@@ -543,8 +544,9 @@ bad_width:
     domain_crash_synchronous();
     return 0;
 
-read_as_zero:
+read_as_zero_32:
     if ( dabt.size != DABT_WORD ) goto bad_width;
+read_as_zero:
     *r = 0;
     return 1;
 }
@@ -562,7 +564,7 @@ static int vgic_v3_rdistr_sgi_mmio_write(struct vcpu *v, mmio_info_t *info,
     {
     case GICR_IGRPMODR0:
         /* We do not implement security extensions for guests, write ignore */
-        goto write_ignore;
+        goto write_ignore_32;
     case GICR_IGROUPR0:
     case GICR_ISENABLER0:
     case GICR_ICENABLER0:
@@ -595,7 +597,7 @@ static int vgic_v3_rdistr_sgi_mmio_write(struct vcpu *v, mmio_info_t *info,
         return 1;
     case GICR_NSACR:
         /* We do not implement security extensions for guests, write ignore */
-        goto write_ignore;
+        goto write_ignore_32;
     default:
         printk(XENLOG_G_ERR
                "%pv: vGICR: SGI: write r%d offset %#08x\n not found",
@@ -610,8 +612,9 @@ bad_width:
     domain_crash_synchronous();
     return 0;
 
-write_ignore:
+write_ignore_32:
     if ( dabt.size != DABT_WORD ) goto bad_width;
+write_ignore:
     return 1;
 }
 
@@ -711,7 +714,7 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
          *  Optional, Not implemented for now.
          *  Update to support guest debugging.
          */
-        goto read_as_zero;
+        goto read_as_zero_32;
     case GICD_IIDR:
         if ( dabt.size != DABT_WORD ) goto bad_width;
         *r = GICV3_GICD_IIDR_VAL;
@@ -719,7 +722,7 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
     case 0x020 ... 0x03c:
     case 0xc000 ... 0xffcc:
         /* Implementation defined -- read as zero */
-        goto read_as_zero;
+        goto read_as_zero_32;
     case GICD_IGROUPR ... GICD_IGROUPRN:
     case GICD_ISENABLER ... GICD_ISENABLERN:
     case GICD_ICENABLER ... GICD_ICENABLERN:
@@ -757,16 +760,16 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
         return 1;
     case GICD_NSACR ... GICD_NSACRN:
         /* We do not implement security extensions for guests, read zero */
-        goto read_as_zero;
+        goto read_as_zero_32;
     case GICD_SGIR:
         /* Read as ICH_SGIR system register with SRE set. So ignore */
-        goto read_as_zero;
+        goto read_as_zero_32;
     case GICD_CPENDSGIR ... GICD_CPENDSGIRN:
         /* Replaced with GICR_ICPENDR0. So ignore write */
-        goto read_as_zero;
+        goto read_as_zero_32;
     case GICD_SPENDSGIR ... GICD_SPENDSGIRN:
         /* Replaced with GICR_ISPENDR0. So ignore write */
-        goto read_as_zero;
+        goto read_as_zero_32;
     case GICD_PIDR0:
         /* GICv3 identification value */
         if ( dabt.size != DABT_WORD ) goto bad_width;
@@ -784,7 +787,7 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
         return 1;
     case GICD_PIDR3:
         /* GICv3 identification value. Manufacturer/Customer defined */
-        goto read_as_zero;
+        goto read_as_zero_32;
     case GICD_PIDR4:
         /* GICv3 identification value */
         if ( dabt.size != DABT_WORD ) goto bad_width;
@@ -792,7 +795,7 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
         return 1;
     case GICD_PIDR5 ... GICD_PIDR7:
         /* Reserved0 */
-        goto read_as_zero;
+        goto read_as_zero_32;
     case 0x00c:
     case 0x044:
     case 0x04c:
@@ -821,10 +824,14 @@ read_as_zero_64:
     *r = 0;
     return 1;
 
-read_as_zero:
+read_as_zero_32:
     if ( dabt.size != DABT_WORD ) goto bad_width;
     *r = 0;
     return 1;
+
+read_as_zero:
+    *r = 0;
+    return 1;
 }
 
 static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
@@ -856,32 +863,32 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
         return 1;
     case GICD_TYPER:
         /* RO -- write ignored */
-        goto write_ignore;
+        goto write_ignore_32;
     case GICD_IIDR:
         /* RO -- write ignored */
-        goto write_ignore;
+        goto write_ignore_32;
     case GICD_STATUSR:
         /* RO -- write ignored */
-        goto write_ignore;
+        goto write_ignore_32;
     case GICD_SETSPI_NSR:
         /* Message based SPI is not implemented */
-        goto write_ignore;
+        goto write_ignore_32;
     case GICD_CLRSPI_NSR:
         /* Message based SPI is not implemented */
-        goto write_ignore;
+        goto write_ignore_32;
     case GICD_SETSPI_SR:
         /* Message based SPI is not implemented */
-        goto write_ignore;
+        goto write_ignore_32;
     case GICD_CLRSPI_SR:
         /* Message based SPI is not implemented */
-        goto write_ignore;
+        goto write_ignore_32;
     case 0x020 ... 0x03c:
     case 0xc000 ... 0xffcc:
         /* Implementation defined -- write ignored */
         printk(XENLOG_G_DEBUG
                "%pv: vGICD: WI on implementation defined register offset %#08x\n",
                v, gicd_reg);
-        goto write_ignore;
+        goto write_ignore_32;
     case GICD_IGROUPR ... GICD_IGROUPRN:
     case GICD_ISENABLER ... GICD_ISENABLERN:
     case GICD_ICENABLER ... GICD_ICENABLERN:
@@ -901,7 +908,7 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
         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);
@@ -944,10 +951,10 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
         return 1;
     case GICD_NSACR ... GICD_NSACRN:
         /* We do not implement security extensions for guests, write ignore */
-        goto write_ignore;
+        goto write_ignore_32;
     case GICD_SGIR:
         /* it is accessed as system register in GICv3 */
-        goto write_ignore;
+        goto write_ignore_32;
     case GICD_CPENDSGIR ... GICD_CPENDSGIRN:
         /* Replaced with GICR_ICPENDR0. So ignore write */
         if ( dabt.size != DABT_WORD ) goto bad_width;
@@ -958,7 +965,7 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
         return 0;
     case GICD_PIDR7... GICD_PIDR0:
         /* RO -- write ignore */
-        goto write_ignore;
+        goto write_ignore_32;
     case 0x00c:
     case 0x044:
     case 0x04c:
@@ -984,13 +991,16 @@ bad_width:
     domain_crash_synchronous();
     return 0;
 
-write_ignore:
+write_ignore_32:
     if ( dabt.size != DABT_WORD ) goto bad_width;
     return 1;
 
 write_ignore_64:
     if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width;
     return 1;
+
+write_ignore:
+    return 1;
 }
 
 static int vgic_v3_to_sgi(struct vcpu *v, register_t sgir)
-- 
2.1.4

[PATCH v3 05/15] xen/arm: vgic-v3: Correctly implement read into GICR_NSACR

[Julien Grall]

The 32-bit register GICR_NSACR is RAZ/WI on non-secure state. Therefore
we should not inject a data abort to the guest.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

---
    This patch should be backport to Xen 4.5. The current implementation will
    inject a data abort into the guest.

    Changes in v3:
        - Add Ian's ack

    Changes in v2:
        - Patch added
---
 xen/arch/arm/vgic-v3.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 1145972..2c14717 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -530,8 +530,9 @@ static int vgic_v3_rdistr_sgi_mmio_read(struct vcpu *v, mmio_info_t *info,
         vgic_unlock_rank(v, rank, flags);
         return 1;
     case GICR_NSACR:
-        if ( dabt.size != DABT_WORD ) goto bad_width;
-        return 1;
+        /* We do not implement security extensions for guests, read zero */
+        goto read_as_zero_32;
+
     default:
         printk(XENLOG_G_ERR
                "%pv: vGICR: SGI: read r%d offset %#08x\n not found",
-- 
2.1.4

[PATCH v3 06/15] xen/arm: vgic-v3: Set stride during domain initialization

[Julien Grall]

The stride may not be set if the hardware GIC is using the default
layout. It happens on the Foundation model.

On GICv3, the default stride is 2 * 64K. Therefore it's possible to avoid
checking at every redistributor MMIO access if the stride is not set.

Because domU uses a static stride configuration this only happens for
dom0, so we can move this code in gicv_v3_init. Take the opportunity to move
the stride setting a bit earlier because the loop to set regions will require
the stride.

Also, use 2 * 64K rather than 128K and explain the reason.

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

---
    I wasn't not sure where to move this code. I find very confusion the
    splitting between vgic and gicv. Maybe we should introduce a
    hwdom_gicv_init and giccc_map callbacks. Then move most of the
    initialization in the vgic one.

    Changes in v3:
        - Fix typoes and update commit message
        - Forgot to remove the check on the stride the rdist write

    Changes in v2:
        - Patch added
---
 xen/arch/arm/gic-v3.c  | 11 ++++++++++-
 xen/arch/arm/vgic-v3.c | 12 ++----------
 2 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 47452ca..7b33ff7 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -897,12 +897,21 @@ static int gicv_v3_init(struct domain *d)
     {
         d->arch.vgic.dbase = gicv3.dbase;
         d->arch.vgic.dbase_size = gicv3.dbase_size;
+
+        d->arch.vgic.rdist_stride = gicv3.rdist_stride;
+        /*
+         * If the stride is not set, the default stride for GICv3 is 2 * 64K:
+         *     - first 64k page for Control and Physical LPIs
+         *     - second 64k page for Control and Generation of SGIs
+         */
+        if ( !d->arch.vgic.rdist_stride )
+            d->arch.vgic.rdist_stride = 2 * SZ_64K;
+
         for ( i = 0; i < gicv3.rdist_count; i++ )
         {
             d->arch.vgic.rbase[i] = gicv3.rdist_regions[i].base;
             d->arch.vgic.rbase_size[i] = gicv3.rdist_regions[i].size;
         }
-        d->arch.vgic.rdist_stride = gicv3.rdist_stride;
         d->arch.vgic.rdist_count = gicv3.rdist_count;
     }
     else
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 2c14717..c5a743a 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -625,11 +625,7 @@ static int vgic_v3_rdistr_mmio_read(struct vcpu *v, mmio_info_t *info)
 
     perfc_incr(vgicr_reads);
 
-    if ( v->domain->arch.vgic.rdist_stride != 0 )
-        offset = info->gpa & (v->domain->arch.vgic.rdist_stride - 1);
-    else
-        /* If stride is not set. Default 128K */
-        offset = info->gpa & (SZ_128K - 1);
+    offset = info->gpa & (v->domain->arch.vgic.rdist_stride - 1);
 
     if ( offset < SZ_64K )
         return __vgic_v3_rdistr_rd_mmio_read(v, info, offset);
@@ -649,11 +645,7 @@ static int vgic_v3_rdistr_mmio_write(struct vcpu *v, mmio_info_t *info)
 
     perfc_incr(vgicr_writes);
 
-    if ( v->domain->arch.vgic.rdist_stride != 0 )
-        offset = info->gpa & (v->domain->arch.vgic.rdist_stride - 1);
-    else
-        /* If stride is not set. Default 128K */
-        offset = info->gpa & (SZ_128K - 1);
+    offset = info->gpa & (v->domain->arch.vgic.rdist_stride - 1);
 
     if ( offset < SZ_64K )
         return __vgic_v3_rdistr_rd_mmio_write(v, info, offset);
-- 
2.1.4

[PATCH v3 07/15] xen/arm: vgic-v3: Use a struct to describe contiguous rdist regions

[Julien Grall]

Also update the different comment to make clear that we register one MMIO
region per contiguous regions and not per re-distributor.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

---
    Changes in v3:
        - Add Ian's ack

    Changes in v2:
        - Patch added
---
 xen/arch/arm/gic-v3.c        | 20 ++++++++++----------
 xen/arch/arm/vgic-v3.c       | 11 ++++++-----
 xen/include/asm-arm/domain.h | 11 +++++++----
 3 files changed, 23 insertions(+), 19 deletions(-)

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 7b33ff7..fdfda0b 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -909,10 +909,10 @@ static int gicv_v3_init(struct domain *d)
 
         for ( i = 0; i < gicv3.rdist_count; i++ )
         {
-            d->arch.vgic.rbase[i] = gicv3.rdist_regions[i].base;
-            d->arch.vgic.rbase_size[i] = gicv3.rdist_regions[i].size;
+            d->arch.vgic.rdist_regions[i].base = gicv3.rdist_regions[i].base;
+            d->arch.vgic.rdist_regions[i].size = gicv3.rdist_regions[i].size;
         }
-        d->arch.vgic.rdist_count = gicv3.rdist_count;
+        d->arch.vgic.nr_regions = gicv3.rdist_count;
     }
     else
     {
@@ -922,13 +922,13 @@ static int gicv_v3_init(struct domain *d)
         /* XXX: Only one Re-distributor region mapped for the guest */
         BUILD_BUG_ON(GUEST_GICV3_RDIST_REGIONS != 1);
 
-        d->arch.vgic.rdist_count = GUEST_GICV3_RDIST_REGIONS;
+        d->arch.vgic.nr_regions = GUEST_GICV3_RDIST_REGIONS;
         d->arch.vgic.rdist_stride = GUEST_GICV3_RDIST_STRIDE;
 
         /* The first redistributor should contain enough space for all CPUs */
         BUILD_BUG_ON((GUEST_GICV3_GICR0_SIZE / GUEST_GICV3_RDIST_STRIDE) < MAX_VIRT_CPUS);
-        d->arch.vgic.rbase[0] = GUEST_GICV3_GICR0_BASE;
-        d->arch.vgic.rbase_size[0] = GUEST_GICV3_GICR0_SIZE;
+        d->arch.vgic.rdist_regions[0].base = GUEST_GICV3_GICR0_BASE;
+        d->arch.vgic.rdist_regions[0].size = GUEST_GICV3_GICR0_SIZE;
     }
 
     return 0;
@@ -1107,7 +1107,7 @@ static int gicv3_make_dt_node(const struct domain *d,
      * CPU interface and virtual cpu interfaces accessesed as System registers
      * So cells are created only for Distributor and rdist regions
      */
-    len = len * (d->arch.vgic.rdist_count + 1);
+    len = len * (d->arch.vgic.nr_regions + 1);
     new_cells = xzalloc_bytes(len);
     if ( new_cells == NULL )
         return -FDT_ERR_XEN(ENOMEM);
@@ -1116,9 +1116,9 @@ static int gicv3_make_dt_node(const struct domain *d,
 
     dt_set_range(&tmp, node, d->arch.vgic.dbase, d->arch.vgic.dbase_size);
 
-    for ( i = 0; i < d->arch.vgic.rdist_count; i++ )
-        dt_set_range(&tmp, node, d->arch.vgic.rbase[i],
-                     d->arch.vgic.rbase_size[i]);
+    for ( i = 0; i < d->arch.vgic.nr_regions; i++ )
+        dt_set_range(&tmp, node, d->arch.vgic.rdist_regions[i].base,
+                     d->arch.vgic.rdist_regions[i].size);
 
     res = fdt_property(fdt, "reg", new_cells, len);
     xfree(new_cells);
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index c5a743a..1d0e52d 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -1108,13 +1108,14 @@ static int vgic_v3_domain_init(struct domain *d)
                           d->arch.vgic.dbase_size);
 
     /*
-     * Register mmio handler per redistributor region but not for
-     * every sgi rdist region which is per core.
-     * The redistributor region encompasses per core sgi region.
+     * Register mmio handler per contiguous region occupied by the
+     * redistributors. The handler will take care to choose which
+     * redistributor is targeted.
      */
-    for ( i = 0; i < d->arch.vgic.rdist_count; i++ )
+    for ( i = 0; i < d->arch.vgic.nr_regions; i++ )
         register_mmio_handler(d, &vgic_rdistr_mmio_handler,
-            d->arch.vgic.rbase[i], d->arch.vgic.rbase_size[i]);
+            d->arch.vgic.rdist_regions[i].base,
+            d->arch.vgic.rdist_regions[i].size);
 
     d->arch.vgic.ctlr = VGICD_CTLR_DEFAULT;
 
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 9018c6a..3eaa7f0 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -102,10 +102,13 @@ struct arch_domain
 #ifdef CONFIG_ARM_64
         /* GIC V3 addressing */
         paddr_t dbase_size; /* Distributor base size */
-        paddr_t rbase[MAX_RDIST_COUNT];      /* Re-Distributor base address */
-        paddr_t rbase_size[MAX_RDIST_COUNT]; /* Re-Distributor size */
-        uint32_t rdist_stride;               /* Re-Distributor stride */
-        int rdist_count;                     /* No. of Re-Distributors */
+        /* List of contiguous occupied by the redistributors */
+        struct vgic_rdist_region {
+            paddr_t base;                   /* Base address */
+            paddr_t size;                   /* Size */
+        } rdist_regions[MAX_RDIST_COUNT];
+        int nr_regions;                     /* Number of rdist regions */
+        uint32_t rdist_stride;              /* Re-Distributor stride */
 #endif
     } vgic;
 
-- 
2.1.4

[PATCH v3 08/15] xen/arm: vgic-v3: Emulate correctly the re-distributor

[Julien Grall]

There is a one-to-one mapping between each re-distributors and processors.
Each re-distributors can be accessed by any processor at any time. For
instance during the initialization of the GIC, the drivers will browse
the re-distributor to find the one associated to the current processor
(via GICR_TYPER). So each re-distributor has its own MMIO region.

The current implementation of the vGICv3 emulation assumes that the
re-distributor region is banked. Therefore, the processor won't be able
to access other re-distributor. While this is working fine for Linux, a
processor will only access GICR_TYPER to find the associated re-distributor,
we have to implement correctly the re-distributor emulation in order to boot
other operating systems.

All emulated registers of the re-distributors take a vCPU in parameter
and necessary lock. Therefore concurrent access is already properly handled.

The missing bit is retrieving the right vCPU following the region accessed.
Retrieving the right vCPU could be slow, so it has been divided in 2 paths:
    - fast path: The current vCPU is accessing its own re-distributor
    - slow path: The current vCPU is accessing another re-distributor

As the processor needs to initialize itself, the former case is very
common. To handle the access quickly, the base address of the
re-distributor is computed and stored per-vCPU during the vCPU initialization.

The latter is less common and more complicate to handle. The re-distributors
can be spread across multiple regions in the memory.

During the domain creation, Xen will browse those regions to find the first
vCPU handled by this region.

When an access hits the slow path, Xen will:
    1) Retrieve the region using the base address of the re-distributor
    accessed
    2) Find the vCPU ID attached to the redistributor
    3) Check the validity of the vCPU. If it's not valid, a data abort
    will be injected to the guest

Finally, this patch also correctly support the bit GICR_TYPER.LAST which
indicates if the redistributor is the last one of the contiguous region.

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

---
    Linux doesn't access the redistributor from another processor,
    except for GICR_TYPER during processor initialization. As it banks
    it will quickly get the "correct" redistributor. But ideally this should
    be backported to Xen 4.5.

    Changes in v3:
        - Typoes and update commit message
        - Clarify/remove some comments in the code
        - Sort the re-distributor regions.

    Changes in v2:
        - Patch added
---
 xen/arch/arm/gic-v3.c        | 24 ++++++++++-
 xen/arch/arm/vgic-v3.c       | 99 +++++++++++++++++++++++++++++++++++++++++++-
 xen/include/asm-arm/domain.h |  6 +++
 3 files changed, 126 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index fdfda0b..e7a7789 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -33,6 +33,7 @@
 #include <xen/device_tree.h>
 #include <xen/sizes.h>
 #include <xen/libfdt/libfdt.h>
+#include <xen/sort.h>
 #include <asm/p2m.h>
 #include <asm/domain.h>
 #include <asm/io.h>
@@ -895,6 +896,8 @@ static int gicv_v3_init(struct domain *d)
      */
     if ( is_hardware_domain(d) )
     {
+        unsigned int first_cpu = 0;
+
         d->arch.vgic.dbase = gicv3.dbase;
         d->arch.vgic.dbase_size = gicv3.dbase_size;
 
@@ -909,8 +912,15 @@ static int gicv_v3_init(struct domain *d)
 
         for ( i = 0; i < gicv3.rdist_count; i++ )
         {
+            paddr_t size = gicv3.rdist_regions[i].size;
+
             d->arch.vgic.rdist_regions[i].base = gicv3.rdist_regions[i].base;
-            d->arch.vgic.rdist_regions[i].size = gicv3.rdist_regions[i].size;
+            d->arch.vgic.rdist_regions[i].size = size;
+
+            /* Set the first CPU handled by this region */
+            d->arch.vgic.rdist_regions[i].first_cpu = first_cpu;
+
+            first_cpu += size / d->arch.vgic.rdist_stride;
         }
         d->arch.vgic.nr_regions = gicv3.rdist_count;
     }
@@ -929,6 +939,7 @@ static int gicv_v3_init(struct domain *d)
         BUILD_BUG_ON((GUEST_GICV3_GICR0_SIZE / GUEST_GICV3_RDIST_STRIDE) < MAX_VIRT_CPUS);
         d->arch.vgic.rdist_regions[0].base = GUEST_GICV3_GICR0_BASE;
         d->arch.vgic.rdist_regions[0].size = GUEST_GICV3_GICR0_SIZE;
+        d->arch.vgic.rdist_regions[0].first_cpu = 0;
     }
 
     return 0;
@@ -1173,6 +1184,14 @@ static const struct gic_hw_operations gicv3_ops = {
     .make_dt_node        = gicv3_make_dt_node,
 };
 
+static int __init cmp_rdist(const void *a, const void *b)
+{
+    const struct rdist_region *l = a, *r = a;
+
+    /* We assume that re-distributor regions can never overlap */
+    return ( l->base < r->base) ? -1 : 0;
+}
+
 /* Set up the GIC */
 static int __init gicv3_init(struct dt_device_node *node, const void *data)
 {
@@ -1228,6 +1247,9 @@ static int __init gicv3_init(struct dt_device_node *node, const void *data)
         rdist_regs[i].size = rdist_size;
     }
 
+    /* The vGIC code requires the region to be sorted */
+    sort(rdist_regs, gicv3.rdist_count, sizeof(*rdist_regs), cmp_rdist, NULL);
+
     /* If stride is not set in dt. Set default to 2 * SZ_64K */
     if ( !dt_property_read_u32(node, "redistributor-stride", &gicv3.rdist_stride) )
         gicv3.rdist_stride = 0;
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 1d0e52d..97249db 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -114,6 +114,10 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info,
                MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 1) << 40 |
                MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 0) << 32);
         *r = aff;
+
+        if ( v->arch.vgic.flags & VGIC_V3_RDIST_LAST )
+            *r |= GICR_TYPER_LAST;
+
         return 1;
     case GICR_STATUSR:
         /* Not implemented */
@@ -619,13 +623,56 @@ write_ignore:
     return 1;
 }
 
+static inline struct vcpu *get_vcpu_from_rdist(paddr_t gpa,
+                                               struct vcpu *v,
+                                               uint32_t *offset)
+{
+    struct domain *d = v->domain;
+    uint32_t stride = d->arch.vgic.rdist_stride;
+    paddr_t base;
+    int i, vcpu_id;
+    struct vgic_rdist_region *region;
+
+    *offset = gpa & (stride - 1);
+    base = gpa & ~((paddr_t)stride - 1);
+
+    /* Fast path: the VCPU is trying to access its re-distributor */
+    if ( likely(v->arch.vgic.rdist_base == base) )
+        return v;
+
+    /* Slow path: the VCPU is trying to access another re-distributor */
+
+    /*
+     * Find the region where the re-distributor lives. For this purpose,
+     * we look one region ahead as only MMIO range for redistributors
+     * traps here.
+     * Note: The region has been ordered during the GIC initialization
+     */
+    for ( i = 1; i < d->arch.vgic.nr_regions; i++ )
+    {
+        if ( base < d->arch.vgic.rdist_regions[i].base )
+            break;
+    }
+
+    region = &d->arch.vgic.rdist_regions[i - 1];
+
+    vcpu_id = region->first_cpu + ((base - region->base) / stride);
+
+    if ( unlikely(vcpu_id >= d->max_vcpus) )
+        return NULL;
+
+    return d->vcpu[vcpu_id];
+}
+
 static int vgic_v3_rdistr_mmio_read(struct vcpu *v, mmio_info_t *info)
 {
     uint32_t offset;
 
     perfc_incr(vgicr_reads);
 
-    offset = info->gpa & (v->domain->arch.vgic.rdist_stride - 1);
+    v = get_vcpu_from_rdist(info->gpa, v, &offset);
+    if ( unlikely(!v) )
+        return 0;
 
     if ( offset < SZ_64K )
         return __vgic_v3_rdistr_rd_mmio_read(v, info, offset);
@@ -645,7 +692,9 @@ static int vgic_v3_rdistr_mmio_write(struct vcpu *v, mmio_info_t *info)
 
     perfc_incr(vgicr_writes);
 
-    offset = info->gpa & (v->domain->arch.vgic.rdist_stride - 1);
+    v = get_vcpu_from_rdist(info->gpa, v, &offset);
+    if ( unlikely(!v) )
+        return 0;
 
     if ( offset < SZ_64K )
         return __vgic_v3_rdistr_rd_mmio_write(v, info, offset);
@@ -1080,6 +1129,13 @@ static int vgic_v3_vcpu_init(struct vcpu *v)
 {
     int i;
     uint64_t affinity;
+    paddr_t rdist_base;
+    struct vgic_rdist_region *region;
+    unsigned int last_cpu;
+
+    /* Convenient alias */
+    struct domain *d = v->domain;
+    uint32_t rdist_stride = d->arch.vgic.rdist_stride;
 
     /* For SGI and PPI the target is always this CPU */
     affinity = (MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 3) << 32 |
@@ -1090,6 +1146,45 @@ static int vgic_v3_vcpu_init(struct vcpu *v)
     for ( i = 0 ; i < 32 ; i++ )
         v->arch.vgic.private_irqs->v3.irouter[i] = affinity;
 
+    /*
+     * Find the region where the re-distributor lives. For this purpose,
+     * we look one region ahead as we have only the first CPU in hand.
+     */
+    for ( i = 1; i < d->arch.vgic.nr_regions; i++ )
+    {
+        if ( v->vcpu_id < d->arch.vgic.rdist_regions[i].first_cpu )
+            break;
+    }
+
+    region = &d->arch.vgic.rdist_regions[i - 1];
+
+    /* Get the base address of the redistributor */
+    rdist_base = region->base;
+    rdist_base += (v->vcpu_id - region->first_cpu) * rdist_stride;
+
+    /* Check if a valid region was found for the re-distributor */
+    if ( (rdist_base < region->base) ||
+         ((rdist_base + rdist_stride) > (region->base + region->size)) )
+    {
+        dprintk(XENLOG_ERR,
+                "d%u: Unable to find a re-distributor for VCPU %u\n",
+                d->domain_id, v->vcpu_id);
+        return -EINVAL;
+    }
+
+    v->arch.vgic.rdist_base = rdist_base;
+
+    /*
+     * If the redistributor is the last one of the
+     * contiguous region of the vCPU is the last of the domain, set
+     * VGIC_V3_RDIST_LAST flags.
+     * Note that we are assuming max_vcpus will never change.
+     */
+    last_cpu = (region->size / rdist_stride) + region->first_cpu - 1;
+
+    if ( v->vcpu_id == last_cpu || (v->vcpu_id == (d->max_vcpus - 1)) )
+        v->arch.vgic.flags |= VGIC_V3_RDIST_LAST;
+
     return 0;
 }
 
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 3eaa7f0..81e3185 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -106,6 +106,7 @@ struct arch_domain
         struct vgic_rdist_region {
             paddr_t base;                   /* Base address */
             paddr_t size;                   /* Size */
+            unsigned int first_cpu;         /* First CPU handled */
         } rdist_regions[MAX_RDIST_COUNT];
         int nr_regions;                     /* Number of rdist regions */
         uint32_t rdist_stride;              /* Re-Distributor stride */
@@ -239,6 +240,11 @@ struct arch_vcpu
          * lr_pending is a subset of vgic.inflight_irqs. */
         struct list_head lr_pending;
         spinlock_t lock;
+
+        /* GICv3: redistributor base and flags for this vCPU */
+        paddr_t rdist_base;
+#define VGIC_V3_RDIST_LAST  (1 << 0)        /* last vCPU of the rdist */
+        uint8_t flags;
     } vgic;
 
     /* Timer registers  */
-- 
2.1.4

[PATCH v3 09/15] xen/arm: vgic-v3: Clarify which distributor is used in the common emulation

[Julien Grall]

The messages in the common emulation doesn't specify which distributor
(redistributor or distributor) is used. This make difficult to find the
correct registers.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

---
    Changes in v3:
        - Add Ian's ack

    Changes in v2:
        - Patch added
---
 xen/arch/arm/vgic-v3.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 97249db..609e3c8 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -275,8 +275,8 @@ write_ignore_32:
     return 1;
 }
 
-static int __vgic_v3_distr_common_mmio_read(struct vcpu *v, mmio_info_t *info,
-                                            uint32_t reg)
+static int __vgic_v3_distr_common_mmio_read(const char *name, struct vcpu *v,
+                                            mmio_info_t *info, uint32_t reg)
 {
     struct hsr_dabt dabt = info->dabt;
     struct cpu_user_regs *regs = guest_cpu_user_regs();
@@ -360,15 +360,14 @@ static int __vgic_v3_distr_common_mmio_read(struct vcpu *v, mmio_info_t *info,
         return 1;
     default:
         printk(XENLOG_G_ERR
-               "%pv: vGICD/vGICR: unhandled read r%d offset %#08x\n",
-               v, dabt.reg, reg);
+               "%pv: %s: unhandled read r%d offset %#08x\n",
+               v, name, dabt.reg, reg);
         return 0;
     }
 
 bad_width:
-    printk(XENLOG_G_ERR
-           "%pv: vGICD/vGICR: bad read width %d r%d offset %#08x\n",
-           v, dabt.size, dabt.reg, reg);
+    printk(XENLOG_G_ERR "%pv: %s: bad read width %d r%d offset %#08x\n",
+           v, name, dabt.size, dabt.reg, reg);
     domain_crash_synchronous();
     return 0;
 
@@ -377,8 +376,8 @@ read_as_zero:
     return 1;
 }
 
-static int __vgic_v3_distr_common_mmio_write(struct vcpu *v, mmio_info_t *info,
-                                             uint32_t reg)
+static int __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v,
+                                             mmio_info_t *info, uint32_t reg)
 {
     struct hsr_dabt dabt = info->dabt;
     struct cpu_user_regs *regs = guest_cpu_user_regs();
@@ -473,15 +472,15 @@ static int __vgic_v3_distr_common_mmio_write(struct vcpu *v, mmio_info_t *info,
         return 1;
     default:
         printk(XENLOG_G_ERR
-               "%pv: vGICD/vGICR: unhandled write r%d=%"PRIregister" offset %#08x\n",
-               v, dabt.reg, *r, reg);
+               "%pv: %s: unhandled write r%d=%"PRIregister" offset %#08x\n",
+               v, name, dabt.reg, *r, reg);
         return 0;
     }
 
 bad_width:
     printk(XENLOG_G_ERR
-           "%pv: vGICD/vGICR: bad write width %d r%d=%"PRIregister" offset %#08x\n",
-           v, dabt.size, dabt.reg, *r, reg);
+           "%pv: %s: bad write width %d r%d=%"PRIregister" offset %#08x\n",
+           v, name, dabt.size, dabt.reg, *r, reg);
     domain_crash_synchronous();
     return 0;
 
@@ -516,7 +515,8 @@ static int vgic_v3_rdistr_sgi_mmio_read(struct vcpu *v, mmio_info_t *info,
           * Above registers offset are common with GICD.
           * So handle in common with GICD handling
           */
-        return __vgic_v3_distr_common_mmio_read(v, info, gicr_reg);
+        return __vgic_v3_distr_common_mmio_read("vGICR: SGI", v, info,
+                                                gicr_reg);
     case GICR_ISPENDR0:
         if ( dabt.size != DABT_WORD ) goto bad_width;
         rank = vgic_rank_offset(v, 1, gicr_reg - GICR_ISPENDR0, DABT_WORD);
@@ -581,7 +581,8 @@ static int vgic_v3_rdistr_sgi_mmio_write(struct vcpu *v, mmio_info_t *info,
           * Above registers offset are common with GICD.
           * So handle common with GICD handling
           */
-        return __vgic_v3_distr_common_mmio_write(v, info, gicr_reg);
+        return __vgic_v3_distr_common_mmio_write("vGICR: SGI", v,
+                                                 info, gicr_reg);
     case GICR_ISPENDR0:
         if ( dabt.size != DABT_WORD ) goto bad_width;
         rank = vgic_rank_offset(v, 1, gicr_reg - GICR_ISACTIVER0, DABT_WORD);
@@ -777,7 +778,7 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
          * Above all register are common with GICR and GICD
          * Manage in common
          */
-        return __vgic_v3_distr_common_mmio_read(v, info, gicd_reg);
+        return __vgic_v3_distr_common_mmio_read("vGICD", v, info, gicd_reg);
     case GICD_IROUTER ... GICD_IROUTER31:
         /* SGI/PPI is RES0 */
         goto read_as_zero_64;
@@ -942,7 +943,7 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
     case GICD_ICFGR ... GICD_ICFGRN:
         /* Above registers are common with GICR and GICD
          * Manage in common */
-        return __vgic_v3_distr_common_mmio_write(v, info, gicd_reg);
+        return __vgic_v3_distr_common_mmio_write("vGICD", v, info, gicd_reg);
     case GICD_IROUTER ... GICD_IROUTER31:
         /* SGI/PPI is RES0 */
         goto write_ignore_64;
-- 
2.1.4

[PATCH v3 10/15] 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 hardcoding the shift and remove useless mask.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

---
    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.

    Changes in v2:
        - Add Ian's ack
        - Fix typoes
---
 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 515faf7..5faef12 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_spis / 32)) & GICD_TYPE_LINES );
         vgic_unlock(v);
         return 1;
-- 
2.1.4

[PATCH v3 11/15] xen/arm: vgic-v2: Correctly handle RAZ/WI registers

[Julien Grall]

All the GICv2 registers are word-accessible. Some them are also
byte-accessible (see GICD_IPRIORITYR*).

Those registers are incorrectly implemented when they should be RAZ. Only
word-access size are currently allowed for them.

To avoid further issues, introduce different label following the access-size
of the registers:
    - read_as_zero_32 and write_ignore_32: Used for registers accessible
    via a word.
    - read_as_zero: Used when we don't have to check the access size.

The latter is used when the access size has already been checked in the
register emulation and/or when the register offset is reserved/implementation
defined.

Note that, only used labels has been introduced.

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.

    Changes in v3:
        - Update commit message

    Changes in v2:
        - This patch replaces https://patches.linaro.org/43318/. A new
        approach has been taken to explicitly use the size in the goto
        label and have one version which don't check the access size. It's
        useful for reserved registers and register we already check the access
        size.
---
 xen/arch/arm/vgic-v2.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index 5faef12..6530ecc 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -74,7 +74,7 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
 
     case GICD_IGROUPR ... GICD_IGROUPRN:
         /* We do not implement security extensions for guests, read zero */
-        goto read_as_zero;
+        goto read_as_zero_32;
 
     case GICD_ISENABLER ... GICD_ISENABLERN:
         if ( dabt.size != DABT_WORD ) goto bad_width;
@@ -166,7 +166,7 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
 
     case GICD_NSACR ... GICD_NSACRN:
         /* We do not implement security extensions for guests, read zero */
-        goto read_as_zero;
+        goto read_as_zero_32;
 
     case GICD_SGIR:
         if ( dabt.size != DABT_WORD ) goto bad_width;
@@ -226,8 +226,9 @@ bad_width:
     domain_crash_synchronous();
     return 0;
 
-read_as_zero:
+read_as_zero_32:
     if ( dabt.size != DABT_WORD ) goto bad_width;
+read_as_zero:
     *r = 0;
     return 1;
 }
@@ -286,7 +287,7 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
     /* R/O -- write ignored */
     case GICD_TYPER:
     case GICD_IIDR:
-        goto write_ignore;
+        goto write_ignore_32;
 
     /* Implementation defined -- write ignored */
     case 0x020 ... 0x03c:
@@ -294,7 +295,7 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
 
     case GICD_IGROUPR ... GICD_IGROUPRN:
         /* We do not implement security extensions for guests, write ignore */
-        goto write_ignore;
+        goto write_ignore_32;
 
     case GICD_ISENABLER ... GICD_ISENABLERN:
         if ( dabt.size != DABT_WORD ) goto bad_width;
@@ -360,7 +361,7 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
 
     case GICD_ITARGETSR ... GICD_ITARGETSR + 7:
         /* SGI/PPI target is read only */
-        goto write_ignore;
+        goto write_ignore_32;
 
     case GICD_ITARGETSR + 8 ... GICD_ITARGETSRN:
     {
@@ -433,10 +434,10 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
         return 1;
 
     case GICD_ICFGR: /* SGIs */
-        goto write_ignore;
+        goto write_ignore_32;
     case GICD_ICFGR + 1: /* PPIs */
         /* It is implementation defined if these are writeable. We chose not */
-        goto write_ignore;
+        goto write_ignore_32;
     case GICD_ICFGR + 2 ... GICD_ICFGRN: /* SPIs */
         if ( dabt.size != DABT_WORD ) goto bad_width;
         rank = vgic_rank_offset(v, 2, gicd_reg - GICD_ICFGR, DABT_WORD);
@@ -448,7 +449,7 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
 
     case GICD_NSACR ... GICD_NSACRN:
         /* We do not implement security extensions for guests, write ignore */
-        goto write_ignore;
+        goto write_ignore_32;
 
     case GICD_SGIR:
         if ( dabt.size != DABT_WORD ) goto bad_width;
@@ -474,7 +475,7 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
 
     /* R/O -- write ignore */
     case GICD_ICPIDR2:
-        goto write_ignore;
+        goto write_ignore_32;
 
     /* Implementation defined -- write ignored */
     case 0xfec ... 0xffc:
@@ -503,8 +504,9 @@ bad_width:
     domain_crash_synchronous();
     return 0;
 
-write_ignore:
+write_ignore_32:
     if ( dabt.size != DABT_WORD ) goto bad_width;
+write_ignore:
     return 1;
 }
 
-- 
2.1.4

[PATCH v3 12/15] 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>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

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

    Although, it won't apply directly for Xen 4.4.

    Changes in v2:
        - Add Ian's ack
---
 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 6530ecc..e7cdf9e 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 v3 13/15] xen/arm: vgic-v2: GICD_I{S, C}PENDR* are only word-accessible

[Julien Grall]

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

---
    This patch is candidate for backporting to Xen 4.4 and Xen 4.5.

    Although, this patch won't apply directly to Xen 4.4.

    Changes in v3:
        - Add Ian's ack

    Changes in v2:
        - Patch added
---
 xen/arch/arm/vgic-v2.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index e7cdf9e..1a02541 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -95,7 +95,7 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
         return 1;
 
     case GICD_ISPENDR ... GICD_ISPENDRN:
-        if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
+        if ( dabt.size != DABT_WORD ) goto bad_width;
         rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ISPENDR, DABT_WORD);
         if ( rank == NULL) goto read_as_zero;
         vgic_lock_rank(v, rank, flags);
@@ -104,8 +104,8 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
         return 1;
 
     case GICD_ICPENDR ... GICD_ICPENDRN:
-        if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
-        rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ICPENDR, DABT_WORD);
+        if ( dabt.size != DABT_WORD ) goto bad_width;
+        rank = vgic_rank_offset(v, 0, gicd_reg - GICD_ICPENDR, DABT_WORD);
         if ( rank == NULL) goto read_as_zero;
         vgic_lock_rank(v, rank, flags);
         *r = vgic_byte_read(rank->ipend, dabt.sign, gicd_reg);
@@ -331,17 +331,17 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
         return 1;
 
     case GICD_ISPENDR ... GICD_ISPENDRN:
-        if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
+        if ( dabt.size != DABT_WORD ) goto bad_width;
         printk(XENLOG_G_ERR
-               "%pv: vGICD: unhandled %s write %#"PRIregister" to ISPENDR%d\n",
-               v, dabt.size ? "word" : "byte", *r, gicd_reg - GICD_ISPENDR);
+               "%pv: vGICD: unhandled word write %#"PRIregister" to ISPENDR%d\n",
+               v, *r, gicd_reg - GICD_ISPENDR);
         return 0;
 
     case GICD_ICPENDR ... GICD_ICPENDRN:
-        if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
+        if ( dabt.size != DABT_WORD ) goto bad_width;
         printk(XENLOG_G_ERR
-               "%pv: vGICD: unhandled %s write %#"PRIregister" to ICPENDR%d\n",
-               v, dabt.size ? "word" : "byte", *r, gicd_reg - GICD_ICPENDR);
+               "%pv: vGICD: unhandled word write %#"PRIregister" to ICPENDR%d\n",
+               v, *r, gicd_reg - GICD_ICPENDR);
         return 0;
 
     case GICD_ISACTIVER ... GICD_ISACTIVERN:
-- 
2.1.4

[PATCH v3 14/15] xen/arm: vgic: Drop iactive, ipend, pendsgi field

[Julien Grall]

The current VGIC code doesn't support to change the pending and active status
of an IRQ via the (re-)distributor.

Futhermore, all the access size wasn't support correctly and some
registers was implemented as write-ignore. The latter make very
difficult for a kernel developer to find that we don't support R/W to
those registers.

Make the support consistent:
    - read will return 0 (RAZ)
    - write will print an error and inject a data abort to the guest

Also, those fields was never set and field such as ipend and pendsgi was
doing the same jobs.

Rather than wasting memory, we should better drop it. We could re-introduce
them if we need it when the support will be made.

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

---
    Changes in v3:
        - Re-work commit message

    Changes in v2:
        - Patch added
---
 xen/arch/arm/vgic-v2.c     |  71 +++++--------------------
 xen/arch/arm/vgic-v3.c     | 125 +++++++++++++++------------------------------
 xen/include/asm-arm/vgic.h |   2 +-
 3 files changed, 55 insertions(+), 143 deletions(-)

diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index 1a02541..3cf67a9 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -94,41 +94,15 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
         vgic_unlock_rank(v, rank, flags);
         return 1;
 
+    /* Read the pending status of an IRQ via GICD is not supported */
     case GICD_ISPENDR ... GICD_ISPENDRN:
-        if ( dabt.size != DABT_WORD ) goto bad_width;
-        rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ISPENDR, DABT_WORD);
-        if ( rank == NULL) goto read_as_zero;
-        vgic_lock_rank(v, rank, flags);
-        *r = vgic_byte_read(rank->ipend, dabt.sign, gicd_reg);
-        vgic_unlock_rank(v, rank, flags);
-        return 1;
-
     case GICD_ICPENDR ... GICD_ICPENDRN:
-        if ( dabt.size != DABT_WORD ) goto bad_width;
-        rank = vgic_rank_offset(v, 0, gicd_reg - GICD_ICPENDR, DABT_WORD);
-        if ( rank == NULL) goto read_as_zero;
-        vgic_lock_rank(v, rank, flags);
-        *r = vgic_byte_read(rank->ipend, dabt.sign, gicd_reg);
-        vgic_unlock_rank(v, rank, flags);
-        return 1;
+        goto read_as_zero;
 
+    /* Read the active status of an IRQ via GICD is not supported */
     case GICD_ISACTIVER ... GICD_ISACTIVERN:
-        if ( dabt.size != DABT_WORD ) goto bad_width;
-        rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ISACTIVER, DABT_WORD);
-        if ( rank == NULL) goto read_as_zero;
-        vgic_lock_rank(v, rank, flags);
-        *r = rank->iactive;
-        vgic_unlock_rank(v, rank, flags);
-        return 1;
-
     case GICD_ICACTIVER ... GICD_ICACTIVERN:
-        if ( dabt.size != DABT_WORD ) goto bad_width;
-        rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ICACTIVER, DABT_WORD);
-        if ( rank == NULL) goto read_as_zero;
-        vgic_lock_rank(v, rank, flags);
-        *r = rank->iactive;
-        vgic_unlock_rank(v, rank, flags);
-        return 1;
+        goto read_as_zero;
 
     case GICD_ITARGETSR ... GICD_ITARGETSRN:
         if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
@@ -174,23 +148,10 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
         *r = 0xdeadbeef;
         return 1;
 
+    /* Setting/Clearing the SGI pending bit via GICD is not supported */
     case GICD_CPENDSGIR ... GICD_CPENDSGIRN:
-        if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
-        rank = vgic_rank_offset(v, 1, gicd_reg - GICD_CPENDSGIR, DABT_WORD);
-        if ( rank == NULL) goto read_as_zero;
-        vgic_lock_rank(v, rank, flags);
-        *r = vgic_byte_read(rank->pendsgi, dabt.sign, gicd_reg);
-        vgic_unlock_rank(v, rank, flags);
-        return 1;
-
     case GICD_SPENDSGIR ... GICD_SPENDSGIRN:
-        if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
-        rank = vgic_rank_offset(v, 1, gicd_reg - GICD_SPENDSGIR, DABT_WORD);
-        if ( rank == NULL) goto read_as_zero;
-        vgic_lock_rank(v, rank, flags);
-        *r = vgic_byte_read(rank->pendsgi, dabt.sign, gicd_reg);
-        vgic_unlock_rank(v, rank, flags);
-        return 1;
+        goto read_as_zero;
 
     /* Implementation defined -- read as zero */
     case 0xfd0 ... 0xfe4:
@@ -346,21 +307,17 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
 
     case GICD_ISACTIVER ... GICD_ISACTIVERN:
         if ( dabt.size != DABT_WORD ) goto bad_width;
-        rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ISACTIVER, DABT_WORD);
-        if ( rank == NULL) goto write_ignore;
-        vgic_lock_rank(v, rank, flags);
-        rank->iactive &= ~*r;
-        vgic_unlock_rank(v, rank, flags);
-        return 1;
+        printk(XENLOG_G_ERR
+               "%pv: vGICD: unhandled word write %#"PRIregister" to ISACTIVER%d\n",
+               v, *r, gicd_reg - GICD_ISACTIVER);
+        return 0;
 
     case GICD_ICACTIVER ... GICD_ICACTIVERN:
         if ( dabt.size != DABT_WORD ) goto bad_width;
-        rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ICACTIVER, DABT_WORD);
-        if ( rank == NULL) goto write_ignore;
-        vgic_lock_rank(v, rank, flags);
-        rank->iactive &= ~*r;
-        vgic_unlock_rank(v, rank, flags);
-        return 1;
+        printk(XENLOG_ERR
+               "%pv: vGICD: unhandled word write %#"PRIregister" to ICACTIVER%d\n",
+               v, *r, gicd_reg - GICD_ICACTIVER);
+        return 0;
 
     case GICD_ITARGETSR ... GICD_ITARGETSR + 7:
         /* SGI/PPI target is read only */
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 609e3c8..c72eb6c 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -306,38 +306,16 @@ static int __vgic_v3_distr_common_mmio_read(const char *name, struct vcpu *v,
         *r = rank->ienable;
         vgic_unlock_rank(v, rank, flags);
         return 1;
+    /* Read the pending status of an IRQ via GICD/GICR is not supported */
     case GICD_ISPENDR ... GICD_ISPENDRN:
-        if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
-        rank = vgic_rank_offset(v, 1, reg - GICD_ISPENDR, DABT_WORD);
-        if ( rank == NULL ) goto read_as_zero;
-        vgic_lock_rank(v, rank, flags);
-        *r = vgic_byte_read(rank->ipend, dabt.sign, reg);
-        vgic_unlock_rank(v, rank, flags);
-        return 1;
     case GICD_ICPENDR ... GICD_ICPENDRN:
-        if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
-        rank = vgic_rank_offset(v, 1, reg - GICD_ICPENDR, DABT_WORD);
-        if ( rank == NULL ) goto read_as_zero;
-        vgic_lock_rank(v, rank, flags);
-        *r = vgic_byte_read(rank->ipend, dabt.sign, reg);
-        vgic_unlock_rank(v, rank, flags);
-        return 1;
+        goto read_as_zero;
+
+    /* Read the active status of an IRQ via GICD/GICR is not supported */
     case GICD_ISACTIVER ... GICD_ISACTIVERN:
-        if ( dabt.size != DABT_WORD ) goto bad_width;
-        rank = vgic_rank_offset(v, 1, reg - GICD_ISACTIVER, DABT_WORD);
-        if ( rank == NULL ) goto read_as_zero;
-        vgic_lock_rank(v, rank, flags);
-        *r = rank->iactive;
-        vgic_unlock_rank(v, rank, flags);
-        return 1;
     case GICD_ICACTIVER ... GICD_ICACTIVERN:
-        if ( dabt.size != DABT_WORD ) goto bad_width;
-        rank = vgic_rank_offset(v, 1, reg - GICD_ICACTIVER, DABT_WORD);
-        if ( rank == NULL ) goto read_as_zero;
-        vgic_lock_rank(v, rank, flags);
-        *r = rank->iactive;
-        vgic_unlock_rank(v, rank, flags);
-        return 1;
+        goto read_as_zero;
+
     case GICD_IPRIORITYR ... GICD_IPRIORITYRN:
         if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
         rank = vgic_rank_offset(v, 8, reg - GICD_IPRIORITYR, DABT_WORD);
@@ -415,36 +393,32 @@ static int __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v,
         return 1;
     case GICD_ISPENDR ... GICD_ISPENDRN:
         if ( dabt.size != DABT_WORD ) goto bad_width;
-        rank = vgic_rank_offset(v, 1, reg - GICD_ISPENDR, DABT_WORD);
-        if ( rank == NULL ) goto write_ignore;
-        vgic_lock_rank(v, rank, flags);
-        rank->ipend = *r;
-        vgic_unlock_rank(v, rank, flags);
-        return 1;
+        printk(XENLOG_G_ERR
+               "%pv: %s: unhandled word write %#"PRIregister" to ISPENDR%d\n",
+               v, name, *r, reg - GICD_ISPENDR);
+        return 0;
+
     case GICD_ICPENDR ... GICD_ICPENDRN:
         if ( dabt.size != DABT_WORD ) goto bad_width;
-        rank = vgic_rank_offset(v, 1, reg - GICD_ICPENDR, DABT_WORD);
-        if ( rank == NULL ) goto write_ignore;
-        vgic_lock_rank(v, rank, flags);
-        rank->ipend &= ~*r;
-        vgic_unlock_rank(v, rank, flags);
-        return 1;
+        printk(XENLOG_G_ERR
+               "%pv: %s: unhandled word write %#"PRIregister" to ICPENDR%d\n",
+               v, name, *r, reg - GICD_ICPENDR);
+        return 0;
+
     case GICD_ISACTIVER ... GICD_ISACTIVERN:
         if ( dabt.size != DABT_WORD ) goto bad_width;
-        rank = vgic_rank_offset(v, 1, reg - GICD_ISACTIVER, DABT_WORD);
-        if ( rank == NULL ) goto write_ignore;
-        vgic_lock_rank(v, rank, flags);
-        rank->iactive &= ~*r;
-        vgic_unlock_rank(v, rank, flags);
-        return 1;
+        printk(XENLOG_G_ERR
+               "%pv: %s: unhandled word write %#"PRIregister" to ISACTIVER%d\n",
+               v, name, *r, reg - GICD_ISACTIVER);
+        return 0;
+
     case GICD_ICACTIVER ... GICD_ICACTIVERN:
         if ( dabt.size != DABT_WORD ) goto bad_width;
-        rank = vgic_rank_offset(v, 1, reg - GICD_ICACTIVER, DABT_WORD);
-        if ( rank == NULL ) goto write_ignore;
-        vgic_lock_rank(v, rank, flags);
-        rank->iactive &= ~*r;
-        vgic_unlock_rank(v, rank, flags);
-        return 1;
+        printk(XENLOG_G_ERR
+               "%pv: %s: unhandled word write %#"PRIregister" to ICACTIVER%d\n",
+               v, name, *r, reg - GICD_ICACTIVER);
+        return 0;
+
     case GICD_IPRIORITYR ... GICD_IPRIORITYRN:
         if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
         rank = vgic_rank_offset(v, 8, reg - GICD_IPRIORITYR, DABT_WORD);
@@ -496,8 +470,6 @@ static int vgic_v3_rdistr_sgi_mmio_read(struct vcpu *v, mmio_info_t *info,
     struct hsr_dabt dabt = info->dabt;
     struct cpu_user_regs *regs = guest_cpu_user_regs();
     register_t *r = select_user_reg(regs, dabt.reg);
-    struct vgic_irq_rank *rank;
-    unsigned long flags;
 
     switch ( gicr_reg )
     {
@@ -517,22 +489,12 @@ static int vgic_v3_rdistr_sgi_mmio_read(struct vcpu *v, mmio_info_t *info,
           */
         return __vgic_v3_distr_common_mmio_read("vGICR: SGI", v, info,
                                                 gicr_reg);
+
+    /* Read the pending status of an SGI is via GICR is not supported */
     case GICR_ISPENDR0:
-        if ( dabt.size != DABT_WORD ) goto bad_width;
-        rank = vgic_rank_offset(v, 1, gicr_reg - GICR_ISPENDR0, DABT_WORD);
-        if ( rank == NULL ) goto read_as_zero;
-        vgic_lock_rank(v, rank, flags);
-        *r = rank->pendsgi;
-        vgic_unlock_rank(v, rank, flags);
-        return 1;
     case GICR_ICPENDR0:
-        if ( dabt.size != DABT_WORD ) goto bad_width;
-        rank = vgic_rank_offset(v, 1, gicr_reg - GICR_ICPENDR0, DABT_WORD);
-        if ( rank == NULL ) goto read_as_zero;
-        vgic_lock_rank(v, rank, flags);
-        *r = rank->pendsgi;
-        vgic_unlock_rank(v, rank, flags);
-        return 1;
+        goto read_as_zero;
+
     case GICR_NSACR:
         /* We do not implement security extensions for guests, read zero */
         goto read_as_zero_32;
@@ -562,8 +524,6 @@ static int vgic_v3_rdistr_sgi_mmio_write(struct vcpu *v, mmio_info_t *info,
     struct hsr_dabt dabt = info->dabt;
     struct cpu_user_regs *regs = guest_cpu_user_regs();
     register_t *r = select_user_reg(regs, dabt.reg);
-    struct vgic_irq_rank *rank;
-    unsigned long flags;
 
     switch ( gicr_reg )
     {
@@ -585,22 +545,18 @@ static int vgic_v3_rdistr_sgi_mmio_write(struct vcpu *v, mmio_info_t *info,
                                                  info, gicr_reg);
     case GICR_ISPENDR0:
         if ( dabt.size != DABT_WORD ) goto bad_width;
-        rank = vgic_rank_offset(v, 1, gicr_reg - GICR_ISACTIVER0, DABT_WORD);
-        if ( rank == NULL ) goto write_ignore;
-        vgic_lock_rank(v, rank, flags);
-        /* TODO: we just store the SGI pending status. Handle it properly */
-        rank->pendsgi |= *r;
-        vgic_unlock_rank(v, rank, flags);
-        return 1;
+        printk(XENLOG_G_ERR
+               "%pv: vGICR: SGI: unhandled word write %#"PRIregister" to ISPENDR0\n",
+               v, *r);
+        return 0;
+
     case GICR_ICPENDR0:
         if ( dabt.size != DABT_WORD ) goto bad_width;
-        rank = vgic_rank_offset(v, 1, gicr_reg - GICR_ISACTIVER0, DABT_WORD);
-        if ( rank == NULL ) goto write_ignore;
-        vgic_lock_rank(v, rank, flags);
-        /* TODO: we just store the SGI pending status. Handle it properly */
-        rank->pendsgi &= ~*r;
-        vgic_unlock_rank(v, rank, flags);
-        return 1;
+        printk(XENLOG_G_ERR
+               "%pv: vGICR: SGI: unhandled word write %#"PRIregister" to ICPENDR0\n",
+               v, *r);
+        return 0;
+
     case GICR_NSACR:
         /* We do not implement security extensions for guests, write ignore */
         goto write_ignore_32;
@@ -620,7 +576,6 @@ bad_width:
 
 write_ignore_32:
     if ( dabt.size != DABT_WORD ) goto bad_width;
-write_ignore:
     return 1;
 }
 
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 74d5a4e..0c7da7f 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -85,7 +85,7 @@ struct pending_irq
 /* Represents state corresponding to a block of 32 interrupts */
 struct vgic_irq_rank {
     spinlock_t lock; /* Covers access to all other members of this struct */
-    uint32_t ienable, iactive, ipend, pendsgi;
+    uint32_t ienable;
     uint32_t icfg[2];
     uint32_t ipriority[8];
     union {
-- 
2.1.4

[PATCH v3 15/15] xen/arm: gic-v3: Update some comments in the code

[Julien Grall]

    - Drop wrong comment about the default stride. It's not always 2 * SZ_64K.
    When the re-distributor support VLPIs (from GICv4), the default
    stride is 4 * SZ_64K

    - Explain why SZ_64K * 2

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

---
    Changes in v3:
        - Update commit message

    Changes in v2:
        - Patch added
---
 xen/arch/arm/gic-v3.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index e7a7789..41042ab 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -639,7 +639,7 @@ static int __init gicv3_populate_rdist(void)
                 ptr += gicv3.rdist_stride;
             else
             {
-                ptr += SZ_64K * 2;
+                ptr += SZ_64K * 2; /* Skip RD_base + SGI_base */
                 if ( typer & GICR_TYPER_VLPIS )
                     ptr += SZ_64K * 2; /* Skip VLPI_base + reserved page */
             }
@@ -1250,7 +1250,6 @@ static int __init gicv3_init(struct dt_device_node *node, const void *data)
     /* The vGIC code requires the region to be sorted */
     sort(rdist_regs, gicv3.rdist_count, sizeof(*rdist_regs), cmp_rdist, NULL);
 
-    /* If stride is not set in dt. Set default to 2 * SZ_64K */
     if ( !dt_property_read_u32(node, "redistributor-stride", &gicv3.rdist_stride) )
         gicv3.rdist_stride = 0;
 
-- 
2.1.4

Re: [PATCH v3 04/15] xen/arm: vgic-v3: Correctly handle RAZ/WI registers

[Ian Campbell]

On Mon, 2015-02-16 at 14:50 +0000, Julien Grall wrote:
> Some of the registers are accessible via multiple size (see GICD_IPRIORITYR*).
> 
> Those registers are incorrectly implemented when they should be RAZ. Only
> word-access size are currently allowed for them.
> 
> The paragraph 5.3.1 in the GICv3 spec (PRD03-GENC-010745 24.0) indicates
> the different access-sizes supported for each register.
> 
> The current vGICv3 driver is not ready for 32 bits guest and will
> require some rework. So, for now, only supporting access-size of a system not
> supporting aarch32.
> 
> To avoid further issues, introduce different label following the access-size
> of the registers:
>     - read_as_zero_64 and write_ignore_64: Used for registers accessible
>     via a double-word.
>     - read_as_zero_32 and write_ignore_32: Used for registers accessible
>     via a word.
>     - read_as_zero: Used when we don't have to check the access size.
> 
> The latter is used when the access size has already been checked in the
> register emulation and/or when the register offset is
> reserved/implementation defined.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

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

Re: [PATCH v3 06/15] xen/arm: vgic-v3: Set stride during domain initialization

[Ian Campbell]

On Mon, 2015-02-16 at 14:50 +0000, Julien Grall wrote:
> The stride may not be set if the hardware GIC is using the default
> layout. It happens on the Foundation model.
> 
> On GICv3, the default stride is 2 * 64K. Therefore it's possible to avoid
> checking at every redistributor MMIO access if the stride is not set.
> 
> Because domU uses a static stride configuration this only happens for
> dom0, so we can move this code in gicv_v3_init. Take the opportunity to move
> the stride setting a bit earlier because the loop to set regions will require
> the stride.
> 
> Also, use 2 * 64K rather than 128K and explain the reason.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

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

>     I wasn't not sure where to move this code. I find very confusion the
>     splitting between vgic and gicv. Maybe we should introduce a
>     hwdom_gicv_init and giccc_map callbacks. Then move most of the
>     initialization in the vgic one.

What is giccc? I'm not familiar enough with this code to rule on what
should go where. Perhaps Stefano has an opinion.

Ian.

Re: [PATCH v3 08/15] xen/arm: vgic-v3: Emulate correctly the re-distributor

[Ian Campbell]

On Mon, 2015-02-16 at 14:50 +0000, Julien Grall wrote:

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

Re: [PATCH v3 06/15] xen/arm: vgic-v3: Set stride during domain initialization

[Julien Grall]

On 19/02/15 15:58, Ian Campbell wrote:
> On Mon, 2015-02-16 at 14:50 +0000, Julien Grall wrote:
>> The stride may not be set if the hardware GIC is using the default
>> layout. It happens on the Foundation model.
>>
>> On GICv3, the default stride is 2 * 64K. Therefore it's possible to avoid
>> checking at every redistributor MMIO access if the stride is not set.
>>
>> Because domU uses a static stride configuration this only happens for
>> dom0, so we can move this code in gicv_v3_init. Take the opportunity to move
>> the stride setting a bit earlier because the loop to set regions will require
>> the stride.
>>
>> Also, use 2 * 64K rather than 128K and explain the reason.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
>>     I wasn't not sure where to move this code. I find very confusion the
>>     splitting between vgic and gicv. Maybe we should introduce a
>>     hwdom_gicv_init and giccc_map callbacks. Then move most of the
>>     initialization in the vgic one.
> 
> What is giccc? I'm not familiar enough with this code to rule on what
> should go where. Perhaps Stefano has an opinion.

I think the name of giccc what bad here. What I wanted to mean is
splitting the current gicv_setup code in 2 parts:
	- Mapping GICV to the guest
	- Set up the hwdom information

This will bring a better partition to the code and bring GICv2 on GICv3
support more quickly.

I will talk with Stefano about this possibility.

Regards,

-- 
Julien Grall

Re: [PATCH v3 11/15] xen/arm: vgic-v2: Correctly handle RAZ/WI registers

[Ian Campbell]

On Mon, 2015-02-16 at 14:50 +0000, Julien Grall wrote:
> All the GICv2 registers are word-accessible. Some them are also
> byte-accessible (see GICD_IPRIORITYR*).
> 
> Those registers are incorrectly implemented when they should be RAZ. Only
> word-access size are currently allowed for them.
> 
> To avoid further issues, introduce different label following the access-size
> of the registers:
>     - read_as_zero_32 and write_ignore_32: Used for registers accessible
>     via a word.
>     - read_as_zero: Used when we don't have to check the access size.
> 
> The latter is used when the access size has already been checked in the
> register emulation and/or when the register offset is reserved/implementation
> defined.
> 
> Note that, only used labels has been introduced.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

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

Re: [PATCH v3 14/15] xen/arm: vgic: Drop iactive, ipend, pendsgi field

[Ian Campbell]

On Mon, 2015-02-16 at 14:50 +0000, Julien Grall wrote:
> The current VGIC code doesn't support to change the pending and active status
> of an IRQ via the (re-)distributor.
> 
> Futhermore, all the access size wasn't support correctly and some
> registers was implemented as write-ignore. The latter make very
> difficult for a kernel developer to find that we don't support R/W to
> those registers.
> 
> Make the support consistent:
>     - read will return 0 (RAZ)
>     - write will print an error and inject a data abort to the guest
> 
> Also, those fields was never set and field such as ipend and pendsgi was
> doing the same jobs.
> 
> Rather than wasting memory, we should better drop it. We could re-introduce
> them if we need it when the support will be made.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
>  
>      case GICD_ICACTIVER ... GICD_ICACTIVERN:
>          if ( dabt.size != DABT_WORD ) goto bad_width;
> -        rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ICACTIVER, DABT_WORD);
> -        if ( rank == NULL) goto write_ignore;
> -        vgic_lock_rank(v, rank, flags);
> -        rank->iactive &= ~*r;
> -        vgic_unlock_rank(v, rank, flags);
> -        return 1;
> +        printk(XENLOG_ERR

I think you meant XENLOG_G_ERR here?

If that's the case then I can fix + ack as I commit.

Ian.

Re: [PATCH v3 15/15] xen/arm: gic-v3: Update some comments in the code

[Ian Campbell]

On Mon, 2015-02-16 at 14:50 +0000, Julien Grall wrote:
>     - Drop wrong comment about the default stride. It's not always 2 * SZ_64K.
>     When the re-distributor support VLPIs (from GICv4), the default
>     stride is 4 * SZ_64K
> 
>     - Explain why SZ_64K * 2
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

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

Re: [PATCH v3 14/15] xen/arm: vgic: Drop iactive, ipend, pendsgi field

[Julien Grall]

Hi Ian,

On 19/02/15 16:09, Ian Campbell wrote:
> On Mon, 2015-02-16 at 14:50 +0000, Julien Grall wrote:
>> The current VGIC code doesn't support to change the pending and active status
>> of an IRQ via the (re-)distributor.
>>
>> Futhermore, all the access size wasn't support correctly and some
>> registers was implemented as write-ignore. The latter make very
>> difficult for a kernel developer to find that we don't support R/W to
>> those registers.
>>
>> Make the support consistent:
>>     - read will return 0 (RAZ)
>>     - write will print an error and inject a data abort to the guest
>>
>> Also, those fields was never set and field such as ipend and pendsgi was
>> doing the same jobs.
>>
>> Rather than wasting memory, we should better drop it. We could re-introduce
>> them if we need it when the support will be made.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>
>>  
>>      case GICD_ICACTIVER ... GICD_ICACTIVERN:
>>          if ( dabt.size != DABT_WORD ) goto bad_width;
>> -        rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ICACTIVER, DABT_WORD);
>> -        if ( rank == NULL) goto write_ignore;
>> -        vgic_lock_rank(v, rank, flags);
>> -        rank->iactive &= ~*r;
>> -        vgic_unlock_rank(v, rank, flags);
>> -        return 1;
>> +        printk(XENLOG_ERR
> 
> I think you meant XENLOG_G_ERR here?

Whoops yes. I re-introduced XSA-118 by inadvertence.

> If that's the case then I can fix + ack as I commit.

Thanks,

-- 
Julien Grall

Re: [PATCH v3 00/15] xen/arm: Bug fixes for the vGIC

[Ian Campbell]

On Mon, 2015-02-16 at 14:50 +0000, Julien Grall wrote:

All applied, thanks.

> Most of this patches should be backported to Xen 4.5/Xen 4.5 (see each patch).
> Although, the one in GICv2 are not critical.

Do you have a handy index of which ones do/don't need backporting to
save me trawling through the mails?

Given the lack of GICv3 hardware on the market today I'm in two minds
about the backports to the gicv3 functionality, especially given the
number of patches involved and the amount of stuff they change. In
practical terms GICv2 support isn't much more than tech-preview in 4.5
anyway.

> 
> Changes since v2:
>     - Sort re-distributor message
>     - Typoes
>     - Update/re-work commit messages
> 
> Changes since v1:
>     - 2 patches of the series turn into an XSA 118 [1]
>     - Correctly implement the re-distributor
>     - Drop the documentation patch as I succedeed to quickly implement the
>     re-distributor emulation
>     - Replace " Don't check the size when we ignore the write/read as
>     zero" patches by a new version to handle correctly RAZ/WI registers
>     - Bunch of new patch to fix registers emulation
> 
> For each changes see in each patch.
> 
> A branch has been pushed for all the patches:
> git://xenbits.xen.org/people/julieng/xen-unstable.git branch vgic-fixes-v3
> 
> Sincerely yours,
> 
> [1] http://xenbits.xen.org/xsa/advisory-118.html
> 
> Cc: Chen Baozi <baozich@gmail.com>
> 
> Julien Grall (15):
>   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: Correctly handle RAZ/WI registers
>   xen/arm: vgic-v3: Correctly implement read into GICR_NSACR
>   xen/arm: vgic-v3: Set stride during domain initialization
>   xen/arm: vgic-v3: Use a struct to describe contiguous rdist regions
>   xen/arm: vgic-v3: Emulate correctly the re-distributor
>   xen/arm: vgic-v3: Clarify which distributor is used in the common
>     emulation
>   xen/arm: vgic-v2: Correctly set GICD_TYPER.CPUNumber
>   xen/arm: vgic-v2: Correctly handle RAZ/WI registers
>   xen/arm: vgic-v2: Take the lock when writing into GICD_CTLR
>   xen/arm: vgic-v2: GICD_I{S,C}PENDR* are only word-accessible
>   xen/arm: vgic: Drop iactive, ipend, pendsgi field
>   xen/arm: gic-v3: Update some comments in the code
> 
>  xen/arch/arm/gic-v3.c             |  56 +++--
>  xen/arch/arm/vgic-v2.c            | 112 ++++------
>  xen/arch/arm/vgic-v3.c            | 416 +++++++++++++++++++++++---------------
>  xen/include/asm-arm/domain.h      |  17 +-
>  xen/include/asm-arm/gic.h         |   1 +
>  xen/include/asm-arm/gic_v3_defs.h |   3 +
>  xen/include/asm-arm/vgic.h        |   2 +-
>  7 files changed, 349 insertions(+), 258 deletions(-)
> 

Re: [PATCH v3 00/15] xen/arm: Bug fixes for the vGIC

[Julien Grall]

On 19/02/15 17:21, Ian Campbell wrote:
> On Mon, 2015-02-16 at 14:50 +0000, Julien Grall wrote:
> 
> All applied, thanks.
> 
>> Most of this patches should be backported to Xen 4.5/Xen 4.5 (see each patch).
>> Although, the one in GICv2 are not critical.
> 
> Do you have a handy index of which ones do/don't need backporting to
> save me trawling through the mails?

The list of GICv2 patches candidate for backporting are: #10, #11, #12, #13.

For GICv3, see my comment below.

> Given the lack of GICv3 hardware on the market today I'm in two minds
> about the backports to the gicv3 functionality, especially given the
> number of patches involved and the amount of stuff they change. In
> practical terms GICv2 support isn't much more than tech-preview in 4.5
> anyway.

I guess you mean GICv3 for the last one?

I agree that there is no hardware available, but someone may want to use
the latest Xen release (currently 4.5) on their GICv3 internal board.

IHMO, GICv3 was not a tech-preview on Xen 4.5. At least we never clearly
say it was. If it's the case we should write-down on the features list.

Regards,

-- 
Julien Grall

Re: [PATCH v3 00/15] xen/arm: Bug fixes for the vGIC

[Ian Campbell]

On Thu, 2015-02-19 at 17:34 +0000, Julien Grall wrote:
> On 19/02/15 17:21, Ian Campbell wrote:
> > On Mon, 2015-02-16 at 14:50 +0000, Julien Grall wrote:
> > 
> > All applied, thanks.
> > 
> >> Most of this patches should be backported to Xen 4.5/Xen 4.5 (see each patch).
> >> Although, the one in GICv2 are not critical.
> > 
> > Do you have a handy index of which ones do/don't need backporting to
> > save me trawling through the mails?
> 
> The list of GICv2 patches candidate for backporting are: #10, #11, #12, #13.
> 
> For GICv3, see my comment below.
> 
> > Given the lack of GICv3 hardware on the market today I'm in two minds
> > about the backports to the gicv3 functionality, especially given the
> > number of patches involved and the amount of stuff they change. In
> > practical terms GICv2 support isn't much more than tech-preview in 4.5
> > anyway.
> 
> I guess you mean GICv3 for the last one?

Yes.

> I agree that there is no hardware available, but someone may want to use
> the latest Xen release (currently 4.5) on their GICv3 internal board.

For new hardware people should be strongly advised to use the latest
version of Xen, not least because that is where their platform patches
need to be based and because that is where all the bleeding edge h/w
enablement is happening.

> IHMO, GICv3 was not a tech-preview on Xen 4.5. At least we never clearly
> say it was. If it's the case we should write-down on the features list.

Whether we said so or not it is clearly the case, in practical terms,
that the support is not production quality in 4.5. The question is then
whether we are willing to accept large numbers of backports trying to
remedy the situation in a stable release.

I'm willing to try applying the backport once these changes are through
staging, but at the first sign of git cherry-pick asking me to resolve a
non-trivial conflict I'd be inclined to declare that a sign that this
series is too big and/or intrusive to be considering for a backport.

Ian.

Re: [PATCH v3 00/15] xen/arm: Bug fixes for the vGIC

[Julien Grall]

On 19/02/15 17:48, Ian Campbell wrote:
> On Thu, 2015-02-19 at 17:34 +0000, Julien Grall wrote:
>> On 19/02/15 17:21, Ian Campbell wrote:
>>> On Mon, 2015-02-16 at 14:50 +0000, Julien Grall wrote:
>>>
>>> All applied, thanks.
>>>
>>>> Most of this patches should be backported to Xen 4.5/Xen 4.5 (see each patch).
>>>> Although, the one in GICv2 are not critical.
>>>
>>> Do you have a handy index of which ones do/don't need backporting to
>>> save me trawling through the mails?
>>
>> The list of GICv2 patches candidate for backporting are: #10, #11, #12, #13.
>>
>> For GICv3, see my comment below.
>>
>>> Given the lack of GICv3 hardware on the market today I'm in two minds
>>> about the backports to the gicv3 functionality, especially given the
>>> number of patches involved and the amount of stuff they change. In
>>> practical terms GICv2 support isn't much more than tech-preview in 4.5
>>> anyway.
>>
>> I guess you mean GICv3 for the last one?
> 
> Yes.
> 
>> I agree that there is no hardware available, but someone may want to use
>> the latest Xen release (currently 4.5) on their GICv3 internal board.
> 
> For new hardware people should be strongly advised to use the latest
> version of Xen, not least because that is where their platform patches
> need to be based and because that is where all the bleeding edge h/w
> enablement is happening.

You may want to use a release version in order to make a product. Xen
4.5 should be able to boot on any platform, supposing you wrote the
platform file. Anyway, it's a matter of taste for this one.

>> IHMO, GICv3 was not a tech-preview on Xen 4.5. At least we never clearly
>> say it was. If it's the case we should write-down on the features list.
> 
> Whether we said so or not it is clearly the case, in practical terms,
> that the support is not production quality in 4.5. The question is then
> whether we are willing to accept large numbers of backports trying to
> remedy the situation in a stable release.
> 
> I'm willing to try applying the backport once these changes are through
> staging, but at the first sign of git cherry-pick asking me to resolve a
> non-trivial conflict I'd be inclined to declare that a sign that this
> series is too big and/or intrusive to be considering for a backport.

AFAICT we don't have any push-gate for GICv3 code. It would be nice to
have one at some point.

For the conflict, we did some renaming on Xen 4.5. Which make the
backport more tricky.


Based on the discussion, what about waiting until someone complain about
GICv3 support on Xen 4.5?

Regards,

-- 
Julien Grall

Re: [PATCH v3 00/15] xen/arm: Bug fixes for the vGIC

[Ian Campbell]

On Thu, 2015-02-19 at 18:01 +0000, Julien Grall wrote:
> Based on the discussion, what about waiting until someone complain about
> GICv3 support on Xen 4.5?

That sounds like a reasonable compromise.

My potential backports list now contains:

xen/arm: Bug fixes for the vGIC
-> 15 patch series, many of which are tagged for backport.

 -> Julien: The list of GICv2 patches candidate for backporting are: #10, #11,
    #12, #13.
fed3569 xen/arm: vgic-v2: Correctly set GICD_TYPER.CPUNumber
1fefa55 xen/arm: vgic-v2: Correctly handle RAZ/WI registers
26ea82f xen/arm: vgic-v2: Take the lock when writing into GICD_CTLR
10af92d xen/arm: vgic-v2: GICD_I{S, C}PENDR* are only word-accessible

 -> Julien: Based on the discussion, what about waiting until someone complain about
    GICv3 support on Xen 4.5?
8206d05 xen/arm: vgic-v3: Correctly set GICD_TYPER.IDbits
834551b xen/arm: vgic-v3: Correctly set GICD_TYPER.CPUNumber
5e6958a xen/arm: vgic-v3: Correctly handle RAZ/WI registers
cfef895 xen/arm: vgic-v3: Correctly implement read into GICR_NSACR
acf65e5 xen/arm: vgic-v3: Emulate correctly the re-distributor

 -> Not tagged for backport:
8d91d64 xen/arm: vgic-v3: Correctly handle GICD_CTLR
4636e9f xen/arm: vgic-v3: Set stride during domain initialization
196f4ef xen/arm: vgic-v3: Use a struct to describe contiguous rdist regions
b697071 xen/arm: vgic-v3: Clarify which distributor is used in the common emulation
a41d809 xen/arm: vgic: Drop iactive, ipend, pendsgi field
98e7c60 xen/arm: gic-v3: Update some comments in the code

Does that tally with what you intended?

Ian.

Re: [PATCH v3 00/15] xen/arm: Bug fixes for the vGIC

[Vijay Kilari]

On Fri, Feb 20, 2015 at 3:44 PM, Ian Campbell <ian.campbell@citrix.com> wrote:
> On Thu, 2015-02-19 at 18:01 +0000, Julien Grall wrote:
>> Based on the discussion, what about waiting until someone complain about
>> GICv3 support on Xen 4.5?
>
> That sounds like a reasonable compromise.

If Xen 4.5 claims GICv3 support, then it is better to backport vgic-v3 patches.
May be ThunderX is the only platform out there with GICv3 support.
At some point of time we might require ThunderX support on 4.5 release.
If you are going to backport only on-demand then it should be for now.

>
> My potential backports list now contains:
>
> xen/arm: Bug fixes for the vGIC
> -> 15 patch series, many of which are tagged for backport.
>
>  -> Julien: The list of GICv2 patches candidate for backporting are: #10, #11,
>     #12, #13.
> fed3569 xen/arm: vgic-v2: Correctly set GICD_TYPER.CPUNumber
> 1fefa55 xen/arm: vgic-v2: Correctly handle RAZ/WI registers
> 26ea82f xen/arm: vgic-v2: Take the lock when writing into GICD_CTLR
> 10af92d xen/arm: vgic-v2: GICD_I{S, C}PENDR* are only word-accessible
>
>  -> Julien: Based on the discussion, what about waiting until someone complain about
>     GICv3 support on Xen 4.5?
> 8206d05 xen/arm: vgic-v3: Correctly set GICD_TYPER.IDbits
> 834551b xen/arm: vgic-v3: Correctly set GICD_TYPER.CPUNumber
> 5e6958a xen/arm: vgic-v3: Correctly handle RAZ/WI registers
> cfef895 xen/arm: vgic-v3: Correctly implement read into GICR_NSACR
> acf65e5 xen/arm: vgic-v3: Emulate correctly the re-distributor
>
>  -> Not tagged for backport:
> 8d91d64 xen/arm: vgic-v3: Correctly handle GICD_CTLR
> 4636e9f xen/arm: vgic-v3: Set stride during domain initialization
> 196f4ef xen/arm: vgic-v3: Use a struct to describe contiguous rdist regions
> b697071 xen/arm: vgic-v3: Clarify which distributor is used in the common emulation
> a41d809 xen/arm: vgic: Drop iactive, ipend, pendsgi field
> 98e7c60 xen/arm: gic-v3: Update some comments in the code
>
> Does that tally with what you intended?
>
> Ian.
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH v3 00/15] xen/arm: Bug fixes for the vGIC

[Ian Campbell]

On Fri, 2015-02-20 at 15:56 +0530, Vijay Kilari wrote:
> On Fri, Feb 20, 2015 at 3:44 PM, Ian Campbell <ian.campbell@citrix.com> wrote:
> > On Thu, 2015-02-19 at 18:01 +0000, Julien Grall wrote:
> >> Based on the discussion, what about waiting until someone complain about
> >> GICv3 support on Xen 4.5?
> >
> > That sounds like a reasonable compromise.
> 
> If Xen 4.5 claims GICv3 support, then it is better to backport vgic-v3 patches.
> May be ThunderX is the only platform out there with GICv3 support.
> At some point of time we might require ThunderX support on 4.5 release.
> If you are going to backport only on-demand then it should be for now.

Is ThunderX going to be available to purchase before the 4.6 release in
the summer?

> 
> >
> > My potential backports list now contains:
> >
> > xen/arm: Bug fixes for the vGIC
> > -> 15 patch series, many of which are tagged for backport.
> >
> >  -> Julien: The list of GICv2 patches candidate for backporting are: #10, #11,
> >     #12, #13.
> > fed3569 xen/arm: vgic-v2: Correctly set GICD_TYPER.CPUNumber
> > 1fefa55 xen/arm: vgic-v2: Correctly handle RAZ/WI registers
> > 26ea82f xen/arm: vgic-v2: Take the lock when writing into GICD_CTLR
> > 10af92d xen/arm: vgic-v2: GICD_I{S, C}PENDR* are only word-accessible
> >
> >  -> Julien: Based on the discussion, what about waiting until someone complain about
> >     GICv3 support on Xen 4.5?
> > 8206d05 xen/arm: vgic-v3: Correctly set GICD_TYPER.IDbits
> > 834551b xen/arm: vgic-v3: Correctly set GICD_TYPER.CPUNumber
> > 5e6958a xen/arm: vgic-v3: Correctly handle RAZ/WI registers
> > cfef895 xen/arm: vgic-v3: Correctly implement read into GICR_NSACR
> > acf65e5 xen/arm: vgic-v3: Emulate correctly the re-distributor
> >
> >  -> Not tagged for backport:
> > 8d91d64 xen/arm: vgic-v3: Correctly handle GICD_CTLR
> > 4636e9f xen/arm: vgic-v3: Set stride during domain initialization
> > 196f4ef xen/arm: vgic-v3: Use a struct to describe contiguous rdist regions
> > b697071 xen/arm: vgic-v3: Clarify which distributor is used in the common emulation
> > a41d809 xen/arm: vgic: Drop iactive, ipend, pendsgi field
> > 98e7c60 xen/arm: gic-v3: Update some comments in the code
> >
> > Does that tally with what you intended?
> >
> > Ian.
> >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel

Re: [PATCH v3 00/15] xen/arm: Bug fixes for the vGIC

[Julien Grall]

On 20/02/15 10:44, Ian Campbell wrote:
> On Fri, 2015-02-20 at 15:56 +0530, Vijay Kilari wrote:
>> On Fri, Feb 20, 2015 at 3:44 PM, Ian Campbell <ian.campbell@citrix.com> wrote:
>>> On Thu, 2015-02-19 at 18:01 +0000, Julien Grall wrote:
>>>> Based on the discussion, what about waiting until someone complain about
>>>> GICv3 support on Xen 4.5?
>>>
>>> That sounds like a reasonable compromise.
>>
>> If Xen 4.5 claims GICv3 support, then it is better to backport vgic-v3 patches.
>> May be ThunderX is the only platform out there with GICv3 support.
>> At some point of time we might require ThunderX support on 4.5 release.
>> If you are going to backport only on-demand then it should be for now.
> 
> Is ThunderX going to be available to purchase before the 4.6 release in
> the summer?

Even if the GICv3 patches backported, there is no GICv3 ITS, SMMUv2 and
48 cores supports in Xen 4.5. That would mean that there is no MSI
neither PCI-passthrough working.

Given that, there is not much interest to have Xen 4.5 running on
ThunderX. You won't be able to use the full capacity of the board.

I think we should focus on upstreaming all the features required in Xen
4.6 as soon as possible.

Regards,

-- 
Julien Grall

Re: [PATCH v3 00/15] xen/arm: Bug fixes for the vGIC

[Julien Grall]

Hi Ian,

On 20/02/15 10:14, Ian Campbell wrote:
> On Thu, 2015-02-19 at 18:01 +0000, Julien Grall wrote:
>> Based on the discussion, what about waiting until someone complain about
>> GICv3 support on Xen 4.5?
> 
> That sounds like a reasonable compromise.
> 
> My potential backports list now contains:
> 
> xen/arm: Bug fixes for the vGIC
> -> 15 patch series, many of which are tagged for backport.
> 
>  -> Julien: The list of GICv2 patches candidate for backporting are: #10, #11,
>     #12, #13.
> fed3569 xen/arm: vgic-v2: Correctly set GICD_TYPER.CPUNumber
> 1fefa55 xen/arm: vgic-v2: Correctly handle RAZ/WI registers
> 26ea82f xen/arm: vgic-v2: Take the lock when writing into GICD_CTLR
> 10af92d xen/arm: vgic-v2: GICD_I{S, C}PENDR* are only word-accessible
> 
>  -> Julien: Based on the discussion, what about waiting until someone complain about
>     GICv3 support on Xen 4.5?
> 8206d05 xen/arm: vgic-v3: Correctly set GICD_TYPER.IDbits
> 834551b xen/arm: vgic-v3: Correctly set GICD_TYPER.CPUNumber
> 5e6958a xen/arm: vgic-v3: Correctly handle RAZ/WI registers
> cfef895 xen/arm: vgic-v3: Correctly implement read into GICR_NSACR
> acf65e5 xen/arm: vgic-v3: Emulate correctly the re-distributor

The re-distributor patch required 196f4ef and 4636e9f.

>  -> Not tagged for backport:
> 8d91d64 xen/arm: vgic-v3: Correctly handle GICD_CTLR

Hmmm... I'm not sure why I didn't request a backport for this one. It
looks like the guest may read it to see what is enabled.

> 4636e9f xen/arm: vgic-v3: Set stride during domain initialization
> 196f4ef xen/arm: vgic-v3: Use a struct to describe contiguous rdist regions
> b697071 xen/arm: vgic-v3: Clarify which distributor is used in the common emulation
> a41d809 xen/arm: vgic: Drop iactive, ipend, pendsgi field
> 98e7c60 xen/arm: gic-v3: Update some comments in the code

Regards,

-- 
Julien Grall

Re: [PATCH v3 00/15] xen/arm: Bug fixes for the vGIC

[Vijay Kilari]

On Fri, Feb 20, 2015 at 4:45 PM, Julien Grall <julien.grall@linaro.org> wrote:
> On 20/02/15 10:44, Ian Campbell wrote:
>> On Fri, 2015-02-20 at 15:56 +0530, Vijay Kilari wrote:
>>> On Fri, Feb 20, 2015 at 3:44 PM, Ian Campbell <ian.campbell@citrix.com> wrote:
>>>> On Thu, 2015-02-19 at 18:01 +0000, Julien Grall wrote:
>>>>> Based on the discussion, what about waiting until someone complain about
>>>>> GICv3 support on Xen 4.5?
>>>>
>>>> That sounds like a reasonable compromise.
>>>
>>> If Xen 4.5 claims GICv3 support, then it is better to backport vgic-v3 patches.
>>> May be ThunderX is the only platform out there with GICv3 support.
>>> At some point of time we might require ThunderX support on 4.5 release.
>>> If you are going to backport only on-demand then it should be for now.
>>
>> Is ThunderX going to be available to purchase before the 4.6 release in
>> the summer?

Yes, ThunderX is in sampling now and customers can get them directly

>
> Even if the GICv3 patches backported, there is no GICv3 ITS, SMMUv2 and
> 48 cores supports in Xen 4.5. That would mean that there is no MSI
> neither PCI-passthrough working.
>
> Given that, there is not much interest to have Xen 4.5 running on
> ThunderX. You won't be able to use the full capacity of the board.
>
> I think we should focus on upstreaming all the features required in Xen
> 4.6 as soon as possible.

Yes we are targeting 4.6 release. On request we can deliver 4.5 based solution
for Thunderx which we already have prototyped