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

[Marc Zyngier <maz@kernel.org>]

Miguel,

On Sat, 12 Aug 2023 04:08:22 +0100,
Miguel Luis <miguel.luis@oracle.com> wrote:
> 
> Hi Marc,
> 
> > On 8 Aug 2023, at 11:46, Marc Zyngier <maz@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@redhat.com>
> > Signed-off-by: Marc Zyngier <maz@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.

> 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.

Thanks,

	M.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel