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:
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.