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.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
