Message-ID: <20220613174928.f4yyvu45rliuuld6@amd.com>
Date:   Mon, 13 Jun 2022 12:49:28 -0500
From:   Michael Roth <michael.roth@....com>
To:     Vishal Annapurve <vannapurve@...gle.com>
CC:     x86 <x86@...nel.org>, kvm list <kvm@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        <linux-kselftest@...r.kernel.org>,
        "Paolo Bonzini" <pbonzini@...hat.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        "Joerg Roedel" <joro@...tes.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        <dave.hansen@...ux.intel.com>, "H . Peter Anvin" <hpa@...or.com>,
        shuah <shuah@...nel.org>, <yang.zhong@...el.com>,
        <drjones@...hat.com>, "Ricardo Koller" <ricarkol@...gle.com>,
        Aaron Lewis <aaronlewis@...gle.com>, <wei.w.wang@...el.com>,
        "Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>,
        Jonathan Corbet <corbet@....net>,
        "Hugh Dickins" <hughd@...gle.com>,
        Jeff Layton <jlayton@...nel.org>,
        "J . Bruce Fields" <bfields@...ldses.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Chao Peng <chao.p.peng@...ux.intel.com>,
        Yu Zhang <yu.c.zhang@...ux.intel.com>,
        Jun Nakajima <jun.nakajima@...el.com>,
        "Dave Hansen" <dave.hansen@...el.com>,
        Quentin Perret <qperret@...gle.com>,
        "Steven Price" <steven.price@....com>,
        Andi Kleen <ak@...ux.intel.com>,
        "David Hildenbrand" <david@...hat.com>,
        Andy Lutomirski <luto@...nel.org>,
        "Vlastimil Babka" <vbabka@...e.cz>, Marc Orr <marcorr@...gle.com>,
        Erdem Aktas <erdemaktas@...gle.com>,
        Peter Gonda <pgonda@...gle.com>,
        "Nikunj A. Dadhania" <nikunj@....com>,
        Sean Christopherson <seanjc@...gle.com>,
        "Austin Diviness" <diviness@...gle.com>, <maz@...nel.org>,
        <dmatlack@...gle.com>, <axelrasmussen@...gle.com>,
        <maciej.szmigiero@...cle.com>, Mingwei Zhang <mizhang@...gle.com>,
        <bgardon@...gle.com>
Subject: Re: [RFC V1 PATCH 0/3] selftests: KVM: sev: selftests for fd-based
 approach of supporting private memory

On Fri, Jun 10, 2022 at 02:01:41PM -0700, Vishal Annapurve wrote:
> ....
> >
> > I ended up adding a KVM_CAP_UNMAPPED_PRIVATE_MEM to distinguish between the
> > 2 modes. With UPM-mode enabled it basically means KVM can/should enforce that
> > all private guest pages are backed by private memslots, and enable a couple
> > platform-specific hooks to handle MAP_GPA_RANGE, and queries from MMU on
> > whether or not an NPT fault is for a private page or not. SEV uses these hooks
> > to manage its encryption bitmap, and uses that bitmap as the authority on
> > whether or not a page is encrypted. SNP uses GHCB page-state-change requests
> > so MAP_GPA_RANGE is a no-op there, but uses the MMU hook to indicate whether a
> > fault is private based on the page fault flags.
> >
> > When UPM-mode isn't enabled, MAP_GPA_RANGE just gets passed on to userspace
> > as before, and platform-specific hooks above are no-ops. That's the mode
> > your SEV self-tests ran in initially. I added a test that runs the
> > PrivateMemoryPrivateAccess in UPM-mode, where the guest's OS memory is also
> > backed by private memslot and the platform hooks are enabled, and things seem
> > to still work okay there. I only added a UPM-mode test for the
> > PrivateMemoryPrivateAccess one though so far. I suppose we'd want to make
> > sure it works exactly as it did with UPM-mode disabled, but I don't see why
> > it wouldn't.
> 
> Thanks Michael for the update. Yeah, using the bitmap to track
> private/shared-ness of gfn ranges should be the better way to go as
> compared to the limited approach I used to just track a single
> contiguous pfn range.
> I spent some time in getting the SEV/SEV-ES priv memfd selftests to
> execute from private fd as well and ended up doing similar changes as
> part of the github tree:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fvishals4gh%2Flinux%2Fcommits%2Fsev_upm_selftests_rfc_v2&amp;data=05%7C01%7Cmichael.roth%40amd.com%7Cf040f8a9f98146f8008508da4b2472c5%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637904917162115269%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=%2Bb3S2xOAWga8k5tsS2EHMQF5CXuKG60qy0ToeEhhQ4A%3D&amp;reserved=0.
> 
> >
> > But probably worth having some discussion on how exactly we should define this
> > mode, and whether that meshes with what TDX folks are planning.
> >
> > I've pushed my UPM-mode selftest additions here:
> >   https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fmdroth%2Flinux%2Fcommits%2Fsev_upm_selftests_rfc_v1_upmmode&amp;data=05%7C01%7Cmichael.roth%40amd.com%7Cf040f8a9f98146f8008508da4b2472c5%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637904917162115269%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=3YLZcCevIkuo5cw%2FpKk5Sf9y6%2F1ZPss6ujZtLYEbV3M%3D&amp;reserved=0
> >
> > And the UPM SEV/SEV-SNP tree I'm running them against (DISCLAIMER: EXPERIMENTAL):
> >   https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fmdroth%2Flinux%2Fcommits%2Fpfdv6-on-snpv6-upm1&amp;data=05%7C01%7Cmichael.roth%40amd.com%7Cf040f8a9f98146f8008508da4b2472c5%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637904917162115269%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=mW8ypNWyREtoDJ%2BHNi20OT8Hzelqk5Na8eC8ihkfCjY%3D&amp;reserved=0
> >
> 
> Thanks for the references here. This helps get a clear picture around
> the status of priv memfd integration with Sev-SNP VMs and this work
> will be the base of future SEV specific priv memfd selftest patches as
> things get more stable.
> 
> I see usage of pwrite to populate initial private memory contents.
> Does it make sense to have SEV_VM_LAUNCH_UPDATE_DATA handle the
> private fd population as well?
> I tried to prototype it via:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fvishals4gh%2Flinux%2Fcommit%2Fc85ee15c8bf9d5d43be9a34898176e8230a3b680%23&amp;data=05%7C01%7Cmichael.roth%40amd.com%7Cf040f8a9f98146f8008508da4b2472c5%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637904917162115269%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=QwP4JioniC06yFV7c%2BY35LtqJy9INGlcQ9Z6gn3nOrI%3D&amp;reserved=0

