linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Marc Zyngier <maz@kernel.org>
To: Jianyong Wu <Jianyong.Wu@arm.com>
Cc: James Morse <James.Morse@arm.com>,
	Andre Przywara <Andre.Przywara@arm.com>,
	"lushenming@huawei.com" <lushenming@huawei.com>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"kvmarm@lists.cs.columbia.edu" <kvmarm@lists.cs.columbia.edu>,
	"linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
	"linux-kernel@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
Date: Thu, 22 Jul 2021 09:11:02 +0100	[thread overview]
Message-ID: <875yx23h0p.wl-maz@kernel.org> (raw)
In-Reply-To: <AM9PR08MB727690189F03ED71450B8EB3F4E49@AM9PR08MB7276.eurprd08.prod.outlook.com>

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.

  reply	other threads:[~2021-07-22  8:11 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-21  9:20 Jianyong Wu
2021-07-21  9:53 ` Marc Zyngier
2021-07-22  2:49   ` Jianyong Wu
2021-07-22  8:11     ` Marc Zyngier [this message]
2021-07-23  5:27       ` Jianyong Wu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=875yx23h0p.wl-maz@kernel.org \
    --to=maz@kernel.org \
    --cc=Andre.Przywara@arm.com \
    --cc=James.Morse@arm.com \
    --cc=Jianyong.Wu@arm.com \
    --cc=Justin.He@arm.com \
    --cc=kvm@vger.kernel.org \
    --cc=kvmarm@lists.cs.columbia.edu \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=lushenming@huawei.com \
    --subject='Re: [PATCH] doc/arm: take care restore order of GICR_* in ITS restore' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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