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

[Peter Maydell]

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.

-- PMM