Re: [PATCH v3 4/4] xen-all: Pass max_ram_below_4g to xen_hvm_init.

["Andreas Färber" <afaerber@suse.de>]

Am 25.03.2014 00:55, schrieb Don Slutz:
> This is the xen part of "pc & q35: Add new object pc-memory-layout."
> 
> Signed-off-by: Don Slutz <dslutz@verizon.com>
> ---
> v3: Adjust for code readability. Set max_ram_below_4g always and use
> it to calculate above_4g_mem_size, below_4g_mem_size.
> 
>  hw/i386/pc_piix.c    |  4 ++--
>  hw/i386/pc_q35.c     |  4 ++--
>  include/hw/xen/xen.h |  4 ++--
>  xen-all.c            | 40 +++++++++++++++++++++-------------------
>  xen-stub.c           |  4 ++--
>  5 files changed, 29 insertions(+), 27 deletions(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 81d730d..8a93548 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -131,8 +131,8 @@ static void pc_init1(QEMUMachineInitArgs *args,
>          below_4g_mem_size = args->ram_size;
>      }
>  
> -    if (xen_enabled() && xen_hvm_init(&below_4g_mem_size, &above_4g_mem_size,
> -                                      &ram_memory) != 0) {
> +    if (xen_enabled() && xen_hvm_init(max_ram_below_4g, &below_4g_mem_size,
> +                                      &above_4g_mem_size, &ram_memory) != 0) {
>          fprintf(stderr, "xen hardware virtual machine initialisation failed\n");
>          exit(1);
>      }
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 529f53d..09e98e6 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -120,8 +120,8 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
>          below_4g_mem_size = args->ram_size;
>      }
>  
> -    if (xen_enabled() && xen_hvm_init(&below_4g_mem_size, &above_4g_mem_size,
> -                                      &ram_memory) != 0) {
> +    if (xen_enabled() && xen_hvm_init(max_ram_below_4g, &below_4g_mem_size,
> +                                      &above_4g_mem_size, &ram_memory) != 0) {
>          fprintf(stderr, "xen hardware virtual machine initialisation failed\n");
>          exit(1);
>      }
> diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
> index 0f3942e..eca39a5 100644
> --- a/include/hw/xen/xen.h
> +++ b/include/hw/xen/xen.h
> @@ -40,8 +40,8 @@ int xen_init(QEMUMachine *machine);
>  void xenstore_store_pv_console_info(int i, struct CharDriverState *chr);
>  
>  #if defined(NEED_CPU_H) && !defined(CONFIG_USER_ONLY)
> -int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
> -                 MemoryRegion **ram_memory);
> +int xen_hvm_init(ram_addr_t max_ram_below_4g, ram_addr_t *below_4g_mem_size,
> +                 ram_addr_t *above_4g_mem_size, MemoryRegion **ram_memory);
>  void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size,
>                     struct MemoryRegion *mr);
>  void xen_modified_memory(ram_addr_t start, ram_addr_t length);
> diff --git a/xen-all.c b/xen-all.c
> index c64300c..3f6f890 100644
> --- a/xen-all.c
> +++ b/xen-all.c
> @@ -155,31 +155,32 @@ qemu_irq *xen_interrupt_controller_init(void)
>  
>  /* Memory Ops */
>  
> -static void xen_ram_init(ram_addr_t *below_4g_mem_size,
> +static void xen_ram_init(ram_addr_t ram_size, ram_addr_t max_ram_below_4g,
> +                         ram_addr_t *below_4g_mem_size,
>                           ram_addr_t *above_4g_mem_size,
> -                         ram_addr_t ram_size, MemoryRegion **ram_memory_p)
> +                         MemoryRegion **ram_memory_p)
>  {
>      MemoryRegion *sysmem = get_system_memory();
>      ram_addr_t block_len;
>  
> -    block_len = ram_size;
> -    if (ram_size >= HVM_BELOW_4G_RAM_END) {
> -        /* Xen does not allocate the memory continuously, and keep a hole at
> -         * HVM_BELOW_4G_MMIO_START of HVM_BELOW_4G_MMIO_LENGTH
> -         */
> -        block_len += HVM_BELOW_4G_MMIO_LENGTH;
> -    }
> -    memory_region_init_ram(&ram_memory, NULL, "xen.ram", block_len);
> -    *ram_memory_p = &ram_memory;
> -    vmstate_register_ram_global(&ram_memory);
> -
> -    if (ram_size >= HVM_BELOW_4G_RAM_END) {
> -        *above_4g_mem_size = ram_size - HVM_BELOW_4G_RAM_END;
> -        *below_4g_mem_size = HVM_BELOW_4G_RAM_END;
> +    max_ram_below_4g = max_ram_below_4g ? max_ram_below_4g : HVM_BELOW_4G_RAM_END;

Isn't max_ram_below_4g still always zero at this point?

Are you guys still using your own Xen machine types? Then you could set
a compat_props entry for all (not just legacy) machines to override the
actual pc-memory-layout max-ram-below-4g property value to avoid the ?:
here. Especially since I'm not seeing this modified value being written
back to the device.

Considering that machines are objects now, might it be easier to set
this property on the machine object itself rather than having an unused
stub object hanging in limbo? Would create a dependency on Marcel's
series though (which like this series I now need to start reviewing).

> +    if (ram_size >= max_ram_below_4g) {
> +        *above_4g_mem_size = ram_size - max_ram_below_4g;
> +        *below_4g_mem_size = max_ram_below_4g;
>      } else {
>          *above_4g_mem_size = 0;
>          *below_4g_mem_size = ram_size;
>      }
> +    if (!*above_4g_mem_size) {
> +        block_len = ram_size;
> +    } else {
> +        /* Xen does not allocate the memory continuously, and keep a hole of
> +         * of the size computed above or passed in. */

Please keep */ in the front for easily spotting where the comment ends.

Possibly even /* is requested to be on its own line by Coding Style...?
But that'd be a mere code movement.

> +        block_len = (1ULL << 32) + *above_4g_mem_size;
> +    }
> +    memory_region_init_ram(&ram_memory, NULL, "xen.ram", block_len);
> +    *ram_memory_p = &ram_memory;
> +    vmstate_register_ram_global(&ram_memory);
>  
>      memory_region_init_alias(&ram_640k, NULL, "xen.ram.640k",
>                               &ram_memory, 0, 0xa0000);
[snip]

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg