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

[PATCH v2 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:
    - 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.

This series depends on XSA 118 [1] which has not been commited yet.

Sincerely yours,

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


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             |  44 ++--
 xen/arch/arm/vgic-v2.c            | 112 ++++------
 xen/arch/arm/vgic-v3.c            | 424 +++++++++++++++++++++++---------------
 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, 345 insertions(+), 258 deletions(-)

-- 
2.1.4

[PATCH v2 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>

---
    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 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..85a6d79 100644
--- a/xen/include/asm-arm/gic_v3_defs.h
+++ b/xen/include/asm-arm/gic_v3_defs.h
@@ -18,6 +18,9 @@
 #ifndef __ASM_ARM_GIC_V3_DEFS_H__
 #define __ASM_ARM_GIC_V3_DEFS_H__
 
+/* Additional bits in GICD_TYPER defined by GICv3 */
+#define GICD_TYPE_ID_BITS_SHIFT 19
+
 /*
  * Additional registers defined in GIC v3.
  * Common GICD registers are defined in gic.h
-- 
2.1.4

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

[Ian Campbell]

On Thu, 2015-01-29 at 18:25 +0000, Julien Grall wrote:
> From Linux 3.19, the GICv3 drivers is using GICD_TYPER.IDbits to check
> the validity of the hardware interrupt number.
> 
> The field IDBits in the register GICD_TYPER is used to know the number of
> interrupt identifiers (SPI, PPIs, SGIs, LPIs) supported by GIC Stream Protocol
> Interface.
> 
> This field contains the number of interrupt identifier bits minus one.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> ---
>     This patch should be backported to Xen 4.5. Without it any Linux
>     kernel > 3.19 won't boot as a Xen domain.
> 
>     I'm wondering if we should add a release note for this bug.
> 
>     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..85a6d79 100644
> --- a/xen/include/asm-arm/gic_v3_defs.h
> +++ b/xen/include/asm-arm/gic_v3_defs.h
> @@ -18,6 +18,9 @@
>  #ifndef __ASM_ARM_GIC_V3_DEFS_H__
>  #define __ASM_ARM_GIC_V3_DEFS_H__
>  
> +/* Additional bits in GICD_TYPER defined by GICv3 */
> +#define GICD_TYPE_ID_BITS_SHIFT 19

Please put this further down with the other bit definitions (i.e. below
the register definitions).

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

[PATCH v2 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 v2 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>

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

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

[Ian Campbell]

On Thu, 2015-01-29 at 18:25 +0000, Julien Grall wrote:
> 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>

[PATCH v2 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*).

Thoses registers are misimplemented 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_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 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

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

[Ian Campbell]

On Thu, 2015-01-29 at 18:25 +0000, Julien Grall wrote:
> Some of the registers are accessible via multiple size (see GICD_IPRIORITYR*).
> 
> Thoses registers are misimplemented when they should be RAZ. Only

"Those" and "incorrectly implemented".

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

5.1.3 suggests there are at least some 64-bit registers where it ought
to be possible to read the upper and lower halves independently. Don't
you need to support that?

BTW, a reference to 5.1.3 in the changelog would be handy.

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

[Julien Grall]

Hi Ian,

On 02/02/15 15:24, Ian Campbell wrote:
> On Thu, 2015-01-29 at 18:25 +0000, Julien Grall wrote:
>> Some of the registers are accessible via multiple size (see GICD_IPRIORITYR*).
>>
>> Thoses registers are misimplemented when they should be RAZ. Only
> 
> "Those" and "incorrectly implemented".
> 
>> 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_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.
> 
> 5.1.3 suggests there are at least some 64-bit registers where it ought
> to be possible to read the upper and lower halves independently. Don't
> you need to support that?

Only when the system is supporting AArch32. If the system only supports
AArch64, 64-bit registers can only be read via a 64-bit access.

I don't think we actually support AArch32 on the vGICv3 drivers. And we
don't emulate 32-bits access on 64-bit registers.

I will give a look to it.

> BTW, a reference to 5.1.3 in the changelog would be handy.

I will also mention the version of the document as this paragraph
doesn't exists on the previous version.

Regards,

-- 
Julien Grall

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

[Ian Campbell]

On Mon, 2015-02-02 at 15:59 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 02/02/15 15:24, Ian Campbell wrote:
> > On Thu, 2015-01-29 at 18:25 +0000, Julien Grall wrote:
> >> Some of the registers are accessible via multiple size (see GICD_IPRIORITYR*).
> >>
> >> Thoses registers are misimplemented when they should be RAZ. Only
> > 
> > "Those" and "incorrectly implemented".
> > 
> >> 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_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.
> > 
> > 5.1.3 suggests there are at least some 64-bit registers where it ought
> > to be possible to read the upper and lower halves independently. Don't
> > you need to support that?
> 
> Only when the system is supporting AArch32. If the system only supports
> AArch64, 64-bit registers can only be read via a 64-bit access.
> 
> I don't think we actually support AArch32 on the vGICv3 drivers. And we
> don't emulate 32-bits access on 64-bit registers.

It's certainly our intention in general to support arm32 guest kernels
on arm64, the v3 vgic may not reach that aspiration though.

Ian.

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

[Julien Grall]

On 02/02/15 16:08, Ian Campbell wrote:
> On Mon, 2015-02-02 at 15:59 +0000, Julien Grall wrote:
>> Hi Ian,
>>
>> On 02/02/15 15:24, Ian Campbell wrote:
>>> On Thu, 2015-01-29 at 18:25 +0000, Julien Grall wrote:
>>>> Some of the registers are accessible via multiple size (see GICD_IPRIORITYR*).
>>>>
>>>> Thoses registers are misimplemented when they should be RAZ. Only
>>>
>>> "Those" and "incorrectly implemented".
>>>
>>>> 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_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.
>>>
>>> 5.1.3 suggests there are at least some 64-bit registers where it ought
>>> to be possible to read the upper and lower halves independently. Don't
>>> you need to support that?
>>
>> Only when the system is supporting AArch32. If the system only supports
>> AArch64, 64-bit registers can only be read via a 64-bit access.
>>
>> I don't think we actually support AArch32 on the vGICv3 drivers. And we
>> don't emulate 32-bits access on 64-bit registers.
> 
> It's certainly our intention in general to support arm32 guest kernels
> on arm64, the v3 vgic may not reach that aspiration though.

AFAICT, only the vGIC v3 is using 64-bit access. So we are fine for now.

Linux seems to allow to build GICv3 for ARM32. I guess we should support
it in the future.

It would be useful for booting 32 bit guest on GICv3 only platform,
assuming a such platform will exists.

Regards,

-- 
Julien Grall

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

[Julien Grall]

On 02/02/15 16:11, Julien Grall wrote:
> On 02/02/15 16:08, Ian Campbell wrote:
>> On Mon, 2015-02-02 at 15:59 +0000, Julien Grall wrote:
>>> Hi Ian,
>>>
>>> On 02/02/15 15:24, Ian Campbell wrote:
>>>> On Thu, 2015-01-29 at 18:25 +0000, Julien Grall wrote:
>>>>> Some of the registers are accessible via multiple size (see GICD_IPRIORITYR*).
>>>>>
>>>>> Thoses registers are misimplemented when they should be RAZ. Only
>>>>
>>>> "Those" and "incorrectly implemented".
>>>>
>>>>> 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_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.
>>>>
>>>> 5.1.3 suggests there are at least some 64-bit registers where it ought
>>>> to be possible to read the upper and lower halves independently. Don't
>>>> you need to support that?
>>>
>>> Only when the system is supporting AArch32. If the system only supports
>>> AArch64, 64-bit registers can only be read via a 64-bit access.
>>>
>>> I don't think we actually support AArch32 on the vGICv3 drivers. And we
>>> don't emulate 32-bits access on 64-bit registers.
>>
>> It's certainly our intention in general to support arm32 guest kernels
>> on arm64, the v3 vgic may not reach that aspiration though.
> 
> AFAICT, only the vGIC v3 is using 64-bit access. So we are fine for now.
> 
> Linux seems to allow to build GICv3 for ARM32. I guess we should support
> it in the future.

So I was wrong, it's not possible to select GICv3 manually on ARM32. So
I will update the commit message that we only support Aarch64 for now,
therefore only 64-bit access is allowed.

Supporting Aarch32 would require more work, so it's defer until someone
care about this use case. Is it ok for you?

Regards,

-- 
Julien Grall

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

[Ian Campbell]

On Thu, 2015-01-29 at 18:25 +0000, Julien Grall wrote:
> Some of the registers are accessible via multiple size (see GICD_IPRIORITYR*).
> 
> Thoses registers are misimplemented when they should be RAZ. Only

"Those" and "incorrectly implemented".

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

5.1.3 suggests there are at least some 64-bit registers where it ought
to be possible to read the upper and lower halves independently.

BTW, a reference to 5.1.3 in the changelog would be handy.

Ian.

[PATCH v2 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>

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

    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

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

[Ian Campbell]

On Thu, 2015-01-29 at 18:25 +0000, Julien Grall wrote:
> 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>

[PATCH v2 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.

This is only happening for DOM0, so we can move this code in
gicv_v3_init. Take the opportunity to move the stride setting a bit ealier
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 v2:
        - Patch added
---
 xen/arch/arm/gic-v3.c  | 11 ++++++++++-
 xen/arch/arm/vgic-v3.c |  6 +-----
 2 files changed, 11 insertions(+), 6 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..db6b514 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);
-- 
2.1.4

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

[Ian Campbell]

On Thu, 2015-01-29 at 18:25 +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.

Can this defaulting not be pulled further to the initialisation of
gicv3.rdist_stride?

> This is only happening for DOM0,

Please say instead "Because domU uses a static stride configuration this
only happens for dom0..." or similar (i.e. include the reason why domU
is excluded)

>  so we can move this code in
> gicv_v3_init. Take the opportunity to move the stride setting a bit ealier

"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 v2:
>         - Patch added
> ---
>  xen/arch/arm/gic-v3.c  | 11 ++++++++++-
>  xen/arch/arm/vgic-v3.c |  6 +-----
>  2 files changed, 11 insertions(+), 6 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..db6b514 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)

Why not the write case too?

>  
>      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);

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

[Julien Grall]

On 02/02/15 15:40, Ian Campbell wrote:
> On Thu, 2015-01-29 at 18:25 +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.
> 
> Can this defaulting not be pulled further to the initialisation of
> gicv3.rdist_stride?

With the upcoming GICv4, the stride may be different for each
distributor (see the check on GICR_TYPER.VLPIS in gicv3_populate_rdist).

So I'd like to avoid the check of rdist_stride.

>> This is only happening for DOM0,
> 
> Please say instead "Because domU uses a static stride configuration this
> only happens for dom0..." or similar (i.e. include the reason why domU
> is excluded)

