Re: [PATCH-for-9.0 9/9] hw/xen/hvm: Inline xen_arch_set_memory()

[Manos Pitsidianakis <manos.pitsidianakis@xxxxxxxxxx>]

On Tue, 14 Nov 2023 18:31, Philippe Mathieu-Daudé <philmd@xxxxxxxxxx> wrote:
> xen_arch_set_memory() is not arch-specific anymore. Being
> called once, inline it in xen_set_memory().
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@xxxxxxxxxx>
> ---
> include/hw/xen/xen-hvm-common.h |   3 -
> hw/xen/xen-hvm-common.c         | 104 ++++++++++++++++----------------
> 2 files changed, 51 insertions(+), 56 deletions(-)
>
> diff --git a/include/hw/xen/xen-hvm-common.h b/include/hw/xen/xen-hvm-common.h
> index 536712dc83..a1b8a2783b 100644
> --- a/include/hw/xen/xen-hvm-common.h
> +++ b/include/hw/xen/xen-hvm-common.h
> @@ -99,8 +99,5 @@ void cpu_ioreq_pio(ioreq_t *req);
>
> void xen_read_physmap(XenIOState *state);
> void xen_arch_handle_ioreq(XenIOState *state, ioreq_t *req);
> -void xen_arch_set_memory(XenIOState *state,
> -                         MemoryRegionSection *section,
> -                         bool add);
>
> #endif /* HW_XEN_HVM_COMMON_H */
> diff --git a/hw/xen/xen-hvm-common.c b/hw/xen/xen-hvm-common.c
> index 50ce45effc..789c6b4b7a 100644
> --- a/hw/xen/xen-hvm-common.c
> +++ b/hw/xen/xen-hvm-common.c
> @@ -426,50 +426,6 @@ void qmp_xen_set_global_dirty_log(bool enable, Error 
> **errp)
>     }
> }
>
> -void xen_arch_set_memory(XenIOState *state, MemoryRegionSection *section,
> -                         bool add)
> -{
> -    unsigned target_page_bits = qemu_target_page_bits();
> -    int page_size = qemu_target_page_size();
> -    int page_mask = -page_size;
> -    hwaddr start_addr = section->offset_within_address_space;
> -    ram_addr_t size = int128_get64(section->size);
> -    bool log_dirty = memory_region_is_logging(section->mr, DIRTY_MEMORY_VGA);
> -    hvmmem_type_t mem_type;
> -
> -    if (!memory_region_is_ram(section->mr)) {
> -        return;
> -    }
> -
> -    if (log_dirty != add) {
> -        return;
> -    }
> -
> -    trace_xen_client_set_memory(start_addr, size, log_dirty);
> -
> -    start_addr &= page_mask;
> -    size = ROUND_UP(size, page_size);
> -
> -    if (add) {
> -        if (!memory_region_is_rom(section->mr)) {
> -            xen_add_to_physmap(state, start_addr, size,
> -                               section->mr, section->offset_within_region);
> -        } else {
> -            mem_type = HVMMEM_ram_ro;
> -            if (xen_set_mem_type(xen_domid, mem_type,
> -                                 start_addr >> target_page_bits,
> -                                 size >> target_page_bits)) {
> -                DPRINTF("xen_set_mem_type error, addr: "HWADDR_FMT_plx"\n",
> -                        start_addr);
> -            }
> -        }
> -    } else {
> -        if (xen_remove_from_physmap(state, start_addr, size) < 0) {
> -            DPRINTF("physmapping does not exist at "HWADDR_FMT_plx"\n", 
> start_addr);
> -        }
> -    }
> -}
> -
> void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, MemoryRegion *mr,
>                    Error **errp)
> {
> @@ -512,20 +468,62 @@ static void xen_set_memory(struct MemoryListener 
> *listener,
>                            bool add)
> {
>     XenIOState *state = container_of(listener, XenIOState, memory_listener);
> +    unsigned target_page_bits = qemu_target_page_bits();
> +    int page_size = qemu_target_page_size();
> +    int page_mask = -page_size;
> +    hwaddr start_addr;
> +    ram_addr_t size;
> +    bool log_dirty;
> +    hvmmem_type_t mem_type;
> +
>
>     if (section->mr == &xen_memory) {
>         return;
> -    } else {
> -        if (add) {
> -            xen_map_memory_section(xen_domid, state->ioservid,
> -                                   section);
> -        } else {
> -            xen_unmap_memory_section(xen_domid, state->ioservid,
> -                                     section);
> -        }
>     }
>
> -    xen_arch_set_memory(state, section, add);
> +    if (add) {
> +        xen_map_memory_section(xen_domid, state->ioservid,
> +                               section);
> +    } else {
> +        xen_unmap_memory_section(xen_domid, state->ioservid,
> +                                 section);
> +    }
> +
> +    if (!memory_region_is_ram(section->mr)) {
> +        return;
> +    }
> +
> +    log_dirty = memory_region_is_logging(section->mr, DIRTY_MEMORY_VGA);
> +
> +    if (log_dirty != add) {
> +        return;
> +    }
> +
> +    start_addr = section->offset_within_address_space;
> +    size = int128_get64(section->size);
> +    trace_xen_client_set_memory(start_addr, size, log_dirty);
> +
> +    start_addr &= page_mask;
> +    size = ROUND_UP(size, page_size);
> +
> +    if (add) {
> +        if (!memory_region_is_rom(section->mr)) {
> +            xen_add_to_physmap(state, start_addr, size,
> +                               section->mr, section->offset_within_region);
> +        } else {
> +            mem_type = HVMMEM_ram_ro;
> +            if (xen_set_mem_type(xen_domid, mem_type,
> +                                 start_addr >> target_page_bits,
> +                                 size >> target_page_bits)) {
> +                DPRINTF("xen_set_mem_type error, addr: "HWADDR_FMT_plx"\n",
> +                        start_addr);
> +            }
> +        }
> +    } else {
> +        if (xen_remove_from_physmap(state, start_addr, size) < 0) {
> +            DPRINTF("physmapping does not exist at "HWADDR_FMT_plx"\n", 
> start_addr);
> +        }
> +    }
> }
>
> void xen_region_add(MemoryListener *listener,
> --
> 2.41.0

Same observation as in previous patch, in Arm xen, qemu doesn't handle 
memory, it is only responsible for devices and their memory.