Re: [PATCH v8 13/38] KVM: arm64: Manage GCS registers for guests

[Mark Brown <broonie@kernel.org>]

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

On Mon, Feb 05, 2024 at 03:34:05PM +0000, Marc Zyngier wrote:
> Mark Brown <broonie@kernel.org> wrote:
> > On Mon, Feb 05, 2024 at 09:46:16AM +0000, Marc Zyngier wrote:

> > > We have had this discussion in the past. This must be based on the
> > > VM's configuration. Guarding the check with the host capability is a
> > > valuable optimisation, but that's nowhere near enough. See the series
> > > that I have posted on this very subject (you're on Cc), but you are
> > > welcome to invent your own mechanism in the meantime.

> > Right, which postdates the version you're replying to and isn't merged
> > yet - the current code was what you were asking for at the time.

> v1 and v2 predate it. And if the above is what I did ask, then I must
> have done a very poor job of explaining what was required. For which I
> apologise profusely.

To be clear it's what was asked for prior to the switch to the
forthcoming switch to the parsing idregs scheme, I haven't pulled in
your idregs work yet since it's being rapidly iterated and this is an
already large series with dependencies.

> > I'm
> > expecting to update all these feature series to work with that once it
> > gets finalised and merged but it's not there yet, I do see I forgot to
> > put a note in v9 about that like I did for dpISA - sorry about that, I
> > was too focused on the clone3() rework when rebasing onto the new
> > kernel.

> > This particular series isn't going to get merged for a while yet anyway
> > due to the time it'll take for userspace testing, I'm expecting your
> > series to be in by the time it becomes an issue.

> Right. Then I'll ignore it for the foreseeable future.

Actually now I think about it would you be open to merging the guest
context switching bit without the rest of the series (pending me fixing
the issues you raise of course)?  If so I'll split that bit out in the
hope that we can reduce the size of the series and CC list for the
userspace support which I imagine would make people a bit happier.

> > > > +		write_sysreg_s(ctxt_sys_reg(ctxt, GCSCRE0_EL1),
> > > > +			       SYS_GCSCRE0_EL1);
> > > > +	}

> > > For the benefit of the unsuspecting reviewers, and in the absence of a
> > > public specification (which the XML drop isn't), it would be good to
> > > have the commit message explaining the rationale of what gets saved
> > > when.

> > What are you looking for in terms of rationale here?  The KVM house
> > style is often very reliant on reader context so it would be good to
> > know what considerations you'd like to see explicitly addressed.

> Nothing to do with style, everything to do with substance: if nothing

The style I'm referring to there is the style for documentation.

> in the host kernel makes any use of these registers, why are they
> eagerly saved/restored on nVHE/hVHE? I'm sure you have a reason for
> it, but it isn't that obvious. Because these two modes need all the
> help they can get in terms of overhead reduction.

Ah, I see - yes, they should probably be moved somewhere else.  Though
I'm not clear why some of the other registers that we're saving and
restoring in the same place are being done eagerly?  The userspace
TPIDRs stand out for example, they're in taken care of in
__sysreg_save_user_state() which is called in the same paths.  IIRC my
thinking there was something along the lines of "this is where we save
and restore everything else that's just a general system register, I
should be consistent".

Am I right in thinking kvm_arch_vcpu_load()/_put() would make sense?
Everything in there currently looked like it was there more due to doing
something more complex than simple register save/restore and we weren't
worrying too much about what was going on with just the sysregs.

> > These
> > registers shouldn't do anything when we aren't running the guest so
> > they're not terribly ordering sensitive, the EL2 ones will need a bit
> > more consideration in the face of nested virt.

> The EL2 registers should follow the exact same pattern, specially once
> you fix the VNCR bugs I pointed out.

Great, that's what I'd thought thanks - I hadn't checked yet.

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