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

On Tue, 15 Aug 2023 16:35:09 +0100,
Miguel Luis [off-list ref] wrote:

quoted

quoted

quoted

+ [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?

We're now forbidding it, but even if we allowed it, it would still be
an OR, while you really need an AND.

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) ?

You have completely lost me. The only valid values we want to check
for at this stage are:

{NV}={1}
{NV,NV1}={1,1}
{NV,NV2}={1,0}
{NV,NV1,NV2}={1,1,0}

We can check any of these in one go. Why should we build something
that implement multiple behaviours in bizarre ways?

I do not know what I missed.

quoted

quoted

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.

Not in our implementation. We ignore NV1 if NV is 0 and behave as "For
all purposes other than reading back the value of the HCR_EL2.NV1 bit,
the implementation behaves as if HCR_EL2.{NV, NV1} is {0, 0}."

The point to understand is that we are implementing *ONE* of the
allowed behaviours. We don't have to support all of them.

	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