Re: [Qemu-devel] [PATCH v3 2/3] machine: make MemoryHotplugState accessible via the machine

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5940 bytes --]

On Fri, Apr 20, 2018 at 02:34:55PM +0200, David Hildenbrand wrote:
> Let's allow to query the MemoryHotplugState from the machine.
> 
> This allows us to generically detect if a certain machine has support
> for memory devices, and to generically manage it (find free address
> range, plug/unplug a memory region).
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

So, we're creating a hook where it seems very likely that the only
implementationss will be simply to retrieve the right field from the
machine specific structure.

So.. should we instead just move the hotplug_memory structure to the
based MachineState type?

> ---
>  hw/i386/pc.c             | 12 ++++++++++++
>  hw/ppc/spapr.c           | 12 ++++++++++++
>  include/hw/boards.h      | 16 ++++++++++++++++
>  include/hw/mem/pc-dimm.h | 12 +-----------
>  4 files changed, 41 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index d36bac8c89..fa8862af33 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -2337,6 +2337,17 @@ static void x86_nmi(NMIState *n, int cpu_index, Error **errp)
>      }
>  }
>  
> +static MemoryHotplugState *pc_machine_get_memory_hotplug_state(MachineState *ms)
> +{
> +    PCMachineState *pcms = PC_MACHINE(ms);
> +
> +    if (!pcms->hotplug_memory.base) {
> +        /* hotplug not supported */
> +        return NULL;
> +    }
> +    return &pcms->hotplug_memory;
> +}
> +
>  static void pc_machine_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
> @@ -2376,6 +2387,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>      hc->unplug = pc_machine_device_unplug_cb;
>      nc->nmi_monitor_handler = x86_nmi;
>      mc->default_cpu_type = TARGET_DEFAULT_CPU_TYPE;
> +    mc->get_memory_hotplug_state = pc_machine_get_memory_hotplug_state;
>  
>      object_class_property_add(oc, PC_MACHINE_MEMHP_REGION_SIZE, "int",
>          pc_machine_get_hotplug_memory_region_size, NULL,
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index a7428f7da7..7ccdb705b3 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3868,6 +3868,17 @@ PowerPCCPU *spapr_find_cpu(int vcpu_id)
>      return NULL;
>  }
>  
> +static MemoryHotplugState *spapr_get_memory_hotplug_state(MachineState *ms)
> +{
> +    sPAPRMachineState *spapr = SPAPR_MACHINE(ms);
> +
> +    if (!spapr->hotplug_memory.base) {
> +        /* hotplug not supported */
> +        return NULL;
> +    }
> +    return &spapr->hotplug_memory;
> +}
> +
>  static void spapr_machine_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
> @@ -3927,6 +3938,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>       * in which LMBs are represented and hot-added
>       */
>      mc->numa_mem_align_shift = 28;
> +    mc->get_memory_hotplug_state = spapr_get_memory_hotplug_state;
>  
>      smc->default_caps.caps[SPAPR_CAP_HTM] = SPAPR_CAP_OFF;
>      smc->default_caps.caps[SPAPR_CAP_VSX] = SPAPR_CAP_ON;
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index a609239112..447bdc7219 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -105,6 +105,17 @@ typedef struct {
>      CPUArchId cpus[0];
>  } CPUArchIdList;
>  
> +/**
> + * MemoryHotplugState:
> + * @base: address in guest physical address space where hotplug memory
> + * address space begins.
> + * @mr: hotplug memory address space container
> + */
> +typedef struct MemoryHotplugState {
> +    hwaddr base;
> +    MemoryRegion mr;
> +} MemoryHotplugState;
> +
>  /**
>   * MachineClass:
>   * @max_cpus: maximum number of CPUs supported. Default: 1
> @@ -156,6 +167,10 @@ typedef struct {
>   *    should instead use "unimplemented-device" for all memory ranges where
>   *    the guest will attempt to probe for a device that QEMU doesn't
>   *    implement and a stub device is required.
> + * @get_memory_hotplug_state:
> + *    If a machine support memory hotplug, the returned data ontains
> + *    information about the portion of guest physical address space
> + *    where memory devices can be mapped to (e.g. to hotplug a pc-dimm).
>   */
>  struct MachineClass {
>      /*< private >*/
> @@ -212,6 +227,7 @@ struct MachineClass {
>                                                           unsigned cpu_index);
>      const CPUArchIdList *(*possible_cpu_arch_ids)(MachineState *machine);
>      int64_t (*get_default_cpu_node_id)(const MachineState *ms, int idx);
> +    MemoryHotplugState *(*get_memory_hotplug_state)(MachineState *ms);
>  };
>  
>  /**
> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> index e88073321f..8bda37adab 100644
> --- a/include/hw/mem/pc-dimm.h
> +++ b/include/hw/mem/pc-dimm.h
> @@ -19,6 +19,7 @@
>  #include "exec/memory.h"
>  #include "sysemu/hostmem.h"
>  #include "hw/qdev.h"
> +#include "hw/boards.h"
>  
>  #define TYPE_PC_DIMM "pc-dimm"
>  #define PC_DIMM(obj) \
> @@ -75,17 +76,6 @@ typedef struct PCDIMMDeviceClass {
>      MemoryRegion *(*get_vmstate_memory_region)(PCDIMMDevice *dimm);
>  } PCDIMMDeviceClass;
>  
> -/**
> - * MemoryHotplugState:
> - * @base: address in guest physical address space where hotplug memory
> - * address space begins.
> - * @mr: hotplug memory address space container
> - */
> -typedef struct MemoryHotplugState {
> -    hwaddr base;
> -    MemoryRegion mr;
> -} MemoryHotplugState;
> -
>  uint64_t pc_dimm_get_free_addr(uint64_t address_space_start,
>                                 uint64_t address_space_size,
>                                 uint64_t *hint, uint64_t align, uint64_t size,

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]