diff mbox

[8/9] nvdimm acpi: emulate dsm method

Message ID 56D6656A.9060501@linux.intel.com
State New
Headers show

Commit Message

Xiao Guangrong March 2, 2016, 4 a.m. UTC 
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.xiao@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:

Comments

Michael S. Tsirkin March 2, 2016, 6:38 a.m. UTC | #1 
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.xiao@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.
diff mbox

Patch

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 = {