Re: [PATCH v3 4/7] memory-attribute-manager: Introduce MemoryAttributeManager to manage RAMBLock with guest_memfd

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

On 3/19/2025 7:56 PM, Gupta, Pankaj wrote:
> On 3/19/2025 12:23 PM, Chenyi Qiang wrote:
>>
>>
>> On 3/19/2025 4:55 PM, Gupta, Pankaj wrote:
>>>
>>>>>>>>> As the commit 852f0048f3 ("RAMBlock: make guest_memfd require
>>>>>>>>> uncoordinated discard") highlighted, some subsystems like VFIO may
>>>>>>>>> disable ram block discard. However, guest_memfd relies on the
>>>>>>>>> discard
>>>>>>>>> operation to perform page conversion between private and shared
>>>>>>>>> memory.
>>>>>>>>> This can lead to stale IOMMU mapping issue when assigning a
>>>>>>>>> hardware
>>>>>>>>> device to a confidential VM via shared memory. To address this,
>>>>>>>>> it is
>>>>>>>>> crucial to ensure systems like VFIO refresh its IOMMU mappings.
>>>>>>>>>
>>>>>>>>> RamDiscardManager is an existing concept (used by virtio-mem) to
>>>>>>>>> adjust
>>>>>>>>> VFIO mappings in relation to VM page assignment. Effectively page
>>>>>>>>> conversion is similar to hot-removing a page in one mode and
>>>>>>>>> adding it
>>>>>>>>> back in the other. Therefore, similar actions are required for
>>>>>>>>> page
>>>>>>>>> conversion events. Introduce the RamDiscardManager to
>>>>>>>>> guest_memfd to
>>>>>>>>> facilitate this process.
>>>>>>>>>
>>>>>>>>> Since guest_memfd is not an object, it cannot directly
>>>>>>>>> implement the
>>>>>>>>> RamDiscardManager interface. One potential attempt is to implement
>>>>>>>>> it in
>>>>>>>>> HostMemoryBackend. This is not appropriate because guest_memfd is
>>>>>>>>> per
>>>>>>>>> RAMBlock. Some RAMBlocks have a memory backend but others do
>>>>>>>>> not. In
>>>>>>>>> particular, the ones like virtual BIOS calling
>>>>>>>>> memory_region_init_ram_guest_memfd() do not.
>>>>>>>>>
>>>>>>>>> To manage the RAMBlocks with guest_memfd, define a new object
>>>>>>>>> named
>>>>>>>>> MemoryAttributeManager to implement the RamDiscardManager
>>>>>>>>> interface. The
>>>>>>>>
>>>>>>>> Isn't this should be the other way around. 'MemoryAttributeManager'
>>>>>>>> should be an interface and RamDiscardManager a type of it, an
>>>>>>>> implementation?
>>>>>>>
>>>>>>> We want to use 'MemoryAttributeManager' to represent RAMBlock to
>>>>>>> implement the RamDiscardManager interface callbacks because
>>>>>>> RAMBlock is
>>>>>>> not an object. It includes some metadata of guest_memfd like
>>>>>>> shared_bitmap at the same time.
>>>>>>>
>>>>>>> I can't get it that make 'MemoryAttributeManager' an interface and
>>>>>>> RamDiscardManager a type of it. Can you elaborate it a little bit? I
>>>>>>> think at least we need someone to implement the RamDiscardManager
>>>>>>> interface.
>>>>>>
>>>>>> shared <-> private is translated (abstracted) to "populated <->
>>>>>> discarded", which makes sense. The other way around would be wrong.
>>>>>>
>>>>>> It's going to be interesting once we have more logical states, for
>>>>>> example supporting virtio-mem for confidential VMs.
>>>>>>
>>>>>> Then we'd have "shared+populated, private+populated, shared+discard,
>>>>>> private+discarded". Not sure if this could simply be achieved by
>>>>>> allowing multiple RamDiscardManager that are effectively chained, or
>>>>>> if we'd want a different interface.
>>>>>
>>>>> Exactly! In any case generic manager (parent class) would make more
>>>>> sense that can work on different operations/states implemented in
>>>>> child
>>>>> classes (can be chained as well).
>>>>
>>>> Ah, we are talking about the generic state management. Sorry for my
>>>> slow
>>>> reaction.
>>>>
>>>> So we need to
>>>> 1. Define a generic manager Interface, e.g.
>>>> MemoryStateManager/GenericStateManager.
>>>> 2. Make RamDiscardManager the child of MemoryStateManager which manages
>>>> the state of populated and discarded.
>>>> 3. Define a new child manager Interface PrivateSharedManager which
>>>> manages the state of private and shared.
>>>> 4. Define a new object ConfidentialMemoryAttribute to implement the
>>>> PrivateSharedManager interface.
>>>> (Welcome to rename the above Interface/Object)
>>>>
>>>> Is my understanding correct?
>>>
>>> Yes, in that direction. Where 'RamDiscardManager' &
>>> 'PrivateSharedManager' are both child of 'GenericStateManager'.
>>>
>>> Depending on listeners registered, corresponding handlers can be called.
>>
>> Yes, it would be more generic and future extensive.
>>
>> Do we need to add this framework change directly? Or keep the current
>> structure (abstract private/shared as discard/populated) and add the
>> generic manager until the real case like virtio-mem for confidential VMs.
>>
> 
> Yes, maybe to start with we should add new (discard/populated) changes
> with the new framework.
> 
> In future the current framework can be extended for in-place conversion
> for private-shared conversion (if require userspace help) and virtio-mem
> like interfaces. Important is to have proper hierarchy with base bits
> there.

Thanks. Then I will follow this direction.

To abstract the common parent class, what I can think of is to abstract
it to manage a pair of opposite states (state set and state clear, like
populate and discard) and define some similar common callbacks like
notify_set() and notify_clear(), as long as we don't use it to manage
more than two states in the future. Otherwise I may define a stub parent
class.

> 
> Thanks,
> Pankaj