I will do.

>>  so we can move this code in
>> gicv_v3_init. Take the opportunity to move the stride setting a bit ealier
> 
> "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 v2:
>>         - Patch added
>> ---
>>  xen/arch/arm/gic-v3.c  | 11 ++++++++++-
>>  xen/arch/arm/vgic-v3.c |  6 +-----
>>  2 files changed, 11 insertions(+), 6 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..db6b514 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)
> 
> Why not the write case too?

By mistake it has been dropped in a following patch ("Emulate correctly
the re-distributor"). I will move the changes here.

Regards,

-- 
Julien Grall

[PATCH v2 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>

---
    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 db6b514..13481ac 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -1112,13 +1112,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

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

[Ian Campbell]

On Thu, 2015-01-29 at 18:25 +0000, Julien Grall wrote:
> 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>

[PATCH v2 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 should have a correct implementation for the other operating system.

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 an other 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 accross 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 v2:
        - Patch added
---
 xen/arch/arm/gic-v3.c        |  12 ++++-
 xen/arch/arm/vgic-v3.c       | 111 ++++++++++++++++++++++++++++++++++++++++---
 xen/include/asm-arm/domain.h |   6 +++
 3 files changed, 122 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index fdfda0b..1b7ddb3 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -895,6 +895,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 +911,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 +938,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;
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 13481ac..378ac82 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,61 @@ 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: We assume that the region are ordered.
+     */
+    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;
+
+    /*
+     * Note: We are assuming that d->vcpu[vcpu_id] is never NULL. If
+     * it's the case, the guest will receive a data abort and won't be
+     * able to boot.
+     */
+    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,11 +697,9 @@ 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);
+    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);
@@ -1084,6 +1134,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 |
@@ -1094,6 +1151,48 @@ 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;
+
+    /*
+     * Safety check mostly for DOM0. It's possible to have more vCPU
+     * than the number of physical CPU. Maybe we should deny this case?
+     */
+    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

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

[Ian Campbell]

On Thu, 2015-01-29 at 18:25 +0000, Julien Grall wrote:
> 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 should have a correct implementation for the other operating system.

You could state something stronger here, which is that it is wrong and
should be fixed as a matter of principal. The fact that we happen to get
away with it for some versions of Linux is an aside (although worth
mentioning)

> 
> 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 an other re-distributor

"another".

> 
> 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 accross multiple regions in the memory.

"across"

> +    /*
> +     * 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: We assume that the region are ordered.

You could also check base+size vs gpa to avoid this limitation.

> +
> +    /*
> +     * Note: We are assuming that d->vcpu[vcpu_id] is never NULL. If
> +     * it's the case, the guest will receive a data abort and won't be
> +     * able to boot.

Is cpu hotplug a factor here? Do we support guests booting with offline
cpus yet?

> +    /*
> +     * Safety check mostly for DOM0. It's possible to have more vCPU
> +     * than the number of physical CPU. Maybe we should deny this case?

In general it's allowed, if a bit silly. Mainly for e.g. people working
on PV spinlock code who want to force contention... Unlikely for dom0 I
suppose.

Ian.

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

[Julien Grall]

Hi Ian,

On 02/02/15 15:59, Ian Campbell wrote:
> On Thu, 2015-01-29 at 18:25 +0000, Julien Grall wrote:
>> 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 should have a correct implementation for the other operating system.
> 
> You could state something stronger here, which is that it is wrong and
> should be fixed as a matter of principal. The fact that we happen to get
> away with it for some versions of Linux is an aside (although worth
> mentioning)

I will rework the paragraph.

>>
>> 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 an other re-distributor
> 
> "another".
> 
>>
>> 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 accross multiple regions in the memory.
> 
> "across"
> 
>> +    /*
>> +     * 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: We assume that the region are ordered.
> 
> You could also check base+size vs gpa to avoid this limitation.

IHMO, this limitation is harmless. This would happen for DOM0 if the
device tree doesn't sort the region.

AFAICT, we have a similar limitation for the memory region. Although I
could sort them when we build DOM0.

>> +
>> +    /*
>> +     * Note: We are assuming that d->vcpu[vcpu_id] is never NULL. If
>> +     * it's the case, the guest will receive a data abort and won't be
>> +     * able to boot.
> 
> Is cpu hotplug a factor here? Do we support guests booting with offline
> cpus yet?

The "problem" is not because of CPU hotpluging. XEN_DOMCTL_max_vcpus
doesn't allow the change of the number of vCPUs. AFAICT, this won't be
supported (see comments within the code).

But, the domctl may not set a vCPU if it's fail to initialize it. So in
theory it would be possible to have d->vcpu[vcpu_id] == NULL. But in
practice, the toolstack won't continue if one of the VCPU has not been
allocated.

I felt it was good to mention it for documentation purpose.

> 
>> +    /*
>> +     * Safety check mostly for DOM0. It's possible to have more vCPU
>> +     * than the number of physical CPU. Maybe we should deny this case?
> 
> In general it's allowed, if a bit silly. Mainly for e.g. people working
> on PV spinlock code who want to force contention... Unlikely for dom0 I
> suppose.

We don't control the DOM0 memory layout, so in practice we can't have
more vCPUs than allowed by the hardware. This is because every
re-distributors have it's own MMIO region.

It's different for guests as we control the memory layout.

Regards,

-- 
Julien Grall

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

[Ian Campbell]

On Mon, 2015-02-02 at 16:33 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 02/02/15 15:59, Ian Campbell wrote:
> > On Thu, 2015-01-29 at 18:25 +0000, Julien Grall wrote:
> >> 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 should have a correct implementation for the other operating system.
> > 
> > You could state something stronger here, which is that it is wrong and
> > should be fixed as a matter of principal. The fact that we happen to get
> > away with it for some versions of Linux is an aside (although worth
> > mentioning)
> 
> I will rework the paragraph.
> 
> >>
> >> 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 an other re-distributor
> > 
> > "another".
> > 
> >>
> >> 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 accross multiple regions in the memory.
> > 
> > "across"
> > 
> >> +    /*
> >> +     * 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: We assume that the region are ordered.
> > 
> > You could also check base+size vs gpa to avoid this limitation.
> 
> IHMO, this limitation is harmless. This would happen for DOM0 if the
> device tree doesn't sort the region.

If it can happen then it isn't harmless, and it's easy to avoid so why
not do so.

> AFAICT, we have a similar limitation for the memory region. Although I
> could sort them when we build DOM0.

I'm not sure we do these days, but in any case two wrongs don't make a
right.

> 
> >> +
> >> +    /*
> >> +     * Note: We are assuming that d->vcpu[vcpu_id] is never NULL. If
> >> +     * it's the case, the guest will receive a data abort and won't be
> >> +     * able to boot.
> > 
> > Is cpu hotplug a factor here? Do we support guests booting with offline
> > cpus yet?
> 
> The "problem" is not because of CPU hotpluging. XEN_DOMCTL_max_vcpus
> doesn't allow the change of the number of vCPUs. AFAICT, this won't be
> supported (see comments within the code).

> I felt it was good to mention it for documentation purpose.

The important thing is that the vcpu array is fully populated by that
hypercall, rather than e.g. deferred until vcpuop_initialise, that's
what matters here really.

I think the comment is redundant then, or at least misleading in that
implies there is some possibility that d->vcpu[valid_vcpu] is NULL when
there isn't.

> >> +    /*
> >> +     * Safety check mostly for DOM0. It's possible to have more vCPU
> >> +     * than the number of physical CPU. Maybe we should deny this case?
> > 
> > In general it's allowed, if a bit silly. Mainly for e.g. people working
> > on PV spinlock code who want to force contention... Unlikely for dom0 I
> > suppose.
> 
> We don't control the DOM0 memory layout, so in practice we can't have
> more vCPUs than allowed by the hardware. This is because every
> re-distributors have it's own MMIO region.

I'd not be all that surprised to see systems with more rdist region
space than they have physical processors e.g. sku's with different
numbers of cores, but otherwise the same platform.

Ian.

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

[Julien Grall]

On 02/02/15 16:47, Ian Campbell wrote:
> On Mon, 2015-02-02 at 16:33 +0000, Julien Grall wrote:
>> Hi Ian,
>>
>> On 02/02/15 15:59, Ian Campbell wrote:
>>> On Thu, 2015-01-29 at 18:25 +0000, Julien Grall wrote:
>>>> 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 should have a correct implementation for the other operating system.
>>>
>>> You could state something stronger here, which is that it is wrong and
>>> should be fixed as a matter of principal. The fact that we happen to get
>>> away with it for some versions of Linux is an aside (although worth
>>> mentioning)
>>
>> I will rework the paragraph.
>>
>>>>
>>>> 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 an other re-distributor
>>>
>>> "another".
>>>
>>>>
>>>> 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 accross multiple regions in the memory.
>>>
>>> "across"
>>>
>>>> +    /*
>>>> +     * 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: We assume that the region are ordered.
>>>
>>> You could also check base+size vs gpa to avoid this limitation.
>>
>> IHMO, this limitation is harmless. This would happen for DOM0 if the
>> device tree doesn't sort the region.
> 
> If it can happen then it isn't harmless, and it's easy to avoid so why
> not do so.

The code is looking one region a-head to avoid check in the case there
is only one big re-distributors region.

>> AFAICT, we have a similar limitation for the memory region. Although I
>> could sort them when we build DOM0.
> 
> I'm not sure we do these days, but in any case two wrongs don't make a
> right.

See consider_modules. We implicitly assume memory ordering.

Anyway, if we do that, we should also check the overlapping between
regions, the size of the region is valid (i.e aligned to 64K and
correctly sized...),...

The main drawbacks here would be DOM0 access the wrong vCPU or receive a
data abort. It's not too bad compare to "slowing down" the lookup.

If you really want to support non-ordered access, I could do at Domain
initialization.

>>
>>>> +
>>>> +    /*
>>>> +     * Note: We are assuming that d->vcpu[vcpu_id] is never NULL. If
>>>> +     * it's the case, the guest will receive a data abort and won't be
>>>> +     * able to boot.
>>>
>>> Is cpu hotplug a factor here? Do we support guests booting with offline
>>> cpus yet?
>>
>> The "problem" is not because of CPU hotpluging. XEN_DOMCTL_max_vcpus
>> doesn't allow the change of the number of vCPUs. AFAICT, this won't be
>> supported (see comments within the code).
> 
>> I felt it was good to mention it for documentation purpose.
> 
> The important thing is that the vcpu array is fully populated by that
> hypercall, rather than e.g. deferred until vcpuop_initialise, that's
> what matters here really.
> 
> I think the comment is redundant then, or at least misleading in that
> implies there is some possibility that d->vcpu[valid_vcpu] is NULL when
> there isn't.
> 
>>>> +    /*
>>>> +     * Safety check mostly for DOM0. It's possible to have more vCPU
>>>> +     * than the number of physical CPU. Maybe we should deny this case?
>>>
>>> In general it's allowed, if a bit silly. Mainly for e.g. people working
>>> on PV spinlock code who want to force contention... Unlikely for dom0 I
>>> suppose.
>>
>> We don't control the DOM0 memory layout, so in practice we can't have
>> more vCPUs than allowed by the hardware. This is because every
>> re-distributors have it's own MMIO region.
> 
> I'd not be all that surprised to see systems with more rdist region
> space than they have physical processors e.g. sku's with different
> numbers of cores, but otherwise the same platform.

So the current check would be enough? Even though, having more vCPUs
than physical CPUs for DOM0 sounds silly.

Regards,

-- 
Julien Grall

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

[Ian Campbell]

On Mon, 2015-02-02 at 17:05 +0000, Julien Grall wrote:
> On 02/02/15 16:47, Ian Campbell wrote:
> > On Mon, 2015-02-02 at 16:33 +0000, Julien Grall wrote:
> >> Hi Ian,
> >>
> >> On 02/02/15 15:59, Ian Campbell wrote:
> >>> On Thu, 2015-01-29 at 18:25 +0000, Julien Grall wrote:
> >>>> 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 should have a correct implementation for the other operating system.
> >>>
> >>> You could state something stronger here, which is that it is wrong and
> >>> should be fixed as a matter of principal. The fact that we happen to get
> >>> away with it for some versions of Linux is an aside (although worth
> >>> mentioning)
> >>
> >> I will rework the paragraph.
> >>
> >>>>
> >>>> 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 an other re-distributor
> >>>
> >>> "another".
> >>>
> >>>>
> >>>> 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 accross multiple regions in the memory.
> >>>
> >>> "across"
> >>>
> >>>> +    /*
> >>>> +     * 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: We assume that the region are ordered.
> >>>
> >>> You could also check base+size vs gpa to avoid this limitation.
> >>
> >> IHMO, this limitation is harmless. This would happen for DOM0 if the
> >> device tree doesn't sort the region.
> > 
> > If it can happen then it isn't harmless, and it's easy to avoid so why
> > not do so.
> 
> The code is looking one region a-head to avoid check in the case there
> is only one big re-distributors region.
> 
> >> AFAICT, we have a similar limitation for the memory region. Although I
> >> could sort them when we build DOM0.
> > 
> > I'm not sure we do these days, but in any case two wrongs don't make a
> > right.
> 
> See consider_modules. We implicitly assume memory ordering.
> 
> Anyway, if we do that, we should also check the overlapping between
> regions, the size of the region is valid (i.e aligned to 64K and
> correctly sized...),...

I don't see how all that follows, certainly not at run time. Those are
the sorts of things which should be checked at initialisation time and
cause us to complain loudly.

> The main drawbacks here would be DOM0 access the wrong vCPU or receive a
> data abort. It's not too bad compare to "slowing down" the lookup.

> If you really want to support non-ordered access, I could do at Domain
> initialization.

If we are at liberty to sort the list at init time then that would be
fine.

We probably ought to do the same with the memory regions

> > I'd not be all that surprised to see systems with more rdist region
> > space than they have physical processors e.g. sku's with different
> > numbers of cores, but otherwise the same platform.
> 
> So the current check would be enough? Even though, having more vCPUs
> than physical CPUs for DOM0 sounds silly.
> 

I expect so, yes.

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

[Julien Grall]

Hi Ian,

On 02/02/15 17:38, Ian Campbell wrote:
> On Mon, 2015-02-02 at 17:05 +0000, Julien Grall wrote:
>> On 02/02/15 16:47, Ian Campbell wrote:
>>> On Mon, 2015-02-02 at 16:33 +0000, Julien Grall wrote:
>>>> Hi Ian,
>>>>
>>>> On 02/02/15 15:59, Ian Campbell wrote:
>>>>> On Thu, 2015-01-29 at 18:25 +0000, Julien Grall wrote:
>>>>>> 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 should have a correct implementation for the other operating system.
>>>>>
>>>>> You could state something stronger here, which is that it is wrong and
>>>>> should be fixed as a matter of principal. The fact that we happen to get
>>>>> away with it for some versions of Linux is an aside (although worth
>>>>> mentioning)
>>>>
>>>> I will rework the paragraph.
>>>>
>>>>>>
>>>>>> 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 an other re-distributor
>>>>>
>>>>> "another".
>>>>>
>>>>>>
>>>>>> 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 accross multiple regions in the memory.
>>>>>
>>>>> "across"
>>>>>
>>>>>> +    /*
>>>>>> +     * 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: We assume that the region are ordered.
>>>>>
>>>>> You could also check base+size vs gpa to avoid this limitation.
>>>>
>>>> IHMO, this limitation is harmless. This would happen for DOM0 if the
>>>> device tree doesn't sort the region.
>>>
>>> If it can happen then it isn't harmless, and it's easy to avoid so why
>>> not do so.
>>
>> The code is looking one region a-head to avoid check in the case there
>> is only one big re-distributors region.
>>
>>>> AFAICT, we have a similar limitation for the memory region. Although I
>>>> could sort them when we build DOM0.
>>>
>>> I'm not sure we do these days, but in any case two wrongs don't make a
>>> right.
>>
>> See consider_modules. We implicitly assume memory ordering.
>>
>> Anyway, if we do that, we should also check the overlapping between
>> regions, the size of the region is valid (i.e aligned to 64K and
>> correctly sized...),...
> 
> I don't see how all that follows, certainly not at run time. Those are
> the sorts of things which should be checked at initialisation time and
> cause us to complain loudly.

My point was, we have to trust a bit the device tree given by the
platform. If the device tree is buggy, then bare-metal linux will
unlikely boot.

But I agree, that we have to sort the region of re-distributors at init.

>> The main drawbacks here would be DOM0 access the wrong vCPU or receive a
>> data abort. It's not too bad compare to "slowing down" the lookup.
> 
>> If you really want to support non-ordered access, I could do at Domain
>> initialization.
> 
> If we are at liberty to sort the list at init time then that would be
> fine.

I don't think it's matter if we don't expose exactly the same layout for
the re-distributors as the physical hardware.

So, we can do whatever we want :).

> We probably ought to do the same with the memory regions
> 
>>> I'd not be all that surprised to see systems with more rdist region
>>> space than they have physical processors e.g. sku's with different
>>> numbers of cores, but otherwise the same platform.
>>
>> So the current check would be enough? Even though, having more vCPUs
>> than physical CPUs for DOM0 sounds silly.
>>
> 
> I expect so, yes.

Great, thanks. I will update the comment in the next version.

Regards,

-- 
Julien Grall

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

[Ian Campbell]

On Tue, 2015-02-03 at 13:13 +0000, Julien Grall wrote:
> My point was, we have to trust a bit the device tree given by the
> platform. If the device tree is buggy, then bare-metal linux will
> unlikely boot.

I think overlapping or not correctly size aligned, assuming such goes
against the binding == buggy device tree, which we can decide to either
proactively test for it and warn, or just trust not to be the case and
damn the consequences on buggy platforms, that's fine (although
pragmatically, we might want to support a buggy platform at some
point...).

But we initially started off talking about the ordering of the entries.
It's not clear to me what the requirements from the spec (ePAPR, the
bindings et al) are in this regard, so I don't know that we can simply
claim device trees which do not sort them are buggy, and therefore
whether we can trust them to be ordered or not.

If something requires that the rdist regions are ordered then we can
trust that and treat it like overlap or sizing/alignment etc. If the
spec doesn't say anything about the entries being ordered in some
particular way then we should try not to assume it or trust them to be
that way.

(AFAICT our current handling of the memory nodes violates this, at least
I can't find anything in ePAPR which requires an ordering but we do).

Anyway...

> But I agree, that we have to sort the region of re-distributors at init.

That would be one way to avoid trawling through all the specs etc ;-)

Ian.

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

[Vijay Kilari]

On Thu, Jan 29, 2015 at 11:55 PM, Julien Grall <julien.grall@linaro.org> wrote:
> 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 should have a correct implementation for the other operating system.
>
> 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 an other 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 accross 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 v2:
>         - Patch added
> ---
>  xen/arch/arm/gic-v3.c        |  12 ++++-
>  xen/arch/arm/vgic-v3.c       | 111 ++++++++++++++++++++++++++++++++++++++++---
>  xen/include/asm-arm/domain.h |   6 +++
>  3 files changed, 122 insertions(+), 7 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index fdfda0b..1b7ddb3 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -895,6 +895,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 +911,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;

  Here you rely on size, The size might not be always map to
number of cpus in that region. We should rely on GICR_TYPER_LAST
to know the last cpu in the region.

In populate_redist as well the re-distributor region of the cpu is checked
till the GICR_TYPER_LAST bit in the region but not on size

>          }
>          d->arch.vgic.nr_regions = gicv3.rdist_count;
>      }
> @@ -929,6 +938,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;
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index 13481ac..378ac82 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,61 @@ 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: We assume that the region are ordered.
> +     */
> +    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;
> +
> +    /*
> +     * Note: We are assuming that d->vcpu[vcpu_id] is never NULL. If
> +     * it's the case, the guest will receive a data abort and won't be
> +     * able to boot.
> +     */
> +    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,11 +697,9 @@ 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);
> +    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);
> @@ -1084,6 +1134,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 |
> @@ -1094,6 +1151,48 @@ 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;
> +
> +    /*
> +     * Safety check mostly for DOM0. It's possible to have more vCPU
> +     * than the number of physical CPU. Maybe we should deny this case?
> +     */
> +    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
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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

