Hi Julien/Stefano/Wei/Jan,
Many thanks for your review.
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. Thus any post indexing ldr/str
instruction at EL1 results in a data abort with ISV=0. As a result, Xen
needs to decode the instruction.
Thus, DomU will be able to read/write to ioreg space with post indexing
instructions for 32 bit.
When Xen gets the exception, EL2_ESR.ISV bit not set. Thus as a
result, data abort is triggered.
Added the logic to decode ldr/str post indexing instructions.
With this, Xen can decode instructions like these:-
ldr w2, [x1], #4
Thus, domU can read ioreg with post indexing instructions.
Hmmm.... Don't you also need to update the register x1 after the
instruction was emulated?
Yes, this is a mistake. X1 needs to be incremented by 4 after W2 is written.
Signed-off-by: Ayan Kumar Halder <ayank...@xilinx.com>
---
Note to reviewer:-
This patch is based on an issue discussed in
https://lists.xenproject.org/archives/html/xen-devel/2021-11/msg00969.html
"Xen/ARM - Query about a data abort seen while reading GICD registers"
xen/arch/arm/decode.c | 77 +++++++++++++++++++++++++++++++++++++++++++
xen/arch/arm/io.c | 14 ++++++--
2 files changed, 88 insertions(+), 3 deletions(-)
diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c
index 792c2e92a7..7b60bedbc5 100644
--- a/xen/arch/arm/decode.c
+++ b/xen/arch/arm/decode.c
@@ -84,6 +84,80 @@ bad_thumb2:
return 1;
}
+static inline int32_t extract32(uint32_t value, int start, int length)
Any reason to have start and length signed?
Again a mistake. There is no reason to use signed types here or in the
other places.
As Jan Beulich has pointed out, I should be using unsigned int as per
the CODING_STYLE.
+{
+ 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)
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.
+
+ return ret;
+}
+
+static int decode_64bit_loadstore_postindexing(register_t pc, struct
hsr_dabt *dabt)
+{
+ uint32_t instr;
+ int size;
+ int v;
+ int opc;
+ int rt;
+ int imm9;
Should all those variables need to be signed?
A mistake. I will change them to unsigned int.
+
+ /* For details on decoding, refer to Armv8 Architecture reference
manual
+ * Section - "Load/store register (immediate post-indexed)", Pg 318
The page number varies between revision of the Armv8 spec. So can you
specify which version you used?
Ack. I will mention the version.
+ */
The coding style for comment in Xen is:
/*
* Foo
* Bar
*/
Ack
+ if ( raw_copy_from_guest(&instr, (void * __user)pc, sizeof
(instr)) )
+ return -EFAULT;
+
+ /* First, let's check for the fixed values */
+
+ /* As per the "Encoding table for the Loads and Stores group",
Pg 299
Same question here about the version.
Ack, I will mention the version.
+ * op4 = 1 - Load/store register (immediate post-indexed)
+ */
Coding style.
Ack
+ if ( extract32(instr, 10, 2) != 1 )
+ goto bad_64bit_loadstore;
+
+ /* For the following, refer to "Load/store register (immediate
post-indexed)"
+ * to get the fixed values at various bit positions.
+ */
+ if ( extract32(instr, 21, 1) != 0 )
+ goto bad_64bit_loadstore;
+
+ if ( extract32(instr, 24, 2) != 0 )
+ goto bad_64bit_loadstore;
+
+ if ( extract32(instr, 27, 3) != 7 )
+ goto bad_64bit_loadstore;
+
+ size = extract32(instr, 30, 2);
+ v = extract32(instr, 26, 1);
+ opc = extract32(instr, 22, 1);
Stefano:- Thanks for catching my mistake. opc is 2 bits (bits 22, 23). I
will fix this.
+
+ /* 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)) )
+ return decode_64bit_loadstore_postindexing(regs->pc, dabt);
+
/* TODO: Handle ARM instruction */
gprintk(XENLOG_ERR, "unhandled ARM instruction\n");
I think this comment should now be updated to "unhandled 32-bit ...".
Ack
diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
index 729287e37c..49e80358c0 100644
--- a/xen/arch/arm/io.c
+++ b/xen/arch/arm/io.c
@@ -106,14 +106,13 @@ enum io_state try_handle_mmio(struct
cpu_user_regs *regs,
.gpa = gpa,
.dabt = dabt
};
+ int rc;
ASSERT(hsr.ec == HSR_EC_DATA_ABORT_LOWER_EL);
handler = find_mmio_handler(v->domain, info.gpa);
if ( !handler )
{
- int rc;
-
rc = try_fwd_ioserv(regs, v, &info);
if ( rc == IO_HANDLED )
return handle_ioserv(regs, v);
@@ -123,7 +122,16 @@ enum io_state try_handle_mmio(struct
cpu_user_regs *regs,
/* All the instructions used on emulated MMIO region should be
valid */
if ( !dabt.valid )
- return IO_ABORT;
+ {
+ /*
+ * Post indexing ldr/str instructions are not emulated by
Xen. So, the
+ * ISS is invalid. In such a scenario, we try to manually
decode the
+ * instruction from the program counter.
I am afraid this is wrong. The problem here is the processor didn't
provide a valid syndrom for post-indexing ldr/str instructions. So in
order to support them, Xen must decode.
Ack
+ */
+ 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() ?
Stefano > It doesn't look like we are setting dabt->write anywhere.
Ayan > Yes, this is a miss. Depending on the opc, this should be
upadeted accordingly in decode_64bit_loadstore_postindexing().
Stefano > Also, is info.gpa in try_handle_mmio already updated in the
pre-index
case? If not, do we need to apply the offset manually here?
Ayan > Sorry, I did not understand you. This patch is only related to
the post indexing ldr/str issue. Why do we need to check for pre-indexing ?
Stefano > In the post-index case, we need to update the base address in
the Rn
register?
Ayan > Yes this is a mistake, As Julien pointed out before, the register
x1 ie Rn needs to the incremented.
Jan > In addition to Julien's comment regarding the function parameters
- why
is the return type int32_t and not uint32_t? Plus as per ./CODING_STYLE
it really shouldn't be a fixed width type anyway, but e.g. unsigned int.
Ayan > Yes, this is a mistake. I will update int32_t to unsigned int in
all the places.
However for extract32(), I don't think we need this function. Rather
Wei's suggestion (ie instr & MASK_for_10_21_24_27 ==
VALID_for_10_21_24_27 ) makes the code simpler and shorter.
- Ayan
+ if ( rc )
+ return IO_ABORT;
+ }
/*
* Erratum 766422: Thumb store translation fault to Hypervisor may
Cheers,
