On Wed, 10 Jul 2024 18:16:46 +0100, Mark Brown broonie@kernel.org wrote:
[1 <text/plain; us-ascii (7bit)>] 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()?
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@ke...
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:
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.