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

[Andre Przywara <andre.przywara@xxxxxxx>]

On Mon, 6 Dec 2021 19:31:06 +0000
Julien Grall <julien@xxxxxxx> wrote:

Hi,

> On 01/12/2021 08:41, Bertrand Marquis wrote:
> > Hi Ayan,
> >   
> >> On 30 Nov 2021, at 19:13, Ayan Kumar Halder <ayan.kumar.halder@xxxxxxxxxx> 
> >> wrote:
> >>
> >> Hi Andre,
> >>
> >> Thanks for your comments. They are useful.
> >>
> >> On 30/11/2021 09:49, Andre Przywara wrote:  
> >>> On Mon, 29 Nov 2021 19:16:38 +0000
> >>> Ayan Kumar Halder <ayan.kumar.halder@xxxxxxxxxx> wrote:
> >>> Hi,  
> >>>> 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.  
> >>> As mentioned in the other email, this is wrong, if this points to MMIO:
> >>> don't let the compiler do MMIO accesses. If a stage 2 fault isn't in
> >>> an MMIO area, you should not see traps that you cannot handle already.
> >>> So I don't think it's a good idea to use that as an example. And since
> >>> this patch only seems to address this use case, I would doubt its
> >>> usefulness in general.  
> >> Yes, I should have fixed the comment.
> >>
> >> Currently, I am testing with baremetal app which uses inline assembly code 
> >> with post indexing instructions, to access the MMIO.
> >>
> >> ATM, I am testing with 32 bit MMIO only.
> >>
> >> On the usefulness, I am kind of torn as it is legitimate for post indexing 
> >> instructions to be used in an inline-assembly code for accessing MMIO. 
> >> However, that may not be something commonly seen.
> >>
> >> @Stefano/Bertrand/Julien/Volodymyr :- As you are the Arm mantainers, can 
> >> you comment if we should have decoding logic or not ?  
> > 
> > Andre gave you the official statement from Arm and there is nothing more I 
> > can say.  
> 
> I think this would be handy for other hypervisor and OS developper to 
> know what they can expect when running in a virtualized environment. So 
> would it be possible to update the Arm Arm reflecting this statement?

I don't think it's within the scope of the ARM ARM to say that. It just
says that "there is no syndrome information", and your mileage may vary in
working around that.

Personally I would say that if you expect your software to work nicely
under a hypervisor, then just avoid those instructions. The Linux kernel
certainly did so.

You can try to do instruction emulation, but doing this right can get
tricky quickly: think about I$/D$ coherency, MMU on or off, etc.

> > I will leave this decision to Stefano and Julien.  
> 
> I have had a chat on IRC with Stefano about this. I think the main 
> sticking point is the Arm Arm doesn't clearly state those instructions 
> should not be used by a virtualized OS on MMIO regions.

I don't understand why the ARM ARM would need to say that. Certainly you
realise that immediately when trying to use them, and apparently it was not
a problem in the last 8ish years of Xen/ARM's existence.

So it's your decision on having the emulation, I personally would only do
it when there is a *good* use case.
And please apply the demanded scrutiny on the review - including all the
corner cases like Rn=Rt, XZR vs. SP (as Jan said) and possibly MMU status.

Cheers,
Andre

> To me, this topic looks similar to the set/way instruction dilemma. They 
> are a pain to virtualize (and the Arm Arm clearly hint it) but we had to 
> do it because some OSes relied on them.
> 
> I think the main difference is the Arm Arm doesn't hint they should not 
> be used (it only says a valid syndrome is not provided) and the 
> implementation should hopefully be smaller and self-contained.
> 
> So I would be inclined to allow Xen to decode post-indexing instructions 
> (pending the review).
> 
> Cheers,
>