Re: [PATCH v4 05/13] memory: Introduce PrivateSharedManager Interface as child of GenericStateManager

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

On 4/9/2025 5:56 PM, Alexey Kardashevskiy wrote:
> 
> 
> On 7/4/25 17:49, Chenyi Qiang wrote:
>> To manage the private and shared RAM states in confidential VMs,
>> introduce a new class of PrivateShareManager as a child of
> 
> missing "d" in "PrivateShareManager"

Fixed. Thanks!

> 
> 
>> GenericStateManager, which inherits the six interface callbacks. With a
>> different interface type, it can be distinguished from the
>> RamDiscardManager object and provide the flexibility for addressing
>> specific requirements of confidential VMs in the future.
> 
> This is still one bit per page, right? What does "set" mean here -
> private or shared? It is either RamPrivateManager or RamSharedManager imho.

This series only allows one bit per page, let's continue the discussion
for this in patch 04/13.

"Set" just mean the bitmap set. Maybe rename to RamPrivateManager
(corresponding to RamDiscardManager) or CVMRamManager (Confidential VM
Ram Manager) if we want to introduce more states in the future.

> 
> 
>> Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com>
>> ---
>> Changes in v4:
>>      - Newly added.
>> ---
>>   include/exec/memory.h | 44 +++++++++++++++++++++++++++++++++++++++++--
>>   system/memory.c       | 17 +++++++++++++++++
>>   2 files changed, 59 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/exec/memory.h b/include/exec/memory.h
>> index 30e5838d02..08f25e5e84 100644
>> --- a/include/exec/memory.h
>> +++ b/include/exec/memory.h
>> @@ -55,6 +55,12 @@ typedef struct RamDiscardManager RamDiscardManager;
>>   DECLARE_OBJ_CHECKERS(RamDiscardManager, RamDiscardManagerClass,
>>                        RAM_DISCARD_MANAGER, TYPE_RAM_DISCARD_MANAGER);
>>   +#define TYPE_PRIVATE_SHARED_MANAGER "private-shared-manager"
>> +typedef struct PrivateSharedManagerClass PrivateSharedManagerClass;
>> +typedef struct PrivateSharedManager PrivateSharedManager;
>> +DECLARE_OBJ_CHECKERS(PrivateSharedManager, PrivateSharedManagerClass,
>> +                     PRIVATE_SHARED_MANAGER,
>> TYPE_PRIVATE_SHARED_MANAGER)
>> +
>>   #ifdef CONFIG_FUZZ
>>   void fuzz_dma_read_cb(size_t addr,
>>                         size_t len,
>> @@ -692,6 +698,14 @@ void
>> generic_state_manager_register_listener(GenericStateManager *gsm,
>>   void generic_state_manager_unregister_listener(GenericStateManager
>> *gsm,
>>                                                  StateChangeListener
>> *scl);
>>   +static inline void state_change_listener_init(StateChangeListener
>> *scl,
>> +                                              NotifyStateSet
>> state_set_fn,
>> +                                              NotifyStateClear
>> state_clear_fn)
> 
> This belongs to 04/13 as there is nothing about PrivateSharedManager.
> Thanks,

Will move it. Thanks.

> 
> 
>> +{
>> +    scl->notify_to_state_set = state_set_fn;
>> +    scl->notify_to_state_clear = state_clear_fn;
>> +}
>> +
>>   typedef struct RamDiscardListener RamDiscardListener;
>>     struct RamDiscardListener {
>> @@ -713,8 +727,7 @@ static inline void
>> ram_discard_listener_init(RamDiscardListener *rdl,
>>                                                NotifyStateClear
>> discard_fn,
>>                                                bool
>> double_discard_supported)
>>   {
>> -    rdl->scl.notify_to_state_set = populate_fn;
>> -    rdl->scl.notify_to_state_clear = discard_fn;
>> +    state_change_listener_init(&rdl->scl, populate_fn, discard_fn);
>>       rdl->double_discard_supported = double_discard_supported;
>>   }
>>   @@ -757,6 +770,25 @@ struct RamDiscardManagerClass {
>>       GenericStateManagerClass parent_class;
>>   };
>>   +typedef struct PrivateSharedListener PrivateSharedListener;
>> +struct PrivateSharedListener {
>> +    struct StateChangeListener scl;
>> +
>> +    QLIST_ENTRY(PrivateSharedListener) next;
>> +};
>> +
>> +struct PrivateSharedManagerClass {
>> +    /* private */
>> +    GenericStateManagerClass parent_class;
>> +};
>> +
>> +static inline void private_shared_listener_init(PrivateSharedListener
>> *psl,
>> +                                                NotifyStateSet
>> populate_fn,
>> +                                                NotifyStateClear
>> discard_fn)
>> +{
>> +    state_change_listener_init(&psl->scl, populate_fn, discard_fn);
>> +}
>> +
>>   /**
>>    * memory_get_xlat_addr: Extract addresses from a TLB entry
>>    *
>> @@ -2521,6 +2553,14 @@ int
>> memory_region_set_generic_state_manager(MemoryRegion *mr,
>>    */
>>   bool memory_region_has_ram_discard_manager(MemoryRegion *mr);
>>   +/**
>> + * memory_region_has_private_shared_manager: check whether a
>> #MemoryRegion has a
>> + * #PrivateSharedManager assigned
>> + *
>> + * @mr: the #MemoryRegion
>> + */
>> +bool memory_region_has_private_shared_manager(MemoryRegion *mr);
>> +
>>   /**
>>    * memory_region_find: translate an address/size relative to a
>>    * MemoryRegion into a #MemoryRegionSection.
>> diff --git a/system/memory.c b/system/memory.c
>> index 7b921c66a6..e6e944d9c0 100644
>> --- a/system/memory.c
>> +++ b/system/memory.c
>> @@ -2137,6 +2137,16 @@ bool
>> memory_region_has_ram_discard_manager(MemoryRegion *mr)
>>       return true;
>>   }
>>   +bool memory_region_has_private_shared_manager(MemoryRegion *mr)
>> +{
>> +    if (!memory_region_is_ram(mr) ||
>> +        !object_dynamic_cast(OBJECT(mr->gsm),
>> TYPE_PRIVATE_SHARED_MANAGER)) {
>> +        return false;
>> +    }
>> +
>> +    return true;
>> +}
>> +
>>   uint64_t generic_state_manager_get_min_granularity(const
>> GenericStateManager *gsm,
>>                                                      const
>> MemoryRegion *mr)
>>   {
>> @@ -3837,12 +3847,19 @@ static const TypeInfo ram_discard_manager_info
>> = {
>>       .class_size         = sizeof(RamDiscardManagerClass),
>>   };
>>   +static const TypeInfo private_shared_manager_info = {
>> +    .parent             = TYPE_GENERIC_STATE_MANAGER,
>> +    .name               = TYPE_PRIVATE_SHARED_MANAGER,
>> +    .class_size         = sizeof(PrivateSharedManagerClass),
>> +};
>> +
>>   static void memory_register_types(void)
>>   {
>>       type_register_static(&memory_region_info);
>>       type_register_static(&iommu_memory_region_info);
>>       type_register_static(&generic_state_manager_info);
>>       type_register_static(&ram_discard_manager_info);
>> +    type_register_static(&private_shared_manager_info);
>>   }
>>     type_init(memory_register_types)
>