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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi,

> On 2 Dec 2020, at 11:12, Bertrand Marquis <bertrand.marquis@xxxxxxx> wrote:
> 
> HI Volodymyr,
> 
>> On 1 Dec 2020, at 16:54, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx> 
>> wrote:
>> 
>> 
>> Hi,
>> 
>> Bertrand Marquis writes:
>> 
>>> Hi Volodymyr,
>>> 
>>>> On 1 Dec 2020, at 12:07, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx> 
>>>> wrote:
>>>> 
>>>> 
>>>> Hi,
>>>> 
>>>> Bertrand Marquis writes:
>>>> 
>>>>> Hi,
>>>>> 
>>>>>> On 30 Nov 2020, at 20:31, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx> 
>>>>>> 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.

Regards
Bertrand

> Regards
> Bertrand
> 
>> 
>> 
>> 
>>> Regards
>>> Bertrand
>>> 
>>> 
>>>> 
>>>> 
>>>>>> 
>>>>>>> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
>>>>>>> ---
>>>>>>> Changes in V2: rebase
>>>>>>> ---
>>>>>>> xen/arch/arm/vcpreg.c | 35 +++++++++++++++++++++++++++++++++++
>>>>>>> 1 file changed, 35 insertions(+)
>>>>>>> 
>>>>>>> diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
>>>>>>> index cdc91cdf5b..d0c6406f34 100644
>>>>>>> --- a/xen/arch/arm/vcpreg.c
>>>>>>> +++ b/xen/arch/arm/vcpreg.c
>>>>>>> @@ -155,6 +155,14 @@ TVM_REG32(CONTEXTIDR, CONTEXTIDR_EL1)
>>>>>>>      break;                                                      \
>>>>>>>  }
>>>>>>> 
>>>>>>> +/* Macro to generate easily case for ID co-processor emulation */
>>>>>>> +#define GENERATE_TID3_INFO(reg,field,offset)                        \
>>>>>>> +    case HSR_CPREG32(reg):                                          \
>>>>>>> +    {                                                               \
>>>>>>> +        return handle_ro_read_val(regs, regidx, cp32.read, hsr,     \
>>>>>>> +                          1, guest_cpuinfo.field.bits[offset]);     \
>>>>>>> +    }
>>>>>>> +
>>>>>>> void do_cp15_32(struct cpu_user_regs *regs, const union hsr hsr)
>>>>>>> {
>>>>>>>  const struct hsr_cp32 cp32 = hsr.cp32;
>>>>>>> @@ -286,6 +294,33 @@ void do_cp15_32(struct cpu_user_regs *regs, const 
>>>>>>> union hsr hsr)
>>>>>>>       */
>>>>>>>      return handle_raz_wi(regs, regidx, cp32.read, hsr, 1);
>>>>>>> 
>>>>>>> +    /*
>>>>>>> +     * HCR_EL2.TID3
>>>>>>> +     *
>>>>>>> +     * This is trapping most Identification registers used by a guest
>>>>>>> +     * to identify the processor features
>>>>>>> +     */
>>>>>>> +    GENERATE_TID3_INFO(ID_PFR0, pfr32, 0)
>>>>>>> +    GENERATE_TID3_INFO(ID_PFR1, pfr32, 1)
>>>>>>> +    GENERATE_TID3_INFO(ID_PFR2, pfr32, 2)
>>>>>>> +    GENERATE_TID3_INFO(ID_DFR0, dbg32, 0)
>>>>>>> +    GENERATE_TID3_INFO(ID_DFR1, dbg32, 1)
>>>>>>> +    GENERATE_TID3_INFO(ID_AFR0, aux32, 0)
>>>>>>> +    GENERATE_TID3_INFO(ID_MMFR0, mm32, 0)
>>>>>>> +    GENERATE_TID3_INFO(ID_MMFR1, mm32, 1)
>>>>>>> +    GENERATE_TID3_INFO(ID_MMFR2, mm32, 2)
>>>>>>> +    GENERATE_TID3_INFO(ID_MMFR3, mm32, 3)
>>>>>>> +    GENERATE_TID3_INFO(ID_MMFR4, mm32, 4)
>>>>>>> +    GENERATE_TID3_INFO(ID_MMFR5, mm32, 5)
>>>>>>> +    GENERATE_TID3_INFO(ID_ISAR0, isa32, 0)
>>>>>>> +    GENERATE_TID3_INFO(ID_ISAR1, isa32, 1)
>>>>>>> +    GENERATE_TID3_INFO(ID_ISAR2, isa32, 2)
>>>>>>> +    GENERATE_TID3_INFO(ID_ISAR3, isa32, 3)
>>>>>>> +    GENERATE_TID3_INFO(ID_ISAR4, isa32, 4)
>>>>>>> +    GENERATE_TID3_INFO(ID_ISAR5, isa32, 5)
>>>>>>> +    GENERATE_TID3_INFO(ID_ISAR6, isa32, 6)
>>>>>>> +    /* MVFR registers are in cp10 no cp15 */
>>>>>>> +
>>>>>>>  /*
>>>>>>>   * HCR_EL2.TIDCP
>>>>>>>   *
>>>>>> 
>>>>>> 
>>>>>> -- 
>>>>>> Volodymyr Babchuk at EPAM
>>>> 
>>>> 
>>>> -- 
>>>> Volodymyr Babchuk at EPAM
>> 
>> 
>> -- 
>> Volodymyr Babchuk at EPAM