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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id D0442C433EF for ; Wed, 23 Mar 2022 07:07:15 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=c7WMLtEKs+69XHpvPmWuJePyAmZTqqmKZ16M1bpap78=; b=cvRgnoA5/B0gg6 X8GgxvbBXgvBCTVHtgt0A36PyC0PhQeAI2m9Qbbs2mVwg5+vLoGXhD7LQhRDxeTngaI1egwoSG4rK RciZBAX5ocUbDau5zHL3z8ZYpL1vFufGMZq4rYU4lYvfmW+QQaO5aT0IRJBK7YHjxCyouEOoPSH3+ bWa7DRXMsdeFzTCkaNbTJtBM9nMxN4LPDeBmlM28v8L/Y0ER33VYePUqdfuBSUT0hzkE3YCu5lYjd 1R8bvLx4Ap+Kvkv/BIS8iPYEeqeDUWqtkiBsPFZevalx/y1kW8IQitP/ThZ4eZAWJ3FWWR2TNAcH4 T11pLtvwCHYy1t6J7xkg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nWv4I-00Cu6c-6P; Wed, 23 Mar 2022 07:06:06 +0000 Received: from mail-io1-xd35.google.com ([2607:f8b0:4864:20::d35]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1nWv4F-00Cu67-EB for linux-arm-kernel@lists.infradead.org; Wed, 23 Mar 2022 07:06:05 +0000 Received: by mail-io1-xd35.google.com with SMTP id 125so651478iov.10 for ; Wed, 23 Mar 2022 00:06:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=/miMnKL4kaQ7vT+uR7jkwmb3+siwfRd5pcJFIiCuqFk=; b=dZgCuH70fW6h2bUCVGqSqcUfjwe9gRFbhE0jEDHSjs1zNzB7/Kp7GVELo20co5+jVG o4Qz1q1XMwbrSOB/I1Z3aHBBH3CSKPqwgjNlHUn1D5hGWd52rqKtA9wB/fFC8/GOj9Po X2ZqD+qpGHzmBu+0RqHjbw3R+s3gYHK+/wE8zNpH4uZqsa7bfk+pBE193lABOu7O5RJh wgd/aTk2+gL1MiIijDTW1373AWjHEeeUgaXVPVr0KEwUWggB+Q2fEXevdU6Wxc/Fb3M/ aSWD8MiClIgCjt02tDXEkCX2xo1FUZZHBs6hGA7Q/mGXyFNzNCGl3mTKRfcpI4GQwF7v pzCA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=/miMnKL4kaQ7vT+uR7jkwmb3+siwfRd5pcJFIiCuqFk=; b=wkZF/yUpx63Vb4pz1z8MamHhxVI59m1NtTMt6rH6PtSZ6LxyJ9az230pOAILxZlR1I MCdh8dCjcJSkrnCttQBWPE90JVlhdvUBLdWfItYw82sANSGs1hH9hO/rN9iBeDhJ8N2p kOhCvUrp0vqVHjQq4JiIOwZVYDGbYvLLjkYGA2UGp0sa51NRuCjIp2H26Wfwthex773E kpzeF0fNryd5zuVAyukShF+75gZAdnz6zpWZq9qVAf1tv7GQjyQYQCt4YZ0IKL53vGaH F7cAkIQfZzSeQPHBtZUBH277Ah/N1oMP5xmQxP5gk6jKysbCazhoaDLEdwzB5Hz18d6Y GrXw== X-Gm-Message-State: AOAM532K+XGgNTf0c27m4pZpoKSrKkuKA9CFZ2YGUXulIwFWMSdOsRbh sfaY6vu7CswlJ63Vs5GFyTu1lA== X-Google-Smtp-Source: ABdhPJzbMKzfJP+9oJi/cTbwf64cP7kfvgXvfrr+u0NJiMiYNSt7CzGj3qdYxmH/yXQ0QuScpJNIlg== X-Received: by 2002:a05:6638:130b:b0:317:cc10:1c88 with SMTP id r11-20020a056638130b00b00317cc101c88mr15649821jad.34.1648019160340; Wed, 23 Mar 2022 00:06:00 -0700 (PDT) Received: from google.com (194.225.68.34.bc.googleusercontent.com. [34.68.225.194]) by smtp.gmail.com with ESMTPSA id r9-20020a6bd909000000b00649276ea9fesm10724253ioc.7.2022.03.23.00.05.58 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 23 Mar 2022 00:05:59 -0700 (PDT) Date: Wed, 23 Mar 2022 07:05:55 +0000 From: Oliver Upton To: Reiji Watanabe Cc: Marc Zyngier , kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org, Linux ARM , James Morse , Alexandru Elisei , Suzuki K Poulose , Paolo Bonzini , Will Deacon , Andrew Jones , Fuad Tabba , Peng Liang , Peter Shier , Ricardo Koller , Jing Zhang , Raghavendra Rao Anata Subject: Re: [PATCH v6 01/25] KVM: arm64: Introduce a validation function for an ID register Message-ID: References: <20220311044811.1980336-1-reijiw@google.com> <20220311044811.1980336-2-reijiw@google.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220323_000603_511966_5F8EE1B0 X-CRM114-Status: GOOD ( 40.92 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Mar 22, 2022 at 11:06:26PM -0700, Reiji Watanabe wrote: > Hi Oliver, > > > On Thu, Mar 10, 2022 at 08:47:47PM -0800, Reiji Watanabe wrote: > > > Introduce arm64_check_features(), which does a basic validity checking > > > of an ID register value against the register's limit value, which is > > > generally the host's sanitized value. > > > > > > This function will be used by the following patches to check if an ID > > > register value that userspace tries to set for a guest can be supported > > > on the host. > > > > > > The validation is done using arm64_ftr_bits_kvm, which is created from > > > arm64_ftr_regs, with some entries overwritten by entries from > > > arm64_ftr_bits_kvm_override. > > > > > > Signed-off-by: Reiji Watanabe > > > > I have some concerns regarding the API between cpufeature and KVM that's > > being proposed here. It would appear that we are adding some of KVM's > > implementation details into the cpufeature code. In particular, I see > > that KVM's limitations on AA64DFR0 are being copied here. > > I assume "KVM's limitation details" you meant is about > ftr_id_aa64dfr0_kvm. > Entries in arm64_ftr_bits_kvm_override shouldn't be added based > on KVM's implementation. When cpufeature.c doesn't handle lower level > of (or fewer) features as the "safe" value for fields, the field should > be added to arm64_ftr_bits_kvm_override. As PMUVER and DEBUGVER are not > treated as LOWER_SAFE, they were added in arm64_ftr_bits_kvm_override. I believe the fact that KVM is more permissive on PMUVER and DEBUGVER than cpufeature is in fact a detail of KVM, no? read_id_reg() already implicitly trusts the cpufeature code filtering and applies additional limitations on top of what we get back. Similarly, there are fields where KVM is more restrictive than cpufeature (ID_AA64DFR0_PMSVER). Each of those constraints could theoretically be expressed as an arm64_ftr_bits structure within KVM. > Having said that, although ftr_id_aa64dfr0 has PMUVER as signed field, > I didn't fix that in ftr_id_aa64dfr0_kvm, and the code abused that.... > I should make PMUVER unsigned field, and fix cpufeature.c to validate > the field based on its special ID scheme for cleaner abstraction. Good point. AA64DFR0 is an annoying register :) > (And KVM should skip the cpufeature.c's PMUVER validation using > id_reg_desc's ignore_mask and have KVM validate it locally based on > the KVM's special requirement) > > > > Additionally, I think it would be preferable to expose this in a manner > > that does not require CONFIG_KVM guards in other parts of the kernel. > > > > WDYT about having KVM keep its set of feature overrides and otherwise > > rely on the kernel's feature tables? I messed around with the idea a > > bit until I could get something workable (attached). I only compile > > tested this :) > > Thanks for the proposal! > But, providing the overrides to arm64_ftr_reg_valid() means that its > consumer knows implementation details of cpufeture.c (values of entries > in arm64_ftr_regs[]), which is a similar abstraction problem I want to > avoid (the default validation by cpufeature.c should be purely based on > ID schemes even with this option). It is certainly a matter of where you choose to draw those lines. We already do bank on the implementation details of cpufeature.c quite heavily, its just stuffed away behind read_sanitised_ftr_reg(). Now we have KVM bits and pieces in cpufeature.c and might wind up forcing others to clean up our dirty laundry in the future. It also seems to me that if I wanted to raise the permitted DEBUGVER for KVM, would I have to make a change outside of KVM. > Another option that I considered earlier was having a full set of > arm64_ftr_bits in KVM for its validation. At the time, I thought > statically) having a full set of arm64_ftr_bits in KVM is not good in > terms of maintenance. But, considering that again, since most of > fields are unsigned and lower safe fields, and KVM doesn't necessarily > have to statically have a full set of arm64_ftr_bits I think the argument could be made for KVM having its own static + verbose cpufeature tables. We've already been bitten by scenarios where cpufeature exposes a feature that we simply do not virtualize in KVM. That really can become a game of whack-a-mole. commit 96f4f6809bee ("KVM: arm64: Don't advertise FEAT_SPE to guests") is a good example, and I can really see no end to these sorts of issues without an overhaul. We'd need to also find a way to leverage the existing infrasturcture for working out a system-wide safe value, but this time with KVM's table of registers. KVM would then need to take a change to expose any new feature that has no involvement of EL2. Personally, I'd take that over the possibility of another unhandled feature slipping through and blowing up a guest kernel when running on newer hardware. > (dynamically generate during KVM's initialization) This was another one of my concerns with the current state of this patch. I found the register table construction at runtime hard to follow. I think it could be avoided with a helper that has a prescribed set of rules (caller-provided field definition takes precedence over the general one). Sorry it took me a bit to comment on everything, needed to really sit down and wrap my head around the series. -- Thanks, Oliver _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel