Received: by 2002:ac0:a5a7:0:0:0:0:0 with SMTP id m36-v6csp550250imm; Mon, 9 Jul 2018 06:39:49 -0700 (PDT) X-Google-Smtp-Source: AAOMgpf7kqaubOSbwOQtoQjc4anjX9xU+06tqRCZTstdlDNVoTjnNX2it1NSnRiVldpYYWvI0K3e X-Received: by 2002:a63:5421:: with SMTP id i33-v6mr19195898pgb.417.1531143589898; Mon, 09 Jul 2018 06:39:49 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1531143589; cv=none; d=google.com; s=arc-20160816; b=fsfnnkS3vtTNgGjTdiWB5o0B1YbA+h0+KTZqHDZ0fn2GcWEhzrLGc5tsoP3k8nZtL6 7QcOzx5FlLksMbCBNTQCu33J4Gg3QDZ2clnDIjNNCKp1YbwZKsroQTqsaK3LKS4njhY8 ZMoQ0/leOHoF2r3pHR7EnT0uYY3xJP4RcX/9n8UYPj12VN7plgpGtJGY3i9kS+85IkC3 /EgElaIGzbwyIljliLSQlz7J4N9toL1JDAjSAkCnGUGAa5Dov9Jo3XQaPCy8ThiveOK7 6N6L2N5i21pVQUoGuKoa5ZDO5owPbM6H5SqLrenizVNzVV5aKMFvOsAKwBqoC/8QkOJ4 0MKA== 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=W6pGwdkT4fKjHo0BzkrifGPkA4hd7hEAto2Um/WxPVM=; b=MkTNEee0tF2Uhxr6MCCDsR1P5sYDUPLPcDLdXHPmiy5ZqA/pLHuona8Rivi95ytLrd vVcBHFKNysAQhE9tOjPGhJ6lOYEzvw6utBXoizBhzDSd6Bo9MW70ByO3sid1grcESJ5H wRSzGMP6f7Wwu4rTEx0errWDWH05ytsf+arfN2qqB9qJOWFQZk/8S8lKPhB1wmklP+gy F1w/Vs0i6aaR1mQKcutZGpqX9Ge5FbS5e/oczg0yNihXae2JBOlWI7owHSWePhrsA+Lf NCH41FeV5eqxzBxcR4HsrWL3cD50rr553NLpMjHeEGFODDcIg4Ktl+gL9KCdOz0+w46G 6Y/Q== 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 34-v6si14116631pgs.243.2018.07.09.06.39.35; Mon, 09 Jul 2018 06:39:49 -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 S1754553AbeGINh5 (ORCPT + 99 others); Mon, 9 Jul 2018 09:37:57 -0400 Received: from usa-sjc-mx-foss1.foss.arm.com ([217.140.101.70]:59186 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754437AbeGINhz (ORCPT ); Mon, 9 Jul 2018 09:37:55 -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 5F235ED1; Mon, 9 Jul 2018 06:37:55 -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 4CA833F5AD; Mon, 9 Jul 2018 06:37:53 -0700 (PDT) Date: Mon, 9 Jul 2018 14:37:50 +0100 From: Dave Martin To: Marc Zyngier Cc: Suzuki K Poulose , cdall@kernel.org, kvm@vger.kernel.org, catalin.marinas@arm.com, punit.agrawal@arm.com, Will Deacon , 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: <20180709133750.GE9486@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> <20180709112326.GD9486@e103592.cambridge.arm.com> <17f8d585-3489-ab6f-6ee1-4d8d337dcf9c@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <17f8d585-3489-ab6f-6ee1-4d8d337dcf9c@arm.com> 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 Mon, Jul 09, 2018 at 01:29:42PM +0100, Marc Zyngier wrote: > On 09/07/18 12:23, Dave Martin wrote: > > 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. > > I think that's a pretty convincing argument for a "better" CREATE_VM, > one that would have a clearly defined, structured (and potentially > extensible) argument. > > I've quickly hacked the following: > > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h > index b6270a3b38e9..3e76214034c2 100644 > --- a/include/uapi/linux/kvm.h > +++ b/include/uapi/linux/kvm.h > @@ -735,6 +735,20 @@ struct kvm_ppc_resize_hpt { > __u32 pad; > }; > > +struct kvm_create_vm2 { > + __u64 version; /* Or maybe not */ > + union { > + struct { > +#define KVM_ARM_SVE_CAPABLE (1 << 0) > +#define KVM_ARM_SELECT_IPA {1 << 1) > + __u64 capabilities; > + __u16 sve_vlen; > + __u8 ipa_size; > + } arm64; > + __u64 dummy[15]; > + }; > +}; > + > #define KVMIO 0xAE > > /* machine type bits, to be used as argument to KVM_CREATE_VM */ > > Other architectures could fill in their own bits if they need to. > > Thoughts? This kind of thing should work, but it may still get messy when we add additional fields. It we want this to work cross-arch, would it make sense to go for a more generic approach, say struct kvm_create_vm_attr_any { __u32 type; }; #define KVM_CREATE_VM_ATTR_ARCH_CAPABILITIES 1 struct kvm_create_vm_attr_arch_capabilities { __u32 type; __u16 size; /* support future expansion of capabilities[] */ __u16 reserved; __u64 capabilities[1]; }; #define KVM_CREATE_VM_ATTR_ARM64_PHYSADDR_SIZE 2 struct kvm_create_vm_attr_arm64_physaddr_size { __u32 type; __u32 physaddr_bits; }; /* ... */ union kvm_create_vm_attr { struct kvm_create_vm_attr_any; struct kvm_create_vm_attr_arch_capabilities; struct kvm_create_vm_attr_arm64_physaddr_size; /* ... */ }; struct kvm_create_vm2 { __u32 version; /* harmless, even if not useful */ __u16 nr_attrs; /* or could just terminate attrs with a NULL entry */ union kvm_create_vm_attr __user *__user *attrs; }; This is quite flexible, but obviously a bit heavy. However, if we're adding a new interface due to lack of extensibility, it may be worth going for something that's freely extensible. Userspace might call this as struct kvm_create_vm_attr_arch_capabilities vm_arch_caps = { .type = KVM_CREATE_VM_ATTR_ARCH_CAPABILITIES, .size = 64, .capabilities[0] = KVM_CREATE_VM_ARM64_VCPU_NEEDS_SET_SVE_VLS, }; struct kvm_create_vm_attr_arch_arm64_physaddr_size = { .type = KVM_CREATE_VM_ATTR_ARM64_PHYSADDR_SIZE, .physaddr_bits = 52, }; union kvm_create_vm_attr **vmattrs[] = { &vm_arch_caps, &vm_arm64_physaddr_size, NULL, /* maybe */ }; struct kvm_create_vm2 vm; vm.version = 0; vm.nr_attrs = 2; /* maybe */ vm.attrs = vmattrs; ioctl(..., KVM_CREATE_VM2, &vm); Cheers ---Dave