Re: [PATCH v2 4/6] memory-attribute-manager: Introduce a callback to notify the shared/private state change

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

On 2/18/2025 5:19 PM, Alexey Kardashevskiy wrote:
> 
> 
> On 17/2/25 19:18, Chenyi Qiang wrote:
>> Introduce a new state_change() callback in MemoryAttributeManagerClass to
>> efficiently notify all registered RamDiscardListeners, including VFIO
>> listeners about the memory conversion events in guest_memfd. The
>> existing VFIO listener can dynamically DMA map/unmap the shared pages
>> based on conversion types:
>> - For conversions from shared to private, the VFIO system ensures the
>>    discarding of shared mapping from the IOMMU.
>> - For conversions from private to shared, it triggers the population of
>>    the shared mapping into the IOMMU.
>>
>> Additionally, there could be some special conversion requests:
>> - When a conversion request is made for a page already in the desired
>>    state, the helper simply returns success.
>> - For requests involving a range partially in the desired state, only
>>    the necessary segments are converted, ensuring the entire range
>>    complies with the request efficiently.
>> - In scenarios where a conversion request is declined by other systems,
>>    such as a failure from VFIO during notify_populate(), the helper will
>>    roll back the request, maintaining consistency.
>>
>> Opportunistically introduce a helper to trigger the state_change()
>> callback of the class.
>>
>> Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com>
>> ---
>> Changes in v2:
>>      - Do the alignment changes due to the rename to
>> MemoryAttributeManager
>>      - Move the state_change() helper definition in this patch.
>> ---
>>   include/system/memory-attribute-manager.h |  20 +++
>>   system/memory-attribute-manager.c         | 148 ++++++++++++++++++++++
>>   2 files changed, 168 insertions(+)
>>
>> diff --git a/include/system/memory-attribute-manager.h b/include/
>> system/memory-attribute-manager.h
>> index 72adc0028e..c3dab4e47b 100644
>> --- a/include/system/memory-attribute-manager.h
>> +++ b/include/system/memory-attribute-manager.h
>> @@ -34,8 +34,28 @@ struct MemoryAttributeManager {
>>     struct MemoryAttributeManagerClass {
>>       ObjectClass parent_class;
>> +
>> +    int (*state_change)(MemoryAttributeManager *mgr, uint64_t offset,
>> uint64_t size,
>> +                        bool shared_to_private);
>>   };
>>   +static inline int
>> memory_attribute_manager_state_change(MemoryAttributeManager *mgr,
>> uint64_t offset,
>> +                                                        uint64_t
>> size, bool shared_to_private)
>> +{
>> +    MemoryAttributeManagerClass *klass;
>> +
>> +    if (mgr == NULL) {
>> +        return 0;
>> +    }
>> +
>> +    klass = MEMORY_ATTRIBUTE_MANAGER_GET_CLASS(mgr);
>> +    if (klass->state_change) {
>> +        return klass->state_change(mgr, offset, size,
>> shared_to_private);
>> +    }
>> +
>> +    return 0;
> 
> 
> nit: MemoryAttributeManagerClass without this only callback defined
> should produce some error imho. Or assert.

Nice catch. Will return error if !klass->state_change.

> 
>> +}
>> +
>>   int memory_attribute_manager_realize(MemoryAttributeManager *mgr,
>> MemoryRegion *mr);
>>   void memory_attribute_manager_unrealize(MemoryAttributeManager *mgr);

[..]

>> +
>> +static bool
>> memory_attribute_is_range_discarded(MemoryAttributeManager *mgr,
>> +                                                uint64_t offset,
>> uint64_t size)
>> +{
>> +    int block_size = memory_attribute_manager_get_block_size(mgr);
>> +    const unsigned long first_bit = offset / block_size;
>> +    const unsigned long last_bit = first_bit + (size / block_size) - 1;
>> +    unsigned long found_bit;
>> +
>> +    /* We fake a shorter bitmap to avoid searching too far. */
> 
> Weird comment imho, why is it a "fake"? You check if all pages within
> [offset, offset+size) are discarded. You do not want to search beyond
> the end of this range anyway, right?

Yes. And I think the "fake" is aimed to describe the inconsistency with
the definition of find_next_bit(). find_next_bit() defines the second
argument as "The bitmap size in bits" but "last_bit + 1" is not the size
of shared_bitmap.

> 
>> +    found_bit = find_next_bit(mgr->shared_bitmap, last_bit + 1,
>> first_bit);
>> +    return found_bit > last_bit;
>> +}
>> +
>> +static int memory_attribute_state_change(MemoryAttributeManager *mgr,
>> uint64_t offset,
>> +                                         uint64_t size, bool
>> shared_to_private)
> 
> Elsewhere it is just "to_private".

I'm OK to change it to "to_private" to keep alignment.

> 
>> +{
>> +    int block_size = memory_attribute_manager_get_block_size(mgr);
>> +    int ret = 0;
>> +
>> +    if (!memory_attribute_is_valid_range(mgr, offset, size)) {
>> +        error_report("%s, invalid range: offset 0x%lx, size 0x%lx",
>> +                     __func__, offset, size);
>> +        return -1;
>> +    }
>> +
>> +    if ((shared_to_private &&
>> memory_attribute_is_range_discarded(mgr, offset, size)) ||
>> +        (!shared_to_private &&
>> memory_attribute_is_range_populated(mgr, offset, size))) {
>> +        return 0;
>> +    }
>> +
>> +    if (shared_to_private) {
>> +        memory_attribute_notify_discard(mgr, offset, size);
>> +    } else {
>> +        ret = memory_attribute_notify_populate(mgr, offset, size);
>> +    }
>> +
>> +    if (!ret) {
>> +        unsigned long first_bit = offset / block_size;
>> +        unsigned long nbits = size / block_size;
>> +
>> +        g_assert((first_bit + nbits) <= mgr->bitmap_size);
>> +
>> +        if (shared_to_private) {
>> +            bitmap_clear(mgr->shared_bitmap, first_bit, nbits);
>> +        } else {
>> +            bitmap_set(mgr->shared_bitmap, first_bit, nbits);
>> +        }
>> +
>> +        return 0;
> 
> Do not need this return. Thanks,

Removed. Thanks!

> 
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>>   int memory_attribute_manager_realize(MemoryAttributeManager *mgr,
>> MemoryRegion *mr)
>>   {
>>       uint64_t bitmap_size;
>> @@ -281,8 +426,11 @@ static void
>> memory_attribute_manager_finalize(Object *obj)
>>     static void memory_attribute_manager_class_init(ObjectClass *oc,
>> void *data)
>>   {
>> +    MemoryAttributeManagerClass *mamc =
>> MEMORY_ATTRIBUTE_MANAGER_CLASS(oc);
>>       RamDiscardManagerClass *rdmc = RAM_DISCARD_MANAGER_CLASS(oc);
>>   +    mamc->state_change = memory_attribute_state_change;
>> +
>>       rdmc->get_min_granularity =
>> memory_attribute_rdm_get_min_granularity;
>>       rdmc->register_listener = memory_attribute_rdm_register_listener;
>>       rdmc->unregister_listener =
>> memory_attribute_rdm_unregister_listener;
>