[Julien Grall]

Hello Vijay,

On 03/02/15 06:47, Vijay Kilari wrote:
> On Thu, Jan 29, 2015 at 11:55 PM, Julien Grall <julien.grall@linaro.org> wrote:
>> @@ -909,8 +911,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;
> 
>   Here you rely on size, The size might not be always map to
> number of cpus in that region. We should rely on GICR_TYPER_LAST
> to know the last cpu in the region.

This is the emulated GIC for DOM0... we don't have to expose the same
re-distributors layout as the hardware. By same layout I mean, the base
address of the redistributor for virtual CPUn is equal to base address
of the re-distributor for physical CPUn.

If your DOM0 rely on having exactly the same layout, that means that
your kernel is clearly buggy...

Regards,

-- 
Julien Grall

[PATCH v2 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>

---
    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 378ac82..b59cc49 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);
@@ -782,7 +783,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;
@@ -947,7 +948,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

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

[Ian Campbell]

On Thu, 2015-01-29 at 18:25 +0000, Julien Grall wrote:
> 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>

[PATCH v2 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 v2 11/15] xen/arm: vgic-v2: Correctly handle RAZ/WI registers

[Julien Grall]

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

Thoses registers are misimplemented 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.

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

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

[Ian Campbell]

On Thu, 2015-01-29 at 18:25 +0000, Julien Grall wrote:
> Some of the registers are accessible via multiple size (see GICD_IPRIORITYR*).

