Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 19 Aug 2022 18:02:39 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Andrew Jones <andrew.jones@linux.dev>
Cc:     Peter Gonda <pgonda@google.com>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org, marcorr@google.com,
        michael.roth@amd.com, thomas.lendacky@amd.com, joro@8bytes.org,
        mizhang@google.com, pbonzini@redhat.com, vannapurve@google.com
Subject: Re: [V3 10/11] KVM: selftests: Add ucall pool based implementation
Message-ID: <Yv/QPxeKczzmxR9H@google.com>
References: <20220810152033.946942-1-pgonda@google.com>
 <20220810152033.946942-11-pgonda@google.com>
 <20220816161350.b7x5brnyz5pyi7te@kamzik>
 <Yv5iKJbjW5VseagS@google.com>
 <20220818190514.ny77xpfwiruah6m5@kamzik>
 <Yv7LR8NMBMKVzS3Z@google.com>
 <20220819051725.6lgggz2ktbd35pqj@kamzik>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220819051725.6lgggz2ktbd35pqj@kamzik>
Precedence: bulk

On Fri, Aug 19, 2022, Andrew Jones wrote:
> On Thu, Aug 18, 2022 at 11:29:11PM +0000, Sean Christopherson wrote:
> > On Thu, Aug 18, 2022, Andrew Jones wrote:
> Dropping the only once-used uc_pool_idx() and adding the comment looks
> better, but I don't think there was a bug because uc == uc->hva.

uc == uc->hva for the host, but not for the guest.  From the guest's perspective,
"hva" is an opaque pointer that has no direct relation to "uc".

> 1) Doing ucall_init() at VM create time may be too early for Arm. Arm
> probes for an unmapped address for the MMIO address it needs, so it's
> best to do that after all mapping.

Argh.  I really hate the MMIO probing.  Is there really no arbitrary address that
selftests can carve out and simply say "thou shalt not create a memslot here"?

Or can we just say that it's always immediate after memslot0?  That would allow
us to delete the searching code in ARM's ucall_arch_init().

> 2) We'll need to change the sanity checks in Arm's get_ucall() to not
> check that the MMIO address == ucall_exit_mmio_addr since there's no
> guarantee that the exiting guest's address matches whatever is lingering
> in the host's version. We can either drop the sanity check altogether
> or try to come up with something else.

Ah, right.  This one at least is easy to handle.  If the address is per-VM, just
track the address.  If it's hardcoded (as a fix for #1) then, it's even simpler.

diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
index 8623c1568f97..74167540815b 100644
--- a/tools/testing/selftests/kvm/include/kvm_util_base.h
+++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
@@ -94,7 +94,8 @@ struct kvm_vm {
        vm_paddr_t pgd;
        vm_vaddr_t gdt;
        vm_vaddr_t tss;
-       vm_vaddr_t idt;
+       vm_paddr_t pgd;
+       vm_vaddr_t ucall_addr;
        vm_vaddr_t handlers;
        uint32_t dirty_ring_size;
        struct vm_memcrypt memcrypt;
diff --git a/tools/testing/selftests/kvm/lib/aarch64/ucall.c b/tools/testing/selftests/kvm/lib/aarch64/ucall.c
index b5d904f074b6..7f1d50dab0df 100644
--- a/tools/testing/selftests/kvm/lib/aarch64/ucall.c
+++ b/tools/testing/selftests/kvm/lib/aarch64/ucall.c
@@ -14,6 +14,8 @@ static vm_vaddr_t *ucall_exit_mmio_addr;

 static void ucall_set_mmio_addr(struct kvm_vm *vm, vm_vaddr_t val)
 {
+       vm->ucall_addr = val;
+
        atomic_sync_global_pointer_to_guest(vm, ucall_exit_mmio_addr, val);
 }

@@ -87,7 +89,7 @@ void *ucall_arch_get_ucall(struct kvm_vcpu *vcpu)
        struct kvm_run *run = vcpu->run;

        if (run->exit_reason == KVM_EXIT_MMIO &&
-           run->mmio.phys_addr == (uint64_t)ucall_exit_mmio_addr) {
+           run->mmio.phys_addr == vcpu->vm->ucall_addr) {
                TEST_ASSERT(run->mmio.is_write && run->mmio.len == sizeof(uint64_t),
                            "Unexpected ucall exit mmio address access");
                return (void *)(*((uint64_t *)run->mmio.data));