Hi Marc,
> On 15 Aug 2023, at 10:39, Marc Zyngier <maz@xxxxxxxxxx> wrote:
>
> Miguel,
>
> On Sat, 12 Aug 2023 04:08:22 +0100,
> Miguel Luis <miguel.luis@xxxxxxxxxx> wrote:
>>
>> Hi Marc,
>>
>>> On 8 Aug 2023, at 11:46, Marc Zyngier <maz@xxxxxxxxxx> wrote:
>>>
>>> Describe the HCR_EL2 register, and associate it with all the sysregs
>>> it allows to trap.
>>>
>>> Reviewed-by: Eric Auger <eric.auger@xxxxxxxxxx>
>>> Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
>>> ---
>>> 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.
>
