Re: [Qemu-arm] [RFC PATCH 3/3] hw/arm/virt: add nvdimm emulation support

[address@hidden]

Hi Peter,

Please, check the comments below. Thanks a lot!

> -----Original Message-----
> From: Peter Maydell [mailto:address@hidden
> Sent: Tuesday, July 26, 2016 1:06 AM
> To: 이광우(LEE KWANGWOO) MS SW
> Cc: Xiao Guangrong; Michael S. Tsirkin; Igor Mammedov; Paolo Bonzini; Richard 
> Henderson; Eduardo
> Habkost; Shannon Zhao; Shannon Zhao; QEMU Developers; qemu-arm; 정우석(CHUNG WOO 
> SUK) MS SW; 김현철(KIM
> HYUNCHUL) MS SW
> Subject: Re: [RFC PATCH 3/3] hw/arm/virt: add nvdimm emulation support
> 
> On 20 July 2016 at 01:49, Kwangwoo Lee <address@hidden> wrote:
> > This patch enables evaluating NVDIMM on aarch64 virt platform. The
> > option - nvdimm - passed after machine type is disabled by default.
> >
> > The command below has been used to test the feature:
> >
> > ./aarch64-softmmu/qemu-system-aarch64                          \
> >     -machine type=virt,nvdimm=on                               \
> >     -cpu cortex-a57 -smp 1                                     \
> >     -bios ~/oss/edk2/Build/ArmVirtQemu-AARCH64/DEBUG_GCC49/FV/QEMU_EFI.fd \
> >     -m 512,maxmem=2G,slots=2                                   \
> >     -object memory-backend-file,id=mem1,share,mem-path=./nvdimm1,size=1G \
> >     -device nvdimm,memdev=mem1,id=nv1                          \
> >     -fsdev local,id=r,path=/media/sf_Share,security_model=none \
> >     -device virtio-9p-device,fsdev=r,mount_tag=r               \
> >     -kernel ../linux/arch/arm64/boot/Image                     \
> >     --append "console=ttyAMA0 acpi=force"                      \
> >     -nographic
> >
> > Signed-off-by: Kwangwoo Lee <address@hidden>
> > ---
> >  default-configs/aarch64-softmmu.mak |  2 ++
> >  hw/arm/virt-acpi-build.c            |  5 +++++
> >  hw/arm/virt.c                       | 34 ++++++++++++++++++++++++++++++++++
> >  include/hw/arm/virt-acpi-build.h    |  1 +
> >  include/hw/arm/virt.h               |  4 ++++
> >  5 files changed, 46 insertions(+)
> >
> > diff --git a/default-configs/aarch64-softmmu.mak 
> > b/default-configs/aarch64-softmmu.mak
> > index 5790cd2..295816b 100644
> > --- a/default-configs/aarch64-softmmu.mak
> > +++ b/default-configs/aarch64-softmmu.mak
> > @@ -8,3 +8,5 @@ CONFIG_DDC=y
> >  CONFIG_DPCD=y
> >  CONFIG_XLNX_ZYNQMP=y
> >  CONFIG_MEM_HOTPLUG=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 28fc59c..c3caaa9 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -648,6 +648,7 @@ struct AcpiBuildState {
> >  static
> >  void virt_acpi_build(VirtGuestInfo *guest_info, AcpiBuildTables *tables)
> >  {
> > +    AcpiNVDIMMState *acpi_nvdimm = guest_info->acpi_nvdimm;
> >      GArray *table_offsets;
> >      unsigned dsdt, rsdt;
> >      GArray *tables_blob = tables->table_data;
> > @@ -695,6 +696,10 @@ void virt_acpi_build(VirtGuestInfo *guest_info, 
> > AcpiBuildTables *tables)
> >          build_srat(tables_blob, tables->linker, guest_info);
> >      }
> >
> > +    if (acpi_nvdimm->is_enabled) {
> > +        nvdimm_build_acpi(table_offsets, tables_blob, tables->linker,
> > +                          acpi_nvdimm);
> > +    }
> >      /* RSDT is pointed to by RSDP */
> >      rsdt = tables_blob->len;
> >      build_rsdt(tables_blob, tables->linker, table_offsets, NULL, NULL);
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index f7ff411..f9db19c 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -94,6 +94,7 @@ typedef struct {
> >      bool highmem;
> >      int32_t gic_version;
> >      MemoryHotplugState hotplug_memory;
> > +    AcpiNVDIMMState acpi_nvdimm;
> >  } VirtMachineState;
> >
> >  #define TYPE_VIRT_MACHINE   MACHINE_TYPE_NAME("virt")
> > @@ -180,6 +181,7 @@ static const MemMapEntry a15memmap[] = {
> >      [VIRT_FW_CFG] =             { 0x09020000, 0x00000018 },
> >      [VIRT_GPIO] =               { 0x09030000, 0x00001000 },
> >      [VIRT_SECURE_UART] =        { 0x09040000, 0x00001000 },
> > +    [VIRT_ACPI_IO] =            { 0x09050000, 0x00001000 },
> >      [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
> >      /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that 
> > size */
> >      [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
> > @@ -1376,6 +1378,7 @@ static void machvirt_init(MachineState *machine)
> >      guest_info->irqmap = vbi->irqmap;
> >      guest_info->use_highmem = vms->highmem;
> >      guest_info->gic_version = gic_version;
> > +    guest_info->acpi_nvdimm = &vms->acpi_nvdimm;
> >      guest_info_state->machine_done.notify = virt_guest_info_machine_done;
> >      qemu_add_machine_init_done_notifier(&guest_info_state->machine_done);
> >
> > @@ -1413,6 +1416,18 @@ static void machvirt_init(MachineState *machine)
> >                                      &vms->hotplug_memory.mr);
> >      }
> >
> > +    if (vms->acpi_nvdimm.is_enabled) {
> > +        AcpiNVDIMMState *acpi_nvdimm = &vms->acpi_nvdimm;
> > +
> > +        acpi_nvdimm->dsm_io.type = NVDIMM_ACPI_IO_MEMORY;
> > +        acpi_nvdimm->dsm_io.base =
> > +                vbi->memmap[VIRT_ACPI_IO].base + NVDIMM_ACPI_IO_BASE;
> > +        acpi_nvdimm->dsm_io.size = NVDIMM_ACPI_IO_LEN;
> > +
> > +        nvdimm_init_acpi_state(acpi_nvdimm, sysmem,
> > +                               guest_info->fw_cfg, OBJECT(vms));
> > +    }
> 
> This seems to be missing code to write the device tree
> information about whatever this device is?

Is it OK to just add a memory region which cannot be used without ACPI?
This is unclear to me. If you suggest that it is better way, I'll revise
this patch to add a device tree node. Please, help me to understand.

> > +
> >      vbi->bootinfo.ram_size = machine->ram_size;
> >      vbi->bootinfo.kernel_filename = machine->kernel_filename;
> >      vbi->bootinfo.kernel_cmdline = machine->kernel_cmdline;
> > @@ -1550,6 +1565,20 @@ static HotplugHandler 
> > *virt_get_hotplug_handler(MachineState *machine,
> >      return NULL;
> >  }
> >
> > +static bool virt_get_nvdimm(Object *obj, Error **errp)
> > +{
> > +    VirtMachineState *vms = VIRT_MACHINE(obj);
> > +
> > +    return vms->acpi_nvdimm.is_enabled;
> > +}
> > +
> > +static void virt_set_nvdimm(Object *obj, bool value, Error **errp)
> > +{
> > +    VirtMachineState *vms = VIRT_MACHINE(obj);
> > +
> > +    vms->acpi_nvdimm.is_enabled = value;
> > +}
> > +
> >  static void virt_machine_class_init(ObjectClass *oc, void *data)
> >  {
> >      MachineClass *mc = MACHINE_CLASS(oc);
> > @@ -1620,6 +1649,11 @@ static void virt_2_7_instance_init(Object *obj)
> >      object_property_set_description(obj, "gic-version",
> >                                      "Set GIC version. "
> >                                      "Valid values are 2, 3 and host", 
> > NULL);
> > +
> > +    /* nvdimm is disabled on default. */
> > +    vms->acpi_nvdimm.is_enabled = false;
> > +    object_property_add_bool(obj, ARCH_VIRT_NVDIMM, virt_get_nvdimm,
> > +                             virt_set_nvdimm, &error_abort);
> >  }
> >
> >  static void virt_machine_2_7_options(MachineClass *mc)
> > diff --git a/include/hw/arm/virt-acpi-build.h 
> > b/include/hw/arm/virt-acpi-build.h
> > index e43330a..167bbb4 100644
> > --- a/include/hw/arm/virt-acpi-build.h
> > +++ b/include/hw/arm/virt-acpi-build.h
> > @@ -33,6 +33,7 @@ typedef struct VirtGuestInfo {
> >      const int *irqmap;
> >      bool use_highmem;
> >      int gic_version;
> > +    AcpiNVDIMMState *acpi_nvdimm;
> >  } VirtGuestInfo;
> >
> >
> > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> > index 35aa3cc..c5cf7e3 100644
> > --- a/include/hw/arm/virt.h
> > +++ b/include/hw/arm/virt.h
> > @@ -32,6 +32,7 @@
> >
> >  #include "qemu-common.h"
> >  #include "exec/hwaddr.h"
> > +#include "hw/mem/nvdimm.h"
> >
> >  #define NUM_GICV2M_SPIS       64
> >  #define NUM_VIRTIO_TRANSPORTS 32
> > @@ -48,6 +49,8 @@
> >  /* 1GB alignment for hotplug memory region */
> >  #define ARCH_VIRT_HOTPLUG_MEM_ALIGN (1ULL << 30)
> >
> > +#define ARCH_VIRT_NVDIMM      "nvdimm"
> 
> Why the #define ?

OK. I'll move this into virt.c when using object_property_add_bool() directly.

> > +
> >  enum {
> >      VIRT_FLASH,
> >      VIRT_MEM,
> > @@ -70,6 +73,7 @@ enum {
> >      VIRT_GPIO,
> >      VIRT_SECURE_UART,
> >      VIRT_SECURE_MEM,
> > +    VIRT_ACPI_IO,
> >  };
> >
> >  typedef struct MemMapEntry {
> > --
> > 2.5.0
> 
> thanks
> -- PMM

Thank you very much!

Best Regards,
Kwangwoo Lee