[XEN,v10,0/4] xen/arm64: io: Decode ldr/str post-indexing instruction

Message

Ayan Kumar Halder March 10, 2022, 5:44 p.m. UTC

Hi All,

The patch series introduces support to decode instructions by Xen when ISS is
invalid. Currently, when the guest executes post indexing ldr/str instructions
on emulated MMIO, these instructions are trapped into Xen as a data abort.
Xen reads hsr_dabt.isv == 0, so ISS is invalid. Therefore, it reads the faulting
instruction's opcode from guest's PC. It decodes and executes the instruction on
the emulated region.

While doing the patch, we found two bugs in the codebase. I have addressed them
in patches 3 and 4. These bugs were discussed with Julien on IRC chat. The
purpose of addressing these bugs (in this series) is that 1. We should not forget
about them. 2. To get clarity if our understanding is correct.

Ayan Kumar Halder (4):
  xen/arm64: Decode ldr/str post increment operations
  xen/arm64: io: Support instructions (for which ISS is not valid) on
    emulated MMIO region using MMIO/ioreq handler
  xen/arm64: io: Handle the abort due to access to stage1 translation
    table
  xen/arm64: io: Handle data abort due to cache maintenance instructions


Ayan Kumar Halder (4):
  xen/arm64: Decode ldr/str post increment operations
  xen/arm64: io: Support instructions (for which ISS is not valid) on
    emulated MMIO region using MMIO/ioreq handler
  xen/arm64: io: Handle the abort due to access to stage1 translation
    table
  xen/arm64: io: Handle data abort due to cache maintenance instructions

 xen/arch/arm/arm32/traps.c        |  11 +++
 xen/arch/arm/arm64/traps.c        |  52 ++++++++++++++
 xen/arch/arm/decode.c             |  81 ++++++++++++++++++++-
 xen/arch/arm/decode.h             |  48 +++++++++++--
 xen/arch/arm/include/asm/domain.h |   4 ++
 xen/arch/arm/include/asm/mmio.h   |  22 +++++-
 xen/arch/arm/include/asm/traps.h  |   2 +
 xen/arch/arm/io.c                 | 115 ++++++++++++++++++++++--------
 xen/arch/arm/ioreq.c              |  21 +++++-
 xen/arch/arm/traps.c              |  77 +++++++++++++++-----
 xen/arch/x86/include/asm/ioreq.h  |   3 +
 xen/include/xen/sched.h           |   2 +
 12 files changed, 379 insertions(+), 59 deletions(-)

Changelog :-
v2 - 1. Updated the rn register after reading from it. (Pointed by Julien,
        Stefano)
     2. Used a union to represent the instruction opcode (Suggestd by Bertrand)
     3. Fixed coding style issues (Pointed by Julien)
     4. In the previous patch, I was updating dabt->sign based on the signedness
        of imm9. This was incorrect. As mentioned in ARMv8 ARM  DDI 0487G.b,
        Page 3221, SSE indicates the signedness of the data item loaded. In our
        case, the data item loaded is always unsigned.

v3- 1. Handled all the variants of ldr/str (ie 64, 32, 16, 8 bit variants).
       Thus, I have removed the check for "instr->code.opc == 0" (Suggested by
       Andre)
    2. Handled the scenario when rn = SP, rt = XZR (Suggested by Jan, Andre)
    3. Added restriction for "rt != rn" (Suggested by Andre)
    4. Moved union ldr_str_instr_class {} to decode.h. This is the header included
       by io.c and decode.c (where the union is referred). (Suggested by Jan)
    5. Indentation and typo fixes (Suggested by Jan)

v4- 1. Fixed the patch as per Stefano's comments on v3. They are as follows :-
        1.1 Use macros to determine the fixed values in the instruction opcode
        1.2 Checked if instr != NULL
        1.3 Changed some data types and added #define ARM_64 for AArch64 specific
            code
        1.4 Moved post_increment_register() to decode.c so that the decoding
            logic is confined to a single file.
        1.5 Moved some checks from post_increment_register() to
            decode_loadstore_postindexing()
        1.6 Removed a duplicate check
    2. Updated the commit message as per Andre's comments.
    3. Changed the names of a label and some comments. *32bit* was erroneously
       mentioned in a label and comments in decode_loadstore_postindexing()
       although the function handled all variants of ldr/str post indexing.

v5- 1. Renamed decode_loadstore_postindexing() to decode_arm64(). The reason
       being this will be extended in future to support more instructions for
       which hsr_badt.isv = 0
    2. Introduce a function try_decode_instruction_invalid_iss() to determine
       if the instruction needs to be decoded before invoking decode_instruction().

       It checks :-
       2.1  dabt->s1ptw - Returns IO_UNHANDLED
       2.2  dabt->cache - Returns IO_IGNORED. (new enum instroduced to let the
            caller know that the instruction needs to be ignored by Xen. Thus
            the caller needs to increment the PC and return to the guest.

    3. Invoked try_decode_instruction_invalid_iss() from the following 2 places :-
        3.a - try_handle_mmio() - When we have determined that there is a valid
              mmio handler.
        3.b - try_fwd_ioserv()
        When ioserver completes the io request, the acknowledgement is sent via
        handle_ioserv(). Here, we need to increment the register. As there is no
        common data shared between try_fwd_ioserv() and handle_ioserv(), we need
        to decode the instruction again in handle_ioserv() to determine rn, imm9.

        (NOTE to Reviewers) - This does not feel correct. However, I could not
        think of a better approach. Please provide your inputs.

    4. Augumented struct hsr_dabt{} with struct hsr_dabt_instr_details{} to hold
       rn and imm9. This is passed to post_increment_register() to update rn.
    5. Other style changes as suggested in v4.

v6 - 1. Split the patch into three parts.

v7 - 1. Merged patch2 and patch3 into a single patch.

v8 - 1. Changes mentioned in the individual patches.

v9 - 1. Added patches 3 and 4 to address the bugs found in the existing codebase.
     2. Changes mentioned in the individual patches.

v10 - 1. Changes mentioned in the individual patches.

Comments

Julien Grall March 14, 2022, 6:07 p.m. UTC | #1

On 10/03/2022 17:44, Ayan Kumar Halder wrote:
> Hi All,

Hi Ayan,

Thank you for your contribution.

> Ayan Kumar Halder (4):
>    xen/arm64: Decode ldr/str post increment operations
>    xen/arm64: io: Support instructions (for which ISS is not valid) on
>      emulated MMIO region using MMIO/ioreq handler
>    xen/arm64: io: Handle the abort due to access to stage1 translation
>      table

I have committed the first 3 patches. Patch #4 requires more discussion.

Cheers,