Re: [RFC PATCH] Added the logic to decode 32 bit ldr/str post-indexing instructions

[Julien Grall <julien@xxxxxxx>]

On 22/11/2021 14:19, Ayan Kumar Halder wrote:
> Hi Julien/Stefano/Wei/Jan,
Hi,

> As some of the comments were inter-related, I have consolidated my 
> response to all the comments below.
> On 19/11/2021 17:26, Julien Grall wrote:
> > Hi Ayan,
> >
> > On 19/11/2021 16:52, Ayan Kumar Halder wrote:
> > > At present, post indexing instructions are not emulated by Xen.
> > Can you explain in the commit message why this is a problem?
> Yes, I will update the message as below :-
> Armv8 hardware does not provide the correct syndrome for decoding post 
> indexing ldr/str instructions. 
This statement implies that the issue happens on both AArch32 and 
AArch64 state. I am OK if we only handle the latter for now. But I would 
clarify it in the commit message.
> Thus any post indexing ldr/str 
> instruction at EL1 results in a data abort with ISV=0. 
Instruction from EL0 may also trap in Xen. So I would prefer if you just 
say "instruction executed by a domain results ...".
> As a result, Xen 
> needs to decode the instruction.
Can you give some information on the domain used. Something like:

"this was discovered with XXX which let the compiler deciding which 
instruction to use."
> Thus, DomU will be able to read/write to ioreg space with post indexing 
I would say "domain" rather "domU" because all the domains are affected.

> instructions for 32 bit.
How about the following commit message:

"
xen/arm64: io: Decode 32-bit ldr/str post-indexing instructions

At the moment, Xen is only handling data abort with valid syndrome (i.e.
ISV=0). Unfortunately, this doesn't cover all the instructions a domain
could use to access MMIO regions.

For instance, Xilinx baremetal OS will use:

        volatile u32 *LocalAddr = (volatile u32 *)Addr;
        *LocalAddr = Value;

This leave the compiler to decide which store instructions to use. This
may be a post-index store instruction where the HW will not provide a
valid syndrome.

In order to handle post-indexing store/load instructions,
Xen will need to fetch and decode the instruction.

This patch only cover post-index store/load instructions from AArch64 
mode. For now, this is left unimplemented for trap from AArch32 mode.
"

> > > +{
> > > +    int32_t ret;
> > > +
> > > +    if ( !(start >= 0 && length > 0 && length <= 32 - start) )
> > > +        return -EINVAL;
> > > +
> > > +    ret = (value >> start) & (~0U >> (32 - length));
> > This would be easier to read if you use GENMASK().
> I see that GENMASK returns a register mask. In my scenario, I wish to 
> compare the offsets 10, 21, 24 and 27 to some fixed value.
> So, instead of using GENMASK four times, I can try the following
> instr & MASK_for_10_21_24_27 == VALID_for_10_21_24_27 (Wei Chen's 
> suggestion)
That would be OK with me. Alternatively, you could use the union 
approach suggested by Bertrand.
> Also for size, v and opc, I can defined another bitmask to compare with 
> VALID_for_32bit_LDR | VALID_for_32bit_STR.
> Wei Chen, You had suggested using vreg_regx_extract(). However, I see 
> that it is used to extract the complete u64 register value. In this 
> case, I wish to compare certain offsets within a 32 bit register to some 
> expected values. So, vreg_regx_extract() might not be appropriate and we 
> can use the method mentioned before.
vreg_reg*_extract() are meant to work on a register. So I think they are 
not appropriate here as you don't deal with registers.
[...]

> > > +
> > > +    /* At the moment, we support STR(immediate) - 32 bit variant and
> > > +     * LDR(immediate) - 32 bit variant only.
> > > +     */
> > Coding style.
> Ack
>
> > > +    if (!((size==2) && (v==0) && ((opc==0) || (opc==1))))
> > The coding style for this should be:
> >
> > if ( !(( size == 2 ) && ( ... ) ... ) )
> Ack
>
> >   +        goto bad_64bit_loadstore;
> > > +
> > > +    rt = extract32(instr, 0, 5);
> > > +    imm9 = extract32(instr, 12, 9);
> > > +
> > > +    if ( imm9 < 0 )
> > > +        update_dabt(dabt, rt, size, true);
> > > +    else
> > > +        update_dabt(dabt, rt, size, false);
> > This could be simplified with:
> >
> > update_dabt(dabt, rt, size, imm9 < 0);
> Ack
>
> > > +
> > > +    dabt->valid = 1;
> > > +
> > > +
> > > +    return 0;
> > > +bad_64bit_loadstore:
> > > +    gprintk(XENLOG_ERR, "unhandled 64bit instruction 0x%x\n", instr);
> > > +    return 1;
> > > +}
> > > +
> > >   static int decode_thumb(register_t pc, struct hsr_dabt *dabt)
> > >   {
> > >       uint16_t instr;
> > > @@ -155,6 +229,9 @@ int decode_instruction(const struct cpu_user_regs 
> > > *regs, struct hsr_dabt *dabt)
> > >       if ( is_32bit_domain(current->domain) && regs->cpsr & PSR_THUMB )
> > >           return decode_thumb(regs->pc, dabt);
> > > +    if ( is_64bit_domain(current->domain) )
> > You can still run 32-bit code in 64-bit domain. So I think you want to 
> > check the SPSR mode.
> Do you mean the following check :-
> if ( (is_64bit_domain(current->domain) && (!(regs->spsr & PSR_MODE_BIT)) )
This should work, but I think you can simplify to use:

!psr_mode_is_32bit()

> > > +         */
> > > +        rc = decode_instruction(regs, &info.dabt);
> > I actually expect this to also be useful when forwarding the I/O to 
> > device-model. So I would move the decoding earlier in the code and the 
> > check of dabt.valid earlier.
> You mean I will move decode_instruction() before find_mmio_handler() ?
Yes.

Cheers,

--
Julien Grall