| Message ID | 1492164934-988-2-git-send-email-eric.auger@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 14 April 2017 at 11:15, Eric Auger <eric.auger@redhat.com> 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> > Acked-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
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? Is there any interaction between when you call KVM_DEV_ARM_ITS_RESTORE_TABLES and restore GITS_CTLR (which enables the ITS)? > 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) > + > + ITS Table ABI REV0: > + ------------------- > + > + Revision 0 of the ABI only supports physical LPIs. > + > + The device table and ITT are indexed by the deviceid and eventid, > + respectively. The collection table is not indexed by collectionid: > + CTE are written in the table in the order of collection creation. All > + entries are 8 bytes. > + > + Device Table Entry (DTE): > + > + bits: | 63| 62 ... 49 | 48 ... 5 | 4 ... 0 | > + values: | V | next | ITT_addr | Size | > + > + where; > + - V indicates whether the entry is valid. If not, other fields > + are not meaningful. > + - next: equals to 0 if this entry is the last one; otherwise it > + corresponds to the deviceid offset to the next DTE, capped by > + 2^14 -1. > + - ITT_addr matches bits [51:8] of the ITT address (256B aligned). I assume the B here is bytes. Where does this requirement come from? > + - Size specifies the supported number of bits for the deviceid, > + minus one deviceid or eventid? > + > + Collection Table Entry (CTE): > + > + bits: | 63| 62 .. 52 | 51 ... 16 | 15 ... 0 | > + values: | V | RES0 | RDBase | ICID | > + > + where: > + - V indicates whether the entry is valid. If not, other fields are > + not meaningful. > + - RES0: reserved field with Should-Be-Zero-or-Preserved behavior. > + - RDBase is the PE number (GICR_TYPER.Processor_Number semantic), > + - ICID is the collection ID > + > + Interrupt Translation Entry (ITE): > + > + bits: | 63 ... 48 | 47 ... 16 | 15 ... 0 | > + values: | next | pINTID | ICID | > + > + where: > + - next: equals to 0 if this entry is the last one; otherwise it corresponds > + to the eventid offset to the next ITE capped by 2^16 -1. > + - pINTID is the physical LPI ID; if zero, it means the entry is not valid > + and other fields are not meaningful. > + - ICID is the collection ID > + > -- > 2.5.5 > Besides the minor suggestions above: Reviewed-by: Christoffer Dall <cdall@linaro.org>
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. > >> 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. > > >> + >> + ITS Table ABI REV0: >> + ------------------- >> + >> + Revision 0 of the ABI only supports physical LPIs. >> + >> + The device table and ITT are indexed by the deviceid and eventid, >> + respectively. The collection table is not indexed by collectionid: >> + CTE are written in the table in the order of collection creation. All >> + entries are 8 bytes. >> + >> + Device Table Entry (DTE): >> + >> + bits: | 63| 62 ... 49 | 48 ... 5 | 4 ... 0 | >> + values: | V | next | ITT_addr | Size | >> + >> + where; >> + - V indicates whether the entry is valid. If not, other fields >> + are not meaningful. >> + - next: equals to 0 if this entry is the last one; otherwise it >> + corresponds to the deviceid offset to the next DTE, capped by >> + 2^14 -1. >> + - ITT_addr matches bits [51:8] of the ITT address (256B aligned). > > I assume the B here is bytes. Where does this requirement come from? Yes this is 256 byte aligned. I guess this is to save memory as the ITT is devised to encode a small range of eventids. > >> + - Size specifies the supported number of bits for the deviceid, >> + minus one > > deviceid or eventid? oups eventid thanks. > >> + >> + Collection Table Entry (CTE): >> + >> + bits: | 63| 62 .. 52 | 51 ... 16 | 15 ... 0 | >> + values: | V | RES0 | RDBase | ICID | >> + >> + where: >> + - V indicates whether the entry is valid. If not, other fields are >> + not meaningful. >> + - RES0: reserved field with Should-Be-Zero-or-Preserved behavior. >> + - RDBase is the PE number (GICR_TYPER.Processor_Number semantic), >> + - ICID is the collection ID >> + >> + Interrupt Translation Entry (ITE): >> + >> + bits: | 63 ... 48 | 47 ... 16 | 15 ... 0 | >> + values: | next | pINTID | ICID | >> + >> + where: >> + - next: equals to 0 if this entry is the last one; otherwise it corresponds >> + to the eventid offset to the next ITE capped by 2^16 -1. >> + - pINTID is the physical LPI ID; if zero, it means the entry is not valid >> + and other fields are not meaningful. >> + - ICID is the collection ID >> + >> -- >> 2.5.5 >> > > Besides the minor suggestions above: > > Reviewed-by: Christoffer Dall <cdall@linaro.org> Thanks Eric >
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
Hi Christoffer, On 27/04/2017 10:57, Christoffer Dall wrote: > 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? 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. KVM_DEV_ARM_ITS_RESTORE_TABLES restores all the ITS tables (device, collection, ITT) Hope it clarifies. Thanks Eric > >>> >>>> 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 > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On Thu, Apr 27, 2017 at 11:33:39AM +0200, Auger Eric wrote: > Hi Christoffer, > > On 27/04/2017 10:57, Christoffer Dall wrote: > > 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? > > 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? > > KVM_DEV_ARM_ITS_RESTORE_TABLES restores all the ITS tables (device, > collection, ITT) Why do you need this if you anyway need to restore the CTLR as the last thing? Just to make it absolutely clear when it happens, or is there something which has to happen between the CTLR and the RESTORE? Thanks, -Christoffer
Hi Christoffer, On 27/04/2017 13:02, Christoffer Dall wrote: > On Thu, Apr 27, 2017 at 11:33:39AM +0200, Auger Eric wrote: >> Hi Christoffer, >> >> On 27/04/2017 10:57, Christoffer Dall wrote: >>> 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? >> >> 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? > >> >> KVM_DEV_ARM_ITS_RESTORE_TABLES restores all the ITS tables (device, >> collection, ITT) > > Why do you need this if you anyway need to restore the CTLR as the last > thing? Just to make it absolutely clear when it happens, or is there > something which has to happen between the CTLR and the RESTORE? Not sure I get this one. by doing the lookup of Device table and ITT, KVM_DEV_ARM_ITS_RESTORE_TABLES enumerates all the LPI INTIDs that exist, re-allocate them Then when GITS_CTLR is set, you can use those INTIDs to parse the exact pending status in the pending tables. In case KVM_DEV_ARM_ITS_RESTORE_TABLES were not called you would be "blind" and you would need to parse the whole pending table. Thanks Eric > > Thanks, > -Christoffer >
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@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? > >> > >> 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. Thanks, -Christoffer
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@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? >>>> >>>> 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? Thanks Eric 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. > > Thanks, > -Christoffer >
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@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? >>>>> >>>>> 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. BTW, we should document the fact that vcpus must have been created before reloading the ITS (that's not completely obvious). Thanks, M.
On Thu, Apr 27, 2017 at 05:29:35PM +0200, 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@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? > >>>> > >>>> 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. Possibly, but I think it's good to write out the whole thing so we clearly understand the flow. That could better be achieved by correcting my proposed text above to say something like "Second, restore ALL redistributors to ensure the pending and configuration tables can be read." > > I understand the above description of the ordering comes in addition to > the existing text, right? Yes > in other words I keep the GITS_READR, > GITS_IIDR specific text as well as KVM_DEV_ARM_ITS_SAVE/RESTORE_TABLES > section. > Yes. But you don't need to do any reading of the pending table on any of the restore operations. Btw., I think it's GITS_CREADR, not GITS_READR. > > > > 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 Pretty much, yes. Thanks, -Christoffer
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@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? >>>>>> >>>>>> 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 Eric > > BTW, we should document the fact that vcpus must have been created > before reloading the ITS (that's not completely obvious). > > Thanks, > > M. >
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@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? > >>>>>> > >>>>>> 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
Hi Christoffer, On 27/04/2017 18:38, Christoffer Dall wrote: > On Thu, Apr 27, 2017 at 05:29:35PM +0200, 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@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? >>>>>> >>>>>> 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. > > Possibly, but I think it's good to write out the whole thing so we > clearly understand the flow. That could better be achieved by > correcting my proposed text above to say something like "Second, restore > ALL redistributors to ensure the pending and configuration tables can be > read." > >> >> I understand the above description of the ordering comes in addition to >> the existing text, right? > > Yes > >> in other words I keep the GITS_READR, >> GITS_IIDR specific text as well as KVM_DEV_ARM_ITS_SAVE/RESTORE_TABLES >> section. >> > > Yes. But you don't need to do any reading of the pending table on any > of the restore operations. well you told me to do it on vgic_add_lpi(). This is obviously called on ITS table restore. /me confused. Obviously this is implicit and should not be documented. Is that what you meant? btw this is not documented atm I think. > > Btw., I think it's GITS_CREADR, not GITS_READR. right. Thanks Eric > >>> >>> 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 > > Pretty much, yes. > > Thanks, > -Christoffer > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On Thu, Apr 27, 2017 at 07:27:22PM +0200, Auger Eric wrote: > Hi Christoffer, > > On 27/04/2017 18:38, Christoffer Dall wrote: > > On Thu, Apr 27, 2017 at 05:29:35PM +0200, 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@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? > >>>>>> > >>>>>> 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. > > > > Possibly, but I think it's good to write out the whole thing so we > > clearly understand the flow. That could better be achieved by > > correcting my proposed text above to say something like "Second, restore > > ALL redistributors to ensure the pending and configuration tables can be > > read." > > > >> > >> I understand the above description of the ordering comes in addition to > >> the existing text, right? > > > > Yes > > > >> in other words I keep the GITS_READR, > >> GITS_IIDR specific text as well as KVM_DEV_ARM_ITS_SAVE/RESTORE_TABLES > >> section. > >> > > > > Yes. But you don't need to do any reading of the pending table on any > > of the restore operations. > well you told me to do it on vgic_add_lpi(). This is obviously called on > ITS table restore. /me confused. Sorry, I meant you do not need to scan the entire table independently from restoring other state that requires building the data structures. > Obviously this is implicit and should > not be documented. Is that what you meant? btw this is not documented > atm I think. What I care about is that the ABI is clear and represents what the architecture does. So in terms of documentation in the ABI, we don't need to mention anything about when this is done, but we also do not need to specify any interaction between the pending tables and the ITS, beyond that the redestributors and memory must be restored before the ITS. Hope this clarifies. Thanks, -Christoffer
On 27/04/2017 19:54, Christoffer Dall wrote: > On Thu, Apr 27, 2017 at 07:27:22PM +0200, Auger Eric wrote: >> Hi Christoffer, >> >> On 27/04/2017 18:38, Christoffer Dall wrote: >>> On Thu, Apr 27, 2017 at 05:29:35PM +0200, 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@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? >>>>>>>> >>>>>>>> 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. >>> >>> Possibly, but I think it's good to write out the whole thing so we >>> clearly understand the flow. That could better be achieved by >>> correcting my proposed text above to say something like "Second, restore >>> ALL redistributors to ensure the pending and configuration tables can be >>> read." >>> >>>> >>>> I understand the above description of the ordering comes in addition to >>>> the existing text, right? >>> >>> Yes >>> >>>> in other words I keep the GITS_READR, >>>> GITS_IIDR specific text as well as KVM_DEV_ARM_ITS_SAVE/RESTORE_TABLES >>>> section. >>>> >>> >>> Yes. But you don't need to do any reading of the pending table on any >>> of the restore operations. >> well you told me to do it on vgic_add_lpi(). This is obviously called on >> ITS table restore. /me confused. > > Sorry, I meant you do not need to scan the entire table independently > from restoring other state that requires building the data structures. > >> Obviously this is implicit and should >> not be documented. Is that what you meant? btw this is not documented >> atm I think. > > What I care about is that the ABI is clear and represents what the > architecture does. So in terms of documentation in the ABI, we don't > need to mention anything about when this is done, but we also do not > need to specify any interaction between the pending tables and the ITS, > beyond that the redestributors and memory must be restored before the > ITS. > > Hope this clarifies. yes it does Thanks Eric > > Thanks, > -Christoffer >
Hi Christoffer, 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@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? >>>> >>>> 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. 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 Also if I understand it correctly this means the sync will be done on both add_lpi and GITS_CTLR setting What do you think? Thanks Eric > > Thanks, > -Christoffer > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On 04/05/17 08:00, Auger Eric wrote: > Hi Christoffer, > > 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@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? >>>>> >>>>> 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.
On Thu, May 04, 2017 at 09:00:29AM +0200, Auger Eric wrote: > Hi Christoffer, > > 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@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? > >>>> > >>>> 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 suppose they could be. I just really dislike introducing wording in the ABI which implies that state that belongs to the redistributor gets restored when you restore a certain ITS register; this gives the wrong impression of how the hardware works and it is going to bite us when we introduce LPI support without the ITS. Implementation wise, I don't care strongly if you wish to scan the table multiple times or just lookup the pending bit when you have to. I find it much cleaner to emulate what the hardware would be doing which is the ITS asking the redistributor for the pending state, and the redistributor is in charge of maintaining consistency between memory an cached state. I don't think it's weird to introduce a dependency between the redistributor and the ITS in the restore sequence, because the ITS is inherently built to be able to interface with the redistributor, and I don't see any way around it, given you need to know the address of the pending table anyway. > > 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. Yes. > So I > would rename vgic_v3_lpi_is_pending into something like > int vgic_v3_sync_lpi_pending_status(struct kvm *kvm, u32 intid) no objection > Since this covers a single LPI, the removes the byte access optimization > found in its_sync_lpi_pending_table sure > > Also if I understand it correctly this means the sync will be done on > both add_lpi and GITS_CTLR setting > No, it would only be done when needing the information for a specific LPI; there should be no real need to scan the entire table until we have support for LPIs without the ITS in which case the table must be scanned when restoring an enabled redistributor. Thanks, -Christoffer
Hi Marc, On 04/05/2017 09:40, Marc Zyngier wrote: > On 04/05/17 08:00, Auger Eric wrote: >> Hi Christoffer, >> >> 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@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? >>>>>> >>>>>> 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... Hum sorry I mixed up. the sync is currently done on GIC*R*_CTLR vgic_mmio_write_v3r_ctlr/vgic_enable_lpis/its_sync_lpi_pending_table As the redistributors are restored *before* the ITS this sync is void as no LPI exist at that time. That's why I did the sync (again) on ITS table restore. Sorry for the noise. OK let's go with the sync in vgic_add_lpi() ... Thanks Eric I believe that vgic_add_lpi() is the only point where > we should snapshot the pending state. > > Thanks, > > M. >
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. + 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 + + ITS Table ABI REV0: + ------------------- + + Revision 0 of the ABI only supports physical LPIs. + + The device table and ITT are indexed by the deviceid and eventid, + respectively. The collection table is not indexed by collectionid: + CTE are written in the table in the order of collection creation. All + entries are 8 bytes. + + Device Table Entry (DTE): + + bits: | 63| 62 ... 49 | 48 ... 5 | 4 ... 0 | + values: | V | next | ITT_addr | Size | + + where; + - V indicates whether the entry is valid. If not, other fields + are not meaningful. + - next: equals to 0 if this entry is the last one; otherwise it + corresponds to the deviceid offset to the next DTE, capped by + 2^14 -1. + - ITT_addr matches bits [51:8] of the ITT address (256B aligned). + - Size specifies the supported number of bits for the deviceid, + minus one + + Collection Table Entry (CTE): + + bits: | 63| 62 .. 52 | 51 ... 16 | 15 ... 0 | + values: | V | RES0 | RDBase | ICID | + + where: + - V indicates whether the entry is valid. If not, other fields are + not meaningful. + - RES0: reserved field with Should-Be-Zero-or-Preserved behavior. + - RDBase is the PE number (GICR_TYPER.Processor_Number semantic), + - ICID is the collection ID + + Interrupt Translation Entry (ITE): + + bits: | 63 ... 48 | 47 ... 16 | 15 ... 0 | + values: | next | pINTID | ICID | + + where: + - next: equals to 0 if this entry is the last one; otherwise it corresponds + to the eventid offset to the next ITE capped by 2^16 -1. + - pINTID is the physical LPI ID; if zero, it means the entry is not valid + and other fields are not meaningful. + - ICID is the collection ID +
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(+)