From patchwork Wed May 18 15:12:23 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Christoffer Dall X-Patchwork-Id: 68050 Delivered-To: patch@linaro.org Received: by 10.140.92.199 with SMTP id b65csp2692527qge; Wed, 18 May 2016 08:13:19 -0700 (PDT) X-Received: by 10.98.55.133 with SMTP id e127mr11548595pfa.81.1463584399891; Wed, 18 May 2016 08:13:19 -0700 (PDT) Return-Path: Received: from bombadil.infradead.org (bombadil.infradead.org. [2001:1868:205::9]) by mx.google.com with ESMTPS id ey9si12676533pab.123.2016.05.18.08.13.19 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 18 May 2016 08:13:19 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-arm-kernel-bounces+patch=linaro.org@lists.infradead.org designates 2001:1868:205::9 as permitted sender) client-ip=2001:1868:205::9; Authentication-Results: mx.google.com; dkim=neutral (body hash did not verify) header.i=@linaro.org; spf=pass (google.com: best guess record for domain of linux-arm-kernel-bounces+patch=linaro.org@lists.infradead.org designates 2001:1868:205::9 as permitted sender) smtp.mailfrom=linux-arm-kernel-bounces+patch=linaro.org@lists.infradead.org; dmarc=fail (p=NONE dis=NONE) header.from=linaro.org Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1b338m-000526-3o; Wed, 18 May 2016 15:12:04 +0000 Received: from mail-wm0-x231.google.com ([2a00:1450:400c:c09::231]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1b338i-00050y-TP for linux-arm-kernel@lists.infradead.org; Wed, 18 May 2016 15:12:02 +0000 Received: by mail-wm0-x231.google.com with SMTP id g17so84493360wme.1 for ; Wed, 18 May 2016 08:11:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=z8osYu1VpOaRRTAWhlQdDpu6lm2yAQJ2GcdM9k1hSwc=; b=Id1h7oNBhg+UO1hyB0ID8/sRPNsvSOwAl2ObfoqTu/LaqXQ6VPfLKmmozuCw8YDwSa 0eyCcJCttAPPQBVBaTOpSQP1Q1Z+ONGZ6XHs8XVpUjEDTIm3XxSPSBREa/QPCHipvNzg 9uCdI2NdBsdB5ChdsSjbduWMJYZ8t3Cl0ZKK8= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=z8osYu1VpOaRRTAWhlQdDpu6lm2yAQJ2GcdM9k1hSwc=; b=KqfWaLaEZivpFgmf+zmG2MyNKDYF7epDu2R6jxmURH9NJ6iU+yoieprOSuEWkT0pqI OeDYDz5L+u0zNehbEppNoDMenuQ4S9exAJTtyZRL0WDNMiZ3TDK2D+hAe6lrPGDTU5YU 8PvfgYbc71XUjSYEHK4kx5aYjpqzz1nTApRzqC+1luLUjh71v2l2BTS6ugMd/XIxYj8u 3QXWT7N+LnZjS1APehZCzs8iWO8OBymHzpQ/IURrz460frkKD/MRwU37443RlKGeUx0+ JuFNKsfbJoKaRzPeEhvKRYoeNYQAXo2VcRYg7sN9JNVaRd9IxpsOvSPviBKlI7YmyGmG Hozw== X-Gm-Message-State: AOPr4FXZfi/Ep4ZcJf/BQhbqhcVXuWENA70N+6hEq+3JrvhIEyGQQ4CMhGZ104/I7mJOgCrX X-Received: by 10.194.79.70 with SMTP id h6mr8955259wjx.157.1463584297012; Wed, 18 May 2016 08:11:37 -0700 (PDT) Received: from localhost ([94.18.191.146]) by smtp.gmail.com with ESMTPSA id yr1sm9190789wjc.9.2016.05.18.08.11.36 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Wed, 18 May 2016 08:11:36 -0700 (PDT) Date: Wed, 18 May 2016 17:12:23 +0200 From: Christoffer Dall To: Andre Przywara Subject: Re: [PATCH v4 46/56] KVM: arm/arm64: vgic-new: Add userland access to VGIC dist registers Message-ID: <20160518151223.GK6666@cbox> References: <1463392481-26583-1-git-send-email-andre.przywara@arm.com> <1463392481-26583-47-git-send-email-andre.przywara@arm.com> <20160518135724.GO3827@cbox> <573C76D2.3050906@arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <573C76D2.3050906@arm.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20160518_081201_251835_21DCD7FD X-CRM114-Status: GOOD ( 28.42 ) X-Spam-Score: -2.7 (--) X-Spam-Report: SpamAssassin version 3.4.0 on bombadil.infradead.org summary: Content analysis details: (-2.7 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.7 RCVD_IN_DNSWL_LOW RBL: Sender listed at http://www.dnswl.org/, low trust [2a00:1450:400c:c09:0:0:0:231 listed in] [list.dnswl.org] -0.0 SPF_PASS SPF: sender matches SPF record -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Marc Zyngier , linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org, Eric Auger Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patch=linaro.org@lists.infradead.org On Wed, May 18, 2016 at 03:06:10PM +0100, Andre Przywara wrote: > Hi, > > On 18/05/16 14:57, Christoffer Dall wrote: > > On Mon, May 16, 2016 at 10:53:34AM +0100, Andre Przywara wrote: > >> Userland may want to save and restore the state of the in-kernel VGIC, > >> so we provide the code which takes a userland request and translate > >> that into calls to our MMIO framework. > >> > >> Signed-off-by: Andre Przywara > >> --- > >> virt/kvm/arm/vgic/vgic-kvm-device.c | 50 ++++++++++++++++++++++++++++++++++++- > >> 1 file changed, 49 insertions(+), 1 deletion(-) > >> > >> diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c > >> index 78621283..c3ec453 100644 > >> --- a/virt/kvm/arm/vgic/vgic-kvm-device.c > >> +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c > >> @@ -238,7 +238,55 @@ static int vgic_attr_regs_access(struct kvm_device *dev, > >> struct kvm_device_attr *attr, > >> u32 *reg, bool is_write) > >> { > >> - return -ENXIO; > >> + gpa_t addr; > >> + int cpuid, ret, c; > >> + struct kvm_vcpu *vcpu, *tmp_vcpu; > >> + > >> + cpuid = (attr->attr & KVM_DEV_ARM_VGIC_CPUID_MASK) >> > >> + KVM_DEV_ARM_VGIC_CPUID_SHIFT; > >> + vcpu = kvm_get_vcpu(dev->kvm, cpuid); > >> + addr = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK; > >> + > >> + mutex_lock(&dev->kvm->lock); > >> + > >> + ret = vgic_init(dev->kvm); > >> + if (ret) > >> + goto out; > >> + > >> + if (cpuid >= atomic_read(&dev->kvm->online_vcpus)) { > >> + ret = -EINVAL; > >> + goto out; > >> + } > >> + > >> + /* > >> + * Ensure that no other VCPU is running by checking the vcpu->cpu > >> + * field. If no other VPCUs are running we can safely access the VGIC > >> + * state, because even if another VPU is run after this point, that > >> + * VCPU will not touch the vgic state, because it will block on > >> + * getting the vgic->lock in kvm_vgic_sync_hwstate(). > >> + */ > > > > We still have a problem here. Want me to write a patch? > > Aargh, this somehow got lost in my pile of "comments to address", sorry! > Short answer: yes, please. > Last thing I did was following your suggestion from last time about > refactoring the check from kvm_vgic_create(): I think this doesn't help, > because it's only a check and we return -EBUSY or so instead of actually > enforcing VCPUs to exit. So if you could try to use your fancy new > make-the-guest-exit feature here? > Am I missing something completely or why doesn't this work: Thanks, -Christoffer _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c index 237d753..0130c4b 100644 --- a/virt/kvm/arm/vgic/vgic-kvm-device.c +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c @@ -241,6 +241,7 @@ static int vgic_attr_regs_access(struct kvm_device *dev, gpa_t addr; int cpuid, ret, c; struct kvm_vcpu *vcpu, *tmp_vcpu; + int vcpu_lock_idx = -1; cpuid = (attr->attr & KVM_DEV_ARM_VGIC_CPUID_MASK) >> KVM_DEV_ARM_VGIC_CPUID_SHIFT; @@ -259,17 +260,16 @@ static int vgic_attr_regs_access(struct kvm_device *dev, } /* - * Ensure that no other VCPU is running by checking the vcpu->cpu - * field. If no other VPCUs are running we can safely access the VGIC - * state, because even if another VPU is run after this point, that - * VCPU will not touch the vgic state, because it will block on - * getting the vgic->lock in kvm_vgic_sync_hwstate(). + * Any time a vcpu is run, vcpu_load is called which tries to grab the + * vcpu->mutex. By grabbing the vcpu->mutex of all VCPUs we ensure + * that no other VCPUs are run and fiddle with the vgic state while we + * access it. */ + ret = -EBUSY; kvm_for_each_vcpu(c, tmp_vcpu, dev->kvm) { - if (unlikely(tmp_vcpu->cpu != -1)) { - ret = -EBUSY; + if (!mutex_trylock(&tmp_vcpu->mutex)) goto out; - } + vcpu_lock_idx = c; } switch (attr->group) { @@ -285,6 +285,11 @@ static int vgic_attr_regs_access(struct kvm_device *dev, } out: + for (; vcpu_lock_idx >= 0; vcpu_lock_idx--) { + tmp_vcpu = kvm_get_vcpu(dev->kvm, vcpu_lock_idx); + mutex_unlock(&tmp_vcpu->mutex); + } + mutex_unlock(&dev->kvm->lock); return ret; }