Re: [PATCH 6/7] RAMBlock: make guest_memfd require coordinate discard

David Hildenbrand
2025-01-20T13:11:50Z

On 14.01.25 02:38, Chenyi Qiang wrote:
>
>
> On 1/13/2025 6:56 PM, David Hildenbrand wrote:
>> On 13.12.24 08:08, Chenyi Qiang wrote:
>>> As guest_memfd is now managed by guest_memfd_manager with
>>> RamDiscardManager, only block uncoordinated discard.
>>>
>>> Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com>
>>> ---
>>>   system/physmem.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/system/physmem.c b/system/physmem.c
>>> index 532182a6dd..585090b063 100644
>>> --- a/system/physmem.c
>>> +++ b/system/physmem.c
>>> @@ -1872,7 +1872,7 @@ static void ram_block_add(RAMBlock *new_block,
>>> Error **errp)
>>>           assert(kvm_enabled());
>>>           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");
>>
>> Would that also unlock virtio-mem by accident?
>
> Hum, that's true. At present, the rdm in MR can only point to one
> instance, thus if we unlock virtio-mem and try to use it with
> guest_memfd, it would trigger assert in
> memory_region_set_ram_discard_manager().
>
> Maybe we need to add some explicit check in virtio-mem to exclude it
> with guest_memfd at present?

Likely we should make memory_region_set_ram_discard_manager() fail if
there is already something, and handle it in the callers?

In case of virtio-mem, we'd have to undo what we did and fail realize().

In case of CC, we'd have to bail out in a different way.

Then, I think if we see new_block->guest_memfd here, that we can assume
that any coordinated discard corresponds to only the guest_memfd one,
not to anything else?

--
Cheers,

David / dhildenb