Date:   Thu, 13 Jul 2023 13:28:34 -0700
From:   Elliot Berman <quic_eberman@...cinc.com>
To:     Will Deacon <will@...nel.org>
CC:     Alex Elder <elder@...aro.org>,
        Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        Prakruthi Deepak Heragu <quic_pheragu@...cinc.com>,
        Murali Nalajala <quic_mnalajal@...cinc.com>,
        Trilok Soni <quic_tsoni@...cinc.com>,
        Srivatsa Vaddagiri <quic_svaddagi@...cinc.com>,
        Carl van Schaik <quic_cvanscha@...cinc.com>,
        Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        "Arnd Bergmann" <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Jonathan Corbet <corbet@....net>,
        "Bagas Sanjaya" <bagasdotme@...il.com>,
        Andy Gross <agross@...nel.org>,
        "Catalin Marinas" <catalin.marinas@....com>,
        Jassi Brar <jassisinghbrar@...il.com>,
        <linux-arm-msm@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-doc@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>, <qperret@...gle.com>
Subject: Re: [PATCH v13 10/24] gunyah: vm_mgr: Add/remove user memory regions

Hi Will,

On 6/22/2023 4:56 PM, Elliot Berman wrote:
> 
> 
> On 6/7/2023 8:54 AM, Elliot Berman wrote:
>>
>>
>> On 6/5/2023 7:18 AM, Will Deacon wrote:
>>> Hi Elliot,
>>>
>>> [+Quentin since he's looked at the MMU notifiers]
>>>
>>> Sorry for the slow response, I got buried in email during a week away.
>>>
>>> On Fri, May 19, 2023 at 10:02:29AM -0700, Elliot Berman wrote:
>>>> On 5/19/2023 4:59 AM, Will Deacon wrote:
>>>>> On Tue, May 09, 2023 at 01:47:47PM -0700, Elliot Berman wrote:
>>>>>> +    ret = account_locked_vm(ghvm->mm, mapping->npages, true);
>>>>>> +    if (ret)
>>>>>> +        goto free_mapping;
>>>>>> +
>>>>>> +    mapping->pages = kcalloc(mapping->npages, 
>>>>>> sizeof(*mapping->pages), GFP_KERNEL_ACCOUNT);
>>>>>> +    if (!mapping->pages) {
>>>>>> +        ret = -ENOMEM;
>>>>>> +        mapping->npages = 0; /* update npages for reclaim */
>>>>>> +        goto unlock_pages;
>>>>>> +    }
>>>>>> +
>>>>>> +    gup_flags = FOLL_LONGTERM;
>>>>>> +    if (region->flags & GH_MEM_ALLOW_WRITE)
>>>>>> +        gup_flags |= FOLL_WRITE;
>>>>>> +
>>>>>> +    pinned = pin_user_pages_fast(region->userspace_addr, 
>>>>>> mapping->npages,
>>>>>> +                    gup_flags, mapping->pages);
>>>>>> +    if (pinned < 0) {
>>>>>> +        ret = pinned;
>>>>>> +        goto free_pages;
>>>>>> +    } else if (pinned != mapping->npages) {
>>>>>> +        ret = -EFAULT;
>>>>>> +        mapping->npages = pinned; /* update npages for reclaim */
>>>>>> +        goto unpin_pages;
>>>>>> +    }
>>>>>
>>>>> Sorry if I missed it, but I still don't see where you reject file 
>>>>> mappings
>>>>> here.
>>>>>
>>>>
>>>> Sure, I can reject file mappings. I didn't catch that was the ask 
>>>> previously
>>>> and thought it was only a comment about behavior of file mappings.
>>>
>>> I thought the mention of filesystem corruption was clear enough! It's
>>> definitely something we shouldn't allow.
>>>
>>>>> This is also the wrong interface for upstream. Please get involved 
>>>>> with
>>>>> the fd-based guest memory discussions [1] and port your series to 
>>>>> that.
>>>>>
>>>>
>>>> The user interface design for *shared* memory aligns with
>>>> KVM_SET_USER_MEMORY_REGION.
>>>
>>> I don't think it does. For example, file mappings don't work (as above),
>>> you're placing additional rlimit requirements on the caller, read-only
>>> memslots are not functional, the memory cannot be swapped or migrated,
>>> dirty logging doesn't work etc. pKVM is in the same boat, but that's why
>>> we're not upstreaming this part in its current form.
>>>
>>
>> I thought pKVM was only holding off on upstreaming changes related to 
>> guest-private memory?
>>
>>>> I understood we want to use restricted memfd for giving 
>>>> guest-private memory
>>>> (Gunyah calls this "lending memory"). When I went through the 
>>>> changes, I
>>>> gathered KVM is using restricted memfd only for guest-private memory 
>>>> and not
>>>> for shared memory. Thus, I dropped support for lending memory to the 
>>>> guest
>>>> VM and only retained the shared memory support in this series. I'd 
>>>> like to
>>>> merge what we can today and introduce the guest-private memory 
>>>> support in
>>>> tandem with the restricted memfd; I don't see much reason to delay the
>>>> series.
>>>
>>> Right, protected guests will use the new restricted memfd ("guest mem"
>>> now, I think?), but non-protected guests should implement the existing
>>> interface *without* the need for the GUP pin on guest memory pages. Yes,
>>> that means full support for MMU notifiers so that these pages can be
>>> managed properly by the host kernel. We're working on that for pKVM, but
>>> it requires a more flexible form of memory sharing over what we 
>>> currently
>>> have so that e.g. the zero page can be shared between multiple entities.
>>
>> Gunyah doesn't support swapping pages out while the guest is running 
>> and the design of Gunyah isn't made to give host kernel full control 
>> over the S2 page table for its guests. As best I can tell from reading 
>> the respective drivers, ACRN and Nitro Enclaves both GUP pin guest 
>> memory pages prior to giving them to the guest, so I don't think this 
>> requirement from Gunyah is particularly unusual.
>>
> 
> I read/dug into mmu notifiers more and I don't think it matches with 
> Gunyah's features today. We don't allow the host to freely manage VM's 
> pages because it requires the guest VM to have a level of trust on the 
> host. Once a page is given to the guest, it's done for the lifetime of 
> the VM. Allowing the host to replace pages in the guest memory map isn't 
> part of any VM's security model that we run in Gunyah. With that 
> requirement, longterm pinning looks like the correct approach to me.

Is my approach of longterm pinning correct given that Gunyah doesn't 
allow host to freely swap pages?

Powered by blists - more mailing lists