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

[Michael S. Tsirkin]

On Wed, Mar 02, 2016 at 03:29:33PM +0800, Xiao Guangrong wrote:
> 
> 
> On 03/02/2016 03:20 PM, Michael S. Tsirkin wrote:
> >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?
> 
> Yep, the comment is not clear here. It should be "No function Supported other
> than function 0 "
> 
> Function 0 is the common function supported by all DSMs to inquire what 
> functions are
> supported by this DSM.
> 
> >how exactly does 0 indicate no command is supported?
> >is it a bitmask of supported commands?
> 
> It is a bitmask. The spec said:
> 
> If Function Index is zero, the return is a buffer containing one bit for each 
> function
> index, starting with zero.

Why not start from 1?
So 0x1 - function 1 supported, 0x2 - function 2, 0x4 - function 3 etc.

> Bit 0 indicates whether there is support for any functions other
> than function 0 for the specified UUID and Revision ID. If set to zero, no 
> functions are
> supported (other than function zero) for the specified UUID and Revision ID.

> >
> >>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?
> 
> Yes.