They are byte accessible, but are they half word accessible? I suspect
not.

> Thoses registers are misimplemented when they should be RAZ. Only

Same typoes as the v3 version.

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

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

[Julien Grall]

Hi Ian,

On 02/02/15 16:02, Ian Campbell wrote:
> On Thu, 2015-01-29 at 18:25 +0000, Julien Grall wrote:
>> Some of the registers are accessible via multiple size (see GICD_IPRIORITYR*).
> 
> They are byte accessible, but are they half word accessible? I suspect
> not.

Only byte accessible.

>> Thoses registers are misimplemented when they should be RAZ. Only
> 
> Same typoes as the v3 version.

I copied/pasted it, because it was too lazy to write a similar message.
I will fix it in the next version.

Regards,

-- 
Julien Grall

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

[Ian Campbell]

On Mon, 2015-02-02 at 16:36 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 02/02/15 16:02, Ian Campbell wrote:
> > On Thu, 2015-01-29 at 18:25 +0000, Julien Grall wrote:
> >> Some of the registers are accessible via multiple size (see GICD_IPRIORITYR*).
> > 
> > They are byte accessible, but are they half word accessible? I suspect
> > not.
> 
> Only byte accessible.

I think we might need a read_as_zero_8_32 then, i.e. explicitly list the
sizes which are allowed, and perhaps omit the un-suffixed version for so
it's clear exactly what is what, although that might be more churn than
you want to have here.

