On 03/07/2019 17:32, Alexandru Elisei wrote:
[...]
>>>> +}
>>>> +
>>>> +#define EL2_SYSREG(el2, el1, translate) \
>>>> + [el2 - FIRST_EL2_SYSREG] = { el2, el1, translate }
>>>> +#define PURE_EL2_SYSREG(el2) \
>>>> + [el2 - FIRST_EL2_SYSREG] = { el2,__INVALID_SYSREG__, NULL }
>>>> +/*
>>>> + * Associate vEL2 registers to their EL1 counterparts on the CPU.
>>>> + * The translate function can be NULL, when the register layout is identical.
>>>> + */
>>>> +struct el2_sysreg_map {
>>>> + int sysreg; /* EL2 register index into the array above */
>>>> + int mapping; /* associated EL1 register */
>>>> + u64 (*translate)(u64 value);
>>>> +} nested_sysreg_map[NR_SYS_REGS - FIRST_EL2_SYSREG] = {
>>>> + PURE_EL2_SYSREG( VPIDR_EL2 ),
>>>> + PURE_EL2_SYSREG( VMPIDR_EL2 ),
>>>> + PURE_EL2_SYSREG( ACTLR_EL2 ),
>>>> + PURE_EL2_SYSREG( HCR_EL2 ),
>>>> + PURE_EL2_SYSREG( MDCR_EL2 ),
>>>> + PURE_EL2_SYSREG( HSTR_EL2 ),
>>>> + PURE_EL2_SYSREG( HACR_EL2 ),
>>>> + PURE_EL2_SYSREG( VTTBR_EL2 ),
>>>> + PURE_EL2_SYSREG( VTCR_EL2 ),
>>>> + PURE_EL2_SYSREG( RVBAR_EL2 ),
>>>> + PURE_EL2_SYSREG( RMR_EL2 ),
>>>> + PURE_EL2_SYSREG( TPIDR_EL2 ),
>>>> + PURE_EL2_SYSREG( CNTVOFF_EL2 ),
>>>> + PURE_EL2_SYSREG( CNTHCTL_EL2 ),
>>>> + PURE_EL2_SYSREG( HPFAR_EL2 ),
>>>> + EL2_SYSREG( SCTLR_EL2, SCTLR_EL1, translate_sctlr ),
>>>> + EL2_SYSREG( CPTR_EL2, CPACR_EL1, translate_cptr ),
>>>> + EL2_SYSREG( TTBR0_EL2, TTBR0_EL1, translate_ttbr0 ),
>>>> + EL2_SYSREG( TTBR1_EL2, TTBR1_EL1, NULL ),
>>>> + EL2_SYSREG( TCR_EL2, TCR_EL1, translate_tcr ),
>>>> + EL2_SYSREG( VBAR_EL2, VBAR_EL1, NULL ),
>>>> + EL2_SYSREG( AFSR0_EL2, AFSR0_EL1, NULL ),
>>>> + EL2_SYSREG( AFSR1_EL2, AFSR1_EL1, NULL ),
>>>> + EL2_SYSREG( ESR_EL2, ESR_EL1, NULL ),
>>>> + EL2_SYSREG( FAR_EL2, FAR_EL1, NULL ),
>>>> + EL2_SYSREG( MAIR_EL2, MAIR_EL1, NULL ),
>>>> + EL2_SYSREG( AMAIR_EL2, AMAIR_EL1, NULL ),
>>>> +};
>>> Figuring out which registers are in this map and which aren't and are supposed
>>> to be treated differently is really cumbersome because they are split into two
>>> types of el2 registers and their order is different from the order in enum
>>> vcpu_sysreg (in kvm_host.h). Perhaps adding a comment about what registers will
>>> be treated differently would make the code a bit easier to follow?
>> I'm not sure what this buys us. We have 3 categories of EL2 sysregs:
>> - Purely emulated
>> - Directly mapped onto an EL1 sysreg
>> - Translated from EL2 to EL1
>>
>> I think the wrappers represent that pretty well, although we could split
>> EL2_SYSREG into DIRECT_EL2_SYSREG and TRANSLATE_EL2_SYSREG. As for the
>> order, does it really matter? We also have the trap table order, which
>> is also different from the enum. Do you propose we reorder everything?
>
> The wrappers and the naming are fine.
>
> I was trying to figure out which EL2 registers are in the nested_sysreg_map and
> which aren't (that's what I meant by "two types of registers") by looking at the
> vcpu_sysreg enum. Because the order in the map is different than the order in
> the enum, I was having a difficult time figuring out which registers are not in
> the nested_sysreg_map to make sure we haven't somehow forgot to emulate a register.
>
> So no, I wasn't asking to reorder everything. I was asking if it would be
> appropriate to write a comment stating the intention to treat registers X, Y and
> Z separately from the registers in nested_sysreg_map.
Ah, fair enough. Yes, that's a very reasonable suggestion.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
