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@xxxxxxxxx>
>>>> ---
>>>> 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).
>
