Re: [PATCH v4 04/13] memory: Introduce generic state change parent class for RamDiscardManager

[Chenyi Qiang <chenyi.qiang@intel.com>]

On 4/10/2025 8:11 AM, Alexey Kardashevskiy wrote:
> 
> 
> 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.

OK, anyway, I can rename to RamStateManager if we follow this direction.

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

Not restrict to 1 bit per page. As mentioned in questions 2, the parent
class can be more generic, e.g. only including
register/unregister_listener().

Like in this way:

The parent class:

struct StateChangeListener {
    MemoryRegionSection *section;
}

struct RamStateManagerClass {
    void (*register_listener)();
    void (*unregister_listener)();
}

The child class:

1. RamDiscardManager

struct RamDiscardListener {
    StateChangeListener scl;
    NotifyPopulate notify_populate;
    NotifyDiscard notify_discard;
    bool double_discard_supported;

    QLIST_ENTRY(RamDiscardListener) next;
}

struct RamDiscardManagerClass {
    RamStateManagerClass parent_class;
    uint64_t (*get_min_granularity)();
    bool (*is_populate)();
    bool (*replay_populate)();
    bool (*replay_discard)();
}

2. PrivateSharedManager (or other name like ConfidentialRamManager?)

struct PrivateSharedListener {
    StateChangeListener scl;
    NotifyShared notify_shared;
    NotifyPrivate notify_private;
    int priority;

    QLIST_ENTRY(PrivateSharedListener) next;
}

struct PrivateSharedManagerClass {
    RamStateManagerClass parent_class;
    uint64_t (*get_min_granularity)();
    bool (*is_shared)();
    // No need to define replay_private/replay_shared as no use case at
present.
}

In the future, if we want to manage three states, we can only extend
PrivateSharedManagerClass/PrivateSharedListener.

> 
> 
>> 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,

Only renaming RamDiscardManager seems not sufficient. Current
RamDiscardManagerClass can only manage two states. For example, its
callback functions only have the name of xxx_populate and xxx_discard.
If we want to extend it to manage three states, we have to modify those
callbacks, e.g. add some new argument like is_populate(bool is_private),
or define some new callbacks like is_populate_private(). It will make
this class more complicated, but actually not necessary in legacy VMs
without the concept of private/shared.

> 
> 
>>
>>>
>>>
>>>>
>>>> Signed-off-by: Chenyi Qiang <chenyi.qiang@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!
>>
>>>
>>>
>