Re: [RFC PATCH v2 5/6] KVM: Add flags to struct kvm_gfn_range

[Isaku Yamahata <isaku.yamahata@gmail.com>]

On Wed, Jun 28, 2023 at 10:21:32AM -0500,
Michael Roth <michael.roth@amd.com> wrote:

> On Thu, Jun 22, 2023 at 04:16:29PM -0700, isaku.yamahata@intel.com wrote:
> > From: Isaku Yamahata <isaku.yamahata@intel.com>
> > 
> > Add flags to strut kvm_gfn_range to indicate who triggered the callback
> > and new memory attributes.
> > 
> > TDX needs to know the reason for a callback by kvm_unmap_gfn_range().  mmu
> > notifier, set memory attributes ioctl or KVM gmem callback.  With TDX,
> > zapping a private page from the encrypted page table and adding the page
> > back to the same private GPA results in zeroing the page, and the guest has
> > to accept the page again.  On the change of memory attribute from private
> > to shared, zapping the GPA range irrespective to private-or-shared and
> > expecting the fault doesn't work for TDX.  Instead, zap shared pages only
> > and keep the private pages.  Concretely
> > - If it's from mmu notifier, zap shared pages.
> > - If it's from KVM gmem, zap private pages.
> > - If setting memory attributes to private, zap shared pages.
> > - If setting memory attributes to shared, zap private pages.
> > 
> > Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> > 
> > ---
> > Changes v1 -> v2:
> > - consolidate KVM_GFN_RANGE_FLAGS_GMEM_{PUNCH_HOLE, RELEASE} into
> >   KVM_GFN_RANGE_FLAGS_GMEM.
> > - Update the commit message to describe TDX more.  Drop SEV_SNP.
> > ---
> >  include/linux/kvm_host.h | 10 +++++++++-
> >  virt/kvm/guest_mem.c     |  9 ++++++---
> >  virt/kvm/kvm_main.c      |  4 +++-
> >  3 files changed, 18 insertions(+), 5 deletions(-)
> > 
> > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > index 1a47cedae8a1..1fe0516fcddf 100644
> > --- a/include/linux/kvm_host.h
> > +++ b/include/linux/kvm_host.h
> > @@ -256,12 +256,20 @@ int kvm_async_pf_wakeup_all(struct kvm_vcpu *vcpu);
> >  #endif
> >  
> >  #ifdef CONFIG_KVM_GENERIC_MMU_NOTIFIER
> > +
> > +#define KVM_GFN_RANGE_FLAGS_SET_MEM_ATTR	BIT(0)
> > +#define KVM_GFN_RANGE_FLAGS_GMEM		BIT(1)
> > +
> >  struct kvm_gfn_range {
> >  	struct kvm_memory_slot *slot;
> >  	gfn_t start;
> >  	gfn_t end;
> > -	pte_t pte;
> > +	union {
> > +		pte_t pte;
> > +		u64 attrs;
> > +	};
> >  	bool may_block;
> > +	unsigned int flags;
> >  };
> >  bool kvm_unmap_gfn_range(struct kvm *kvm, struct kvm_gfn_range *range);
> >  bool kvm_age_gfn(struct kvm *kvm, struct kvm_gfn_range *range);
> > diff --git a/virt/kvm/guest_mem.c b/virt/kvm/guest_mem.c
> > index cdf2d84683c8..387226136960 100644
> > --- a/virt/kvm/guest_mem.c
> > +++ b/virt/kvm/guest_mem.c
> > @@ -99,7 +99,8 @@ static struct folio *kvm_gmem_get_folio(struct file *file, pgoff_t index)
> >  }
> >  
> >  static void kvm_gmem_invalidate_begin(struct kvm *kvm, struct kvm_gmem *gmem,
> > -				      pgoff_t start, pgoff_t end)
> > +				      pgoff_t start, pgoff_t end,
> > +				      unsigned int flags)
> >  {
> >  	struct kvm_memory_slot *slot;
> >  	unsigned long index;
> > @@ -118,6 +119,7 @@ static void kvm_gmem_invalidate_begin(struct kvm *kvm, struct kvm_gmem *gmem,
> >  			.slot = slot,
> >  			.pte = __pte(0),
> >  			.may_block = true,
> > +			.flags = flags,
> >  		};
> >  
> >  		kvm_mmu_invalidate_range_add(kvm, gfn_range.start, gfn_range.end);
> > @@ -156,7 +158,8 @@ static long kvm_gmem_punch_hole(struct file *file, loff_t offset, loff_t len)
> >  	 */
> >  	filemap_invalidate_lock(file->f_mapping);
> >  
> > -	kvm_gmem_invalidate_begin(kvm, gmem, start, end);
> > +	kvm_gmem_invalidate_begin(kvm, gmem, start, end,
> > +				  KVM_GFN_RANGE_FLAGS_GMEM);
> 
> Do you anticipate ever needing to pass a different flag via
> kvm_gmem_invalidate_begin()? If not it might make sense to just
> hard-code it rather than passing as a parameter.

I'll update this patch following the suggestion by Sean. [1]
I'll drop the flag. the struct kvm_gfn_range will be

struct kvm_gfn_range {
 struct kvm_memory_slot *slot;
 gfn_t start;
 gfn_t end;
 union {
  struct test_clear_young_metadata *metadata;
  unsigned long attributes;
  pte_t pte;
  unsigned long callback_arg; /* needs a better name */
 };
 bool only_private;
 bool only_shared;
 bool may_block;
};


[1] https://lore.kernel.org/all/ZJX0hk+KpQP0KUyB@google.com/
-- 
Isaku Yamahata <isaku.yamahata@gmail.com>