| Message ID | 1422555950-31821-12-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
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,
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,
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,
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; }
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(-)