Re: [PATCH v4 01/13] memory: Export a helper to get intersection of a MemoryRegionSection with a given range

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

On 4/9/2025 10:47 AM, Alexey Kardashevskiy wrote:
> 
> On 7/4/25 17:49, Chenyi Qiang wrote:
>> Rename the helper to memory_region_section_intersect_range() to make it
>> more generic. Meanwhile, define the @end as Int128 and replace the
>> related operations with Int128_* format since the helper is exported as
>> a wider API.
>>
>> Suggested-by: Alexey Kardashevskiy <aik@amd.com>
>> Reviewed-by: David Hildenbrand <david@redhat.com>
>> Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com>
> 
> ./scripts/checkpatch.pl complains "WARNING: line over 80 characters"
> 
> with that fixed,

I observed many places in QEMU ignore the WARNING for over 80
characters, so I also ignored them in my series.

After checking the rule in docs/devel/style.rst, I think I should try
best to make it not longer than 80. But if it is hard to do so due to
long function or symbol names, it is acceptable to not wrap it.

Then, I would modify the first warning code. For the latter two
warnings, see code below

> 
> Reviewed-by: Alexey Kardashevskiy <aik@amd.com>
> 
>> ---
>> Changes in v4:
>>      - No change.
>>
>> Changes in v3:
>>      - No change
>>
>> Changes in v2:
>>      - Make memory_region_section_intersect_range() an inline function.
>>      - Add Reviewed-by from David
>>      - Define the @end as Int128 and use the related Int128_* ops as a
>> wilder
>>        API (Alexey)
>> ---
>>   hw/virtio/virtio-mem.c | 32 +++++---------------------------
>>   include/exec/memory.h  | 27 +++++++++++++++++++++++++++
>>   2 files changed, 32 insertions(+), 27 deletions(-)
>>
>> diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
>> index b1a003736b..21f16e4912 100644
>> --- a/hw/virtio/virtio-mem.c
>> +++ b/hw/virtio/virtio-mem.c
>> @@ -244,28 +244,6 @@ static int
>> virtio_mem_for_each_plugged_range(VirtIOMEM *vmem, void *arg,
>>       return ret;
>>   }
>>   -/*
>> - * Adjust the memory section to cover the intersection with the given
>> range.
>> - *
>> - * Returns false if the intersection is empty, otherwise returns true.
>> - */
>> -static bool virtio_mem_intersect_memory_section(MemoryRegionSection *s,
>> -                                                uint64_t offset,
>> uint64_t size)
>> -{
>> -    uint64_t start = MAX(s->offset_within_region, offset);
>> -    uint64_t end = MIN(s->offset_within_region + int128_get64(s->size),
>> -                       offset + size);
>> -
>> -    if (end <= start) {
>> -        return false;
>> -    }
>> -
>> -    s->offset_within_address_space += start - s->offset_within_region;
>> -    s->offset_within_region = start;
>> -    s->size = int128_make64(end - start);
>> -    return true;
>> -}
>> -
>>   typedef int (*virtio_mem_section_cb)(MemoryRegionSection *s, void
>> *arg);
>>     static int virtio_mem_for_each_plugged_section(const VirtIOMEM *vmem,
>> @@ -287,7 +265,7 @@ static int
>> virtio_mem_for_each_plugged_section(const VirtIOMEM *vmem,
>>                                         first_bit + 1) - 1;
>>           size = (last_bit - first_bit + 1) * vmem->block_size;
>>   -        if (!virtio_mem_intersect_memory_section(&tmp, offset,
>> size)) {
>> +        if (!memory_region_section_intersect_range(&tmp, offset,
>> size)) {
>>               break;
>>           }
>>           ret = cb(&tmp, arg);
>> @@ -319,7 +297,7 @@ static int
>> virtio_mem_for_each_unplugged_section(const VirtIOMEM *vmem,
>>                                    first_bit + 1) - 1;
>>           size = (last_bit - first_bit + 1) * vmem->block_size;
>>   -        if (!virtio_mem_intersect_memory_section(&tmp, offset,
>> size)) {
>> +        if (!memory_region_section_intersect_range(&tmp, offset,
>> size)) {
>>               break;
>>           }
>>           ret = cb(&tmp, arg);
>> @@ -355,7 +333,7 @@ static void virtio_mem_notify_unplug(VirtIOMEM
>> *vmem, uint64_t offset,
>>       QLIST_FOREACH(rdl, &vmem->rdl_list, next) {
>>           MemoryRegionSection tmp = *rdl->section;
>>   -        if (!virtio_mem_intersect_memory_section(&tmp, offset,
>> size)) {
>> +        if (!memory_region_section_intersect_range(&tmp, offset,
>> size)) {
>>               continue;
>>           }
>>           rdl->notify_discard(rdl, &tmp);
>> @@ -371,7 +349,7 @@ static int virtio_mem_notify_plug(VirtIOMEM *vmem,
>> uint64_t offset,
>>       QLIST_FOREACH(rdl, &vmem->rdl_list, next) {
>>           MemoryRegionSection tmp = *rdl->section;
>>   -        if (!virtio_mem_intersect_memory_section(&tmp, offset,
>> size)) {
>> +        if (!memory_region_section_intersect_range(&tmp, offset,
>> size)) {
>>               continue;
>>           }
>>           ret = rdl->notify_populate(rdl, &tmp);
>> @@ -388,7 +366,7 @@ static int virtio_mem_notify_plug(VirtIOMEM *vmem,
>> uint64_t offset,
>>               if (rdl2 == rdl) {
>>                   break;
>>               }
>> -            if (!virtio_mem_intersect_memory_section(&tmp, offset,
>> size)) {
>> +            if (!memory_region_section_intersect_range(&tmp, offset,
>> size)) {
>>                   continue;
>>               }
>>               rdl2->notify_discard(rdl2, &tmp);
>> diff --git a/include/exec/memory.h b/include/exec/memory.h
>> index 3ee1901b52..3bebc43d59 100644
>> --- a/include/exec/memory.h
>> +++ b/include/exec/memory.h
>> @@ -1202,6 +1202,33 @@ MemoryRegionSection
>> *memory_region_section_new_copy(MemoryRegionSection *s);
>>    */
>>   void memory_region_section_free_copy(MemoryRegionSection *s);
>>   +/**
>> + * memory_region_section_intersect_range: Adjust the memory section
>> to cover
>> + * the intersection with the given range.
>> + *
>> + * @s: the #MemoryRegionSection to be adjusted
>> + * @offset: the offset of the given range in the memory region
>> + * @size: the size of the given range
>> + *
>> + * Returns false if the intersection is empty, otherwise returns true.
>> + */
>> +static inline bool
>> memory_region_section_intersect_range(MemoryRegionSection *s,
>> +                                                         uint64_t
>> offset, uint64_t size)
>> +{
>> +    uint64_t start = MAX(s->offset_within_region, offset);
>> +    Int128 end = int128_min(int128_add(int128_make64(s-
>> >offset_within_region), s->size),
>> +                            int128_add(int128_make64(offset),
>> int128_make64(size)));

The Int128_* format helper make the line over 80. I think it's better
not wrap it for readability.

>> +
>> +    if (int128_le(end, int128_make64(start))) {
>> +        return false;
>> +    }
>> +
>> +    s->offset_within_address_space += start - s->offset_within_region;
>> +    s->offset_within_region = start;
>> +    s->size = int128_sub(end, int128_make64(start));
>> +    return true;
>> +}
>> +
>>   /**
>>    * memory_region_init: Initialize a memory region
>>    *
>