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

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

Hi Jan/All,

On 25/01/2022 08:55, Jan Beulich wrote:
> On 24.01.2022 19:41, Stefano Stabellini wrote:
> > On Mon, 24 Jan 2022, Ayan Kumar Halder wrote:
> > > As for the patch, I will mention this issue (as a comment in the code) where
> > > we are loading the instruction from PC. Stefano/Julien/Bertrand/Volodymyr:-
> > > Does it look fine with you ?
> > As this issue could happen on any architecture (the guest could change
> > the instruction from another vcpu while the other is trapping in Xen)
> > and given that we do quite a bit of emulation on x86 I asked Jan on IRC
> > how do we handle this kind of things on x86 today. He had a good answer:
> > "By not making any assumptions on what we're going to find."
> >
> > In other words, don't assume you are going to find a store or a load
> > instruction at the memory location pointed by the PC. You could find
> > total garbage (because it was changed in between). Make sure to check
> > everything is as expected before taking any actions.
> >
> > And I think you are already doing that in decode_loadstore_postindexing.
> >
> > These are the fields:
> >
> > + * 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 */
> > +        signed int imm9:9;            /* imm9 */
> > +        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;
> > +};
> >
> >
> > This patch already checks for:
> > - the fixed values
> > - v
> > - opc
> > - some special rt and rn values
> >
> > Considering that:
> > - size is fine either way
> > - as rt and rn are 5 bits wide, all values are acceptable (x0->x31)
> >
> > It doesn't look like we are missing anything, unless imm9 is restricted
> > to some ranges only.
> Beyond decoding there's at least one further assumption one may
> mistakenly make: The address may not be suitably aligned and it may
> not reference MMIO (or, should that matter, not the specific region
> of MMIO that other trap-provided info my hint at).
As I see, Xen will read/write to the MMIO address provided either by 
gva_to_ipa_pa() or HPFAR_EL2.
However, (you are correct), that the address pointed by Rn might not 
point to the same address (assuming that the instruction was changed 
after being loaded in I cache). In any case, Xen will simply increment 
(or decrement) Rn. The guest will find this new value of Rn (and that 
should be fine it was the guest who had changed the instruction).
In any case, I don't see Xen doing something erroneous.

I will send out a v4 patch addressing the issues pointed by Stefano and 
Andre (commit message).
- Ayan

> Jan