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

> 
> Best regards,
> Pankaj
>