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

[Xiao Guangrong <guangrong.xiao@linux.intel.com>]

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