[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 08/10] arm/hvf: Add a WFI handler
From: |
Roman Bolshakov |
Subject: |
Re: [PATCH v3 08/10] arm/hvf: Add a WFI handler |
Date: |
Fri, 4 Dec 2020 21:15:36 +0300 |
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
- [PATCH v3 05/10] hvf: arm: Mark CPU as dirty on reset, (continued)
[PATCH v3 07/10] arm: Add Hypervisor.framework build target, Alexander Graf, 2020/12/02
[PATCH v3 08/10] arm/hvf: Add a WFI handler, Alexander Graf, 2020/12/02
[PATCH v3 10/10] hvf: arm: Implement -cpu host, Alexander Graf, 2020/12/02
[PATCH v3 06/10] hvf: Add Apple Silicon support, Alexander Graf, 2020/12/02
[PATCH v3 09/10] hvf: arm: Add support for GICv3, Alexander Graf, 2020/12/02
Re: [PATCH v3 00/10] hvf: Implement Apple Silicon Support, no-reply, 2020/12/02