On Thu, Jul 06, 2023 at 01:52:08PM +0900, David Stevens wrote:

On Wed, Jul 5, 2023 at 7:17 PM Yu Zhang [off-list ref] wrote:

quoted

On Tue, Jul 04, 2023 at 04:50:50PM +0900, David Stevens wrote:

quoted

From: David Stevens <redacted>

Stop passing FOLL_GET to __kvm_follow_pfn. This allows the host to map
memory into the guest that is backed by un-refcounted struct pages - for
example, higher order non-compound pages allocated by the amdgpu driver
via ttm_pool_alloc_page.

I guess you mean the tail pages of the higher order non-compound pages?
And as to the head page, it is said to be set to one coincidentally[*],
and shall not be considered as refcounted.  IIUC, refcount of this head
page will be increased and decreased soon in hva_to_pfn_remapped(), so
this may not be a problem(?). But treating this head page differently,
as a refcounted one(e.g., to set the A/D flags), is weired.

Or maybe I missed some context, e.g., can the head page be allocted to
guest at all?

Yes, this is to allow mapping the tail pages of higher order
non-compound pages - I should have been more precise in my wording.
The head pages can already be mapped into the guest.

Treating the head and tail pages would require changing how KVM
behaves in a situation it supports today (rather than just adding
support for an unsupported situation). Currently, without this series,
KVM can map VM_PFNMAP|VM_IO memory backed by refcounted pages into the
guest. When that happens, KVM sets the A/D flags. I'm not sure whether
that's actually valid behavior, nor do I know whether anyone actually
cares about it. But it's what KVM does today, and I would shy away
from modifying that behavior without good reason.

I know the A/D status of the refcounted, VM_PFNMAP|VM_IO backed pages
will be recorded. And I have no idea if this is a necessary requirement
either.

But it feels awkward to see the head and the tail ones of non-compound
pages be treated inconsistently. After all, the head page just happens
to have its refcount being 1, it is not a real refcounted page.

So I would suggest to mention such different behehavior in the commit
message at least. :)

quoted

quoted

@@ -883,7 +884,7 @@ static gpa_t FNAME(gva_to_gpa)(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
  */
 static int FNAME(sync_spte)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp, int i)
 {
-     bool host_writable;
+     bool host_writable, is_refcounted;
      gpa_t first_pte_gpa;
      u64 *sptep, spte;
      struct kvm_memory_slot *slot;

@@ -940,10 +941,12 @@ static int FNAME(sync_spte)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp, int
      sptep = &sp->spt[i];
      spte = *sptep;
      host_writable = spte & shadow_host_writable_mask;
+     // TODO: is this correct?
+     is_refcounted = spte & SPTE_MMU_PAGE_REFCOUNTED;
      slot = kvm_vcpu_gfn_to_memslot(vcpu, gfn);
      make_spte(vcpu, sp, slot, pte_access, gfn,
                spte_to_pfn(spte), spte, true, false,
-               host_writable, &spte);
+               host_writable, is_refcounted, &spte);

Could we restrict that a non-refcounted page shall not be used as shadow page?

I'm not very familiar with the shadow mmu, so my response might not
make sense. But do you mean not allowing non-refcoutned pages as the
guest page tables shadowed by a kvm_mmu_page? It would probably be
possible to do that, and I doubt anyone would care about the
restriction. But as far as I can tell, the guest page table is only
accessed via kvm_vcpu_read_guest_atomic, which handles non-refcounted
pages just fine.

Sorry, my brain just got baked... Pls just ignore this question :)

B.R.
Yu