RE: [PATCH Part2 v6 26/49] KVM: SVM: Add KVM_SEV_SNP_LAUNCH_UPDATE command

[Kalra, Ashish]

[Public]

>> +static inline void snp_leak_pages(u64 pfn, enum pg_level level) {
>> +       unsigned int npages = page_level_size(level) >> PAGE_SHIFT;
>> +
>> +       WARN(1, "psc failed pfn 0x%llx pages %d (leaking)\n", pfn, 
>> + npages);
>> +
>> +       while (npages) {
>> +               memory_failure(pfn, 0);
>> +               dump_rmpentry(pfn);
>> +               npages--;
>> +               pfn++;
>> +       }
>> +}

>Should this be deduplicated with the snp_leak_pages() in "crypto: ccp:
>Handle the legacy TMR allocation when SNP is enabled" ?

Yes, probably should.

>> +static bool is_hva_registered(struct kvm *kvm, hva_t hva, size_t len) 
>> +{
>> +       struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
>> +       struct list_head *head = &sev->regions_list;
>> +       struct enc_region *i;
>> +
>> +       lockdep_assert_held(&kvm->lock);
>> +
>> +       list_for_each_entry(i, head, list) {
>> +               u64 start = i->uaddr;
>> +               u64 end = start + i->size;
>> +
>> +               if (start <= hva && end >= (hva + len))
>> +                       return true;
>> +       }

>Given that usersapce could load sev->regions_list with any # of any sized regions. Should we add a  cond_resched() like in sev_vm_destroy()?

Actually, is_hva_registered() is also called from PSC handler with kvm->lock mutex held. Even though it is a mutex, I am not really sure if
it is a good idea to do cond_resched() with the kvm->lock mutex held ?

>> +e_unpin:
>> +       /* Content of memory is updated, mark pages dirty */
>> +       for (i = 0; i < n; i++) {

>Since |n| is not only a loop variable but actually carries the number of private pages over to e_unpin can we use a more descriptive name?
>How about something like 'nprivate_pages'?

Yes.

Thanks,
Ashish