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

[David Hildenbrand <david@redhat.com>]

On 21.01.25 07:26, Chenyi Qiang wrote:
> 
> 
> On 1/20/2025 9:11 PM, David Hildenbrand wrote:
>> 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?
> 
> LGTM. In case of CC, I think we can also check the
> memory_region_set_ram_discard_manager() failure, undo what we did and
> make the ram_block_add() fail (set errno).

As we have memory_region_has_ram_discard_manager(), we could also check 
that instead of failing memory_region_set_ram_discard_manager().

But failing memory_region_set_ram_discard_manager() will force everybody 
to handle that, so it might be the better choice.

Of course, setting it to "NULL" should be guaranteed to never fail.

-- 
Cheers,

David / dhildenb