Re: [PATCH v3 14/18] KVM: arm64: nv: Add SW walker for AT S1 emulation

Marc Zyngier
2024-08-14T10:08:06Z

On 2024-08-13 11:05, Marc Zyngier wrote:
> In order to plug the brokenness of our current AT implementation,
> we need a SW walker that is going to... err.. walk the S1 tables
> and tell us what it finds.
>
> Of course, it builds on top of our S2 walker, and share similar
> concepts. The beauty of it is that since it uses kvm_read_guest(),
> it is able to bring back pages that have been otherwise evicted.
>
> This is then plugged in the two AT S1 emulation functions as
> a "slow path" fallback. I'm not sure it is that slow, but hey.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
> arch/arm64/kvm/at.c | 607 +++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 605 insertions(+), 2 deletions(-)

[...]

> +static u64 handle_at_slow(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
> +{
> + bool perm_fail, ur, uw, ux, pr, pw, px;
> + struct s1_walk_result wr = {};
> + struct s1_walk_info wi = {};
> + int ret, idx;
> +
> + ret = setup_s1_walk(vcpu, op, &wi, &wr, vaddr);
> + if (ret)
> + goto compute_par;
> +
> + if (wr.level == S1_MMU_DISABLED)
> + goto compute_par;
> +
> + idx = srcu_read_lock(&vcpu->kvm->srcu);
> +
> + ret = walk_s1(vcpu, &wi, &wr, vaddr);
> +
> + srcu_read_unlock(&vcpu->kvm->srcu, idx);
> +
> + if (ret)
> + goto compute_par;
> +
> + /* FIXME: revisit when adding indirect permission support */
> + /* AArch64.S1DirectBasePermissions() */
> + if (wi.regime != TR_EL2) {
> + switch (FIELD_GET(PTE_USER | PTE_RDONLY, wr.desc)) {
> + case 0b00:
> + pr = pw = true;
> + ur = uw = false;
> + break;
> + case 0b01:
> + pr = pw = ur = uw = true;
> + break;
> + case 0b10:
> + pr = true;
> + pw = ur = uw = false;
> + break;
> + case 0b11:
> + pr = ur = true;
> + pw = uw = false;
> + break;
> + }
> +
> + switch (wr.APTable) {
> + case 0b00:
> + break;
> + case 0b01:
> + ur = uw = false;
> + break;
> + case 0b10:
> + pw = uw = false;
> + break;
> + case 0b11:
> + pw = ur = uw = false;
> + break;
> + }
> +
> + /* We don't use px for anything yet, but hey... */
> + px = !((wr.desc & PTE_PXN) || wr.PXNTable || pw);

Annoying (but so far harmless) bug here: the last term should be 'uw',
and not 'pw'. It is a *userspace* writable permission that disables
privileged execution. 'pw' would only make sense if with WXN, and
that has nothing to do with AT at all.

I've fixed that locally.

M.
--
Jazz is not dead. It just smells funny...