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

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

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.



> 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