| Message ID | 20220310174501.62040-5-ayankuma@xilinx.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm64: io: Decode ldr/str post-indexing instruction | expand |
On Thu, 10 Mar 2022, Ayan Kumar Halder wrote: > 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. > > xen/arch/arm/include/asm/mmio.h | 1 + > xen/arch/arm/io.c | 20 ++++++++++++++++++++ > xen/arch/arm/ioreq.c | 15 ++++++++++++++- > 3 files changed, 35 insertions(+), 1 deletion(-) > > 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 e6c77e16bf..c5b2980a5f 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 > @@ -177,6 +188,15 @@ 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 cc9bf23213..0dd2d452f7 100644 > --- a/xen/arch/arm/ioreq.c > +++ b/xen/arch/arm/ioreq.c > @@ -29,10 +29,14 @@ enum io_state handle_ioserv(struct cpu_user_regs *regs, struct vcpu *v) > const struct hsr_dabt dabt = hsr.dabt; > /* Code is similar to handle_read */ > register_t r = v->io.req.data; > + const struct instr_details instr = v->io.info.dabt_instr; > > /* We are done with the IO */ > v->io.req.state = STATE_IOREQ_NONE; > > + if ( instr.state == INSTR_CACHE ) > + return IO_HANDLED; It might be possible to get rid of this check here by rearranging the code in try_handle_mmio a little bit so that handle_ioserv is not called when INSTR_CACHE. But I don't have an opinion about it. The patch does what it says on the tin and as far as I can tell followed Julien's requests so: Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Hi Stefano, On 11/03/2022 02:25, Stefano Stabellini wrote: > On Thu, 10 Mar 2022, Ayan Kumar Halder wrote: >> 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. >> >> xen/arch/arm/include/asm/mmio.h | 1 + >> xen/arch/arm/io.c | 20 ++++++++++++++++++++ >> xen/arch/arm/ioreq.c | 15 ++++++++++++++- >> 3 files changed, 35 insertions(+), 1 deletion(-) >> >> 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 e6c77e16bf..c5b2980a5f 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 >> @@ -177,6 +188,15 @@ 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 cc9bf23213..0dd2d452f7 100644 >> --- a/xen/arch/arm/ioreq.c >> +++ b/xen/arch/arm/ioreq.c >> @@ -29,10 +29,14 @@ enum io_state handle_ioserv(struct cpu_user_regs *regs, struct vcpu *v) >> const struct hsr_dabt dabt = hsr.dabt; >> /* Code is similar to handle_read */ >> register_t r = v->io.req.data; >> + const struct instr_details instr = v->io.info.dabt_instr; >> >> /* We are done with the IO */ >> v->io.req.state = STATE_IOREQ_NONE; >> >> + if ( instr.state == INSTR_CACHE ) >> + return IO_HANDLED; > It might be possible to get rid of this check here by rearranging the > code in try_handle_mmio a little bit so that handle_ioserv is not called > when INSTR_CACHE. But I don't have an opinion about it. AFAIU, we need to invoke ioreq_server_select() to determine if there is a ioreq server for the faulting gpa. If so, then we know that the address is emulated. I think what you are suggesting is to check this before invoking try_fwd_ioserv() in try_handle_mmio(). If so, the code will look like this :- enum io_state try_handle_mmio(struct cpu_user_regs *regs, mmio_info_t *info) { struct vcpu *v = current; const struct mmio_handler *handler = NULL; int rc; ASSERT(info->dabt.ec == HSR_EC_DATA_ABORT_LOWER_EL); if ( !info->dabt.valid ) { ASSERT_UNREACHABLE(); return IO_ABORT; } handler = find_mmio_handler(v->domain, info->gpa); if ( !handler ) { #ifdef CONFIG_IOREQ_SERVER struct vcpu_io *vio = &v->io; const struct instr_details instr = info->dabt_instr; struct hsr_dabt dabt = info->dabt; ioreq_t p = { .type = IOREQ_TYPE_COPY, .addr = info->gpa, .size = 1 << info->dabt.size, .count = 1, .dir = !info->dabt.write, /* * On x86, df is used by 'rep' instruction to tell the direction * to iterate (forward or backward). * On Arm, all the accesses to MMIO region will do a single * 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; enum io_state rc; if ( vio->req.state != STATE_IOREQ_NONE ) { gdprintk(XENLOG_ERR, "wrong state %u\n", vio->req.state); return IO_ABORT; } s = ioreq_server_select(v->domain, &p); if ( !s ) return IO_UNHANDLED; if ( instr.state == INSTR_CACHE ) { return IO_HANDLED; } rc = try_fwd_ioserv(vio, &p, s); if ( rc == IO_HANDLED ) return handle_ioserv(regs, v); #endif return rc; } I am not be inclined to have "#ifdef CONFIG_IOREQ_SERVER" in xen/xen/arch/arm/io.c as the file is generic. - Ayan > > The patch does what it says on the tin and as far as I can tell followed > Julien's requests so: > > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Hi, On 10/03/2022 17:45, Ayan Kumar Halder wrote: > 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. > > xen/arch/arm/include/asm/mmio.h | 1 + > xen/arch/arm/io.c | 20 ++++++++++++++++++++ > xen/arch/arm/ioreq.c | 15 ++++++++++++++- > 3 files changed, 35 insertions(+), 1 deletion(-) > > 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 e6c77e16bf..c5b2980a5f 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 > @@ -177,6 +188,15 @@ 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 cc9bf23213..0dd2d452f7 100644 > --- a/xen/arch/arm/ioreq.c > +++ b/xen/arch/arm/ioreq.c > @@ -29,10 +29,14 @@ enum io_state handle_ioserv(struct cpu_user_regs *regs, struct vcpu *v) > const struct hsr_dabt dabt = hsr.dabt; > /* Code is similar to handle_read */ > register_t r = v->io.req.data; > + const struct instr_details instr = v->io.info.dabt_instr; > > /* We are done with the IO */ > v->io.req.state = STATE_IOREQ_NONE; > > + if ( instr.state == INSTR_CACHE ) > + return IO_HANDLED; The request will not be forwarded to the IOREQ, so why do we need check instr.state here? > + > if ( dabt.write ) > return IO_HANDLED; > > @@ -47,7 +51,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, > @@ -78,6 +82,15 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs, > 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); > > vio->req = p; Cheers,
Hi, On 11/03/2022 13:21, Ayan Kumar Halder wrote: > enum io_state try_handle_mmio(struct cpu_user_regs *regs, > mmio_info_t *info) > { > struct vcpu *v = current; > const struct mmio_handler *handler = NULL; > int rc; > > ASSERT(info->dabt.ec == HSR_EC_DATA_ABORT_LOWER_EL); > > if ( !info->dabt.valid ) > { > ASSERT_UNREACHABLE(); > return IO_ABORT; > } > > handler = find_mmio_handler(v->domain, info->gpa); > if ( !handler ) > { > > #ifdef CONFIG_IOREQ_SERVER > > struct vcpu_io *vio = &v->io; > const struct instr_details instr = info->dabt_instr; > struct hsr_dabt dabt = info->dabt; > ioreq_t p = { > .type = IOREQ_TYPE_COPY, > .addr = info->gpa, > .size = 1 << info->dabt.size, > .count = 1, > .dir = !info->dabt.write, > /* > * On x86, df is used by 'rep' instruction to tell the direction > * to iterate (forward or backward). > * On Arm, all the accesses to MMIO region will do a single > * 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; > enum io_state rc; > > if ( vio->req.state != STATE_IOREQ_NONE ) > { > gdprintk(XENLOG_ERR, "wrong state %u\n", vio->req.state); > return IO_ABORT; > } > > s = ioreq_server_select(v->domain, &p); > if ( !s ) > return IO_UNHANDLED; > > if ( instr.state == INSTR_CACHE ) > { > return IO_HANDLED; > } > > rc = try_fwd_ioserv(vio, &p, s); > if ( rc == IO_HANDLED ) > return handle_ioserv(regs, v); > > #endif > return rc; > } > > I am not be inclined to have "#ifdef CONFIG_IOREQ_SERVER" in > xen/xen/arch/arm/io.c as the file is generic. +1. I much prefer your first approach. Cheers,
Hi Julien, On 11/03/2022 18:34, Julien Grall wrote: > Hi, > > On 10/03/2022 17:45, Ayan Kumar Halder wrote: >> 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. >> >> xen/arch/arm/include/asm/mmio.h | 1 + >> xen/arch/arm/io.c | 20 ++++++++++++++++++++ >> xen/arch/arm/ioreq.c | 15 ++++++++++++++- >> 3 files changed, 35 insertions(+), 1 deletion(-) >> >> 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 e6c77e16bf..c5b2980a5f 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 >> @@ -177,6 +188,15 @@ 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 cc9bf23213..0dd2d452f7 100644 >> --- a/xen/arch/arm/ioreq.c >> +++ b/xen/arch/arm/ioreq.c >> @@ -29,10 +29,14 @@ enum io_state handle_ioserv(struct cpu_user_regs >> *regs, struct vcpu *v) >> const struct hsr_dabt dabt = hsr.dabt; >> /* Code is similar to handle_read */ >> register_t r = v->io.req.data; >> + const struct instr_details instr = v->io.info.dabt_instr; >> /* We are done with the IO */ >> v->io.req.state = STATE_IOREQ_NONE; >> + if ( instr.state == INSTR_CACHE ) >> + return IO_HANDLED; > > The request will not be forwarded to the IOREQ, so why do we need > check instr.state here? I think it is not needed for the following reason. leave_hypervisor_to_guest() ---> check_for_vcpu_work() --> vcpu_ioreq_handle_completion() --> get_pending_vcpu(v, &s) will return NULL. Is my understanding correct ? - Ayan > >> + >> if ( dabt.write ) >> return IO_HANDLED; >> @@ -47,7 +51,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, >> @@ -78,6 +82,15 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs >> *regs, >> 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); >> vio->req = p; > > 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 e6c77e16bf..c5b2980a5f 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 @@ -177,6 +188,15 @@ 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 cc9bf23213..0dd2d452f7 100644 --- a/xen/arch/arm/ioreq.c +++ b/xen/arch/arm/ioreq.c @@ -29,10 +29,14 @@ enum io_state handle_ioserv(struct cpu_user_regs *regs, struct vcpu *v) const struct hsr_dabt dabt = hsr.dabt; /* Code is similar to handle_read */ register_t r = v->io.req.data; + const struct instr_details instr = v->io.info.dabt_instr; /* We are done with the IO */ v->io.req.state = STATE_IOREQ_NONE; + if ( instr.state == INSTR_CACHE ) + return IO_HANDLED; + if ( dabt.write ) return IO_HANDLED; @@ -47,7 +51,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, @@ -78,6 +82,15 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs, 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); vio->req = p;
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. xen/arch/arm/include/asm/mmio.h | 1 + xen/arch/arm/io.c | 20 ++++++++++++++++++++ xen/arch/arm/ioreq.c | 15 ++++++++++++++- 3 files changed, 35 insertions(+), 1 deletion(-)