[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [XEN v5] xen/arm64: io: Decode ldr/str post-indexing instructions
Hi,
On 31/01/2022 19:37, Ayan Kumar Halder wrote:
At the moment, Xen is only handling data abort with valid syndrome (i.e.
ISV=0). Unfortunately, this doesn't cover all the instructions a domain
could use to access MMIO regions.
For instance, a baremetal OS can use any of the following instructions, where
x1 contains the address of the MMIO region:
1. ldr x2, [x1], #8
2. ldr w2, [x1], #-4
3. ldr x2, [x1], #-8
4. ldr w2, [x1], #4
5. ldrh w2, [x1], #2
6. ldrb w2, [x1], #1
7. str x2, [x1], #8
8. str w2, [x1], #-4
9. strh w2, [x1], #2
10. strb w2, [x1], #1
In the following two instructions, Rn could theoretically be stack pointer which
might contain the address of the MMIO region:-
11. ldrb w2, [Rn], #1
12. ldrb wzr, [Rn], #1
In order to handle post-indexing store/load instructions (like those mentioned
above), Xen will need to fetch and decode the instruction.
Xen will not decode the instructions if the data abort is caused by stage1
translation table walk or cache instructions. In the former scenario, it will
try to update the page tables and in the latter scenario, it will ignore the
instruction (ie simply increment the program counter).
If Xen is unable to decode the instructions, it will abort the guest.
This patch only cover post-index store/load instructions from AArch64 mode.
For now, AArch32 mode is left unimplemented.
Signed-off-by: Ayan Kumar Halder <ayankuma@xxxxxxxxxx>
---
Changelog :-
v2 - 1. Updated the rn register after reading from it. (Pointed by Julien,
Stefano)
2. Used a union to represent the instruction opcode (Suggestd by Bertrand)
3. Fixed coding style issues (Pointed by Julien)
4. In the previous patch, I was updating dabt->sign based on the
signedness
of imm9. This was incorrect. As mentioned in ARMv8 ARM DDI 0487G.b,
Page 3221, SSE indicates the signedness of the data item loaded. In our
case, the data item loaded is always unsigned.
v3- 1. Handled all the variants of ldr/str (ie 64, 32, 16, 8 bit variants).
Thus, I have removed the check for "instr->code.opc == 0" (Suggested by
Andre)
2. Handled the scenario when rn = SP, rt = XZR (Suggested by Jan, Andre)
3. Added restriction for "rt != rn" (Suggested by Andre)
4. Moved union ldr_str_instr_class {} to decode.h. This is the header
included
by io.c and decode.c (where the union is referred). (Suggested by Jan)
5. Indentation and typo fixes (Suggested by Jan)
v4- 1. Fixed the patch as per Stefano's comments on v3. They are as follows :-
1.1 Use macros to determine the fixed values in the instruction opcode
1.2 Checked if instr != NULL
1.3 Changed some data types and added #define ARM_64 for AArch64
specific
code
1.4 Moved post_increment_register() to decode.c so that the decoding
logic is confined to a single file.
1.5 Moved some checks from post_increment_register() to
decode_loadstore_postindexing()
1.6 Removed a duplicate check
2. Updated the commit message as per Andre's comments.
3. Changed the names of a label and some comments. *32bit* was erroneously
mentioned in a label and comments in decode_loadstore_postindexing()
although the function handled all variants of ldr/str post indexing.
v5- 1. Renamed decode_loadstore_postindexing() to decode_arm64(). The reason
being this will be extended in future to support more instructions for
which hsr_badt.isv = 0
2. Introduce a function try_decode_instruction_invalid_iss() to determine
if the instruction needs to be decoded before invoking
decode_instruction().
It checks :-
2.1 dabt->s1ptw - Returns IO_UNHANDLED
2.2 dabt->cache - Returns IO_IGNORED. (new enum instroduced to let the
caller know that the instruction needs to be ignored by Xen. Thus
the caller needs to increment the PC and return to the guest.
3. Invoked try_decode_instruction_invalid_iss() from the following 2
places :-
3.a - try_handle_mmio() - When we have determined that there is a valid
mmio handler.
3.b - try_fwd_ioserv()
When ioserver completes the io request, the acknowledgement is sent via
handle_ioserv(). Here, we need to increment the register. As there is
no
common data shared between try_fwd_ioserv() and handle_ioserv(), we
need
to decode the instruction again in handle_ioserv() to determine rn,
imm9.
(NOTE to Reviewers) - This does not feel correct. However, I could not
think of a better approach. Please provide your inputs.
4. Augumented struct hsr_dabt{} with struct hsr_dabt_instr_details{} to
hold
rn and imm9. This is passed to post_increment_register() to update rn.
5. Other style changes as suggested in v4.
Patch has been based on the comments received on the following two patches:-
1. https://lists.xenproject.org/archives/html/xen-devel/2022-01/msg01245.html
2. https://lists.xenproject.org/archives/html/xen-devel/2022-01/msg01279.html
xen/arch/arm/arm32/traps.c | 7 ++++
xen/arch/arm/arm64/traps.c | 42 +++++++++++++++++++++
xen/arch/arm/decode.c | 73 +++++++++++++++++++++++++++++++++++++
xen/arch/arm/decode.h | 42 ++++++++++++++++++++-
xen/arch/arm/io.c | 66 +++++++++++++++++++++++++++------
xen/arch/arm/ioreq.c | 35 +++++++++++++++++-
xen/arch/arm/traps.c | 1 +
xen/include/asm-arm/hsr.h | 5 +++
xen/include/asm-arm/mmio.h | 1 +
xen/include/asm-arm/traps.h | 1 +
I have tried to apply the patch on the latest staging. But this failed
because xen/include/asm-arm was renamed to arch/arm/include/asm.
When sending e-mail to Xen-devel, please make sure to use the latest
staging available.
Cheers,
--
Julien Grall
|
