On Wed, Jan 22, 2025 at 08:39:37AM -0600, Tom Lendacky wrote:
> On 12/12/24 00:36, Michael Roth wrote:
> > Currently __kvm_gmem_get_pfn() sets 'is_prepared' so callers can skip
> > calling kvm_gmem_prepare_folio(). However, subsequent patches will
> > introduce some locking constraints around setting/checking preparedness
> > that will require filemap_invalidate_lock*() to be held while checking
> > for preparedness. This locking could theoretically be done inside
> > __kvm_gmem_get_pfn(), or by requiring that filemap_invalidate_lock*() is
> > held while calling __kvm_gmem_get_pfn(), but that places unnecessary
> > constraints around when __kvm_gmem_get_pfn() can be called, whereas
> > callers could just as easily call kvm_gmem_is_prepared() directly.
> >
> > So, in preparation for these locking changes, drop the 'is_prepared'
> > argument, and leave it up to callers to handle checking preparedness
> > where needed and with the proper locking constraints.
> >
> > Signed-off-by: Michael Roth <michael.roth@xxxxxxx>
> > ---
> > virt/kvm/guest_memfd.c | 13 +++++--------
> > 1 file changed, 5 insertions(+), 8 deletions(-)
> >
> > diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> > index b69af3580bef..aa0038ddf4a4 100644
> > --- a/virt/kvm/guest_memfd.c
> > +++ b/virt/kvm/guest_memfd.c
> > @@ -773,7 +773,7 @@ void kvm_gmem_unbind(struct kvm_memory_slot *slot)
> > static struct folio *__kvm_gmem_get_pfn(struct file *file,
> > struct kvm_memory_slot *slot,
> > pgoff_t index, kvm_pfn_t *pfn,
> > - bool *is_prepared, int *max_order)
> > + int *max_order)
> > {
> > struct kvm_gmem *gmem = file->private_data;
> > struct folio *folio;
> > @@ -803,7 +803,6 @@ static struct folio *__kvm_gmem_get_pfn(struct file *file,
> > if (max_order)
> > *max_order = 0;
> >
> > - *is_prepared = kvm_gmem_is_prepared(file, index, folio);
> > return folio;
> > }
> >
> > @@ -814,19 +813,18 @@ int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
> > pgoff_t index = kvm_gmem_get_index(slot, gfn);
> > struct file *file = kvm_gmem_get_file(slot);
> > struct folio *folio;
> > - bool is_prepared = false;
> > int r = 0;
> >
> > if (!file)
> > return -EFAULT;
> >
> > - folio = __kvm_gmem_get_pfn(file, slot, index, pfn, &is_prepared, max_order);
> > + folio = __kvm_gmem_get_pfn(file, slot, index, pfn, max_order);
> > if (IS_ERR(folio)) {
> > r = PTR_ERR(folio);
> > goto out;
> > }
> >
> > - if (!is_prepared)
> > + if (kvm_gmem_is_prepared(file, index, folio))
>
> Shouldn't this be !kvm_gmem_is_prepared() ?
Yes indeed. It looks like I fixed this up later, but accidentally squashed it
into PATCH #2 rather than here. Will fix for the next spin.
Thanks,
Mike
>
> Thanks,
> Tom
>
> > r = kvm_gmem_prepare_folio(kvm, file, slot, gfn, folio);
> >
> > folio_unlock(folio);
> > @@ -872,7 +870,6 @@ long kvm_gmem_populate(struct kvm *kvm, gfn_t start_gfn, void __user *src, long
> > struct folio *folio;
> > gfn_t gfn = start_gfn + i;
> > pgoff_t index = kvm_gmem_get_index(slot, gfn);
> > - bool is_prepared = false;
> > kvm_pfn_t pfn;
> >
> > if (signal_pending(current)) {
> > @@ -880,13 +877,13 @@ long kvm_gmem_populate(struct kvm *kvm, gfn_t start_gfn, void __user *src, long
> > break;
> > }
> >
> > - folio = __kvm_gmem_get_pfn(file, slot, index, &pfn, &is_prepared, &max_order);
> > + folio = __kvm_gmem_get_pfn(file, slot, index, &pfn, &max_order);
> > if (IS_ERR(folio)) {
> > ret = PTR_ERR(folio);
> > break;
> > }
> >
> > - if (is_prepared) {
> > + if (kvm_gmem_is_prepared(file, index, folio)) {
> > folio_unlock(folio);
> > folio_put(folio);
> > ret = -EEXIST;
>
