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

[Mark Brown <broonie@kernel.org>]

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

On Wed, Jul 10, 2024 at 04:17:02PM +0100, Marc Zyngier wrote:
> Mark Brown <broonie@kernel.org> 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()?

> > +		ctxt_sys_reg(ctxt, GCSPR_EL1)	= read_sysreg_el1(SYS_GCSPR);
> > +		ctxt_sys_reg(ctxt, GCSCR_EL1)	= read_sysreg_el1(SYS_GCSCR);
> > +		ctxt_sys_reg(ctxt, GCSCRE0_EL1)	= read_sysreg_s(SYS_GCSCRE0_EL1);

> Why is this part of the EL1 context? It clearly only matters to EL0
> execution, so it could be switched in load/put on nVHE as well. And
> actually, given that the whole thing is strictly for userspace, why do
> we switch *anything* eagerly at all?

GCS can also be used independently at EL1 (and EL2 for that matter),
it's not purely for userspace even though this series only implements
use in userspace.  GCSPR_EL1 and GCSCR_EL1 control the use of GCS at
EL1, not EL0, and the guest might be using GCS at EL1 even if the host
doesn't.

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.

TBH I'm not following your comments about switching eagerly too here at
all, where would you expect to see the switching done?  You've said
something along these lines before which prompted me to send a patch to
only save the S1PIE registers if they'd been written to which you were
quite reasonably not happy with given the extra traps it would cause:

   https://lore.kernel.org/r/20240301-kvm-arm64-defer-regs-v1-1-401e3de92e97@kernel.org

but I'm at a loss as to how to make things less eager otherwise.

> > @@ -2306,7 +2323,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> >  		   ID_AA64PFR0_EL1_GIC |
> >  		   ID_AA64PFR0_EL1_AdvSIMD |
> >  		   ID_AA64PFR0_EL1_FP), },
> > -	ID_SANITISED(ID_AA64PFR1_EL1),
> > +	ID_WRITABLE(ID_AA64PFR1_EL1, ~(ID_AA64PFR1_EL1_RES0 |
> > +				       ID_AA64PFR1_EL1_BT)),

> I don't know what you're trying to do here, but that's not right. If
> you want to make this register writable, here's the shopping list:

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

Yes, trying to make things writable.  I do see we need to exclude more
bits there and I'm not clear why I excluded BTI, looks like I forgot to
add a TODO comment at some point and finish that off.  Sorry about that.

In the linked mail you say you want to see all fields explicitly
handled, could you be more direct about what such explicit handling
would look like?  I see a number of examples in the existing code like:

	ID_WRITABLE(ID_AA64ZFR0_EL1, ~ID_AA64ZFR0_EL1_RES0),

	ID_WRITABLE(ID_AA64ISAR0_EL1, ~ID_AA64ISAR0_EL1_RES0),
	ID_WRITABLE(ID_AA64ISAR1_EL1, ~(ID_AA64ISAR1_EL1_GPI |
					ID_AA64ISAR1_EL1_GPA |
					ID_AA64ISAR1_EL1_API |
					ID_AA64ISAR1_EL1_APA)),

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.

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