"David Hildenbrand (Arm)" <[email protected]> writes:
>
> [...snip...]
>
>> +static int kvm_gmem_folio_zap_direct_map(struct folio *folio)
>> +{
>> + u64 gmem_flags = GMEM_I(folio_inode(folio))->flags;
>> + int r = 0;
>> +
>> + if (kvm_gmem_folio_no_direct_map(folio) || !(gmem_flags &
>> GUEST_MEMFD_FLAG_NO_DIRECT_MAP))
>
> The function is only called when
>
> kvm_gmem_no_direct_map(folio_inode(folio))
>
> Does it really make sense to check for GUEST_MEMFD_FLAG_NO_DIRECT_MAP again?
>
Good point that GUEST_MEMFD_FLAG_NO_DIRECT_MAP was already checked in
the caller. I think we can drop this second check.
> If, at all, it should be a warning if GUEST_MEMFD_FLAG_NO_DIRECT_MAP is
> not set?
>
> Further, kvm_gmem_folio_zap_direct_map() uses the folio lock to
> synchronize, right? Might be worth pointing that out somehow (e.g.,
> lockdep check if possible).
>
>> + goto out;
>> +
>> + r = folio_zap_direct_map(folio);
>> + if (!r)
>> + folio->private = (void *)((u64)folio->private |
>> KVM_GMEM_FOLIO_NO_DIRECT_MAP);
>> +
>> +out:
>> + return r;
>> +}
>> +
>> +static void kvm_gmem_folio_restore_direct_map(struct folio *folio)
>> +{
>
> kvm_gmem_folio_zap_direct_map() is allowed to be called on folios that
> already have the directmap remove, kvm_gmem_folio_restore_direct_map()
> cannot be called if the directmap was already restored.
>
This inconsistency was probably introduced by my comments [1] (sorry!)
I think the inconsistency here is mostly because
kvm_gmem_folio_zap_direct_map() is called from two places but restore is
only called from one place :P
[1]
https://lore.kernel.org/all/CAEvNRgEzVhEzr-3GWTsE7GSBsPdvVLq7WFEeLHzcmMe=r9s...@mail.gmail.com/
> Should we make that more consistent?
>
>
> Hoping Sean can find some time to review
>
> --
> Cheers,
>
> David
