From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57639) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ab2iq-0001rc-LW for qemu-devel@nongnu.org; Wed, 02 Mar 2016 04:05:33 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ab2im-0002b1-FU for qemu-devel@nongnu.org; Wed, 02 Mar 2016 04:05:32 -0500 Received: from mga01.intel.com ([192.55.52.88]:46483) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ab2im-0002ac-4n for qemu-devel@nongnu.org; Wed, 02 Mar 2016 04:05:28 -0500 References: <1456829771-71553-1-git-send-email-guangrong.xiao@linux.intel.com> <1456829771-71553-9-git-send-email-guangrong.xiao@linux.intel.com> <20160301190240-mutt-send-email-mst@redhat.com> <56D65E42.1050108@linux.intel.com> <20160302081841-mutt-send-email-mst@redhat.com> <56D69307.6040902@linux.intel.com> <20160302091830-mutt-send-email-mst@redhat.com> <56D6965D.9010906@linux.intel.com> <20160302103829-mutt-send-email-mst@redhat.com> From: Xiao Guangrong Message-ID: <56D6ACC8.6020709@linux.intel.com> Date: Wed, 2 Mar 2016 17:05:12 +0800 MIME-Version: 1.0 In-Reply-To: <20160302103829-mutt-send-email-mst@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 8/9] nvdimm acpi: emulate dsm method List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: ehabkost@redhat.com, kvm@vger.kernel.org, gleb@kernel.org, mtosatti@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, imammedo@redhat.com, pbonzini@redhat.com, dan.j.williams@intel.com, rth@twiddle.net 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...