* [PATCH v11 0/3] xen/arm64: io: Decode ldr/str post-indexing instruction
@ 2022-03-17 14:00 Ayan Kumar Halder
2022-03-17 14:00 ` [PATCH v11 1/3] xen/arm64: io: Emulate instructions (with invalid ISS) on MMIO region Ayan Kumar Halder
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Ayan Kumar Halder @ 2022-03-17 14:00 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 continues with the 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.
This is a continuation of the previous patch series "[XEN v10 0/4] xen/arm64: io:
Decode ldr/str post-indexing instruction". In the previous series, patches 2 and
3 had to be reverted as they cause a build break on x86 and boot failure on arm32.
Only patch 1 (ie "[XEN v10 1/4] xen/arm64: Decode ldr/str post increment operations"
was committed)
While doing the patch, we found two bugs in the codebase. I have addressed them
in patches 2 and 3. This was discussed with Julien on the IRC.
Ayan Kumar Halder (3):
xen/arm64: io: Emulate instructions (with invalid ISS) on MMIO region
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 | 12 +++
xen/arch/arm/arm64/traps.c | 52 +++++++++++++
xen/arch/arm/decode.c | 2 +
xen/arch/arm/include/asm/domain.h | 4 +
xen/arch/arm/include/asm/mmio.h | 18 ++++-
xen/arch/arm/include/asm/traps.h | 2 +
xen/arch/arm/io.c | 117 +++++++++++++++++++++---------
xen/arch/arm/ioreq.c | 15 +++-
xen/arch/arm/traps.c | 77 ++++++++++++++++----
xen/arch/x86/include/asm/domain.h | 3 +
xen/include/xen/sched.h | 2 +
11 files changed, 251 insertions(+), 53 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.
v10 - 1. Changes mentioned in the individual patches.
v11 - 1. Patch 1/4 from v10 was committed. Thus, this series contains patches 2..4.
2. Changes mentioned in the individual patches.
--
2.17.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v11 1/3] xen/arm64: io: Emulate instructions (with invalid ISS) on MMIO region
2022-03-17 14:00 [PATCH v11 0/3] xen/arm64: io: Decode ldr/str post-indexing instruction Ayan Kumar Halder
@ 2022-03-17 14:00 ` Ayan Kumar Halder
2022-03-17 22:24 ` Stefano Stabellini
2022-03-18 18:14 ` Julien Grall
2022-03-17 14:00 ` [PATCH v11 2/3] xen/arm64: io: Handle the abort due to access to stage1 translation table Ayan Kumar Halder
2022-03-17 14:00 ` [PATCH v11 3/3] xen/arm64: io: Handle data abort due to cache maintenance instructions Ayan Kumar Halder
2 siblings, 2 replies; 15+ messages in thread
From: Ayan Kumar Halder @ 2022-03-17 14:00 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.
Also, 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.
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.
v10 - 1. Set 'dabt.valid=1' when the instruction is fully decoded. This is
checked in try_handle_mmio() and try_fwd_ioserv().
2. Various other style issues pointed in v9.
v11 - 1. Renamed post_increment_register() to finalize_instr_emulation().
2. Moved "struct arch_vcpu_io { }" from xen/arch/x86/include/asm/ioreq.h to
xen/arch/x86/include/asm/domain.h as this is included in sched.h. This fixes the
build break for x86.
3. For arm32, check "( instr->state == INSTR_LDR_STR_POSTINDEXING )" before
calling domain_crash(). This fixes the boot failure for arm32.
4. Restricted the commit header to 80 chars.
xen/arch/arm/arm32/traps.c | 12 +++++
xen/arch/arm/arm64/traps.c | 52 ++++++++++++++++++
xen/arch/arm/decode.c | 2 +
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 | 8 ++-
xen/arch/arm/traps.c | 77 ++++++++++++++++++++------
xen/arch/x86/include/asm/domain.h | 3 ++
xen/include/xen/sched.h | 2 +
11 files changed, 215 insertions(+), 54 deletions(-)
diff --git a/xen/arch/arm/arm32/traps.c b/xen/arch/arm/arm32/traps.c
index 9c9790a6d1..a4ce2b92d9 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,16 @@ void do_trap_data_abort(struct cpu_user_regs *regs)
do_unexpected_trap("Data Abort", regs);
}
+void finalize_instr_emulation(const struct instr_details *instr)
+{
+ /*
+ * We have not implemented decoding of post indexing instructions for 32 bit.
+ * Thus, this should be unreachable.
+ */
+ if ( instr->state == INSTR_LDR_STR_POSTINDEXING )
+ 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..3f8858acec 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,57 @@ void do_bad_mode(struct cpu_user_regs *regs, int reason)
panic("bad mode\n");
}
+void finalize_instr_emulation(const struct instr_details *instr)
+{
+ struct cpu_user_regs *regs = guest_cpu_user_regs();
+ register_t val = 0;
+ uint8_t psr_mode = (regs->cpsr & PSR_MODE_MASK);
+
+ /* 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 )
+ {
+ switch ( psr_mode )
+ {
+ case PSR_MODE_EL1h:
+ val = regs->sp_el1;
+ break;
+ case PSR_MODE_EL1t:
+ case PSR_MODE_EL0t:
+ val = regs->sp_el0;
+ break;
+
+ default:
+ 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..f5f6562600 100644
--- a/xen/arch/arm/decode.c
+++ b/xen/arch/arm/decode.c
@@ -146,8 +146,10 @@ 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;
+ dabt->valid = 1;
return 0;
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..ca259a79c2 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..08bc0b484c 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 finalize_instr_emulation(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..fd903b7b03 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.valid )
{
- 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..54167aebcb 100644
--- a/xen/arch/arm/ioreq.c
+++ b/xen/arch/arm/ioreq.c
@@ -47,6 +47,8 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs,
struct vcpu *v, mmio_info_t *info)
{
struct vcpu_io *vio = &v->io;
+ struct instr_details instr = info->dabt_instr;
+ struct hsr_dabt dabt = info->dabt;
ioreq_t p = {
.type = IOREQ_TYPE_COPY,
.addr = info->gpa,
@@ -76,10 +78,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 +97,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 +109,7 @@ bool arch_ioreq_complete_mmio(void)
if ( handle_ioserv(regs, v) == IO_HANDLED )
{
+ finalize_instr_emulation(&dabt_instr);
advance_pc(regs, hsr);
return true;
}
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 7a1b679b8c..11f970d926 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.
+ */
+ finalize_instr_emulation(&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_instr.state == INSTR_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/domain.h b/xen/arch/x86/include/asm/domain.h
index e62e109598..35898d725f 100644
--- a/xen/arch/x86/include/asm/domain.h
+++ b/xen/arch/x86/include/asm/domain.h
@@ -756,6 +756,9 @@ static inline void pv_inject_sw_interrupt(unsigned int vector)
: is_pv_32bit_domain(d) ? PV32_VM_ASSIST_MASK \
: PV64_VM_ASSIST_MASK)
+struct arch_vcpu_io {
+};
+
#endif /* __ASM_DOMAIN_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] 15+ messages in thread
* [PATCH v11 2/3] xen/arm64: io: Handle the abort due to access to stage1 translation table
2022-03-17 14:00 [PATCH v11 0/3] xen/arm64: io: Decode ldr/str post-indexing instruction Ayan Kumar Halder
2022-03-17 14:00 ` [PATCH v11 1/3] xen/arm64: io: Emulate instructions (with invalid ISS) on MMIO region Ayan Kumar Halder
@ 2022-03-17 14:00 ` Ayan Kumar Halder
2022-03-17 22:27 ` Stefano Stabellini
2022-03-18 18:15 ` Julien Grall
2022-03-17 14:00 ` [PATCH v11 3/3] xen/arm64: io: Handle data abort due to cache maintenance instructions Ayan Kumar Halder
2 siblings, 2 replies; 15+ messages in thread
From: Ayan Kumar Halder @ 2022-03-17 14:00 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 try to set the p2m entry (assuming that the Stage 1 translation
table is in a non MMIO region).
If there is no such entry found, then Xen will try to map the address as
a MMIO region (assuming that the Stage 1 translation table is in a
direct MMIO region).
If that fails as well, then there are the two following scenarios:-
1. Stage 1 translation table being in an emulated MMIO region - Xen
can read the region, but it has no way to return the value read to the
CPU page table walker (which tries to go through the stage1 tables to
resolve the translation fault).
2. Stage 1 translation table address is invalid.
In both the above scenarios, 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.
v10 - 1. Enabled checking for stage1 translation table address in the
MMIO region. The reason being Arm Arm does not have any restrictions.
2. Updated the commit message to explain all the possible scenarios.
v11 - 1. Fixed some wordings in comments and commit message (pointed
by Julien in v10).
xen/arch/arm/io.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
index fd903b7b03..6f458ee7fd 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 either in an
+ * emulated MMIO region or its address is invalid . 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
--
2.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v11 3/3] xen/arm64: io: Handle data abort due to cache maintenance instructions
2022-03-17 14:00 [PATCH v11 0/3] xen/arm64: io: Decode ldr/str post-indexing instruction Ayan Kumar Halder
2022-03-17 14:00 ` [PATCH v11 1/3] xen/arm64: io: Emulate instructions (with invalid ISS) on MMIO region Ayan Kumar Halder
2022-03-17 14:00 ` [PATCH v11 2/3] xen/arm64: io: Handle the abort due to access to stage1 translation table Ayan Kumar Halder
@ 2022-03-17 14:00 ` Ayan Kumar Halder
2022-03-17 22:33 ` Stefano Stabellini
2022-03-18 18:26 ` Julien Grall
2 siblings, 2 replies; 15+ messages in thread
From: Ayan Kumar Halder @ 2022-03-17 14:00 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 three scenarios:-
1. Address belonging to a non emulated region - For this, Xen should
set the corresponding bit in the translation table entry to valid and
return to the guest to retry the instruction. This can happen sometimes
as Xen need to set the translation table entry to invalid. (for eg
'Break-Before-Make' sequence). Xen returns to the guest to retry the
instruction.
2. Address belongs to an emulated region - Xen should ignore the
instruction (ie increment the PC) and return to the guest.
3. Address is invalid - Xen should forward the data abort to the guest.
Signed-off-by: Ayan Kumar Halder <ayankuma@xilinx.com>
---
Changelog:-
v1...v8 - NA
v9 - Extracted this change from "[XEN v7 2/2] xen/arm64: io: Support
instructions (for which ISS is not ..." into a separate patch of its
own. The reason being this addresses an existing bug in the codebase.
v10 - 1. To check if the address belongs to an emulated region, one
needs to check if it has a mmio handler or an ioreq server. In this
case, Xen should increment the PC
2. If the address is invalid (niether emulated MMIO nor the translation
could be resolved via p2m or mapping the MMIO region), then Xen should
forward the abort to the guest.
v11 - 1. Removed the un-necessary check "( instr.state == INSTR_CACHE )"
in handle_ioserv(). The reason being the ioserv request is not forwarded
by try_fwd_ioserv() when instr.state == INSTR_CACHE.
xen/arch/arm/include/asm/mmio.h | 1 +
xen/arch/arm/io.c | 18 ++++++++++++++++++
xen/arch/arm/ioreq.c | 9 ++++++++-
3 files changed, 27 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/include/asm/mmio.h b/xen/arch/arm/include/asm/mmio.h
index ca259a79c2..79e64d9af8 100644
--- a/xen/arch/arm/include/asm/mmio.h
+++ b/xen/arch/arm/include/asm/mmio.h
@@ -35,6 +35,7 @@ enum instr_decode_state
* instruction.
*/
INSTR_LDR_STR_POSTINDEXING,
+ INSTR_CACHE, /* Cache Maintenance instr */
};
typedef struct
diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
index 6f458ee7fd..26c716b4a5 100644
--- a/xen/arch/arm/io.c
+++ b/xen/arch/arm/io.c
@@ -139,6 +139,17 @@ void try_decode_instruction(const struct cpu_user_regs *regs,
return;
}
+ /*
+ * When the data abort is caused due to cache maintenance, Xen should check
+ * if the address belongs to an emulated MMIO region or not. The behavior
+ * will differ accordingly.
+ */
+ if ( info->dabt.cache )
+ {
+ info->dabt_instr.state = INSTR_CACHE;
+ return;
+ }
+
/*
* Armv8 processor does not provide a valid syndrome for decoding some
* instructions. So in order to process these instructions, Xen must
@@ -177,6 +188,13 @@ enum io_state try_handle_mmio(struct cpu_user_regs *regs,
return rc;
}
+ /*
+ * When the data abort is caused due to cache maintenance and the address
+ * belongs to an emulated region, Xen should ignore this instruction.
+ */
+ if ( info->dabt_instr.state == INSTR_CACHE )
+ return IO_HANDLED;
+
/*
* At this point, we know that the instruction is either valid or has been
* decoded successfully. Thus, Xen should be allowed to execute the
diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c
index 54167aebcb..cc66713386 100644
--- a/xen/arch/arm/ioreq.c
+++ b/xen/arch/arm/ioreq.c
@@ -47,7 +47,7 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs,
struct vcpu *v, mmio_info_t *info)
{
struct vcpu_io *vio = &v->io;
- struct instr_details instr = info->dabt_instr;
+ const struct instr_details instr = info->dabt_instr;
struct hsr_dabt dabt = info->dabt;
ioreq_t p = {
.type = IOREQ_TYPE_COPY,
@@ -78,6 +78,13 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs,
if ( !s )
return IO_UNHANDLED;
+ /*
+ * When the data abort is caused due to cache maintenance and the address
+ * belongs to an emulated region, Xen should ignore this instruction.
+ */
+ if ( instr.state == INSTR_CACHE )
+ return IO_HANDLED;
+
ASSERT(dabt.valid);
vio->req = p;
--
2.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v11 1/3] xen/arm64: io: Emulate instructions (with invalid ISS) on MMIO region
2022-03-17 14:00 ` [PATCH v11 1/3] xen/arm64: io: Emulate instructions (with invalid ISS) on MMIO region Ayan Kumar Halder
@ 2022-03-17 22:24 ` Stefano Stabellini
2022-03-18 18:14 ` Julien Grall
1 sibling, 0 replies; 15+ messages in thread
From: Stefano Stabellini @ 2022-03-17 22:24 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 Thu, 17 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.
>
> Also, 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.
>
> Signed-off-by: Ayan Kumar Halder <ayankuma@xilinx.com>
I managed to reproduce (on QEMU emulating a cortex-a15, see recent patch
series for automation) the original crash. And I also tested that this
version of the patch doesn't have the same issue. Now it works, so:
Tested-by: Stefano Stabellini <sstabellini@kernel.org>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
>
> 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.
>
>
> v10 - 1. Set 'dabt.valid=1' when the instruction is fully decoded. This is
> checked in try_handle_mmio() and try_fwd_ioserv().
>
> 2. Various other style issues pointed in v9.
>
>
> v11 - 1. Renamed post_increment_register() to finalize_instr_emulation().
>
> 2. Moved "struct arch_vcpu_io { }" from xen/arch/x86/include/asm/ioreq.h to
> xen/arch/x86/include/asm/domain.h as this is included in sched.h. This fixes the
> build break for x86.
>
> 3. For arm32, check "( instr->state == INSTR_LDR_STR_POSTINDEXING )" before
> calling domain_crash(). This fixes the boot failure for arm32.
>
> 4. Restricted the commit header to 80 chars.
>
> xen/arch/arm/arm32/traps.c | 12 +++++
> xen/arch/arm/arm64/traps.c | 52 ++++++++++++++++++
> xen/arch/arm/decode.c | 2 +
> 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 | 8 ++-
> xen/arch/arm/traps.c | 77 ++++++++++++++++++++------
> xen/arch/x86/include/asm/domain.h | 3 ++
> xen/include/xen/sched.h | 2 +
> 11 files changed, 215 insertions(+), 54 deletions(-)
>
> diff --git a/xen/arch/arm/arm32/traps.c b/xen/arch/arm/arm32/traps.c
> index 9c9790a6d1..a4ce2b92d9 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,16 @@ void do_trap_data_abort(struct cpu_user_regs *regs)
> do_unexpected_trap("Data Abort", regs);
> }
>
> +void finalize_instr_emulation(const struct instr_details *instr)
> +{
> + /*
> + * We have not implemented decoding of post indexing instructions for 32 bit.
> + * Thus, this should be unreachable.
> + */
> + if ( instr->state == INSTR_LDR_STR_POSTINDEXING )
> + 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..3f8858acec 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,57 @@ void do_bad_mode(struct cpu_user_regs *regs, int reason)
> panic("bad mode\n");
> }
>
> +void finalize_instr_emulation(const struct instr_details *instr)
> +{
> + struct cpu_user_regs *regs = guest_cpu_user_regs();
> + register_t val = 0;
> + uint8_t psr_mode = (regs->cpsr & PSR_MODE_MASK);
> +
> + /* 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 )
> + {
> + switch ( psr_mode )
> + {
> + case PSR_MODE_EL1h:
> + val = regs->sp_el1;
> + break;
> + case PSR_MODE_EL1t:
> + case PSR_MODE_EL0t:
> + val = regs->sp_el0;
> + break;
> +
> + default:
> + 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..f5f6562600 100644
> --- a/xen/arch/arm/decode.c
> +++ b/xen/arch/arm/decode.c
> @@ -146,8 +146,10 @@ 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;
> + dabt->valid = 1;
>
> return 0;
>
> 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..ca259a79c2 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..08bc0b484c 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 finalize_instr_emulation(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..fd903b7b03 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.valid )
> {
> - 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..54167aebcb 100644
> --- a/xen/arch/arm/ioreq.c
> +++ b/xen/arch/arm/ioreq.c
> @@ -47,6 +47,8 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs,
> struct vcpu *v, mmio_info_t *info)
> {
> struct vcpu_io *vio = &v->io;
> + struct instr_details instr = info->dabt_instr;
> + struct hsr_dabt dabt = info->dabt;
> ioreq_t p = {
> .type = IOREQ_TYPE_COPY,
> .addr = info->gpa,
> @@ -76,10 +78,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 +97,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 +109,7 @@ bool arch_ioreq_complete_mmio(void)
>
> if ( handle_ioserv(regs, v) == IO_HANDLED )
> {
> + finalize_instr_emulation(&dabt_instr);
> advance_pc(regs, hsr);
> return true;
> }
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 7a1b679b8c..11f970d926 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.
> + */
> + finalize_instr_emulation(&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_instr.state == INSTR_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/domain.h b/xen/arch/x86/include/asm/domain.h
> index e62e109598..35898d725f 100644
> --- a/xen/arch/x86/include/asm/domain.h
> +++ b/xen/arch/x86/include/asm/domain.h
> @@ -756,6 +756,9 @@ static inline void pv_inject_sw_interrupt(unsigned int vector)
> : is_pv_32bit_domain(d) ? PV32_VM_ASSIST_MASK \
> : PV64_VM_ASSIST_MASK)
>
> +struct arch_vcpu_io {
> +};
> +
> #endif /* __ASM_DOMAIN_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 [flat|nested] 15+ messages in thread
* Re: [PATCH v11 2/3] xen/arm64: io: Handle the abort due to access to stage1 translation table
2022-03-17 14:00 ` [PATCH v11 2/3] xen/arm64: io: Handle the abort due to access to stage1 translation table Ayan Kumar Halder
@ 2022-03-17 22:27 ` Stefano Stabellini
2022-03-18 18:15 ` Julien Grall
1 sibling, 0 replies; 15+ messages in thread
From: Stefano Stabellini @ 2022-03-17 22:27 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 Thu, 17 Mar 2022, Ayan Kumar Halder wrote:
> If the abort was caused due to access to stage1 translation table, Xen
> will try to set the p2m entry (assuming that the Stage 1 translation
> table is in a non MMIO region).
> If there is no such entry found, then Xen will try to map the address as
> a MMIO region (assuming that the Stage 1 translation table is in a
> direct MMIO region).
>
> If that fails as well, then there are the two following scenarios:-
> 1. Stage 1 translation table being in an emulated MMIO region - Xen
> can read the region, but it has no way to return the value read to the
> CPU page table walker (which tries to go through the stage1 tables to
> resolve the translation fault).
>
> 2. Stage 1 translation table address is invalid.
>
> In both the above scenarios, Xen will forward the abort to the guest.
>
> Signed-off-by: Ayan Kumar Halder <ayankuma@xilinx.com>
Tested-by: Stefano Stabellini <sstabellini@kernel.org>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
>
> 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.
>
> v10 - 1. Enabled checking for stage1 translation table address in the
> MMIO region. The reason being Arm Arm does not have any restrictions.
> 2. Updated the commit message to explain all the possible scenarios.
>
> v11 - 1. Fixed some wordings in comments and commit message (pointed
> by Julien in v10).
>
> xen/arch/arm/io.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
> index fd903b7b03..6f458ee7fd 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 either in an
> + * emulated MMIO region or its address is invalid . 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
> --
> 2.17.1
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v11 3/3] xen/arm64: io: Handle data abort due to cache maintenance instructions
2022-03-17 14:00 ` [PATCH v11 3/3] xen/arm64: io: Handle data abort due to cache maintenance instructions Ayan Kumar Halder
@ 2022-03-17 22:33 ` Stefano Stabellini
2022-03-18 18:26 ` Julien Grall
1 sibling, 0 replies; 15+ messages in thread
From: Stefano Stabellini @ 2022-03-17 22:33 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 Thu, 17 Mar 2022, Ayan Kumar Halder wrote:
> When the data abort is caused due to cache maintenance for an address,
> there are three scenarios:-
>
> 1. Address belonging to a non emulated region - For this, Xen should
> set the corresponding bit in the translation table entry to valid and
> return to the guest to retry the instruction. This can happen sometimes
> as Xen need to set the translation table entry to invalid. (for eg
> 'Break-Before-Make' sequence). Xen returns to the guest to retry the
> instruction.
>
> 2. Address belongs to an emulated region - Xen should ignore the
> instruction (ie increment the PC) and return to the guest.
>
> 3. Address is invalid - Xen should forward the data abort to the guest.
>
> Signed-off-by: Ayan Kumar Halder <ayankuma@xilinx.com>
Tested-by: Stefano Stabellini <sstabellini@kernel.org>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v11 1/3] xen/arm64: io: Emulate instructions (with invalid ISS) on MMIO region
2022-03-17 14:00 ` [PATCH v11 1/3] xen/arm64: io: Emulate instructions (with invalid ISS) on MMIO region Ayan Kumar Halder
2022-03-17 22:24 ` Stefano Stabellini
@ 2022-03-18 18:14 ` Julien Grall
1 sibling, 0 replies; 15+ messages in thread
From: Julien Grall @ 2022-03-18 18:14 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 17/03/2022 14:00, 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.
>
> Also, 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.
>
> Signed-off-by: Ayan Kumar Halder <ayankuma@xilinx.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v11 2/3] xen/arm64: io: Handle the abort due to access to stage1 translation table
2022-03-17 14:00 ` [PATCH v11 2/3] xen/arm64: io: Handle the abort due to access to stage1 translation table Ayan Kumar Halder
2022-03-17 22:27 ` Stefano Stabellini
@ 2022-03-18 18:15 ` Julien Grall
1 sibling, 0 replies; 15+ messages in thread
From: Julien Grall @ 2022-03-18 18:15 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 17/03/2022 14:00, Ayan Kumar Halder wrote:
> If the abort was caused due to access to stage1 translation table, Xen
> will try to set the p2m entry (assuming that the Stage 1 translation
> table is in a non MMIO region).
> If there is no such entry found, then Xen will try to map the address as
> a MMIO region (assuming that the Stage 1 translation table is in a
> direct MMIO region).
>
> If that fails as well, then there are the two following scenarios:-
> 1. Stage 1 translation table being in an emulated MMIO region - Xen
> can read the region, but it has no way to return the value read to the
> CPU page table walker (which tries to go through the stage1 tables to
> resolve the translation fault).
>
> 2. Stage 1 translation table address is invalid.
>
> In both the above scenarios, Xen will forward the abort to the guest.
>
> Signed-off-by: Ayan Kumar Halder <ayankuma@xilinx.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v11 3/3] xen/arm64: io: Handle data abort due to cache maintenance instructions
2022-03-17 14:00 ` [PATCH v11 3/3] xen/arm64: io: Handle data abort due to cache maintenance instructions Ayan Kumar Halder
2022-03-17 22:33 ` Stefano Stabellini
@ 2022-03-18 18:26 ` Julien Grall
2022-03-22 12:06 ` Ayan Kumar Halder
1 sibling, 1 reply; 15+ messages in thread
From: Julien Grall @ 2022-03-18 18:26 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 17/03/2022 14:00, Ayan Kumar Halder wrote:
> diff --git a/xen/arch/arm/include/asm/mmio.h b/xen/arch/arm/include/asm/mmio.h
> index ca259a79c2..79e64d9af8 100644
> --- a/xen/arch/arm/include/asm/mmio.h
> +++ b/xen/arch/arm/include/asm/mmio.h
> @@ -35,6 +35,7 @@ enum instr_decode_state
> * instruction.
> */
> INSTR_LDR_STR_POSTINDEXING,
> + INSTR_CACHE, /* Cache Maintenance instr */
> };
>
> typedef struct
> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
> index 6f458ee7fd..26c716b4a5 100644
> --- a/xen/arch/arm/io.c
> +++ b/xen/arch/arm/io.c
> @@ -139,6 +139,17 @@ void try_decode_instruction(const struct cpu_user_regs *regs,
> return;
> }
>
> + /*
> + * When the data abort is caused due to cache maintenance, Xen should check
> + * if the address belongs to an emulated MMIO region or not. The behavior
> + * will differ accordingly.
> + */
> + if ( info->dabt.cache )
> + {
> + info->dabt_instr.state = INSTR_CACHE;
> + return;
> + }
> +
> /*
> * Armv8 processor does not provide a valid syndrome for decoding some
> * instructions. So in order to process these instructions, Xen must
> @@ -177,6 +188,13 @@ enum io_state try_handle_mmio(struct cpu_user_regs *regs,
> return rc;
> }
>
> + /*
> + * When the data abort is caused due to cache maintenance and the address
> + * belongs to an emulated region, Xen should ignore this instruction.
> + */
> + if ( info->dabt_instr.state == INSTR_CACHE )
Reading the Arm Arm, the ISS should be invalid for cache instructions.
So, I think the check at the beginning of try_handle_mmio() would
prevent us to reach this check.
Can you check that cache instructions on emulated region will
effectively be ignored?
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v11 3/3] xen/arm64: io: Handle data abort due to cache maintenance instructions
2022-03-18 18:26 ` Julien Grall
@ 2022-03-22 12:06 ` Ayan Kumar Halder
2022-03-22 12:38 ` Ayan Kumar Halder
0 siblings, 1 reply; 15+ messages in thread
From: Ayan Kumar Halder @ 2022-03-22 12:06 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 18/03/2022 18:26, Julien Grall wrote:
> Hi Ayan,
Hi Julien,
>
> On 17/03/2022 14:00, Ayan Kumar Halder wrote:
>> diff --git a/xen/arch/arm/include/asm/mmio.h
>> b/xen/arch/arm/include/asm/mmio.h
>> index ca259a79c2..79e64d9af8 100644
>> --- a/xen/arch/arm/include/asm/mmio.h
>> +++ b/xen/arch/arm/include/asm/mmio.h
>> @@ -35,6 +35,7 @@ enum instr_decode_state
>> * instruction.
>> */
>> INSTR_LDR_STR_POSTINDEXING,
>> + INSTR_CACHE, /* Cache Maintenance instr */
>> };
>> typedef struct
>> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
>> index 6f458ee7fd..26c716b4a5 100644
>> --- a/xen/arch/arm/io.c
>> +++ b/xen/arch/arm/io.c
>> @@ -139,6 +139,17 @@ void try_decode_instruction(const struct
>> cpu_user_regs *regs,
>> return;
>> }
>> + /*
>> + * When the data abort is caused due to cache maintenance, Xen
>> should check
>> + * if the address belongs to an emulated MMIO region or not. The
>> behavior
>> + * will differ accordingly.
>> + */
>> + if ( info->dabt.cache )
>> + {
>> + info->dabt_instr.state = INSTR_CACHE;
>> + return;
>> + }
>> +
>> /*
>> * Armv8 processor does not provide a valid syndrome for
>> decoding some
>> * instructions. So in order to process these instructions, Xen
>> must
>> @@ -177,6 +188,13 @@ enum io_state try_handle_mmio(struct
>> cpu_user_regs *regs,
>> return rc;
>> }
>> + /*
>> + * When the data abort is caused due to cache maintenance and
>> the address
>> + * belongs to an emulated region, Xen should ignore this
>> instruction.
>> + */
>> + if ( info->dabt_instr.state == INSTR_CACHE )
>
> Reading the Arm Arm, the ISS should be invalid for cache instructions.
> So, I think the check at the beginning of try_handle_mmio() would
> prevent us to reach this check.
>
> Can you check that cache instructions on emulated region will
> effectively be ignored?
Yes, you are correct.
I tested with the following (dis)assembly snippet :-
0x3001000 is the base address of GIC Distributor base.
__asm__ __volatile__("ldr x1, =0x3001000");
40000ca8: 58000301 ldr x1, 40000d08 <main+0x70>
__asm __volatile__("DC CVAU, x1");
40000cac: d50b7b21 dc cvau, x1
This resulting in hitting the assertion :-
(XEN) Assertion 'unreachable' failed at arch/arm/io.c:178
I dumped the registers as follows, to determine that the fault is caused
by the instruction at 40000cac.
HSR=0x00000092000147 regs->pc = 0x40000cac info.gpa = 0x3001000
So, my patch needs to be modified as follows:-
@@ -172,7 +173,7 @@ enum io_state try_handle_mmio(struct cpu_user_regs
*regs,
ASSERT(info->dabt.ec == HSR_EC_DATA_ABORT_LOWER_EL);
- if ( !info->dabt.valid )
+ if ( !(info->dabt.valid || (info->dabt_instr.state == INSTR_CACHE)) )
{
ASSERT_UNREACHABLE();
return IO_ABORT;
I will send a v12 patch with this change.
- Ayan
>
> Cheers,
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v11 3/3] xen/arm64: io: Handle data abort due to cache maintenance instructions
2022-03-22 12:06 ` Ayan Kumar Halder
@ 2022-03-22 12:38 ` Ayan Kumar Halder
2022-03-22 13:22 ` Julien Grall
0 siblings, 1 reply; 15+ messages in thread
From: Ayan Kumar Halder @ 2022-03-22 12:38 UTC (permalink / raw)
To: Ayan Kumar Halder, Julien Grall, xen-devel
Cc: sstabellini, stefanos, Volodymyr_Babchuk, bertrand.marquis,
andrew.cooper3, george.dunlap, jbeulich, wl, paul, roger.pau
On 22/03/2022 12:06, Ayan Kumar Halder wrote:
>
> On 18/03/2022 18:26, Julien Grall wrote:
>> Hi Ayan,
> Hi Julien,
>>
>> On 17/03/2022 14:00, Ayan Kumar Halder wrote:
>>> diff --git a/xen/arch/arm/include/asm/mmio.h
>>> b/xen/arch/arm/include/asm/mmio.h
>>> index ca259a79c2..79e64d9af8 100644
>>> --- a/xen/arch/arm/include/asm/mmio.h
>>> +++ b/xen/arch/arm/include/asm/mmio.h
>>> @@ -35,6 +35,7 @@ enum instr_decode_state
>>> * instruction.
>>> */
>>> INSTR_LDR_STR_POSTINDEXING,
>>> + INSTR_CACHE, /* Cache Maintenance instr */
>>> };
>>> typedef struct
>>> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
>>> index 6f458ee7fd..26c716b4a5 100644
>>> --- a/xen/arch/arm/io.c
>>> +++ b/xen/arch/arm/io.c
>>> @@ -139,6 +139,17 @@ void try_decode_instruction(const struct
>>> cpu_user_regs *regs,
>>> return;
>>> }
>>> + /*
>>> + * When the data abort is caused due to cache maintenance, Xen
>>> should check
>>> + * if the address belongs to an emulated MMIO region or not.
>>> The behavior
>>> + * will differ accordingly.
>>> + */
>>> + if ( info->dabt.cache )
>>> + {
>>> + info->dabt_instr.state = INSTR_CACHE;
>>> + return;
>>> + }
>>> +
>>> /*
>>> * Armv8 processor does not provide a valid syndrome for
>>> decoding some
>>> * instructions. So in order to process these instructions,
>>> Xen must
>>> @@ -177,6 +188,13 @@ enum io_state try_handle_mmio(struct
>>> cpu_user_regs *regs,
>>> return rc;
>>> }
>>> + /*
>>> + * When the data abort is caused due to cache maintenance and
>>> the address
>>> + * belongs to an emulated region, Xen should ignore this
>>> instruction.
>>> + */
>>> + if ( info->dabt_instr.state == INSTR_CACHE )
>>
>> Reading the Arm Arm, the ISS should be invalid for cache
>> instructions. So, I think the check at the beginning of
>> try_handle_mmio() would prevent us to reach this check.
>>
>> Can you check that cache instructions on emulated region will
>> effectively be ignored?
>
> Yes, you are correct.
>
> I tested with the following (dis)assembly snippet :-
>
> 0x3001000 is the base address of GIC Distributor base.
>
> __asm__ __volatile__("ldr x1, =0x3001000");
> 40000ca8: 58000301 ldr x1, 40000d08 <main+0x70>
> __asm __volatile__("DC CVAU, x1");
> 40000cac: d50b7b21 dc cvau, x1
>
> This resulting in hitting the assertion :-
>
> (XEN) Assertion 'unreachable' failed at arch/arm/io.c:178
>
> I dumped the registers as follows, to determine that the fault is
> caused by the instruction at 40000cac.
>
> HSR=0x00000092000147 regs->pc = 0x40000cac info.gpa = 0x3001000
>
>
> So, my patch needs to be modified as follows:-
>
> @@ -172,7 +173,7 @@ enum io_state try_handle_mmio(struct cpu_user_regs
> *regs,
>
> ASSERT(info->dabt.ec == HSR_EC_DATA_ABORT_LOWER_EL);
>
> - if ( !info->dabt.valid )
> + if ( !(info->dabt.valid || (info->dabt_instr.state ==
> INSTR_CACHE)) )
Actually this is not needed.
The following change is sufficient :-
@@ -146,7 +146,9 @@ void try_decode_instruction(const struct
cpu_user_regs *regs,
*/
if ( info->dabt.cache )
{
info->dabt_instr.state = INSTR_CACHE;
+ info->dabt.valid = 1;
return;
}
"info->dabt.valid == 1" means the instruction is valid or decoded
successfully (this holds true for INSTR_CACHE as well).
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v11 3/3] xen/arm64: io: Handle data abort due to cache maintenance instructions
2022-03-22 12:38 ` Ayan Kumar Halder
@ 2022-03-22 13:22 ` Julien Grall
2022-03-22 16:16 ` Ayan Kumar Halder
0 siblings, 1 reply; 15+ messages in thread
From: Julien Grall @ 2022-03-22 13:22 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 Ayan,
On 22/03/2022 12:38, Ayan Kumar Halder wrote:
>
> On 22/03/2022 12:06, Ayan Kumar Halder wrote:
>>
>> On 18/03/2022 18:26, Julien Grall wrote:
>>> Hi Ayan,
>> Hi Julien,
>>>
>>> On 17/03/2022 14:00, Ayan Kumar Halder wrote:
>>>> diff --git a/xen/arch/arm/include/asm/mmio.h
>>>> b/xen/arch/arm/include/asm/mmio.h
>>>> index ca259a79c2..79e64d9af8 100644
>>>> --- a/xen/arch/arm/include/asm/mmio.h
>>>> +++ b/xen/arch/arm/include/asm/mmio.h
>>>> @@ -35,6 +35,7 @@ enum instr_decode_state
>>>> * instruction.
>>>> */
>>>> INSTR_LDR_STR_POSTINDEXING,
>>>> + INSTR_CACHE, /* Cache Maintenance instr */
>>>> };
>>>> typedef struct
>>>> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
>>>> index 6f458ee7fd..26c716b4a5 100644
>>>> --- a/xen/arch/arm/io.c
>>>> +++ b/xen/arch/arm/io.c
>>>> @@ -139,6 +139,17 @@ void try_decode_instruction(const struct
>>>> cpu_user_regs *regs,
>>>> return;
>>>> }
>>>> + /*
>>>> + * When the data abort is caused due to cache maintenance, Xen
>>>> should check
>>>> + * if the address belongs to an emulated MMIO region or not.
>>>> The behavior
>>>> + * will differ accordingly.
>>>> + */
>>>> + if ( info->dabt.cache )
>>>> + {
>>>> + info->dabt_instr.state = INSTR_CACHE;
>>>> + return;
>>>> + }
>>>> +
>>>> /*
>>>> * Armv8 processor does not provide a valid syndrome for
>>>> decoding some
>>>> * instructions. So in order to process these instructions,
>>>> Xen must
>>>> @@ -177,6 +188,13 @@ enum io_state try_handle_mmio(struct
>>>> cpu_user_regs *regs,
>>>> return rc;
>>>> }
>>>> + /*
>>>> + * When the data abort is caused due to cache maintenance and
>>>> the address
>>>> + * belongs to an emulated region, Xen should ignore this
>>>> instruction.
>>>> + */
>>>> + if ( info->dabt_instr.state == INSTR_CACHE )
>>>
>>> Reading the Arm Arm, the ISS should be invalid for cache
>>> instructions. So, I think the check at the beginning of
>>> try_handle_mmio() would prevent us to reach this check.
>>>
>>> Can you check that cache instructions on emulated region will
>>> effectively be ignored?
>>
>> Yes, you are correct.
>>
>> I tested with the following (dis)assembly snippet :-
>>
>> 0x3001000 is the base address of GIC Distributor base.
>>
>> __asm__ __volatile__("ldr x1, =0x3001000");
>> 40000ca8: 58000301 ldr x1, 40000d08 <main+0x70>
>> __asm __volatile__("DC CVAU, x1");
>> 40000cac: d50b7b21 dc cvau, x1
>>
>> This resulting in hitting the assertion :-
>>
>> (XEN) Assertion 'unreachable' failed at arch/arm/io.c:178
>>
>> I dumped the registers as follows, to determine that the fault is
>> caused by the instruction at 40000cac.
>>
>> HSR=0x00000092000147 regs->pc = 0x40000cac info.gpa = 0x3001000
>>
>>
>> So, my patch needs to be modified as follows:-
>>
>> @@ -172,7 +173,7 @@ enum io_state try_handle_mmio(struct cpu_user_regs
>> *regs,
>>
>> ASSERT(info->dabt.ec == HSR_EC_DATA_ABORT_LOWER_EL);
>>
>> - if ( !info->dabt.valid )
>> + if ( !(info->dabt.valid || (info->dabt_instr.state ==
>> INSTR_CACHE)) )
>
> Actually this is not needed.
>
> The following change is sufficient :-
>
> @@ -146,7 +146,9 @@ void try_decode_instruction(const struct
> cpu_user_regs *regs,
> */
> if ( info->dabt.cache )
> {
> info->dabt_instr.state = INSTR_CACHE;
> + info->dabt.valid = 1;
To me, 'info->dabt.valid' indicates whether the syndrome is valid. We
set to 1 for emulated instruction because the syndrome will be updated.
But this is not the case for the cache instructions. So I would prefer
if it is kept as 0 and use your previous suggestion.
Furthermore, I think try_fwd_ioserv() need to be adapted because the
function will use the fields SAS and SRT. From the Arm Arm they are
RES0, so while they are 0 today, we should not rely on this.
Therefore, to be fully compliant with the Arm, we want to reorder a bit
the code:
* The field data could be set past ioreq_select_server().
* The field size should be set to the cache line size.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v11 3/3] xen/arm64: io: Handle data abort due to cache maintenance instructions
2022-03-22 13:22 ` Julien Grall
@ 2022-03-22 16:16 ` Ayan Kumar Halder
2022-03-22 16:24 ` Julien Grall
0 siblings, 1 reply; 15+ messages in thread
From: Ayan Kumar Halder @ 2022-03-22 16:16 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 22/03/2022 13:22, Julien Grall wrote:
> Hi Ayan,
>
> On 22/03/2022 12:38, Ayan Kumar Halder wrote:
>>
>> On 22/03/2022 12:06, Ayan Kumar Halder wrote:
>>>
>>> On 18/03/2022 18:26, Julien Grall wrote:
>>>> Hi Ayan,
>>> Hi Julien,
>>>>
>>>> On 17/03/2022 14:00, Ayan Kumar Halder wrote:
>>>>> diff --git a/xen/arch/arm/include/asm/mmio.h
>>>>> b/xen/arch/arm/include/asm/mmio.h
>>>>> index ca259a79c2..79e64d9af8 100644
>>>>> --- a/xen/arch/arm/include/asm/mmio.h
>>>>> +++ b/xen/arch/arm/include/asm/mmio.h
>>>>> @@ -35,6 +35,7 @@ enum instr_decode_state
>>>>> * instruction.
>>>>> */
>>>>> INSTR_LDR_STR_POSTINDEXING,
>>>>> + INSTR_CACHE, /* Cache Maintenance instr */
>>>>> };
>>>>> typedef struct
>>>>> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
>>>>> index 6f458ee7fd..26c716b4a5 100644
>>>>> --- a/xen/arch/arm/io.c
>>>>> +++ b/xen/arch/arm/io.c
>>>>> @@ -139,6 +139,17 @@ void try_decode_instruction(const struct
>>>>> cpu_user_regs *regs,
>>>>> return;
>>>>> }
>>>>> + /*
>>>>> + * When the data abort is caused due to cache maintenance,
>>>>> Xen should check
>>>>> + * if the address belongs to an emulated MMIO region or not.
>>>>> The behavior
>>>>> + * will differ accordingly.
>>>>> + */
>>>>> + if ( info->dabt.cache )
>>>>> + {
>>>>> + info->dabt_instr.state = INSTR_CACHE;
>>>>> + return;
>>>>> + }
>>>>> +
>>>>> /*
>>>>> * Armv8 processor does not provide a valid syndrome for
>>>>> decoding some
>>>>> * instructions. So in order to process these instructions,
>>>>> Xen must
>>>>> @@ -177,6 +188,13 @@ enum io_state try_handle_mmio(struct
>>>>> cpu_user_regs *regs,
>>>>> return rc;
>>>>> }
>>>>> + /*
>>>>> + * When the data abort is caused due to cache maintenance and
>>>>> the address
>>>>> + * belongs to an emulated region, Xen should ignore this
>>>>> instruction.
>>>>> + */
>>>>> + if ( info->dabt_instr.state == INSTR_CACHE )
>>>>
>>>> Reading the Arm Arm, the ISS should be invalid for cache
>>>> instructions. So, I think the check at the beginning of
>>>> try_handle_mmio() would prevent us to reach this check.
>>>>
>>>> Can you check that cache instructions on emulated region will
>>>> effectively be ignored?
>>>
>>> Yes, you are correct.
>>>
>>> I tested with the following (dis)assembly snippet :-
>>>
>>> 0x3001000 is the base address of GIC Distributor base.
>>>
>>> __asm__ __volatile__("ldr x1, =0x3001000");
>>> 40000ca8: 58000301 ldr x1, 40000d08 <main+0x70>
>>> __asm __volatile__("DC CVAU, x1");
>>> 40000cac: d50b7b21 dc cvau, x1
>>>
>>> This resulting in hitting the assertion :-
>>>
>>> (XEN) Assertion 'unreachable' failed at arch/arm/io.c:178
>>>
>>> I dumped the registers as follows, to determine that the fault is
>>> caused by the instruction at 40000cac.
>>>
>>> HSR=0x00000092000147 regs->pc = 0x40000cac info.gpa = 0x3001000
>>>
>>>
>>> So, my patch needs to be modified as follows:-
>>>
>>> @@ -172,7 +173,7 @@ enum io_state try_handle_mmio(struct
>>> cpu_user_regs *regs,
>>>
>>> ASSERT(info->dabt.ec == HSR_EC_DATA_ABORT_LOWER_EL);
>>>
>>> - if ( !info->dabt.valid )
>>> + if ( !(info->dabt.valid || (info->dabt_instr.state ==
>>> INSTR_CACHE)) )
>>
>> Actually this is not needed.
>>
>> The following change is sufficient :-
>>
>> @@ -146,7 +146,9 @@ void try_decode_instruction(const struct
>> cpu_user_regs *regs,
>> */
>> if ( info->dabt.cache )
>> {
>> info->dabt_instr.state = INSTR_CACHE;
>> + info->dabt.valid = 1;
>
> To me, 'info->dabt.valid' indicates whether the syndrome is valid. We
> set to 1 for emulated instruction because the syndrome will be updated.
>
> But this is not the case for the cache instructions. So I would prefer
> if it is kept as 0 and use your previous suggestion.
>
> Furthermore, I think try_fwd_ioserv() need to be adapted because the
> function will use the fields SAS and SRT. From the Arm Arm they are
> RES0, so while they are 0 today, we should not rely on this.
>
> Therefore, to be fully compliant with the Arm, we want to reorder a
> bit the code:
>
> * The field data could be set past ioreq_select_server().
> * The field size should be set to the cache line size.
I am assuming that we need to invoke dcache_line_size() (from
xen/arch/arm/arm64/cache.S ) to get the cache line size.
I think the cache line may be 32 or 64 bytes. In which case, this cannot
be represented by SAS (as it can represent 1, 2, 4 and 8 bytes).
Also, we are invoking ioreq_select_server() to determine if the address
is emulated or not. So, can we use an assumed size (= 1 byte) ?
If it is emulated, Xen will ignore the instruction. If it is not
emulated, Xen will forward the abort to the guest.
Thus, Xen will never execute the instruction. So the correctness of the
size should not matter here.
- Ayan
>
> Cheers,
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v11 3/3] xen/arm64: io: Handle data abort due to cache maintenance instructions
2022-03-22 16:16 ` Ayan Kumar Halder
@ 2022-03-22 16:24 ` Julien Grall
0 siblings, 0 replies; 15+ messages in thread
From: Julien Grall @ 2022-03-22 16:24 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 22/03/2022 16:16, Ayan Kumar Halder wrote:
> On 22/03/2022 13:22, Julien Grall wrote:
>> Furthermore, I think try_fwd_ioserv() need to be adapted because the
>> function will use the fields SAS and SRT. From the Arm Arm they are
>> RES0, so while they are 0 today, we should not rely on this.
>>
>> Therefore, to be fully compliant with the Arm, we want to reorder a
>> bit the code:
>>
>> * The field data could be set past ioreq_select_server().
>> * The field size should be set to the cache line size.
>
> I am assuming that we need to invoke dcache_line_size() (from
> xen/arch/arm/arm64/cache.S ) to get the cache line size.
You would want to use dcache_line_bytes.
>
> I think the cache line may be 32 or 64 bytes. In which case, this cannot
> be represented by SAS (as it can represent 1, 2, 4 and 8 bytes).
You are correct that this cannot be represented by SAS. However, I was
referring to the field 'size' in the ioreq structure. It is a 32-bit
integer and could therefore represent the size of the cache line.
>
> Also, we are invoking ioreq_select_server() to determine if the address
> is emulated or not. So, can we use an assumed size (= 1 byte) ?
I thought about this. This is technically incorrect but would be OK if
we cannot find the correct size.
Per above, I think the correct size could be found.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2022-03-22 16:25 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-17 14:00 [PATCH v11 0/3] xen/arm64: io: Decode ldr/str post-indexing instruction Ayan Kumar Halder
2022-03-17 14:00 ` [PATCH v11 1/3] xen/arm64: io: Emulate instructions (with invalid ISS) on MMIO region Ayan Kumar Halder
2022-03-17 22:24 ` Stefano Stabellini
2022-03-18 18:14 ` Julien Grall
2022-03-17 14:00 ` [PATCH v11 2/3] xen/arm64: io: Handle the abort due to access to stage1 translation table Ayan Kumar Halder
2022-03-17 22:27 ` Stefano Stabellini
2022-03-18 18:15 ` Julien Grall
2022-03-17 14:00 ` [PATCH v11 3/3] xen/arm64: io: Handle data abort due to cache maintenance instructions Ayan Kumar Halder
2022-03-17 22:33 ` Stefano Stabellini
2022-03-18 18:26 ` Julien Grall
2022-03-22 12:06 ` Ayan Kumar Halder
2022-03-22 12:38 ` Ayan Kumar Halder
2022-03-22 13:22 ` Julien Grall
2022-03-22 16:16 ` Ayan Kumar Halder
2022-03-22 16:24 ` 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.