Thanks for the pointer and for taking a stab at this approach (hadn't
realized you were looking into this so sorry for the overlap with your
code).

> as I got this suggestion from Erdem Aktas(erdemaktas@...gle) while
> discussing about executing guest code from private fd.

The way we way have the host patches implemented currently is sort of based
around the idea that userspace handles all private/shared conversion via
allocations/deallocations from the private backing store, since I
thought that was one of the design goals. For SNP that means allocating a
page from backing store will trigger the additional hooks in the kernel needed
to do some additional bookkeeping like RMP updates and removing from directmap,
which I'm doing via a platform-specific callback I've added to the KVM memfile
notifier callback.

There was some talk of allowing a sort of pre-boot stage to the
MFD_INACCESSIBLE protections where writes would be allowed up until a
certain point. The kernel hack to allow pwrite() was sort of a holdover
for this support.

Handling pre-population as part of SNP_LAUNCH_UPDATE seems sort of
incompatible with this, since it reads from shared memory and writes
into private memory. So either:

a) userspace pre-allocated the private backing page before calling
   SNP_LAUNCH_UPDATE to fill it, in which case the kernel is mapping
   private memory into it's address space that is now already
   guest-owned which will cause an RMP fault, or

b) userspace lets SNP_LAUNCH_UPDATE allocate the private page as part of
   copying over the data from shared page, in which case we'd get the
   invalidation notifier callback going through the normal shmem
   allocation path, and would need to bypass this to ensure that
   notifier trigger after the memory has been populated.

Maybe some other sort of notifier to handle the RMP/directmap changes
would avoid these issues, but that seems to move us closer to just
having a KVM ioctl to handle the conversions and manage this
book-keeping via KVM ioctls rather than mem FD callbacks/notifiers.
There seems to be some discussion around doing something of this sort
but still need to get some clarity on this.

> Apart from the aspects I might not be aware of, this can have
> performance overhead depending on the initial Guest UEFI boot memory
> requirements. But this can allow the userspace VMM to keep most of the
> guest vm boot memory setup the same and
> avoid changing the host kernel to allow private memfd writes from userspace.

I think it would be good if we could reduce the complexity on the VMM
side. Having a KVM ioctl that instructs KVM to convert a range of GPAs
between private<->shared via private memslot/FD would also achieve that,
while also avoiding needing special handling for this pre-launch case
vs. conversions during run-time. Probably worth discussing more in
Chao's thread where there's some related discussion.

Thanks,

Mike

> 
> Regards,
> Vishal

Powered by blists - more mailing lists