On 22/11/2021 14:19, Ayan Kumar Halder wrote:
Hi Julien/Stefano/Wei/Jan,
Hi,
As some of the comments were inter-related, I have consolidated my
response to all the comments below.
On 19/11/2021 17:26, Julien Grall wrote:
Hi Ayan,
On 19/11/2021 16:52, Ayan Kumar Halder wrote:
At present, post indexing instructions are not emulated by Xen.
Can you explain in the commit message why this is a problem?
Yes, I will update the message as below :-
Armv8 hardware does not provide the correct syndrome for decoding post
indexing ldr/str instructions.
This statement implies that the issue happens on both AArch32 and
AArch64 state. I am OK if we only handle the latter for now. But I would
clarify it in the commit message.
Thus any post indexing ldr/str
instruction at EL1 results in a data abort with ISV=0.
Instruction from EL0 may also trap in Xen. So I would prefer if you just
say "instruction executed by a domain results ...".
As a result, Xen
needs to decode the instruction.
Can you give some information on the domain used. Something like:
"this was discovered with XXX which let the compiler deciding which
instruction to use."
Thus, DomU will be able to read/write to ioreg space with post indexing
I would say "domain" rather "domU" because all the domains are affected.
instructions for 32 bit.
How about the following commit message:
"
xen/arm64: io: Decode 32-bit ldr/str post-indexing instructions
At the moment, Xen is only handling data abort with valid syndrome (i.e.
ISV=0). Unfortunately, this doesn't cover all the instructions a domain
could use to access MMIO regions.
For instance, Xilinx baremetal OS will use:
volatile u32 *LocalAddr = (volatile u32 *)Addr;
*LocalAddr = Value;
This leave the compiler to decide which store instructions to use. This
may be a post-index store instruction where the HW will not provide a
valid syndrome.
In order to handle post-indexing store/load instructions,
Xen will need to fetch and decode the instruction.
This patch only cover post-index store/load instructions from AArch64
mode. For now, this is left unimplemented for trap from AArch32 mode.
"
+{
+ int32_t ret;
+
+ if ( !(start >= 0 && length > 0 && length <= 32 - start) )
+ return -EINVAL;
+
+ ret = (value >> start) & (~0U >> (32 - length));
This would be easier to read if you use GENMASK().
I see that GENMASK returns a register mask. In my scenario, I wish to
compare the offsets 10, 21, 24 and 27 to some fixed value.
So, instead of using GENMASK four times, I can try the following
instr & MASK_for_10_21_24_27 == VALID_for_10_21_24_27 (Wei Chen's
suggestion)
That would be OK with me. Alternatively, you could use the union
approach suggested by Bertrand.
Also for size, v and opc, I can defined another bitmask to compare with
VALID_for_32bit_LDR | VALID_for_32bit_STR.
Wei Chen, You had suggested using vreg_regx_extract(). However, I see
that it is used to extract the complete u64 register value. In this
case, I wish to compare certain offsets within a 32 bit register to some
expected values. So, vreg_regx_extract() might not be appropriate and we
can use the method mentioned before.
vreg_reg*_extract() are meant to work on a register. So I think they are
not appropriate here as you don't deal with registers.
[...]
+
+ /* At the moment, we support STR(immediate) - 32 bit variant and
+ * LDR(immediate) - 32 bit variant only.
+ */
Coding style.
Ack
+ if (!((size==2) && (v==0) && ((opc==0) || (opc==1))))
The coding style for this should be:
if ( !(( size == 2 ) && ( ... ) ... ) )
Ack
+ goto bad_64bit_loadstore;
+
+ rt = extract32(instr, 0, 5);
+ imm9 = extract32(instr, 12, 9);
+
+ if ( imm9 < 0 )
+ update_dabt(dabt, rt, size, true);
+ else
+ update_dabt(dabt, rt, size, false);
This could be simplified with:
update_dabt(dabt, rt, size, imm9 < 0);
Ack
+
+ dabt->valid = 1;
+
+
+ return 0;
+bad_64bit_loadstore:
+ gprintk(XENLOG_ERR, "unhandled 64bit instruction 0x%x\n", instr);
+ return 1;
+}
+
static int decode_thumb(register_t pc, struct hsr_dabt *dabt)
{
uint16_t instr;
@@ -155,6 +229,9 @@ int decode_instruction(const struct cpu_user_regs
*regs, struct hsr_dabt *dabt)
if ( is_32bit_domain(current->domain) && regs->cpsr & PSR_THUMB )
return decode_thumb(regs->pc, dabt);
+ if ( is_64bit_domain(current->domain) )
You can still run 32-bit code in 64-bit domain. So I think you want to
check the SPSR mode.
Do you mean the following check :-
if ( (is_64bit_domain(current->domain) && (!(regs->spsr & PSR_MODE_BIT)) )
This should work, but I think you can simplify to use:
!psr_mode_is_32bit()
+ */
+ rc = decode_instruction(regs, &info.dabt);
I actually expect this to also be useful when forwarding the I/O to
device-model. So I would move the decoding earlier in the code and the
check of dabt.valid earlier.
You mean I will move decode_instruction() before find_mmio_handler() ?
Yes.
Cheers,
--
Julien Grall
