Message-ID: <diqzr08exqu6.fsf@ackerleytng-ctop-specialist.c.googlers.com>
Date: Thu, 17 Oct 2024 23:16:49 +0000
From: Ackerley Tng <ackerleytng@...gle.com>
To: Patrick Roy <roypat@...zon.co.uk>
Cc: seanjc@...gle.com, quic_eberman@...cinc.com, tabba@...gle.com, 
	jgg@...dia.com, peterx@...hat.com, david@...hat.com, rientjes@...gle.com, 
	fvdl@...gle.com, jthoughton@...gle.com, pbonzini@...hat.com, 
	zhiquan1.li@...el.com, fan.du@...el.com, jun.miao@...el.com, 
	isaku.yamahata@...el.com, muchun.song@...ux.dev, mike.kravetz@...cle.com, 
	erdemaktas@...gle.com, vannapurve@...gle.com, qperret@...gle.com, 
	jhubbard@...dia.com, willy@...radead.org, shuah@...nel.org, 
	brauner@...nel.org, bfoster@...hat.com, kent.overstreet@...ux.dev, 
	pvorel@...e.cz, rppt@...nel.org, richard.weiyang@...il.com, 
	anup@...infault.org, haibo1.xu@...el.com, ajones@...tanamicro.com, 
	vkuznets@...hat.com, maciej.wieczor-retman@...el.com, pgonda@...gle.com, 
	oliver.upton@...ux.dev, linux-kernel@...r.kernel.org, linux-mm@...ck.org, 
	kvm@...r.kernel.org, linux-kselftest@...r.kernel.org, linux-fsdevel@...ck.org, 
	jgowans@...zon.com, kalyazin@...zon.co.uk, derekmn@...zon.com
Subject: Re: [RFC PATCH 30/39] KVM: guest_memfd: Handle folio preparation for
 guest_memfd mmap

Patrick Roy <roypat@...zon.co.uk> writes:

> On Tue, 2024-10-08 at 20:56 +0100, Sean Christopherson wrote:
>> On Tue, Oct 08, 2024, Ackerley Tng wrote:
>>> Patrick Roy <roypat@...zon.co.uk> writes:
>>>> For the "non-CoCo with direct map entries removed" VMs that we at AWS
>>>> are going for, we'd like a VM type with host-controlled in-place
>>>> conversions which doesn't zero on transitions,
>> 
>> Hmm, your use case shouldn't need conversions _for KVM_, as there's no need for
>> KVM to care if userspace or the guest _wants_ a page to be shared vs. private.
>> Userspace is fully trusted to manage things; KVM simply reacts to the current
>> state of things.
>> 
>> And more importantly, whether or not the direct map is zapped needs to be a
>> property of the guest_memfd inode, i.e. can't be associated with a struct kvm.
>> I forget who got volunteered to do the work,
>
> I think me? At least we talked about it briefly
>
>> but we're going to need similar
>> functionality for tracking the state of individual pages in a huge folio, as
>> folio_mark_uptodate() is too coarse-grained.  I.e. at some point, I expect that
>> guest_memfd will make it easy-ish to determine whether or not the direct map has
>> been obliterated.
>> 
>> The shared vs. private attributes tracking in KVM is still needed (I think), as
>> it communicates what userspace _wants_, whereas he guest_memfd machinery will
>> track what the state _is_.
>
> If I'm understanding this patch series correctly, the approach taken
> here is to force the KVM memory attributes and the internal guest_memfd
> state to be in-sync, because the VMA from mmap()ing guest_memfd is
> reflected back into the userspace_addr of the memslot.

In this patch series, we're also using guest_memfd state (faultability
xarray) to prevent any future faults before checking that there are no
mappings. Further explanation at [1].

Reason (a) at [1] is what Sean describes above to be what userspace
_wants_ vs what the state _is_.

