Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <57562db8-bacf-e82d-8417-ab6343c1d2fa@amd.com>
Date:   Mon, 15 Aug 2022 12:51:02 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH v1] drm/ttm: Refcount allocated tail pages
Content-Language: en-US
To:     Dmitry Osipenko <dmitry.osipenko@collabora.com>,
        David Airlie <airlied@linux.ie>, Huang Rui <ray.huang@amd.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        Trigger Huang <Trigger.Huang@gmail.com>,
        Gert Wollny <gert.wollny@collabora.com>,
        Antonio Caggiano <antonio.caggiano@collabora.com>
Cc:     dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
        Dmitry Osipenko <digetx@gmail.com>, kvm@vger.kernel.org,
        kernel@collabora.com, virtualization@lists.linux-foundation.org
References: <20220815095423.11131-1-dmitry.osipenko@collabora.com>
 <8230a356-be38-f228-4a8e-95124e8e8db6@amd.com>
 <134bce02-58d6-8553-bb73-42dfda18a595@collabora.com>
 <8caf3008-dcf3-985a-631e-e019b277c6f0@amd.com>
 <4fcc4739-2da9-1b89-209c-876129604d7d@amd.com>
 <14be3b22-1d60-732b-c695-ddacc6b21055@collabora.com>
 <2df57a30-2afb-23dc-c7f5-f61c113dd5b4@collabora.com>
