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

[Alexey Kardashevskiy <aik@amd.com>]

On 16/4/25 13:32, Chenyi Qiang wrote:
> 
> 
> 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?


Sorry it is taking a while, I'll comment after the holidays. It is just a bit hard to follow how we started with just 1 patch and ended up with 13 patches with no clear answer why. Thanks,


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

-- 
Alexey