Re: [XEN v9 2/4] 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 Julien,

Many thanks for the feedback.

I have some clarifications.

On 04/03/2022 10:28, Julien Grall wrote:
> Hi Ayan,
>
> On 01/03/2022 12:40, Ayan Kumar Halder wrote:
> > +void post_increment_register(const struct instr_details *instr)
> > +{
> > +    struct cpu_user_regs *regs = guest_cpu_user_regs();
> > +    register_t val = 0;
> > +
> > +    /* Currently, we handle only ldr/str post indexing instructions */
> > +    if ( instr->state != INSTR_LDR_STR_POSTINDEXING )
> > +        return;
> > +
> > +    /*
> > +     * Handle when rn = SP
> > +     * Refer ArmV8 ARM DDI 0487G.b, Page - D1-2463 "Stack pointer 
> > register
> > +     * selection"
> > +     * t = SP_EL0
> > +     * h = SP_ELx
> > +     * and M[3:0] (Page - C5-474 "When exception taken from AArch64 
> > state:")
> > +     */
> > +    if (instr->rn == 31 )
> > +    {
> > +        if ( (regs->cpsr & PSR_MODE_MASK) == PSR_MODE_EL1h )
> > +            val = regs->sp_el1;
> > +        else if ( ((regs->cpsr & PSR_MODE_MASK) == PSR_MODE_EL1t) ||
> > +                    ((regs->cpsr & PSR_MODE_MASK) == PSR_MODE_EL0t) )
> You are using 3 times regs->cpsr & PSR_MODE_MASK. Can you introduce a 
> temporary variable?
> Alternatively, a switch could be used here.
Yes, a switch is better. I will address that in v10.
> [...]
>
> > diff --git a/xen/arch/arm/include/asm/mmio.h 
> > b/xen/arch/arm/include/asm/mmio.h
> > index 3354d9c635..ef2c57a2d5 100644
> > --- a/xen/arch/arm/include/asm/mmio.h
> > +++ b/xen/arch/arm/include/asm/mmio.h
> > @@ -26,12 +26,24 @@
> >     #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 */
> > +    /*
> > +     * Instruction is decoded successfully. It is a ldr/str post 
> > indexing
> > +     * instruction.
> > +     */
> > +    INSTR_LDR_STR_POSTINDEXING
> NIT: Please add ',' even for the last item. This would reduce the diff 
> if we add new one.
Ack (To be addressed in v10)
> > +};
> > +
> >   typedef struct
> >   {
> >       struct hsr_dabt dabt;
> >       struct instr_details {
> >           unsigned long rn:5;
> >           signed int imm9:9;
> > +        enum instr_decode_state state;
> >       } dabt_instr;
> >       paddr_t gpa;
> >   } mmio_info_t;
> > @@ -69,14 +81,15 @@ struct vmmio {
> >   };
> >     enum io_state try_handle_mmio(struct cpu_user_regs *regs,
> > -                              const union hsr hsr,
> > -                              paddr_t gpa);
> > +                              mmio_info_t *info);
> >   void register_mmio_handler(struct domain *d,
> >                              const struct mmio_handler_ops *ops,
> >                              paddr_t addr, paddr_t size, void *priv);
> >   int domain_io_init(struct domain *d, int max_count);
> >   void domain_io_free(struct domain *d);
> >   +void try_decode_instruction(const struct cpu_user_regs *regs,
> > +                            mmio_info_t *info);
> >     #endif  /* __ASM_ARM_MMIO_H__ */
> >   diff --git a/xen/arch/arm/include/asm/traps.h 
> > b/xen/arch/arm/include/asm/traps.h
> > index 2ed2b85c6f..95c46ad391 100644
> > --- a/xen/arch/arm/include/asm/traps.h
> > +++ b/xen/arch/arm/include/asm/traps.h
> > @@ -109,6 +109,8 @@ static inline register_t sign_extend(const struct 
> > hsr_dabt dabt, register_t r)
> >       return r;
> >   }
> >   +void post_increment_register(const struct instr_details *instr);
> > +
> >   #endif /* __ASM_ARM_TRAPS__ */
> >   /*
> >    * Local variables:
> > diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
> > index fad103bdbd..bea69ffb08 100644
> > --- a/xen/arch/arm/io.c
> > +++ b/xen/arch/arm/io.c
> > @@ -102,57 +102,79 @@ static const struct mmio_handler 
> > *find_mmio_handler(struct domain *d,
> >       return handler;
> >   }
> >   +void try_decode_instruction(const struct cpu_user_regs *regs,
> > +                            mmio_info_t *info)
> > +{
> > +    int rc;
> > +
> > +    if ( info->dabt.valid )
> > +    {
> > +        info->dabt_instr.state = INSTR_VALID;
> > +
> > +        /*
> > +         * Erratum 766422: Thumb store translation fault to 
> > Hypervisor may
> > +         * not have correct HSR Rt value.
> > +         */
> > +        if ( check_workaround_766422() && (regs->cpsr & PSR_THUMB) &&
> > +             info->dabt.write )
> This change is not explained in the commit message. TBH, I think it 
> should be separate but I am not going to request that at v9.
I will leave this as it is then. I will explain in the commit message 
that the logic to infer the type of instruction has been moved from 
try_handle_mmio() to try_decode_instruction() which is called before. 
try_handle_mmio() is solely responsible for handling the mmio operation.
> > +        {
> > +            rc = decode_instruction(regs, info);
> > +            if ( rc )
> > +            {
> > +                gprintk(XENLOG_DEBUG, "Unable to decode 
> > instruction\n");
> > +                info->dabt_instr.state = INSTR_ERROR;
> > +        }
> > +        return;
> > +    }
> > +
> > +    /*
> > +     * Armv8 processor does not provide a valid syndrome for 
> > decoding some
> > +     * instructions. So in order to process these instructions, Xen 
> > must
> > +     * decode them.
> > +     */
> > +    rc = decode_instruction(regs, info);
> > +    if ( rc )
> > +    {
> > +        gprintk(XENLOG_ERR, "Unable to decode instruction\n");
> > +        info->dabt_instr.state = INSTR_ERROR;
> > +    }
> > +}
> > +
> >   enum io_state try_handle_mmio(struct cpu_user_regs *regs,
> > -                              const union hsr hsr,
> > -                              paddr_t gpa)
> > +                              mmio_info_t *info)
> >   {
> >       struct vcpu *v = current;
> >       const struct mmio_handler *handler = NULL;
> > -    const struct hsr_dabt dabt = hsr.dabt;
> > -    mmio_info_t info = {
> > -        .gpa = gpa,
> > -        .dabt = dabt
> > -    };
> > +    int rc;
> >   -    ASSERT(hsr.ec == HSR_EC_DATA_ABORT_LOWER_EL);
> > +    ASSERT(info->dabt.ec == HSR_EC_DATA_ABORT_LOWER_EL);
> >   -    handler = find_mmio_handler(v->domain, info.gpa);
> > -    if ( !handler )
> > +    if ( !((info->dabt_instr.state == INSTR_VALID) || 
> > (info->dabt_instr.state == INSTR_LDR_STR_POSTINDEXING)) )
> This check will become quite large if we decode more class. I would 
> instead set the dabt.valid bit whenever we successfully decoded the 
> instruction and check that if dabt.valid here.
Actually the main reason to introduce INSTR_LDR_STR_POSTINDEXING is to 
distinguish the scenario where the ISS was valid vs when instruction was 
decoded manually.
In the later scenario, one would need to do the post increment of the rn.

It makes sense to me to have a unque 'info->dabt_instr.state' for each 
type of instruction decoded as the post processing will vary. In this 
case, the post processing logic checks that the instruction is 
ldr_str_postindexing.
However your concern that the check will become large is valid. I would 
introduce a function as follows :-
bool check_instr_is_valid(enum instr_decode_state state)

{

    if (state == INSTR_VALID) || (state == INSTR_LDR_STR_POSTINDEXING) 
|| ...)
        return true;

    else

        return false;

}

