| Message ID | 20210915181049.27597-7-agraf@csgraf.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hvf: Implement Apple Silicon Support | expand |
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 > + */ > + 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. Otherwise Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
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 >> + */ >> + 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. So by making it 0 we are effectively enabling PAN support on behalf of the firmware, which sort-of goes against the whole idea of being backwards compatible, no? I'd say that OVMF behaves just fine by assuming PAN support is not enabled unless it does it explicitly itself. To make sure we get full agreement, I've added Ard to CC as well though :) Alex
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
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.
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.) > 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. Put another way, if you want to argue that this is an "invalid state" you need to point to the specification that defines the valid state that OVMF should see when it is handed control. -- PMM
On 16.09.21 17:55, Peter Maydell 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? Yes, m1n1 prints the full 64bit value: https://github.com/AsahiLinux/m1n1/blob/main/src/memory.c#L459 That said, I'm not sure we really have to model the guest's reset SCTLR in EL1 to be identical to the host's reset SCTLR in EL2. I think it's a great start, but as long as there is no spec that indicates what SCTLR should be in EL1, we can make our own rules IMHO. Alex
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. > > 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. > Put another way, if you want to argue that this is an "invalid > state" you need to point to the specification that defines > the valid state that OVMF should see when it is handed control. > > -- PMM
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 ? > > > 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 :-)) -- PMM
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.
diff --git a/target/arm/cpu.c b/target/arm/cpu.c index ba0741b20e..016df2e1e7 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -39,6 +39,7 @@ #include "sysemu/tcg.h" #include "sysemu/hw_accel.h" #include "kvm_arm.h" +#include "hvf_arm.h" #include "disas/capstone.h" #include "fpu/softfloat.h" @@ -1400,8 +1401,8 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) * this is the first point where we can report it. */ if (cpu->host_cpu_probe_failed) { - if (!kvm_enabled()) { - error_setg(errp, "The 'host' CPU type can only be used with KVM"); + if (!kvm_enabled() && !hvf_enabled()) { + error_setg(errp, "The 'host' CPU type can only be used with KVM or HVF"); } else { error_setg(errp, "Failed to retrieve host CPU features"); } @@ -2061,15 +2062,19 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data) #endif /* CONFIG_TCG */ } -#ifdef CONFIG_KVM +#if defined(CONFIG_KVM) || defined(CONFIG_HVF) static void arm_host_initfn(Object *obj) { ARMCPU *cpu = ARM_CPU(obj); +#ifdef CONFIG_KVM kvm_arm_set_cpu_features_from_host(cpu); if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) { aarch64_add_sve_properties(obj); } +#else + hvf_arm_set_cpu_features_from_host(cpu); +#endif arm_cpu_post_init(obj); } @@ -2129,7 +2134,7 @@ static void arm_cpu_register_types(void) { type_register_static(&arm_cpu_type_info); -#ifdef CONFIG_KVM +#if defined(CONFIG_KVM) || defined(CONFIG_HVF) type_register_static(&host_arm_cpu_type_info); #endif } diff --git a/target/arm/cpu.h b/target/arm/cpu.h index 09d9027734..3ed03391a8 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -3015,6 +3015,8 @@ bool write_cpustate_to_list(ARMCPU *cpu, bool kvm_sync); #define ARM_CPU_TYPE_NAME(name) (name ARM_CPU_TYPE_SUFFIX) #define CPU_RESOLVING_TYPE TYPE_ARM_CPU +#define TYPE_ARM_HOST_CPU "host-" TYPE_ARM_CPU + #define cpu_signal_handler cpu_arm_signal_handler #define cpu_list arm_cpu_list diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c index 49f265cc08..025280f4c9 100644 --- a/target/arm/hvf/hvf.c +++ b/target/arm/hvf/hvf.c @@ -17,6 +17,7 @@ #include "sysemu/hvf.h" #include "sysemu/hvf_int.h" #include "sysemu/hw_accel.h" +#include "hvf_arm.h" #include <mach/mach_time.h> @@ -54,6 +55,16 @@ typedef struct HVFVTimer { static HVFVTimer vtimer; +typedef struct ARMHostCPUFeatures { + ARMISARegisters isar; + uint64_t features; + uint64_t midr; + uint32_t reset_sctlr; + const char *dtb_compatible; +} ARMHostCPUFeatures; + +static ARMHostCPUFeatures arm_host_cpu_features; + struct hvf_reg_match { int reg; uint64_t offset; @@ -416,6 +427,86 @@ static uint64_t hvf_get_reg(CPUState *cpu, int rt) return val; } +static bool hvf_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf) +{ + ARMISARegisters host_isar = {}; + const struct isar_regs { + int reg; + uint64_t *val; + } regs[] = { + { HV_SYS_REG_ID_AA64PFR0_EL1, &host_isar.id_aa64pfr0 }, + { HV_SYS_REG_ID_AA64PFR1_EL1, &host_isar.id_aa64pfr1 }, + { HV_SYS_REG_ID_AA64DFR0_EL1, &host_isar.id_aa64dfr0 }, + { HV_SYS_REG_ID_AA64DFR1_EL1, &host_isar.id_aa64dfr1 }, + { HV_SYS_REG_ID_AA64ISAR0_EL1, &host_isar.id_aa64isar0 }, + { HV_SYS_REG_ID_AA64ISAR1_EL1, &host_isar.id_aa64isar1 }, + { HV_SYS_REG_ID_AA64MMFR0_EL1, &host_isar.id_aa64mmfr0 }, + { HV_SYS_REG_ID_AA64MMFR1_EL1, &host_isar.id_aa64mmfr1 }, + { HV_SYS_REG_ID_AA64MMFR2_EL1, &host_isar.id_aa64mmfr2 }, + }; + hv_vcpu_t fd; + hv_return_t r = HV_SUCCESS; + hv_vcpu_exit_t *exit; + int i; + + ahcf->dtb_compatible = "arm,arm-v8"; + ahcf->features = (1ULL << ARM_FEATURE_V8) | + (1ULL << ARM_FEATURE_NEON) | + (1ULL << ARM_FEATURE_AARCH64) | + (1ULL << ARM_FEATURE_PMU) | + (1ULL << ARM_FEATURE_GENERIC_TIMER); + + /* We set up a small vcpu to extract host registers */ + + if (hv_vcpu_create(&fd, &exit, NULL) != HV_SUCCESS) { + return false; + } + + for (i = 0; i < ARRAY_SIZE(regs); i++) { + r |= hv_vcpu_get_sys_reg(fd, regs[i].reg, regs[i].val); + } + r |= hv_vcpu_get_sys_reg(fd, HV_SYS_REG_MIDR_EL1, &ahcf->midr); + r |= hv_vcpu_destroy(fd); + + ahcf->isar = host_isar; + + /* + * 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 + */ + ahcf->reset_sctlr = 0x30100180; + /* OVMF chokes on boot if SPAN is not set, so default it to on */ + ahcf->reset_sctlr |= 0x00800000; + + /* Make sure we don't advertise AArch32 support for EL0/EL1 */ + if ((host_isar.id_aa64pfr0 & 0xff) != 0x11) { + return false; + } + + return r == HV_SUCCESS; +} + +void hvf_arm_set_cpu_features_from_host(ARMCPU *cpu) +{ + if (!arm_host_cpu_features.dtb_compatible) { + if (!hvf_enabled() || + !hvf_arm_get_host_cpu_features(&arm_host_cpu_features)) { + /* + * We can't report this error yet, so flag that we need to + * in arm_cpu_realizefn(). + */ + cpu->host_cpu_probe_failed = true; + return; + } + } + + cpu->dtb_compatible = arm_host_cpu_features.dtb_compatible; + cpu->isar = arm_host_cpu_features.isar; + cpu->env.features = arm_host_cpu_features.features; + cpu->midr = arm_host_cpu_features.midr; + cpu->reset_sctlr = arm_host_cpu_features.reset_sctlr; +} + void hvf_arch_vcpu_destroy(CPUState *cpu) { } diff --git a/target/arm/hvf_arm.h b/target/arm/hvf_arm.h new file mode 100644 index 0000000000..603074a331 --- /dev/null +++ b/target/arm/hvf_arm.h @@ -0,0 +1,19 @@ +/* + * QEMU Hypervisor.framework (HVF) support -- ARM specifics + * + * Copyright (c) 2021 Alexander Graf + * + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. + * + */ + +#ifndef QEMU_HVF_ARM_H +#define QEMU_HVF_ARM_H + +#include "qemu/accel.h" +#include "cpu.h" + +void hvf_arm_set_cpu_features_from_host(struct ARMCPU *cpu); + +#endif diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h index 0613454975..b7f78b5215 100644 --- a/target/arm/kvm_arm.h +++ b/target/arm/kvm_arm.h @@ -214,8 +214,6 @@ bool kvm_arm_create_scratch_host_vcpu(const uint32_t *cpus_to_try, */ void kvm_arm_destroy_scratch_host_vcpu(int *fdarray); -#define TYPE_ARM_HOST_CPU "host-" TYPE_ARM_CPU - /** * ARMHostCPUFeatures: information about the host CPU (identified * by asking the host kernel)