Re: [Qemu-devel] [PATCH v4 8/8] hw/arm/boot: Expose the PC-DIMM nodes in the DT

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Linuxarm [mailto:address@hidden On Behalf Of
> Shameerali Kolothum Thodi
> Sent: 10 April 2019 09:49
> To: Laszlo Ersek <address@hidden>; address@hidden;
> address@hidden; address@hidden; address@hidden
> Cc: address@hidden; address@hidden;
> address@hidden; Linuxarm <address@hidden>;
> address@hidden; address@hidden; xuwei (O)
> <address@hidden>
> Subject: RE: [PATCH v4 8/8] hw/arm/boot: Expose the PC-DIMM nodes in the
> DT
> 
> 
> > -----Original Message-----
> > From: Laszlo Ersek [mailto:address@hidden
> > Sent: 09 April 2019 16:09
> > To: Shameerali Kolothum Thodi <address@hidden>;
> > address@hidden; address@hidden; address@hidden;
> > address@hidden
> > Cc: address@hidden; address@hidden;
> > address@hidden; address@hidden; xuwei (O)
> > <address@hidden>; address@hidden; Linuxarm
> > <address@hidden>
> > Subject: Re: [PATCH v4 8/8] hw/arm/boot: Expose the PC-DIMM nodes in the
> > DT
> >
> > On 04/09/19 12:29, Shameer Kolothum wrote:
> > > This patch adds memory nodes corresponding to PC-DIMM regions.
> > > This will enable support for cold plugged device memory for Guests
> > > with DT boot.
> > >
> > > Signed-off-by: Shameer Kolothum
> <address@hidden>
> > > Signed-off-by: Eric Auger <address@hidden>
> > > ---
> > >  hw/arm/boot.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 42 insertions(+)
> > >
> > > diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> > > index 8c840ba..150e1ed 100644
> > > --- a/hw/arm/boot.c
> > > +++ b/hw/arm/boot.c
> > > @@ -19,6 +19,7 @@
> > >  #include "sysemu/numa.h"
> > >  #include "hw/boards.h"
> > >  #include "hw/loader.h"
> > > +#include "hw/mem/memory-device.h"
> > >  #include "elf.h"
> > >  #include "sysemu/device_tree.h"
> > >  #include "qemu/config-file.h"
> > > @@ -538,6 +539,41 @@ static void fdt_add_psci_node(void *fdt)
> > >      qemu_fdt_setprop_cell(fdt, "/psci", "migrate", migrate_fn);
> > >  }
> > >
> > > +static int fdt_add_hotpluggable_memory_nodes(void *fdt,
> > > +                                             uint32_t acells,
> > uint32_t scells) {
> > > +    MemoryDeviceInfoList *info, *info_list = qmp_memory_device_list();
> > > +    MemoryDeviceInfo *mi;
> > > +    int ret = 0;
> > > +
> > > +    for (info = info_list; info != NULL; info = info->next) {
> > > +        mi = info->value;
> > > +        switch (mi->type) {
> > > +        case MEMORY_DEVICE_INFO_KIND_DIMM:
> > > +        {
> > > +            PCDIMMDeviceInfo *di = mi->u.dimm.data;
> > > +
> > > +            ret = fdt_add_memory_node(fdt, acells, di->addr, scells,
> > > +                                      di->size, di->node, true);
> > > +            if (ret) {
> > > +                fprintf(stderr,
> > > +                        "couldn't add PCDIMM
> /address@hidden"PRIx64"
> > node\n",
> > > +                        di->addr);
> > > +                goto out;
> > > +            }
> > > +            break;
> > > +        }
> > > +        default:
> > > +            fprintf(stderr, "%s memory nodes are not yet supported\n",
> > > +                    MemoryDeviceInfoKind_str(mi->type));
> > > +            ret = -ENOENT;
> > > +            goto out;
> > > +        }
> > > +    }
> > > +out:
> > > +    qapi_free_MemoryDeviceInfoList(info_list);
> > > +    return ret;
> > > +}
> > > +
> > >  int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
> > >                   hwaddr addr_limit, AddressSpace *as)
> > >  {
> > > @@ -637,6 +673,12 @@ int arm_load_dtb(hwaddr addr, const struct
> > arm_boot_info *binfo,
> > >          }
> > >      }
> > >
> > > +    rc = fdt_add_hotpluggable_memory_nodes(fdt, acells, scells);
> > > +    if (rc < 0) {
> > > +        fprintf(stderr, "couldn't add hotpluggable memory nodes\n");
> > > +        goto fail;
> > > +    }
> > > +
> > >      rc = fdt_path_offset(fdt, "/chosen");
> > >      if (rc < 0) {
> > >          qemu_fdt_add_subnode(fdt, "/chosen");
> > >
> >
> >
> > Given patches #7 and #8, as I understand them, the firmware cannot
> > distinguish hotpluggable & present, from hotpluggable & absent. The
> firmware
> > can only skip both hotpluggable cases. That's fine in that the firmware will
> hog
> > neither type -- but is that OK for the OS as well, for both ACPI boot and DT
> > boot?
> 
> Right. This only handles the hotpluggable-and-present condition.
> 
> > Consider in particular the "hotpluggable & present, ACPI boot" case.
> Assuming
> > we modify the firmware to skip "hotpluggable" altogether, the UEFI memmap
> > will not include the range despite it being present at boot. Presumably, 
> > ACPI
> > will refer to the range somehow, however. Will that not confuse the OS?
> 
> From my testing so far, without patches #7 and #8(ie, no UEFI memmap entry),
> ACPI boots fine. I think ACPI only relies on aml and SRAT.
> 
> > When Igor raised this earlier, I suggested that hotpluggable-and-present
> > should be added by the firmware, but also allocated immediately, as
> > EfiBootServicesData type memory. This will prevent other drivers in the
> > firmware from allocating AcpiNVS or Reserved chunks from the same memory
> > range, the UEFI memmap will contain the range as EfiBootServicesData, and
> > then the OS can release that allocation in one go early during boot.
> 
> Ok. Agree that hotpluggable-and-present case it may make sense to make use
> of
> that memory rather than just hiding it altogether.
> 
> On another note, Does hotpluggable-and-absent case make any valid use case
> for
> a DT boot? I am not sure how we can hot-add memory in the case of DT boot
> later.
> I have not verified the sysfs probe interface yet and there are discussions of
> dropping
> that interface altogether.
> 
> > But this really has to be clarified from the Linux kernel's expectations. 
> > Please
> > formalize all of the following cases:
> 
> Sure. I will wait for suggestions here and work on it.

To continue the discussion on this, this is my proposal,

OS boot (DT/ACPI)  hotpluggable & ...  GetMemoryMap() should report as  DT/ACPI 
should report as
-----------------  ------------------  -------------------------------  
------------------------
DT                present           Normal memory             Report as normal 
DT memory node

DT                absent            Not reported                  Not reported.

ACPI              present            EfiBootServicesData memory  SRAT specifies 
the memory region
                                                               and device 
memory discovered
                                                               during Guest 
boot up through AML.

ACPI              absent             Not reported               SRAT specifies 
the memory region.
                                                               Supports hot add 
later.

Not sure I got all the scenarios correct here, please take a look and let me 
know.

Thanks,
Shameer

> 
> Thanks,
> Shameer
> 
> > OS boot (DT/ACPI)  hotpluggable & ...  GetMemoryMap() should report as
> > DT/ACPI should report as
> > -----------------  ------------------  -------------------------------  
> > ------------------------
> > DT
> > present             ?                                ?
> > DT
> > absent              ?                                ?
> > ACPI
> > present             ?                                ?
> > ACPI
> > absent              ?                                ?
> >
> > Again, this table is dictated by Linux.
> >
> > Thanks
> > Laszlo
> _______________________________________________
> Linuxarm mailing list
> address@hidden
> http://hulk.huawei.com/mailman/listinfo/linuxarm