Received-SPF: pass (google.com: domain of linux-kernel+bounces-101627-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <0b109bc4-ee4c-4f13-996f-b89fbee09c0b@amd.com>
Date: Wed, 13 Mar 2024 15:37:21 +0100
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v11 0/8] KVM: allow mapping non-refcounted pages
Content-Language: en-US
To: Sean Christopherson <seanjc@google.com>
Cc: David Stevens <stevensd@chromium.org>,
 Christoph Hellwig <hch@infradead.org>, Paolo Bonzini <pbonzini@redhat.com>,
 Yu Zhang <yu.c.zhang@linux.intel.com>,
 Isaku Yamahata <isaku.yamahata@gmail.com>,
 Zhi Wang <zhi.wang.linux@gmail.com>, Maxim Levitsky <mlevitsk@redhat.com>,
 kvmarm@lists.linux.dev, linux-kernel@vger.kernel.org, kvm@vger.kernel.org
References: <20240229025759.1187910-1-stevensd@google.com>
 <ZeCIX5Aw5s1L0YEh@infradead.org>
 <CAD=HUj7fT2CVXLfi5mty0rSzpG_jK9fhcKYGQnTf_H8Hg-541Q@mail.gmail.com>
 <72285e50-6ffc-4f24-b97b-8c381b1ddf8e@amd.com> <ZfGrS4QS_WhBWiDl@google.com>
