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

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

On 03/02/2016 04:44 PM, Michael S. Tsirkin wrote:
> 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.

It is defined by the spec in ACPI 6.0 "9.14.1 _DSM (Device Specific Method)" on page 532:

If set to zero, no functions are supported (other than function zero) for the specified UUID and
Revision ID. If set to one, at least one additional function is supported.

I audaciously guess the ACPI guys just want to reserve 0 to quickly identify if any function is
valid other than walking the bits one by one. :) but who knows...