[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [XEN v10 1/4] xen/arm64: Decode ldr/str post increment operations



On Thu, 10 Mar 2022, Ayan Kumar Halder wrote:
> At the moment, Xen does not decode any of the arm64 instructions. This
> means that when hsr_dabt.isv == 0, Xen cannot handle those instructions.
> This will lead to Xen to abort the guests (from which those instructions
> originate).
> 
> With this patch, Xen is able to decode ldr/str post indexing instructions.
> These are a subset of instructions for which hsr_dabt.isv == 0.
> 
> The following instructions are now supported by Xen :-
> 1.      ldr     x2,    [x1],    #8
> 2.      ldr     w2,    [x1],    #-4
> 3.      ldr     x2,    [x1],    #-8
> 4.      ldr     w2,    [x1],    #4
> 5.      ldrh    w2,    [x1],    #2
> 6.      ldrb    w2,    [x1],    #1
> 7.      str     x2,    [x1],    #8
> 8.      str     w2,    [x1],    #-4
> 9.      strh    w2,    [x1],    #2
> 10.     strb    w2,    [x1],    #1
> 
> In the subsequent patch, decode_arm64() will get invoked when
> hsr_dabt.isv == 0.
> 
> Signed-off-by: Ayan Kumar Halder <ayankuma@xxxxxxxxxx>

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>

Note that if there are no changes (or small changes) to a patch and
previously someone gave his reviewed-by, it is customary to keep the tag
in the next version of the series.

I.e. you should have added my Reviewed-by directly to the commit message
in v10. See https://marc.info/?l=xen-devel&m=164635451928651


> ---
> 
> Changelog :-
> 
> v2..v5 - Mentioned in the cover letter.
> 
> v6 - 1. Fixed the code style issues as mentioned in v5.
> 
> v7 - No change.
> 
> v8 - 1. Removed some un-necessary header files inclusion.
>      2. Some style changes pointed out in v7.
> 
> v9 - 1. Rebased on top of the master.
>      2. Renamed psr_mode_is_32bit to regs_mode_is_32bit.
> 
> v10 - 1. No change.
> 
>  xen/arch/arm/decode.c           | 79 ++++++++++++++++++++++++++++++++-
>  xen/arch/arm/decode.h           | 48 +++++++++++++++++---
>  xen/arch/arm/include/asm/mmio.h |  4 ++
>  xen/arch/arm/io.c               |  2 +-
>  4 files changed, 124 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c
> index 792c2e92a7..3add87e83a 100644
> --- a/xen/arch/arm/decode.c
> +++ b/xen/arch/arm/decode.c
> @@ -84,6 +84,78 @@ bad_thumb2:
>      return 1;
>  }
>  
> +static int decode_arm64(register_t pc, mmio_info_t *info)
> +{
> +    union instr opcode = {0};
> +    struct hsr_dabt *dabt = &info->dabt;
> +    struct instr_details *dabt_instr = &info->dabt_instr;
> +
> +    if ( raw_copy_from_guest(&opcode.value, (void * __user)pc, sizeof 
> (opcode)) )
> +    {
> +        gprintk(XENLOG_ERR, "Could not copy the instruction from PC\n");
> +        return 1;
> +    }
> +
> +    /*
> +     * Refer Arm v8 ARM DDI 0487G.b, Page - C6-1107
> +     * "Shared decode for all encodings" (under ldr immediate)
> +     * If n == t && n != 31, then the return value is implementation defined
> +     * (can be WBSUPPRESS, UNKNOWN, UNDEFINED or NOP). Thus, we do not 
> support
> +     * this. This holds true for ldrb/ldrh immediate as well.
> +     *
> +     * Also refer, Page - C6-1384, the above described behaviour is same for
> +     * str immediate. This holds true for strb/strh immediate as well
> +     */
> +    if ( (opcode.ldr_str.rn == opcode.ldr_str.rt) && (opcode.ldr_str.rn != 
> 31) )
> +    {
> +        gprintk(XENLOG_ERR, "Rn should not be equal to Rt except for r31\n");
> +        goto bad_loadstore;
> +    }
> +
> +    /* First, let's check for the fixed values */
> +    if ( (opcode.value & POST_INDEX_FIXED_MASK) != POST_INDEX_FIXED_VALUE )
> +    {
> +        gprintk(XENLOG_ERR,
> +                "Decoding instruction 0x%x is not supported\n", 
> opcode.value);
> +        goto bad_loadstore;
> +    }
> +
> +    if ( opcode.ldr_str.v != 0 )
> +    {
> +        gprintk(XENLOG_ERR,
> +                "ldr/str post indexing for vector types are not 
> supported\n");
> +        goto bad_loadstore;
> +    }
> +
> +    /* Check for STR (immediate) */
> +    if ( opcode.ldr_str.opc == 0 )
> +        dabt->write = 1;
> +    /* Check for LDR (immediate) */
> +    else if ( opcode.ldr_str.opc == 1 )
> +        dabt->write = 0;
> +    else
> +    {
> +        gprintk(XENLOG_ERR,
> +                "Decoding ldr/str post indexing is not supported for this 
> variant\n");
> +        goto bad_loadstore;
> +    }
> +
> +    gprintk(XENLOG_INFO,
> +            "opcode->ldr_str.rt = 0x%x, opcode->ldr_str.size = 0x%x, 
> opcode->ldr_str.imm9 = %d\n",
> +            opcode.ldr_str.rt, opcode.ldr_str.size, opcode.ldr_str.imm9);
> +
> +    update_dabt(dabt, opcode.ldr_str.rt, opcode.ldr_str.size, false);
> +
> +    dabt_instr->rn = opcode.ldr_str.rn;
> +    dabt_instr->imm9 = opcode.ldr_str.imm9;
> +
> +    return 0;
> +
> + bad_loadstore:
> +    gprintk(XENLOG_ERR, "unhandled Arm instruction 0x%x\n", opcode.value);
> +    return 1;
> +}
> +
>  static int decode_thumb(register_t pc, struct hsr_dabt *dabt)
>  {
>      uint16_t instr;
> @@ -150,10 +222,13 @@ 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, mmio_info_t *info)
>  {
>      if ( is_32bit_domain(current->domain) && regs->cpsr & PSR_THUMB )
> -        return decode_thumb(regs->pc, dabt);
> +        return decode_thumb(regs->pc, &info->dabt);
> +
> +    if ( !regs_mode_is_32bit(regs) )
> +        return decode_arm64(regs->pc, info);
>  
>      /* TODO: Handle ARM instruction */
>      gprintk(XENLOG_ERR, "unhandled ARM instruction\n");
> diff --git a/xen/arch/arm/decode.h b/xen/arch/arm/decode.h
> index 4613763bdb..13db8ac968 100644
> --- a/xen/arch/arm/decode.h
> +++ b/xen/arch/arm/decode.h
> @@ -23,19 +23,55 @@
>  #include <asm/regs.h>
>  #include <asm/processor.h>
>  
> -/**
> +/*
> + * Refer to the ARMv8 ARM (DDI 0487G.b), Section C4.1.4 Loads and Stores
> + * Page 318 specifies the following bit pattern for
> + * "load/store register (immediate post-indexed)".
> + *
> + * 31 30 29  27 26 25  23   21 20              11   9         4       0
> + * ___________________________________________________________________
> + * |size|1 1 1 |V |0 0 |opc |0 |      imm9     |0 1 |  Rn     |  Rt   |
> + * |____|______|__|____|____|__|_______________|____|_________|_______|
> + */
> +union instr {
> +    uint32_t value;
> +    struct {
> +        unsigned int rt:5;     /* Rt register */
> +        unsigned int rn:5;     /* Rn register */
> +        unsigned int fixed1:2; /* value == 01b */
> +        signed int imm9:9;     /* imm9 */
> +        unsigned int fixed2:1; /* value == 0b */
> +        unsigned int opc:2;    /* opc */
> +        unsigned int fixed3:2; /* value == 00b */
> +        unsigned int v:1;      /* vector */
> +        unsigned int fixed4:3; /* value == 111b */
> +        unsigned int size:2;   /* size */
> +    } ldr_str;
> +};
> +
> +#define POST_INDEX_FIXED_MASK   0x3B200C00
> +#define POST_INDEX_FIXED_VALUE  0x38000400
> +
> +/*
>   * Decode an instruction from pc
> - * /!\ This function is not intended to fully decode an instruction. It
> - * considers that the instruction is valid.
> + * /!\ This function is intended to decode an instruction. It considers that 
> the
> + * instruction is valid.
>   *
> - * This function will get:
> - *  - The transfer register
> + * In case of thumb mode, this function will get:
> + *  - The transfer register (ie Rt)
>   *  - Sign bit
>   *  - Size
> + *
> + * In case of arm64 mode, this function will get:
> + * - The transfer register (ie Rt)
> + * - The source register (ie Rn)
> + * - Size
> + * - Immediate offset
> + * - Read or write
>   */
>  
>  int decode_instruction(const struct cpu_user_regs *regs,
> -                       struct hsr_dabt *dabt);
> +                       mmio_info_t *info);
>  
>  #endif /* __ARCH_ARM_DECODE_H_ */
>  
> diff --git a/xen/arch/arm/include/asm/mmio.h b/xen/arch/arm/include/asm/mmio.h
> index 7ab873cb8f..3354d9c635 100644
> --- a/xen/arch/arm/include/asm/mmio.h
> +++ b/xen/arch/arm/include/asm/mmio.h
> @@ -29,6 +29,10 @@
>  typedef struct
>  {
>      struct hsr_dabt dabt;
> +    struct instr_details {
> +        unsigned long rn:5;
> +        signed int imm9:9;
> +    } dabt_instr;
>      paddr_t gpa;
>  } mmio_info_t;
>  
> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
> index 1a066f9ae5..fad103bdbd 100644
> --- a/xen/arch/arm/io.c
> +++ b/xen/arch/arm/io.c
> @@ -141,7 +141,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);
>          if ( rc )
>          {
>              gprintk(XENLOG_DEBUG, "Unable to decode instruction\n");
> -- 
> 2.17.1
> 
> 



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.