lkml.org 
[lkml]   [2022]   [Jan]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH 2/5] kvm/arm64: rework guest entry logic
    On Thu, Jan 13, 2022 at 11:43:30AM +0000, Marc Zyngier wrote:
    > On Thu, 13 Jan 2022 11:17:53 +0000,
    > Mark Rutland <mark.rutland@arm.com> wrote:
    > >
    > > On Tue, Jan 11, 2022 at 05:55:20PM +0000, Marc Zyngier wrote:
    > > > On Tue, 11 Jan 2022 15:35:36 +0000,
    > > > Mark Rutland <mark.rutland@arm.com> wrote:
    >
    > [...]
    >
    > > > > @@ -891,26 +909,23 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
    > > > > kvm_arch_vcpu_ctxsync_fp(vcpu);
    > > > >
    > > > > /*
    > > > > - * We may have taken a host interrupt in HYP mode (ie
    > > > > - * while executing the guest). This interrupt is still
    > > > > - * pending, as we haven't serviced it yet!
    > > > > + * We must ensure that any pending interrupts are taken before
    > > > > + * we exit guest timing so that timer ticks are accounted as
    > > > > + * guest time. Transiently unmask interrupts so that any
    > > > > + * pending interrupts are taken.
    > > > > *
    > > > > - * We're now back in SVC mode, with interrupts
    > > > > - * disabled. Enabling the interrupts now will have
    > > > > - * the effect of taking the interrupt again, in SVC
    > > > > - * mode this time.
    > > > > + * Per ARM DDI 0487G.b section D1.13.4, an ISB (or other
    > > > > + * context synchronization event) is necessary to ensure that
    > > > > + * pending interrupts are taken.
    > > > > */
    > > > > local_irq_enable();
    > > > > + isb();
    > > > > + local_irq_disable();
    > > >
    > > > Small nit: we may be able to elide this enable/isb/disable dance if a
    > > > read of ISR_EL1 returns 0.
    > >
    > > Wouldn't that be broken when using GIC priority masking, since that
    > > can prevent IRQS being signalled ot the PE?
    >
    > You're right. But this can be made even simpler. We already know if
    > we've exited the guest because of an IRQ (ret tells us that), and
    > that's true whether we're using priority masking or not. It could be
    > as simple as:
    >
    > if (ARM_EXCEPTION_CODE(ret) == ARM_EXCEPTION_IRQ) {
    > // We exited because of an interrupt. Let's take
    > // it now to account timer ticks to the guest.
    > local_irq_enable();
    > isb();
    > local_irq_disable();
    > }
    >
    > and that would avoid accounting the interrupt to the guest if it fired
    > after the exit took place.
    >
    > > I'm happy to rework this, but I'll need to think a bit harder about
    > > it. Would you be happy if we did that as a follow-up?
    >
    > Oh, absolutely. I want the flow to be correct before we make it
    > fast(-ish).

    Cool; I'll leave that for now on the assumption we'll address that with a
    follow-up patch, though your suggestion above looks "obviously" correct to me.

    Thanks,
    Mark.

    \
     
     \ /
      Last update: 2022-01-13 13:59    [W:6.335 / U:0.040 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site