MIME-Version: 1.0
References: <20211209155257.128747-1-marcorr@google.com> <5f8c31b4-6223-a965-0e91-15b4ffc0335e@redhat.com>
 <CALMp9eThf3UtvoLFjajkrXtvOEWQvc8_=Xf6-m6fHXkOhET+GA@mail.gmail.com> <YbJJXKFevTV/L3in@google.com>
In-Reply-To: <YbJJXKFevTV/L3in@google.com>
From:   Marc Orr <marcorr@google.com>
Date:   Thu, 9 Dec 2021 10:29:36 -0800
Message-ID: <CAA03e5GE-UGB6YvZfWfWEzpC7+M+EZU3hJsTEOzN0i5UyD5Vpw@mail.gmail.com>
Subject: Re: [PATCH v2] KVM: x86: Always set kvm_run->if_flag
To:     Sean Christopherson <seanjc@google.com>
Cc:     Jim Mattson <jmattson@google.com>,
        Paolo Bonzini <pbonzini@redhat.com>, vkuznets@redhat.com,
        wanpengli@tencent.com, joro@8bytes.org, tglx@linutronix.de,
        mingo@redhat.com, bp@alien8.de, dave.hansen@linux.intel.com,
        x86@kernel.org, hpa@zytor.com, Thomas.Lendacky@amd.com,
        mlevitsk@redhat.com, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Dec 9, 2021 at 10:22 AM Sean Christopherson <seanjc@google.com> wrote:
>
> On Thu, Dec 09, 2021, Jim Mattson wrote:
> > On Thu, Dec 9, 2021 at 9:48 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
> > >
> > > On 12/9/21 16:52, Marc Orr wrote:
> > > > The kvm_run struct's if_flag is a part of the userspace/kernel API. The
> > > > SEV-ES patches failed to set this flag because it's no longer needed by
> > > > QEMU (according to the comment in the source code). However, other
> > > > hypervisors may make use of this flag. Therefore, set the flag for
> > > > guests with encrypted registers (i.e., with guest_state_protected set).
> > > >
> > > > Fixes: f1c6366e3043 ("KVM: SVM: Add required changes to support intercepts under SEV-ES")
> > > > Signed-off-by: Marc Orr<marcorr@google.com>
> > >
> > > Applied, though I wonder if it is really needed by those other VMMs
> > > (which? gVisor is the only one that comes to mind that is interested in
> > > userspace APIC).
> >
> > Vanadium appears to have one use of it.
> >
> > > It shouldn't be necessary for in-kernel APIC (where userspace can inject
> > > interrupts at any time), and ready_for_interrupt_injection is superior
> > > for userspace APIC.
> >
> > LOL. Here's that one use...
> >
> > if (vcpu_run_state_->request_interrupt_window &&
> > vcpu_run_state_->ready_for_interrupt_injection &&
> > vcpu_run_state_->if_flag) {
> > ...
> > }
> >
> > So, maybe this is much ado about nothing?
>
> I assume the issue is that SEV-ES always squishes if_flag, so that above statement
> can never evaluate true.

Correct. And the Vanadium code snippet above is what motivated this
patch. Technically Vanadium uses the if_flag in one other place, but I
also think that place can be replaced with the
`ready_for_interrupt_injection` flag. In fact, I had an internal patch
prepped to do this because my reading of the KVM code was identical to
Paolo's that the ready_for_interrupt_injection is superior to the
if_flag. But then after some internal discussion, we felt that the
userspace/kernel API shouldn't have been changed.

All that being said, after Jim added his Ack to this patch (which I
forgot to attach to the v2), we realized that technically the ES
patches were within their right to redefine if_flag since it's
previous semantics are maintained for non-ES VMs and ES requires
userspace changes anyway (PSP commands, guest memory pinning, etc.).

I'm OK either way here. But I assume that if this flag is giving us
pains it will give others pains. And this patch seems reasonable to
me. So all things being equal, I'd prefer to proceed with it.