Ian.

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

[Julien Grall]

On 02/02/15 16:50, Ian Campbell wrote:
> On Mon, 2015-02-02 at 16:36 +0000, Julien Grall wrote:
>> Hi Ian,
>>
>> On 02/02/15 16:02, Ian Campbell wrote:
>>> On Thu, 2015-01-29 at 18:25 +0000, Julien Grall wrote:
>>>> Some of the registers are accessible via multiple size (see GICD_IPRIORITYR*).
>>>
>>> They are byte accessible, but are they half word accessible? I suspect
>>> not.
>>
>> Only byte accessible.
> 
> I think we might need a read_as_zero_8_32 then, i.e. explicitly list the
> sizes which are allowed, and perhaps omit the un-suffixed version for so
> it's clear exactly what is what, although that might be more churn than
> you want to have here.

Hmmm, why? I was talking for registers defined in the spec. We don't
know if reserved/implementation defined registers will allow half-word
access.

The un-suffixed version is there when we don't need to check the size
because it has already been done. It seems pointless to check it again.

Regards,

-- 
Julien Grall

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

[Ian Campbell]

On Mon, 2015-02-02 at 17:08 +0000, Julien Grall wrote:
> On 02/02/15 16:50, Ian Campbell wrote:
> > On Mon, 2015-02-02 at 16:36 +0000, Julien Grall wrote:
> >> Hi Ian,
> >>
> >> On 02/02/15 16:02, Ian Campbell wrote:
> >>> On Thu, 2015-01-29 at 18:25 +0000, Julien Grall wrote:
> >>>> Some of the registers are accessible via multiple size (see GICD_IPRIORITYR*).
> >>>
> >>> They are byte accessible, but are they half word accessible? I suspect
> >>> not.
> >>
> >> Only byte accessible.
> > 
> > I think we might need a read_as_zero_8_32 then, i.e. explicitly list the
> > sizes which are allowed, and perhaps omit the un-suffixed version for so
> > it's clear exactly what is what, although that might be more churn than
> > you want to have here.
> 
> Hmmm, why? I was talking for registers defined in the spec. We don't
> know if reserved/implementation defined registers will allow half-word
> access.
> 
> The un-suffixed version is there when we don't need to check the size
> because it has already been done. It seems pointless to check it again.

