Re: [PATCH v5 01/22] KVM: arm/arm64: Add ITS save/restore API documentation

[Christoffer Dall <christoffer.dall@linaro.org>]

On Wed, Apr 26, 2017 at 05:48:32PM +0200, Auger Eric wrote:
> Hi Christoffer,
> 
> 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@redhat.com>
> >>
> >> ---
> >> 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?

> > 
> >>    Errors:
> >>      -ENXIO:  ITS not properly configured as required prior to setting
> >>               this attribute
> >>      -ENOMEM: Memory shortage when allocating ITS internal data
> >> +    -EINVAL: Inconsistent restored data
> >> +    -EFAULT: Invalid guest ram access
> >> +    -EBUSY:  One or more VCPUS are running
> >> +
> >> +  KVM_DEV_ARM_VGIC_GRP_ITS_REGS
> >> +  Attributes:
> >> +      The attr field of kvm_device_attr encodes the offset of the
> >> +      ITS register, relative to the ITS control frame base address
> >> +      (ITS_base).
> >> +
> >> +      kvm_device_attr.addr points to a __u64 value whatever the width
> >> +      of the addressed register (32/64 bits). 64 bit registers can only
> >> +      be accessed with full length.
> >> +
> >> +      Writes to read-only registers are ignored by the kernel except for:
> >> +      - GITS_READR. It needs to be restored otherwise commands in the queue
> >> +        will be re-executed after restoring CWRITER. GITS_READR must be restored
> >> +        before restoring the GITS_CTLR which is likely to enable the ITS.
> >> +        Also it needs to be restored after GITS_CBASER since a write to
> >> +        GITS_CBASER resets GITS_CREADR.
> >> +      - GITS_IIDR. Its Revision field encodes the table layout ABI revision.
> >> +        In the future we might implement direct injection of virtual LPIS.
> >> +        This will require an upgrade of the table layout and an evolution of
> >> +        the ABI. GITS_IIDR must be restored before the table restoration.
> >> +
> >> +      For other registers, getting or setting a register has the same
> >> +      effect as reading/writing the register on real hardware.
> >> +  Errors:
> >> +    -ENXIO: Offset does not correspond to any supported register
> >> +    -EFAULT: Invalid user pointer for attr->addr
> >> +    -EINVAL: Offset is not 64-bit aligned
> >> +    -EBUSY: one or more VCPUS are running
> > 
> > 
> > It may be helpful to state the ordering requirements somewhere:
> > 
> > Restoring the ITS:
> > ------------------
> > Restoring the ITS requires certain things to happen in order.
> > Specifically:
> >  1. Initialize the ITS (KVM_DEV_ARM_VGIC_CTRL_INIT)
> >  2. Restore GITS_IIDR
> >  3. Restore GITS_CBASER
> >  4. Restore GITS_READR
> >  5. Restore remainin registers except GITS_CTLR
> >  6. Make sure all guest memory is restored
> >  7. Load the ITS table data (KVM_DEV_ARM_ITS_RESTORE_TABLES)
> 
> OK I will try to fit that description somewhere.
> 

Thanks,
-Christoffer