Re: [PATCH v5 05/10] ram-block-attribute: Introduce a helper to notify shared/private state changes

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

On 5/27/2025 3:35 PM, Alexey Kardashevskiy wrote:
> 
> 
> On 20/5/25 20:28, Chenyi Qiang wrote:
>> A new state_change() helper is introduced for RamBlockAttribute
>> to efficiently notify all registered RamDiscardListeners, including
>> VFIO listeners, about memory conversion events in guest_memfd. The VFIO
>> listener can dynamically DMA map/unmap 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.
>>
>> Currently, memory conversion failures cause QEMU to quit instead of
>> resuming the guest or retrying the operation. It would be a future work
>> to add more error handling or rollback mechanisms once conversion
>> failures are allowed. For example, in-place conversion of guest_memfd
>> could retry the unmap operation during the conversion from shared to
>> private. However, for now, keep the complex error handling out of the
>> picture as it is not required:
>>
>> - If 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, there
>>    is no such scenario in practice at present. Simply return error.
>> - If a conversion request is declined by other systems, such as a
>>    failure from VFIO during notify_to_populated(), the failure is
>>    returned directly. As for notify_to_discard(), VFIO cannot fail
>>    unmap/unpin, so no error is returned.
>>
>> Note that the bitmap status is updated before callbacks, allowing
>> listeners to handle memory based on the latest status.
>>
>> Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com>
>> ---
>> Change in v5:
>>      - Move the state_change() back to a helper instead of a callback of
>>        the class since there's no child for the RamBlockAttributeClass.
>>      - Remove the error handling and move them to an individual patch for
>>        simple management.
>>
>> Changes in v4:
>>      - Add the state_change() callback in PrivateSharedManagerClass
>>        instead of the RamBlockAttribute.
>>
>> Changes in v3:
>>      - Move the bitmap update before notifier callbacks.
>>      - Call the notifier callbacks directly in notify_discard/populate()
>>        with the expectation that the request memory range is in the
>>        desired attribute.
>>      - For the case that only partial range in the desire status, handle
>>        the range with block_size granularity for ease of rollback
>>        (https://lore.kernel.org/qemu-devel/812768d7-a02d-4b29-95f3-
>> fb7a125cf54e@redhat.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/ramblock.h    |   2 +
>>   system/ram-block-attribute.c | 134 +++++++++++++++++++++++++++++++++++
>>   2 files changed, 136 insertions(+)
>>
>> diff --git a/include/system/ramblock.h b/include/system/ramblock.h
>> index 09255e8495..270dffb2f3 100644
>> --- a/include/system/ramblock.h
>> +++ b/include/system/ramblock.h
>> @@ -108,6 +108,8 @@ struct RamBlockAttribute {
>>       QLIST_HEAD(, RamDiscardListener) rdl_list;
>>   };
>>   +int ram_block_attribute_state_change(RamBlockAttribute *attr,
>> uint64_t offset,
>> +                                     uint64_t size, bool to_private);
> 
> Not sure about the "to_private" name. I'd think private/shared is
> something KVM operates with and here, in RamBlock, it is discarded/
> populated.

Make sense. To keep consistent, I will rename it as to_discard.

> 
>>   RamBlockAttribute *ram_block_attribute_create(MemoryRegion *mr);
>>   void ram_block_attribute_destroy(RamBlockAttribute *attr);
>>   diff --git a/system/ram-block-attribute.c b/system/ram-block-
>> attribute.c
>> index 8d4a24738c..f12dd4b881 100644
>> --- a/system/ram-block-attribute.c
>> +++ b/system/ram-block-attribute.c
>> @@ -253,6 +253,140 @@ ram_block_attribute_rdm_replay_discard(const
>> RamDiscardManager *rdm,
>>                                              
>> ram_block_attribute_rdm_replay_cb);
>>   }
>>   +static bool ram_block_attribute_is_valid_range(RamBlockAttribute
>> *attr,
>> +                                               uint64_t offset,
>> uint64_t size)
>> +{
>> +    MemoryRegion *mr = attr->mr;
>> +
>> +    g_assert(mr);
>> +
>> +    uint64_t region_size = memory_region_size(mr);
>> +    int block_size = ram_block_attribute_get_block_size(attr);
> 
> It is size_t, not int.

Fixed this and all below. Thanks!

> 
>> +
>> +    if (!QEMU_IS_ALIGNED(offset, block_size)) {
> 
> Does not the @size have to be aligned too?

Yes. Actually, the "start" and "size" are already do the alignment check
in kvm_convert_memory(). I doubt if we still need it here. Anyway, in
case of other users in the future, I'll add it.

> 
>> +        return false;
>> +    }
>> +    if (offset + size < offset || !size) {
> 
> This could be just (offset + size <= offset).
> (these overflow checks always blow up my little brain)

Modified.

> 
>> +        return false;
>> +    }
>> +    if (offset >= region_size || offset + size > region_size) {
> 
> Just (offset + size > region_size) should do.

Ditto.

> 
>> +        return false;
>> +    }
>> +    return true;
>> +}
>> +
>> +static void ram_block_attribute_notify_to_discard(RamBlockAttribute
>> *attr,
>> +                                                  uint64_t offset,
>> +                                                  uint64_t size)
>> +{
>> +    RamDiscardListener *rdl;
>> +
>> +    QLIST_FOREACH(rdl, &attr->rdl_list, next) {
>> +        MemoryRegionSection tmp = *rdl->section;
>> +
>> +        if (!memory_region_section_intersect_range(&tmp, offset,
>> size)) {
>> +            continue;
>> +        }
>> +        rdl->notify_discard(rdl, &tmp);
>> +    }
>> +}
>> +
>> +static int
>> +ram_block_attribute_notify_to_populated(RamBlockAttribute *attr,
>> +                                        uint64_t offset, uint64_t size)
>> +{
>> +    RamDiscardListener *rdl;
>> +    int ret = 0;
>> +
>> +    QLIST_FOREACH(rdl, &attr->rdl_list, next) {
>> +        MemoryRegionSection tmp = *rdl->section;
>> +
>> +        if (!memory_region_section_intersect_range(&tmp, offset,
>> size)) {
>> +            continue;
>> +        }
>> +        ret = rdl->notify_populate(rdl, &tmp);
>> +        if (ret) {
>> +            break;
>> +        }
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>> +static bool ram_block_attribute_is_range_populated(RamBlockAttribute
>> *attr,
>> +                                                   uint64_t offset,
>> +                                                   uint64_t size)
>> +{
>> +    const int block_size = ram_block_attribute_get_block_size(attr);
> 
> size_t.
> 
>> +    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. */
> 
> What is "fake" about it? We truthfully check here that every bit in
> [first_bit, last_bit] is set.

Aha, you ask this question again :)
(https://lore.kernel.org/qemu-devel/7131b4a3-a836-4efd-bcfc-982a0112ef05@intel.com/)

If it is really confusing, let me remove this comment in next version.

> 
>> +    found_bit = find_next_zero_bit(attr->bitmap, last_bit + 1,
>> +                                   first_bit);
>> +    return found_bit > last_bit;
>> +}
>> +
>> +static bool
>> +ram_block_attribute_is_range_discard(RamBlockAttribute *attr,
>> +                                     uint64_t offset, uint64_t size)
>> +{
>> +    const int block_size = ram_block_attribute_get_block_size(attr);
> 
> size_t.
> 
>> +    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. */
>> +    found_bit = find_next_bit(attr->bitmap, last_bit + 1, first_bit);
>> +    return found_bit > last_bit;
>> +}
>> +
>> +int ram_block_attribute_state_change(RamBlockAttribute *attr,
>> uint64_t offset,
>> +                                     uint64_t size, bool to_private)
>> +{
>> +    const int block_size = ram_block_attribute_get_block_size(attr);
> 
> size_t.
> 
>> +    const unsigned long first_bit = offset / block_size;
>> +    const unsigned long nbits = size / block_size;
>> +    int ret = 0;
>> +
>> +    if (!ram_block_attribute_is_valid_range(attr, offset, size)) {
>> +        error_report("%s, invalid range: offset 0x%lx, size 0x%lx",
>> +                     __func__, offset, size);
>> +        return -1;
> 
> May be -EINVAL?

Modified.

> 
>> +    }
>> +
>> +    /* Already discard/populated */
>> +    if ((ram_block_attribute_is_range_discard(attr, offset, size) &&
>> +         to_private) ||
>> +        (ram_block_attribute_is_range_populated(attr, offset, size) &&
>> +         !to_private)) {
> 
> A tracepoint would be useful here imho.

[...]

> 
>> +        return 0;
>> +    }
>> +
>> +    /* Unexpected mixture */
>> +    if ((!ram_block_attribute_is_range_populated(attr, offset, size) &&
>> +         to_private) ||
>> +        (!ram_block_attribute_is_range_discard(attr, offset, size) &&
>> +         !to_private)) {
>> +        error_report("%s, the range is not all in the desired state: "
>> +                     "(offset 0x%lx, size 0x%lx), %s",
>> +                     __func__, offset, size,
>> +                     to_private ? "private" : "shared");
>> +        return -1;
> 
> -EBUSY?

Maybe also -EINVAL since it is due to the invalid provided mixture
range? But Anyway, according to the discussion in patch #10, I'll add
the support for this mixture scenario. No need to return the error.

> 
>> +    }
>> +
>> +    if (to_private) {
>> +        bitmap_clear(attr->bitmap, first_bit, nbits);
>> +        ram_block_attribute_notify_to_discard(attr, offset, size);
>> +    } else {
>> +        bitmap_set(attr->bitmap, first_bit, nbits);
>> +        ret = ram_block_attribute_notify_to_populated(attr, offset,
>> size);
>> +    }
> 
> and a successful tracepoint here may be?

Good suggestion! I'll add tracepoint in next version.

> 
>> +
>> +    return ret;
>> +}
>> +
>>   RamBlockAttribute *ram_block_attribute_create(MemoryRegion *mr)
>>   {
>>       uint64_t bitmap_size;
>