Re: [PATCH 1/7] memory: Export a helper to get intersection of a MemoryRegionSection with a given range

[Alexey Kardashevskiy]

On 13/12/24 18:08, Chenyi Qiang wrote:
> Rename the helper to memory_region_section_intersect_range() to make it
> more generic.
>
> Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com>
> ---
>   hw/virtio/virtio-mem.c | 32 +++++---------------------------
>   include/exec/memory.h  | 13 +++++++++++++
>   system/memory.c        | 17 +++++++++++++++++
>   3 files changed, 35 insertions(+), 27 deletions(-)
>
> diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
> index 80ada89551..e3d1ccaeeb 100644
> --- a/hw/virtio/virtio-mem.c
> +++ b/hw/virtio/virtio-mem.c
> @@ -242,28 +242,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,
> @@ -285,7 +263,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);
> @@ -317,7 +295,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);
> @@ -353,7 +331,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);
> @@ -369,7 +347,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);
> @@ -386,7 +364,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 e5e865d1a9..ec7bc641e8 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -1196,6 +1196,19 @@ 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.
> + */
> +bool memory_region_section_intersect_range(MemoryRegionSection *s,
> +                                           uint64_t offset, uint64_t size);
> +
>   /**
>    * memory_region_init: Initialize a memory region
>    *
> diff --git a/system/memory.c b/system/memory.c
> index 85f6834cb3..ddcec90f5e 100644
> --- a/system/memory.c
> +++ b/system/memory.c
> @@ -2898,6 +2898,23 @@ void memory_region_section_free_copy(MemoryRegionSection 
> *s)
>       g_free(s);
>   }
>   
> +bool memory_region_section_intersect_range(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);

imho @end needs to be Int128 and s/MIN/int128_min/, etc to be totally 
correct (although it is going to look horrendous). May be it was alright 
when it was just virtio but now it is a wider API. I understand this is 
cut-n-paste and unlikely scenario of offset+size crossing 1<<64 but 
still. Thanks,


> +
> +    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;
> +}
> +
>   bool memory_region_present(MemoryRegion *container, hwaddr addr)
>   {
>       MemoryRegion *mr;

--
Alexey