From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marc Zyngier Subject: Re: [PATCH v5 01/22] KVM: arm/arm64: Add ITS save/restore API documentation Date: Thu, 4 May 2017 08:40:40 +0100 Message-ID: <73c4e7c5-14f0-8d47-cd30-7fd072a8f365@arm.com> References: <1492164934-988-1-git-send-email-eric.auger@redhat.com> <1492164934-988-2-git-send-email-eric.auger@redhat.com> <20170426123120.GL4104@cbox> <88a194fa-9143-960e-e0a6-6b5c388cef82@redhat.com> <20170427085709.GB50776@lvm> <20170427110223.GH50776@lvm> <20170427144516.GL50776@lvm> <3c16c95a-6bbc-a25c-7dc2-b4fd13685327@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Cc: Christoffer Dall , drjones@redhat.com, kvm@vger.kernel.org, peter.maydell@linaro.org, andre.przywara@arm.com, quintela@redhat.com, dgilbert@redhat.com, Vijaya.Kumar@cavium.com, vijayak@caviumnetworks.com, pbonzini@redhat.com, Prasun.Kapoor@cavium.com, kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org, eric.auger.pro@gmail.com To: Auger Eric , Christoffer Dall Return-path: Received: from foss.arm.com ([217.140.101.70]:35558 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751953AbdEDHku (ORCPT ); Thu, 4 May 2017 03:40:50 -0400 In-Reply-To: <3c16c95a-6bbc-a25c-7dc2-b4fd13685327@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On 04/05/17 08:00, Auger Eric wrote: > Hi Christoffer, > > On 27/04/2017 16:45, Christoffer Dall wrote: >> Hi Eric, >> >> On Thu, Apr 27, 2017 at 02:51:00PM +0200, Auger Eric wrote: >>> On 27/04/2017 13:02, Christoffer Dall wrote: >>>> On Thu, Apr 27, 2017 at 11:33:39AM +0200, Auger Eric wrote: >>>>> On 27/04/2017 10:57, Christoffer Dall wrote: >>>>>> On Wed, Apr 26, 2017 at 05:48:32PM +0200, Auger Eric wrote: >>>>>>> On 26/04/2017 14:31, Christoffer Dall wrote: >>>>>>>> On Fri, Apr 14, 2017 at 12:15:13PM +0200, Eric Auger wrote: >>>>>>>>> Add description for how to access ITS registers and how to save/restore >>>>>>>>> ITS tables into/from memory. >>>>>>>>> >>>>>>>>> Signed-off-by: Eric Auger >>>>>>>>> >>>>>>>>> --- >>>>>>>>> v4 -> v5: >>>>>>>>> - take into account Christoffer's comments >>>>>>>>> - pending table save on GICV3 side now >>>>>>>>> >>>>>>>>> v3 -> v4: >>>>>>>>> - take into account Peter's comments: >>>>>>>>> - typos >>>>>>>>> - KVM_DEV_ARM_VGIC_GRP_ITS_TABLES kvm_device_attr = 0 >>>>>>>>> - add a validity bit in DTE >>>>>>>>> - document all fields in CTE and ITE >>>>>>>>> - document ABI revision >>>>>>>>> - take into account Andre's comments: >>>>>>>>> - document restrictions about GITS_CREADR writing and GITS_IIDR >>>>>>>>> - document -EBUSY error if one or more VCPUS are runnning >>>>>>>>> - document 64b registers only can be accessed with 64b access >>>>>>>>> - itt_addr field matches bits [51:8] of the itt_addr >>>>>>>>> >>>>>>>>> v1 -> v2: >>>>>>>>> - DTE and ITE now are 8 bytes >>>>>>>>> - DTE and ITE now indexed by deviceid/eventid >>>>>>>>> - use ITE name instead of ITTE >>>>>>>>> - mentions ITT_addr matches bits [51:8] of the actual address >>>>>>>>> - mentions LE layout >>>>>>>>> --- >>>>>>>>> Documentation/virtual/kvm/devices/arm-vgic-its.txt | 99 ++++++++++++++++++++++ >>>>>>>>> 1 file changed, 99 insertions(+) >>>>>>>>> >>>>>>>>> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt >>>>>>>>> index 6081a5b..b5f010d 100644 >>>>>>>>> --- a/Documentation/virtual/kvm/devices/arm-vgic-its.txt >>>>>>>>> +++ b/Documentation/virtual/kvm/devices/arm-vgic-its.txt >>>>>>>>> @@ -32,7 +32,106 @@ Groups: >>>>>>>>> KVM_DEV_ARM_VGIC_CTRL_INIT >>>>>>>>> request the initialization of the ITS, no additional parameter in >>>>>>>>> kvm_device_attr.addr. >>>>>>>>> + >>>>>>>>> + KVM_DEV_ARM_ITS_SAVE_TABLES >>>>>>>>> + save the ITS table data into guest RAM, at the location provisioned >>>>>>>>> + by the guest in corresponding registers/table entries. >>>>>>>>> + >>>>>>>>> + The layout of the tables in guest memory defines an ABI. The entries >>>>>>>>> + are laid out in little endian format as described in the last paragraph. >>>>>>>>> + >>>>>>>>> + KVM_DEV_ARM_ITS_RESTORE_TABLES >>>>>>>>> + restore the ITS tables from guest RAM to ITS internal structures. >>>>>>>>> + >>>>>>>>> + The GICV3 must be restored before the ITS and all ITS registers but >>>>>>>>> + the GITS_CTLR must be restored before restoring the ITS tables. >>>>>>>>> + >>>>>>>>> + The GITS_IIDR read-only register must also be restored before >>>>>>>>> + the table restore as the IIDR revision field encodes the ABI revision. >>>>>>>>> + >>>>>>>> >>>>>>>> what is the expected sequence of operations. For example, to restore >>>>>>>> the ITS, do I call KVM_DEV_ARM_VGIC_CTRL_INIT first, then restore all >>>>>>>> the memory and registers, and finally call KVM_DEV_ARM_ITS_RESTORE_TABLES? >>>>>>> Yes KVM_DEV_ARM_VGIC_CTRL_INIT comes first, then restore all registers >>>>>>> except GITS_CTLR, then table restore, then GITS_CTLR >>>>>>>> >>>>>>>> Is there any interaction between when you call KVM_DEV_ARM_ITS_RESTORE_TABLES >>>>>>>> and restore GITS_CTLR (which enables the ITS)? >>>>>>> >>>>>>> Yep, when GITS_CTLR is set, LPIs may be enabled and this on that event >>>>>>> that the pending table is read. But the whole pending table is not read >>>>>>> as we only iterate on registered LPIs. So the ITT must have been >>>>>>> restored previously. >>>>>>> >>>>>>> I became aware that the pending table sync is done twice, once in the >>>>>>> pending table restore, and once in the GITS_CTLR restore. So if we >>>>>>> leave this order specification, I should be able to remove the sync on >>>>>>> table restore. This was the original reason why GITS_CTLR restore has >>>>>>> been done at the very end. >>>>>> >>>>>> I'm sorry, I'm a bit confused. Do we not need >>>>>> KVM_DEV_ARM_ITS_RESTORE_TABLES at all then? >>>>> >>>>> Yes you do. I was talking about the RDIST pending table sync. The save >>>>> is explicit using GICV3 device KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES. >>>>> However the sync is implicit on GITS_CTLR restore if LPIs are enabled. >>>>> and today I do it also on ITS device KVM_DEV_ARM_ITS_RESTORE_TABLES >>>>> which is not requested I think since GITS_CTLR restore does it already. >>>> >>>> Shouldn't restoring the pending tables happen when restoring some >>>> redeistributor state and not anything related to the ITS? >>> >>> Marc wrote: >>> " >>> I don't think you necessarily need a coarse map. When restoring the ITS >>> tables, you can always read the pending bit when creating the LPI >>> structure (it has been written to RAM at save time). Note that we >>> already do something like this in vgic_enable_lpis(). >>> " >>> >>> This is currently what is implemented I think. the pending tables are >>> currently sync'ed on GITS_CTLR set (if LPI are enabled) + erroneously >>> also on on ITS table restore >>> >>> The problematic is: Either you know in advance which LPI INTIDare used >>> or you need to parse the whole pending table (possibly using the 1st kB >>> as coarse mapping). >>> >>> If you don't know the LPI INTIDs in advance it is only possible to >>> restore the pending bit of pending LPIs. At that time you would >>> re-allocate those pending LPI (vgic_add_lpi) and when you restore the >>> ITS ITT you would do the same for those which were not pending. Looks >>> really heavy to me: coarse mapping + dual vgic_add_lpi path. >>> >>> Otherwise we would need to add another dependency between RDIST pending >>> table restore and ITS table restore but this looks even more weird, no? >>> >>> >> So I just sat down with Andre and Marc and we tried to work through this >> and came up with the best scheme. I apologize in advance for the >> one-way nature of this e-mail, and I am of course open to discussing the >> following proposal again if you do not agree. >> >> What I think this document should say, is that the following ordering >> must be followed when restoring the GIC and the ITS: >> >> First, restore all guest memory >> >> Second, restore ALL redistributors >> >> Third, restore the ITS, in the following order: >> 1. Initialize the ITS (KVM_DEV_ARM_VGIC_CTRL_INIT) >> 2. Restore GITS_CBASER >> 3. Restore all other GITS_ registers, except GITS_CTLR! >> 4. Load the ITS table data (KVM_DEV_ARM_ITS_RESTORE_TABLES) >> 5. Restore GITS_CTLR >> >> The rationale is that we really want the redistributor and the ITS >> restore to be independent and follow the architecture. This means that >> our ABI for the redistributor should still work without restoring an ITS >> (if we ever decide to support LPIs for KVM without the ITS). >> >> In terms of our current implementation this means that vgic_add_lpi() >> should ask the redistributor what the state of the LPI is (priority, >> enabled, pending). I suggest you do the pending check by adding a >> function called something like vgic_v3_lpi_is_pending() which scans the >> bit in memory, clears the memory bit, and returns the value. Clearing >> the pending bit in memory when moving it to the struct irq is nice, >> because you then don't have to clear out the entire pending table later >> and we don't keep 'consumed' data lying around. This change should be >> implemented in its_sync_lpi_pending_table() as well, but note that you >> need never call that function in the normal restore path using this >> design. >> >> I hope this makes sense. > > I am dubious about the above changes at the moment. > its_sync_lpi_pending_table() gets called on GITS_CTLR setting which is > documented to be the last step of the restoration. I wonder why the > above changes cannot be part of another series later on. I think that's one of the issues. See below. > Consuming the RAM bit status means we record it in irq->pending_latch so > I guess we should have the irq->pending_latch setting in the same > function as the one that retrieves the bit status in guest RAM. So I > would rename vgic_v3_lpi_is_pending into something like > int vgic_v3_sync_lpi_pending_status(struct kvm *kvm, u32 intid) > Since this covers a single LPI, the removes the byte access optimization > found in its_sync_lpi_pending_table Well, never mind the optimization. How many LPIs are we restoring in a typical VM? 10? 1000? That's just one byte access per LPI. Of course, I'd rather have fewer guest memory accesses, but a restore is an incredibly rare event, so I'm not too bothered about the extra usec! ;-) > > Also if I understand it correctly this means the sync will be done on > both add_lpi and GITS_CTLR setting Why GITS_CTLR? The Enable bit only controls the effect of GITS_TRANSLATER... I believe that vgic_add_lpi() is the only point where we should snapshot the pending state. Thanks, M. -- Jazz is not dead. It just smells funny... From mboxrd@z Thu Jan 1 00:00:00 1970 From: marc.zyngier@arm.com (Marc Zyngier) Date: Thu, 4 May 2017 08:40:40 +0100 Subject: [PATCH v5 01/22] KVM: arm/arm64: Add ITS save/restore API documentation In-Reply-To: <3c16c95a-6bbc-a25c-7dc2-b4fd13685327@redhat.com> References: <1492164934-988-1-git-send-email-eric.auger@redhat.com> <1492164934-988-2-git-send-email-eric.auger@redhat.com> <20170426123120.GL4104@cbox> <88a194fa-9143-960e-e0a6-6b5c388cef82@redhat.com> <20170427085709.GB50776@lvm> <20170427110223.GH50776@lvm> <20170427144516.GL50776@lvm> <3c16c95a-6bbc-a25c-7dc2-b4fd13685327@redhat.com> Message-ID: <73c4e7c5-14f0-8d47-cd30-7fd072a8f365@arm.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 04/05/17 08:00, Auger Eric wrote: > Hi Christoffer, > > On 27/04/2017 16:45, Christoffer Dall wrote: >> Hi Eric, >> >> On Thu, Apr 27, 2017 at 02:51:00PM +0200, Auger Eric wrote: >>> On 27/04/2017 13:02, Christoffer Dall wrote: >>>> On Thu, Apr 27, 2017 at 11:33:39AM +0200, Auger Eric wrote: >>>>> On 27/04/2017 10:57, Christoffer Dall wrote: >>>>>> On Wed, Apr 26, 2017 at 05:48:32PM +0200, Auger Eric wrote: >>>>>>> On 26/04/2017 14:31, Christoffer Dall wrote: >>>>>>>> On Fri, Apr 14, 2017 at 12:15:13PM +0200, Eric Auger wrote: >>>>>>>>> Add description for how to access ITS registers and how to save/restore >>>>>>>>> ITS tables into/from memory. >>>>>>>>> >>>>>>>>> Signed-off-by: Eric Auger >>>>>>>>> >>>>>>>>> --- >>>>>>>>> v4 -> v5: >>>>>>>>> - take into account Christoffer's comments >>>>>>>>> - pending table save on GICV3 side now >>>>>>>>> >>>>>>>>> v3 -> v4: >>>>>>>>> - take into account Peter's comments: >>>>>>>>> - typos >>>>>>>>> - KVM_DEV_ARM_VGIC_GRP_ITS_TABLES kvm_device_attr = 0 >>>>>>>>> - add a validity bit in DTE >>>>>>>>> - document all fields in CTE and ITE >>>>>>>>> - document ABI revision >>>>>>>>> - take into account Andre's comments: >>>>>>>>> - document restrictions about GITS_CREADR writing and GITS_IIDR >>>>>>>>> - document -EBUSY error if one or more VCPUS are runnning >>>>>>>>> - document 64b registers only can be accessed with 64b access >>>>>>>>> - itt_addr field matches bits [51:8] of the itt_addr >>>>>>>>> >>>>>>>>> v1 -> v2: >>>>>>>>> - DTE and ITE now are 8 bytes >>>>>>>>> - DTE and ITE now indexed by deviceid/eventid >>>>>>>>> - use ITE name instead of ITTE >>>>>>>>> - mentions ITT_addr matches bits [51:8] of the actual address >>>>>>>>> - mentions LE layout >>>>>>>>> --- >>>>>>>>> Documentation/virtual/kvm/devices/arm-vgic-its.txt | 99 ++++++++++++++++++++++ >>>>>>>>> 1 file changed, 99 insertions(+) >>>>>>>>> >>>>>>>>> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt >>>>>>>>> index 6081a5b..b5f010d 100644 >>>>>>>>> --- a/Documentation/virtual/kvm/devices/arm-vgic-its.txt >>>>>>>>> +++ b/Documentation/virtual/kvm/devices/arm-vgic-its.txt >>>>>>>>> @@ -32,7 +32,106 @@ Groups: >>>>>>>>> KVM_DEV_ARM_VGIC_CTRL_INIT >>>>>>>>> request the initialization of the ITS, no additional parameter in >>>>>>>>> kvm_device_attr.addr. >>>>>>>>> + >>>>>>>>> + KVM_DEV_ARM_ITS_SAVE_TABLES >>>>>>>>> + save the ITS table data into guest RAM, at the location provisioned >>>>>>>>> + by the guest in corresponding registers/table entries. >>>>>>>>> + >>>>>>>>> + The layout of the tables in guest memory defines an ABI. The entries >>>>>>>>> + are laid out in little endian format as described in the last paragraph. >>>>>>>>> + >>>>>>>>> + KVM_DEV_ARM_ITS_RESTORE_TABLES >>>>>>>>> + restore the ITS tables from guest RAM to ITS internal structures. >>>>>>>>> + >>>>>>>>> + The GICV3 must be restored before the ITS and all ITS registers but >>>>>>>>> + the GITS_CTLR must be restored before restoring the ITS tables. >>>>>>>>> + >>>>>>>>> + The GITS_IIDR read-only register must also be restored before >>>>>>>>> + the table restore as the IIDR revision field encodes the ABI revision. >>>>>>>>> + >>>>>>>> >>>>>>>> what is the expected sequence of operations. For example, to restore >>>>>>>> the ITS, do I call KVM_DEV_ARM_VGIC_CTRL_INIT first, then restore all >>>>>>>> the memory and registers, and finally call KVM_DEV_ARM_ITS_RESTORE_TABLES? >>>>>>> Yes KVM_DEV_ARM_VGIC_CTRL_INIT comes first, then restore all registers >>>>>>> except GITS_CTLR, then table restore, then GITS_CTLR >>>>>>>> >>>>>>>> Is there any interaction between when you call KVM_DEV_ARM_ITS_RESTORE_TABLES >>>>>>>> and restore GITS_CTLR (which enables the ITS)? >>>>>>> >>>>>>> Yep, when GITS_CTLR is set, LPIs may be enabled and this on that event >>>>>>> that the pending table is read. But the whole pending table is not read >>>>>>> as we only iterate on registered LPIs. So the ITT must have been >>>>>>> restored previously. >>>>>>> >>>>>>> I became aware that the pending table sync is done twice, once in the >>>>>>> pending table restore, and once in the GITS_CTLR restore. So if we >>>>>>> leave this order specification, I should be able to remove the sync on >>>>>>> table restore. This was the original reason why GITS_CTLR restore has >>>>>>> been done at the very end. >>>>>> >>>>>> I'm sorry, I'm a bit confused. Do we not need >>>>>> KVM_DEV_ARM_ITS_RESTORE_TABLES at all then? >>>>> >>>>> Yes you do. I was talking about the RDIST pending table sync. The save >>>>> is explicit using GICV3 device KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES. >>>>> However the sync is implicit on GITS_CTLR restore if LPIs are enabled. >>>>> and today I do it also on ITS device KVM_DEV_ARM_ITS_RESTORE_TABLES >>>>> which is not requested I think since GITS_CTLR restore does it already. >>>> >>>> Shouldn't restoring the pending tables happen when restoring some >>>> redeistributor state and not anything related to the ITS? >>> >>> Marc wrote: >>> " >>> I don't think you necessarily need a coarse map. When restoring the ITS >>> tables, you can always read the pending bit when creating the LPI >>> structure (it has been written to RAM at save time). Note that we >>> already do something like this in vgic_enable_lpis(). >>> " >>> >>> This is currently what is implemented I think. the pending tables are >>> currently sync'ed on GITS_CTLR set (if LPI are enabled) + erroneously >>> also on on ITS table restore >>> >>> The problematic is: Either you know in advance which LPI INTIDare used >>> or you need to parse the whole pending table (possibly using the 1st kB >>> as coarse mapping). >>> >>> If you don't know the LPI INTIDs in advance it is only possible to >>> restore the pending bit of pending LPIs. At that time you would >>> re-allocate those pending LPI (vgic_add_lpi) and when you restore the >>> ITS ITT you would do the same for those which were not pending. Looks >>> really heavy to me: coarse mapping + dual vgic_add_lpi path. >>> >>> Otherwise we would need to add another dependency between RDIST pending >>> table restore and ITS table restore but this looks even more weird, no? >>> >>> >> So I just sat down with Andre and Marc and we tried to work through this >> and came up with the best scheme. I apologize in advance for the >> one-way nature of this e-mail, and I am of course open to discussing the >> following proposal again if you do not agree. >> >> What I think this document should say, is that the following ordering >> must be followed when restoring the GIC and the ITS: >> >> First, restore all guest memory >> >> Second, restore ALL redistributors >> >> Third, restore the ITS, in the following order: >> 1. Initialize the ITS (KVM_DEV_ARM_VGIC_CTRL_INIT) >> 2. Restore GITS_CBASER >> 3. Restore all other GITS_ registers, except GITS_CTLR! >> 4. Load the ITS table data (KVM_DEV_ARM_ITS_RESTORE_TABLES) >> 5. Restore GITS_CTLR >> >> The rationale is that we really want the redistributor and the ITS >> restore to be independent and follow the architecture. This means that >> our ABI for the redistributor should still work without restoring an ITS >> (if we ever decide to support LPIs for KVM without the ITS). >> >> In terms of our current implementation this means that vgic_add_lpi() >> should ask the redistributor what the state of the LPI is (priority, >> enabled, pending). I suggest you do the pending check by adding a >> function called something like vgic_v3_lpi_is_pending() which scans the >> bit in memory, clears the memory bit, and returns the value. Clearing >> the pending bit in memory when moving it to the struct irq is nice, >> because you then don't have to clear out the entire pending table later >> and we don't keep 'consumed' data lying around. This change should be >> implemented in its_sync_lpi_pending_table() as well, but note that you >> need never call that function in the normal restore path using this >> design. >> >> I hope this makes sense. > > I am dubious about the above changes at the moment. > its_sync_lpi_pending_table() gets called on GITS_CTLR setting which is > documented to be the last step of the restoration. I wonder why the > above changes cannot be part of another series later on. I think that's one of the issues. See below. > Consuming the RAM bit status means we record it in irq->pending_latch so > I guess we should have the irq->pending_latch setting in the same > function as the one that retrieves the bit status in guest RAM. So I > would rename vgic_v3_lpi_is_pending into something like > int vgic_v3_sync_lpi_pending_status(struct kvm *kvm, u32 intid) > Since this covers a single LPI, the removes the byte access optimization > found in its_sync_lpi_pending_table Well, never mind the optimization. How many LPIs are we restoring in a typical VM? 10? 1000? That's just one byte access per LPI. Of course, I'd rather have fewer guest memory accesses, but a restore is an incredibly rare event, so I'm not too bothered about the extra usec! ;-) > > Also if I understand it correctly this means the sync will be done on > both add_lpi and GITS_CTLR setting Why GITS_CTLR? The Enable bit only controls the effect of GITS_TRANSLATER... I believe that vgic_add_lpi() is the only point where we should snapshot the pending state. Thanks, M. -- Jazz is not dead. It just smells funny...