Re: [XEN v10 4/4] xen/arm64: io: Handle data abort due to cache maintenance instructions

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 10 Mar 2022, Ayan Kumar Halder wrote:
> When the data abort is caused due to cache maintenance for an address,
> there are three scenarios:-
> 
> 1. Address belonging to a non emulated region - For this, Xen should
> set the corresponding bit in the translation table entry to valid and
> return to the guest to retry the instruction. This can happen sometimes
> as Xen need to set the translation table entry to invalid. (for eg
> 'Break-Before-Make' sequence). Xen returns to the guest to retry the
> instruction.
> 
> 2. Address belongs to an emulated region - Xen should ignore the
> instruction (ie increment the PC) and return to the guest.
> 
> 3. Address is invalid - Xen should forward the data abort to the guest.
> 
> Signed-off-by: Ayan Kumar Halder <ayankuma@xxxxxxxxxx>
> ---
> 
> Changelog:-
> 
> v1...v8 - NA
> 
> v9 - Extracted this change from "[XEN v7 2/2] xen/arm64: io: Support
> instructions (for which ISS is not ..." into a separate patch of its
> own. The reason being this addresses an existing bug in the codebase.
> 
> v10 - 1. To check if the address belongs to an emulated region, one
> needs to check if it has a mmio handler or an ioreq server. In this
> case, Xen should increment the PC
> 2. If the address is invalid (niether emulated MMIO nor the translation
> could be resolved via p2m or mapping the MMIO region), then Xen should
> forward the abort to the guest.
> 
>  xen/arch/arm/include/asm/mmio.h |  1 +
>  xen/arch/arm/io.c               | 20 ++++++++++++++++++++
>  xen/arch/arm/ioreq.c            | 15 ++++++++++++++-
>  3 files changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/include/asm/mmio.h b/xen/arch/arm/include/asm/mmio.h
> index ca259a79c2..79e64d9af8 100644
> --- a/xen/arch/arm/include/asm/mmio.h
> +++ b/xen/arch/arm/include/asm/mmio.h
> @@ -35,6 +35,7 @@ enum instr_decode_state
>       * instruction.
>       */
>      INSTR_LDR_STR_POSTINDEXING,
> +    INSTR_CACHE,                    /* Cache Maintenance instr */
>  };
>  
>  typedef struct
> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
> index e6c77e16bf..c5b2980a5f 100644
> --- a/xen/arch/arm/io.c
> +++ b/xen/arch/arm/io.c
> @@ -139,6 +139,17 @@ void try_decode_instruction(const struct cpu_user_regs 
> *regs,
>          return;
>      }
>  
> +    /*
> +     * When the data abort is caused due to cache maintenance, Xen should 
> check
> +     * if the address belongs to an emulated MMIO region or not. The behavior
> +     * will differ accordingly.
> +     */
> +    if ( info->dabt.cache )
> +    {
> +        info->dabt_instr.state = INSTR_CACHE;
> +        return;
> +    }
> +
>      /*
>       * Armv8 processor does not provide a valid syndrome for decoding some
>       * instructions. So in order to process these instructions, Xen must
> @@ -177,6 +188,15 @@ enum io_state try_handle_mmio(struct cpu_user_regs *regs,
>          return rc;
>      }
>  
> +    /*
> +     * When the data abort is caused due to cache maintenance and the address
> +     * belongs to an emulated region, Xen should ignore this instruction.
> +     */
> +    if ( info->dabt_instr.state == INSTR_CACHE )
> +    {
> +        return IO_HANDLED;
> +    }
>      /*
>       * At this point, we know that the instruction is either valid or has 
> been
>       * decoded successfully. Thus, Xen should be allowed to execute the
> diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c
> index cc9bf23213..0dd2d452f7 100644
> --- a/xen/arch/arm/ioreq.c
> +++ b/xen/arch/arm/ioreq.c
> @@ -29,10 +29,14 @@ enum io_state handle_ioserv(struct cpu_user_regs *regs, 
> struct vcpu *v)
>      const struct hsr_dabt dabt = hsr.dabt;
>      /* Code is similar to handle_read */
>      register_t r = v->io.req.data;
> +    const struct instr_details instr = v->io.info.dabt_instr;
>  
>      /* We are done with the IO */
>      v->io.req.state = STATE_IOREQ_NONE;
>  
> +    if ( instr.state == INSTR_CACHE )
> +        return IO_HANDLED;

It might be possible to get rid of this check here by rearranging the
code in try_handle_mmio a little bit so that handle_ioserv is not called
when INSTR_CACHE. But I don't have an opinion about it.

The patch does what it says on the tin and as far as I can tell followed
Julien's requests so:

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>