On Fri, Feb 2, 2024 at 11:55 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> > > > + struct kvm_sev_snp_launch_update {
> > > > + __u64 start_gfn; /* Guest page number to start from. */
> > > > + __u64 uaddr; /* userspace address need to be encrypted */
> > >
> > > Huh? Why is KVM taking a userspace address? IIUC, the address unconditionally
> > > gets translated into a gfn, so why not pass a gfn?
> > >
> > > And speaking of gfns, AFAICT start_gfn is never used.
> >
> > I think having both the uaddr and start_gfn parameters makes sense. I
> > think it's only awkward because how I'm using the memslot to translate
> > the uaddr to a GFN in the current implementation,
>
> Yes.
>
> > > Oof, reading more of the code, this *requires* an effective in-place copy-and-convert
> > > of guest memory.
> >
> > So that's how it's done here, KVM_SNP_LAUNCH_UPDATE copies the pages into
> > gmem, then passes those pages on to firmware for encryption. Then the
> > VMM is expected to mark the GFN range as private via
> > KVM_SET_MEMORY_ATTRIBUTES, since the VMM understands what constitutes
> > initial private/encrypted payload. I should document that better in
> > KVM_SNP_LAUNCH_UPDATE docs however.
>
> That's fine. As above, my complaints are that the unencrypted source *must* be
> covered by a memslot. That's beyond ugly.
Yes, if there's one field that has to go it's uaddr, and then you'll
have a non-in-place encrypt (any copy performed by KVM it is hidden).
> > > > + kvaddr = pfn_to_kaddr(pfns[i]);
> > > > + if (!virt_addr_valid(kvaddr)) {
> > >
> > > I really, really don't like that this assume guest_memfd is backed by struct page.
> >
> > There are similar enforcements in the SEV/SEV-ES code. There was some
> > initial discussion about relaxing this for SNP so we could support
> > things like /dev/mem-mapped guest memory, but then guest_memfd came
> > along and made that to be an unlikely use-case in the near-term given
> > that it relies on alloc_pages() currently and explicitly guards against
> > mmap()'ing pages in userspace.
> >
> > I think it makes to keep the current tightened restrictions in-place
> > until such a use-case comes along, since otherwise we are relaxing a
> > bunch of currently-useful sanity checks that span all throughout the code
What sanity is being checked for, in other words why are they useful?
If all you get for breaking the promise is a KVM_BUG_ON, for example,
that's par for the course. If instead you get an oops, then we have a
problem.
I may be a bit less draconian than Sean, but the assumptions need to
be documented and explained because they _are_ going to go away.
> > > (b) Why are KVM's memory attributes never consulted?
> >
> > It doesn't really matter if the attributes are set before or after
> > KVM_SNP_LAUNCH_UPDATE, only that by the time the guest actually launches
> > they pages get set to private so they get faulted in from gmem. We could
> > document our expectations and enforce them here if that's preferable
> > however. Maybe requiring KVM_SET_MEMORY_ATTRIBUTES(private) in advance
> > would make it easier to enforce that userspace does the right thing.
> > I'll see how that looks if there are no objections.
>
> Userspace owns whether a page is PRIVATE or SHARED, full stop. If KVM can't
> honor that, then we need to come up with better uAPI.
Can you explain more verbosely what you mean?
> > > > + * When invalid CPUID function entries are detected, the firmware
> > > > + * corrects these entries for debugging purpose and leaves the
> > > > + * page unencrypted so it can be provided users for debugging
> > > > + * and error-reporting.
> > >
> > > Why? IIUC, this is basically backdooring reads/writes into guest_memfd to avoid
> > > having to add proper mmap() support.
>
> Yes, I am specifically complaining about writing guest memory on failure, which is
> all kinds of weird.
It is weird but I am not sure if you are complaining about firmware
behavior or something else.
Paolo
