| Message ID | 20220301124022.10168-5-ayankuma@xilinx.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm64: io: Decode ldr/str post-indexing instruction | expand |
On Tue, 1 Mar 2022, Ayan Kumar Halder wrote: > When the data abort is caused due to cache maintenance for an address, > there are two 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). > > 2. Address belongs to an emulated region - Xen should ignore the > instruction (ie increment the PC) and return to the guest. > > We try to deal with scenario#1, by invoking check_p2m(). If this is > unsuccessful, then we assume scenario#2. > > Signed-off-by: Ayan Kumar Halder <ayankuma@xilinx.com> Acked-by: Stefano Stabellini <sstabellini@kernel.org> > --- > > 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. > > xen/arch/arm/include/asm/mmio.h | 3 ++- > xen/arch/arm/io.c | 11 +++++++++++ > xen/arch/arm/traps.c | 6 ++++++ > 3 files changed, 19 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/arm/include/asm/mmio.h b/xen/arch/arm/include/asm/mmio.h > index ef2c57a2d5..75d362d5f5 100644 > --- a/xen/arch/arm/include/asm/mmio.h > +++ b/xen/arch/arm/include/asm/mmio.h > @@ -34,7 +34,8 @@ enum instr_decode_state > * Instruction is decoded successfully. It is a ldr/str post indexing > * instruction. > */ > - INSTR_LDR_STR_POSTINDEXING > + INSTR_LDR_STR_POSTINDEXING, > + INSTR_IGNORE /* Instruction is ignored */ > }; > > typedef struct > diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c > index ebcb8ed548..7e9dd4bb08 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 ignore > + * this instruction as the cache maintenance was caused on an address belonging > + * to the emulated region. > + */ > + if ( info->dabt.cache ) > + { > + info->dabt_instr.state = INSTR_IGNORE; > + return; > + } > + > /* > * Armv8 processor does not provide a valid syndrome for decoding some > * instructions. So in order to process these instructions, Xen must > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c > index e491ca15d7..5879640b73 100644 > --- a/xen/arch/arm/traps.c > +++ b/xen/arch/arm/traps.c > @@ -2011,6 +2011,12 @@ static void do_trap_stage2_abort_guest(struct cpu_user_regs *regs, > > try_decode_instruction(regs, &info); > > + if ( info.dabt_instr.state == INSTR_IGNORE ) > + { > + advance_pc(regs, hsr); > + return; > + } > + > /* > * If Xen could not decode the instruction or encountered an error > * while decoding, then it should forward the abort to the guest. > -- > 2.17.1 >
Hi Ayan, On 01/03/2022 12:40, Ayan Kumar Halder wrote: > When the data abort is caused due to cache maintenance for an address, > there are two 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). > > 2. Address belongs to an emulated region - Xen should ignore the > instruction (ie increment the PC) and return to the guest. I would be explicit and say something along the lines: "Xen doesn't cache data for emulated regions. So we can safely ignore them". There is a third scenarios: The address belongs to neither an emulated region nor has a valid mapping in the P2M. > > We try to deal with scenario#1, by invoking check_p2m(). If this is > unsuccessful, then we assume scenario#2. This means that you will ignore cache maintenance on invalid region. I think we should send an abort to the guest rather than let them believe it worked. Cheers,
Hi Julien, On 04/03/2022 10:46, Julien Grall wrote: > Hi Ayan, > > On 01/03/2022 12:40, Ayan Kumar Halder wrote: >> When the data abort is caused due to cache maintenance for an address, >> there are two 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). >> >> 2. Address belongs to an emulated region - Xen should ignore the >> instruction (ie increment the PC) and return to the guest. > > I would be explicit and say something along the lines: > > "Xen doesn't cache data for emulated regions. So we can safely ignore > them". > > There is a third scenarios: > > The address belongs to neither an emulated region nor has a valid > mapping in the P2M. To check this, we should test "try_handle_mmio() == IO_UNHANDLED". If so then send an abort to the guest. Is this correct ? - Ayan >> >> We try to deal with scenario#1, by invoking check_p2m(). If this is >> unsuccessful, then we assume scenario#2. > > This means that you will ignore cache maintenance on invalid region. I > think we should send an abort to the guest rather than let them > believe it worked. > > Cheers, >
On 04/03/2022 12:13, Ayan Kumar Halder wrote: > Hi Julien, Hi, > > On 04/03/2022 10:46, Julien Grall wrote: >> Hi Ayan, >> >> On 01/03/2022 12:40, Ayan Kumar Halder wrote: >>> When the data abort is caused due to cache maintenance for an address, >>> there are two 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). >>> >>> 2. Address belongs to an emulated region - Xen should ignore the >>> instruction (ie increment the PC) and return to the guest. >> >> I would be explicit and say something along the lines: >> >> "Xen doesn't cache data for emulated regions. So we can safely ignore >> them". >> >> There is a third scenarios: >> >> The address belongs to neither an emulated region nor has a valid >> mapping in the P2M. > > To check this, we should test "try_handle_mmio() == IO_UNHANDLED". If so > then send an abort to the guest. > > Is this correct ? I think it would be too late because if the region is emulated, then we would have already tried to handle it. Instead, I think we need to check after we confirmed that the region is emulated or we need to forward to an IOREQ server. So the check would have to be duplicated here. Cheers,
Hi Julien, I have a question. On 04/03/2022 12:49, Julien Grall wrote: > > > On 04/03/2022 12:13, Ayan Kumar Halder wrote: >> Hi Julien, > > Hi, > >> >> On 04/03/2022 10:46, Julien Grall wrote: >>> Hi Ayan, >>> >>> On 01/03/2022 12:40, Ayan Kumar Halder wrote: >>>> When the data abort is caused due to cache maintenance for an address, >>>> there are two 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). >>>> >>>> 2. Address belongs to an emulated region - Xen should ignore the >>>> instruction (ie increment the PC) and return to the guest. >>> >>> I would be explicit and say something along the lines: >>> >>> "Xen doesn't cache data for emulated regions. So we can safely >>> ignore them". >>> >>> There is a third scenarios: >>> >>> The address belongs to neither an emulated region nor has a valid >>> mapping in the P2M. >> >> To check this, we should test "try_handle_mmio() == IO_UNHANDLED". If >> so then send an abort to the guest. >> >> Is this correct ? > I think it would be too late because if the region is emulated, then > we would have already tried to handle it. > > Instead, I think we need to check after we confirmed that the region > is emulated or we need to forward to an IOREQ server. > > So the check would have to be duplicated here. When do we know that a particular address does not belong to an emulated MMIO region ? Is this after both "find_mmio_handler()" and "ioreq_server_select()" have returned NULL ? - Ayan > > Cheers, >
On 04/03/2022 14:40, Ayan Kumar Halder wrote: > Hi Julien, Hi, > I have a question. > > On 04/03/2022 12:49, Julien Grall wrote: >> >> >> On 04/03/2022 12:13, Ayan Kumar Halder wrote: >>> Hi Julien, >> >> Hi, >> >>> >>> On 04/03/2022 10:46, Julien Grall wrote: >>>> Hi Ayan, >>>> >>>> On 01/03/2022 12:40, Ayan Kumar Halder wrote: >>>>> When the data abort is caused due to cache maintenance for an address, >>>>> there are two 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). >>>>> >>>>> 2. Address belongs to an emulated region - Xen should ignore the >>>>> instruction (ie increment the PC) and return to the guest. >>>> >>>> I would be explicit and say something along the lines: >>>> >>>> "Xen doesn't cache data for emulated regions. So we can safely >>>> ignore them". >>>> >>>> There is a third scenarios: >>>> >>>> The address belongs to neither an emulated region nor has a valid >>>> mapping in the P2M. >>> >>> To check this, we should test "try_handle_mmio() == IO_UNHANDLED". If >>> so then send an abort to the guest. >>> >>> Is this correct ? >> I think it would be too late because if the region is emulated, then >> we would have already tried to handle it. >> >> Instead, I think we need to check after we confirmed that the region >> is emulated or we need to forward to an IOREQ server. >> >> So the check would have to be duplicated here. > > When do we know that a particular address does not belong to an emulated > MMIO region ? > > Is this after both "find_mmio_handler()" and "ioreq_server_select()" > have returned NULL ? Correct. Cheers,
diff --git a/xen/arch/arm/include/asm/mmio.h b/xen/arch/arm/include/asm/mmio.h index ef2c57a2d5..75d362d5f5 100644 --- a/xen/arch/arm/include/asm/mmio.h +++ b/xen/arch/arm/include/asm/mmio.h @@ -34,7 +34,8 @@ enum instr_decode_state * Instruction is decoded successfully. It is a ldr/str post indexing * instruction. */ - INSTR_LDR_STR_POSTINDEXING + INSTR_LDR_STR_POSTINDEXING, + INSTR_IGNORE /* Instruction is ignored */ }; typedef struct diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c index ebcb8ed548..7e9dd4bb08 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 ignore + * this instruction as the cache maintenance was caused on an address belonging + * to the emulated region. + */ + if ( info->dabt.cache ) + { + info->dabt_instr.state = INSTR_IGNORE; + return; + } + /* * Armv8 processor does not provide a valid syndrome for decoding some * instructions. So in order to process these instructions, Xen must diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c index e491ca15d7..5879640b73 100644 --- a/xen/arch/arm/traps.c +++ b/xen/arch/arm/traps.c @@ -2011,6 +2011,12 @@ static void do_trap_stage2_abort_guest(struct cpu_user_regs *regs, try_decode_instruction(regs, &info); + if ( info.dabt_instr.state == INSTR_IGNORE ) + { + advance_pc(regs, hsr); + return; + } + /* * If Xen could not decode the instruction or encountered an error * while decoding, then it should forward the abort to the guest.
When the data abort is caused due to cache maintenance for an address, there are two 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). 2. Address belongs to an emulated region - Xen should ignore the instruction (ie increment the PC) and return to the guest. We try to deal with scenario#1, by invoking check_p2m(). If this is unsuccessful, then we assume scenario#2. 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. xen/arch/arm/include/asm/mmio.h | 3 ++- xen/arch/arm/io.c | 11 +++++++++++ xen/arch/arm/traps.c | 6 ++++++ 3 files changed, 19 insertions(+), 1 deletion(-)