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: Thu, 16 Sep 2021 18:05:16 +0200	[thread overview]
Message-ID: <CAMj1kXEEN+J4k_Kib8gRHcy8v1vVRwk7c847yT_Kuv+jnLf9ww@mail.gmail.com> (raw)
In-Reply-To: <CAFEAcA8xs3x5WCqT+FnPYip=nbQx6=UEH90Dkjq48YLboV9qAw@mail.gmail.com>

On Thu, 16 Sept 2021 at 17:56, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 16 Sept 2021 at 16:30, Alexander Graf <agraf@csgraf.de> wrote:
> >
> >
> > On 16.09.21 14:24, Peter Maydell wrote:
> > > On Wed, 15 Sept 2021 at 19:10, Alexander Graf <agraf@csgraf.de> wrote:
> > >> Now that we have working system register sync, we push more target CPU
> > >> properties into the virtual machine. That might be useful in some
> > >> situations, but is not the typical case that users want.
> > >>
> > >> So let's add a -cpu host option that allows them to explicitly pass all
> > >> CPU capabilities of their host CPU into the guest.
> > >>
> > >> Signed-off-by: Alexander Graf <agraf@csgraf.de>
> > >> Acked-by: Roman Bolshakov <r.bolshakov@yadro.com>
> > >> Reviewed-by: Sergio Lopez <slp@redhat.com>
> > >>
> > >> +    /*
> > >> +     * A scratch vCPU returns SCTLR 0, so let's fill our default with the M1
> > >> +     * boot SCTLR from https://github.com/AsahiLinux/m1n1/issues/97
>
> Side note: SCTLR_EL1 is a 64-bit register, do you have anything that
> prints the full 64-bits to confirm that [63:32] are indeed all 0?
>
> > >> +     */
> > >> +    ahcf->reset_sctlr = 0x30100180;
> > >> +    /* OVMF chokes on boot if SPAN is not set, so default it to on */
> > >> +    ahcf->reset_sctlr |= 0x00800000;
> > > Isn't that just an OVMF bug ? If you want this then you need to
> > > convince me why this isn't just a workaround for a buggy guest.
> >
> >
> > I couldn't find anything in the ARMv8 spec that explicitly says "If you
> > support PAN, SCTLR.SPAN should be 1 by default". It is RES1 for CPUs
> > that do not implement PAN. Beware that for SPAN, "1" means disabled and
> > "0" means enabled.
>
> It's UNKNOWN on reset. So unless OVMF is relying on whatever
> is launching it to set SCTLR correctly (ie there is some part of
> the "firmware-to-OVMF" contract it is relying on) then it seems to
> me that it's OVMF's job to initialize it to what it needs. (Lots of
> SCTLR is like that.)
>
> Linux does this here:
>  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/kernel/head.S?h=v5.15-rc1#n485
>  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/include/asm/sysreg.h?h=v5.15-rc1#n695
> because the INIT_SCTLR_EL1_MMU_OFF constant includes forcing
> all "this kernel expects these to be RES0/RES1 because that's all
> the architectural features we know about at this time" bits to
> their RESn values.
>
> But we can probably construct an argument for why having it set
> makes sense, yes.
>

I'd argue that compliance with the architecture means that the
software should not clear RES1 bits, but I don't think we can blame it
for not touching bits that were in in invalid state upon entry.