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

[Michael S. Tsirkin]

On Wed, Mar 02, 2016 at 03:15:19PM +0800, Xiao Guangrong wrote:
> 
> 
> On 03/02/2016 02:36 PM, Michael S. Tsirkin wrote:
> >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 :(
> >
> 
> Hmm... Or use GArray? But it is :)
> 
> >>>>+    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.
> >
> 
> Yes, this memory is allocated by BIOS and we asked it to align the memory
> with PAGE_SIZE:
> 
>     bios_linker_loader_alloc(linker, NVDIMM_DSM_MEM_FILE, TARGET_PAGE_SIZE,
>                              false /* high memory */);
> 
> >>>>+
> >>>>+    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?
> 
> It is good to me, but as your suggestions, this code will be removed.
> 
> >
> >>>>+
> >>>>+    /*
> >>>>+     * 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 ...
> 
> If its the function 0, we return 0 that indicates no command is supported yet.

first comment says no function supported.
clearly function 0 is supported, is it not?
how exactly does 0 indicate no command is supported?
is it a bitmask of supported commands?

> Other wise, it is a command request from a evil guest regardless of the result
> returned by function 0, we return the status code 1 to indicates this command
> is not supported.

is command same as function?

> 
> >
> >
> >>>>+    } 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 ...
> >
> 
> Okay, i prefer the former too ;).
> 
> Thank you, Michael!
>