Re: [PATCH v3 08/10] arm/hvf: Add a WFI handler

[Peter Collingbourne]

On Thu, Dec 3, 2020 at 2:39 AM Roman Bolshakov <r.bolshakov@yadro.com> wrote:
>
> On Wed, Dec 02, 2020 at 08:04:06PM +0100, Alexander Graf wrote:
> > From: Peter Collingbourne <pcc@google.com>
> >
> > Sleep on WFI until the VTIMER is due but allow ourselves to be woken
> > up on IPI.
> >
> > Signed-off-by: Peter Collingbourne <pcc@google.com>
> > [agraf: Remove unused 'set' variable, always advance PC on WFX trap]
> > Signed-off-by: Alexander Graf <agraf@csgraf.de>
> > ---
> >  accel/hvf/hvf-cpus.c     |  5 ++--
> >  include/sysemu/hvf_int.h |  1 +
> >  target/arm/hvf/hvf.c     | 55 ++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 58 insertions(+), 3 deletions(-)
> >
> > diff --git a/accel/hvf/hvf-cpus.c b/accel/hvf/hvf-cpus.c
> > index e613c22ad0..a981ccde70 100644
> > --- a/accel/hvf/hvf-cpus.c
> > +++ b/accel/hvf/hvf-cpus.c
> > @@ -337,15 +337,14 @@ static int hvf_init_vcpu(CPUState *cpu)
> >      cpu->hvf = g_malloc0(sizeof(*cpu->hvf));
> >
> >      /* init cpu signals */
> > -    sigset_t set;
> >      struct sigaction sigact;
> >
> >      memset(&sigact, 0, sizeof(sigact));
> >      sigact.sa_handler = dummy_signal;
> >      sigaction(SIG_IPI, &sigact, NULL);
> >
> > -    pthread_sigmask(SIG_BLOCK, NULL, &set);
> > -    sigdelset(&set, SIG_IPI);
> > +    pthread_sigmask(SIG_BLOCK, NULL, &cpu->hvf->unblock_ipi_mask);
> > +    sigdelset(&cpu->hvf->unblock_ipi_mask, SIG_IPI);
> >
> >  #ifdef __aarch64__
> >      r = hv_vcpu_create(&cpu->hvf->fd, (hv_vcpu_exit_t **)&cpu->hvf->exit, 
> > NULL);
> > diff --git a/include/sysemu/hvf_int.h b/include/sysemu/hvf_int.h
> > index 5f15119184..13adf6ea77 100644
> > --- a/include/sysemu/hvf_int.h
> > +++ b/include/sysemu/hvf_int.h
> > @@ -62,6 +62,7 @@ extern HVFState *hvf_state;
> >  struct hvf_vcpu_state {
> >      uint64_t fd;
> >      void *exit;
> > +    sigset_t unblock_ipi_mask;
> >  };
> >
> >  void assert_hvf_ok(hv_return_t ret);
> > diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
> > index 5ecce36d4a..79aeeb237b 100644
> > --- a/target/arm/hvf/hvf.c
> > +++ b/target/arm/hvf/hvf.c
> > @@ -2,6 +2,7 @@
> >   * QEMU Hypervisor.framework support for Apple Silicon
> >
> >   * Copyright 2020 Alexander Graf <agraf@csgraf.de>
> > + * Copyright 2020 Google LLC
> >   *
> >   * This work is licensed under the terms of the GNU GPL, version 2 or 
> > later.
> >   * See the COPYING file in the top-level directory.
> > @@ -18,6 +19,7 @@
> >  #include "sysemu/hw_accel.h"
> >
> >  #include <Hypervisor/Hypervisor.h>
> > +#include <mach/mach_time.h>
> >
> >  #include "exec/address-spaces.h"
> >  #include "hw/irq.h"
> > @@ -413,6 +415,7 @@ int hvf_arch_init_vcpu(CPUState *cpu)
> >
> >  void hvf_kick_vcpu_thread(CPUState *cpu)
> >  {
> > +    cpus_kick_thread(cpu);
> >      hv_vcpus_exit(&cpu->hvf->fd, 1);
> >  }
> >
> > @@ -468,6 +471,18 @@ static int hvf_inject_interrupts(CPUState *cpu)
> >      return 0;
> >  }
> >
> > +static void hvf_wait_for_ipi(CPUState *cpu, struct timespec *ts)
> > +{
> > +    /*
> > +     * Use pselect to sleep so that other threads can IPI us while we're
> > +     * sleeping.
> > +     */
> > +    qatomic_mb_set(&cpu->thread_kicked, false);
> > +    qemu_mutex_unlock_iothread();
>
> I raised a concern earlier, but I don't for sure if a kick could be lost
> right here. On x86 it could be lost.

If the signal is sent right before the pselect() it will be blocked
i.e. left pending. With the pselect() we get an atomic unblock of
SIG_IPI at the same time as we begin sleeping, which means that we
will receive the signal and leave the pselect() immediately.

I think at some point macOS had an incorrect implementation of
pselect() where the signal mask was non-atomically set in userspace
which could lead to the signal being missed but I checked the latest
XNU sources and it looks like the pselect() implementation has been
moved to the kernel.

> > +    pselect(0, 0, 0, 0, ts, &cpu->hvf->unblock_ipi_mask);
> > +    qemu_mutex_lock_iothread();
> > +}
> > +
> >  int hvf_vcpu_exec(CPUState *cpu)
> >  {
> >      ARMCPU *arm_cpu = ARM_CPU(cpu);
> > @@ -579,6 +594,46 @@ int hvf_vcpu_exec(CPUState *cpu)
> >          }
> >          case EC_WFX_TRAP:
> >              advance_pc = true;
> > +            if (!(syndrome & WFX_IS_WFE) && !(cpu->interrupt_request &
> > +                (CPU_INTERRUPT_HARD | CPU_INTERRUPT_FIQ))) {
> > +
> > +                uint64_t ctl;
> > +                r = hv_vcpu_get_sys_reg(cpu->hvf->fd, 
> > HV_SYS_REG_CNTV_CTL_EL0,
> > +                                        &ctl);
> > +                assert_hvf_ok(r);
> > +
> > +                if (!(ctl & 1) || (ctl & 2)) {
> > +                    /* Timer disabled or masked, just wait for an IPI. */
> > +                    hvf_wait_for_ipi(cpu, NULL);
> > +                    break;
> > +                }
> > +
> > +                uint64_t cval;
> > +                r = hv_vcpu_get_sys_reg(cpu->hvf->fd, 
> > HV_SYS_REG_CNTV_CVAL_EL0,
> > +                                        &cval);
> > +                assert_hvf_ok(r);
> > +
> > +                int64_t ticks_to_sleep = cval - mach_absolute_time();
>
>
> Apple reference recommends to use [1]:
>
>   clock_gettime_nsec_np(CLOCK_UPTIME_RAW)
>
> It, internally in Libc, invokes mach_absolute_time() [2].
>
> 1. https://developer.apple.com/documentation/kernel/1462446-mach_absolute_time
> 2. 
> https://opensource.apple.com/source/Libc/Libc-1158.1.2/gen/clock_gettime.c.auto.html

I think that recommendation is because most people want to deal with
seconds, not ticks. In our case we specifically want ticks because
we're comparing against a ticks value from the guest, so I don't see
the benefit of converting from ticks to seconds and back again.

Peter

>
> Thanks,
> Roman
>
> > +                if (ticks_to_sleep < 0) {
> > +                    break;
> > +                }
> > +
> > +                uint64_t seconds = ticks_to_sleep / arm_cpu->gt_cntfrq_hz;
> > +                uint64_t nanos =
> > +                    (ticks_to_sleep - arm_cpu->gt_cntfrq_hz * seconds) *
> > +                    1000000000 / arm_cpu->gt_cntfrq_hz;
> > +
> > +                /*
> > +                 * Don't sleep for less than 2ms. This is believed to 
> > improve
> > +                 * latency of message passing workloads.
> > +                 */
> > +                if (!seconds && nanos < 2000000) {
> > +                    break;
> > +                }
> > +
> > +                struct timespec ts = { seconds, nanos };
> > +                hvf_wait_for_ipi(cpu, &ts);
> > +            }
> >              break;
> >          case EC_AA64_HVC:
> >              cpu_synchronize_state(cpu);
> > --
> > 2.24.3 (Apple Git-128)
> >