| Message ID | 20201126215017.41156-9-agraf@csgraf.de |
|---|---|
| State | New |
| Headers | show |
| Series | hvf: Implement Apple Silicon Support | expand |
On Thu, Nov 26, 2020 at 10:50:17PM +0100, Alexander Graf wrote: > The Apple M1 only supports up to 36 bits of physical address space. That > means we can not fit the 64bit MMIO BAR region into our address space. > > To fix this, let's not expose a 64bit MMIO BAR region when running on > Apple Silicon. > > I have not been able to find a way to enumerate that easily, so let's > just assume we always have that little PA space on hypervisor.framework > systems. > > Signed-off-by: Alexander Graf <agraf@csgraf.de> > --- > hw/arm/virt.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index 27dbeb549e..d74053ecd4 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -45,6 +45,7 @@ > #include "hw/display/ramfb.h" > #include "net/net.h" > #include "sysemu/device_tree.h" > +#include "sysemu/hvf.h" > #include "sysemu/numa.h" > #include "sysemu/runstate.h" > #include "sysemu/sysemu.h" > @@ -1746,6 +1747,14 @@ static void machvirt_init(MachineState *machine) > unsigned int smp_cpus = machine->smp.cpus; > unsigned int max_cpus = machine->smp.max_cpus; > > + /* > + * On Hypervisor.framework capable systems, we only have 36 bits of PA > + * space, which is not enough to fit a 64bit BAR space > + */ > + if (hvf_enabled()) { > + vms->highmem = false; > + } Direct checks for *_enabled() are a pain to clean up later when we add support to new accelerators. Can't this be implemented as (e.g.) a AccelClass::max_physical_address_bits field? > + > /* > * In accelerated mode, the memory map is computed earlier in kvm_type() > * to create a VM with the right number of IPA bits. > -- > 2.24.3 (Apple Git-128) >
On Thu, 26 Nov 2020 at 22:14, Eduardo Habkost <ehabkost@redhat.com> wrote: > > On Thu, Nov 26, 2020 at 10:50:17PM +0100, Alexander Graf wrote: > > The Apple M1 only supports up to 36 bits of physical address space. That > > means we can not fit the 64bit MMIO BAR region into our address space. > > > > To fix this, let's not expose a 64bit MMIO BAR region when running on > > Apple Silicon. > > > > I have not been able to find a way to enumerate that easily, so let's > > just assume we always have that little PA space on hypervisor.framework > > systems. > > > > Signed-off-by: Alexander Graf <agraf@csgraf.de> > > --- > > hw/arm/virt.c | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > > index 27dbeb549e..d74053ecd4 100644 > > --- a/hw/arm/virt.c > > +++ b/hw/arm/virt.c > > @@ -45,6 +45,7 @@ > > #include "hw/display/ramfb.h" > > #include "net/net.h" > > #include "sysemu/device_tree.h" > > +#include "sysemu/hvf.h" > > #include "sysemu/numa.h" > > #include "sysemu/runstate.h" > > #include "sysemu/sysemu.h" > > @@ -1746,6 +1747,14 @@ static void machvirt_init(MachineState *machine) > > unsigned int smp_cpus = machine->smp.cpus; > > unsigned int max_cpus = machine->smp.max_cpus; > > > > + /* > > + * On Hypervisor.framework capable systems, we only have 36 bits of PA > > + * space, which is not enough to fit a 64bit BAR space > > + */ > > + if (hvf_enabled()) { > > + vms->highmem = false; > > + } > > Direct checks for *_enabled() are a pain to clean up later when > we add support to new accelerators. Can't this be implemented as > (e.g.) a AccelClass::max_physical_address_bits field? It's a property of the CPU (eg our emulated TCG CPUs may have varying supported numbers of physical address bits). So the virt board ought to look at the CPU, and the CPU should be set up with the right information for all of KVM, TCG, HVF (either a specific max_phys_addr_bits value or just ensure its ID_AA64MMFR0_EL1.PARange is right, not sure which would be easier/nicer). thanks -- PMM
On Thu, Nov 26, 2020 at 10:29:01PM +0000, Peter Maydell wrote: > On Thu, 26 Nov 2020 at 22:14, Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > On Thu, Nov 26, 2020 at 10:50:17PM +0100, Alexander Graf wrote: > > > The Apple M1 only supports up to 36 bits of physical address space. That > > > means we can not fit the 64bit MMIO BAR region into our address space. > > > > > > To fix this, let's not expose a 64bit MMIO BAR region when running on > > > Apple Silicon. > > > > > > I have not been able to find a way to enumerate that easily, so let's > > > just assume we always have that little PA space on hypervisor.framework > > > systems. > > > > > > Signed-off-by: Alexander Graf <agraf@csgraf.de> > > > --- > > > hw/arm/virt.c | 9 +++++++++ > > > 1 file changed, 9 insertions(+) > > > > > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > > > index 27dbeb549e..d74053ecd4 100644 > > > --- a/hw/arm/virt.c > > > +++ b/hw/arm/virt.c > > > @@ -45,6 +45,7 @@ > > > #include "hw/display/ramfb.h" > > > #include "net/net.h" > > > #include "sysemu/device_tree.h" > > > +#include "sysemu/hvf.h" > > > #include "sysemu/numa.h" > > > #include "sysemu/runstate.h" > > > #include "sysemu/sysemu.h" > > > @@ -1746,6 +1747,14 @@ static void machvirt_init(MachineState *machine) > > > unsigned int smp_cpus = machine->smp.cpus; > > > unsigned int max_cpus = machine->smp.max_cpus; > > > > > > + /* > > > + * On Hypervisor.framework capable systems, we only have 36 bits of PA > > > + * space, which is not enough to fit a 64bit BAR space > > > + */ > > > + if (hvf_enabled()) { > > > + vms->highmem = false; > > > + } > > > > Direct checks for *_enabled() are a pain to clean up later when > > we add support to new accelerators. Can't this be implemented as > > (e.g.) a AccelClass::max_physical_address_bits field? > > It's a property of the CPU (eg our emulated TCG CPUs may have > varying supported numbers of physical address bits). So the > virt board ought to look at the CPU, and the CPU should be > set up with the right information for all of KVM, TCG, HVF > (either a specific max_phys_addr_bits value or just ensure > its ID_AA64MMFR0_EL1.PARange is right, not sure which would > be easier/nicer). Agreed. My suggestion would still apply to the CPU code that will pick the address size; ideally, accel-specific behaviour should be represented as meaningful fields in AccelClass (either data or virtual methods) instead of direct *_enabled() checks.
On Fri, 27 Nov 2020 at 16:26, Eduardo Habkost <ehabkost@redhat.com> wrote: > > On Thu, Nov 26, 2020 at 10:29:01PM +0000, Peter Maydell wrote: > > On Thu, 26 Nov 2020 at 22:14, Eduardo Habkost <ehabkost@redhat.com> wrote: > > > Direct checks for *_enabled() are a pain to clean up later when > > > we add support to new accelerators. Can't this be implemented as > > > (e.g.) a AccelClass::max_physical_address_bits field? > > > > It's a property of the CPU (eg our emulated TCG CPUs may have > > varying supported numbers of physical address bits). So the > > virt board ought to look at the CPU, and the CPU should be > > set up with the right information for all of KVM, TCG, HVF > > (either a specific max_phys_addr_bits value or just ensure > > its ID_AA64MMFR0_EL1.PARange is right, not sure which would > > be easier/nicer). > > Agreed. > > My suggestion would still apply to the CPU code that will pick > the address size; ideally, accel-specific behaviour should be > represented as meaningful fields in AccelClass (either data or > virtual methods) instead of direct *_enabled() checks. Having looked a bit more closely at some of the relevant target/arm code, I think the best approach is going to be that in virt.c we just check the PARange ID register field (probably via a convenience function that does the conversion of that to a nice number-of-bits return value; we might even have one already). KVM and TCG both already set that ID register field in the CPU struct correctly in their existing implicitly-accelerator-specific code; HVF needs to do the same. thanks -- PMM
On Fri, Nov 27, 2020 at 04:38:18PM +0000, Peter Maydell wrote: > On Fri, 27 Nov 2020 at 16:26, Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > On Thu, Nov 26, 2020 at 10:29:01PM +0000, Peter Maydell wrote: > > > On Thu, 26 Nov 2020 at 22:14, Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > Direct checks for *_enabled() are a pain to clean up later when > > > > we add support to new accelerators. Can't this be implemented as > > > > (e.g.) a AccelClass::max_physical_address_bits field? > > > > > > It's a property of the CPU (eg our emulated TCG CPUs may have > > > varying supported numbers of physical address bits). So the > > > virt board ought to look at the CPU, and the CPU should be > > > set up with the right information for all of KVM, TCG, HVF > > > (either a specific max_phys_addr_bits value or just ensure > > > its ID_AA64MMFR0_EL1.PARange is right, not sure which would > > > be easier/nicer). > > > > Agreed. > > > > My suggestion would still apply to the CPU code that will pick > > the address size; ideally, accel-specific behaviour should be > > represented as meaningful fields in AccelClass (either data or > > virtual methods) instead of direct *_enabled() checks. > > Having looked a bit more closely at some of the relevant target/arm > code, I think the best approach is going to be that in virt.c > we just check the PARange ID register field (probably via > a convenience function that does the conversion of that to > a nice number-of-bits return value; we might even have one > already). KVM and TCG both already set that ID register field > in the CPU struct correctly in their existing > implicitly-accelerator-specific code; HVF needs to do the same. Do you know how the implicitly-accelerator-specific code is implemented? PARange is in id_aa64mmfr0, correct? I don't see any accel-specific code for initializing id_aa64mmfr0.
On Fri, 27 Nov 2020 at 16:38, Peter Maydell <peter.maydell@linaro.org> wrote: > Having looked a bit more closely at some of the relevant target/arm > code, I think the best approach is going to be that in virt.c > we just check the PARange ID register field (probably via > a convenience function that does the conversion of that to > a nice number-of-bits return value; we might even have one > already). Ha, in fact we're already doing something quite close to this, though instead of saying "decide whether to use highmem based on the CPU's PA range" we go for "report error to user if PA range is insufficient" and let the user pick some command line options that disable highmem if they want: if (aarch64 && vms->highmem) { int requested_pa_size = 64 - clz64(vms->highest_gpa); int pamax = arm_pamax(ARM_CPU(first_cpu)); if (pamax < requested_pa_size) { error_report("VCPU supports less PA bits (%d) than " "requested by the memory map (%d)", pamax, requested_pa_size); exit(1); } } thanks -- PMM
On Fri, 27 Nov 2020 at 16:47, Eduardo Habkost <ehabkost@redhat.com> wrote: > Do you know how the implicitly-accelerator-specific code is > implemented? PARange is in id_aa64mmfr0, correct? I don't see > any accel-specific code for initializing id_aa64mmfr0. For TCG, the value of id_aa64mmfr0 is set by the per-cpu init functions aarch64_a57_initfn(), aarch64_a72_initfn(), etc. For KVM, if we're using "-cpu cortex-a53" or "-cpu cortex-a57" these only work if the host CPU really is an A53 or A57, in which case the reset value set by the initfn is correct. In the more usual case of "-cpu host", we ask the kernel for the ID register values in kvm_arm_get_host_cpu_features(), which is part of the implementation of kvm_arm_set_cpu_features_from_host(), which gets called in aarch64_max_initfn() (inside a kvm_enabled() conditional). So there is a *_enabled() check involved, which I hadn't realised until I worked back up to where this stuff is called. thanks -- PMM
On Fri, Nov 27, 2020 at 04:53:59PM +0000, Peter Maydell wrote: > On Fri, 27 Nov 2020 at 16:47, Eduardo Habkost <ehabkost@redhat.com> wrote: > > Do you know how the implicitly-accelerator-specific code is > > implemented? PARange is in id_aa64mmfr0, correct? I don't see > > any accel-specific code for initializing id_aa64mmfr0. > > For TCG, the value of id_aa64mmfr0 is set by the per-cpu > init functions aarch64_a57_initfn(), aarch64_a72_initfn(), etc. > > For KVM, if we're using "-cpu cortex-a53" or "-cpu cortex-a57" > these only work if the host CPU really is an A53 or A57, in > which case the reset value set by the initfn is correct. > In the more usual case of "-cpu host", we ask the kernel for > the ID register values in kvm_arm_get_host_cpu_features(), > which is part of the implementation of > kvm_arm_set_cpu_features_from_host(), which gets called > in aarch64_max_initfn() (inside a kvm_enabled() conditional). > > So there is a *_enabled() check involved, which I hadn't > realised until I worked back up to where this stuff is called. > Thanks! Is the data returned by kvm_arm_get_host_cpu_features() supposed to eventually affect the value of id_aa64mmfr0? I don't see how that could happen.
On Fri, 27 Nov 2020 at 17:18, Eduardo Habkost <ehabkost@redhat.com> wrote: > Thanks! Is the data returned by kvm_arm_get_host_cpu_features() > supposed to eventually affect the value of id_aa64mmfr0? I don't > see how that could happen. kvm_arm_get_host_cpu_features() does: err |= read_sys_reg64(fdarray[2], &ahcf->isar.id_aa64mmfr0, ARM64_SYS_REG(3, 0, 0, 7, 0)); which is filling in data in the ARMHostCPUFeatures* that it is passed as an argument. The caller is kvm_arm_set_cpu_features_from_host(), which does kvm_arm_get_host_cpu_features(&arm_host_cpu_features) (assuming it hasn't already done it once and cached the results; arm_host_cpu_features is a global) and then cpu->isar = arm_host_cpu_features.isar; thus copying the ID values into the "struct ARMISARegisters isar" that is part of the ARMCPU struct. (It also copies across the 'features' word which gets set up with ARM_FEATURE_* flags for the benefit of the parts of the target code which key off those rather than ID register fields.) thanks -- PMM
On Fri, Nov 27, 2020 at 06:16:27PM +0000, Peter Maydell wrote: > On Fri, 27 Nov 2020 at 17:18, Eduardo Habkost <ehabkost@redhat.com> wrote: > > Thanks! Is the data returned by kvm_arm_get_host_cpu_features() > > supposed to eventually affect the value of id_aa64mmfr0? I don't > > see how that could happen. > > kvm_arm_get_host_cpu_features() does: > err |= read_sys_reg64(fdarray[2], &ahcf->isar.id_aa64mmfr0, > ARM64_SYS_REG(3, 0, 0, 7, 0)); > > which is filling in data in the ARMHostCPUFeatures* that it is > passed as an argument. The caller is kvm_arm_set_cpu_features_from_host(), > which does > kvm_arm_get_host_cpu_features(&arm_host_cpu_features) > (assuming it hasn't already done it once and cached the results; > arm_host_cpu_features is a global) and then > cpu->isar = arm_host_cpu_features.isar; > thus copying the ID values into the "struct ARMISARegisters isar" > that is part of the ARMCPU struct. (It also copies across the > 'features' word which gets set up with ARM_FEATURE_* flags > for the benefit of the parts of the target code which key off > those rather than ID register fields.) Thanks! For some reason I missed the line above when grepping for id_aa64mmfr0.
On 27.11.20 17:47, Peter Maydell wrote: > On Fri, 27 Nov 2020 at 16:38, Peter Maydell <peter.maydell@linaro.org> wrote: >> Having looked a bit more closely at some of the relevant target/arm >> code, I think the best approach is going to be that in virt.c >> we just check the PARange ID register field (probably via >> a convenience function that does the conversion of that to >> a nice number-of-bits return value; we might even have one >> already). > Ha, in fact we're already doing something quite close to this, > though instead of saying "decide whether to use highmem based > on the CPU's PA range" we go for "report error to user if PA > range is insufficient" and let the user pick some command line > options that disable highmem if they want: > > if (aarch64 && vms->highmem) { > int requested_pa_size = 64 - clz64(vms->highest_gpa); > int pamax = arm_pamax(ARM_CPU(first_cpu)); > > if (pamax < requested_pa_size) { > error_report("VCPU supports less PA bits (%d) than " > "requested by the memory map (%d)", > pamax, requested_pa_size); > exit(1); > } > } Turns out I can sync aa64mfr0 just fine as well. So I'll just do that and remove this patch. Alex
diff --git a/hw/arm/virt.c b/hw/arm/virt.c index 27dbeb549e..d74053ecd4 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -45,6 +45,7 @@ #include "hw/display/ramfb.h" #include "net/net.h" #include "sysemu/device_tree.h" +#include "sysemu/hvf.h" #include "sysemu/numa.h" #include "sysemu/runstate.h" #include "sysemu/sysemu.h" @@ -1746,6 +1747,14 @@ static void machvirt_init(MachineState *machine) unsigned int smp_cpus = machine->smp.cpus; unsigned int max_cpus = machine->smp.max_cpus; + /* + * On Hypervisor.framework capable systems, we only have 36 bits of PA + * space, which is not enough to fit a 64bit BAR space + */ + if (hvf_enabled()) { + vms->highmem = false; + } + /* * In accelerated mode, the memory map is computed earlier in kvm_type() * to create a VM with the right number of IPA bits.
The Apple M1 only supports up to 36 bits of physical address space. That means we can not fit the 64bit MMIO BAR region into our address space. To fix this, let's not expose a 64bit MMIO BAR region when running on Apple Silicon. I have not been able to find a way to enumerate that easily, so let's just assume we always have that little PA space on hypervisor.framework systems. Signed-off-by: Alexander Graf <agraf@csgraf.de> --- hw/arm/virt.c | 9 +++++++++ 1 file changed, 9 insertions(+)