Hi Marc, On 12/01/2017 17:52, Marc Zyngier wrote: > Hi Eric, > > On 12/01/17 15:56, 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> >> --- >> Documentation/virtual/kvm/devices/arm-vgic-its.txt | 70 ++++++++++++++++++++++ >> 1 file changed, 70 insertions(+) >> >> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt >> index 6081a5b..bd74613 100644 >> --- a/Documentation/virtual/kvm/devices/arm-vgic-its.txt >> +++ b/Documentation/virtual/kvm/devices/arm-vgic-its.txt >> @@ -36,3 +36,73 @@ 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). >> + >> + Writes to read-only registers are ignored by the kernel except >> + for a single register, GITS_READR. Normally this register is RO >> + but it needs to be restored otherwise commands in the queue will >> + be re-executed after CWRITER setting. >> + >> + 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 >> + >> + KVM_DEV_ARM_VGIC_GRP_ITS_TABLES >> + Attributes >> + The attr field of kvm_device_attr is not used. >> + >> + request the flush-save/restore of the ITS tables, namely >> + the device table, the collection table, all the ITT tables, >> + the LPI pending tables. On save, the tables are flushed >> + into guest memory at the location provisionned by the guest > > provisioned > >> + in GITS_BASER (device and collection tables), on MAPD command >> + (ITT_addr), GICR_PENDBASERs (pending tables). >> + >> + This means the GIC should be restored before the ITS and all >> + ITS registers but the GITS_CTRL must be restored before >> + restoring the ITS tables. >> + >> + Note the LPI configuration table is read-only for the >> + in-kernel ITS and its save/restore goes through the standard >> + RAM save/restore. >> + >> + The layout of the tables in guest memory defines an ABI. >> + The entries are laid out in memory as follows; >> + >> + Device Table Entry (DTE) layout: entry size = 16 bytes >> + >> + bits: | 63 ... 32 | 31 ... 6 | 5 | 4 ... 0 | >> + values: | DeviceID | Resv | V | Size | >> + >> + bits: | 63 ... 44 | 43 ... 0 | >> + values: | Resv | ITT_addr | > > While I appreciate this layout represents the absolute maximum an ITS > could implement, I'm a bit concerned about the amount of memory we may > end-up requiring here. All the ITSs implementations I know of seem to > get away with 8 bytes per entry. Maybe I'm just too worried. OK so I would propose a 16b DeviceId and 16b eventid bits: | 63 ... 48 | 47 ... 4 | 3 ... 0 | values: | DeviceID | ITT_addr | Size | I can use the size field as a validity indicator > > Also, please mention that ITT_addr is actually ITT_addr[51:8], as we're > guaranteed to have an ITT that is 256 byte aligned. sure > >> + >> + Collection Table Entry (CTE) layout: entry size = 8 bytes >> + >> + bits: | 63| 62 .. 52 | 51 ... 16 | 15 ... 0 | >> + values: | V | RES0 | RDBase | ICID | >> + >> + Interrupt Translation Table Entry (ITTE) layout: entry size = 16 bytes > > The actual name is Interrupt Translation Entry (ITE). I have a patch > renaming this all over the vgic-its.c file... ok > >> + >> + bits: | 63 ... 32 | 31 ... 17 | 16 | 15 ... 0 | >> + values: | DeviceID | RES0 | V | ICID | >> + >> + bits: | 63 ... 32 | 31 ... 0 | >> + values: | pINTID | EventID | > > Same concern here. 32bit DevID, EventID and INTID seem a bit over the > top. But maybe we shouldn't be concerned about memory... ;-) So I would suggest encoding 16b DeviceId 16b eventid 16b collection ID 16b pINTID bits: | 63 ... 48 | 47 ... 32 | 31 ... 15 | 15 ... 0 | values: | DeviceID | pINTID | EventId | ICID | a null pINTID would meen the ITE is invalid. Does that make sense or should I instead reduce the number of bits allocated to collections and keep the pINTID bit number larger? > >> + >> + LPI Pending Table layout: >> + >> + As specified in the ARM Generic Interrupt Controller Architecture >> + Specification GIC Architecture version 3.0 and version 4. The first >> + 1kB contains only zeros. >> > > You definitely want to relax this. An ITS implementation is allowed (and > actually encouraged) to maintain a coarse map in the first kB, and use > this to quickly scan the table, which would be very useful on restore. Maybe I miss something here. Currently I restore the ITEs before the pending tables. So considering all the ITEs I know which LPI are defined and which pending bits need to be restored. Why would I need to use a coarse map for? I understand the CPU cannot write the pending tables in our back, spec says behavior would be unpredictable, right? Thanks Eric > > Thanks, > > M. > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
