Hi Ayan,
> -----Original Message-----
> From: Xen-devel <xen-devel-boun...@lists.xenproject.org> On Behalf Of Ayan
> Kumar Halder
> Sent: 2021年11月20日 0:52
> To: xen-devel@lists.xenproject.org
> Cc: sstabell...@kernel.org; stefano.stabell...@xilinx.com; jul...@xen.org;
> volodymyr_babc...@epam.com; Bertrand Marquis <bertrand.marq...@arm.com>;
> Rahul Singh <rahul.si...@arm.com>; ayank...@xilinx.com
> Subject: [RFC PATCH] Added the logic to decode 32 bit ldr/str post-
> indexing instructions
>
> At present, post indexing instructions are not emulated by Xen.
> 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.
>
> 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)
> +{
> + int32_t ret;
> +
> + if ( !(start >= 0 && length > 0 && length <= 32 - start) )
> + return -EINVAL;
> +
> + ret = (value >> start) & (~0U >> (32 - length));
> +
> + return ret;
> +}
> +
This function's behavior is very similar to the helps vreg_regx_extra
in vreg.h. If we will introduce more reg bit operation functions like
extract32. Can we consider to reuse them?
> +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;
> +
> + /* For details on decoding, refer to Armv8 Architecture reference
> manual
> + * Section - "Load/store register (immediate post-indexed)", Pg 318
> + */
I have seen two styles of multiple line comments in this patch.
I checked the original file and it has no special comment.
So I think you may need to follow the multiple line comments you
have done for arm/io.c in this patch. And the same for some comments below.
> + 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
> + * op4 = 1 - Load/store register (immediate post-indexed)
> + */
> + 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;
> +
If the check is fixed, why we don't define a VALID_MASK to check it,
something like:
if ( instr & MASK_for_21_24_27 == VALID_FOR_21_24_27 )
> + size = extract32(instr, 30, 2);
> + v = extract32(instr, 26, 1);
> + opc = extract32(instr, 22, 1);
> +
> + /* At the moment, we support STR(immediate) - 32 bit variant and
> + * LDR(immediate) - 32 bit variant only.
> + */
> + if (!((size==2) && (v==0) && ((opc==0) || (opc==1))))
> + 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);
> +
> + 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) )
> + return decode_64bit_loadstore_postindexing(regs->pc, dabt);
> +
> /* TODO: Handle ARM instruction */
> gprintk(XENLOG_ERR, "unhandled ARM instruction\n");
>
> 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.
> + */
> + rc = decode_instruction(regs, &info.dabt);
> + if ( rc )
> + return IO_ABORT;
> + }
>
> /*
> * Erratum 766422: Thumb store translation fault to Hypervisor may
> --
> 2.17.1
>
