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

[Xiao Guangrong]

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

> > > > +    } 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!