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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

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.

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