Re: [Qemu-arm] [PATCH v3 07/10] hw/arm/virt: Introduce opt-in feature "fdt"

[Auger Eric]

Hi Shameer,

[ + Laszlo, Ard, Leif ]

On 3/21/19 11:47 AM, Shameer Kolothum wrote:
> This is to disable/enable populating DT nodes in case
> any conflict with acpi tables. The default is "off".
The name of the option sounds misleading to me. Also we don't really
know the scope of the disablement. At the moment this just aims to
prevent the hotpluggable dt nodes from being added if we boot in ACPI mode.

> 
> This will be used in subsequent patch where cold plug
> device-memory support is added for DT boot.
I am concerned about the fact that in dt mode, by default, you won't see
any PCDIMM nodes.
> 
> If DT memory node support is added for cold-plugged device
> memory, those memory will be visible to Guest kernel via
> UEFI GetMemoryMap() and gets treated as early boot memory.
Don't we have an issue in UEFI then. Normally the SRAT indicates whether
the slots are hotpluggable or not. Shouldn't the UEFI code look at this
info.
> Hence memory becomes non hot-un-unpluggable even if Guest
> is booted in ACPI mode.



> 
> Signed-off-by: Shameer Kolothum <address@hidden>
> ---
>  hw/arm/virt.c         | 23 +++++++++++++++++++++++
>  include/hw/arm/virt.h |  1 +
>  2 files changed, 24 insertions(+)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 13db0e9..b602151 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1717,6 +1717,20 @@ static void virt_set_highmem(Object *obj, bool value, 
> Error **errp)
>      vms->highmem = value;
>  }
>  
> +static bool virt_get_fdt(Object *obj, Error **errp)
> +{
> +    VirtMachineState *vms = VIRT_MACHINE(obj);
> +
> +    return vms->use_fdt;
> +}
> +
> +static void virt_set_fdt(Object *obj, bool value, Error **errp)
> +{
> +    VirtMachineState *vms = VIRT_MACHINE(obj);
> +
> +    vms->use_fdt = value;
> +}
> +
>  static bool virt_get_its(Object *obj, Error **errp)
>  {
>      VirtMachineState *vms = VIRT_MACHINE(obj);
> @@ -2005,6 +2019,15 @@ static void virt_instance_init(Object *obj)
>      object_property_set_description(obj, "gic-version",
>                                      "Set GIC version. "
>                                      "Valid values are 2, 3 and host", NULL);
> +    /* fdt is disabled by default */
> +    vms->use_fdt = false;
> +    object_property_add_bool(obj, "fdt", virt_get_fdt,
> +                             virt_set_fdt, NULL);
> +    object_property_set_description(obj, "fdt",
> +                                    "Set on/off to enable/disable device 
> tree "
> +                                    "nodes in case any conflict with ACPI"
in case of

Thanks

Eric
> +                                    "(eg: device memory node)",
> +                                    NULL);
>  
>      vms->highmem_ecam = !vmc->no_highmem_ecam;
>  
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index c5e4c96..14b2e0a 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -119,6 +119,7 @@ typedef struct {
>      bool highmem_ecam;
>      bool its;
>      bool virt;
> +    bool use_fdt;
>      int32_t gic_version;
>      VirtIOMMUType iommu;
>      struct arm_boot_info bootinfo;
>