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

[Alexey Kardashevskiy <aik@amd.com>]

On 9/4/25 16:26, Chenyi Qiang wrote:
> 
> 
> 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.

I'd just reduce indent to previous line + 4 spaces vs the current "under 
the opening bracket" rule which I dislike anyway :) Thanks,


>>> +
>>> +    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
>>>     *
>>
> 

-- 
Alexey