On Thu, Apr 27, 2017 at 07:14:29PM +0200, Auger Eric wrote: > Hi Marc, Christoffer, > > On 27/04/2017 18:23, Marc Zyngier wrote: > > On 27/04/17 16:29, Auger Eric wrote: > >> > >> > >> 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 <eric.auger@xxxxxxxxxx> > >>>>>>>>>> > >>>>>>>>>> --- > >>>>>>>>>> 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). > >> > >> OK. Note I already mentioned that GICv3 must be restored before the ITS. > >> To me this comprised the RDIST. > >> > >> I understand the above description of the ordering comes in addition to > >> the existing text, right? in other words I keep the GITS_READR, > >> GITS_IIDR specific text as well as KVM_DEV_ARM_ITS_SAVE/RESTORE_TABLES > >> section. > >> > >>> > >>> 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). > >> this practically means I move update_lpi_config call from > >> vgic_its_restore_ite to vgic_add_lpi(). OK > >> > >> However for getting the LPI pending state I must know which RDIST the > >> LPI is attached to. This is not known at LPI allocation time. Do I > >> misunderstand something? > > > > Once you have rebuilt the ITS data structures and allocated the IRQ > > structures, you should have a target_cpu field pointing to the right > > vcpu. From there, you can surely find the corresponding redistributor > > and the pending table. > Yes that's understood but Christoffer said "vgic_add_lpi() should ask > the redistributor what the state of the LPI is (priority,enabled, > pending)." Fetching the properties is fine. > > vgic_add_lpi() is called before update_affinity_ite() which uses > ite->irq and sets the target_vcpu. > > Well at least this requires some function reshape. I will investigate > though. Thanks. If it looks impossible, let me know, and I can help having a look at the code. -Christoffer
