Re: [PATCH 1/2] KVM: x86/xen: PV oneshot timer fixes

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 3792 bytes --]

On Wed, 2022-03-09 at 17:28 +0100, Paolo Bonzini wrote:
> On 3/9/22 17:11, David Woodhouse wrote:
> > That's OK, as the pending events are in the shared_info and vcpu_info
> > regions which we have explicitly declared exempt from dirty tracking.
> > Userspace must always consider them dirty any time an interrupt (which
> > includes timers) might be delivered, so it has to migrate that memory
> > in the final sync, after serializing the vCPU state.
> > 
> > In the local APIC delivery mode, the actual interrupt is injected as an
> > MSI, so userspace needs to read the timer state before the local APIC
> > state.
> 
> Please document this.  It's not obvious from the fact that they're not 
> dirty-tracked.

The dirty tracking part is already there:

  Note that the shared info page may be constantly written to by KVM;
  it contains the event channel bitmap used to deliver interrupts to
  a Xen guest, amongst other things. It is exempt from dirty tracking
  mechanisms — KVM will not explicitly mark the page as dirty each
  time an event channel interrupt is delivered to the guest! Thus,
  userspace should always assume that the designated GFN is dirty if
  any vCPU has been running or any event channel interrupts can be
  routed to the guest.

And for vcpu_info:

KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO
  Sets the guest physical address of the vcpu_info for a given vCPU.
  As with the shared_info page for the VM, the corresponding page may be
  dirtied at any time if event channel interrupt delivery is enabled, so
  userspace should always assume that the page is dirty without relying
  on dirty logging.


For the ordering of the various vCPU state fetches... is there any
existing documentation on that? An emacs buffer just to the right of
this compose window contains the only notes on that topic that I've
ever seen (much of which may well be out of date by now)...

	/*
	 * Notes on ordering requirements (and other ramblings).
	 *
	 * KVM_GET_MP_STATE calls kvm_apic_accept_events(), which might modify
	 * vCPU/LAPIC state. As such, it must be done before most everything
	 * else, otherwise we cannot restore everything and expect it to work.
	 *
	 * KVM_GET_VCPU_EVENTS/KVM_SET_VCPU_EVENTS is unsafe if other vCPUs are
	 * still running.
	 *
	 * Some SET ioctls depend on kvm_vcpu_is_bsp(), so if we ever change
	 * the BSP, we have to do that before restoring anything. The same
	 * seems to be true for CPUID stuff.
	 *
	 * KVM_GET_LAPIC may change state of LAPIC before returning it.
	 *
	 * GET_VCPU_EVENTS should probably be last to save. The code looks as
	 * it might as well be affected by internal state modifications of the
	 * GET ioctls.
	 *
	 * SREGS saves/restores a pending interrupt, similar to what
	 * VCPU_EVENTS also does.
	 *
	 * SET_REGS clears pending exceptions unconditionally, thus, it must be
	 * done before SET_VCPU_EVENTS, which restores it.
	 *
	 * SET_LAPIC must come after SET_SREGS, because the latter restores
	 * the apic base msr.
	 *
	 * SET_LAPIC must come before SET_MSRS, because the TSC deadline MSR
	 * only restores successfully, when the LAPIC is correctly configured.
	 *
	 * GET_MSRS requires a pre-populated data structure to do something
	 * meaningful. For SET_MSRS it will then contain good data.
	 *
	 * The KVM API documentation is wrong for GET_MSRS/SET_MSRS. They
	 * return the number of successfully read/written values, which is
	 * actually helpful to determine where things went wrong.
	 */


If there's existing documentation on the ordering, I'd be happy to add
to it. If not, then my comment earlier about timers causing an
interrupt to be injected to the local APIC is far *more* obvious than
most of the rest... :)

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]