I'd forgotten the existing check was there, yes it does make sense to
use it in this case.

Ian.

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

[Julien Grall]

On 02/02/15 17:41, Ian Campbell wrote:
> On Mon, 2015-02-02 at 17:08 +0000, Julien Grall wrote:
>> On 02/02/15 16:50, Ian Campbell wrote:
>>> On Mon, 2015-02-02 at 16:36 +0000, Julien Grall wrote:
>>>> Hi Ian,
>>>>
>>>> On 02/02/15 16:02, Ian Campbell wrote:
>>>>> On Thu, 2015-01-29 at 18:25 +0000, Julien Grall wrote:
>>>>>> Some of the registers are accessible via multiple size (see GICD_IPRIORITYR*).
>>>>>
>>>>> They are byte accessible, but are they half word accessible? I suspect
>>>>> not.
>>>>
>>>> Only byte accessible.
>>>
>>> I think we might need a read_as_zero_8_32 then, i.e. explicitly list the
>>> sizes which are allowed, and perhaps omit the un-suffixed version for so
>>> it's clear exactly what is what, although that might be more churn than
>>> you want to have here.
>>
>> Hmmm, why? I was talking for registers defined in the spec. We don't
>> know if reserved/implementation defined registers will allow half-word
>> access.
>>
>> The un-suffixed version is there when we don't need to check the size
>> because it has already been done. It seems pointless to check it again.
> 
> I'd forgotten the existing check was there, yes it does make sense to
> use it in this case.

