[PATCH v3 15/27] KVM: arm64: nv: Add trap forwarding for HCR_EL2

[Miguel Luis]

Hi Marc,

> On 15 Aug 2023, at 10:39, Marc Zyngier <maz at kernel.org> wrote:
> 
> Miguel,
> 
> On Sat, 12 Aug 2023 04:08:22 +0100,
> Miguel Luis <miguel.luis at oracle.com> wrote:
>> 
>> Hi Marc,
>> 
>>> On 8 Aug 2023, at 11:46, Marc Zyngier <maz at kernel.org> wrote:
>>> 
>>> Describe the HCR_EL2 register, and associate it with all the sysregs
>>> it allows to trap.
>>> 
>>> Reviewed-by: Eric Auger <eric.auger at redhat.com>
>>> Signed-off-by: Marc Zyngier <maz at kernel.org>
>>> ---
>>> arch/arm64/kvm/emulate-nested.c | 486 ++++++++++++++++++++++++++++++++
>>> 1 file changed, 486 insertions(+)
>>> 
>>> diff --git a/arch/arm64/kvm/emulate-nested.c b/arch/arm64/kvm/emulate-nested.c
>>> index 1b1148770d45..2122d16bdeeb 100644
>>> --- a/arch/arm64/kvm/emulate-nested.c
>>> +++ b/arch/arm64/kvm/emulate-nested.c
>>> @@ -37,12 +37,48 @@ enum trap_group {
>>> * on their own instead of being part of a combination of
>>> * trap controls.
>>> */
>>> + CGT_HCR_TID1,
>>> + CGT_HCR_TID2,
>>> + CGT_HCR_TID3,
>>> + CGT_HCR_IMO,
>>> + CGT_HCR_FMO,
>>> + CGT_HCR_TIDCP,
>>> + CGT_HCR_TACR,
>>> + CGT_HCR_TSW,
>>> + CGT_HCR_TPC,
>>> + CGT_HCR_TPU,
>>> + CGT_HCR_TTLB,
>>> + CGT_HCR_TVM,
>>> + CGT_HCR_TDZ,
>>> + CGT_HCR_TRVM,
>>> + CGT_HCR_TLOR,
>>> + CGT_HCR_TERR,
>>> + CGT_HCR_APK,
>>> + CGT_HCR_NV,
>>> + CGT_HCR_NV_nNV2,
>>> + CGT_HCR_NV1_nNV2,
>>> + CGT_HCR_AT,
>>> + CGT_HCR_FIEN,
>>> + CGT_HCR_TID4,
>>> + CGT_HCR_TICAB,
>>> + CGT_HCR_TOCU,
>>> + CGT_HCR_ENSCXT,
>>> + CGT_HCR_TTLBIS,
>>> + CGT_HCR_TTLBOS,
>>> 
>>> /*
>>> * Anything after this point is a combination of trap controls,
>>> * which all must be evaluated to decide what to do.
>>> */
>>> __MULTIPLE_CONTROL_BITS__,
>>> + CGT_HCR_IMO_FMO = __MULTIPLE_CONTROL_BITS__,
>>> + CGT_HCR_TID2_TID4,
>>> + CGT_HCR_TTLB_TTLBIS,
>>> + CGT_HCR_TTLB_TTLBOS,
>>> + CGT_HCR_TVM_TRVM,
>>> + CGT_HCR_TPU_TICAB,
>>> + CGT_HCR_TPU_TOCU,
>>> + CGT_HCR_NV1_nNV2_ENSCXT,
>>> 
>>> /*
>>> * Anything after this point requires a callback evaluating a
>>> @@ -55,6 +91,174 @@ enum trap_group {
>>> };
>>> 
>>> static const struct trap_bits coarse_trap_bits[] = {
>>> + [CGT_HCR_TID1] = {
>>> + .index = HCR_EL2,
>>> + .value = HCR_TID1,
>>> + .mask = HCR_TID1,
>>> + .behaviour = BEHAVE_FORWARD_READ,
>>> + },
>>> + [CGT_HCR_TID2] = {
>>> + .index = HCR_EL2,
>>> + .value = HCR_TID2,
>>> + .mask = HCR_TID2,
>>> + .behaviour = BEHAVE_FORWARD_ANY,
>>> + },
>>> + [CGT_HCR_TID3] = {
>>> + .index = HCR_EL2,
>>> + .value = HCR_TID3,
>>> + .mask = HCR_TID3,
>>> + .behaviour = BEHAVE_FORWARD_READ,
>>> + },
>>> + [CGT_HCR_IMO] = {
>>> + .index = HCR_EL2,
>>> + .value = HCR_IMO,
>>> + .mask = HCR_IMO,
>>> + .behaviour = BEHAVE_FORWARD_WRITE,
>>> + },
>>> + [CGT_HCR_FMO] = {
>>> + .index = HCR_EL2,
>>> + .value = HCR_FMO,
>>> + .mask = HCR_FMO,
>>> + .behaviour = BEHAVE_FORWARD_WRITE,
>>> + },
>>> + [CGT_HCR_TIDCP] = {
>>> + .index = HCR_EL2,
>>> + .value = HCR_TIDCP,
>>> + .mask = HCR_TIDCP,
>>> + .behaviour = BEHAVE_FORWARD_ANY,
>>> + },
>>> + [CGT_HCR_TACR] = {
>>> + .index = HCR_EL2,
>>> + .value = HCR_TACR,
>>> + .mask = HCR_TACR,
>>> + .behaviour = BEHAVE_FORWARD_ANY,
>>> + },
>>> + [CGT_HCR_TSW] = {
>>> + .index = HCR_EL2,
>>> + .value = HCR_TSW,
>>> + .mask = HCR_TSW,
>>> + .behaviour = BEHAVE_FORWARD_ANY,
>>> + },
>>> + [CGT_HCR_TPC] = { /* Also called TCPC when FEAT_DPB is implemented */
>>> + .index = HCR_EL2,
>>> + .value = HCR_TPC,
>>> + .mask = HCR_TPC,
>>> + .behaviour = BEHAVE_FORWARD_ANY,
>>> + },
>>> + [CGT_HCR_TPU] = {
>>> + .index = HCR_EL2,
>>> + .value = HCR_TPU,
>>> + .mask = HCR_TPU,
>>> + .behaviour = BEHAVE_FORWARD_ANY,
>>> + },
>>> + [CGT_HCR_TTLB] = {
>>> + .index = HCR_EL2,
>>> + .value = HCR_TTLB,
>>> + .mask = HCR_TTLB,
>>> + .behaviour = BEHAVE_FORWARD_ANY,
>>> + },
>>> + [CGT_HCR_TVM] = {
>>> + .index = HCR_EL2,
>>> + .value = HCR_TVM,
>>> + .mask = HCR_TVM,
>>> + .behaviour = BEHAVE_FORWARD_WRITE,
>>> + },
>>> + [CGT_HCR_TDZ] = {
>>> + .index = HCR_EL2,
>>> + .value = HCR_TDZ,
>>> + .mask = HCR_TDZ,
>>> + .behaviour = BEHAVE_FORWARD_ANY,
>>> + },
>>> + [CGT_HCR_TRVM] = {
>>> + .index = HCR_EL2,
>>> + .value = HCR_TRVM,
>>> + .mask = HCR_TRVM,
>>> + .behaviour = BEHAVE_FORWARD_READ,
>>> + },
>>> + [CGT_HCR_TLOR] = {
>>> + .index = HCR_EL2,
>>> + .value = HCR_TLOR,
>>> + .mask = HCR_TLOR,
>>> + .behaviour = BEHAVE_FORWARD_ANY,
>>> + },
>>> + [CGT_HCR_TERR] = {
>>> + .index = HCR_EL2,
>>> + .value = HCR_TERR,
>>> + .mask = HCR_TERR,
>>> + .behaviour = BEHAVE_FORWARD_ANY,
>>> + },
>>> + [CGT_HCR_APK] = {
>>> + .index = HCR_EL2,
>>> + .value = 0,
>>> + .mask = HCR_APK,
>>> + .behaviour = BEHAVE_FORWARD_ANY,
>>> + },
>>> + [CGT_HCR_NV] = {
>>> + .index = HCR_EL2,
>>> + .value = HCR_NV,
>>> + .mask = HCR_NV,
>>> + .behaviour = BEHAVE_FORWARD_ANY,
>>> + },
>>> + [CGT_HCR_NV_nNV2] = {
>>> + .index = HCR_EL2,
>>> + .value = HCR_NV,
>>> + .mask = HCR_NV | HCR_NV2,
>>> + .behaviour = BEHAVE_FORWARD_ANY,
>>> + },
>>> + [CGT_HCR_NV1_nNV2] = {
>>> + .index = HCR_EL2,
>>> + .value = HCR_NV | HCR_NV1,
>>> + .mask = HCR_NV | HCR_NV1 | HCR_NV2,
>>> + .behaviour = BEHAVE_FORWARD_ANY,
>>> + },
>> 
>> The declaration above seems to be a coarse control combination that could be
>> decomposed in the following, more readable, equivalent by adding a
>> combination of two MCBs
>> (eg. CGT_HCR_NV_NV1, CGT_HCR_NV_NV1_nNV2)
>> 
>>       [CGT_HCR_NV1] = {
>>               .index          = HCR_EL2,
>>               .value          = HCR_NV1,
>>               .mask           = HCR_NV1,
>>               .behaviour      = BEHAVE_FORWARD_ANY,
>>       },
>>       [CGT_HCR_NV1_nNV2] = {
>>               .index          = HCR_EL2,
>>               .value          = HCR_NV1,
>>               .mask           = HCR_NV1 | HCR_NV2,
>>               .behaviour      = BEHAVE_FORWARD_ANY,
>>       },
>> 
>> /* FEAT_NV and FEAT_NV2 */
>> MCB(CGT_HCR_NV_NV1, CGT_HCR_NV, CGT_HCR_NV1) 
>> 
>> /* FEAT_NV2 and HCR_EL2.NV2 is 0 behaves as FEAT_NV */
>> MCB(CGT_HCR_NV_NV1_nNV2, CGT_HCR_NV_nNV2, CGT_HCR_NV1_nNV2 )
> 
> This is not equivalent at all, as a MCB is a logical OR, not an AND.
> 

A logical OR as I would expect, which can be used recursively, meaning
IIUC that an MCB can contain other MCB ids, is this correct?

Therefore, the equivalent for the declared ‘CGT_HCR_NV1_nNV2’ would be

MCB(CGT_HCR_NV1_nNV2, CGT_HCR_NV_NV1, CGT_HCR_NV_NV1_nNV2) ?

I do not know what I missed.

>> On the above all the coarse HCR_EL2.{NV,NV1} traps are covered but not the
>> constrained unpredictable one when HCR_EL2.{NV,NV1} is {0,1} which traps in
>> two of its behaviours and doesn't trap on one.
> 
> The current approach makes it plain that HCR_EL2.NV==0 doesn't result
> in any trap forwarding, consistent with the current wording of
> architecture.
> 

Indeed but it could result in trap forwarding as an expected behaviour
in D8.11.4 of DDI0487J.a.

Thanks,
Miguel

> Thanks,
> 
> M.
> 
> -- 
> Without deviation from the norm, progress is not possible.
>