Hi Eric,
Most of my comments below are just me being picky about text when
defining a user space ABI, so I think this mainly looks good, but just
needs a bit of clarify, except the versioning aspect and exporting the
pending table vie the redistributor instead, as Marc and Andre have
pointed out.
On Mon, Mar 27, 2017 at 11:30:51AM +0200, Eric Auger wrote:
> Add description for how to access vITS registers and how to flush/restore
> vITS tables into/from memory
>
> Signed-off-by: Eric Auger <eric.auger@xxxxxxxxxx>
>
> ---
> 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 | 118 +++++++++++++++++++++
> 1 file changed, 118 insertions(+)
>
> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> index 6081a5b..0902d20 100644
> --- a/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> +++ b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> @@ -36,3 +36,121 @@ Groups:
> -ENXIO: ITS not properly configured as required prior to setting
> this attribute
> -ENOMEM: Memory shortage when allocating ITS internal data
> +
> + 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 CWRITER setting. Writing this register is
^^^^^^^^^^^^^^^
"after restoring CWRITER." ?
> + allowed if the ITS is not enabled (GITS_CTLR.enable = 0). Also it
Does that mean that you can only save/restore a disabled ITS or does it
mean that initially after creating the ITS it is disabled and userspace
should restore the CWRITER before restoring GITS_CTLR (which may enable
the ITS) ?
> + 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.
How is this really going to work? Your ABI here must be backwards
compatible for future revisions, so what is userspace supposed to do,
when it reads a newer revision than it was programmed for?
I think we need a more clear description of how the revision is going to
be used, such that each operation on the ITS here is described as
requiring a minimum revision X, and making sure that userspace can
safely ignore things that are of a higher revision number, while at the
same time userspace can decide not to use newer features with older
kernels.
> +
> + 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
> +
> + KVM_DEV_ARM_VGIC_GRP_ITS_TABLES
Shouldn't this be called KVM_DEV_ARM_VGIC_GRP_FLUSH_ITS_TABLES ?
> + Attributes
> + The attr field of kvm_device_attr must be zero.
> +
> + request the flush-save/restore of the ITS tables, namely
Nit: Request (upper case R)
what does flush-save/restore mean as opposed to just flush?
> + the device table, the collection table, all the ITT tables,
> + the LPI pending tables. On save, the tables are flushed
, and the LPI pending table.
> + into guest memory at the location provisioned by the guest
> + in GITS_BASER (device and collection tables), in the MAPD
> + command (ITT_addr), GICR_PENDBASERs (pending tables).
> +
> + This means the GIC should be restored before the ITS and all
should or must? Is this enforced?
> + ITS registers but the GITS_CTLR must be restored before
> + restoring the ITS tables.
> +
> + The GITS_READR and GITS_IIDR read-only registers must also
> + be restored before the table restore. The IIDR revision field
> + encodes the ABI revision of the table layout. If not set by
> + user space, the restoration of the tables will fail.
consider rewording: ", restoring the ITS tables will fail."
> +
> + Note the LPI configuration table is read-only for the
Note that
> + in-kernel ITS and its save/restore goes through the standard
and saving/restoring it is done via the normal process to save/restore
guest RAM.
> + RAM save/restore.
> +
> + The layout of the tables in guest memory defines an ABI.
> + The entries are laid in little endian format as follows;
s/;/:/
It's a bit weird to say "as follows:" and then proceed with the error
descriptions. I would simply say "as described in the following
paragraph."
> +
> + Errors:
> + -EINVAL: kvm_device_attr not equal to 0, invalid table data
> + -EFAULT: invalid guest ram access
> + -EBUSY: one or more VCPUS are running
> +
> + ITS Table ABI REV1:
> + -------------------
> +
> + The device table and ITE are respectively indexed by device id and
s/ITE/ITT/
are indexed by the device id and eventid, respectively.
> + eventid. The collection table however is not indexed by collection id:
...by collection id, instead all the CTEs are written...
> + CTE are written at the beginning of the buffer.
in any particular order, or?
> +
> + Device Table Entry (DTE) layout: entry size = 8 bytes
> +
> + bits: | 63| 62 ... 49 | 48 ... 5 | 4 ... 0 |
> + values: | V | next | ITT_addr | Size |
> +
> + where;
> + - V indicates whether the entry is valid,
> + - ITT_addr matches bits [51:8] of the ITT address (256B aligned),
> + - next field is meaningful only if the entry is valid.
is the ITT_addr meaningful if the entry is not valid?
> + It equals to 0 if this entry is the last one; otherwise it corresponds
> + to the minimum between the offset to the next device id and 2^14 -1.
I don't easily understand this last paragraph and the indentation is
weird and makes it look like it's not explaining the next field.
You're missing a description of the size field. Size in what unit?
Size of what?
> +
> + Collection Table Entry (CTE) layout: entry size = 8 bytes
> +
> + bits: | 63| 62 .. 52 | 51 ... 16 | 15 ... 0 |
> + values: | V | RES0 | RDBase | ICID |
> +
> + where:
> + - V indicates whether the entry is valid,
Do we explain anywhere what RES0 means?
> + - RDBase matches the PE number (GICR_TYPER.Processor_Number),
is 'matches' the right verb to use here?
What exactly is the format of GICR_TYPER.Processor_Number ?
> + - ICID matches the collection ID
again, is 'matches' the right verb to use here?
> +
> + Interrupt Translation Entry (ITE) layout: entry size = 8 bytes
> +
> + bits: | 63 ... 48 | 47 ... 16 | 15 ... 0 |
> + values: | next | pINTID | ICID |
> +
> + where:
> + - pINTID is the physical LPI ID,
> + - ICID is the collection ID,
here you use 'is' intead of 'matches'. At leat be consistent.
> + - next field is meaningful only if the entry is valid (pINTID != 0).
> + It equals to 0 if this entry is the last one; otherwise it corresponds
> + to the minimum between the eventid offset to the next ITE and 2^16 -1.
same comments, as above.
also, can you list the field in the order they appear?
> +
> + LPI Pending Table layout:
> +
> + As specified in the ARM Generic Interrupt Controller Architecture
> + Specification GIC Architecture version 3.0 and version 4. The first
"...version 4, the first". ("As specified in X." is not a sentence).
> + 1kB is not modified and therefore should contain zeroes.
should or must? or always contains zeros? Will you return an error if
userspace puts something non-zero in there?
> +
> + Future evolutions of the ITS table layout:
> +
> + At the moment the table layout is defined and optimized for physical
> + LPI support.
This comment is a bit confusing, because this is all about virtual
interrupts really, so 'physical from the point of view of the VM', but I
think you should just drop this sentence.
> +
> + In the future we might implement direct injection of virtual LPIS.
For nesting, yes? (on the host this should not be visible here, should
it?)
> + This will require an upgrade of the table layout and an evolution of
> + the ABI. The ABI revision is encoded in the GITS_IIDR revision field.
> + That register must be restored before the table restoration, otherwise
> + the operation will fail.
Hmm, I thought we dealt with that before, feels a bit out of place.
> +
> + ABI V1: GITS_IIDR.Revision = 1
I feel like this should go in the ITS register description of the IIDR.
Most likely, I think this particular line can be dropped, but all other
definitions in this file can be annoted with a minimum revision number
ensuring that future revisions implement this, plus potentially more
stuff.
Thanks,
-Christoffer
