On Fri, Jan 13, 2023 at 03:48:59PM +0000, Sean Christopherson wrote:

On Fri, Jan 13, 2023, Borislav Petkov wrote:

quoted

On Wed, Jan 04, 2023 at 08:42:56PM -0600, Michael Roth wrote:

quoted

Obviously I need to add some proper documentation for this, but a 1
return basically means 'private_fault' pass-by-ref arg has been set
with the appropriate value, whereas 0 means "there's no platform-specific
handling for this, so if you have some generic way to determine this
then use that instead".

Still binary, tho, and can be bool, right?

I.e., you can just as well do:

        if (static_call(kvm_x86_fault_is_private)(kvm, gpa, err, &private_fault))
                goto out;

at the call site.

Ya.  Don't spend too much time trying to make this look super pretty though, there
are subtle bugs inherited from the base UPM series that need to be sorted out and
will impact this code.  E.g. invoking kvm_mem_is_private() outside of the protection
of mmu_invalidate_seq means changes to the attributes may not be reflected in the
page tables.

I'm also hoping we can avoid a callback entirely, though that may prove to be
more pain than gain.  I'm poking at the UPM and testing series right now, will
circle back to this and TDX in a few weeks to see if there's a sane way to communicate
shared vs. private without having to resort to a callback, and without having
races between page faults, KVM_SET_MEMORY_ATTRIBUTES, and KVM_SET_USER_MEMORY_REGION2.

Can circle back on this, but for v8 at least I've kept the callback, but
simplified SVM implementation of it so that it's only needed for SNP. For
protected-SEV it will fall through to the same generic handling used by UPM
self-tests.

It seems like it's safe to have a callback of that sort here for TDX/SNP (or
whatever we end up replacing the callback with), since the #NPF flags
themselves won't change based on attribute updates, and the subsequent
comparison to kvm_mem_is_private() will happen after mmu_invalidate_seq
is logged.

But for protected-SEV and UPM selftests the initial kvm_mem_is_private()
can become stale vs. the one in __kvm_faultin_pfn(), but it seems like ATM
it would only lead to a spurious KVM_EXIT_MEMORY_FAULT, which SEV at least
should treat at an implicit page-state change and be able to recover from.
But yah, not ideal, and maybe for self-tests that makes it difficult to tell
if things are working as expected or not.

Maybe we should just skip setting fault->is_private here in the
non-TDX/non-SNP cases, and just have some other indicator so it's
initialized/ignored in kvm_mem_is_private() later. I think some iterations
of UPM did it this way prior to 'is_private' becoming const.

quoted

quoted

This is mainly to handle CONFIG_HAVE_KVM_PRIVATE_MEM_TESTING, which
just parrots whatever kvm_mem_is_private() returns to support running
KVM selftests without needed hardware/platform support. If we don't
take care to skip this check where the above fault_is_private() hook
returns 1, then it ends up breaking SNP in cases where the kernel has
been compiled with CONFIG_HAVE_KVM_PRIVATE_MEM_TESTING, since SNP
relies on the page fault flags to make this determination, not
kvm_mem_is_private(), which normally only tracks the memory attributes
set by userspace via KVM_SET_MEMORY_ATTRIBUTES ioctl.

Some of that explanation belongs into the commit message, which is a bit
lacking...

I'll circle back to this too when I give this series (and TDX) a proper look,
there's got too be a better way to handle this.

It seems like for SNP/TDX we just need to register the shared/encrypted
bits with KVM MMU and let it handle checking the #NPF flags, but can
iterate on that for the next spin when we have a better idea what it
should look like.

-Mike