On 4/9/2025 2:45 PM, Alexey Kardashevskiy wrote:
>
>
> 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 <a...@amd.com>
>>>> Reviewed-by: David Hildenbrand <da...@redhat.com>
>>>> Signed-off-by: Chenyi Qiang <chenyi.qi...@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 <a...@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,
I can make the adjustment for this line. As for the previous line which
also reports a warning, do you think it needs to wrap?
>
>
