Re: [Qemu-devel] [PATCH v6 16/18] hw/arm/virt: Add nvdimm hot-plug infrastructure

[Igor Mammedov]

On Wed, 20 Feb 2019 16:21:05 +0100
Auger Eric <address@hidden> wrote:

> Hi Igor,
> 
> On 2/18/19 11:30 AM, Igor Mammedov wrote:
> > On Tue,  5 Feb 2019 18:33:04 +0100
> > Eric Auger <address@hidden> wrote:
> >   
> >> From: Kwangwoo Lee <address@hidden>
> >>
> >> Pre-plug and plug handlers are prepared for NVDIMM support.
> >>
> >> Signed-off-by: Eric Auger <address@hidden>
> >> Signed-off-by: Kwangwoo Lee <address@hidden>
> >> ---
> >>  default-configs/arm-softmmu.mak |  2 ++
> >>  hw/arm/virt-acpi-build.c        |  6 ++++++
> >>  hw/arm/virt.c                   | 22 ++++++++++++++++++++++
> >>  include/hw/arm/virt.h           |  3 +++
> >>  4 files changed, 33 insertions(+)
> >>
> >> diff --git a/default-configs/arm-softmmu.mak 
> >> b/default-configs/arm-softmmu.mak
> >> index dc4624794f..ddbe87ed15 100644
> >> --- a/default-configs/arm-softmmu.mak
> >> +++ b/default-configs/arm-softmmu.mak
> >> @@ -162,3 +162,5 @@ CONFIG_HIGHBANK=y
> >>  CONFIG_MUSICPAL=y
> >>  CONFIG_MEM_DEVICE=y
> >>  CONFIG_DIMM=y
> >> +CONFIG_NVDIMM=y
> >> +CONFIG_ACPI_NVDIMM=y
> >> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> >> index 781eafaf5e..f086adfa82 100644
> >> --- a/hw/arm/virt-acpi-build.c
> >> +++ b/hw/arm/virt-acpi-build.c
> >> @@ -784,6 +784,7 @@ static
> >>  void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
> >>  {
> >>      VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
> >> +    MachineState *ms = MACHINE(vms);
> >>      GArray *table_offsets;
> >>      unsigned dsdt, xsdt;
> >>      GArray *tables_blob = tables->table_data;
> >> @@ -824,6 +825,11 @@ void virt_acpi_build(VirtMachineState *vms, 
> >> AcpiBuildTables *tables)
> >>          }
> >>      }
> >>  
> >> +    if (vms->acpi_nvdimm_state.is_enabled) {
> >> +        nvdimm_build_acpi(table_offsets, tables_blob, tables->linker,
> >> +                          &vms->acpi_nvdimm_state, ms->ram_slots);
> >> +    }
> >> +
> >>      if (its_class_name() && !vmc->no_its) {
> >>          acpi_add_table(table_offsets, tables_blob);
> >>          build_iort(tables_blob, tables->linker, vms);
> >> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> >> index b683902991..0c8c2cc191 100644
> >> --- a/hw/arm/virt.c
> >> +++ b/hw/arm/virt.c
> >> @@ -132,6 +132,7 @@ static const MemMapEntry a15memmap[] = {
> >>      [VIRT_GPIO] =               { 0x09030000, 0x00001000 },
> >>      [VIRT_SECURE_UART] =        { 0x09040000, 0x00001000 },
> >>      [VIRT_SMMU] =               { 0x09050000, 0x00020000 },
> >> +    [VIRT_ACPI_IO] =            { 0x09070000, 0x00010000 },  
> > where does this range come from and is its size sufficient?  
> I understand it can be anywhere in low mem and must be large enough to
> contain [NVDIMM_ACPI_IO_BASE, NDIMM_ACPI_IO_BASE + NVDIMM_ACPI_IO_LEN].
it looked to like generic ACPI arrea rather than NVDIMM
so I'd suggest to name it properly and probably use NVDIMM_ACPI_IO_BASE & co
to add this entry so that reader won't have to wonder where this magic numbers
come from.

> So one 64kB page should do the job?
Should it be located in lowmem?
(do we care about device_memory & AVMF & ACPI & not 64bit guests?)


> Thanks
> 
> Eric
> >   
> >>      [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
> >>      /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that 
> >> size */
> >>      [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
> >> @@ -1637,6 +1638,18 @@ static void machvirt_init(MachineState *machine)
> >>  
> >>      create_platform_bus(vms, pic);
> >>  
> >> +    if (vms->acpi_nvdimm_state.is_enabled) {
> >> +        AcpiNVDIMMState *acpi_nvdimm_state = &vms->acpi_nvdimm_state;
> >> +
> >> +        acpi_nvdimm_state->dsm_io.type = NVDIMM_ACPI_IO_MEMORY;
> >> +        acpi_nvdimm_state->dsm_io.base =
> >> +                vms->memmap[VIRT_ACPI_IO].base + NVDIMM_ACPI_IO_BASE;
> >> +        acpi_nvdimm_state->dsm_io.len = NVDIMM_ACPI_IO_LEN;
> >> +
> >> +        nvdimm_init_acpi_state(acpi_nvdimm_state, sysmem,
> >> +                               vms->fw_cfg, OBJECT(vms));
> >> +    }
> >> +
> >>      vms->bootinfo.ram_size = machine->ram_size;
> >>      vms->bootinfo.kernel_filename = machine->kernel_filename;
> >>      vms->bootinfo.kernel_cmdline = machine->kernel_cmdline;
> >> @@ -1822,10 +1835,19 @@ static void virt_memory_plug(HotplugHandler 
> >> *hotplug_dev,
> >>                               DeviceState *dev, Error **errp)
> >>  {
> >>      VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> >> +    bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
> >>      Error *local_err = NULL;
> >>  
> >>      pc_dimm_plug(PC_DIMM(dev), MACHINE(vms), &local_err);
> >> +    if (local_err) {
> >> +        goto out;
> >> +    }
> >>  
> >> +    if (is_nvdimm) {
> >> +        nvdimm_plug(&vms->acpi_nvdimm_state);
> >> +    }
> >> +
> >> +out:
> >>      error_propagate(errp, local_err);
> >>  }
> >>  
> >> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> >> index c88f67a492..56d73b0e86 100644
> >> --- a/include/hw/arm/virt.h
> >> +++ b/include/hw/arm/virt.h
> >> @@ -37,6 +37,7 @@
> >>  #include "hw/arm/arm.h"
> >>  #include "sysemu/kvm.h"
> >>  #include "hw/intc/arm_gicv3_common.h"
> >> +#include "hw/mem/nvdimm.h"
> >>  
> >>  #define NUM_GICV2M_SPIS       64
> >>  #define NUM_VIRTIO_TRANSPORTS 32
> >> @@ -77,6 +78,7 @@ enum {
> >>      VIRT_GPIO,
> >>      VIRT_SECURE_UART,
> >>      VIRT_SECURE_MEM,
> >> +    VIRT_ACPI_IO,
> >>      VIRT_LOWMEMMAP_LAST,
> >>  };
> >>  
> >> @@ -134,6 +136,7 @@ typedef struct {
> >>      hwaddr high_io_base;
> >>      hwaddr highest_gpa;
> >>      bool extended_memmap;
> >> +    AcpiNVDIMMState acpi_nvdimm_state;
> >>  } VirtMachineState;
> >>  
> >>  #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM : VIRT_PCIE_ECAM)  
> >