Re: [PATCH v9 13/39] KVM: arm64: Manage GCS registers for guests

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4609 bytes --]

On Wed, Jul 10, 2024 at 07:28:09PM +0100, Marc Zyngier wrote:
> Mark Brown <broonie@kernel.org> wrote:
> > On Wed, Jul 10, 2024 at 04:17:02PM +0100, Marc Zyngier wrote:

> > > > +	if (ctxt_has_gcs(ctxt)) {

> > > Since this is conditioned on S1PIE, it should be only be evaluated
> > > when PIE is enabled in the guest.

> > So make ctxt_has_gcs() embed a check of ctxt_has_s1pie()?

> No. I mean nest the whole thing *under* the check for S1PIE.

OK, increasing the level of nesting.  Got it.  Does that just apply for
the EL1 registers given that there's no _user S1PIE registers so no
existing check there?

Should we also be doing a similar thing for features that depend on TCR2
- currently that's just PIE but it'll grow?  Probably only when we get
more features rather than now since we don't currently check if the
guest has TCR2, just the system.

> > GCSCRE0_EL1 is for EL0 though, it ended up here mainly because it's an
> > _EL1 register and we are already context switching PIRE0_EL1 in the EL1
> > functions so it seemed consistent to follow the same approach for GCS.
> > The _el1 and _user save/restore functions are called from the same place
> > for both VHE and nVHE so the practical impact of the placement should be
> > minimal AFAICT.  Unlike PIRE0_EL1 GCSCRE0_EL1 only has an impact for
> > code runnning at EL0 so I can move it to the _user functions.

> Exactly. That's where it belongs, because we never execute EL0 while a
> vcpu is loaded. On the contrary, we can make use of a uaccess helper
> while a vcpu is loaded, and that makes a hell of a difference.

OK, to be clear here "it" is GCSCRE0_EL1, not GCSPR_EL1 and GCSCR_EL1
which are for EL1?

> And it makes a difference because it would allow the loading of
> EL0-specific context differently. We had this at some point, and it
> was a reasonable optimisation that we lost. I'm keen on bringing it
> back.

Ah, that'd be good - not only for the optimistation but also since at
the minute it's a bit unclear why there are separate EL0/1 functions.

> > > you want to make this register writable, here's the shopping list:

> > > https://lore.kernel.org/all/87ikxsi0v9.wl-maz@kernel.org/

> > In the linked mail you say you want to see all fields explicitly
> > handled, could you be more direct about what such explicit handling

> This emails enumerate, point after point, everything that needs to be
> done. I really cannot be clearer or more direct. This email is the
> clearer I can be, short of writing the code myself. And I have decided
> not to do it for once, unless I really need to. And as it turns out, I
> don't.

See below, to be clear the only bit I was querying here was:

| - you *must* handle *all* the fields described in that register. There
|   are 15 valid fields there, and I want to see all 15 fields being
|   explicitly dealt with.

TBH it'd probably good to have that whole list in the kernel somewhere.

> > would look like?  I see a number of examples in the existing code like:

> > 	ID_WRITABLE(ID_AA64ZFR0_EL1, ~ID_AA64ZFR0_EL1_RES0),

> This is clear: Everything is writable, and there are no bits here that
> are otherwise conditional or unsupported.

Ah, I think I see.  I would not have interpreted this as making
everything explicit, to me this makes all the writeable fields writeable
implicitly through them just not being mentioned.  For everything to be
explicit I would expect to see a direct, visible reference in the code
to every single field rather than something like we have here where some
of the fields are not mentioned directly.  The end result is an explicit
value but that's true for any use of ID_WRITABLE().

If my understanding is correct then were I writing the bit I quoted
above I'd probably just drop the "explicitly" from that bullet point due
to the handling of simple writable fields with ID_WRITABLE(), the key
point being that every field needs to be handled with the other points
enumerating the specific options for how each field might be handled.
Does my understanding sound correct?

> > which look to my eye very similar to the above, they do not visibliy
> > explictly enumerate every field in the registers and given that there's
> > a single mask specified it's not clear how that would look.  If
> > ID_WRITABLE() took separate read/write masks and combined them it'd be
> > more obvious but it's just not written that way.

> I don't really see what it would buy us, but never mind.

That was me trying to reconcile my understanding of you asking to make
everything explicit with the code as it is.  I suppose the advantage
would be documentation.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]