Shall I clarify the commit message for this bit?

Regards,

-- 
Julien Grall

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

[Ian Campbell]

On Tue, 2015-02-03 at 13:14 +0000, Julien Grall wrote:
> On 02/02/15 17:41, Ian Campbell wrote:
> > On Mon, 2015-02-02 at 17:08 +0000, Julien Grall wrote:
> >> On 02/02/15 16:50, Ian Campbell wrote:
> >>> On Mon, 2015-02-02 at 16:36 +0000, Julien Grall wrote:
> >>>> Hi Ian,
> >>>>
> >>>> On 02/02/15 16:02, Ian Campbell wrote:
> >>>>> On Thu, 2015-01-29 at 18:25 +0000, Julien Grall wrote:
> >>>>>> Some of the registers are accessible via multiple size (see GICD_IPRIORITYR*).
> >>>>>
> >>>>> They are byte accessible, but are they half word accessible? I suspect
> >>>>> not.
> >>>>
> >>>> Only byte accessible.
> >>>
> >>> I think we might need a read_as_zero_8_32 then, i.e. explicitly list the
> >>> sizes which are allowed, and perhaps omit the un-suffixed version for so
> >>> it's clear exactly what is what, although that might be more churn than
> >>> you want to have here.
> >>
> >> Hmmm, why? I was talking for registers defined in the spec. We don't
> >> know if reserved/implementation defined registers will allow half-word
> >> access.
> >>
> >> The un-suffixed version is there when we don't need to check the size
> >> because it has already been done. It seems pointless to check it again.
> > 
> > I'd forgotten the existing check was there, yes it does make sense to
> > use it in this case.
> 
> Shall I clarify the commit message for this bit?