From: =?UTF-8?Q?Christian_K=C3=B6nig?= <christian.koenig@amd.com>
In-Reply-To: <ZfGrS4QS_WhBWiDl@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?alFYMWFzbUdNdVR6cm84OFV3d01GSldVWnQraVhpeFFmWElTazhYY3NyMGwz?=
 =?utf-8?B?NlJiMktPeFNxU2RsaXcrSnU2T1VuZTBPZW9Lc2VCM3g4QXRtZ0pYN3daS1pR?=
 =?utf-8?B?VGRzczdKaGNqaExmNW5ya1pKSGpzNUxETHp0amVCa3ZVYkxuTG1DSnhXU1c1?=
 =?utf-8?B?WkI0RUpVWXdYajA4akhOc1NSdkpIVytQektSMUxoOWJJK2V2RlpMUUs1T0lx?=
 =?utf-8?B?V1Z3NmQvSEpwNFpDc29ZaEQ1S1N2K1BpamNMRjU2cnB4eWE1ZmpyWlZ2blI5?=
 =?utf-8?B?V0dkcmJxeS9ZdGtIUGw0c1gySWFUY3A0Y282WFZXRHBUVlR4Vjg0Y1JwL21m?=
 =?utf-8?B?WXc1QmVKZG5MR1ZkZFdOWGNwVGNNcnlYQ1VJc09rcVhNQ2VRNytRQ0dEeDZk?=
 =?utf-8?B?L0R2SXJXVk8rZ1Q4WXdBdHF6VXB2SVFZM0VrNTcrRmVMdmpndnF1WkoxYzBj?=
 =?utf-8?B?L3pqZ2ErU3czTk5oUEhSYkkwbDBxbTl4WHVFUXJrZ1RBWUdpeWkrY2I4ZSto?=
 =?utf-8?B?VTRPNW9SNVJFUWNTazVuVnFxTnV3N2cvVXMzWldkcjRuUDhLWEdadGc3VllP?=
 =?utf-8?B?Z0I5cCtGb0hZRHJ4UktZeEdXWlltQXVUeDZSZVRsLzJYZjhzV3FHTWNSQkJ4?=
 =?utf-8?B?R1VZYXRlR3F3U3dZUmZMU0xxUTI4NmRTNDZXcDZqbWRieUtPMUVCOUc2ZzJF?=
 =?utf-8?B?R1gwaVh5U2VuZ0VOenNYN3NUUjZQbDdySjFDeTM0czlkVUlibmhSU1FyLy94?=
 =?utf-8?B?MUYxa09SM2xLaXpQQkN5bXhQQXNSYUNZREprOTY4emxJY1h4SUVQUGpxUlFD?=
 =?utf-8?B?WlIrNWhkWFQxSEpqanJ0ZVU5d25aWWJsYjlhRDIwdUpLTjdBSVZDNThsSWo0?=
 =?utf-8?B?UTBFakFwM0NlRExmVDlBUGpaNGwxRWcvenhZb1V5OVhIZ1BEMnZNOWFXOG1F?=
 =?utf-8?B?aDA4MEN0Q2Rma0RsTjhjbUgwMENXbjFrVXVtU0tRMDFNRndYeDdkNmhjNTVT?=
 =?utf-8?B?WTVQY1RpR0dQbFpJTms2WkRHUG56TUpSN2Z1THdpMWIxajJWNUxoMUcvOGhr?=
 =?utf-8?B?SzZtYVV0OWVOWVlkVlJKVnpZMTNreTBPc2xlOUFnaTZqdzFZMDlVOFkvS21y?=
 =?utf-8?B?RFdRaEtxQkR3T0dtck1mR3BhT3E2R3VwUHZKemkzMlRzSEVvS3VkbmJDLzBn?=
 =?utf-8?B?NWJQa3JIUXVtWTVTNy9uNUZ0Ry96TTRvTTNXeGdKZ0RyT01aSmc4Q1QxMCtV?=
 =?utf-8?B?cWt2RW9wS3RsRUQ5djVsYlA3ZjU1TzduanhTL3hsTWFXNUNJNmY4NVVWSGF6?=
 =?utf-8?B?ZXlVcDZTN3NIS05hTXA5U201Qk9zS2VMYkI1aTIzRUxRay8rR1NrWGJwV1VM?=
 =?utf-8?B?VnBmVkpvRUQ4cnpQNjkrY21kNmZBMVJ4QzdNUWdYTVBzUUVzVC9ReEZUNEdv?=
 =?utf-8?B?Rlk5NmFxTEFYckdHUmt1b2tHSml1VldoSXlKdUdSZnFKeWVQNlJrUU5tMlNW?=
 =?utf-8?B?N1ZCcEJGMWJtVnIwemhhUHhlMTZlcTQvUHBiRkppZnI2NlE5SWRLTUlrWDVI?=
 =?utf-8?B?OG8wV3FWejNFZlZ0WVpqMDVSQ2tPVHg0VzJ6WXhxM2dUM0hmdEx1eENxYm5u?=
 =?utf-8?B?cFFmTyt1KzQ0bzRnZWRBM1JNeXhUYVAxQmszWFprQ0hpR3l2eTFXYnRLMW9k?=
 =?utf-8?B?SFQwcm5pUUZDQ2Jpa2NSdjk5MXg1bnphR2J4bUI5cDVxYVJDYjJHNWNrWG9J?=
 =?utf-8?B?MzdBN2p3NXJHdDNtVDg1R3hNSSs1S1pUSmtjMDYyTnVEWnlKOHZlMjNGV0VV?=
 =?utf-8?B?YWdFVzlhNzlUeG1qaDRuNVN5WXJhcEFqUDZNMzJoelJCV0NGODRZM0xKVXha?=
 =?utf-8?B?NDV4dlduWXczdEtocE5RNDlDc3IxeUFZTzNQc08wV0xiN0NjcFVJRG8raWpH?=
 =?utf-8?B?Y2hoQ1ZsRFVjc2lyckp6Y2xvcjZaL0ZaWStTUnowdURxMmlDNWZrNXRoZVBn?=
 =?utf-8?B?eDdRR2dsSmJCSEQ1QWVmNjdVYi96VVlCNEJxQ3FsaVRvN0hqRnBGbDhxOGVK?=
 =?utf-8?B?MCtWalpmc1lYSldqQ3lkZDhZbEF5WENSdzlWRVNqWlZKNngzNE9MQ3VHU2NH?=
 =?utf-8?Q?tJy+weRegPp8pbEZxF+LlBwa1?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 91f45481-c2f7-452e-38f1-08dc436b1bb5