From:   =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>
In-Reply-To: <2df57a30-2afb-23dc-c7f5-f61c113dd5b4@collabora.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Mk5lUDFzK2xqa3o4djJRcUR4N3ZZMU1lOFI3UHZpVGtHOUNmb3EwSDlWc1FC?=
 =?utf-8?B?N2Q4ZHBIS3FwOVVCcUFEaXVrNU80TWp5WkdqRTRJcTJuWEZ0UXhVNG5zcXZu?=
 =?utf-8?B?RnVTZnQ2djhJY1JuTTZISkVqTTJLOUVWZVNuZFZxY2VzZE1KY2hKMUEvWjJI?=
 =?utf-8?B?eFVSNXRscy9yS1ZBVTRmQXVqWjU0bVR4UEtmRTdmM0QzS3dkR1ptUmhEa2gv?=
 =?utf-8?B?MDYyL2F1VTFmQ3ZibnZDK1F1QjlnUjhRVVZKTlZNVTJ6dzVUTXF3R2xxbWla?=
 =?utf-8?B?TTQxbW9FYVBGajgwclFsWm5MdmwxWUsvRUkycWNNYld2T3ExN3Fma0d4WFJm?=
 =?utf-8?B?VVBQQ2FBRkhuV2tDNFRqN1l2NDRYbDFHNjFVdHRrMS9RS0U1ZlB5UmlqaTBC?=
 =?utf-8?B?L2kwQjNrSnhWSEZlZkc4SXp4ZU9zczdxK3lIR083WUh3RkRuYzZ5WXp0MXJp?=
 =?utf-8?B?V25TUW5uMnVpQ0xMVVV4dEtsRTNIdCt3SU5oSFBoL1pIRGgwUVE2YVY4Unk3?=
 =?utf-8?B?VGovZm8vV0RGSTQ3M0E0eDNVSjlIbjZYSWQ4SXI1VmE2QW5kcjdvQkxIUzIx?=
 =?utf-8?B?RHpjRlZkWkMxbTZWa0FVSVhaaUlqVzkweTJZWk1RRGlUR3UyZG5lN3QxUFYr?=
 =?utf-8?B?dUdCRStYbmphWFc3U1VyZE91ZmNwbWdyejBkYmpCaXdzK28yYVNZcXZneUxF?=
 =?utf-8?B?U05pem1OblpjaTRTVklHQ1dPZ3hIZXJ0UU1Xa1kxcitlbHY5YUtBS29JMEpj?=
 =?utf-8?B?aDFwbWlKSVhhUEp0QnVocGVXWkdteE5ZOERiRVFkSTRJclA2eFY1R2puclVN?=
 =?utf-8?B?emJXR1ArM0FuN0ZOb1dGcW9WZXZTRC9QUFFvQTY1RjVFT0I5clJMNXBXcTNP?=
 =?utf-8?B?Skd2a3pwaEhNbXludXlGWjNCUWZwUGFUOTY2ZWpBeC9QMUh3bStiVSt5MW83?=
 =?utf-8?B?K0xOeklodWxtQ2l5bFhkNTRGRGZhaG52b0RIenR1VU9UcTZXTmxtWnFXbEpT?=
 =?utf-8?B?ZjVjMzFkZXBZYjVQTHI5YmhOZjgwWTExQXpYV2sycUdZYnQrNkIvOVZJM1pv?=
 =?utf-8?B?T2FqKzBZa0tqN1RhSHF4V1ZtRXk1NDZlakNSOWhXWnJZMWZNM2hUM2pKWWNs?=
 =?utf-8?B?RzMvU2gyYWszcHhJLzZVRUd2RW1TblVFSDA0N3pxajI0U2NEbEozSGhqalNq?=
 =?utf-8?B?LzNoWllnZksrU2tsVmpkbXRMU1ZLNUhwbXh3anRCK25QVG16dEVWQzhSY2Jw?=
 =?utf-8?B?K3hVazZkQ1pERkk2Qk5tZjlYdm4rVXpyN1ZONjRUdVovT1pkQVA0MTFVZVNh?=
 =?utf-8?B?MTh0NDBQWmp2a0VUUGJSUS95d05CQ3lGQ3VIUGFXWXpSVUxlWWdTdXhYcnMz?=
 =?utf-8?B?Z0htR2d2RVM5REtHY2drdzNZYlp1em1YQjJzNWpCcU81cUo3N2w0c2lrRkh6?=
 =?utf-8?B?VTEvSDBmK0hYZlB1S1lTWTJMR1dQdlRFLzZvc1RDQ2Q4ZlN4VDYvcUFzMkJu?=
 =?utf-8?B?STZkMDdHaWl3eHZWcXZ1RGQ0WkFtZTFUSVNkNmJndlBXYUxJWFd5T3VsNkhP?=
 =?utf-8?B?cUVmT1NRajdqSXQvWHpkQjJuVGhja0I0ajhxZTc1aDhSRmw2ckE0MW8rNU5y?=
 =?utf-8?B?cTNVRE9PUGlLRFlTWGNRK0d6NjYydkdIekF6Njh4ZlZQUEQreU0rbkg4ZGhj?=
 =?utf-8?B?clc5OGxXYm9mLzRzNVU2N3h4cDBKcGR1UzRYRDlLbjNHQStsNys4akJMWUVx?=
 =?utf-8?B?RVNkeEs1Y25BYnVjcEQvQXhwR3M5aFBjZzg0NGNtZExmWkVrUDd6WEZhTzdw?=
 =?utf-8?B?QUpUTFE0bGI3YUxrbUczazZEdUl3Z0RWS3J5UHRVOFdQZmYvM3I5R0tmZGwz?=
 =?utf-8?B?blVNWFpvYWIwcVZCSUpTVTg4SE14YVQ0WDlsNHdrNmVZQWtoZmVNTWQ2dmtn?=
 =?utf-8?B?YkxVMElLTjg2VXBvYVBmL1hQZ1FSc2Y3c2NWMkdYZXpZYkVyM1hsa2N4cTZQ?=
 =?utf-8?B?aGhQZmlEeGRidjBwaXljR0hIRjhTZzRrMHY3Y3U1eU1UWVZPUUphMHRYR3Jy?=
 =?utf-8?B?aFphVlQ1VVRrRzVhV0VDdCsyL3daQW43cTNQTzhqc3hscks1Y1J5bjBrN0Rt?=
 =?utf-8?B?TXNiem1TVkdvWHFJNDdLL1JnSEsvdDRGVjlMUkoybE1HWTZ6RUg2clprdFpY?=
 =?utf-8?Q?DeKC7oDJvAZA7/dR97/7A/kmXDnOnVPIwRmwvHXugRdi?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: b573563c-8429-41a2-a060-08da7eac0f8b
