Re: [RFC PATCH 00/26] Runtime paravirt patching

[Ankur Arora]

On 2020-04-08 5:08 a.m., Peter Zijlstra wrote:
> On Tue, Apr 07, 2020 at 10:02:57PM -0700, Ankur Arora wrote:
> > A KVM host (or another hypervisor) might advertise paravirtualized
> > features and optimization hints (ex KVM_HINTS_REALTIME) which might
> > become stale over the lifetime of the guest. For instance, the
> > host might go from being undersubscribed to being oversubscribed
> > (or the other way round) and it would make sense for the guest
> > switch pv-ops based on that.
>
> So what, the paravirt spinlock stuff works just fine when you're not
> oversubscribed.
>
> > We keep an interesting subset of pv-ops (pv_lock_ops only for now,
> > but PV-TLB ops are also good candidates)
>
> The PV-TLB ops also work just fine when not oversubscribed. IIRC
> kvm_flush_tlb_others() is pretty much the same in that case.
>
> > in .parainstructions.runtime,
> > while discarding the .parainstructions as usual at init. This is then
> > used for switching back and forth between native and paravirt mode.
> > ([1] lists some representative numbers of the increased memory
> > footprint.)
> >
> > Mechanism: the patching itself is done using stop_machine(). That is
> > not ideal -- text_poke_stop_machine() was replaced with INT3+emulation
> > via text_poke_bp(), but I'm using this to address two issues:
> >   1) emulation in text_poke() can only easily handle a small set
> >   of instructions and this is problematic for inlined pv-ops (and see
> >   a possible alternatives use-case below.)
> >   2) paravirt patching might have inter-dependendent ops (ex.
> >   lock.queued_lock_slowpath, lock.queued_lock_unlock are paired and
> >   need to be updated atomically.)
>
> And then you hope that the spinlock state transfers.. That is that both
> implementations agree what an unlocked spinlock looks like.
>
> Suppose the native one was a ticket spinlock, where unlocked means 'head
> == tail' while the paravirt one is a test-and-set spinlock, where
> unlocked means 'val == 0'.
>
> That just happens to not be the case now, but it was for a fair while.
>
> > The alternative use-case is a runtime version of apply_alternatives()
> > (not posted with this patch-set) that can be used for some safe subset
> > of X86_FEATUREs. This could be useful in conjunction with the ongoing
> > late microcode loading work that Mihai Carabas and others have been
> > working on.
>
> The whole late-microcode loading stuff is crazy already; you're making
> it take double bonghits.
That's fair. I was talking in a fairly limited sense, ex making static_cpu_has()
catch up with boot_cpu_has() after a microcode update but I should have
specified that.

> > Also, there are points of similarity with the ongoing static_call work
> > which does rewriting of indirect calls.
>
> Only in so far as that code patching is involved. An analogy would be
> comparing having a beer with shooting dope. They're both 'drugs'.
I meant closer to updating indirect pointers, like static_call_update()
semantics. But of course I don't know static_call code well enough.

> > The difference here is that
> > we need to switch a group of calls atomically and given that
> > some of them can be inlined, need to handle a wider variety of opcodes.
> >
> > To patch safely we need to satisfy these constraints:
> >
> >   - No references to insn sequences under replacement on any kernel stack
> >     once replacement is in progress. Without this constraint we might end
> >     up returning to an address that is in the middle of an instruction.
>
> Both ftrace and optprobes have that issue, neither of them are quite as
> crazy as this.
I did look at ftrace. Will look at optprobes. Thanks.

> >   - handle inter-dependent ops: as above, lock.queued_lock_unlock(),
> >     lock.queued_lock_slowpath() and the rest of the pv_lock_ops are
> >     a good example.
>
> While I'm sure this is a fun problem, why are we solving it?
>
> >   - handle a broader set of insns than CALL and JMP: some pv-ops end up
> >     getting inlined. Alternatives can contain arbitrary instructions.
>
> So can optprobes.> 
> >   - locking operations can be called from interrupt handlers which means
> >     we cannot trivially use IPIs for flushing.
>
> Heck, some NMI handlers use locks..
This does handle the NMI locking problem. The solution -- doing it
in the NMI handler was of course pretty ugly.

> > Handling these, necessitates that target pv-ops not be preemptible.
>
> I don't think that is a correct inferrence.The non-preemptibility requirement was to ensure that any pv-op under
replacement not be under execution after it is patched out.
(Not a concern for pv_lock_ops.)

Ensuring that we don't return to an address in the middle of an instruction
could be done by moving the NOPs in the prefix, but I couldn't think of
any other way to ensure that a function not be under execution.

Thanks
Ankur

> > Once that is a given (for safety these need to be explicitly whitelisted
> > in runtime_patch()), use a state-machine with the primary CPU doing the
> > patching and secondary CPUs in a sync_core() loop.
> >
> > In case we hit an INT3/BP (in NMI or thread-context) we makes forward
> > progress by continuing the patching instead of emulating.
> >
> > One remaining issue is inter-dependent pv-ops which are also executed in
> > the NMI handler -- patching can potentially deadlock in case of multiple
> > NMIs. Handle these by pushing some of this work in the NMI handler where
> > we know it will be uninterrupted.
>
> I'm just seeing a lot of bonghits without sane rationale. Why is any of
> this important?