From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 2E086145FE4 for ; Thu, 13 Jun 2024 14:44:09 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1718289850; cv=none; b=S+TMQ6lOwDrFfB5jUmmDG2esdycAuhWItCtFM0DJA0XveXhz+hz4vHLwFdzLW6lsrRj5ajANqz7ZJOZNTW0afZtIfFdfRclfGLYuLRtQdA+L87xgmqwgeQI2Y/3bjTdOzc0LUDCQJx5D4HfGIEV2Jryn3dwpxWgeuRSJhgpunhg= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1718289850; c=relaxed/simple; bh=CQgcUN37V00aFSaArAlD/UsaQMLBynNNZwRe1t2wcog=; h=Date:Message-ID:From:To:Cc:Subject:In-Reply-To:References: MIME-Version:Content-Type; b=SqBUCeqxt2lrUC5Ht+gbdfYRkb0wY5Fi8jKIpruZT4tTHym28Rq9f2ZePVzVRR65B4RNiEjTIUmsRkH1RoWS6uhGgOHOltpXHf5Lsn4f1d5LcE0xkAwRLzLQAu6LcslK/UrPo2uihOykx1owhTkTpTFlXYktK+hXiKyIlKipjBA= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=YfJyOtgR; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="YfJyOtgR" Received: by smtp.kernel.org (Postfix) with ESMTPSA id A5E51C2BBFC; Thu, 13 Jun 2024 14:44:09 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1718289849; bh=CQgcUN37V00aFSaArAlD/UsaQMLBynNNZwRe1t2wcog=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=YfJyOtgRWgtTMT8vumFIaG/ieV0Yga113xw6owxq4vgc8mvWhFYHE62X8aF/bsWvD IyFOSI/clfkT51A+kPFxFcfr7I+ZorEviStc4tDE4gZfPh6hnvJilIOdMeYZ4In0GP Nip3vFJlRylgIE9oCa16oZ5NhgoiIjp71de0wZqJ4TWy+UmhdJ7ytQT8P337tzsHCt SMYQVm5tIrk51Lj6SVSFpJUymmjVnf4Aq7pdgNha6mbRdorRKbSF6XLiRYXjTHIpgf ge9TSwKQbKXWZy8pzPTN5cNXCAP9AOhYqWPfksXkxbADxCK4bc8K8o2NSAdjbKRvvW yL9LaPf0k1bNA== 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 1sHlgN-003arf-Ea; Thu, 13 Jun 2024 15:44:07 +0100 Date: Thu, 13 Jun 2024 15:44:06 +0100 Message-ID: <86v82ckimh.wl-maz@kernel.org> From: Marc Zyngier To: Kunkun Jiang Cc: Oliver Upton , James Morse , Suzuki K Poulose , Zenghui Yu , Catalin Marinas , Will Deacon , Randy Dunlap , , , , , Eric Auger Subject: Re: [PATCH] KVM: arm64: vgic-its: clear dte when mapd unmaps a device In-Reply-To: <20240613093811.710-1-jiangkunkun@huawei.com> References: <20240613093811.710-1-jiangkunkun@huawei.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.2 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO) Precedence: bulk X-Mailing-List: kvmarm@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: 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: jiangkunkun@huawei.com, oliver.upton@linux.dev, james.morse@arm.com, suzuki.poulose@arm.com, yuzenghui@huawei.com, catalin.marinas@arm.com, will@kernel.org, rdunlap@infradead.org, linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev, wanghaibin.wang@huawei.com, lishusen2@huawei.com, eauger@redhat.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false On Thu, 13 Jun 2024 10:38:11 +0100, Kunkun Jiang wrote: > > 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 devices, we do not invalidate the > corresponding dte. In the scenario of continuous save > and restore, 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. > > Singed-off-by: Shusen Li ^^^^^^^^^^^^^ This isn't a valid tag! > Signed-off-by: Kunkun Jiang Something is wrong. Either Shusen Li is the author and you are merely posting it, in which case there should be a 'From' line in the commit message, or you are co-authors and there should be a 'Co-developed-by' right after Shusen Li's SoB. Please read Documentation/process/submitting-patches.rst for the details. > --- > arch/arm64/kvm/vgic/vgic-its.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c > index 40bb43f20bf3..dd5fba1e8ba3 100644 > --- a/arch/arm64/kvm/vgic/vgic-its.c > +++ b/arch/arm64/kvm/vgic/vgic-its.c > @@ -1140,8 +1140,12 @@ 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; > + const struct vgic_its_abi *abi; > + int dte_esz; > + u64 val; Aside from 'gpa', all the extra variables should be moved into the !valid block. And it would be nicer if this variable was called something more descriptive. > > - 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 +1165,13 @@ 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) { > + abi = vgic_its_get_abi(its); > + dte_esz = abi->dte_esz; > + val = cpu_to_le64(0); > + vgic_write_guest_lock(kvm, gpa, &val, dte_esz); This is just a bit wrong: - you store 0 in a u64 after having (pointlessly) converted it to le64 - you write 'dte_esz' bytes into guest memory, but you don't have any check whether this is *larger* than the u64 you have passed Similar issue exists in all the vgic_its_save_*() functions, calling for a generic fix, as we're just lucky to never have needed a new ABI so far. At the very least a check that we deal with data that is exactly 8 bytes. Also, is the device table the only one that is subject to this 'reload' problem? How about the Collection table? Thanks, M. -- Without deviation from the norm, progress is not possible.