On 9/4/25 22:57, Chenyi Qiang wrote:
On 4/9/2025 5:56 PM, Alexey Kardashevskiy wrote:
On 7/4/25 17:49, Chenyi Qiang wrote:
RamDiscardManager is an interface used by virtio-mem to adjust VFIO
mappings in relation to VM page assignment. It manages the state of
populated and discard for the RAM. To accommodate future scnarios for
managing RAM states, such as private and shared states in confidential
VMs, the existing RamDiscardManager interface needs to be generalized.
Introduce a parent class, GenericStateManager, to manage a pair of
"GenericState" is the same as "State" really. Call it RamStateManager.
OK to me.
Sorry, nah. "Generic" would mean "machine" in QEMU.
opposite states with RamDiscardManager as its child. The changes include
- Define a new abstract class GenericStateChange.
- Extract six callbacks into GenericStateChangeClass and allow the child
classes to inherit them.
- Modify RamDiscardManager-related helpers to use GenericStateManager
ones.
- Define a generic StatChangeListener to extract fields from
"e" missing in StateChangeListener.
Fixed. Thanks.
RamDiscardManager listener which allows future listeners to embed it
and avoid duplication.
- Change the users of RamDiscardManager (virtio-mem, migration, etc.) to
switch to use GenericStateChange helpers.
It can provide a more flexible and resuable framework for RAM state
management, facilitating future enhancements and use cases.
I fail to see how new interface helps with this. RamDiscardManager
manipulates populated/discarded. It would make sense may be if the new
class had more bits per page, say private/shared/discarded but it does
not. And PrivateSharedManager cannot coexist with RamDiscard. imho this
is going in a wrong direction.
I think we have two questions here:
1. whether we should define an abstract parent class and distinguish the
RamDiscardManager and PrivateSharedManager?
If it is 1 bit per page with the meaning "1 == populated == shared",
then no, one class will do.
I vote for this. First, After making the distinction, the
PrivateSharedManager won't go into the RamDiscardManager path which
PrivateSharedManager may have not supported yet. e.g. the migration
related path. In addtional, we can extend the PrivateSharedManager for
specific handling, e.g. the priority listener, state_change() callback.
2. How we should abstract the parent class?
I think this is the problem. My current implementation extracts all the
callbacks in RamDiscardManager into the parent class and call them
state_set and state_clear, which can only manage a pair of opposite
states. As you mentioned, there could be private/shared/discarded three
states in the future, which is not compatible with current design. Maybe
we can make the parent class more generic, e.g. only extract the
register/unregister_listener() into it.
Or we could rename RamDiscardManager to RamStateManager, implement 2bit
per page (0 = discarded, 1 = populated+shared, 2 = populated+private).
Eventually we will have to deal with the mix of private and shared
mappings for the same device, how 1 bit per page is going to work? Thanks,
Signed-off-by: Chenyi Qiang <chenyi.qi...@intel.com>
---
Changes in v4:
- Newly added.
---
hw/vfio/common.c | 30 ++--
hw/virtio/virtio-mem.c | 95 ++++++------
include/exec/memory.h | 313 ++++++++++++++++++++++------------------
migration/ram.c | 16 +-
system/memory.c | 106 ++++++++------
system/memory_mapping.c | 6 +-
6 files changed, 310 insertions(+), 256 deletions(-)
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index f7499a9b74..3172d877cc 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -335,9 +335,10 @@ out:
rcu_read_unlock();
}
-static void vfio_ram_discard_notify_discard(RamDiscardListener *rdl,
+static void vfio_ram_discard_notify_discard(StateChangeListener *scl,
MemoryRegionSection
*section)
{
+ RamDiscardListener *rdl = container_of(scl, RamDiscardListener,
scl);
VFIORamDiscardListener *vrdl = container_of(rdl,
VFIORamDiscardListener,
listener);
VFIOContainerBase *bcontainer = vrdl->bcontainer;
@@ -353,9 +354,10 @@ static void
vfio_ram_discard_notify_discard(RamDiscardListener *rdl,
}
}
-static int vfio_ram_discard_notify_populate(RamDiscardListener *rdl,
+static int vfio_ram_discard_notify_populate(StateChangeListener *scl,
MemoryRegionSection
*section)
{
+ RamDiscardListener *rdl = container_of(scl, RamDiscardListener,
scl);
VFIORamDiscardListener *vrdl = container_of(rdl,
VFIORamDiscardListener,
listener);
VFIORamDiscardListener *vrdl = container_of(scl, VFIORamDiscardListener,
listener.scl) and drop @ rdl? Thanks,
Modified. Thanks!
--
Alexey
