From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932618AbdJYKXS (ORCPT ); Wed, 25 Oct 2017 06:23:18 -0400 Received: from usa-sjc-mx-foss1.foss.arm.com ([217.140.101.70]:35660 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932267AbdJYKXP (ORCPT ); Wed, 25 Oct 2017 06:23:15 -0400 From: Marc Zyngier To: Eric Auger Cc: , , , , , , , , , , Subject: Re: [PATCH v5 08/10] KVM: arm/arm64: vgic-its: Free caches when GITS_BASER Valid bit is cleared In-Reply-To: <1508767709-15256-9-git-send-email-eric.auger@redhat.com> (Eric Auger's message of "Mon, 23 Oct 2017 16:08:27 +0200") Organization: ARM Ltd References: <1508767709-15256-1-git-send-email-eric.auger@redhat.com> <1508767709-15256-9-git-send-email-eric.auger@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) Date: Wed, 25 Oct 2017 11:23:09 +0100 Message-ID: <86mv4fedbm.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 On Mon, Oct 23 2017 at 4:08:27 pm BST, Eric Auger wrote: > When the GITS_BASER.Valid gets cleared, the data structures in > guest RAM are not valid anymore. The device, collection > and LPI lists stored in the in-kernel ITS represent the same > information in some form of cache. So let's void the cache. > > Signed-off-by: Eric Auger > > --- > v4 -> v5: > - add comment about locking > > v2 -> v3: > - add a comment and clear cache in if block > --- > virt/kvm/arm/vgic/vgic-its.c | 27 +++++++++++++++++++++++---- > 1 file changed, 23 insertions(+), 4 deletions(-) > > diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c > index 8098f91..bdfceb4 100644 > --- a/virt/kvm/arm/vgic/vgic-its.c > +++ b/virt/kvm/arm/vgic/vgic-its.c > @@ -1434,8 +1434,9 @@ static void vgic_mmio_write_its_baser(struct kvm *kvm, > unsigned long val) > { > const struct vgic_its_abi *abi = vgic_its_get_abi(its); > - u64 entry_size, device_type; > + u64 entry_size; > u64 reg, *regptr, clearbits = 0; > + int type; > > /* When GITS_CTLR.Enable is 1, we ignore write accesses. */ > if (its->enabled) > @@ -1445,12 +1446,12 @@ static void vgic_mmio_write_its_baser(struct kvm *kvm, > case 0: > regptr = &its->baser_device_table; > entry_size = abi->dte_esz; > - device_type = GITS_BASER_TYPE_DEVICE; > + type = GITS_BASER_TYPE_DEVICE; > break; > case 1: > regptr = &its->baser_coll_table; > entry_size = abi->cte_esz; > - device_type = GITS_BASER_TYPE_COLLECTION; > + type = GITS_BASER_TYPE_COLLECTION; > clearbits = GITS_BASER_INDIRECT; > break; > default: > @@ -1462,10 +1463,28 @@ static void vgic_mmio_write_its_baser(struct kvm *kvm, > reg &= ~clearbits; > > reg |= (entry_size - 1) << GITS_BASER_ENTRY_SIZE_SHIFT; > - reg |= device_type << GITS_BASER_TYPE_SHIFT; > + reg |= (u64)type << GITS_BASER_TYPE_SHIFT; Nit: Why having changed the type to being an int? The existing type (u64) was perfectly sane, and avoids the ugly cast. Also, table_type would be more explicit than the pretty generic "type". > reg = vgic_sanitise_its_baser(reg); > > *regptr = reg; > + > + /* > + * If the table is no longer valid, we clear the associated cached data. > + * Note: there cannot be any race with save/restore code which locks > + * all vcpus. > + */ > + if (!(reg & GITS_BASER_VALID)) { > + switch (type) { > + case GITS_BASER_TYPE_DEVICE: > + vgic_its_free_device_list(kvm, its); > + break; > + case GITS_BASER_TYPE_COLLECTION: > + vgic_its_free_collection_list(kvm, its); > + break; > + default: > + break; Nit: this default clause is useless. > + } > + } > } > > static unsigned long vgic_mmio_read_its_ctlr(struct kvm *vcpu, Otherwise: Acked-by: Marc Zyngier M. -- Jazz is not dead. It just smells funny.