On 04/05/17 08:00, Auger Eric wrote:

Hi Christoffer,

On 27/04/2017 16:45, Christoffer Dall wrote:

quoted

Hi Eric,

On Thu, Apr 27, 2017 at 02:51:00PM +0200, Auger Eric wrote:

quoted

On 27/04/2017 13:02, Christoffer Dall wrote:

quoted

On Thu, Apr 27, 2017 at 11:33:39AM +0200, Auger Eric wrote:

quoted

On 27/04/2017 10:57, Christoffer Dall wrote:

quoted

On Wed, Apr 26, 2017 at 05:48:32PM +0200, Auger Eric wrote:

quoted

On 26/04/2017 14:31, Christoffer Dall wrote:

quoted

On Fri, Apr 14, 2017 at 12:15:13PM +0200, Eric Auger wrote:

quoted

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

quoted

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...