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

[Paolo Bonzini <pbonzini@redhat.com>]

On 3/9/22 17:37, David Woodhouse wrote:
> 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)...

Ok, guilty as charged.

Paolo

> 	/*
> 	 * 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...:)