Hi Marc,
On 7/13/23 17:53, Marc Zyngier wrote:
> Hey Eric,
>
> Thanks for looking into this, much appreciated given how tedious it
> is.
>
> On Thu, 13 Jul 2023 15:05:33 +0100,
> Eric Auger <eric.auger@xxxxxxxxxx> wrote:
>> Hi Marc,
>>
>> On 7/12/23 16:57, Marc Zyngier wrote:
>>> Describe the HCR_EL2 register, and associate it with all the sysregs
>>> it allows to trap.
>>>
>>> Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
>>> ---
>>> arch/arm64/kvm/emulate-nested.c | 475 ++++++++++++++++++++++++++++++++
>>> 1 file changed, 475 insertions(+)
>>>
>>> diff --git a/arch/arm64/kvm/emulate-nested.c b/arch/arm64/kvm/emulate-nested.c
>>> index 5bab2e85d70c..51901e85e43d 100644
>>> --- a/arch/arm64/kvm/emulate-nested.c
>>> +++ b/arch/arm64/kvm/emulate-nested.c
> [...]
>
>>> + [CGT_HCR_TPC] = {
>> modern revisions now refer to TPCP, maybe worth a comment?
> Absolutely.
>
> [...]
>
>>> + SR_RANGE_TRAP(SYS_ID_PFR0_EL1,
>>> + sys_reg(3, 0, 0, 7, 7), CGT_HCR_TID3),
>> in the spec I see this upper limit in the FEAT_FGT section. Out of
>> curiosity how were you able to convert the sys reg names into this Op0,
>> Op1, CRn, CRm, Op2. Is there any ordering logic documented somewhere for
>> those group3 regs?
> If you look at the sysreg encoding described on page D18-6308 if
> version J.a of the ARM ARM, you will find a block of 56 contiguous
> encodings ranging from (3, 0, 0, 1, 0), which happens to be
> ID_PFR0_EL1, all the way to a reserved range ending in (3, 0, 0, 7,
> 7).
>
> This is the block of register that is controlled by TID3.
OK thanks
>
>> I checked Table D18-2 and this looks good but I wonder if there isn't
>> any more efficient way to review this.
> Not that I know of, unfortunately. Even the pseudocode isn't enough
> for this as it doesn't described the trapping of unallocated regions.
OK
>
>>> + SR_TRAP(SYS_ICC_SGI0R_EL1, CGT_HCR_IMO_FMO),
>>> + SR_TRAP(SYS_ICC_ASGI1R_EL1, CGT_HCR_IMO_FMO),
>>> + SR_TRAP(SYS_ICC_SGI1R_EL1, CGT_HCR_IMO_FMO),
>>> + SR_RANGE_TRAP(sys_reg(3, 0, 11, 0, 0),
>>> + sys_reg(3, 0, 11, 15, 7), CGT_HCR_TIDCP),
>>> + SR_RANGE_TRAP(sys_reg(3, 1, 11, 0, 0),
>>> + sys_reg(3, 1, 11, 15, 7), CGT_HCR_TIDCP),
>>> + SR_RANGE_TRAP(sys_reg(3, 2, 11, 0, 0),
>>> + sys_reg(3, 2, 11, 15, 7), CGT_HCR_TIDCP),
>>> + SR_RANGE_TRAP(sys_reg(3, 3, 11, 0, 0),
>>> + sys_reg(3, 3, 11, 15, 7), CGT_HCR_TIDCP),
>>> + SR_RANGE_TRAP(sys_reg(3, 4, 11, 0, 0),
>>> + sys_reg(3, 4, 11, 15, 7), CGT_HCR_TIDCP),
>>> + SR_RANGE_TRAP(sys_reg(3, 5, 11, 0, 0),
>>> + sys_reg(3, 5, 11, 15, 7), CGT_HCR_TIDCP),
>>> + SR_RANGE_TRAP(sys_reg(3, 6, 11, 0, 0),
>>> + sys_reg(3, 6, 11, 15, 7), CGT_HCR_TIDCP),
>>> + SR_RANGE_TRAP(sys_reg(3, 7, 11, 0, 0),
>>> + sys_reg(3, 7, 11, 15, 7), CGT_HCR_TIDCP),
>>> + SR_RANGE_TRAP(sys_reg(3, 0, 15, 0, 0),
>>> + sys_reg(3, 0, 15, 15, 7), CGT_HCR_TIDCP),
>>> + SR_RANGE_TRAP(sys_reg(3, 1, 15, 0, 0),
>>> + sys_reg(3, 1, 15, 15, 7), CGT_HCR_TIDCP),
>>> + SR_RANGE_TRAP(sys_reg(3, 2, 15, 0, 0),
>>> + sys_reg(3, 2, 15, 15, 7), CGT_HCR_TIDCP),
>>> + SR_RANGE_TRAP(sys_reg(3, 3, 15, 0, 0),
>>> + sys_reg(3, 3, 15, 15, 7), CGT_HCR_TIDCP),
>>> + SR_RANGE_TRAP(sys_reg(3, 4, 15, 0, 0),
>>> + sys_reg(3, 4, 15, 15, 7), CGT_HCR_TIDCP),
>>> + SR_RANGE_TRAP(sys_reg(3, 5, 15, 0, 0),
>>> + sys_reg(3, 5, 15, 15, 7), CGT_HCR_TIDCP),
>>> + SR_RANGE_TRAP(sys_reg(3, 6, 15, 0, 0),
>>> + sys_reg(3, 6, 15, 15, 7), CGT_HCR_TIDCP),
>>> + SR_RANGE_TRAP(sys_reg(3, 7, 15, 0, 0),
>>> + sys_reg(3, 7, 15, 15, 7), CGT_HCR_TIDCP),
>>> + SR_TRAP(SYS_ACTLR_EL1, CGT_HCR_TACR),
>>> + SR_TRAP(SYS_DC_ISW, CGT_HCR_TSW),
>>> + SR_TRAP(SYS_DC_CSW, CGT_HCR_TSW),
>>> + SR_TRAP(SYS_DC_CISW, CGT_HCR_TSW),
>>> + SR_TRAP(SYS_DC_IGSW, CGT_HCR_TSW),
>>> + SR_TRAP(SYS_DC_IGDSW, CGT_HCR_TSW),
>>> + SR_TRAP(SYS_DC_CGSW, CGT_HCR_TSW),
>>> + SR_TRAP(SYS_DC_CGDSW, CGT_HCR_TSW),
>>> + SR_TRAP(SYS_DC_CIGSW, CGT_HCR_TSW),
>>> + SR_TRAP(SYS_DC_CIGDSW, CGT_HCR_TSW),
>>> + SR_TRAP(SYS_DC_CIVAC, CGT_HCR_TPC),
>> I don't see CVADP?
> Me neither! Good catch!
>
> [...]
>
>>> + SR_TRAP(SYS_SCTLR_EL1, CGT_HCR_TVM_TRVM),
>>> + SR_TRAP(SYS_TTBR0_EL1, CGT_HCR_TVM_TRVM),
>>> + SR_TRAP(SYS_TTBR1_EL1, CGT_HCR_TVM_TRVM),
>>> + SR_TRAP(SYS_TCR_EL1, CGT_HCR_TVM_TRVM),
>>> + SR_TRAP(SYS_ESR_EL1, CGT_HCR_TVM_TRVM),
>>> + SR_TRAP(SYS_FAR_EL1, CGT_HCR_TVM_TRVM),
>>> + SR_TRAP(SYS_AFSR0_EL1, CGT_HCR_TVM_TRVM),*
>> Looking at the SFSR0_EL1 MRS/MSR pseudo code I understand TRVM is tested
>> on read and
>> TVM is tested on write. However CGT_HCR_TVM has FORWARD_ANY behaviour
>> while TRVM looks good as FORWARD_READ? Do I miss something.
> You're not missing anything. For some reason, I had in my head that
> TVM was trapping both reads and writes, while the spec is clear that
> it only traps writes.
>
>>> + SR_TRAP(SYS_AFSR1_EL1, CGT_HCR_TVM_TRVM),*
>> same here and below
> Yup, I need to fix the TVM encoding like this:
>
> @@ -176,7 +176,7 @@ static const struct trap_bits coarse_trap_bits[] = {
> .index = HCR_EL2,
> .value = HCR_TVM,
> .mask = HCR_TVM,
> - .behaviour = BEHAVE_FORWARD_ANY,
> + .behaviour = BEHAVE_FORWARD_WRITE,
yes matches my understanding
> },
> [CGT_HCR_TDZ] = {
> .index = HCR_EL2,
>
> [...]
>
>>> + /* All _EL2 registers */
>>> + SR_RANGE_TRAP(sys_reg(3, 4, 0, 0, 0),
>>> + sys_reg(3, 4, 10, 15, 7), CGT_HCR_NV),
>>> + SR_RANGE_TRAP(sys_reg(3, 4, 12, 0, 0),
>>> + sys_reg(3, 4, 14, 15, 7), CGT_HCR_NV),
>>> + /* All _EL02, _EL12 registers */
>>> + SR_RANGE_TRAP(sys_reg(3, 5, 0, 0, 0),
>>> + sys_reg(3, 5, 10, 15, 7), CGT_HCR_NV),
>>> + SR_RANGE_TRAP(sys_reg(3, 5, 12, 0, 0),
>>> + sys_reg(3, 5, 14, 15, 7), CGT_HCR_NV),
>> same question as bove, where in the ARM ARM do you find those
>> ranges?
> I went over the encoding with a fine comb, and realised that all the
> (3, 4, ...) encodings are EL2, and all the (3, 5, ...) ones are EL02
> and EL12.
Oh good catch
>
> I appreciate that this is taking a massive bet on the future, but
> there is no such rule in the ARM ARM as such...
yeah that's unfortunate that rule is not stated anywhere
>
>>> + SR_TRAP(SYS_SP_EL1, CGT_HCR_NV),*
>>> + SR_TRAP(OP_AT_S1E2R, CGT_HCR_NV),*
>>> + SR_TRAP(OP_AT_S1E2W, CGT_HCR_NV),*
>>> + SR_TRAP(OP_AT_S12E1R, CGT_HCR_NV),*
>>> + SR_TRAP(OP_AT_S12E1W, CGT_HCR_NV),*
>>> + SR_TRAP(OP_AT_S12E0R, CGT_HCR_NV),*
>> according to the pseudo code NV2 is not checked
>> shouldn't we have a separate CGT? Question also valid for a bunch of ops
>> below
> Hmmm. Yes, this is wrong. Well spotted. I guess I need a
> CGT_HCR_NV_nNV2 for the cases that want that particular condition (NV1
> probably needs a similar fix).
NV1 looks good. There NV2 is checked
>
> [...]
>
>> CIGDPAE?
>> CIPAE?
> These two are part of RME (well, technically MEC, but that an RME
> thing), and I have no plan to support this with NV -- yet.
OK
>
>> CFP/CPP/DVP RCTX?
> These are definitely missing. I'll add them.
>
> Thanks again for going through this list, this is awesome work!
you are welcome. This is cumbersome to review but less than writing it I
suspect ;-)
Eric
>
> M.
>
