Re: [RFC PATCH 6/6] KVM: SVM: Pin SEV pages in MMU during sev_launch_update_data()

["Nikunj A. Dadhania" <nikunj@amd.com>]

On 1/21/2022 9:30 PM, Peter Gonda wrote:
> On Thu, Jan 20, 2022 at 9:08 PM Nikunj A. Dadhania <nikunj@amd.com> wrote:
>>
>> On 1/20/2022 9:47 PM, Peter Gonda wrote:
>>> On Tue, Jan 18, 2022 at 4:07 AM Nikunj A Dadhania <nikunj@amd.com> wrote:

>>>>  static int sev_launch_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
>>>>  {
>>>>         unsigned long vaddr, vaddr_end, next_vaddr, npages, pages, size, i;
>>>> @@ -510,15 +615,21 @@ static int sev_launch_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
>>>>         vaddr_end = vaddr + size;
>>>>
>>>>         /* Lock the user memory. */
>>>> -       inpages = sev_pin_memory(kvm, vaddr, size, &npages, 1);
>>>> +       if (atomic_read(&kvm->online_vcpus))
>>>> +               inpages = sev_pin_memory_in_mmu(kvm, vaddr, size, &npages);
>>>
>>> IIUC we can only use the sev_pin_memory_in_mmu() when there is an
>>> online vCPU because that means the MMU has been setup enough to use?
>>> Can we add a variable and a comment to help explain that?
>>>
>>> bool mmu_usable = atomic_read(&kvm->online_vcpus) > 0;
>>
>> Sure, will add comment and the variable.
>>
>>>
>>>> +       else
>>>> +               inpages = sev_pin_memory(kvm, vaddr, size, &npages, 1);
>>>
>>> So I am confused about this case. Since svm_register_enc_region() is
>>> now a NOOP how can a user ensure that memory remains pinned from
>>> sev_launch_update_data() to when the memory would be demand pinned?
>>>
>>> Before users could svm_register_enc_region() which pins the region,
>>> then sev_launch_update_data(), then the VM could run an the data from
>>> sev_launch_update_data() would have never moved. I don't think that
>>> same guarantee is held here?
>>
>> Yes, you are right. One way is to error out of this call if MMU is not setup.
>> Other one would require us to maintain all list of pinned memory via sev_pin_memory()
>> and unpin them in the destroy path.
> 
> Got it. So we'll probably still need regions_list to track those
> pinned regions and free them on destruction.
>
Yes, I will have to bring that structure back.
 
> Also similar changes are probably needed in sev_receive_update_data()?

Right, there are multiple locations where sev_pin_memory() is used, I will go through each 
case and make changes. Alternatively, add to the region_list in sev_pin_memory() and free in 
destruction.

Regards
Nikunj