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

[Roman Bolshakov <r.bolshakov@yadro.com>]

On Thu, Dec 03, 2020 at 10:18:14AM -0800, Peter Collingbourne wrote:
> 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>
> > > ---
> > > +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.
> 

Yeah, you're right here.

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

Thanks for the clarifications, Peter.

Regards,
Roman