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

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

On 4/10/2025 9:44 AM, Chenyi Qiang wrote:
> 
> 
> 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.

Hi Alexey & David,

Any thoughts on this proposal?

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