Re: [PATCH v5 07/10] RAMBlock: Make guest_memfd require coordinate discard

[Chenyi Qiang <chenyi.qiang@intel.com>]

On 5/27/2025 7:20 PM, David Hildenbrand wrote:
> On 27.05.25 07:47, Chenyi Qiang wrote:
>>
>>
>> On 5/26/2025 5:08 PM, David Hildenbrand wrote:
>>> On 20.05.25 12:28, Chenyi Qiang wrote:
>>>> As guest_memfd is now managed by RamBlockAttribute with
>>>> RamDiscardManager, only block uncoordinated discard.
>>>>
>>>> Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com>
>>>> ---
>>>> Changes in v5:
>>>>       - Revert to use RamDiscardManager.
>>>>
>>>> Changes in v4:
>>>>       - Modify commit message (RamDiscardManager-
>>>> >PrivateSharedManager).
>>>>
>>>> Changes in v3:
>>>>       - No change.
>>>>
>>>> Changes in v2:
>>>>       - Change the ram_block_discard_require(false) to
>>>>         ram_block_coordinated_discard_require(false).
>>>> ---
>>>>    system/physmem.c | 6 +++---
>>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/system/physmem.c b/system/physmem.c
>>>> index f05f7ff09a..58b7614660 100644
>>>> --- a/system/physmem.c
>>>> +++ b/system/physmem.c
>>>> @@ -1916,7 +1916,7 @@ static void ram_block_add(RAMBlock *new_block,
>>>> Error **errp)
>>>>            }
>>>>            assert(new_block->guest_memfd < 0);
>>>>    -        ret = ram_block_discard_require(true);
>>>> +        ret = ram_block_coordinated_discard_require(true);
>>>>            if (ret < 0) {
>>>>                error_setg_errno(errp, -ret,
>>>>                                 "cannot set up private guest memory:
>>>> discard currently blocked");
>>>> @@ -1939,7 +1939,7 @@ static void ram_block_add(RAMBlock *new_block,
>>>> Error **errp)
>>>>                 * ever develops a need to check for errors.
>>>>                 */
>>>>                close(new_block->guest_memfd);
>>>> -            ram_block_discard_require(false);
>>>> +            ram_block_coordinated_discard_require(false);
>>>>                qemu_mutex_unlock_ramlist();
>>>>                goto out_free;
>>>>            }
>>>> @@ -2302,7 +2302,7 @@ static void reclaim_ramblock(RAMBlock *block)
>>>>        if (block->guest_memfd >= 0) {
>>>>            ram_block_attribute_destroy(block->ram_shared);
>>>>            close(block->guest_memfd);
>>>> -        ram_block_discard_require(false);
>>>> +        ram_block_coordinated_discard_require(false);
>>>>        }
>>>>          g_free(block);
>>>
>>>
>>> I think this patch should be squashed into the previous one, then the
>>> story in that single patch is consistent.
>>
>> I think this patch is a gate to allow device assignment with guest_memfd
>> and want to make it separately. Can we instead add some commit message
>> in previous one? like:
>>
>> "Using guest_memfd with vfio is still blocked via
>> ram_block_discard_disable()/ram_block_discard_require()."
> 
> For the title it should probably be something like:
> 
> "physmem: support coordinated discarding of RAM with guest_memdfd"
> 
> Then explain how we install the RAMDiscardManager that will notify
> listeners (esp. vfio).

Make sense. Will do the squash and change the title. Thanks!

>