On Wed, 10 Jul 2024 18:16:46 +0100, Mark Brown <broonie@xxxxxxxxxx> wrote: > > [1 <text/plain; us-ascii (7bit)>] > On Wed, Jul 10, 2024 at 04:17:02PM +0100, Marc Zyngier wrote: > > Mark Brown <broonie@xxxxxxxxxx> 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. > > > > + 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. 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. 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. > > 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@xxxxxxxxxx > > 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@xxxxxxxxxx/ > > 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 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. > 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. > > ID_WRITABLE(ID_AA64ISAR0_EL1, ~ID_AA64ISAR0_EL1_RES0), Same thing. > ID_WRITABLE(ID_AA64ISAR1_EL1, ~(ID_AA64ISAR1_EL1_GPI | > ID_AA64ISAR1_EL1_GPA | > ID_AA64ISAR1_EL1_API | > ID_AA64ISAR1_EL1_APA)), This one needs fixing because of LS64, and I have an in-progress series for it. > 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. M. -- Without deviation from the norm, progress is not possible.