* [XEN v9 0/4] xen/arm64: io: Decode ldr/str post-indexing instruction
@ 2022-03-01 12:40 Ayan Kumar Halder
2022-03-01 12:40 ` [XEN v9 1/4] xen/arm64: Decode ldr/str post increment operations Ayan Kumar Halder
` (3 more replies)
0 siblings, 4 replies; 28+ messages in thread
From: Ayan Kumar Halder @ 2022-03-01 12:40 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, stefanos, julien, Volodymyr_Babchuk,
bertrand.marquis, andrew.cooper3, george.dunlap, jbeulich, wl,
paul, roger.pau, Ayan Kumar Halder
Hi All,
The patch series introduces support to decode instructions by Xen when ISS is
invalid. Currently, when the guest executes post indexing ldr/str instructions
on emulated MMIO, these instructions are trapped into Xen as a data abort.
Xen reads hsr_dabt.isv == 0, so ISS is invalid. Therefore, it reads the faulting
instruction's opcode from guest's PC. It decodes and executes the instruction on
the emulated region.
While doing the patch, we found two bugs in the codebase. I have addressed them
in patches 3 and 4. These bugs were discussed with Julien on IRC chat. The
purpose of addressing these bugs (in this series) is that 1. We should not forget
about them. 2. To get clarity if our understanding is correct.
Ayan Kumar Halder (4):
xen/arm64: Decode ldr/str post increment operations
xen/arm64: io: Support instructions (for which ISS is not valid) on
emulated MMIO region using MMIO/ioreq handler
xen/arm64: io: Handle the abort due to access to stage1 translation
table
xen/arm64: io: Handle data abort due to cache maintenance instructions
xen/arch/arm/arm32/traps.c | 11 +++
xen/arch/arm/arm64/traps.c | 47 +++++++++++++
xen/arch/arm/decode.c | 80 ++++++++++++++++++++-
xen/arch/arm/decode.h | 48 +++++++++++--
xen/arch/arm/include/asm/domain.h | 4 ++
xen/arch/arm/include/asm/mmio.h | 22 +++++-
xen/arch/arm/include/asm/traps.h | 2 +
xen/arch/arm/io.c | 112 +++++++++++++++++++++---------
xen/arch/arm/ioreq.c | 7 +-
xen/arch/arm/traps.c | 93 ++++++++++++++++++++-----
xen/arch/x86/include/asm/ioreq.h | 3 +
xen/include/xen/sched.h | 2 +
12 files changed, 369 insertions(+), 62 deletions(-)
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.
v6 - 1. Split the patch into three parts.
v7 - 1. Merged patch2 and patch3 into a single patch.
v8 - 1. Changes mentioned in the individual patches.
v9 - 1. Added patches 3 and 4 to address the bugs found in the existing codebase.
2. Changes mentioned in the individual patches.
--
2.17.1
^ permalink raw reply [flat|nested] 28+ messages in thread
* [XEN v9 1/4] xen/arm64: Decode ldr/str post increment operations
2022-03-01 12:40 [XEN v9 0/4] xen/arm64: io: Decode ldr/str post-indexing instruction Ayan Kumar Halder
@ 2022-03-01 12:40 ` Ayan Kumar Halder
2022-03-04 0:42 ` Stefano Stabellini
2022-03-01 12:40 ` [XEN v9 2/4] xen/arm64: io: Support instructions (for which ISS is not valid) on emulated MMIO region using MMIO/ioreq handler Ayan Kumar Halder
` (2 subsequent siblings)
3 siblings, 1 reply; 28+ messages in thread
From: Ayan Kumar Halder @ 2022-03-01 12:40 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, stefanos, julien, Volodymyr_Babchuk,
bertrand.marquis, andrew.cooper3, george.dunlap, jbeulich, wl,
paul, roger.pau, Ayan Kumar Halder
At the moment, Xen does not decode any of the arm64 instructions. This
means that when hsr_dabt.isv == 0, Xen cannot handle those instructions.
This will lead to Xen to abort the guests (from which those instructions
originate).
With this patch, Xen is able to decode ldr/str post indexing instructions.
These are a subset of instructions for which hsr_dabt.isv == 0.
The following instructions are now supported by Xen :-
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 subsequent patch, decode_arm64() will get invoked when
hsr_dabt.isv == 0.
Signed-off-by: Ayan Kumar Halder <ayankuma@xilinx.com>
---
Changelog :-
v2..v5 - Mentioned in the cover letter.
v6 - 1. Fixed the code style issues as mentioned in v5.
v7 - No change.
v8 - 1. Removed some un-necessary header files inclusion.
2. Some style changes pointed out in v7.
v9 - 1. Rebased on top of the master.
2. Renamed psr_mode_is_32bit to regs_mode_is_32bit.
xen/arch/arm/decode.c | 79 ++++++++++++++++++++++++++++++++-
xen/arch/arm/decode.h | 48 +++++++++++++++++---
xen/arch/arm/include/asm/mmio.h | 4 ++
xen/arch/arm/io.c | 2 +-
4 files changed, 124 insertions(+), 9 deletions(-)
diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c
index 792c2e92a7..3add87e83a 100644
--- a/xen/arch/arm/decode.c
+++ b/xen/arch/arm/decode.c
@@ -84,6 +84,78 @@ bad_thumb2:
return 1;
}
+static int decode_arm64(register_t pc, mmio_info_t *info)
+{
+ union instr opcode = {0};
+ struct hsr_dabt *dabt = &info->dabt;
+ struct instr_details *dabt_instr = &info->dabt_instr;
+
+ if ( raw_copy_from_guest(&opcode.value, (void * __user)pc, sizeof (opcode)) )
+ {
+ gprintk(XENLOG_ERR, "Could not copy the instruction from PC\n");
+ return 1;
+ }
+
+ /*
+ * Refer Arm v8 ARM DDI 0487G.b, Page - C6-1107
+ * "Shared decode for all encodings" (under ldr immediate)
+ * If n == t && n != 31, then the return value is implementation defined
+ * (can be WBSUPPRESS, UNKNOWN, UNDEFINED or NOP). Thus, we do not support
+ * this. This holds true for ldrb/ldrh immediate as well.
+ *
+ * Also refer, Page - C6-1384, the above described behaviour is same for
+ * str immediate. This holds true for strb/strh immediate as well
+ */
+ if ( (opcode.ldr_str.rn == opcode.ldr_str.rt) && (opcode.ldr_str.rn != 31) )
+ {
+ gprintk(XENLOG_ERR, "Rn should not be equal to Rt except for r31\n");
+ goto bad_loadstore;
+ }
+
+ /* First, let's check for the fixed values */
+ if ( (opcode.value & POST_INDEX_FIXED_MASK) != POST_INDEX_FIXED_VALUE )
+ {
+ gprintk(XENLOG_ERR,
+ "Decoding instruction 0x%x is not supported\n", opcode.value);
+ goto bad_loadstore;
+ }
+
+ if ( opcode.ldr_str.v != 0 )
+ {
+ gprintk(XENLOG_ERR,
+ "ldr/str post indexing for vector types are not supported\n");
+ goto bad_loadstore;
+ }
+
+ /* Check for STR (immediate) */
+ if ( opcode.ldr_str.opc == 0 )
+ dabt->write = 1;
+ /* Check for LDR (immediate) */
+ else if ( opcode.ldr_str.opc == 1 )
+ dabt->write = 0;
+ else
+ {
+ gprintk(XENLOG_ERR,
+ "Decoding ldr/str post indexing is not supported for this variant\n");
+ goto bad_loadstore;
+ }
+
+ gprintk(XENLOG_INFO,
+ "opcode->ldr_str.rt = 0x%x, opcode->ldr_str.size = 0x%x, opcode->ldr_str.imm9 = %d\n",
+ opcode.ldr_str.rt, opcode.ldr_str.size, opcode.ldr_str.imm9);
+
+ update_dabt(dabt, opcode.ldr_str.rt, opcode.ldr_str.size, false);
+
+ dabt_instr->rn = opcode.ldr_str.rn;
+ dabt_instr->imm9 = opcode.ldr_str.imm9;
+
+ return 0;
+
+ bad_loadstore:
+ gprintk(XENLOG_ERR, "unhandled Arm instruction 0x%x\n", opcode.value);
+ return 1;
+}
+
static int decode_thumb(register_t pc, struct hsr_dabt *dabt)
{
uint16_t instr;
@@ -150,10 +222,13 @@ bad_thumb:
return 1;
}
-int decode_instruction(const struct cpu_user_regs *regs, struct hsr_dabt *dabt)
+int decode_instruction(const struct cpu_user_regs *regs, mmio_info_t *info)
{
if ( is_32bit_domain(current->domain) && regs->cpsr & PSR_THUMB )
- return decode_thumb(regs->pc, dabt);
+ return decode_thumb(regs->pc, &info->dabt);
+
+ if ( !regs_mode_is_32bit(regs) )
+ return decode_arm64(regs->pc, info);
/* TODO: Handle ARM instruction */
gprintk(XENLOG_ERR, "unhandled ARM instruction\n");
diff --git a/xen/arch/arm/decode.h b/xen/arch/arm/decode.h
index 4613763bdb..13db8ac968 100644
--- a/xen/arch/arm/decode.h
+++ b/xen/arch/arm/decode.h
@@ -23,19 +23,55 @@
#include <asm/regs.h>
#include <asm/processor.h>
-/**
+/*
+ * Refer to the ARMv8 ARM (DDI 0487G.b), Section C4.1.4 Loads and Stores
+ * Page 318 specifies the following bit pattern for
+ * "load/store register (immediate post-indexed)".
+ *
+ * 31 30 29 27 26 25 23 21 20 11 9 4 0
+ * ___________________________________________________________________
+ * |size|1 1 1 |V |0 0 |opc |0 | imm9 |0 1 | Rn | Rt |
+ * |____|______|__|____|____|__|_______________|____|_________|_______|
+ */
+union instr {
+ uint32_t value;
+ struct {
+ unsigned int rt:5; /* Rt register */
+ unsigned int rn:5; /* Rn register */
+ unsigned int fixed1:2; /* value == 01b */
+ signed int imm9:9; /* imm9 */
+ unsigned int fixed2:1; /* value == 0b */
+ unsigned int opc:2; /* opc */
+ unsigned int fixed3:2; /* value == 00b */
+ unsigned int v:1; /* vector */
+ unsigned int fixed4:3; /* value == 111b */
+ unsigned int size:2; /* size */
+ } ldr_str;
+};
+
+#define POST_INDEX_FIXED_MASK 0x3B200C00
+#define POST_INDEX_FIXED_VALUE 0x38000400
+
+/*
* Decode an instruction from pc
- * /!\ This function is not intended to fully decode an instruction. It
- * considers that the instruction is valid.
+ * /!\ This function is intended to decode an instruction. It considers that the
+ * instruction is valid.
*
- * This function will get:
- * - The transfer register
+ * In case of thumb mode, this function will get:
+ * - The transfer register (ie Rt)
* - Sign bit
* - Size
+ *
+ * In case of arm64 mode, this function will get:
+ * - The transfer register (ie Rt)
+ * - The source register (ie Rn)
+ * - Size
+ * - Immediate offset
+ * - Read or write
*/
int decode_instruction(const struct cpu_user_regs *regs,
- struct hsr_dabt *dabt);
+ mmio_info_t *info);
#endif /* __ARCH_ARM_DECODE_H_ */
diff --git a/xen/arch/arm/include/asm/mmio.h b/xen/arch/arm/include/asm/mmio.h
index 7ab873cb8f..3354d9c635 100644
--- a/xen/arch/arm/include/asm/mmio.h
+++ b/xen/arch/arm/include/asm/mmio.h
@@ -29,6 +29,10 @@
typedef struct
{
struct hsr_dabt dabt;
+ struct instr_details {
+ unsigned long rn:5;
+ signed int imm9:9;
+ } dabt_instr;
paddr_t gpa;
} mmio_info_t;
diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
index 1a066f9ae5..fad103bdbd 100644
--- a/xen/arch/arm/io.c
+++ b/xen/arch/arm/io.c
@@ -141,7 +141,7 @@ enum io_state try_handle_mmio(struct cpu_user_regs *regs,
{
int rc;
- rc = decode_instruction(regs, &info.dabt);
+ rc = decode_instruction(regs, &info);
if ( rc )
{
gprintk(XENLOG_DEBUG, "Unable to decode instruction\n");
--
2.17.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [XEN v9 2/4] xen/arm64: io: Support instructions (for which ISS is not valid) on emulated MMIO region using MMIO/ioreq handler
2022-03-01 12:40 [XEN v9 0/4] xen/arm64: io: Decode ldr/str post-indexing instruction Ayan Kumar Halder
2022-03-01 12:40 ` [XEN v9 1/4] xen/arm64: Decode ldr/str post increment operations Ayan Kumar Halder
@ 2022-03-01 12:40 ` Ayan Kumar Halder
2022-03-04 0:42 ` Stefano Stabellini
2022-03-04 10:28 ` Julien Grall
2022-03-01 12:40 ` [XEN v9 3/4] xen/arm64: io: Handle the abort due to access to stage1 translation table Ayan Kumar Halder
2022-03-01 12:40 ` [XEN v9 4/4] xen/arm64: io: Handle data abort due to cache maintenance instructions Ayan Kumar Halder
3 siblings, 2 replies; 28+ messages in thread
From: Ayan Kumar Halder @ 2022-03-01 12:40 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, stefanos, julien, Volodymyr_Babchuk,
bertrand.marquis, andrew.cooper3, george.dunlap, jbeulich, wl,
paul, roger.pau, Ayan Kumar Halder
When an instruction is trapped in Xen due to translation fault, Xen
checks if the ISS is invalid (for data abort) or it is an instruction
abort. If so, Xen tries to resolve the translation fault using p2m page
tables. In case of data abort, Xen will try to map the mmio region to
the guest (ie tries to emulate the mmio region).
If the ISS is not valid and it is a data abort, then Xen tries to
decode the instruction. In case of ioreq, Xen saves the decoding state,
rn and imm9 to vcpu_io. Whenever the vcpu handles the ioreq successfully,
it will read the decoding state to determine if the instruction decoded
was a ldr/str post indexing (ie INSTR_LDR_STR_POSTINDEXING). If so, it
uses these details to post increment rn.
In case of mmio handler, if the mmio operation was successful, then Xen
retrives the decoding state, rn and imm9. For state ==
INSTR_LDR_STR_POSTINDEXING, Xen will update rn.
If there is an error encountered while decoding/executing the instruction,
Xen will forward the abort to the guest.
Signed-off-by: Ayan Kumar Halder <ayankuma@xilinx.com>
---
Changelog :-
v2..v5 - Mentioned in the cover letter.
v6 - 1. Mantained the decoding state of the instruction. This is used by the
caller to either abort the guest or retry or ignore or perform read/write on
the mmio region.
2. try_decode() invokes decoding for both aarch64 and thumb state. (Previously
it used to invoke decoding only for aarch64 state). Thus, it handles all the
checking of the registers before invoking any decoding of instruction.
try_decode_instruction_invalid_iss() has thus been removed.
3. Introduced a new field('enum instr_decode_state state') inside
'struct instr_details'. This holds the decoding state of the instruction.
This is later read by the post_increment_register() to determine if rn needs to
be incremented. Also, this is read by the callers of try_decode_instruction()
to determine if the instruction was valid or ignored or to be retried or
error or decoded successfully.
4. Also stored 'instr_details' inside 'struct ioreq'. This enables
arch_ioreq_complete_mmio() to invoke post_increment_register() without decoding
the instruction again.
5. Check hsr.dabt.valid in do_trap_stage2_abort_guest(). If it is not valid,
then decode the instruction. This ensures that try_handle_mmio() is invoked only
when the instruction is either valid or decoded successfully.
6. Inside do_trap_stage2_abort_guest(), if hsr.dabt.valid is not set, then
resolve the translation fault before trying to decode the instruction. If
translation fault is resolved, then return to the guest to execute the instruction
again.
v7 - 1. Moved the decoding instruction details ie instr_details from 'struct ioreq'
to 'struct vcpu_io'.
2. The instruction is decoded only when we get a data abort.
3. Replaced ASSERT_UNREACHABLE() with domain_crash(). The reason being asserts
can be disabled in some builds. In this scenario when the guest's cpsr is in an
erroneous state, Xen should crash the guest.
4. Introduced check_p2m() which invokes p2m_resolve_translation_fault() and
try_map_mmio() to resolve translation fault by configuring the page tables. This
gets invoked first if ISS is invalid and it is an instruction abort. If it is
a data abort and hsr.dabt.s1ptw is set or try_handle_mmio() returns IO_UNHANDLED,
then check_p2m() gets invoked again.
v8 - 1. Removed the handling of data abort when info->dabt.cache is set. This will
be implemented in a subsequent patch. (Not as part of this series)
2. When the data abort is due to access to stage 1 translation tables, Xen will
try to fix the mapping of the page table for the corresponding address. If this
returns an error, Xen will abort the guest. Else, it will ask the guest to retry
the instruction.
3. Changed v->io.info.dabt_instr from pointer to variable. The reason being that
arch_ioreq_complete_mmio() is called from leave_hypervisor_to_guest().
That is after do_trap_stage2_abort_guest() has been invoked. So the original
variable will be no longer valid.
4. Some other style issues pointed out in v7.
v9 - 1. Ensure that "Erratum 766422" is handled only when ISS is valid.
2. Whenever Xen receives and instruction abort or data abort (with invalid ISS),
Xen should first try to resolve the p2m translation fault or see if it it needs
to map a MMIO region. If it succeeds, it should return to the guest to retry the
instruction.
3. Removed handling of "dabt.s1ptw == 1" aborts. This is addressed in patch3 as
it is an existing bug in codebase.
4. Various style issues pointed by Julien in v8.
xen/arch/arm/arm32/traps.c | 11 ++++
xen/arch/arm/arm64/traps.c | 47 ++++++++++++++++
xen/arch/arm/decode.c | 1 +
xen/arch/arm/include/asm/domain.h | 4 ++
xen/arch/arm/include/asm/mmio.h | 17 +++++-
xen/arch/arm/include/asm/traps.h | 2 +
xen/arch/arm/io.c | 90 +++++++++++++++++++------------
xen/arch/arm/ioreq.c | 7 ++-
xen/arch/arm/traps.c | 77 ++++++++++++++++++++------
xen/arch/x86/include/asm/ioreq.h | 3 ++
xen/include/xen/sched.h | 2 +
11 files changed, 207 insertions(+), 54 deletions(-)
diff --git a/xen/arch/arm/arm32/traps.c b/xen/arch/arm/arm32/traps.c
index 9c9790a6d1..159e3cef8b 100644
--- a/xen/arch/arm/arm32/traps.c
+++ b/xen/arch/arm/arm32/traps.c
@@ -18,9 +18,11 @@
#include <xen/lib.h>
#include <xen/kernel.h>
+#include <xen/sched.h>
#include <public/xen.h>
+#include <asm/mmio.h>
#include <asm/processor.h>
#include <asm/traps.h>
@@ -82,6 +84,15 @@ void do_trap_data_abort(struct cpu_user_regs *regs)
do_unexpected_trap("Data Abort", regs);
}
+void post_increment_register(const struct instr_details *instr)
+{
+ /*
+ * We have not implemented decoding of post indexing instructions for 32 bit.
+ * Thus, this should be unreachable.
+ */
+ domain_crash(current->domain);
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/arch/arm/arm64/traps.c b/xen/arch/arm/arm64/traps.c
index 9113a15c7a..e18b6b2626 100644
--- a/xen/arch/arm/arm64/traps.c
+++ b/xen/arch/arm/arm64/traps.c
@@ -17,6 +17,7 @@
*/
#include <xen/lib.h>
+#include <xen/sched.h>
#include <asm/hsr.h>
#include <asm/system.h>
@@ -44,6 +45,52 @@ void do_bad_mode(struct cpu_user_regs *regs, int reason)
panic("bad mode\n");
}
+void post_increment_register(const struct instr_details *instr)
+{
+ struct cpu_user_regs *regs = guest_cpu_user_regs();
+ register_t val = 0;
+
+ /* Currently, we handle only ldr/str post indexing instructions */
+ if ( instr->state != INSTR_LDR_STR_POSTINDEXING )
+ return;
+
+ /*
+ * Handle when rn = SP
+ * Refer ArmV8 ARM DDI 0487G.b, Page - D1-2463 "Stack pointer register
+ * selection"
+ * t = SP_EL0
+ * h = SP_ELx
+ * and M[3:0] (Page - C5-474 "When exception taken from AArch64 state:")
+ */
+ if (instr->rn == 31 )
+ {
+ if ( (regs->cpsr & PSR_MODE_MASK) == PSR_MODE_EL1h )
+ val = regs->sp_el1;
+ else if ( ((regs->cpsr & PSR_MODE_MASK) == PSR_MODE_EL1t) ||
+ ((regs->cpsr & PSR_MODE_MASK) == PSR_MODE_EL0t) )
+ val = regs->sp_el0;
+ else
+ {
+ domain_crash(current->domain);
+ return;
+ }
+ }
+ else
+ val = get_user_reg(regs, instr->rn);
+
+ val += instr->imm9;
+
+ if ( instr->rn == 31 )
+ {
+ if ( (regs->cpsr & PSR_MODE_MASK) == PSR_MODE_EL1h )
+ regs->sp_el1 = val;
+ else
+ regs->sp_el0 = val;
+ }
+ else
+ set_user_reg(regs, instr->rn, val);
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c
index 3add87e83a..16ad0747bb 100644
--- a/xen/arch/arm/decode.c
+++ b/xen/arch/arm/decode.c
@@ -146,6 +146,7 @@ static int decode_arm64(register_t pc, mmio_info_t *info)
update_dabt(dabt, opcode.ldr_str.rt, opcode.ldr_str.size, false);
+ dabt_instr->state = INSTR_LDR_STR_POSTINDEXING;
dabt_instr->rn = opcode.ldr_str.rn;
dabt_instr->imm9 = opcode.ldr_str.imm9;
diff --git a/xen/arch/arm/include/asm/domain.h b/xen/arch/arm/include/asm/domain.h
index c56f6e4398..ed63c2b6f9 100644
--- a/xen/arch/arm/include/asm/domain.h
+++ b/xen/arch/arm/include/asm/domain.h
@@ -281,6 +281,10 @@ static inline void arch_vcpu_block(struct vcpu *v) {}
/* vPCI is not available on Arm */
#define has_vpci(d) ({ (void)(d); false; })
+struct arch_vcpu_io {
+ struct instr_details dabt_instr; /* when the instruction is decoded */
+};
+
#endif /* __ASM_DOMAIN_H__ */
/*
diff --git a/xen/arch/arm/include/asm/mmio.h b/xen/arch/arm/include/asm/mmio.h
index 3354d9c635..ef2c57a2d5 100644
--- a/xen/arch/arm/include/asm/mmio.h
+++ b/xen/arch/arm/include/asm/mmio.h
@@ -26,12 +26,24 @@
#define MAX_IO_HANDLER 16
+enum instr_decode_state
+{
+ INSTR_ERROR, /* Error encountered while decoding instr */
+ INSTR_VALID, /* ISS is valid, so no need to decode */
+ /*
+ * Instruction is decoded successfully. It is a ldr/str post indexing
+ * instruction.
+ */
+ INSTR_LDR_STR_POSTINDEXING
+};
+
typedef struct
{
struct hsr_dabt dabt;
struct instr_details {
unsigned long rn:5;
signed int imm9:9;
+ enum instr_decode_state state;
} dabt_instr;
paddr_t gpa;
} mmio_info_t;
@@ -69,14 +81,15 @@ struct vmmio {
};
enum io_state try_handle_mmio(struct cpu_user_regs *regs,
- const union hsr hsr,
- paddr_t gpa);
+ mmio_info_t *info);
void register_mmio_handler(struct domain *d,
const struct mmio_handler_ops *ops,
paddr_t addr, paddr_t size, void *priv);
int domain_io_init(struct domain *d, int max_count);
void domain_io_free(struct domain *d);
+void try_decode_instruction(const struct cpu_user_regs *regs,
+ mmio_info_t *info);
#endif /* __ASM_ARM_MMIO_H__ */
diff --git a/xen/arch/arm/include/asm/traps.h b/xen/arch/arm/include/asm/traps.h
index 2ed2b85c6f..95c46ad391 100644
--- a/xen/arch/arm/include/asm/traps.h
+++ b/xen/arch/arm/include/asm/traps.h
@@ -109,6 +109,8 @@ static inline register_t sign_extend(const struct hsr_dabt dabt, register_t r)
return r;
}
+void post_increment_register(const struct instr_details *instr);
+
#endif /* __ASM_ARM_TRAPS__ */
/*
* Local variables:
diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
index fad103bdbd..bea69ffb08 100644
--- a/xen/arch/arm/io.c
+++ b/xen/arch/arm/io.c
@@ -102,57 +102,79 @@ static const struct mmio_handler *find_mmio_handler(struct domain *d,
return handler;
}
+void try_decode_instruction(const struct cpu_user_regs *regs,
+ mmio_info_t *info)
+{
+ int rc;
+
+ if ( info->dabt.valid )
+ {
+ info->dabt_instr.state = INSTR_VALID;
+
+ /*
+ * Erratum 766422: Thumb store translation fault to Hypervisor may
+ * not have correct HSR Rt value.
+ */
+ if ( check_workaround_766422() && (regs->cpsr & PSR_THUMB) &&
+ info->dabt.write )
+ {
+ rc = decode_instruction(regs, info);
+ if ( rc )
+ {
+ gprintk(XENLOG_DEBUG, "Unable to decode instruction\n");
+ info->dabt_instr.state = INSTR_ERROR;
+ }
+ }
+ return;
+ }
+
+ /*
+ * Armv8 processor does not provide a valid syndrome for decoding some
+ * instructions. So in order to process these instructions, Xen must
+ * decode them.
+ */
+ rc = decode_instruction(regs, info);
+ if ( rc )
+ {
+ gprintk(XENLOG_ERR, "Unable to decode instruction\n");
+ info->dabt_instr.state = INSTR_ERROR;
+ }
+}
+
enum io_state try_handle_mmio(struct cpu_user_regs *regs,
- const union hsr hsr,
- paddr_t gpa)
+ mmio_info_t *info)
{
struct vcpu *v = current;
const struct mmio_handler *handler = NULL;
- const struct hsr_dabt dabt = hsr.dabt;
- mmio_info_t info = {
- .gpa = gpa,
- .dabt = dabt
- };
+ int rc;
- ASSERT(hsr.ec == HSR_EC_DATA_ABORT_LOWER_EL);
+ ASSERT(info->dabt.ec == HSR_EC_DATA_ABORT_LOWER_EL);
- handler = find_mmio_handler(v->domain, info.gpa);
- if ( !handler )
+ if ( !((info->dabt_instr.state == INSTR_VALID) || (info->dabt_instr.state == INSTR_LDR_STR_POSTINDEXING)) )
{
- int rc;
+ ASSERT_UNREACHABLE();
+ return IO_ABORT;
+ }
- rc = try_fwd_ioserv(regs, v, &info);
+ handler = find_mmio_handler(v->domain, info->gpa);
+ if ( !handler )
+ {
+ rc = try_fwd_ioserv(regs, v, info);
if ( rc == IO_HANDLED )
return handle_ioserv(regs, v);
return rc;
}
- /* All the instructions used on emulated MMIO region should be valid */
- if ( !dabt.valid )
- return IO_ABORT;
-
/*
- * Erratum 766422: Thumb store translation fault to Hypervisor may
- * not have correct HSR Rt value.
+ * At this point, we know that the instruction is either valid or has been
+ * decoded successfully. Thus, Xen should be allowed to execute the
+ * instruction on the emulated MMIO region.
*/
- if ( check_workaround_766422() && (regs->cpsr & PSR_THUMB) &&
- dabt.write )
- {
- int rc;
-
- rc = decode_instruction(regs, &info);
- if ( rc )
- {
- gprintk(XENLOG_DEBUG, "Unable to decode instruction\n");
- return IO_ABORT;
- }
- }
-
- if ( info.dabt.write )
- return handle_write(handler, v, &info);
+ if ( info->dabt.write )
+ return handle_write(handler, v, info);
else
- return handle_read(handler, v, &info);
+ return handle_read(handler, v, info);
}
void register_mmio_handler(struct domain *d,
diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c
index 308650b400..58cd320b5a 100644
--- a/xen/arch/arm/ioreq.c
+++ b/xen/arch/arm/ioreq.c
@@ -47,6 +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 dabt_instr instr = info->dabt_instr;
ioreq_t p = {
.type = IOREQ_TYPE_COPY,
.addr = info->gpa,
@@ -76,10 +77,10 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs,
if ( !s )
return IO_UNHANDLED;
- if ( !info->dabt.valid )
- return IO_ABORT;
+ ASSERT(dabt.valid);
vio->req = p;
+ vio->info.dabt_instr = instr;
rc = ioreq_send(s, &p, 0);
if ( rc != IO_RETRY || v->domain->is_shutting_down )
@@ -95,6 +96,7 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs,
bool arch_ioreq_complete_mmio(void)
{
struct vcpu *v = current;
+ struct instr_details dabt_instr = v->io.info.dabt_instr;
struct cpu_user_regs *regs = guest_cpu_user_regs();
const union hsr hsr = { .bits = regs->hsr };
@@ -106,6 +108,7 @@ bool arch_ioreq_complete_mmio(void)
if ( handle_ioserv(regs, v) == IO_HANDLED )
{
+ post_increment_register(&dabt_instr);
advance_pc(regs, hsr);
return true;
}
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 7a1b679b8c..120c971b0f 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1893,6 +1893,21 @@ static bool try_map_mmio(gfn_t gfn)
return !map_regions_p2mt(d, gfn, 1, mfn, p2m_mmio_direct_c);
}
+static inline bool check_p2m(bool is_data, paddr_t gpa)
+{
+ /*
+ * First check if the translation fault can be resolved by the P2M subsystem.
+ * If that's the case nothing else to do.
+ */
+ if ( p2m_resolve_translation_fault(current->domain , gaddr_to_gfn(gpa)) )
+ return true;
+
+ if ( is_data && try_map_mmio(gaddr_to_gfn(gpa)) )
+ return true;
+
+ return false;
+}
+
static void do_trap_stage2_abort_guest(struct cpu_user_regs *regs,
const union hsr hsr)
{
@@ -1906,6 +1921,8 @@ static void do_trap_stage2_abort_guest(struct cpu_user_regs *regs,
paddr_t gpa;
uint8_t fsc = xabt.fsc & ~FSC_LL_MASK;
bool is_data = (hsr.ec == HSR_EC_DATA_ABORT_LOWER_EL);
+ mmio_info_t info;
+ enum io_state state;
/*
* If this bit has been set, it means that this stage-2 abort is caused
@@ -1959,21 +1976,52 @@ static void do_trap_stage2_abort_guest(struct cpu_user_regs *regs,
return;
}
case FSC_FLT_TRANS:
+ {
+ info.gpa = gpa;
+ info.dabt = hsr.dabt;
+
/*
- * Attempt first to emulate the MMIO as the data abort will
- * likely happen in an emulated region.
- *
- * Note that emulated region cannot be executed
+ * Assumption :- Most of the times when we get a data abort and the ISS
+ * is invalid or an instruction abort, the underlying cause is that the
+ * page tables have not been set up correctly.
*/
- if ( is_data )
+ if ( !is_data || !info.dabt.valid )
{
- enum io_state state = try_handle_mmio(regs, hsr, gpa);
+ if ( check_p2m(is_data, gpa) )
+ return;
- switch ( state )
- {
+ /*
+ * If the instruction abort could not be resolved by setting the
+ * appropriate bits in the translation table, then Xen should
+ * forward the abort to the guest.
+ */
+ if ( !is_data )
+ goto inject_abt;
+ }
+
+ try_decode_instruction(regs, &info);
+
+ /*
+ * If Xen could not decode the instruction or encountered an error
+ * while decoding, then it should forward the abort to the guest.
+ */
+ if ( info.dabt_instr.state == INSTR_ERROR )
+ goto inject_abt;
+
+ state = try_handle_mmio(regs, &info);
+
+ switch ( state )
+ {
case IO_ABORT:
goto inject_abt;
case IO_HANDLED:
+ /*
+ * If the instruction was decoded and has executed successfully
+ * on the MMIO region, then Xen should execute the next part of
+ * the instruction. (for eg increment the rn if it is a
+ * post-indexing instruction.
+ */
+ post_increment_register(&info.dabt_instr);
advance_pc(regs, hsr);
return;
case IO_RETRY:
@@ -1982,21 +2030,18 @@ static void do_trap_stage2_abort_guest(struct cpu_user_regs *regs,
case IO_UNHANDLED:
/* IO unhandled, try another way to handle it. */
break;
- }
}
/*
- * First check if the translation fault can be resolved by the
- * P2M subsystem. If that's the case nothing else to do.
+ * If the instruction syndrome was invalid, then we already checked if
+ * this was due to a P2M fault. So no point to check again as the result
+ * will be the same.
*/
- if ( p2m_resolve_translation_fault(current->domain,
- gaddr_to_gfn(gpa)) )
- return;
-
- if ( is_data && try_map_mmio(gaddr_to_gfn(gpa)) )
+ if ( info.dabt.valid && check_p2m(is_data, gpa) )
return;
break;
+ }
default:
gprintk(XENLOG_WARNING,
"Unsupported FSC: HSR=%#"PRIregister" DFSC=%#x\n",
diff --git a/xen/arch/x86/include/asm/ioreq.h b/xen/arch/x86/include/asm/ioreq.h
index d06ce9a6ea..ecfe7f9fdb 100644
--- a/xen/arch/x86/include/asm/ioreq.h
+++ b/xen/arch/x86/include/asm/ioreq.h
@@ -26,6 +26,9 @@
#include <asm/hvm/ioreq.h>
#endif
+struct arch_vcpu_io {
+};
+
#endif /* __ASM_X86_IOREQ_H__ */
/*
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 10ea969c7a..406d9bc610 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -160,6 +160,8 @@ struct vcpu_io {
/* I/O request in flight to device model. */
enum vio_completion completion;
ioreq_t req;
+ /* Arch specific info pertaining to the io request */
+ struct arch_vcpu_io info;
};
struct vcpu
--
2.17.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [XEN v9 3/4] xen/arm64: io: Handle the abort due to access to stage1 translation table
2022-03-01 12:40 [XEN v9 0/4] xen/arm64: io: Decode ldr/str post-indexing instruction Ayan Kumar Halder
2022-03-01 12:40 ` [XEN v9 1/4] xen/arm64: Decode ldr/str post increment operations Ayan Kumar Halder
2022-03-01 12:40 ` [XEN v9 2/4] xen/arm64: io: Support instructions (for which ISS is not valid) on emulated MMIO region using MMIO/ioreq handler Ayan Kumar Halder
@ 2022-03-01 12:40 ` Ayan Kumar Halder
2022-03-04 1:43 ` Stefano Stabellini
2022-03-04 10:39 ` Julien Grall
2022-03-01 12:40 ` [XEN v9 4/4] xen/arm64: io: Handle data abort due to cache maintenance instructions Ayan Kumar Halder
3 siblings, 2 replies; 28+ messages in thread
From: Ayan Kumar Halder @ 2022-03-01 12:40 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, stefanos, julien, Volodymyr_Babchuk,
bertrand.marquis, andrew.cooper3, george.dunlap, jbeulich, wl,
paul, roger.pau, Ayan Kumar Halder
If the abort was caused due to access to stage1 translation table, Xen
will assume that the stage1 translation table is in the non MMIO region.
It will try to resolve the translation fault. If it succeeds, it will
return to the guest to retry the instruction. If not, then it means
that the table is in MMIO region which is not expected by Xen. Thus,
Xen will forward the abort to the guest.
Signed-off-by: Ayan Kumar Halder <ayankuma@xilinx.com>
---
Changelog :-
v1..v8 - NA
v9 - 1. Extracted this change from "[XEN v8 2/2] xen/arm64: io: Support
instructions (for which ISS is not..." into a separate patch of its own.
The reason being this is an existing bug in the codebase.
xen/arch/arm/io.c | 11 +++++++++++
xen/arch/arm/traps.c | 12 +++++++++++-
2 files changed, 22 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
index bea69ffb08..ebcb8ed548 100644
--- a/xen/arch/arm/io.c
+++ b/xen/arch/arm/io.c
@@ -128,6 +128,17 @@ void try_decode_instruction(const struct cpu_user_regs *regs,
return;
}
+ /*
+ * At this point, we know that the stage1 translation table is in the MMIO
+ * region. This is not expected by Xen and thus it forwards the abort to the
+ * guest.
+ */
+ if ( info->dabt.s1ptw )
+ {
+ info->dabt_instr.state = INSTR_ERROR;
+ 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 120c971b0f..e491ca15d7 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1923,6 +1923,7 @@ static void do_trap_stage2_abort_guest(struct cpu_user_regs *regs,
bool is_data = (hsr.ec == HSR_EC_DATA_ABORT_LOWER_EL);
mmio_info_t info;
enum io_state state;
+ bool check_mmio_region = true;
/*
* If this bit has been set, it means that this stage-2 abort is caused
@@ -1987,7 +1988,16 @@ static void do_trap_stage2_abort_guest(struct cpu_user_regs *regs,
*/
if ( !is_data || !info.dabt.valid )
{
- if ( check_p2m(is_data, gpa) )
+ /*
+ * If the translation fault was caused due to access to stage 1
+ * translation table, then we try to set the translation table entry
+ * for page1 translation table (assuming that it is in the non mmio
+ * region).
+ */
+ if ( xabt.s1ptw )
+ check_mmio_region = false;
+
+ if ( check_p2m((is_data && check_mmio_region), gpa) )
return;
/*
--
2.17.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [XEN v9 4/4] xen/arm64: io: Handle data abort due to cache maintenance instructions
2022-03-01 12:40 [XEN v9 0/4] xen/arm64: io: Decode ldr/str post-indexing instruction Ayan Kumar Halder
` (2 preceding siblings ...)
2022-03-01 12:40 ` [XEN v9 3/4] xen/arm64: io: Handle the abort due to access to stage1 translation table Ayan Kumar Halder
@ 2022-03-01 12:40 ` Ayan Kumar Halder
2022-03-04 1:44 ` Stefano Stabellini
2022-03-04 10:46 ` Julien Grall
3 siblings, 2 replies; 28+ messages in thread
From: Ayan Kumar Halder @ 2022-03-01 12:40 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, stefanos, julien, Volodymyr_Babchuk,
bertrand.marquis, andrew.cooper3, george.dunlap, jbeulich, wl,
paul, roger.pau, Ayan Kumar Halder
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(-)
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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [XEN v9 2/4] xen/arm64: io: Support instructions (for which ISS is not valid) on emulated MMIO region using MMIO/ioreq handler
2022-03-01 12:40 ` [XEN v9 2/4] xen/arm64: io: Support instructions (for which ISS is not valid) on emulated MMIO region using MMIO/ioreq handler Ayan Kumar Halder
@ 2022-03-04 0:42 ` Stefano Stabellini
2022-03-04 10:30 ` Julien Grall
2022-03-04 10:28 ` Julien Grall
1 sibling, 1 reply; 28+ messages in thread
From: Stefano Stabellini @ 2022-03-04 0:42 UTC (permalink / raw)
To: Ayan Kumar Halder
Cc: xen-devel, sstabellini, stefanos, julien, Volodymyr_Babchuk,
bertrand.marquis, andrew.cooper3, george.dunlap, jbeulich, wl,
paul, roger.pau, Ayan Kumar Halder
On Tue, 1 Mar 2022, Ayan Kumar Halder wrote:
> When an instruction is trapped in Xen due to translation fault, Xen
> checks if the ISS is invalid (for data abort) or it is an instruction
> abort. If so, Xen tries to resolve the translation fault using p2m page
> tables. In case of data abort, Xen will try to map the mmio region to
> the guest (ie tries to emulate the mmio region).
>
> If the ISS is not valid and it is a data abort, then Xen tries to
> decode the instruction. In case of ioreq, Xen saves the decoding state,
> rn and imm9 to vcpu_io. Whenever the vcpu handles the ioreq successfully,
> it will read the decoding state to determine if the instruction decoded
> was a ldr/str post indexing (ie INSTR_LDR_STR_POSTINDEXING). If so, it
> uses these details to post increment rn.
>
> In case of mmio handler, if the mmio operation was successful, then Xen
> retrives the decoding state, rn and imm9. For state ==
> INSTR_LDR_STR_POSTINDEXING, Xen will update rn.
>
> If there is an error encountered while decoding/executing the instruction,
> Xen will forward the abort to the guest.
>
> Signed-off-by: Ayan Kumar Halder <ayankuma@xilinx.com>
> ---
>
> Changelog :-
>
> v2..v5 - Mentioned in the cover letter.
>
> v6 - 1. Mantained the decoding state of the instruction. This is used by the
> caller to either abort the guest or retry or ignore or perform read/write on
> the mmio region.
>
> 2. try_decode() invokes decoding for both aarch64 and thumb state. (Previously
> it used to invoke decoding only for aarch64 state). Thus, it handles all the
> checking of the registers before invoking any decoding of instruction.
> try_decode_instruction_invalid_iss() has thus been removed.
>
> 3. Introduced a new field('enum instr_decode_state state') inside
> 'struct instr_details'. This holds the decoding state of the instruction.
> This is later read by the post_increment_register() to determine if rn needs to
> be incremented. Also, this is read by the callers of try_decode_instruction()
> to determine if the instruction was valid or ignored or to be retried or
> error or decoded successfully.
>
> 4. Also stored 'instr_details' inside 'struct ioreq'. This enables
> arch_ioreq_complete_mmio() to invoke post_increment_register() without decoding
> the instruction again.
>
> 5. Check hsr.dabt.valid in do_trap_stage2_abort_guest(). If it is not valid,
> then decode the instruction. This ensures that try_handle_mmio() is invoked only
> when the instruction is either valid or decoded successfully.
>
> 6. Inside do_trap_stage2_abort_guest(), if hsr.dabt.valid is not set, then
> resolve the translation fault before trying to decode the instruction. If
> translation fault is resolved, then return to the guest to execute the instruction
> again.
>
>
> v7 - 1. Moved the decoding instruction details ie instr_details from 'struct ioreq'
> to 'struct vcpu_io'.
>
> 2. The instruction is decoded only when we get a data abort.
>
> 3. Replaced ASSERT_UNREACHABLE() with domain_crash(). The reason being asserts
> can be disabled in some builds. In this scenario when the guest's cpsr is in an
> erroneous state, Xen should crash the guest.
>
> 4. Introduced check_p2m() which invokes p2m_resolve_translation_fault() and
> try_map_mmio() to resolve translation fault by configuring the page tables. This
> gets invoked first if ISS is invalid and it is an instruction abort. If it is
> a data abort and hsr.dabt.s1ptw is set or try_handle_mmio() returns IO_UNHANDLED,
> then check_p2m() gets invoked again.
>
>
> v8 - 1. Removed the handling of data abort when info->dabt.cache is set. This will
> be implemented in a subsequent patch. (Not as part of this series)
>
> 2. When the data abort is due to access to stage 1 translation tables, Xen will
> try to fix the mapping of the page table for the corresponding address. If this
> returns an error, Xen will abort the guest. Else, it will ask the guest to retry
> the instruction.
>
> 3. Changed v->io.info.dabt_instr from pointer to variable. The reason being that
> arch_ioreq_complete_mmio() is called from leave_hypervisor_to_guest().
> That is after do_trap_stage2_abort_guest() has been invoked. So the original
> variable will be no longer valid.
>
> 4. Some other style issues pointed out in v7.
>
>
> v9 - 1. Ensure that "Erratum 766422" is handled only when ISS is valid.
>
> 2. Whenever Xen receives and instruction abort or data abort (with invalid ISS),
> Xen should first try to resolve the p2m translation fault or see if it it needs
> to map a MMIO region. If it succeeds, it should return to the guest to retry the
> instruction.
>
> 3. Removed handling of "dabt.s1ptw == 1" aborts. This is addressed in patch3 as
> it is an existing bug in codebase.
>
> 4. Various style issues pointed by Julien in v8.
>
> xen/arch/arm/arm32/traps.c | 11 ++++
> xen/arch/arm/arm64/traps.c | 47 ++++++++++++++++
> xen/arch/arm/decode.c | 1 +
> xen/arch/arm/include/asm/domain.h | 4 ++
> xen/arch/arm/include/asm/mmio.h | 17 +++++-
> xen/arch/arm/include/asm/traps.h | 2 +
> xen/arch/arm/io.c | 90 +++++++++++++++++++------------
> xen/arch/arm/ioreq.c | 7 ++-
> xen/arch/arm/traps.c | 77 ++++++++++++++++++++------
> xen/arch/x86/include/asm/ioreq.h | 3 ++
> xen/include/xen/sched.h | 2 +
> 11 files changed, 207 insertions(+), 54 deletions(-)
>
> diff --git a/xen/arch/arm/arm32/traps.c b/xen/arch/arm/arm32/traps.c
> index 9c9790a6d1..159e3cef8b 100644
> --- a/xen/arch/arm/arm32/traps.c
> +++ b/xen/arch/arm/arm32/traps.c
> @@ -18,9 +18,11 @@
>
> #include <xen/lib.h>
> #include <xen/kernel.h>
> +#include <xen/sched.h>
>
> #include <public/xen.h>
>
> +#include <asm/mmio.h>
> #include <asm/processor.h>
> #include <asm/traps.h>
>
> @@ -82,6 +84,15 @@ void do_trap_data_abort(struct cpu_user_regs *regs)
> do_unexpected_trap("Data Abort", regs);
> }
>
> +void post_increment_register(const struct instr_details *instr)
> +{
> + /*
> + * We have not implemented decoding of post indexing instructions for 32 bit.
> + * Thus, this should be unreachable.
> + */
> + domain_crash(current->domain);
> +}
> +
> /*
> * Local variables:
> * mode: C
> diff --git a/xen/arch/arm/arm64/traps.c b/xen/arch/arm/arm64/traps.c
> index 9113a15c7a..e18b6b2626 100644
> --- a/xen/arch/arm/arm64/traps.c
> +++ b/xen/arch/arm/arm64/traps.c
> @@ -17,6 +17,7 @@
> */
>
> #include <xen/lib.h>
> +#include <xen/sched.h>
>
> #include <asm/hsr.h>
> #include <asm/system.h>
> @@ -44,6 +45,52 @@ void do_bad_mode(struct cpu_user_regs *regs, int reason)
> panic("bad mode\n");
> }
>
> +void post_increment_register(const struct instr_details *instr)
> +{
> + struct cpu_user_regs *regs = guest_cpu_user_regs();
> + register_t val = 0;
> +
> + /* Currently, we handle only ldr/str post indexing instructions */
> + if ( instr->state != INSTR_LDR_STR_POSTINDEXING )
> + return;
> +
> + /*
> + * Handle when rn = SP
> + * Refer ArmV8 ARM DDI 0487G.b, Page - D1-2463 "Stack pointer register
> + * selection"
> + * t = SP_EL0
> + * h = SP_ELx
> + * and M[3:0] (Page - C5-474 "When exception taken from AArch64 state:")
> + */
> + if (instr->rn == 31 )
> + {
> + if ( (regs->cpsr & PSR_MODE_MASK) == PSR_MODE_EL1h )
> + val = regs->sp_el1;
> + else if ( ((regs->cpsr & PSR_MODE_MASK) == PSR_MODE_EL1t) ||
> + ((regs->cpsr & PSR_MODE_MASK) == PSR_MODE_EL0t) )
> + val = regs->sp_el0;
> + else
> + {
> + domain_crash(current->domain);
> + return;
> + }
> + }
> + else
> + val = get_user_reg(regs, instr->rn);
> +
> + val += instr->imm9;
> +
> + if ( instr->rn == 31 )
> + {
> + if ( (regs->cpsr & PSR_MODE_MASK) == PSR_MODE_EL1h )
> + regs->sp_el1 = val;
> + else
> + regs->sp_el0 = val;
> + }
> + else
> + set_user_reg(regs, instr->rn, val);
> +}
> +
> /*
> * Local variables:
> * mode: C
> diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c
> index 3add87e83a..16ad0747bb 100644
> --- a/xen/arch/arm/decode.c
> +++ b/xen/arch/arm/decode.c
> @@ -146,6 +146,7 @@ static int decode_arm64(register_t pc, mmio_info_t *info)
>
> update_dabt(dabt, opcode.ldr_str.rt, opcode.ldr_str.size, false);
>
> + dabt_instr->state = INSTR_LDR_STR_POSTINDEXING;
> dabt_instr->rn = opcode.ldr_str.rn;
> dabt_instr->imm9 = opcode.ldr_str.imm9;
>
> diff --git a/xen/arch/arm/include/asm/domain.h b/xen/arch/arm/include/asm/domain.h
> index c56f6e4398..ed63c2b6f9 100644
> --- a/xen/arch/arm/include/asm/domain.h
> +++ b/xen/arch/arm/include/asm/domain.h
> @@ -281,6 +281,10 @@ static inline void arch_vcpu_block(struct vcpu *v) {}
> /* vPCI is not available on Arm */
> #define has_vpci(d) ({ (void)(d); false; })
>
> +struct arch_vcpu_io {
> + struct instr_details dabt_instr; /* when the instruction is decoded */
> +};
> +
> #endif /* __ASM_DOMAIN_H__ */
>
> /*
> diff --git a/xen/arch/arm/include/asm/mmio.h b/xen/arch/arm/include/asm/mmio.h
> index 3354d9c635..ef2c57a2d5 100644
> --- a/xen/arch/arm/include/asm/mmio.h
> +++ b/xen/arch/arm/include/asm/mmio.h
> @@ -26,12 +26,24 @@
>
> #define MAX_IO_HANDLER 16
>
> +enum instr_decode_state
> +{
> + INSTR_ERROR, /* Error encountered while decoding instr */
> + INSTR_VALID, /* ISS is valid, so no need to decode */
> + /*
> + * Instruction is decoded successfully. It is a ldr/str post indexing
> + * instruction.
> + */
> + INSTR_LDR_STR_POSTINDEXING
> +};
> +
> typedef struct
> {
> struct hsr_dabt dabt;
> struct instr_details {
> unsigned long rn:5;
> signed int imm9:9;
> + enum instr_decode_state state;
> } dabt_instr;
> paddr_t gpa;
> } mmio_info_t;
> @@ -69,14 +81,15 @@ struct vmmio {
> };
>
> enum io_state try_handle_mmio(struct cpu_user_regs *regs,
> - const union hsr hsr,
> - paddr_t gpa);
> + mmio_info_t *info);
> void register_mmio_handler(struct domain *d,
> const struct mmio_handler_ops *ops,
> paddr_t addr, paddr_t size, void *priv);
> int domain_io_init(struct domain *d, int max_count);
> void domain_io_free(struct domain *d);
>
> +void try_decode_instruction(const struct cpu_user_regs *regs,
> + mmio_info_t *info);
>
> #endif /* __ASM_ARM_MMIO_H__ */
>
> diff --git a/xen/arch/arm/include/asm/traps.h b/xen/arch/arm/include/asm/traps.h
> index 2ed2b85c6f..95c46ad391 100644
> --- a/xen/arch/arm/include/asm/traps.h
> +++ b/xen/arch/arm/include/asm/traps.h
> @@ -109,6 +109,8 @@ static inline register_t sign_extend(const struct hsr_dabt dabt, register_t r)
> return r;
> }
>
> +void post_increment_register(const struct instr_details *instr);
> +
> #endif /* __ASM_ARM_TRAPS__ */
> /*
> * Local variables:
> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
> index fad103bdbd..bea69ffb08 100644
> --- a/xen/arch/arm/io.c
> +++ b/xen/arch/arm/io.c
> @@ -102,57 +102,79 @@ static const struct mmio_handler *find_mmio_handler(struct domain *d,
> return handler;
> }
>
> +void try_decode_instruction(const struct cpu_user_regs *regs,
> + mmio_info_t *info)
> +{
> + int rc;
> +
> + if ( info->dabt.valid )
> + {
> + info->dabt_instr.state = INSTR_VALID;
> +
> + /*
> + * Erratum 766422: Thumb store translation fault to Hypervisor may
> + * not have correct HSR Rt value.
> + */
> + if ( check_workaround_766422() && (regs->cpsr & PSR_THUMB) &&
> + info->dabt.write )
> + {
> + rc = decode_instruction(regs, info);
> + if ( rc )
> + {
> + gprintk(XENLOG_DEBUG, "Unable to decode instruction\n");
> + info->dabt_instr.state = INSTR_ERROR;
> + }
> + }
> + return;
> + }
> +
> + /*
> + * Armv8 processor does not provide a valid syndrome for decoding some
> + * instructions. So in order to process these instructions, Xen must
> + * decode them.
> + */
> + rc = decode_instruction(regs, info);
> + if ( rc )
> + {
> + gprintk(XENLOG_ERR, "Unable to decode instruction\n");
> + info->dabt_instr.state = INSTR_ERROR;
> + }
> +}
> +
> enum io_state try_handle_mmio(struct cpu_user_regs *regs,
> - const union hsr hsr,
> - paddr_t gpa)
> + mmio_info_t *info)
> {
> struct vcpu *v = current;
> const struct mmio_handler *handler = NULL;
> - const struct hsr_dabt dabt = hsr.dabt;
> - mmio_info_t info = {
> - .gpa = gpa,
> - .dabt = dabt
> - };
> + int rc;
>
> - ASSERT(hsr.ec == HSR_EC_DATA_ABORT_LOWER_EL);
> + ASSERT(info->dabt.ec == HSR_EC_DATA_ABORT_LOWER_EL);
>
> - handler = find_mmio_handler(v->domain, info.gpa);
> - if ( !handler )
> + if ( !((info->dabt_instr.state == INSTR_VALID) || (info->dabt_instr.state == INSTR_LDR_STR_POSTINDEXING)) )
> {
> - int rc;
> + ASSERT_UNREACHABLE();
> + return IO_ABORT;
> + }
>
> - rc = try_fwd_ioserv(regs, v, &info);
> + handler = find_mmio_handler(v->domain, info->gpa);
> + if ( !handler )
> + {
> + rc = try_fwd_ioserv(regs, v, info);
> if ( rc == IO_HANDLED )
> return handle_ioserv(regs, v);
>
> return rc;
> }
>
> - /* All the instructions used on emulated MMIO region should be valid */
> - if ( !dabt.valid )
> - return IO_ABORT;
> -
> /*
> - * Erratum 766422: Thumb store translation fault to Hypervisor may
> - * not have correct HSR Rt value.
> + * At this point, we know that the instruction is either valid or has been
> + * decoded successfully. Thus, Xen should be allowed to execute the
> + * instruction on the emulated MMIO region.
> */
> - if ( check_workaround_766422() && (regs->cpsr & PSR_THUMB) &&
> - dabt.write )
> - {
> - int rc;
> -
> - rc = decode_instruction(regs, &info);
> - if ( rc )
> - {
> - gprintk(XENLOG_DEBUG, "Unable to decode instruction\n");
> - return IO_ABORT;
> - }
> - }
> -
> - if ( info.dabt.write )
> - return handle_write(handler, v, &info);
> + if ( info->dabt.write )
> + return handle_write(handler, v, info);
> else
> - return handle_read(handler, v, &info);
> + return handle_read(handler, v, info);
> }
>
> void register_mmio_handler(struct domain *d,
> diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c
> index 308650b400..58cd320b5a 100644
> --- a/xen/arch/arm/ioreq.c
> +++ b/xen/arch/arm/ioreq.c
> @@ -47,6 +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 dabt_instr instr = info->dabt_instr;
> ioreq_t p = {
> .type = IOREQ_TYPE_COPY,
> .addr = info->gpa,
> @@ -76,10 +77,10 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs,
> if ( !s )
> return IO_UNHANDLED;
>
> - if ( !info->dabt.valid )
> - return IO_ABORT;
> + ASSERT(dabt.valid);
I cannot see where we set dabt.valid on successfully decoding the
instruction. It looks like we don't? If we don't, then here the ASSERT
would fail in case of postindexing instructions, right?
If we don't, then we should probably just get rid of this ASSERT: it is
not worth setting dabt.valid just so that this ASSERT would succeed.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [XEN v9 1/4] xen/arm64: Decode ldr/str post increment operations
2022-03-01 12:40 ` [XEN v9 1/4] xen/arm64: Decode ldr/str post increment operations Ayan Kumar Halder
@ 2022-03-04 0:42 ` Stefano Stabellini
0 siblings, 0 replies; 28+ messages in thread
From: Stefano Stabellini @ 2022-03-04 0:42 UTC (permalink / raw)
To: Ayan Kumar Halder
Cc: xen-devel, sstabellini, stefanos, julien, Volodymyr_Babchuk,
bertrand.marquis, andrew.cooper3, george.dunlap, jbeulich, wl,
paul, roger.pau, Ayan Kumar Halder
On Tue, 1 Mar 2022, Ayan Kumar Halder wrote:
> At the moment, Xen does not decode any of the arm64 instructions. This
> means that when hsr_dabt.isv == 0, Xen cannot handle those instructions.
> This will lead to Xen to abort the guests (from which those instructions
> originate).
>
> With this patch, Xen is able to decode ldr/str post indexing instructions.
> These are a subset of instructions for which hsr_dabt.isv == 0.
>
> The following instructions are now supported by Xen :-
> 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 subsequent patch, decode_arm64() will get invoked when
> hsr_dabt.isv == 0.
>
> Signed-off-by: Ayan Kumar Halder <ayankuma@xilinx.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
>
> Changelog :-
>
> v2..v5 - Mentioned in the cover letter.
>
> v6 - 1. Fixed the code style issues as mentioned in v5.
>
> v7 - No change.
>
> v8 - 1. Removed some un-necessary header files inclusion.
> 2. Some style changes pointed out in v7.
>
> v9 - 1. Rebased on top of the master.
> 2. Renamed psr_mode_is_32bit to regs_mode_is_32bit.
>
> xen/arch/arm/decode.c | 79 ++++++++++++++++++++++++++++++++-
> xen/arch/arm/decode.h | 48 +++++++++++++++++---
> xen/arch/arm/include/asm/mmio.h | 4 ++
> xen/arch/arm/io.c | 2 +-
> 4 files changed, 124 insertions(+), 9 deletions(-)
>
> diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c
> index 792c2e92a7..3add87e83a 100644
> --- a/xen/arch/arm/decode.c
> +++ b/xen/arch/arm/decode.c
> @@ -84,6 +84,78 @@ bad_thumb2:
> return 1;
> }
>
> +static int decode_arm64(register_t pc, mmio_info_t *info)
> +{
> + union instr opcode = {0};
> + struct hsr_dabt *dabt = &info->dabt;
> + struct instr_details *dabt_instr = &info->dabt_instr;
> +
> + if ( raw_copy_from_guest(&opcode.value, (void * __user)pc, sizeof (opcode)) )
> + {
> + gprintk(XENLOG_ERR, "Could not copy the instruction from PC\n");
> + return 1;
> + }
> +
> + /*
> + * Refer Arm v8 ARM DDI 0487G.b, Page - C6-1107
> + * "Shared decode for all encodings" (under ldr immediate)
> + * If n == t && n != 31, then the return value is implementation defined
> + * (can be WBSUPPRESS, UNKNOWN, UNDEFINED or NOP). Thus, we do not support
> + * this. This holds true for ldrb/ldrh immediate as well.
> + *
> + * Also refer, Page - C6-1384, the above described behaviour is same for
> + * str immediate. This holds true for strb/strh immediate as well
> + */
> + if ( (opcode.ldr_str.rn == opcode.ldr_str.rt) && (opcode.ldr_str.rn != 31) )
> + {
> + gprintk(XENLOG_ERR, "Rn should not be equal to Rt except for r31\n");
> + goto bad_loadstore;
> + }
> +
> + /* First, let's check for the fixed values */
> + if ( (opcode.value & POST_INDEX_FIXED_MASK) != POST_INDEX_FIXED_VALUE )
> + {
> + gprintk(XENLOG_ERR,
> + "Decoding instruction 0x%x is not supported\n", opcode.value);
> + goto bad_loadstore;
> + }
> +
> + if ( opcode.ldr_str.v != 0 )
> + {
> + gprintk(XENLOG_ERR,
> + "ldr/str post indexing for vector types are not supported\n");
> + goto bad_loadstore;
> + }
> +
> + /* Check for STR (immediate) */
> + if ( opcode.ldr_str.opc == 0 )
> + dabt->write = 1;
> + /* Check for LDR (immediate) */
> + else if ( opcode.ldr_str.opc == 1 )
> + dabt->write = 0;
> + else
> + {
> + gprintk(XENLOG_ERR,
> + "Decoding ldr/str post indexing is not supported for this variant\n");
> + goto bad_loadstore;
> + }
> +
> + gprintk(XENLOG_INFO,
> + "opcode->ldr_str.rt = 0x%x, opcode->ldr_str.size = 0x%x, opcode->ldr_str.imm9 = %d\n",
> + opcode.ldr_str.rt, opcode.ldr_str.size, opcode.ldr_str.imm9);
> +
> + update_dabt(dabt, opcode.ldr_str.rt, opcode.ldr_str.size, false);
> +
> + dabt_instr->rn = opcode.ldr_str.rn;
> + dabt_instr->imm9 = opcode.ldr_str.imm9;
> +
> + return 0;
> +
> + bad_loadstore:
> + gprintk(XENLOG_ERR, "unhandled Arm instruction 0x%x\n", opcode.value);
> + return 1;
> +}
> +
> static int decode_thumb(register_t pc, struct hsr_dabt *dabt)
> {
> uint16_t instr;
> @@ -150,10 +222,13 @@ bad_thumb:
> return 1;
> }
>
> -int decode_instruction(const struct cpu_user_regs *regs, struct hsr_dabt *dabt)
> +int decode_instruction(const struct cpu_user_regs *regs, mmio_info_t *info)
> {
> if ( is_32bit_domain(current->domain) && regs->cpsr & PSR_THUMB )
> - return decode_thumb(regs->pc, dabt);
> + return decode_thumb(regs->pc, &info->dabt);
> +
> + if ( !regs_mode_is_32bit(regs) )
> + return decode_arm64(regs->pc, info);
>
> /* TODO: Handle ARM instruction */
> gprintk(XENLOG_ERR, "unhandled ARM instruction\n");
> diff --git a/xen/arch/arm/decode.h b/xen/arch/arm/decode.h
> index 4613763bdb..13db8ac968 100644
> --- a/xen/arch/arm/decode.h
> +++ b/xen/arch/arm/decode.h
> @@ -23,19 +23,55 @@
> #include <asm/regs.h>
> #include <asm/processor.h>
>
> -/**
> +/*
> + * Refer to the ARMv8 ARM (DDI 0487G.b), Section C4.1.4 Loads and Stores
> + * Page 318 specifies the following bit pattern for
> + * "load/store register (immediate post-indexed)".
> + *
> + * 31 30 29 27 26 25 23 21 20 11 9 4 0
> + * ___________________________________________________________________
> + * |size|1 1 1 |V |0 0 |opc |0 | imm9 |0 1 | Rn | Rt |
> + * |____|______|__|____|____|__|_______________|____|_________|_______|
> + */
> +union instr {
> + uint32_t value;
> + struct {
> + unsigned int rt:5; /* Rt register */
> + unsigned int rn:5; /* Rn register */
> + unsigned int fixed1:2; /* value == 01b */
> + signed int imm9:9; /* imm9 */
> + unsigned int fixed2:1; /* value == 0b */
> + unsigned int opc:2; /* opc */
> + unsigned int fixed3:2; /* value == 00b */
> + unsigned int v:1; /* vector */
> + unsigned int fixed4:3; /* value == 111b */
> + unsigned int size:2; /* size */
> + } ldr_str;
> +};
> +
> +#define POST_INDEX_FIXED_MASK 0x3B200C00
> +#define POST_INDEX_FIXED_VALUE 0x38000400
> +
> +/*
> * Decode an instruction from pc
> - * /!\ This function is not intended to fully decode an instruction. It
> - * considers that the instruction is valid.
> + * /!\ This function is intended to decode an instruction. It considers that the
> + * instruction is valid.
> *
> - * This function will get:
> - * - The transfer register
> + * In case of thumb mode, this function will get:
> + * - The transfer register (ie Rt)
> * - Sign bit
> * - Size
> + *
> + * In case of arm64 mode, this function will get:
> + * - The transfer register (ie Rt)
> + * - The source register (ie Rn)
> + * - Size
> + * - Immediate offset
> + * - Read or write
> */
>
> int decode_instruction(const struct cpu_user_regs *regs,
> - struct hsr_dabt *dabt);
> + mmio_info_t *info);
>
> #endif /* __ARCH_ARM_DECODE_H_ */
>
> diff --git a/xen/arch/arm/include/asm/mmio.h b/xen/arch/arm/include/asm/mmio.h
> index 7ab873cb8f..3354d9c635 100644
> --- a/xen/arch/arm/include/asm/mmio.h
> +++ b/xen/arch/arm/include/asm/mmio.h
> @@ -29,6 +29,10 @@
> typedef struct
> {
> struct hsr_dabt dabt;
> + struct instr_details {
> + unsigned long rn:5;
> + signed int imm9:9;
> + } dabt_instr;
> paddr_t gpa;
> } mmio_info_t;
>
> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
> index 1a066f9ae5..fad103bdbd 100644
> --- a/xen/arch/arm/io.c
> +++ b/xen/arch/arm/io.c
> @@ -141,7 +141,7 @@ enum io_state try_handle_mmio(struct cpu_user_regs *regs,
> {
> int rc;
>
> - rc = decode_instruction(regs, &info.dabt);
> + rc = decode_instruction(regs, &info);
> if ( rc )
> {
> gprintk(XENLOG_DEBUG, "Unable to decode instruction\n");
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [XEN v9 3/4] xen/arm64: io: Handle the abort due to access to stage1 translation table
2022-03-01 12:40 ` [XEN v9 3/4] xen/arm64: io: Handle the abort due to access to stage1 translation table Ayan Kumar Halder
@ 2022-03-04 1:43 ` Stefano Stabellini
2022-03-04 17:04 ` Ayan Kumar Halder
2022-03-04 10:39 ` Julien Grall
1 sibling, 1 reply; 28+ messages in thread
From: Stefano Stabellini @ 2022-03-04 1:43 UTC (permalink / raw)
To: Ayan Kumar Halder
Cc: xen-devel, sstabellini, stefanos, julien, Volodymyr_Babchuk,
bertrand.marquis, andrew.cooper3, george.dunlap, jbeulich, wl,
paul, roger.pau, Ayan Kumar Halder
On Tue, 1 Mar 2022, Ayan Kumar Halder wrote:
> If the abort was caused due to access to stage1 translation table, Xen
> will assume that the stage1 translation table is in the non MMIO region.
> It will try to resolve the translation fault. If it succeeds, it will
> return to the guest to retry the instruction. If not, then it means
> that the table is in MMIO region which is not expected by Xen. Thus,
> Xen will forward the abort to the guest.
>
> Signed-off-by: Ayan Kumar Halder <ayankuma@xilinx.com>
> ---
>
> Changelog :-
>
> v1..v8 - NA
>
> v9 - 1. Extracted this change from "[XEN v8 2/2] xen/arm64: io: Support
> instructions (for which ISS is not..." into a separate patch of its own.
> The reason being this is an existing bug in the codebase.
>
> xen/arch/arm/io.c | 11 +++++++++++
> xen/arch/arm/traps.c | 12 +++++++++++-
> 2 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
> index bea69ffb08..ebcb8ed548 100644
> --- a/xen/arch/arm/io.c
> +++ b/xen/arch/arm/io.c
> @@ -128,6 +128,17 @@ void try_decode_instruction(const struct cpu_user_regs *regs,
> return;
> }
>
> + /*
> + * At this point, we know that the stage1 translation table is in the MMIO
> + * region. This is not expected by Xen and thus it forwards the abort to the
> + * guest.
> + */
> + if ( info->dabt.s1ptw )
> + {
> + info->dabt_instr.state = INSTR_ERROR;
> + 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 120c971b0f..e491ca15d7 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1923,6 +1923,7 @@ static void do_trap_stage2_abort_guest(struct cpu_user_regs *regs,
> bool is_data = (hsr.ec == HSR_EC_DATA_ABORT_LOWER_EL);
> mmio_info_t info;
> enum io_state state;
> + bool check_mmio_region = true;
>
> /*
> * If this bit has been set, it means that this stage-2 abort is caused
> @@ -1987,7 +1988,16 @@ static void do_trap_stage2_abort_guest(struct cpu_user_regs *regs,
> */
> if ( !is_data || !info.dabt.valid )
> {
> - if ( check_p2m(is_data, gpa) )
> + /*
> + * If the translation fault was caused due to access to stage 1
> + * translation table, then we try to set the translation table entry
> + * for page1 translation table (assuming that it is in the non mmio
^ stage1
Do you mean to say maybe:
If the translation fault was caused by an access to stage 1 translation
table, then no need to change the stage 2 p2m.
?
> + * region).
> + */
> + if ( xabt.s1ptw )
> + check_mmio_region = false;
> +
> + if ( check_p2m((is_data && check_mmio_region), gpa) )
> return;
>
> /*
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [XEN v9 4/4] xen/arm64: io: Handle data abort due to cache maintenance instructions
2022-03-01 12:40 ` [XEN v9 4/4] xen/arm64: io: Handle data abort due to cache maintenance instructions Ayan Kumar Halder
@ 2022-03-04 1:44 ` Stefano Stabellini
2022-03-04 10:46 ` Julien Grall
1 sibling, 0 replies; 28+ messages in thread
From: Stefano Stabellini @ 2022-03-04 1:44 UTC (permalink / raw)
To: Ayan Kumar Halder
Cc: xen-devel, sstabellini, stefanos, julien, Volodymyr_Babchuk,
bertrand.marquis, andrew.cooper3, george.dunlap, jbeulich, wl,
paul, roger.pau, Ayan Kumar Halder
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
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [XEN v9 2/4] xen/arm64: io: Support instructions (for which ISS is not valid) on emulated MMIO region using MMIO/ioreq handler
2022-03-01 12:40 ` [XEN v9 2/4] xen/arm64: io: Support instructions (for which ISS is not valid) on emulated MMIO region using MMIO/ioreq handler Ayan Kumar Halder
2022-03-04 0:42 ` Stefano Stabellini
@ 2022-03-04 10:28 ` Julien Grall
2022-03-04 11:27 ` Ayan Kumar Halder
1 sibling, 1 reply; 28+ messages in thread
From: Julien Grall @ 2022-03-04 10:28 UTC (permalink / raw)
To: Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefanos, Volodymyr_Babchuk, bertrand.marquis,
andrew.cooper3, george.dunlap, jbeulich, wl, paul, roger.pau,
Ayan Kumar Halder
Hi Ayan,
On 01/03/2022 12:40, Ayan Kumar Halder wrote:
> +void post_increment_register(const struct instr_details *instr)
> +{
> + struct cpu_user_regs *regs = guest_cpu_user_regs();
> + register_t val = 0;
> +
> + /* Currently, we handle only ldr/str post indexing instructions */
> + if ( instr->state != INSTR_LDR_STR_POSTINDEXING )
> + return;
> +
> + /*
> + * Handle when rn = SP
> + * Refer ArmV8 ARM DDI 0487G.b, Page - D1-2463 "Stack pointer register
> + * selection"
> + * t = SP_EL0
> + * h = SP_ELx
> + * and M[3:0] (Page - C5-474 "When exception taken from AArch64 state:")
> + */
> + if (instr->rn == 31 )
> + {
> + if ( (regs->cpsr & PSR_MODE_MASK) == PSR_MODE_EL1h )
> + val = regs->sp_el1;
> + else if ( ((regs->cpsr & PSR_MODE_MASK) == PSR_MODE_EL1t) ||
> + ((regs->cpsr & PSR_MODE_MASK) == PSR_MODE_EL0t) )
You are using 3 times regs->cpsr & PSR_MODE_MASK. Can you introduce a
temporary variable?
Alternatively, a switch could be used here.
[...]
> diff --git a/xen/arch/arm/include/asm/mmio.h b/xen/arch/arm/include/asm/mmio.h
> index 3354d9c635..ef2c57a2d5 100644
> --- a/xen/arch/arm/include/asm/mmio.h
> +++ b/xen/arch/arm/include/asm/mmio.h
> @@ -26,12 +26,24 @@
>
> #define MAX_IO_HANDLER 16
>
> +enum instr_decode_state
> +{
> + INSTR_ERROR, /* Error encountered while decoding instr */
> + INSTR_VALID, /* ISS is valid, so no need to decode */
> + /*
> + * Instruction is decoded successfully. It is a ldr/str post indexing
> + * instruction.
> + */
> + INSTR_LDR_STR_POSTINDEXING
NIT: Please add ',' even for the last item. This would reduce the diff
if we add new one.
> +};
> +
> typedef struct
> {
> struct hsr_dabt dabt;
> struct instr_details {
> unsigned long rn:5;
> signed int imm9:9;
> + enum instr_decode_state state;
> } dabt_instr;
> paddr_t gpa;
> } mmio_info_t;
> @@ -69,14 +81,15 @@ struct vmmio {
> };
>
> enum io_state try_handle_mmio(struct cpu_user_regs *regs,
> - const union hsr hsr,
> - paddr_t gpa);
> + mmio_info_t *info);
> void register_mmio_handler(struct domain *d,
> const struct mmio_handler_ops *ops,
> paddr_t addr, paddr_t size, void *priv);
> int domain_io_init(struct domain *d, int max_count);
> void domain_io_free(struct domain *d);
>
> +void try_decode_instruction(const struct cpu_user_regs *regs,
> + mmio_info_t *info);
>
> #endif /* __ASM_ARM_MMIO_H__ */
>
> diff --git a/xen/arch/arm/include/asm/traps.h b/xen/arch/arm/include/asm/traps.h
> index 2ed2b85c6f..95c46ad391 100644
> --- a/xen/arch/arm/include/asm/traps.h
> +++ b/xen/arch/arm/include/asm/traps.h
> @@ -109,6 +109,8 @@ static inline register_t sign_extend(const struct hsr_dabt dabt, register_t r)
> return r;
> }
>
> +void post_increment_register(const struct instr_details *instr);
> +
> #endif /* __ASM_ARM_TRAPS__ */
> /*
> * Local variables:
> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
> index fad103bdbd..bea69ffb08 100644
> --- a/xen/arch/arm/io.c
> +++ b/xen/arch/arm/io.c
> @@ -102,57 +102,79 @@ static const struct mmio_handler *find_mmio_handler(struct domain *d,
> return handler;
> }
>
> +void try_decode_instruction(const struct cpu_user_regs *regs,
> + mmio_info_t *info)
> +{
> + int rc;
> +
> + if ( info->dabt.valid )
> + {
> + info->dabt_instr.state = INSTR_VALID;
> +
> + /*
> + * Erratum 766422: Thumb store translation fault to Hypervisor may
> + * not have correct HSR Rt value.
> + */
> + if ( check_workaround_766422() && (regs->cpsr & PSR_THUMB) &&
> + info->dabt.write )
This change is not explained in the commit message. TBH, I think it
should be separate but I am not going to request that at v9.
> + {
> + rc = decode_instruction(regs, info);
> + if ( rc )
> + {
> + gprintk(XENLOG_DEBUG, "Unable to decode instruction\n");
> + info->dabt_instr.state = INSTR_ERROR;
> + }
> + return;
> + }
> +
> + /*
> + * Armv8 processor does not provide a valid syndrome for decoding some
> + * instructions. So in order to process these instructions, Xen must
> + * decode them.
> + */
> + rc = decode_instruction(regs, info);
> + if ( rc )
> + {
> + gprintk(XENLOG_ERR, "Unable to decode instruction\n");
> + info->dabt_instr.state = INSTR_ERROR;
> + }
> +}
> +
> enum io_state try_handle_mmio(struct cpu_user_regs *regs,
> - const union hsr hsr,
> - paddr_t gpa)
> + mmio_info_t *info)
> {
> struct vcpu *v = current;
> const struct mmio_handler *handler = NULL;
> - const struct hsr_dabt dabt = hsr.dabt;
> - mmio_info_t info = {
> - .gpa = gpa,
> - .dabt = dabt
> - };
> + int rc;
>
> - ASSERT(hsr.ec == HSR_EC_DATA_ABORT_LOWER_EL);
> + ASSERT(info->dabt.ec == HSR_EC_DATA_ABORT_LOWER_EL);
>
> - handler = find_mmio_handler(v->domain, info.gpa);
> - if ( !handler )
> + if ( !((info->dabt_instr.state == INSTR_VALID) || (info->dabt_instr.state == INSTR_LDR_STR_POSTINDEXING)) )
This check will become quite large if we decode more class. I would
instead set the dabt.valid bit whenever we successfully decoded the
instruction and check that if dabt.valid here.
> {
> - int rc;
> + ASSERT_UNREACHABLE();
> + return IO_ABORT;
> + }
[...]
> @@ -1982,21 +2030,18 @@ static void do_trap_stage2_abort_guest(struct cpu_user_regs *regs,
> case IO_UNHANDLED:
> /* IO unhandled, try another way to handle it. */
> break;
> - }
> }
>
> /*
> - * First check if the translation fault can be resolved by the
> - * P2M subsystem. If that's the case nothing else to do.
> + * If the instruction syndrome was invalid, then we already checked if
> + * this was due to a P2M fault. So no point to check again as the result
> + * will be the same.
> */
> - if ( p2m_resolve_translation_fault(current->domain,
> - gaddr_to_gfn(gpa)) )
> - return;
> -
> - if ( is_data && try_map_mmio(gaddr_to_gfn(gpa)) )
> + if ( info.dabt.valid && check_p2m(is_data, gpa) )
This check would need to be adjusted to check the instruction state is
INSTR_VALID.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [XEN v9 2/4] xen/arm64: io: Support instructions (for which ISS is not valid) on emulated MMIO region using MMIO/ioreq handler
2022-03-04 0:42 ` Stefano Stabellini
@ 2022-03-04 10:30 ` Julien Grall
2022-03-04 22:23 ` Stefano Stabellini
0 siblings, 1 reply; 28+ messages in thread
From: Julien Grall @ 2022-03-04 10:30 UTC (permalink / raw)
To: Stefano Stabellini, Ayan Kumar Halder
Cc: xen-devel, stefanos, Volodymyr_Babchuk, bertrand.marquis,
andrew.cooper3, george.dunlap, jbeulich, wl, paul, roger.pau,
Ayan Kumar Halder
Hi Stefano,
On 04/03/2022 00:42, Stefano Stabellini wrote:
>> void register_mmio_handler(struct domain *d,
>> diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c
>> index 308650b400..58cd320b5a 100644
>> --- a/xen/arch/arm/ioreq.c
>> +++ b/xen/arch/arm/ioreq.c
>> @@ -47,6 +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 dabt_instr instr = info->dabt_instr;
>> ioreq_t p = {
>> .type = IOREQ_TYPE_COPY,
>> .addr = info->gpa,
>> @@ -76,10 +77,10 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs,
>> if ( !s )
>> return IO_UNHANDLED;
>>
>> - if ( !info->dabt.valid )
>> - return IO_ABORT;
>> + ASSERT(dabt.valid);
>
> I cannot see where we set dabt.valid on successfully decoding the
> instruction. It looks like we don't? If we don't, then here the ASSERT
> would fail in case of postindexing instructions, right?
We don't currently set dabt.valid. There are other reasons to set it
(see my reply to Ayan). So...
>
> If we don't, then we should probably just get rid of this ASSERT: it is
> not worth setting dabt.valid just so that this ASSERT would succeed.
... I would keep the ASSERT.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [XEN v9 3/4] xen/arm64: io: Handle the abort due to access to stage1 translation table
2022-03-01 12:40 ` [XEN v9 3/4] xen/arm64: io: Handle the abort due to access to stage1 translation table Ayan Kumar Halder
2022-03-04 1:43 ` Stefano Stabellini
@ 2022-03-04 10:39 ` Julien Grall
2022-03-07 14:27 ` Ayan Kumar Halder
1 sibling, 1 reply; 28+ messages in thread
From: Julien Grall @ 2022-03-04 10:39 UTC (permalink / raw)
To: Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefanos, Volodymyr_Babchuk, bertrand.marquis,
andrew.cooper3, george.dunlap, jbeulich, wl, paul, roger.pau,
Ayan Kumar Halder
Hi Ayan,
On 01/03/2022 12:40, Ayan Kumar Halder wrote:
> If the abort was caused due to access to stage1 translation table, Xen
> will assume that the stage1 translation table is in the non MMIO region.
> It will try to resolve the translation fault. If it succeeds, it will
> return to the guest to retry the instruction. If not, then it means
> that the table is in MMIO region which is not expected by Xen. Thus,
> Xen will forward the abort to the guest.
>
> Signed-off-by: Ayan Kumar Halder <ayankuma@xilinx.com>
> ---
>
> Changelog :-
>
> v1..v8 - NA
>
> v9 - 1. Extracted this change from "[XEN v8 2/2] xen/arm64: io: Support
> instructions (for which ISS is not..." into a separate patch of its own.
> The reason being this is an existing bug in the codebase.
>
> xen/arch/arm/io.c | 11 +++++++++++
> xen/arch/arm/traps.c | 12 +++++++++++-
> 2 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
> index bea69ffb08..ebcb8ed548 100644
> --- a/xen/arch/arm/io.c
> +++ b/xen/arch/arm/io.c
> @@ -128,6 +128,17 @@ void try_decode_instruction(const struct cpu_user_regs *regs,
> return;
> }
>
> + /*
> + * At this point, we know that the stage1 translation table is in the MMIO
> + * region. This is not expected by Xen and thus it forwards the abort to the
We don't know that. We only know that there are no corresponding valid
mapping in the P2M. So the address may be part of an emulated MMIO
region or invalid.
For both cases, we will want to send an abort.
Furthermore, I would say "emulated MMIO region" rather than MMIO region
because the P2M can also contain MMIO mapping (we usually call then
"direct MMIO").
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [XEN v9 4/4] xen/arm64: io: Handle data abort due to cache maintenance instructions
2022-03-01 12:40 ` [XEN v9 4/4] xen/arm64: io: Handle data abort due to cache maintenance instructions Ayan Kumar Halder
2022-03-04 1:44 ` Stefano Stabellini
@ 2022-03-04 10:46 ` Julien Grall
2022-03-04 12:13 ` Ayan Kumar Halder
1 sibling, 1 reply; 28+ messages in thread
From: Julien Grall @ 2022-03-04 10:46 UTC (permalink / raw)
To: Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefanos, Volodymyr_Babchuk, bertrand.marquis,
andrew.cooper3, george.dunlap, jbeulich, wl, paul, roger.pau,
Ayan Kumar Halder
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,
--
Julien Grall
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [XEN v9 2/4] xen/arm64: io: Support instructions (for which ISS is not valid) on emulated MMIO region using MMIO/ioreq handler
2022-03-04 10:28 ` Julien Grall
@ 2022-03-04 11:27 ` Ayan Kumar Halder
2022-03-04 12:45 ` Julien Grall
0 siblings, 1 reply; 28+ messages in thread
From: Ayan Kumar Halder @ 2022-03-04 11:27 UTC (permalink / raw)
To: Julien Grall, Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefanos, Volodymyr_Babchuk, bertrand.marquis,
andrew.cooper3, george.dunlap, jbeulich, wl, paul, roger.pau
Hi Julien,
Many thanks for the feedback.
I have some clarifications.
On 04/03/2022 10:28, Julien Grall wrote:
> Hi Ayan,
>
> On 01/03/2022 12:40, Ayan Kumar Halder wrote:
>> +void post_increment_register(const struct instr_details *instr)
>> +{
>> + struct cpu_user_regs *regs = guest_cpu_user_regs();
>> + register_t val = 0;
>> +
>> + /* Currently, we handle only ldr/str post indexing instructions */
>> + if ( instr->state != INSTR_LDR_STR_POSTINDEXING )
>> + return;
>> +
>> + /*
>> + * Handle when rn = SP
>> + * Refer ArmV8 ARM DDI 0487G.b, Page - D1-2463 "Stack pointer
>> register
>> + * selection"
>> + * t = SP_EL0
>> + * h = SP_ELx
>> + * and M[3:0] (Page - C5-474 "When exception taken from AArch64
>> state:")
>> + */
>> + if (instr->rn == 31 )
>> + {
>> + if ( (regs->cpsr & PSR_MODE_MASK) == PSR_MODE_EL1h )
>> + val = regs->sp_el1;
>> + else if ( ((regs->cpsr & PSR_MODE_MASK) == PSR_MODE_EL1t) ||
>> + ((regs->cpsr & PSR_MODE_MASK) == PSR_MODE_EL0t) )
>
> You are using 3 times regs->cpsr & PSR_MODE_MASK. Can you introduce a
> temporary variable?
>
> Alternatively, a switch could be used here.
Yes, a switch is better. I will address that in v10.
>
> [...]
>
>> diff --git a/xen/arch/arm/include/asm/mmio.h
>> b/xen/arch/arm/include/asm/mmio.h
>> index 3354d9c635..ef2c57a2d5 100644
>> --- a/xen/arch/arm/include/asm/mmio.h
>> +++ b/xen/arch/arm/include/asm/mmio.h
>> @@ -26,12 +26,24 @@
>> #define MAX_IO_HANDLER 16
>> +enum instr_decode_state
>> +{
>> + INSTR_ERROR, /* Error encountered while
>> decoding instr */
>> + INSTR_VALID, /* ISS is valid, so no need to
>> decode */
>> + /*
>> + * Instruction is decoded successfully. It is a ldr/str post
>> indexing
>> + * instruction.
>> + */
>> + INSTR_LDR_STR_POSTINDEXING
>
> NIT: Please add ',' even for the last item. This would reduce the diff
> if we add new one.
Ack (To be addressed in v10)
>
>> +};
>> +
>> typedef struct
>> {
>> struct hsr_dabt dabt;
>> struct instr_details {
>> unsigned long rn:5;
>> signed int imm9:9;
>> + enum instr_decode_state state;
>> } dabt_instr;
>> paddr_t gpa;
>> } mmio_info_t;
>> @@ -69,14 +81,15 @@ struct vmmio {
>> };
>> enum io_state try_handle_mmio(struct cpu_user_regs *regs,
>> - const union hsr hsr,
>> - paddr_t gpa);
>> + mmio_info_t *info);
>> void register_mmio_handler(struct domain *d,
>> const struct mmio_handler_ops *ops,
>> paddr_t addr, paddr_t size, void *priv);
>> int domain_io_init(struct domain *d, int max_count);
>> void domain_io_free(struct domain *d);
>> +void try_decode_instruction(const struct cpu_user_regs *regs,
>> + mmio_info_t *info);
>> #endif /* __ASM_ARM_MMIO_H__ */
>> diff --git a/xen/arch/arm/include/asm/traps.h
>> b/xen/arch/arm/include/asm/traps.h
>> index 2ed2b85c6f..95c46ad391 100644
>> --- a/xen/arch/arm/include/asm/traps.h
>> +++ b/xen/arch/arm/include/asm/traps.h
>> @@ -109,6 +109,8 @@ static inline register_t sign_extend(const struct
>> hsr_dabt dabt, register_t r)
>> return r;
>> }
>> +void post_increment_register(const struct instr_details *instr);
>> +
>> #endif /* __ASM_ARM_TRAPS__ */
>> /*
>> * Local variables:
>> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
>> index fad103bdbd..bea69ffb08 100644
>> --- a/xen/arch/arm/io.c
>> +++ b/xen/arch/arm/io.c
>> @@ -102,57 +102,79 @@ static const struct mmio_handler
>> *find_mmio_handler(struct domain *d,
>> return handler;
>> }
>> +void try_decode_instruction(const struct cpu_user_regs *regs,
>> + mmio_info_t *info)
>> +{
>> + int rc;
>> +
>> + if ( info->dabt.valid )
>> + {
>> + info->dabt_instr.state = INSTR_VALID;
>> +
>> + /*
>> + * Erratum 766422: Thumb store translation fault to
>> Hypervisor may
>> + * not have correct HSR Rt value.
>> + */
>> + if ( check_workaround_766422() && (regs->cpsr & PSR_THUMB) &&
>> + info->dabt.write )
>
> This change is not explained in the commit message. TBH, I think it
> should be separate but I am not going to request that at v9.
I will leave this as it is then. I will explain in the commit message
that the logic to infer the type of instruction has been moved from
try_handle_mmio() to try_decode_instruction() which is called before.
try_handle_mmio() is solely responsible for handling the mmio operation.
>
>> + {
>> + rc = decode_instruction(regs, info);
>> + if ( rc )
>> + {
>> + gprintk(XENLOG_DEBUG, "Unable to decode
>> instruction\n");
>> + info->dabt_instr.state = INSTR_ERROR;
>> + }
>> + return;
>> + }
>> +
>> + /*
>> + * Armv8 processor does not provide a valid syndrome for
>> decoding some
>> + * instructions. So in order to process these instructions, Xen
>> must
>> + * decode them.
>> + */
>> + rc = decode_instruction(regs, info);
>> + if ( rc )
>> + {
>> + gprintk(XENLOG_ERR, "Unable to decode instruction\n");
>> + info->dabt_instr.state = INSTR_ERROR;
>> + }
>> +}
>> +
>> enum io_state try_handle_mmio(struct cpu_user_regs *regs,
>> - const union hsr hsr,
>> - paddr_t gpa)
>> + mmio_info_t *info)
>> {
>> struct vcpu *v = current;
>> const struct mmio_handler *handler = NULL;
>> - const struct hsr_dabt dabt = hsr.dabt;
>> - mmio_info_t info = {
>> - .gpa = gpa,
>> - .dabt = dabt
>> - };
>> + int rc;
>> - ASSERT(hsr.ec == HSR_EC_DATA_ABORT_LOWER_EL);
>> + ASSERT(info->dabt.ec == HSR_EC_DATA_ABORT_LOWER_EL);
>> - handler = find_mmio_handler(v->domain, info.gpa);
>> - if ( !handler )
>> + if ( !((info->dabt_instr.state == INSTR_VALID) ||
>> (info->dabt_instr.state == INSTR_LDR_STR_POSTINDEXING)) )
>
> This check will become quite large if we decode more class. I would
> instead set the dabt.valid bit whenever we successfully decoded the
> instruction and check that if dabt.valid here.
Actually the main reason to introduce INSTR_LDR_STR_POSTINDEXING is to
distinguish the scenario where the ISS was valid vs when instruction was
decoded manually.
In the later scenario, one would need to do the post increment of the rn.
It makes sense to me to have a unque 'info->dabt_instr.state' for each
type of instruction decoded as the post processing will vary. In this
case, the post processing logic checks that the instruction is
ldr_str_postindexing.
However your concern that the check will become large is valid. I would
introduce a function as follows :-
bool check_instr_is_valid(enum instr_decode_state state)
{
if (state == INSTR_VALID) || (state == INSTR_LDR_STR_POSTINDEXING)
|| ...)
return true;
else
return false;
}
And then in
enum io_state try_handle_mmio(struct cpu_user_regs *regs, ...)
{
...
if ( !check_instr_is_valid(info->dabt_instr.state) )
{
ASSERT_UNREACHABLE();
return IO_ABORT;
}
...
}
Please let me know your thoughts,
>
>> {
>> - int rc;
>> + ASSERT_UNREACHABLE();
>> + return IO_ABORT;
>> + }
>
> [...]
>
>> @@ -1982,21 +2030,18 @@ static void do_trap_stage2_abort_guest(struct
>> cpu_user_regs *regs,
>> case IO_UNHANDLED:
>> /* IO unhandled, try another way to handle it. */
>> break;
>> - }
>> }
>> /*
>> - * First check if the translation fault can be resolved by the
>> - * P2M subsystem. If that's the case nothing else to do.
>> + * If the instruction syndrome was invalid, then we already
>> checked if
>> + * this was due to a P2M fault. So no point to check again
>> as the result
>> + * will be the same.
>> */
>> - if ( p2m_resolve_translation_fault(current->domain,
>> - gaddr_to_gfn(gpa)) )
>> - return;
>> -
>> - if ( is_data && try_map_mmio(gaddr_to_gfn(gpa)) )
>> + if ( info.dabt.valid && check_p2m(is_data, gpa) )
>
> This check would need to be adjusted to check the instruction state is
> INSTR_VALID.
Ack (To be addressed in v10).
- Ayan
>
> Cheers,
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [XEN v9 4/4] xen/arm64: io: Handle data abort due to cache maintenance instructions
2022-03-04 10:46 ` Julien Grall
@ 2022-03-04 12:13 ` Ayan Kumar Halder
2022-03-04 12:49 ` Julien Grall
0 siblings, 1 reply; 28+ messages in thread
From: Ayan Kumar Halder @ 2022-03-04 12:13 UTC (permalink / raw)
To: Julien Grall, Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefanos, Volodymyr_Babchuk, bertrand.marquis,
andrew.cooper3, george.dunlap, jbeulich, wl, paul, roger.pau
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,
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [XEN v9 2/4] xen/arm64: io: Support instructions (for which ISS is not valid) on emulated MMIO region using MMIO/ioreq handler
2022-03-04 11:27 ` Ayan Kumar Halder
@ 2022-03-04 12:45 ` Julien Grall
0 siblings, 0 replies; 28+ messages in thread
From: Julien Grall @ 2022-03-04 12:45 UTC (permalink / raw)
To: Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefanos, Volodymyr_Babchuk, bertrand.marquis,
andrew.cooper3, george.dunlap, jbeulich, wl, paul, roger.pau
Hi,
On 04/03/2022 11:27, Ayan Kumar Halder wrote:
>>
>>> + {
>>> + rc = decode_instruction(regs, info);
>>> + if ( rc )
>>> + {
>>> + gprintk(XENLOG_DEBUG, "Unable to decode
>>> instruction\n");
>>> + info->dabt_instr.state = INSTR_ERROR;
>>> + }
>>> + return;
>>> + }
>>> +
>>> + /*
>>> + * Armv8 processor does not provide a valid syndrome for
>>> decoding some
>>> + * instructions. So in order to process these instructions, Xen
>>> must
>>> + * decode them.
>>> + */
>>> + rc = decode_instruction(regs, info);
>>> + if ( rc )
>>> + {
>>> + gprintk(XENLOG_ERR, "Unable to decode instruction\n");
>>> + info->dabt_instr.state = INSTR_ERROR;
>>> + }
>>> +}
>>> +
>>> enum io_state try_handle_mmio(struct cpu_user_regs *regs,
>>> - const union hsr hsr,
>>> - paddr_t gpa)
>>> + mmio_info_t *info)
>>> {
>>> struct vcpu *v = current;
>>> const struct mmio_handler *handler = NULL;
>>> - const struct hsr_dabt dabt = hsr.dabt;
>>> - mmio_info_t info = {
>>> - .gpa = gpa,
>>> - .dabt = dabt
>>> - };
>>> + int rc;
>>> - ASSERT(hsr.ec == HSR_EC_DATA_ABORT_LOWER_EL);
>>> + ASSERT(info->dabt.ec == HSR_EC_DATA_ABORT_LOWER_EL);
>>> - handler = find_mmio_handler(v->domain, info.gpa);
>>> - if ( !handler )
>>> + if ( !((info->dabt_instr.state == INSTR_VALID) ||
>>> (info->dabt_instr.state == INSTR_LDR_STR_POSTINDEXING)) )
>>
>> This check will become quite large if we decode more class. I would
>> instead set the dabt.valid bit whenever we successfully decoded the
>> instruction and check that if dabt.valid here.
>
> Actually the main reason to introduce INSTR_LDR_STR_POSTINDEXING is to
> distinguish the scenario where the ISS was valid vs when instruction was
> decoded manually.
>
> In the later scenario, one would need to do the post increment of the rn.
>
> It makes sense to me to have a unque 'info->dabt_instr.state' for each
> type of instruction decoded as the post processing will vary. In this
> case, the post processing logic checks that the instruction is
> ldr_str_postindexing.
So I agree we want to have a unique state for type of instruction. I
wasn't suggesting to remove it. Instead, I was suggesting to use
dabt.valid as "This is a valid instruction for accessing an emulated MMIO".
>
> However your concern that the check will become large is valid. I would
> introduce a function as follows :-
>
> bool check_instr_is_valid(enum instr_decode_state state)
>
> {
>
> if (state == INSTR_VALID) || (state == INSTR_LDR_STR_POSTINDEXING)
> || ...)
>
> return true;
>
> else
>
> return false;
>
> }
>
> And then in
>
> enum io_state try_handle_mmio(struct cpu_user_regs *regs, ...)
>
> {
>
> ...
>
> if ( !check_instr_is_valid(info->dabt_instr.state) )
>
> {
>
> ASSERT_UNREACHABLE();
> return IO_ABORT;
>
> }
>
> ...
>
> }
>
> Please let me know your thoughts,
This is only moving the check to a separate function. It doesn't help
with the fact that the check in check_instr_is_valid() is going to grow.
I can see two options:
* Using dabt.valid as "The instruction was fully decoded".
* Check that the state is not INSTR_ERROR
Above, I was suggesting the former. But I am open to use latter.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [XEN v9 4/4] xen/arm64: io: Handle data abort due to cache maintenance instructions
2022-03-04 12:13 ` Ayan Kumar Halder
@ 2022-03-04 12:49 ` Julien Grall
2022-03-04 14:40 ` Ayan Kumar Halder
0 siblings, 1 reply; 28+ messages in thread
From: Julien Grall @ 2022-03-04 12:49 UTC (permalink / raw)
To: Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefanos, Volodymyr_Babchuk, bertrand.marquis,
andrew.cooper3, george.dunlap, jbeulich, wl, paul, roger.pau
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,
--
Julien Grall
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [XEN v9 4/4] xen/arm64: io: Handle data abort due to cache maintenance instructions
2022-03-04 12:49 ` Julien Grall
@ 2022-03-04 14:40 ` Ayan Kumar Halder
2022-03-04 18:37 ` Julien Grall
0 siblings, 1 reply; 28+ messages in thread
From: Ayan Kumar Halder @ 2022-03-04 14:40 UTC (permalink / raw)
To: Julien Grall, Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefanos, Volodymyr_Babchuk, bertrand.marquis,
andrew.cooper3, george.dunlap, jbeulich, wl, paul, roger.pau
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,
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [XEN v9 3/4] xen/arm64: io: Handle the abort due to access to stage1 translation table
2022-03-04 1:43 ` Stefano Stabellini
@ 2022-03-04 17:04 ` Ayan Kumar Halder
2022-03-04 22:34 ` Stefano Stabellini
0 siblings, 1 reply; 28+ messages in thread
From: Ayan Kumar Halder @ 2022-03-04 17:04 UTC (permalink / raw)
To: Stefano Stabellini, Ayan Kumar Halder
Cc: xen-devel, stefanos, julien, Volodymyr_Babchuk, bertrand.marquis,
andrew.cooper3, george.dunlap, jbeulich, wl, paul, roger.pau
Hi Stefano,
On 04/03/2022 01:43, Stefano Stabellini wrote:
> On Tue, 1 Mar 2022, Ayan Kumar Halder wrote:
>> If the abort was caused due to access to stage1 translation table, Xen
>> will assume that the stage1 translation table is in the non MMIO region.
>> It will try to resolve the translation fault. If it succeeds, it will
>> return to the guest to retry the instruction. If not, then it means
>> that the table is in MMIO region which is not expected by Xen. Thus,
>> Xen will forward the abort to the guest.
>>
>> Signed-off-by: Ayan Kumar Halder <ayankuma@xilinx.com>
>> ---
>>
>> Changelog :-
>>
>> v1..v8 - NA
>>
>> v9 - 1. Extracted this change from "[XEN v8 2/2] xen/arm64: io: Support
>> instructions (for which ISS is not..." into a separate patch of its own.
>> The reason being this is an existing bug in the codebase.
>>
>> xen/arch/arm/io.c | 11 +++++++++++
>> xen/arch/arm/traps.c | 12 +++++++++++-
>> 2 files changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
>> index bea69ffb08..ebcb8ed548 100644
>> --- a/xen/arch/arm/io.c
>> +++ b/xen/arch/arm/io.c
>> @@ -128,6 +128,17 @@ void try_decode_instruction(const struct cpu_user_regs *regs,
>> return;
>> }
>>
>> + /*
>> + * At this point, we know that the stage1 translation table is in the MMIO
>> + * region. This is not expected by Xen and thus it forwards the abort to the
>> + * guest.
>> + */
>> + if ( info->dabt.s1ptw )
>> + {
>> + info->dabt_instr.state = INSTR_ERROR;
>> + 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 120c971b0f..e491ca15d7 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -1923,6 +1923,7 @@ static void do_trap_stage2_abort_guest(struct cpu_user_regs *regs,
>> bool is_data = (hsr.ec == HSR_EC_DATA_ABORT_LOWER_EL);
>> mmio_info_t info;
>> enum io_state state;
>> + bool check_mmio_region = true;
>>
>> /*
>> * If this bit has been set, it means that this stage-2 abort is caused
>> @@ -1987,7 +1988,16 @@ static void do_trap_stage2_abort_guest(struct cpu_user_regs *regs,
>> */
>> if ( !is_data || !info.dabt.valid )
>> {
>> - if ( check_p2m(is_data, gpa) )
>> + /*
>> + * If the translation fault was caused due to access to stage 1
>> + * translation table, then we try to set the translation table entry
>> + * for page1 translation table (assuming that it is in the non mmio
> ^ stage1
>
> Do you mean to say maybe:
Yes, it should be stage1. Sorry for typo.
>
> If the translation fault was caused by an access to stage 1 translation
> table, then no need to change the stage 2 p2m.
>
> ?
The translation fault was caused due to access to stage1 translation
table. As per my understanding, the address of stage1 tables is in
stage2 translation table entries. Thus, Xen needs to modify the
corresponding stage2 p2m entries.
- Ayan
>
>
>
>> + * region).
>> + */
>> + if ( xabt.s1ptw )
>> + check_mmio_region = false;
>> +
>> + if ( check_p2m((is_data && check_mmio_region), gpa) )
>> return;
>>
>> /*
>> --
>> 2.17.1
>>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [XEN v9 4/4] xen/arm64: io: Handle data abort due to cache maintenance instructions
2022-03-04 14:40 ` Ayan Kumar Halder
@ 2022-03-04 18:37 ` Julien Grall
0 siblings, 0 replies; 28+ messages in thread
From: Julien Grall @ 2022-03-04 18:37 UTC (permalink / raw)
To: Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefanos, Volodymyr_Babchuk, bertrand.marquis,
andrew.cooper3, george.dunlap, jbeulich, wl, paul, roger.pau
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,
--
Julien Grall
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [XEN v9 2/4] xen/arm64: io: Support instructions (for which ISS is not valid) on emulated MMIO region using MMIO/ioreq handler
2022-03-04 10:30 ` Julien Grall
@ 2022-03-04 22:23 ` Stefano Stabellini
0 siblings, 0 replies; 28+ messages in thread
From: Stefano Stabellini @ 2022-03-04 22:23 UTC (permalink / raw)
To: Julien Grall
Cc: Stefano Stabellini, Ayan Kumar Halder, xen-devel, stefanos,
Volodymyr_Babchuk, bertrand.marquis, andrew.cooper3,
george.dunlap, jbeulich, wl, paul, roger.pau, Ayan Kumar Halder
On Fri, 4 Mar 2022, Julien Grall wrote:
> Hi Stefano,
>
> On 04/03/2022 00:42, Stefano Stabellini wrote:
> > > void register_mmio_handler(struct domain *d,
> > > diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c
> > > index 308650b400..58cd320b5a 100644
> > > --- a/xen/arch/arm/ioreq.c
> > > +++ b/xen/arch/arm/ioreq.c
> > > @@ -47,6 +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 dabt_instr instr = info->dabt_instr;
> > > ioreq_t p = {
> > > .type = IOREQ_TYPE_COPY,
> > > .addr = info->gpa,
> > > @@ -76,10 +77,10 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs
> > > *regs,
> > > if ( !s )
> > > return IO_UNHANDLED;
> > > - if ( !info->dabt.valid )
> > > - return IO_ABORT;
> > > + ASSERT(dabt.valid);
> >
> > I cannot see where we set dabt.valid on successfully decoding the
> > instruction. It looks like we don't? If we don't, then here the ASSERT
> > would fail in case of postindexing instructions, right?
>
> We don't currently set dabt.valid. There are other reasons to set it (see my
> reply to Ayan). So...
>
> >
> > If we don't, then we should probably just get rid of this ASSERT: it is
> > not worth setting dabt.valid just so that this ASSERT would succeed.
>
> ... I would keep the ASSERT.
OK
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [XEN v9 3/4] xen/arm64: io: Handle the abort due to access to stage1 translation table
2022-03-04 17:04 ` Ayan Kumar Halder
@ 2022-03-04 22:34 ` Stefano Stabellini
0 siblings, 0 replies; 28+ messages in thread
From: Stefano Stabellini @ 2022-03-04 22:34 UTC (permalink / raw)
To: Ayan Kumar Halder
Cc: Stefano Stabellini, xen-devel, stefanos, julien,
Volodymyr_Babchuk, bertrand.marquis, andrew.cooper3,
george.dunlap, jbeulich, wl, paul, roger.pau
On Fri, 4 Mar 2022, Ayan Kumar Halder wrote:
> On 04/03/2022 01:43, Stefano Stabellini wrote:
> > On Tue, 1 Mar 2022, Ayan Kumar Halder wrote:
> > > If the abort was caused due to access to stage1 translation table, Xen
> > > will assume that the stage1 translation table is in the non MMIO region.
> > > It will try to resolve the translation fault. If it succeeds, it will
> > > return to the guest to retry the instruction. If not, then it means
> > > that the table is in MMIO region which is not expected by Xen. Thus,
> > > Xen will forward the abort to the guest.
> > >
> > > Signed-off-by: Ayan Kumar Halder <ayankuma@xilinx.com>
> > > ---
> > >
> > > Changelog :-
> > >
> > > v1..v8 - NA
> > >
> > > v9 - 1. Extracted this change from "[XEN v8 2/2] xen/arm64: io: Support
> > > instructions (for which ISS is not..." into a separate patch of its own.
> > > The reason being this is an existing bug in the codebase.
> > >
> > > xen/arch/arm/io.c | 11 +++++++++++
> > > xen/arch/arm/traps.c | 12 +++++++++++-
> > > 2 files changed, 22 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
> > > index bea69ffb08..ebcb8ed548 100644
> > > --- a/xen/arch/arm/io.c
> > > +++ b/xen/arch/arm/io.c
> > > @@ -128,6 +128,17 @@ void try_decode_instruction(const struct
> > > cpu_user_regs *regs,
> > > return;
> > > }
> > > + /*
> > > + * At this point, we know that the stage1 translation table is in the
> > > MMIO
> > > + * region. This is not expected by Xen and thus it forwards the abort
> > > to the
> > > + * guest.
> > > + */
> > > + if ( info->dabt.s1ptw )
> > > + {
> > > + info->dabt_instr.state = INSTR_ERROR;
> > > + 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 120c971b0f..e491ca15d7 100644
> > > --- a/xen/arch/arm/traps.c
> > > +++ b/xen/arch/arm/traps.c
> > > @@ -1923,6 +1923,7 @@ static void do_trap_stage2_abort_guest(struct
> > > cpu_user_regs *regs,
> > > bool is_data = (hsr.ec == HSR_EC_DATA_ABORT_LOWER_EL);
> > > mmio_info_t info;
> > > enum io_state state;
> > > + bool check_mmio_region = true;
> > > /*
> > > * If this bit has been set, it means that this stage-2 abort is
> > > caused
> > > @@ -1987,7 +1988,16 @@ static void do_trap_stage2_abort_guest(struct
> > > cpu_user_regs *regs,
> > > */
> > > if ( !is_data || !info.dabt.valid )
> > > {
> > > - if ( check_p2m(is_data, gpa) )
> > > + /*
> > > + * If the translation fault was caused due to access to stage
> > > 1
> > > + * translation table, then we try to set the translation
> > > table entry
> > > + * for page1 translation table (assuming that it is in the
> > > non mmio
> > ^ stage1
> >
> > Do you mean to say maybe:
> Yes, it should be stage1. Sorry for typo.
> >
> > If the translation fault was caused by an access to stage 1 translation
> > table, then no need to change the stage 2 p2m.
> >
> > ?
>
> The translation fault was caused due to access to stage1 translation table. As
> per my understanding, the address of stage1 tables is in stage2 translation
> table entries. Thus, Xen needs to modify the corresponding stage2 p2m entries.
OK, I follow what you are saying and what this patch is doing now. I suggest:
If the translation fault was caused due to access to the stage 1
translation table, then we try to set the p2m entry for the stage 1
translation table, but we don't handle stage 1 translation tables in
MMIO regions.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [XEN v9 3/4] xen/arm64: io: Handle the abort due to access to stage1 translation table
2022-03-04 10:39 ` Julien Grall
@ 2022-03-07 14:27 ` Ayan Kumar Halder
2022-03-07 19:37 ` Julien Grall
0 siblings, 1 reply; 28+ messages in thread
From: Ayan Kumar Halder @ 2022-03-07 14:27 UTC (permalink / raw)
To: Julien Grall, Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefanos, Volodymyr_Babchuk, bertrand.marquis,
andrew.cooper3, george.dunlap, jbeulich, wl, paul, roger.pau
Hi Julien,
One clarification.
On 04/03/2022 10:39, Julien Grall wrote:
> Hi Ayan,
>
> On 01/03/2022 12:40, Ayan Kumar Halder wrote:
>> If the abort was caused due to access to stage1 translation table, Xen
>> will assume that the stage1 translation table is in the non MMIO region.
>> It will try to resolve the translation fault. If it succeeds, it will
>> return to the guest to retry the instruction. If not, then it means
>> that the table is in MMIO region which is not expected by Xen. Thus,
>> Xen will forward the abort to the guest.
>>
>> Signed-off-by: Ayan Kumar Halder <ayankuma@xilinx.com>
>> ---
>>
>> Changelog :-
>>
>> v1..v8 - NA
>>
>> v9 - 1. Extracted this change from "[XEN v8 2/2] xen/arm64: io: Support
>> instructions (for which ISS is not..." into a separate patch of its own.
>> The reason being this is an existing bug in the codebase.
>>
>> xen/arch/arm/io.c | 11 +++++++++++
>> xen/arch/arm/traps.c | 12 +++++++++++-
>> 2 files changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
>> index bea69ffb08..ebcb8ed548 100644
>> --- a/xen/arch/arm/io.c
>> +++ b/xen/arch/arm/io.c
>> @@ -128,6 +128,17 @@ void try_decode_instruction(const struct
>> cpu_user_regs *regs,
>> return;
>> }
>> + /*
>> + * At this point, we know that the stage1 translation table is
>> in the MMIO
>> + * region. This is not expected by Xen and thus it forwards the
>> abort to the
>
> We don't know that. We only know that there are no corresponding valid
> mapping in the P2M. So the address may be part of an emulated MMIO
> region or invalid.
>
> For both cases, we will want to send an abort.
>
> Furthermore, I would say "emulated MMIO region" rather than MMIO
> region because the P2M can also contain MMIO mapping (we usually call
> then "direct MMIO").
Can I say MMIO region (to indicate both emulated and direct) ? The
reason being the assumption that stage1 page tables cannot be in the
MMIO region. And thus, when check_p2m() is invoked, we do not invoke
try_map_mmio(gaddr_to_gfn(gpa).
See this snippet :-
if ( xabt.s1ptw )
check_mmio_region = false;
if ( check_p2m((is_data && check_mmio_region), gpa) )
return;
- Ayan
>
> Cheers,
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [XEN v9 3/4] xen/arm64: io: Handle the abort due to access to stage1 translation table
2022-03-07 14:27 ` Ayan Kumar Halder
@ 2022-03-07 19:37 ` Julien Grall
2022-03-07 22:23 ` Ayan Kumar Halder
0 siblings, 1 reply; 28+ messages in thread
From: Julien Grall @ 2022-03-07 19:37 UTC (permalink / raw)
To: Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefanos, Volodymyr_Babchuk, bertrand.marquis,
andrew.cooper3, george.dunlap, jbeulich, wl, paul, roger.pau
On 07/03/2022 14:27, Ayan Kumar Halder wrote:
> Hi Julien,
Hi Ayan,
>
> One clarification.
>
> On 04/03/2022 10:39, Julien Grall wrote:
>> Hi Ayan,
>>
>> On 01/03/2022 12:40, Ayan Kumar Halder wrote:
>>> If the abort was caused due to access to stage1 translation table, Xen
>>> will assume that the stage1 translation table is in the non MMIO region.
Reading this commit message again. I think you want to explain why we
want to do that because, from my understanding, this is technically not
forbidden by the Arm Arm.
From the previous discussion, we want to do this because we can't
easily handle such fault on emulated region (we have no away to the
walker the value read).
>>> It will try to resolve the translation fault. If it succeeds, it will
>>> return to the guest to retry the instruction. If not, then it means
>>> that the table is in MMIO region which is not expected by Xen. Thus,
>>> Xen will forward the abort to the guest.
>>>
>>> Signed-off-by: Ayan Kumar Halder <ayankuma@xilinx.com>
>>> ---
>>>
>>> Changelog :-
>>>
>>> v1..v8 - NA
>>>
>>> v9 - 1. Extracted this change from "[XEN v8 2/2] xen/arm64: io: Support
>>> instructions (for which ISS is not..." into a separate patch of its own.
>>> The reason being this is an existing bug in the codebase.
>>>
>>> xen/arch/arm/io.c | 11 +++++++++++
>>> xen/arch/arm/traps.c | 12 +++++++++++-
>>> 2 files changed, 22 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
>>> index bea69ffb08..ebcb8ed548 100644
>>> --- a/xen/arch/arm/io.c
>>> +++ b/xen/arch/arm/io.c
>>> @@ -128,6 +128,17 @@ void try_decode_instruction(const struct
>>> cpu_user_regs *regs,
>>> return;
>>> }
>>> + /*
>>> + * At this point, we know that the stage1 translation table is
>>> in the MMIO
>>> + * region. This is not expected by Xen and thus it forwards the
>>> abort to the
>>
>> We don't know that. We only know that there are no corresponding valid
>> mapping in the P2M. So the address may be part of an emulated MMIO
>> region or invalid.
>>
>> For both cases, we will want to send an abort.
>>
>> Furthermore, I would say "emulated MMIO region" rather than MMIO
>> region because the P2M can also contain MMIO mapping (we usually call
>> then "direct MMIO").
>
> Can I say MMIO region (to indicate both emulated and direct) ? The
> reason being the assumption that stage1 page tables cannot be in the
> MMIO region. And thus, when check_p2m() is invoked, we do not invoke
> try_map_mmio(gaddr_to_gfn(gpa).
>
> See this snippet :-
>
> if ( xabt.s1ptw )
> check_mmio_region = false;
Thinking a bit more of this. I would actually drop this check. We don't
need to decode the instruction, so I don't think there are much benefits
here to restrict access for direct MMIO. Did I miss anything?
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [XEN v9 3/4] xen/arm64: io: Handle the abort due to access to stage1 translation table
2022-03-07 19:37 ` Julien Grall
@ 2022-03-07 22:23 ` Ayan Kumar Halder
2022-03-07 23:59 ` Julien Grall
0 siblings, 1 reply; 28+ messages in thread
From: Ayan Kumar Halder @ 2022-03-07 22:23 UTC (permalink / raw)
To: Julien Grall, Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefanos, Volodymyr_Babchuk, bertrand.marquis,
andrew.cooper3, george.dunlap, jbeulich, wl, paul, roger.pau
On 07/03/2022 19:37, Julien Grall wrote:
>
>
> On 07/03/2022 14:27, Ayan Kumar Halder wrote:
>> Hi Julien,
>
> Hi Ayan,
Hi Julien,
I need a bit of clarification to understand this.
>
>>
>> One clarification.
>>
>> On 04/03/2022 10:39, Julien Grall wrote:
>>> Hi Ayan,
>>>
>>> On 01/03/2022 12:40, Ayan Kumar Halder wrote:
>>>> If the abort was caused due to access to stage1 translation table, Xen
>>>> will assume that the stage1 translation table is in the non MMIO
>>>> region.
>
> Reading this commit message again. I think you want to explain why we
> want to do that because, from my understanding, this is technically
> not forbidden by the Arm Arm.
>
> From the previous discussion, we want to do this because we can't
> easily handle such fault on emulated region (we have no away to the
> walker the value read).
Sorry, Can you explain this a bit more ? Do you mean that if the page
table is located in the emulated region, map_domain_page() (called from
p2m_next_level()) will fail.
But for emulated region, shouldn't pages be already mapped for Xen to
access them ?
>
>>>> It will try to resolve the translation fault. If it succeeds, it will
>>>> return to the guest to retry the instruction. If not, then it means
>>>> that the table is in MMIO region which is not expected by Xen. Thus,
>>>> Xen will forward the abort to the guest.
>>>>
>>>> Signed-off-by: Ayan Kumar Halder <ayankuma@xilinx.com>
>>>> ---
>>>>
>>>> Changelog :-
>>>>
>>>> v1..v8 - NA
>>>>
>>>> v9 - 1. Extracted this change from "[XEN v8 2/2] xen/arm64: io:
>>>> Support
>>>> instructions (for which ISS is not..." into a separate patch of its
>>>> own.
>>>> The reason being this is an existing bug in the codebase.
>>>>
>>>> xen/arch/arm/io.c | 11 +++++++++++
>>>> xen/arch/arm/traps.c | 12 +++++++++++-
>>>> 2 files changed, 22 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
>>>> index bea69ffb08..ebcb8ed548 100644
>>>> --- a/xen/arch/arm/io.c
>>>> +++ b/xen/arch/arm/io.c
>>>> @@ -128,6 +128,17 @@ void try_decode_instruction(const struct
>>>> cpu_user_regs *regs,
>>>> return;
>>>> }
>>>> + /*
>>>> + * At this point, we know that the stage1 translation table is
>>>> in the MMIO
>>>> + * region. This is not expected by Xen and thus it forwards
>>>> the abort to the
>>>
>>> We don't know that. We only know that there are no corresponding
>>> valid mapping in the P2M. So the address may be part of an emulated
>>> MMIO region or invalid.
>>>
>>> For both cases, we will want to send an abort.
>>>
>>> Furthermore, I would say "emulated MMIO region" rather than MMIO
>>> region because the P2M can also contain MMIO mapping (we usually
>>> call then "direct MMIO").
>>
>> Can I say MMIO region (to indicate both emulated and direct) ? The
>> reason being the assumption that stage1 page tables cannot be in the
>> MMIO region. And thus, when check_p2m() is invoked, we do not invoke
>> try_map_mmio(gaddr_to_gfn(gpa).
>>
>> See this snippet :-
>>
>> if ( xabt.s1ptw )
>> check_mmio_region = false;
>
> Thinking a bit more of this. I would actually drop this check. We
> don't need to decode the instruction, so I don't think there are much
> benefits here to restrict access for direct MMIO. Did I miss anything?
>
Can Linux or any OS keep its page tables in the direct MMIO space ? If
yes, then try_map_mmio() needs to be invoked to map the region, so that
OS can access it. If not, then Xen needs to return abort because the OS
may be behaving maliciously.
My understanding from previous discussion was that it does not make
sense for linux or any OS to keep its page tables in any MMIO region
(emulated or direct). Please correct me if mistaken.
- Ayan
> Cheers,
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [XEN v9 3/4] xen/arm64: io: Handle the abort due to access to stage1 translation table
2022-03-07 22:23 ` Ayan Kumar Halder
@ 2022-03-07 23:59 ` Julien Grall
2022-03-08 11:22 ` Ayan Kumar Halder
0 siblings, 1 reply; 28+ messages in thread
From: Julien Grall @ 2022-03-07 23:59 UTC (permalink / raw)
To: Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefanos, Volodymyr_Babchuk, bertrand.marquis,
andrew.cooper3, george.dunlap, jbeulich, wl, paul, roger.pau
Hi,
On 07/03/2022 22:23, Ayan Kumar Halder wrote:
>
> On 07/03/2022 19:37, Julien Grall wrote:
>>
>>
>> On 07/03/2022 14:27, Ayan Kumar Halder wrote:
>>> Hi Julien,
>>
>> Hi Ayan,
>
> Hi Julien,
>
> I need a bit of clarification to understand this.
>
>>
>>>
>>> One clarification.
>>>
>>> On 04/03/2022 10:39, Julien Grall wrote:
>>>> Hi Ayan,
>>>>
>>>> On 01/03/2022 12:40, Ayan Kumar Halder wrote:
>>>>> If the abort was caused due to access to stage1 translation table, Xen
>>>>> will assume that the stage1 translation table is in the non MMIO
>>>>> region.
>>
>> Reading this commit message again. I think you want to explain why we
>> want to do that because, from my understanding, this is technically
>> not forbidden by the Arm Arm.
>>
>> From the previous discussion, we want to do this because we can't
>> easily handle such fault on emulated region (we have no away to the
>> walker the value read).
>
> Sorry, Can you explain this a bit more ? Do you mean that if the page
> table is located in the emulated region, map_domain_page() (called from
> p2m_next_level()) will fail.
For data abort with valid syndrome, you will have a register to write
back the value read. When the data abort has s1ptw == 1, AFAICT, we have
no information how to return the value.
>
> But for emulated region, shouldn't pages be already mapped for Xen to
> access them ?
I am not sure which "pages" you are referring to here. The
implementation of emulated regions is left to the discretion of Xen.
This may be reading/writing to a buffer allocated by Xen or return a
fixed value.
>
>>
>>>>> It will try to resolve the translation fault. If it succeeds, it will
>>>>> return to the guest to retry the instruction. If not, then it means
>>>>> that the table is in MMIO region which is not expected by Xen. Thus,
>>>>> Xen will forward the abort to the guest.
>>>>>
>>>>> Signed-off-by: Ayan Kumar Halder <ayankuma@xilinx.com>
>>>>> ---
>>>>>
>>>>> Changelog :-
>>>>>
>>>>> v1..v8 - NA
>>>>>
>>>>> v9 - 1. Extracted this change from "[XEN v8 2/2] xen/arm64: io:
>>>>> Support
>>>>> instructions (for which ISS is not..." into a separate patch of its
>>>>> own.
>>>>> The reason being this is an existing bug in the codebase.
>>>>>
>>>>> xen/arch/arm/io.c | 11 +++++++++++
>>>>> xen/arch/arm/traps.c | 12 +++++++++++-
>>>>> 2 files changed, 22 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
>>>>> index bea69ffb08..ebcb8ed548 100644
>>>>> --- a/xen/arch/arm/io.c
>>>>> +++ b/xen/arch/arm/io.c
>>>>> @@ -128,6 +128,17 @@ void try_decode_instruction(const struct
>>>>> cpu_user_regs *regs,
>>>>> return;
>>>>> }
>>>>> + /*
>>>>> + * At this point, we know that the stage1 translation table is
>>>>> in the MMIO
>>>>> + * region. This is not expected by Xen and thus it forwards
>>>>> the abort to the
>>>>
>>>> We don't know that. We only know that there are no corresponding
>>>> valid mapping in the P2M. So the address may be part of an emulated
>>>> MMIO region or invalid.
>>>>
>>>> For both cases, we will want to send an abort.
>>>>
>>>> Furthermore, I would say "emulated MMIO region" rather than MMIO
>>>> region because the P2M can also contain MMIO mapping (we usually
>>>> call then "direct MMIO").
>>>
>>> Can I say MMIO region (to indicate both emulated and direct) ? The
>>> reason being the assumption that stage1 page tables cannot be in the
>>> MMIO region. And thus, when check_p2m() is invoked, we do not invoke
>>> try_map_mmio(gaddr_to_gfn(gpa).
>>>
>>> See this snippet :-
>>>
>>> if ( xabt.s1ptw )
>>> check_mmio_region = false;
>>
>> Thinking a bit more of this. I would actually drop this check. We
>> don't need to decode the instruction, so I don't think there are much
>> benefits here to restrict access for direct MMIO. Did I miss anything?
>>
> Can Linux or any OS keep its page tables in the direct MMIO space ? If
> yes, then try_map_mmio() needs to be invoked to map the region, so that
> OS can access it. If not, then Xen needs to return abort because the OS
> may be behaving maliciously.
I think what matters is whether the Arm Arm would or would not allow it.
From what I can tell there are no such restriction. So we would need to
be cautious to restrict it further than necessary.
>
> My understanding from previous discussion was that it does not make
> sense for linux or any OS to keep its page tables in any MMIO region
> (emulated or direct). Please correct me if mistaken.
At the moment, none of the regions emulated by Xen could be used for
page-tables. I am also not sure how we should handle such access if they
arise. So it is more convenient to simply forbid them.
Regarding direct MMIO, see above. Correct me if I am wrong, but it
should not be a problem for Xen to deal with them. So while I agree this
doesn't seem to make sense, the restriction seems unnecessary.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [XEN v9 3/4] xen/arm64: io: Handle the abort due to access to stage1 translation table
2022-03-07 23:59 ` Julien Grall
@ 2022-03-08 11:22 ` Ayan Kumar Halder
2022-03-08 17:38 ` Julien Grall
0 siblings, 1 reply; 28+ messages in thread
From: Ayan Kumar Halder @ 2022-03-08 11:22 UTC (permalink / raw)
To: Julien Grall, Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefanos, Volodymyr_Babchuk, bertrand.marquis,
andrew.cooper3, george.dunlap, jbeulich, wl, paul, roger.pau
Hi Julien,
On 07/03/2022 23:59, Julien Grall wrote:
> Hi,
>
> On 07/03/2022 22:23, Ayan Kumar Halder wrote:
>>
>> On 07/03/2022 19:37, Julien Grall wrote:
>>>
>>>
>>> On 07/03/2022 14:27, Ayan Kumar Halder wrote:
>>>> Hi Julien,
>>>
>>> Hi Ayan,
>>
>> Hi Julien,
>>
>> I need a bit of clarification to understand this.
>>
>>>
>>>>
>>>> One clarification.
>>>>
>>>> On 04/03/2022 10:39, Julien Grall wrote:
>>>>> Hi Ayan,
>>>>>
>>>>> On 01/03/2022 12:40, Ayan Kumar Halder wrote:
>>>>>> If the abort was caused due to access to stage1 translation
>>>>>> table, Xen
>>>>>> will assume that the stage1 translation table is in the non MMIO
>>>>>> region.
>>>
>>> Reading this commit message again. I think you want to explain why
>>> we want to do that because, from my understanding, this is
>>> technically not forbidden by the Arm Arm.
>>>
>>> From the previous discussion, we want to do this because we can't
>>> easily handle such fault on emulated region (we have no away to the
>>> walker the value read).
>>
>> Sorry, Can you explain this a bit more ? Do you mean that if the page
>> table is located in the emulated region, map_domain_page() (called
>> from p2m_next_level()) will fail.
>
> For data abort with valid syndrome, you will have a register to write
> back the value read. When the data abort has s1ptw == 1, AFAICT, we
> have no information how to return the value.
Do you mean that for s1ptw, we get an intermediate physical address ?
if ( hpfar_is_valid(xabt.s1ptw, fsc) )
gpa = get_faulting_ipa(gva);
If the IPA corresponds to an emulated region, then Xen can read the
emulated address, but can't return the value to the guest OS.
(I actually want to understand this very well).
>
>>
>> But for emulated region, shouldn't pages be already mapped for Xen to
>> access them ?
>
> I am not sure which "pages" you are referring to here. The
> implementation of emulated regions is left to the discretion of Xen.
> This may be reading/writing to a buffer allocated by Xen or return a
> fixed value.
>
>>
>>>
>>>>>> It will try to resolve the translation fault. If it succeeds, it
>>>>>> will
>>>>>> return to the guest to retry the instruction. If not, then it means
>>>>>> that the table is in MMIO region which is not expected by Xen. Thus,
>>>>>> Xen will forward the abort to the guest.
>>>>>>
>>>>>> Signed-off-by: Ayan Kumar Halder <ayankuma@xilinx.com>
>>>>>> ---
>>>>>>
>>>>>> Changelog :-
>>>>>>
>>>>>> v1..v8 - NA
>>>>>>
>>>>>> v9 - 1. Extracted this change from "[XEN v8 2/2] xen/arm64: io:
>>>>>> Support
>>>>>> instructions (for which ISS is not..." into a separate patch of
>>>>>> its own.
>>>>>> The reason being this is an existing bug in the codebase.
>>>>>>
>>>>>> xen/arch/arm/io.c | 11 +++++++++++
>>>>>> xen/arch/arm/traps.c | 12 +++++++++++-
>>>>>> 2 files changed, 22 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
>>>>>> index bea69ffb08..ebcb8ed548 100644
>>>>>> --- a/xen/arch/arm/io.c
>>>>>> +++ b/xen/arch/arm/io.c
>>>>>> @@ -128,6 +128,17 @@ void try_decode_instruction(const struct
>>>>>> cpu_user_regs *regs,
>>>>>> return;
>>>>>> }
>>>>>> + /*
>>>>>> + * At this point, we know that the stage1 translation table
>>>>>> is in the MMIO
>>>>>> + * region. This is not expected by Xen and thus it forwards
>>>>>> the abort to the
>>>>>
>>>>> We don't know that. We only know that there are no corresponding
>>>>> valid mapping in the P2M. So the address may be part of an
>>>>> emulated MMIO region or invalid.
>>>>>
>>>>> For both cases, we will want to send an abort.
>>>>>
>>>>> Furthermore, I would say "emulated MMIO region" rather than MMIO
>>>>> region because the P2M can also contain MMIO mapping (we usually
>>>>> call then "direct MMIO").
>>>>
>>>> Can I say MMIO region (to indicate both emulated and direct) ? The
>>>> reason being the assumption that stage1 page tables cannot be in
>>>> the MMIO region. And thus, when check_p2m() is invoked, we do not
>>>> invoke try_map_mmio(gaddr_to_gfn(gpa).
>>>>
>>>> See this snippet :-
>>>>
>>>> if ( xabt.s1ptw )
>>>> check_mmio_region = false;
>>>
>>> Thinking a bit more of this. I would actually drop this check. We
>>> don't need to decode the instruction, so I don't think there are
>>> much benefits here to restrict access for direct MMIO. Did I miss
>>> anything?
>>>
>> Can Linux or any OS keep its page tables in the direct MMIO space ?
>> If yes, then try_map_mmio() needs to be invoked to map the region, so
>> that OS can access it. If not, then Xen needs to return abort because
>> the OS may be behaving maliciously.
>
> I think what matters is whether the Arm Arm would or would not allow
> it. From what I can tell there are no such restriction. So we would
> need to be cautious to restrict it further than necessary.
>
>>
>> My understanding from previous discussion was that it does not make
>> sense for linux or any OS to keep its page tables in any MMIO region
>> (emulated or direct). Please correct me if mistaken.
>
> At the moment, none of the regions emulated by Xen could be used for
> page-tables. I am also not sure how we should handle such access if
> they arise. So it is more convenient to simply forbid them.
>
> Regarding direct MMIO, see above. Correct me if I am wrong, but it
> should not be a problem for Xen to deal with them. So while I agree
> this doesn't seem to make sense, the restriction seems unnecessary.
So the behavior will be :-
1. If the stage1 translation table is in the non MMIO region or 'direct
mapped' MMIO region, then invoke p2m_resolve_translation_fault() and
try_map_mmio() to resolve the fault. If it succeeds, then return to the
guest to retry.
2. If the previous step fails and for any other scenario (ie stage1
translation table is in emulated MMIO region or the address is invalid),
return the abort to the guest.
- Ayan
>
> Cheers,
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [XEN v9 3/4] xen/arm64: io: Handle the abort due to access to stage1 translation table
2022-03-08 11:22 ` Ayan Kumar Halder
@ 2022-03-08 17:38 ` Julien Grall
0 siblings, 0 replies; 28+ messages in thread
From: Julien Grall @ 2022-03-08 17:38 UTC (permalink / raw)
To: Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefanos, Volodymyr_Babchuk, bertrand.marquis,
andrew.cooper3, george.dunlap, jbeulich, wl, paul, roger.pau
Hi,
On 08/03/2022 11:22, Ayan Kumar Halder wrote:
> Hi Julien,
>
> On 07/03/2022 23:59, Julien Grall wrote:
>> Hi,
>>
>> On 07/03/2022 22:23, Ayan Kumar Halder wrote:
>>>
>>> On 07/03/2022 19:37, Julien Grall wrote:
>>>>
>>>>
>>>> On 07/03/2022 14:27, Ayan Kumar Halder wrote:
>>>>> Hi Julien,
>>>>
>>>> Hi Ayan,
>>>
>>> Hi Julien,
>>>
>>> I need a bit of clarification to understand this.
>>>
>>>>
>>>>>
>>>>> One clarification.
>>>>>
>>>>> On 04/03/2022 10:39, Julien Grall wrote:
>>>>>> Hi Ayan,
>>>>>>
>>>>>> On 01/03/2022 12:40, Ayan Kumar Halder wrote:
>>>>>>> If the abort was caused due to access to stage1 translation
>>>>>>> table, Xen
>>>>>>> will assume that the stage1 translation table is in the non MMIO
>>>>>>> region.
>>>>
>>>> Reading this commit message again. I think you want to explain why
>>>> we want to do that because, from my understanding, this is
>>>> technically not forbidden by the Arm Arm.
>>>>
>>>> From the previous discussion, we want to do this because we can't
>>>> easily handle such fault on emulated region (we have no away to the
>>>> walker the value read).
>>>
>>> Sorry, Can you explain this a bit more ? Do you mean that if the page
>>> table is located in the emulated region, map_domain_page() (called
>>> from p2m_next_level()) will fail.
>>
>> For data abort with valid syndrome, you will have a register to write
>> back the value read. When the data abort has s1ptw == 1, AFAICT, we
>> have no information how to return the value.
>
> Do you mean that for s1ptw, we get an intermediate physical address ?
>
> if ( hpfar_is_valid(xabt.s1ptw, fsc) )
> gpa = get_faulting_ipa(gva);
No. That's not relevant here. We always need a IPA in order to deal with
the fault.
>
> If the IPA corresponds to an emulated region, then Xen can read the
> emulated address, but can't return the value to the guest OS.
That wouldn't be the guest OS. But the page-table walker in the CPU.
>>> Can Linux or any OS keep its page tables in the direct MMIO space ?
>>> If yes, then try_map_mmio() needs to be invoked to map the region, so
>>> that OS can access it. If not, then Xen needs to return abort because
>>> the OS may be behaving maliciously.
>>
>> I think what matters is whether the Arm Arm would or would not allow
>> it. From what I can tell there are no such restriction. So we would
>> need to be cautious to restrict it further than necessary.
>>
>>>
>>> My understanding from previous discussion was that it does not make
>>> sense for linux or any OS to keep its page tables in any MMIO region
>>> (emulated or direct). Please correct me if mistaken.
>>
>> At the moment, none of the regions emulated by Xen could be used for
>> page-tables. I am also not sure how we should handle such access if
>> they arise. So it is more convenient to simply forbid them.
>>
>> Regarding direct MMIO, see above. Correct me if I am wrong, but it
>> should not be a problem for Xen to deal with them. So while I agree
>> this doesn't seem to make sense, the restriction seems unnecessary.
>
> So the behavior will be :-
>
> 1. If the stage1 translation table is in the non MMIO region or 'direct
> mapped' MMIO region, then invoke p2m_resolve_translation_fault() and
> try_map_mmio() to resolve the fault. If it succeeds, then return to the
> guest to retry.
When 1. happens we don't know yet if the stage1 will be a non MMIO
region or 'direct mapped'. Instead, we only know that the ISS syndrome
is invalid.
If it is a prefetch abort or the syndrome is invalid, then we should
call check_p2m().
>
> 2. If the previous step fails and for any other scenario (ie stage1
> translation table is in emulated MMIO region or the address is invalid),
> return the abort to the guest.
If check_p2m() didn't success and it is a fault on stage-1 walk, then we
should send an abort.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2022-03-08 17:39 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-01 12:40 [XEN v9 0/4] xen/arm64: io: Decode ldr/str post-indexing instruction Ayan Kumar Halder
2022-03-01 12:40 ` [XEN v9 1/4] xen/arm64: Decode ldr/str post increment operations Ayan Kumar Halder
2022-03-04 0:42 ` Stefano Stabellini
2022-03-01 12:40 ` [XEN v9 2/4] xen/arm64: io: Support instructions (for which ISS is not valid) on emulated MMIO region using MMIO/ioreq handler Ayan Kumar Halder
2022-03-04 0:42 ` Stefano Stabellini
2022-03-04 10:30 ` Julien Grall
2022-03-04 22:23 ` Stefano Stabellini
2022-03-04 10:28 ` Julien Grall
2022-03-04 11:27 ` Ayan Kumar Halder
2022-03-04 12:45 ` Julien Grall
2022-03-01 12:40 ` [XEN v9 3/4] xen/arm64: io: Handle the abort due to access to stage1 translation table Ayan Kumar Halder
2022-03-04 1:43 ` Stefano Stabellini
2022-03-04 17:04 ` Ayan Kumar Halder
2022-03-04 22:34 ` Stefano Stabellini
2022-03-04 10:39 ` Julien Grall
2022-03-07 14:27 ` Ayan Kumar Halder
2022-03-07 19:37 ` Julien Grall
2022-03-07 22:23 ` Ayan Kumar Halder
2022-03-07 23:59 ` Julien Grall
2022-03-08 11:22 ` Ayan Kumar Halder
2022-03-08 17:38 ` Julien Grall
2022-03-01 12:40 ` [XEN v9 4/4] xen/arm64: io: Handle data abort due to cache maintenance instructions Ayan Kumar Halder
2022-03-04 1:44 ` Stefano Stabellini
2022-03-04 10:46 ` Julien Grall
2022-03-04 12:13 ` Ayan Kumar Halder
2022-03-04 12:49 ` Julien Grall
2022-03-04 14:40 ` Ayan Kumar Halder
2022-03-04 18:37 ` Julien Grall
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.