Re: [PATCH v3 11/18] KVM: arm64: Introduce __pkvm_host_unshare_guest()

[Marc Zyngier <maz@kernel.org>]

On Tue, 17 Dec 2024 13:33:57 +0000,
Quentin Perret <qperret@google.com> wrote:
> 
> On Tuesday 17 Dec 2024 at 11:29:03 (+0000), Marc Zyngier wrote:
> > > +static int __check_host_shared_guest(struct pkvm_hyp_vm *vm, u64 *__phys, u64 ipa)
> > > +{
> > > +	enum pkvm_page_state state;
> > > +	struct hyp_page *page;
> > > +	kvm_pte_t pte;
> > > +	u64 phys;
> > > +	s8 level;
> > > +	int ret;
> > > +
> > > +	ret = kvm_pgtable_get_leaf(&vm->pgt, ipa, &pte, &level);
> > > +	if (ret)
> > > +		return ret;
> > > +	if (level != KVM_PGTABLE_LAST_LEVEL)
> > 
> > So there is still a very strong assumption that a guest is only
> > provided page mappings, and no blocks?
> 
> Yep, very much so. It's one of the main limitations of the series as-is
> (with the absence of support for mapping anything else than memory in
> guests). Those limitations were mentioned in the cover letter of v1, but
> I should have kept that mention in later versions, sorry!
> 
> The last patch of the series has a tweak to user_mem_abort() to force
> mappings to PTE level which is trivial to do as we already need to do
> similar things for dirty logging. And __pkvm_host_share_guest() doesn't
> take a 'size' parameter in its current form, it assumes it is being
> passed a single pfn. So all in all this works well, and simplifies the
> series a lot.
> 
> Huge-page support should come as a natural extension to this series, but
> I was hoping it could be done separately as that should have no
> *functional* impact observable from userspace. I'm slightly more
> concerned about the lack of support for mapping MMIO, but that too is
> going to be some work, and I guess you should just turn pKVM off if
> you want that for now...
> 
> Happy to address either or both of these limitations as part of this
> series if we think they're strictly required to land this stuff
> upstream, this is obviously up for debate. But that's going to be quite
> a few patches on top :-)

No, I just wanted to make sure I did have the correct interpretation
of what this does. 'd rather have something that works first, and then
add large mapping support. You can even sell it as a performance
improvement! :)

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.