From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f51.google.com (mail-wr1-f51.google.com [209.85.221.51]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 5175E2C182 for ; Fri, 20 Dec 2024 15:14:41 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.221.51 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1734707683; cv=none; b=e2z57RMP8dXo9DXGPp8GQfKyjRAr/RZdhMNCzNRBtUkpyjV9QqPkMsVec0teqMPFUj2z8ys6dqwyolTk0/fl92N1A9vYVHjVQUvbZkYG1G0fyDWMkot0JunKIEd+RcWZVrPdKg3PbxXugvtSYZDMAPg3QL+dbtcMu6gM9kftlG0= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1734707683; c=relaxed/simple; bh=KfvyMmxFI8nm5OnFkvjaCBVSaSinWfbI/SB9mOMvufM=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=dxS4+kvrDVUBfv/S5Cm3Sd3i9GxnLrWwq8f38npASIk83axhmLUh8SyA30M38MDzEPCoFqOmAk4t+pNTsW6g4mIK3ZM53M45Llxb2MV5CfnY3RXsz20fkMpqvYPk28OBSPztBYqdU++NxaC4AHMWxvN3244NssjW5mFY/HfGZI8= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com; spf=pass smtp.mailfrom=google.com; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b=QtguznFt; arc=none smtp.client-ip=209.85.221.51 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=google.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="QtguznFt" Received: by mail-wr1-f51.google.com with SMTP id ffacd0b85a97d-38633b5dbcfso1990628f8f.2 for ; Fri, 20 Dec 2024 07:14:41 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1734707680; x=1735312480; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=i/FJ/BWMcAeSqIEo73tjEOakYKuswlMcLnlIzYTAvbU=; b=QtguznFt8hLk5c4atUvumRwBKzCg5VwywxkOdeWIqZtp/VP40gXKQGsnQU8Ush5giV adUaFERm3mQxiuDllPa7i9Xch8hlb6FBxJS5YjVfM1rOzxYCMa9W0fy4CDE/yiFOps3Y PDUQBWELN5zB1gkG9kE3183jS9rfkvdkKvaf/m++yID6TvQ5+za7M2UwYj5SltKn356h 9mE2v+b5lIc+qjHkTf/XpNYu8MavXdFCEFI5WzpLPWeuItGhGYOGmMCjfVIsiceR14/0 8Zia1rxzMrLe5n1VUG8FOkAFg4/0/mPqBMqbuXTB52nL7faaPIR2NVOz1Bf3NUtcgU+N Lsog== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1734707680; x=1735312480; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=i/FJ/BWMcAeSqIEo73tjEOakYKuswlMcLnlIzYTAvbU=; b=UNKVtcpq3NSbpclDHRCX4pstx2Di+I0iAzTppIhyoGedNCn4QPqxCyEc8kl2mG5Qfh nxaAUHxfZB0F2EQ4XGE86r44lYZuKC2M4btZqPlipvkca3YaVRlZU4/qptLxPPLFiI96 pHbjp8MC1q3iV5cerxkxma57GoLWGGEMrkTgPh6P07Eb0BqU2tdavQui8BV7w4Yj75+k jOauQZYQkOXb1UWb7LUZfXP9YMfrSUU+3sf8OVOxMubHT7KPaibyiUWqdFO7o61gaT22 EWQLnMfvs/3y9NBx8QK9JavdDEbQo9OxScDi+6bsuczd4n5V2PnERyOipvmUQ3JaKVhP EWeA== X-Forwarded-Encrypted: i=1; AJvYcCVVSWq6Cr3RA/OSbZTQOJeAL2ZirEOkzlcv5GwL9tcYYE7i0SvItumPSSzKlOa7XoKodXh2QoZX3gtylfY=@vger.kernel.org X-Gm-Message-State: AOJu0Yzt0d7vtLO/xTpHlJz4LjrNPcwznqr2jc9HCAoHlPzFW05wryQi 49lmyawPXCrygLIsutvn09f1qZB2zEAmdT4P7bmn6wXyEA8dklVqfj9d69LHow== X-Gm-Gg: ASbGncsPajriG7IDw+3VqIXt5S7o+0qw25SqFFKGwGIlZw+HFTkgk3BDQtAFc4kApu9 KluG3Io8OiQYl5wPmuyiEGs07ZXas3qLYCtvIRnr+bIps94pL+ZvCdVkTBwr/KTCcxCdulTM6bG vrVSyGKqC8dRvaY+o9ZjFnIf+j3OQogggLBvMeX8EUV/upjsXzY9F4FXNGzZS+KtnWBVzae+wAt RL/siTVGoREth/VNK6ll4nLg4/zbwKCA9owM2JdGY0oIFxr32cDj9JhyscWCogOV/Fjb2mqEowt 1gowQGWWZZNjifM= X-Google-Smtp-Source: AGHT+IEG5BXn0iGAPBX0qNzaXa7+CoRYPQ9C8lBuFf1IDtIo1tsZXMYQwfRblUjictWqaHhz0sRuhA== X-Received: by 2002:a05:6000:4a12:b0:385:df73:2f42 with SMTP id ffacd0b85a97d-38a221fa0cdmr2976447f8f.32.1734707679558; Fri, 20 Dec 2024 07:14:39 -0800 (PST) Received: from google.com (61.134.90.34.bc.googleusercontent.com. [34.90.134.61]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-aac0f05fd7csm185283966b.172.2024.12.20.07.14.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 20 Dec 2024 07:14:39 -0800 (PST) Date: Fri, 20 Dec 2024 15:14:36 +0000 From: Quentin Perret To: Will Deacon Cc: Marc Zyngier , Oliver Upton , Joey Gouly , Suzuki K Poulose , Zenghui Yu , Catalin Marinas , linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev, linux-kernel@vger.kernel.org Subject: Re: [PATCH] KVM: arm64: Selftest for pKVM transitions Message-ID: References: <20241129125800.992468-1-qperret@google.com> <20241220141321.GA26794@willie-the-truck> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20241220141321.GA26794@willie-the-truck> On Friday 20 Dec 2024 at 14:13:22 (+0000), Will Deacon wrote: > On Fri, Nov 29, 2024 at 12:58:00PM +0000, Quentin Perret wrote: > > We have recently found a bug [1] in the pKVM memory ownership > > transitions by code inspection, but it could have been caught with a > > test. > > > > Introduce a boot-time selftest exercising all the known pKVM memory > > transitions and importantly checks the rejection of illegal transitions. > > Thanks for doing this! > > > > > The new test is hidden behind a new Kconfig option separate from > > CONFIG_EL2_NVHE_DEBUG on purpose as that has side effects on the > > transition checks ([1] doesn't reproduce with EL2 debug enabled). > > Hmm. What does a test failure look like without CONFIG_EL2_NVHE_DEBUG > enabled? Do we still get file:line information? Nope, sadly we don't, but if you get a failure after turning on that config then you can at least be confident there is a problem with your code :-) As discussed in the other thread, once we get the hyp state into the vmemmap (I've got patches that do exactly that, happy to send them out) we could just plain remove the 'skip_pgtable_check()' logic, and then I'm not opposed to merging CONFIG_PKVM_SELFTESTS and CONFIG_EL2_NVHE_DEBUG together as the latter won't have the same side effects. > > +void pkvm_ownership_selftest(void) > > +{ > > + void *virt = hyp_alloc_pages(&host_s2_pool, 0); > > + u64 phys, size, pfn; > > + > > + WARN_ON(!virt); > > + selftest_page = hyp_virt_to_page(virt); > > + selftest_page->refcount = 0; > > + > > + size = PAGE_SIZE << selftest_page->order; > > + phys = hyp_virt_to_phys(virt); > > + pfn = hyp_phys_to_pfn(phys); > > + > > + selftest_state.host = PKVM_NOPAGE; > > + selftest_state.hyp = PKVM_PAGE_OWNED; > > + assert_page_state(); > > + assert_transition_res(-EPERM, __pkvm_host_donate_hyp, pfn, 1); > > + assert_transition_res(-EPERM, __pkvm_host_share_hyp, pfn); > > + assert_transition_res(-EPERM, __pkvm_host_unshare_hyp, pfn); > > + assert_transition_res(-EPERM, __pkvm_host_share_ffa, pfn, 1); > > + assert_transition_res(-EPERM, __pkvm_host_unshare_ffa, pfn, 1); > > + assert_transition_res(-EPERM, hyp_pin_shared_mem, virt, virt + size); > > + > > + selftest_state.host = PKVM_PAGE_OWNED; > > + selftest_state.hyp = PKVM_NOPAGE; > > + assert_transition_res(0, __pkvm_hyp_donate_host, pfn, 1); > > + assert_transition_res(-EPERM, __pkvm_hyp_donate_host, pfn, 1); > > + assert_transition_res(-EPERM, hyp_pin_shared_mem, virt, virt + size); > > Should we recheck the unshare transitions here? Yep, sounds like a good idea. > > + selftest_state.host = PKVM_PAGE_SHARED_OWNED; > > + selftest_state.hyp = PKVM_PAGE_SHARED_BORROWED; > > + assert_transition_res(0, __pkvm_host_share_hyp, pfn); > > + assert_transition_res(-EPERM, __pkvm_host_share_hyp, pfn); > > + assert_transition_res(-EPERM, __pkvm_host_donate_hyp, pfn, 1); > > + assert_transition_res(-EPERM, __pkvm_host_share_ffa, pfn, 1); > > We could check unshare_ffa here. Sadly no, unshare_ffa() isn't super smart, it'll only check that a page is SHARED_OWNED from the host's PoV and turn it to PAGE_OWNED. So we'd break the state in this case :/ > > + assert_transition_res(-EPERM, __pkvm_hyp_donate_host, pfn, 1); > > + > > + assert_transition_res(0, hyp_pin_shared_mem, virt, virt + size); > > Is it worth trying an extra pin + unpin? You mean doing pin > pin > unpin > sanity checks > unpin ? I guess that would test that we can indeed stack the pins, so sounds like a good idea too. > > + WARN_ON(!hyp_page_count(virt)); > > Can we assert an exact value (e.g. count == 1)? Sure. > > + assert_transition_res(-EBUSY, __pkvm_host_unshare_hyp, pfn); > > + assert_transition_res(-EPERM, __pkvm_host_share_hyp, pfn); > > + assert_transition_res(-EPERM, __pkvm_host_donate_hyp, pfn, 1); > > + assert_transition_res(-EPERM, __pkvm_host_share_ffa, pfn, 1); > > unshare_ffa again here. Same as above. > > + assert_transition_res(-EPERM, __pkvm_hyp_donate_host, pfn, 1); > > + > > + hyp_unpin_shared_mem(virt, virt + size); > > + assert_page_state(); > > + WARN_ON(hyp_page_count(virt)); > > + assert_transition_res(-EPERM, __pkvm_host_share_hyp, pfn); > > + assert_transition_res(-EPERM, __pkvm_host_donate_hyp, pfn, 1); > > + assert_transition_res(-EPERM, __pkvm_host_share_ffa, pfn, 1); > > + assert_transition_res(-EPERM, __pkvm_hyp_donate_host, pfn, 1); > > Is this block actually testing anything new? Once we've asserted the page > state and checked the refcount, the transitions seem redundant to me. The idea was to check if the previous transitions had other sad effects not visible through the pair that would cause problems. But I guess that is a bit far fetched so happy to remove that part. > > + selftest_state.host = PKVM_PAGE_OWNED; > > + selftest_state.hyp = PKVM_NOPAGE; > > + assert_transition_res(0, __pkvm_host_unshare_hyp, pfn); > > + > > + selftest_state.host = PKVM_PAGE_SHARED_OWNED; > > + selftest_state.hyp = PKVM_NOPAGE; > > + assert_transition_res(0, __pkvm_host_share_ffa, pfn, 1); > > + assert_transition_res(-EPERM, __pkvm_host_share_ffa, pfn, 1); > > + assert_transition_res(-EPERM, __pkvm_host_donate_hyp, pfn, 1); > > + assert_transition_res(-EPERM, __pkvm_host_share_hyp, pfn); > > + assert_transition_res(-EPERM, __pkvm_host_unshare_hyp, pfn); > > + assert_transition_res(-EPERM, __pkvm_hyp_donate_host, pfn, 1); > > + assert_transition_res(-EPERM, hyp_pin_shared_mem, virt, virt + size); > > + > > + selftest_state.host = PKVM_PAGE_OWNED; > > + selftest_state.hyp = PKVM_NOPAGE; > > + assert_transition_res(0, __pkvm_host_unshare_ffa, pfn, 1); > > Try it twice? Sg. Thanks! Quentin