| Message ID | 1490607072-21610-9-git-send-email-eric.auger@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Mar 27 2017 at 10:30:58 AM, Eric Auger <eric.auger@redhat.com> wrote: > The GITS_IIDR revision field is used to encode the version of the > table layout (ABI). So we need to restore it to check the table > layout to be restored is compatible with the destination vITS. > > The user selected revision is stored in the user_revision field. > It will be compared against the REV num at table restoration time. Why isn't it sufficient to keep it GITS_IIDR RO and let userspace find out about the ABI revision that the kernel understands? Or are we planning on supporting multiple ABIs in the kernel? If so, do we have a deprecation policy/plan? I don't mind either way, but it would be good to document it... Maybe it is documented already and I missed it (which is perfectly possible!). Thanks, M.
Hi Marc, On 08/04/2017 12:42, Marc Zyngier wrote: > On Mon, Mar 27 2017 at 10:30:58 AM, Eric Auger <eric.auger@redhat.com> wrote: >> The GITS_IIDR revision field is used to encode the version of the >> table layout (ABI). So we need to restore it to check the table >> layout to be restored is compatible with the destination vITS. >> >> The user selected revision is stored in the user_revision field. >> It will be compared against the REV num at table restoration time. > > Why isn't it sufficient to keep it GITS_IIDR RO and let userspace find > out about the ABI revision that the kernel understands? > > Or are we planning on supporting multiple ABIs in the kernel? Yes as discussed with Peter, the plan is to allow several ABIs. So the userspace informs the destination about the ABI revision of the stored tables (contained by the GITS_IIDR). If the destination KVM does not support this ABI revision, table restore will fail. If so, do > we have a deprecation policy/plan? I don't mind either way, but it would > be good to document it... > > Maybe it is documented already and I missed it (which is perfectly > possible!). Well this is partly documented in Documentation/virtual/kvm/devices/arm-vgic-its.txt. No plan to deprecate. migration from KVM supporting v1 to KVM supporting V2 would be possible but not the contrary. Does it make sense? Thanks Eric > > Thanks, > > M. >
On 10/04/17 15:32, Auger Eric wrote: > Hi Marc, > > On 08/04/2017 12:42, Marc Zyngier wrote: >> On Mon, Mar 27 2017 at 10:30:58 AM, Eric Auger <eric.auger@redhat.com> wrote: >>> The GITS_IIDR revision field is used to encode the version of the >>> table layout (ABI). So we need to restore it to check the table >>> layout to be restored is compatible with the destination vITS. >>> >>> The user selected revision is stored in the user_revision field. >>> It will be compared against the REV num at table restoration time. >> >> Why isn't it sufficient to keep it GITS_IIDR RO and let userspace find >> out about the ABI revision that the kernel understands? >> >> Or are we planning on supporting multiple ABIs in the kernel? > Yes as discussed with Peter, the plan is to allow several ABIs. So the > userspace informs the destination about the ABI revision of the stored > tables (contained by the GITS_IIDR). If the destination KVM does not > support this ABI revision, table restore will fail. > If so, do >> we have a deprecation policy/plan? I don't mind either way, but it would >> be good to document it... >> >> Maybe it is documented already and I missed it (which is perfectly >> possible!). > Well this is partly documented in > Documentation/virtual/kvm/devices/arm-vgic-its.txt. No plan to > deprecate. migration from KVM supporting v1 to KVM supporting V2 would > be possible but not the contrary. > > Does it make sense? Sort of. Say you have three systems: A and C, which only supports v1; B, which supports v1 and v2. Let's say you migrate from A to B, and from B to C. Is B mandated to be able to export the tables as v1 and v2? Or can it restrict what it can export? Thanks, M.
On 10 April 2017 at 15:57, Marc Zyngier <marc.zyngier@arm.com> wrote: > On 10/04/17 15:32, Auger Eric wrote: >> Well this is partly documented in >> Documentation/virtual/kvm/devices/arm-vgic-its.txt. No plan to >> deprecate. migration from KVM supporting v1 to KVM supporting V2 would >> be possible but not the contrary. >> >> Does it make sense? > > Sort of. Say you have three systems: A and C, which only supports v1; B, > which supports v1 and v2. Let's say you migrate from A to B, and from B > to C. Is B mandated to be able to export the tables as v1 and v2? Or can > it restrict what it can export? Generally for QEMU we only require forwards-compatibility (so in this case old-kernel to new-kernel has to work but new-kernel to old-kernel need not). Export-as-old-format if the in-kernel data can be represented in the old format would be a nice to have property only (and depending what the unanticipated future need is, might not be possible at all). The idea here is not that we expect to need this, but to give ourselves an escape hatch for future use if we do find we need to add something which won't fit in the currently defined format. thanks -- PMM
Hi Marc, On 10/04/2017 16:57, Marc Zyngier wrote: > On 10/04/17 15:32, Auger Eric wrote: >> Hi Marc, >> >> On 08/04/2017 12:42, Marc Zyngier wrote: >>> On Mon, Mar 27 2017 at 10:30:58 AM, Eric Auger <eric.auger@redhat.com> wrote: >>>> The GITS_IIDR revision field is used to encode the version of the >>>> table layout (ABI). So we need to restore it to check the table >>>> layout to be restored is compatible with the destination vITS. >>>> >>>> The user selected revision is stored in the user_revision field. >>>> It will be compared against the REV num at table restoration time. >>> >>> Why isn't it sufficient to keep it GITS_IIDR RO and let userspace find >>> out about the ABI revision that the kernel understands? >>> >>> Or are we planning on supporting multiple ABIs in the kernel? >> Yes as discussed with Peter, the plan is to allow several ABIs. So the >> userspace informs the destination about the ABI revision of the stored >> tables (contained by the GITS_IIDR). If the destination KVM does not >> support this ABI revision, table restore will fail. >> If so, do >>> we have a deprecation policy/plan? I don't mind either way, but it would >>> be good to document it... >>> >>> Maybe it is documented already and I missed it (which is perfectly >>> possible!). >> Well this is partly documented in >> Documentation/virtual/kvm/devices/arm-vgic-its.txt. No plan to >> deprecate. migration from KVM supporting v1 to KVM supporting V2 would >> be possible but not the contrary. >> >> Does it make sense? > > Sort of. Say you have three systems: A and C, which only supports v1; B, > which supports v1 and v2. Let's say you migrate from A to B, and from B > to C. Is B mandated to be able to export the tables as v1 and v2? Or can > it restrict what it can export? At the moment migration from B to C will fail because source ABI rev = v2 > destination support ABI = v1. A (v1) -> B (v1 & v2): migration OK B (v1 & v2) -> C (v1): migration NOK What could be done if we want something clever is source KVM detects which ABI is sufficient and choose the lowest revision for the save. for instance if no vLPI choose rev1, otherwise rev2. rev2 typically is needed for vLPI support for nested as we need to save/restore vPE table and vLPIs in ITE (2x8 byte entries). The problem is that once you have migrated to B and you start playing with vLPIs and C do not know the table layout for vPE/vLPIs, you can't migrate anymore. Thanks Eric > > Thanks, > > M. >
On 10/04/17 16:17, Auger Eric wrote: > Hi Marc, > > On 10/04/2017 16:57, Marc Zyngier wrote: >> On 10/04/17 15:32, Auger Eric wrote: >>> Hi Marc, >>> >>> On 08/04/2017 12:42, Marc Zyngier wrote: >>>> On Mon, Mar 27 2017 at 10:30:58 AM, Eric Auger <eric.auger@redhat.com> wrote: >>>>> The GITS_IIDR revision field is used to encode the version of the >>>>> table layout (ABI). So we need to restore it to check the table >>>>> layout to be restored is compatible with the destination vITS. >>>>> >>>>> The user selected revision is stored in the user_revision field. >>>>> It will be compared against the REV num at table restoration time. >>>> >>>> Why isn't it sufficient to keep it GITS_IIDR RO and let userspace find >>>> out about the ABI revision that the kernel understands? >>>> >>>> Or are we planning on supporting multiple ABIs in the kernel? >>> Yes as discussed with Peter, the plan is to allow several ABIs. So the >>> userspace informs the destination about the ABI revision of the stored >>> tables (contained by the GITS_IIDR). If the destination KVM does not >>> support this ABI revision, table restore will fail. >>> If so, do >>>> we have a deprecation policy/plan? I don't mind either way, but it would >>>> be good to document it... >>>> >>>> Maybe it is documented already and I missed it (which is perfectly >>>> possible!). >>> Well this is partly documented in >>> Documentation/virtual/kvm/devices/arm-vgic-its.txt. No plan to >>> deprecate. migration from KVM supporting v1 to KVM supporting V2 would >>> be possible but not the contrary. >>> >>> Does it make sense? >> >> Sort of. Say you have three systems: A and C, which only supports v1; B, >> which supports v1 and v2. Let's say you migrate from A to B, and from B >> to C. Is B mandated to be able to export the tables as v1 and v2? Or can >> it restrict what it can export? > At the moment migration from B to C will fail because source ABI rev = > v2 > destination support ABI = v1. > > A (v1) -> B (v1 & v2): migration OK > B (v1 & v2) -> C (v1): migration NOK So what does IIDR report on B once the A->B migration has taken place? Does it report v2? Thanks, M.
Marc, On 11/04/2017 12:05, Marc Zyngier wrote: > On 10/04/17 16:17, Auger Eric wrote: >> Hi Marc, >> >> On 10/04/2017 16:57, Marc Zyngier wrote: >>> On 10/04/17 15:32, Auger Eric wrote: >>>> Hi Marc, >>>> >>>> On 08/04/2017 12:42, Marc Zyngier wrote: >>>>> On Mon, Mar 27 2017 at 10:30:58 AM, Eric Auger <eric.auger@redhat.com> wrote: >>>>>> The GITS_IIDR revision field is used to encode the version of the >>>>>> table layout (ABI). So we need to restore it to check the table >>>>>> layout to be restored is compatible with the destination vITS. >>>>>> >>>>>> The user selected revision is stored in the user_revision field. >>>>>> It will be compared against the REV num at table restoration time. >>>>> >>>>> Why isn't it sufficient to keep it GITS_IIDR RO and let userspace find >>>>> out about the ABI revision that the kernel understands? >>>>> >>>>> Or are we planning on supporting multiple ABIs in the kernel? >>>> Yes as discussed with Peter, the plan is to allow several ABIs. So the >>>> userspace informs the destination about the ABI revision of the stored >>>> tables (contained by the GITS_IIDR). If the destination KVM does not >>>> support this ABI revision, table restore will fail. >>>> If so, do >>>>> we have a deprecation policy/plan? I don't mind either way, but it would >>>>> be good to document it... >>>>> >>>>> Maybe it is documented already and I missed it (which is perfectly >>>>> possible!). >>>> Well this is partly documented in >>>> Documentation/virtual/kvm/devices/arm-vgic-its.txt. No plan to >>>> deprecate. migration from KVM supporting v1 to KVM supporting V2 would >>>> be possible but not the contrary. >>>> >>>> Does it make sense? >>> >>> Sort of. Say you have three systems: A and C, which only supports v1; B, >>> which supports v1 and v2. Let's say you migrate from A to B, and from B >>> to C. Is B mandated to be able to export the tables as v1 and v2? Or can >>> it restrict what it can export? >> At the moment migration from B to C will fail because source ABI rev = >> v2 > destination support ABI = v1. >> >> A (v1) -> B (v1 & v2): migration OK >> B (v1 & v2) -> C (v1): migration NOK > > So what does IIDR report on B once the A->B migration has taken place? > Does it report v2? yes that was the plan Thanks Eric > > Thanks, > > M. >
On 11 April 2017 at 11:08, Auger Eric <eric.auger@redhat.com> wrote: > On 11/04/2017 12:05, Marc Zyngier wrote: >> On 10/04/17 16:17, Auger Eric wrote: >>> A (v1) -> B (v1 & v2): migration OK >>> B (v1 & v2) -> C (v1): migration NOK >> >> So what does IIDR report on B once the A->B migration has taken place? >> Does it report v2? > > yes that was the plan Hmm, the IIDR value shouldn't change across a migration I think. It's guest visible so the guest should see it still the same value even after migration. thanks -- PMM
On 11/04/17 11:16, Peter Maydell wrote: > On 11 April 2017 at 11:08, Auger Eric <eric.auger@redhat.com> wrote: >> On 11/04/2017 12:05, Marc Zyngier wrote: >>> On 10/04/17 16:17, Auger Eric wrote: >>>> A (v1) -> B (v1 & v2): migration OK >>>> B (v1 & v2) -> C (v1): migration NOK >>> >>> So what does IIDR report on B once the A->B migration has taken place? >>> Does it report v2? >> >> yes that was the plan > > Hmm, the IIDR value shouldn't change across a migration I think. > It's guest visible so the guest should see it still the same > value even after migration. That's my worry. But we then have a problem when this VM migrates again. How does userspace find out which ABI to use then? Thanks, M.
On 11 April 2017 at 11:29, Marc Zyngier <marc.zyngier@arm.com> wrote: > On 11/04/17 11:16, Peter Maydell wrote: >> On 11 April 2017 at 11:08, Auger Eric <eric.auger@redhat.com> wrote: >>> On 11/04/2017 12:05, Marc Zyngier wrote: >>>> On 10/04/17 16:17, Auger Eric wrote: >>>>> A (v1) -> B (v1 & v2): migration OK >>>>> B (v1 & v2) -> C (v1): migration NOK >>>> >>>> So what does IIDR report on B once the A->B migration has taken place? >>>> Does it report v2? >>> >>> yes that was the plan >> >> Hmm, the IIDR value shouldn't change across a migration I think. >> It's guest visible so the guest should see it still the same >> value even after migration. > > That's my worry. But we then have a problem when this VM migrates again. > How does userspace find out which ABI to use then? Userspace shouldn't need to care; the idea of this scheme is that if the kernel has to change its ABI then the kernel is what has to deal with the change. I think that means that if you can read the v1 format on input you have to output it too, though. thanks -- PMM
Hi, On 11/04/2017 12:29, Marc Zyngier wrote: > On 11/04/17 11:16, Peter Maydell wrote: >> On 11 April 2017 at 11:08, Auger Eric <eric.auger@redhat.com> wrote: >>> On 11/04/2017 12:05, Marc Zyngier wrote: >>>> On 10/04/17 16:17, Auger Eric wrote: >>>>> A (v1) -> B (v1 & v2): migration OK >>>>> B (v1 & v2) -> C (v1): migration NOK >>>> >>>> So what does IIDR report on B once the A->B migration has taken place? >>>> Does it report v2? >>> >>> yes that was the plan >> >> Hmm, the IIDR value shouldn't change across a migration I think. >> It's guest visible so the guest should see it still the same >> value even after migration. > > That's my worry. But we then have a problem when this VM migrates again. > How does userspace find out which ABI to use then? Today the userspace just conveys the information from source kernel to destination kernel through the IIDR value. This allows the destination kernel to know what was the table layout used during the migration. If we report the source ABI revision value on the destination, what is the strategy we should adopt: - limit the features to those that will be migratable through this ABI - allow the functionalities (GICv4 for instance) but reject save? Thanks Eric > > Thanks, > > M. >
diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h index b72dd2a..41b71ca 100644 --- a/include/kvm/arm_vgic.h +++ b/include/kvm/arm_vgic.h @@ -162,6 +162,8 @@ struct vgic_its { u32 creadr; u32 cwriter; + u32 user_revision; + /* Protects the device and collection lists */ struct mutex its_lock; struct list_head device_list; diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c index a5f3abe..169b486 100644 --- a/virt/kvm/arm/vgic/vgic-its.c +++ b/virt/kvm/arm/vgic/vgic-its.c @@ -33,6 +33,9 @@ #include "vgic.h" #include "vgic-mmio.h" +/* ITS Table Layout ABI Revision */ +#define REV 0x1 + /* * Creates a new (reference to a) struct vgic_irq for a given LPI. * If this LPI is already mapped on another ITS, we increase its refcount @@ -384,7 +387,19 @@ static unsigned long vgic_mmio_read_its_iidr(struct kvm *kvm, struct vgic_its *its, gpa_t addr, unsigned int len) { - return (PRODUCT_ID_KVM << 24) | (IMPLEMENTER_ARM << 0); + return (PRODUCT_ID_KVM << 24) | (REV << 12) | IMPLEMENTER_ARM; +} + +static void vgic_mmio_uaccess_write_its_iidr(struct kvm *kvm, + struct vgic_its *its, + gpa_t addr, unsigned int len, + unsigned long val) +{ + u64 tmp = 0; + + tmp = update_64bit_reg(tmp, addr & 3, len, val); + tmp = (tmp & GENMASK(15, 12)) >> 12; + its->user_revision = tmp; } static unsigned long vgic_mmio_read_its_idregs(struct kvm *kvm, @@ -1359,8 +1374,9 @@ static struct vgic_register_region its_registers[] = { REGISTER_ITS_DESC(GITS_CTLR, vgic_mmio_read_its_ctlr, vgic_mmio_write_its_ctlr, 4, VGIC_ACCESS_32bit), - REGISTER_ITS_DESC(GITS_IIDR, - vgic_mmio_read_its_iidr, its_mmio_write_wi, 4, + REGISTER_ITS_DESC_UACCESS(GITS_IIDR, + vgic_mmio_read_its_iidr, its_mmio_write_wi, + vgic_mmio_uaccess_write_its_iidr, 4, VGIC_ACCESS_32bit), REGISTER_ITS_DESC(GITS_TYPER, vgic_mmio_read_its_typer, its_mmio_write_wi, 8, @@ -1458,6 +1474,8 @@ static int vgic_its_create(struct kvm_device *dev, u32 type) ((u64)GITS_BASER_TYPE_COLLECTION << GITS_BASER_TYPE_SHIFT); dev->kvm->arch.vgic.propbaser = INITIAL_PROPBASER_VALUE; + its->user_revision = 0; + dev->private = its; return 0;
The GITS_IIDR revision field is used to encode the version of the table layout (ABI). So we need to restore it to check the table layout to be restored is compatible with the destination vITS. The user selected revision is stored in the user_revision field. It will be compared against the REV num at table restoration time. Signed-off-by: Eric Auger <eric.auger@redhat.com> --- v4: creation --- include/kvm/arm_vgic.h | 2 ++ virt/kvm/arm/vgic/vgic-its.c | 24 +++++++++++++++++++++--- 2 files changed, 23 insertions(+), 3 deletions(-)