And then in

enum io_state try_handle_mmio(struct cpu_user_regs *regs, ...)

{

...

    if ( !check_instr_is_valid(info->dabt_instr.state) )

    {

        ASSERT_UNREACHABLE();
        return IO_ABORT;

    }

...

}

Please let me know your thoughts,

> >       {
> > -        int rc;
> > +        ASSERT_UNREACHABLE();
> > +        return IO_ABORT;
> > +    }
> [...]
>
> > @@ -1982,21 +2030,18 @@ static void do_trap_stage2_abort_guest(struct 
> > cpu_user_regs *regs,
> >               case IO_UNHANDLED:
> >                   /* IO unhandled, try another way to handle it. */
> >                   break;
> > -            }
> >           }
> >             /*
> > -         * First check if the translation fault can be resolved by the
> > -         * P2M subsystem. If that's the case nothing else to do.
> > +         * If the instruction syndrome was invalid, then we already 
> > checked if
> > +         * this was due to a P2M fault. So no point to check again 
> > as the result
> > +         * will be the same.
> >            */
> > -        if ( p2m_resolve_translation_fault(current->domain,
> > -                                           gaddr_to_gfn(gpa)) )
> > -            return;
> > -
> > -        if ( is_data && try_map_mmio(gaddr_to_gfn(gpa)) )
> > +        if ( info.dabt.valid && check_p2m(is_data, gpa) )
> This check would need to be adjusted to check the instruction state is 
> INSTR_VALID.
Ack (To be addressed in v10).

- Ayan

> Cheers,