Re: [PATCH v2 5/7] xen/arm: Add handler for cp15 ID registers

[Stefano Stabellini <sstabellini@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 5465 bytes --]

On Wed, 2 Dec 2020, Bertrand Marquis wrote:
> > On 2 Dec 2020, at 11:12, Bertrand Marquis <bertrand.marquis@arm.com> wrote:
> > 
> > HI Volodymyr,
> > 
> >> On 1 Dec 2020, at 16:54, Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> wrote:
> >> 
> >> 
> >> Hi,
> >> 
> >> Bertrand Marquis writes:
> >> 
> >>> Hi Volodymyr,
> >>> 
> >>>> On 1 Dec 2020, at 12:07, Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> wrote:
> >>>> 
> >>>> 
> >>>> Hi,
> >>>> 
> >>>> Bertrand Marquis writes:
> >>>> 
> >>>>> Hi,
> >>>>> 
> >>>>>> On 30 Nov 2020, at 20:31, Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> wrote:
> >>>>>> 
> >>>>>> 
> >>>>>> Bertrand Marquis writes:
> >>>>>> 
> >>>>>>> Add support for emulation of cp15 based ID registers (on arm32 or when
> >>>>>>> running a 32bit guest on arm64).
> >>>>>>> The handlers are returning the values stored in the guest_cpuinfo
> >>>>>>> structure.
> >>>>>>> In the current status the MVFR registers are no supported.
> >>>>>> 
> >>>>>> It is unclear what will happen with registers that are not covered by
> >>>>>> guest_cpuinfo structure. According to ARM ARM, it is implementation
> >>>>>> defined if such accesses will be trapped. On other hand, there are many
> >>>>>> registers which are RAZ. So, good behaving guest can try to read one of
> >>>>>> that registers and it will get undefined instruction exception, instead
> >>>>>> of just reading all zeroes.
> >>>>> 
> >>>>> This is true in the status of this patch but this is solved by the next patch
> >>>>> which is adding proper handling of those registers (add CP10 exception
> >>>>> support), at least for MVFR ones.
> >>>>> 
> >>>>> From ARM ARM point of view, I did handle all registers listed I think.
> >>>>> If you think some are missing please point me to them as O do not
> >>>>> completely understand what are the “registers not covered” unless
> >>>>> you mean the MVFR ones.
> >>>> 
> >>>> Well, I may be wrong for aarch32 case, but for aarch64, there are number
> >>>> of reserved registers in IDs range. Those registers should read as
> >>>> zero. You can find them in the section "C5.1.6 op0==0b11, Moves to and
> >>>> from non-debug System registers and Special-purpose registers" of ARM
> >>>> DDI 0487B.a. Check out "Table C5-6 System instruction encodings for
> >>>> non-Debug System register accesses".
> >>> 
> >>> The point of the serie is to handle all registers trapped due to TID3 bit in HCR_EL2.
> >>> 
> >>> And i think I handled all of them but I might be wrong.
> >>> 
> >>> Handling all registers for op0==0b11 will cover a lot more things.
> >>> This can be done of course but this was not the point of this serie.
> >>> 
> >>> The listing in HCR_EL2 documentation is pretty complete and if I miss any register
> >>> there please tell me but I do no understand from the documentation that all registers
> >>> with op0 3 are trapped by TID3.
> >> 
> >> My concern is that the same code may observe different effects when
> >> running in baremetal mode and as VM.
> >> 
> >> For example, we are trying to run a newer version of a kernel, that
> >> supports some hypothetical ARMv8.9. And it tries to read a new ID
> >> register which is absent in ARMv8.2. There are possible cases:
> >> 
> >> 0. It runs as a baremetal code on a compatible architecture. So it just
> >> accesses this register and all is fine.
> >> 
> >> 1. It runs as baremetal code on older ARM8 architecture. Current
> >> reference manual states that those registers should read as zero, so
> >> all is fine, as well.
> >> 
> >> 2. It runs as VM on an older architecture. It is IMPLEMENTATION DEFINED
> >> if HCR.ID3 will cause traps when access to unassigned registers:
> >> 
> >> 2a. Platform does not cause traps and software reads zeros directly from
> >> real registers. This is a good outcome.
> >> 
> >> 2b. Platform causes trap, and your code injects the undefined
> >> instruction exception. This is a case that bothers me. Well written code
> >> that is tries to be compatible with different versions of architecture
> >> will fail there.
> >> 
> >> 3. It runs as VM on a never architecture. I can only speculate there,
> >> but I think, that list of registers trapped by HCR.ID3 will be extended
> >> when new features are added. At least, this does not contradict with the
> >> current IMPLEMENTATION DEFINED constraint. In this case, hypervisor will
> >> inject exception when guest tries to access a valid register.
> >> 
> >> 
> >> So, in my opinion and to be compatible with the reference manual, we
> >> should allow guests to read "Reserved, RAZ" registers.
> > 
> > Thanks for the very detailed explanation, I know better understand what you
> > mean here.
> > 
> > I will try to check if we could return RAZ for “other” op0=3 registers and what
> > should be done on cp15 registers to have something equivalent.
> > 
> 
> In fact I need to add handling for other registers mentionned by the TID3
> description in the armv8 architecture manual:
> "This field traps all MRS accesses to registers in the following range that are not
> already mentioned in this field description: Op0 == 3, op1 == 0, CRn == c0,
>  CRm == {c1-c7}, op2 == {0-7}.”
> "This field traps all MRC accesses to encodings in the following range that are not
> already mentioned in this field description: coproc == p15, opc1 == 0, CRn == c0,
> CRm == {c2-c7}, opc2 == {0-7}.”
> 
> I will check how i can do that.
> 
> Thanks a lot for the review.

Well spotted Volodymyr!