| Message ID | 20220324133705.37882-1-ayankuma@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v12] xen/arm64: io: Handle data abort due to cache maintenance instructions | expand |
Hi Ayan, On 24/03/2022 13:37, Ayan Kumar Halder wrote: > /* > * At this point, we know that the instruction is either valid or has been > * decoded successfully. Thus, Xen should be allowed to execute the > diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c > index 54167aebcb..87a6240f2a 100644 > --- a/xen/arch/arm/ioreq.c > +++ b/xen/arch/arm/ioreq.c > @@ -47,7 +47,7 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs, > struct vcpu *v, mmio_info_t *info) > { > struct vcpu_io *vio = &v->io; > - struct instr_details instr = info->dabt_instr; > + const struct instr_details instr = info->dabt_instr; > struct hsr_dabt dabt = info->dabt; > ioreq_t p = { > .type = IOREQ_TYPE_COPY, > @@ -62,7 +62,6 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs, > * memory access. So for now, we can safely always set to 0. > */ > .df = 0, > - .data = get_user_reg(regs, info->dabt.reg), > .state = STATE_IOREQ_READY, > }; > struct ioreq_server *s = NULL; > @@ -74,12 +73,23 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs, > return IO_ABORT; > } > > + if ( instr.state == INSTR_CACHE ) > + p.size = dcache_line_bytes; I think it would be best to only set the p.size when instr.state != INSTR_CACHE in the else here. I can do that on commit. I will also give a chance to Stefano to reply. Cheers,
On Fri, 1 Apr 2022, Julien Grall wrote: > On 24/03/2022 13:37, Ayan Kumar Halder wrote: > > /* > > * At this point, we know that the instruction is either valid or has > > been > > * decoded successfully. Thus, Xen should be allowed to execute the > > diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c > > index 54167aebcb..87a6240f2a 100644 > > --- a/xen/arch/arm/ioreq.c > > +++ b/xen/arch/arm/ioreq.c > > @@ -47,7 +47,7 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs, > > struct vcpu *v, mmio_info_t *info) > > { > > struct vcpu_io *vio = &v->io; > > - struct instr_details instr = info->dabt_instr; > > + const struct instr_details instr = info->dabt_instr; > > struct hsr_dabt dabt = info->dabt; > > ioreq_t p = { > > .type = IOREQ_TYPE_COPY, > > @@ -62,7 +62,6 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs, > > * memory access. So for now, we can safely always set to 0. > > */ > > .df = 0, > > - .data = get_user_reg(regs, info->dabt.reg), > > .state = STATE_IOREQ_READY, > > }; > > struct ioreq_server *s = NULL; > > @@ -74,12 +73,23 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs, > > return IO_ABORT; > > } > > + if ( instr.state == INSTR_CACHE ) > > + p.size = dcache_line_bytes; > I think it would be best to only set the p.size when instr.state != > INSTR_CACHE in the else here. > > I can do that on commit. I will also give a chance to Stefano to reply. The patch looks OK to me, please go ahead and make the change on commit.
Hi, On 18/04/2022 22:02, Stefano Stabellini wrote: > On Fri, 1 Apr 2022, Julien Grall wrote: >> On 24/03/2022 13:37, Ayan Kumar Halder wrote: >>> /* >>> * At this point, we know that the instruction is either valid or has >>> been >>> * decoded successfully. Thus, Xen should be allowed to execute the >>> diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c >>> index 54167aebcb..87a6240f2a 100644 >>> --- a/xen/arch/arm/ioreq.c >>> +++ b/xen/arch/arm/ioreq.c >>> @@ -47,7 +47,7 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs, >>> struct vcpu *v, mmio_info_t *info) >>> { >>> struct vcpu_io *vio = &v->io; >>> - struct instr_details instr = info->dabt_instr; >>> + const struct instr_details instr = info->dabt_instr; >>> struct hsr_dabt dabt = info->dabt; >>> ioreq_t p = { >>> .type = IOREQ_TYPE_COPY, >>> @@ -62,7 +62,6 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs, >>> * memory access. So for now, we can safely always set to 0. >>> */ >>> .df = 0, >>> - .data = get_user_reg(regs, info->dabt.reg), >>> .state = STATE_IOREQ_READY, >>> }; >>> struct ioreq_server *s = NULL; >>> @@ -74,12 +73,23 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs, >>> return IO_ABORT; >>> } >>> + if ( instr.state == INSTR_CACHE ) >>> + p.size = dcache_line_bytes; >> I think it would be best to only set the p.size when instr.state != >> INSTR_CACHE in the else here. >> >> I can do that on commit. I will also give a chance to Stefano to reply. > > The patch looks OK to me, please go ahead and make the change on > commit. I have commited with my reviewed-by tag after folding the following diff: For the record, I folded this diff: diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c index 87a6240f2aa9..bdd536e873e5 100644 --- a/xen/arch/arm/ioreq.c +++ b/xen/arch/arm/ioreq.c @@ -52,7 +52,6 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs, ioreq_t p = { .type = IOREQ_TYPE_COPY, .addr = info->gpa, - .size = 1 << info->dabt.size, .count = 1, .dir = !info->dabt.write, /* @@ -75,6 +74,8 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs, if ( instr.state == INSTR_CACHE ) p.size = dcache_line_bytes; + else + p.size = 1U << info->dabt.size; s = ioreq_server_select(v->domain, &p); if ( !s ) Cheers,
diff --git a/xen/arch/arm/include/asm/mmio.h b/xen/arch/arm/include/asm/mmio.h index ca259a79c2..79e64d9af8 100644 --- a/xen/arch/arm/include/asm/mmio.h +++ b/xen/arch/arm/include/asm/mmio.h @@ -35,6 +35,7 @@ enum instr_decode_state * instruction. */ INSTR_LDR_STR_POSTINDEXING, + INSTR_CACHE, /* Cache Maintenance instr */ }; typedef struct diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c index 6f458ee7fd..4ce94243aa 100644 --- a/xen/arch/arm/io.c +++ b/xen/arch/arm/io.c @@ -139,6 +139,17 @@ void try_decode_instruction(const struct cpu_user_regs *regs, return; } + /* + * When the data abort is caused due to cache maintenance, Xen should check + * if the address belongs to an emulated MMIO region or not. The behavior + * will differ accordingly. + */ + if ( info->dabt.cache ) + { + info->dabt_instr.state = INSTR_CACHE; + return; + } + /* * Armv8 processor does not provide a valid syndrome for decoding some * instructions. So in order to process these instructions, Xen must @@ -161,7 +172,7 @@ enum io_state try_handle_mmio(struct cpu_user_regs *regs, ASSERT(info->dabt.ec == HSR_EC_DATA_ABORT_LOWER_EL); - if ( !info->dabt.valid ) + if ( !(info->dabt.valid || (info->dabt_instr.state == INSTR_CACHE)) ) { ASSERT_UNREACHABLE(); return IO_ABORT; @@ -177,6 +188,13 @@ enum io_state try_handle_mmio(struct cpu_user_regs *regs, return rc; } + /* + * When the data abort is caused due to cache maintenance and the address + * belongs to an emulated region, Xen should ignore this instruction. + */ + if ( info->dabt_instr.state == INSTR_CACHE ) + return IO_HANDLED; + /* * At this point, we know that the instruction is either valid or has been * decoded successfully. Thus, Xen should be allowed to execute the diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c index 54167aebcb..87a6240f2a 100644 --- a/xen/arch/arm/ioreq.c +++ b/xen/arch/arm/ioreq.c @@ -47,7 +47,7 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs, struct vcpu *v, mmio_info_t *info) { struct vcpu_io *vio = &v->io; - struct instr_details instr = info->dabt_instr; + const struct instr_details instr = info->dabt_instr; struct hsr_dabt dabt = info->dabt; ioreq_t p = { .type = IOREQ_TYPE_COPY, @@ -62,7 +62,6 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs, * memory access. So for now, we can safely always set to 0. */ .df = 0, - .data = get_user_reg(regs, info->dabt.reg), .state = STATE_IOREQ_READY, }; struct ioreq_server *s = NULL; @@ -74,12 +73,23 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs, return IO_ABORT; } + if ( instr.state == INSTR_CACHE ) + p.size = dcache_line_bytes; + s = ioreq_server_select(v->domain, &p); if ( !s ) return IO_UNHANDLED; + /* + * When the data abort is caused due to cache maintenance and the address + * belongs to an emulated region, Xen should ignore this instruction. + */ + if ( instr.state == INSTR_CACHE ) + return IO_HANDLED; + ASSERT(dabt.valid); + p.data = get_user_reg(regs, info->dabt.reg); vio->req = p; vio->info.dabt_instr = instr;
When the data abort is caused due to cache maintenance for an address, there are three scenarios:- 1. Address belonging to a non emulated region - For this, Xen should set the corresponding bit in the translation table entry to valid and return to the guest to retry the instruction. This can happen sometimes as Xen need to set the translation table entry to invalid. (for eg 'Break-Before-Make' sequence). Xen returns to the guest to retry the instruction. 2. Address belongs to an emulated region - Xen should ignore the instruction (ie increment the PC) and return to the guest. 3. Address is invalid - Xen should forward the data abort to the guest. Signed-off-by: Ayan Kumar Halder <ayankuma@xilinx.com> --- Changelog:- v1...v8 - NA v9 - Extracted this change from "[XEN v7 2/2] xen/arm64: io: Support instructions (for which ISS is not ..." into a separate patch of its own. The reason being this addresses an existing bug in the codebase. v10 - 1. To check if the address belongs to an emulated region, one needs to check if it has a mmio handler or an ioreq server. In this case, Xen should increment the PC 2. If the address is invalid (niether emulated MMIO nor the translation could be resolved via p2m or mapping the MMIO region), then Xen should forward the abort to the guest. v11 - 1. Removed the un-necessary check "( instr.state == INSTR_CACHE )" in handle_ioserv(). The reason being the ioserv request is not forwarded by try_fwd_ioserv() when instr.state == INSTR_CACHE. v12 - 1. try_handle_mmio() should assert for "(info->dabt.valid || (info->dabt_instr.state == INSTR_CACHE))" 2. In try_fwd_ioserv(), ioreq size should be set to the cache line size for INSTR_CACHE. Also ioreq data should be set only when ISS is valid. xen/arch/arm/include/asm/mmio.h | 1 + xen/arch/arm/io.c | 20 +++++++++++++++++++- xen/arch/arm/ioreq.c | 14 ++++++++++++-- 3 files changed, 32 insertions(+), 3 deletions(-)