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

[Marc Zyngier <maz@kernel.org>]

On Tue, 15 Aug 2023 16:35:09 +0100,
Miguel Luis <miguel.luis@oracle.com> wrote:
> >>> + [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.
> 
> >> 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