Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751585AbdIOWc2 (ORCPT ); Fri, 15 Sep 2017 18:32:28 -0400 Received: from foss.arm.com ([217.140.101.70]:53186 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751230AbdIOWc1 (ORCPT ); Fri, 15 Sep 2017 18:32:27 -0400 From: Marc Zyngier To: Christoffer Dall Cc: Auger Eric , eric.auger.pro@gmail.com, "linux-kernel\@vger.kernel.org" , "kvm\@vger.kernel.org" , Peter Maydell , Andre Przywara , Haibin Wang , wu.wubin@huawei.com Subject: Re: [RFC] KVM: arm/arm64: Introduce KVM_DEV_ARM_ITS_CTRL_RESET In-Reply-To: (Christoffer Dall's message of "Fri, 15 Sep 2017 10:56:06 -0700") Organization: ARM Ltd References: <1505379448-19583-1-git-send-email-eric.auger@redhat.com> <864ls5rz0h.fsf@arm.com> <2bb2cec8-ff29-e5e7-59dc-727924a51898@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) Date: Fri, 15 Sep 2017 23:32:20 +0100 Message-ID: <86y3pfpp8r.fsf@arm.com> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2739 Lines: 61 On Fri, Sep 15 2017 at 10:56:06 am BST, Christoffer Dall wrote: > On Fri, Sep 15, 2017 at 5:26 AM, Auger Eric wrote: >> Hi, >> >> On 14/09/2017 19:06, Marc Zyngier wrote: >>> On Thu, Sep 14 2017 at 10:57:28 am BST, Eric Auger wrote: >>>> At the moment, the in-kernel emulated ITS is not properly reset. >>>> On guest restart/reset some registers keep their old values and >>>> internal structures like device, ITE, collection lists are not emptied. >>>> >>>> This may lead to various bugs. Among them, we can have incorrect state >>>> backup or failure when saving the ITS state at early guest boot stage. >>>> >>>> This patch introduces a new attribute, KVM_DEV_ARM_ITS_CTRL_RESET in >>>> the KVM_DEV_ARM_VGIC_GRP_CTRL group. >>>> >>>> Upon this action, we can invalidate the various memory structures >>>> pointed by GITS_BASERn and GITS_CBASER, free the ITS internal caches >>>> and reset the relevant registers. >>>> >>>> Signed-off-by: Eric Auger >>>> >>>> --- >>>> >>>> An alternative would consist in having the userspace writing >>>> individual registers with default values: GITS_BASERn, GITS_CBASER >>>> and GITS_CTLR. On kernel side we would reset related lists when >>>> detecting the valid bit is set to false. >>> >>> I'm not sure this is necessarily a "either/or" situation. It looks to me >>> that we're not completely doing the right thing when writing to the >>> GITS_BASER registers, and that writing a new value (with the valid bit >>> set or not) should have an action of some sort on the fate of the >>> existing mappings. >> >> I agree. I think whenever the GITS_BASERn or GITS_CBASER validity bit is >> reset, we should empty the internal lists and assure the code does not >> attempt to read the data structures in caches/RAM anymore. >> > > I don't think that is likely to match the behavior suggested in the > GIC/ITS spec. I doubt that hardware implementations will support > software changing the BASERs without turning off the GIC, and > therefore I don't think we'll see drivers doing this any time soon, > and I don't think we need to support that. I've managed to check this, and at least one implementation does clear its caches on write to the corresponding BASERn registers, which makes some sense. It is slightly annoying that the spec doesn't outline this, but I'll enquire to see if that can be clarified. > What I do think we should support is the ITS power management sequence > pointed out in Section 6.6 in the spec. But I don't think this is > urgent, as we don't seem to have any guests that power down and power > up the ITS yet. +1 on both point. M. -- Jazz is not dead. It just smells funny.