Re: [Qemu-devel] [PATCH 2/7] acpi: add vmcoreinfo device

2017-07-05 Thread Marc-André Lureau 
Hi

- Original Message -
> comments below
> 
> On 06/29/17 15:23, Marc-André Lureau wrote:
> > The VM coreinfo (vmcoreinfo) device is an emulated device which
> > exposes a 4k memory range to the guest to store various informations
> > useful to debug the guest OS. (it is greatly inspired by the VMGENID
> > device implementation)
> > 
> > This is an early-boot alternative to the qemu-ga VMDUMP_INFO event
> > proposed in "[PATCH 00/21] WIP: dump: add kaslr support".
> > 
> > A proof-of-concept kernel module:
> > https://github.com/elmarco/vmgenid-test/blob/master/qemuvmci-test.c
> > 
> > Signed-off-by: Marc-André Lureau 
> > ---
> >  include/hw/acpi/aml-build.h|   1 +
> >  include/hw/acpi/vmcoreinfo.h   |  36 +++
> >  hw/acpi/aml-build.c|   2 +
> >  hw/acpi/vmcoreinfo.c   | 198
> >  +
> >  hw/i386/acpi-build.c   |  14 +++
> >  default-configs/arm-softmmu.mak|   1 +
> >  default-configs/i386-softmmu.mak   |   1 +
> >  default-configs/x86_64-softmmu.mak |   1 +
> >  docs/specs/vmcoreinfo.txt  | 138 ++
> >  hw/acpi/Makefile.objs  |   1 +
> >  10 files changed, 393 insertions(+)
> >  create mode 100644 include/hw/acpi/vmcoreinfo.h
> >  create mode 100644 hw/acpi/vmcoreinfo.c
> >  create mode 100644 docs/specs/vmcoreinfo.txt
> >
> > diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> > index 88d0738d76..cf781bcd34 100644
> > --- a/include/hw/acpi/aml-build.h
> > +++ b/include/hw/acpi/aml-build.h
> > @@ -211,6 +211,7 @@ struct AcpiBuildTables {
> >  GArray *rsdp;
> >  GArray *tcpalog;
> >  GArray *vmgenid;
> > +GArray *vmcoreinfo;
> >  BIOSLinker *linker;
> >  } AcpiBuildTables;
> >  
> > diff --git a/include/hw/acpi/vmcoreinfo.h b/include/hw/acpi/vmcoreinfo.h
> > new file mode 100644
> > index 00..40fe99c3ed
> > --- /dev/null
> > +++ b/include/hw/acpi/vmcoreinfo.h
> > @@ -0,0 +1,36 @@
> > +#ifndef ACPI_VMCOREINFO_H
> > +#define ACPI_VMCOREINFO_H
> > +
> > +#include "hw/acpi/bios-linker-loader.h"
> > +#include "hw/qdev.h"
> > +
> > +#define VMCOREINFO_DEVICE   "vmcoreinfo"
> > +#define VMCOREINFO_FW_CFG_FILE  "etc/vmcoreinfo"
> > +#define VMCOREINFO_ADDR_FW_CFG_FILE "etc/vmcoreinfo-addr"
> > +
> > +#define VMCOREINFO_FW_CFG_SIZE  4096 /* Occupy a page of memory */
> > +#define VMCOREINFO_OFFSET   40   /* allow space for
> > +  * OVMF SDT Header Probe
> > Supressor
> > +  */
> > +
> > +#define VMCOREINFO(obj) OBJECT_CHECK(VmcoreinfoState, (obj),
> > VMCOREINFO_DEVICE)
> > +
> > +typedef struct VmcoreinfoState {
> 
> I think this should be spelled with a bit more camel-casing, like
> VMCoreInfoState or some such.
> 

ok

> > +DeviceClass parent_obj;
> > +uint8_t vmcoreinfo_addr_le[8];   /* Address of memory region */
> > +bool write_pointer_available;
> > +} VmcoreinfoState;
> > +
> > +/* returns NULL unless there is exactly one device */
> > +static inline Object *find_vmcoreinfo_dev(void)
> > +{
> > +return object_resolve_path_type("", VMCOREINFO_DEVICE, NULL);
> > +}
> > +
> > +void vmcoreinfo_build_acpi(VmcoreinfoState *vis, GArray *table_data,
> > +   GArray *vmci, BIOSLinker *linker);
> > +void vmcoreinfo_add_fw_cfg(VmcoreinfoState *vis, FWCfgState *s, GArray
> > *vmci);
> > +bool vmcoreinfo_get(VmcoreinfoState *vis, uint64_t *paddr, uint64_t *size,
> > +Error **errp);
> > +
> > +#endif
> > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > index 36a6cc450e..47043ade4a 100644
> > --- a/hw/acpi/aml-build.c
> > +++ b/hw/acpi/aml-build.c
> > @@ -1561,6 +1561,7 @@ void acpi_build_tables_init(AcpiBuildTables *tables)
> >  tables->table_data = g_array_new(false, true /* clear */, 1);
> >  tables->tcpalog = g_array_new(false, true /* clear */, 1);
> >  tables->vmgenid = g_array_new(false, true /* clear */, 1);
> > +tables->vmcoreinfo = g_array_new(false, true /* clear */, 1);
> >  tables->linker = bios_linker_loader_init();
> >  }
> >  
> > @@ -1571,6 +1572,7 @@ void acpi_build_tables_cleanup(AcpiBuildTables
> > *tables, bool mfre)
> >  g_array_free(tables->table_data, true);
> >  g_array_free(tables->tcpalog, mfre);
> >  g_array_free(tables->vmgenid, mfre);
> > +g_array_free(tables->vmcoreinfo, mfre);
> >  }
> >  
> >  /* Build rsdt table */
> > diff --git a/hw/acpi/vmcoreinfo.c b/hw/acpi/vmcoreinfo.c
> > new file mode 100644
> > index 00..216e0bb83a
> > --- /dev/null
> > +++ b/hw/acpi/vmcoreinfo.c
> > @@ -0,0 +1,198 @@
> > +/*
> > + *  Virtual Machine coreinfo device
> > + *  (based on Virtual Machine Generation ID Device)
> > + *
> > + *  Copyright (C) 2017 Red Hat, Inc.
> > + *  Copyright (C) 2017 Skyport Systems.
> > + *
> > + *  Authors: Marc-André Lureau

Re: [Qemu-devel] [PATCH 2/7] acpi: add vmcoreinfo device

2017-07-04 Thread Laszlo Ersek 
comments below

On 06/29/17 15:23, Marc-André Lureau wrote:
> The VM coreinfo (vmcoreinfo) device is an emulated device which
> exposes a 4k memory range to the guest to store various informations
> useful to debug the guest OS. (it is greatly inspired by the VMGENID
> device implementation)
> 
> This is an early-boot alternative to the qemu-ga VMDUMP_INFO event
> proposed in "[PATCH 00/21] WIP: dump: add kaslr support".
> 
> A proof-of-concept kernel module:
> https://github.com/elmarco/vmgenid-test/blob/master/qemuvmci-test.c
> 
> Signed-off-by: Marc-André Lureau 
> ---
>  include/hw/acpi/aml-build.h|   1 +
>  include/hw/acpi/vmcoreinfo.h   |  36 +++
>  hw/acpi/aml-build.c|   2 +
>  hw/acpi/vmcoreinfo.c   | 198 
> +
>  hw/i386/acpi-build.c   |  14 +++
>  default-configs/arm-softmmu.mak|   1 +
>  default-configs/i386-softmmu.mak   |   1 +
>  default-configs/x86_64-softmmu.mak |   1 +
>  docs/specs/vmcoreinfo.txt  | 138 ++
>  hw/acpi/Makefile.objs  |   1 +
>  10 files changed, 393 insertions(+)
>  create mode 100644 include/hw/acpi/vmcoreinfo.h
>  create mode 100644 hw/acpi/vmcoreinfo.c
>  create mode 100644 docs/specs/vmcoreinfo.txt
>
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index 88d0738d76..cf781bcd34 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -211,6 +211,7 @@ struct AcpiBuildTables {
>  GArray *rsdp;
>  GArray *tcpalog;
>  GArray *vmgenid;
> +GArray *vmcoreinfo;
>  BIOSLinker *linker;
>  } AcpiBuildTables;
>  
> diff --git a/include/hw/acpi/vmcoreinfo.h b/include/hw/acpi/vmcoreinfo.h
> new file mode 100644
> index 00..40fe99c3ed
> --- /dev/null
> +++ b/include/hw/acpi/vmcoreinfo.h
> @@ -0,0 +1,36 @@
> +#ifndef ACPI_VMCOREINFO_H
> +#define ACPI_VMCOREINFO_H
> +
> +#include "hw/acpi/bios-linker-loader.h"
> +#include "hw/qdev.h"
> +
> +#define VMCOREINFO_DEVICE   "vmcoreinfo"
> +#define VMCOREINFO_FW_CFG_FILE  "etc/vmcoreinfo"
> +#define VMCOREINFO_ADDR_FW_CFG_FILE "etc/vmcoreinfo-addr"
> +
> +#define VMCOREINFO_FW_CFG_SIZE  4096 /* Occupy a page of memory */
> +#define VMCOREINFO_OFFSET   40   /* allow space for
> +  * OVMF SDT Header Probe Supressor
> +  */
> +
> +#define VMCOREINFO(obj) OBJECT_CHECK(VmcoreinfoState, (obj), 
> VMCOREINFO_DEVICE)
> +
> +typedef struct VmcoreinfoState {

I think this should be spelled with a bit more camel-casing, like
VMCoreInfoState or some such.

> +DeviceClass parent_obj;
> +uint8_t vmcoreinfo_addr_le[8];   /* Address of memory region */
> +bool write_pointer_available;
> +} VmcoreinfoState;
> +
> +/* returns NULL unless there is exactly one device */
> +static inline Object *find_vmcoreinfo_dev(void)
> +{
> +return object_resolve_path_type("", VMCOREINFO_DEVICE, NULL);
> +}
> +
> +void vmcoreinfo_build_acpi(VmcoreinfoState *vis, GArray *table_data,
> +   GArray *vmci, BIOSLinker *linker);
> +void vmcoreinfo_add_fw_cfg(VmcoreinfoState *vis, FWCfgState *s, GArray 
> *vmci);
> +bool vmcoreinfo_get(VmcoreinfoState *vis, uint64_t *paddr, uint64_t *size,
> +Error **errp);
> +
> +#endif
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 36a6cc450e..47043ade4a 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -1561,6 +1561,7 @@ void acpi_build_tables_init(AcpiBuildTables *tables)
>  tables->table_data = g_array_new(false, true /* clear */, 1);
>  tables->tcpalog = g_array_new(false, true /* clear */, 1);
>  tables->vmgenid = g_array_new(false, true /* clear */, 1);
> +tables->vmcoreinfo = g_array_new(false, true /* clear */, 1);
>  tables->linker = bios_linker_loader_init();
>  }
>  
> @@ -1571,6 +1572,7 @@ void acpi_build_tables_cleanup(AcpiBuildTables *tables, 
> bool mfre)
>  g_array_free(tables->table_data, true);
>  g_array_free(tables->tcpalog, mfre);
>  g_array_free(tables->vmgenid, mfre);
> +g_array_free(tables->vmcoreinfo, mfre);
>  }
>  
>  /* Build rsdt table */
> diff --git a/hw/acpi/vmcoreinfo.c b/hw/acpi/vmcoreinfo.c
> new file mode 100644
> index 00..216e0bb83a
> --- /dev/null
> +++ b/hw/acpi/vmcoreinfo.c
> @@ -0,0 +1,198 @@
> +/*
> + *  Virtual Machine coreinfo device
> + *  (based on Virtual Machine Generation ID Device)
> + *
> + *  Copyright (C) 2017 Red Hat, Inc.
> + *  Copyright (C) 2017 Skyport Systems.
> + *
> + *  Authors: Marc-André Lureau 
> + *   Ben Warren 
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +#include "qemu/osdep.h"
> +#include "hw/acpi/acpi.h"
> +#include