Re: [XEN v7 2/2] xen/arm64: io: Support instructions (for which ISS is not valid) on emulated MMIO region using MMIO/ioreq handler

[Ayan Kumar Halder <ayan.kumar.halder@xxxxxxxxxx>]

Hi All,

I had a discussion with Julien on IRC and this patch need a correction 
(based on my understanding):-
On 05/02/2022 22:58, Ayan Kumar Halder wrote:
> If the instruction was related to cache maintenance, Xen
> will not decode the instruction or do any MMIO operation. Rather it simply
> increments the PC and returns to the guest.
This is not entirely correct :-

If the instruction is cache maintenance and the address is emulated, 
then Xen needs to ignore the ignore the instruction ie increment the PC 
and return to the guest.
However, if the address is not emulated, then Xen will get invoked only 
if the translation table entry corresponding to the address is set 
invalid. In this case, Xen will need to iterate through the translation 
tables and mark the entry as valid and return to the guest to retry the 
instruction.
As of today, we do not handle cache condition (ie dabt.cache == 1) in 
Xen. Thus, I will drop this logic in my patch and will resend the 
current series (unless there are any objections).
I will send out a separate patch to handle the cache condition.

> If the instruction was trapped due
> to stage1 page translation table walk, Xen will update the page tables and will
> return to the guest so that it can retry the instruction. To handle all these
> different states, we have introduced 'enum instr_decode_state'.
>
> Signed-off-by: Ayan Kumar Halder <ayankuma@xxxxxxxxxx>
> ---
<snip>
> diff --git a/xen/arch/arm/include/asm/mmio.h b/xen/arch/arm/include/asm/mmio.h
> index 3354d9c635..fb7ff72cdc 100644
> --- a/xen/arch/arm/include/asm/mmio.h
> +++ b/xen/arch/arm/include/asm/mmio.h
> @@ -26,12 +26,23 @@
>   
>   #define MAX_IO_HANDLER  16
>   
> +enum instr_decode_state
> +{
> +    INSTR_ERROR,                    /* Error encountered while decoding instr 
> */
> +    INSTR_VALID,                    /* ISS is valid, so no need to decode */
> +    INSTR_LDR_STR_POSTINDEXING,     /* Instruction is decoded successfully.
> +                                       It is ldr/str post indexing */
> +    INSTR_IGNORE,                   /* Instruction is to be ignored (ie NOP) */
Drop this entry.
> +    INSTR_RETRY                     /* Instruction is to be retried */
> +};
> +
>   
<snip>
> +    /*
> +     * If the fault occurred due to cache maintenance or address translation
> +     * instructions, then Xen needs to ignore these instructions.
> +     */
> +    if ( info->dabt.cache )
> +    {
> +        info->dabt_instr.state = INSTR_IGNORE;
> +        return;
> +    }
I need to drop this. As stated before, this needs to be done in a 
separate patch.
<snip>

> +
> +            try_decode_instruction(regs, &info);
> +
> +            /*
> +             * If the instruction was to be ignored by Xen, then it should 
> return
> +             * to the caller which will increment the PC, so that the guest can
> +             * execute the next instruction.
> +             */
> +            if ( info.dabt_instr.state == INSTR_IGNORE )
> +            {
> +                advance_pc(regs, hsr);
> +                return;
> +            }
Drop this.

<snip>

- Ayan