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

[Ard Biesheuvel]

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.