* [PATCH] doc/arm: take care restore order of GICR_* in ITS restore @ 2021-07-21 9:20 Jianyong Wu 2021-07-21 9:53 ` Marc Zyngier 0 siblings, 1 reply; 5+ messages in thread From: Jianyong Wu @ 2021-07-21 9:20 UTC (permalink / raw) To: maz, james.morse, andre.przywara, lushenming Cc: kvm, kvmarm, linux-doc, linux-kernel, justin.he, jianyong.wu When restore GIC/ITS, GICR_CTLR must be restored after GICR_PROPBASER and GICR_PENDBASER. That is important, as both of GICR_PROPBASER and GICR_PENDBASER will fail to be loaded when lpi has enabled yet in GICR_CTLR. Keep the restore order above will avoid that issue. Shout it out at the doc is very helpful that may avoid lots of debug work. Signed-off-by: Jianyong Wu <jianyong.wu@arm.com> --- Documentation/virt/kvm/devices/arm-vgic-its.rst | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Documentation/virt/kvm/devices/arm-vgic-its.rst b/Documentation/virt/kvm/devices/arm-vgic-its.rst index d257eddbae29..6b36de6937f8 100644 --- a/Documentation/virt/kvm/devices/arm-vgic-its.rst +++ b/Documentation/virt/kvm/devices/arm-vgic-its.rst @@ -126,7 +126,8 @@ ITS Restore Sequence: The following ordering must be followed when restoring the GIC and the ITS: a) restore all guest memory and create vcpus -b) restore all redistributors +b) restore all redistributors: + make sure restore GICR_CTLR after GICR_PROPBASER and GICR_PENDBASER c) provide the ITS base address (KVM_DEV_ARM_VGIC_GRP_ADDR) d) restore the ITS in the following order: -- 2.17.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] doc/arm: take care restore order of GICR_* in ITS restore 2021-07-21 9:20 [PATCH] doc/arm: take care restore order of GICR_* in ITS restore Jianyong Wu @ 2021-07-21 9:53 ` Marc Zyngier 2021-07-22 2:49 ` Jianyong Wu 0 siblings, 1 reply; 5+ messages in thread From: Marc Zyngier @ 2021-07-21 9:53 UTC (permalink / raw) To: Jianyong Wu Cc: james.morse, andre.przywara, lushenming, kvm, kvmarm, linux-doc, linux-kernel, justin.he On Wed, 21 Jul 2021 10:20:19 +0100, Jianyong Wu <jianyong.wu@arm.com> wrote: > > When restore GIC/ITS, GICR_CTLR must be restored after GICR_PROPBASER > and GICR_PENDBASER. That is important, as both of GICR_PROPBASER and > GICR_PENDBASER will fail to be loaded when lpi has enabled yet in > GICR_CTLR. Keep the restore order above will avoid that issue. > Shout it out at the doc is very helpful that may avoid lots of debug work. But that's something that is already mandated by the architecture, isn't it? See "5.1 LPIs" in the architecture spec: <quote> If GICR_PROPBASER is updated when GICR_CTLR.EnableLPIs == 1, the effects are UNPREDICTABLE. [...] If GICR_PENDBASER is updated when GICR_CTLR.EnableLPIs == 1, the effects are UNPREDICTABLE. </quote> The point of this documentation is to make it explicit what is *not* covered by the architecture. Anything that is in the architecture still applies, and shouldn't be overlooked. Thanks, M. -- Without deviation from the norm, progress is not possible. ^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH] doc/arm: take care restore order of GICR_* in ITS restore 2021-07-21 9:53 ` Marc Zyngier @ 2021-07-22 2:49 ` Jianyong Wu 2021-07-22 8:11 ` Marc Zyngier 0 siblings, 1 reply; 5+ messages in thread From: Jianyong Wu @ 2021-07-22 2:49 UTC (permalink / raw) To: Marc Zyngier Cc: James Morse, Andre Przywara, lushenming, kvm, kvmarm, linux-doc, linux-kernel, Justin He Hello Marc, > -----Original Message----- > From: Marc Zyngier <maz@kernel.org> > Sent: Wednesday, July 21, 2021 5:54 PM > To: Jianyong Wu <Jianyong.Wu@arm.com> > Cc: James Morse <James.Morse@arm.com>; Andre Przywara > <Andre.Przywara@arm.com>; lushenming@huawei.com; > kvm@vger.kernel.org; kvmarm@lists.cs.columbia.edu; linux- > doc@vger.kernel.org; linux-kernel@vger.kernel.org; Justin He > <Justin.He@arm.com> > Subject: Re: [PATCH] doc/arm: take care restore order of GICR_* in ITS > restore > > On Wed, 21 Jul 2021 10:20:19 +0100, > Jianyong Wu <jianyong.wu@arm.com> wrote: > > > > When restore GIC/ITS, GICR_CTLR must be restored after GICR_PROPBASER > > and GICR_PENDBASER. That is important, as both of GICR_PROPBASER and > > GICR_PENDBASER will fail to be loaded when lpi has enabled yet in > > GICR_CTLR. Keep the restore order above will avoid that issue. > > Shout it out at the doc is very helpful that may avoid lots of debug work. > > But that's something that is already mandated by the architecture, isn't it? > See "5.1 LPIs" in the architecture spec: > > <quote> > > If GICR_PROPBASER is updated when GICR_CTLR.EnableLPIs == 1, the effects > are UNPREDICTABLE. > > [...] > > If GICR_PENDBASER is updated when GICR_CTLR.EnableLPIs == 1, the effects > are UNPREDICTABLE. > I think this "UNPREDICTABLE" related with the "physical machine". Am I right? In virtualization environment, kernel gives the definite answer that we should not enable GICR_CTLR.EnableLPIs before restoring GICR_PROPBASER(GICR_PENDBASER either) when restore GIC ITS in VMM, see [1]. Thus, should we consider the virtualization environment as a special case? [1] linux/arch/arm64/kvm/vgic/vgic-mmio-v3.c static void vgic_mmio_write_propbase(struct kvm_vcpu *vcpu, gpa_t addr, unsigned int len, unsigned long val) { struct vgic_dist *dist = &vcpu->kvm->arch.vgic; struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu; u64 old_propbaser, propbaser; /* Storing a value with LPIs already enabled is undefined */ if (vgic_cpu->lpis_enabled) return; ... } Thanks Jianyong > </quote> > > The point of this documentation is to make it explicit what is *not* covered > by the architecture. Anything that is in the architecture still applies, and > shouldn't be overlooked. > > Thanks, > > M. > > -- > Without deviation from the norm, progress is not possible. IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] doc/arm: take care restore order of GICR_* in ITS restore 2021-07-22 2:49 ` Jianyong Wu @ 2021-07-22 8:11 ` Marc Zyngier 2021-07-23 5:27 ` Jianyong Wu 0 siblings, 1 reply; 5+ messages in thread From: Marc Zyngier @ 2021-07-22 8:11 UTC (permalink / raw) To: Jianyong Wu Cc: James Morse, Andre Przywara, lushenming, kvm, kvmarm, linux-doc, linux-kernel, Justin He On Thu, 22 Jul 2021 03:49:52 +0100, Jianyong Wu <Jianyong.Wu@arm.com> wrote: > > Hello Marc, > > > -----Original Message----- > > From: Marc Zyngier <maz@kernel.org> > > Sent: Wednesday, July 21, 2021 5:54 PM > > To: Jianyong Wu <Jianyong.Wu@arm.com> > > Cc: James Morse <James.Morse@arm.com>; Andre Przywara > > <Andre.Przywara@arm.com>; lushenming@huawei.com; > > kvm@vger.kernel.org; kvmarm@lists.cs.columbia.edu; linux- > > doc@vger.kernel.org; linux-kernel@vger.kernel.org; Justin He > > <Justin.He@arm.com> > > Subject: Re: [PATCH] doc/arm: take care restore order of GICR_* in ITS > > restore > > > > On Wed, 21 Jul 2021 10:20:19 +0100, > > Jianyong Wu <jianyong.wu@arm.com> wrote: > > > > > > When restore GIC/ITS, GICR_CTLR must be restored after GICR_PROPBASER > > > and GICR_PENDBASER. That is important, as both of GICR_PROPBASER and > > > GICR_PENDBASER will fail to be loaded when lpi has enabled yet in > > > GICR_CTLR. Keep the restore order above will avoid that issue. > > > Shout it out at the doc is very helpful that may avoid lots of debug work. > > > > But that's something that is already mandated by the architecture, isn't it? > > See "5.1 LPIs" in the architecture spec: > > > > <quote> > > > > If GICR_PROPBASER is updated when GICR_CTLR.EnableLPIs == 1, the effects > > are UNPREDICTABLE. > > > > [...] > > > > If GICR_PENDBASER is updated when GICR_CTLR.EnableLPIs == 1, the effects > > are UNPREDICTABLE. > > > > I think this "UNPREDICTABLE" related with the "physical machine". Am > I right? No, you are unfortunately wrong. The architecture applies to *any* implementation, and makes no distinction between a HW implementation of a SW version. This is why we call it an architecture, and not an implementation. > In virtualization environment, kernel gives the definite answer that > we should not enable GICR_CTLR.EnableLPIs before restoring > GICR_PROPBASER(GICR_PENDBASER either) when restore GIC ITS in VMM, > see [1]. Thus, should we consider the virtualization environment as > a special case? Absolutely not. If you start special casing things, then we end-up having stupidly designed SW that tries to do stupid things based on the supposed properties of an implementation. We follow the architecture, all the architecture, nothing but the architecture. The architecture is the only barrier between insanity and pure madness! ;-) > > [1] linux/arch/arm64/kvm/vgic/vgic-mmio-v3.c > static void vgic_mmio_write_propbase(struct kvm_vcpu *vcpu, > gpa_t addr, unsigned int len, > unsigned long val) > { > struct vgic_dist *dist = &vcpu->kvm->arch.vgic; > struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu; > u64 old_propbaser, propbaser; > > /* Storing a value with LPIs already enabled is undefined */ > if (vgic_cpu->lpis_enabled) > return; > ... > } Do you see how the kernel does exactly what the architecture says we can do? Ignoring the write is a perfectly valid implementation of UNPREDICTABLE. So what we do is completely in line with the architecture. As such, no need to document it any further, everything is already where it should be. If someone tries to write code dealing with the GIC without understanding the architecture, no amount of additional documentation will help. Thanks, M. -- Without deviation from the norm, progress is not possible. ^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH] doc/arm: take care restore order of GICR_* in ITS restore 2021-07-22 8:11 ` Marc Zyngier @ 2021-07-23 5:27 ` Jianyong Wu 0 siblings, 0 replies; 5+ messages in thread From: Jianyong Wu @ 2021-07-23 5:27 UTC (permalink / raw) To: Marc Zyngier Cc: James Morse, Andre Przywara, lushenming, kvm, kvmarm, linux-doc, linux-kernel, Justin He Hi Marc, Get it! Thanks for your explanation. Thanks Jianyong > -----Original Message----- > From: Marc Zyngier <maz@kernel.org> > Sent: Thursday, July 22, 2021 4:11 PM > To: Jianyong Wu <Jianyong.Wu@arm.com> > Cc: James Morse <James.Morse@arm.com>; Andre Przywara > <Andre.Przywara@arm.com>; lushenming@huawei.com; > kvm@vger.kernel.org; kvmarm@lists.cs.columbia.edu; linux- > doc@vger.kernel.org; linux-kernel@vger.kernel.org; Justin He > <Justin.He@arm.com> > Subject: Re: [PATCH] doc/arm: take care restore order of GICR_* in ITS > restore > > On Thu, 22 Jul 2021 03:49:52 +0100, > Jianyong Wu <Jianyong.Wu@arm.com> wrote: > > > > Hello Marc, > > > > > -----Original Message----- > > > From: Marc Zyngier <maz@kernel.org> > > > Sent: Wednesday, July 21, 2021 5:54 PM > > > To: Jianyong Wu <Jianyong.Wu@arm.com> > > > Cc: James Morse <James.Morse@arm.com>; Andre Przywara > > > <Andre.Przywara@arm.com>; lushenming@huawei.com; > > > kvm@vger.kernel.org; kvmarm@lists.cs.columbia.edu; linux- > > > doc@vger.kernel.org; linux-kernel@vger.kernel.org; Justin He > > > <Justin.He@arm.com> > > > Subject: Re: [PATCH] doc/arm: take care restore order of GICR_* in > > > ITS restore > > > > > > On Wed, 21 Jul 2021 10:20:19 +0100, > > > Jianyong Wu <jianyong.wu@arm.com> wrote: > > > > > > > > When restore GIC/ITS, GICR_CTLR must be restored after > > > > GICR_PROPBASER and GICR_PENDBASER. That is important, as both of > > > > GICR_PROPBASER and GICR_PENDBASER will fail to be loaded when lpi > > > > has enabled yet in GICR_CTLR. Keep the restore order above will avoid > that issue. > > > > Shout it out at the doc is very helpful that may avoid lots of debug work. > > > > > > But that's something that is already mandated by the architecture, isn't it? > > > See "5.1 LPIs" in the architecture spec: > > > > > > <quote> > > > > > > If GICR_PROPBASER is updated when GICR_CTLR.EnableLPIs == 1, the > > > effects are UNPREDICTABLE. > > > > > > [...] > > > > > > If GICR_PENDBASER is updated when GICR_CTLR.EnableLPIs == 1, the > > > effects are UNPREDICTABLE. > > > > > > > I think this "UNPREDICTABLE" related with the "physical machine". Am I > > right? > > No, you are unfortunately wrong. The architecture applies to *any* > implementation, and makes no distinction between a HW implementation of > a SW version. This is why we call it an architecture, and not an > implementation. > > > In virtualization environment, kernel gives the definite answer that > > we should not enable GICR_CTLR.EnableLPIs before restoring > > GICR_PROPBASER(GICR_PENDBASER either) when restore GIC ITS in VMM, > see > > [1]. Thus, should we consider the virtualization environment as a > > special case? > > Absolutely not. If you start special casing things, then we end-up having > stupidly designed SW that tries to do stupid things based on the supposed > properties of an implementation. > > We follow the architecture, all the architecture, nothing but the architecture. > The architecture is the only barrier between insanity and pure madness! ;-) > > > > > [1] linux/arch/arm64/kvm/vgic/vgic-mmio-v3.c > > static void vgic_mmio_write_propbase(struct kvm_vcpu *vcpu, > > gpa_t addr, unsigned int len, > > unsigned long val) { > > struct vgic_dist *dist = &vcpu->kvm->arch.vgic; > > struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu; > > u64 old_propbaser, propbaser; > > > > /* Storing a value with LPIs already enabled is undefined */ > > if (vgic_cpu->lpis_enabled) > > return; > > ... > > } > > Do you see how the kernel does exactly what the architecture says we can do? > Ignoring the write is a perfectly valid implementation of UNPREDICTABLE. > > So what we do is completely in line with the architecture. As such, no need to > document it any further, everything is already where it should be. If > someone tries to write code dealing with the GIC without understanding the > architecture, no amount of additional documentation will help. > > Thanks, > > M. > > -- > Without deviation from the norm, progress is not possible. IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-07-23 5:27 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-21 9:20 [PATCH] doc/arm: take care restore order of GICR_* in ITS restore Jianyong Wu 2021-07-21 9:53 ` Marc Zyngier 2021-07-22 2:49 ` Jianyong Wu 2021-07-22 8:11 ` Marc Zyngier 2021-07-23 5:27 ` Jianyong Wu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).