X-MS-Exchange-CrossTenant-AuthSource: SJ0PR12MB5673.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Mar 2024 14:37:28.8136
 (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: 0BBiRzJLUgkJaE0kD2znBkQB1Txgqy9tUw7DBOzVZRKL9becUSTrP7Ooqk9ckmeH
X-MS-Exchange-Transport-CrossTenantHeadersStamped: LV3PR12MB9186

Am 13.03.24 um 14:34 schrieb Sean Christopherson:
> On Wed, Mar 13, 2024, Christian König wrote:
>> Am 13.03.24 um 05:55 schrieb David Stevens:
>>> On Thu, Feb 29, 2024 at 10:36 PM Christoph Hellwig <hch@infradead.org> wrote:
>>>> On Thu, Feb 29, 2024 at 11:57:51AM +0900, David Stevens wrote:
>>>>> Our use case is virtio-gpu blob resources [1], which directly map host
>>>>> graphics buffers into the guest as "vram" for the virtio-gpu device.
>>>>> This feature currently does not work on systems using the amdgpu driver,
>>>>> as that driver allocates non-compound higher order pages via
>>>>> ttm_pool_alloc_page().
>>>> .. and just as last time around that is still the problem that needs
>>>> to be fixed instead of creating a monster like this to map
>>>> non-refcounted pages.
>>>>
>>> Patches to amdgpu to have been NAKed [1] with the justification that
>>> using non-refcounted pages is working as intended and KVM is in the
>>> wrong for wanting to take references to pages mapped with VM_PFNMAP
>>> [2].
>>>
>>> The existence of the VM_PFNMAP implies that the existence of
>>> non-refcounted pages is working as designed. We can argue about
>>> whether or not VM_PFNMAP should exist, but until VM_PFNMAP is removed,
>>> KVM should be able to handle it. Also note that this is not adding a
>>> new source of non-refcounted pages, so it doesn't make removing
>>> non-refcounted pages more difficult, if the kernel does decide to go
>>> in that direction.
>> Well, the meaning of VM_PFNMAP is that you should not touch the underlying
>> struct page the PTE is pointing to. As far as I can see this includes
>> grabbing a reference count.
>>
>> But that isn't really the problem here. The issue is rather that KVM assumes
>> that by grabbing a reference count to the page that the driver won't change
>> the PTE to point somewhere else.. And that is simply not true.
> No, KVM doesn't assume that.
>
>> So what KVM needs to do is to either have an MMU notifier installed so that
>> updates to the PTEs on the host side are reflected immediately to the PTEs
>> on the guest side.
> KVM already has an MMU notifier and reacts accordingly.
>
>> Or (even better) you use hardware functionality like nested page tables so
>> that we don't actually need to update the guest PTEs when the host PTEs
>> change.
> That's not how stage-2 page tables work.
>
>> And when you have either of those two functionalities the requirement to add
>> a long term reference to the struct page goes away completely. So when this
>> is done right you don't need to grab a reference in the first place.
> The KVM issue that this series is solving isn't that KVM grabs a reference, it's
> that KVM assumes that any non-reserved pfn that is backed by "struct page" is
> refcounted.

Well why does it assumes that? When you have a MMU notifier that seems 
unnecessary.

> What Christoph is objecting to is that, in this series, KVM is explicitly adding
> support for mapping non-compound (huge)pages into KVM guests.  David is arguing
> that Christoph's objection to _KVM_ adding support is unfair, because the real
> problem is that the kernel already maps such pages into host userspace.  I.e. if
> the userspace mapping ceases to exist, then there are no mappings for KVM to follow
> and propagate to KVM's stage-2 page tables.

And I have to agree with Christoph that this doesn't make much sense. 
KVM should *never* map (huge) pages from VMAs marked with VM_PFNMAP into 
KVM guests in the first place.

What it should do instead is to mirror the PFN from the host page tables 
into the guest page tables. If there is a page behind that or not *must* 
be completely irrelevant to KVM.

The background here is that drivers are modifying the page table on the 
fly to point to either MMIO or real memory, this also includes switching 
the caching attributes.

The real question is why is KVM trying to grab a page reference when 
there is an MMU notifier installed.

Regards,
Christian.