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