From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christoffer Dall Subject: Re: [PATCH v6 16/24] KVM: arm64: vgic-its: KVM_DEV_ARM_ITS_SAVE/RESTORE_TABLES Date: Fri, 5 May 2017 04:55:07 -0700 Message-ID: <20170505115507.GD1419@lvm> References: <1493898284-29504-1-git-send-email-eric.auger@redhat.com> <1493898284-29504-17-git-send-email-eric.auger@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: eric.auger.pro@gmail.com, marc.zyngier@arm.com, andre.przywara@arm.com, vijayak@caviumnetworks.com, Vijaya.Kumar@cavium.com, peter.maydell@linaro.org, linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org, Prasun.Kapoor@cavium.com, drjones@redhat.com, pbonzini@redhat.com, dgilbert@redhat.com, quintela@redhat.com, bjsprakash.linux@gmail.com To: Eric Auger Return-path: Received: from mail-lf0-f42.google.com ([209.85.215.42]:36436 "EHLO mail-lf0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751180AbdEELzK (ORCPT ); Fri, 5 May 2017 07:55:10 -0400 Received: by mail-lf0-f42.google.com with SMTP id h4so2095621lfj.3 for ; Fri, 05 May 2017 04:55:09 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1493898284-29504-17-git-send-email-eric.auger@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Thu, May 04, 2017 at 01:44:36PM +0200, Eric Auger wrote: > Introduce new attributes in KVM_DEV_ARM_VGIC_GRP_CTRL group: > - KVM_DEV_ARM_ITS_SAVE_TABLES: saves the ITS tables into guest RAM > - KVM_DEV_ARM_ITS_RESTORE_TABLES: restores them into VGIC internal > structures. > > We hold the vcpus lock during the save and restore to make > sure no vcpu is running. > > At this stage the functionality is not yet implemented. Only > the skeleton is put in place. > > Signed-off-by: Eric Auger > > --- > v5 -> v6: > - remove the pending table sync from the ITS table restore > > v4 -> v5: > - use KVM_DEV_ARM_ITS_SAVE_TABLES and KVM_DEV_ARM_ITS_RESTORE_TABLES > - rename *flush* into *save* > - call its_sync_lpi_pending_table at the end of restore > - use abi framework > > v3 -> v4: > - pass kvm struct handle to vgic_its_flush/restore_pending_tables > - take the kvm lock and vcpu locks > - ABI revision check > - check attr->attr is null > > v1 -> v2: > - remove useless kvm parameter > --- > arch/arm/include/uapi/asm/kvm.h | 4 +- > arch/arm64/include/uapi/asm/kvm.h | 4 +- > virt/kvm/arm/vgic/vgic-its.c | 101 ++++++++++++++++++++++++++++++++++++-- > 3 files changed, 103 insertions(+), 6 deletions(-) > > diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h > index 4beb83b..8e6563c 100644 > --- a/arch/arm/include/uapi/asm/kvm.h > +++ b/arch/arm/include/uapi/asm/kvm.h > @@ -199,7 +199,9 @@ struct kvm_arch_memory_slot { > #define KVM_DEV_ARM_VGIC_LINE_LEVEL_INTID_MASK 0x3ff > #define VGIC_LEVEL_INFO_LINE_LEVEL 0 > > -#define KVM_DEV_ARM_VGIC_CTRL_INIT 0 > +#define KVM_DEV_ARM_VGIC_CTRL_INIT 0 > +#define KVM_DEV_ARM_ITS_SAVE_TABLES 1 > +#define KVM_DEV_ARM_ITS_RESTORE_TABLES 2 > > /* KVM_IRQ_LINE irq field index values */ > #define KVM_ARM_IRQ_TYPE_SHIFT 24 > diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h > index 7e8dd69..1e35115 100644 > --- a/arch/arm64/include/uapi/asm/kvm.h > +++ b/arch/arm64/include/uapi/asm/kvm.h > @@ -219,7 +219,9 @@ struct kvm_arch_memory_slot { > #define KVM_DEV_ARM_VGIC_LINE_LEVEL_INTID_MASK 0x3ff > #define VGIC_LEVEL_INFO_LINE_LEVEL 0 > > -#define KVM_DEV_ARM_VGIC_CTRL_INIT 0 > +#define KVM_DEV_ARM_VGIC_CTRL_INIT 0 > +#define KVM_DEV_ARM_ITS_SAVE_TABLES 1 > +#define KVM_DEV_ARM_ITS_RESTORE_TABLES 2 > > /* Device Control API on vcpu fd */ > #define KVM_ARM_VCPU_PMU_V3_CTRL 0 > diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c > index 3ad615a..5b251dc 100644 > --- a/virt/kvm/arm/vgic/vgic-its.c > +++ b/virt/kvm/arm/vgic/vgic-its.c > @@ -1669,12 +1669,68 @@ int vgic_its_attr_regs_access(struct kvm_device *dev, > } > > /** > + * vgic_its_save_device_tables - Save the device table and all ITT > + * into guest RAM > + */ > +static int vgic_its_save_device_tables(struct vgic_its *its) > +{ > + return -ENXIO; > +} > + > +/** > + * vgic_its_restore_device_tables - Restore the device table and all ITT > + * from guest RAM to internal data structs > + */ > +static int vgic_its_restore_device_tables(struct vgic_its *its) > +{ > + return -ENXIO; > +} > + > +/** > + * vgic_its_save_collection_table - Save the collection table into > + * guest RAM > + */ > +static int vgic_its_save_collection_table(struct vgic_its *its) > +{ > + return -ENXIO; > +} > + > +/** > + * vgic_its_restore_collection_table - reads the collection table > + * in guest memory and restores the ITS internal state. Requires the > + * BASER registers to be restored before. > + */ > +static int vgic_its_restore_collection_table(struct vgic_its *its) > +{ > + return -ENXIO; > +} > + > +/** > * vgic_its_save_tables_v0 - Save the ITS tables into guest ARM > * according to v0 ABI > */ > static int vgic_its_save_tables_v0(struct vgic_its *its) > { > - return -ENXIO; > + struct kvm *kvm = its->dev->kvm; > + int ret; > + > + mutex_lock(&kvm->lock); > + > + if (!lock_all_vcpus(kvm)) { > + mutex_unlock(&kvm->lock); > + return -EBUSY; > + } > + > + ret = vgic_its_save_device_tables(its); > + if (ret) > + goto out; > + > + ret = vgic_its_save_collection_table(its); > + > +out: > + unlock_all_vcpus(kvm); > + mutex_unlock(&kvm->lock); > + return ret; > } > > /** > @@ -1684,7 +1740,34 @@ static int vgic_its_save_tables_v0(struct vgic_its *its) > */ > static int vgic_its_restore_tables_v0(struct vgic_its *its) > { > - return -ENXIO; > + struct kvm *kvm = its->dev->kvm; > + int ret; > + > + mutex_lock(&kvm->lock); > + > + if (!lock_all_vcpus(kvm)) { > + mutex_unlock(&kvm->lock); > + return -EBUSY; > + } > + > + ret = vgic_its_restore_collection_table(its); > + if (ret) > + goto out; > + > + ret = vgic_its_restore_device_tables(its); > + > +out: > + unlock_all_vcpus(kvm); > + mutex_unlock(&kvm->lock); > + > + if (ret) > + return ret; > + > + /* > + * On restore path, MSI injections can happen before the > + * first VCPU run so let's complete the GIC init here. > + */ > + return kvm_vgic_map_resources(its->dev->kvm); So not sure where we left this one in our other discussion. I feel like this is a bit out of place currently, and registering the iodevs should rather be done when setting the base addresses used by that, but I think we can fix that later, or are we implying ordering towards userspace here? Marc, any thoughts? Thanks, -Christoffer From mboxrd@z Thu Jan 1 00:00:00 1970 From: christoffer.dall@linaro.org (Christoffer Dall) Date: Fri, 5 May 2017 04:55:07 -0700 Subject: [PATCH v6 16/24] KVM: arm64: vgic-its: KVM_DEV_ARM_ITS_SAVE/RESTORE_TABLES In-Reply-To: <1493898284-29504-17-git-send-email-eric.auger@redhat.com> References: <1493898284-29504-1-git-send-email-eric.auger@redhat.com> <1493898284-29504-17-git-send-email-eric.auger@redhat.com> Message-ID: <20170505115507.GD1419@lvm> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Thu, May 04, 2017 at 01:44:36PM +0200, Eric Auger wrote: > Introduce new attributes in KVM_DEV_ARM_VGIC_GRP_CTRL group: > - KVM_DEV_ARM_ITS_SAVE_TABLES: saves the ITS tables into guest RAM > - KVM_DEV_ARM_ITS_RESTORE_TABLES: restores them into VGIC internal > structures. > > We hold the vcpus lock during the save and restore to make > sure no vcpu is running. > > At this stage the functionality is not yet implemented. Only > the skeleton is put in place. > > Signed-off-by: Eric Auger > > --- > v5 -> v6: > - remove the pending table sync from the ITS table restore > > v4 -> v5: > - use KVM_DEV_ARM_ITS_SAVE_TABLES and KVM_DEV_ARM_ITS_RESTORE_TABLES > - rename *flush* into *save* > - call its_sync_lpi_pending_table at the end of restore > - use abi framework > > v3 -> v4: > - pass kvm struct handle to vgic_its_flush/restore_pending_tables > - take the kvm lock and vcpu locks > - ABI revision check > - check attr->attr is null > > v1 -> v2: > - remove useless kvm parameter > --- > arch/arm/include/uapi/asm/kvm.h | 4 +- > arch/arm64/include/uapi/asm/kvm.h | 4 +- > virt/kvm/arm/vgic/vgic-its.c | 101 ++++++++++++++++++++++++++++++++++++-- > 3 files changed, 103 insertions(+), 6 deletions(-) > > diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h > index 4beb83b..8e6563c 100644 > --- a/arch/arm/include/uapi/asm/kvm.h > +++ b/arch/arm/include/uapi/asm/kvm.h > @@ -199,7 +199,9 @@ struct kvm_arch_memory_slot { > #define KVM_DEV_ARM_VGIC_LINE_LEVEL_INTID_MASK 0x3ff > #define VGIC_LEVEL_INFO_LINE_LEVEL 0 > > -#define KVM_DEV_ARM_VGIC_CTRL_INIT 0 > +#define KVM_DEV_ARM_VGIC_CTRL_INIT 0 > +#define KVM_DEV_ARM_ITS_SAVE_TABLES 1 > +#define KVM_DEV_ARM_ITS_RESTORE_TABLES 2 > > /* KVM_IRQ_LINE irq field index values */ > #define KVM_ARM_IRQ_TYPE_SHIFT 24 > diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h > index 7e8dd69..1e35115 100644 > --- a/arch/arm64/include/uapi/asm/kvm.h > +++ b/arch/arm64/include/uapi/asm/kvm.h > @@ -219,7 +219,9 @@ struct kvm_arch_memory_slot { > #define KVM_DEV_ARM_VGIC_LINE_LEVEL_INTID_MASK 0x3ff > #define VGIC_LEVEL_INFO_LINE_LEVEL 0 > > -#define KVM_DEV_ARM_VGIC_CTRL_INIT 0 > +#define KVM_DEV_ARM_VGIC_CTRL_INIT 0 > +#define KVM_DEV_ARM_ITS_SAVE_TABLES 1 > +#define KVM_DEV_ARM_ITS_RESTORE_TABLES 2 > > /* Device Control API on vcpu fd */ > #define KVM_ARM_VCPU_PMU_V3_CTRL 0 > diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c > index 3ad615a..5b251dc 100644 > --- a/virt/kvm/arm/vgic/vgic-its.c > +++ b/virt/kvm/arm/vgic/vgic-its.c > @@ -1669,12 +1669,68 @@ int vgic_its_attr_regs_access(struct kvm_device *dev, > } > > /** > + * vgic_its_save_device_tables - Save the device table and all ITT > + * into guest RAM > + */ > +static int vgic_its_save_device_tables(struct vgic_its *its) > +{ > + return -ENXIO; > +} > + > +/** > + * vgic_its_restore_device_tables - Restore the device table and all ITT > + * from guest RAM to internal data structs > + */ > +static int vgic_its_restore_device_tables(struct vgic_its *its) > +{ > + return -ENXIO; > +} > + > +/** > + * vgic_its_save_collection_table - Save the collection table into > + * guest RAM > + */ > +static int vgic_its_save_collection_table(struct vgic_its *its) > +{ > + return -ENXIO; > +} > + > +/** > + * vgic_its_restore_collection_table - reads the collection table > + * in guest memory and restores the ITS internal state. Requires the > + * BASER registers to be restored before. > + */ > +static int vgic_its_restore_collection_table(struct vgic_its *its) > +{ > + return -ENXIO; > +} > + > +/** > * vgic_its_save_tables_v0 - Save the ITS tables into guest ARM > * according to v0 ABI > */ > static int vgic_its_save_tables_v0(struct vgic_its *its) > { > - return -ENXIO; > + struct kvm *kvm = its->dev->kvm; > + int ret; > + > + mutex_lock(&kvm->lock); > + > + if (!lock_all_vcpus(kvm)) { > + mutex_unlock(&kvm->lock); > + return -EBUSY; > + } > + > + ret = vgic_its_save_device_tables(its); > + if (ret) > + goto out; > + > + ret = vgic_its_save_collection_table(its); > + > +out: > + unlock_all_vcpus(kvm); > + mutex_unlock(&kvm->lock); > + return ret; > } > > /** > @@ -1684,7 +1740,34 @@ static int vgic_its_save_tables_v0(struct vgic_its *its) > */ > static int vgic_its_restore_tables_v0(struct vgic_its *its) > { > - return -ENXIO; > + struct kvm *kvm = its->dev->kvm; > + int ret; > + > + mutex_lock(&kvm->lock); > + > + if (!lock_all_vcpus(kvm)) { > + mutex_unlock(&kvm->lock); > + return -EBUSY; > + } > + > + ret = vgic_its_restore_collection_table(its); > + if (ret) > + goto out; > + > + ret = vgic_its_restore_device_tables(its); > + > +out: > + unlock_all_vcpus(kvm); > + mutex_unlock(&kvm->lock); > + > + if (ret) > + return ret; > + > + /* > + * On restore path, MSI injections can happen before the > + * first VCPU run so let's complete the GIC init here. > + */ > + return kvm_vgic_map_resources(its->dev->kvm); So not sure where we left this one in our other discussion. I feel like this is a bit out of place currently, and registering the iodevs should rather be done when setting the base addresses used by that, but I think we can fix that later, or are we implying ordering towards userspace here? Marc, any thoughts? Thanks, -Christoffer