From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 9D6EEC4345F for ; Fri, 3 May 2024 15:29:33 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=exZs5mtf5XDZO3nRNON8b++t500vmAmNkMDxo9aYYAk=; b=EiOuD6+DKOjqyz FsGlDnbDI7+1cLMcDbY6AnhC/qo2QRQWgQVUTskhNNkudb33G/lbnTGO7raFqE53pf/GGiVHPXtBf sZe67oObx/HNCGTvqYCDBiCO5UyQhPc0qWzd9t7MUF1S+wOgmOaoNF+d402DDlJmNBaGSCSQ5zRoO N29WhRCzpNQp9stK/mMd8YBAUXdDCGSzx5QrEukeSSwotbhjV0+NdVj7+UX0NGwN5qbYXRulQwC6A sB3cKk8XHXphm39YfmCCkNqPrVwgM+sG66oMZQBOM6h6bOW4yVHxV2dqV3Xk2DQWupCwWwMUgswww QAbnrEZbeLi616thT7Tg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1s2uqf-0000000H2Lv-0D25; Fri, 03 May 2024 15:29:21 +0000 Received: from mail-wm1-x32b.google.com ([2a00:1450:4864:20::32b]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1s2uqc-0000000H2LF-2ksU for linux-arm-kernel@lists.infradead.org; Fri, 03 May 2024 15:29:20 +0000 Received: by mail-wm1-x32b.google.com with SMTP id 5b1f17b1804b1-41bff91ecdcso68855e9.1 for ; Fri, 03 May 2024 08:29:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1714750155; x=1715354955; darn=lists.infradead.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=JdeKXLmwOpYZ0RbFT9ZtQGKC2WTKqWCZd1KazXfzCOo=; b=IK+20uwh3XAodB1P3iNBvYIelH1Zd1MfI8tAcubcK4CGxDQJCOQRIOVP9XRvRtns3j woUrJ2QyPhbRyYmqGl2h/ArVaU2f6u/u3ayzufQW6cu+SqSL40cNPSYaaEF75LhT3q+a LBUTJtkDW3SQK/sULJKwtZzSPPsNm0c6o8hbWDprUHjozu4MHxYG37uKqSM8MMyH4nLi MOZpiAqduTL6XoI4jfHtB2Wj47Hjz5vX1EE6bLb4RfsgN8Pla6I+9O/HH3qPVUkWcezr 58hTAmpIMlxdbOmp+fnoju25CKVAb3q/B3uV6/t0z2iL3yg3kueQ4s9uMow+my7wQvZq +OFA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1714750155; x=1715354955; 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=JdeKXLmwOpYZ0RbFT9ZtQGKC2WTKqWCZd1KazXfzCOo=; b=cUd65drlmY0ReDiRrQBGWJeRahvdf2lcVVe1sRZMVQwtRHSmXxx1kPvVio5xr+57YL bQqQSJLBvEsKmSwXHU1291stE2LHSBog+caQzMTZOn1tpKUf9p6UDI9tn7hhvODx68+Q 6D3SY4EtL5VmpF3NwomfB/RLni+eJ1pesRx5+eeLkajV1xQNX4sePzFeAmFfS6/7VXgy TPBuGpFUE0ed1FFCwYb8yy0g9mCHHhhIRPRacyCM9uU/hV90j37byReuxx0ocbvLcV2/ xF0qy443HqHoOMHaiCVdS/YUmW/agOXDi90MGnlfIfkUUjKKYhsuAecggLkNfxNBivQk gyRw== X-Forwarded-Encrypted: i=1; AJvYcCVZ/r6WJ3Tfo7j3i1DdjML9uIA/o+IsVnOoMqU1pBsd74nTA2KvucFiaeTKBN2U+2nFaSwAvFq8IZN872ewR1nrYWYi0xy+GIgKIA1J79PuEh6TOwU= X-Gm-Message-State: AOJu0YwytWd5fQusKk8qgbo6XG6Hc1YR/McaVzjkknEwW0G0UXIfRzk2 Afo3JsLK4XHHWWUBIoyZdo5Us7eGMwG9VyuhZ/DIsS/k/gFez6nqj/s4p8jpHQ== X-Google-Smtp-Source: AGHT+IFjXoILf96ERxLCwkweSq+7qACC3MvxOQoxACDo+E2WIpBq6N8yorxEawyLEqjx1LD+zYl/Nw== X-Received: by 2002:a05:600c:46cf:b0:419:fa3c:fb46 with SMTP id 5b1f17b1804b1-41e1cf11b6dmr1529905e9.5.1714750154988; Fri, 03 May 2024 08:29:14 -0700 (PDT) Received: from google.com (216.131.76.34.bc.googleusercontent.com. [34.76.131.216]) by smtp.gmail.com with ESMTPSA id r12-20020a05600c458c00b0041bfa2171efsm5970563wmo.40.2024.05.03.08.29.13 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 03 May 2024 08:29:14 -0700 (PDT) Date: Fri, 3 May 2024 15:29:12 +0000 From: Sebastian Ene To: Will Deacon Cc: catalin.marinas@arm.com, james.morse@arm.com, jean-philippe@linaro.org, maz@kernel.org, oliver.upton@linux.dev, qperret@google.com, qwandor@google.com, sudeep.holla@arm.com, suzuki.poulose@arm.com, tabba@google.com, yuzenghui@huawei.com, lpieralisi@kernel.org, kvmarm@lists.linux.dev, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, kernel-team@android.com Subject: Re: [PATCH 1/4] KVM: arm64: Trap FFA_VERSION host call in pKVM Message-ID: References: <20240418163025.1193763-2-sebastianene@google.com> <20240418163025.1193763-3-sebastianene@google.com> <20240503143937.GA18656@willie-the-truck> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240503143937.GA18656@willie-the-truck> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240503_082918_722027_CFA9ABBF X-CRM114-Status: GOOD ( 49.67 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Fri, May 03, 2024 at 03:39:38PM +0100, Will Deacon wrote: Hello Will, > Hi Seb, > > On Thu, Apr 18, 2024 at 04:30:23PM +0000, Sebastian Ene wrote: > > The pKVM hypervisor initializes with FF-A version 1.0. Keep the > > supported version inside the host structure and prevent the host > > drivers from overwriting the FF-A version with an increased version. > > Without trapping the call, the host drivers can negotiate a higher > > version number with TEE which can result in a different memory layout > > described during the memory sharing calls. > > > > Signed-off-by: Sebastian Ene > > --- > > arch/arm64/kvm/hyp/nvhe/ffa.c | 43 ++++++++++++++++++++++++++++++++--- > > 1 file changed, 40 insertions(+), 3 deletions(-) > > > > diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c > > index 320f2eaa14a9..023712e8beeb 100644 > > --- a/arch/arm64/kvm/hyp/nvhe/ffa.c > > +++ b/arch/arm64/kvm/hyp/nvhe/ffa.c > > @@ -58,6 +58,7 @@ struct kvm_ffa_buffers { > > hyp_spinlock_t lock; > > void *tx; > > void *rx; > > + u32 ffa_version; > > }; > > Why should this be part of 'struct kvm_ffa_buffers'? The host, proxy and > Secure side will end up using the same version, so a simple global > variable would suffice, no? > I prefer keeping it here as we will have more clients in the future / different VMs and each one of them will have its own version and its own pair of buffers. > > /* > > @@ -640,6 +641,39 @@ static bool do_ffa_features(struct arm_smccc_res *res, > > return true; > > } > > > > +static void do_ffa_version(struct arm_smccc_res *res, > > + struct kvm_cpu_context *ctxt) > > +{ > > + DECLARE_REG(u32, ffa_req_version, ctxt, 1); > > + u32 current_version; > > + > > + hyp_spin_lock(&host_buffers.lock); > > Why do you need to take the lock for this? > Because we interpret the host buffer content based on the version that we end up setting here and each time we are accessing these buffers we are protected by this lock. > > + current_version = host_buffers.ffa_version; > > + if (FFA_MAJOR_VERSION(ffa_req_version) != FFA_MAJOR_VERSION(current_version)) { > > + res->a0 = FFA_RET_NOT_SUPPORTED; > > + goto unlock; > > + } > > We won't have probed the proxy if the Secure side doesn't support 1.x > so I think you should just do: > > if (FFA_MAJOR_VERSION(ffa_req_version) != 1) > ... > Ack. > > + /* > > + * If the client driver tries to downgrade the version, we need to ask > > + * first if TEE supports it. > > + */ > > + if (FFA_MINOR_VERSION(ffa_req_version) < FFA_MINOR_VERSION(current_version)) { > > Similarly here, I don't think 'current_version' is what we should expose. > Rather, we should be returning the highest version that the proxy > supports in the host, which is 1.0 at this point in the patch series. We already report the highest version that the proxy supports on line: `res->a0 = current_version;` 'current_version' is assigned during proxy initialization. This check allows us to downgrade the supported ffa_version if the Host requested it and only if TF-A supports it. > > > + arm_smccc_1_1_smc(FFA_VERSION, ffa_req_version, 0, > > + 0, 0, 0, 0, 0, > > + res); > > Hmm, I'm struggling to see how this is supposed to work per the spec. > The FF-A spec says: > > | ... negotiation of the version must happen before an invocation of > | any other FF-A ABI. I think that is a bit vague in my opinion but what I get is that the first call executed should always be the get version ff-a call. > > and: > > | Once the caller invokes any FF-A ABI other than FFA_VERSION, the > | version negotiation phase is complete. > | > | Once an FF-A version has been negotiated between a caller and a > | callee, the version may not be changed for the lifetime of the > | calling component. The callee must treat the negotiated version as > | the only supported version for any subsequent interactions with the > | caller.> > So by the time we get here, we've already settled on our version with > the Secure side and the host cannot downgrade. At this stage I think the spec didn't take into account that there can be a hypervisor in between. > > That's a bit rubbish if you ask me, but I think it means we'll have to > defer some of the proxy initialisation until the host calls FFA_VERSION, > at which point we'll need to negotiate a common version between the host, > the proxy and Secure. Once we've done that, our FFA_VERSION handler will > just return that negotiated version. We are already doing this when the ARM driver is built as an external module. If it is not as an external module and is builtin we have a bigger issue because it loads before pKVM at subsys_initcall. This means that we won't trap FFA_MAP* and other setup calls. > > Will > Thank you, Seb > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com. > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel