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 720D5D44D5C for ; Wed, 6 Nov 2024 13:42:53 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Type:MIME-Version: References:In-Reply-To:Subject:Cc:To:From:Message-ID:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=lkVF5Ck2aL5/nRoWWiSHMfNZ2x9Tn/8nkzKZBMBhoFw=; b=QBBBTFIx+zlvxZd8ney91FNINB Uqgyle30fhEZTL0p1CMtD8mZAlZ25Tb3bJ8qeZB6NueacgNlzXOcA3wlcmummBB7wFUgeKqQtKePB rDWmxAHaON892tqvWj5xEhMstMb4lNw3iaaXnxlxAfbCPDPFIm5RWiaSX3U4bmnSQKvr2ZrvHY7Zq EEoWdAeqok7LuXVznOWw69JSnvyK0aK+XL2JjLdoNmgfEPudNoHzSI4t0iyK57Ok3Xaw3RFqLLCzh YNWYdKTrVmWbCzM2RcoOjDTeNMkPDMPhYQip3/EI6uD6UEZt+WbXJRbfZixkUM8iWIeX2oxqzFzQr Exhy652g==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98 #2 (Red Hat Linux)) id 1t8gJ1-00000003See-2YS2; Wed, 06 Nov 2024 13:42:43 +0000 Received: from dfw.source.kernel.org ([2604:1380:4641:c500::1]) by bombadil.infradead.org with esmtps (Exim 4.98 #2 (Red Hat Linux)) id 1t8frK-00000003Mxd-45hh for linux-arm-kernel@lists.infradead.org; Wed, 06 Nov 2024 13:14:08 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id A6FCE5C0407; Wed, 6 Nov 2024 13:13:21 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 1C02DC4CECD; Wed, 6 Nov 2024 13:14:06 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1730898846; bh=MmGh5gnPG4ku4438t9wd6nhTmZITzHrRx9liC5G5jFQ=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=IDXVl+LEXd/wyx3XOjQDob0T+MiPN1VloT44CWnpCTwy/KdgfZd9pPNPsCNpzbAAW PD11WODvJst53qsb+lR5TlXq2F1GA3/ITB5zmGykjiO3RxEVxrSWiC2cn8tm+Yvu19 9mzV1vweS2GFOWjJVC4xeL6ULQVrq51jLfLNdtRUDKGl6ZR9rXKrz2Lq1dtm49toQp sXVcB1ejVB/Mb6qBFRexOmBqRnWckXxCxhmv74Dy1oOz2P6XWpYHN/a8Emif82+Y9L j7ZLlB9iQpLJGX0vuwWajcZgLa0W4myZOKmxtXmCpKQ9OVuReusJ3br36TSFtN0aZU uU3pWdb/WjnMQ== Received: from sofa.misterjones.org ([185.219.108.64] helo=goblin-girl.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1t8frH-00ANY6-Nt; Wed, 06 Nov 2024 13:14:03 +0000 Date: Wed, 06 Nov 2024 13:14:02 +0000 Message-ID: <86bjys1p45.wl-maz@kernel.org> From: Marc Zyngier To: Jing Zhang Cc: KVM , KVMARM , ARMLinux , Oliver Upton , Joey Gouly , Zenghui Yu , Suzuki K Poulose , Kunkun Jiang , Paolo Bonzini , Andre Przywara , Colton Lewis , Raghavendra Rao Ananta , Shusen Li Subject: Re: [PATCH v3 2/4] KVM: arm64: vgic-its: Clear DTE when MAPD unmaps a device In-Reply-To: <20241106083035.2813799-3-jingzhangos@google.com> References: <20241106083035.2813799-1-jingzhangos@google.com> <20241106083035.2813799-3-jingzhangos@google.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/29.4 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: jingzhangos@google.com, kvm@vger.kernel.org, kvmarm@lists.linux.dev, linux-arm-kernel@lists.infradead.org, oupton@google.com, joey.gouly@arm.com, yuzenghui@huawei.com, suzuki.poulose@arm.com, jiangkunkun@huawei.com, pbonzini@redhat.com, andre.przywara@arm.com, coltonlewis@google.com, rananta@google.com, lishusen2@huawei.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20241106_051407_127220_7D091E33 X-CRM114-Status: GOOD ( 31.58 ) 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: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Wed, 06 Nov 2024 08:30:33 +0000, Jing Zhang wrote: > > From: Kunkun Jiang > > vgic_its_save_device_tables will traverse its->device_list to > save DTE for each device. vgic_its_restore_device_tables will > traverse each entry of device table and check if it is valid. > Restore if valid. > > But when MAPD unmaps a device, it does not invalidate the > corresponding DTE. In the scenario of continuous saves > and restores, there may be a situation where a device's DTE > is not saved but is restored. This is unreasonable and may > cause restore to fail. This patch clears the corresponding > DTE when MAPD unmaps a device. > > Co-developed-by: Shusen Li > Signed-off-by: Shusen Li > Signed-off-by: Kunkun Jiang > Signed-off-by: Jing Zhang > --- > arch/arm64/kvm/vgic/vgic-its.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c > index 2381bc5ce544..7c57c7c6fbff 100644 > --- a/arch/arm64/kvm/vgic/vgic-its.c > +++ b/arch/arm64/kvm/vgic/vgic-its.c > @@ -1140,8 +1140,9 @@ static int vgic_its_cmd_handle_mapd(struct kvm *kvm, struct vgic_its *its, > u8 num_eventid_bits = its_cmd_get_size(its_cmd); > gpa_t itt_addr = its_cmd_get_ittaddr(its_cmd); > struct its_device *device; > + gpa_t gpa; > > - if (!vgic_its_check_id(its, its->baser_device_table, device_id, NULL)) > + if (!vgic_its_check_id(its, its->baser_device_table, device_id, &gpa)) > return E_ITS_MAPD_DEVICE_OOR; > > if (valid && num_eventid_bits > VITS_TYPER_IDBITS) > @@ -1161,8 +1162,17 @@ static int vgic_its_cmd_handle_mapd(struct kvm *kvm, struct vgic_its *its, > * The spec does not say whether unmapping a not-mapped device > * is an error, so we are done in any case. > */ > - if (!valid) > + if (!valid) { > + struct kvm *kvm = its->dev->kvm; > + int dte_esz = vgic_its_get_abi(its)->dte_esz; > + u64 val = 0; > + > + if (KVM_BUG_ON(dte_esz != sizeof(val), kvm)) > + return -EINVAL; I find it pretty odd to bug only in that case, and the sprinkling of these checks all over the place is horrible. I'm starting to wonder if we shouldn't simply wrap vgic_write_guest() and co to do the checking. > + > + vgic_write_guest_lock(kvm, gpa, &val, dte_esz); I'm thinking of something like: diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c index ba945ba78cc7d..d8e57aefcd3a5 100644 --- a/arch/arm64/kvm/vgic/vgic-its.c +++ b/arch/arm64/kvm/vgic/vgic-its.c @@ -1128,6 +1128,19 @@ static struct its_device *vgic_its_alloc_device(struct vgic_its *its, return device; } + +#define its_write_entry_lock(i, g, valp, t) \ + ({ \ + struct kvm *__k = (i)->dev->kvm; \ + int __sz = vgic_its_get_abi(i)->t; \ + int __ret = 0; \ + if (KVM_BUG_ON(__sz != sizeof(*(valp)), __k)) \ + __ret = -EINVAL; \ + else \ + vgic_write_guest_lock(__k, (g), (valp), __sz); \ + __ret; \ + }) + /* * MAPD maps or unmaps a device ID to Interrupt Translation Tables (ITTs). * Must be called with the its_lock mutex held. @@ -1140,8 +1153,9 @@ static int vgic_its_cmd_handle_mapd(struct kvm *kvm, struct vgic_its *its, u8 num_eventid_bits = its_cmd_get_size(its_cmd); gpa_t itt_addr = its_cmd_get_ittaddr(its_cmd); struct its_device *device; + gpa_t gpa; - if (!vgic_its_check_id(its, its->baser_device_table, device_id, NULL)) + if (!vgic_its_check_id(its, its->baser_device_table, device_id, &gpa)) return E_ITS_MAPD_DEVICE_OOR; if (valid && num_eventid_bits > VITS_TYPER_IDBITS) @@ -1161,8 +1175,10 @@ static int vgic_its_cmd_handle_mapd(struct kvm *kvm, struct vgic_its *its, * The spec does not say whether unmapping a not-mapped device * is an error, so we are done in any case. */ - if (!valid) - return 0; + if (!valid) { + u64 val = 0; + return its_write_entry_lock(its, gpa, &val, dte_esz); + } device = vgic_its_alloc_device(its, device_id, itt_addr, num_eventid_bits); which can be generalised everywhere (you can even extract the check and move it to an out-of-line helper as required). M. -- Without deviation from the norm, progress is not possible.