On 26/01/2022 12:21, Ayan Kumar Halder wrote:
Hi Julien/Stefano,
Hi,
On 25/01/2022 23:02, Julien Grall wrote:
+ }
+
+ /*
+ * Handle when rn = SP
+ * Refer ArmV8 ARM DDI 0487G.b, Page - D1-2463 "Stack pointer
register selection"
+ * As we are interested in handling exceptions only from EL1 in
AArch64 state,
+ * thus M[3:0] == EL1h (Page - C5-480 "When exception taken from
AArch64 state:")
I read the last sentence as "We only support decoding from instruction
run at EL1". But I can't find a check guarantee that.
Sorry, the check below does that but is used only for rn == 31. I should
move ((regs->cpsr & PSR_MODE_MASK) != PSR_MODE_EL1h) ) into a separate
check of its own.
The question is why do we want to limit instruction decoding to EL1?
+ */
+ if ( (instr->code.rn == 31) && ((regs->cpsr & PSR_MODE_MASK) !=
PSR_MODE_EL1h) )
+ {
+ gprintk(XENLOG_ERR, "SP is valid only for EL1h\n");
+ goto bad_loadstore;
+ }
+
+ if ( instr->code.v != 0 )
+ {
+ gprintk(XENLOG_ERR,
+ "ldr/str post indexing for vector types are not
supported\n");
+ goto bad_loadstore;
+ }
+
+ /* Check for STR (immediate) */
+ if ( instr->code.opc == 0 )
+ {
+ dabt->write = 1;
+ }
Coding style: We don't use {} for single line. In this case, it would
also result to have a more readable code.
+ /* Check for LDR (immediate) */
+ else if ( instr->code.opc == 1 )
+ {
+ dabt->write = 0;
+ }
Same.
+ else
+ {
+ gprintk(XENLOG_ERR,
+ "Decoding ldr/str post indexing is not supported for
this variant\n");
The indentation looks wrong here.
+ goto bad_loadstore;
+ }
+
+ gprintk(XENLOG_INFO,
+ "instr->code.rt = 0x%x, instr->code.size = 0x%x,
instr->code.imm9 = %d\n",
+ instr->code.rt, instr->code.size, instr->code.imm9);
The indentation looks wrong here.
+
+ update_dabt(dabt, instr->code.rt, instr->code.size, false);
+ dabt->valid = 1;
+
+ return 0;
+
+ bad_loadstore:
+ gprintk(XENLOG_ERR, "unhandled Arm instruction 0x%x\n",
instr->value);
+ return 1;
+}
+
static int decode_thumb(register_t pc, struct hsr_dabt *dabt)
{
uint16_t instr;
@@ -150,17 +245,44 @@ 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, struct
hsr_dabt *dabt,
+ union ldr_str_instr_class *instr)
I would like to avoid make the assumption that the instr we decode
will always be a store/load. So please rename it to something more
generic.
A difference of thought. Should we not name it as per the current usage
? This will avoid any ambiguity.
What ambiguity? The caller should be completely agnostic to what
instruction we decode.
Later, if this gets extended, then it
can be named more appropriately depending on the usage.
Renaming afterwards is exactly what I want to avoid.
Also, the bit-pattern in "union ldr_str_instr_class" is very much
specific to ldr/str.
I agree that the bit-pattern is specific to load/store. But that's just
one way to interpret the 32-bit value. It would be easy to add another
way to interpret it. I.e:
union
{
value;
struct {
} str_ldr;
struct {
} brk;
}
I think moving if ( !dabt.valid ) earlier should be part of a
pre-patch. This would allows us to backport it as we don't want to
forward the I/O to an IOREQ server if ISV=0.
I would say that in the pre-patch, we should move "if ( !dabt.valid )"
before "find_mmio_handler()". The reason being if the intruction was not
decoded successfully (ie ISV=0), then there is no need to find the mmio
handler corresponding to the gpa. Please let me know your thoughts and I
can send the pre-patch.
Sounds good to me.
/*
* Erratum 766422: Thumb store translation fault to Hypervisor may
* not have correct HSR Rt value.
@@ -134,7 +145,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.dabt, NULL);
Could we combine the two decode_instruction()?
Do you mean something like this :-
Yes. But...
if ( (!info.dabt.valid ) || (( check_workaround_766422() && (regs->cpsr
& PSR_THUMB) &&
dabt.write) )
{
rc = decode_instruction(regs, &info.dabt, &instr); // We know
that for PSR_THUMB, instr is ignored.
... without this comment. Also see my reply to Stefano's email.
[...]
So you mean something like this (in traps.c):-
#if CONFIG_ARM_64
void post_increment_register(union ldr_str_instr_class *instr)
{
// handle the post increment
}
#else
void post_increment_register(union ldr_str_instr_class *instr)
{
ASSERT_UNREACHABLE();
}
#endif
If so, I am fine with this.
Yes.
Cheers,
--
Julien Grall