[Paolo, can you please take a look at this as well?]
On 11.05.23 23:15, Sean Christopherson wrote:
> On Mon, May 08, 2023, Mathias Krause wrote:
>> This is a backport of the CR0.WP KVM series[1] to Linux v5.15. It
>> differs from the v6.1 backport as in needing additional prerequisite
>> patches from Lai Jiangshan (and fixes for those) to ensure the
>> assumption it's safe to let CR0.WP be a guest owned bit still stand.
>
> NAK.
>
> The CR0.WP changes also very subtly rely on commit 2ba676774dfc ("KVM: x86/mmu:
> cleanup computation of MMU roles for two-dimensional paging"), which hardcodes
> WP=1 in the mmu role.
Well, that commit has the MMU role split into two (mmu_role and
cpu_role) which was not the case for 5.15 and below. In fact, that split
was more confusing than helpful, so commit 7a458f0e1ba1 ("KVM: x86/mmu:
remove extended bits from mmu_role, rename field") /degraded/ mmu_role
to root_role and made clear that bit test helpers like is_cr0_wp() look
at cpu_role instead. In that regard, the backport should be equivalent
to what we have in 6.4, as it's using mmu_role for the older kernels
instead of cpu_role (which does not exist there yet).
> Without that, KVM will end up in a weird state when
> reinitializing the MMU context without reloading the root, as KVM will effectively
> change the role of an active root. E.g. child pages in the legacy MMU will have
> a mix of WP=0 and WP=1 in their role.
But does that really matter? Or, asked differently, don't we have that
very same situation for 6.4 with cpu_role.base.cr0_wp being the bit
looked at and not root_role.cr0_wp?
Either way, with EPT this should only matter if emulation is required
and patch 8 makes sure we'll use the proper value of CR0.WP prior to
starting the guest page table walk by refreshing the relevant cr0_wp
bit. Or am I missing something?
>
> The inconsistency may or may not cause functional problems (I honestly don't know),
> but this missed dependency is exactly the type of problem that I am/was worried
> about with respect to backporting these changes all the way to 5.15.
While doing the backports I carefully looked at the changes and
differences between the trees and, honestly, I don't think I missed this
dependency as I did account for the mmu_role->{cpu,root}_role split (or
rather unification regarding the backport) as explained above.
> I'm simply
> not comfortable backporting these changes due to the number of modifications and
> enhancements that we've made to the TDP MMU, and to KVM's MMU handling in general,
> between 5.15 and 6.1.
I understand your concerns, fiddling with the MMU implementation is not
easy at all, as it has so many combinations to keep in mind and quite
some implicit assumptions throughout the code. Moreover, I'm a newcomer
to this part of the kernel. However, I tried very hard to look at the
changes for the individual backports and double- or even triple-checked
the code to make sure the changes are still consistent with the rest of
the code base. But I'm just a human, so I might have missed something,
but a vague bad feeling doesn't convince me that I did something wrong.
Less so, as KUT supports me in not having messed up things too badly.
I know your backlog is huge and your review time is a scarce resource.
But could you or Paolo please take another look?
Thanks,
Mathias