> So, to me, in
> this world, "direct map zapped iff
> kvm_has_mem_attributes(KVM_MEMORY_ATTRIBUTES_PRIVATE)", with memory
> attribute changes forcing the corresponding gmem state change. That's
> why I was talking about conversions above.

I think if we do continue to have state in guest_memfd, then direct map
removal should be based on guest_memfd's state, rather than
KVM_MEMORY_ATTRIBUTE_PRIVATE in mem_attr_array.

> I've played around with this locally, and since KVM seems to generally
> use copy_from_user and friends to access the userspace_addr VMA, (aka
> private mem that's reflected back into memslots here), with this things
> like MMIO emulation can be oblivious to gmem's existence, since
> copy_from_user and co don't require GUP or presence of direct map
> entries (well, "oblivious" in the sense that things like kvm_read_guest
> currently ignore memory attributes and unconditionally access
> userspace_addr, which I suppose is not really wanted for VMs where
> userspace_addr and guest_memfd aren't short-circuited like this). The
> exception is kvm_clock, where the pv_time page would need to be
> explicitly converted to shared to restore the direct map entry, although
> I think we could just let userspace deal with making sure this page is
> shared (and then, if gmem supports GUP on shared memory, even the
> gfn_to_pfn_caches could work without gmem knowledge. Without GUP, we'd
> still need a tiny hack in the uhva->pfn translation somewhere to handle
> gmem vmas, but iirc you did mention that having kvm-clock be special
> might be fine).
>
> I guess it does come down to what you note below, answering the question
> of "how does KVM internally access guest_memfd for non-CoCo VMs".  Is
> there any way we can make uaccesses like above work? I've finally gotten
> around to re-running some performance benchmarks of my on-demand
> reinsertion patches with all the needed TLB flushes added, and my fio
> benchmark on a virtio-blk device suffers a ~50% throughput regression,
> which does not necessarily spark joy. And I think James H.  mentioned at
> LPC that making the userfault stuff work with my patches would be quite
> hard. All this in addition to you also not necessarily sounding too keen
> on it either :D
>
>>>> so if KVM_X86_SW_PROTECTED_VM ends up zeroing, we'd need to add another new
>>>> VM type for that.
>> 
>> Maybe we should sneak in a s/KVM_X86_SW_PROTECTED_VM/KVM_X86_SW_HARDENED_VM rename?
>> The original thought behind "software protected VM" was to do a slow build of
>> something akin to pKVM, but realistically I don't think that idea is going anywhere.
>
> Ah, admittedly I've thought of KVM_X86_SW_PROTECTED_VM as a bit of a
> playground where various configurations other VM types enforce can be
> mixed and matched (e.g. zero on conversions yes/no, direct map removal
> yes/no) so more of a KVM_X86_GMEM_VM, but am happy to update my
> understanding :) 
>

Given the different axes of possible configurations for guest_memfd
(zero on conversion, direct map removal), I think it's better to let
userspace choose, than to enumerate the combinations in VM types.

Independently of whether to use a flag or VM type to configure
guest_memfd, the "zeroed" state has to be stored somewhere.

For folios to at least be zeroed once, presence in the filemap could
indicated "zeroed".

Presence in the filemap may be awkward to use as an indication of
"zeroed" for the conversion case.

What else can we use to store "zeroed"? Suggestions: 

1. Since "prepared" already took the dirty bit on the folio, "zeroed"
   can use the checked bit on the folio. [2] indicates that it is for
   filesystems, which sounds like guest_memfd :)
2. folio->private (which we may already need to use)
3. Another xarray

>> <snip>

[1] https://lore.kernel.org/all/diqz1q0qtqnd.fsf@ackerleytng-ctop.c.googlers.com/T/#ma6f828d7a50c4de8a2f829a16c9bb458b53d8f3f
[2] https://elixir.bootlin.com/linux/v6.11.4/source/include/linux/page-flags.h#L147

Powered by blists - more mailing lists