Re: [Qemu-devel] [PATCH 2/2] pc: memhotplug: keep reserved-memory-end broken on 2.4 and earlier machines

[Eduardo Habkost]

On Mon, Sep 07, 2015 at 01:55:32PM +0200, Igor Mammedov wrote:
> it will prevent guests on old machines from seeing
> inconsistent memory mapping in firmware/ACPI views.
> 
> Signed-off-by: Igor Mammedov <address@hidden>
> ---
> PS: this patch probably clashes with Eduardo's
>     pc_compat refactoring, so I've split it from
>     main fix, if applied via Eduardo's it could
>     be squashed in main fix after merging on top
>     of his patches or as is if goes in first.

It conflicts with my "pc: Initialization and compat function cleanup"
series, but my series needs to be rebased because of smbios changes
anyway. I can rebase my series after your fix gets in.

[...]
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 354e1b3..b5107f7 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1412,8 +1412,12 @@ FWCfgState *pc_memory_init(PCMachineState *pcms,
>  
>      if (guest_info->has_reserved_memory && pcms->hotplug_memory.base) {
>          uint64_t *val = g_malloc(sizeof(*val));
> -        uint64_t res_mem_end = pcms->hotplug_memory.base +
> -                               memory_region_size(&pcms->hotplug_memory.mr);
> +        PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);

Style suggestion: move pcmc to the beginning of the function, because
other blocks inside the function are likely to look at PCMachineClass
fields in the future.

> +        uint64_t res_mem_end = pcms->hotplug_memory.base;
> +
> +        if (!pcmc->broken_reserved_end) {
> +            res_mem_end += memory_region_size(&pcms->hotplug_memory.mr);
> +        }
>          *val = cpu_to_le64(ROUND_UP(res_mem_end, 0x1ULL << 30));
>          fw_cfg_add_file(fw_cfg, "etc/reserved-memory-end", val, 
> sizeof(*val));
>      }
[...]
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index d0cad87..ff0b48b 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -59,6 +59,7 @@ struct PCMachineClass {
>      MachineClass parent_class;
>  
>      /*< public >*/
> +    bool broken_reserved_end;

A comment describing what exactly the new field does would be welcome.

I don't think my suggestions should block the patch, so:

Reviewed-by: Eduardo Habkost <address@hidden>

-- 
Eduardo