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

[Michael S. Tsirkin]

On Wed, Mar 02, 2016 at 11:30:10AM +0800, Xiao Guangrong wrote:
> 
> 
> On 03/02/2016 01:09 AM, Michael S. Tsirkin wrote:
> 
> >
> >Can't guest trigger this?
> >If yes, don't put such code in production please:
> >this will fill up disk on the host.
> >
> 
> Okay, the evil guest can read the IO port freely. I will use nvdimm_debug() 
> instead.
> 
> >
> >>
> >>  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);

ugh. manual memory management :(

> >>+    cpu_physical_memory_read(dsm_mem_addr, in, TARGET_PAGE_SIZE);

is there a requirement address is aligned?
if not this might cross page and crash qemu.
better read just what you need.

> >>+
> >>+    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);

export build_alloc_array then, and reuse?

> >>+
> >>+    /*
> >>+     * 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));

What does this mean? Same comment here and below ...


> >>+    } 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?
> 
> I think no.
> 
> It is the SERIALIZED DSM so there is no race in guest. And the CPU has exited
> from guest mode when we fill the buffer in the same CPU-context so the guest
> can not read the buffer at this point also memory-barrier is not needed here.
> 
> >
> >>+    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?
> 
> The layout of result written into the buffer is like this:
> struct NvdimmDsmOut {
>     /* the size of buffer filled by QEMU. */
>     uint32_t len;
>     uint8_t data[0];
> } QEMU_PACKED;
> typedef struct NvdimmDsmOut NvdimmDsmOut;
> 
> So the first cpu_physical_memory_write() writes the @len and the second one 
> you
> pointed out writes the real payload.


So either write a function that gets parameters and formats
buffer, or use a structure to do this.
Do not open-code formatting and don't mess with
offsets.

E.g.

struct NvdimmDsmFunc0Out {
     /* the size of buffer filled by QEMU. */
     uint32_t len;
     uint8_t supported;
} QEMU_PACKED;
typedef struct NvdimmDsmFunc0Out NvdimmDsmFunc0Out;


And now

NvdimmDsmFunc0Out func0 = { .len = cpu_to_le32(sizeof(func0)); suppported = 
func == 0; };

cpu_physical_memory_write(dsm_mem_addr, &func0, sizeof func0);


Or if you really insist on using GArray:

build_dsm_out_func0(int function...)
{
    uint32_t len;
    uint8_t result;

    len = sizeof result;
    if (function == 0) {
        result = 0 /* No function Supported */;
   } else {
        /* No function is supported yet. */
        result = 1 /* Not Supported */;
   }

    build_append_int_noprefix(out, len, sizeof len);
    build_append_int_noprefix(out, result, sizeof result);

    assert(out->len < PAGE_SIZE); - is this right?
    cpu_physical_memory_write(dsm_mem_addr, out->data,
                              out->len);
}


but I prefer the former ...

-- 
MST