Re: [Qemu-devel] [PATCH 8/9] nvdimm acpi: emulate dsm method

[Michael S. Tsirkin]

On Tue, Mar 01, 2016 at 06:56:10PM +0800, Xiao Guangrong wrote:
> Emulate dsm method after IO VM-exit
> 
> Currently, we only introduce the framework and no function is actually
> supported
> 
> Signed-off-by: Xiao Guangrong <address@hidden>
> ---
>  hw/acpi/aml-build.c         |  2 +-
>  hw/acpi/nvdimm.c            | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/acpi/aml-build.h |  1 +
>  include/hw/mem/nvdimm.h     |  8 ++++++++
>  4 files changed, 54 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index ab89ca6..da11bf8 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -227,7 +227,7 @@ static void build_extop_package(GArray *package, uint8_t 
> op)
>      build_prepend_byte(package, 0x5B); /* ExtOpPrefix */
>  }
>  
> -static void build_append_int_noprefix(GArray *table, uint64_t value, int 
> size)
> +void build_append_int_noprefix(GArray *table, uint64_t value, int size)
>  {
>      int i;
>  
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 781f6c1..e0b483a 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -393,12 +393,56 @@ typedef struct NvdimmDsmOut NvdimmDsmOut;
>  static uint64_t
>  nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned size)
>  {
> +    fprintf(stderr, "BUG: we never read _DSM IO Port.\n");
>      return 0;
>  }

Can't guest trigger this?
If yes, don't put such code in production please:
this will fill up disk on the host.


>  
>  static void
>  nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
>  {
> +    NvdimmDsmIn *in;
> +    GArray *out;
> +    uint32_t buf_size;
> +    hwaddr dsm_mem_addr = val;
> +
> +    nvdimm_debug("dsm memory address %#lx.\n", dsm_mem_addr);
> +
> +    /*
> +     * The DSM memory is mapped to guest address space so an evil guest
> +     * can change its content while we are doing DSM emulation. Avoid
> +     * this by copying DSM memory to QEMU local memory.
> +     */
> +    in = g_malloc(TARGET_PAGE_SIZE);
> +    cpu_physical_memory_read(dsm_mem_addr, in, TARGET_PAGE_SIZE);
> +
> +    le32_to_cpus(&in->revision);
> +    le32_to_cpus(&in->function);
> +    le32_to_cpus(&in->handle);
> +
> +    nvdimm_debug("Revision %#x Handler %#x Function %#x.\n", in->revision,
> +                 in->handle, in->function);
> +
> +    out = g_array_new(false, true /* clear */, 1);
> +
> +    /*
> +     * function 0 is called to inquire what functions are supported by
> +     * OSPM
> +     */
> +    if (in->function == 0) {
> +        build_append_int_noprefix(out, 0 /* No function Supported */,
> +                                  sizeof(uint8_t));
> +    } else {
> +        /* No function is supported yet. */
> +        build_append_int_noprefix(out, 1 /* Not Supported */,
> +                                  sizeof(uint8_t));
> +    }
> +
> +    buf_size = cpu_to_le32(out->len);
> +    cpu_physical_memory_write(dsm_mem_addr, &buf_size, sizeof(buf_size));

is there a race here?
can guest read this before data is written?

> +    cpu_physical_memory_write(dsm_mem_addr + sizeof(buf_size), out->data,
> +                              out->len);

What is this doing?
Is this actually writing AML bytecode into guest memory?


> +    g_free(in);
> +    g_array_free(out, true);
>  }
>  
>  static const MemoryRegionOps nvdimm_dsm_ops = {
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index 7404e2a..b0826f0 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -357,6 +357,7 @@ Aml *aml_derefof(Aml *arg);
>  Aml *aml_sizeof(Aml *arg);
>  Aml *aml_concatenate(Aml *source1, Aml *source2, Aml *target);
>  
> +void build_append_int_noprefix(GArray *table, uint64_t value, int size);
>  void
>  build_header(GArray *linker, GArray *table_data,
>               AcpiTableHeader *h, const char *sig, int len, uint8_t rev,
> diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
> index 634c60b..aaa2608 100644
> --- a/include/hw/mem/nvdimm.h
> +++ b/include/hw/mem/nvdimm.h
> @@ -25,6 +25,14 @@
>  
>  #include "hw/mem/pc-dimm.h"
>  
> +#define NVDIMM_DEBUG 0
> +#define nvdimm_debug(fmt, ...)                                \
> +    do {                                                      \
> +        if (NVDIMM_DEBUG) {                                   \
> +            fprintf(stderr, "nvdimm: " fmt, ## __VA_ARGS__);  \
> +        }                                                     \
> +    } while (0)
> +
>  #define TYPE_NVDIMM             "nvdimm"
>  
>  #define NVDIMM_DSM_MEM_FILE     "etc/acpi/nvdimm-mem"
> -- 
> 1.8.3.1