Received: by 2002:ac0:a5a7:0:0:0:0:0 with SMTP id m36-v6csp420163imm; Mon, 9 Jul 2018 04:24:28 -0700 (PDT) X-Google-Smtp-Source: AAOMgpfsPuSbjQyZpHm9vY9r4QfOAus/rjMC1/EtCaDYrRRrnrI6k+DVZi3PWvFf9t5NK0ptm7Gl X-Received: by 2002:a63:7252:: with SMTP id c18-v6mr18089303pgn.186.1531135468517; Mon, 09 Jul 2018 04:24:28 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1531135468; cv=none; d=google.com; s=arc-20160816; b=PHtQLfdmOUD1tItGQRj7wDzvt03tK3VqcUVoKDzHQUN7Yck3WnJLsvIONadOdOYEp/ pbc+wYnHa2nM8m2vHG1e6kysJHKHff6VLsrMo9Ni9cUdB8crpU1vLqINpur8JbZMRdxc Aov/p/S4PS8CruzjpWULHBLFHgWe16im1hg+IZgLCCj+xYISVJAdLYlyELnEBorRZb9q cRw+xJrwuPGxZ21QRW/i7UNNB+yupe7PEYdKHpBOcg8OAz/GFL2grZOIR83nF/rmDuA/ YWrqdCET4oG5RYhHhiixdLE5L9UVRrQaQOluh0uRVcs2lwnSxm7LzKI2SCqb0mr5aTrK 9LhQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-transfer-encoding:content-disposition:mime-version :references:message-id:subject:cc:to:from:date :arc-authentication-results; bh=xDtRNfW/G86LPYo0hnHrDpdCNQJmc7ZJrWJ6GWfVGNI=; b=J1jJGUMK6G6k0jEn345426GYZcBhzQ7s+gxHlkso0xYgfQIlyzKXYNHMhLtsG8DLWG IdIHKKXLAK0lsw0EtsNbnnpGHD3Ig1CdGvi0GOjbcql+tIUcnWCY51R6Q0yGieo2F2m8 ufu6kPKUFZHSbf+BwDfzJGwDRtWbXeqxK1db0xOU2v0kqm0Erwd8vJdk6YB22/0ao2Ta m8mv4lAjdTG7ETOOkbxFy01yA84iIInRfTu/28fIC3wNJXndr8xv5/WaMzKqn4SzRKoq 4PPbNd3sPv3juu8MOnOqbf01yjdP+l5BUQy9jgHWG7eqnbtDOKLwvN2q/2vyz6jWW+LP 4lXg== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id d11-v6si10697614pgm.556.2018.07.09.04.24.09; Mon, 09 Jul 2018 04:24:28 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932940AbeGILXd (ORCPT + 99 others); Mon, 9 Jul 2018 07:23:33 -0400 Received: from usa-sjc-mx-foss1.foss.arm.com ([217.140.101.70]:57302 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932866AbeGILXb (ORCPT ); Mon, 9 Jul 2018 07:23:31 -0400 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 0FFAB7A9; Mon, 9 Jul 2018 04:23:31 -0700 (PDT) Received: from e103592.cambridge.arm.com (usa-sjc-imap-foss1.foss.arm.com [10.72.51.249]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id F2BA03F318; Mon, 9 Jul 2018 04:23:28 -0700 (PDT) Date: Mon, 9 Jul 2018 12:23:26 +0100 From: Dave Martin To: Suzuki K Poulose Cc: Marc Zyngier , Will Deacon , cdall@kernel.org, kvm@vger.kernel.org, catalin.marinas@arm.com, punit.agrawal@arm.com, linux-kernel@vger.kernel.org, qemu-devel@nongnu.org, Paolo Bonzini , kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v3 15/20] kvm: arm/arm64: Allow tuning the physical address size for VM Message-ID: <20180709112326.GD9486@e103592.cambridge.arm.com> References: <1530270944-11351-1-git-send-email-suzuki.poulose@arm.com> <1530270944-11351-16-git-send-email-suzuki.poulose@arm.com> <20180704155104.GN4828@arm.com> <12d1832a-1a13-7dd4-662b-addf58400789@arm.com> <9f1af26e-2913-2b0b-1352-63160096f78f@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Jul 06, 2018 at 05:39:00PM +0100, Suzuki K Poulose wrote: > On 07/06/2018 04:09 PM, Marc Zyngier wrote: > >On 06/07/18 14:49, Suzuki K Poulose wrote: > >>On 04/07/18 23:03, Suzuki K Poulose wrote: > >>>On 07/04/2018 04:51 PM, Will Deacon wrote: > >>>>Hi Suzuki, > >>>> > >>>>On Fri, Jun 29, 2018 at 12:15:35PM +0100, Suzuki K Poulose wrote: > >>>>>Allow specifying the physical address size for a new VM via > >>>>>the kvm_type argument for KVM_CREATE_VM ioctl. This allows > >>>>>us to finalise the stage2 page table format as early as possible > >>>>>and hence perform the right checks on the memory slots without > >>>>>complication. The size is encoded as Log2(PA_Size) in the bits[7:0] > >>>>>of the type field and can encode more information in the future if > >>>>>required. The IPA size is still capped at 40bits. > >>>>> > >>>>>Cc: Marc Zyngier > >>>>>Cc: Christoffer Dall > >>>>>Cc: Peter Maydel > >>>>>Cc: Paolo Bonzini > >>>>>Cc: Radim Krčmář > >>>>>Signed-off-by: Suzuki K Poulose > >>>>>--- > >>>>>   arch/arm/include/asm/kvm_mmu.h   |  2 ++ > >>>>>   arch/arm64/include/asm/kvm_arm.h | 10 +++------- > >>>>>   arch/arm64/include/asm/kvm_mmu.h |  2 ++ > >>>>>   include/uapi/linux/kvm.h         | 10 ++++++++++ > >>>>>   virt/kvm/arm/arm.c               | 24 ++++++++++++++++++++++-- > >>>>>   5 files changed, 39 insertions(+), 9 deletions(-) > >>>> > >>>>[...] > >>>> > >>>>>diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h > >>>>>index 4df9bb6..fa4cab0 100644 > >>>>>--- a/include/uapi/linux/kvm.h > >>>>>+++ b/include/uapi/linux/kvm.h > >>>>>@@ -751,6 +751,16 @@ struct kvm_ppc_resize_hpt { > >>>>>   #define KVM_S390_SIE_PAGE_OFFSET 1 > >>>>>   /* > >>>>>+ * On arm/arm64, machine type can be used to request the physical > >>>>>+ * address size for the VM. Bits [7-0] have been reserved for the > >>>>>+ * PA size shift (i.e, log2(PA_Size)). For backward compatibility, > >>>>>+ * value 0 implies the default IPA size, which is 40bits. > >>>>>+ */ > >>>>>+#define KVM_VM_TYPE_ARM_PHYS_SHIFT_MASK    0xff > >>>>>+#define KVM_VM_TYPE_ARM_PHYS_SHIFT(x)        \ > >>>>>+    ((x) & KVM_VM_TYPE_ARM_PHYS_SHIFT_MASK) > >>>> > >>>>This seems like you're allocating quite a lot of bits in a non-extensible > >>>>interface to a fairly esoteric parameter. Would it be better to add another > >>>>ioctl, or condense the number of sizes you support instead? > >>> > >>>As I explained in the other thread, we need the size as soon as the VM > >>>is created. The major challenge is keeping the backward compatibility by > >>>mapping 0 to 40bits. I will give it a thought. > >> > >>Here is one option. We could re-use the {V}TCR_ELx.{I}PS field format, which > >>occupies 3 bits and has the following definitions. (ID_AA64MMFR0_EL1:PARange > >>also has the field definitions, except that the field is 4bits wide, but > >>only 3bits are used) > >> > >>000 32 bits, 4GB. > >>001 36 bits, 64GB. > >>010 40 bits, 1TB. > >>011 42 bits, 4TB. > >>100 44 bits, 16TB. > >>101 48 bits, 256TB. > >>110 52 bits, 4PB > >> > >>But we need to map 0 => 40bits IPA to make our ABI backward compatible. So > >>we could use the additional one bit to indicate that IPA size is requested > >>in the 3 bits. > >> > >>i.e, > >> > >>machine_type: > >> > >>Bit [2:0] - Requested IPA size. Values follow VTCR_EL2.PS format. > >> > >>Bit [3] - 1 => IPA Size bits (Bits[2:0]) requested. > >> 0 => Not requested > >> > >>The only minor down side is restricting to the predefined values above, > >>which is not a real issue for a VM. > >> > >>Thoughts ? > > > >I'd be very wary of using that 4th bit to do something that is not in > >the architecture. We have only a single value left to be used (0b111), > >and then your scheme clashes with the architecture definition. > > I agree. However, if we ever go beyond the 3bits in PARange, we have an > issue with {V}TCR counter part. But lets not take that chance. > > > > >I'd rather encode things in a way that is independent from the > >architecture, and be done with it. You can map 0 to 40bits, and we have > >the ability to express all values the architecture has (just in a > >different order). > > The other option I can think of is encoding a signed number which is the > difference of the IPA from 40. But that would need 5 bits if we were to > encode it as it is. And if we want to squeeze it in 4bit, we could store > half the difference (limiting the IPA limit to even numbers). > > i.e IPA = 40 + 2 * sign_extend(bits[3:0); I came across similar issues when trying to work out how to enable SVE for KVM. In the end I reduced this to a per-vcpu feature, but it means that there is no global opt-in for the SVE-specific KVM API extensions: That's a bit gross, because SVE may require a change to the way vcpus are initialised. The set of supported SVE vector lengths needs to be set somehow before the vcpu is set running, but it's tricky do do that without a new ioctl -- which would mean that if SVE is enabled for a vcpu then the vcpu is not considered runnable until the new magic ioctl is called. Opting into that semantic change globally at VM creation time might be preferable. On the SVE side, this is still very much subject to review/change. Here: The KVM_CREATE_VM init argument seems undefined by the KVM core code and is available for arches to abuse in creative ways. x86 and arm have nothing here and reject non-zero values with -EINVAL; s390 treats it as a bitmask, and defines a sincle feature-like bit here; powerpc treats it as an enumeration of VM types. If we want to be extensible, we could a) Pass a pointer in type, and come up with some extensible VM parameter struct for it to point to (which then wouldn't need a cryptic compressed encoding), or b) Introduce a new "KVM_CREATE_VM2" variant that either takes such an argument, or mandates a parameter negotiation phase involving additional ioctls before marking the VM as ready for vcpu and device creation. (a) feels like an easy backwards-compatible approach, but cannot be readily adopted by other arches (maybe not an issue). (b) might be considered overengineered, so it would need a bit of thought. Wedging arguments into a few bits in the type argument feels awkward, and may be regretted later if we run out of bits, or something can't be represented in the chosen encoding. Cheers ---Dave