Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH 1/1] drm/ttm: Set memory as decrypted for ttm framebuffer
 mappings
To:     =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>,
        Jiandi An <Jiandi.An@amd.com>, dri-devel@lists.freedesktop.org
Cc:     airlied@linux.ie, linux-kernel@vger.kernel.org, ray.huang@amd.com,
        Jerry.Zhang@amd.com
References: <20180822185754.104729-1-jiandi.an@amd.com>
 <d22f2dce-7229-aac8-036b-5312927632e4@gmail.com>
 <2eeaae95-181c-b856-09ae-4ee2423fe3fd@amd.com>
 <92cfac31-a408-210a-d901-1e6511a1cb9a@amd.com>
From:   Jiandi An <jiandi@amd.com>
Message-ID: <3ef2e755-eb99-0e4b-1e0c-c2ae62aafc10@amd.com>
Date:   Thu, 23 Aug 2018 18:05:57 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.9.1
MIME-Version: 1.0
In-Reply-To: <92cfac31-a408-210a-d901-1e6511a1cb9a@amd.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Received-SPF: None (protection.outlook.com: amd.com does not designate
 permitted sender hosts)
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Aug 2018 23:06:03.4220 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: aa8cd16d-7c63-4093-bf7f-08d6094d0010
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR1201MB2532
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 08/23/2018 01:47 AM, Christian König wrote:
> Am 22.08.2018 um 22:57 schrieb Jiandi An:
>>
>> On 08/22/2018 02:09 PM, Christian König wrote:
>>> Am 22.08.2018 um 20:57 schrieb Jiandi An:
>>>> Framebuffer memory needs to be accessed decrypted.  Ensure the
>>>> memory encryption mask is not set for the ttm framebuffer mappings.
>>> NAK, the memory not only needs to be decrypted while CPU accessed 
>>> but all the time.
>>>
>>> ttm_page_alloc.c and ttm_page_alloc_dma.c should already take care 
>>> of that while mapping the pages.
>>>
>>> Regards,
>>> Christian.
>>>
>> The path where the issue comes in is booting guest with AMD SEV 
>> enabled while using virtio-vga device.
>> The ttm->pages doesn't go through ttm_populate_and_map_pages().
>
> And that is the point where you need to jump in and fix the problem.
>
> When a driver implements the populate() and unpopulate() callbacks 
> themselves it needs to take care of all that handling.
Thanks for the suggestion and guidance.  So you want me to register the 
callbacks something like virtio_gpu_ttm_populate() and 
virtio_gpu_ttm_unpopulate() in the virtio_gpu_bo_driver, and perform 
mapping by calling ttm_bo_kmap()?  Then bring setting memory 
decrypted/encryped by calling 
set_memory_decrypted()/set_memory_encrypted() outside of ttm_bo_kmap(), 
do it within virtio_gpu_ttm_populate() and virtio_gpu_ttm_unpopulate() 
callbacks?

Then get rid of the separate call of virtio_gpu_vmap_fb() the virtio_gpu 
driver does?

>
>>
>> In the kernel path, the virtio_gpu driver calls 
>> virtio_gpu_alloc_object() and goes down to ttm to
>> allocate pages in ttm_pool_populate().  Driver in guest then does 
>> virtio_gpu_vmap_fb() which goes
>> down to ttm_bo_kmap_ttm() and does a vmap() for GVA to GPA for those 
>> pages.  If SEV is enabled,
>> decryption should be set in this GVA to GPA mapping.
>
> That assumption is what is incorrect here.
>
> TTM can't work with encrypted pages, so you need to set the pages as 
> decrypted directly after calling ttm_pool_populate().
>
> And obviously set it to encrypted again before calling 
> ttm_pool_unpopulate().
>
> Probably best if we add that to ttm_tt_populate()/ttm_tt_unpopulate().

Here when you say ttm_tt_populate() and ttm_tt_unpopulate() you mean the 
virtio_gpu_ttm_populate() and virtio_gpu_ttm_unpopulate() that I 
register in virtio_gpu_bo_driver right?

