Re: [XEN v2] xen/arm64: io: Decode 32-bit ldr/str post-indexing instructions

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

Hi Jan,

Thanks a lot for the feedback.
I need a clarification.

On 30/11/2021 07:57, Jan Beulich wrote:
> On 29.11.2021 20:16, Ayan Kumar Halder wrote:
> > 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.
> >
> > Signed-off-by: Ayan Kumar Halder <ayankuma@xxxxxxxxxx>
> Just a couple of general remarks, with no judgement towards its use
> in the codebase, and leaving out several obvious style issues.
>
> > 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.
> >
> > This has been tested for the following cases :-
> > ldr x2, [x1], #4
> DYM "ldr w2, [x1], #4" or "ldr x2, [x1], #8" here?
Yes, you are correct.
It is "ldr w2, [x1], #4"

> > ldr w2, [x1], #-4
> >
> > str x2, [x1], #4
> Similar aspect here.
"str w2, [x1], #4"

> > str w2, [x1], #-4
> >
> > The reason being  I am testing on 32bit MMIO registers only. I don't see a 8bit
> > or 16bit MMIO register.
> As per this, perhaps the former of the two.
>
> > --- a/xen/arch/arm/decode.c
> > +++ b/xen/arch/arm/decode.c
> > @@ -84,6 +84,66 @@ bad_thumb2:
> >       return 1;
> >   }
> >   
> > +static int decode_32bit_loadstore_postindexing(register_t pc,
> > +                                               struct hsr_dabt *dabt,
> > +                                               union ldr_str_instr_class 
> > *instr)
> > +{
> > +    if ( raw_copy_from_guest(&instr->value, (void * __user)pc, sizeof (instr)) 
> > )
> > +        return -EFAULT;
> > +
> > +    /* First, let's check for the fixed values */
> > +    if ( !((instr->code.fixed1 == 1) && (instr->code.fixed2 == 0) &&
> > +         (instr->code.fixed3 == 0) && (instr->code.fixed4 == 7)) )
> > +    {
> > +        gprintk(XENLOG_ERR, "Decoding not supported for instructions other 
> > than"
> > +            " ldr/str post indexing\n");
> > +        goto bad_32bit_loadstore;
> > +    }
> > +
> > +    if ( instr->code.size != 2 )
> > +    {
> > +        gprintk(XENLOG_ERR,
> > +            "ldr/str post indexing is supported for 32 bit variant only\n");
> > +        goto bad_32bit_loadstore;
> > +    }
> > +
> > +    if ( instr->code.v != 0 )
> > +    {
> > +        gprintk(XENLOG_ERR,
> > +            "ldr/str post indexing for vector types are not supported\n");
> > +        goto bad_32bit_loadstore;
> > +    }
> > +
> > +    /* Check for STR (immediate) - 32 bit variant */
> > +    if ( instr->code.opc == 0 )
> > +    {
> > +        dabt->write = 1;
> > +    }
> > +    /* Check for LDR (immediate) - 32 bit variant */
> > +    else if ( instr->code.opc == 1 )
> > +    {
> > +        dabt->write = 0;
> > +    }
> > +    else
> > +    {
> > +        gprintk(XENLOG_ERR,
> > +            "Decoding ldr/str post indexing is not supported for this 
> > variant\n");
> > +        goto bad_32bit_loadstore;
> > +    }
> > +
> > +    gprintk(XENLOG_INFO,
> > +        "instr->code.rt = 0x%x, instr->code.size = 0x%x, instr->code.imm9 = 
> > %d\n",
> > +        instr->code.rt, instr->code.size, instr->code.imm9);
> > +
> > +    update_dabt(dabt, instr->code.rt, instr->code.size, false);
> > +    dabt->valid = 1;
> > +
> > +    return 0;
> > +bad_32bit_loadstore:
> Please indent labels by at least a blank. I also think this would
> benefit from a preceding blank line.
Ack

> > --- a/xen/arch/arm/io.c
> > +++ b/xen/arch/arm/io.c
> > @@ -65,6 +65,16 @@ static enum io_state handle_write(const struct mmio_handler 
> > *handler,
> >       return ret ? IO_HANDLED : IO_ABORT;
> >   }
> >   
> > +static void post_incremenet_register(union ldr_str_instr_class *instr)
> I think you mean post_increment_register()?
Ack. Sorry for my carelessness. :(

> > +{
> > +    struct cpu_user_regs *regs = guest_cpu_user_regs();
> > +    unsigned int val;
> > +
> > +    val = get_user_reg(regs, instr->code.rn);
> > +    val += instr->code.imm9;
> > +    set_user_reg(regs, instr->code.rn, val);
> I don't think this handles the SP case correctly, and I also don't see
> that case getting rejected elsewhere.
Sorry, I did not understand you. Can you explain a bit more ?

Following 
https://www.keil.com/support/man/docs/armasm/armasm_dom1361289873425.htm 
, Are you saying that we need to handle this restriction
"You can use SP for Rt in non-word instructions in ARM code but this is 
deprecated in ARMv6T2 and above"

> > --- a/xen/include/asm-arm/hsr.h
> > +++ b/xen/include/asm-arm/hsr.h
> > @@ -15,6 +15,32 @@ enum dabt_size {
> >       DABT_DOUBLE_WORD = 3,
> >   };
> >   
> > +/*
> > + * Refer to the ARMv8 ARM (DDI 0487G.b), Section C4.1.4 Loads and Stores
> > + * Page 318 specifies the following bit pattern for
> > + * "load/store register (immediate post-indexed)".
> > + *
> > + * 31 30 29  27 26 25  23   21 20              11   9         4       0
> > + * ___________________________________________________________________
> > + * |size|1 1 1 |V |0 0 |opc |0 |      imm9     |0 1 |  Rn     |  Rt   |
> > + * |____|______|__|____|____|__|_______________|____|_________|_______|
> > + */
> > +union ldr_str_instr_class {
> > +    uint32_t value;
> > +    struct ldr_str {
> > +        unsigned int rt:5;     /* Rt register */
> > +        unsigned int rn:5;     /* Rn register */
> > +        unsigned int fixed1:2; /* value == 01b */
> > +        int imm9:9;            /* imm9 */
> Plain int bitfields are, iirc, signed or unsigned at the compiler's
> discretion. Hence I think you mean explicitly "signed int" here.
>
> > +        unsigned int fixed2:1; /* value == 0b */
> > +        unsigned int opc:2;    /* opc */
> > +        unsigned int fixed3:2; /* value == 00b */
> > +        unsigned int v:1;      /* vector */
> > +        unsigned int fixed4:3; /* value == 111b */
> > +        unsigned int size:2;   /* size */
> > +    } code;
> > +};
> I'd recommend types needed in just one CU to live there, rather than
> getting exposed to every source file including this header (even more
> so when - aiui - this is entirely unrelated to HSR). When used in
> just a single function, it might even want to live here (i.e. as
> close as possible to the [only] use).
Ack. I will try to use bitmask as Andre suggested.

- Ayan
> Jan