On Wed, Mar 02, 2016 at 12:00:42PM +0800, Xiao Guangrong wrote:
>
>
> On 03/02/2016 01:12 AM, Michael S. Tsirkin wrote:
> >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 <guangrong.x...@linux.intel.com>
> >>---
> >> 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;
> >> }
> >>
> >> 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));
> >>+ cpu_physical_memory_write(dsm_mem_addr + sizeof(buf_size), out->data,
> >>+ out->len);
> >
> >BTW, how do we know buffer is big enough? Add assert here?
>
> I planed to do it when we introduce the real handler of NVDIMM command, but
> yes,
> it is better doing it in this patchset. Will follow it in the next version.
>
> >
> >Also, you have a packed structure with the layout, correct?
> >Can't you use that instead of open-coding it?
>
>
> Okay, how about do it like this:
>
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index a6359cc..2812f7a 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -401,7 +401,7 @@ static void
> nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
> {
> NvdimmDsmIn *in;
> - GArray *out;
> + NvdimmDsmOut *out;
> uint32_t buf_size;
> hwaddr dsm_mem_addr = val;
>
> @@ -422,27 +422,33 @@ nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t
> val, unsigned size)
> nvdimm_debug("Revision %#x Handler %#x Function %#x.\n", in->revision,
> in->handle, in->function);
>
> - out = g_array_new(false, true /* clear */, 1);
> + out = g_malloc(TARGET_PAGE_SIZE);
> +
> + out->len = sizeof(out);
>
> /*
> * 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));
> + /* No function Supported */
> + uint32_t cmd_list = cpu_to_le32(0);
> +
> + out->len += sizeof(cmd_list);
> } else {
> - /* No function is supported yet. */
> - build_append_int_noprefix(out, 1 /* Not Supported */,
> - sizeof(uint8_t));
> + /* Not Supported */
> + uint32_t status = cpu_to_le32(1);
> +
> + out->len = sizeof(status);
> }
>
> - buf_size = cpu_to_le32(out->len);
> - cpu_physical_memory_write(dsm_mem_addr, &buf_size, sizeof(buf_size));
> - cpu_physical_memory_write(dsm_mem_addr + sizeof(buf_size), out->data,
> - out->len);
> + buf_size = out->len;
> + assert(buf_size <= TARGET_PAGE_SIZE);
> +
> + out->len = cpu_to_le32(out->len);
> + cpu_physical_memory_write(dsm_mem_addr, out, buf_size);
> g_free(in);
> - g_array_free(out, true);
> + g_free(out);
> }
>
> static const MemoryRegionOps nvdimm_dsm_ops = {
Not really much better. I suggested a better version in
another thread.