>
> Regards,
> Christian.
>
>>    Guest then sends object attach command to host
>> via virtio_gpu_object_attach() which stuffs the pages' physical 
>> addresses (guest physical address)
>> in a scatter list and send them to host QEMU. Host QEMU maps those 
>> guest pages GPA to HPA and access
>> via HVA while guest write stuff in those pages via GVA.
>>
>> virtio_gpu_alloc_object()
>>     virtio_gpu_object_create()
>>        sturct virtio_gpu_object bo kzalloc()
>>           ttm_bo_init()
>>           ...
>>              ttm_tt_create()
>>                 bo->ttm = bdev->driver->ttm_tt_create()
>>                     virtio_gpu_ttm_tt_create()
>>                     ...
>>                        ttm_tt_populate()
>>                           ttm_pool_populate()
>>                              ttm_get_pages(ttm->pages, ttm->num_pages)
>>
>> virtio_gpu_vmap_fb()
>>     virtio_gpu_object_kmap(obj, NULL)
>>        ttm_bo_kmap
>>           ttm_mem_io_reserve()
>>              ttm_bo_kmap_ttm()
>>                 vmap()
>>              struct virtio_gpu_object bo->vmap = 
>> ttm_kmap_obj_virtual(&bo->kmap, &is_iomem);
>>
>>
>> virtio_gpu_object_attach() <<== Sending attach backing command
>>     virtio_gpu_object_get_sg_table()
>>        if (bo->tbo.ttm->state == tt_unpopulated)
>>           virtio_gpu_object 
>> bo.ttm->bdev->driver->ttm_tt_populate(bo->tbo.ttm, &ctx);
>>              bo->pages = kmalloc(sizeof(struct sg_table), GFP_KERNEL);
>> sg_alloc_table_from_pages(bo->tbo.ttm->pages)  //Getfrom ttm->pages 
>> and put in sg
>>                 __sg_alloc_table_from_pages()
>>
>>
>> There is a separate userspace path for example when displace manager 
>> kicks in,
>> virtio_gpu_alloc_object() followed by virtio_gpu_object_attach() are 
>> called through
>> the ioctl virtio_gpu_resource_create_ioctl().  The mapping of the 
>> pages created in this
>> page are handled in the do_page_fault() path in ttm_bo_vm_ops's 
>> ttm_bo_vm_fault() handler.
>>
>> do_page_fault()
>>     handle_mm_fault()
>>        __do_page_fault()
>>           ttm_bo_vm_fault()
>>              ttm_bo_reserve()
>>                 __ttm_bo_reserve()
>>
>>              ttm_mem_io_reserve_vm()
>>                 ttm_mem_io_reserve()
>>                    bdev->driver->io_mem_reserve()
>>                       virtio_gpu_ttm_io_mem_reserve()
>>                          mem->bus.is_iomem = false
>>                          mem->mem_type == TTM_PL_TT
>>
>>
>> I originally handled setting pages decrypted for the kernel path for 
>> GVA to GPA mapping in guest
>> in virtio_gpu_vmap_fb() at the virtio_gpu driver layer.  But for the 
>> user space path
>> virtio_gpu_vmap_fb() is not called.  Mapping is handled in the 
>> page_fault handler.  So I moved it
>> to the ttm layer.
>>
>> What layer do you recommend as more appropriate to handle setting 
>> memory decrypted for GVA to GPA
>> mapping for thos ttm->pages?
>>
>> Thanks
>> Jiandi
>>
>>>> Signed-off-by: Jiandi An <jiandi.an@amd.com>
>>>> ---
>>>>    drivers/gpu/drm/ttm/ttm_bo_util.c | 12 +++++++++++-
>>>>    drivers/gpu/drm/ttm/ttm_bo_vm.c   |  6 ++++--
>>>>    2 files changed, 15 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c 
>>>> b/drivers/gpu/drm/ttm/ttm_bo_util.c
>>>> index 046a6dda690a..b3f5d26f571e 100644
>>>> --- a/drivers/gpu/drm/ttm/ttm_bo_util.c
>>>> +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
>>>> @@ -29,6 +29,7 @@
>>>>     * Authors: Thomas Hellstrom <thellstrom-at-vmware-dot-com>
>>>>     */
>>>>    +#include <asm/set_memory.h>
>>>>    #include <drm/ttm/ttm_bo_driver.h>
>>>>    #include <drm/ttm/ttm_placement.h>
>>>>    #include <drm/drm_vma_manager.h>
>>>> @@ -639,7 +640,11 @@ int ttm_bo_kmap(struct ttm_buffer_object *bo,
>>>>        if (ret)
>>>>            return ret;
>>>>        if (!bo->mem.bus.is_iomem) {
>>>> -        return ttm_bo_kmap_ttm(bo, start_page, num_pages, map);
>>>> +        ret = ttm_bo_kmap_ttm(bo, start_page, num_pages, map);
>>>> +        if (!ret && sev_active())
>>>> +            set_memory_decrypted((unsigned long) map->virtual,
>>>> +                         num_pages);
>>>> +        return ret;
>>>>        } else {
>>>>            offset = start_page << PAGE_SHIFT;
>>>>            size = num_pages << PAGE_SHIFT;
>>>> @@ -661,9 +666,14 @@ void ttm_bo_kunmap(struct ttm_bo_kmap_obj *map)
>>>>            iounmap(map->virtual);
>>>>            break;
>>>>        case ttm_bo_map_vmap:
>>>> +        if (sev_active())
>>>> +            set_memory_encrypted((unsigned long) map->virtual,
>>>> +                         bo->num_pages);
>>>>            vunmap(map->virtual);
>>>>            break;
>>>>        case ttm_bo_map_kmap:
>>>> +        if (sev_active())
>>>> +            set_memory_encrypted((unsigned long) map->virtual, 1);
>>>>            kunmap(map->page);
>>>>            break;
>>>>        case ttm_bo_map_premapped:
>>>> diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c 
>>>> b/drivers/gpu/drm/ttm/ttm_bo_vm.c
>>>> index 6fe91c1b692d..211d3549fd9f 100644
>>>> --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
>>>> +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
>>>> @@ -249,10 +249,12 @@ static vm_fault_t ttm_bo_vm_fault(struct 
>>>> vm_fault *vmf)
>>>>         * Speculatively prefault a number of pages. Only error on
>>>>         * first page.
>>>>         */
>>>> +
>>>> +    /* Mark framebuffer pages decrypted */
>>>> +    cvma.vm_page_prot = pgprot_decrypted(cvma.vm_page_prot);
>>>> +
>>>>        for (i = 0; i < TTM_BO_VM_NUM_PREFAULT; ++i) {
>>>>            if (bo->mem.bus.is_iomem) {
>>>> -            /* Iomem should not be marked encrypted */
>>>> -            cvma.vm_page_prot = pgprot_decrypted(cvma.vm_page_prot);
>>>>                pfn = ttm_bo_io_mem_pfn(bo, page_offset);
>>>>            } else {
>>>>                page = ttm->pages[page_offset];
>