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,