X-MS-Exchange-CrossTenant-AuthSource: BN8PR12MB3587.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Aug 2022 10:51:08.8973
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 0JcvsjCGdy0ZWL5dwb25N4w0AIDvw5kD2cbqLcN6zhIDKX6e8Irf5d/0TPf3owXI
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR12MB1297
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Am 15.08.22 um 12:47 schrieb Dmitry Osipenko:
> On 8/15/22 13:18, Dmitry Osipenko wrote:
>> On 8/15/22 13:14, Christian König wrote:
>>> Am 15.08.22 um 12:11 schrieb Christian König:
>>>> Am 15.08.22 um 12:09 schrieb Dmitry Osipenko:
>>>>> On 8/15/22 13:05, Christian König wrote:
>>>>>> Am 15.08.22 um 11:54 schrieb Dmitry Osipenko:
>>>>>>> Higher order pages allocated using alloc_pages() aren't refcounted and
>>>>>>> they
>>>>>>> need to be refcounted, otherwise it's impossible to map them by
>>>>>>> KVM. This
>>>>>>> patch sets the refcount of the tail pages and fixes the KVM memory
>>>>>>> mapping
>>>>>>> faults.
>>>>>>>
>>>>>>> Without this change guest virgl driver can't map host buffers into
>>>>>>> guest
>>>>>>> and can't provide OpenGL 4.5 profile support to the guest. The host
>>>>>>> mappings are also needed for enabling the Venus driver using host GPU
>>>>>>> drivers that are utilizing TTM.
>>>>>>>
>>>>>>> Based on a patch proposed by Trigger Huang.
>>>>>> Well I can't count how often I have repeated this: This is an
>>>>>> absolutely
>>>>>> clear NAK!
>>>>>>
>>>>>> TTM pages are not reference counted in the first place and because of
>>>>>> this giving them to virgl is illegal.
>>>>> A? The first page is refcounted when allocated, the tail pages are not.
>>>> No they aren't. The first page is just by coincident initialized with
>>>> a refcount of 1. This refcount is completely ignored and not used at all.
>>>>
>>>> Incrementing the reference count and by this mapping the page into
>>>> some other address space is illegal and corrupts the internal state
>>>> tracking of TTM.
>>> See this comment in the source code as well:
>>>
>>>          /* Don't set the __GFP_COMP flag for higher order allocations.
>>>           * Mapping pages directly into an userspace process and calling
>>>           * put_page() on a TTM allocated page is illegal.
>>>           */
>>>
>>> I have absolutely no idea how somebody had the idea he could do this.
>> I saw this comment, but it doesn't make sense because it doesn't explain
>> why it's illegal. Hence it looks like a bogus comment since the
>> refcouting certainly works, at least to a some degree because I haven't
>> noticed any problems in practice, maybe by luck :)
>>
>> I'll try to dig out the older discussions, thank you for the quick reply!
> Are you sure it was really discussed in public previously? All I can
> find is yours two answers to a similar patches where you're saying that
> this it's a wrong solution without in-depth explanation and further
> discussions.

Yeah, that's my problem as well I can't find that of hand.

But yes it certainly was discussed in public.

>
> Maybe it was discussed privately? In this case I will be happy to get
> more info from you about the root of the problem so I could start to
> look at how to fix it properly. It's not apparent where the problem is
> to a TTM newbie like me.
>

Well this is completely unfixable. See the whole purpose of TTM is to 
allow tracing where what is mapped of a buffer object.

If you circumvent that and increase the page reference yourself than 
that whole functionality can't work correctly any more.

Regards,
Christian.