It can't hurt.

Ian.

[PATCH v2 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 v2 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>

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

Re: [PATCH v2 13/15] xen/arm: vgic-v2: GICD_I{S, C}PENDR* are only word-accessible

[Ian Campbell]

On Thu, 2015-01-29 at 18:25 +0000, Julien Grall wrote:
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

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

[PATCH v2 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.
If we plan to support it in the future, it will unlikely require a specific
bitfield as we already store the status per vIRQ.

Rather than wasting memory for nothing, drop thoses field. Any read to
these registers will be RAZ and any write will print an error and inject
a data abort to the guest.

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

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

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

[Ian Campbell]

On Thu, 2015-01-29 at 18:25 +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.
> If we plan to support it in the future, it will unlikely require a specific
> bitfield as we already store the status per vIRQ.

I think we would want to track the state like we do for enabled, since
walking 32 virqs to construct the value would be a bit expensive.

Stefano, what do you think?

> Rather than wasting memory for nothing, drop thoses field. Any read to

"those"

> these registers will be RAZ and any write will print an error and inject
> a data abort to the guest.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> ---
>     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 b59cc49..c68ef05 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 {

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

[Julien Grall]

On 02/02/15 16:05, Ian Campbell wrote:
> On Thu, 2015-01-29 at 18:25 +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.
>> If we plan to support it in the future, it will unlikely require a specific
>> bitfield as we already store the status per vIRQ.
> 
> I think we would want to track the state like we do for enabled, since
> walking 32 virqs to construct the value would be a bit expensive.
> 
> Stefano, what do you think?

The current code is actually buggy, when not doing nothing.

Hiding the fact that those bits are supported is the worst things to do.
The kernel developer may spend hours, if not days, before finding that
we just ignore the write in those registers.

As it's not supported, and may require some works, it better to drop it
than silently ignore failure (in case of vGICv3).

Regards,

-- 
Julien Grall

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

[Stefano Stabellini]

On Mon, 2 Feb 2015, Ian Campbell wrote:
> On Thu, 2015-01-29 at 18:25 +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.
> > If we plan to support it in the future, it will unlikely require a specific
> > bitfield as we already store the status per vIRQ.
> 
> I think we would want to track the state like we do for enabled, since
> walking 32 virqs to construct the value would be a bit expensive.
> 
> Stefano, what do you think?

I think that if we really need those fields, we can always use git log
to find the patch and reintroduce them. It doesn't make too much sense
to keep them now as we do nothing with them.  We don't even implement
the corresponding register reads and writes in the vgic v2 driver.


> > Rather than wasting memory for nothing, drop thoses field. Any read to
> 
> "those"
> 
> > these registers will be RAZ and any write will print an error and inject
> > a data abort to the guest.
> > 
> > Signed-off-by: Julien Grall <julien.grall@linaro.org>
> > 
> > ---
> >     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 b59cc49..c68ef05 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 {
> 
> 

[PATCH v2 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
    - Explain why SZ_64K * 2

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

---
    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 1b7ddb3..9035e3b 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -638,7 +638,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 */
             }
@@ -1238,7 +1238,6 @@ static int __init gicv3_init(struct dt_device_node *node, const void *data)
         rdist_regs[i].size = rdist_size;
     }
 
-    /* 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 v2 15/15] xen/arm: gic-v3: Update some comments in the code

[Ian Campbell]

On Thu, 2015-01-29 at 18:25 +0000, Julien Grall wrote:
>     - Drop wrong comment about the default stride. It's not always 2 * SZ_64K

What other defaults are possible and under what circumstances?

>     - Explain why SZ_64K * 2
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> ---
>     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 1b7ddb3..9035e3b 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -638,7 +638,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 */
>              }
> @@ -1238,7 +1238,6 @@ static int __init gicv3_init(struct dt_device_node *node, const void *data)
>          rdist_regs[i].size = rdist_size;
>      }
>  
> -    /* 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;
>  

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

[Julien Grall]

Hi Ian,

On 02/02/15 16:05, Ian Campbell wrote:
> On Thu, 2015-01-29 at 18:25 +0000, Julien Grall wrote:
>>     - Drop wrong comment about the default stride. It's not always 2 * SZ_64K
> 
> What other defaults are possible and under what circumstances?

128K, when the re-distributor supports VLPIs.

Regards,

-- 
Julien Grall

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

[Ian Campbell]

On Mon, 2015-02-02 at 16:37 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 02/02/15 16:05, Ian Campbell wrote:
> > On Thu, 2015-01-29 at 18:25 +0000, Julien Grall wrote:
> >>     - Drop wrong comment about the default stride. It's not always 2 * SZ_64K
> > 
> > What other defaults are possible and under what circumstances?
> 
> 128K, when the re-distributor supports VLPIs.

Thanks, please mention that in the message.

> 
> Regards,
>