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

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

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

[Alexander Graf]

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)) {
-- 
2.24.3 (Apple Git-128)