linux-kernel - Re: [PATCH v2] KVM: x86: Use fast path for Xen timer delivery

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 3 Oct 2023 09:12:50 -0700
From:   Sean Christopherson <seanjc@...gle.com>
To:     David Woodhouse <dwmw2@...radead.org>
Cc:     kvm <kvm@...r.kernel.org>, Paul Durrant <paul@....org>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
        "H. Peter Anvin" <hpa@...or.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] KVM: x86: Use fast path for Xen timer delivery

On Mon, Oct 02, 2023, David Woodhouse wrote:
> On Mon, 2023-10-02 at 11:45 -0700, Sean Christopherson wrote:
> > E.g. there's an assumption that -EWOULDBLOCK is the only non-zero return code where
> > the correct response is to go down the slow path.
> > 
> > I'm not asking to spell out every single condition, I'm just asking for clarification
> > on what the intended behavior is, e.g.
> > 
> >   Use kvm_xen_set_evtchn_fast() directly from the timer callback, and fall
> >   back to the slow path if the event is valid but fast delivery isn't
> >   possible, which currently can only happen if delivery needs to block,
> >   e.g. because the gfn=>pfn cache is invalid or stale.
> > 
> > instead of simply saying "when it's necessary to do so" and leaving it up to the
> > reader to figure what _they_ think that means, which might not always align with
> > what the author actually meant.
> 
> 
> Fair enough. There's certainly scope for something along the lines of
> 
> 
> +	rc = kvm_xen_set_evtchn_fast(&e, vcpu->kvm);
> +	if (rc != -EWOULDBLOCK) {
> 
>    /*
>     * If kvm_xen_set_evtchn_fast() returned -EWOULDBLOCK, then set the
>     * timer_pending flag and kick the vCPU, to defer delivery of the 
>     * event channel to a context which can sleep. If it fails for any
>     * other reasons, just let it fail silently. The slow path fails 
>     * silently too; a warning in that case may be guest triggerable,
>     * should never happen anyway, and guests are generally going to
>     * *notice* timers going missing.
>     */
> 
> +		vcpu->arch.xen.timer_expires = 0;
> +		return HRTIMER_NORESTART;
> +	}
> 
> That's documenting *this* code, not the function it happens to call.
> It's more verbose than I would normally have bothered to be, but I'm
> all for improving the level of commenting in our code as long as it's
> adding value. 

I'm completely ok with no comment, I just want something in the changelog.  I'm
also not opposed to a comment, but I don't think it's necessary.

I don't have a problem with digging around code to understand the subtleties, or
even the high level "what" in many cases.  What I don't like is encountering code
where *nothing* explains the author's intent.  All too often I've encountered
historical code in KVM where it's not at all obvious if code does what the author
intended, e.g. if a bug was a simple goof or a completely misguided design choice.

Holler if you plan on sending a v4 with the comment.  I'm a-ok applying v3 with a
massaged changelog to fold in the gist of the comment.