Re: [PATCH v2 8/8] KVM: arm64: Implement lazy vCPU state sync for non-protected guests

[Vincent Donnefort]

[...]

> > > diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> > > index 54aedf93c78b..8963621bcdd1 100644
> > > --- a/arch/arm64/kvm/handle_exit.c
> > > +++ b/arch/arm64/kvm/handle_exit.c
> > > @@ -422,6 +422,20 @@ static int handle_trap_exceptions(struct kvm_vcpu *vcpu)
> > >  {
> > >       int handled;
> > >
> > > +     /*
> > > +      * If we run a non-protected VM when protection is enabled
> > > +      * system-wide, resync the state from the hypervisor and mark
> > > +      * it as dirty on the host side if it wasn't dirty already
> > > +      * (which could happen if preemption has taken place).
> > > +      */
> > > +     if (is_protected_kvm_enabled() && !kvm_vm_is_protected(vcpu->kvm)) {
> > > +             guard(preempt)();
> > > +             if (!(vcpu_get_flag(vcpu, PKVM_HOST_STATE_DIRTY))) {
> > > +                     kvm_call_hyp_nvhe(__pkvm_vcpu_sync_state);
> > > +                     vcpu_set_flag(vcpu, PKVM_HOST_STATE_DIRTY);
> > > +             }
> > > +     }
> > > +
> >
> > Could we remove this update here and let handle_exit_early() do the sync
> > regardless of the SError injection? One of the main point of handle_exit_early()
> > is to do things under !prempt().
> 
> Agreed on the move: handle_exit_early() is already preempt-off, so the
> guard() goes away. Not on every exit though. handle_exit_early() runs
> on every exit, and sync_hyp_vcpu() only copies PC/PSTATE/fault back
> for a non-protected guest; the GPRs and sysregs cross solely via
> __pkvm_vcpu_sync_state. Syncing unconditionally would pull the full
> context back on plain IRQ exits, which is the copy this patch avoids.
> So I will gate it on trap-or-SError and drop the
> handle_trap_exceptions() block.
> 
> >
> >
> > >       /*
> > >        * See ARM ARM B1.14.1: "Hyp traps on instructions
> > >        * that fail their condition code check"
> > > @@ -489,6 +503,22 @@ int handle_exit(struct kvm_vcpu *vcpu, int exception_index)
> > >  /* For exit types that need handling before we can be preempted */
> > >  void handle_exit_early(struct kvm_vcpu *vcpu, int exception_index)
> > >  {
> > > +     bool inject_serror = ARM_SERROR_PENDING(exception_index) ||
> > > +             ARM_EXCEPTION_CODE(exception_index) == ARM_EXCEPTION_EL1_SERROR;
> > > +
> > > +     /*
> > > +      * An SError injected below writes the host ctxt; for a non-protected
> > > +      * guest, sync from the hyp vCPU and keep it dirty so it isn't dropped.
> > > +      */
> > > +     if (is_protected_kvm_enabled()) {
> >
> > Should we test !kvm_vm_is_protected(vcpu->kvm) here, as the
> > PKVM_HOST_STATE_DIRTY is only updated for p-guests everywhere else?
> 
> Yes. The flag is only ever set for non-protected guests, so clearing it
> for a protected one is a no-op, but gating it matches the invariant.
> 
> Both fold into one block in handle_exit_early():
> 
>       if (is_protected_kvm_enabled() && !kvm_vm_is_protected(vcpu->kvm)) {
>               if (inject_serror ||
>                   ARM_EXCEPTION_CODE(exception_index) == ARM_EXCEPTION_TRAP) {
>                       kvm_call_hyp_nvhe(__pkvm_vcpu_sync_state);
>                       vcpu_set_flag(vcpu, PKVM_HOST_STATE_DIRTY);
>               } else {
>                       vcpu_clear_flag(vcpu, PKVM_HOST_STATE_DIRTY);
>               }
>       }
> 
> I will fold this into the next respin.

Ah yes of course, I was hoping we could just have a switch here, just like
handle_exit() does, but that's not possible because of ARM_SERROR_PENDING().

Perhaps it would look cleaner if done in a separate function
handle_exit_pkvm_state()?


> 
> Thanks for the reviews!
> /fuad
> 
> >
> > > +             vcpu_clear_flag(vcpu, PKVM_HOST_STATE_DIRTY);
> > > +
> > > +             if (inject_serror && !kvm_vm_is_protected(vcpu->kvm)) {
> > > +                     kvm_call_hyp_nvhe(__pkvm_vcpu_sync_state);
> > > +                     vcpu_set_flag(vcpu, PKVM_HOST_STATE_DIRTY);
> > > +             }
> > > +     }
> > > +
> > >       if (ARM_SERROR_PENDING(exception_index)) {
> > >               if (this_cpu_has_cap(ARM64_HAS_RAS_EXTN)) {
> > >                       u64 disr = kvm_vcpu_get_disr(vcpu);
> >
> > [...]