Re: [PATCH v10 13/45] cxl: Machine level control on whether CXL support is enabled

[Paolo Bonzini]

On 4/29/22 16:40, Jonathan Cameron via wrote:
> From: Jonathan Cameron <jonathan.cameron@huawei.com>
>
> There are going to be some potential overheads to CXL enablement,
> for example the host bridge region reserved in memory maps.
> Add a machine level control so that CXL is disabled by default.
>
> Signed-off-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   hw/core/machine.c    | 28 ++++++++++++++++++++++++++++
>   hw/i386/pc.c         |  1 +
>   include/hw/boards.h  |  2 ++
>   include/hw/cxl/cxl.h |  4 ++++
>   4 files changed, 35 insertions(+)

Another belated review, I think this shouldn't be added to machines that 
do not support CXL (yes, there are options like -M usb but they are from 
olden times---and CXL is a little more niche, too :)).

Can you move the CXL code for machines to e.g. hw/cxl/machine.c and have
the various machines call back into hooks to add the properties, resolve
the memory window targets etc.?.  A CXLState* like cxl_devices_state can 
be added to the machines and passed as CXLState** to one of these hooks.

Thanks,

Paolo

> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index cb9bbc844d..6ae2997f16 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -31,6 +31,7 @@
>   #include "sysemu/qtest.h"
>   #include "hw/pci/pci.h"
>   #include "hw/mem/nvdimm.h"
> +#include "hw/cxl/cxl.h"
>   #include "migration/global_state.h"
>   #include "migration/vmstate.h"
>   #include "exec/confidential-guest-support.h"
> @@ -550,6 +551,20 @@ static void machine_set_nvdimm_persistence(Object *obj, 
> const char *value,
>       nvdimms_state->persistence_string = g_strdup(value);
>   }
>   
> +static bool machine_get_cxl(Object *obj, Error **errp)
> +{
> +    MachineState *ms = MACHINE(obj);
> +
> +    return ms->cxl_devices_state->is_enabled;
> +}
> +
> +static void machine_set_cxl(Object *obj, bool value, Error **errp)
> +{
> +    MachineState *ms = MACHINE(obj);
> +
> +    ms->cxl_devices_state->is_enabled = value;
> +}
> +
>   void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char 
> *type)
>   {
>       QAPI_LIST_PREPEND(mc->allowed_dynamic_sysbus_devices, g_strdup(type));
> @@ -782,6 +797,8 @@ static void machine_class_init(ObjectClass *oc, void *data)
>       mc->default_ram_size = 128 * MiB;
>       mc->rom_file_has_mr = true;
>   
> +    /* Few machines support CXL, so default to off */
> +    mc->cxl_supported = false;
>       /* numa node memory size aligned on 8MB by default.
>        * On Linux, each node's border has to be 8MB aligned
>        */
> @@ -927,6 +944,16 @@ static void machine_initfn(Object *obj)
>                                           "Valid values are cpu, mem-ctrl");
>       }
>   
> +    if (mc->cxl_supported) {
> +        Object *obj = OBJECT(ms);
> +
> +        ms->cxl_devices_state = g_new0(CXLState, 1);
> +        object_property_add_bool(obj, "cxl", machine_get_cxl, machine_set_cxl);
> +        object_property_set_description(obj, "cxl",
> +                                        "Set on/off to enable/disable "
> +                                        "CXL instantiation");
> +    }
> +
>       if (mc->cpu_index_to_instance_props && mc->get_default_cpu_node_id) {
>           ms->numa_state = g_new0(NumaState, 1);
>           object_property_add_bool(obj, "hmat",
> @@ -961,6 +988,7 @@ static void machine_finalize(Object *obj)
>       g_free(ms->device_memory);
>       g_free(ms->nvdimms_state);
>       g_free(ms->numa_state);
> +    g_free(ms->cxl_devices_state);
>   }
>   
>   bool machine_usb(MachineState *machine)
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 23bba9d82c..b752339beb 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1761,6 +1761,7 @@ static void pc_machine_class_init(ObjectClass *oc, void 
> *data)
>       mc->default_cpu_type = TARGET_DEFAULT_CPU_TYPE;
>       mc->nvdimm_supported = true;
>       mc->smp_props.dies_supported = true;
> +    mc->cxl_supported = true;
>       mc->default_ram_id = "pc.ram";
>   
>       object_class_property_add(oc, PC_MACHINE_MAX_RAM_BELOW_4G, "size",
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index d64b5481e8..f756a1d5fc 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -269,6 +269,7 @@ struct MachineClass {
>       bool ignore_boot_device_suffixes;
>       bool smbus_no_migration_support;
>       bool nvdimm_supported;
> +    bool cxl_supported;
>       bool numa_mem_supported;
>       bool auto_enable_numa;
>       SMPCompatProps smp_props;
> @@ -360,6 +361,7 @@ struct MachineState {
>       CPUArchIdList *possible_cpus;
>       CpuTopology smp;
>       struct NVDIMMState *nvdimms_state;
> +    struct CXLState *cxl_devices_state;
>       struct NumaState *numa_state;
>   };
>   
> diff --git a/include/hw/cxl/cxl.h b/include/hw/cxl/cxl.h
> index 554ad93b6b..31af92fd5e 100644
> --- a/include/hw/cxl/cxl.h
> +++ b/include/hw/cxl/cxl.h
> @@ -17,4 +17,8 @@
>   #define CXL_COMPONENT_REG_BAR_IDX 0
>   #define CXL_DEVICE_REG_BAR_IDX 2
>   
> +typedef struct CXLState {
> +    bool is_enabled;
> +} CXLState;
> +
>   #endif