Re: [PATCH v11 06/10] hvf: arm: Implement -cpu host

From: Ard Biesheuvel <ardb@kernel.org>
To: Peter Maydell <peter.maydell@linaro.org>
Cc: "Eduardo Habkost" <ehabkost@redhat.com>,
	"Sergio Lopez" <slp@redhat.com>,
	"Philippe Mathieu-Daudé" <philmd@redhat.com>,
	"Richard Henderson" <richard.henderson@linaro.org>,
	"QEMU Developers" <qemu-devel@nongnu.org>,
	"Cameron Esfahani" <dirty@apple.com>,
	"Roman Bolshakov" <r.bolshakov@yadro.com>,
	"Alexander Graf" <agraf@csgraf.de>,
	qemu-arm <qemu-arm@nongnu.org>, "Frank Yang" <lfy@google.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Peter Collingbourne" <pcc@google.com>
Subject: Re: [PATCH v11 06/10] hvf: arm: Implement -cpu host
Date: Wed, 22 Sep 2021 18:10:38 +0200	[thread overview]
Message-ID: <CAMj1kXG6VDp_rSU+S_TTPdK8rUt2zF=V1E+brGfzsTUa7oF+yw@mail.gmail.com> (raw)
In-Reply-To: <CAFEAcA_fxW0yKwxqBuDi8+ux-jtVKeJxNhDV1=ROb+VamLiZRQ@mail.gmail.com>

On Wed, 22 Sept 2021 at 14:45, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Wed, 22 Sept 2021 at 12:41, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Thu, 16 Sept 2021 at 18:17, Peter Maydell <peter.maydell@linaro.org> wrote:
> > >
> > > On Thu, 16 Sept 2021 at 17:05, Ard Biesheuvel <ardb@kernel.org> wrote:
> > > > I'd argue that compliance with the architecture means that the
> > > > software should not clear RES1 bits
> > >
> > > Architecturally, RES1 means that "software
> > >  * Must not rely on the bit reading as 1.
> > >  * Must use an SBOP policy to write to the bit."
> > > (SBOP=="should be 1 or preserved", ie you can preserve the existing value,
> > > as in "read register, change some bits, write back", or you can write a 1.)
> > >
> >
> > OVMF preserves the bit, and does not reason or care about its value.
> > So in this sense, it is compliant.
>
> Hmm. Alex, can you give more details about what fails here ?
>

It seems that EDK2 ends up setting EL0 r/o or r/w permissions on some
mappings, even though it never runs anything at EL0. So any execution
that gets initiated via the timer interrupt causing a EL1->EL1 IRQ
exception will run with PAN enabled and lose access to those mappings.

So it seems like a definite bug that is unrelated to reset state of
the registers and assumptions related to that.

> > > > but I don't think we can blame it
> > > > for not touching bits that were in in invalid state upon entry.
> > >
> > > SCTLR_EL1.SPAN == 0 is perfectly valid for a CPU that supports the
> > > PAN feature. It's just not the value OVMF wants, so OVMF should
> > > be setting it to what it does want. Also, as the first thing to
> > > run after reset (ie firmware) OVMF absolutely is responsible for
> > > dealing with system registers which have UNKNOWN values out of
> > > reset.
> > >
> >
> > Fair enough. But I'd still suggest fixing this at both ends.
>
> Yes, the version of this code that we committed sets SPAN to 1.
> (This argument is mostly about what the comment justifying that
> value should say :-))
>

OK, that makes sense. But I'd like to get EDK2 fixed as well, obviously.