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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 560FCC433EF for ; Tue, 26 Apr 2022 17:35:00 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1350684AbiDZRiG (ORCPT ); Tue, 26 Apr 2022 13:38:06 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35584 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1349305AbiDZRiF (ORCPT ); Tue, 26 Apr 2022 13:38:05 -0400 Received: from sin.source.kernel.org (sin.source.kernel.org [145.40.73.55]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id E3E529A99B for ; Tue, 26 Apr 2022 10:34:56 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by sin.source.kernel.org (Postfix) with ESMTPS id D6351CE1C7D for ; Tue, 26 Apr 2022 17:34:54 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id EFDC6C385A4; Tue, 26 Apr 2022 17:34:52 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1650994493; bh=QlCeVmaTIzYpkU1ozYLeC73MQquWV/1ULsequJfZYUg=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=mV2OwYADBWSDYyJG3uHV+/jSthN2FqaulMSTWfZrTPipxaRZRj01KaCghXV0H2kgA TdA9zmAFWJF4YLclD++12Zg7q/uHWg430RNNZ6es9CGu8PpbT8DzOrO1fRuNP3i4rO ArtW59gnj8AqVFJcDEBEyAN3uYk0+O+D1QSXYmFxZkcOp3hxQ96uL/npAyWnhby5uF SU+YpZlJpcRyCVqvy0jrdGsMHZOtqYDG2XBwgeR3jiqnt2DCQPfzV2jszQStYJPdO0 YxEPnl4+XE6zkLia2lF82/4Sl4ifPjHrylUUiqF1LGvpPHMFccawXob5mGEAC7/rTF CVi0HCt/juQWQ== Received: from sofa.misterjones.org ([185.219.108.64] helo=why.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1njP5O-0079Ta-AF; Tue, 26 Apr 2022 18:34:50 +0100 Date: Tue, 26 Apr 2022 18:34:49 +0100 Message-ID: <8735hzague.wl-maz@kernel.org> From: Marc Zyngier To: Ricardo Koller Cc: kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu, pbonzini@redhat.com, andre.przywara@arm.com, drjones@redhat.com, alexandru.elisei@arm.com, eric.auger@redhat.com, oupton@google.com, reijiw@google.com, pshier@google.com Subject: Re: [PATCH 1/4] KVM: arm64: vgic: Check that new ITEs could be saved in guest memory In-Reply-To: References: <20220425185534.57011-1-ricarkol@google.com> <20220425185534.57011-2-ricarkol@google.com> <871qxkcws3.wl-maz@kernel.org> 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/27.1 (x86_64-pc-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: ricarkol@google.com, kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu, pbonzini@redhat.com, andre.przywara@arm.com, drjones@redhat.com, alexandru.elisei@arm.com, eric.auger@redhat.com, oupton@google.com, reijiw@google.com, pshier@google.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org On Tue, 26 Apr 2022 17:21:07 +0100, Ricardo Koller wrote: > > On Tue, Apr 26, 2022 at 05:07:40AM +0100, Marc Zyngier wrote: > > On Mon, 25 Apr 2022 19:55:31 +0100, > > Ricardo Koller wrote: > > > > > > A command that adds an entry into an ITS table that is not in guest > > > memory should fail, as any command should be treated as if it was > > > actually saving entries in guest memory (KVM doesn't until saving). > > > Add the corresponding check for the ITT when adding ITEs. > > > > > > Signed-off-by: Ricardo Koller > > > --- > > > arch/arm64/kvm/vgic/vgic-its.c | 34 ++++++++++++++++++++++++++++++++++ > > > 1 file changed, 34 insertions(+) > > > > > > diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c > > > index 2e13402be3bd..d7c1a3a01af4 100644 > > > --- a/arch/arm64/kvm/vgic/vgic-its.c > > > +++ b/arch/arm64/kvm/vgic/vgic-its.c > > > @@ -976,6 +976,37 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id, > > > return ret; > > > } > > > > > > +/* > > > + * Check whether an event ID can be stored in the corresponding Interrupt > > > + * Translation Table, which starts at device->itt_addr. > > > + */ > > > +static bool vgic_its_check_ite(struct vgic_its *its, struct its_device *device, > > > + u32 event_id) > > > +{ > > > + const struct vgic_its_abi *abi = vgic_its_get_abi(its); > > > + int ite_esz = abi->ite_esz; > > > + gpa_t gpa; > > > + gfn_t gfn; > > > + int idx; > > > + bool ret; > > > + > > > + /* max table size is: BIT_ULL(device->num_eventid_bits) * ite_esz */ > > > + if (event_id >= BIT_ULL(device->num_eventid_bits)) > > > + return false; > > > > We have already checked this condition, it seems. > > > > > + > > > + gpa = device->itt_addr + event_id * ite_esz; > > > + gfn = gpa >> PAGE_SHIFT; > > > + > > > + idx = srcu_read_lock(&its->dev->kvm->srcu); > > > + ret = kvm_is_visible_gfn(its->dev->kvm, gfn); > > > + srcu_read_unlock(&its->dev->kvm->srcu, idx); > > > + return ret; > > > > Why should we care? If the guest doesn't give us the memory that is > > required, that's its problem. > > The issue is that if the guest does that, then the pause will fail and > we won't be able to migrate the VM. This series objective is to help > with failed migrations due to the ITS. This commit tries to do it by > avoiding them. But the guest is buggy, isn't it? No memory, no cookie! ;-) I understand that you want save/restore to be predictable even when the guest is too crap for words. I think clarifying this in your commit message would help. > > The only architectural requirement is > > that the EID fits into the device table. There is no guarantee that > > the ITS will actually write to the memory. > > If I understand it correctly, failing the command in this case would > also be architectural (right?). If the ITS tries to write the new > entry into memory immediately, then the command would fail. This > proposed check is just making this assumption. Neither behaviour is architectural (they are both allowed, but none is required). Not providing the memory, however, is unpredictable. I'm OK with your approach because it makes things consistent (we fail early rather than late). But the commit message doesn't really reflect that (it sort of hints to it, but not in a clear way). Thanks, M. -- Without deviation from the norm, progress is not possible.