| Message ID | 20201202190408.2041-6-agraf@csgraf.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hvf: Implement Apple Silicon Support | expand |
On Wed, Dec 02, 2020 at 08:04:03PM +0100, Alexander Graf wrote: > 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. > I'm sorry if I'm asking something dumb. But isn't cpu_synchronize_all_post_reset() is supposed to push QEMU state into HVF (or any other accel) after reset? For x86 it used to work: static void do_hvf_cpu_synchronize_post_reset(CPUState *cpu, run_on_cpu_data arg) { hvf_put_registers(cpu); cpu->vcpu_dirty = false; } Thanks, Roman > 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> > --- > target/arm/arm-powerctl.c | 1 + > target/arm/cpu.c | 2 ++ > 2 files changed, 3 insertions(+) > > 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 > > -- > 2.24.3 (Apple Git-128) >
On 03.12.20 02:52, Roman Bolshakov wrote: > On Wed, Dec 02, 2020 at 08:04:03PM +0100, Alexander Graf wrote: >> 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. >> > I'm sorry if I'm asking something dumb. But isn't > cpu_synchronize_all_post_reset() is supposed to push QEMU state into HVF > (or any other accel) after reset? > > For x86 it used to work: > > static void do_hvf_cpu_synchronize_post_reset(CPUState *cpu, > run_on_cpu_data arg) > { > hvf_put_registers(cpu); cpu->vcpu_dirty = false; > } Yes, it works because after the reset is done, there is no other register modification happening. With the PSCI emulation code in QEMU, we still do modify CPU state after reset though. Different question though: Why do we need the post_reset() call at all here to push state? We would just push it on the next run anyways, right? So if we don't set vcpu_dirty to false then, we wouldn't need this patch here I think. Alex
On Thu, Dec 03, 2020 at 11:55:17AM +0100, Alexander Graf wrote: > > On 03.12.20 02:52, Roman Bolshakov wrote: > > On Wed, Dec 02, 2020 at 08:04:03PM +0100, Alexander Graf wrote: > > > 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. > > > > > I'm sorry if I'm asking something dumb. But isn't > > cpu_synchronize_all_post_reset() is supposed to push QEMU state into HVF > > (or any other accel) after reset? > > > > For x86 it used to work: > > > > static void do_hvf_cpu_synchronize_post_reset(CPUState *cpu, > > run_on_cpu_data arg) > > { > > hvf_put_registers(cpu); > > cpu->vcpu_dirty = false; > > } > > > Yes, it works because after the reset is done, there is no other register > modification happening. With the PSCI emulation code in QEMU, we still do > modify CPU state after reset though. > Maybe I miss something but that doesn't seem correct. Why PSCI reset is split from machine reset? > Different question though: Why do we need the post_reset() call at all here > to push state? My understanding that post_reset is akin to a commit of the CPU state after all reset actions have been done to QEMU CPU Arch env state. i.e. arch/machine reset modifies env state and then the env is pushed to accel. cpu->vcpu_dirty is cleared because env is in-sync with vcpu. > We would just push it on the next run anyways, right? That's correct (at least for x86 HVF). > So if we don't set vcpu_dirty to false then, we wouldn't need this > patch here I think. > That's right but the same post-reset approach is used for all accels, including KVM. But I haven't found this for TCG. Regards, Roman
On 03.12.20 14:02, Roman Bolshakov wrote: > On Thu, Dec 03, 2020 at 11:55:17AM +0100, Alexander Graf wrote: >> On 03.12.20 02:52, Roman Bolshakov wrote: >>> On Wed, Dec 02, 2020 at 08:04:03PM +0100, Alexander Graf wrote: >>>> 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. >>>> >>> I'm sorry if I'm asking something dumb. But isn't >>> cpu_synchronize_all_post_reset() is supposed to push QEMU state into HVF >>> (or any other accel) after reset? >>> >>> For x86 it used to work: >>> >>> static void do_hvf_cpu_synchronize_post_reset(CPUState *cpu, >>> run_on_cpu_data arg) >>> { >>> hvf_put_registers(cpu); >>> cpu->vcpu_dirty = false; >>> } >> >> Yes, it works because after the reset is done, there is no other register >> modification happening. With the PSCI emulation code in QEMU, we still do >> modify CPU state after reset though. >> > Maybe I miss something but that doesn't seem correct. Why PSCI reset is > split from machine reset? Because with PSCI, you can online/offline individual CPUs, not just the full system. > >> Different question though: Why do we need the post_reset() call at all here >> to push state? > My understanding that post_reset is akin to a commit of the CPU state > after all reset actions have been done to QEMU CPU Arch env state. i.e. > arch/machine reset modifies env state and then the env is pushed to > accel. cpu->vcpu_dirty is cleared because env is in-sync with vcpu. I think that's only half the truth. What it semantically means is "QEMU's env structure is what holds the current state." Which basically translates to cpu->vcpu_dirty = true. So all of these callbacks could literally just be that, no? Alex
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
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> --- target/arm/arm-powerctl.c | 1 + target/arm/cpu.c | 2 ++ 2 files changed, 3 insertions(+)