On 4/18/2025 7:10 AM, Alexey Kardashevskiy wrote:
>
>
> 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,
Have a nice holiday! :)
And sorry for the confusion. I missed to paste the history link for the
motivation of the change
(https://lore.kernel.org/qemu-devel/0ed6faf8-f6f4-4050-994b-2722d2726...@amd.com/)
I think the original RamDiscardManager solution can just work. This
framework change is mainly to facilitate future extension.
>
>
>>
>>>
>>>>
>>>>
>>>>> 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.
>>>
>>
>>
>
