Re: [PATCH 4/8] arm: Synchronize CPU on PSCI on

Re: [PATCH 4/8] arm: Synchronize CPU on PSCI on

[Peter Maydell]

On Thu, 26 Nov 2020 at 21:36, Alexander Graf <agraf@csgraf.de> wrote:
>
> We are going to reuse the TCG PSCI code for HVF. This however means that we
> need to ensure that CPU register state is synchronized properly between the
> two worlds.
>
> So let's make sure that at least on the PSCI on call, the secondary core gets
> to sync its registers after reset, so that changes also propagate.
>
> Signed-off-by: Alexander Graf <agraf@csgraf.de>
> ---
>  target/arm/arm-powerctl.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/target/arm/arm-powerctl.c b/target/arm/arm-powerctl.c
> index b75f813b40..256f7cfdcd 100644
> --- a/target/arm/arm-powerctl.c
> +++ b/target/arm/arm-powerctl.c
> @@ -15,6 +15,7 @@
>  #include "arm-powerctl.h"
>  #include "qemu/log.h"
>  #include "qemu/main-loop.h"
> +#include "sysemu/hw_accel.h"
>
>  #ifndef DEBUG_ARM_POWERCTL
>  #define DEBUG_ARM_POWERCTL 0
> @@ -66,6 +67,8 @@ static void arm_set_cpu_on_async_work(CPUState *target_cpu_state,
>      cpu_reset(target_cpu_state);
>      target_cpu_state->halted = 0;
>
> +    cpu_synchronize_state(target_cpu_state);
> +
>      if (info->target_aa64) {
>          if ((info->target_el < 3) && arm_feature(&target_cpu->env,
>                                                   ARM_FEATURE_EL3))

This looks weird. The CPU was off, so not running anything.
Why doesn't the state we set up here get synchronized to
HVF as part of the normal enter-guest-code process that we
do when we do whatever HVF's equivalent of KVM_RUN is ?

Also, we change more bits of CPU state later in this function,
so if we do need to manually sychronize in this function this
doesn't seem like the right place...

thanks
-- PMM

Re: [PATCH 4/8] arm: Synchronize CPU on PSCI on

[Alexander Graf]

On 26.11.20 22:47, Peter Maydell wrote:
> On Thu, 26 Nov 2020 at 21:36, Alexander Graf <agraf@csgraf.de> wrote:
>> We are going to reuse the TCG PSCI code for HVF. This however means that we
>> need to ensure that CPU register state is synchronized properly between the
>> two worlds.
>>
>> So let's make sure that at least on the PSCI on call, the secondary core gets
>> to sync its registers after reset, so that changes also propagate.
>>
>> Signed-off-by: Alexander Graf <agraf@csgraf.de>
>> ---
>>   target/arm/arm-powerctl.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/target/arm/arm-powerctl.c b/target/arm/arm-powerctl.c
>> index b75f813b40..256f7cfdcd 100644
>> --- a/target/arm/arm-powerctl.c
>> +++ b/target/arm/arm-powerctl.c
>> @@ -15,6 +15,7 @@
>>   #include "arm-powerctl.h"
>>   #include "qemu/log.h"
>>   #include "qemu/main-loop.h"
>> +#include "sysemu/hw_accel.h"
>>
>>   #ifndef DEBUG_ARM_POWERCTL
>>   #define DEBUG_ARM_POWERCTL 0
>> @@ -66,6 +67,8 @@ static void arm_set_cpu_on_async_work(CPUState *target_cpu_state,
>>       cpu_reset(target_cpu_state);
>>       target_cpu_state->halted = 0;
>>
>> +    cpu_synchronize_state(target_cpu_state);
>> +
>>       if (info->target_aa64) {
>>           if ((info->target_el < 3) && arm_feature(&target_cpu->env,
>>                                                    ARM_FEATURE_EL3))
> This looks weird. The CPU was off, so not running anything.
> Why doesn't the state we set up here get synchronized to
> HVF as part of the normal enter-guest-code process that we
> do when we do whatever HVF's equivalent of KVM_RUN is ?
>
> Also, we change more bits of CPU state later in this function,
> so if we do need to manually sychronize in this function this
> doesn't seem like the right place...


cpu_synchronize_state() sets the CPU registers into "dirty" state which 
means that env now holds the current copy. On the next entry, we then 
sync them back into HVF.

Without the cpu_synchronize_state() call, HVF never knows that the CPU 
state is actually dirty. I guess it could as well live in cpu_reset() 
somewhere, but we have to get the state switched over to dirty one way 
or another.

One interesting thing to note here is that the CPU actually comes up in 
"dirty" after init. But init is done on realization already. I'm not 
sure why we lose the dirty state in between that and the reset.


Alex

Re: [PATCH 4/8] arm: Synchronize CPU on PSCI on

[Peter Maydell]

On Thu, 26 Nov 2020 at 22:16, Alexander Graf <agraf@csgraf.de> wrote:
> cpu_synchronize_state() sets the CPU registers into "dirty" state which
> means that env now holds the current copy. On the next entry, we then
> sync them back into HVF.
>
> Without the cpu_synchronize_state() call, HVF never knows that the CPU
> state is actually dirty. I guess it could as well live in cpu_reset()
> somewhere, but we have to get the state switched over to dirty one way
> or another.
>
> One interesting thing to note here is that the CPU actually comes up in
> "dirty" after init. But init is done on realization already. I'm not
> sure why we lose the dirty state in between that and the reset.

Yeah, it sounds like you need to figure out where the dirty
to not-dirty transitions ought to be happening rather than
just fudging things here...

-- PMM

Re: [PATCH 4/8] arm: Synchronize CPU on PSCI on

[Alexander Graf]

On 26.11.20 23:26, Peter Maydell wrote:
> On Thu, 26 Nov 2020 at 22:16, Alexander Graf <agraf@csgraf.de> wrote:
>> cpu_synchronize_state() sets the CPU registers into "dirty" state which
>> means that env now holds the current copy. On the next entry, we then
>> sync them back into HVF.
>>
>> Without the cpu_synchronize_state() call, HVF never knows that the CPU
>> state is actually dirty. I guess it could as well live in cpu_reset()
>> somewhere, but we have to get the state switched over to dirty one way
>> or another.
>>
>> One interesting thing to note here is that the CPU actually comes up in
>> "dirty" after init. But init is done on realization already. I'm not
>> sure why we lose the dirty state in between that and the reset.
> Yeah, it sounds like you need to figure out where the dirty
> to not-dirty transitions ought to be happening rather than
> just fudging things here...


When init is complete (system is ready to launch), the CPU state is 
pushed to HVF and dirty is set to false. So by design, a normal 
cpu_reset doesn't have vcpu_dirty set.

How about this patch instead?

Alex



commit 8c61bc4d613b01e251b6b2f892d1a55a333c6e37
Author: Alexander Graf <agraf@csgraf.de>
Date:   Thu Nov 26 02:47:09 2020 +0100

     hvf: arm: Mark CPU as dirty on reset

     When clearing internal state of a CPU, we should also make sure 
that HVF
     knows about it and can push the new values down to vcpu state.

     Make sure that with HVF enabled, we tell it that it should synchronize
     CPU state on next entry after a reset.

     This fixes PSCI handling, because now newly pushed state such as X0 and
     PC on remote CPU enablement also get pushed into HVF.

     Signed-off-by: Alexander Graf <agraf@csgraf.de>

diff --git a/target/arm/arm-powerctl.c b/target/arm/arm-powerctl.c
index b75f813b40..a49a5b32e6 100644
--- a/target/arm/arm-powerctl.c
+++ b/target/arm/arm-powerctl.c
@@ -15,6 +15,7 @@
  #include "arm-powerctl.h"
  #include "qemu/log.h"
  #include "qemu/main-loop.h"
+#include "sysemu/hw_accel.h"

  #ifndef DEBUG_ARM_POWERCTL
  #define DEBUG_ARM_POWERCTL 0
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index db6f7c34ed..9a501ea4bd 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -411,6 +411,8 @@ static void arm_cpu_reset(DeviceState *dev)
  #ifndef CONFIG_USER_ONLY
      if (kvm_enabled()) {
          kvm_arm_reset_vcpu(cpu);
+    } else if (hvf_enabled()) {
+        s->vcpu_dirty = true;
      }
  #endif

Re: [PATCH 4/8] arm: Synchronize CPU on PSCI on

[Paolo Bonzini]

On 27/11/20 00:32, Alexander Graf wrote:
> 
> On 26.11.20 23:26, Peter Maydell wrote:
>> On Thu, 26 Nov 2020 at 22:16, Alexander Graf <agraf@csgraf.de> wrote:
>>> cpu_synchronize_state() sets the CPU registers into "dirty" state which
>>> means that env now holds the current copy. On the next entry, we then
>>> sync them back into HVF.
>>>
>>> Without the cpu_synchronize_state() call, HVF never knows that the CPU
>>> state is actually dirty. I guess it could as well live in cpu_reset()
>>> somewhere, but we have to get the state switched over to dirty one way
>>> or another.
>>>
>>> One interesting thing to note here is that the CPU actually comes up in
>>> "dirty" after init. But init is done on realization already. I'm not
>>> sure why we lose the dirty state in between that and the reset.
>> Yeah, it sounds like you need to figure out where the dirty
>> to not-dirty transitions ought to be happening rather than
>> just fudging things here...
> 
> 
> When init is complete (system is ready to launch), the CPU state is 
> pushed to HVF and dirty is set to false. So by design, a normal 
> cpu_reset doesn't have vcpu_dirty set.
> 
> How about this patch instead?
> 
> Alex
> 
> 
> 
> commit 8c61bc4d613b01e251b6b2f892d1a55a333c6e37
> Author: Alexander Graf <agraf@csgraf.de>
> Date:   Thu Nov 26 02:47:09 2020 +0100
> 
>      hvf: arm: Mark CPU as dirty on reset
> 
>      When clearing internal state of a CPU, we should also make sure 
> that HVF
>      knows about it and can push the new values down to vcpu state.
> 
>      Make sure that with HVF enabled, we tell it that it should synchronize
>      CPU state on next entry after a reset.
> 
>      This fixes PSCI handling, because now newly pushed state such as X0 
> and
>      PC on remote CPU enablement also get pushed into HVF.
> 
>      Signed-off-by: Alexander Graf <agraf@csgraf.de>
> 
> diff --git a/target/arm/arm-powerctl.c b/target/arm/arm-powerctl.c
> index b75f813b40..a49a5b32e6 100644
> --- a/target/arm/arm-powerctl.c
> +++ b/target/arm/arm-powerctl.c
> @@ -15,6 +15,7 @@
>   #include "arm-powerctl.h"
>   #include "qemu/log.h"
>   #include "qemu/main-loop.h"
> +#include "sysemu/hw_accel.h"
> 
>   #ifndef DEBUG_ARM_POWERCTL
>   #define DEBUG_ARM_POWERCTL 0
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index db6f7c34ed..9a501ea4bd 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -411,6 +411,8 @@ static void arm_cpu_reset(DeviceState *dev)
>   #ifndef CONFIG_USER_ONLY
>       if (kvm_enabled()) {
>           kvm_arm_reset_vcpu(cpu);
> +    } else if (hvf_enabled()) {
> +        s->vcpu_dirty = true;
>       }
>   #endif
> 

Why only for HVF and only for ARM?  For example hax_init_vcpu and 
whpx_init_vcpu both set s->vcpu_dirty; should you just set it 
unconditionally in cpu_common_reset?

Thanks,

Paolo

Re: [PATCH 4/8] arm: Synchronize CPU on PSCI on

[Alexander Graf]

On 27.11.20 05:41, Paolo Bonzini wrote:
> On 27/11/20 00:32, Alexander Graf wrote:
>>
>> On 26.11.20 23:26, Peter Maydell wrote:
>>> On Thu, 26 Nov 2020 at 22:16, Alexander Graf <agraf@csgraf.de> wrote:
>>>> cpu_synchronize_state() sets the CPU registers into "dirty" state 
>>>> which
>>>> means that env now holds the current copy. On the next entry, we then
>>>> sync them back into HVF.
>>>>
>>>> Without the cpu_synchronize_state() call, HVF never knows that the CPU
>>>> state is actually dirty. I guess it could as well live in cpu_reset()
>>>> somewhere, but we have to get the state switched over to dirty one way
>>>> or another.
>>>>
>>>> One interesting thing to note here is that the CPU actually comes 
>>>> up in
>>>> "dirty" after init. But init is done on realization already. I'm not
>>>> sure why we lose the dirty state in between that and the reset.
>>> Yeah, it sounds like you need to figure out where the dirty
>>> to not-dirty transitions ought to be happening rather than
>>> just fudging things here...
>>
>>
>> When init is complete (system is ready to launch), the CPU state is 
>> pushed to HVF and dirty is set to false. So by design, a normal 
>> cpu_reset doesn't have vcpu_dirty set.
>>
>> How about this patch instead?
>>
>> Alex
>>
>>
>>
>> commit 8c61bc4d613b01e251b6b2f892d1a55a333c6e37
>> Author: Alexander Graf <agraf@csgraf.de>
>> Date:   Thu Nov 26 02:47:09 2020 +0100
>>
>>      hvf: arm: Mark CPU as dirty on reset
>>
>>      When clearing internal state of a CPU, we should also make sure 
>> that HVF
>>      knows about it and can push the new values down to vcpu state.
>>
>>      Make sure that with HVF enabled, we tell it that it should 
>> synchronize
>>      CPU state on next entry after a reset.
>>
>>      This fixes PSCI handling, because now newly pushed state such as 
>> X0 and
>>      PC on remote CPU enablement also get pushed into HVF.
>>
>>      Signed-off-by: Alexander Graf <agraf@csgraf.de>
>>
>> diff --git a/target/arm/arm-powerctl.c b/target/arm/arm-powerctl.c
>> index b75f813b40..a49a5b32e6 100644
>> --- a/target/arm/arm-powerctl.c
>> +++ b/target/arm/arm-powerctl.c
>> @@ -15,6 +15,7 @@
>>   #include "arm-powerctl.h"
>>   #include "qemu/log.h"
>>   #include "qemu/main-loop.h"
>> +#include "sysemu/hw_accel.h"
>>
>>   #ifndef DEBUG_ARM_POWERCTL
>>   #define DEBUG_ARM_POWERCTL 0
>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>> index db6f7c34ed..9a501ea4bd 100644
>> --- a/target/arm/cpu.c
>> +++ b/target/arm/cpu.c
>> @@ -411,6 +411,8 @@ static void arm_cpu_reset(DeviceState *dev)
>>   #ifndef CONFIG_USER_ONLY
>>       if (kvm_enabled()) {
>>           kvm_arm_reset_vcpu(cpu);
>> +    } else if (hvf_enabled()) {
>> +        s->vcpu_dirty = true;
>>       }
>>   #endif
>>
>
> Why only for HVF and only for ARM?  For example hax_init_vcpu and 
> whpx_init_vcpu both set s->vcpu_dirty; should you just set it 
> unconditionally in cpu_common_reset?


Mostly because there is a lot of super fragile logic all over resets 
atm. Init setts dirty, post-init clears it. Then the arch reset handlers 
assume that state is not dirty and fiddle with KVM reset ioctls and KVM 
register modification ioctls directly. Mostly because KVM for the most 
part implements its own reset logic.

I'm fairy sure I'd break someone unintentionally if I just throw this 
into the common reset handler.

However, if we're happy to fix the fallout when it arises, I'm happy to 
do so.


Alex

Re: [PATCH 4/8] arm: Synchronize CPU on PSCI on

[Paolo Bonzini]

On 27/11/20 11:58, Alexander Graf wrote:
> Mostly because there is a lot of super fragile logic all over resets 
> atm. Init setts dirty, post-init clears it. Then the arch reset handlers 
> assume that state is not dirty and fiddle with KVM reset ioctls and KVM 
> register modification ioctls directly. Mostly because KVM for the most 
> part implements its own reset logic.
> 
> I'm fairy sure I'd break someone unintentionally if I just throw this 
> into the common reset handler.
> 
> However, if we're happy to fix the fallout when it arises, I'm happy to 
> do so.

I'll decline the offer. ;)

Paolo

Re: [PATCH 8/8] hw/arm/virt: Disable highmem when on hypervisor.framework

[Peter Maydell]

On Thu, 26 Nov 2020 at 21:36, Alexander Graf <agraf@csgraf.de> 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.

If you have direct access to the host ID_AA64MMFR0_EL1 you can look
at the PARange field; otherwise start a stunt VM and look at its
ID_AA64MMFR0_EL1.PARange (this is what we do for KVM to query various
things about the VM's capabilities/ID regs).

thanks
-- PMM

Re: [PATCH 8/8] hw/arm/virt: Disable highmem when on hypervisor.framework

[Alexander Graf]

On 26.11.20 22:54, Peter Maydell wrote:
> On Thu, 26 Nov 2020 at 21:36, Alexander Graf <agraf@csgraf.de> 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.
> If you have direct access to the host ID_AA64MMFR0_EL1 you can look
> at the PARange field; otherwise start a stunt VM and look at its
> ID_AA64MMFR0_EL1.PARange (this is what we do for KVM to query various
> things about the VM's capabilities/ID regs).


When I tried, I couldn't fetch IID_AA64MMFR0_EL1 as sysreg via hvf.

Are you suggesting that on boot, we start a tiny mini-VM to enumerate 
the PARange and set highmem based on it? That sounds like absolute 
overkill to me ...


Alex

Re: [PATCH 8/8] hw/arm/virt: Disable highmem when on hypervisor.framework

[Peter Maydell]

On Thu, 26 Nov 2020 at 22:17, Alexander Graf <agraf@csgraf.de> wrote:
> Are you suggesting that on boot, we start a tiny mini-VM to enumerate
> the PARange and set highmem based on it? That sounds like absolute
> overkill to me ...

You'll likely want that for a bunch of other information too.
The KVM version of this is kvm_arm_create_scratch_host_vcpu().

In particular you must populate the ID register information
correctly because target/arm code is steadily migrating
towards "don't have ARM_FEATURE_* bits, just look at ID
register fields". You'll want some equivalent of
kvm_arm_get_host_cpu_features(), I expect.

thanks
-- PMM

[PATCH 0/8] hvf: Implement Apple Silicon Support

[Alexander Graf]

Now that Apple Silicon is widely available, people are obviously excited
to try and run virtualized workloads on them, such as Linux and Windows.

This patch set implements a rudimentary, first version to get the ball
going on that. With this applied, I can successfully run both Linux and
Windows as guests, albeit with a few caveats:

  * no WFI emulation, a vCPU always uses 100%
  * vtimer handling is a bit hacky
  * we handle most sysregs flying blindly, just returning 0
  * XHCI breaks in OVMF, works in Linux+Windows

Despite those drawbacks, it's still an exciting place to start playing
with the power of Apple Silicon.

Enjoy!

Alex

Alexander Graf (8):
  hvf: Add hypervisor entitlement to output binaries
  hvf: Move common code out
  arm: Set PSCI to 0.2 for HVF
  arm: Synchronize CPU on PSCI on
  hvf: Add Apple Silicon support
  hvf: Use OS provided vcpu kick function
  arm: Add Hypervisor.framework build target
  hw/arm/virt: Disable highmem when on hypervisor.framework

 MAINTAINERS                  |  14 +-
 accel/hvf/entitlements.plist |   8 +
 accel/hvf/hvf-all.c          |  56 ++++
 accel/hvf/hvf-cpus.c         | 484 +++++++++++++++++++++++++++++++++++
 accel/hvf/meson.build        |   7 +
 accel/meson.build            |   1 +
 hw/arm/virt.c                |   9 +
 include/hw/core/cpu.h        |   3 +-
 include/sysemu/hvf_int.h     |  69 +++++
 meson.build                  |  39 ++-
 scripts/entitlement.sh       |  11 +
 target/arm/arm-powerctl.c    |   3 +
 target/arm/cpu.c             |   4 +
 target/arm/hvf/hvf.c         | 345 +++++++++++++++++++++++++
 target/arm/hvf/meson.build   |   3 +
 target/arm/meson.build       |   2 +
 target/i386/hvf/hvf-cpus.c   | 131 ----------
 target/i386/hvf/hvf-cpus.h   |  25 --
 target/i386/hvf/hvf-i386.h   |  48 +---
 target/i386/hvf/hvf.c        | 360 +-------------------------
 target/i386/hvf/meson.build  |   1 -
 target/i386/hvf/x86hvf.c     |  11 +-
 target/i386/hvf/x86hvf.h     |   2 -
 23 files changed, 1061 insertions(+), 575 deletions(-)
 create mode 100644 accel/hvf/entitlements.plist
 create mode 100644 accel/hvf/hvf-all.c
 create mode 100644 accel/hvf/hvf-cpus.c
 create mode 100644 accel/hvf/meson.build
 create mode 100644 include/sysemu/hvf_int.h
 create mode 100755 scripts/entitlement.sh
 create mode 100644 target/arm/hvf/hvf.c
 create mode 100644 target/arm/hvf/meson.build
 delete mode 100644 target/i386/hvf/hvf-cpus.c
 delete mode 100644 target/i386/hvf/hvf-cpus.h

-- 
2.24.3 (Apple Git-128)

[PATCH 8/8] hw/arm/virt: Disable highmem when on hypervisor.framework

[Alexander Graf]

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;
+    }
+
     /*
      * 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)

Re: [PATCH 8/8] hw/arm/virt: Disable highmem when on hypervisor.framework

[Eduardo Habkost]

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)
> 

-- 
Eduardo

Re: [PATCH 8/8] hw/arm/virt: Disable highmem when on hypervisor.framework

[Peter Maydell]

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

Re: [PATCH 8/8] hw/arm/virt: Disable highmem when on hypervisor.framework

[Eduardo Habkost]

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.

-- 
Eduardo

Re: [PATCH 8/8] hw/arm/virt: Disable highmem when on hypervisor.framework

[Peter Maydell]

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

Re: [PATCH 8/8] hw/arm/virt: Disable highmem when on hypervisor.framework

[Eduardo Habkost]

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.

-- 
Eduardo

Re: [PATCH 8/8] hw/arm/virt: Disable highmem when on hypervisor.framework

[Peter Maydell]

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

Re: [PATCH 8/8] hw/arm/virt: Disable highmem when on hypervisor.framework

[Eduardo Habkost]

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.

-- 
Eduardo

Re: [PATCH 8/8] hw/arm/virt: Disable highmem when on hypervisor.framework

[Peter Maydell]

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

Re: [PATCH 8/8] hw/arm/virt: Disable highmem when on hypervisor.framework

[Eduardo Habkost]

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.

-- 
Eduardo

Re: [PATCH 8/8] hw/arm/virt: Disable highmem when on hypervisor.framework

[Peter Maydell]

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

Re: [PATCH 8/8] hw/arm/virt: Disable highmem when on hypervisor.framework

[Alexander Graf]

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