| Message ID | 1421684957-29884-5-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hi Ian, On 20/01/15 15:57, Ian Campbell wrote: > On Mon, 2015-01-19 at 16:29 +0000, Julien Grall wrote: >> In general, it's not necessary/important to check the size. > > Only if the docs say this register can be accessed by a partial > read/write, or if it is implementation defined what the result would be > (and RAZ/WI is within the set of allowable actions). > > Do you have a reference for the behaviour of GICR accesses which aren't > of the register's natural size? It's clearly specify in the spec if the register can be accessed with a non-natural size. AFAICT, the spec doesn't give a specific behavior if the register doesn't support byte/word/double word access. IHMO, for RAZ register we can safely accept any kind of access as the final register will in fine always contains 0. That will also any issue with WI/RAZ register because we may have miss a line in the spec stating a register is byte and word accessible. (see the case of vgic-v2). >> It's better >> to log it to let know the guest that its access will have no effect. >> >> Note: On debug build it may happen to see some of these messages during >> domain boot. > > We should only print if the guest has done something wrong, and reading > a RAZ register (or one which we have implemented that way) is not > inherently wrong. That's why it's log in DEBUG and not in ERR. Although, I agree that on read it's not important. But on write it's help the developer to understand which his GIC driver is not working. > IOW read_as_zero* should be silent, and a different code path used for > "guest did something wrong". > > IOW I think the current distinction between bad_width and read_as_zero* > is correct currently, although perhaps the goto's which target them need > adjusting in some cases. > > Perhaps you want to add a read_as_zero_32 which has the check, making > read_as_zero accept either 32- or 64-bit accesses, and goto the correct > label for each register? It's register defined which access is allowed for a register. Regards,
On 20/01/15 17:41, Julien Grall wrote: > Hi Ian, > > On 20/01/15 15:57, Ian Campbell wrote: >> On Mon, 2015-01-19 at 16:29 +0000, Julien Grall wrote: >>> In general, it's not necessary/important to check the size. >> >> Only if the docs say this register can be accessed by a partial >> read/write, or if it is implementation defined what the result would be >> (and RAZ/WI is within the set of allowable actions). >> >> Do you have a reference for the behaviour of GICR accesses which aren't >> of the register's natural size? > > It's clearly specify in the spec if the register can be accessed with a > non-natural size. > > AFAICT, the spec doesn't give a specific behavior if the register > doesn't support byte/word/double word access. Hmmm, I read quickly the spec. 5.1.3 says: "Accessing any of these registers using other accesses is UNPREDICTABLE". So I think it's fine to go on this behavior. It would help to have a simpler code. Regards,
On 20/01/15 17:57, Ian Campbell wrote: > On Tue, 2015-01-20 at 17:41 +0000, Julien Grall wrote: >> Hi Ian, >> >> On 20/01/15 15:57, Ian Campbell wrote: >>> On Mon, 2015-01-19 at 16:29 +0000, Julien Grall wrote: >>>> In general, it's not necessary/important to check the size. >>> >>> Only if the docs say this register can be accessed by a partial >>> read/write, or if it is implementation defined what the result would be >>> (and RAZ/WI is within the set of allowable actions). >>> >>> Do you have a reference for the behaviour of GICR accesses which aren't >>> of the register's natural size? >> >> It's clearly specify in the spec if the register can be accessed with a >> non-natural size. >> >> AFAICT, the spec doesn't give a specific behavior if the register >> doesn't support byte/word/double word access. > > 5.1.3 of the GICv3 spec defines it as UNPREDICATABLE (and also lists all > the valid options for each register). I saw it after sending the mail. >> IHMO, for RAZ register we can safely accept any kind of access as the >> final register will in fine always contains 0. >> >> That will also any issue with WI/RAZ register because we may have miss a >> line in the spec stating a register is byte and word accessible. (see >> the case of vgic-v2). >> >>>> It's better >>>> to log it to let know the guest that its access will have no effect. >>>> >>>> Note: On debug build it may happen to see some of these messages during >>>> domain boot. >>> >>> We should only print if the guest has done something wrong, and reading >>> a RAZ register (or one which we have implemented that way) is not >>> inherently wrong. >> >> That's why it's log in DEBUG and not in ERR. Although, I agree that on >> read it's not important. But on write it's help the developer to >> understand which his GIC driver is not working. > > If the driver is making a legitimate access (i.e. correct size etc) then > we shouldn't be logging, irrespective of whether we are implementing as > RAZ/WI or anything else. I agree for RAZ, but WI would mean something will goes wrong. For instance if the guest is trying to set a bit to 1, while the bit should be 0. So I'd like to keep at least a debug message for WI. It's could be very helpful for the developper. > Given the contents of 5.1.3 I could perhaps be convinced accept making > the bad_width: behaviour less catastrophic to the guest, but killing the > guest meets the defintion of UNPREDICTABLE I think and in general not > letting guests take unexpect advantage of odd corner cases is a good > idea IMHO, lest they come to rely on something. > >>> IOW read_as_zero* should be silent, and a different code path used for >>> "guest did something wrong". >>> >>> IOW I think the current distinction between bad_width and read_as_zero* >>> is correct currently, although perhaps the goto's which target them need >>> adjusting in some cases. >>> >>> Perhaps you want to add a read_as_zero_32 which has the check, making >>> read_as_zero accept either 32- or 64-bit accesses, and goto the correct >>> label for each register? >> >> It's register defined which access is allowed for a register. > > Right, so the decoding switch would need to use the right goto for its > case. As the spec defines the access to be UNPREDICATABLE, I would rather prefer to not decode the size and directly read as zero/write ignore. I would end up to a smaller code and more comprehensible one. Regards,
On 21/01/15 12:11, Ian Campbell wrote: > I thought about this overnight, and I would like to keep UNPREDICATABLE > as the current log + crash please. Apart from the fact that I don't want > guests to be able to rely on unpredictable accesses returning 0 it is > also more consistent with the ARM ARM which says "UNPREDICTABLE > behaviour must not be documented or promoted as having a defined > effect". I was actually planning to suggest this as the access for each registers are clearly define in the specs. > So, in summary: > > 1) Any access which is described as UNPREDICTABLE in GIC spec 5.1.3 > should result in the current bad_width behaviour, that is: a log message > and a domain crash. Ok. > > 2) Accesses which are valid and which are correctly emulated according > to the features which our virtual GIC exposes to the guest should > succeed silently, regardless of whether that means WI, read a constant > or actually have some effect. Ok. > > 3) Accesses which are valid but which do not correctly emulate according > to the features of the virtual gic which we are exposing can log if we > think it is useful to do so. I gave a look to the code. We have few registers we don't correctly emulate. The current behavior seems to be inconsistent, we either inject a data abort (such as ICPENDR) or ignore the error (such as ICACTIVER). Shall we take a domain_crash approach (as bad_width) or inject a data abort? Regards,
On 21/01/15 12:36, Ian Campbell wrote: > On Wed, 2015-01-21 at 12:28 +0000, Julien Grall wrote: > >>> 3) Accesses which are valid but which do not correctly emulate according >>> to the features of the virtual gic which we are exposing can log if we >>> think it is useful to do so. >> >> I gave a look to the code. We have few registers we don't correctly >> emulate. The current behavior seems to be inconsistent, we either inject >> a data abort (such as ICPENDR) or ignore the error (such as ICACTIVER). >> >> Shall we take a domain_crash approach (as bad_width) or inject a data abort? > > I think for these cases since we do update and/or return > rank->iactive/ipend a debug log and continue is appropriate. > > Assuming we do need to do something more than track the status of > i{active,pend}, which I expect we do -- e.g. to cancel or inject as > appropriate. This code is actually buggy as we never use those fields. I have a patch to drop iactive/ipend fields as we will unlikely handle ACTIVE/PENDING status via those bit. We already track in different way as we already have tracking per vIRQ. My plan was to replace them with an error log and maybe a data abort. If you think a debug message is enough, I could go this way. Though, it may be more difficult for a developer to find the actual error if there is may logs. Regards,
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c index 1aa2f58..1fa1413 100644 --- a/xen/arch/arm/vgic-v3.c +++ b/xen/arch/arm/vgic-v3.c @@ -123,22 +123,22 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info, goto read_as_zero; case GICR_SETLPIR: /* WO. Read as zero */ - goto read_as_zero_64; + goto read_as_zero; case GICR_CLRLPIR: /* WO. Read as zero */ - goto read_as_zero_64; + goto read_as_zero; case GICR_PROPBASER: /* LPI's not implemented */ - goto read_as_zero_64; + goto read_as_zero; case GICR_PENDBASER: /* LPI's not implemented */ - goto read_as_zero_64; + goto read_as_zero; case GICR_INVLPIR: /* WO. Read as zero */ - goto read_as_zero_64; + goto read_as_zero; case GICR_INVALLR: /* WO. Read as zero */ - goto read_as_zero_64; + goto read_as_zero; return 0; case GICR_SYNCR: if ( dabt.size != DABT_WORD ) goto bad_width; @@ -147,10 +147,10 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info, return 1; case GICR_MOVLPIR: /* WO Read as zero */ - goto read_as_zero_64; + goto read_as_zero; case GICR_MOVALLR: /* WO Read as zero */ - goto read_as_zero_64; + goto read_as_zero; case GICR_PIDR0: if ( dabt.size != DABT_WORD ) goto bad_width; *r = GICV3_GICR_PIDR0; @@ -184,13 +184,9 @@ bad_width: domain_crash_synchronous(); return 0; -read_as_zero_64: - if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width; - *r = 0; - return 1; - read_as_zero: - if ( dabt.size != DABT_WORD ) goto bad_width; + gdprintk(XENLOG_DEBUG, "vGICR: read as zero width, %d r%d offset %#08x\n", + dabt.size, dabt.reg, gicr_reg); *r = 0; return 1; } @@ -199,8 +195,6 @@ static int __vgic_v3_rdistr_rd_mmio_write(struct vcpu *v, mmio_info_t *info, uint32_t gicr_reg) { struct hsr_dabt dabt = info->dabt; - struct cpu_user_regs *regs = guest_cpu_user_regs(); - register_t *r = select_user_reg(regs, dabt.reg); switch ( gicr_reg ) { @@ -212,7 +206,7 @@ static int __vgic_v3_rdistr_rd_mmio_write(struct vcpu *v, mmio_info_t *info, goto write_ignore; case GICR_TYPER: /* RO */ - goto write_ignore_64; + goto write_ignore; case GICR_STATUSR: /* Not implemented */ goto write_ignore; @@ -221,31 +215,31 @@ static int __vgic_v3_rdistr_rd_mmio_write(struct vcpu *v, mmio_info_t *info, goto write_ignore; case GICR_SETLPIR: /* LPI is not implemented */ - goto write_ignore_64; + goto write_ignore; case GICR_CLRLPIR: /* LPI is not implemented */ - goto write_ignore_64; + goto write_ignore; case GICR_PROPBASER: /* LPI is not implemented */ - goto write_ignore_64; + goto write_ignore; case GICR_PENDBASER: /* LPI is not implemented */ - goto write_ignore_64; + goto write_ignore; case GICR_INVLPIR: /* LPI is not implemented */ - goto write_ignore_64; + goto write_ignore; case GICR_INVALLR: /* LPI is not implemented */ - goto write_ignore_64; + goto write_ignore; case GICR_SYNCR: /* RO */ goto write_ignore; case GICR_MOVLPIR: /* LPI is not implemented */ - goto write_ignore_64; + goto write_ignore; case GICR_MOVALLR: /* LPI is not implemented */ - goto write_ignore_64; + goto write_ignore; case GICR_PIDR7... GICR_PIDR0: /* RO */ goto write_ignore; @@ -253,18 +247,9 @@ static int __vgic_v3_rdistr_rd_mmio_write(struct vcpu *v, mmio_info_t *info, printk("vGICR: write r%d offset %#08x\n not found", dabt.reg, gicr_reg); return 0; } -bad_width: - printk("vGICR: bad write width %d r%d=%"PRIregister" offset %#08x\n", - dabt.size, dabt.reg, *r, gicr_reg); - domain_crash_synchronous(); - return 0; - -write_ignore_64: - if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width; - return 1; - write_ignore: - if ( dabt.size != DABT_WORD ) goto bad_width; + gdprintk(XENLOG_DEBUG, "vGICR: ignore write width %d r%d offset %#08x\n", + dabt.size, dabt.reg, gicr_reg); return 1; } @@ -364,7 +349,9 @@ bad_width: return 0; read_as_zero: - if ( dabt.size != DABT_WORD ) goto bad_width; + gdprintk(XENLOG_DEBUG, + "vGIC{D,R}: read as zero width, %d r%d offset %#08x\n", + dabt.size, dabt.reg, reg); *r = 0; return 1; } @@ -477,7 +464,9 @@ bad_width: return 0; write_ignore: - if ( dabt.size != DABT_WORD ) goto bad_width; + gdprintk(XENLOG_DEBUG, + "vGIC{D,R}: ignore write width %d r%d offset %#08x\n", + dabt.size, dabt.reg, reg); return 1; } @@ -538,7 +527,9 @@ bad_width: return 0; read_as_zero: - if ( dabt.size != DABT_WORD ) goto bad_width; + gdprintk(XENLOG_DEBUG, + "vGICR: SGI: read as zero width, %d r%d offset %#08x\n", + dabt.size, dabt.reg, gicr_reg); *r = 0; return 1; } @@ -603,7 +594,8 @@ bad_width: return 0; write_ignore: - if ( dabt.size != DABT_WORD ) goto bad_width; + gdprintk(XENLOG_DEBUG, "vGICR: SGI: ignore write width %d r%d offset %#08x\n", + dabt.size, dabt.reg, gicr_reg); return 1; } @@ -732,7 +724,7 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info) return __vgic_v3_distr_common_mmio_read(v, info, gicd_reg); case GICD_IROUTER ... GICD_IROUTER31: /* SGI/PPI is RES0 */ - goto read_as_zero_64; + goto read_as_zero; case GICD_IROUTER32 ... GICD_IROUTERN: if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width; rank = vgic_rank_offset(v, 64, gicd_reg - GICD_IROUTER, @@ -797,8 +789,6 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info) case 0xf30 ... 0x5fcc: case 0x8000 ... 0xbfcc: /* These are reserved register addresses */ - printk("vGICv3: vGICD: read unknown 0x00c .. 0xfcc r%d offset %#08x\n", - dabt.reg, gicd_reg); goto read_as_zero; default: printk("vGICv3: vGICD: unhandled read r%d offset %#08x\n", @@ -812,13 +802,9 @@ bad_width: domain_crash_synchronous(); return 0; -read_as_zero_64: - if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width; - *r = 0; - return 1; - read_as_zero: - if ( dabt.size != DABT_WORD ) goto bad_width; + gdprintk(XENLOG_DEBUG, "vGICD: read as zero width, %d r%d offset %#08x\n", + dabt.size, dabt.reg, gicd_reg); *r = 0; return 1; } @@ -891,12 +877,12 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info) return __vgic_v3_distr_common_mmio_write(v, info, gicd_reg); case GICD_IROUTER ... GICD_IROUTER31: /* SGI/PPI is RES0 */ - goto write_ignore_64; + goto write_ignore; case GICD_IROUTER32 ... GICD_IROUTERN: if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width; rank = vgic_rank_offset(v, 64, gicd_reg - GICD_IROUTER, DABT_DOUBLE_WORD); - if ( rank == NULL ) goto write_ignore_64; + if ( rank == NULL ) goto write_ignore; BUG_ON(v->domain->max_vcpus > 8); new_irouter = *r; vgic_lock_rank(v, rank, flags); @@ -977,11 +963,8 @@ bad_width: return 0; write_ignore: - if ( dabt.size != DABT_WORD ) goto bad_width; - return 1; - -write_ignore_64: - if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width; + gdprintk(XENLOG_DEBUG, "vGICD: ignore write width %d r%d offset %#08x\n", + dabt.size, dabt.reg, gicd_reg); return 1; }
In general, it's not necessary/important to check the size. It's better to log it to let know the guest that its access will have no effect. Note: On debug build it may happen to see some of these messages during domain boot. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- xen/arch/arm/vgic-v3.c | 95 +++++++++++++++++++++----------------------------- 1 file changed, 39 insertions(+), 56 deletions(-)