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

[Ian Campbell <Ian.Campbell@citrix.com>]

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